nvda icon indicating copy to clipboard operation
nvda copied to clipboard

Create PR revert template

Open seanbudd opened this issue 2 months ago • 3 comments

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.

seanbudd avatar Apr 24 '24 04:04 seanbudd

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.

CyrilleB79 avatar Apr 29 '24 15:04 CyrilleB79

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?

seanbudd avatar May 01 '24 01:05 seanbudd

  • 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

AppVeyorBot avatar May 01 '24 02:05 AppVeyorBot