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

Fix relationship modal on item page #1139

Conversation

ybnd
Copy link
Member

@ybnd ybnd commented Apr 29, 2021

References

Description

Remove submission-based initialization of DsDynamicLookupRelationModalComponent#item

Instructions for Reviewers

  • Removed DsDynamicLookupRelationModalComponent#setItem
    • Along with calls, services & subs attribute
    • Removed services from unit tests
  • When opening a relationship lookup modal from EditRelationShipListComponent, set the modal's collection to the owning Collection of the Item being edited

Reviewing

  1. Authenticate as an admin

  2. Edit an item. Open its "Relationships" tab. Click on any of the "Add" buttons.

    The relationship lookup modal should open (and not get stuck in the "loading" animation)

  3. Open a submission in a collection with lookup enabled. Click on the lookup button next to the field

    The relationship lookup modal should open (and not get stuck in the "loading" animation)

Checklist

  • 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 artlowel added bug 1 APPROVAL pull request only requires a single approval to merge e/1 Estimate in hours medium priority labels Apr 29, 2021
@artlowel artlowel added this to the 7.0 milestone Apr 29, 2021
@tdonohue tdonohue self-requested a review April 29, 2021 14:33
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.

👍 This fixes the main issue and the modal now works for adding new relationships (from the Edit Item screen). Thanks @ybnd .

However, in testing this PR, I noticed two other bugs/usability issues here that are semi-related:

  1. While the modal window lets you add new relationships, it doesn't show which entities already have an existing relationship. So, the "Current Selection" tab in the modal always says zero, and every entity in the system is listed as unchecked. This means that you can attempt to add a new relationship to an Entity which already has an existing relationship as there's no way to tell in the popup which ones are already related. (If you attempt to add a duplicate relationship, nothing happens though...the UI knows that one already exists.)
  2. When an Entity has a large number of relationships, it becomes impossible to see them all on the "Relationships" tab. Either the "Relationships" tab needs pagination, or the "Current Selection" tab in the popup should list all existing relationships with pagination. Currently, if there's more than 20 relationships, only the first 20 relationships appear. So, it's impossible to see (or delete) any relationship after the first 20.

@artlowel : I suspect these both should be logged separately & we should move this PR forward as-is. But, let me know if either of these are easier to fix as part of this PR.

@artlowel
Copy link
Member

artlowel commented May 4, 2021

Yes, I'd prefer to keep this PR small and fix the other issues separately

@tdonohue
Copy link
Member

tdonohue commented May 4, 2021

Merging as the main bug is fixed. Moved the other feedback to #1148 Thanks again @ybnd !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 APPROVAL pull request only requires a single approval to merge bug e/1 Estimate in hours medium priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding relationships from the edit item page is broken
3 participants