Skip to content

Nathan separate edit time entry permissions#924

Merged
one-community merged 3 commits into
developmentfrom
Nathan-separate-editTimeEntry-permissions
Jun 30, 2024
Merged

Nathan separate edit time entry permissions#924
one-community merged 3 commits into
developmentfrom
Nathan-separate-editTimeEntry-permissions

Conversation

@nathanah
Copy link
Copy Markdown
Contributor

@nathanah nathanah commented May 4, 2024

Description

This PR divides the editTimeEntry permission into several subpermissions controlling each part which can be edited (date, isTangible, time, and description).

Related PRS (if any):

To test this backend PR you need to checkout the #2221 frontend PR.

Main changes explained:

  • Added permissions to PermissionsConst.js
  • Replaced editTimeEntry with editTimeEntryTime, editTimeEntryDescription, editTimeEntryDate, editTimeEntryToggleTangible (change tangibility for other users' time entries), and toggleTangibleTime (change tangibility for own time entries)

How to test:

  1. check into current branch
  2. do npm install and ... to run this PR locally
  3. Clear site data/cache
  4. log as owner user
  5. go to dashboard→ Other Links→ Permissions Management → Manage User Permissions
  6. grant a volunteer account one of the listed permissions individually (editTimeEntryTime, editTimeEntryDescription, editTimeEntryDate, editTimeEntryToggleTangible, and toggleTangibleTime)
  7. login to your volunteer account in an incognito window (so you can stay logged in with your owner account)
  8. test the granted permission with volunteer
  9. logout as volunteer
  10. repeat steps 6-10 for each permission

Notes

Check the Permissions Management spreadsheet or the descriptions in the Permissions Management Page for desired behaviors of permissions
https://docs.google.com/spreadsheets/d/1TfiJY9OLDZuyP2UgF0C1wI9tOSLKbS9MiJFH6n1Dsao/edit#gid=0

@one-community one-community added the High Priority - Please Review First This is an important PR we'd like to get merged as soon as possible label May 4, 2024
@mSharifHub
Copy link
Copy Markdown

Hello, I tested the code and I have not seen any issues or warning related to the functionality itself.
Screenshot 2024-05-04 at 3 24 40 PM

Copy link
Copy Markdown

@mSharifHub mSharifHub left a comment

Choose a reason for hiding this comment

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

I would suggest a middleware approach to check for permissions to keep code more organized and easy to understand
I would suggest the following:
`const { hasPermission } = require('./permissions');

function requirePermission(permission) {
return async function(req, res, next) {
const hasPerm = await hasPermission(req.body.requestor, permission);
if (!hasPerm) {
return res.status(403).send({ error: 'You do not have the required permission' });
}
next();
};
}

module.exports = {
requirePermission
};`

besides this. the code functionality works as expected. I took a screen shot in the comments sections and have not noticed any issues.

Copy link
Copy Markdown
Contributor

@nathcarnevalli nathcarnevalli left a comment

Choose a reason for hiding this comment

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

I tested every permission and all of them worked for me without any issues. Great job! However, I've found some problems with the frontend that I'll review on PR #2221.

Screenshot 2024-05-10 092023

Screenshot 2024-05-10 093304

Copy link
Copy Markdown

@hbodgal hbodgal left a comment

Choose a reason for hiding this comment

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

image

I was able to give permission and access it from volunteer account. I did not face any difficulties.

@KaushikMreddy
Copy link
Copy Markdown

I've left a detailed review in the front-end PR: OneCommunityGlobal/HighestGoodNetworkApp#2221 (review)

Copy link
Copy Markdown

@Wenboooooo Wenboooooo left a comment

Choose a reason for hiding this comment

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

image
1716016928538
Looks good to me, all functions can be edited.

@cecilia-uu
Copy link
Copy Markdown

cecilia-uu commented May 21, 2024

I have tested it and the #2221 frontend. It works well when I try to edit the time entries. But if I got it right, this change is to make this feature divided into different parts. I only add the 'edit Timelog description', but it seems that I can edit all the parts...
截屏2024-05-21 上午11 35 46
截屏2024-05-21 上午11 17 54

Copy link
Copy Markdown

@Sandhya1236 Sandhya1236 left a comment

Choose a reason for hiding this comment

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

I checked out the current branch, ran npm install and necessary commands to set up the PR locally, and cleared the site data/cache. As the owner user, I navigated to Dashboard → Other Links → Permissions Management → Manage User Permissions. I granted a volunteer account each of the listed permissions (editTimeEntryTime, editTimeEntryDescription, editTimeEntryDate, editTimeEntryToggleTangible, and toggleTangibleTime) individually. I logged into the volunteer account in an incognito window and tested each granted permission successfully. After logging out of the volunteer account, I repeated this process for each permission. All tests were successful, confirming that the permission management functionality works as intended.

image
image

Copy link
Copy Markdown

@Kavil-Jain-514 Kavil-Jain-514 left a comment

Choose a reason for hiding this comment

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

Everything worked great for me. The volunteer was able to edit the tangible time with permission.
image

Copy link
Copy Markdown
Contributor

@Parth-tech Parth-tech left a comment

Choose a reason for hiding this comment

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

Hey Nathan,
The codebase looks great and the required functionality works as intended. Great job! I have left a detailed review on the front-end repo, review link.

@peter6866
Copy link
Copy Markdown
Contributor

Hi Nathan, I left a detailed review for the frontend PR.

Copy link
Copy Markdown
Contributor

@ImzIssa ImzIssa left a comment

Choose a reason for hiding this comment

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

This looks good to me.

Here's my frontend review

@one-community
Copy link
Copy Markdown
Member

Thank you all, merging!

@one-community one-community merged commit 1606c5c into development Jun 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

High Priority - Please Review First This is an important PR we'd like to get merged as soon as possible

Projects

None yet

Development

Successfully merging this pull request may close these issues.