#202 - Step 2, 3, 4: Implement Add/Remove/Edit permissions#2895
#202 - Step 2, 3, 4: Implement Add/Remove/Edit permissions#2895
Add/Remove/Edit permissions#2895Conversation
…Modal.vue and implement some front-end logics
…ttons and style them
…or member dropdown / dynamic label
…d getters / change the contract logic for 'gi.contracts/group/updatePermissions' action
group-income
|
||||||||||||||||||||||||||||
| Project |
group-income
|
| Branch Review |
sebin/task/#202-roles-and-permission-step2
|
| Run status |
|
| Run duration | 14m 31s |
| Commit |
|
| Committer | Sebin Song |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
10
|
Skipped
|
0
|
Passing
|
117
|
| View all changes introduced in this branch ↗︎ | |
…es-and-permission-step2
|
The PR is ready for review again. |
|
/review_fast AI review started. |
Advanced AI Review
Click to expand reviewI'll analyze the changes for bugs, security issues, and potential improvements. Let me first examine some key files to understand the patterns and context better. Issue 1: 🔴 Incorrect HTML nesting in RemoveRoleModal.vue
Location: The "Role" list item is incorrectly nested inside the "Member" list item, creating malformed HTML: ul.c-details-list(v-if='data')
li.c-list-item
i18n.c-label(tag='span') Member:
member-name.c-list-item-content.c-member-details-container(:memberID='data.memberID')
li.c-list-item // <-- INCORRECTLY NESTED inside the previous li
i18n.c-label(tag='span') Role:Suggested fix: ul.c-details-list(v-if='data')
li.c-list-item
i18n.c-label(tag='span') Member:
member-name.c-list-item-content.c-member-details-container(:memberID='data.memberID')
li.c-list-item // <-- Should be a sibling, not nested
i18n.c-label(tag='span') Role:Issue 2: 🔴 Missing validation: roleName required for UPSERT action in contract
Location: The validation for if (item.action === GROUP_PERMISSION_CHANGE_ACTIONS.UPSERT) {
if (item.roleName === GROUP_ROLES.ADMIN) { // if roleName is undefined, this is false
throw new TypeError(L('Cannot assign admin role.'))
}
// ... no check that roleName is actually provided
}This allows storing Suggested fix: if (item.action === GROUP_PERMISSION_CHANGE_ACTIONS.UPSERT) {
if (!item.roleName) {
throw new TypeError(L('Role name is required.'))
}
if (item.roleName === GROUP_ROLES.ADMIN) {
throw new TypeError(L('Cannot assign admin role.'))
}
// ... rest of validation
}Issue 3: 🟡 Potential undefined array access in EditPermissionsModal
Location: permissionsToDisplay () {
return this.config.permissionPresets[this.ephemeral.role]
}If Suggested fix: permissionsToDisplay () {
return this.config.permissionPresets[this.ephemeral.role] || []
}Issue 4: 🟡 Potential null reference when user has no role
Location: groupPermissionsToDisplay () {
const myEntry = this.allGroupMemberRolesAndPermissions.find(entry => entry.memberID === this.ourIdentityContractId)
// ...
return this.canViewOtherMembersPermissions
? sortedEntries
: [myEntry] // myEntry could be undefined
}If the current user doesn't have a role and Suggested fix: return this.canViewOtherMembersPermissions
? sortedEntries
: (myEntry ? [myEntry] : [])Issue 5: 🟡 Missing null check in globalProfile call
Location: allActiveMembers () {
return Object.keys(this.profilesByGroup(this.groupID || this.currentGroupId))
.map(memberId => this.globalProfile(memberId))
}If Suggested fix: allActiveMembers () {
return Object.keys(this.profilesByGroup(this.groupID || this.currentGroupId))
.map(memberId => this.globalProfile(memberId))
.filter(Boolean)
}Issue 6: 🟡 Potential undefined permissions causing runtime error
Location: updateRole (e) {
const value = e.target.value
this.ephemeral.selectedRole = value
if (value === GROUP_ROLES.CUSTOM) {
// ...
} else {
this.ephemeral.selectedPermissions = this.config.permissionPresets[value] // could be undefined
}If Then in enableUpdateBtn () {
return this.ephemeral.roleEntries.every(entry => entry.permissions.length) // throws if permissions is undefined
}Suggested fix (AddPermissionsListItem.vue): } else {
this.ephemeral.selectedPermissions = this.config.permissionPresets[value] || []
}Issue 7: 🟡 enableSubmitBtn allows submission with null role in EditPermissionsModal
Location: enableSubmitBtn () {
const isCurrentRoleCustom = this.data.roleName === GROUP_ROLES.CUSTOM
const { permissions, role } = this.ephemeral
if (isCurrentRoleCustom) {
return role !== GROUP_ROLES.CUSTOM || (
permissions.length !== this.data.permissions.length ||
permissions.some(p => !this.data.permissions.includes(p))
)
} else {
return role === GROUP_ROLES.CUSTOM
? permissions.length > 0
: role !== this.data.roleName // true if role is null/empty and original wasn't
}
}If Suggested fix: enableSubmitBtn () {
const isCurrentRoleCustom = this.data.roleName === GROUP_ROLES.CUSTOM
const { permissions, role } = this.ephemeral
if (!role) return false // Add this guard
if (isCurrentRoleCustom) {
// ...
}
// ...
}Issue 8: 🟡 Modal close in created hook without preventing further rendering
Location:
created () {
if (!this.data?.memberID) {
this.close()
}
}After calling Suggested fix: created () {
if (!this.data?.memberID) {
this.close()
return // Add explicit return
}
this.initComponent()
}Issue 9: ⚪ Unused variable in PermissionTableRow.vue
Location: data () {
return {
ephemeral: {
isSubmitting: false // Never used
}
}
}This variable is declared but never referenced anywhere in the component. Issue 10: ⚪ Typo in CSS class name (multiple files)
Locations:
.c-perimission-items-container // Should be "permission" not "perimission"Issue 11: ⚪ Unused import in RemoveRoleModal.vue
Location: import AvatarUser from '@components/AvatarUser.vue'This import is not used directly in the component (it's used within Issue 12: ⚪ Unused import in AddPermissionsListItem.vue
Location: import AvatarUser from '@components/AvatarUser.vue'This import is not used directly in the component (it's used within Review generated using |
|
/crush_fast AI review started. |
Advanced AI Review
Click to expand reviewCode Review: Permissions/Roles System ImplementationIssue 1: Null Pointer Exception in Validation 🔴
Location: Problem: In the if (myRoleName === GROUP_ROLES.MODERATOR_DELEGATOR &&
item.permissions.some(permission => !myPermissions.includes(permission))) {Suggested Fix: if (myRoleName === GROUP_ROLES.MODERATOR_DELEGATOR &&
item.permissions?.some(permission => !myPermissions.includes(permission))) {Issue 2: Getter Returns Undefined for Custom Roles Without Permissions 🟡
Location: Problem: The return profile.role.name === GROUP_ROLES.CUSTOM
? profile.role.permissions
: GROUP_PERMISSIONS_PRESET[profile.role.name]Suggested Fix: return profile.role.name === GROUP_ROLES.CUSTOM
? (profile.role.permissions || [])
: (GROUP_PERMISSIONS_PRESET[profile.role.name] || [])Issue 3: allGroupMemberRolesAndPermissions Returns Undefined Permissions 🟡
Location: Problem: Similar to Issue 2, the permissions: profile.role.name === GROUP_ROLES.CUSTOM
? profile.role.permissions
: GROUP_PERMISSIONS_PRESET[profile.role.name],Suggested Fix: permissions: profile.role.name === GROUP_ROLES.CUSTOM
? (profile.role.permissions || [])
: (GROUP_PERMISSIONS_PRESET[profile.role.name] || []),Issue 4: Computed Property Returns Undefined in EditPermissionsModal 🟡
Location: Problem: permissionsToDisplay () {
return this.config.permissionPresets[this.ephemeral.role]
},Suggested Fix: permissionsToDisplay () {
return this.config.permissionPresets[this.ephemeral.role] || []
},Issue 5: Similar Undefined Access in AddPermissionsListItem 🟡
Location: Problem: this.ephemeral.permissionOptionsToDisplay = GROUP_PERMISSIONS_PRESET[role]Suggested Fix: this.ephemeral.permissionOptionsToDisplay = GROUP_PERMISSIONS_PRESET[role] || []Issue 6: Missing Optional Chaining in updateRole Method 🟡
Location: Problem: Similar to Issue 5, accessing this.ephemeral.selectedPermissions = this.config.permissionPresets[value]Suggested Fix: this.ephemeral.selectedPermissions = this.config.permissionPresets[value] || []Issue 7: Duplicate Permission Preset Definition ⚪️
Location: Problem: Both components define Suggested Improvement: Consider extracting this configuration to a shared utility or constants file. Issue 8: Unused Import ⚪️
Location: Problem: The import {
getRoleDisplayName,
getPermissionDisplayName // unused
} from './permissions-utils.js'Suggested Fix: Remove the unused import. Summary
Review generated using |
|
@SebinSong Above are two AI reviews using the same model but different environments (opencode vs crush), please let me know which one did better, or if they were about the same. |
|
/crush_fast (Sorry @SebinSong, I remembered that there's a bug in crush's default max context size for GLM-5, it's about half of what it should be. That's why I'm re-running the crush review as I've added a fix for that.) AI review started. |
Advanced AI Review
Click to expand reviewCode Review: Add Permissions Feature🔴 Issue 1: Runtime Error in Validation -
|
| Issue | Severity | Confidence | Status |
|---|---|---|---|
1. Runtime error with item.permissions.some() |
🔴 High | High | Needs fix |
2. Potential undefined in Vue template |
🟡 Medium | Medium | Consider fix |
| 3. Missing null check for chatroom | 🟡 Medium | High | Should fix |
4. permissionsToDisplay can return undefined |
🟡 Medium | High | Should fix |
| 5. Misleading error message | 🟡 Medium | High | Consider fix |
| 6. Unused computed property | ⚪️ Low | High | Optional |
| 7. Typo in error message | ⚪️ Low | High | Optional |
| 8. CSS class name typo | ⚪️ Low | High | Optional |
Review generated using glm-5 via Z.AI. Comment /review to re-run.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@taoeffect PR is ready for review again. Personally It actually didn't feel like one was better than the other. They performance seem equal and also have several overlapping items. But it feels like it had multiple items(in both Also, there was one item( But the rest items were helpful in finding several unused codes and typos. Thanks, |
|
/crush_fast {{openrouter, google/gemini-3.1-pro-preview, google/gemini-3-flash-preview}} Thanks @SebinSong! I'm gonna try one more model that's supposed to be better than GLM-5 just in case it's able to find some things those missed. I'll also double-check with @corrideat to see if there's anything he thinks we should wait for before merging this. EDIT: (I will also test these changes manually before merging as well) AI review started. |
Advanced AI Review
Click to expand reviewHere is my review of the PR. I have checked for bugs, security vulnerabilities, edge cases, and areas of improvement, evaluating and prioritizing them by importance. 1. 🔴 Vulnerability in
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
corrideat
left a comment
corrideat
left a comment
There was a problem hiding this comment.
This looks much better. Thank you for addressing the feedback.
One thing I've noticed now is that both in naming and errors there's a lot of personal pronouns (you, my, etc.) This form of address doesn't seem very accurate for checks that everyone is running. Concrete example: someone builds a custom client and attempts to change permissions in a way that's not allowed (say, giving themselves the admin permission). While this should be rejected, it'd be confusing for other people to get a message having 'you' in it. Of course, it's fine to say 'you' in the UI, so long as only the user performing the action receives it.
…es-and-permission-step2
3 participants
Work on step 2 items in this TODO list
This PR is about building this 'Add Permissions' modal and implement various features & updates associated with it.
Permission details set up via this modal will be displayed in the Roles and permissions section in the Group Settings page below.
add permissions button in orange box is only exposed to members who have
delegate-permissionspermission granted, which areadminrole andmoderator(delegator)role.Note: Implementing hide/enable other functionalities depending on granted permissions will be worked in Step 4 here, FYI
For those who don't have
view-permissionspermission granted, This table will only show their own permission details like below:@taoeffect
These two UIs below I built in this PR have no Figma reference actually. (built by combining existing ingredients in the app) Once the project has a designer back, they could be reviewed again and improved. Hope it looks good to you anyways.
EDIT: As part of Step 3 I built below two modals and implement functionalities associated with them
['Remove role' modal]
['Edit permissions' modal]
These are accessed via below menu: