Kakfa Fix for Center and User Data#727
Conversation
Kakfa Fix for Center and User Data
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting WalkthroughUpdated the Kafka user-event cohort query in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
There was a problem hiding this comment.
Code Review
This pull request updates the cohortQuery in UserService to include users directly assigned to cohorts in addition to those in batches. The changes introduce conditional logic for selecting cohort names and joining academic years. Feedback suggests that the current implementation for resolving cohortName and cohortType is inconsistent when a parent cohort exists. A suggestion was provided to simplify the query by resolving the appropriate ID within the Common Table Expression (CTE) to ensure consistent joins and data selection.
| batch."parentId" as "cohortId" | ||
| FROM public."CohortMembers" cm | ||
| JOIN public."Cohort" batch ON cm."cohortId" = batch."cohortId" | ||
| WHERE cm."userId" = $1 AND batch."type" = 'BATCH' | ||
| WHERE cm."userId" = $1 AND batch."type" IN ('BATCH', 'COHORT') | ||
| ) | ||
| SELECT | ||
| SELECT | ||
| bd.*, | ||
| cohort."name" as "cohortName", | ||
| cohort."type" as "cohortType", | ||
| COALESCE( | ||
| parent_cohort."name", | ||
| CASE WHEN bd."batchType" = 'COHORT' THEN bd."batchName" END | ||
| ) as "cohortName", | ||
| COALESCE(parent_cohort."type", CASE WHEN bd."batchType" = 'COHORT' THEN 'COHORT' END) as "cohortType", | ||
| cay."academicYearId", | ||
| ay."session" as "academicYearSession" | ||
| FROM BatchData bd | ||
| LEFT JOIN public."Cohort" cohort ON bd."cohortId":: UUID = cohort."cohortId" AND cohort."type" = 'COHORT' | ||
| LEFT JOIN public."CohortAcademicYear" cay ON bd."cohortId":: UUID = cay."cohortId" | ||
| LEFT JOIN public."Cohort" parent_cohort ON bd."cohortId"::uuid = parent_cohort."cohortId" AND parent_cohort."type" = 'COHORT' | ||
| LEFT JOIN public."CohortAcademicYear" cay ON ( | ||
| CASE WHEN bd."batchType" = 'BATCH' THEN bd."cohortId"::uuid ELSE bd."batchId"::uuid END | ||
| ) = cay."cohortId" | ||
| LEFT JOIN public."AcademicYears" ay ON cay."academicYearId" = ay."id" |
There was a problem hiding this comment.
The current logic for determining cohortName, cohortType, and the join for CohortAcademicYear is inconsistent for users directly assigned to a COHORT (Center). While the cay join correctly uses the cohort's own ID when the type is 'COHORT', the COALESCE logic for cohortName and cohortType still prefers the parent cohort if one exists (e.g., a District). This results in the Kafka message reporting the parent's name as the cohort name, while using the cohort's own academic year.
A cleaner and more consistent approach is to resolve the "Center-level" ID in the BatchData CTE. By setting cohortId to the parent ID for batches and the cohort's own ID for cohorts, all subsequent joins and selections can be simplified and will remain consistent.
| batch."parentId" as "cohortId" | |
| FROM public."CohortMembers" cm | |
| JOIN public."Cohort" batch ON cm."cohortId" = batch."cohortId" | |
| WHERE cm."userId" = $1 AND batch."type" = 'BATCH' | |
| WHERE cm."userId" = $1 AND batch."type" IN ('BATCH', 'COHORT') | |
| ) | |
| SELECT | |
| SELECT | |
| bd.*, | |
| cohort."name" as "cohortName", | |
| cohort."type" as "cohortType", | |
| COALESCE( | |
| parent_cohort."name", | |
| CASE WHEN bd."batchType" = 'COHORT' THEN bd."batchName" END | |
| ) as "cohortName", | |
| COALESCE(parent_cohort."type", CASE WHEN bd."batchType" = 'COHORT' THEN 'COHORT' END) as "cohortType", | |
| cay."academicYearId", | |
| ay."session" as "academicYearSession" | |
| FROM BatchData bd | |
| LEFT JOIN public."Cohort" cohort ON bd."cohortId":: UUID = cohort."cohortId" AND cohort."type" = 'COHORT' | |
| LEFT JOIN public."CohortAcademicYear" cay ON bd."cohortId":: UUID = cay."cohortId" | |
| LEFT JOIN public."Cohort" parent_cohort ON bd."cohortId"::uuid = parent_cohort."cohortId" AND parent_cohort."type" = 'COHORT' | |
| LEFT JOIN public."CohortAcademicYear" cay ON ( | |
| CASE WHEN bd."batchType" = 'BATCH' THEN bd."cohortId"::uuid ELSE bd."batchId"::uuid END | |
| ) = cay."cohortId" | |
| LEFT JOIN public."AcademicYears" ay ON cay."academicYearId" = ay."id" | |
| CASE WHEN batch."type" = 'BATCH' THEN batch."parentId" ELSE batch."cohortId"::text END as "cohortId" | |
| FROM public."CohortMembers" cm | |
| JOIN public."Cohort" batch ON cm."cohortId" = batch."cohortId" | |
| WHERE cm."userId" = $1 AND batch."type" IN ('BATCH', 'COHORT') | |
| ) | |
| SELECT | |
| bd.*, | |
| parent_cohort."name" as "cohortName", | |
| parent_cohort."type" as "cohortType", | |
| cay."academicYearId", | |
| ay."session" as "academicYearSession" | |
| FROM BatchData bd | |
| LEFT JOIN public."Cohort" parent_cohort ON bd."cohortId"::uuid = parent_cohort."cohortId" AND parent_cohort."type" = 'COHORT' | |
| LEFT JOIN public."CohortAcademicYear" cay ON bd."cohortId"::uuid = cay."cohortId" | |
| LEFT JOIN public."AcademicYears" ay ON cay."academicYearId" = ay."id" |



Summary by CodeRabbit