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

Notifying users of available add-on updates #16636

Merged
merged 34 commits into from
Jun 12, 2024
Merged

Notifying users of available add-on updates #16636

merged 34 commits into from
Jun 12, 2024

Conversation

seanbudd
Copy link
Member

@seanbudd seanbudd commented May 31, 2024

Link to issue number:

Closes #15035

Summary of the issue:

Users would like a push notification on NVDA start-up letting them know if add-ons have available updates

Description of user facing changes

When starting NVDA, a dialog will appear notifying users of updatable add-ons if there are available updates.
This will only notify users if the available update is in the same channel as the installed add-on.
There is an "update all" button on the dialog and a list of add-ons with updates available.
There is also an "Open Add-on Store" button to open the add-on store to the updatable add-ons tab.

A setting to disable this. This is a combobox so that in future users can decide between:

Description of development approach

Created a scheduling module to schedule tasks on NVDA start up, using the schedule pip module.
This is so we can have conflict free scheduling. e.g. so NVDA's update notification won't clash with the add-on update notification.
Uses their suggested code to run a background thread for task scheduling: https://schedule.readthedocs.io/en/stable/background-execution.html

Changed much of the Add-on Store downloading code to be classmethods. This allows us to use it from the message dialog without creating a store instance.

Testing strategy:

Manual testing
See the manual test plan for the add-on store.

Unit testing
Unit testing to smoke test handling the daily job at startup

Known issues with pull request:

None

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.

Summary by CodeRabbit

  • New Features

    • Introduced automatic update notifications for add-ons.
    • Added a daily job to check for updatable add-ons.
  • Enhancements

    • Updated Add-on Store to support updating multiple add-ons at once.
    • Improved Add-on Store dialog to open to a specific tab based on installed add-ons.
  • Documentation

    • Updated manual with instructions for simulating add-on updates and testing automatic update notifications.
  • Tests

    • Added unit tests for scheduling daily jobs and handling job clashes.

@josephsl

This comment was marked as resolved.

@AppVeyorBot

This comment was marked as resolved.

@seanbudd

This comment was marked as resolved.

@AppVeyorBot

This comment was marked as resolved.

Co-authored-by: WangFeng Huang <[email protected]>
@AppVeyorBot

This comment was marked as resolved.

@Adriani90
Copy link
Collaborator

Really cool to see NV Access starting work on this. Thanks Sean for this great contribution.

@AppVeyorBot

This comment was marked as resolved.

@AppVeyorBot

This comment was marked as resolved.

@AppVeyorBot

This comment was marked as resolved.

@AppVeyorBot

This comment was marked as resolved.

@seanbudd seanbudd marked this pull request as ready for review June 3, 2024 06:04
@seanbudd seanbudd requested review from a team as code owners June 3, 2024 06:04
@seanbudd

This comment was marked as resolved.

@CyrilleB79
Copy link
Collaborator

The feature is nice and I think that this can ship as is to be tested in alpha to get first public feedback.

Though, I can see these little points that may be fixed/updated:

  1. In the "Add-on updates available", there is a "State" column. But the state seems to be always "Update available". Or is there another possibility? If not, I'd remove this unuseful column.
  2. The Add-on Store settings panel indicates the profile name but I think that the notification parameter is only saved in the base config. So the title should not change (as per general settings)
  3. I have only two items in the "Update notifications" combo-box: "Notify (default)" and "Disabled". I would also expect "Notify" as done with with feature flags. Either you use a feature flag and use 3 values (Default (Notify), Notify and Disabled), or you just use a standard combo-box without indicating what the default is.

@seanbudd
Copy link
Member Author

seanbudd commented Jun 6, 2024

@CyrilleB79 thanks for looking into this:

  1. The state updates as add-ons are downloaded and installed, like within the add-on store
  2. I think I've fixed up profile announcements, please confirm
  3. We are using a combox box in this situation as a third state will be added in future, please see the PR description. I can remove the "default" part of the label

Copy link
Member

@Qchristensen Qchristensen left a comment

Choose a reason for hiding this comment

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

Looks good from docs point of view. Great work!

@seanbudd
Copy link
Member Author

seanbudd commented Jun 7, 2024

@coderabbitai review

@seanbudd seanbudd requested a review from michaelDCurran June 7, 2024 07:18
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 7

@nvdaes

This comment was marked as off-topic.

@CyrilleB79
Copy link
Collaborator

@seanbudd:

1. The state updates as add-ons are downloaded and installed, like within the add-on store

OK. Makes sense, especially when the download time is long.

2. I think I've fixed up profile announcements, please confirm

Confirmed. Thanks!

3. We are using a combox box in this situation as a third state will be added in future, please see the PR description. I can remove the "default" part of the label

Yes, it should be removed. But it is actually already.

@nvdaes, I agree to add in the settings panel (at the end) the checkbox corresponding to the "Don't show me again" checkbox of the add-on store. Though, it is not mandatory to discuss and add it in the same PR.

@nvdaes

This comment was marked as off-topic.

@CyrilleB79

This comment was marked as off-topic.

@Adriani90

This comment was marked as off-topic.

@nvdaes

This comment was marked as off-topic.

@seanbudd
Copy link
Member Author

Can we please keep conversation on topic for this PR, and create new issues for these. It's adding a lot of noise to the discussion here.

Copy link
Collaborator

@nvdaes nvdaes left a comment

Choose a reason for hiding this comment

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

I've seen the code for commits made today and also I've tested the Powershell scripts. For me it works as expected.

@seanbudd seanbudd merged commit d7facd1 into master Jun 12, 2024
3 checks passed
@seanbudd seanbudd deleted the updateAddons branch June 12, 2024 01:37
seanbudd added a commit that referenced this pull request Jun 17, 2024
Fixes #16684

Summary of the issue:
In #16636 / d7facd1, config.ConfigManager.BASE_ONLY_SECTIONS underwent a mutation from a set to a frozenset.
Thhis modification hinders extensions from easily expanding this set.

There are perfectly legit scenarios where an extension’s settings should not be overridden by profiles. Take webAccess, for instance: accessolutions/WebAccessForNVDA@4251aae/addon/globalPlugins/webAccess/config.py
SaschaCowley pushed a commit that referenced this pull request Feb 21, 2025
Fixes #17110

Summary of the issue:
Ever since #16636, users have been able to press the "update all" button to execute downloading and installing all the add-ons multiple times. This is because the update all button is disabled after the add-ons have finished downloading, not immediately after it is clicked, preventing subsequent activations.

Description of user facing changes
Users are unable to press update all button multiple times until add-ons finish downloading.

Description of development approach
Disable update all button immediately after first activation.

Testing strategy:
Manual testing by simulating slow internet updating multiple add-ons
gexgd0419 pushed a commit to gexgd0419/nvda that referenced this pull request Feb 22, 2025
Fixes nvaccess#17110

Summary of the issue:
Ever since nvaccess#16636, users have been able to press the "update all" button to execute downloading and installing all the add-ons multiple times. This is because the update all button is disabled after the add-ons have finished downloading, not immediately after it is clicked, preventing subsequent activations.

Description of user facing changes
Users are unable to press update all button multiple times until add-ons finish downloading.

Description of development approach
Disable update all button immediately after first activation.

Testing strategy:
Manual testing by simulating slow internet updating multiple add-ons
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add-on store: present updatable add-ons or add-on update notification at NVDA startup