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

Yashwanth individual permission change show #2868

Open
wants to merge 5 commits into
base: development
Choose a base branch
from

Conversation

yashwanth170
Copy link
Contributor

@yashwanth170 yashwanth170 commented Nov 14, 2024

Description

The permission change log table only logs changes made to role, so I have added additional features to log permission changes made to individual account. Now the table shows latest logs of changes made to both individual and role

Bug_description

Related PRS (if any):

This frontend PR is related to the #1149 backend PR.
To test this frontend PR you need to checkout the 'Yashwanth_individual_permission_change_show_backend' Backend PR.

Main changes explained:

  • When changes are made to indvidual or user, the function calls endpoint POST_USER_PERMISSION_CHANGE_LOGS to post changes made to permissions
  • Created new endpoint GET_USER_PERMISSION_CHANGE_LOGS to fetch changes made to both individual and role

How to test:

  1. check into current branch, and check into backend branch Yashwanth_individual_permission_change_show_backend
  2. do npm install and ... to run this PR locally
  3. Clear site data/cache
  4. log as admin user
  5. go to dashboard→ Other Links → Permission Management → Make changes to a role or individual and verify that the table updates accordingly

Screenshots or videos of changes:

Note:

Test it across different user types, and let me know if I missed any expected conventions or if you have any other suggestions.

Copy link

netlify bot commented Nov 14, 2024

Deploy Preview for highestgoodnetwork-dev ready!

Name Link
🔨 Latest commit 3df8692
🔍 Latest deploy log https://app.netlify.com/sites/highestgoodnetwork-dev/deploys/673fb03350f0a700080ca3b9
😎 Deploy Preview https://deploy-preview-2868--highestgoodnetwork-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@yashwanth170 yashwanth170 added the High Priority - Please Review First This is an important PR we'd like to get merged as soon as possible label Nov 14, 2024
Copy link
Contributor

@manikittu810 manikittu810 left a comment

Choose a reason for hiding this comment

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

Hello @yashwanth170
I have tested this , and it is displaying the text Loading for a long time instead of a table and I hjave also tried using clearing the cache.
Screenshot 2024-11-14 at 5 52 42 PM

Thank you.

@yashwanth170
Copy link
Contributor Author

Hello @yashwanth170 I have tested this , and it is displaying the text Loading for a long time instead of a table and I hjave also tried using clearing the cache. Screenshot 2024-11-14 at 5 52 42 PM

Thank you.

Hello,
Did you checkout the backend branch as well?

Copy link
Contributor

@nathanah nathanah left a comment

Choose a reason for hiding this comment

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

Most of this logic should be on the backend. The only frontend changes required should be accommodating the new info in the permissionChangeLogTable.

@@ -94,6 +135,7 @@ function UserPermissionsPopUp({
autoClose: 10000,
});
setToastShown(true);
logPermissionChanges(logPermissionChangesUrl, permissionChangeLog);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
logPermissionChanges(logPermissionChangesUrl, permissionChangeLog);

This is not atomic. This request could fail after the permission change request succeeds, so it wouldn't show up in the history. If a dev wanted to test updating permissions manually with something like Postman, they could avoid also logging those changes which could cause confusion or potential problems.
Logging permission changes should be part of the same API call as updating the user's permissions.

src/utils/URL.js Outdated
@@ -5,6 +5,8 @@ const APIEndpoint =
export const ENDPOINTS = {
APIEndpoint: () => APIEndpoint,
USER_PROFILE: userId => `${APIEndpoint}/userprofile/${userId}`,
POST_USER_PERMISSION_CHANGE_LOGS: `${APIEndpoint}/logPermissionChanges`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
POST_USER_PERMISSION_CHANGE_LOGS: `${APIEndpoint}/logPermissionChanges`,

Comment on lines 97 to 126
let existingPermissions = allUserInfo.permissions?.frontPermissions || [];
const existingPermissionsSet = new Set(existingPermissions || []);
const newPermissionsSet = new Set(userPermissions || []);
const addedPermissions = (userPermissions || []).filter(
permission => !existingPermissionsSet.has(permission),
);
const removedPermissions = (existingPermissions || []).filter(
permission => !newPermissionsSet.has(permission),
);

existingPermissions = [
...existingPermissions.filter(permission => !removedPermissions.includes(permission)),
...addedPermissions,
];

const permissionChangeLog = {
actualUserProfile: {
firstName: actualUserProfile.firstName,
lastName: actualUserProfile.lastName,
},
authUser: {
email: authUser.email,
role: authUser.role,
},
userId,
existingPermissions, // Existing permissions before change
addedPermissions, // New permissions added
removedPermissions, // Permissions removed
};

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let existingPermissions = allUserInfo.permissions?.frontPermissions || [];
const existingPermissionsSet = new Set(existingPermissions || []);
const newPermissionsSet = new Set(userPermissions || []);
const addedPermissions = (userPermissions || []).filter(
permission => !existingPermissionsSet.has(permission),
);
const removedPermissions = (existingPermissions || []).filter(
permission => !newPermissionsSet.has(permission),
);
existingPermissions = [
...existingPermissions.filter(permission => !removedPermissions.includes(permission)),
...addedPermissions,
];
const permissionChangeLog = {
actualUserProfile: {
firstName: actualUserProfile.firstName,
lastName: actualUserProfile.lastName,
},
authUser: {
email: authUser.email,
role: authUser.role,
},
userId,
existingPermissions, // Existing permissions before change
addedPermissions, // New permissions added
removedPermissions, // Permissions removed
};

This logic should be on the backend and part of the same API call as the update itself.

@@ -80,9 +90,40 @@ function UserPermissionsPopUp({
const userId = actualUserProfile?._id;

const url = ENDPOINTS.USER_PROFILE(userId);
const logPermissionChangesUrl = ENDPOINTS.POST_USER_PERMISSION_CHANGE_LOGS;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const logPermissionChangesUrl = ENDPOINTS.POST_USER_PERMISSION_CHANGE_LOGS;

Comment on lines 67 to 76
const logPermissionChanges = async (logPermissionChangesUrl, permissionChangeLog) => {
try {
await axios.post(logPermissionChangesUrl, permissionChangeLog);
} catch (error) {
toast.error('Error logging permissions:', {
autoClose: 10000,
});
}
};

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const logPermissionChanges = async (logPermissionChangesUrl, permissionChangeLog) => {
try {
await axios.post(logPermissionChangesUrl, permissionChangeLog);
} catch (error) {
toast.error('Error logging permissions:', {
autoClose: 10000,
});
}
};

@@ -243,7 +244,7 @@ function PermissionsManagement({ roles, auth, getUserRole, userProfile, darkMode
</div>
{loading && <p className="loading-message">Loading...</p>}
{changeLogs?.length > 0 && (
<PermissionChangeLogTable changeLogs={changeLogs.slice().reverse()} darkMode={darkMode} />
<PermissionChangeLogTable changeLogs={changeLogs} darkMode={darkMode} />
Copy link
Contributor

@nathanah nathanah Nov 16, 2024

Choose a reason for hiding this comment

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

removing reverse? Nvm I see the sort on the backend now.

Copy link

@audreydieuanh audreydieuanh left a comment

Choose a reason for hiding this comment

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

The table doesn't show, and I have done git checkout for backend PR #1149.
test11

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.

4 participants