feat(votes): add permission check on committee Create Vote CTA click#997
feat(votes): add permission check on committee Create Vote CTA click#997MRashad26 wants to merge 6 commits into
Conversation
Replace routerLink on both Create Vote buttons (table-actions slot and empty-state) with an onCreateVote() click handler that fetches fresh committee permissions before navigating. If the member's role was downgraded from Manager to Member since the page loaded, the stale canEdit() signal would still show the button; the click handler catches this and redirects to /project/overview?_notice=votes so AppComponent shows the "Access Denied" toast — consistent with the writerGuard denial flow used for meetings. Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughCommittee vote creation now uses a click handler that rechecks committee write access before routing, and the votes table edit action now forwards query parameters through a new input. ChangesVote navigation updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/lfx-one/src/app/modules/committees/components/committee-votes/committee-votes.component.ts (1)
45-67: 💤 Low valueConsider disabling the button during the permission check to prevent duplicate requests.
While the implementation correctly fetches fresh permissions and handles denial/error cases, rapid clicks could trigger multiple concurrent API calls before navigation completes. Adding a brief loading state would improve UX.
♻️ Optional: Add loading guard
+ public creatingVote = signal<boolean>(false); + public onCreateVote(): void { + if (this.creatingVote()) return; + this.creatingVote.set(true); const committee = this.committee(); const denyParams: Record<string, string> = { _notice: 'votes' }; if (committee.project_slug) denyParams['project'] = committee.project_slug; - const deny = () => void this.router.navigate(['/project/overview'], { queryParams: denyParams }); + const deny = () => { + this.creatingVote.set(false); + void this.router.navigate(['/project/overview'], { queryParams: denyParams }); + }; this.committeeService .getCommittee(committee.uid) .pipe(take(1)) .subscribe({ next: (fresh) => { if (fresh?.writer !== true) { deny(); return; } void this.router.navigate(['/votes', 'create'], { queryParams: this.createVoteQueryParams() }); }, error: () => deny(), }); }Then in template:
[disabled]="creatingVote()"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/lfx-one/src/app/modules/committees/components/committee-votes/committee-votes.component.ts` around lines 45 - 67, To prevent duplicate API requests from rapid button clicks during the permission check in the onCreateVote method, add a loading state signal (named something like creatingVote) initialized to false. Set this signal to true before the getCommittee API call starts subscribing, then set it back to false in both the next and error callbacks of the subscribe handler to ensure it resets regardless of outcome. Finally, update the template to disable the button by binding [disabled]="creatingVote()" to the create vote button element.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@apps/lfx-one/src/app/modules/committees/components/committee-votes/committee-votes.component.ts`:
- Around line 45-67: To prevent duplicate API requests from rapid button clicks
during the permission check in the onCreateVote method, add a loading state
signal (named something like creatingVote) initialized to false. Set this signal
to true before the getCommittee API call starts subscribing, then set it back to
false in both the next and error callbacks of the subscribe handler to ensure it
resets regardless of outcome. Finally, update the template to disable the button
by binding [disabled]="creatingVote()" to the create vote button element.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 98e32e74-2730-4773-8207-41ce2e0eecd4
📒 Files selected for processing (2)
apps/lfx-one/src/app/modules/committees/components/committee-votes/committee-votes.component.htmlapps/lfx-one/src/app/modules/committees/components/committee-votes/committee-votes.component.ts
🚀 Deployment StatusYour branch has been deployed to: https://ui-pr-997.dev.v2.cluster.linuxfound.info Deployment Details:
The deployment will be automatically removed when this PR is closed. |
There was a problem hiding this comment.
Pull request overview
Adds a “fresh permission” check when a user clicks the Committee “Create Vote” CTA so that if their committee writer access is revoked after page load, they’re redirected through the same _notice=votes access-denied toast flow rather than relying solely on the route guard.
Changes:
- Added
onCreateVote()to re-fetch committee permissions viaCommitteeService.getCommittee()before navigating to the create vote route. - Updated both “Create Vote” buttons to call
(click)="onCreateVote()"instead of using[routerLink]+[queryParams].
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| apps/lfx-one/src/app/modules/committees/components/committee-votes/committee-votes.component.ts | Adds click handler that re-checks committee writer permission and redirects with _notice=votes on denial. |
| apps/lfx-one/src/app/modules/committees/components/committee-votes/committee-votes.component.html | Routes both CTAs through the new click handler. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…rect Inject LensService and derive the overview path from the active lens so the access-denied redirect lands on /foundation/overview when the component is rendered under the foundation lens (/foundation/groups/...) rather than always redirecting to /project/overview — consistent with writerGuard's lens-aware denial behavior (per @copilot-pull-request-reviewer). Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
Review Feedback AddressedCommit: 1b0e903 Changes Made
No Change Needed
Threads Resolved1 of 1 unresolved threads addressed in this iteration. Follow-up
|
|
@MRashad26 I've opened a new pull request, #998, to work on those changes. Once the pull request is ready, I'll request review from you. |
Add editQueryParams input to lfx-votes-table and wire [queryParams] onto the edit routerLink so writerGuard receives project and committee_uid when a committee manager navigates to edit a draft vote — mirrors the meetings fix (PR #1025). - votes-table: add editQueryParams input (defaults to {}; non-breaking for all existing usages outside committee context) - votes-table: bind [queryParams]="editQueryParams()" on edit <a> - committee-votes: add editVoteQueryParams computed signal via buildCommitteeCreateQueryParams; pass to [editQueryParams] binding Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
Edit permission handling added (commit `e4839f644`)Follow-up to the create-vote permission check: added the same committee context to the Edit button navigation, consistent with the meetings fix (PR #1025). Changes
EffectWhen a committee manager clicks Edit on a draft vote in the committee tab, the navigation to |
Audit: PR #997 — feat(votes): add permission check on committee Create Vote CTA clickMRashad26 · Scope: Fresh Lanes run: 1. Why this changeCreate Vote stays visible via stale 2. How it was done
3. Is it correctYes — merge-ready. Gates green in isolated worktree. Prior review: Lens hard-code → fixed (1b0e903). CodeRabbit loading guard → open (optional UX nit). Copilot duplicate-computed / error-path threads → open (non-blocking). 4. Missing / gaps
5. Q&A this PR resolves
6. Business rules & ADRs
7. Open questions & confirmations
Verdict: 🟡 Minor comments |
ahmedomosanya
left a comment
ahmedomosanya
left a comment
There was a problem hiding this comment.
Audit result: 🟡 Minor comments
The referenced Jira looks too broad for this PR. LFXV2-2252 is the parent epic for Org Lens parity; the specific ticket appears to be LFXV2-2470 (“Allow committee writers to create/manage votes for their committee”). Please update the PR description to reference LFXV2-2470 directly, optionally keeping LFXV2-2252 as the parent epic.
A few non-blocking implementation notes:
onCreateVote()callsCommitteeService.getCommittee(), which mutates shared committee service state viatap(). Since this is only a permission probe,fetchCommittee()is the safer no-side-effect API.- The error path treats every
getCommittee()failure as access denied. A transient 5xx/network failure would redirect with_notice=votes, which can mislead a legitimate writer. - The broader permission contract still needs confirmation:
writerGuardaccepts committee writer access only for meetings today; votes still require project writer access.
Local checks passed: yarn lint:check, yarn check-types, and yarn format:check.
Address Copilot review comments: - committee-votes.component.ts: switch getCommittee() to fetchCommittee() in onCreateVote() to avoid mutating shared committee service state during a permission-only check (per T1) - committee-votes.component.ts: alias editVoteQueryParams to createVoteQueryParams instead of duplicating the computed (per T3) - votes-table.component.html: replace <a> wrapper with direct [routerLink] and [queryParams] inputs on <lfx-button> — ButtonComponent already supports these inputs; wrapper created invalid nested interactive elements - votes-table.component.html: add (onClick)=stopPropagation to prevent row-select from firing alongside edit route navigation in selectionMode table - votes-table.component.ts: remove now-unused RouterLink import Resolves 3 review threads (T1, T3, implicit <a> wrapper issue). Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
Review Feedback AddressedCommit: 7268ddc Changes Made
Questions Answered
Threads Resolved3 of 4 unresolved threads resolved (T1, T3, T4). T2 left open for discussion — no code change taken. |
dealako
left a comment
dealako
left a comment
There was a problem hiding this comment.
@MRashad26 — solid iteration here. The lens-aware redirect, the fetchCommittee() switch, the editVoteQueryParams alias, and the PR description update all landed cleanly. The permission-gate pattern matches the meetings approach well.
Two open Copilot comments from the latest review round still need a commit:
🔴 Blocking: 0 issues
🟡 Minor: 2 issues — redundant take(1) + unused take import (Copilot flagged both; not yet addressed)
⚪ Nit: 2 issues — double-click guard, public → protected visibility on onCreateVote
Revision tracking — previous rounds:
- ✅ Hard-coded
/project/overview→ fixed (LensService injection, commit1b0e903) - ✅
getCommittee()tap side-effect → switched tofetchCommittee()(commit7268ddcb) - ✅
editVoteQueryParamsduplication → aliased (commit7268ddcb) - ✅ PR description missing votes-table context → updated
- ✅ Error-path treat-all-as-denied → accepted trade-off, reasoning documented (consistent with other LFXV2-2252 components)
Context note (no code change needed): The writerGuard for /votes/create uses writeFeature: 'votes' with no committee.writer fast-path (only project.writer passes). So the fresh check in onCreateVote() catches revoked committee writers and denies immediately; project-writer-only denials still flow through the guard redirect. Defense-in-depth is intact. Worth tracking as a follow-up if committee writers should be able to create votes without project.writer.
🔴 Needs changes before approval — two minor Copilot comments in a single fix(review): commit, then this is ready.
Address review comments from @dealako and @copilot: - committee-votes.component.ts: remove redundant .pipe(take(1)) from onCreateVote() — fetchCommittee() already applies take(1) internally (CommitteeService line 77), so the extra pipe was misleading - committee-votes.component.ts: drop now-unused `take` from RxJS imports - committee-votes.component.ts: change onCreateVote() from public to protected — it is only called from the template (Angular 20 convention) Resolves 5 review threads (4 from @dealako + @copilot on take(1)/import, 1 from @dealako on public/protected). Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
Review Feedback Addressed (Round 2)Commit: 97eb97b Changes Made
Threads Resolved5 of 5 unresolved threads resolved in this iteration. |
Summary
canEdit()— derived fromcommittee.writerat page-load time. If the member's role is downgraded from Manager to Member after the page loads, the stale signal still shows the button. Clicking it without this fix would navigate directly to/votes/createand rely solely on thewriterGuardredirect, with no immediate UI feedback.onCreateVote(), which fetches fresh committee permissions viafetchCommittee()before navigating. On denial, redirects to the lens-appropriate overview with_notice=votessoAppComponent.initAccessDeniedToast()shows the contextual "Access Denied" toast — consistent with thewriterGuarddenial flow from feat(meetings): add access-denied toast and fix meeting coordinator permissions #992.votes-tablenow passescommittee_uidandprojectquery params so the edit route has the same context as create — consistent with fix(meetings): pass project+committee_uid queryParams on meeting edit navigation #1025 (meetings) and feat(surveys): add permission check on committee Create Survey CTA click #1000 (surveys).writerGuardon/votes/createremains as the final safety net for direct URL access.Changed files
committee-votes.component.tsCommitteeService,LensService+Router; addonCreateVote()with freshfetchCommittee()permission check and lens-aware deny redirect; addeditVoteQueryParamsaliased fromcreateVoteQueryParamscommittee-votes.component.html[routerLink]+[queryParams]on both Create Vote buttons with(click)="onCreateVote()"; pass[editQueryParams]tolfx-votes-tablevotes-table.component.tseditQueryParamsinput (default{})votes-table.component.html[routerLink],[queryParams], and(onClick)=stopPropagationdirectly on the Edit<lfx-button>(no<a>wrapper)References
committee-meetings.component.tsfrom feat(meetings): add access-denied toast and fix meeting coordinator permissions #992Test plan
/votes/createwithcommittee_uidandprojectquery paramscanEdit()) but clicking it redirects to the project overview with an "Access Denied" toast/votes/create?committee_uid=...—writerGuardblocks and shows the toast/votes/<uid>/edit?committee_uid=...&project=<slug>and does not also trigger the row-results drawer