Task#0000 Changes For Hidden Country Field - #796
Conversation
|
Warning Review limit reached
Next review available in: 35 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe pull request adds cohort-member country snapshots and a role-scoped report-filter endpoint. It also adds an Aspire Leaders-specific country-list API with validation, filtering, pagination, repository integration, and application-module wiring. ChangesCohort-member reporting
Aspire Leaders country listing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CohortMembersController
participant PostgresCohortMembersService
participant UserAndCountryData
participant CohortMembers
CohortMembersController->>PostgresCohortMembersService: Submit validated cohort and user filters
PostgresCohortMembersService->>UserAndCountryData: Resolve caller role and Regional Admin country IDs
PostgresCohortMembersService->>CohortMembers: Query permitted cohort members
CohortMembers-->>PostgresCohortMembersService: Return filtered records
PostgresCohortMembersService-->>CohortMembersController: Return standardized response
sequenceDiagram
participant Client
participant AspireLeadersSpecificController
participant AspireLeadersSpecificService
participant CountryRepository
Client->>AspireLeadersSpecificController: Submit country-list query
AspireLeadersSpecificController->>AspireLeadersSpecificService: Pass validated query
AspireLeadersSpecificService->>CountryRepository: Query filtered countries and count
CountryRepository-->>AspireLeadersSpecificService: Return countries and count
AspireLeadersSpecificService-->>AspireLeadersSpecificController: Return standardized response
AspireLeadersSpecificController-->>Client: Return HTTP 200 response
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 `@src/app.module.ts`:
- Line 99: Update the module inventory array used by onModuleInit to include
AspireLeadersSpecificModule, keeping the existing “all loaded modules” claim
accurate.
In `@src/aspire-leaders-specific/aspire-leaders-specific.controller.ts`:
- Around line 34-38: Update the OpenAPI metadata in
AspireLeadersSpecificController: change the list-countries description to state
a default limit of 500, and update the successful-response example to use limit:
500. Apply both changes in
src/aspire-leaders-specific/aspire-leaders-specific.controller.ts at lines 34-38
and 70-91.
In `@src/aspire-leaders-specific/dto/list-countries.dto.ts`:
- Around line 1-11: Run the configured Prettier formatter on the modified
imports and code in src/aspire-leaders-specific/dto/list-countries.dto.ts (lines
1-11), src/aspire-leaders-specific/aspire-leaders-specific.service.ts (lines
1-14), src/aspire-leaders-specific/aspire-leaders-specific.controller.ts (lines
10-23), src/aspire-leaders-specific/aspire-leaders-specific.module.ts (lines
1-5), and src/app.module.ts (line 39). Ensure formatting uses double quotes and
removes trailing commas according to the project configuration; no behavioral
changes are needed.
- Around line 62-65: Update the boolean query-property decorators in the
countries list DTO around `@Type`(() => Boolean) so string values are parsed
explicitly: map "true" and "false" to their corresponding booleans instead of
relying on JavaScript truthiness, while preserving optional and boolean
validation behavior.
In `@src/cohortMembers/cohortMembers.controller.ts`:
- Around line 217-237: Update reportFilterCohortMembers in
src/cohortMembers/cohortMembers.controller.ts:217-237 to obtain the caller ID
from the authenticated principal or signed service identity instead of the
userId query parameter, and reject missing, unknown, deleted, or non-admin roles
before invoking the adapter. Update the corresponding reportFilterCohortMembers
implementation in src/adapters/postgres/cohortMembers-adapter.ts:7020-7048 to
fail closed for any unrecognized or non-Regional-Admin role, preventing
unrestricted cohortId plus userId filtering.
In `@src/cohortMembers/dto/cohortMembers-report-filter.dto.ts`:
- Around line 1-31: Apply the repository Prettier configuration to the changed
TypeScript code. In src/cohortMembers/dto/cohortMembers-report-filter.dto.ts
lines 1-31, format imports, Swagger strings, and the UUID version string; in
src/adapters/cohortMembersservicelocator.ts line 4, format the DTO import; in
src/cohortMembers/entities/cohort-member.entity.ts line 73, format TypeORM
column options; in src/adapters/postgres/cohortMembers-adapter.ts lines
6954-7021, format changed string literals; and in
src/cohortMembers/cohortMembers.controller.ts lines 32-230, format the DTO
import and endpoint strings.
- Line 19: Update the UUID example in the cohort members report filter DTO to
use a valid UUIDv4 with a valid variant, replacing the current version-2 example
while preserving the existing API example metadata.
In `@src/cohortMembers/entities/cohort-member.entity.ts`:
- Around line 73-74: Add a deployment migration that backfills
user_cohort_country_id for existing CohortMembers using available historical
country-at-insertion data, and document the fallback behavior when that data
cannot be recovered. Ensure reportFilterCohortMembers continues including
applicable historical rows rather than omitting null snapshots, while preserving
the nullable column for records without a reliable country value.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 147823ee-b98c-47bc-8895-8964cbdce290
📒 Files selected for processing (12)
src/adapters/cohortMembersservicelocator.tssrc/adapters/postgres/cohortMembers-adapter.tssrc/app.module.tssrc/aspire-leaders-specific/aspire-leaders-specific.controller.tssrc/aspire-leaders-specific/aspire-leaders-specific.module.tssrc/aspire-leaders-specific/aspire-leaders-specific.service.tssrc/aspire-leaders-specific/dto/list-countries.dto.tssrc/cohortMembers/cohortMembers.controller.tssrc/cohortMembers/dto/cohortMembers-report-filter.dto.tssrc/cohortMembers/entities/cohort-member.entity.tssrc/common/utils/api-id.config.tssrc/common/utils/response.messages.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/adapters/postgres/cohortMembers-adapter.ts (2)
2527-2544: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftBackfill country snapshots for existing cohort memberships.
These assignments only populate new rows. Existing rows, including rows updated through the
existingMemberpath, retainuserCohortCountryId = NULL. Line 7052 then excludes those members from Regional Admin reports, even when their country is allowed.Add a one-time migration that fills null snapshots before this report filter is enabled. Only populate a null snapshot during later updates; do not overwrite an existing snapshot.
🤖 Prompt for 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. In `@src/adapters/postgres/cohortMembers-adapter.ts` around lines 2527 - 2544, Add a one-time migration to backfill userCohortCountryId for existing cohort memberships where the snapshot is null, resolving each member’s country through resolveUserCohortCountryId and preserving any non-null snapshot. Update the existingMember path to populate only null snapshots during later updates, without overwriting existing values, before the Regional Admin report filter at line 7052 is enabled.
7027-7028: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftDerive report scope from all assigned roles.
getFirstRoleName()orders roles byUserTenantMapping.Idand returns one role. A user with bothAdminandRegional Adminroles can pass as Regional Admin when that row returns first, and see only countries they should not. Query the assigned roles on this path and apply explicit precedence: Admin has global access; Regional Admin without Admin stays country-scoped.🤖 Prompt for 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. In `@src/adapters/postgres/cohortMembers-adapter.ts` around lines 7027 - 7028, Update the report-scope logic around getFirstRoleName in the cohort-members adapter to query all roles assigned to adminUserId instead of relying on a single first role. Apply explicit precedence so any Admin role grants global access, while Regional Admin only grants country-scoped access when Admin is absent; preserve the existing scope behavior for other roles.
🤖 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 `@src/adapters/postgres/cohortMembers-adapter.ts`:
- Line 6990: Reformat the assignment to remaining in the cohort-members adapter
so it conforms to the repository’s Prettier style and passes formatting checks,
without changing its behavior.
---
Outside diff comments:
In `@src/adapters/postgres/cohortMembers-adapter.ts`:
- Around line 2527-2544: Add a one-time migration to backfill
userCohortCountryId for existing cohort memberships where the snapshot is null,
resolving each member’s country through resolveUserCohortCountryId and
preserving any non-null snapshot. Update the existingMember path to populate
only null snapshots during later updates, without overwriting existing values,
before the Regional Admin report filter at line 7052 is enabled.
- Around line 7027-7028: Update the report-scope logic around getFirstRoleName
in the cohort-members adapter to query all roles assigned to adminUserId instead
of relying on a single first role. Apply explicit precedence so any Admin role
grants global access, while Regional Admin only grants country-scoped access
when Admin is absent; preserve the existing scope behavior for other roles.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d3c9deb3-99d4-4d36-8224-5c0b1eab16e4
📒 Files selected for processing (1)
src/adapters/postgres/cohortMembers-adapter.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/adapters/postgres/cohortMembers-adapter.ts (2)
2532-2537: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winCache the country ID for each user during bulk creation.
resolveUserCohortCountryId(userId)runs inside thecohortIdloop. A request that adds one user to multiple cohorts repeats the same database join for each cohort.Resolve once per
userIdafter user validation. Reuse the value for each new membership.As per path instructions, review TypeScript code for best practices recommended for performance.
🤖 Prompt for 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. In `@src/adapters/postgres/cohortMembers-adapter.ts` around lines 2532 - 2537, Move the resolveUserCohortCountryId call out of the cohortId loop and execute it once per validated user in the bulk-creation flow. Cache the result in the user-level scope, then reuse that value when creating every membership for the user while preserving the existing insert behavior.Source: Path instructions
7066-7084: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftBackfill historical country snapshots before enabling Regional Admin filtering.
Existing memberships can have
user_cohort_country_id = NULL, andIn(allowedCountryIds)excludes them. Add a deployment backfill with an explicit historical-data policy. Do not fall back to liveUsers.country.🤖 Prompt for 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. In `@src/adapters/postgres/cohortMembers-adapter.ts` around lines 7066 - 7084, Before enabling the Regional Admin filter in the query flow around resolveRegionalAdminCountryIds, add a deployment backfill that populates NULL user_cohort_country_id values from the intended historical snapshot source. Define and apply an explicit policy for records whose historical country cannot be resolved, and ensure the filtering logic uses only persisted cohort-country snapshots—not live Users.country.
🤖 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 `@src/cohortMembers/cohortMembers.controller.ts`:
- Around line 226-229: Update the Swagger setup in main.ts to register the
access-token scheme with addBearerAuth, then replace ApiBasicAuth with
ApiBearerAuth('access-token') on the report-filter endpoint protected by
JwtAuthGuard.
---
Outside diff comments:
In `@src/adapters/postgres/cohortMembers-adapter.ts`:
- Around line 2532-2537: Move the resolveUserCohortCountryId call out of the
cohortId loop and execute it once per validated user in the bulk-creation flow.
Cache the result in the user-level scope, then reuse that value when creating
every membership for the user while preserving the existing insert behavior.
- Around line 7066-7084: Before enabling the Regional Admin filter in the query
flow around resolveRegionalAdminCountryIds, add a deployment backfill that
populates NULL user_cohort_country_id values from the intended historical
snapshot source. Define and apply an explicit policy for records whose
historical country cannot be resolved, and ensure the filtering logic uses only
persisted cohort-country snapshots—not live Users.country.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 352b4662-efaa-403c-8e55-813bef5f9a31
📒 Files selected for processing (6)
src/adapters/postgres/cohortMembers-adapter.tssrc/app.module.tssrc/aspire-leaders-specific/aspire-leaders-specific.controller.tssrc/aspire-leaders-specific/dto/list-countries.dto.tssrc/cohortMembers/cohortMembers.controller.tssrc/cohortMembers/dto/cohortMembers-report-filter.dto.ts
|
1 participant
Summary by CodeRabbit