-
Notifications
You must be signed in to change notification settings - Fork 438
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
#3678, #3217 - Fix ordering and changing values of repeatable inputs in submission form #3679
Conversation
@kshepherd : this PR claims to fix #3217, which it looks like was assigned to you (and you may have been working on?). Would you be interested in reviewing/testing this, since you've been looking into the same issue. (Perhaps there's a way to merge your work into this work, if we need to do so) |
Outside of the official review/approval process, I have also tested this behaviour and it looks good to me, although I have not been able to reproduce the behaviour described in #3217, but #3678 has been repeatedly reproducible and seems to be fixed with these code changes. UPDATE: Even if I recorded the "authors" form fields to show the console error, the same behaviour is reproducable for the "Type" field which was used in other screen recordings. Just add some items, re-order and remove them again. pr-3679-submission-form-bug-720p.mov |
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.
+1 this looks good and I prefer it to my fix (which may still be worthy of merging anyway if that sub is never needed)
Note, to fully get re-ordering a mix of plain values and relationships for e.g. Author, I also had to have the following PR applied: DSpace/DSpace#9744 (already merged to main and 8.x but not in 8.0) - it fixes an important left-right relationship comparison test.
Successfully created backport PR for |
Successfully created backport PR for |
References
Description
Wrapped the
control.get
method call in order to maintain a consistent link between Models and Controls.Instructions for Reviewers
Please, try to reproduce the bug described in #3678 following the same steps included in that issue.
Here is a video showcasing the bugfix.
reordering-fixed.mp4
List of changes in this PR:
control.get
method in a custom method that adds thestartingIndex
property to thegroupModel
and uses that value as the key to get the relativecontrol
. Since controls are never updated, but elements change their index when they are being reordered, this fixes issues regarding value changes and consequential reordering of items.formGroupName
as the index stored in thegroupModel
object. This way we can avoid using thengFor
index, since thegroupModel.index
property is being updated after reordering items.Checklist
main
branch of code (unless it is a backport or is fixing an issue specific to an older branch).npm run lint
npm run check-circ-deps
)My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.Tests are not available for theDynamicFormArray
component.package.json
), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.