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

Supervised Order with OBSERVER permissions can only view the item #2226

Merged
merged 14 commits into from May 23, 2023

Conversation

alisaismailati
Copy link
Contributor

References

Description

Supervised Order with OBSERVER permissions should only view an item and not be able to delete or edit it, so the buttons for delete and edit actions should not be displayed. In order not to show the buttons we check the authenticated user permissions for editing the item.

Instructions for Reviewers

To show only the view button for the Supervised Order with OBSERVER permissions on MyDSpace, we check if the authenticated user permissions allows hit to edit/delete the item. First, we identify if the user is an administrator, this way the user has all rights. We also need to identify based on FeatureID.CanEditItem if the user can edit the item, therefore he can delete the item (since if the user is able to access the edit page, the item can be discarded also from there).

List of changes in this PR:

  • First, we can identify if the user is an administrator or not by calling the isAuthorized() method of an authorizationService, bypassing in the FeatureID.AdministratorOf constant and the user UUID.
  • Second, we check again if the user eperson has the permission to edit the item FeatureID.CanEditItem constant and the user UUID.
  • Lastly, we add the checks in the template to corresponding buttons.

Include guidance for how to test or review your PR.
Follow the steps from #2094 to be able to reproduce the behavior and
notice that Edit & Delete button are not appeared on MyDSpace page for the OBSERVER role.

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 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.
  • 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 tdonohue added bug 1 APPROVAL pull request only requires a single approval to merge component: MyDSpace labels May 2, 2023
@tdonohue tdonohue added this to the 7.6 milestone May 2, 2023
@tdonohue tdonohue self-requested a review May 4, 2023 14:35
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.

@alisaismailati : Thanks for this bug fix! I've verified that it works. I can only see the "Edit" and "Delete" buttons when a supervision order gives EDITOR permissions...not with just OBSERVER.

However, I found a small bug in the existing code (I see an error in my browser's DevTools console). I also think there's a small performance improvement we could make. Once these fixes are applied, I can retest this.

const activeEPerson$ = this.authService.getAuthenticatedUserFromStore();

this.isAdmin$ = activeEPerson$.pipe(
switchMap((user: EPerson) => this.authorizationService.isAuthorized(FeatureID.AdministratorOf, user.uuid)));
Copy link
Member

Choose a reason for hiding this comment

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

I think this isAuthorized() call is incorrect, as I'm seeing an error in my browser's DevTools console. The backend returns 400 Bad Request on this line.

I believe it should simply be:

this.isAdmin$ = this.authorizationService.isAuthorized(FeatureID.AdministratorOf);

(See for example item-page.component.ts which has this same code).

@atarix83 atarix83 requested a review from tdonohue May 18, 2023 09:54
@tdonohue
Copy link
Member

@alisaismailati or @atarix83 : Could this be rebased on latest main? I believe the test failures here were the same ones we just fixed on main, so a rebase should get this to pass tests.

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.

👍 Thanks @alisaismailati and @atarix83 . This looks good now & works except there's an unused isAdmin variable still in the code. I'll wait to merge this until that can be removed, see inline below.

@tdonohue tdonohue merged commit 9c78020 into DSpace:main May 23, 2023
10 checks passed
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: MyDSpace
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Supervised Order with OBSERVER permissions shows too many buttons on MyDSpace.
3 participants