Skip to content

feat(i18n): Wrap bare strings and convert Dutch to English keys#1273

Open
rubenvdlinde wants to merge 16 commits intobetafrom
feature/i18n-complete-translations
Open

feat(i18n): Wrap bare strings and convert Dutch to English keys#1273
rubenvdlinde wants to merge 16 commits intobetafrom
feature/i18n-complete-translations

Conversation

@rubenvdlinde
Copy link
Copy Markdown
Contributor

Summary

  • Wrap all user-facing strings in t() / $this->l10n->t() translation calls
  • Ensure English is the primary language for all translation keys (per ADR-007)
  • Convert any hardcoded Dutch strings to English keys with Dutch in nl.json
  • Complete l10n/en.json (identity-mapped) and l10n/nl.json (Dutch translations)
  • Fix <script setup> components missing direct t import from @nextcloud/l10n

Test plan

  • Verify app loads without JavaScript errors
  • Switch Nextcloud language to Dutch and verify translations display correctly
  • Switch back to English and verify English strings display correctly
  • Spot-check key pages (dashboard, settings, detail views) in both languages

🤖 Generated with Claude Code

- Convert Dutch hardcoded strings to English t() keys in OrganisationsIndex, UploadFiles
- Wrap bare attributes and template strings across 20+ Vue files
- Wrap showSuccess/showError notifications in settings store
- Update l10n files with new keys and Dutch translations
@github-actions
Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ dcba3ad

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
composer ✅ 147/147
npm ✅ 599/599
PHPUnit
Newman
Playwright ⏭️

Quality workflow — 2026-04-16 17:30 UTC

Download the full PDF report from the workflow artifacts.

rubenvdlinde and others added 2 commits April 16, 2026 20:04
Standardize on #actions across all components per the updated
slot naming convention in @conduction/nextcloud-vue.
Update all translation keys to use sentence case (only first letter capitalized)
instead of title case. Keys changed:
- "Add Groups" → "Add groups"
- "Active Collections" → "Active collections"
- "API Key" → "API key"
- And 400+ similar keys across all apps

Sentence case for keys improves consistency and readability while preserving
proper English grammar in translated values.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ 79bf2f2

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
composer ✅ 147/147
npm ✅ 599/599
PHPUnit
Newman
Playwright ⏭️

Quality workflow — 2026-04-16 18:12 UTC

Download the full PDF report from the workflow artifacts.

@github-actions
Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ c1f49ce

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
composer ✅ 147/147
npm ✅ 599/599
PHPUnit ⏭️
Newman ⏭️
Playwright ⏭️

Quality workflow — 2026-04-16 18:15 UTC

Download the full PDF report from the workflow artifacts.

- Wrap bare strings in 20+ Vue files (modals, views, settings, workflow)
- Convert Dutch strings in OrganisationsIndex and UploadFiles to English
- Fix template attributes (label=, placeholder=, title=, aria-label=)
- Wrap showSuccess/showError notifications in t()
- Regenerate l10n files with 1,863 keys (sentence case)
@github-actions
Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ ee21b6c

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
composer ✅ 147/147
npm ✅ 599/599
PHPUnit ⏭️
Newman ⏭️
Playwright ⏭️

Quality workflow — 2026-04-16 22:09 UTC

Download the full PDF report from the workflow artifacts.

Base automatically changed from development to beta April 22, 2026 12:38
…-translations

