Merge remote-tracking branch 'origin/main' into claude/add-tests-issue-1276-cTfRS

Author: JSv4

CHANGELOG.md

@@ -61,6 +61,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   - **Keyboard navigation in CAML extract picker**: Added a listbox keyboard handler to the "Insert Extract Grid" dropdown in `CamlArticleEditor` — Arrow keys / Home / End move the focused option, Enter selects, Escape closes and returns focus to the trigger button. Active option is reflected via `aria-activedescendant` and an `$active` highlight on `ExtractPickerItem`.
   - **`buildSourceLink` page-indexing convention documented**: Added an explanatory comment clarifying that `Annotation.page` is 1-based (model default=1) and is only used for the chip label (`p.{page}`) — the document viewer navigates by `annotationId` alone, so there is no URL-layer indexing convention to worry about (`frontend/src/components/extracts/ExtractGridEmbed.tsx`).
   - **Verified `GET_EXTRACTS` already selects `fullDocumentList`** (`frontend/src/graphql/queries.ts:1901`) and that `CamlDirectiveRenderer` still wires `resolveImageSrc` through to `MarkdownMessageRenderer` — both no-ops requested by the issue's verification items.
+- **`User.__init__` mutated the shared `Field.validators` list on every instantiation** (Issue #1358, `opencontractserver/users/models.py:150-153`): `User.__init__` overwrote `self._meta.get_field("username").validators[0]` with a fresh `UserUnicodeUsernameValidator()` on every `User(...)` construction — including every ORM hydration, form save, and QuerySet materialisation. Two problems: (1) per-instance allocation + assignment on a hot path, and (2) a hard-coded `validators[0]` index that would silently flip the slot back to Django's default `UnicodeUsernameValidator` if any third-party code or future migration prepended a validator, breaking usernames that rely on `\` or `|`. Fix: declare the validator on the `username` field at class-body time (`opencontractserver/users/models.py:78-90`) and drop the `__init__` override. Added migration `0026_alter_user_username_validator.py` to update the stored field definition. Regression tests in `opencontractserver/tests/test_user_username_validator.py` verify the validator survives repeated instantiation and continues to accept `\`, `|`, `*`.
 - **`RemoveLabelsFromLabelsetMutation` silently did nothing** (Issue #1359, `config/graphql/label_mutations.py:296`): The resolver referenced `labelset.documents.filter(pk__in=label_pks)`, but `LabelSet` has no `documents` relation — the M2M to labels is `annotation_labels` (see `opencontractserver/annotations/models.py:1284`). Every invocation therefore raised `AttributeError`, which was swallowed by the surrounding `except Exception as e:` block and returned as a generic `"Error removing label(s) from labelset: ..."` with `ok=False`. Because the frontend `REMOVE_ANNOTATION_LABELS_FROM_LABELSET` mutation was itself unused (#1244 swept it out), this bug went unnoticed in production for an unknown length of time; discovered while grading mypy errors for #1332 (`config/graphql/label_mutations.py:296: error: "LabelSet" has no attribute "documents"  [attr-defined]`). Swapped `documents` → `annotation_labels`, removed the now-resolved error from `docs/typing/mypy_baseline.txt`, and added `opencontractserver/tests/test_label_mutations.py` with four regression cases covering: labels are actually removed from the M2M, IDs not in the labelset are silently ignored, a non-owner / non-public caller cannot mutate the labelset, and a public labelset remains editable (pinning the current `Q(creator=user) | Q(is_public=True)` resolver behaviour so any future permission hardening is explicit).
 - **`package_annotated_docs` silently corrupted exports when a document failed to burn** (Issue #1356, `opencontractserver/tasks/export_tasks.py:150-212`, `opencontractserver/utils/etl.py:198-463`, `opencontractserver/tasks/doc_tasks.py:463-504`): `build_document_export()` returns `("", "", None, {}, {})` when a per-document export fails (e.g. the underlying file cannot be loaded). The V1 consumer `package_annotated_docs` had no guard for that placeholder — it ran `doc[1].encode("utf-8")` on the empty string (harmless, no crash), wrote an empty-named entry into the zip, and inserted `annotated_docs[""] = None` into the final `data.json`, so a single failed document silently poisoned the export. The V2 pipeline in `export_tasks_v2.py:126-128` already has this guard; V1 did not. Added `if not doc_name or doc_export is None: continue` (mirroring V2's check) and logged a warning identifying the skipped doc. Also tightened the return-type annotations on `build_document_export` and `burn_doc_annotations`: slots 0 and 1 are always `str` (never `None`; they are empty strings on the failure path), so the signature is now `tuple[str, str, OpenContractDocExport | None, dict[...], dict[...]]`. Corrected the `burned_docs` parameter annotation on `package_annotated_docs` from a single-element tuple-of-tuples to a variadic `tuple[tuple[...], ...]` — the runtime iteration is variadic, and the previous annotation was a red herring uncovered while typing for #1334. Regression test in `opencontractserver/tests/test_package_annotated_docs.py` covers both the mixed-success and all-failed scenarios, asserting the zip contains no empty-named entries and `annotated_docs` holds no `None` values.
 - **Frontend coverage badge stuck on "unknown"** (`README.md:12`, `.github/workflows/codecov-notify.yml`, `.github/workflows/frontend.yml`, `.github/workflows/frontend-e2e.yml`, `frontend/package.json`, `frontend/yarn.lock`): PR #1322 pointed the README badge at a new merged `frontend` flag fed by a cross-workflow `lcov-result-merger@5` step inside `codecov-notify.yml`. Every `frontend-merged-coverage` upload since has landed at Codecov with `state: error` / `totals: null`, so the badge rendered "unknown" even though `frontend-unit` (31%), `frontend-component` (61%), and `frontend-e2e` (24%) were all processing correctly. Two defects in the merged lcov confirmed by local repro of the CI merge step: (1) `lcov-result-merger@5` emits a stripped lcov containing only `SF:`, `DA:`, `BRDA:`, `end_of_record` — it drops `TN:`, `FN`, `FNDA`, `FNF`, `FNH`, `LF`, `LH`, `BRF`, `BRH`, so line-summary fields required by Codecov's parser are absent; (2) Vitest v8 emits `src/...` (relative to `frontend/`) while `vite-plugin-istanbul` + `nyc report` emit `/home/runner/work/OpenContracts/OpenContracts/frontend/src/...` (absolute), and the merger keys on the literal path string so the same file appears as two records with conflicting hit counts. `codecov-notify.yml` also ran without `actions/checkout`, which Codecov's action docs explicitly recommend against. Fix: stop merging client-side and let Codecov aggregate server-side, since that is what flags are for. Each per-suite upload now declares two flags — `frontend-unit,frontend`, `frontend-component,frontend`, `frontend-e2e,frontend` — so the `frontend` flag total is the union of the three uploads computed by Codecov. `codecov-notify.yml` is reduced to its original gate-and-notify role (no artifact downloads, no `lcov-result-merger`, no merged upload). Deleted the `frontend-{unit,ct,e2e}-lcov` artifact publishes in `frontend.yml` / `frontend-e2e.yml`, removed the `lcov-result-merger@^5.0.1` devDep and the `coverage:merge` script from `frontend/package.json`, and pruned the orphaned entries from `frontend/yarn.lock`. README badge URL unchanged (`flag=frontend`).

opencontractserver/tests/test_user_username_validator.py

@@ -0,0 +1,78 @@
+"""Regression tests for shared-state mutation in ``User.username`` validators."""
+
+from django.contrib.auth import get_user_model
+from django.test import TestCase
+
+from opencontractserver.users.validators import UserUnicodeUsernameValidator
+
+User = get_user_model()
+
+
+class UsernameValidatorRegressionTests(TestCase):
+    """Guardrails against shared-state mutation of ``User.username`` validators."""
+
+    def test_username_field_has_opencontracts_validator(self):
+        """The custom validator must be declared on the field, not patched in."""
+        field = User._meta.get_field("username")
+        self.assertTrue(
+            any(isinstance(v, UserUnicodeUsernameValidator) for v in field.validators),
+            "Expected UserUnicodeUsernameValidator to be declared on User.username",
+        )
+
+    def test_permissive_characters_accepted(self):
+        """Usernames containing ``\\``, ``|``, and ``*`` must pass validation."""
+        permissive_username = r"name\with|pipes*and-slash"
+        user = User(username=permissive_username, email="perm@example.com")
+        # full_clean() runs every validator on the field; should not raise.
+        user.full_clean(exclude=["password"])
+
+    def test_validators_list_stable_across_instantiations(self):
+        """Creating many ``User`` instances must not grow/shrink the validators list."""
+        field = User._meta.get_field("username")
+        baseline = list(field.validators)
+        baseline_len = len(baseline)
+
+        for i in range(100):
+            User(username=f"stable_user_{i}", email=f"stable{i}@example.com")
+
+        self.assertEqual(
+            len(field.validators),
+            baseline_len,
+            "User instantiation mutated the shared Field.validators list.",
+        )
+        # Identity of each validator should also be preserved — we should not be
+        # rebinding ``validators[0]`` on every ``User(...)`` call.
+        for before, after in zip(baseline, field.validators):
+            self.assertIs(
+                before,
+                after,
+                "A validator on User.username was replaced during instantiation.",
+            )
+
+    def test_third_party_prepend_does_not_corrupt_username_validator(self):
+        """A third-party validator prepended to ``Field.validators`` must survive ``User`` instantiation."""
+        from django.core.validators import RegexValidator
+
+        field = User._meta.get_field("username")
+        sentinel = RegexValidator(regex=r".*", message="sentinel")
+        field.validators.insert(0, sentinel)
+        try:
+            # Instantiate a few users — previously this would reassign
+            # ``validators[0]`` and overwrite the sentinel.
+            for i in range(5):
+                User(username=f"corrupt_check_{i}")
+
+            self.assertIs(
+                field.validators[0],
+                sentinel,
+                "User.__init__ should not mutate Field.validators",
+            )
+            self.assertTrue(
+                any(
+                    isinstance(v, UserUnicodeUsernameValidator)
+                    for v in field.validators
+                ),
+                "UserUnicodeUsernameValidator must remain in the validators list",
+            )
+        finally:
+            field.validators.remove(sentinel)

opencontractserver/users/migrations/0026_alter_user_username_validator.py

@@ -0,0 +1,31 @@
+from django.db import migrations, models
+
+import opencontractserver.users.validators
+
+
+class Migration(migrations.Migration):
+    """Declare ``UserUnicodeUsernameValidator`` on ``User.username`` at the model layer."""
+
+    dependencies = [
+        ("users", "0025_alter_userexport_format_add_v2"),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name="user",
+            name="username",
+            field=models.CharField(
+                error_messages={"unique": "A user with that username already exists."},
+                help_text=(
+                    "Required. 150 characters or fewer. Letters, digits and "
+                    "@/./+/-/_/|/*/\\ only."
+                ),
+                max_length=150,
+                unique=True,
+                validators=[
+                    opencontractserver.users.validators.UserUnicodeUsernameValidator()
+                ],
+                verbose_name="username",
+            ),
+        ),
+    ]

opencontractserver/users/models.py

@@ -70,6 +70,25 @@ def visible_to_user(self, user=None):
 class User(AbstractUser):
     """Default user for OpenContractServer."""
 
+    # Class attribute — referenced by Django admin forms (UserChangeForm,
+    # UserCreationForm) directly, separate from the field validators list.
+    username_validator = UserUnicodeUsernameValidator()
+
+    # Declared at class body to avoid mutating the shared Field.validators list on every User() call.
+    username = django.db.models.CharField(
+        _("username"),
+        max_length=150,
+        unique=True,
+        help_text=_(
+            "Required. 150 characters or fewer. Letters, digits and "
+            "@/./+/-/_/|/*/\\ only."
+        ),
+        validators=[UserUnicodeUsernameValidator()],
+        error_messages={
+            "unique": _("A user with that username already exists."),
+        },
+    )
+
     #: First and last name do not cover name patterns around the globe
     name = django.db.models.CharField(_("Name of User"), blank=True, max_length=255)
     first_name = django.db.models.CharField("First Name", blank=True, max_length=255)
@@ -147,11 +166,6 @@ class User(AbstractUser):
     def __str__(self):
         return f"{self.username}: {self.email}"
 
-    def __init__(self, *args, **kwargs):
-        self._meta.get_field("username").validators[0] = UserUnicodeUsernameValidator()
-
-        super().__init__(*args, **kwargs)
-
     def get_absolute_url(self):
         """Get url for user's detail view.