DSpace / dspace-angular

DSpace User Interface built on Angular.io
https://wiki.lyrasis.org/display/DSDOC8x/
BSD 3-Clause "New" or "Revised" License
134 stars 433 forks source link

Relationship lookup component displays misleading information #3312

Open jlipka opened 1 month ago

jlipka commented 1 month ago

Describe the bug

There is a bug in DSpace when you are in the submission process and use the lookup to search for an author to create a new relationship. The information displayed in the component is incorrect and does not reflect the current relationship state of the submission item.

My test was made locally with DSpace in version 8, the same error occurs in the sandbox (sandbox.dspace.org) with Version 9.

To reproduce

  1. Login (using an account/role that is allowed to create any new submission)
  2. Create a new item in any collection that contains an author lookup (e.g., "Publications > Publications 2").
  3. Open the author lookup modal by clicking on the search icon.
  4. (implicit: some authors are listed automatically; when testing with a new submission, they are unchecked).
  5. Start typing in the search input field and enter a string that will certainly not return any results, e.g. "asdf".
  6. Click the arrow next to " Person Search Results" and click "Select Page".

Now there are several problems to the user:

  1. The still visible "Current Selection (X)" tab indicates that there are 5 (example) active relationships, even if you click inside the tab, it seems like multiple entities are chosen as a relation
  2. If you close the lookup modal the authors-list of the submission form has been extended by only one author (the first of the initial search result)
  3. if you click on "Add more", start a new (empty) search and the modal opens once again, the "Current selection" tab still indicates the wrong list of active relationships.
  4. if you click on "Add more", start a new search using a nonsense query (e.g. "asdf") again. The modal opens, and after clicking on "Select page" an error occurs (see in browser console); the error seems to break the functionality of the component and any further search will lead in an endless loading animation.

Expected behavior

  1. The drop-down menu next to the search results to select all items should only appear if there is a list of results.
  2. The functionality behind "Select page" / "Deselect page" should only be based on the current search results, not on any old data from any previous/initial search.

Screenshots

2024-09-09_17-40-author-lookup-1

2024-09-09_17-40-author-lookup-2

2024-09-09_17-40-author-lookup-3

2024-09-09_17-40-author-lookup-4

2024-09-09_17-40-author-lookup-5

tdonohue commented 1 month ago

If I'm understanding correctly, the core issue here is that that arrow dropdown (to the right of "Person Search Results") should not be displayed when there are no results. Instead, in the example above, if you select "Select Page" it seems it is trying to select the first page of results as if there was no search.

For instance, I see that the "Select Page" option appears to work properly when "Person Search Results" has results to display.

So, I believe this bug may be fixed if the "Select Page" and "Deselect Page" options no longer exist when the page of search results is empty.

Needs a volunteer.

jlipka commented 1 month ago

If I'm understanding correctly, the core issue here is that that arrow dropdown (to the right of "Person Search Results") should not be displayed when there are no results.

Yes, I think you're right - your proposed change should solve all the problems described.

Andrea-Guevara commented 3 weeks ago

Good morning @tdonohue!

I've been looking for a few days and testing possible solutions to this problem and I've come up with a few possibilities:

  1. Put the following conditional with ngIf in the select/deselect dropdown of the page: ngIf="(resultsRD$ | async)?.page.length > 0”

  2. Put a disable property on the button, so that if the search returns no results, the select/deselect options are disabled. This would take advantage of the variable “*ngVar=”(resultsRD$ | async) as resultsRD” : [disabled]=”!resultsRD?.page.length > 0”

  3. Make the “selectPage()” method only run when the page has an item on it: selectPage(page: SearchResult[]) { if (!page || page.length === 0) { return; } this.selection$ .pipe(take(1)) .subscribe((selection: SearchResult[]) => { const filteredPage: SearchResult[] = page.filter((pageItem: SearchResult) => selection.findIndex((selected: SearchResult) => selected.equals(pageItem)) < 0); if (filteredPage.length > 0) { this.selectObject.emit(...filteredPage); } }); this.selectableListService.select(this.listId, page); }

I've tested all these different ways and it works, but unfortunately the condition is only reproduced once and then it doesn't work anymore. The tests were carried out on the component: src/app/shared/form/builder/ds-dynamic-form-ui/relation-lookup-modal/search-tab

I hope this helps in some way, and if you have any suggestions, please let me know.

tdonohue commented 3 weeks ago

@Andrea-Guevara : I don't understand what you mean by this..

I've tested all these different ways and it works, but unfortunately the condition is only reproduced once and then it doesn't work anymore.

Are you saying that all the fixes you tried are unsuccessful? Or they only partially work?

I'm not sure what to recommend here as I'm not familiar with this area of the code. But, if the fixes that are attempted only partially work, that means there must be a larger bug here. I'm not sure where it would be offhand, but that may imply that someone would need to dig deeper as to why a basic fix only works temporarily.

Andrea-Guevara commented 3 weeks ago

Good afternoon @tdonohue, I hope you're well!

I didn't explain myself very well! I meant that my attempts to solve this problem only work once. It seems that some cache is stored and the conditionals are not able to identify the changes. For example:

If I open the modal and it has results the return would be “true” but if no results are present, the return would be “false”. It seems that this switch between true and false isn't identified, so it's almost impossible to condition the dropdown so that it doesn't appear if no results are found.

I hope I've made myself clear haha. If it's still not clear, let me know.

tdonohue commented 3 weeks ago

@Andrea-Guevara : Ok, I understand now.

Unfortunately, I'm not sure of the answer myself. It does sound like something is cached here. It may need some digging / debugging to understand what is going on. That's not something I have time for at the moment.

The only alternative is to find a different way to solve this issue. Obviously, the core issue here is that you shouldn't be able to select a page if there are no pages. I'm not sure myself of a better solution at the moment. Hopefully, if another developer has an idea here, they'll chime in on this discussion.

jlipka commented 2 weeks ago

@Andrea-Guevara Thank you for your initial research and solutions. I have taken your suggestions and discussion and submitted a PullRequest proposal. There you can also see the possible solution that an event is still triggered if the result set is empty - I think that was still the problem? I followed your approach to disable the dropdown options if there are no results.

I look forward to your feedback, or possibly a merge if everything is OK.