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

Added support for change list sizes on bundle edit #3266

Closed
wants to merge 3 commits into from

Conversation

oscar-escire
Copy link
Contributor

References

Add references/links to any related issues or PRs. These may include:

Description

Added a button for change element list size

Instructions for Reviewers

  • Generate an item with 11 or more bitstreams in a bundle
  • Click on the gear icon on the bundle name row and change size of list

List of changes in this PR:

  • Added a custom gear button that enables change size of bundle paginator drag and drop list

Checklist

This checklist provides a reminder of what we are going to look for when reviewing your PR. You do not need to complete this checklist prior creating your PR (draft PRs are always welcome).
However, reviewers may request that you complete any actions in this list if you have not done so. If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!

  • My PR is created against the main branch of code (unless it is a backport or is fixing an issue specific to an older branch).
  • 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 ESLint validation using yarn lint
  • My PR doesn't introduce circular dependencies (verified via yarn check-circ-deps)
  • 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.
  • My PR aligns with Accessibility guidelines if it makes changes to the user interface.
  • My PR uses i18n (internationalization) keys instead of hardcoded English text, to allow for translations.
  • My PR includes details on how to test it. I've provided clear instructions to reviewers on how to successfully test this fix or feature.
  • If my PR includes new libraries/dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.
  • If my PR includes new features or configurations, I've provided basic technical documentation in the PR itself.
  • If my PR fixes an issue ticket, I've linked them together.

@tdonohue
Copy link
Member

@oscar-escire : Thanks for the PR. Just a note that this PR is failing existing specs (yarn test) in ItemEditBitstreamBundleComponent. It's possible the refactor you made either needs to be taken into account in those tests, or similar.

@tdonohue tdonohue added 1 APPROVAL pull request only requires a single approval to merge component: Item (Archived) Item display or editing bug port to dspace-8_x This PR needs to be ported to `dspace-8_x` branch for next bug-fix release labels Aug 27, 2024
@tdonohue tdonohue requested a review from artlowel August 29, 2024 14:58
Copy link
Member

@artlowel artlowel left a comment

Choose a reason for hiding this comment

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

Thanks @oscar-escire!

As I commented on the ticket, there is already a way to move a bitstream to a different page, though at the moment it seems it's a bit buggy, and clearly people don't know about it, so it needs to be made more apparent

However, that doesn't mean it's a bad idea to add the ability to set a page-size to this page. Your PR works, so I don't see why we couldn't include this as well

A few comments though:

  • The tests are currently failing, it seems likely these changes have broken some pre-existing tests
  • It would be a good idea to refactor the gear and all the logic involved into a separate, dedicated component

@mwoodiupui
Copy link
Member

I don't oppose changing the list size, but in yesterday's meeting I thought of an alternate approach which should work (if it is workable!) for lists of any size: cut/paste. Many tools do this by simply remembering that the user requested to cut a list element and then moving the element to the target position when "paste" is done.

@oscar-escire
Copy link
Contributor Author

Thanks @tdonohue, @artlowel, @mwoodiupui!

I have updated the PR attending your comments.

  • I improved the function to drag and drop on a specific page, the function was based on hovering the mouse to select when and to which page the item should be moved, which caused that if the user quickly moved the mouse to another place after releasing the element, the element will only move to the end of the list instead of the selected page, it now relies on the point where the item is dropped to make this calculation.
  • I updated the unit tests based on the changes of the components
  • Also added a tooltip to "make more visible" the drag and drop function to a item page (maybe the position of the tooltip need to be improved)

By now I prefer no to move the logic of the page size to a separated component because the PaginationComponent include the necessary to change the page size in the lists, but it doesn't fit very wheel in the bundle's drag and drop list' case so, that configuration options its hidden

@tdonohue tdonohue requested a review from artlowel September 3, 2024 22:02
Copy link
Member

@artlowel artlowel left a comment

Choose a reason for hiding this comment

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

Thanks @oscar-escire!

The latest changes make it more clear how moving an item to a different page works.

The issue where the loading component for the next page sometimes doesn't disappear until something else moves isn't solved yet. But we're already working on #3154, so we'll fix that in the context of that PR

