Update Add-on Store mirror to use new UX #17228

SaschaCowley · 2024-09-30T02:56:29Z

Link to issue number:

Follow up to #17099

Summary of the issue:

In PR #17099, the ability to set a mirror to use when accessing the Add-on Store was added.
PR #17151 built upon this, adding the ability to set a mirror for NVDA update checks.
However, it was decided that we needed a more robust way of checking the mirror URLs, and a means of protecting users from accidental changes to the URL.
These UX improvements were implemented for the NVDA update mirror, but not for the Add-on Store server mirror, so as to keep PRs single-issue.

Description of user facing changes

The Add-on Store server mirror settings now use the same UI as the NVDA update check mirror.

Description of development approach

Added a responseValidator option to _SetURLDialog, which defaults to always returning True.
Added a validator for request.Response objects to check if they're a valid Add-on Store cache hash (JSON comprising a single string of 7-40 hex digits ).
Copied the implementation of NVDA update mirror to the Add-on Store page, using the cache hash validator as the responseValidator function.
Made failure messages when setting a URL more descriptive.
Updated user guide, including consolidating the description of the NVDA update check and Add-on Store server mirror dialogs into one.

Testing strategy:

Tested that using the new dialog sets the Add-on Store server mirror as expected.
Ran through the NVDA update mirror process and Add-on Store server mirror process, checking that context help works on all controls.

Known issues with pull request:

While not directly related to this issue, note that this work lays part of the groundwork for #17205.

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.

@coderabbitai summary

Added a new section to the settings section of the user guide describing the update mirror and add-on store mirror dialogs in one. Removed the specific section for the update mirror dialog. Changed the links that pointed to the add-on store dialog to refer to the new unified section. Changed references in source code to refer to the correct user guide sections.

seanbudd · 2024-09-30T03:09:29Z

source/gui/_SetURLDialog.py

+			message = _(
+				# Translators: Message shown to users when testing a given URL has failed because the response was invalid.
+				"The URL you have entered failed the connection test. The response received from the server was invalid. Check the URL is correct before trying again.",
+			)
+		elif isinstance(error, requests.HTTPError):
+			message = _(
+				# Translators: Message shown to users when testing a given URL has failed because the server returned an error.
+				"The URL you have entered failed the connection test. The server returned an error. Check that the URL is correct before trying again.",
+			)
+		elif isinstance(error, requests.ConnectionError):
+			message = _(
+				# Translators: Message shown to users when testing a given URL has failed because of a network error.
+				"The URL you have entered failed the connection test. There was a network error. Check that you are connected to the internet and try again.",
+			)
+		else:
+			message = _(
+				# Translators: Message shown to users when testing a given URL has failed for unknown reasons.
+				"The URL you have entered failed the connection test. Make sure you are connected to the internet and the URL is correct.",
+			)


I think perhaps this should be a datastructure such as an enum or dataclass.
I think it might also be good to translate the first sentence separately, and prepend it on.

seanbudd · 2024-09-30T03:10:39Z

source/gui/settingsDialogs.py

@@ -62,7 +63,6 @@
 	Set,
 	cast,
 )
-from url_normalize import url_normalize


can we remove this from requirements then?

seanbudd · 2024-09-30T03:11:05Z

source/gui/settingsDialogs.py

