Unreliable save when changing order (drag and drop) of multiple entries per MD field in submission form

[Marwa1988-S]

Describe the bug
When changing the values order of a metadata field, or adding a new one and then reordering them, sometimes this change is not properly passed in the request, and when testing the newly changed metadata, it was found that the request does not contain the current correct order of metadata values and sometimes not all metadata values themselves if one was deleted after reordering them.

To Reproduce
Steps to reproduce the behavior:
1. Login
2. Navigate to the Workflow tasks, claim an item then edit
3. In edit-submission form add some new keywords
4. Change the keywords order, then delete one or some of them, then save

Expected behavior
The new keywords with their new order should be correct saved but aren't.

Here is an example:

• There are already these 3 keywords:

• Then add new one (kw1), drag it to be the first, and delete the second. then save:
  

• It is expected that the new keywords are properly saved and displayed, but the result was so in item view page:
  

Angular is firing off an ADD patch and when testing the value in the patch operation where the changed metadata is located, it found that it doesn't always come with the correct metadata.

[tdonohue]

@Marwa1988-S : Thanks for reporting this. While your example is showing a custom metadata field (that language-based "Keywords" field doesn't exist by default), I can reproduce this with the default "Author" field for an Item:

1. Submit a normal Item with 3 authors (Author 1, Author 2, Author 3) to a Collection with workflow enabled
2. As a reviewer, go in and edit that Workflow Item.
3. Add "Author 4". Drag it to the top of the list.
4. Remove "Author 1".
5. The ordering should now be "Author 4, Author 2, Author 3". However, if you save and reload (or click "Save for Later" and go back in to "Edit" again), you'll see the order is wrongly "Author 1, Author 3, Author 4".

My best guess here is that the drag and drop reordering of a new value is having some sort of issue. It appears that "Author 4" is added, but the drag & drop reorder doesn't fully trigger, which means when you remove the second author, you remove "Author 2" instead of "Author 1".

I'll pull this over to our 7.5 board and see if I can find someone to dig into this immediately.

[tdonohue]

Should be retested in 7.6 to verify problem still exists

[MW3000]

Yes, I just tested this. The problem is still the same in DSpace 7.6

[kshepherd]

I have added a longer comment to #3217, I believe it is a problem in the subscriptions to valueChanges for onebox components handling fields configured for controlled vocab / authority (even if the values are uncontrolled), and the fact that it does not allow for the group index to change.

[jlipka]

Due to the many different constellations, I'm not sure whether my approach will help here, but it's worth a try:

Please have a look at this file:
https://github.com/DSpace/DSpace/blob/8bc235b3c9a8bd02bf5eb5506b3d39e9cc43c746/dspace-server-webapp/src/main/java/org/dspace/app/rest/submit/factory/impl/ItemMetadataValueAddPatchOperation.java#L217

Line 217 should AFAIK look like this:
if (rs.getLeftItem().equals(dso)) {

This will set the "place" property correctly in the PATCH response and the entries should be displayed correctly in the frontend.

I would appreciate feedback - I may be able to contribute a merge request next week.

[saschaszott]

I would appreciate feedback - I may be able to contribute a merge request next week.

@jlipka , this is a valuable finding - I have found more places in the Java code base where the == operator is used to compare values (instead of object references). I will create a separate ticket on this 👉 DSpace/DSpace#9739

At least, the problem described in issue #3217 is not fixed (⚠️ in DSpace CRIS based on DS7) by replacing == with equals() in https://github.com/DSpace/DSpace/blob/8bc235b3c9a8bd02bf5eb5506b3d39e9cc43c746/dspace-server-webapp/src/main/java/org/dspace/app/rest/submit/factory/impl/ItemMetadataValueAddPatchOperation.java#L217

[saschaszott]

@jlipka , the same line of code can be found in ItemServiceImpl::moveSingleMetadataValue:

https://github.com/DSpace/DSpace/blob/8bc235b3c9a8bd02bf5eb5506b3d39e9cc43c746/dspace-api/src/main/java/org/dspace/content/ItemServiceImpl.java#L1802

[saschaszott]

The invalid usage of the == operator in DSpaceObjectServiceImpl was fixed a year ago:

https://github.com/DSpace/DSpace/blame/8bc235b3c9a8bd02bf5eb5506b3d39e9cc43c746/dspace-api/src/main/java/org/dspace/content/DSpaceObjectServiceImpl.java#L667

[kshepherd]

Line 217 should AFAIK look like this: if (rs.getLeftItem().equals(dso)) {
...
I would appreciate feedback - I may be able to contribute a merge request next week.

Hi @jlipka , thanks - yes, this is a good catch and we should definitely be using .equals() here. So I think a PR would be very welcome!

This change may solve any issues related to re-ordering/patch update for existing relationships displayed in a submission form, but #3217 was specifically about onebox metadata values, as far as I know (typically authority controlled fields), which would not have a relationship and so would not be impacted by this change.

[saschaszott]

@jlipka I've included the proposed bug fix (line 217 in ItemMetadataValueAddPatchOperation) in this PR 👉 DSpace/DSpace#9742

[pilasou]

HI @jlipka, hi @saschaszott, does the fix added in the already merged PR #9742 also fixed the present issue on multiple MD entries ordering? Thanks!

[jlipka]

@pilasou I am not able to reproduce the incorrect behavior described above (on demo.dspace.org), so things look good to me.

[pilasou]

Thanks @jlipka ! @tdonohue given that the incorrect behavior cannot be reproduced (i have also tried on the sandbox both on Subject Keywords and Authors fields: changing order and deleted and then save, the changes are kept), it seems that the PR DSpace/DSpace#9742 has solved this issue and it can be closed.

[tdonohue]

Closing, appears to have been fixed by DSpace/DSpace#9742 (and was already ported to 7.x for 7.6.3 and 8.x for 8.1)

Thanks for the testing @jlipka and @pilasou !