Make software field for create/update repository required.#949
Make software field for create/update repository required.#949
Conversation
|
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 presence validation for the repository Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/models/repository.js`:
- Around line 160-162: The repository factory is missing the required software
attribute, causing validations to fail when tests call make('repository') and
access model.validations; update the factory in tests/factories/repository.js to
include a software field that satisfies the model presence validator (e.g., add
a software property with a valid default or association), so instances created
by make('repository') pass validation when used by tests like
repository-form-errors-test.js.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9a699b80-8df7-48d2-a9a0-cae2176ec6f7
📒 Files selected for processing (4)
app/models/repository.jsapp/styles/local.cssapp/templates/providers/show/repositories/new.hbsapp/templates/repositories/show/edit.hbs
|
@svogt0511 Thanks Suzanne, is there a Vercel preview for this that I can can check? |
|
There will be later on this evening. I had to go out. Sorry about that.
Next time I will make sure there is a preview ready for you.
…On Wed, Mar 11, 2026 at 5:37 PM Kelly Stathis ***@***.***> wrote:
*KellyStathis* left a comment (datacite/bracco#949)
<#949 (comment)>
@svogt0511 <https://github.com/svogt0511> Thanks Suzanne, is there a
Vercel preview for this that I can can check?
—
Reply to this email directly, view it on GitHub
<#949 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABDW4WFIC3RPOOJLWXIAFSD4QHMCDAVCNFSM6AAAAACWOYRLVSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DANBSGM4DENBTGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
No problem at all! Just wanted to make sure I had everything for review. :) Thank you! |
svogt0511
temporarily deployed
to
vercel-bracco-preview
GitHub Actions
Inactive
app/styles/local.css
Outdated
| .required-label label.form-label:before { | ||
| color: #e74c3c; | ||
| content: "* " | ||
| } No newline at end of file |
There was a problem hiding this comment.
I think this is the only change to this file, despite the GitHub diff showing the entire file as changed. And I'm not sure this is working as intended:
When the software field is blank, would it be possible to have the error styling be consistent with elsewhere in the form? As in, red border and red error message.
Here's what I'm seeing for the Software field when I try to update an existing Repository and Software is blank:
vs. what it looks like if I leave System Email blank:
There was a problem hiding this comment.
I missed that there was a small red * in front of the "Software" label, which seems to be what this CSS is doing. Regardless, I think we should have consistent styling across all required fields!
There was a problem hiding this comment.
I am also seeing that the star is showing as red even when the field is populated, even though there is no error.
There was a problem hiding this comment.
@KellyStathis - I will fix the field styling to match what is shown in 'System Email'.
The small red star in front of the label is consistent with 'required' for a field. If you look at the doi form, that is where I took that styling form. So I think that we need to decide which way we will go. Red '*' or no red star for required fields across all forms.
The additional blank line was an oopsie. I had originally put an additional style in there and then found out I didn't need it. Too bad the linter doesn't catch that.
I will leave that up to you. Let me know.
There was a problem hiding this comment.
Ah, got it! Thanks—let me check this thoroughly across the different forms (DOI, Repository, Direct Member, Consortium Organization, Consortium Lead) and get back to you. We may already have some inconsistency between them.
There was a problem hiding this comment.
Per your comment in the iteration-a channel, I will remove the red star (on this form)!
There was a problem hiding this comment.
I think that's the best solution—that way, it is consistent within this form, even if there are differences across them. So to recap:
- remove star
- add field styling (red border)
- change "This field can't be blank" text to red
…before the label.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
app/controllers/providers/show/repositories/new.js (1)
25-27: Consider extracting shared software-field activation logic.This logic now exists in both new/edit controllers and can drift. A shared helper/mixin/service would keep behavior consistent.
Also applies to: 233-237
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/providers/show/repositories/new.js` around lines 25 - 27, The repeated software-field activation logic (setting isSoftwareFieldActive and mapping clientTypeList → clientTypes) should be extracted into a shared helper (e.g., a service or mixin) and consumed by both the new and edit controllers; create a function like determineSoftwareFieldState(clientTypeList) that returns { isSoftwareFieldActive, clientTypes } (or exposes a method to set those properties) and replace the inline assignments in the new controller (symbols: isSoftwareFieldActive, clientTypeList, clientTypes) and the corresponding code in the edit controller (lines ~233-237) to call the new helper to keep behavior consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/controllers/providers/show/repositories/new.js`:
- Line 25: Add a setupController hook on the route that resets the transient UI
flag so the controller singleton doesn't leak state: in the route that uses the
controller with the isSoftwareFieldActive property, implement
setupController(controller, model) { this._super(controller, model);
controller.set('isSoftwareFieldActive', false); } to ensure each visit clears
the flag so activateSoftwareField (the action that only sets it true) behaves
correctly; apply the same pattern for the edit-repository route/controller pair
that also uses activateSoftwareField/isSoftwareFieldActive.
In `@app/controllers/repositories/show/edit.js`:
- Line 32: Controller flag isSoftwareFieldActive persists across route
transitions and must be cleared; add a resetController hook on the corresponding
route that, when exiting, sets controller.isSoftwareFieldActive = false (e.g.,
in resetController(controller, isExiting) if (isExiting)
controller.set('isSoftwareFieldActive', false)) so the validation UI is not
stale; apply the same resetController change for the route backing
app/controllers/providers/show/repositories/new.js and ensure
activateSoftwareField() remains unchanged.
In `@app/styles/local.css`:
- Around line 143-150: The .form-group .form-text CSS rule contains a duplicate
font-size declaration; remove the redundant one so only a single font-size:
16px; remains in the .form-group .form-text block to eliminate the duplicate
property.
- Around line 227-229: Remove the unused CSS element selector "lock" from the
stylesheet: delete the rule block that begins with "lock { color: `#2ecc71`; }"
since no <lock> elements or .lock classes are used; if the intent was to style a
class, replace "lock" with ".lock" instead—otherwise simply remove the dead
rule.
- Around line 549-552: There is an unmatched comment terminator (`*/`) after the
CSS rule for the selector `.form-group input.is-valid + div.form-text` which
breaks stylesheet parsing; remove the stray `*/` (or, if that section was meant
to be a commented block, wrap the entire block in a proper /* ... */ comment) so
the rule is valid and the stylesheet parses correctly.
- Around line 292-295: Remove the legacy IE filter declaration from the .show
.dropdown-menu rule: delete the "filter: alpha(opacity = 100);" line in the CSS
block (the rule targeting ".show .dropdown-menu") since "opacity: 1" already
covers the intended behavior and the filter syntax is non-standard and flagged
by tooling.
---
Nitpick comments:
In `@app/controllers/providers/show/repositories/new.js`:
- Around line 25-27: The repeated software-field activation logic (setting
isSoftwareFieldActive and mapping clientTypeList → clientTypes) should be
extracted into a shared helper (e.g., a service or mixin) and consumed by both
the new and edit controllers; create a function like
determineSoftwareFieldState(clientTypeList) that returns {
isSoftwareFieldActive, clientTypes } (or exposes a method to set those
properties) and replace the inline assignments in the new controller (symbols:
isSoftwareFieldActive, clientTypeList, clientTypes) and the corresponding code
in the edit controller (lines ~233-237) to call the new helper to keep behavior
consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dc7b085e-c31b-44ce-9381-8e202f00d186
📒 Files selected for processing (5)
app/controllers/providers/show/repositories/new.jsapp/controllers/repositories/show/edit.jsapp/styles/local.cssapp/templates/providers/show/repositories/new.hbsapp/templates/repositories/show/edit.hbs
🚧 Files skipped from review as they are similar to previous changes (2)
- app/templates/repositories/show/edit.hbs
- app/templates/providers/show/repositories/new.hbs
svogt0511
temporarily deployed
to
vercel-bracco-preview
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/routes/repositories/show/edit.js`:
- Around line 44-50: There are two setupController definitions so the latter
overrides the former and drops initialization of clientTypeList/clientTypes;
merge the logic into a single setupController: call
super.setupController(controller, model) to preserve controller.set('model',
model), then run the earlier method's clientTypeList/clientTypes initialization
based on the model state and finally set isSoftwareFieldActive as done in the
second method; ensure you reference and update controller.set('clientTypeList',
...), controller.set('clientTypes', ...), and
controller.set('isSoftwareFieldActive', false) within that one merged
setupController.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e83097fe-5342-498d-8811-6970aedaa0f2
📒 Files selected for processing (3)
app/routes/providers/show/repositories/new.jsapp/routes/repositories/show/edit.jsapp/styles/local.css
svogt0511
temporarily deployed
to
vercel-bracco-preview
GitHub Actions
Inactive
3 participants
Purpose
closes: https://github.com/datacite/product-backlog/issues/646
preview: https://bracco-oldkpq0vl-datacite.vercel.app/
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
Style
Tests