Skip to content
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

Create relationships during the submission #531

Merged
merged 120 commits into from
Dec 20, 2019

Conversation

artlowel
Copy link
Member

@artlowel artlowel commented Nov 26, 2019

This PR adds the ability to create relationships between items during the submission.

It depends on #508

Description

It will add a lookup button next to fields that have a selectableRelationship section in the submission form.

Clicking that button will open a modal that allows you to search for related items using the relationship, filter and search configuration specified in the selectableRelationship section.

The search results will have a checkbox or a radio next to them based on if you can have one or multiple relationships of that type.

Selecting a result will immediately create a relationship with the item being submitted (meaning you don't need to close the popup first).

You can see an overview of the current relationships in the current selection tab.

If name variants are enabled for the selectableRelationship. The name field of the search results will be comboboxes, they contain any name variants that are saved in the item metadata (currently always in the field dc.title.alternative but that will likely change and depend on the entity type in the future).

You can enter a new name variant in the input section of the combobox. If you are an admin (in the future this will check for edit rights on the related item, but currently there's no way to do that) you'll see a popup after that gives you the option to store it to the related entity.

Still to do for this PR:

  • Bug: After a merge right before we opened this PR, selected relationships no longer show up in te submission, only in the modal
  • Fix linting issues
  • Write more tests and docs

Features not included in this PR:

  • The ability to mix relationships with plain metadata values for the same field
  • A search as you type dropdown on the submission field inputs
  • The ability to reorder related entities in the submission
  • Pagination for selected related entities in the submission.

…849_relationships-in-submission

Conflicts:
	src/app/+search-page/search-page.module.ts
	src/app/+search-page/search-results/search-results.component.html
	src/app/core/shared/search/search-configuration.service.ts
	src/app/shared/search/search-result-element-decorator.ts
Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LotteHofstede and @artlowel : I gave this another thorough code review again today, including the new tests. Most of it looks great. I do however have some additional inline comments -- I believe all of these are very minor & can be cleaned up very quickly.

I will also give this code another test later today and report back.

UPDATE: After testing, I can report back that this mostly works great, but I've hit upon a few bugs / oddities along the way. The previous bugs I had reported seem fixed though! Here's the new issues:

  1. In the popup search modal, pagination is slow. I'm using the test data provided by Ben, which has 68 authors. If I click on page "6" it takes about 3-4 seconds before the data from that page appears. It's possible my backend is just slow, but wanted to see if this is reproducible.
    • In Chrome Dev Tools, it looks like my backend is speedy enough, but each time I click a new page, for each item returned I see it querying the /bundles, /relationships and /owningCollection paths (so 3 separate REST requests per Item). So, it looks like it might be gathering more info than is necessary.
  2. In that modal, if I add a filter and then remove it by clicking the label under the search box, the (greyed out) page behind randomly jumps to the homepage (I.e. I'm exited from the entire submission but the modal stays open). It looks like the URL of the filter removal button is incorrect.
  3. If I exit the submission and return to that same submission, the first author is listed in the input form boxes (rather than them being empty by default). The list of previously added authors sometimes takes a second or two to load, but it does appear.
  4. It appears that the input boxes for author first & last name do nothing, even though I can enter text into them. If I enter something there and click "Lookup" or just hit the Enter key, I expect it to either perform a search, or let me add a textual author. Neither occurs.
  5. I'm noticing (in general) that as I click around in the popup modal (switch between pages, run searches, change name variants, change tabs) things seem to get slower and slower. I'm not sure if this is reproducible to others or if it's my local environment. But, eventually I have to open a new tab, or rerun yarn start to speed it back up again (The behavior is almost like a large cache that gets slower and slower. If I open a new browser, it's speedy again at first, but then slows down again after ~10 clicks around).
    • DISREGARD: This may have been an issue in my Docker backend. After restarting, things do not get worse over time, though pagination is still a bit slow (at least the first time I visit each page)
  6. In the modal, the "Sort By" option does not appear to work. It defaults to "Relevance" (which seems to be a random order for Authors -- I would expect them to be ordered alphabetically). If I change to "Title Ascending" or Descending, I get 400 errors in Chrome Dev Tools.
  7. Not a huge deal, but we may want to improve the usability of the select/deselect all options in this popup modal. I accidentally clicked it for 68 authors, and all 68 author relationships were created (so it works!), but I'm not sure that's something we'd want to have happen if your institution has thousands of authors. It should likely either warn "are you sure?" or maybe not even appear on this popup (though keeping "select/deselect page" seems fine).

Overall, I will say the feature looks to work. I'm just seeing occasional slowness and some usability issues overall. I'm not sure if these are reproducible to others or are somehow specific to my local environment (I'm using Docker to run my REST API, but just running the Angular UI using yarn run clean; yarn install; yarn start)

config/environment.default.js Outdated Show resolved Hide resolved
src/app/+search-page/search.component.html Show resolved Hide resolved
src/app/core/core.module.ts Show resolved Hide resolved
src/app/core/shared/operators.ts Outdated Show resolved Hide resolved
src/app/shared/object-list/object-list.component.ts Outdated Show resolved Hide resolved
src/app/shared/utils/route.utils.ts Show resolved Hide resolved
@LotteHofstede
Copy link
Contributor

LotteHofstede commented Dec 20, 2019

@tdonohue
Answering a few of your issues, the rest will be covered by @artlowel:

  1. Fixed this issue, it might have been introduced by a merge conflict
  2. This issue was already fixed in a later PR, where we changed a significant part of the way the values are rendered.
  3. Same as 3.

@artlowel
Copy link
Member Author

@tdonohue

  1. In the popup search modal, pagination is slow. I'm using the test data provided by Ben, which has 68 authors. If I click on page "6" it takes about 3-4 seconds before the data from that page appears. It's possible my backend is just slow, but wanted to see if this is reproducible.
    In Chrome Dev Tools, it looks like my backend is speedy enough, but each time I click a new page, for each item returned I see it querying the /bundles, /relationships and /owningCollection paths (so 3 separate REST requests per Item). So, it looks like it might be gathering more info than is necessary.

This is due to:

  1. The fact that currently when the RemoteDataBuildService constructs an object, it will greedily go over all its relationships and retrieve any that aren't already cached, whether they're needed or not. I've been working on that for a while now, and will create a PR to solve that issue early next year
  2. The fact that the submission was designed not to cache anything at all. forceByPassCache was introduced in the submission PR, and later when we tried to remove it, it became clear that the submission just wouldn't work with the cache without major changes, so the solution then was to use a cache time of 0 for submission requests for the time being. As a consequence, every rest resource used for the modal needs to be fetched from the backend every time. We'll likely need to address this. But perhaps we can wait and see if my solution for 1. improves performance by enough, because it doesn't look like it's easy to fix in the submission.
  1. In the modal, the "Sort By" option does not appear to work. It defaults to "Relevance" (which seems to be a random order for Authors -- I would expect them to be ordered alphabetically). If I change to "Title Ascending" or Descending, I get 400 errors in Chrome Dev Tools.

This appears to be an issue with search configurations everywhere. If you try it on the regular search page, with the same configuration you have the same problem. The reason is that the sort options were hardcoded in the UI at first, but never updated after it became possible to get the available sort options from rest. I've created an issue to track it: #549

  1. Not a huge deal, but we may want to improve the usability of the select/deselect all options in this popup modal. I accidentally clicked it for 68 authors, and all 68 author relationships were created (so it works!), but I'm not sure that's something we'd want to have happen if your institution has thousands of authors. It should likely either warn "are you sure?" or maybe not even appear on this popup (though keeping "select/deselect page" seems fine).

Yeah I noticed the same things, but I was thinking more along the lines of: don't immediately create all relationships, but wait either a set amount of time (a few seconds), or until you close the modal to create them. In any case we'd prefer to tackle this in a separate, dedicated PR later on.

@paulo-graca
Copy link
Contributor

paulo-graca commented Dec 20, 2019

These is more an usability and expectancy things that I think can be addressed in later and smaller PRs...
when I type anything in the form, I was expecting the typed valued could be used to filter the results in the model:
image

and the introduced value is ignored:
image

Another thing is the "Current selection" tab I think it should be distinguished from the other normal tabs, otherwise, the user will ignore it:
image

And the "Close" option isn't obvious to the user that the process of item association is ended, because there isn't any visual feedback and if the related field is mandatory the submission can't be finished:
image
(mandatory field validation should also include relations presence verification)

@artlowel
Copy link
Member Author

@tdonohue
All of your feedback should have been addressed now. Could you re-review?

@paulo-graca
Both of those are good points. However it's a lot less work for us to address them in #541, so we'll fix them there if you don't mind.

@paulo-graca
Copy link
Contributor

@paulo-graca
Both of those are good points. However it's a lot less work for us to address them in #541, so we'll fix them there if you don't mind

Ok, and since this PR is quite big in the number of changes. I also agree to have them addressed in a separate PR.

Copy link
Contributor

@paulo-graca paulo-graca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reported an issue on Slack related with the "endless loading". But it looks it's related with my local setup since it doesn't occurs if I use a different server like the one in 4Science (dspace7.4science.cloud).

Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I've reviewed the changes made today, and the code all now looks good to me. I've also retested and it works well. Based on what @artlowel and @LotteHofstede report, this looks good to merge, as the other issues I noted have plans to resolve. Here's a summary of the resolution of what I previously reported:

  1. Slow pagination in search results popup. Art noted this is not specific to this PR, and he's working on it. That sounds fine to me to resolve in a later PR.
  2. Filter removal not working. This is now fixed & works.
  3. Will be fixed in related PR Combining relationships and metadata during submission #541 & I'll retest the fixes there.
  4. Also will be fixed in related PR Combining relationships and metadata during submission #541 & I'll retest the fixes there.
  5. This issue no longer exists...seems to have been my backend not behaving well
  6. "Sort By" option is not working. I've verified this is a separate bug as it also exists elsewhere in the system (not just in submission). Art logged it already as Search sort options are hardcoded #549
  7. "Select All" needs a warning or should be removed from relationship modal. This is a usability issue that I'll log as a separate bug ticket. Logged at "Select All" option in Entity Relationships modal should not create relationships immediately #550

All in all, I approve of this PR as is. The remaining (minor) issues all will be tracked via bug tickets & resolved in follow-up PRs.

@tdonohue
Copy link
Member

As this is now at +2 and has had a thorough code review & test by @paulo-graca and I (including several rounds of feedback over the last 3 weeks), I'm merging this as-is. Thanks all for the hard work @artlowel , @LotteHofstede and @paulo-graca !

@tdonohue tdonohue merged commit 6b5303f into DSpace:master Dec 20, 2019
@tdonohue tdonohue added this to the 7.0beta1 milestone Jan 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants