Allow more batch actions in the add-on store (remove, enable and disable) by CyrilleB79 · Pull Request #15646 · nvaccess/nvda

CyrilleB79 · 2023-10-17T22:24:09Z

Link to issue number:

Closes #15623

Summary of the issue:

In the store, install action can be executed on multiple add-ons. Other batch actions are needed.

Description of user facing changes

The following additional batch actions are now available in the store:

Remove selected add-ons
Update selected add-ons
Enable selected add-ons
Disable selected add-ons

Description of development approach

Use the same condition as the one required for the corresponding single action to occur.
For code simplicity there is no distinction between Enable and Enable (override) for the corresponding multiple action. Anyway, when an add-on should override compatibility to be enabled, there is a confirmation dialog box.
Also for simplicity, there is no multiple replace action:
- if a multiple selection contains installable add-on with or without updatable or replaceable add-ons, the "Install selected add-ons" batch action will be available
- If a multiple selection contains updatable add-ons, replaceable add-ons or both types, the "Update selected add-ons" batch action will be available.
For batch actions requiring a confirmation (e.g. override incompatibility, uninstall), the confirmation dialog box has a "Remember this choice" checkbox so that the user can answer yes or no for all the same questions.
When many add-ons are selected and a batch action is applied to them, the action actually applies only to the add-ons for which it is appropriate. E.g batch enable will not be applied to already enabled add-ons.

Testing strategy:

Manual testing of each of the multiple actions. Also re-tested the corresponding single selection actions.

Known issues with pull request:

Change log:
I have modified the change log item since batch install was available but I have implemented all the other actions. I have also added my GitHub's name. Unfortunately, it gives the impression that I have made all the job, whereas @seanbudd / NV Access has provided the initial work in #15350. Is it worth thinking to a way to show that an item has been contributed both by NV Access and an external contributor? Or by two external contributors? This would need to be clarified.

Code Review Checklist:

Documentation:
- Change log entry
- User Documentation
- Developer / Technical Documentation
- Context sensitive help for GUI changes
Testing:
- Unit tests
- System (end to end) tests
- Manual testing
UX of all users considered:
- Speech
- Braille
- Low Vision
- Different web browsers
- Localization in other languages / culture than English
API is compatible with existing add-ons.
Security precautions taken.

…ble)

amirmahdifard · 2023-10-17T23:57:19Z

@CyrilleB79 hi, thanks for this: please also make this for updating addons too: emajin there 8 addons in the updatable addons tab, wich needs update: we have to update them one by one, but if you make this action available to updatable addons as whel, we can select all addons we want to update, and then do update to update them at final. thanks.

AppVeyorBot · 2023-10-22T21:03:14Z

PASS: Translation comments check.
PASS: Unit tests.
FAIL: Lint check. See test results for more information.
PASS: System tests (tags: installer NVDA).
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/19ix1qupy4dtsln3/artifacts/output/nvda_snapshot_pr15646-29693,33addd59.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.9,
INSTALL_END 0.9,
BUILD_START 0.0,
BUILD_END 26.0,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 8.1,
FINISH_END 0.1

See test results for failed build of commit 33addd5950

…ble, uninstall and enable incompatible: use a "Remember my choice" checkbox.

AppVeyorBot · 2023-11-10T17:08:04Z

FAIL: Translation comments check. Translation comments missing or unexpectedly included. See build log for more information.
PASS: Unit tests.
PASS: Lint check.
PASS: System tests (tags: installer NVDA).
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/ps9h66exujjk7ojl/artifacts/output/nvda_snapshot_pr15646-29938,b29435b5.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.9,
INSTALL_END 0.9,
BUILD_START 0.0,
BUILD_END 29.3,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 16.9,
FINISH_END 0.1

See test results for failed build of commit b29435b52e

seanbudd

Changes generally look good to me

CyrilleB79 · 2023-11-14T10:36:14Z

@seanbudd, I have applied review comments. Moreover, I have use more canUseXxxAction when possible and rephrased the log messages.

I am still concerned with one point:
In your previous work, batch install was operating either on available add-ons and on updatable add-ons. In the initial description of this PR, I indicate that I handle batch install, update and replace separately for simplicity but that's not true for install since I have kept your original condition (available or updatable).
After thinking again to this, maybe the best solution would be to commonalize these three task and just provide:

