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

VPN-6749: Fix shared addon ids #10065

Merged
merged 6 commits into from
Dec 2, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
110 changes: 110 additions & 0 deletions .github/l10n/check_l10n_issues.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,121 @@
import xml.etree.ElementTree as etree
import json
import os
import subprocess
import sys
import tempfile

script_folder = os.path.abspath(os.path.dirname(__file__))
vpn_root_folder = os.path.realpath(os.path.join(script_folder, os.pardir, os.pardir))

def fileContentsAsJSON(filepath):
try:
with open(filepath, 'r') as file:
content = json.load(file)

if not content:
print(f"No content found in: {filepath}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
print(f"No content found in: {filepath}")
sys.exit(f"No content found in: {filepath}")

You can exit directly with the message (across the file).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had it that way initially, but moved to print combined with sys.exit(1) to match the style in the rest of this file.

sys.exit(1)
return content
except FileNotFoundError:
print(f"File not found: {filepath}")
sys.exit(1)
except Exception as e:
print(f"An error occurred while loading {filepath}: {e}")
sys.exit(1)

# Check if it uses shared strings. If so, pull the value for “title”, “subtitle”, then look within "blocks"
def getSharedStringsInManifest(mainifest_contents):
shared_string_ids = []
# If it includes the key “message” and if within "message", there is a “usesSharedStrings” key that is true...
if "message" in manifest_contents and "usesSharedStrings" in manifest_contents["message"] and manifest_contents["message"]["usesSharedStrings"]:
flodolo marked this conversation as resolved.
Show resolved Hide resolved
#...collect all the string IDs
if "title" in manifest_contents["message"]:
shared_string_ids.append(manifest_contents["message"]["title"])
else:
print(f"No title found for: {addon}")
sys.exit(1)
if "subtitle" in manifest_contents["message"]:
# It is acceptable if there is no subtitle (but title is required)
shared_string_ids.append(manifest_contents["message"]["subtitle"])
if not "blocks" in manifest_contents["message"]:
print(f"No blocks found for: {addon}")
sys.exit(1)
for block in manifest_contents["message"]["blocks"]:
if not "content" in block:
print(f"No content found in {addon} for {block}")
sys.exit(1)
if isinstance(block["content"], str):
shared_string_ids.append(block["content"])
elif isinstance(block["content"], list):
# This is the contents of a bulleted or ordered list; go one layer deeper.
for list_item in block["content"]:
if isinstance(list_item["content"], str):
shared_string_ids.append(list_item["content"])
else:
print(f"No content found in {addon} for {list_item}")
sys.exit(1)
else:
print(f"Content found in {addon} was unknown type for {block}")
sys.exit(1)
return shared_string_ids

### 1. Check Addons' shared strings - all string IDs should be present in the translation file. ###
# Find list of all addon folders
addon_path = os.path.join(vpn_root_folder, "addons")
try:
addon_list = [item for item in os.listdir(addon_path) if os.path.isdir(os.path.join(addon_path, item))]
except FileNotFoundError:
print(f"Path not found: {addon_path}")
sys.exit(1)
except Exception as e:
print(f"An error occurred when finding all addons: {e}")
sys.exit(1)

if len(addon_list) == 0:
print(f"No themes found")
flodolo marked this conversation as resolved.
Show resolved Hide resolved
sys.exit(1)

shared_string_ids = []

# For each, open the manifest file and pull out the shared strings
for addon in addon_list:
manifest_path = os.path.join(addon_path, addon, "manifest.json")
manifest_contents = fileContentsAsJSON(manifest_path)
shared_string_ids = shared_string_ids + getSharedStringsInManifest(manifest_contents)

# Create temporary translation file
tmp_path = tempfile.mkdtemp()
shared_addon_strings = os.path.join(addon_path, "strings.yaml")
shared_addons_tmp_file = os.path.join(tmp_path, "strings.xliff")
generate_addon_python_file = os.path.join(vpn_root_folder, "scripts", "utils", "generate_shared_addon_xliff.py")
p = subprocess.run(['python', generate_addon_python_file, '-i', shared_addon_strings, '-o', shared_addons_tmp_file])

# Check that the shared strings are present in the translation file
if not os.path.isfile(shared_addons_tmp_file):
print(f"Unable to find {shared_addons_tmp_file}")
sys.exit(1)
try:
with open(shared_addons_tmp_file, 'r') as file:
shared_string_content = file.read()
if not shared_string_content:
print(f"No content found in: {shared_addons_tmp_file}")
sys.exit(1)
except FileNotFoundError:
print(f"File not found: {shared_addons_tmp_file}")
sys.exit(1)
except Exception as e:
print(f"An error occurred while loading {shared_addons_tmp_file}: {e}")
sys.exit(1)

for shared_string_id in shared_string_ids:
string_id_unit = "<trans-unit id=\"" + shared_string_id + "\">"
if string_id_unit not in shared_string_content:
print(f"{string_id_unit} not found in shared string file")
sys.exit(1)

### 2. Check all strings for overall issues. ###

# Paths are relative to the root folder
paths = [
"translations.ts",
Expand Down
11 changes: 6 additions & 5 deletions docs/Components/Addons/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -152,18 +152,19 @@ If you want to implement new add-ons, you need to follow these steps:

1. Create a manifest in a separate folder in the `addons` directory of the mozilla VPN repository.
2. Read and follow the documentation for the add-on type you want to implement.
3. Build the CMake project in the `addons` directory.
3. If new shared strings were added, process them so that strings are shown in the addon: `python ./scripts/utils/generate_shared_addon_xliff.py -i ./addons/strings.yaml -o ./3rdparty/i18n/en/addons/strings.xliff` (This step is not necessary, strictly speaking. This script is what will be run as new strings are ingested into l10n repo, and once the new strings have done a round trip to the l10n repo and then back to the client repo, the string will show up in the addon as expected. Manually running this script as part of the build process prevents the brand new addon from showing "vpn.newStringCategory.newString" until this round trip happens.)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: could you please split this line and keep the file under 100-ish characters wide?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, done

4. Build the CMake project in the `addons` directory.

```
mkdir -p build-addons/
cmake -S <source>/addons -B build-addons -GNinja
cmake --build build-addons
```

4. Expose the generated build directory through a webservice. For example: `python3 -m http.server --directory build-addons/`
5. Open the dev-menu from the get-help view and set a custom add-on URL: `http://localhost:8000/`
6. Scroll down and disable the signature-addon feature from the dev-menu, list of features
7. Be sure you are doing all of this using a staging environment
5. Expose the generated build directory through a webservice. For example: `python3 -m http.server --directory build-addons/`
6. Open the dev-menu from the get-help view and set a custom add-on URL: `http://localhost:8000/`
7. Scroll down and disable the signature-addon feature from the dev-menu, list of features
8. Be sure you are doing all of this using a staging environment

If all has done correctly, you can see the app fetching the manifest.json (and
not! the manifest.json.sig) resource from the webservice.
12 changes: 12 additions & 0 deletions scripts/utils/generate_shared_addon_xliff.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,16 @@ def prune_lists_to_strings(data):
sys.exit("Unexpected input")
return data

def use_proper_id(data):
return_data = {}
for value in data.values():
string_id = value["string_id"]
return_data[string_id] = {
"value": value["value"],
"comments": value["comments"]
}
return return_data

# Make sure we have all the required things
# Lookup our required tools for addon generation.

Expand Down Expand Up @@ -59,6 +69,8 @@ def prune_lists_to_strings(data):
print("First, pull in the strings from the YAML file")
translation_strings = parseYAMLTranslationStrings(args.infile)
translation_strings = prune_lists_to_strings(translation_strings)
# parseYAMLTranslationStrings gives a different ID than we want to use
translation_strings = use_proper_id(translation_strings)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of regenerating, have you considered changing write_en_language(). It looks like a cleaner approach to me. This would be the diff compared to your current patch.

diff --git a/scripts/shared.py b/scripts/shared.py
index 46ea19700..5b6222561 100644
--- a/scripts/shared.py
+++ b/scripts/shared.py
@@ -2,7 +2,7 @@ import os
 import sys
 import xml.etree.ElementTree as ET
 
-def write_en_language(filename, strings):
+def write_en_language(filename, strings, key_as_id=True):
     ts = ET.Element("TS")
     ts.set("version", "2.1")
     ts.set("language", "en")
@@ -11,8 +11,9 @@ def write_en_language(filename, strings):
     ET.SubElement(context, "name")
 
     for key, value in strings.items():
+        id = key if key_as_id else value["string_id"]
         message = ET.SubElement(context, "message")
-        message.set("id", key)
+        message.set("id", id)
 
         location = ET.SubElement(message, "location")
         location.set("filename", "addon.qml")
diff --git a/scripts/utils/generate_shared_addon_xliff.py b/scripts/utils/generate_shared_addon_xliff.py
index 56efdc486..39b2f91e2 100644
--- a/scripts/utils/generate_shared_addon_xliff.py
+++ b/scripts/utils/generate_shared_addon_xliff.py
@@ -28,16 +28,6 @@ def prune_lists_to_strings(data):
             sys.exit("Unexpected input")
     return data
 
-def use_proper_id(data):
-    return_data = {}
-    for value in data.values():
-        string_id = value["string_id"]
-        return_data[string_id] = {
-            "value": value["value"],
-            "comments": value["comments"]
-        }
-    return return_data
-
 # Make sure we have all the required things
 # Lookup our required tools for addon generation.
 
@@ -69,13 +59,11 @@ print("Preparing the addons shared string file")
 print("First, pull in the strings from the YAML file")
 translation_strings = parseYAMLTranslationStrings(args.infile)
 translation_strings = prune_lists_to_strings(translation_strings)
-# parseYAMLTranslationStrings gives a different ID than we want to use
-translation_strings = use_proper_id(translation_strings)
 
 print("Then, write the strings to a .ts file")
 tmp_path = tempfile.mkdtemp()
 ts_file = os.path.join(tmp_path, "sharedAddonsStrings.ts")
-write_en_language(ts_file, translation_strings)
+write_en_language(ts_file, translation_strings, key_as_id=False)
 
 print("Finally, convert the ts file into an xliff file")
 os.system(f"{lconvert} -if ts -i {ts_file} -of xlf -o {args.outfile}")

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good call, thanks. I've updated this.


print("Then, write the strings to a .ts file")
tmp_path = tempfile.mkdtemp()
Expand Down
Loading