feat(akrites): implement stewardship assignment with real member picker#1001
Open
gaspergrom wants to merge 4 commits into
Open
feat(akrites): implement stewardship assignment with real member picker#1001gaspergrom wants to merge 4 commits into
gaspergrom wants to merge 4 commits into
Conversation
Replace placeholder userId text input in assign-steward modal with a searchable steward picker that loads from the LFX Akrites committee endpoint (GET /api/committees/:id/members). - Add AkritesSearchStewardResult interface and AKRITES_STEWARD_COMMITTEE_UID constant - Update AkritesAssignStewardRequest with required username/displayName fields - Update CdpStewardSummary and AkritesSteward with username/displayName fields - Update backend validation helper to require username and displayName - Update akrites.service (server) mapStewards to populate username/displayName - Add searchStewards() to frontend AkritesService with CommitteeMember mapping - Rewrite assign-steward modal: member list with avatar initials, search filter, role selection, and moveToAssessing checkbox — all driven by signals - Wire assign-steward modal into package drawer with Assign/Reassign button visible for unassigned/open/assessing/escalated/inactive statuses - Wire assign-steward modal into triage tab for unassigned and inactive columns Signed-off-by: Gašper Grom <gasper.grom@gmail.com>
Signed-off-by: Gašper Grom <gasper.grom@gmail.com>
Contributor
There was a problem hiding this comment.
Copilot encountered an error: Your billing is not configured or you have Copilot licenses from multiple standalone organizations or enterprises. To use premium requests, select a billing entity via the GitHub site, under Settings > Copilot > Features.
Contributor
|
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)
WalkthroughThe PR refactors the assign-steward modal from reactive forms to Angular signals, adds a searchable steward picker that fetches live committee members, and integrates the modal into both the triage tab and package drawer with full assignment confirmation flows. Shared interfaces, server validation, and server-side mapping are extended to carry ChangesAkrites Steward Assignment Flow
Sequence Diagram(s)sequenceDiagram
participant User
participant AkritesTriageTabComponent
participant AssignStewardModal
participant AkritesService as AkritesService (frontend)
rect rgba(100, 149, 237, 0.5)
note over User, AssignStewardModal: Modal open + steward search
User->>AkritesTriageTabComponent: clicks action (unassigned/inactive pkg)
AkritesTriageTabComponent->>AssignStewardModal: open (assignModalVisible=true)
AssignStewardModal->>AkritesService: searchStewards()
AkritesService-->>AssignStewardModal: AkritesSearchStewardResult[]
User->>AssignStewardModal: types search query
AssignStewardModal->>AssignStewardModal: filteredStewards recomputed
User->>AssignStewardModal: selects steward row
AssignStewardModal->>AssignStewardModal: selectedSteward signal set
end
rect rgba(60, 179, 113, 0.5)
note over User, AkritesService: Assignment confirmation
User->>AssignStewardModal: clicks Assign steward
AssignStewardModal->>AkritesTriageTabComponent: emits AkritesAssignStewardRequest
alt pkg already has stewardshipId
AkritesTriageTabComponent->>AkritesService: assignSteward(stewardshipId, body)
else no stewardshipId
AkritesTriageTabComponent->>AkritesService: openStewardship(purl)
AkritesService-->>AkritesTriageTabComponent: new stewardshipId
AkritesTriageTabComponent->>AkritesService: assignSteward(stewardshipId, body)
end
AkritesService-->>AkritesTriageTabComponent: success
AkritesTriageTabComponent->>AkritesTriageTabComponent: hide modal, show toast, emit stewardshipChanged
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/lfx-one/src/app/modules/akrites/components/akrites-triage-tab/akrites-triage-tab.component.ts (1)
83-133: ⚡ Quick winConsider refactoring to eliminate duplicated subscription logic.
The current implementation defines an
assignfunction that subscribes internally, then duplicates the same subscribe logic in theopenStewardshipchain. This can be simplified by branching early in a single observable stream.♻️ Cleaner RxJS pattern
protected onAssignStewardConfirm(body: AkritesAssignStewardRequest): void { const pkg = this.assignTargetPackage(); if (!pkg || this.actionLoading()) return; this.actionLoading.set(true); - const assign = (id: number) => - this.akritesService - .assignSteward(id, body) - .pipe(take(1), takeUntilDestroyed(this.destroyRef)) - .subscribe({ - next: () => { - this.assignModalVisible.set(false); - this.assignTargetPackage.set(null); - this.actionLoading.set(false); - this.messageService.add({ severity: 'success', summary: 'Assigned', detail: `Steward assigned to ${pkg.name}.` }); - this.stewardshipChanged.emit(); - }, - error: () => { - this.actionLoading.set(false); - this.messageService.add({ severity: 'error', summary: 'Error', detail: 'Could not assign steward. Please try again.' }); - }, - }); - - if (pkg.stewardshipId !== null) { - assign(pkg.stewardshipId); - } else { - this.akritesService - .openStewardship(pkg.purl) - .pipe( - switchMap((res) => { - const id = parseInt(res.stewardship.id, 10); - return this.akritesService.assignSteward(id, body); - }), - take(1), - takeUntilDestroyed(this.destroyRef) - ) - .subscribe({ - next: () => { - this.assignModalVisible.set(false); - this.assignTargetPackage.set(null); - this.actionLoading.set(false); - this.messageService.add({ severity: 'success', summary: 'Assigned', detail: `Steward assigned to ${pkg.name}.` }); - this.stewardshipChanged.emit(); - }, - error: () => { - this.actionLoading.set(false); - this.messageService.add({ severity: 'error', summary: 'Error', detail: 'Could not assign steward. Please try again.' }); - }, - }); - } + const stewardshipId$ = pkg.stewardshipId !== null + ? of(pkg.stewardshipId) + : this.akritesService.openStewardship(pkg.purl).pipe( + map((res) => parseInt(res.stewardship.id, 10)) + ); + + stewardshipId$ + .pipe( + switchMap((id) => this.akritesService.assignSteward(id, body)), + take(1), + takeUntilDestroyed(this.destroyRef) + ) + .subscribe({ + next: () => { + this.assignModalVisible.set(false); + this.assignTargetPackage.set(null); + this.actionLoading.set(false); + this.messageService.add({ severity: 'success', summary: 'Assigned', detail: `Steward assigned to ${pkg.name}.` }); + this.stewardshipChanged.emit(); + }, + error: () => { + this.actionLoading.set(false); + this.messageService.add({ severity: 'error', summary: 'Error', detail: 'Could not assign steward. Please try again.' }); + }, + }); }🤖 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/akrites/components/akrites-triage-tab/akrites-triage-tab.component.ts` around lines 83 - 133, The onAssignStewardConfirm method contains duplicated subscription logic in two places: once inside the assign function and again in the else block for the openStewardship chain. Both have identical next and error handlers. Refactor by creating a single observable stream that branches based on whether pkg.stewardshipId is null using the iif operator or a conditional switchMap. This allows you to define the assign subscription logic once and apply it to both paths, eliminating the duplication of the next and error handler blocks while maintaining the same functionality.
🤖 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/akrites/components/akrites-assign-steward-modal/akrites-assign-steward-modal.component.html`:
- Around line 29-35: The input field in the akrites-assign-steward-modal
template lacks an accessible name, making it inaccessible to screen readers. The
visible "Select steward" text is not associated with the input through a label
element, and placeholder text alone is insufficient for accessibility. Add an
aria-label attribute to the input element with the value "Select steward" to
provide a stable accessible name that matches the visible text in the interface,
or alternatively create a proper label element with a for attribute linked to
the input's id.
- Around line 121-125: The button element that controls the moveToAssessing
toggle is not exposing its state to assistive technology. Add an aria-pressed
attribute bound to the moveToAssessing() signal to the button element (the one
with data-testid="akrites-assign-move-assessing"), so that screen readers can
properly communicate the toggle's current state to users.
In
`@apps/lfx-one/src/app/modules/akrites/components/akrites-assign-steward-modal/akrites-assign-steward-modal.component.ts`:
- Around line 49-51: The onSearchInput method updates the searchQuery signal
when the user types a filter, but it does not clear the selectedSteward signal.
This causes the selectedSteward to remain set even after the search query hides
that steward from the visible list, allowing the confirm button to assign a
steward that is no longer visible. Modify the onSearchInput method to clear the
selectedSteward selection whenever the searchQuery is updated, ensuring that the
selection state stays synchronized with the currently filtered and visible
stewards in the picker.
---
Nitpick comments:
In
`@apps/lfx-one/src/app/modules/akrites/components/akrites-triage-tab/akrites-triage-tab.component.ts`:
- Around line 83-133: The onAssignStewardConfirm method contains duplicated
subscription logic in two places: once inside the assign function and again in
the else block for the openStewardship chain. Both have identical next and error
handlers. Refactor by creating a single observable stream that branches based on
whether pkg.stewardshipId is null using the iif operator or a conditional
switchMap. This allows you to define the assign subscription logic once and
apply it to both paths, eliminating the duplication of the next and error
handler blocks while maintaining the same functionality.
🪄 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: 5aef58a9-708a-4457-a8a4-40465606581a
📒 Files selected for processing (11)
apps/lfx-one/src/app/modules/akrites/components/akrites-assign-steward-modal/akrites-assign-steward-modal.component.htmlapps/lfx-one/src/app/modules/akrites/components/akrites-assign-steward-modal/akrites-assign-steward-modal.component.tsapps/lfx-one/src/app/modules/akrites/components/akrites-package-drawer/akrites-package-drawer.component.htmlapps/lfx-one/src/app/modules/akrites/components/akrites-package-drawer/akrites-package-drawer.component.tsapps/lfx-one/src/app/modules/akrites/components/akrites-triage-tab/akrites-triage-tab.component.htmlapps/lfx-one/src/app/modules/akrites/components/akrites-triage-tab/akrites-triage-tab.component.tsapps/lfx-one/src/app/shared/services/akrites.service.tsapps/lfx-one/src/server/helpers/validation.helper.tsapps/lfx-one/src/server/services/akrites.service.tspackages/shared/src/constants/akrites.constants.tspackages/shared/src/interfaces/akrites.interface.ts
Signed-off-by: Gašper Grom <gasper.grom@gmail.com>
Signed-off-by: Gašper Grom <gasper.grom@gmail.com>
Contributor
There was a problem hiding this comment.
Copilot encountered an error: Your billing is not configured or you have Copilot licenses from multiple standalone organizations or enterprises. To use premium requests, select a billing entity via the GitHub site, under Settings > Copilot > Features.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
akrites-assign-steward-modalwith a searchable member picker that loads from the LFX Akrites committee endpoint (GET /api/committees/:id/members)AkritesAssignStewardRequest/CdpStewardSummary/AkritesStewardwith requiredusernameanddisplayNamefields to satisfy the CDP assign endpoint contractChanges
Shared package (
@lfx-one/shared)AkritesSearchStewardResult— new interface mappingCommitteeMemberto an assignable steward shapeAkritesAssignStewardRequest— adds requiredusername: stringanddisplayName: stringCdpStewardSummary/AkritesSteward— addsusernameanddisplayNamefieldsAKRITES_STEWARD_COMMITTEE_UID— constant pointing to the LFX Akrites working group (8bffb08a-3707-4f8f-9e1c-0cbca8a4dfb6)Backend (
apps/lfx-one/src/server)validation.helper.ts—parseAssignStewardBodynow validates requiredusernameanddisplayNameakrites.service.ts—mapStewardspopulatesusernameandnamefrom the CDP responseFrontend
akrites.service.ts— addssearchStewards(): fetches committee members and maps them toAkritesSearchStewardResult[]akrites-assign-steward-modal— full rewrite: initials-avatar list, client-side search filter (name/username/org), single-select with blue border + checkmark, role radio buttons, Move to Assessing toggle; all driven by signalsakrites-package-drawer— Assign steward / Reassign button shown for actionable statuses; modal wired toonAssignStewardConfirm(open-then-assign chain for packages without a stewardship row)akrites-triage-tab— Assign steward modal wired to unassigned and inactive column actionsJIRA
CM-1268