WIP on customizing contributors step of deposit workflow for instruments #1641
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is a work in progress. At this point, I think that the workflow is working the way that we want it to, but I haven’t written unit tests for the new form objects yet because I’m not quite happy with the design, and I was hoping to improve it a bit first. There are also still some minor to-dos and potential refactorings to address.
|
@EricDurante No problem if you don't have time to finish this. We'll revisit the instruments stuff in the next ScholarSphere cycle. |
|
Thanks, @ajkiessl. I likely won't have time to finish, but I'm hoping that I can at least push up a little bit more work on it while it's still fresh in my mind. |
The inheritance hierarchy of these form classes is becoming a little more clear now that there are more of them, and the base class was collecting too much behavior that wasn’t needed by all of its subclasses.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I'd like to do more work on this if I have time, but I'm pushing up what I have currently in case I don't.
In terms of UI behavior, I think that this is correct/complete based on what we've discussed, and it should at least convey the gist of how I think that this should work. At the very least, it needs some additional unit tests for some of the new code as it's currently written. Those are missing because I'm not quite settled on the design of the new form objects that I've introduced.
There were a handful of failing tests in the base branch when I started, and those that were directly related to the new instruments pathway are fixed now, but there are a few tests that use VCR that are still failing for me. I'm not sure why they were failing to begin with since they don't seem related to this work, and I haven't looked into it.
In terms of the overall design, I'm also not yet quite happy with how we're handling data validations in general. Right now we have a mixture of validations on the form objects that validate data in the context of the deposit UI steps, and also validations on the ActiveRecord model that validate data in other contexts such as depositing works via the API. This has the effect of layering validations on top of other validations that are sometimes redundant, and it forces us to incorporate conditional logic in some of the validations. Some validations are common/shared, but they're all context-specific to some extent. In the long run, I think that it might be better to remove most or all of the validation logic from the
WorkVersionActiveRecord model and validate input from the API using another, separate form/model object that may share some validation logic with the existing UI forms. However, that's a larger refactoring that doesn't specifically have to do with this new deposit pathway.