nvda
nvda copied to clipboard
Create PR revert template
Link to issue number:
None
Summary of the issue:
Revert PRs are janitorial actions which do not make sense to follow the PR template. Typically these are made adhoc using the GitHub UX. When performing a revert, it is important to note issues being opened and closed by the reversion. It is also vital to note the PR being reverted. Adding a short reason for reversion will help people understand a summary of what is going on.
Description of user facing changes
None
Description of development approach
Created a new template to be copy-pasted when performing a revert
Testing strategy:
N/A
Known issues with pull request:
GitHub has a suggested UX for multiple PR templates: https://github.blog/2018-01-25-multiple-issue-and-pull-request-templates/ The GitHub UX for having multiple PR templates is less than ideal. It's like landing at https://github.com/nvaccess/nvda/issues/new rather than https://github.com/nvaccess/nvda/issues/new/choose. I figure this process may annoy some contributors.
Additionally, when performing a revert through GitHubs UX, the PR template is automatically overriden anyway, meaning the UX for multiple PR templates is bypassed anyway. I've opened a feature request here to improve this design: https://github.com/orgs/community/discussions/121757
Code Review Checklist:
- [x] Documentation:
- Change log entry
- User Documentation
- Developer / Technical Documentation
- Context sensitive help for GUI changes
- [x] Testing:
- Unit tests
- System (end to end) tests
- Manual testing
- [x] UX of all users considered:
- Speech
- Braille
- Low Vision
- Different web browsers
- Localization in other languages / culture than English
- [x] API is compatible with existing add-ons.
- [x] Security precautions taken.
IMO, the expectation from NV Access should be clarified when reverting, maybe in the "Reason for revert" section?
E.g.: "We have reverted because the feature is buggy and we are in beta stage" is correct. But it may not be enough for a dev who want to tackle the issue again. The dev want to have such additional information:
- the feature may be accepted if the issue #XYZ is fixed in the future?
- some design decision need to be made before a new implementation is proposed
- NV Access has changed their mind and think this feature should rather belong to an add-on, due to XYZ.
This information may be provided in the revert PR or in the issue being reopened (if any), but in any case, it should be made available at the same time the revert occurs.
I've updated this PR significantly to avoid GitHub's poor UX for multiple PRs.
@CyrilleB79 - do you think anything is missing from the new template?
- PASS: Translation comments check.
- PASS: Unit tests.
- PASS: Lint check.
- FAIL: System tests (tags: installer NVDA). See test results for more information.
- Build (for testing PR): https://ci.appveyor.com/api/buildjobs/oqrb89e2bbsnfj1a/artifacts/output/nvda_snapshot_pr16446-31694,dcaa64e5.exe
- CI timing (mins): INIT 0.0, INSTALL_START 1.1, INSTALL_END 1.1, BUILD_START 0.0, BUILD_END 30.9, TESTSETUP_START 0.0, TESTSETUP_END 0.4, TEST_START 0.0, TEST_END 2.6, FINISH_END 0.2
See test results for failed build of commit dcaa64e51e