"Install selected add-ons" in "Available add-ons tab to operate on available, updatable and migratable add-ons, i.e. canUseInstallAction or canUseUpdateAction or canUseReplaceAction
"Update selected add-ons" in other tabs

The limitation of this solution are:

You can have "Install selected add-ons" on a selection that contains only updatable or migratable add-ons. I think that it is understandable anyway.
You can have "Update selected add-ons" on a selection containing only migratable add-ons. I think that's not a problem since there is the dialog box recording that the versions cannot be compared.

Do you agree with this approach? If no, what do you suggest instead?

seanbudd · 2023-11-21T23:36:50Z

The approach you describe sounds good, I don't think multiple actions are required for install vs. update vs. replace

CyrilleB79 · 2023-11-24T09:34:23Z

@seanbudd, at the end, I have implemented a mid-way solution regarding the install/update/migrate batch actions. It allows to have update batch action when only updatable or migratable (replaceable) add-ons are selected, but no installable one. I have updated the initial description accordingly; see it for more details.

seanbudd

Thanks @CyrilleB79

Qchristensen

Reads well, good work.

Allow more batch actions in the add-on store (remove, enable and disa…

da170c4

…ble)

CyrilleB79 added 3 commits October 18, 2023 18:19

Add update batch action

ee36d89

Merge branch 'master' into batchActions

4375dac

Batch install only if one add-on is Available or Incompatible Available

12e7dad

CyrilleB79 added 6 commits October 23, 2023 17:55

Display available batch actions according to the selected add-ons

81aad2d

Fix "Replace" always present

38d79ac

Fix issue when batch disabling add-ons

89b9ae6

Merge branch 'master' into batchActions

42e5716

New process to confirm/decline multiple actions for install incompati…

dbb18b9

…ble, uninstall and enable incompatible: use a "Remember my choice" checkbox.

Some code clean-up

5c255bb

CyrilleB79 added 5 commits November 11, 2023 00:00

Fix checkPot

86a4cff

Allow batch update in same conditions as single update

33ea0ed

Add batch replace action

502361e

Documentaztion: Change log and User Guide

f4212ed

Merge branch 'master' into batchActions

00eb6f4

CyrilleB79 marked this pull request as ready for review November 13, 2023 13:50

CyrilleB79 requested review from a team as code owners November 13, 2023 13:50

CyrilleB79 requested review from Qchristensen and seanbudd November 13, 2023 13:50

seanbudd reviewed Nov 14, 2023

View reviewed changes

CyrilleB79 added 2 commits November 14, 2023 11:07

Address review comments and more

c274ec2

Rewrite log messages

d76bd8a

seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Nov 21, 2023

seanbudd marked this pull request as draft November 21, 2023 23:37

CyrilleB79 added 3 commits November 22, 2023 14:42

Addressing review

fabeece

Rework multiple action availability

d8e3ed7

Merge branch 'master' into batchActions

957ea80

CyrilleB79 marked this pull request as ready for review November 24, 2023 09:36

Remove debug code

7f031ce

lukaszgo1 reviewed Nov 24, 2023

View reviewed changes

Comment thread source/gui/addonStoreGui/viewModels/store.py Outdated

Comment thread source/gui/addonStoreGui/viewModels/store.py Outdated

Small clean-up

6bea8ed

seanbudd approved these changes Nov 27, 2023

View reviewed changes

Qchristensen approved these changes Nov 27, 2023

View reviewed changes

seanbudd merged commit 7a328bd into nvaccess:master Nov 27, 2023

nvaccessAuto added this to the 2024.1 milestone Nov 27, 2023

CyrilleB79 deleted the batchActions branch November 27, 2023 21:22

Uh oh!

Conversation

CyrilleB79 commented Oct 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to issue number:

Summary of the issue:

Description of user facing changes

Description of development approach

Testing strategy:

Known issues with pull request:

Code Review Checklist:

Uh oh!

amirmahdifard commented Oct 17, 2023

Uh oh!

AppVeyorBot commented Oct 22, 2023

Uh oh!

AppVeyorBot commented Nov 10, 2023

Uh oh!

seanbudd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CyrilleB79 commented Nov 14, 2023

Uh oh!

seanbudd commented Nov 21, 2023

Uh oh!

CyrilleB79 commented Nov 24, 2023

Uh oh!

Uh oh!

Uh oh!

seanbudd left a comment

Choose a reason for hiding this comment

Uh oh!

Qchristensen left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

CyrilleB79 commented Oct 17, 2023 •

edited

Loading