Conversation
…ld to models and appropriate forms.
…s not use the month-chart component and it always breaks on the new year, so eliminate the test for now.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Relation Type Information text field to related-identifier and related-item UIs and models; extends relation/identifier/resource type lists (Other, RAiD, SWHID, Poster, Presentation); adds presence validations; updates tests; adds a DataCite XML fixture; refactors Cypress DOI tests to use dynamic repository paths; skips one QUnit test. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/components/doi-related-item-identifier.js (1)
117-136:⚠️ Potential issue | 🟠 MajorPre-existing bug: inconsistent property name
relatedIdentifierTypevsrelatedItemIdentifierType.Lines 119, 124, and 134 set
relatedIdentifierTypeinstead ofrelatedItemIdentifierType(which is the correct property used everywhere else in this component, e.g., lines 79, 84, 89, 94, 99, 104, 109, 114, 129, 140). This means URN, ISBN, and RRID auto-detection silently sets the wrong property on the fragment.Not introduced by this PR, but worth fixing while you're in this file.
🐛 Proposed fix
case urn.test(value): this.fragment.set('relatedItemIdentifier', value); - this.fragment.set('relatedIdentifierType', 'URN'); + this.fragment.set('relatedItemIdentifierType', 'URN'); this.set('controlledIdentifierType', true); break; case isISBN(value): this.fragment.set('relatedItemIdentifier', value); - this.fragment.set('relatedIdentifierType', 'ISBN'); + this.fragment.set('relatedItemIdentifierType', 'ISBN'); this.set('controlledIdentifierType', true); break; case isURL(value): this.fragment.set('relatedItemIdentifier', value); this.fragment.set('relatedItemIdentifierType', 'URL'); this.set('controlledIdentifierType', true); break; case rrid.test(value): this.fragment.set('relatedItemIdentifier', value); - this.fragment.set('relatedIdentifierType', 'RRID'); + this.fragment.set('relatedItemIdentifierType', 'RRID'); this.set('controlledIdentifierType', true); break;
🤖 Fix all issues with AI agents
In `@app/components/doi-related-identifier.hbs`:
- Around line 40-50: The label's for="relationTypeInformation" doesn't match any
input id, so add id="relationTypeInformation" to the corresponding input element
(the <input> with class "form-control relation-type-information-field" that
binds value={{this.fragment.relationTypeInformation}} and uses oninput={{action
"updateRelationTypeInformationAction" value="target.value"}}) so the label
correctly associates and clicking the label focuses the input.
In `@app/models/related-identifier.js`:
- Around line 61-72: The presence validator on relationTypeInformation is too
strict for an optional field; remove the validator('presence', ...) for
relationTypeInformation (or replace it with a non-blocking/optional check) so
records with a relatedIdentifier but no relationTypeInformation can still be
published; if you need any validation, use a conditional validator that only
runs when the Metadata 4.7 spec requires the field (e.g., swap the presence
validator for a validator that allows blank values such as validator('length', {
allowBlank: true }) or gate the presence check with the existing computed
disabled condition on model.{relatedIdentifier,state}).
In `@app/models/related-item.js`:
- Around line 36-47: The presence validator on relationTypeInformation
(validator key relationTypeInformation in app/models/related-item.js) currently
sets presence: true when not draft and title is present, which conflicts with
the template that omits the "required-label" CSS; either make the UI match the
validation or relax the validator: (A) add "required-label" to the field label
in doi-related-item.hbs so the UI shows it as required, or (B) remove or
conditionalize presence: true in relationTypeInformation so the field is
optional per Metadata 4.7; apply the same change to the analogous presence
validator in related-identifier.js to keep behavior consistent.
In `@cypress/e2e/client_admin/doi.test.ts`:
- Line 37: The test uses it.only which limits execution to a single test and
skips the rest; remove the .only so the test reads it('is logged in to dois
page', ...) (or replace with it.skip if intentionally disabling) to restore full
suite execution — look for the occurrence of it.only in the doi.test.ts test
case titled "is logged in to dois page" and update it accordingly.
🧹 Nitpick comments (1)
cypress/e2e/client_admin/doi.test.ts (1)
339-341: Inconsistent mix of dynamicrepositoryPathand hardcodeddatacite.testpaths across tests.The first two tests use the new dynamic
repositoryPath, but several other tests still reference hardcoded/repositories/datacite.test/...paths (e.g., "creating a doi - FILE UPLOAD" at Line 340, "deleting a doi" at Line 384, and "can see dois..." at Line 494). Consider migrating these to userepositoryPathas well for consistency, or add a TODO if this is intentional incremental work.Also applies to: 384-384, 493-494
| <div> | ||
| <label for="relationTypeInformation" class="subtitle">Relation Type Information</label> | ||
| </div> | ||
| <div doi-relation-type-information data-test-relation-type-information-field> | ||
| <input type="text" class="form-control relation-type-information-field" value={{this.fragment.relationTypeInformation}} placeholder="Relation Type Information" | ||
| oninput={{action "updateRelationTypeInformationAction" value="target.value"}} data-test-relation-type-information /> | ||
| </div> | ||
| <div | ||
| class="help-block help-block-fragment relation-type-information-field"> | ||
| Text to support the Relation Type. | ||
| </div> |
There was a problem hiding this comment.
Minor accessibility gap: <label for="relationTypeInformation"> has no matching id on the <input>.
The for attribute on the label won't associate with the input because the input lacks id="relationTypeInformation". Clicking the label won't focus the input field, which is a minor accessibility gap. This is easily fixable for a plain <input> (unlike the power-select labels elsewhere in this file where the pattern is inherited).
🔧 Proposed fix
- <input type="text" class="form-control relation-type-information-field" value={{this.fragment.relationTypeInformation}} placeholder="Relation Type Information"
+ <input type="text" id="relationTypeInformation" class="form-control relation-type-information-field" value={{this.fragment.relationTypeInformation}} placeholder="Relation Type Information"
oninput={{action "updateRelationTypeInformationAction" value="target.value"}} data-test-relation-type-information />📝 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.
| <div> | |
| <label for="relationTypeInformation" class="subtitle">Relation Type Information</label> | |
| </div> | |
| <div doi-relation-type-information data-test-relation-type-information-field> | |
| <input type="text" class="form-control relation-type-information-field" value={{this.fragment.relationTypeInformation}} placeholder="Relation Type Information" | |
| oninput={{action "updateRelationTypeInformationAction" value="target.value"}} data-test-relation-type-information /> | |
| </div> | |
| <div | |
| class="help-block help-block-fragment relation-type-information-field"> | |
| Text to support the Relation Type. | |
| </div> | |
| <div> | |
| <label for="relationTypeInformation" class="subtitle">Relation Type Information</label> | |
| </div> | |
| <div doi-relation-type-information data-test-relation-type-information-field> | |
| <input type="text" id="relationTypeInformation" class="form-control relation-type-information-field" value={{this.fragment.relationTypeInformation}} placeholder="Relation Type Information" | |
| oninput={{action "updateRelationTypeInformationAction" value="target.value"}} data-test-relation-type-information /> | |
| </div> | |
| <div | |
| class="help-block help-block-fragment relation-type-information-field"> | |
| Text to support the Relation Type. | |
| </div> |
🤖 Prompt for AI Agents
In `@app/components/doi-related-identifier.hbs` around lines 40 - 50, The label's
for="relationTypeInformation" doesn't match any input id, so add
id="relationTypeInformation" to the corresponding input element (the <input>
with class "form-control relation-type-information-field" that binds
value={{this.fragment.relationTypeInformation}} and uses oninput={{action
"updateRelationTypeInformationAction" value="target.value"}}) so the label
correctly associates and clicking the label focuses the input.
| relationTypeInformation: [ | ||
| validator('presence', { | ||
| presence: true, | ||
| message: 'Please enter text to support the Relation Type.', | ||
| disabled: computed('model.{relatedIdentifier,state}', function () { | ||
| return ( | ||
| this.model.get('state') === 'draft' || | ||
| isBlank(this.model.get('relatedIdentifier')) | ||
| ); | ||
| }) | ||
| }) | ||
| ], |
There was a problem hiding this comment.
Same concern: presence validation may be too strict for an optional field.
Same issue as flagged in related-item.js — this presence validator will require relationTypeInformation for every related identifier that has a relatedIdentifier value and is not in draft state. If the Metadata 4.7 spec treats this as optional, this will block publishing for existing records that lack this field.
🤖 Prompt for AI Agents
In `@app/models/related-identifier.js` around lines 61 - 72, The presence
validator on relationTypeInformation is too strict for an optional field; remove
the validator('presence', ...) for relationTypeInformation (or replace it with a
non-blocking/optional check) so records with a relatedIdentifier but no
relationTypeInformation can still be published; if you need any validation, use
a conditional validator that only runs when the Metadata 4.7 spec requires the
field (e.g., swap the presence validator for a validator that allows blank
values such as validator('length', { allowBlank: true }) or gate the presence
check with the existing computed disabled condition on
model.{relatedIdentifier,state}).
| relationTypeInformation: [ | ||
| validator('presence', { | ||
| presence: true, | ||
| message: 'Please enter text to support the Relation Type.', | ||
| disabled: computed('model.{title,state}', function () { | ||
| return ( | ||
| this.model.get('state') === 'draft' || | ||
| isBlank(this.model.get('title')) | ||
| ); | ||
| }) | ||
| }) | ||
| ], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find the relevant files mentioned in the review
find . -type f -name "related-item.js" -o -name "doi-related-item.hbs" -o -name "related-identifier.js" 2>/dev/null | head -20Repository: datacite/bracco
Length of output: 194
🏁 Script executed:
#!/bin/bash
# Explore the structure of app/models
ls -la app/models/ 2>/dev/null | head -30Repository: datacite/bracco
Length of output: 1996
🏁 Script executed:
#!/bin/bash
# Search for relationTypeInformation across the codebase
rg "relationTypeInformation" -n --type=js --type=hbs -B2 -A2Repository: datacite/bracco
Length of output: 3434
🏁 Script executed:
#!/bin/bash
# Look at the full validation block for relationTypeInformation in both files
# Get lines 36-47 from related-item.js
sed -n '36,47p' app/models/related-item.jsRepository: datacite/bracco
Length of output: 406
🏁 Script executed:
#!/bin/bash
# Get the full validation block from related-identifier.js
sed -n '61,72p' app/models/related-identifier.jsRepository: datacite/bracco
Length of output: 430
🏁 Script executed:
#!/bin/bash
# Check what "required-label" is used for in templates
rg "required-label" --type=hbs -B2 -A2 | head -40Repository: datacite/bracco
Length of output: 3278
🏁 Script executed:
#!/bin/bash
# Look at other validators with presence to see the pattern
rg "validator\('presence'" -B2 -A10 --type=js app/models/related-item.jsRepository: datacite/bracco
Length of output: 1584
🏁 Script executed:
#!/bin/bash
# Check if relatedItemType and relationType have required-label in the template
rg -n "relatedItemType|relationType" app/components/doi-related-item.hbs -B1 -A1 | head -40Repository: datacite/bracco
Length of output: 1613
🏁 Script executed:
#!/bin/bash
# Check the Metadata 4.7 schema or any documentation about relationTypeInformation
find . -type f \( -name "*.md" -o -name "*.txt" \) -exec grep -l "relationTypeInformation\|RelationTypeInformation" {} \; 2>/dev/nullRepository: datacite/bracco
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Look for any schema definitions or constants that document field requirements
rg "relationTypeInformation" --type=ts --type=js -B3 -A3 | grep -i "schema\|required\|optional" | head -20Repository: datacite/bracco
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Check if there are any comments or documentation about this field being optional/required
rg -n "relation.*type.*information" -i --type=js -B5 -A5 app/models/related-item.jsRepository: datacite/bracco
Length of output: 659
🏁 Script executed:
#!/bin/bash
# Check if there are tests or comments that clarify the intended optionality
rg "relationTypeInformation" --type=js -B5 -A5 app/ | grep -i "test\|optional\|required" | head -20Repository: datacite/bracco
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Look at the broader context - check if there's a README or docs about the related-item structure
find . -type f \( -name "README*" -o -name "SCHEMA*" -o -name "*.md" \) | xargs grep -l "related.item\|relationTypeInformation" 2>/dev/null | head -5Repository: datacite/bracco
Length of output: 41
Presence validation inconsistency for relationTypeInformation.
The template (doi-related-item.hbs line 44) labels this field with class "subtitle" only, while required fields like relatedItemType and relationType use "required-label subtitle". However, the validator (lines 36-47) enforces presence: true, making this field mandatory whenever the state is not draft and title is not blank.
This creates an inconsistency: the UI signals the field is optional, but validation enforces it as required. Either add "required-label" to the template to match the mandatory validation, or remove the presence requirement if the field should be optional per the Metadata 4.7 schema. The same issue exists in related-identifier.js.
🤖 Prompt for AI Agents
In `@app/models/related-item.js` around lines 36 - 47, The presence validator on
relationTypeInformation (validator key relationTypeInformation in
app/models/related-item.js) currently sets presence: true when not draft and
title is present, which conflicts with the template that omits the
"required-label" CSS; either make the UI match the validation or relax the
validator: (A) add "required-label" to the field label in doi-related-item.hbs
so the UI shows it as required, or (B) remove or conditionalize presence: true
in relationTypeInformation so the field is optional per Metadata 4.7; apply the
same change to the analogous presence validator in related-identifier.js to keep
behavior consistent.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cypress/e2e/client_admin/doi.test.ts (2)
400-424:⚠️ Potential issue | 🟠 MajorHardcoded repository path in
target_uridefeats the dynamic-path refactoring.Line 407 still uses a hardcoded path
'/repositories/datacite.test/dois'while the rest of the file was refactored to use the dynamicrepositoryPath. This will break ifclient_admin_usernamechanges.🐛 Proposed fix
- const target_uri = '/repositories/datacite.test/dois'; + const target_uri = repositoryPath + '/dois';
342-356:⚠️ Potential issue | 🟡 MinorCommented-out tests with no tracking issue.
Two test blocks (lines 342-356 and 516-526) are disabled with block comments and a
// FIX IT!note. This silently reduces E2E coverage with no linked issue to ensure follow-up. Consider usingit.skip(...)instead so test runners report them as skipped, and link a GitHub issue to track the fix.
🧹 Nitpick comments (1)
cypress/e2e/client_admin/doi.test.ts (1)
16-18:repositoryTitleis hardcoded whilerepositoryName/repositoryPathare dynamic.The entire point of this refactor is to derive values from
Cypress.env('client_admin_username'), butrepositoryTitleremains a magic string. If the test environment changes (different repository), the title assertion on line 47 will silently break.Consider deriving or parameterizing this value similarly to the other repository constants, or at minimum add a comment explaining why it's safe to hardcode.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cypress/e2e/client_admin/doi.test.ts (1)
407-407:⚠️ Potential issue | 🟡 MinorHard-coded repository path inconsistent with the dynamic
repositoryPathrefactor.All other tests were updated to use the dynamic
repositoryPathconstant, but this line still has a hard-coded path. Ifclient_admin_usernamechanges, this assertion will break.Proposed fix
- const target_uri = '/repositories/datacite.test/dois'; + const target_uri = repositoryPath + '/dois';
🤖 Fix all issues with AI agents
In `@cypress/fixtures/doi_v4_7.xml`:
- Line 32: Update the stale "4.4" references in the doi_v4_7 XML fixture: change
the date tag attribute dateInformation="Updated with 4.4 properties" to "Updated
with 4.7 properties", update the URL path that contains
"kernel-4.4/example/datacite-example-full-v4.4.xml" to use
"kernel-4.7/...v4.7.xml", and update the description text that reads "v4.4
properties" to "v4.7 properties" so all in-file references (the <date> element's
dateInformation attribute, the kernel URL string, and the description string)
consistently reference 4.7.
🧹 Nitpick comments (1)
cypress/e2e/client_admin/doi.test.ts (1)
342-356: Preferit.skipover block comments for disabled tests.Block-commented tests are invisible in test reports. Using
it.skip(...)keeps them visible as "pending" in CI output, so they aren't forgotten. Same applies to the block at lines 517–526.Example
- // FIX IT! - /* - it('is editing a doi - FORM', () => { + // FIX IT! + it.skip('is editing a doi - FORM', () => { ... - }); - */ + });
| </contributor> | ||
| </contributors> | ||
| <dates> | ||
| <date dateType="Updated" dateInformation="Updated with 4.4 properties">2021-01-26</date> |
There was a problem hiding this comment.
Stale "4.4" references in a kernel-4.7 fixture.
This fixture targets schema 4.7 (per Line 2), but several text values still reference 4.4:
- Line 32:
dateInformation="Updated with 4.4 properties" - Line 37: URL path contains
kernel-4.4/example/datacite-example-full-v4.4.xml - Line 54: description text says
"v4.4 properties"
These should be updated to 4.7 for consistency. While they won't break tests, they will be confusing to future maintainers and could mask issues if tests assert on this text.
Proposed fix
- <date dateType="Updated" dateInformation="Updated with 4.4 properties">2021-01-26</date>
+ <date dateType="Updated" dateInformation="Updated with 4.7 properties">2021-01-26</date>- <alternateIdentifier alternateIdentifierType="URL">https://schema.datacite.org/meta/kernel-4.4/example/datacite-example-full-v4.4.xml</alternateIdentifier>
+ <alternateIdentifier alternateIdentifierType="URL">https://schema.datacite.org/meta/kernel-4.7/example/datacite-example-full-v4.7.xml</alternateIdentifier>- <description xml:lang="en-US" descriptionType="Abstract">XML example of all DataCite Metadata Schema v4.4 properties.</description>
+ <description xml:lang="en-US" descriptionType="Abstract">XML example of all DataCite Metadata Schema v4.7 properties.</description>Also applies to: 37-37, 54-54
🤖 Prompt for AI Agents
In `@cypress/fixtures/doi_v4_7.xml` at line 32, Update the stale "4.4" references
in the doi_v4_7 XML fixture: change the date tag attribute
dateInformation="Updated with 4.4 properties" to "Updated with 4.7 properties",
update the URL path that contains
"kernel-4.4/example/datacite-example-full-v4.4.xml" to use
"kernel-4.7/...v4.7.xml", and update the description text that reads "v4.4
properties" to "v4.7 properties" so all in-file references (the <date> element's
dateInformation attribute, the kernel URL string, and the description string)
consistently reference 4.7.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cypress/e2e/client_admin/doi.test.ts (1)
405-429:⚠️ Potential issue | 🟡 MinorHardcoded repository path not updated to use
repositoryPath.Line 412 still uses the hardcoded string
'/repositories/datacite.test/dois'fortarget_uri, while the rest of the file was refactored to use the dynamicrepositoryPathconstant. Ifclient_admin_usernameever differs fromdatacite.test, this assertion will fail.♻️ Proposed fix
- const target_uri = '/repositories/datacite.test/dois'; + const target_uri = repositoryPath + '/dois';
🧹 Nitpick comments (4)
cypress/e2e/client_admin/doi.test.ts (4)
39-42: Avoid hardcodedcy.wait(10000)— prefer waiting on a specific element or network request.A 10-second unconditional wait (also repeated on line 362) makes tests slow and brittle. If the page loads faster, time is wasted; if it loads slower, the test flakes. Prefer
cy.intercept+cy.wait('@alias')or asserting on a visible element that signals readiness.
344-358: Commented-out tests reduce coverage — useit.skipor remove with a tracking issue.Two test blocks are disabled via block comments (
/* */) with only a "FIX IT!" note (also at line 521–531). Block-commented tests are invisible to test reporters and easy to forget. If these are intentionally deferred, useit.skip(...)so they appear in test reports as skipped, and link a tracking issue.
16-18:repositoryTitleis hardcoded whilerepositoryNameis dynamic — potential mismatch.
repositoryNameis correctly derived fromCypress.env('client_admin_username'), butrepositoryTitleis hardcoded as"DataCite Test Repository". If the environment targets a different repository, the title assertion on line 48 will fail with no obvious reason. Consider also sourcing this from an env variable, or at minimum add a comment explaining the assumption.
89-342: The creation test is very long (~250 lines) with no intermediate save points — a single failure anywhere loses all diagnostic context.Consider breaking this into smaller focused tests (e.g., one per form section) using
beforeEachto navigate to the form, or at minimum addcy.log()markers at each section boundary to improve failure diagnostics. This is a non-blocking suggestion given the nature of E2E form tests.
svogt0511
temporarily deployed
to
vercel-bracco-preview
GitHub Actions
Inactive
1 participant
Purpose
Changes to lupo as required for the Metadata 4.7 release, described in the document at the link, below.
Metadata Schema Release 4.7 - Product Spec
closes: Add github issue that originated this PR
Approach
Open Questions and Pre-Merge TODOs
Learning
Types of changes
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Reviewer, please remember our guidelines:
Summary by CodeRabbit
New Features
Tests
Chores