Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extraneous rows and incorrect sort order in report status dialog table #796

Closed
mcking65 opened this issue Sep 26, 2023 · 7 comments
Closed
Labels
bug Something isn't working

Comments

@mcking65
Copy link

From #649:

The table is sorted by required Yes, then required No, then AT, then browser.

I advanced two test plans to recommended in staging. Six reports are required, and both test plans have all six required reports. I expected the report status table to have 6 rows in the following order:

  1. JAWS/Chrome
  2. JAWS/Firefox
  3. NVDA/Chrome
  4. NVDA/Firefox
  5. VoiceOver/Chrome
  6. VoiceOver/Safari

Instead the table has 9 rows in a different order. The last 3 rows are errant; they are duplicative of 3 other rows except that they have "No" in the required column.

The tables in candidate and draft phases are correct.

It appears that the code is not fully accounting for the fact that some reports transition from optional to required when the test plan moves to recommended. The three that get duplicated were previously not required. Additionally, the table the row sequence needs to be updated because there are multiple required for the same AT.

Screenshot of link test plan report status

Screen shot of the command button report status dialog

@howard-e
Copy link
Contributor

they are duplicative of 3 other rows

@mcking65 on the ones that were duplicated, I think this was described, but quite some time ago now, so thanks for creating this issue to track it! This is a consequence of the merging of test plan versions and reports data, that some duplicate AT + Browser report pairings were created for the updated test plan versions, and not by any actions done in the application.

I found 11 duplicates by running the following query on the data:

select *
from "TestPlanReport"
where ("testPlanVersionId", "atId", "browserId") in ( select "testPlanVersionId", "atId", "browserId"
                                                      from "TestPlanReport"
                                                               join "TestPlanVersion" "testPlanVersion"
                                                                    on "testPlanVersion".id = "TestPlanReport"."testPlanVersionId"
                                                      group by "testPlanVersionId", "atId", "browserId"
                                                      having count(*) > 1 )
order by "testPlanVersionId", "atId", "browserId", "createdAt";

We determined that the reports with older createdAt dates could be safely removed as they aren't being surfaced on the production site.

So this should be a note to myself, or anyone else to clean up those duplicates after this lands. I'll also run that clean up query on the testing environment later.

List of Test Plan Versions with duplicates (11 instances)
  • Color Viewer Slider (V22.04.14)

    • JAWS + Chrome Reports
  • Command Button Example (V22.05.04)

    • JAWS + Chrome Reports
    • NVDA + Chrome Reports
    • VO + Safari Reports
  • Link Example 1 (span element with text content)

    • JAWS + Chrome Reports
    • VO + Safari Reports
    • NVDA + Chrome Reports
  • Modal Dialog Example

    • JAWS + Chrome Reports
  • Toggle Button

    • NVDA + Chrome Reports
    • JAWS + Chrome Reports
    • VO + Safari Reports

@alflennik
Copy link
Contributor

Thanks @howard-e for detailing the situation that led to the extra reports appearing, and the query you used to find them all. I recently went through the app trying to verify that duplicates like the ones shown above can no longer be generated.

First I tried to create duplicates using the UI.

There are three ways to manually create reports in the UI. The first and second are to use the "Add to test queue" forms on the data management page and the test queue page. If you try to create a duplicate here, you will pull any existing report back to the queue without creating a duplicate. The third way is the "Add to Test Queue" button inside the report status dialog. You can't create duplicates there because the button will only show when the report is missing.

Second I tried to create duplicates using the API.

There are two methods that can create new database records, createTestPlanReport and getOrCreateTestPlanReport, and the methods are used as part of three graphql fields.

  • Number 1 is findOrCreateTestPlanReport, and calling it directly through the API has the same behavior as adding to test queue, if there is a duplicate, it pulls the duplicate back to the test queue.
  • Number 2 is updateTestPlanReportTestPlanVersion, which is API only, not exposed to the UI, and allows a report to be copied from an old version to a more recent version. When trying to create a duplicate I got a message "A report already exists and continuing will overwrite its data." and no duplicate was created.
  • Number 3 is updatePhaseResolver which will attempt to copy any reports from a newly deprecated version to a new version. I verified that the previously-collected reports were copied over without creating any duplicates.

Finally, I reran the query Howard wrote on a recent dump of the production database and got zero results.

I think it is safe to close this issue.

@mcking65
Copy link
Author

mcking65 commented Dec 4, 2023

@alflennik wrote:

Thanks @howard-e for detailing the situation that led to the extra reports appearing, and the query you used to find them all.

We have 3 wiki pages for the data model; it would be helpful to have a single page with complete documentation for the current model. If I am understanding the wiki and the above query correctly, the current model uses browser ID and AT ID as the distinct attributes of a report. However, we need that to be AT ID, AT version ID, Browser ID, and browser version ID.

For a given AT/Browser pairing, we need to be able to report on changes across versions. Currently, the report site only shows the latest data.

In 2024, we are only going to require new reports for each public AT version for recommended reports. We have discussed this several times in CG meetings. The most recent discussion is documented in #809.

I recently went through the app trying to verify that duplicates like the ones shown above can no longer be generated.

First I tried to create duplicates using the UI.

There are three ways to manually create reports in the UI. The first and second are to use the "Add to test queue" forms on the data management page and the test queue page. If you try to create a duplicate here, you will pull any existing report back to the queue without creating a duplicate.

This would be a bug if we are trying to make a report for a newer version of the AT or browser.

The third way is the "Add to Test Queue" button inside the report status dialog. You can't create duplicates there because the button will only show when the report is missing.

This will also be a problem. This is related to #792 and #791.

The more I think about this, there are so many consequences related to requirements for reporting on specific AT/Browser versions that we may need to build a new 2024/Q1 project around reporting on interoperability trends across AT/Browser versions.

@mcking65
Copy link
Author

mcking65 commented Dec 4, 2023

I verified this problem is resolved in sandbox for one set of conditions. That is, if a test plan is advanced from candidate to recommended and has reports for 6 AT./browser combinations, the report status dialog is correct. I was not able to verify that it would work correctly if the test plan has only the minimum 3 required during draft and candidate review.`

I think this issue can be closed once the fix is deployed to prod.

@mcking65
Copy link
Author

mcking65 commented Dec 4, 2023

@ccanash

The status says "In production" but there are no dates on either sandbox or production.

@alflennik
Copy link
Contributor

Hi @mcking65, thanks for linking issues #809, #792 and #791, which collectively were a really interesting read. I can appreciate the motivation for the shift. I agree that this sounds like the kind of major effort which can we can tackle over the course of a quarter.

The fix was deployed to production previously so I'm closing this issue.

@alflennik
Copy link
Contributor

Regarding when this was released to production, it was on Sep 28 along with the rest of the changes for #648

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
No open projects
Development

No branches or pull requests

3 participants