Skip to content

feat(committee): generate sso_group_name slug for public committees - #172

Merged
MRashad26 merged 11 commits into
mainfrom
feat/LFXV2-2012-public-name
Aug 7, 2026
Merged

feat(committee): generate sso_group_name slug for public committees#172
MRashad26 merged 11 commits into
mainfrom
feat/LFXV2-2012-public-name

Conversation

@MRashad26

Copy link
Copy Markdown
Contributor

Summary

  • Add public_name (string, optional, max 200 chars) to the committee base model, enabling human-readable slugs for public group detail pages instead of UUIDs
  • Field flows through Goa DSL, create/update payload mappers, base and full response converters, indexer (name_and_aliases with deduplication + public_name:{value} tag), and indexer contract docs
  • Client (PCC) owns the default — when public is enabled, PCC defaults public_name to the committee Name if not provided

Test plan

  • make build passes
  • make test — all existing tests pass, plus:
    • Updated converter test cases with PublicName across create, update, base-to-response, full-to-response
    • New TestBuildCommitteeIndexingConfig_PublicNameDedup covering all dedup scenarios (distinct, matches name, matches display_name, empty)
  • go vet ./... clean
  • Post-commit reviewer trio clean on both commits
  • Full-branch sweep clean (all 3 reviewers)

Related

  • Part of LFXV2-2012 (public group detail page slug support)
  • Frontend changes in lfx-v2-ui PR #1214 will consume public_name for slug-based routing

🤖 Generated with Claude Code

@MRashad26
MRashad26 requested a review from a team as a code owner August 5, 2026 11:10
Copilot AI balanced review requested due to automatic review settings August 5, 2026 11:10
@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

The change adds an optional public_name field to committee API contracts, domain models, responses, indexing, and uniqueness storage. It maps the field through create and update flows and manages public-name lookup keys.

Changes

Committee public name support

Layer / File(s) Summary
API contract and conversion
cmd/committee-api/design/type.go, cmd/committee-api/service/committee_service_response.go, cmd/committee-api/service/committee_service_response_test.go, internal/domain/model/committee_base.go
The API defines an optional public_name slug with a 200-character limit. Create, update, domain, and response conversions map PublicName. CommitteeBase stores the value as public_name.
Indexing aliases and tags
internal/service/committee_writer.go, internal/service/committee_writer_test.go, docs/indexer-contract.md
Indexing adds distinct public names to aliases and emits public_name: tags. Tests cover deduplication and empty values. The indexer contract documents the field and search behavior.
Public name uniqueness lifecycle
internal/domain/port/committee_writer.go, internal/infrastructure/nats/storage.go, internal/infrastructure/mock/committee.go, internal/service/committee_writer.go, internal/service/committee_member_writer_test.go, internal/service/committee_writer_test.go, pkg/constants/storage.go
The writer contract and storage implementations reserve globally unique public-name keys. Create, update, and delete flows reserve, replace, and remove public-name indexes. Test writers and mocks support the uniqueness operation.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant APIClient
  participant CommitteeAPI
  participant CommitteeWriter
  participant CommitteeStorage
  participant CommitteeIndexer
  APIClient->>CommitteeAPI: Submit committee with public_name
  CommitteeAPI->>CommitteeWriter: Map PublicName into committee
  CommitteeWriter->>CommitteeStorage: Reserve public-name lookup key
  CommitteeStorage-->>CommitteeWriter: Return key or conflict
  CommitteeWriter->>CommitteeIndexer: Add distinct alias and public_name tag
Loading
🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ⚠️ Warning The title refers to an sso_group_name slug, but the changes add and manage the public_name field. Change the title to describe adding public_name slug support for public committees.
✅ Passed checks (3 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The description accurately summarizes public_name support, indexing, uniqueness enforcement, client defaults, and test coverage.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/LFXV2-2012-public-name

Comment @coderabbitai help to get the list of available commands.

Copilot AI left a comment

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.

Pull request overview

Adds optional committee public_name support for public URL slugs and indexing.

Changes:

  • Extends Goa API models, converters, and generated clients/specifications.
  • Persists and indexes public_name as an alias and tag.
  • Adds converter/indexing tests and contract documentation.

Reviewed changes

Copilot reviewed 9 out of 16 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
internal/service/committee_writer.go Adds public_name indexing.
internal/service/committee_writer_test.go Tests alias deduplication and tags.
internal/domain/model/committee_base.go Adds the domain field and tag.
gen/http/openapi3.yaml Regenerates OpenAPI 3 definitions.
gen/http/openapi.yaml Regenerates OpenAPI definitions.
gen/http/committee_service/server/types.go Regenerates server transport types.
gen/http/committee_service/client/types.go Regenerates client transport types.
gen/http/committee_service/client/cli.go Updates generated payload handling.
gen/http/cli/committee/cli.go Updates generated CLI examples.
gen/committee_service/service.go Regenerates Goa service types.
docs/indexer-contract.md Documents the indexed field and tag.
cmd/committee-api/service/committee_service_response.go Maps the field across domain/API boundaries.
cmd/committee-api/service/committee_service_response_test.go Tests converter mappings.
cmd/committee-api/design/type.go Defines the public Goa attribute.
Files not reviewed (5)
  • gen/committee_service/service.go: Generated file
  • gen/http/cli/committee/cli.go: Generated file
  • gen/http/committee_service/client/cli.go: Generated file
  • gen/http/committee_service/client/types.go: Generated file
  • gen/http/committee_service/server/types.go: Generated file

Comment thread internal/service/committee_writer.go Outdated
Comment thread internal/service/committee_writer_test.go Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 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 `@cmd/committee-api/design/type.go`:
- Around line 325-330: Update PublicNameAttribute to add dsl.Pattern validation
enforcing slug syntax such as alphanumeric segments separated by hyphens, while
retaining the existing length limit and example. Also update the PCC
default-generation path so committee-name-derived public_name values are
normalized into the same valid slug format.

In `@docs/indexer-contract.md`:
- Line 95: Update the fulltext field documentation in docs/indexer-contract.md
to include public_name alongside the existing name and display_name fields,
matching the values assembled by nameAndAliases in committee_writer.go.
🪄 Autofix

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: 53d109e6-f4ed-46b1-8d41-fe8bf73ba233

📥 Commits

Reviewing files that changed from the base of the PR and between 98b1cc6 and b3352d3.

⛔ Files ignored due to path filters (9)
  • gen/committee_service/service.go is excluded by !**/gen/**
  • gen/http/cli/committee/cli.go is excluded by !**/gen/**
  • gen/http/committee_service/client/cli.go is excluded by !**/gen/**
  • gen/http/committee_service/client/types.go is excluded by !**/gen/**
  • gen/http/committee_service/server/types.go is excluded by !**/gen/**
  • gen/http/openapi.json is excluded by !**/gen/**
  • gen/http/openapi.yaml is excluded by !**/gen/**
  • gen/http/openapi3.json is excluded by !**/gen/**
  • gen/http/openapi3.yaml is excluded by !**/gen/**
📒 Files selected for processing (7)
  • cmd/committee-api/design/type.go
  • cmd/committee-api/service/committee_service_response.go
  • cmd/committee-api/service/committee_service_response_test.go
  • docs/indexer-contract.md
  • internal/domain/model/committee_base.go
  • internal/service/committee_writer.go
  • internal/service/committee_writer_test.go

Comment thread cmd/committee-api/design/type.go Outdated
Comment thread docs/indexer-contract.md Outdated

@audigregorie audigregorie left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review Summary

Well-structured feature addition that follows the existing converter and indexer patterns, with thorough dedup test coverage. Two Major findings to address before merge: the fulltext row in the indexer contract was not updated to reflect that public_name is now also included in fulltext (via the nameAndAliases loop), and public_name lacks a dsl.Pattern constraint despite being described as a URL slug — a client could store values with spaces or slashes that break URL routing.

Major — outside the diff

  • Uniqueness of public_name is not enforced. The public_name:{value} tag implies single-committee lookup by slug, but nothing in this service prevents two committees from sharing the same public_name. If uniqueness is expected to be enforced elsewhere (frontend routing, a future PR), consider documenting that here or in the contract. Not blocking since the PR description says the frontend owns routing, but worth confirming.

What's done well

  • Dedup logic in buildCommitteeIndexingConfig correctly checks public_name against both Name and DisplayName before appending, matching the existing DisplayName pattern.
  • TestBuildCommitteeIndexingConfig_PublicNameDedup covers all four scenarios (distinct, matches name, matches display_name, empty).
  • Response converters correctly omit PublicName when empty, consistent with omitempty and the DisplayName pattern.
  • Generated code is consistently regenerated across all formats (OpenAPI 2/3, JSON/YAML, CLI, kodata).

Comment thread docs/indexer-contract.md Outdated
Comment thread cmd/committee-api/design/type.go Outdated
Comment thread cmd/committee-api/design/type.go Outdated
Comment thread internal/service/committee_writer_test.go Outdated
MRashad26 added a commit that referenced this pull request Aug 5, 2026
Address review comments from @audigregorie, @coderabbitai, @copilot-pull-request-reviewer:

- docs/indexer-contract.md: add public_name to fulltext row to match
  actual code behavior where nameAndAliases feeds fulltext (per @audigregorie,
  @coderabbitai, @copilot-pull-request-reviewer)
- cmd/committee-api/design/type.go: remove PCC client reference from DSL
  description to keep API contract client-agnostic (per @audigregorie)
- internal/service/committee_writer_test.go: use strings.HasPrefix instead
  of magic number prefix check, and assert full tag value instead of just
  prefix presence (per @audigregorie, @copilot-pull-request-reviewer)
- gen/: regenerated Goa output for updated DSL description

Resolves 8 review threads.

Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
Copilot AI review requested due to automatic review settings August 5, 2026 11:35
@MRashad26

Copy link
Copy Markdown
Contributor Author

Review Feedback Addressed

Commit: 0322bd9

Changes Made

  • docs/indexer-contract.md: Added public_name to the fulltext row — the code already includes it via nameAndAliases, but the contract was out of sync (per @audigregorie, @coderabbitai, @copilot-pull-request-reviewer)
  • cmd/committee-api/design/type.go: Removed PCC client reference from DSL description to keep the API contract client-agnostic (per @audigregorie)
  • internal/service/committee_writer_test.go: Replaced magic number prefix check with strings.HasPrefix and strengthened assertion to check full tag value instead of just prefix presence (per @audigregorie, @copilot-pull-request-reviewer)
  • gen/: Regenerated Goa output for updated DSL description

No Change Needed

  • design/type.go:328 — slug pattern validation: Intentional design choice — public_name is free-form at the API layer; the consuming client owns default generation and normalization. Strict patterns would reject valid URL segments and couple the API to client slugification logic. (flagged by @audigregorie, @coderabbitai)
  • Uniqueness of public_name: Intentionally not enforced server-side — frontend routing does exact tag match lookups; uniqueness enforcement is a client/UX concern. (flagged by @audigregorie)

Threads Resolved

8 of 8 unresolved threads addressed in this iteration.

@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

git show --no-ext-diff --format=fuller --stat 0322bd9
git show --no-ext-diff --format= -- \
  docs/indexer-contract.md \
  cmd/committee-api/design/type.go \
  internal/service/committee_writer_test.go \
  gen \
  0322bd9

printf '\n--- DSL outline ---\n'
ast-grep outline cmd/committee-api/design/type.go --items all --match 'PublicNameAttribute|CommitteeBaseAttributes' || true

printf '\n--- Relevant current references ---\n'
rg -n -C 3 'public_name|PublicName|name_and_aliases|fulltext|HasPrefix' \
  docs/indexer-contract.md \
  cmd/committee-api/design/type.go \
  internal/service/committee_writer_test.go \
  internal/domain/model/committee_base.go \
  internal/service/committee_writer.go

Length of output: 201205


@MRashad26 Verified commit 0322bd9.

The indexer contract now includes public_name in both fulltext and name_and_aliases.

The DSL description is client-agnostic. The generated Goa artifacts match the DSL description.

TestBuildCommitteeIndexingConfig_PublicNameDedup now uses strings.HasPrefix and verifies the complete public_name:{value} tag.

I accept the stated API contract decisions. public_name remains free-form. The service does not enforce uniqueness. The consuming client owns normalization and routing behavior.


✏️ Learnings added
Learnt from: MRashad26
URL: https://github.com/linuxfoundation/lfx-v2-committee-service/pull/172

Timestamp: 2026-08-05T11:36:52.396Z
Learning: In `cmd/committee-api/design/type.go` and the committee API contract, `public_name` is an optional free-form string with a maximum length of 200 characters. The API must not impose slug-pattern validation because the consuming client owns public URL generation and normalization.

Learnt from: MRashad26
URL: https://github.com/linuxfoundation/lfx-v2-committee-service/pull/172

Timestamp: 2026-08-05T11:36:52.396Z
Learning: For committee `public_name`, server-side uniqueness is intentionally not enforced. Frontend routing performs exact `public_name:{value}` index-tag lookups, and duplicate-value handling is a client and UX concern.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

You are interacting with an AI system.

@prabodhcs prabodhcs left a comment

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.

Hey @MRashad26 — clean, well-scoped feature wiring; the remaining items are the two Majors from the previous round plus a test gap on the new fulltext side effect.

Reviewed at b3352d3 in a worktree pinned to that SHA, with the affected test packages run locally.

Nice Work

  • Every converter boundary got the field, with no misses. DisplayName appears at exactly four mapping sites in cmd/committee-api/service/committee_service_response.go (lines 51, 132, 272, 362) and PublicName was added at all four with the same nil-check/omitempty shape. I checked for a missed sibling converter and there isn't one.
  • Generated code is genuinely consistent. PublicName *string lands in all four Goa service types, both request bodies, all response bodies, both New*Payload constructors, and both Validate*RequestBody functions with the MaxLength(200) check — no hand-edited or half-regenerated files.
  • TestBuildCommitteeIndexingConfig_PublicNameDedup covers all four dedup permutations (distinct, equals name, equals display_name, empty), which is the right table for the guard at internal/service/committee_writer.go:34.

Prior-round items — still open at b3352d3

I re-verified each rather than taking the earlier comments at face value:

  • fulltext contract row is wrong (docs/indexer-contract.md:94) — confirmed by execution; details inline. Raised by @audigregorie, CodeRabbit, and Copilot; unaddressed.
  • No slug pattern on public_name (cmd/committee-api/design/type.go:325-330) — still only MaxLength(200). Worth adding: github.com/gosimple/slug is already a dependency and already used for exactly this purpose at internal/domain/model/committee_base.go:76 (slug.Make(...) for SSO group names), so normalizing on the way in has an in-repo precedent rather than needing a new helper.
  • Tag assertion only checks the prefix (internal/service/committee_writer_test.go:2588) — still len(tag) > 12 && tag[:12] == "public_name:", so the test passes even if the wrong value is indexed. Copilot's point stands; @audigregorie's strings.HasPrefix suggestion fixes readability, but asserting the full public_name:<value> string is what actually closes the hole.
  • PCC named in the API description (cmd/committee-api/design/type.go:327) — still present, and it propagates into the generated OpenAPI specs and Go doc comments that external consumers read.
  • public_name uniqueness is unenforced — I confirmed there is no uniqueness index for it (only UniqueNameProject and the SSO name reservation exist in internal/service/committee_writer.go). Out of this diff; worth a line in the contract if the frontend is expected to own it.

Things I checked and am not raising

  • kodata OpenAPI copies aren't stalecmd/committee-api/kodata/gen/http/* are symlinks into gen/http/, so they track automatically.
  • Update clearing public_name when omitted is the convention, not a bugmergeCommitteeData (internal/service/committee_writer.go:383-401) only preserves UID, CreatedAt, and SSOGroupName; Description, DisplayName, and JoinMode are all full-replace under the ETag'd PUT, so public_name behaves the same as its siblings.
  • Spaces in the emitted tag value aren't new — a local probe produced public_name:Some Public Name/2024?x=1, but category:Government Advisory Council already has the same shape, so this is precedent rather than a regression introduced here. The URL-safety argument for a pattern still holds on its own.

Issue Count

Severity Count Items
blocking 1 fulltext contract row out of sync (prior round, verified)
minor 2 no Fulltext assertion; slug pattern still missing (prior round)
nit 3 200-char cap; tag assertion strength; PCC in API description

Verification

  • Worktree integrity gate (git rev-parse HEAD == b3352d38503f06097378a8b55a57ac38cac2895a): pass
  • go build ./...: pass
  • go test ./internal/service/... ./cmd/committee-api/service/... ./internal/domain/...: pass (4 packages ok)
  • Throwaway probe test proving public_name reaches Fulltext: fails as designed against the documented contract — this is the evidence for the blocking item (probe deleted afterwards, not proposed for the branch)
  • golangci-lint: not run (would only duplicate CI)

Comments only — flagging severity for your judgement rather than gating the merge; the code owners keep the approval decision.

Suggested next step: update the fulltext row in docs/indexer-contract.md, then decide on the slug pattern and pin whichever fulltext behavior you choose with a test.

Comment thread docs/indexer-contract.md Outdated
Comment thread internal/service/committee_writer_test.go
Comment thread cmd/committee-api/design/type.go Outdated

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 9 out of 16 changed files in this pull request and generated 1 comment.

Files not reviewed (5)
  • gen/committee_service/service.go: Generated file
  • gen/http/cli/committee/cli.go: Generated file
  • gen/http/committee_service/client/cli.go: Generated file
  • gen/http/committee_service/client/types.go: Generated file
  • gen/http/committee_service/server/types.go: Generated file
Suppressed comments (1)

cmd/committee-api/design/type.go:327

  • The generated public API contract says the service defaults this field to the committee name, but the create/update converters only copy PublicName when it is supplied and persist an empty value otherwise. Non-PCC API clients that set public: true and omit this field therefore receive no documented default. Either implement the default server-side or describe this as client-owned behavior instead.
	dsl.Attribute("public_name", dsl.String, "Human-readable slug for public URLs; optional, defaults to the committee name when public is enabled and no value is provided", func() {

Comment thread internal/domain/model/committee_base.go Outdated
Copilot AI review requested due to automatic review settings August 5, 2026 11:49
@MRashad26

Copy link
Copy Markdown
Contributor Author

Review Feedback Addressed (Round 2)

Commit: f81ba57

Changes Made

  • committee_writer_test.go: Added wantFulltext column and assertion to TestBuildCommitteeIndexingConfig_PublicNameDedup — pins the Fulltext value across all four dedup scenarios to prevent silent regressions when nameAndAliases changes (per @prabodhcs)

Already Fixed (Prior Commit)

  • indexer-contract.md:94: The fulltext row was already updated to include public_name in commit 0322bd9 (pushed before @prabodhcs's review at b3352d3)

No Change Needed

  • type.go:328: MaxLength(200) is deliberate — accommodates manually-set slugs that may be more descriptive than the committee name (capped at 100)

Threads Resolved

3 of 3 unresolved threads from @prabodhcs addressed in this iteration.

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 9 out of 16 changed files in this pull request and generated no new comments.

Files not reviewed (5)
  • gen/committee_service/service.go: Generated file
  • gen/http/cli/committee/cli.go: Generated file
  • gen/http/committee_service/client/cli.go: Generated file
  • gen/http/committee_service/client/types.go: Generated file
  • gen/http/committee_service/server/types.go: Generated file
Suppressed comments (1)

cmd/committee-api/design/type.go:327

  • The API contract promises a server-side default that is not implemented: with public: true and no public_name, convertPayloadToBase leaves PublicName empty, so storage, responses, and indexing all omit it. This misleads API clients that rely on the generated OpenAPI description. Since the default is owned by PCC, describe this as client-supplied; otherwise implement the default in the service.
	dsl.Attribute("public_name", dsl.String, "Human-readable slug for public URLs; optional, defaults to the committee name when public is enabled and no value is provided", func() {

audigregorie
audigregorie previously approved these changes Aug 5, 2026
prabodhcs
prabodhcs previously approved these changes Aug 5, 2026
@MRashad26 MRashad26 self-assigned this Aug 5, 2026
Copilot AI review requested due to automatic review settings August 5, 2026 12:32
@MRashad26
MRashad26 dismissed stale reviews from prabodhcs and audigregorie via d52d622 August 5, 2026 12:32
@MRashad26

Copy link
Copy Markdown
Contributor Author

Review Feedback Addressed (Round 3)

Commit: d52d622

Changes Made

  • Global uniqueness enforcement for public_name: Added UniquePublicName to the CommitteeBaseWriter port with NATS KV implementation (same pattern as UniqueSSOGroupName). Wired into create, update (with old key cleanup), and delete flows. Two committees can no longer share the same public_name — returns 409 Conflict if taken. (per @copilot-pull-request-reviewer)

Files Changed

  • pkg/constants/storage.go — new KVLookupPublicNamePrefix
  • internal/domain/port/committee_writer.go — new UniquePublicName interface method
  • internal/infrastructure/nats/storage.go — NATS KV implementation
  • internal/infrastructure/mock/committee.go — mock implementation
  • internal/service/committee_writer.go — create/update/delete wiring
  • internal/service/committee_writer_test.go — test mock
  • internal/service/committee_member_writer_test.go — test mock

Threads Resolved

1 of 1 remaining unresolved thread addressed.

All review threads are now resolved.

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 14 out of 21 changed files in this pull request and generated 2 comments.

Files not reviewed (5)
  • gen/committee_service/service.go: Generated file
  • gen/http/cli/committee/cli.go: Generated file
  • gen/http/committee_service/client/cli.go: Generated file
  • gen/http/committee_service/client/types.go: Generated file
  • gen/http/committee_service/server/types.go: Generated file
Suppressed comments (3)

internal/service/committee_writer.go:695

  • This stale key is deleted only when updateSucceeded becomes true at the end of the method. UpdateBase commits earlier, so an operational GetSettings failure afterward returns before setting that flag and permanently leaves the old public_name reserved even though the committee now has the new value. Mark persistence success immediately after UpdateBase, or otherwise run stale-key cleanup on every post-commit return.
		if existing.PublicName != "" {
			oldPublicNameKey := fmt.Sprintf(constants.KVLookupPublicNamePrefix, existing.PublicName)
			staleKeys = append(staleKeys, oldPublicNameKey)

internal/infrastructure/mock/committee.go:652

  • This mock returns an existing committee UID on conflict and a different key format on success, while the NATS implementation always returns the actual lookup key. That violates the rollback contract documented in docs/reviews/knowledge-base/nats-storage-kv.md:85-101 and prevents tests from faithfully exercising cleanup. Return fmt.Sprintf(constants.KVLookupPublicNamePrefix, committee.PublicName) in both cases.
	for _, existing := range w.mock.committees {
		if existing.PublicName == committee.PublicName && existing.CommitteeBase.UID != committee.CommitteeBase.UID {
			return existing.CommitteeBase.UID, errors.NewConflict(fmt.Sprintf("committee with public_name %s already exists", committee.PublicName))
		}
	}

	publicNameKey := "public_name:" + committee.PublicName
	return publicNameKey, nil

internal/service/committee_writer_test.go:2525

  • The added test covers only indexer deduplication; none of the existing create/update/delete tables exercise the new uniqueness reservation, duplicate conflict, rollback, stale-key replacement, or delete cleanup. This stateful secondary-index behavior needs lifecycle cases, especially because failures can strand globally reserved slugs.
func TestBuildCommitteeIndexingConfig_PublicNameDedup(t *testing.T) {

Comment thread internal/service/committee_writer.go Outdated
Comment thread internal/infrastructure/nats/storage.go Outdated

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 6 out of 8 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

gen/http/openapi3.yaml:1057

  • These avatar-only changes are generated output, but this PR changes neither the Goa design source nor the generator/runtime version. The repository rule at .claude/skills/committee-service-dev/SKILL.md:69-73 requires changes under gen/ to come from a design change followed by make apigen; unexplained generated drift should not be committed. Remove these unrelated changes, or include the corresponding source/tool change and regenerate all outputs.
                                    avatar: https://example.com/avatar.png

Comment thread internal/service/committee_writer.go
@MRashad26

Copy link
Copy Markdown
Contributor Author

Review Feedback Addressed

No Change Needed

  • committee_writer.go (outdated thread): DisplayName uniqueness enforcement was removed in commit 2c1fb9a when the approach pivoted from display_name to sso_group_name
  • committee_writer.go:487: sso_group_name repurposing as URL slug is an intentional design decision — both SSO and public URL flows share the same field per product direction

Threads Resolved

2 of 2 unresolved threads addressed.

MRashad26 and others added 9 commits August 6, 2026 19:10
Add public_name to the committee base model so public group detail
pages can use human-readable slugs instead of UUIDs. The field flows
through the Goa DSL, create/update payload mappers, base and full
response converters, the indexer (name_and_aliases + tag), and the
indexer contract docs.

LFXV2-2012

Signed-off-by: Mohamed Rashad <mrashad@contractor.linuxfoundation.org>
Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
Address general code reviewer findings:

- Add PublicName to existing converter test cases (create payload,
  update payload, domain-to-full response, base-to-response)
- Add TestBuildCommitteeIndexingConfig_PublicNameDedup covering all
  deduplication combinations (distinct, matches name, matches
  display_name, empty)
- Clarify DSL description: the client (PCC) owns the default, not
  the backend

LFXV2-2012

Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
Address review comments from @audigregorie, @coderabbitai, @copilot-pull-request-reviewer:

- docs/indexer-contract.md: add public_name to fulltext row to match
  actual code behavior where nameAndAliases feeds fulltext (per @audigregorie,
  @coderabbitai, @copilot-pull-request-reviewer)
- cmd/committee-api/design/type.go: remove PCC client reference from DSL
  description to keep API contract client-agnostic (per @audigregorie)
- internal/service/committee_writer_test.go: use strings.HasPrefix instead
  of magic number prefix check, and assert full tag value instead of just
  prefix presence (per @audigregorie, @copilot-pull-request-reviewer)
- gen/: regenerated Goa output for updated DSL description

Resolves 8 review threads.

Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
Address review comment from @prabodhcs:

- committee_writer_test.go: add wantFulltext column to
  TestBuildCommitteeIndexingConfig_PublicNameDedup to pin the Fulltext
  value and prevent silent regressions when nameAndAliases changes

Resolves 1 review thread.

Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
Address review comment from @copilot-pull-request-reviewer:

- pkg/constants/storage.go: add KVLookupPublicNamePrefix constant
- internal/domain/port/committee_writer.go: add UniquePublicName to
  CommitteeBaseWriter interface
- internal/infrastructure/nats/storage.go: implement UniquePublicName
  via NATS KV Create (same pattern as UniqueSSOGroupName)
- internal/infrastructure/mock/committee.go: implement UniquePublicName
  for test mock
- internal/service/committee_writer.go: wire uniqueness check into
  create (when PublicName != ""), update (when PublicName changes, with
  old key cleanup), and delete (clean up public_name index key) flows
- internal/service/committee_writer_test.go: add UniquePublicName to
  test mock
- internal/service/committee_member_writer_test.go: add UniquePublicName
  to test mock

Resolves 1 review thread.

Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
Address review comments from @copilot-pull-request-reviewer:

- committee_base.go: add BuildPublicNameKey() that SHA-256 hashes the
  public_name for safe NATS KV keys (raw values with spaces/special
  chars are invalid in JetStream key names)
- storage.go: use BuildPublicNameKey() instead of raw public_name in
  the KV key
- committee_writer.go: set rollbackRequired = true before returning
  UniquePublicName errors in the create flow so earlier reservations
  (UniqueNameProject) are cleaned up by the deferred rollback
- committee_writer.go: use BuildPublicNameKey() for stale key cleanup
  in update and delete flows
- mock/committee.go: align mock key format with hashed pattern

Resolves 2 review threads.

Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
Address CodeRabbit review comments:

- committee_writer.go: set rollbackRequired before SSO reservation error
  return in create flow so earlier keys are cleaned up (per @coderabbitai)
- committee_writer.go: move updateSucceeded=true right after UpdateBase
  succeeds so staleKeys are cleaned up even if post-update steps
  (indexer, settings, publishing) fail (per @coderabbitai)

Resolves 2 review threads with code changes; 1 thread addressed with
explanation (best-effort index cleanup follows existing repo pattern).

LFXV2-2012

Signed-off-by: Mohamed Rashad <mrashad@contractor.linuxfoundation.org>
Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
…URLs

V1 Salesforce already has a public_name field that v1-sync-helper maps
to display_name in V2. Adding a separate public_name field duplicated
this existing concept. Pivot all uniqueness enforcement, KV indexing,
tags, and API surface to use display_name instead.

- Remove public_name field from model, Goa design, and API types
- Rename UniquePublicName → UniqueDisplayName across port/storage/mock
- Rename KVLookupPublicNamePrefix → KVLookupDisplayNamePrefix
- Update create/update/delete flows to enforce display_name uniqueness
- Replace public_name tag with display_name tag in Tags()
- Update response mapper and all tests
- Regenerate Goa types (make apigen)

LFXV2-2012

Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
…blic URL slugs

Per Jordan's feedback, use sso_group_name as the slug for public committee
URLs. When a committee is public, require an SSO group name even if SSO is
not enabled — both flows share the same field.

- Remove display_name uniqueness infrastructure (KV prefix, port method,
  NATS/mock implementations, test wrappers)
- Widen SSO gate in create/update/delete: SSOGroupEnabled → SSOGroupEnabled || Public
- Handle first-time SSO name generation when Public toggled on for existing committee
- Simplify delete cleanup: clean up SSO key if it exists regardless of flags
- Add sso_group_name tag to Tags() for query-service slug lookup

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
Copilot AI review requested due to automatic review settings August 6, 2026 16:12
@MRashad26
MRashad26 force-pushed the feat/LFXV2-2012-public-name branch from 2c1fb9a to 44a1279 Compare August 6, 2026 16:12

@andrest50 andrest50 left a comment

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.

Review: indexer-contract.md is stale after the public_name → sso_group_name pivot

The core logic changes look correct — extending sso_group_name generation/reservation to trigger on committee.Public == true is the right approach for slug-based public URLs, and all the rollback/cleanup fixes are solid. But the docs/indexer-contract.md changes weren't updated after the pivot away from public_name, so the contract now documents fields and behaviors that don't exist in the code.

What the contract says vs what the code does

Contract claim Actual code (HEAD 44a1279)
public_name field exists in model No such field. CommitteeBase has SSOGroupName and DisplayName only.
public_name:{value} is a searchable tag Tags() generates display_name:{value} and sso_group_name:{value} — not public_name:.
fulltext includes public_name buildCommitteeIndexingConfig builds fulltext from nameAndAliases = name + DisplayName + description. No public_name.
name_and_aliases includes public_name Same — only name + DisplayName.

What the contract should say

  • Remove the public_name field row
  • Remove the public_name:{value} tag row
  • Revert fulltext to name, display_name, description (deduplicated)
  • Revert name_and_aliases to name, display_name (deduplicated)
  • Add display_name:{value} as a new search tag (newly generated by this PR's Tags() change)
  • Add sso_group_name:{value} as a new search tag (now serves double duty as SSO slug and public URL slug)

Also: PR title is outdated

The title says "add public_name field" but that field was removed. Something like feat(committee): generate sso_group_name slug for public committees would be more accurate.

Everything else looks good to merge once the contract docs are corrected.

@andrest50 andrest50 left a comment

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.

Missing: sso_group_enabled required when public: true

The PR doesn't enforce that sso_group_enabled must be true when a committee is made public. The code uses SSOGroupEnabled || Public as the condition everywhere (slug generation, reservation, update, delete cleanup), which means a committee can be created or updated with public: true, sso_group_enabled: false and it silently gets an sso_group_name slug reserved — without the SSO flag being set.

If the intent is that enabling public requires SSO to be enabled too, a validation rule needs to be added in the Create and Update handlers:

if committee.Public && !committee.SSOGroupEnabled {
    return nil, errors.NewValidation("sso_group_enabled must be true when public is enabled")
}

Without this, the two flags are effectively decoupled and the API accepts an inconsistent state.

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

internal/service/committee_writer.go:1

  • This introduces new update-path behavior: reserving/generating an SSO group name when committee.Public is enabled (including the “public/SSO just enabled, no name change” branch). Please add/update unit tests to cover these branches (e.g., toggling Public from false→true with unchanged Name, and ensuring checkReserveSSOName is invoked and rollback flags/keys behave as expected on error).
// Copyright The Linux Foundation and each contributor to LFX.

docs/indexer-contract.md:95

  • The contract now states public_name participates in fulltext and name_and_aliases, but the provided buildCommitteeIndexingConfig implementation (in the context excerpt) currently only adds name, display_name, and description. Update the indexing config builder to include committee.PublicName (deduplicated) so runtime behavior matches this documentation.
| `fulltext` | `name`, `display_name`, `public_name`, `description` (deduplicated) |
| `name_and_aliases` | `name`, `display_name`, `public_name` (deduplicated) |

Comment thread internal/service/committee_writer.go
Copilot AI review requested due to automatic review settings August 6, 2026 23:22

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

Comment thread docs/indexer-contract.md Outdated
Comment thread internal/service/committee_writer.go
@MRashad26

Copy link
Copy Markdown
Contributor Author

Addressing @andrest50's reviews

Review 1: indexer-contract.md stale after public_name → sso_group_name pivot

Good catch — fixed in commit 3a4912c:

  • Removed the public_name field row from the data schema
  • Removed the public_name:{value} tag row
  • Added display_name:{value} and sso_group_name:{value} tag rows (matching Tags() output)
  • Corrected fulltext to name, display_name, description (deduplicated)
  • Corrected name_and_aliases to name, display_name (deduplicated)

Review 2: Missing sso_group_enabled required when public: true

This is intentional by design — Jordan's direction was that both flags independently trigger SSO group name generation. The SSOGroupEnabled || Public gate means either flag on its own is sufficient for slug generation, without coupling them. The reasoning: sso_group_name serves dual purpose (SSO group identifier + public URL slug), and a public committee needs the slug for its URL regardless of whether SSO is enabled. The SSO Group checkbox in PCC is gated by a feature flag (sso-group-committee) — when that flag is off, the backend still generates the slug for public committees without requiring the SSO checkbox.

If we added if committee.Public && !committee.SSOGroupEnabled { return error }, public committees couldn't get slugs unless an admin also toggled the SSO checkbox — which would be confusing UX when SSO isn't relevant to the committee.

PR title

Updated title separately — good call.

@MRashad26 MRashad26 changed the title feat(committee): add public_name field for slug-based public URLs feat(committee): generate sso_group_name slug for public committees Aug 6, 2026
Remove public_name field, tag, and search entries that no longer exist
after the pivot to sso_group_name. Add display_name:{value} and
sso_group_name:{value} tag rows. Correct fulltext and name_and_aliases
to match buildCommitteeIndexingConfig (name + DisplayName, no
public_name).

Addresses review from @andrest50 on PR #172.

LFXV2-2012

Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
Copilot AI review requested due to automatic review settings August 6, 2026 23:31

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (2)

internal/domain/model/committee_base.go:164

  • The new contract-bearing sso_group_name: tag is not exercised by any test; the added indexing-config test only inspects display_name:. Removing this tag or emitting the wrong value would leave the suite green while slug lookup fails. Add exact-value and empty-value omission cases for SSOGroupName.
	if c.SSOGroupName != "" {
		tag := fmt.Sprintf("sso_group_name:%s", c.SSOGroupName)
		tags = append(tags, tag)

internal/service/committee_writer.go:655

  • A public committee whose project_uid changes while its name stays the same never reaches this regeneration block, even though SSOGroupNameBuild derives the slug from both the project slug and committee name. The persisted/indexed public URL therefore keeps the old project-prefixed slug. Treat a project change like a name change for SSO-name reservation and stale-key cleanup, and add a project-move regression case.
		// Step 3.1: Regenerate SSO group name when name changed and SSO/public active
		if committee.SSOGroupEnabled || committee.Public {

@MRashad26
MRashad26 merged commit 8d335e5 into main Aug 7, 2026
11 checks passed
@MRashad26
MRashad26 deleted the feat/LFXV2-2012-public-name branch August 7, 2026 00:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants