feat(surveys): add permission check on committee Create Survey CTA click#1000
feat(surveys): add permission check on committee Create Survey CTA click#1000MRashad26 wants to merge 5 commits into
Conversation
Replace routerLink on both Create Survey buttons (table-actions slot and empty-state) with an onCreateSurvey() click handler that fetches fresh committee permissions via getCommittee() 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 the lens-appropriate overview (/foundation/overview or /project/overview) with _notice=surveys so AppComponent shows the "Access Denied" toast — consistent with the writerGuard denial flow. Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughCreate Survey actions now run through a permission check before navigation, and the surveys table edit action now uses router links with configurable query parameters. ChangesSurvey navigation and edit routing 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 |
There was a problem hiding this comment.
Pull request overview
Adds a “fresh permission” check to the Committee Surveys tab’s Create Survey CTA so that stale, page-load-time canEdit() state can’t incorrectly allow navigation when committee write access has been revoked mid-session. This aligns the UX with the existing writerGuard denial flow by redirecting with _notice=surveys for the Access Denied toast, and makes the redirect lens-aware (foundation vs project).
Changes:
- Introduces
onCreateSurvey()that re-fetches committee permissions viaCommitteeService.getCommittee()before routing to/surveys/create. - On denied/errored permission refresh, redirects to
/foundation/overviewor/project/overviewwith_notice=surveys(andprojectwhen available) to trigger the standard Access Denied toast. - Replaces
[routerLink]navigation on both Create Survey buttons with the new click handler.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| apps/lfx-one/src/app/modules/committees/components/committee-surveys/committee-surveys.component.ts | Adds fresh committee permission check on CTA click and lens-aware denial redirect before navigating to survey creation. |
| apps/lfx-one/src/app/modules/committees/components/committee-surveys/committee-surveys.component.html | Routes Create Survey CTAs through (click)="onCreateSurvey()" instead of direct routerLink navigation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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-surveys/committee-surveys.component.ts (1)
47-70: Button double-click during permission check — consistent with meetings component pattern.The
onCreateSurvey()method implements the same permission check pattern as theonScheduleMeeting()method in the meetings component, and both lack loading state protection. While multiple API calls won't cause functional issues (Angular Router handles duplicate navigations gracefully), consider adding a loading signal to disable the button during the check for better UX consistency.🤖 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-surveys/committee-surveys.component.ts` around lines 47 - 70, The onCreateSurvey method lacks loading state protection, allowing users to trigger multiple API calls by double-clicking the button during the permission check. Add a loading signal (similar to the meetings component's onScheduleMeeting pattern) that is set to true before initiating the getCommittee call and set to false in both the next and error callback handlers. Bind this loading signal to disable the create survey button to prevent duplicate submissions during the permission verification.
🤖 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-surveys/committee-surveys.component.ts`:
- Around line 47-70: The onCreateSurvey method lacks loading state protection,
allowing users to trigger multiple API calls by double-clicking the button
during the permission check. Add a loading signal (similar to the meetings
component's onScheduleMeeting pattern) that is set to true before initiating the
getCommittee call and set to false in both the next and error callback handlers.
Bind this loading signal to disable the create survey button to prevent
duplicate submissions during the permission verification.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 29baa182-25ea-4fbb-84d3-b3921f65a3a1
📒 Files selected for processing (2)
apps/lfx-one/src/app/modules/committees/components/committee-surveys/committee-surveys.component.htmlapps/lfx-one/src/app/modules/committees/components/committee-surveys/committee-surveys.component.ts
🚀 Deployment StatusYour branch has been deployed to: https://ui-pr-1000.dev.v2.cluster.linuxfound.info Deployment Details:
The deployment will be automatically removed when this PR is closed. |
Wire up the non-functional survey Edit button and pass project + committee_uid queryParams so writerGuard receives the committee context when a committee manager navigates to edit a draft survey — mirrors the meetings fix (PR #1025). - surveys-table: add RouterLink import + editQueryParams input (defaults to {}; non-breaking for all existing usages outside committee context) - surveys-table: wrap Edit button in <a [routerLink]="/surveys/:id/edit"> and bind [queryParams]="editQueryParams()" (button was previously non-functional with no routerLink) - committee-surveys: add editSurveyQueryParams computed signal via buildCommitteeCreateQueryParams; pass to [editQueryParams] binding Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
Edit permission handling added (commit `867a750e7`)Follow-up to the create-survey permission check: wired up the non-functional Edit button and added committee context to its navigation, consistent with the meetings fix (PR #1025). Changes
EffectWhen a committee manager clicks Edit on a draft survey in the committee tab, the navigation to |
Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In
`@apps/lfx-one/src/app/modules/surveys/components/surveys-table/surveys-table.component.html`:
- Around line 182-185: The Edit action inside the selectable row is bubbling the
click into the row selection flow, which triggers
SurveysTableComponent.onRowSelect and the rowClick handler in
committee-surveys.component.html. Update the Edit link/button in
surveys-table.component.html to stop event propagation before navigation so
clicking Edit only follows the edit route and does not also fire the row
navigation. Use the existing routerLink and lfx-button markup as the place to
apply the fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 45b7918d-debb-446b-a7fe-4dc2e4cd61a0
📒 Files selected for processing (4)
apps/lfx-one/src/app/modules/committees/components/committee-surveys/committee-surveys.component.htmlapps/lfx-one/src/app/modules/committees/components/committee-surveys/committee-surveys.component.tsapps/lfx-one/src/app/modules/surveys/components/surveys-table/surveys-table.component.htmlapps/lfx-one/src/app/modules/surveys/components/surveys-table/surveys-table.component.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/lfx-one/src/app/modules/committees/components/committee-surveys/committee-surveys.component.ts
Address review comments from CodeRabbit (PRRT_kwDOPMiRQ86MjyOQ): - committee-surveys.component.ts: switch getCommittee() to fetchCommittee() in onCreateSurvey() to avoid unintended shared service state mutation during permission-only checks - committee-surveys.component.ts: alias editSurveyQueryParams to createSurveyQueryParams instead of duplicating the computed - surveys-table.component.html: add stopPropagation on edit anchor to prevent row-select firing alongside route navigation in selectionMode table Resolves 1 review thread. Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
Audit: PR #1000 — feat(surveys): add permission check on committee Create Survey CTA clickMRashad26 · Scope: Fresh Lanes run: 1. Why this changeSame stale- 2. How it was done
3. Is it correctYes — merge-ready. Gates green. Edit click does not bubble to Prior review: CodeRabbit loading guard → open (optional). Duplicate-computed thread → open (non-blocking). PR test-plan foundation-lens checkbox → unchecked in body. 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: 🔴 Required changes
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-2471 (“Allow committee writers to create/manage surveys for their committee”). Please update the PR description to reference LFXV2-2471 directly, optionally keeping LFXV2-2252 as the parent epic.
Blocking issue:
- The Survey Edit action still wraps
<lfx-button>in an<a [routerLink]>inside a selectable table row. The latest commit addedstopPropagation()and switched the permission probe tofetchCommittee(), which addresses part of the earlier feedback, but the nested interactive markup remains invalid HTML/accessibility-wise.lfx-buttonalready supports[routerLink]and[queryParams]; please move those bindings directly ontolfx-button.
Additional notes:
- The error path still treats every fetch failure as access denied, which can misreport transient failures as permission loss.
- The broader permission contract still needs confirmation:
writerGuardaccepts committee writer access only for meetings today; surveys still require project writer access.
Local checks passed: yarn lint:check, yarn check-types, and yarn format:check.
Review Feedback AddressedCommits: 443bff8, 333d82967 Changes Made
Threads Resolved3 of 3 unresolved threads addressed in this iteration. |
| <a [routerLink]="['/surveys', survey.uid, 'edit']" [queryParams]="editQueryParams()" (click)="$event.stopPropagation()"> | ||
| <lfx-button label="Edit" severity="primary" size="small" [text]="true" [attr.data-testid]="'surveys-edit-' + rowId"> </lfx-button> | ||
| </a> |
dealako
left a comment
dealako
left a comment
There was a problem hiding this comment.
Hey @MRashad26 — solid direction here. The lens-aware deny path is a genuine improvement over the meetings pattern, and fetchCommittee() (vs getCommittee()) is the right choice to avoid mutating shared state. Two items need attention before this is ready to merge.
🔴 Blocking: 2 issues
- Nested
<a>+<lfx-button>— invalid HTML, accessibility violation (see inline) - Client/server permission scope mismatch — committee-only writer passes the client gate, gets denied by
writerGuard(see inline)
🟡 Minor: 3 issues
- No loading/disabled state during async
fetchCommittee()call — double-click triggers multiple in-flight requests - Multi-line JSDoc on
onCreateSurvey()violates the repo "one short line max" rule - Redundant
take(1)—fetchCommittee()already pipestake(1)internally
⚪ Nit: 1 issue
RouterLinkimport insurveys-table.component.tsbecomes orphaned once the<a>wrapper is removed
🔴 Needs changes before approval
Revision history
| Round | Finding | Status |
|---|---|---|
| Prior | CodeRabbit: missing stopPropagation on Edit link |
✅ Addressed (via (click) on <a>) |
| Prior | Copilot: <a> wrapping <lfx-button> — nested interactive elements |
333d82967 referenced in your comment does not appear in the branch; <a> wrapper is still present |
| Prior | Copilot: editSurveyQueryParams duplication |
✅ Resolved (correctly aliased) |
| @if (hasPMOAccess() && survey.displayStatus === SurveyStatus.DRAFT) { | ||
| <lfx-button label="Edit" severity="primary" size="small" [text]="true" styleClass="" [attr.data-testid]="'surveys-edit-' + rowId"> | ||
| </lfx-button> | ||
| <a [routerLink]="['/surveys', survey.uid, 'edit']" [queryParams]="editQueryParams()" (click)="$event.stopPropagation()"> |
There was a problem hiding this comment.
[blocking] The <a> wrapper around <lfx-button> creates nested interactive elements — invalid per the HTML spec and a screen-reader/focus hazard.
ButtonComponent already exposes [routerLink], [queryParams], and (onClick) inputs (button.component.ts lines 53–57), so the wrapper is unnecessary.
<!-- Before -->
<a [routerLink]="['/surveys', survey.uid, 'edit']" [queryParams]="editQueryParams()" (click)="$event.stopPropagation()">
<lfx-button label="Edit" ...> </lfx-button>
</a>
<!-- After -->
<lfx-button
label="Edit"
severity="primary"
size="small"
[text]="true"
[routerLink]="['/surveys', survey.uid, 'edit']"
[queryParams]="editQueryParams()"
(onClick)="$event.stopPropagation()"
[attr.data-testid]="'surveys-edit-' + rowId">
</lfx-button>Also remove the now-unused RouterLink import from surveys-table.component.ts (CodeRabbit had flagged this already; Copilot left two unresolved comments on it — this needs to land).
| .pipe(take(1)) | ||
| .subscribe({ | ||
| next: (fresh) => { | ||
| if (fresh?.writer !== true) { |
There was a problem hiding this comment.
[blocking] Client gates on fresh?.writer !== true (committee writer), but the server-side writerGuard for writeFeature: 'surveys' only accepts project.writer — the committee-writer code path in the guard is gated to 'meetings' only.
Consequence: a committee Manager who is not a project writer will pass this client check, then be denied by writerGuard and shown the access-denied toast — a confusing double-redirect UX loop that makes it look like the permission check is broken.
Two options:
- Align the client check to
project.writer— gate here onfresh?.project?.writer === true(or equivalent field on the committee response) to match the server rule. - Update
writerGuardto accept committee writers for surveys — if the intent is that committee writers should be able to create surveys, extend the guard accordingly.
Worth confirming which is the intended design with the team before the fix (ref LFXV2-2252).
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.onCreateSurvey(), which fetches fresh committee permissions viagetCommittee()before navigating. On denial, redirects to the lens-appropriate overview (/foundation/overviewor/project/overview) with_notice=surveyssoAppComponent.initAccessDeniedToast()shows the "Access Denied" toast — consistent with thewriterGuarddenial flow and lens-aware (unlike the equivalent fix in feat(meetings): add access-denied toast and fix meeting coordinator permissions #992 for meetings).writerGuardon/surveys/createremains as the final safety net for direct URL access.Changed files
committee-surveys.component.tsCommitteeService,LensService,Router; addonCreateSurvey()click handler with fresh permission check and lens-aware deny redirectcommittee-surveys.component.html[routerLink]+[queryParams]on both Create Survey buttons with(click)="onCreateSurvey()"References
committee-votes(PR feat(votes): add permission check on committee Create Vote CTA click #997) andcommittee-meetings(PR feat(meetings): add access-denied toast and fix meeting coordinator permissions #992)Test plan
/surveys/createwithcommittee_uidandprojectquery paramscanEdit()) but clicking it redirects to the project overview with an "Access Denied" toast/foundation/overview, not/project/overview/surveys/create?committee_uid=...—writerGuardblocks and shows the toast