-		self.addonMetadataMirrorTextbox = sHelper.addLabeledControl(
-			self.addonMetadataMirrorLabelText,
-			wx.TextCtrl,
+		# Translators: The label for the server  mirror on the Add-on store Settings panel.


Suggested change

# Translators: The label for the server mirror on the Add-on store Settings panel.

# Translators: The label for the server mirror on the Add-on Store Settings panel.

seanbudd · 2024-09-30T03:11:56Z

source/gui/settingsDialogs.py

-			self.addonMetadataMirrorLabelText,
-			wx.TextCtrl,
+		# Translators: The label for the server  mirror on the Add-on store Settings panel.
+		mirrorBoxSizer = wx.StaticBoxSizer(wx.HORIZONTAL, self, label=_("Server mirror"))


Pedantic/subjective but maybe this is more accurate phrasing right? Making the subject the server/URL, not the mirror

Suggested change

mirrorBoxSizer = wx.StaticBoxSizer(wx.HORIZONTAL, self, label=_("Server mirror"))

mirrorBoxSizer = wx.StaticBoxSizer(wx.HORIZONTAL, self, label=_("Mirror server"))

seanbudd · 2024-09-30T03:18:16Z

source/gui/settingsDialogs.py

+	# Add-on Store cache hash is a git commit hash as a string.
+	return isinstance(data, str) and bool(re.fullmatch("[0-9a-fA-F]{7,40}", data))


ah I don't think this has to be true - we use a commit hash but mirrors may want to use their own cache hash, like a new UUID every X minutes to cache break, or whenever they generate new data.

I would just ensure there is a non-empty string

Suggested change

# Add-on Store cache hash is a git commit hash as a string.

return isinstance(data, str) and bool(re.fullmatch("[0-9a-fA-F]{7,40}", data))

# Add-on Store cache hash must be a non-empty JSON string

return isinstance(data, str) and bool(re.fullmatch(".+", data))

seanbudd · 2024-09-30T03:23:16Z

user_docs/en/userGuide.md

 This may be of use in locations where access to the NV Access Add-on Store server is slow or unavailable.

-Leave this blank to use the default NV Access Add-on Store server.
+The read-only text box shows the current mirror URL.
+If no mirror is in use (I.E. the NV Access NVDA Add-on Store server is being used), "No mirror" is displayed.


I think we lowercase i.e. more consistently, also maybe NVDA can be dropped

Suggested change

If no mirror is in use (I.E. the NV Access NVDA Add-on Store server is being used), "No mirror" is displayed.

If no mirror is in use (i.e. the NV Access Add-on Store server is being used), "No mirror" is displayed.

seanbudd · 2024-09-30T03:26:40Z

user_docs/en/userGuide.md

@@ -3131,6 +3115,28 @@ This can be very useful when you want to monitor constantly changing content, su
 The refresh takes place every one and a half seconds.
 This option is disabled by default.

+#### Set Mirror Dialog {#SetURLDialog}


I think this should go under "Advanced settings" or "misc settings" since all the other headers at this level are for settings panels

I disagree. Putting it in sub-paragraphs of the advanced settings has no sense because this dialog is not accessible from the advanced settings panel. Regarding "Misc", would it be a new paragraph to create?

There are already other level 4 headings which are not settings panels:

12.1.3. Select Synthesizer

12.1.4. Synth settings ring
12.1.6. Select Braille Display

Though, I acknowledge that Select synthesizer / braille display dialogs are dialogs that can be opened independently as a setting dialog whereas the Set URL dialog cannotn i.e. it is always a sub-dialog of a setting panel.

My proposal is to keep this at level 4, even if it's not a stand-alone settings dialog. But state explicitly in the introduction text of this paragraph that this type of dialog may be opened to set the mirror URL and the add-on store URL.

makes a lot of sense, this is quite similar to select synthesizer, settings ring and selecting a braille display. Though perhaps grouping those dialogs with this one separately might be worth doing

AppVeyorBot · 2024-09-30T03:32:58Z

PASS: Translation comments check.
PASS: License 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/83b9cqunyogbkfim/artifacts/output/nvda_snapshot_pr17228-34178,73915913.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 1.3,
INSTALL_END 0.9,
BUILD_START 0.0,
BUILD_END 14.5,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 18.2,
FINISH_END 0.1

See test results for failed build of commit 739159131a

SaschaCowley added 8 commits September 25, 2024 17:32

Added responseValidator option to _SetURLDialog

5443f63

Added validator for response object to check if it's a cache hash

651b4f0

Implemented new UX in addon store settings

889e69c

Removed some old references

fbdf96b

Made errors more descriptive

4e2d0d0

Updated user guide

56ce9ca

Merge branch 'master' into addonStoreMirrorUx

5eddff4

SaschaCowley requested review from a team as code owners September 30, 2024 02:56

SaschaCowley requested review from Qchristensen and seanbudd September 30, 2024 02:56

seanbudd reviewed Sep 30, 2024

View reviewed changes

seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Oct 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Add-on Store mirror to use new UX #17228

Update Add-on Store mirror to use new UX #17228

SaschaCowley commented Sep 30, 2024

seanbudd Sep 30, 2024

seanbudd Sep 30, 2024

seanbudd Sep 30, 2024

seanbudd Sep 30, 2024

seanbudd Sep 30, 2024

seanbudd Sep 30, 2024

seanbudd Sep 30, 2024

seanbudd Sep 30, 2024

CyrilleB79 Sep 30, 2024

seanbudd Sep 30, 2024

AppVeyorBot commented Sep 30, 2024

	# Translators: The label for the server mirror on the Add-on store Settings panel.
	# Translators: The label for the server mirror on the Add-on Store Settings panel.

	mirrorBoxSizer = wx.StaticBoxSizer(wx.HORIZONTAL, self, label=_("Server mirror"))
	mirrorBoxSizer = wx.StaticBoxSizer(wx.HORIZONTAL, self, label=_("Mirror server"))

		# Add-on Store cache hash is a git commit hash as a string.
		return isinstance(data, str) and bool(re.fullmatch("[0-9a-fA-F]{7,40}", data))

	If no mirror is in use (I.E. the NV Access NVDA Add-on Store server is being used), "No mirror" is displayed.
	If no mirror is in use (i.e. the NV Access Add-on Store server is being used), "No mirror" is displayed.

Update Add-on Store mirror to use new UX #17228

Are you sure you want to change the base?

Update Add-on Store mirror to use new UX #17228

Conversation

SaschaCowley commented Sep 30, 2024

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:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AppVeyorBot commented Sep 30, 2024