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

Move item component #335

Merged
merged 28 commits into from Sep 13, 2019
Merged

Move item component #335

merged 28 commits into from Sep 13, 2019

Conversation

YanaDePauw
Copy link
Contributor

@YanaDePauw YanaDePauw commented Dec 6, 2018

Add the item move component to the Edit Item menu.

This feature is based on the “Move…” feature in DSpace 6. When clicking the “Move…” button in the “Edit Item” menu, the user will be redirected to a page where the user can select a collection. A search box will be present that allows the user to enter a query to narrow down the list of collections.

To avoid a big dropdown menu, the maximum amount of collections is restricted to 10. In case the collection is not present in the list, the search menu needs to be used to narrow down the results to a list that does contain the collection. Clicking the collection in the dropdown will select it.

A checkbox is present to indicate whether policies from the new collection need to be inherited. No support is present yet in the backend, so this is added as a TODO in the code.
This TODO refers to #333

Another TODO is present in the code. Currently no support is present to only display a collection to which the current user has ADD rights.
This issue is addressed in #334

To test the functionality, navigate to an item page, add "/edit" at the url, and click the Move button.

This PR depends on DSpace/DSpace#2283

@tdonohue tdonohue requested a review from atarix83 July 11, 2019 14:44
Copy link
Contributor

@atarix83 atarix83 left a comment

Choose a reason for hiding this comment

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

@YanaDePauw thanks for PR

I've made a quick test against DS-4096, move operation seems to work properly.

I've added some inline comment and other than these, I'd ask if is possible to add information about the current item collection in the move page


/**
* Load all available collections to move the item to.
* TODO: When the API support it, only fetch collections where user has ADD rights to.
Copy link
Contributor

Choose a reason for hiding this comment

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

Collection endpoint has a findAuthorized method https://dspace7.4science.cloud/dspace-spring-rest/#https://dspace7.4science.cloud/dspace-spring-rest/api/core/collections/search/findAuthorized
Should be useful to only fetch collections where user has ADD rights to? It should be checked if it accepts the query parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, this endpoint does not allow a user to search for a collection (using name or uuid), which means it cannot be used in suggestion based dropdown menu.
What I ideally would need is an endpoint that finds the authorized collections that also correspond to a certain search query.

Add a title to the move page
Disable submit button when no target is selected
Add operation in progress indicator
Use updated inputSuggestion to fix bug with value selection
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.

The overall functionality here appears to work, based on basic tests. However, the code branch is outdated (needs updating to latest master, and also merge conflict resolved). I added a few inline comments of things to cleanup as well (and I agree with the notes @atarix83 already added)

src/app/core/data/item-data.service.ts Show resolved Hide resolved
src/app/core/data/item-data.service.ts Show resolved Hide resolved
 Conflicts:
	resources/i18n/en.json
	src/app/+item-page/edit-item-page/item-metadata/edit-in-place-field/edit-in-place-field.component.spec.ts
	src/app/+item-page/edit-item-page/item-metadata/edit-in-place-field/edit-in-place-field.component.ts
@atarix83
Copy link
Contributor

@YanaDePauw thanks for making the required changes.

I review again and it looks good, I should report only a little annoying behavior.
In the move page, while searching for a collection and selecting a result, if you click exactly on the collection name this redirect to the collection page, because it's a link. Is it possible to correct this behavior? I attach a screenshot of what i mean
Schermata del 2019-08-29 11-11-10

@YanaDePauw
Copy link
Contributor Author

Fixed the issue with the links being clickable.

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.

👍 Retested & re-reviewed today. This works as described and the code looks good. Thanks @YanaDePauw !

However, the PR must be rebased on the lastest master before it can be merged, as it has code conflicts.

Copy link
Contributor

@atarix83 atarix83 left a comment

Choose a reason for hiding this comment

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

@YanaDePauw thanks for making the required changes.

I've reviewed again and it all looks good to me

@tdonohue tdonohue merged commit ef8f857 into DSpace:master Sep 13, 2019
@benbosman benbosman deleted the Move-item-component branch September 11, 2020 07:48
@tdonohue tdonohue added this to the 7.0beta1 milestone Jan 26, 2021
4science-it pushed a commit to 4Science/dspace-angular that referenced this pull request Oct 12, 2022
[DSC-768] Inner form discard icon overlays form buttons Fixed

Approved-by: Davide Negretti
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

6 participants