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

Explicitly support 'DEPRECATED' phase for TestPlanVersion.phase #749

Merged
merged 7 commits into from
Aug 28, 2023

Conversation

howard-e
Copy link
Contributor

@howard-e howard-e commented Aug 21, 2023

Closes #741

Currently, the 'deprecated' phase can be derived within the client. But it can be be easier to have that explicitly set so the state of the database is represented better, and so special queries and resolvers can be avoided.

Deriving in-app evaluations such as figuring out when a deprecation happened, such as for the Test Plan Version History page also proves to be more manageable in this instance.

This PR updates the usage of the 'DEPRECATED' phase for:

  • TestPlanVersionsPage
  • Preventing deprecated (and R&D) test plan versions from being shown in the data management dropdown

@@ -84,8 +84,9 @@ const content = {
const DisclaimerInfo = ({ phase }) => {
const [expanded, setExpanded] = useState(false);

const title = content[phase].title;
const messageContent = content[phase].messageContent;
const title = content[phase]?.title || content.CANDIDATE.title;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needed in the instance that a TestPlanVersion's phase is marked as DEPRECATED, but reports did exist if it was deprecated during the candidate or recommended phase.

It could be considered as 'unapproved' in that case but should another copy be defined for this instead? Or some alert that this set of reports has been deprecated?

Copy link
Contributor

Choose a reason for hiding this comment

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

My personal opinion is that different copy should be defined for TestPlanVersion's with a phase of DEPRECATED. The copy could be similar to the CANDIDATE copy but could mention the deprecation, and potentially one day point to the current related TestPlanVersion if it exists.

Making different copy for DEPRECATED would also make this code easier to understand without context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My personal opinion is that different copy should be defined for TestPlanVersion's with a phase of DEPRECATED. The copy could be similar to the CANDIDATE copy but could mention the deprecation, and potentially one day point to the current related TestPlanVersion if it exists.

Yep, agreed. That would be good.

Making different copy for DEPRECATED would also make this code easier to understand without context.

Thanks for your thoughts there. I agree, will draft something share.

@howard-e howard-e requested a review from alflennik August 21, 2023 17:10
@howard-e howard-e marked this pull request as ready for review August 21, 2023 17:10
@howard-e howard-e requested a review from stalgiag August 21, 2023 17:11
Copy link
Contributor

@stalgiag stalgiag left a comment

Choose a reason for hiding this comment

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

With these changes, the related code is much easier to understand with limited context. Great work!

I manually updated two test plan versions in my DB to have a phase of DEPRECATED and a deprecatedAt of NOW(). I did some manual testing and everything appears to be rendering as expected with one exception, the "Overall Status" in the Data Management page table renders nothing. Since this PR adds explicit support for the DEPRECATED, it might be good to handle that case in renderCellForPhase() in DataManagementRow. The same phasePill color used on the TestPlanVersionsPage and a deprecated at ... string would feel natural here.

Another small comment inline in response to your question.

@howard-e
Copy link
Contributor Author

...with one exception, the "Overall Status" in the Data Management page table renders nothing. Since this PR adds explicit support for the DEPRECATED, it might be good to handle that case in renderCellForPhase() in DataManagementRow. The same phasePill color used on the TestPlanVersionsPage and a deprecated at ... string would feel natural here.

Thanks for catching that and the suggestion. Will look into it!

@howard-e howard-e force-pushed the support-deprecated-phase branch from 84d2dee to 8015688 Compare August 21, 2023 22:40
@howard-e
Copy link
Contributor Author

howard-e commented Aug 24, 2023

I did some manual testing and everything appears to be rendering as expected with one exception, the "Overall Status" in the Data Management page table renders nothing. Since this PR adds explicit support for the DEPRECATED, it might be good to handle that case in renderCellForPhase() in DataManagementRow. The same phasePill color used on the TestPlanVersionsPage and a deprecated at ... string would feel natural here.

@stalgiag looking over #648, and considering the flow of information, I don't think this condition should ever be possible. Versions are ideally only deprecated by being "overwritten" by a more recent version. So a deprecated test plan version should never be able to be shown at the top level (unless something was deleted directly from the database I suppose and that shouldn't happen). With this in mind, do you still think we should support displaying this?

If you agree with this, I'm thinking to be even safer, we could make sure the DataManagement page excludes the DEPRECATED TestPlanVersions during the query so there's no chance of any being displayed.

@howard-e howard-e force-pushed the support-deprecated-phase branch from 8015688 to 1cc16a1 Compare August 24, 2023 21:53
@stalgiag
Copy link
Contributor

I did some manual testing and everything appears to be rendering as expected with one exception, the "Overall Status" in the Data Management page table renders nothing. Since this PR adds explicit support for the DEPRECATED, it might be good to handle that case in renderCellForPhase() in DataManagementRow. The same phasePill color used on the TestPlanVersionsPage and a deprecated at ... string would feel natural here.

@stalgiag looking over #648, and considering the flow of information, I don't think this condition should ever be possible. Versions are ideally only deprecated by being "overwritten" by a more recent version. So a deprecated test plan version should never be able to be shown at the top level (unless something was deleted directly from the database I suppose and that shouldn't happen). With this in mind, do you still think we should support displaying this?

If you agree with this, I'm thinking to be even safer, we could make sure the DataManagement page excludes the DEPRECATED TestPlanVersions during the query so there's no chance of any being displayed.

This makes sense to me. I would be in favor of excluding DEPRECATED TestPlanVersions from being displayed.

@howard-e howard-e force-pushed the support-deprecated-phase branch from 87ccd24 to c0eee87 Compare August 28, 2023 20:04
@howard-e howard-e merged commit 232f0d0 into update-database-impl Aug 28, 2023
@howard-e howard-e deleted the support-deprecated-phase branch August 28, 2023 20:17
@mcking65
Copy link

that is correct, deprecated versions should never show on data management. they will only be visible from the test plan versions page.

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 &nbsp; 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]>
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.

Should not be able to add deprecated test plan versions to the test queue
3 participants