VPN-6473: check for OS in addons #9689
Conversation
| @@ -4,7 +4,8 @@ | |||
| "name": "Update to Mozilla VPN 2.23", | |||
| "type": "message", | |||
| "conditions": { | |||
| "max_client_version": "2.22.9" | |||
| "max_client_version": "2.22.9", | |||
| "javascript": "osCheck.js" | |||
| }, | |||
| "javascript": { | |||
| "enable": "enable.js" | |||
There was a problem hiding this comment.
Nice! Any reason to not just do this in enable.js where the other show/hide decision making happens?
There was a problem hiding this comment.
enable is run when the addon is enabled. We use it to modify the data in the addon based on conditions.
There is an addon condition type (conditions are used to decide whether to even show the addon) of javascript, which can enable arbitrary javascript as part of a test as to whether to show the addon. (Other conditions are client version, time, and a handful of others.) It seems more appropriate to use a condition for this, and I'm not sure it's even possible to use an enable js snippet to halt usage of the addon. If you know of a way, I'm happy to explore.
There was a problem hiding this comment.
Discussed in person, we're all comfortable w/ this. Going to merge.
Description
Checking OS via a JS condition.
I don't believe this adds any translations, but perhaps those strings did something weird? I'm not sure why @flodolo got auto-added to this PR.
This PR overrides #9686. However, I'm keeping that one open for now - I asked whether we want to modify that to add the condition in, so that we can use it in the future once enough users are on a version that include that condition.
Also, I might be doing over-defensive code with this check:
if (!osVersion || (typeof osVersion !== "string") || osVersion.length === 0). I don't write JS often; please let me know if there is a better way to do this.Reference
VPN-6473
Checklist