Skip to content

Conversation

@mauriciozanettisalomao
Copy link
Contributor

Overview

Jira Ticket: https://linuxfoundation.atlassian.net/browse/LFXV2-755

This pull request removes the total_members and total_voting_repos fields from all committee-related API types, models, and response objects. The change ensures these fields are no longer exposed in API responses or handled within the service layer, simplifying the committee data structures and API contracts.

Note: The aggregate data can be retrieved via the query-service.

… API responses

Changes:
- Removed total_members and total_voting_repos attributes from Goa design types
- Updated service response conversion functions to exclude these fields
- Regenerated client/server types and OpenAPI specifications
- Bumped Chart version to 0.2.11

Jira Ticket: https://linuxfoundation.atlassian.net/browse/LFXV2-755

Signed-off-by: Mauricio Zanetti Salomao <[email protected]>
@mauriciozanettisalomao mauriciozanettisalomao requested a review from a team as a code owner November 18, 2025 14:43
Copilot AI review requested due to automatic review settings November 18, 2025 14:43
@coderabbitai
Copy link

coderabbitai bot commented Nov 18, 2025

Walkthrough

Removes TotalMembers and TotalVotingRepos from domain models, API DSL and response mappings; renames SSOGroupEnabledSsoGroupEnabled; adds Calendar mapping and CreatedAt/UpdatedAt for member responses; updates tests and mocks accordingly; bumps Helm chart version 0.2.10 → 0.2.11.

Changes

Cohort / File(s) Summary
Helm chart
charts/lfx-v2-committee-service/Chart.yaml
Chart version bumped from 0.2.10 to 0.2.11 (appVersion remains "latest").
Domain model
internal/domain/model/committee_base.go
Removed TotalMembers and TotalVotingRepos fields from CommitteeBase struct.
API design / DSL
cmd/committee-api/design/type.go
Removed TotalMembersAttribute() and TotalVotingReposAttribute() DSL functions and their usages in readonly attribute type definitions.
Response mapping / service
cmd/committee-api/service/committee_service_response.go
Removed mapping of TotalMembers and TotalVotingRepos; source UID now uses CommitteeBase.UID; added Calendar mapping; adjusted pointer usage in converted responses.
Tests & mocks
cmd/committee-api/service/committee_service_response_test.go, internal/infrastructure/mock/committee.go, internal/service/*_test.go, internal/service/message_handler_test.go
Updated tests and mocks to remove TotalMembers/TotalVotingRepos, reflect SSOGroupEnabledSsoGroupEnabled rename, add CreatedAt/UpdatedAt for member responses, and adjust expected pointer semantics and fields.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Domain as Domain Model
  participant Service as Committee Service
  participant Response as GOA Response

  rect rgba(100,150,250,0.06)
    Domain->>Service: provide CommitteeBase / CommitteeFull (no TotalMembers/TotalVotingRepos)
    Note right of Service: convertBaseToResponse / convertDomainToFullResponse\n(UID ← CommitteeBase.UID, Calendar added, pointer fields adjusted)
    Service-->>Response: mapped GOA response (fields & pointers, member CreatedAt/UpdatedAt)
  end

  rect rgba(200,100,100,0.06)
    Note left of Domain: Previous flow also mapped TotalMembers/TotalVotingRepos
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Focus areas:
    • cmd/committee-api/design/type.go — ensure removal of DSL functions doesn't leave dangling references.
    • cmd/committee-api/service/committee_service_response.go — confirm UID source, Calendar mapping, and pointer semantics are consistent.
    • Tests and mocks — ensure expectations and fixtures align with removed fields and renamed SsoGroupEnabled.

Possibly related PRs

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 22.22% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: removal of unused total_members and total_voting_repos fields from the codebase.
Description check ✅ Passed The description is related to the changeset, explaining what fields are being removed and why (aggregate data now comes from query-service).
Linked Issues check ✅ Passed The PR fully addresses LFXV2-755 requirements: TotalMembersAttribute() and TotalVotingReposAttribute() functions removed from design/type.go, fields removed from CommitteeBase model and API response structures, and aggregate responsibility delegated to Query Service.
Out of Scope Changes check ✅ Passed All changes are directly related to removing the two specified fields; the Chart.yaml version bump and field renaming (SSOGroupEnabled → SsoGroupEnabled) are necessary consequential changes within scope.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between d570370 and dec5606.

📒 Files selected for processing (1)
  • cmd/committee-api/service/committee_service_response_test.go (4 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: mauriciozanettisalomao
Repo: linuxfoundation/lfx-v2-committee-service PR: 18
File: cmd/committee-api/service/committee_service.go:317-321
Timestamp: 2025-08-22T18:56:22.131Z
Learning: In the lfx-v2-committee-service project, mauriciozanettisalomao prefers to defer security enhancements like CommitteeUID validation checks in deletion handlers to future PRs rather than expanding the current PR scope, maintaining focused PR boundaries while acknowledging the need to address security concerns in follow-up work.
Learnt from: mauriciozanettisalomao
Repo: linuxfoundation/lfx-v2-committee-service PR: 6
File: cmd/committee-api/service/committee_service_response_test.go:391-398
Timestamp: 2025-08-08T20:51:15.942Z
Learning: In the lfx-v2-committee-service project, the createdAt and updatedAt timestamp fields in committee data are not returned in API responses and are not used in validation logic currently, as clarified by mauriciozanettisalomao.
Learnt from: mauriciozanettisalomao
Repo: linuxfoundation/lfx-v2-committee-service PR: 16
File: internal/infrastructure/nats/storage.go:238-240
Timestamp: 2025-08-21T14:17:53.056Z
Learning: In the lfx-v2-committee-service project, mauriciozanettisalomao prefers to defer implementation of unimplemented methods like GetMemberRevision to future PRs rather than implementing them immediately, allowing for better PR scope management.
Learnt from: mauriciozanettisalomao
Repo: linuxfoundation/lfx-v2-committee-service PR: 16
File: internal/service/committee_reader.go:119-0
Timestamp: 2025-08-21T14:30:42.464Z
Learning: In the lfx-v2-committee-service project, mauriciozanettisalomao prefers to defer implementation of the GetMember orchestration method in the committee reader to future PRs, keeping it as a placeholder that returns "not yet implemented" error for better PR scope management.
Learnt from: mauriciozanettisalomao
Repo: linuxfoundation/lfx-v2-committee-service PR: 16
File: internal/service/committee_member_writer.go:158-166
Timestamp: 2025-08-21T14:55:02.207Z
Learning: In the lfx-v2-committee-service project, mauriciozanettisalomao prefers to defer implementation of placeholder validation functions (like validateUsernameExists and validateOrganizationExists in committee_member_writer.go) until their corresponding tickets are ready for implementation, rather than adding minimal validation checks, maintaining better PR scope management.
Learnt from: mauriciozanettisalomao
Repo: linuxfoundation/lfx-v2-committee-service PR: 19
File: cmd/committee-api/service/committee_service_response.go:332-416
Timestamp: 2025-08-25T14:42:30.476Z
Learning: In the lfx-v2-committee-service project, mauriciozanettisalomao has designed the PUT committee member endpoint with intentional full replacement semantics, as indicated by the endpoint description "Replace an existing committee member (requires complete resource)". The expectation is that consumers should send complete resource data even though the implementation uses partial update logic with nil-checking.
Learnt from: mauriciozanettisalomao
Repo: linuxfoundation/lfx-v2-committee-service PR: 19
File: internal/service/committee_member_writer.go:407-423
Timestamp: 2025-08-25T17:59:36.255Z
Learning: In the lfx-v2-committee-service project, the PUT committee member endpoint (/committees/{uid}/members/{member_uid}) is intentionally designed with full replacement semantics where users must send complete resource data. When optional fields are omitted from the request, the system intentionally clears/resets those fields as documented "cleanup" behavior, making the change detection logic that treats omitted fields as changes the correct implementation.
Learnt from: mauriciozanettisalomao
Repo: linuxfoundation/lfx-v2-committee-service PR: 16
File: internal/service/committee_member_writer_test.go:82-88
Timestamp: 2025-08-21T14:24:05.370Z
Learning: In the lfx-v2-committee-service project, mauriciozanettisalomao prefers to keep the DeleteMember method in mock test implementations simple, handling only member deletion from the local map rather than implementing complex uniqueness key cleanup logic. The simpler approach is sufficient for testing purposes.
Learnt from: mauriciozanettisalomao
Repo: linuxfoundation/lfx-v2-committee-service PR: 16
File: internal/infrastructure/mock/committee.go:556-586
Timestamp: 2025-08-21T14:16:11.193Z
Learning: In the lfx-v2-committee-service project, mauriciozanettisalomao prefers to keep the DeleteMember method in mock implementations simple, handling only member UID deletion rather than implementing complex logic to handle both member UIDs and lookup keys for rollback scenarios. The simpler approach is sufficient for development and testing purposes.
Learnt from: mauriciozanettisalomao
Repo: linuxfoundation/lfx-v2-committee-service PR: 16
File: internal/infrastructure/mock/committee.go:589-607
Timestamp: 2025-08-21T14:16:34.089Z
Learning: In the lfx-v2-committee-service project, mauriciozanettisalomao prefers to keep the UniqueMember method in mock implementations simple, using global email uniqueness checks rather than implementing complex per-committee scoped uniqueness logic that matches storage behavior exactly. The simpler approach is sufficient for development and testing purposes.
📚 Learning: 2025-08-25T17:59:36.255Z
Learnt from: mauriciozanettisalomao
Repo: linuxfoundation/lfx-v2-committee-service PR: 19
File: internal/service/committee_member_writer.go:407-423
Timestamp: 2025-08-25T17:59:36.255Z
Learning: In the lfx-v2-committee-service project, the PUT committee member endpoint (/committees/{uid}/members/{member_uid}) is intentionally designed with full replacement semantics where users must send complete resource data. When optional fields are omitted from the request, the system intentionally clears/resets those fields as documented "cleanup" behavior, making the change detection logic that treats omitted fields as changes the correct implementation.

Applied to files:

  • cmd/committee-api/service/committee_service_response_test.go
📚 Learning: 2025-08-25T14:42:30.476Z
Learnt from: mauriciozanettisalomao
Repo: linuxfoundation/lfx-v2-committee-service PR: 19
File: cmd/committee-api/service/committee_service_response.go:332-416
Timestamp: 2025-08-25T14:42:30.476Z
Learning: In the lfx-v2-committee-service project, mauriciozanettisalomao has designed the PUT committee member endpoint with intentional full replacement semantics, as indicated by the endpoint description "Replace an existing committee member (requires complete resource)". The expectation is that consumers should send complete resource data even though the implementation uses partial update logic with nil-checking.

Applied to files:

  • cmd/committee-api/service/committee_service_response_test.go
📚 Learning: 2025-08-25T14:51:22.941Z
Learnt from: mauriciozanettisalomao
Repo: linuxfoundation/lfx-v2-committee-service PR: 19
File: internal/infrastructure/nats/storage.go:282-311
Timestamp: 2025-08-25T14:51:22.941Z
Learning: In the lfx-v2-committee-service project, both UpdateBase and UpdateMember methods in the NATS storage implementation have the same error handling pattern where only jetstream.ErrKeyNotFound is specifically handled, while all other errors including revision mismatches fall through to errs.NewUnexpected, potentially causing HTTP 500 responses instead of proper HTTP 409 for optimistic locking failures.

Applied to files:

  • cmd/committee-api/service/committee_service_response_test.go
📚 Learning: 2025-08-08T21:06:14.690Z
Learnt from: mauriciozanettisalomao
Repo: linuxfoundation/lfx-v2-committee-service PR: 6
File: internal/service/committee_reader_test.go:0-0
Timestamp: 2025-08-08T21:06:14.690Z
Learning: In the lfx-v2-committee-service project, the `errors.NewNotFound()` function returns `NotFound{}` as a struct value (non-pointer), not a pointer. Tests should use `errs.NotFound{}` (non-pointer) when asserting error types with `assert.IsType()`.

Applied to files:

  • cmd/committee-api/service/committee_service_response_test.go
🧬 Code graph analysis (1)
cmd/committee-api/service/committee_service_response_test.go (2)
internal/domain/model/committee_base.go (1)
  • Calendar (52-54)
gen/committee_service/service.go (1)
  • CommitteeBaseWithReadonlyAttributes (69-105)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: MegaLinter
🔇 Additional comments (1)
cmd/committee-api/service/committee_service_response_test.go (1)

403-997: LGTM! Test updates correctly reflect the API changes.

The test expectations have been properly updated to:

  • Remove TotalMembers and TotalVotingRepos fields (aligning with PR objectives)
  • Rename SSOGroupEnabledSsoGroupEnabled consistently across all response types
  • Convert response fields to pointers with appropriate stringPtr calls
  • Add CreatedAt/UpdatedAt to member responses with correct nil handling for zero timestamps (lines 996-997)
  • Map the Calendar field from inline struct to model.Calendar

The test coverage is comprehensive, including edge cases for nil values, zero timestamps, and partial data.


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

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request removes the total_members and total_voting_repos fields from all committee-related API types and responses, as these aggregate values will be retrieved from a separate query service instead.

  • Removes field definitions and examples from OpenAPI v2 and v3 specifications
  • Updates generated Go HTTP server and client code to remove field mappings and validations
  • Removes DSL attribute definitions from the API design layer

Reviewed Changes

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

Show a summary per file
File Description
gen/http/openapi3.yaml Removed total_members and total_voting_repos field definitions and examples from CommitteeBaseWithReadonlyAttributes and CommitteeFullWithReadonlyAttributes schemas
gen/http/openapi3.json Removed field definitions from minified JSON OpenAPI v3 spec
gen/http/openapi.yaml Removed field definitions from OpenAPI v2 spec (Swagger 2.0)
gen/http/openapi.json Removed field definitions from minified JSON OpenAPI v2 spec
gen/http/committee_service/server/types.go Removed struct fields, field assignments in constructors, and validation logic for the removed fields
gen/http/committee_service/client/types.go Removed struct fields, field assignments in constructors, and validation logic for the removed fields
gen/committee_service/service.go Removed TotalMembers and TotalVotingRepos fields from service result types
cmd/committee-api/service/committee_service_response.go Removed field mappings in domain-to-response conversion functions
cmd/committee-api/design/type.go Removed DSL attribute functions TotalMembersAttribute() and TotalVotingReposAttribute() and their usage in type definitions
charts/lfx-v2-committee-service/Chart.yaml Bumped chart version from 0.2.10 to 0.2.11

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…voting_repos fields

Changes:
- Updated CommitteeBase struct to remove total_members and total_voting_repos attributes.
- Adjusted related test cases and mock data to reflect the removal of these fields.
- Ensured consistency across service response conversions and tests.

This refactor aligns with the recent changes to API responses and improves code clarity.

Jira Ticket: https://linuxfoundation.atlassian.net/browse/LFXV2-755

Generated with [Claude Code](https://claude.com/claude-code)

Signed-off-by: Mauricio Zanetti Salomao <[email protected]>
@mauriciozanettisalomao mauriciozanettisalomao force-pushed the feat/lfxv2-755-remove-unused-counts branch from a8b1002 to d570370 Compare November 18, 2025 16:13
This commit eliminates the intPtr function from the committee_service_response_test.go file, streamlining the test code. The stringPtr function remains intact for continued use.

Jira Ticket: https://linuxfoundation.atlassian.net/browse/LFXV2-755

Signed-off-by: Mauricio Zanetti Salomao <[email protected]>
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.

2 participants