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

Support for copy changes for no data #583

Merged
merged 2 commits into from
May 18, 2023
Merged

Conversation

evmiguel
Copy link
Contributor

@evmiguel evmiguel commented Mar 28, 2023

The easiest way to test this is to make a draft test plan with a combination that hasn't been tested before.

To make the tests pass, run:

In psql,

\c aria_at_report_test;
INSERT INTO "TestPlanReport" (id, "status", "testPlanVersionId", "createdAt", "atId", "browserId") VALUES (8, 'DRAFT', get_test_plan_version_id(text 'Alert Example'), '2021-05-14 14:18:23.602-05', 3, 2);

@evmiguel evmiguel requested a review from alflennik March 28, 2023 19:49
@evmiguel evmiguel force-pushed the support-tables-not-applicable branch from 1329218 to ab13bcd Compare March 28, 2023 19:50
@evmiguel evmiguel marked this pull request as ready for review March 28, 2023 20:14
@evmiguel evmiguel requested a review from howard-e March 28, 2023 20:25
Copy link
Contributor

@alflennik alflennik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job! I successfully ran it locally, tested the migrations and ran the tests without issue.

Copy link
Contributor

@howard-e howard-e left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@evmiguel looks good!

Something of note is that while this solution does work under the ideal happy case, it is also pretty easy to get through (in the absolute rarest case of an admin overlooking several reviews or the process), and this also has nothing to do in particular with this PR.

Example being that going to /embed/reports/<plan>, it correctly notes that the combination of JAWS + Safari is Not Applicable but I am still able to submit a JAWS + Safari combination from the Test Queue, push it all the way through to CANDIDATE or RECOMMENDED and a percentage value is displayed for JAWS + Safari even though it previously said Not Applicable.

I think we recently discussed this internally and the question would be how can we explicitly define valid Browser + AT Combinations, to be ideally used at the point when a Test Plan is being added to the Queue to prevent invalid combinations from being added? And potentially added as an additional safeguard for this embedded report?

cc @alflennik

@alflennik
Copy link
Contributor

@howard-e Yes, the way I proposed to implement the algorithm for not applicable vs. not available is to check if the AT / Browser combination exists in the current data. Were you saying that this is an issue which is cropping up in other places? If so maybe we can think of an explicit API change in GraphQL for this?

@howard-e
Copy link
Contributor

howard-e commented Apr 4, 2023

Were you saying that this is an issue which is cropping up in other places?

Yep potentially. I'm not sure how far-reaching it could be but just identifying what's happening. We can track it in another issue, as it doesn't impact this PR landing (looks like it can be merged whenever ready now as well)

If so maybe we can think of an explicit API change in GraphQL for this?

Yep, thinking something like that would be good

@alflennik
Copy link
Contributor

@evmiguel What do you think? Do you have any ideas about how we could change the GraphQL API?

@evmiguel
Copy link
Contributor Author

@alflennik @howard-e We could possibly have a new query item called atBrowserCombinations that has similar code to the function, getAllAtBrowserCombinations, in embed.js as a resolver, but that resolver would be able to validate the correct combinations. How does that sound?

@howard-e
Copy link
Contributor

@alflennik @howard-e We could possibly have a new query item called atBrowserCombinations that has similar code to the function, getAllAtBrowserCombinations, in embed.js as a resolver, but that resolver would be able to validate the correct combinations. How does that sound?

Sounds good! I agree with the suggestion but I think we can continue the discussion in a separate issue/PR. I've filed #625.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AT Support Tables Embedding: Distinguish data not yet available from not applicable
3 participants