feat: ROR: add support for ROR#6949
Conversation
* master: bug/medium: search query: fix helpers (#6948)
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds Research Organization Registry (ROR) association management at instance, team, and user scope levels. Three new database tables ( ChangesROR Association Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
|
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/ts/misc.ts (1)
235-240:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNumber One: this thrown validation error is not safely absorbed by async handlers.
Line 240 now throws, but several async UI actions call
collectForm()without localtry/catch, and the central click dispatcher insrc/ts/common.ts(Line 1567) does not await handler promises. Result: invalid input can surface as unhandled rejections instead of controlled user feedback.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ts/misc.ts` around lines 235 - 240, The validation error thrown in the collectForm() function when reportValidity() fails is not being safely handled by its callers. Wrap all async calls to collectForm() with try/catch blocks to catch the thrown Error from line 240, and update the central click dispatcher in src/ts/common.ts at line 1567 to await handler promises so that any rejected promises from validation errors are properly caught rather than surfacing as unhandled rejections. This ensures invalid input triggers controlled user feedback instead of unhandled promise rejections.src/ts/langs/fr_FR.ts (1)
97-121:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLocalize the new ROR copy and make
no-rorsscope-neutral.These locale files still ship English placeholders for the new ROR UI, so non-English users will see untranslated text.
no-rorsalso says “this team” even thoughRors.sveltereuses that key for instance, team, and user views.
src/ts/langs/fr_FR.ts#L97-L121: translate the new ROR keys and replaceno-rorswith a generic message.src/ts/langs/id_ID.ts#L97-L121: translate the new ROR keys and replaceno-rorswith a generic message.src/ts/langs/it_IT.ts#L97-L121: translate the new ROR keys and replaceno-rorswith a generic message.src/ts/langs/ja_JP.ts#L97-L121: translate the new ROR keys and replaceno-rorswith a generic message.src/ts/langs/ko_KR.ts#L97-L121: translate the new ROR keys and replaceno-rorswith a generic message.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ts/langs/fr_FR.ts` around lines 97 - 121, The ROR-related locale strings contain untranslated English text and the `no-rors` key uses team-specific language despite being reused across instance, team, and user views. In all five language files (src/ts/langs/fr_FR.ts lines 97-121, src/ts/langs/id_ID.ts lines 97-121, src/ts/langs/it_IT.ts lines 97-121, src/ts/langs/ja_JP.ts lines 97-121, and src/ts/langs/ko_KR.ts lines 97-121), translate the English placeholder strings for the keys ror-description, ror-description-team, ror-description-user, ror-input-label, and ror-input-title into the appropriate language for each file. Additionally, replace the `no-rors` value in each file with a scope-neutral generic message that does not mention "team" specifically, since this key is reused across instance-level, team-level, and user-level views in Rors.svelte.src/ts/langs/ca_ES.ts (1)
62-121:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize the new ROR copy across all locale bundles.
The new ROR strings were added in English everywhere, and
no-rorsis also too team-specific for a key thatRors.svelterenders on instance, team, and user pages. That leaks English copy into localized UIs and shows the wrong empty-state wording outside team screens.
src/ts/langs/ca_ES.ts#L62-L121: translate the new ROR block and replaceno-rorswith neutral copy.src/ts/langs/cs_CZ.ts#L62-L121: translate the new ROR block and replaceno-rorswith neutral copy.src/ts/langs/de_DE.ts#L62-L121: translate the new ROR block and replaceno-rorswith neutral copy.src/ts/langs/el_GR.ts#L62-L121: translate the new ROR block and replaceno-rorswith neutral copy.src/ts/langs/sl_SI.ts#L62-L121: translate the new ROR block and replaceno-rorswith neutral copy.src/ts/langs/uz_UZ.ts#L62-L121: translate the new ROR block and replaceno-rorswith neutral copy.src/ts/langs/zh_CN.ts#L62-L121: translate the new ROR block and replaceno-rorswith neutral copy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ts/langs/ca_ES.ts` around lines 62 - 121, Translate the new ROR-related strings to their respective languages and fix the team-specific wording in all locale files. In src/ts/langs/ca_ES.ts#L62-L121 (anchor), src/ts/langs/cs_CZ.ts#L62-L121, src/ts/langs/de_DE.ts#L62-L121, src/ts/langs/el_GR.ts#L62-L121, src/ts/langs/sl_SI.ts#L62-L121, src/ts/langs/uz_UZ.ts#L62-L121, and src/ts/langs/zh_CN.ts#L62-L121 (siblings): Translate the keys for "no-rors", "ror-description", "ror-description-team", "ror-description-user", "ror-input-label", and "ror-input-title" into each language. Replace the "no-rors" value with neutral wording instead of "No ROR associated with this team yet." so it works correctly on instance, team, and user pages without leaking English strings or team-specific language into localized UIs.
🧹 Nitpick comments (1)
tests/unit/Make/MakeElnTest.php (1)
53-56: 🏗️ Heavy liftCaptain, add behavior assertions for ROR export content, not just constructor wiring.
These tests currently confirm instantiation/basic output shape, but not that ROR affiliations are actually emitted.
tests/unit/Make/MakeElnTest.php#L53-L56: assert ELN JSON-LD includes expectedmemberOf/affiliation ROR identifiers.tests/unit/Make/MakePdfTest.php#L79-L87: assert generated PDF content/render payload includes affiliation block when RORs exist.tests/unit/Make/MakeMultiPdfTest.php#L47-L50: add at least one assertion that multi-export output carries ROR affiliation text for included entities.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/Make/MakeElnTest.php` around lines 53 - 56, The three test methods across multiple files currently only verify basic instantiation and output shape but do not assert that ROR affiliation data is actually included in the exported content. In tests/unit/Make/MakeElnTest.php (lines 53-56), the testGetElnExp method must assert that the ELN JSON-LD output contains expected memberOf or affiliation ROR identifiers after calling getStreamZip(). In tests/unit/Make/MakePdfTest.php (lines 79-87), add assertions to verify the generated PDF content or render payload includes an affiliation block when ROR data exists. In tests/unit/Make/MakeMultiPdfTest.php (lines 47-50), add at least one assertion that the multi-export output carries ROR affiliation text for the included entities. Each test should move beyond verifying object construction to actually inspect the exported data structures and confirm ROR affiliations are present and properly formatted.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Controllers/Apiv2Controller.php`:
- Around line 173-174: The assignment of $submodel on line 173 directly accesses
$req[4] without a guard, which can trigger an undefined-offset notice when the
route segment is missing (such as for a plain /api/v2/instance request). Apply a
null coalescing operator with an empty string default to $req[4], matching the
pattern already used for $subIdString on line 174, so that missing segments are
safely handled without throwing notices.
- Around line 177-180: The issue is that $subIdString is unconditionally coerced
to an integer on line 177, which converts non-numeric string identifiers (like
"04t0gwh46") to 0, breaking single-resource GET routing for string-based
identifiers like RORs. Instead of always casting to int, check if $subIdString
contains only numeric characters before converting; if it's non-numeric,
preserve it as a string identifier in $this->subId. Then ensure the downstream
getArray() method and routing logic properly check for both numeric $this->subId
and string identifiers to correctly route to readOne() for single-resource
requests instead of collection reads.
In `@src/Elabftw/i18n4Js.php`:
- Line 141: The message string for the `no-rors` key in the i18n4Js.php file
contains "this team" which is scope-specific and inaccurate when the same key is
reused in instance and user page contexts via Rors.svelte. Replace the
scope-specific text with a neutral message that works correctly across all three
contexts (instance, team, and user pages) without referencing any particular
scope.
In `@src/Make/MakeEln.php`:
- Around line 446-460: The getUserRors method is using
$this->users2Rors->readAll() which reads RORs for the requester (the bound user
of this instance), but it should read RORs for the $user parameter passed into
the method. Replace the $this->users2Rors->readAll() call with a method that
reads RORs specific to $user by using $user->getUserid(), similar to how
$UsersHelper is used to fetch team RORs. This ensures each author gets their own
correct affiliations in the export instead of inheriting the requester's
affiliations.
In `@src/Make/MakePdf.php`:
- Line 285: The getRors() method call at the location where 'rors' is assigned
has its arguments reversed. The method signature expects getRors(int $teamid,
int $userid) with team ID first, but the call currently passes userid first and
team second. Reverse the order of arguments in the getRors() call so that the
team value is passed first and the userid value is passed second to ensure
correct team and user lookups.
In `@src/Models/Teams.php`:
- Around line 323-325: The canWrite() method currently throws a RuntimeException
when the team id is null, but since this method is used as a boolean probe in
API submodel routing, the exception can bubble into a 500 error instead of a
controlled permission denial. Modify the canWrite() method to return false when
the team id is null instead of throwing the exception. Move the throwing
behavior into a separate method named canWriteOrExplode() that performs the same
check and throws the RuntimeException, ensuring that code that explicitly wants
to fail on null team id can call that method instead.
In `@src/ts/components/Rors.svelte`:
- Around line 44-60: The `translateRor` function has unhandled async operations
that can throw exceptions during the fetch call and JSON parsing, causing
unhandled rejections that bubble up to `loadOrganizationNames()` and
`loadRors()` when the ROR API is unavailable. Wrap the entire body of
`translateRor` (starting from the fetch call through the data processing) in a
try-catch block, and in the catch block return the original `ror` parameter as a
graceful fallback. This ensures network failures and JSON parsing errors are
contained at the source instead of propagating as unhandled errors.
In `@src/ts/langs/en_US.ts`:
- Line 107: The en_US locale file contains British English spelling for the
"organisation-name" key. Change "Organisation name" to "Organization name" to
use the correct American English spelling for US users in the en_US locale.
In `@src/ts/langs/nl_BE.ts`:
- Line 97: The `"no-rors"` translation string in the nl_BE locale file is
currently team-specific ("No ROR associated with this team yet.") but this key
is shared across multiple contexts including team, instance, and user endpoints,
making the message inaccurate in most use cases. Change the message to be
scope-neutral (for example, "No ROR associations yet.") that works across all
contexts, then regenerate all locale files to propagate this change
consistently.
In `@src/ts/langs/pl_PL.ts`:
- Line 12: Remove the trailing whitespace from the "add-user-error" translation
string in the Polish language file. The string currently ends with a space after
"użytkownika " which should be removed so it reads "użytkownika" without the
trailing space to prevent rendering issues in UI concatenations and tooltips.
In `@src/ts/misc.ts`:
- Around line 1036-1069: The mountRors function currently mounts all ROR
components eagerly by querying and processing all elements with the
data-svelte-component attribute in a single operation, which causes unnecessary
API loads on startup especially when many ROR components are inside collapsed
details elements. Instead of mounting all components immediately, refactor
mountRors to attach event listeners to parent details elements and mount the
corresponding ROR component lazily only when a details element is opened. This
will defer component initialization and API calls until the user actually
expands a details section, reducing startup load on large instances.
In `@tests/unit/models/Instance2RorsTest.php`:
- Around line 41-49: To ensure test cleanup is guaranteed even if assertions
fail, refactor each test to use a try/finally pattern. In
tests/unit/models/Instance2RorsTest.php#L41-L49 in method testAll(), wrap the
postAction() call and subsequent assertions in a try block with
$this->Instance2Rors->destroy() in the finally block. Apply the identical
try/finally cleanup pattern to tests/unit/models/Teams2RorsTest.php#L39-L48 and
tests/unit/models/Users2RorsTest.php#L45-L54, placing the destroy() call in the
finally block to guarantee cleanup occurs regardless of assertion success or
failure, preventing state leakage between tests.
---
Outside diff comments:
In `@src/ts/langs/ca_ES.ts`:
- Around line 62-121: Translate the new ROR-related strings to their respective
languages and fix the team-specific wording in all locale files. In
src/ts/langs/ca_ES.ts#L62-L121 (anchor), src/ts/langs/cs_CZ.ts#L62-L121,
src/ts/langs/de_DE.ts#L62-L121, src/ts/langs/el_GR.ts#L62-L121,
src/ts/langs/sl_SI.ts#L62-L121, src/ts/langs/uz_UZ.ts#L62-L121, and
src/ts/langs/zh_CN.ts#L62-L121 (siblings): Translate the keys for "no-rors",
"ror-description", "ror-description-team", "ror-description-user",
"ror-input-label", and "ror-input-title" into each language. Replace the
"no-rors" value with neutral wording instead of "No ROR associated with this
team yet." so it works correctly on instance, team, and user pages without
leaking English strings or team-specific language into localized UIs.
In `@src/ts/langs/fr_FR.ts`:
- Around line 97-121: The ROR-related locale strings contain untranslated
English text and the `no-rors` key uses team-specific language despite being
reused across instance, team, and user views. In all five language files
(src/ts/langs/fr_FR.ts lines 97-121, src/ts/langs/id_ID.ts lines 97-121,
src/ts/langs/it_IT.ts lines 97-121, src/ts/langs/ja_JP.ts lines 97-121, and
src/ts/langs/ko_KR.ts lines 97-121), translate the English placeholder strings
for the keys ror-description, ror-description-team, ror-description-user,
ror-input-label, and ror-input-title into the appropriate language for each
file. Additionally, replace the `no-rors` value in each file with a
scope-neutral generic message that does not mention "team" specifically, since
this key is reused across instance-level, team-level, and user-level views in
Rors.svelte.
In `@src/ts/misc.ts`:
- Around line 235-240: The validation error thrown in the collectForm() function
when reportValidity() fails is not being safely handled by its callers. Wrap all
async calls to collectForm() with try/catch blocks to catch the thrown Error
from line 240, and update the central click dispatcher in src/ts/common.ts at
line 1567 to await handler promises so that any rejected promises from
validation errors are properly caught rather than surfacing as unhandled
rejections. This ensures invalid input triggers controlled user feedback instead
of unhandled promise rejections.
---
Nitpick comments:
In `@tests/unit/Make/MakeElnTest.php`:
- Around line 53-56: The three test methods across multiple files currently only
verify basic instantiation and output shape but do not assert that ROR
affiliation data is actually included in the exported content. In
tests/unit/Make/MakeElnTest.php (lines 53-56), the testGetElnExp method must
assert that the ELN JSON-LD output contains expected memberOf or affiliation ROR
identifiers after calling getStreamZip(). In tests/unit/Make/MakePdfTest.php
(lines 79-87), add assertions to verify the generated PDF content or render
payload includes an affiliation block when ROR data exists. In
tests/unit/Make/MakeMultiPdfTest.php (lines 47-50), add at least one assertion
that the multi-export output carries ROR affiliation text for the included
entities. Each test should move beyond verifying object construction to actually
inspect the exported data structures and confirm ROR affiliations are present
and properly formatted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e9a7a988-e1e7-40bb-bb20-479243e56190
📒 Files selected for processing (66)
containers/elabimg/nginx/common.confsrc/Commands/ExportEln.phpsrc/Controllers/Apiv2Controller.phpsrc/Controllers/MakeController.phpsrc/Elabftw/SchemaVersionChecker.phpsrc/Elabftw/i18n4Js.phpsrc/Enums/Action.phpsrc/Enums/ApiSubModels.phpsrc/Make/AbstractMakeEln.phpsrc/Make/AbstractMakeTimestamp.phpsrc/Make/Exports.phpsrc/Make/MakeEln.phpsrc/Make/MakeElnHtml.phpsrc/Make/MakePdf.phpsrc/Make/MakeStreamZip.phpsrc/Make/MakeTeamEln.phpsrc/Models/Abstract2Rors.phpsrc/Models/Dspace.phpsrc/Models/Instance2Rors.phpsrc/Models/Teams.phpsrc/Models/Teams2Rors.phpsrc/Models/Users2Rors.phpsrc/Services/Check.phpsrc/sql/schema212-down.sqlsrc/sql/schema212.sqlsrc/sql/structure.sqlsrc/templates/admin.htmlsrc/templates/edit-user-modal.htmlsrc/templates/pdf.htmlsrc/templates/profile.htmlsrc/templates/sysconfig.htmlsrc/ts/common.tssrc/ts/components/Rors.sveltesrc/ts/langs/ca_ES.tssrc/ts/langs/cs_CZ.tssrc/ts/langs/de_DE.tssrc/ts/langs/el_GR.tssrc/ts/langs/en_GB.tssrc/ts/langs/en_US.tssrc/ts/langs/es_ES.tssrc/ts/langs/et_EE.tssrc/ts/langs/fi_FI.tssrc/ts/langs/fr_FR.tssrc/ts/langs/id_ID.tssrc/ts/langs/it_IT.tssrc/ts/langs/ja_JP.tssrc/ts/langs/ko_KR.tssrc/ts/langs/nl_BE.tssrc/ts/langs/pl_PL.tssrc/ts/langs/pt_BR.tssrc/ts/langs/pt_PT.tssrc/ts/langs/ru_RU.tssrc/ts/langs/sk_SK.tssrc/ts/langs/sl_SI.tssrc/ts/langs/uz_UZ.tssrc/ts/langs/zh_CN.tssrc/ts/langs/zh_TW.tssrc/ts/misc.tstests/cypress/integration/rors.cy.tstests/unit/Make/MakeElnTest.phptests/unit/Make/MakeMultiPdfTest.phptests/unit/Make/MakePdfTest.phptests/unit/models/Instance2RorsTest.phptests/unit/models/Teams2RorsTest.phptests/unit/models/Users2RorsTest.phpweb/admin.php
💤 Files with no reviewable changes (1)
- web/admin.php
| "only-a-sysadmin": "Only a Sysadmin can modify this.", | ||
| "owner": "Owner", | ||
| "ownership-transfer": "Your entry has been successfully transferred to the selected user.", | ||
| "organisation-name": "Organisation name", |
There was a problem hiding this comment.
Number One: use American spelling in en_US.
This locale should not ship “Organisation”; US users will see the wrong copy for their locale.
💡 Suggested edit
- "organisation-name": "Organisation name",
+ "organisation-name": "Organization name",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "organisation-name": "Organisation name", | |
| "organisation-name": "Organization name", |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ts/langs/en_US.ts` at line 107, The en_US locale file contains British
English spelling for the "organisation-name" key. Change "Organisation name" to
"Organization name" to use the correct American English spelling for US users in
the en_US locale.
| "add-task": "Add task", | ||
| "add-team": "Dodaj zespół", | ||
| "add-to-team": "Dodaj wybranych użytkowników do zespołu", | ||
| "add-user-error": "Użyj menu autowypełniania by dodać użytkownika ", |
There was a problem hiding this comment.
Trim trailing whitespace in localized error copy.
At Line 12, the translation ends with a trailing space ("…użytkownika "), which can render awkwardly in UI concatenations/tooltips. Please remove the trailing space.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ts/langs/pl_PL.ts` at line 12, Remove the trailing whitespace from the
"add-user-error" translation string in the Polish language file. The string
currently ends with a space after "użytkownika " which should be removed so it
reads "użytkownika" without the trailing space to prevent rendering issues in UI
concatenations and tooltips.
| public function testAll(): void | ||
| { | ||
| $rors = $this->Instance2Rors->readAll(); | ||
| $initialCount = count($rors); | ||
| $this->Instance2Rors->postAction(Action::Create, array()); | ||
| $this->assertEquals($initialCount + 1, count($this->Instance2Rors->readAll())); | ||
| $this->assertEquals($this->ror, $this->Instance2Rors->readOne()['ror']); | ||
| $this->Instance2Rors->destroy(); | ||
| $this->assertEquals($initialCount, count($this->Instance2Rors->readAll())); |
There was a problem hiding this comment.
Captain, secure deterministic test cleanup for ROR lifecycle cases.
If an assertion fails before destroy(), state can leak into subsequent tests and create flaky order-dependent failures.
tests/unit/models/Instance2RorsTest.php#L41-L49: wrap create/assert block intry { ... } finally { $this->Instance2Rors->destroy(); }.tests/unit/models/Teams2RorsTest.php#L39-L48: usetry/finallycleanup around the created ROR row.tests/unit/models/Users2RorsTest.php#L45-L54: apply the same guaranteed cleanup pattern.
📍 Affects 3 files
tests/unit/models/Instance2RorsTest.php#L41-L49(this comment)tests/unit/models/Teams2RorsTest.php#L39-L48tests/unit/models/Users2RorsTest.php#L45-L54
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/models/Instance2RorsTest.php` around lines 41 - 49, To ensure test
cleanup is guaranteed even if assertions fail, refactor each test to use a
try/finally pattern. In tests/unit/models/Instance2RorsTest.php#L41-L49 in
method testAll(), wrap the postAction() call and subsequent assertions in a try
block with $this->Instance2Rors->destroy() in the finally block. Apply the
identical try/finally cleanup pattern to
tests/unit/models/Teams2RorsTest.php#L39-L48 and
tests/unit/models/Users2RorsTest.php#L45-L54, placing the destroy() call in the
finally block to guarantee cleanup occurs regardless of assertion success or
failure, preventing state leakage between tests.
* master: feat: ROR: add support for ROR (#6949) bug/medium: search query: fix helpers (#6948) feat: add Archive action back: will archive user in all teams (#6934) bug/minor: tables: fix thead transparent (#6943) ux: ag-grid: increase default pagination (#6939) api: enable extended mode on search (#6927)
fix #5569
Pull Request Checklist
Pull Request Description
Please provide clear context for your proposed change:
What issue or need does this PR address?
Add support for ROR at instance, team and user levels.
How did you approach the solution?
Create three tables, expose new submodel endpoints for rors on
/instance/rors,/teams/current/rorsand/users/me/rors, use one svelte component on sysconfig, admin and profile pages.Add it in .eln exports too.
If applicable, include screenshots or examples (especially for UI changes).
Summary by CodeRabbit
Release Notes
New Features
UI Updates
Internationalization