Repeatable fields with look up still broken in some cases

[alemarte]

References

• Fixes Repeatable fields with look up still broken in some cases #1091
• Requires Patch Add entire array with virtual values does not work DSpace#3224

Description

With a working REST implementation of the PATCH ADD entire array with virtual metadata, some minor bugs arose angular side.
This PR aims to solve them.

Instructions for Reviewers

Fixes:

1. If the first inserted value comes from a lookup, the form now fires an auto save and the form doesn't break.
2. If you remove a virtual value by clicking on the Trash button, the form now fires an auto save and the form doesn't break.
3. If there is only one virtual value, the Trash button is now disabled (as happens for a plain value).

Checklist

This checklist provides a reminder of what we are going to look for when reviewing your PR. You need not complete this checklist prior to creating your PR (draft PRs are always welcome). If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!

• My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
• My PR passes TSLint validation using yarn run lint
• My PR doesn't introduce circular dependencies
• [] My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
• My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
• If my PR includes new, third-party dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.

[artlowel]

Thanks @alemarte! The code looks good, and the issues reported in #1091 are fixed.

An issue I reported for #1025 is back however: if you enter a query in a mixed field (e.g. the author field in the Publications 2 collection), then do a lookup and select an entity. The relationship is saved, but your query is also saved as metadata

Also, disabling the trash can on the final field doesn't seem like the best approach to me.

For example, if you have a relationship as your only value and you want to replace it with something else, you have to add a new field first, only then can you remove the original. That's not very intuitive in my opinion.

It makes more sense to me to always allow the deletion of a field, and to show the initial empty field we show when you first arrive on the submission if all fields have been deleted.

[alemarte]

@artlowel thanks for your quick review!

An issue I reported for #1025 is back however: if you enter a query in a mixed field (e.g. the author field in the Publications 2 collection), then do a lookup and select an entity. The relationship is saved, but your query is also saved as metadata

I spent time with @atarix83 to investigate this.

We can reproduce this and you're right. This bug has been reintroduced after the fix of the other related issue (opening the modal and closing it without select anything makes the field empty). It's seems that it is almost impossible to fix both.

• if the field is not emptied on opening, it remains as plain value
• if the field is emptied on opening the content is lost

We can't make assumption on what happens inside the modal. The user could select many options, or none. To fix this we should get the user to choose only one option every time he opens the modal, and the selected option without doubts can then replace the originator plain value.

@tdonohue what could be the preferable option which cause the minor damage, waiting for the ideal solution which is planned to be implemented in upcoming releases?

Also, disabling the trash can on the final field doesn't seem like the best approach to me.

For example, if you have a relationship as your only value and you want to replace it with something else, you have to add a new field first, only then can you remove the original. That's not very intuitive in my opinion.

It makes more sense to me to always allow the deletion of a field, and to show the initial empty field we show when you first arrive on the submission if all fields have been deleted.

It makes sense, we have a solution for this and we'll implement on next monday.

[tdonohue]

@alemarte and @artlowel : I did some testing as well here.

I agree with @artlowel that the trash can should not be disabled when you have a relationship as the only value, because this means that last value cannot be modified/removed.

Regarding the lookup button behavior: For the purposes of this PR, if you enter a name and then click Lookup, I think the name should remain in the plain text field -- as there's no guarantee your lookup will match any values. I agree that this behavior is less than ideal, but I'd rather the value remain instead of being deleted/cleared when the modal window opens.

As a sidenote (not necessarily for this PR, but perhaps for 7.0 final), I'm starting the realize the behavior of the lookup popup window is not user friendly. The lookup window acts like it should only appear once per metadata field (in that all your current selections always appear in the popup & you can add multiple relationships at once from the popup). However, as we know, we changed it to appear once per metadata value (next to each value). The fact that it appears once per value seems to be causing these unexpected behavior issues, since it is able to modify more than that single value & is not actually "linked" back to that specific value. I'm now starting to wonder if we should change Lookup back to appearing once per field (i.e. next to the "+ Add more" link), as this would remove a number of these issues regarding how it behaves for individual values. If we agree this is a more appropriate fix to the issues posed in this PR, I'd be OK with changing the Lookup to only appear once per field (at least for now), whether in this PR or in a followup PR.

[atarix83]

I agree with @artlowel that the trash can should not be disabled when you have a relationship as the only value, because this means that last value cannot be modified/removed.

we are fixing it

I'm now starting to wonder if we should change Lookup back to appearing once per field (i.e. next to the "+ Add more" link), as this would remove a number of these issues regarding how it behaves for individual values. If we agree this is a more appropriate fix to the issues posed in this PR, I'd be OK with changing the Lookup to only appear once per field (at least for now), whether in this PR or in a followup PR.

@tdonohue we agree with this approch, indeed it was the first solution used when we've opened repeatable fields PR. If all agree we can restore that implementation in a follow-up PR. It should take about 2/3 hours

[benbosman]

This PR in combination with DSpace/DSpace#3224 sadly still doesn't fix the bugs introduced in #1025

I've tested it with Tim's configuration and database from #3232, but the submission of a Person entity, and relating it to a Publication entity, keeps failing

[benbosman]

I just noticed you've updated it while I was reviewing it, I will review again

[benbosman]

The latest update seems to fix:

• In the publication submission: Adding a relationship after adding a plain text value will first save the metadata so it ensures no data is lost
• The person submission no longer fails completely

It doesn't seem to fix:

• The Person submission doesn't display the relationship to the Publication, unless the submission is reloaded: https://www.dropbox.com/s/agt7t4ht8efszbb/Schermopname%202021-04-12%20om%2011.07.35.mov?dl=0

[abollini]

IMHO relationships not linked to a metadata cannot be supported at this time as the relationship support in the submission was build in the wrong way since its conception, see DSpace/DSpace#2953

The current options are

• change the configuration to include a linked metadata in all the case, this would make the person submission similar to the publication one so the problem will be solved
• remove the publication from the Person submission. This also make sense as other we give the possibility to whom can create a person to link it to existing publications and this would create other permission issues. The item edit page has it own way to link Person with Publication later on.

[benbosman]

This functionality has always worked. It's not because it was recently broken in #1025 that it suddenly doesn't need to be supported anymore

I've re-tested it with an older instance I still had (codebase from start of February), and this all worked correctly

[abollini]

Since its introduction the lookup button has created tons of issues, indeed it was agreed that it must be redone as it was faulty.
Unfortunately, it was decided to don't invest on that refactoring and instead try to fix issues piece by piece as much as possible. After so much effort spent on that personally I feel that we have spent enough, if we insist in trying to fix something that we know need to be redone differently we only risk to broken again something else that is more important. It is hard to me to understand why one of the two options provided above could be not good enough for now.

[benbosman]

I agree it's not in scope of this PR to perform a full refactor of the functionality.
But there are only 2 ways relationships are created, and they each have a completely different implication for metadata. Breaking one of these two implementation, is not a realistic solution.

An alternative solution can be to reverse the commits from #1025 which did break the entities submission, but based on the behavior it seems the only remaining problem is related to an observables error, or a lack of reading the item's status.

If it's helpful, it's always possible to check out how this worked start of February, that codebase did still work correctly

[abollini]

The current solution is much more stable and less problematic than what we had in February. Moreover, this PR comes from specific issues that were left pending when #1025 was merged see #1025 (review) and all of them are fixed now.
The issue with the Person submission has been reported here for the first time so it is out of scope of the current PR and as I have described above can be easily avoid with a minor configuration change that would not imply any major issues or drop of functionality.
If someone found time to work on further improvements these can be reviewed in a future PR but IMHO it is time now to close the beta 5.

[tdonohue]

@alemarte : I tested this today. It mostly works, but I found a bug in the Publication form. If you select (one or more) existing Authors, the textbox disappears (text entry is no longer possible) and the Author names are shown instead. This seems unexpected, as you should still be able to add a free-text Author name even after linking up an existing Person entity (as an Author). Here's how to reproduce with the api7.dspace.org as the backend:

1. Login as an Admin
2. Create a new Submission to the "Articles" Collection...this will use the Publication form.
3. Click the lookup button next to Author and select 1-2 Persons. Close the window
4. The 1-2 Persons will appear on the Submission form, but the Author textbox will now be gone.

@benbosman and @abollini : I also did some thorough testing regarding your comments/discussion above. The relationship bug reported by @benbosman in this comment does not appear to be related to this PR, so I've moved it into a separate ticket (along with all my detailed testing notes), See #1094. I'll also note that I've verified that this same bug existed before #1025 was applied...I'm not yet sure when it started though. But, please refer to all my testing details to the new ticket.

For the purposes of this PR, I'd therefore like to keep the scope very small. We are only looking to resolve the issues detailed in #1091 without introducing any new issues.

Finally, I'd like to again note that I'm starting to think the behavior of the "Lookup" button may require it to only appear once per field (not once per value). I mentioned this in my previous comment, and I still think it is true. I'd like @artlowel 's feedback on that concept though before we change anything regarding the "Lookup" button...specifically I'm not sure if it'd make solving #1094 easier or harder. In any case, we may want to keep it out of the scope of this current PR for now.

[alemarte]

@alemarte : I tested this today. It mostly works, but I found a bug in the Publication form. If you select (one or more) existing Authors, the textbox disappears (text entry is no longer possible) and the Author names are shown instead. This seems unexpected, as you should still be able to add a free-text Author name even after linking up an existing Person entity (as an Author). Here's how to reproduce with the api7.dspace.org as the backend:

1. Login as an Admin
2. Create a new Submission to the "Articles" Collection...this will use the Publication form.
3. Click the lookup button next to Author and select 1-2 Persons. Close the window
4. The 1-2 Persons will appear on the Submission form, but the Author textbox will now be gone.

@tdonohue with the last modification this also should now work properly.

[artlowel]

I'd like @artlowel 's feedback on that concept though before we change anything regarding the "Lookup" button

The only reason I'm a bit hesitant to agree is because we'd lose the behavior where type in a value in the metadata field, and click the lookup button next to it to use it as a query.

Perhaps we can do both? Keep the little spyglass button we have next to each metadata value, but also add a bigger lookup button at the bottom next to "add more"?

[tdonohue]

👍 Thanks @alemarte . This appears to now solve the initially reported issues (at least for me).

(As a sidenote, I'm still not certain the behavior of the Lookup field is correct.. I'm less and less sure it should be on each value, but we can leave it that way for now until we get feedback from actual users on the current behavior.)

@artlowel or @benbosman : Any objections to moving this PR forward? It seems to me like all prior feedback was resolved....but feel free to give one last test if you feel it's needed.

[artlowel]

@artlowel or @benbosman : Any objections to moving this PR forward? It seems to me like all prior feedback was resolved....but feel free to give one last test if you feel it's needed.

Go ahead

[artlowel]

Thanks @alemarte