Skip to content

feat(team): Role in team members list#6423

Merged
ssiyad merged 12 commits into
developfrom
feat/team/members_role_column
May 14, 2026
Merged

feat(team): Role in team members list#6423
ssiyad merged 12 commits into
developfrom
feat/team/members_role_column

Conversation

@ssiyad

@ssiyad ssiyad commented May 13, 2026

Copy link
Copy Markdown
Member

No description provided.

@ssiyad ssiyad requested review from shadrak98 and siduck as code owners May 13, 2026 08:29
@greptile-apps

greptile-apps Bot commented May 13, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR surfaces the team member role field in the dashboard's Team settings page, replacing the previously hardcoded "Developer" placeholder with the actual stored value and a live role-change Select dropdown. A data-migration patch back-fills NULL roles on existing rows, and a new perm_team_members guard in Team.before_validate prevents unauthorized role changes server-side.

  • get_members query now selects Member.role from the DB instead of emitting a static ValueWrapper(\"Developer\"); get_roles gains a RoleDict TypedDict and proper return type.
  • perm_team_members + validate_member_role are added to Team; the permission guard mirrors the existing perm_relaxed_roles pattern.
  • Frontend adds updateTeam (set_value) and updateRole to wire the Select dropdown to the API; the patch registers team_member_default_role in patches.txt.

Confidence Score: 4/5

Safe to merge after adding an admin guard to the Role Select in Team.vue; the silent failure path for non-admin users is the main concern.

The backend permission checks work correctly for admins and owners. The gap is in the frontend: the role Select is rendered for every active member regardless of whether they have permission to change it. When a non-admin clicks to change a role, the server returns a PermissionError that updateTeam never surfaces.

dashboard/src/components/settings/Team.vue — the Role column's Select needs a session.isTeamAdmin guard matching the pattern already used by rowActions.

Important Files Changed

Filename Overview
dashboard/src/components/settings/Team.vue Adds role select dropdown to team member list; introduces updateRole/updateTeam resources. Role Select is shown to all active members with no admin guard, causing a silent failure for non-admins.
press/press/doctype/team/team.py Adds perm_team_members guard and validate_member_role validation. validate_member_role runs unconditionally on every save rather than only when team_members changes.
press/press/doctype/team/team_members.py get_members now reads actual role from DB instead of hardcoding "Developer"; get_roles gains RoleDict TypedDict and proper type hint. Clean change.
press/patches/v0_8_0/team_member_default_role.py Back-fills NULL role values on existing Team Member rows to "Developer"; correctly uses frappe.qb and explicit commit.
press/press/doctype/team_member/team_member.json Adds role field as Data type with reqd=1 and default "Developer"; role is free-text rather than Select, so invalid values are only caught server-side.
press/press/doctype/team_member/team_member.py Adds role: DF.Data to auto-generated type stubs; removes legacy encoding comment. Straightforward.
press/patches.txt Registers the new team_member_default_role patch in the correct position.
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
dashboard/src/components/settings/Team.vue:150-172
**Role Select rendered for non-admin members with no feedback on failure**

The Role column always renders an interactive `Select` for active members, but `rowActions` at line 202 correctly gates admin-only actions behind `session.isTeamAdmin`. A non-admin team member can open the role dropdown, pick a new value, and trigger `updateTeam.submit`. The server will throw a `PermissionError` via `perm_team_members`, but `updateTeam` has no `onError` handler, so the failure is completely silent — no toast, no revert indicator. The `members.fetch()` in `onSuccess` is never called, leaving the user with no acknowledgement of what happened. The role Select should be replaced with a read-only `Badge` (similar to the Pending branch) when the current user is not a team admin.

### Issue 2 of 2
press/press/doctype/team/team.py:282-290
`validate_member_role` runs on every `Team.validate()` call regardless of whether `team_members` actually changed. This means saves triggered by billing updates, partner-email changes, or any other unrelated field will re-validate all member roles. If a row somehow carries an invalid role value (direct DB edit, future import), it will block all subsequent team saves permanently — not just role-change saves. Adding the same `has_value_changed` guard used by `perm_team_members` keeps the validation scoped to the affected field.

```suggestion
	def validate_member_role(self):
		"""
		Validate that the role assigned to each team member is a valid role.
		This is to prevent any issues with role-based access control and ensure
		that team members have the correct permissions based on their assigned
		roles.
		"""
		if not self.has_value_changed("team_members"):
			return
		# Get a list of valid roles for this team.
		roles = [role["label"] for role in get_roles(self.name)]
```

Reviews (3): Last reviewed commit: "fix(team): Skip `team_members` perm chec..." | Re-trigger Greptile

Comment thread press/press/doctype/team_member/team_member.json
Comment thread press/press/doctype/team_member/team_member.json
Comment thread dashboard/src/components/settings/Team.vue
@ssiyad ssiyad marked this pull request as draft May 13, 2026 08:35
@codecov

codecov Bot commented May 13, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.91525% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.47%. Comparing base (8d03eb1) to head (aa586a6).
⚠️ Report is 5 commits behind head on develop.

Files with missing lines Patch % Lines
press/press/doctype/team/team.py 83.33% 4 Missing ⚠️
press/press/doctype/team/test_team_members.py 97.72% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #6423      +/-   ##
===========================================
+ Coverage    49.79%   56.47%   +6.68%     
===========================================
  Files          945      945              
  Lines        78270    78333      +63     
  Branches       355      509     +154     
===========================================
+ Hits         38972    44237    +5265     
+ Misses       39275    34072    -5203     
- Partials        23       24       +1     
Flag Coverage Δ
dashboard 89.70% <ø> (+29.96%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ssiyad ssiyad marked this pull request as ready for review May 13, 2026 09:26
Comment thread press/press/doctype/team/team.py
@ssiyad

ssiyad commented May 13, 2026

Copy link
Copy Markdown
Member Author

@greptileai rereview

@ssiyad ssiyad force-pushed the feat/team/members_role_column branch from 516f2d7 to 2210451 Compare May 14, 2026 05:04
@ssiyad ssiyad force-pushed the feat/team/members_role_column branch from f91db89 to 1021216 Compare May 14, 2026 05:41
@ssiyad ssiyad merged commit c4f086e into develop May 14, 2026
14 of 15 checks passed
@ssiyad ssiyad deleted the feat/team/members_role_column branch May 14, 2026 06:26
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.

1 participant