feat(crowdin): allow source string edits and deletes

README.md

@@ -9,34 +9,58 @@ via `scripts/crowdin_sync.py`.
   `bible_loop.po`, ...).
 - `crowdin/crowdin.yml` — Crowdin CLI config for project 257.
 - `scripts/crowdin_sync.py` — CI entrypoint. Pulls current source strings
-  from Crowdin (default mode, via `scripts/update_strings.py`) or pushes new
-  local strings to Crowdin (`--push`). Runs on Bitrise; see the script's
+  from Crowdin (default mode, via `scripts/update_strings.py`) or pushes
+  local string changes to Crowdin (`--push`). Runs on Bitrise; see the script's
   docstring for the three modes.
 - `scripts/update_strings.py` — downloads a Crowdin bundle
   (`CROWDIN_BUNDLE_ID`) and installs its English `.po` files into
   `strings/en/`. Standalone/runnable on its own (`python3
   scripts/update_strings.py`) for local testing; requires a bundle scoped to
   these files with "include source language" enabled (there is no
   source-only download in crowdin-cli 4.12.0 — see the script's docstring).
-- `scripts/crowdin_validator.py` — CI gate that fails a PR if it edits or
-  deletes an existing string, or introduces a key that collides with one in
-  another file. Needs no Crowdin credentials.
+- `scripts/crowdin_validator.py` — CI gate on PRs touching `strings/en/*.po`.
+  Reports every added, edited, deleted and renamed string, and fails only on
+  a mistake: removing a whole `.po` file, a key that duplicates one in
+  another file, a key defined twice in the same file, an unparseable entry,
+  or an empty value. Needs no Crowdin credentials.
 
-## Adding a new source string
+## Changing source strings
 
-Existing strings can only be edited or deleted via the Crowdin UI. To add a
-brand-new one, edit the target `.po` file directly under `strings/en/`
-(`app.po`, `felt_needs.po`, etc.) and append a new entry in the same style as
-the rest of the file:
+Add, edit and delete English strings by editing the target `.po` file
+directly under `strings/en/` (`app.po`, `felt_needs.po`, etc.), in the same
+style as the rest of the file:
 
 ```
 msgid "new_key"
 msgstr "New English text"
 ```
 
 Then commit and open a normal PR. `scripts/crowdin_validator.py` runs in CI
-on that PR and fails if it edits or deletes an existing entry, or introduces
-a key that collides with one already in another file — this needs no
-Crowdin credentials. The next `scripts/crowdin_sync.py --push` run pushes the
-new entries to Crowdin as an unconditional first step, ahead of its normal
-pull.
+on that PR (no Crowdin credentials needed) and prints what changed. It fails
+the build only on a mistake — removing a whole `.po` file, a key that
+duplicates one already in another file, the same key defined twice in one
+file, an entry it cannot parse, or an entry with an empty value. The next
+`scripts/crowdin_sync.py --push` run propagates all of it to Crowdin.
+
+What each kind of change costs on the Crowdin side, since the validator
+reports but does not block them:
+
+| Change | Effect on existing translations |
+| --- | --- |
+| Add a `msgid` | None — the new string starts untranslated. |
+| Edit a `msgstr` | Kept, but marked unapproved for re-review (`update_option` in `crowdin/crowdin.yml`). |
+| Delete a `msgid` | Discarded along with the string. |
+| Rename a `msgid` | **Lost.** Crowdin matches by key, so this is a delete plus an untranslated add. |
+
+**The `.po` files themselves are fixed: never delete, rename or move one.**
+A catalog tracked on `main` has to still be there, whether it holds 600
+strings or none — deleting it, renaming it, moving it out of `strings/en/`,
+or emptying it in place all fail the build. Delete the individual `msgid`
+entries and keep the file.
+
+The reason is that `crowdin upload sources` can only ever *upload* a file.
+A deleted catalog survives in Crowdin and comes back, with all its strings,
+on the next pull. A renamed one is worse: the new name uploads as a new
+file while the old one stays, so the pull restores **both** and every string
+then exists twice. Retiring or renaming a catalog has to start in the
+Crowdin UI. The non-English translations are likewise Crowdin's alone.

crowdin/crowdin.yml

@@ -11,6 +11,12 @@
 # Pushes from here must land on these existing Crowdin files, or they'd
 # create duplicate strings instead of updating existing ones.
 #
+# `update_option` covers the case where a push changes the English text of an
+# existing string (these files carry `X-Crowdin-SourceKey: msgstr`, so editing
+# a msgstr *is* a source-text change). Crowdin's default is to treat the
+# changed string as new and discard its existing translations;
+# `update_as_unapproved` keeps them and flags them for re-review instead.
+#
 "project_id": "257"
 "base_path": ".."
 "base_url": "https://youversion.crowdin.com"
@@ -21,6 +27,7 @@
         "source": "strings/en/*.po",
         "dest": "Bible Loop (Master)/%original_file_name%",
         "translation": "Bible Loop (Master)/%two_letters_code%/%original_file_name%",
+        "update_option": "update_as_unapproved",
     },
 ]
 

scripts/crowdin_sync.py

@@ -15,22 +15,22 @@
       parsing details.
 
   --push
