Refine raise an issue behavior #753
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
howard-e
requested changes
Aug 22, 2023
There was a problem hiding this comment.
@alflennik thanks! I went through the functionality and the queries and also verified that the various raise an issue buttons are working as expected by bringing up the proper populated content on the GitHub page. And the issues can also be queried as expected, through the app. That's great!
I've also noticed a few issues which I think need to be addressed first, and have shared those thoughts below.
Something additional which can be looked at is that the issues can be duplicated when displayed on the Test Plan Versions History page since the same NVDA issue could be displayed across the Chrome and Firefox results as an example.
An example of this can be seen in the below screenshot. I could see these changes on the Color Viewer Slider Test Plan Versions page since there were already some examples of those in the testing repo. (thanks for that!)
| @@ -30,7 +30,6 @@ | |||
| "@fortawesome/fontawesome-svg-core": "^6.2.1", | |||
| "@fortawesome/free-solid-svg-icons": "^6.2.1", | |||
| "@fortawesome/react-fontawesome": "^0.2.0", | |||
| "@react-hook/resize-observer": "^1.2.6", | |||
There was a problem hiding this comment.
1 less dependency is great!
Comment on lines
333
to
338
| const generalFeedbackUrl = createIssueLink({ | ||
| ...issue, | ||
| testTitle: undefined, | ||
| testRowNumber: undefined, | ||
| testRenderedUrl: undefined | ||
| }); |
There was a problem hiding this comment.
I'd think this could also be isCandidateReviewChangesRequested: false which makes me feel like some "control" is also needed here. Where this is used (on the Thank You modal), the overall feedback could really either be just feedback or requested changes.
Should we make that explicit with the links that are present on that modal?
There was a problem hiding this comment.
I think I addressed that unless I possibly misunderstood you, would you mind looking to confirm?
| const feedbackGithubUrl = getIssueSearchLink(issueQuery); | ||
|
|
||
| const changesRequestedGithubUrl = getIssueSearchLink({ | ||
| issueQuery, |
There was a problem hiding this comment.
Suggested change
| issueQuery, | |
| ...issueQuery, |
client/utils/createIssueLink.js
Outdated
|
|
||
| let title; | ||
| if (hasTest) { | ||
| const titleStart = isCandidateReviewChangesRequested |
There was a problem hiding this comment.
I do think maintaining the title as only including "Feedback" and then relying on relevant labels has been the standard, but I do also understand how this works out well to help with making less assumptions searches from the applicaton.
I'd be curious if there would be anyone against this particular change.
There was a problem hiding this comment.
Your comment actually helped me realize that things were not inconsistent by accident but actually had a kind of structure to it. I'm going to make it use the previous behavior.
client/utils/createIssueLink.js
Outdated
| : 'label:feedback', | ||
| `label:${atLabelMap[atName]}`, | ||
| username ? `author:${username}` : '', | ||
| `"${atName} Feedback"`, |
There was a problem hiding this comment.
Suggested change
| `"${atName} Feedback"`, | |
| `"${atName} ${isCandidateReviewChangesRequested ? 'Changes Requested' : 'Feedback'}"`, |
Given that the title could be different if changes were requested through the application
There was a problem hiding this comment.
Thanks, I decided to make this use the at label!
| isCandidateReview: true, | ||
| isCandidateReviewChangesRequested: | ||
| index === 0, | ||
| username: data.me.username, |
There was a problem hiding this comment.
Suggested change
| username: data.me.username, | |
| // username: data.me.username, |
I think it's safer to not include the username so the getIssueSearchLink doesn't parse that into the link being checked. It's safer in the instances where the feedback is coming from an author who isn't data.me. Otherwise, special checks should be done when generating this GitHub url.
| <thead> | ||
| <tr> | ||
| <th>Author</th> | ||
| <th>Issue</th> |
There was a problem hiding this comment.
I wonder if having an additional column dedicated to the AT the issue is filed against, would be helpful in quickly parsing the content of the issues table
There was a problem hiding this comment.
Nice idea, added.
|
@howard-e this should be ready for a second look |
howard-e
approved these changes
Aug 24, 2023
There was a problem hiding this comment.
@alflennik LGTM! I've gone through the functionality and it all looks good to me, and thanks for addressing the feedback!
It's great that the raise an issue buttons are now all consistent and a natural next step here, after this hits production would be updating the titles of the aria-at feedback issues so the app can properly query for them.
This was referenced Aug 24, 2023
howard-e
added a commit
that referenced
this pull request
Sep 28, 2023
…abase Implementation to support #648 (#688) Changes added to primarily support #648, but also #518 and w3c/aria-at#950. This includes functional changes to the Data Management, the Test Queue and the Candidate Review pages. It also includes changes to the database structure, described in #632. * feature: updates functionality of Data Management page (#713) * feature: updates functionality of Test Queue page (#715) * feature: updates functionality of Candidate Review page (#715) * feature: include functionality to support the concept of required reports (#722) * feature: adds Test Plan Report Status dialog (#728) * enhancement: move Candidate Phase Start Date and Target Completion Date into dedicated columns on Candidate Review page (#730) * feature: adds Test Plan Version page (#747) * enhancement: explicitly support ‘DEPRECATED’ phase status for TestPlanVersions (#749) * feature: adds filter and sorting functionality by columns headers on Data Management page (#750) * bugfix: update semantic structure of cells with multiple list items on Data Management page (#752) * enhancement: include GitHub issues on Test Plan Version page (#753) * bugfix: revision of the required reports conditions for updating to CANDIDATE and RECOMMENDED phases (#764) * bugfix: removes superfluous header from Test Plan Report Status dialog (#766) * bugfix: update and revise sorts, headings and descriptions of elements on Test Plan Version page (#767) * bugfix: account for several other update phase scenarios that could prevent the update from happening if there is an older TestPlanVersion that exists with results (#771) * bugfix: update headings and revise deprecated dates shown on Test Plan Version page (#773) * enhancement: allow updating of GitHub issues being presented in the app to be more easily understood (#775) * bugfix: correct deprecatedAt date to be relative to when the ‘next’ TestPlanVersion was added (#780) * enhancement: update the text shown when deprecation occurs during a phase on Test Plan Version page (#781) * bugfix: fix inverted sort descriptions and pin sort of of Test Plan name columns on Data Management page (#790) --------- Co-authored-by: Erika Miguel <[email protected]> Co-authored-by: Paul Clue <[email protected]> Co-authored-by: alflennik <[email protected]> Co-authored-by: Stalgia Grigg <[email protected]> Co-authored-by: Howard Edwards <[email protected]>
alflennik
added a commit
that referenced
this pull request
Oct 18, 2023
* Fix bug that would allow testPlanVersion to be updated to RECOMMENDED before the associated reports were all marked as final (but the tests were 100% done in the Test Queue) * Refine raise an issue behavior (#753) * Refine raise an issue behavior * Address feedback * Fixed squished dot icon * Address last feedback * Hide closed issues on datamgmt page * Fix for test results not being automatically saved when navigating through Test Run * Filter and sort functionality for Data Management table (#750) * First pass on functioning sort buttons for DataManagePage * Functioning sort * Filter functionality for DataManagement * Refactor filter buttons on DataManagement to reduce complexity * Filter buttons as separate component * DataManagement page, break sort out into dedicated hook * DataManagement page, dedicated hook for filtering * DataManagement, add constant for test plan version phases to simplify hooks code * Add relevant dynamic aria attributes to DataManagement column sort elements * Add relevant dynamic aria attributes to DataManagement filter buttons * unit tests for SortableTableHeader * unit tests for FilterButtons * unit tests for useDataManagementTableSorting hook * Add unit tests for useDataManagementTableFiltering hook * Filter buttons only show buttons that have have label, handle testPlans with multiple versions, ... useDataManagementTableFiltering don't generate label for buttons that have no associated plans * Break out overall phase derivation logic into dedicated hooks, functional filter and sort * Simplify useDerivedTestPlanOverallPhase * Additional unit tests for additional hooks and to test scenarios with multiple test plan versions for a single test plan hook rename * Different UX click interaction sequence with SortableTableHeader * Correct interpretation of alphabetical ascending/descending * Rename DataManagement/hooks.js to filterSortHooks * Move sorting and filtering enums to more specific locations based on use * Fix file locations in unit tests, DataManagement, FilterButtons, SortableTableHeader * Smaller margin between buttons, FilterButtons * First pass on functioning sort buttons for DataManagePage * Functioning sort * Filter functionality for DataManagement * Refactor filter buttons on DataManagement to reduce complexity * Filter buttons as separate component * DataManagement page, break sort out into dedicated hook * DataManagement page, dedicated hook for filtering * DataManagement, add constant for test plan version phases to simplify hooks code * Add relevant dynamic aria attributes to DataManagement column sort elements * Add relevant dynamic aria attributes to DataManagement filter buttons * unit tests for SortableTableHeader * unit tests for FilterButtons * unit tests for useDataManagementTableSorting hook * Add unit tests for useDataManagementTableFiltering hook * Filter buttons only show buttons that have have label, handle testPlans with multiple versions, ... useDataManagementTableFiltering don't generate label for buttons that have no associated plans * Break out overall phase derivation logic into dedicated hooks, functional filter and sort * Simplify useDerivedTestPlanOverallPhase * Additional unit tests for additional hooks and to test scenarios with multiple test plan versions for a single test plan hook rename * Different UX click interaction sequence with SortableTableHeader * Correct interpretation of alphabetical ascending/descending * Rename DataManagement/hooks.js to filterSortHooks * Move sorting and filtering enums to more specific locations based on use * Fix file locations in unit tests, DataManagement, FilterButtons, SortableTableHeader * Smaller margin between buttons, FilterButtons * Cleanup after rebase * Render SortableTableHeader inside <table> for test to dismiss warning * Add AriaLiveRegionProvider, Update DataManagement table to use it * Allow SortableTableHeader component to work without AriaLiveRegionProvider * Explicitly support 'DEPRECATED' phase for `TestPlanVersion.phase` (#749) * Start to support for sunset phase * Explicitly use 'DEPRECATED' phase for TestPlanVersion.phase * Add checks to prevent test plan versions in R&D or Deprecated from being shown in data management dropdown * Remove duplicate updateTestPlanVersion call * Reuse phase variable * Add copy for deprecated reports with DisclaimerInfo component * Exclude 'DEPRECATED' testPlanVersions on DataManagement page query * Ensure only reports marked as final are displayed on /embed/reports/<pattern> * Fix test * Update failing test * Adjust semantic structure on Data Management Page (#752) * Adjust cell items for data management row to use list-related roles; update aria-labels * Address PR feedback * Remove width:max-content * Address PR feedback * Formatting * Address feedback * Adjust BasicModal to support AtAndBrowserDetailsModal closing * Stop assign menu dropdown from creating unintended bottom space with the parent container * Formatting * Close #755 * Close #754 * Remove superfluous header from TestPlanReportStatusDialog (#766) * Revise required reports conditions (#764) * Revise required reports approach * Update tests * Revert dev.env * Update Version History Page (#767) * Apply correct sort order for Timeline for All Versions section * Update headings used on Versions page * Add aria-labelledby's for tables * Add for applicable spaces so the text is properly announced by NVDA * Add migration to add missing deprecatedAt dates and to also properly set the candidatePhaseReachedAt dates to more practical dates after the migrations (if candidatePhaseReachedAt < draftPhaseReachedAt, set it candidatePhaseReachedAt to draftPhaseReachedAt + 1day) * Test Plan Versions Page: Use standard testPlanVersions descending sort and show all phases being included in Version Summary * Switch issues search support for checking against hidden body content instead * Fix TestPlanReportStatusDialog using d (day of the week) and y (era) * Explicit check for older date when deprecating existing RD test plan versions during import * Update hidden message content in github issue * Update query for anon user on TestRun page * Keep overall status pill from drifting to center of cell * Fix R&D only TestPlanVersion's table not being shown * Use aria-label for heading to prevent space being announced * Update date format used in aria-label * Make version history a drop down * comment out unused variable * Fix responsiveness * Remove comments and logs * Changes after merge * Fix icon color * Fix padding for timeline heading * Fix console errors and delete comments * Fix conflicts that caused error * Implement required changes from review --------- Co-authored-by: Howard Edwards <[email protected]> Co-authored-by: Alexander Flenniken <[email protected]> Co-authored-by: Stalgia Grigg <[email protected]>
stalgiag
added a commit
that referenced
this pull request
Jan 22, 2024
* Fix bug that would allow testPlanVersion to be updated to RECOMMENDED before the associated reports were all marked as final (but the tests were 100% done in the Test Queue) * Refine raise an issue behavior (#753) * Refine raise an issue behavior * Address feedback * Fixed squished dot icon * Address last feedback * Hide closed issues on datamgmt page * Fix for test results not being automatically saved when navigating through Test Run * CollectionPass model added to schema, functioning getById resolver * Working add mutation for CollectionJob * Create, find, findorcreate, findorupdateorcreate for CollectionJob * Delete for CollectionJob, Add tests for schema * Routes for Automation Scheduler interaction with unit tests * Unit tests for updating CollectionJob * Verify DB updates in Automation Scheduler unit tests, rollback DB changes after tests * Add tests for CollectionJob spec * Unit tests for CollectionJobService * Add testPlanRun to CollectionJob schema * Remove unused dependency, automation-scheduler.test * Link CollectionJob to TestPlanRun for specific TestPlanReport * Populate test database with CollectionJob, Nullable relation on TestPlanRun delete * Create services to replace resolver dependency for Test and TestResult retrieval * Fix unit tests * Singleton for AtLoader * Singleton for BrowserLoader * Decouple populateData and GraphQL context * Service for getFinalizedTestResults * findOrCreateTestResult Service * Move auth checks into resolvers for submitTestResult and saveTestResult, avoid passing context for persistConflictCount, more work on AutomationController * Create test result from automated result submitted to route '/result' and test * Cleanup of AutomationController * Remove TestPlanRun when deleting CollectionJob * Remove all remaining instances of AtLoader or BrowserLoader use from GraphQLContext * Remove unused import * Spike on validation of Automated TestResult against historical Test Results * Refactor to isolate state in automation scheduler tests * CollectionJob model testing, test association * Speed improvement on automation scheduler tests, Remove unneeded CollectionJob seeder * Remove unused imports * Cleanup AutomationController and CollectionJobService * Construct Test data request body for Automation Scheduler, tests * Remove hard-coded values in automation scheduler test * Remove unused imports automation scheduler * Switch tested report row for service tests to prevent intermittent deadlock issues with concurrent tests * Secure CollectionJob resolvers * Error handling middleware and more error checking for AutomationController * Replace Failed CollectionJobStatus with Error, use enum for statuses * Schedule new collection job only available through authenticated gql mutation * Restart collection job only available through authenticated gql mutation * Cancel collection job only available through authenticated gql mutation * Skip schema test for cancelCollectionJob * Delete collection job only available through authenticated gql mutation * Update server/graphql-schema.js Co-authored-by: jugglinmike <[email protected]> * Literal strings for job status in tests * router.use handleError in automation routes * Cleanup after merge * Multiple action buttons supported and test, BasicModal * Schedule collection job through Test Plan Report Status Dialog * Completion status for automated runs * Allow mock automation scheduler to post results * Real time test completion re-renders * Finish bot run button skeleton * Support reassigning of bot jobs, add related UI * Column in TestPlanRun for indicating that a job was initially automated * Allow assertion writing by reassigned human tester, correctly calculate possible assertions * Render tests evaluated for reassigned bot tests * Support marking collection job as finished on server * Real-time status updates for automated tests * Info boxes when reviewing bot run * Create new collection job directly from assigning bot tester * Delete test plan run from Mark Finished automation dialog * Test run navigation bot-specific status icons * Rename bot user * Create FinishBotRunDialog.test.jsx * Test custom components with actions in BasicModal * Testing for AssignTesterDropdown * Fix merge issues, handle alert for AssignTesterDropdown, move relevant styles to dedicated file * Remove errant useRef * max-height instead of height for assign-menu * useOnHide and Allow multiple modals to be open at a time. * Add tests for BotRunTestStatusList, ensure correct display * Allow testing of historical results flow * Silence console errors related to initiatedByAutomation in client test mocks * Ensure correct at id and browser id is used for getting historical results * Rename 'FinishBotRun' dialog to 'ManageBotRun', add missing actions * Fix failing test with incorrect import, ManageBotRunDialog * Don't delete test results but test plan run * Simplify and correct historical test validation * Remove unused imports * Mark as finished button behavior, allow submission of existing results without providing complete input * Fix bug in AddTestToQueueWithConfirmation when no title present, remove jest debug * Centralize ApolloClient * Setup for ManageTestBotRunDialog actions * Retry cancelled jobs * Remove deprecated collection job cancellation * Force IPv4 for internal HTTP requests * fixup! Force IPv4 for internal HTTP requests * Add updated copy in body of add test to queue modal * Fix sequencing of collectionjobs for manual id creation * Unassign yourself on historical bot run does not reassign to bot * Only show manage bot run button when run is not finished * Switch to using testCsvRow for updating results * Safely handle viewing from logged out state * External logs url for CollectionJob * Update AutomationController to support creating missing At or Browser Versions * Cleanup * Don't allow assigning bot runs to tester with run in same test plan report * Defer update resolution after the timeout fires in closeWithUpdate, AddTestToQueueWithConfirmation * Pluralization for BotRunTestStatusUnorderedList * Plurals for 0, update unit test, rename filteredTestPlanRuns to botTestPlanRuns in BotRunTestStatusList * Remove unnecessary checks, BotRunTestStatusList * Correct useTriggerLoad use * Better distinguish click assertions in BasicModal unit test * Prevent failing unit tests in dev * getBotUserFromAtBrowser -> getBotUsernameFromAtBrowser * Type fix in ManageBotRRunDialog Co-authored-by: jugglinmike <[email protected]> * Missing conflicts length conditional for Mark as Final button in TestQueueRow * Fix issue with TestNavigator recognizing test skeleton as complete * Handle initial render null browserVersion graphql issue * readOnly for TestRun textarea when reviewing bot run * Use test plan report runnable tests instead of test plan version tests for matching results to ids * Remove unused imports from previous commit * Only show bots on assign dropdown for bot-supporting test plan reports * Update client/hooks/useTestPlanRunValidatedAssertionCounts.js Co-authored-by: jugglinmike <[email protected]> * Simplify testsResultsLength calculation in useTestPlanRunValidatedAssertionCounts.js Co-authored-by: jugglinmike <[email protected]> * Remove unused useMemo dependencies, line length edit * Default value for testResults in PreviouslyAutomatedTestCompletionStatus * Remove deprecated props for BasicModal * Default parameter for new Date() in AtService * Typo in BotRunTestStatusList, Additional testing for AssignTesterDropdown * Fix rename in export, retryCancelledCollections * Fix issue with merge conflict resolution * Additional fix to readOnly text area after merge conflict resolution * Do not render zeros in place of omitted content (#853) By relying on the "truthiness" of Number values to short-circuit boolean expressions, some rendering logic produced the text "0" instead of omitting any markup at all. Replace the sub-expressions whose value could be coerced to booleans with sub-expressions which directly evaluate to booleans (and which do not produce markup). * Correct heuristic for identifying "finished" runs (#854) * Correct heuristic for identifying "finished" runs Prior to this commit, the user interface reported that Test Plan Runs were "finished" when they included zero test results. This state, though difficult to observe in development environments, will be apparent whenever there is a perceivable delay while scheduling collection jobs (including when collection jobs are never scheduled due to some unexpected error). Test Admins were unable to manage Test Plan Runs from this state. Update the heuristic to identify Test Plan runs in this state as "unfinished." * fixup! Correct heuristic for identifying "finished" runs * Store "No output was detected." for empty string output during automated tests (#857) * When an empty string is recorded as an output by a bot, store as No Output string * Handle null and white space cases * Use constant for no output string * Annotate the mirrored constants and update the name of the client constant * Add cutoff date as temporary fix to handle deviation of automation-mvp from main (#858) * Correct test identification heuristic Co-author: Stalgia Grigg <[email protected]> * Show tests as queued even before the job starts running, BotRunTestStatusList (#860) * Make automation result posting unit tests and simulator use rowNumber instead of index (#862) * fixup! make automation result posting unit tests use rowNumber instead of index * Update mock automation scheduler to send rowNumber instead of index * Start Automation Collector via github action (#831) * Squash of first draft WIP for automation collector via github * Insert JWT-signing key * move github workflow service, load key on app startup * move files around, lint cleanup * trying to cleanup switchpoint/implement retry * review fixes * more conversion to trigger workflows * more review comment cleanup * removing nonce * remove TODO that wont ever be TODONE * re-add return job that was forgotten * moving mock server endpoints around * pull switchpoint into triggerWorkflow * adding testIds param to triggerWorkflow * await updateCollectionJob and handle null in restartCollectionJob * add missing await * convert fetch to axios --------- Co-authored-by: Mike Pennisi <[email protected]> Co-authored-by: Stalgia Grigg <[email protected]> * Correct erroneous path in deployment script * Define automation-related variables in ENV files * single source of truth for env var (#864) * Nullable atVersion * Fixes after merge * Remove aria-at git commit cut off date * Clean up leftover import * Correct bug in dropdown rendering (#869) Limit the insertion of horizontal padding to the specific case which requires it. * self-tester right of passage moment (#865) * Handle v2 pass/fail for "tests evaluated" and "assertionsAssigned" view * Rename retryCancelledCollections to retryCanceledCollections * Shorten comment * Require scenarioResults in schema * Remove unexpected behaviors default * Fix tests for null unexpectedBehavior * Prevent multiple bot runs in the same test plan report (#866) * Prevent multiple bot runs in AddTestToQueueWithConfirmation * Render checkmark for assigned bot * Update AddTestToQueueWithConfirmation test * More readable conditional sequence in AssignTeserDropdown * useMemo for alreadyHasBotInTestPlanReport in AddTestToQueueWithConfirmation * Update deploy readme for Workflow Automation * Raise height of text area slightly to make possible scroll more obvious * JSDoc for TestResultReadService * Move new get tests code from resolver to service * Add NVDA bot user using a seeder (#881) * Add NVDA Bot user using seeder * Populate NVDA Bot user from single source, remove code for creating user in service * Use centralized username for bot user Co-authored-by: jugglinmike <[email protected]> --------- Co-authored-by: jugglinmike <[email protected]> * Support automation reporting test results by presentationNumber for v2 tests (#886) * Support rowNumber identification using presentationNumber for v2 tests * Update error message --------- Co-authored-by: Mike Pennisi <[email protected]> * Require intentional cache clearing for AtLoader/BrowserLoader * Specify AT on test result retrieval * Only check at on v2 tests --------- Co-authored-by: Howard Edwards <[email protected]> Co-authored-by: Alexander Flenniken <[email protected]> Co-authored-by: jugglinmike <[email protected]> Co-authored-by: Mx. Corey Frang <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Note: For testing purposes new issues will be created in the
howard-e/aria-at-apprepo instead of the correctw3c/aria-atrepo. Once the review process is complete I will point the links back at the correct location.This standardizes the raise an issue buttons across the app and fixes a few bugs related to them. It also adds a table in the TestPlanVersion page which will show all issues.