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 13 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:
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 unit tests (beta)
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. |
Review Feedback AddressedCommit: 3a2f7f9 Changes Made
Threads ResolvedAll previously unresolved threads were addressed in prior iterations. This commit resolves the design gap raised by @dealako on PR #1000, which applies equally to this branch. |
The three-line JSDoc said "Redirects to project overview" but the implementation chooses between /foundation/overview and /project/overview based on the active lens. Remove the comment — the method name is self-explanatory and the comment was misleading (per @copilot). LFXV2-2252 Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
Review Feedback AddressedCommit: 5215a48 Changes Made
Threads Resolved1 of 1 unresolved threads addressed in this iteration. |
LFXV2-2252 Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
Review Feedback AddressedChanges Made
Threads Resolved1 of 1 unresolved threads addressed in this iteration. |
Replace the repeated writeFeature triple-OR chain with a single supportsCommitteeWriter boolean so the allowlist is defined once and both check sites stay in sync automatically (per @copilot). LFXV2-2252 Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
…isfy member-ordering lint LFXV2-2252 Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
apps/lfx-one/src/app/modules/votes/components/votes-table/votes-table.component.html:172
- The Edit action now stops event propagation to avoid triggering the table row selection handler, but the adjacent Delete action still allows the click to bubble. Since the table uses
(onRowSelect)to open vote/results, clicking Delete can also trigger row selection/drawer logic in addition to opening the confirm dialog.
</lfx-button>
<lfx-button
label="Delete"
severity="danger"
size="small"
dealako
left a comment
dealako
left a comment
There was a problem hiding this comment.
Follow-up review — round 3 👋
@MRashad26 thanks for the steady, methodical turnaround on this one. Every item from the previous rounds was addressed with a clear commit reference and rationale, and the small refactors (the supportsCommitteeWriter extraction, the fetchCommittee() swap) genuinely improved the change. Reviewed at HEAD 862b0f2f.
👏 Nice work
- Lens-aware deny redirect —
onCreateVote()now mirrorswriterGuard's/foundation/overviewvs/project/overviewselection exactly. Good consistency with the guard's authoritative behavior. fetchCommittee()overgetCommittee()— correctly avoids thetap()side-effect that mutated the sharedCommitteeService.committeesignal for a permission-only check.supportsCommitteeWriterflag — the allowlist (['meetings','surveys','votes']) is now defined once and reused at both check sites; no more drift risk.- End-to-end coherence verified — both
/votes/createand/votes/:id/editare wired withwriterGuard+data.writeFeature: 'votes', and passingeditQueryParams(project + committee_uid) is exactly what lets a committee writer clear the edit guard. The whole flow hangs together. - Fail-closed posture — strict
!== truechecks anderror: () => deny()both default to denial. Correct security stance for a stale-CTA guard.
Revision tracking (prior feedback)
- ✅ Hard-coded
/project/overview→ lens-aware redirect (1b0e9039) - ✅
getCommittee()side-effect →fetchCommittee()(7268ddcb) - ✅
editVoteQueryParamsduplication → aliased tocreateVoteQueryParams(7268ddcb) - ✅ Redundant
.pipe(take(1))removed + unusedtakeimport dropped (97eb97b6) - ✅
onCreateVotepublic→protected(97eb97b6) - ✅ Stale
onCreateVotedoc comment removed (5215a48a) - ✅ Duplicated committee-writer feature check →
supportsCommitteeWriterflag (9c721b7f) - ✅ PR description updated to mention votes-table changes
⚠️ Deny-on-any-error (Copilot) — author declined with sound rationale:fetchCommittee()is a no-retry GET, andwriterGuardre-evaluates on arrival, so a false-deny on a transient error costs only a round-trip, never a lock. Accepted as a deliberate trade-off.- ⚪ CodeRabbit nitpick (loading guard to debounce rapid clicks) — still unaddressed; optional, see nit below.
Open items (all non-blocking)
🟡 Minor (2)
votes-table.component.ts:48—viewVoteResults()ispublicbut only called from the template ((viewResults)/(viewVote)). Same conventiononCreateVotenow follows — making itprotectedresolves the visibility convention and removes the reason the last commit (862b0f2f) had to reorder it for member-ordering lint. (committee-votes.component.ts:48 — note the file is committee-votes, not votes-table.)- Test coverage — no specs exist for
committee-votes.component.ts,votes-table.component.ts, orwriter.guard.ts. The new branching is worth pinning, especiallywriterGuard'ssupportsCommitteeWritergate and theproject === null → checkCommittee()salvage path, plusonCreateVote's allow/deny/error paths. Flagged minor since the repo has no pre-existing specs for these files, but it's the highest-value gap.
⚪ Nit (3)
3. votes-table.component.html:174 — the new Edit button correctly adds (onClick)="$event.stopPropagation()" to avoid double-firing onRowSelect, but sibling action buttons (Delete/View/View Results) don't. For Delete this means the results/vote drawer opens behind the confirm dialog. Pre-existing for those buttons and outside this PR's diff — agreeing with Copilot's low-confidence note — worth a follow-up for consistency, not a blocker here.
4. committee-votes.component.ts:57 vs writer.guard.ts:56 — the component derives the lens from lensService.activeLens() while the guard reads route.data['lens']. Equivalent in practice; a one-line comment noting the intentional mirroring would prevent future divergence.
5. committee-votes.component.ts:46 — editVoteQueryParams aliasing createVoteQueryParams is fine, but the distinct name implies they might differ; a short comment that edit intentionally reuses create params would clarify intent.
AI reconciliation
- Copilot (06-29 16:46) — clean, "generated no new comments." ✅ Agree.
- Copilot low-confidence (Delete stopPropagation) — valid; captured as nit #3 with the pre-existing caveat.
- CodeRabbit (loading guard) — take-it-or-leave-it; concurrent clicks each spawn a self-completing
fetchCommittee(), so no correctness issue, only a minor UX debounce. Author's call.
Security
No blocking findings. The client-side check is correctly a UX/stale-CTA guard layered atop the authoritative writerGuard + BFF enforcement; the allowlist extension is properly gated on committee_uid presence; fail-closed throughout. No injection, secrets, or sensitive-PII exposure (project slug + committee_uid in query params are non-PID routing identifiers, consistent with existing conventions).
Decision: ✅ Approved with minor comments
No blocking issues, no merge conflicts, no actionable security findings. The two minor items (protected visibility + test coverage) and three nits are safe to address in a quick follow-up or fast-follow. Note the branch is BEHIND main — a rebase/merge before merging is needed, but that's mechanical.
…buttons Add `creating = signal(false)` pending flag to CommitteeVotesComponent — mirrors the same pattern from CommitteeSurveysComponent. Set before fetchCommittee() and cleared in finalize() so both next-deny and error paths reset it. Also fix both Create Vote buttons: (click) → (onClick) (ButtonComponent emits (onClick), not a native click event) and bind [loading]="creating()" for visual feedback and re-entry prevention. Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
|
Follow-up PR #1042 opened to bring |
Review Feedback AddressedAll threads from previous review iterations are resolved. Summary of changes on this branch
Note: The latest commit ( Thanks again, @dealako! |
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 (/foundation/overviewor/project/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): enforce fresh committee writer checks for create/edit flows #1000 (surveys).writerGuardextended for votes:writer.guard.tsnow acceptscommittee.writeras a passing condition forwriteFeature: 'votes'(and'surveys'), matching the existing'meetings'fast-path. A committee Manager who is not a project writer can now reach/votes/createwithout a double-redirect from the client-side permission check.writerGuardon/votes/createremains as the final safety net for direct URL access.Changed files
committee-votes.component.tsCommitteeService,LensService+Router; addprotected onCreateVote()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)writer.guard.tswriteFeature: 'votes'and'surveys'(was'meetings'only)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 lens-appropriate overview with an "Access Denied" toast/foundation/overview(not/project/overview)/votes/create?committee_uid=...—writerGuardnow acceptscommittee.writerand allows access/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