# Conflicts:
#	lib/Db/MagicMapper/MagicSearchHandler.php
#	src/mail-sidebar/MailSidebar.vue
Comment thread l10n/en.json
@@ -1,42 +1,62 @@
{
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Minor — Key "1000" — a bare number — added as a translation key

The key "1000": "1000" was added to en.json (and identity-mapped in nl.json). This is used as a placeholder value: :placeholder="t('openregister', '1000')". Numeric placeholders are not translatable text and should not be in the translation file. They add noise and may produce translator confusion.

Suggested: Use a literal string: :placeholder="'1000'" or a computed property. Remove the key from en.json.

Comment thread l10n/en.json
@@ -1,42 +1,62 @@
{
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Minor — Sentence-split translation "Disable this to make the agent" is context-free and breaks word order

The string "Disable this to make the agent" is used as a fragment, with <strong>{{ t('openregister', 'public') }}</strong> and a trailing fragment inserted between. This pattern fails for languages (including Dutch) where adjectives or verb objects have different positions in the sentence. The translator of nl.json cannot produce a correct Dutch sentence from three disconnected fragments.

Suggested: Use a single key with an HTML-safe interpolation or split into two complete standalone sentences that each make sense alone.

Comment thread l10n/en.json
@@ -1,42 +1,62 @@
{
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Minor — Keys "updated", "created", "by", "public" added as standalone single-word translation keys

Standalone single words like updated, created, by, public are added as translation keys. These words have multiple meanings depending on context (e.g. by means different things in "created by", "by date", "sorted by"). Translators cannot produce correct context-sensitive translations without knowing the surrounding sentence.

Suggested: Replace sentence fragments with full sentences or use parameterised keys. E.g. instead of t('Application successfully') + t('updated'), use t('Application successfully updated').

Comment thread l10n/en.json
@@ -1,42 +1,62 @@
{
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Minor — Mixed key naming conventions: some keys are camelCase, some are sentence case, some are Title Case

The PR changes keys from Title Case to sentence case in some places (e.g. "Clear Filters""Clear filters") but not uniformly. Keys like "Active Organisation:", "API Token Configuration", "Add User to Organisation" remain Title Case while new keys like "Add endpoint", "Add range", "Active Organisation:" are mixed case. Nextcloud convention is sentence case for full sentences and Title Case for UI labels/headings.

Suggested: Establish a consistent key casing convention and apply it uniformly. ADR-007 or Nextcloud's i18n guide is the reference.

Copy link
Copy Markdown
Contributor

@WilcoLouwerse WilcoLouwerse left a comment

Choose a reason for hiding this comment

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

Strict-mode review — 9 blockers, 9 concerns, 4 minors.

Strict mode requires REQUEST_CHANGES on any 🔴 or 🟡. Please address blockers and concerns before merge.

@WilcoLouwerse
Copy link
Copy Markdown
Contributor

🔴 Blocker — PR targets beta instead of development

The base branch is beta, not development. ConductionNL convention requires all feature PRs to target development. The branch-protection / check-branch CI check is failing for exactly this reason. Merge to beta would bypass review-then-promote workflow and risk shipping unreviewed i18n regressions directly to a pre-release branch.

Suggested fix: Retarget the PR: close this one and open a new PR from the same head SHA against development.

@WilcoLouwerse
Copy link
Copy Markdown
Contributor

[composer.lock] ### 🔴 Blocker — CI: quality / Security (composer) is failing

The composer security audit CI job is red. This is a blocker regardless of the severity of the CVE — a failing security gate means a known-vulnerable dependency is present in the lockfile. The PR description does not mention any composer dependency changes, but the CI run is unambiguous. Investigate with composer audit and update or pin the affected package.

Suggested fix: Run composer audit locally, identify the flagged package, and update it or document a justified suppression in composer.jsonignore-platform-reqs / abandoned.

@WilcoLouwerse
Copy link
Copy Markdown
Contributor

🔴 Blocker — CI: quality / PHP Quality (phpcs) is failing

PHPCS is red on the PR. The diff includes new PHP in lib/Controller/TasksController.php, lib/Db/AuditTrailMapper.php, lib/Db/MagicMapper.php, lib/Db/MagicMapper/MagicSearchHandler.php, lib/Db/Organisation.php, lib/Listener/MailAppScriptListener.php, lib/Service/LinkedEntityService.php, and lib/Service/TaskService.php. At least one of these files contains a PHPCS violation. The PR title claims to be i18n-only but bundles significant functional PHP changes.

Suggested fix: Run composer cs-check locally and fix all violations before review.

@WilcoLouwerse
Copy link
Copy Markdown
Contributor

🔴 Blocker — CI: quality / PHP Quality (lint) is failing

PHP lint is red. This may be a syntax error introduced by the PHP changes bundled into this i18n PR. Syntax errors prevent the application from loading entirely.

Suggested fix: Run php -l on all changed PHP files and fix any parse errors.

@WilcoLouwerse
Copy link
Copy Markdown
Contributor

🔴 Blocker — CI: quality / Vue Quality (eslint) is failing

ESLint is red. The PR modifies 90+ Vue/JS files. At minimum one file has a lint violation. This could be an unused import, a missing binding colon on a prop, or a structural issue in the new <script setup> blocks added to files that previously used the Options API.

Suggested fix: Run npm run lint locally and fix all ESLint violations.

Comment thread l10n/nl.json
Comment thread l10n/en.json
Comment thread l10n/en.json
Comment thread l10n/en.json
Comment thread l10n/en.json
@@ -1,42 +1,62 @@
{
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Concern — Sentence capitalisation regressions: >15 multi-sentence keys have lowercase after period

A systematic capitalisation error was introduced during key normalisation. Sentences following a period within a longer key are lowercased, producing grammatically incorrect English strings that will be shown as-is to EN users and used as keys for other locales. Representative examples:

  • "Are you sure you want to cleanup old search trails? this will delete entries older than 30 days."
  • "Configure basic connection settings…. use the separate configset and collection management dialogs…"
  • "Additional feedback saved. thank you!"
  • "Are you sure you want to delete the selected audit trails? this action cannot be undone."
  • "Do you want to permanently delete all filtered audit trail entries? this action cannot be undone."
  • "Configure large language model (LLM) providers for ai-powered features…"
  • (10+ more)

This makes the en.json keys incorrect English, and since keys are identity-mapped in EN, users on the EN locale also see these errors.

Suggested fix: Restore proper sentence capitalisation: every sentence starting after a period should be capitalised. This likely requires fixing the key-extraction tooling rather than manually editing 15+ keys.

@@ -4,13 +4,13 @@ import { applicationStore, organisationStore, navigationStore } from '../../stor
</script>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Concern — Sentence split across t() calls breaks word-order for non-English locales

In EditApplication.vue the success message is built as:

t('openregister', 'Application successfully') + t('openregister', 'updated'|'created')

This pattern — splitting a sentence into fragments between t() calls — is incorrect for i18n. Dutch or German have different word order and the two fragments cannot be composed by simple concatenation. The key "Application successfully" in isolation is meaningless without context. The same anti-pattern appears in EditSchemaProperty.vue (Property successfully updated/added) though that one uses a conditional inside a single t() call, which is better.

Suggested fix: Use two complete, context-appropriate keys: t('openregister', 'Application successfully updated') and t('openregister', 'Application successfully created') — matching the pattern correctly used in EditAgent.vue.

@@ -8,7 +8,7 @@
<div class="info-box">
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Concern — URL http://openregister-presidio-analyzer:3000 wrapped in t() as a placeholder

The placeholder attribute was changed to t('openregister', 'http://openregister-presidio-analyzer:3000'), creating a translation key that is a raw internal URL. This URL should not be translatable — it is a configuration default, not user-facing text. This key now appears in en.json and nl.json with identity-mapped English values, polluting the translation files.

Suggested fix: Use the URL as a literal string: :placeholder="'http://openregister-presidio-analyzer:3000'" or a named constant. Do not wrap infrastructure defaults in t().

@@ -1,7 +1,11 @@
<script setup>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Concern — Three loading-message attributes remain unwrapped in t() after partial migration

The name and description props on <SettingsSection> were correctly wrapped in t(), but the loading-message attribute was missed in three files:

  • src/views/settings/sections/MultitenancyConfiguration.vue: loading-message="Loading multitenancy settings..."
  • src/views/settings/sections/SolrConfiguration.vue: loading-message="Loading search configuration..."
  • One additional settings section

These strings are visible to users while the section loads.

Suggested fix: Change all three to :loading-message="t('openregister', '...')" and add the keys to en.json.

@@ -1,27 +1,31 @@
<script setup>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Concern — Dynamic action column headers rendered raw — not translatable

In PermissionMatrix.vue, the table headers iterate v-for="action in actions" and render {{ action }} directly. If these action names (e.g. read, write, delete) are sourced from the API and meant for end-users, they need either translation calls or a mapping to localised labels. The surrounding static headers (Register / Schema, Public) were correctly wrapped in t() in this PR, making the raw {{ action }} inconsistency more visible.

Suggested fix: Determine if action values are user-facing display strings. If so, add a translation map and wrap them in t().

@@ -1,28 +1,64 @@
<template>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Concern — Hardcoded Register #{{ obj.register }} string not wrapped in t()

In the ObjectsTab component, the register label is rendered as:

<span v-if="obj.register" class="or-mail-object-card__register">Register #{{ obj.register }}</span>

The prefix Register # is a bare English string. This is new code added in this PR.

Suggested fix: Change to {{ t('openregister', 'Register #{id}', { id: obj.register }) }} and add the key to en.json.

@@ -78,6 +78,41 @@ public function __construct(
$this->objectService = $objectService;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Concern — Functional PHP changes bundled into i18n PR — 8 PHP files with non-i18n changes

This PR titled feat(i18n): Wrap bare strings and convert Dutch to English keys also includes:

  • New allUserTasks() endpoint in TasksController.php
  • New getProcessingActivities() and findByIdentifier() methods in AuditTrailMapper.php
  • SQL quoting fix for reserved keywords in MagicMapper.php and MagicSearchHandler.php
  • New linked-entity fields on Organisation.php entity
  • Logic change in MailAppScriptListener.php (instanceof check refactor)
  • LinkedEntityService.php RBAC bypass change
  • Extensive TaskService.php additions
  • New routes in appinfo/routes.php

Mixing functional changes with i18n in a 113-file PR makes the review surface much larger than necessary and obscures which failures are i18n-related vs functional.

Suggested fix: Extract functional PHP changes into a separate PR. Keep this PR to l10n/ and Vue/JS i18n wrapping only.

@WilcoLouwerse
Copy link
Copy Markdown
Contributor

[.phpunit.cache] 🟡 Concern — PHPUnit cache file committed to the repository

.phpunit.cache/test-results is a local developer cache file that records test failure states for incremental re-runs. It should be in .gitignore and not committed. It is modified in this diff (the defect list has changed). Committing this file causes unnecessary conflicts and diff noise for all contributors.

Suggested fix: Add .phpunit.cache/ to .gitignore and remove the file from tracking with git rm --cached .phpunit.cache/test-results.

@@ -1,21 +1,28 @@
<script setup>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Concern — step.status and step.role rendered raw in approval workflow — dynamic values not localised

After the i18n pass, ApprovalStepList.vue now wraps static strings like Approval Progress and Approve/Reject buttons in t(). However, the dynamic step.status (e.g. pending, approved, rejected) and step.role are still rendered raw:

<span :class="['status-badge', `status-${step.status}`]">{{ step.status }}</span>
<span class="step-role">{{ step.role }}</span>

These API-sourced strings are displayed to end-users as-is in English.

Suggested fix: If status and role are user-visible, add a translation map (e.g. statusLabels = { pending: t(...), approved: t(...) }) and render statusLabels[step.status].

Co-authored-by: Cursor <cursoragent@cursor.com>
Copy link
Copy Markdown
Contributor

@WilcoLouwerse WilcoLouwerse left a comment

Choose a reason for hiding this comment

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

Re-review of fix commit 27178135 (Standard mode)

Resolved since prior review (4 of 4 i18n blockers)

  • ✅ 8 truncated keys at apostrophes — fully fixed, no zombies remaining (e.g. Add Property to '{title}', Tools respect the agent's own role and organization boundaries.).
  • ✅ Removed embeddings-warning key restored.
  • plurals block populated — 4 plural forms in en.json with proper Dutch translations and pluralForm: nplurals=2; plural=(n != 1); declared.
  • nl.json identity-map mostly fixed: 410/602 prior Dutch translations preserved verbatim, 191 prior keys also removed from en.json (no runtime regression), 434 actual Dutch translations now. Many newly-added keys remain identity-mapped — acceptable as wrap-first/translate-later, but ADR-007 expects Dutch coverage to grow before next release.
  • ✅ Six 🟡 concerns also addressed in fix commit (sentence-split t(), URL in t(), three loading-message attrs, hardcoded Register #, raw step.role/status, raw action header).

Still open from prior review (UNADDRESSED)

  • 🔴 PR still targets beta instead of developmentbranch-protection / check-branch (the only required CI check) is failing exactly for this reason.
  • 🔴 quality / PHP Quality (lint) red.
  • 🔴 quality / PHP Quality (phpcs) red.
  • 🔴 quality / Vue Quality (eslint) red.
  • 🔴 quality / Security (composer) red — known-vulnerable dependency in composer.lock.
  • 🟡 Functional PHP changes still bundled (TasksController, AuditTrailMapper, TaskService, …) into an i18n-titled PR.
  • 🟡 36 keys with sentence-capitalisation regressions (lowercase letter after sentence-ending period).

New findings — mechanical gates against the diff scope (gates exit code: 4 failed)

  • 🔴 IDOR in new GET /api/tasks → TasksController::allUserTasks (gate-7 protected, ADR-005 Rule 3) — see inline comment.
  • 🔴 Inline <NcModal> in src/views/organisation/OrganisationsIndex.vue (gate-13 protected, ADR-004) — see inline comment.
  • 🔴 55 <NcSelect> without inputLabel / ariaLabelCombobox (gate-12 protected, ADR-004, WCAG 2.1 AA SC 1.3.1 + 4.1.2) — top offenders: EditConfiguration.vue (12), MigrationObject.vue, ViewObject.vue, EditEndpoint.vue, EditAgent.vue. Most appear pre-existing in beta but are in this PR's diff scope (ADR-020).
  • 🟡 lib/Service/LinkedEntityService.php missing @copyright PHPDoc tag (gate-1).
  • 🟡 Dynamic translation keys without catalog entries on ApprovalStepList.vue / PermissionMatrix.vue — see inline comment.

Formal verdict follows in a separate review.

Comment thread lib/Controller/TasksController.php
Comment thread src/views/organisation/OrganisationsIndex.vue Outdated
<span v-if="step.decidedBy" class="decided-by">by {{ step.decidedBy }}</span>
<span class="step-order">{{ t('openregister', 'Step') }} {{ step.stepOrder }}</span>
<span class="step-role">{{ t('openregister', step.role) }}</span>
<span :class="['status-badge', `status-${step.status}`]">{{ t('openregister', step.status) }}</span>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Concern — Dynamic translation keys with values not in the catalog

The new t('openregister', step.role) and t('openregister', step.status) here (and t('openregister', action) in PermissionMatrix.vue) are correct in principle, but the values they pass at runtime aren't all present as keys in l10n/en.json / l10n/nl.json. Audit of the new catalog:

  • step.status: pending, approved, rejected — all present + translated
  • step.status: Approved, Rejected, in_progress, completednot in catalog
  • step.role: fully dynamic — any role value not pre-registered will fall back to the raw English token, even in NL locale
  • action (PermissionMatrix): create/read/update/delete (lowercase + capitalised) — present + translated
  • action (PermissionMatrix): list, execute, view (lowercase) — not in catalog (View capitalised exists but its NL value is also View)

Static key-extraction tools also won't see dynamic args, so future role/status/action values added to the schema won't get auto-registered.

Suggested fix: either restrict the value space to a known set and register every variant in en.json/nl.json, or pipe values through a label-mapper (e.g. const ROLE_LABELS = { reviewer: t('openregister', 'Reviewer'), approver: t('openregister', 'Approver') }) so static extraction can see the strings.

Copy link
Copy Markdown
Contributor

@WilcoLouwerse WilcoLouwerse left a comment

Choose a reason for hiding this comment

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

8 🔴 blocker(s) require fixes before merge: 1 required CI check failing (branch-protection / check-branch — PR targets beta instead of development), 4 non-required CI failures (PHP lint, phpcs, Vue eslint, composer security), and 3 protected mechanical-gate violations from new findings — IDOR in allUserTasks, inline <NcModal> in OrganisationsIndex.vue, and 55 <NcSelect> without inputLabel (gate-12, full list in the COMMENT review body). The four prior i18n blockers (truncated apostrophe keys, removed embeddings key, empty plurals, nl.json identity-map) are all resolved cleanly — nice work on those. Two prior 🟡 concerns remain open (functional PHP changes still bundled into an i18n PR; 36 sentence-capitalisation regressions); one new 🟡 added — dynamic translation keys without catalog entries.

WilcoLouwerse and others added 5 commits May 7, 2026 16:09
Address review #3200778886: nl.json regression where keys whose casing was
normalised in this PR (e.g. "Clear Filters" -> "Clear filters") lost their
Dutch translation. Recover them via case-insensitive lookup against
origin/beta:l10n/nl.json.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…} object plurals

Address review #3200779796: the plurals catalog was missing entries for two
n() call sites: ObjectCard.vue ('{count} email') and RestoreMultiple.vue
('Successfully restored {count} object'). Without these, Dutch users would
see English plural forms regardless of locale.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…atalog

Address review #3200779536: the confirm() message was a single-line t()
call broken across two source lines (literal newline) with lowercase
sentence starts ('you', 'this', 'are'). The l10n catalog already contains
the correctly-cased key with '\n\n' escapes for the paragraph break, so
runtime lookup fell back to the raw broken-cased English. Rewrite the
t() call to a single-line string with proper '\n\n' so it matches the
catalog key.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… scope

Address review #3201893333 (IDOR concern): document in the docblock that
the endpoint is anchored to the session user via TaskService's use of
IUserSession::getUser()->getUID(); the `assignee` request parameter is a
free-text description filter, not an identity claim, and cannot be used
to read another user's tasks.

No code change: the underlying behaviour is already safe (calendars are
fetched for the current user's principal). This commit only makes the
authorization model explicit so future readers and gate checks can
verify the binding without re-deriving it from the service.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…al.vue

Address review #3201893357 (ADR-004 / hydra-gate-13 modal-isolation):
the "Switch Active Organisation" NcModal was defined inline inside
OrganisationsIndex.vue, coupling the dialog to the parent's lifecycle
and bloating the parent component.

- New: src/modals/organisation/SwitchOrganisationModal.vue
  - Props: show, organisations, activeOrganisationUuid
  - Emits: close, switch
  - Owns the modal markup and the .organisationSwitcher styles
- OrganisationsIndex.vue: replace inline <NcModal> with
  <SwitchOrganisationModal>, drop NcModal from imports/components,
  drop the orphaned styles. Behaviour is preserved: parent still owns
  showOrganisationSwitcher state and the switchToOrganisation handler.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@WilcoLouwerse
Copy link
Copy Markdown
Contributor

Re-review status check (HEAD 33503fa1)

Verified the 3 fix commits since my prior REQUEST_CHANGES land cleanly:

  • 174ebae3 — LLMConfigModal confirmClearEmbeddings() key recapitalised + \n\n properly escaped.
  • 3aa41592TasksController::allUserTasks docblock now documents auth via IUserSession; verified at lib/Service/TaskService.php:110-115 ($principal = 'principals/users/'.$user->getUID();). The assignee parameter is a description-text filter only — not an identity claim. Gate-7 IDOR is a docblock-resolved false positive.
  • 33503fa1SwitchOrganisationModal.vue extracted (109-line ADR-004-compliant component); OrganisationsIndex.vue -74/+9 with proper component import. Gate-13 (modal-isolation) now PASSES.

Plus the 48 case-only-renamed Dutch translations restored in nl.json — confirmed via diff.

I've resolved the 6 inline threads acknowledged in the 14:14Z replies. The prior REQUEST_CHANGES remains current — the 5 still-open blockers are all PR-level and not addressable by code review:

  • 🔴 Required CI: branch-protection / check-branch (because PR still targets beta instead of development)
  • 🔴 quality / PHP Quality (lint) red
  • 🔴 quality / PHP Quality (phpcs) red
  • 🔴 quality / Vue Quality (eslint) red
  • 🔴 quality / Security (composer) red

Re-target to development (or open a hotfix/* branch if this needs to land on beta directly) and fix the four quality jobs — happy to APPROVE once the suite is green.

Copy link
Copy Markdown
Contributor

@WilcoLouwerse WilcoLouwerse left a comment

Choose a reason for hiding this comment

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

Re-review at HEAD 33503fa1. All 4 prior i18n inline blockers + the 3 new findings I raised in the second pass (IDOR allUserTasks, inline <NcModal>, dynamic translation keys) are now resolved (verified — the 6 affected threads have been GraphQL-resolved; details in the status update). 5 🔴 remain — all PR-level: 1 required CI (branch-protection / check-branch because PR targets beta, not development) plus 4 non-required CI failures (PHP lint, phpcs, Vue eslint, composer security). Re-target the PR (or open a hotfix/* branch) and clear the four quality jobs — happy to APPROVE once CI is green.

WilcoLouwerse and others added 5 commits May 7, 2026 16:46
…nisation.php

PHP lint failed with `Cannot redeclare OCA\OpenRegister\Db\Organisation::$mail`:
the linked-entity properties (mail, contacts, notes, todos, calendar, talk,
deck) were declared twice — once with single-line `/** ... */` docblocks
that phpcs also rejected (14 errors), and again with proper multi-line
docblocks. Drop the first (duplicate, malformed) set; the second has the
correct documentation.

Resolves the PHP Quality (lint) check and 14 of the 21 phpcs errors in
this file.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Last remaining phpcs error after the Organisation.php duplicate-property
cleanup (commit 0ed1ad1). Auto-fixed by phpcbf.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Resolves the 57 eslint errors on the PR branch:

- 14 .vue files had duplicate \`@nextcloud/l10n\` imports (line-2
  \`<script setup>\` plus a redundant copy in the Options API \`<script>\`).
  Vue SFC merges both blocks into one module scope, so the second
  import is unnecessary AND triggers \`no-redeclare\` /
  \`import/no-duplicates\`. Drop the second import in each file.
- ApprovalStepList.vue: same pattern with axios + NcButton +
  generateUrl. Drop the duplicate Options API imports.
- ApiTokenConfiguration.vue, OrganisationConfiguration.vue: missing
  \`>\` to close the \`<NcSettingsSection>\` opening tag, which made
  the next-line comment / element parse as further attributes
  (5 errors and 6 errors respectively, all stemming from the missing
  closing bracket).
- EditWebhook.vue: \`t()\` placeholder string contained a literal
  newline (\`X-Custom-Header: value\\nauthorization: bearer token\`)
  spanning two source lines. Replace with an escaped \\n so the
  JS string is syntactically valid.

Files:
- src/components/workflow/ApprovalStepList.vue (drop dup imports)
- src/views/configuration/ConfigurationsIndex.vue
- src/views/organisation/OrganisationDetails.vue
- src/views/organisation/OrganisationsIndex.vue
- src/views/settings/sections/SolrConfiguration.vue
- src/views/settings/sections/ApiTokenConfiguration.vue (close tag)
- src/views/settings/sections/OrganisationConfiguration.vue (close tag)
- src/modals/configuration/{Import,Preview}Configuration.vue
- src/modals/deleted/RestoreMultiple.vue
- src/modals/file/UploadFiles.vue
- src/modals/object/MassDeleteObject.vue
- src/modals/organisation/{Edit,Join}Organisation.vue
- src/modals/register/ExportRegister.vue
- src/modals/settings/{DeleteCollectionModal,FacetConfigModal,FileWarmupModal}.vue
- src/modals/webhook/EditWebhook.vue (escape \\n in placeholder)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Resolves the composer security audit failure on the PR branch:

- GHSA-r7cg-qjjm-xhqq (high): unbounded recursion in parser causes
  stack overflow on crafted nested input. Fixed in 15.32.3.
- GHSA-fc86-6rv6-2jpm (high): quadratic validation cost in
  OverlappingFieldsCanBeMerged via inline fragments. Fixed in 15.32.2.

Composer constraint in composer.json (\`^15.0\`) already permits this
upgrade — only composer.lock changes. Verified with \`composer audit\`:
"No security vulnerability advisories found."

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Second-pass cleanup after the first CI run:

phpcs (3 files, 27 errors total — 21 auto-fixed, 6 manual):
- lib/Db/AuditTrailMapper.php (16 errors): mostly auto-fixable spacing/
  alignment in getProcessingActivities() / findByIdentifier(); plus 4 manual
  fixes — two inline-comment full-stops + two `!isset()` -> `isset(...) === false`
  per the no-bang-operator sniff.
- lib/Service/TaskService.php (10 errors): default-value spacing on 4 method
  parameters + missing `//end try`, all auto-fixed; one manual fix for the
  ternary `is_string($comp) ? ...` -> explicit `=== true` comparison.
- lib/Listener/MailAppScriptListener.php (1 error): convert `if (!($event
  instanceof ...))` to `=== false`.

eslint (4 files):
- .vue files where the previous duplicate-import cleanup left a double
  blank line between the @nextcloud/vue import block and the
  vue-material-design-icons imports. Collapse the runs of >2 \\n into
  exactly 2. Affects RestoreMultiple, MassDeleteObject, JoinOrganisation,
  ExportRegister.

Local verification: \`vendor/bin/phpcs --standard=phpcs.xml --warning-severity=0\`
reports 0 errors.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@WilcoLouwerse WilcoLouwerse left a comment

Choose a reason for hiding this comment

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

CI cleanup pushed — currently on commit 9358edfc0. The 4 quality checks you flagged are now ✅ green.

Check Status Resolving commits What was wrong
PHP Quality (lint) ✅ pass 0ed1ad154 lib/Db/Organisation.php declared $mail (and 6 other linked-entity properties) twice — once at lines 287–306 with single-line /** ... */ docblocks, once at lines 315–362 with proper docblocks. Dropped the first set. PHP fatal cleared.
PHP Quality (phpcs) ✅ pass 0ed1ad154 + ade7e7dea + 9358edfc0 First pass cleared 14 of 21 errors in Organisation.php via the duplicate-block removal, plus 1 alignment error in MagicSearchHandler.php:491 via phpcbf. CI then surfaced more (AuditTrailMapper.php, MailAppScriptListener.php, TaskService.php) — second pass auto-fixed 21 of those via phpcbf, plus 6 manual fixes (4× !isset()isset(...) === false, 2× inline-comment full-stops). Local vendor/bin/phpcs --standard=phpcs.xml --warning-severity=0 now reports 0 errors.
Vue Quality (eslint) ✅ pass cb52f6b1d + 9358edfc0 57 errors broke down as: (a) 14 .vue files with duplicate @nextcloud/l10n imports — a <script setup> line-2 import plus a redundant copy in the Options API <script>; (b) ApprovalStepList.vue with the same pattern for axios/NcButton/generateUrl; (c) ApiTokenConfiguration.vue and OrganisationConfiguration.vue missing the closing > on <NcSettingsSection ...>; (d) EditWebhook.vue:243 had a t() placeholder string spanning two source lines (unterminated string constant). Second-pass commit collapsed 4× double-blank lines that the import removal left behind.
Security (composer) ✅ pass 223618165 composer update webonyx/graphql-php lock-only — bumped v15.31.5 → v15.32.3, past both advisories (GHSA-r7cg-qjjm-xhqq, GHSA-fc86-6rv6-2jpm). Verified locally with composer audit: No security vulnerability advisories found.

branch-protection / check-branch stays 🔴 because the PR still targets beta (author's decision — flagging this back to you for the retarget call).

Two follow-ups outside the original 5 blockers:

  • Integration Tests (Newman) is now red, but appears unrelated to the i18n / quality cleanup (binary file fixtures missing on the runner: /var/www/html/custom_apps/op...). It was skipping on prior runs — happy to dig if you want it triaged here.
  • PHPUnit (PHP 8.3, NC stable32) and (PHP 8.4, NC stable32) were still pending at last check; will tail and follow up if either fails.

Copy link
Copy Markdown
Contributor

@WilcoLouwerse WilcoLouwerse left a comment

Choose a reason for hiding this comment

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

Re-review at HEAD 9358edfc. All four quality CI jobs from my prior review are now ✅ green (PHP lint/phpcs, Vue eslint, composer security) — verified the cleanup-commit window is purely style/lint with no functional changes. Composer security GHSA-r7cg-qjjm-xhqq + GHSA-fc86-6rv6-2jpm cleared by the graphql-php v15.32.3 bump. Code review is essentially complete.

One required check remains red: branch-protection / check-branch because the PR targets beta instead of development. ConductionNL's beta ruleset requires source branches 'development', 'main', or 'hotfix/*'. This is structural, not a code finding. Two options to clear it:

  1. Re-target to development (preferred for a feature PR) — gh pr edit 1273 --base development
  2. Rename the branch to hotfix/i18n-… if this needs to land on beta directly

Once retargeted and CI is green, ping for a fresh APPROVE — there are no other blockers from my side. (Newman integration tests are also red but appear to be a runner-fixture issue per your status comment, not blocking.)

Copy link
Copy Markdown
Contributor

@WilcoLouwerse WilcoLouwerse left a comment

Choose a reason for hiding this comment

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

Approving on substance. All four prior i18n inline blockers + the three findings I raised in subsequent passes (IDOR allUserTasks, inline <NcModal>, dynamic translation keys) are resolved (verified — see status update). All four quality CI jobs from my prior REQUEST_CHANGES are now ✅ green (PHP lint/phpcs, Vue eslint, composer security GHSA-r7cg-qjjm-xhqq + GHSA-fc86-6rv6-2jpm cleared by graphql-php v15.32.3 bump).

Caveats (not blocking, owner-acknowledged):

  • branch-protection / check-branch stays 🔴 because the PR targets beta. Per your call to land here directly — this required check will continue to block merge until either retargeted or bypassed by an admin/ruleset bypass.
  • Integration Tests (Newman) is red but appears unrelated (binary fixture missing on runner per your status comment).
  • PHPUnit (PHP 8.3 / 8.4 NC stable32) was still in_progress at approval time — heads-up if either turns red.
  • Mechanical gates 7/12 still surface results but are docblock-resolved false positives (gate-7) or mostly pre-existing-in-beta backlog (gate-12, 55 NcSelect across files). Worth filing as separate cleanup tickets.

🟡 Non-blocking concerns from prior passes that remain open: 36 sentence-capitalisation regressions (lowercase after period in keys), dynamic t() keys without full catalog coverage on PermissionMatrix/ApprovalStepList, lib/Service/LinkedEntityService.php missing @copyright. Happy to follow up on any of these as a separate PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants