feat(committees): require organization fields when voting or business…#373
feat(committees): require organization fields when voting or business…#373
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review.
Tip: disable this comment in your organization's Code Review settings.
github-actions
bot
requested review from
andrest50,
asithade,
dealako and
mauriciozanettisalomao
|
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 (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughMember form now conditionally requires organization and organization URL based on committee settings; a URL regex constant was added. The template shows a conditional required asterisk and displays validation messages for the organization controls. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Adds conditional requirement for member organization name and website when a committee has voting enabled or requires business email, ensuring member records include org details under those committee configurations.
Changes:
- Introduced an
orgRequiredflag derived fromcommittee.enable_voting || committee.business_email_required. - Added conditional
Validators.requiredtoorganizationandorganization_urlform controls. - Updated the member form UI to show a required indicator and inline required-field error messages for organization fields.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| apps/lfx-one/src/app/modules/committees/components/member-form/member-form.component.ts | Adds orgRequired and applies conditional required validators to organization fields. |
| apps/lfx-one/src/app/modules/committees/components/member-form/member-form.component.html | Displays conditional required indicator and required validation messages for organization fields. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
apps/lfx-one/src/app/modules/committees/components/member-form/member-form.component.ts
Outdated
Show resolved
Hide resolved
apps/lfx-one/src/app/modules/committees/components/member-form/member-form.component.html
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/lfx-one/src/app/modules/committees/components/member-form/member-form.component.ts`:
- Around line 213-214: The "Add Member" button can open the member dialog before
committee data is ready causing validators in MemberFormComponent (e.g.,
organization and organization_url FormControls) to initialize with incomplete
data; update the add-member-button in committee-members-manager.component.html
to include [disabled]="loading()" (or equivalent check for committee readiness)
and add a guard at the top of openEditMemberDialog() in
CommitteeMembersManagerComponent (return early if loading() is true or if the
committee signal is not yet populated) so the dialog is only opened once
committee data is available.
🪄 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: 90aafbd6-b35e-41b5-88ab-0f1d264d9628
📒 Files selected for processing (2)
apps/lfx-one/src/app/modules/committees/components/member-form/member-form.component.htmlapps/lfx-one/src/app/modules/committees/components/member-form/member-form.component.ts
| organization: new FormControl('', this.orgRequired ? [Validators.required] : []), | ||
| organization_url: new FormControl('', this.orgRequired ? [Validators.required] : []), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify all open paths and whether committee is passed as a snapshot value.
rg -n -C4 'open\(MemberFormComponent'
# Inspect committee-members callsites mentioned in current context.
rg -n -C6 'openAddMemberDialog\(|committee:\s*this\.committee\(\)' \
apps/lfx-one/src/app/modules/committees/components/committee-members/committee-members.component.ts \
apps/lfx-one/src/app/modules/committees/components/committee-members-manager/committee-members-manager.component.ts
# Check whether dialog-open actions are guarded by loading/committee-ready conditions.
rg -n -C4 'loading|committee\(\)|openAddMemberDialog' \
apps/lfx-one/src/app/modules/committees/components/committee-members/committee-members.component.ts \
apps/lfx-one/src/app/modules/committees/components/committee-members-manager/committee-members-manager.component.tsRepository: linuxfoundation/lfx-v2-ui
Length of output: 31348
🏁 Script executed:
# Get the member-form component file to see the validator setup
fd member-form.component.ts --type f | head -1 | xargs wc -l
# Read the relevant section around lines 213-214
fd member-form.component.ts --type f | head -1 | xargs sed -n '190,230p'Repository: linuxfoundation/lfx-v2-ui
Length of output: 1850
🏁 Script executed:
# Check the orgRequired definition and how it depends on committee
fd member-form.component.ts --type f | head -1 | xargs grep -n "orgRequired" | head -20Repository: linuxfoundation/lfx-v2-ui
Length of output: 301
🏁 Script executed:
# Search for committee signal initialization in member-form.component.ts
fd member-form.component.ts --type f | head -1 | xargs grep -n "@Input\|committee\s*=" | head -20Repository: linuxfoundation/lfx-v2-ui
Length of output: 121
🏁 Script executed:
# Check if there are guards preventing dialog open while loading
rg -n "disabled.*loading\|openAddMemberDialog\|openEditMemberDialog" \
apps/lfx-one/src/app/modules/committees/components/committee-members-manager/committee-members-manager.component.htmlRepository: linuxfoundation/lfx-v2-ui
Length of output: 51
🏁 Script executed:
# Get the orgRequired getter definition
fd member-form.component.ts --type f | head -1 | xargs sed -n '40,60p'Repository: linuxfoundation/lfx-v2-ui
Length of output: 956
🏁 Script executed:
# Check the full committee assignment context and constructor
fd member-form.component.ts --type f | head -1 | xargs sed -n '1,80p'Repository: linuxfoundation/lfx-v2-ui
Length of output: 3837
🏁 Script executed:
# Check the committee-members-manager template for button guards
fd committee-members-manager.component.html --type f | head -1 | xargs catRepository: linuxfoundation/lfx-v2-ui
Length of output: 6686
Add loading guard to prevent dialog opening before committee data is fully available.
The "Add Member" button in committee-members-manager.component.html lacks a disabled state while loading() is true. This allows the dialog to open before the committee signal is populated, causing form validators to be initialized with incomplete or undefined committee data. Since validators are set once at form construction time (lines 213-214 of member-form.component.ts), they won't auto-refresh when committee data arrives later.
Add [disabled]="loading()" to the add-member-button and similarly guard openEditMemberDialog() to ensure committee data is ready before the form constructs its validators.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/lfx-one/src/app/modules/committees/components/member-form/member-form.component.ts`
around lines 213 - 214, The "Add Member" button can open the member dialog
before committee data is ready causing validators in MemberFormComponent (e.g.,
organization and organization_url FormControls) to initialize with incomplete
data; update the add-member-button in committee-members-manager.component.html
to include [disabled]="loading()" (or equivalent check for committee readiness)
and add a guard at the top of openEditMemberDialog() in
CommitteeMembersManagerComponent (return early if loading() is true or if the
committee signal is not yet populated) so the dialog is only opened once
committee data is available.
… email is enabled Organization name and website are now required on the member form when either enable_voting or business_email_required is true on the committee. Adds conditional validators, required indicator, and error messages. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Jordan Evans <jevans@linuxfoundation.org>
MRashad26
left a comment
MRashad26
left a comment
There was a problem hiding this comment.
Code Review — 3 Major Issues
The feature logic is sound, but there are three issues that should be addressed before merge. Details inline.
| // Wizard mode: returns data instead of calling API (used when committee doesn't exist yet) | ||
| public readonly wizardMode: boolean; | ||
|
|
||
| // Organization fields are required when voting is enabled OR business email is required |
There was a problem hiding this comment.
🔴 Major: orgRequired is a plain getter, not a signal or computed
Problem:
get orgRequired() is a plain TypeScript getter. In this zoneless / signals-first codebase, every other piece of derived state uses computed() or linkedSignal(). A getter is re-evaluated on every change-detection pass and is not memoised.
Why it matters:
- Convention break — the rest of this component (and the wider app) derives state with signals. Mixing in getters makes the reactivity model inconsistent and harder to reason about for the next developer.
- No memoisation —
computed()caches its result and only re-evaluates when a dependency signal changes. A getter runs unconditionally, which matters when it fans out into template@ifchecks and validator setup.
Fix:
private readonly orgRequired = computed(() =>
!!(this.committee?.enable_voting || this.committee?.business_email_required)
);Then reference it as orgRequired() in the template and when setting validators.
| linkedin_profile: new FormControl('', [Validators.pattern(LINKEDIN_PROFILE_PATTERN)]), | ||
| organization: new FormControl(''), | ||
| organization_url: new FormControl(''), | ||
| organization: new FormControl('', this.orgRequired ? [Validators.required] : []), |
There was a problem hiding this comment.
🔴 Major: Validators are baked in at construction time — won't react to committee changes
Problem:
this.orgRequired ? [Validators.required] : [] is evaluated once when createMemberFormGroup() runs in the constructor. The validators are frozen from that point on — if committee data changes (e.g. a parent updates the dialog config, or the form is reused for a different committee), the required/optional state won't update.
Why it matters:
Right now committee comes from DynamicDialogConfig and is available at construction, so this works today. But it's a latent trap:
- If this form is ever refactored to receive
committeeas a signal input, or reused across committees, validators silently stay stale. - The
roleandvoting_statuslines directly below have the same issue, but those pre-date this PR.
Fix:
Use an effect() to sync validators reactively:
effect(() => {
const required = this.orgRequired();
const orgCtrl = this.form().get('organization');
const urlCtrl = this.form().get('organization_url');
orgCtrl?.setValidators(required ? [Validators.required] : []);
urlCtrl?.setValidators(
required
? [Validators.required, Validators.pattern(WEBSITE_URL_PATTERN)]
: [Validators.pattern(WEBSITE_URL_PATTERN)]
);
orgCtrl?.updateValueAndValidity();
urlCtrl?.updateValueAndValidity();
});This keeps validators in sync with the committee state and pairs naturally with the computed() fix above.
| <p class="mt-1 text-xs text-red-600">Organization name is required</p> | ||
| } | ||
| @if (form().get('organization_url')?.errors?.['required'] && form().get('organization_url')?.touched) { | ||
| <p class="mt-1 text-xs text-red-600">Organization website is required</p> |
There was a problem hiding this comment.
🔴 Major: Validation errors reference a field the user cannot directly edit
Problem:
organization_url is auto-populated by lfx-organization-search when the user selects an organization (via normalizeToUrl(domain)). The user never types into an organization URL input. Showing "Organization website is required" and "Please enter a valid URL" is confusing because the user has no direct action to fix it — their only action is to select an organization from the autocomplete.
Why it matters:
- UX confusion — a user who selected an org but got an invalid URL back from the search component will see "Please enter a valid URL" with no URL field to correct.
- Required error misleading — if the org search returns a name but no domain, the user sees "Organization website is required" but cannot supply one. The real fix is to select a different org or report a data issue.
Fix:
Replace the two organization_url error messages with a single, actionable message tied to the org selection:
@if (form().get('organization_url')?.errors && form().get('organization_url')?.touched) {
<p class="mt-1 text-xs text-red-600">
The selected organization is missing a valid website. Please select a different organization.
</p>
}This tells the user what to do rather than describing a field they can't see.
There was a problem hiding this comment.
The user also can enter a url manually -- this is a "find it via search, or type it yourself" flow.
… email is enabled
Organization name and website are now required on the member form when either enable_voting or business_email_required is true on the committee. Adds conditional validators, required indicator, and error messages.