Add Minimum / Exact AT Version to Test Plan Reports #997
Conversation
client/components/Home/Home.jsx
Outdated
| support. The W3C ARIA-AT Community Group is currently | ||
| focusing on a stable and mature test suite for five | ||
| screen readers. In the future, we plan to test |
There was a problem hiding this comment.
Great eye for detail to propose this change! This should probably go in it's own PR to get feedback on the copy change.
There was a problem hiding this comment.
Okay I created #999 and removed the changes from here. Do you want to either assign yourself or whoever you think should review it?
| @@ -1,634 +0,0 @@ | |||
| 'use strict'; | |||
| browserId, | ||
| exactAtVersionId, | ||
| minimumAtVersionId, | ||
| vendorReviewStatus |
There was a problem hiding this comment.
Not sure I follow why there would be a need for vendorReviewStatus to now be set when creating the test plan report. Could you explain?
There was a problem hiding this comment.
Oh you're right, I removed it. Previously I got confused by code in the updateTestPlanReportTestPlanVersion file and that was why I abortively changed this function.
| // The proceed button is disabled because it will now create | ||
| // duplicate testPlanReports, and support has not yet been | ||
| // implemented across the full frontend to account for | ||
| // duplicates. |
There was a problem hiding this comment.
Probably want to defend against this on the backend as well.
There was a problem hiding this comment.
Sure, I added temporary code to enforce that.
| 'browser and test plan version. Work is currently ' + | ||
| 'underway to remove this limitation.' |
There was a problem hiding this comment.
I do think it might become necessary at some point to have a branch for trend report changes. However personally I feel like this code is still mergeable to main, and we haven't arrived at the "ok now we need a separate branch" point.
Other than removing the "proceed" button, I don't think there's much impact on the user experience. In the event the admins do want to move a report from the reports page back to the test queue, we can accommodate that through the API.
That's my perspective but I'm happy to defer to whatever you think is better.
Do you think removing the "work is underway" sentence would mitigate your concern?
There was a problem hiding this comment.
Sounds good, I'm okay with not yet considering this the "breaking point", so it could go to main but not just yet for 2 reasons:
- This particular feature has been used in recent times to make use of the current results copy featureset, in that it was unexpectedly only working if the results to be copied was in a report currently on the Test Queue, so this allowed a report to be pulled back. This won't be an issue once the enhancements identified in Improving the test results copy functionality #935 are also a part of
main(PRs Support results being copied even when commands have been added or removed between versions #967, Copy results from advanced phase #976 and Copy results when adding to Test Queue #978) though. - There is a planned release early next week and this new restriction hasn't yet been discussed. So this allows us the chance to notify them that this restriction is coming and so it also doesn't have to be forcibly excluded during the next push.
There was a problem hiding this comment.
@howard-e do you want to merge it into a branch with a nice name like "trends"?
|
@howard-e should be ready for a second pass. |
There was a problem hiding this comment.
@alflennik thanks for addressing the feedback! This is great and all very exciting for the next phase or process change!
Changes LGTM and I'm approving but also note that I don't think we should push to main just yet for reasons described in #997 (comment)
|
This has been superseded by #1055! |
See issue #791 - this PR addresses the server part of this task.