-
Notifications
You must be signed in to change notification settings - Fork 8
Metadata 4.7 updates for Fabrica #939
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
723eb27
77dacce
bd0ab3d
f801df0
5927bc4
f92ac73
15b8e76
b424d0c
69881b3
848e398
71299a1
0fd2ef1
0d7e20e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -58,6 +58,18 @@ const Validations = buildValidations({ | |
| }) | ||
| }) | ||
| ], | ||
| 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')) | ||
| ); | ||
| }) | ||
| }) | ||
| ], | ||
|
Comment on lines
+61
to
+72
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same concern: presence validation may be too strict for an optional field. Same issue as flagged in 🤖 Prompt for AI Agents |
||
| resourceTypeGeneral: [ | ||
| validator('presence', { | ||
| presence: true, | ||
|
|
@@ -86,6 +98,9 @@ export default class RelatedIdentifier extends Fragment.extend(Validations) { | |
| @attr('string', { defaultValue: null }) | ||
| relationType; | ||
|
|
||
| @attr('string', { defaultValue: null }) | ||
| relationTypeInformation; | ||
|
|
||
| @attr('string', { defaultValue: null }) | ||
| relatedMetadataScheme; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,18 @@ const Validations = buildValidations({ | |
| }) | ||
| }) | ||
| ], | ||
| 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')) | ||
| ); | ||
| }) | ||
| }) | ||
| ], | ||
|
Comment on lines
+36
to
+47
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 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 The template ( This creates an inconsistency: the UI signals the field is optional, but validation enforces it as required. Either add 🤖 Prompt for AI Agents |
||
| title: [ | ||
| validator('presence', { | ||
| presence: true, | ||
|
|
@@ -96,6 +108,9 @@ export default class RelatedItem extends Fragment.extend(Validations) { | |
| @attr('string', { defaultValue: null }) | ||
| relationType; | ||
|
|
||
| @attr('string', { defaultValue: null }) | ||
| relationTypeInformation; | ||
|
|
||
| @fragment('related-item-identifier', { | ||
| defaultValue: {} | ||
| }) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor accessibility gap:
<label for="relationTypeInformation">has no matchingidon the<input>.The
forattribute on the label won't associate with the input because the input lacksid="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
📝 Committable suggestion
🤖 Prompt for AI Agents