-
Notifications
You must be signed in to change notification settings - Fork 7
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
Create instruments work type and pathway #1595
base: main
Are you sure you want to change the base?
Conversation
…t visible but instead automatically set to Scholarsphere
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.
Looks good to me! I'll let someone more familiar with this repo give the official approve but it looks like good test coverage and good organization.
openapi.yml
Outdated
example: "Classifying Independent Hybridity" | ||
funding_reference: | ||
type: "string" | ||
example: "Classifying Independent Hybridity" |
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.
Do we care about these examples? If so, should they all be the same?
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.
I believe we do, but I still need to touch base with Briana about what those values would actually be
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.
@ajkiessl do you know if we want to have examples here? Looking further, it looks like some have them & some don't
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.
I think these will be fine without examples. If anyone is using our API for uploading instruments, they'll probably know what these fields should be, or they'll at least be in close contact with us if they need to know.
end | ||
|
||
context 'when there are multiple creators' do | ||
let(:expected_citation) { 'Grant, Alan; Sattler, Ellie; Malcolm, Ian (2024). Citation Title [Data set]. Scholarsphere. https://doi.org/10.26207/123' } |
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.
There's uh, another example. See, here, now I'm sitting by myself, uh, uh, talking to myself. That's-that's chaos theory.
@@ -30,6 +30,7 @@ def update | |||
# WorkVersion#set_thumbnail_selection may be unreliable if the Shrine::ThumbnailJob is delayed | |||
@resource.set_thumbnail_selection | |||
@resource.indexing_source = Proc.new { nil } | |||
@resource.set_publisher_as_scholarsphere if deposit_pathway.instrument? |
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.
This logic could also potentially go in the model...a before_save
or even as part of the act of publishing.
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.
This is looking good. Most of my requests are pretty minor. There are some questions in there too.
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.
Just two things below, but everything else looks good!
available_date: :string, | ||
decommission_date: :string, |
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.
Sorry I didn't notice this before. We should run these dates through the date validation that published_date
runs through below:
validates :published_date,
presence: true,
edtf_date: true,
if: :published?
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.
In the EdtfDate module #valid?
required the date to be a valid and present to return true so an empty available and/or decommission date would throw an error. This feels redundant because presence: true
on published_date
already handles this. I modified it so the module just validates format, allowing for blank values, and let presence: true
handle that validation as needed.
I just noticed this part of the open_api.yml while working on the culminating experiences stuff: Lines 260 to 284 in 70a1606
We'll want instruments in here too. |
…ument. Involved changing edtf module to allow a date to be valid if it is in an allowed format or blank. Other validations are checking for presence on required dates.
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.
LGTM
Fixes #1586