-      Pushes any new source strings added locally (a contributor hand-edits
-      the target `.po` file directly, no script needed) to Crowdin, via an
-      unconditional `crowdin upload sources`, then exits -- no pull, no
-      commit, no PR. Crowdin matches by identifier, so this is a no-op for
-      anything unchanged -- no local diffing needed. Does not require
-      `GH_TOKEN`.
+      Pushes locally-made source string changes -- additions, edits and
+      deletions alike (a contributor hand-edits the target `.po` file
+      directly, no script needed) -- to Crowdin, via an unconditional
+      `crowdin upload sources`, then exits: no pull, no commit, no PR.
+      Crowdin matches by identifier, so this is a no-op for anything
+      unchanged -- no local diffing needed. Does not require `GH_TOKEN`.
 
   --dry-run
       Preview the pull without switching branches, committing, pushing, or
       opening a PR. Combine with `--push` to preview just the push instead
       (`crowdin upload sources --dryrun`). Does not require `GH_TOKEN`.
 
-Crowdin is the single source of truth for existing strings; they're never
-hand-edited here. New strings are added by hand-editing the target `.po`
-file directly and are gated on PRs by `scripts/crowdin_validator.py`, but
-only this script ever writes to Crowdin.
+Source strings are added, edited and deleted by hand-editing the target
+`.po` file directly; `scripts/crowdin_validator.py` reports those deltas on
+the PR and blocks only outright mistakes. Crowdin stays the single source of
+truth for *translations*, and only this script ever writes to Crowdin.
 
 Bundle 13 -- the same bundle youversion-flutter-loop's own pull uses -- was
 confirmed via a live download to be scoped to exactly "Bible Loop
@@ -236,20 +236,64 @@ def changed_po_files():
         print("No parent commit (e.g. first commit); uploading all source files.")
         return None
 
+    parent_ref = parent.stdout.strip()
     diff = capture([
-        "git", "diff", "--name-only", "--diff-filter=ACMR",
-        parent.stdout.strip(), "HEAD", "--", "strings/en/*.po",
+        "git", "diff", "--name-only", "--no-renames", "--diff-filter=ACM",
+        parent_ref, "HEAD", "--", "strings/en/*.po",
     ])
+    warn_deleted_files(parent_ref)
     return [line for line in diff.splitlines() if line]
 
 
+def warn_deleted_files(parent_ref):
+    # A deleted .po can't be uploaded, so it falls outside the ACMR filter
+    # above and would otherwise vanish without a trace: the file stays in
+    # Crowdin and the next pull restores it, strings and all.
+    #
+    # crowdin_validator.py rejects this on the PR, so it should never reach
+    # here -- but the validator skips crowdin_sync.py's own branch and only
+    # runs on PRs, so this is the backstop for anything that got in another
+    # way. Warn loudly rather than failing the push of the files that did
+    # change; by this point the merge has already happened.
+    # --no-renames here too: without it a `git mv` of a catalog reports a
+    # single R entry, which this filter misses entirely -- so the old file
+    # would stay in Crowdin unmentioned while the new one is uploaded
+    # alongside it. As a delete plus an add, the removal is visible.
+    removed = capture([
+        "git", "diff", "--name-only", "--no-renames", "--diff-filter=D",
+        parent_ref, "HEAD", "--", "strings/en/*.po",
+    ])
+    for path in removed.splitlines():
+        if path:
+            name = Path(path).name
+            print(
+                f"WARNING: {path} was deleted locally but is NOT removed from "
+                f"Crowdin by this push. Delete \"{DEST_PATTERN.split('%')[0]}{name}\" "
+                "in the Crowdin UI, or it will come back -- with every string "
+                "it held -- on the next pull.",
+                flush=True,
+            )
+
+
 def push_new_strings(dry_run=False):
-    # Push any locally-added source strings (hand-edited into the target
+    # Push locally-made source string changes (hand-edited into the target
     # .po file directly; see scripts/crowdin_validator.py for the PR-time
-    # gate) to Crowdin. This is unconditional per-file: Crowdin matches by
+    # report) to Crowdin. This is unconditional per-file: Crowdin matches by
     # identifier and no-ops anything unchanged, so no per-string diffing is
     # needed here -- just per-file scoping (see changed_po_files() above).
-    # Never pass --delete-obsolete; this must stay additive/update-only.
+    #
+    # Additions, edits AND deletions all propagate from this one command, via
+    # the CLI's default --auto-update: Crowdin re-reads the uploaded source
+    # file and reconciles the whole file against it, so a key dropped locally
+    # goes obsolete in Crowdin too. crowdin.yml's `update_option` decides what
+    # an *edit* costs (see that file).
+    #
+    # Never pass --delete-obsolete. Despite the name it does not delete
+    # obsolete strings -- per `crowdin upload sources --help` (CLI 4.12.0) it
+    # deletes "obsolete files and folders ... that no longer match the source
+    # configuration", and since each invocation below is scoped with
+    # `-s <one file>`, the other 29 .po files in "Bible Loop (Master)/" would
+    # all look obsolete and be deleted.
     files = changed_po_files()
     if files == []:
         print("No strings/en/*.po changes vs. the previous commit; nothing to push.")