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

Nathan permissions merge #931

Merged
merged 36 commits into from
Aug 21, 2023
Merged

Conversation

nathanah
Copy link
Contributor

@nathanah nathanah commented Jun 21, 2023

Description

Fixes #6 (HIGH) Verify that permissions are implemented on the back-end and not just the front-end.
Permissions were split into front-end permissions and back-end permissions with some systems designed to accommodate this. This PR merges both permission sets into one so it's easier to make sure all permissions have relevant security checks implemented.

Related PRS (if any):

This frontend PR is related to the #416 backend PR.
OneCommunityGlobal/HGNRest#416

Main changes explained:

  • I did a search and replace on all the front-end permissions to replace them with relevant back-end permissions.
  • Deleted associatedPermissions.js and all references to it.
  • Added Redux to hasPermission() calls to simplify arguments
  • Memoized hasPermission() results

How to test:

  1. check into current branch
  2. do npm install and ... to run this PR locally
  3. Everything should work the same as before for every user.

Screenshots or videos of changes:

Everything should look the same other than the code.

Note:

If someone wants to help with unit testing, that would be appreciated. I'm considering it out of the scope of this PR though.

Copy link
Contributor

@cone-7 cone-7 left a comment

Choose a reason for hiding this comment

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

We should follow the naming convention for components.

src/components/Projects/Projects.jsx Outdated Show resolved Hide resolved
@yufeiStella
Copy link
Contributor

As far as it is good on the local site.

@nathanah nathanah requested a review from cone-7 June 26, 2023 19:19
@nathanah nathanah requested a review from cone-7 July 8, 2023 19:24
@nathanah nathanah requested a review from KurtisIvey July 22, 2023 22:25
@nathanah nathanah requested a review from NidaZaki July 23, 2023 21:24
@nathanah nathanah requested a review from EvianTan July 23, 2023 21:56
Copy link
Contributor

@ThapeloMasasa ThapeloMasasa left a comment

Choose a reason for hiding this comment

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

The code looks pretty well written, good recognition for proper use of camelCase. But the permission's option is no longer available from the drop down menu of the otherlinks tab
The drop down on this PR
Screenshot 2023-08-03 at 3 31 31 PM

The drop down on development

Screenshot 2023-08-03 at 3 27 50 PM

Copy link
Contributor

@DavidC0126 DavidC0126 left a comment

Choose a reason for hiding this comment

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

Hi @nathanah,
I have tested your PR and everything works great except for create user. When I click user management -> create new user, the page became blank. (I tried to use different roles like Admin or Owner but they both behaved like this.)
blank_page

iTvX
iTvX previously requested changes Aug 10, 2023
Copy link
Contributor

@iTvX iTvX left a comment

Choose a reason for hiding this comment

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

I have done my best to check everything, everything looks fine except for I can not found permission management under Other links tab. I think you should fix that

MyRecord_20230810114255.mp4

@nathanah nathanah requested a review from iTvX August 11, 2023 19:05
Abiddy
Abiddy previously approved these changes Aug 13, 2023
Copy link
Contributor

@Abiddy Abiddy left a comment

Choose a reason for hiding this comment

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

Everything looks good as far as I have checked. A lot of useful changes here. Nice.

winghojackyli
winghojackyli previously approved these changes Aug 14, 2023
Copy link
Contributor

@winghojackyli winghojackyli left a comment

Choose a reason for hiding this comment

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

Hi @nathanah , I've tested this PR and everything looks good as the development branch.
Owner, admin and volunteer accounts have been tested on this PR. Owner can see permission list but admin and volunteer cannot. I can also use owner and admin accounts to create new users, new teams and new projects and edit them.
Great work!

Copy link
Contributor

@KurtisIvey KurtisIvey left a comment

Choose a reason for hiding this comment

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

volunteer account functions as it should. Doesn't have access to different areas that it shouldn't.
admin & owner account seems to have the exact same permissions as the volunteers except for on the tasks section of the dashboard. I checked the redux state, and it seems they should be able to. However, I haven't gone into the code to try and diagnose why this isn't occurring since I'm not familiar with that area of the codebase.

admin should be able to pause people:
pr931 admin cant pause volunteer

admin: can't access reports or others tab
pr931 admin acct does not have access to reports   others

owner: can't access reports or others tab:
pr931 owner cant access reports or others tab

veronicacheng2
veronicacheng2 previously approved these changes Aug 16, 2023
Copy link

@veronicacheng2 veronicacheng2 left a comment

Choose a reason for hiding this comment

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

Hi @nathanah, I have tested the PR using admin and owner account. There are no breaking changes and the app works as usual when I am navigating through different pages.

Logged in as admin

HGN.APP.-.Google.Chrome.2023-08-16.15-39-46.mp4

Logged in as owner

HGN.APP.-.Google.Chrome.2023-08-16.15-46-33.mp4

KurtisIvey
KurtisIvey previously approved these changes Aug 17, 2023
Copy link
Contributor

@KurtisIvey KurtisIvey left a comment

Choose a reason for hiding this comment

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

Functions well in subsequent re-reviews with pull requesters guidance. Was able to clear up the reports and other links issue by running npm run build. Great Job and thanks for your patience.

@nathanah nathanah dismissed iTvX’s stale review August 17, 2023 23:13

Fixed on back end/db

KurtisIvey
KurtisIvey previously approved these changes Aug 18, 2023
Copy link
Contributor

@KurtisIvey KurtisIvey left a comment

Choose a reason for hiding this comment

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

Fixes remained consistent after merge conflict resolved. Everything functions as it should

Copy link

@ss23046 ss23046 left a comment

Choose a reason for hiding this comment

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

Hi @nathanah ,
I read all the comments and tested the PR. App runs normally. The above requested changes have been resolved.

Permissions management tab is seen
Screen Shot 2023-08-19 at 4 14 24 PM

Able to navigate and access reports and other links(can create user too)
Screen Shot 2023-08-19 at 4 17 16 PM
Screen Shot 2023-08-19 at 4 16 22 PM

Admin is able to pause
Screen Shot 2023-08-19 at 4 16 34 PM

@one-community one-community merged commit 520cb79 into development Aug 21, 2023
1 check passed
@nathanah nathanah deleted the nathan_permissions_merge branch September 9, 2023 21:07
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.

Set permissions for time log.