Adopt old/new event payloads and replace change-sets with extractDelta#837
Adopt old/new event payloads and replace change-sets with extractDelta#837nevil-mathew merged 8 commits intodevelopfrom
Conversation
WalkthroughReplaces flat changedValues with oldValues/newValues across event flows; adds utils.extractDelta and flattenLeafPaths; updates DTO signatures; tenant/tenant-query now use Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant OrgSvc as Organization Service
participant DB as Database
participant Utils as Utils.extractDelta
participant Broadcaster as Event Broadcaster
Client->>OrgSvc: updateOrganization(bodyData)
OrgSvc->>DB: fetch orgDetails (before, raw)
OrgSvc->>DB: update organization
DB-->>OrgSvc: orgDetailsAfter (raw/plain)
OrgSvc->>Utils: extractDelta(old=before, new=after, updateData=bodyData)
Utils-->>OrgSvc: newValues / {}
alt No changes
OrgSvc-->>Client: 200 OK (no-op)
else Changes detected
note right of OrgSvc: oldValues (deep clone) + newValues\n(optional related_org_details fetched when present)
OrgSvc->>Broadcaster: broadcast(org.update with oldValues/newValues)
Broadcaster-->>OrgSvc: ack
OrgSvc-->>Client: 200 OK (updated)
end
sequenceDiagram
autonumber
participant Admin
participant AdminSvc as Admin Service
participant DB as Database
participant Broadcaster as Event Broadcaster
Admin->>AdminSvc: deactivateOrg(orgId)
AdminSvc->>DB: deactivate org + users
DB-->>AdminSvc: rowsAffected, orgAfter
note right of AdminSvc: Build eventType "deactivate"\nPayload: id, code, audit fields, tenant_code,\ndeactivated_users_count
AdminSvc->>Broadcaster: broadcast(deactivate event)
Broadcaster-->>AdminSvc: ack
AdminSvc-->>Admin: 200 OK (deactivated_users count)
sequenceDiagram
autonumber
participant Client
participant TenantSvc as Tenant Service
participant DB as Database
participant Utils as Utils.extractDelta
participant Broadcaster as Event Broadcaster
Client->>TenantSvc: updateTenant(bodyData)
TenantSvc->>DB: findOne(tenant, raw: true)
DB-->>TenantSvc: tenantBefore
TenantSvc->>DB: update tenant (bodyData)
DB-->>TenantSvc: tenantAfter (raw)
TenantSvc->>Utils: extractDelta(tenantBefore, tenantAfter, bodyData)
Utils-->>TenantSvc: newValues / {}
alt No changes
TenantSvc-->>Client: 200 OK (no-op)
else Changes detected
note right of TenantSvc: event includes oldValues (deep clone) and newValues
TenantSvc->>Broadcaster: broadcast(tenant.update)
Broadcaster-->>TenantSvc: ack
TenantSvc-->>Client: 200 OK
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/constants/blacklistConfig.js(1 hunks)src/database/queries/tenants.js(1 hunks)src/distributionColumns.sql(1 hunks)src/dtos/organizationDTO.js(1 hunks)src/generics/utils.js(2 hunks)src/helpers/eventBroadcasterMain.js(2 hunks)src/services/admin.js(1 hunks)src/services/organization.js(1 hunks)src/services/tenant.js(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/services/**
⚙️ CodeRabbit configuration file
This is core business logic. Please check for correctness, efficiency, and potential edge cases.
Files:
src/services/organization.jssrc/services/admin.jssrc/services/tenant.js
src/database/queries/**
⚙️ CodeRabbit configuration file
Review database queries for performance. Check for N+1 problems and ensure indexes can be used.
Files:
src/database/queries/tenants.js
🧠 Learnings (6)
📚 Learning: 2025-09-12T10:40:34.860Z
Learnt from: praveenKDass
PR: ELEVATE-Project/user#816
File: src/services/organization.js:831-0
Timestamp: 2025-09-12T10:40:34.860Z
Learning: In the ELEVATE-Project/user codebase, the organizationDTO.eventBodyDTO is designed to handle empty changes arrays properly, so organization update events should always be emitted even when changedValues is empty. The DTO handles this scenario correctly and there may be other reasons to emit events beyond just field changes (like metadata updates).
Applied to files:
src/services/organization.jssrc/dtos/organizationDTO.js
📚 Learning: 2025-09-12T11:02:51.008Z
Learnt from: praveenKDass
PR: ELEVATE-Project/user#816
File: src/dtos/organizationDTO.js:14-43
Timestamp: 2025-09-12T11:02:51.008Z
Learning: In the ELEVATE-Project/user codebase, oldValue and newValue in the changedValues array for organizationDTO.eventBodyDTO are guaranteed to not be undefined by the business logic, and filtering out falsy values (0, false, '') from changes is the intended behavior.
Applied to files:
src/services/organization.jssrc/dtos/organizationDTO.js
📚 Learning: 2025-09-12T11:02:51.008Z
Learnt from: praveenKDass
PR: ELEVATE-Project/user#816
File: src/dtos/organizationDTO.js:14-43
Timestamp: 2025-09-12T11:02:51.008Z
Learning: In the ELEVATE-Project/user codebase, the changedValues parameter in organizationDTO.eventBodyDTO is guaranteed to always be an array by the calling code, so additional Array.isArray validation is not needed.
Applied to files:
src/dtos/organizationDTO.js
📚 Learning: 2025-09-12T10:36:43.529Z
Learnt from: praveenKDass
PR: ELEVATE-Project/user#816
File: src/dtos/organizationDTO.js:26-29
Timestamp: 2025-09-12T10:36:43.529Z
Learning: In the ELEVATE-Project/user codebase, the changedValues array passed to organizationDTO.eventBodyDTO and tenantDTO.eventBodyDTO always contains a fieldName property when oldValue or newValue are present, so additional fieldName validation is not needed according to the business logic.
Applied to files:
src/dtos/organizationDTO.js
📚 Learning: 2025-09-12T10:35:28.285Z
Learnt from: praveenKDass
PR: ELEVATE-Project/user#816
File: src/services/admin.js:704-704
Timestamp: 2025-09-12T10:35:28.285Z
Learning: In ELEVATE-Project/user admin.js, there's an inconsistency in organizationQueries.update usage: addOrgAdmin calls it without options (returns number), while deactivateOrg calls it with { returning: true, raw: true } (returns object). The validation logic 'if (orgRowsAffected === 0)' only works for the first case - the second case needs 'if (orgRowsAffected.rowsAffected === 0)' to properly validate updates.
Applied to files:
src/services/admin.js
📚 Learning: 2025-09-12T10:35:28.285Z
Learnt from: praveenKDass
PR: ELEVATE-Project/user#816
File: src/services/admin.js:704-704
Timestamp: 2025-09-12T10:35:28.285Z
Learning: In the ELEVATE-Project/user codebase, when organizationQueries.update is called with { returning: true, raw: true }, it returns an object { rowsAffected, updatedRows }, not just a number. The validation logic in admin.js deactivateOrg method checking 'if (orgRowsAffected === 0)' is incorrect and should be 'if (orgRowsAffected.rowsAffected === 0)' to properly validate successful updates.
Applied to files:
src/services/admin.js
🧬 Code graph analysis (6)
src/services/organization.js (4)
src/services/tenant.js (7)
newValues(854-854)utils(29-29)oldValues(860-860)_(30-30)eventBodyData(545-562)eventBodyData(678-695)eventBodyData(862-870)src/services/admin.js (6)
utils(17-17)updatedOrgDetails(724-724)_(10-10)organizationQueries(20-20)eventBodyData(726-742)organizationDTO(43-43)src/services/user.js (4)
utils(12-12)oldValues(164-165)_(18-18)organizationQueries(16-16)src/generics/utils.js (1)
_(23-23)
src/generics/utils.js (1)
src/services/user.js (1)
_(18-18)
src/dtos/organizationDTO.js (5)
src/migrations/pushDataToKafka/pushUserDataToKafka.js (1)
eventType(182-182)src/dtos/eventBody.js (1)
disallowedArgs(8-8)src/helpers/userInvite.js (1)
args(1133-1147)src/services/tenant.js (2)
oldValues(860-860)newValues(854-854)src/services/organization.js (2)
oldValues(839-839)newValues(833-833)
src/helpers/eventBroadcasterMain.js (5)
src/services/tenant.js (2)
require(32-32)require(33-33)src/services/admin.js (5)
require(11-11)require(27-27)require(34-34)require(36-36)require(37-37)src/services/organization.js (4)
require(12-12)require(14-14)require(15-15)require(18-18)src/helpers/userInvite.js (6)
require(18-18)require(23-23)require(27-27)require(28-28)require(29-29)requestBody(969-973)src/services/userInvite.js (1)
requestBody(379-383)
src/services/admin.js (1)
src/services/organization.js (5)
eventBodyData(151-169)eventBodyData(874-894)organizationDTO(19-19)common(2-2)tenantCode(281-281)
src/services/tenant.js (2)
src/helpers/userInvite.js (5)
tenantDetails(510-510)tenantQueries(34-34)utils(3-3)oldValues(842-844)_(2-2)src/services/organization.js (6)
newValues(833-833)utils(5-5)oldValues(839-839)_(13-13)eventBodyData(151-169)eventBodyData(874-894)
🔇 Additional comments (3)
src/services/tenant.js (3)
435-442: LGTM! Consistent with codebase patterns.Using
raw: truealigns with similar queries in the codebase (e.g.,src/helpers/userInvite.js:509) and is appropriate when model instance methods aren't needed.
545-563: LGTM! Domain event payload structure is clear.The use of singular
oldValueandnewValue(vs.oldValues/newValuesin the main update) appropriately distinguishes domain-specific events from general field updates. The logic correctly captures the before/after state of the domains array.
678-696: LGTM! Consistent with addDomain event structure.The event payload correctly tracks the domain removal using
oldValue/newValuefields, maintaining consistency with theaddDomainevent pattern.
…es; update tenantQueries to fetch raw data
…leForApprovedRequest to broadcast user update events
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/services/org-admin.js (1)
661-671: Document the asymmetry betweenoldValuesandnewValuesparameters.The current implementation passes the entire old user object as
oldValuesbut only the delta (changed fields) asnewValues. While this pattern appears intentional based on similar usage insrc/services/organization.js, the parameter names suggest symmetry that doesn't exist.Consider one of the following:
- Rename parameters to better reflect their content (e.g.,
oldSnapshotandchangedFields)- Extract only the relevant old values to match the changed fields in
newValues- Add a comment explaining the intentional asymmetry
Based on learnings about similar DTOs accepting full objects with guaranteed defined values.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/dtos/userDTO.js(2 hunks)src/generics/utils.js(2 hunks)src/services/org-admin.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/generics/utils.js
🧰 Additional context used
📓 Path-based instructions (1)
src/services/**
⚙️ CodeRabbit configuration file
This is core business logic. Please check for correctness, efficiency, and potential edge cases.
Files:
src/services/org-admin.js
🧠 Learnings (4)
📓 Common learnings
Learnt from: praveenKDass
PR: ELEVATE-Project/user#816
File: src/services/organization.js:831-0
Timestamp: 2025-09-12T10:40:34.860Z
Learning: In the ELEVATE-Project/user codebase, the organizationDTO.eventBodyDTO is designed to handle empty changes arrays properly, so organization update events should always be emitted even when changedValues is empty. The DTO handles this scenario correctly and there may be other reasons to emit events beyond just field changes (like metadata updates).
Learnt from: praveenKDass
PR: ELEVATE-Project/user#816
File: src/dtos/organizationDTO.js:14-43
Timestamp: 2025-09-12T11:02:51.008Z
Learning: In the ELEVATE-Project/user codebase, oldValue and newValue in the changedValues array for organizationDTO.eventBodyDTO are guaranteed to not be undefined by the business logic, and filtering out falsy values (0, false, '') from changes is the intended behavior.
📚 Learning: 2025-09-12T11:02:51.008Z
Learnt from: praveenKDass
PR: ELEVATE-Project/user#816
File: src/dtos/organizationDTO.js:14-43
Timestamp: 2025-09-12T11:02:51.008Z
Learning: In the ELEVATE-Project/user codebase, oldValue and newValue in the changedValues array for organizationDTO.eventBodyDTO are guaranteed to not be undefined by the business logic, and filtering out falsy values (0, false, '') from changes is the intended behavior.
Applied to files:
src/dtos/userDTO.js
📚 Learning: 2025-09-12T10:40:34.860Z
Learnt from: praveenKDass
PR: ELEVATE-Project/user#816
File: src/services/organization.js:831-0
Timestamp: 2025-09-12T10:40:34.860Z
Learning: In the ELEVATE-Project/user codebase, the organizationDTO.eventBodyDTO is designed to handle empty changes arrays properly, so organization update events should always be emitted even when changedValues is empty. The DTO handles this scenario correctly and there may be other reasons to emit events beyond just field changes (like metadata updates).
Applied to files:
src/dtos/userDTO.js
📚 Learning: 2025-09-12T11:03:00.543Z
Learnt from: praveenKDass
PR: ELEVATE-Project/user#816
File: src/dtos/tenantDTO.js:23-0
Timestamp: 2025-09-12T11:03:00.543Z
Learning: In the ELEVATE-Project/user codebase, the oldValue and newValue properties in changedValues objects passed to tenantDTO.eventBodyDTO are guaranteed to be defined when present, so undefined checks are not necessary according to the data contracts.
Applied to files:
src/dtos/userDTO.js
🧬 Code graph analysis (2)
src/dtos/userDTO.js (5)
src/dtos/eventBody.js (1)
disallowedArgs(8-8)src/services/organization.js (2)
oldValues(839-839)newValues(833-833)src/services/user.js (1)
oldValues(164-165)src/helpers/userInvite.js (2)
oldValues(842-844)args(1133-1147)src/services/org-admin.js (1)
newValues(656-656)
src/services/org-admin.js (3)
src/services/admin.js (12)
require(11-11)require(27-27)require(34-34)require(36-36)require(37-37)user(55-55)user(201-204)user(339-339)user(450-450)utils(17-17)eventBody(64-81)eventBody(208-219)src/helpers/userInvite.js (9)
require(18-18)require(23-23)require(27-27)require(28-28)require(29-29)user(664-668)utils(3-3)eventBody(910-924)eventBody(1153-1158)src/database/queries/users.js (1)
user(225-270)
🔇 Additional comments (4)
src/services/org-admin.js (4)
31-32: LGTM!The new imports support the delta-based event broadcasting pattern introduced in this PR.
635-642: LGTM!The explicit null check and error throw provide clear fail-fast behavior when the role is not found.
633-683: LGTM! Well-structured refactor with clear error handling.The async refactor with delta-based event generation is well-implemented:
- Clear fail-fast behavior with explicit null checks
- Conditional event broadcasting based on deltas prevents emitting empty events
- Proper error propagation through try-catch
- Clean separation of concerns
675-676: Redis key pattern is consistent. All occurrences usecommon.redisUserPrefix + tenantCode + '_' + id(with or without.toString()), so no changes needed.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/services/tenant.js (2)
864-869: Add null check for tenantAfter parameter.The function passes
tenantAfterdirectly toutils.extractDeltaat line 866 without validating it's defined. If the update query didn't return data (despite the validation at lines 460-466, edge cases could exist), this will causeextractDeltato fail.Apply this diff to add validation:
async function tenantEventEmitter(tenantBefore, tenantAfter, bodyData) { + if (!tenantAfter) { + console.error('tenantEventEmitter called with undefined tenantAfter') + return + } + // compute delta based on attempted update fields const newValues = utils.extractDelta(tenantBefore, tenantAfter, bodyData)
874-882: Inconsistent event payload structure still exists.The main tenant update event (via
tenantEventEmitter) doesn't include theargswrapper, butaddDomain(lines 552-569) andremoveDomain(lines 690-707) events still have theargswrapper. This inconsistency could break event consumers expecting uniform structure.Apply this diff to add the
argswrapper for consistency:const eventBodyData = { entity: 'tenant', eventType: 'update', entityId: tenantBefore.code, oldValues, newValues, - created_at: tenantBefore?.created_at || new Date(), - updated_at: tenantAfter?.updated_at || new Date(), + args: { + created_by: tenantBefore.created_by, + name: tenantBefore.name, + code: tenantBefore.code, + created_at: tenantBefore?.created_at || new Date(), + updated_at: tenantAfter?.updated_at || new Date(), + status: tenantBefore?.status || common.ACTIVE_STATUS, + meta: tenantBefore?.meta || {}, + deleted: false, + description: tenantBefore.description, + logo: tenantBefore.logo, + }, }
🧹 Nitpick comments (1)
src/services/tenant.js (1)
460-470: Good validation, but optional chaining is now redundant.The validation at lines 460-466 correctly ensures
updatedRowscontains data before proceeding, addressing the previous review concern. However, line 468 still uses optional chaining (updatedRows?.[0]) even though we've already confirmedupdatedRowsis not empty.Apply this diff to simplify line 468:
- let updatedTenantDetails = updatedRows?.[0] + const updatedTenantDetails = updatedRows[0]
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/services/tenant.js(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/services/**
⚙️ CodeRabbit configuration file
This is core business logic. Please check for correctness, efficiency, and potential edge cases.
Files:
src/services/tenant.js
🧠 Learnings (1)
📚 Learning: 2025-09-12T10:35:28.285Z
Learnt from: praveenKDass
PR: ELEVATE-Project/user#816
File: src/services/admin.js:704-704
Timestamp: 2025-09-12T10:35:28.285Z
Learning: In the ELEVATE-Project/user codebase, when using organizationQueries.update with { returning: true, raw: true } options, the method consistently returns a structured result with updatedRows property when successful. The initial check for orgRowsAffected === 0 already handles failed updates, so additional null checking for updatedRows is redundant when the code execution reaches the event emission logic.
Applied to files:
src/services/tenant.js
🧬 Code graph analysis (1)
src/services/tenant.js (1)
src/services/organization.js (6)
eventBodyData(151-169)eventBodyData(874-894)newValues(833-833)utils(5-5)oldValues(839-839)_(13-13)
🔇 Additional comments (4)
src/services/tenant.js (4)
435-442: LGTM! Consistent query pattern.Using
raw: trueensures the query returns a plain object instead of a Sequelize instance, which is appropriate for event emission and avoids potential serialization issues.
494-494: LGTM! Consistent pattern for domain events.The changes align with the new event payload structure using
oldValues/newValues. Line 550 intentionally adds thedomainsfield totenantDetailsbefore creating the event payload, ensuring the old state includes domain information.Also applies to: 550-550, 556-557
596-601: LGTM! Consistent with addDomain pattern.The removeDomain implementation mirrors the addDomain changes, maintaining consistency across domain management operations.
Also applies to: 688-688, 694-695
866-866: extractDelta edge-case behavior is correct
- Flattens nested fields via
flattenLeafPathsand uses_.isEqualfor deep comparisons → nested objects, null values and unchanged fields are skipped as expected- Strict type equality means
1 !== "1"will show up in the delta (adjust only if you need coercion)
1 participant
Summary by CodeRabbit
New Features
Bug Fixes