MBS-9063: Ignore dates in rels editor for rel types without them #3368
Conversation
root/static/scripts/relationship-editor/utility/prepareHtmlFormSubmission.js
Outdated
Show resolved
Hide resolved
Rather than going through the effort of removing and re-adding the dates as the link types change, this just ignores it for display while editing and only removes them from the data being sent once the edits are being submitted using a rel type that does not support dates.
This adds a fallback check that makes sure even if the JS layer tries to submit dates when editing relationship types that do not support them, they will be ignored.
reosarevok
force-pushed
the
MBS-9063-v2
branch
from
79c3b9e
to
52faf0d
Compare
reosarevok
force-pushed
the
MBS-9063-v2
branch
from
52faf0d
to
ed52479
Compare
| begin_date => { year => 1999, month => 1, day => 1 }, | ||
| end_date => { year => 2009, month => 9, day => 9 }, | ||
| ended => 1, |
There was a problem hiding this comment.
I was thinking it'd make sense for this to be an error--if dates are submitted where they aren't supported, then I'm not sure we should make any assumptions (e.g., the link type could be wrong instead). What do you think?
At the same time, I'd suggest we should be able to omit the begin_date, end_date, and ended properties entirely if dates aren't supported, so this should probably be two tests if the former is an error.
There was a problem hiding this comment.
Hmm. When are you thinking we would submit the wrong link type through this internal use API? I guess we could error, but it feels submitting the wrong link type would be a strange fuckup to make in our own code?
There was a problem hiding this comment.
I was more thinking about the user selecting the wrong one accidentally, and there being a bug in the code where we aren't clearing them from the JS side. But that would be rare and probably annoying for the user.
There was a problem hiding this comment.
I honestly feel in that case it would be still clearer if the change was made and the dates dropped, rather than giving an error about dates being passed that the user had nothing to do with since they either expected dates to be passed and why is it an error, or expected them to be dropped and how is it their fault.
Fix MBS-9063
Problem
We keep having issues where people will hit an ISE after changing from a relationship type with dates (such as member) to one without (such as founder), because the dates are still stored and submitted with the relationship even though it doesn't make sense.
It's even more confusing with the new editor because the relationship preview will still show the dates, even though the type does not support them, and even though the user has no way to actually change them anymore.
Solution
Since we do not want to lose the dates in a situation where the user ends up selecting a second type that does support them, I was originally stashing and blanking them if a type without dates is selected, and restoring them if a type with dates was selected again (#2873). @mwiencek suggested that rather than going through the effort of removing and re-adding the dates as the link types change, we could just ignore them for display while editing and only remove them from the data being sent once the edits are being submitted using a rel type that does not support dates. This implements that approach - submitted as a separate PR just in case we find problems in it and decide to go back to the previous approach, although it seems sensible to me.
The second commit implements a Perl level fallback that still fixes the dates being present in the case JS sends them through for whatever reason.
Testing
Tested both the JS and the Perl implementations (without the other being present, obviously) manually by changing from a writer relationship with dates to a previous attribution one, both in the work editor and the release editor (since they use different ways of submitting so the dates need to be stripped separately), and ensuring they are correctly submitted with no dates.
Added a basic for the Perl implementation.