@tdonohue
Copy link
Member

tdonohue commented Oct 8, 2024

@oscar-escire and @artlowel : I noticed this was already at +1, so I was trying it out myself. While the Page Size option works well, I cannot seem to get the drag & drop to a different page to work. Is it supposed to work?

I've tried this:

  • Create an Item with 6 bitstreams in the ORIGINAL Bundle
  • Edit that Item & go to the Bitstreams tab
  • Change the page size of the ORIGINAL Bundle to 5. This creates two pages.
  • Now, drag the 3rd item in the list to Page 2. The page reloads but no reordering occurs.

In the backend logs, I see this error:

2024-10-08 19:19:06,072 ERROR ec49885b-3617-4441-a022-20dcf8505f51 1f1d6765-f820-46d9-a084-bb1097082f7c org.dspace.app.rest.RestResourceController @ Failed moving bitstreams of bundle with id 2c08572e-b393-4c8d-886f-ee5a8671d768 from location 2 to 10: "to" location out of bounds. Latest available position: 5
 org.dspace.app.rest.exception.DSpaceBadRequestException: Failed moving bitstreams of bundle with id 2c08572e-b393-4c8d-886f-ee5a8671d768 from location 2 to 10: "to" location out of bounds. Latest available position: 5
   at org.dspace.app.rest.repository.patch.operation.BundleMoveOperation.perform(BundleMoveOperation.java:73) ~[dspace-server-webapp-9.0-SNAPSHOT.jar!/:9.0-SNAPSHOT]
   at org.dspace.app.rest.repository.patch.operation.BundleMoveOperation.perform(BundleMoveOperation.java:32) ~[dspace-server-webapp-9.0-SNAPSHOT.jar!/:9.0-SNAPSHOT]
   at org.dspace.app.rest.repository.patch.ResourcePatch.performPatchOperation(ResourcePatch.java:58) ~[dspace-server-webapp-9.0-SNAPSHOT.jar!/:9.0-SNAPSHOT]
   at org.dspace.app.rest.repository.patch.ResourcePatch.patch(ResourcePatch.java:42) ~[dspace-server-webapp-9.0-SNAPSHOT.jar!/:9.0-SNAPSHOT]
   at org.dspace.app.rest.repository.DSpaceObjectRestRepository.patchDSpaceObject(DSpaceObjectRestRepository.java:64) ~[dspace-server-webapp-9.0-SNAPSHOT.jar!/:9.0-SNAPSHOT]
   at org.dspace.app.rest.repository.BundleRestRepository.patch(BundleRestRepository.java:111) ~[dspace-server-webapp-9.0-SNAPSHOT.jar!/:9.0-SNAPSHOT]

It looks like it's trying to add the moved bitstream to position 10 (the last position on page two?), when it should be likely moving it to the first position on page two?

I've also tried moving the one Bitstream on page 2 onto page 1. But, that also fails with a similar error:

org.dspace.app.rest.exception.DSpaceBadRequestException: Failed moving bitstreams of bundle with id 2c08572e-b393-4c8d-886f-ee5a8671d768 from location 10 to 0: "from" location out of bounds. Latest available position: 5

Maybe it doesn't work as soon as you change the page size from the default of 10?

@tdonohue
Copy link
Member

tdonohue commented Dec 6, 2024

Closing, as this doesn't fully work & now appears to have been fixed in #3464 (and the port PRs). I've just verified that PR fixes the same issue, along with several accessibility fixes. At that PR is nearly completed / approved, I'm closing this one as a duplicate.

Nonetheless, thanks @oscar-escire for your work on this. It's a shame we were not able to get this merged prior to another PR fixing the same problem.

@tdonohue tdonohue closed this Dec 6, 2024
@tdonohue tdonohue added duplicate and removed port to dspace-8_x This PR needs to be ported to `dspace-8_x` branch for next bug-fix release labels Dec 6, 2024
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 component: Item (Archived) Item display or editing duplicate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Re-ordering bitstreams on the item edit page not possible when pagination is triggered
4 participants