Skip to content

feat(committees): require organization fields when voting or business… #373

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jordane wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat(committees): require organization fields when voting or business… #373

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f213532
feat(committees): require organization fields when voting or business…
jordane Mar 25, 2026
d4831f5
Merge branch 'main' into jme/LFXV2-1317
asithade Apr 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion apps/lfx-one/src/app/modules/committees/components/member-form/member-form.component.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,9 @@

<!-- Organization Search -->
<div>
<label for="organization-search" class="block text-sm font-medium text-gray-700 mb-1">Organization</label>
<label for="organization-search" class="block text-sm font-medium text-gray-700 mb-1">
Comment thread
jordane marked this conversation as resolved.

@asithade asithade May 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT — accessibility

The <label for="organization-search"> still references an id that does not exist on the underlying control (Copilot raised this; author explicitly deferred). Re-flagging because the deferral has been outstanding ~6 weeks. Either remove for= or wire an id through to the autocomplete input.

Organization @if (orgRequired) { <span class="text-red-500">*</span> }
</label>
<lfx-organization-search
[form]="form()"
nameControl="organization"
Expand All @@ -99,6 +101,15 @@
panelStyleClass="text-sm w-full"
dataTestId="member-form-organization-search">
</lfx-organization-search>
@if (form().get('organization')?.errors?.['required'] && form().get('organization')?.touched) {
<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>

@MRashad26 MRashad26 Mar 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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.

@jordane jordane Mar 25, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The user also can enter a url manually -- this is a "find it via search, or type it yourself" flow.

}
@if (form().get('organization_url')?.errors?.['pattern'] && form().get('organization_url')?.touched) {
<p class="mt-1 text-xs text-red-600">Please enter a valid URL</p>
}
</div>
</div>

Expand Down
11 changes: 8 additions & 3 deletions apps/lfx-one/src/app/modules/committees/components/member-form/member-form.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { CalendarComponent } from '@components/calendar/calendar.component';
import { InputTextComponent } from '@components/input-text/input-text.component';
import { OrganizationSearchComponent } from '@components/organization-search/organization-search.component';
import { SelectComponent } from '@components/select/select.component';
import { APPOINTED_BY_OPTIONS, LINKEDIN_PROFILE_PATTERN, MEMBER_ROLES, VOTING_STATUSES } from '@lfx-one/shared/constants';
import { APPOINTED_BY_OPTIONS, LINKEDIN_PROFILE_PATTERN, MEMBER_ROLES, VOTING_STATUSES, WEBSITE_URL_PATTERN } from '@lfx-one/shared/constants';
import { Committee, CommitteeMember, CreateCommitteeMemberRequest, MemberFormValue } from '@lfx-one/shared/interfaces';
import { formatDateToISOString, parseISODateString } from '@lfx-one/shared/utils';
import { CommitteeService } from '@services/committee.service';
Expand Down Expand Up @@ -44,6 +44,11 @@ export class MemberFormComponent {
// 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

@MRashad26 MRashad26 Mar 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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:

  1. 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.
  2. 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 @if checks 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.

public get orgRequired(): boolean {
return !!(this.committee?.enable_voting || this.committee?.business_email_required);

@asithade asithade May 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SHOULD_FIX — component-organization.md § 1, § 6 + CLAUDE.md (signals-first / zoneless)

orgRequired is a plain TypeScript getter, which is re-evaluated on every change detection pass and isn't memoised. The rest of this codebase derives state through signals — keeping a getter here breaks the reactivity contract and the section-order convention in component-organization.md (computed signals go in slot 6 via private init functions). This was raised in the prior review (MRashad26) and not addressed.

Suggested fix:

// slot 6 — complex computed signals (declared near other signals)
public readonly orgRequired: Signal<boolean> = this.initOrgRequired();

private initOrgRequired(): Signal<boolean> {
  return computed(() => !!(this.committee?.enable_voting || this.committee?.business_email_required));
}

Then call it as orgRequired() in the template and in createMemberFormGroup().

}

// Member options
public roleOptions = MEMBER_ROLES;
public votingStatusOptions = VOTING_STATUSES;
Expand Down Expand Up @@ -206,8 +211,8 @@ export class MemberFormComponent {
email: new FormControl('', [Validators.required, Validators.email]),
job_title: new FormControl(''),
linkedin_profile: new FormControl('', [Validators.pattern(LINKEDIN_PROFILE_PATTERN)]),
organization: new FormControl(''),
organization_url: new FormControl(''),
organization: new FormControl('', this.orgRequired ? [Validators.required] : []),

@MRashad26 MRashad26 Mar 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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 committee as a signal input, or reused across committees, validators silently stay stale.
  • The role and voting_status lines 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.

@asithade asithade May 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT — component-organization.md § 6 (signal reactivity)

Validators are evaluated once at form construction. Today committee is set synchronously from DynamicDialogConfig so this works, but if the form is ever reused or committee becomes a signal input, the required/optional state will silently stay stale. Combined with the orgRequired finding above, an effect() (or computed validator factory) keyed off orgRequired() would future-proof this.

organization_url: new FormControl('', this.orgRequired ? [Validators.required, Validators.pattern(WEBSITE_URL_PATTERN)] : [Validators.pattern(WEBSITE_URL_PATTERN)]),
role: new FormControl('', this.committee?.enable_voting ? [Validators.required] : []),
voting_status: new FormControl('', this.committee?.enable_voting ? [Validators.required] : []),
appointed_by: new FormControl(''),
Expand Down
3 changes: 3 additions & 0 deletions packages/shared/src/constants/validation.constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,3 +27,6 @@
* - linkedin.com (missing path)
*/
export const LINKEDIN_PROFILE_PATTERN = /^(https?:\/\/)?([a-z]{2,3}\.)?linkedin\.com\/.*$/;

/** Website URL pattern matching the upstream backend validation */
export const WEBSITE_URL_PATTERN = /^(https?:\/\/)?[^\s/$.?#].[^\s]*$/;

@asithade asithade May 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT — development-rules.md — Documentation maintenance

The new WEBSITE_URL_PATTERN has a one-line JSDoc. The sibling LINKEDIN_PROFILE_PATTERN directly above has a thorough block-style JSDoc with Valid/Invalid examples. For consistency mirror that format here — and ideally link the "upstream backend validation" claim to the specific regex in lfx-v2-committee-service.

Loading