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 1 commit 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
1 commit
Select commit
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">
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>
Copy link
Copy Markdown
Contributor

@MRashad26 MRashad26 Mar 25, 2026

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.

Copy link
Copy Markdown
Member Author

@jordane jordane Mar 25, 2026

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 @@ -43,6 +43,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
Copy link
Copy Markdown
Contributor

@MRashad26 MRashad26 Mar 25, 2026

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);
}

// Member options
public roleOptions = MEMBER_ROLES;
public votingStatusOptions = VOTING_STATUSES;
Expand Down Expand Up @@ -205,8 +210,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] : []),
Copy link
Copy Markdown
Contributor

@MRashad26 MRashad26 Mar 25, 2026

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.

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]*$/;
Loading