Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 32 additions & 1 deletion backend/src/services/memberService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
insertMemberSegmentAggregates,
queryMembersAdvanced,
} from '@crowd/data-access-layer/src/members'
import { fetchMemberOrganizations } from '@crowd/data-access-layer/src/members/organizations'
import { QueryExecutor, optionsQx } from '@crowd/data-access-layer/src/queryExecutor'
import { fetchManySegments } from '@crowd/data-access-layer/src/segments'
import { LoggerBase } from '@crowd/logging'
Expand Down Expand Up @@ -276,6 +277,8 @@ export default class MemberService extends LoggerBase {
data.displayName = data.username[data.platform].username
}

let existingOrgIds: string[] = []

const transaction = await SequelizeRepository.createTransaction(this.options)

try {
Expand Down Expand Up @@ -436,6 +439,13 @@ export default class MemberService extends LoggerBase {
let record
if (existing) {
const { id } = existing

if (data.organizations) {
existingOrgIds = (await fetchMemberOrganizations(optionsQx(this.options), id)).map(
(o) => o.organizationId,
)
}

delete existing.id
const toUpdate = CommonMemberService.membersMerge(existing, data)

Expand Down Expand Up @@ -505,6 +515,21 @@ export default class MemberService extends LoggerBase {

await SequelizeRepository.commitTransaction(transaction)

if (data.organizations) {
const commonMemberService = new CommonMemberService(
optionsQx(this.options),
this.options.temporal,
this.options.log,
)
const newOrgIds = data.organizations.map((o) => o.id)
Comment thread
ulemons marked this conversation as resolved.
Comment thread
ulemons marked this conversation as resolved.
const allAffectedOrgIds = [...new Set([...existingOrgIds, ...newOrgIds])]
await commonMemberService.startAffiliationRecalculation(
record.id,
allAffectedOrgIds,
syncToOpensearch,
)
}

if (syncToOpensearch) {
await searchSyncService.triggerMemberSync(record.id)
}
Expand Down Expand Up @@ -800,6 +825,10 @@ export default class MemberService extends LoggerBase {
) {
let transaction
try {
const existingOrgIds = data.organizations
? (await fetchMemberOrganizations(optionsQx(this.options), id)).map((o) => o.organizationId)
: []

const repoOptions = await SequelizeRepository.createTransactionalRepositoryOptions(
this.options,
)
Expand All @@ -824,9 +853,11 @@ export default class MemberService extends LoggerBase {
this.options.temporal,
this.options.log,
)
const newOrgIds = (data.organizations || []).map((o) => o.id)
Comment thread
ulemons marked this conversation as resolved.
Comment thread
ulemons marked this conversation as resolved.
const allAffectedOrgIds = [...new Set([...existingOrgIds, ...newOrgIds])]
await commonMemberService.startAffiliationRecalculation(
id,
(data.organizations || []).map((o) => o.id),
allAffectedOrgIds,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Unnecessary Temporal workflow on every member update

Medium Severity

In the update method, startAffiliationRecalculation is called unconditionally — even when data.organizations is falsy and no org changes occurred. When there are no orgs, both existingOrgIds and newOrgIds are empty, yet a Temporal memberUpdate workflow is still started (calling refreshMemberOrganizationAffiliations). The upsert method correctly guards this with if (data.organizations), but the update method lacks this guard. Every member field update (e.g. displayName) unnecessarily triggers an affiliation recalculation workflow.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit b65c266. Configure here.

syncToOpensearch,
)

Expand Down
22 changes: 22 additions & 0 deletions services/apps/data_sink_worker/src/service/member.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -481,6 +481,17 @@ export default class MemberService extends LoggerBase {
this.log,
'memberService -> create -> addToMember',
)

const commonMemberService = new CommonMemberService(
dbStoreQx(this.store),
this.temporal,
this.log,
)
await commonMemberService.startAffiliationRecalculation(
id,
orgsToAdd.map((o) => o.id),
false,
)
}
}

Expand Down Expand Up @@ -717,6 +728,17 @@ export default class MemberService extends LoggerBase {
this.log,
'memberService -> update -> addToMember',
)

const commonMemberService = new CommonMemberService(
dbStoreQx(this.store),
this.temporal,
this.log,
)
await commonMemberService.startAffiliationRecalculation(
id,
orgsToAdd.map((o) => o.id),
false,
)
}
}
} catch (err) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,10 @@ export class CommonMemberService extends LoggerBase {
return moment(v).toISOString()
}

const normalizedOrgs = organizations.map((item) =>
typeof item === 'string' ? { id: item } : item,
)

await captureApiChange(
options,
memberEditOrganizationsAction(memberId, async (captureOldState, captureNewState) => {
Expand All @@ -95,7 +99,7 @@ export class CommonMemberService extends LoggerBase {
if (replace) {
const toDelete = originalOrgs.filter(
(originalOrg: any) =>
!organizations.find(
!normalizedOrgs.find(
(newOrg) =>
originalOrg.organizationId === newOrg.id &&
(originalOrg.title === (newOrg.title || null) ||
Expand All @@ -111,17 +115,15 @@ export class CommonMemberService extends LoggerBase {
}
}

for (const item of organizations) {
const org = typeof item === 'string' ? { id: item } : item

for (const org of normalizedOrgs) {
// we don't need to touch exactly same existing work experiences
if (
!originalOrgs.some(
(w) =>
w.organizationId === item.id &&
w.title === (item.title || null) &&
w.dateStart === (item.startDate || null) &&
w.dateEnd === (item.endDate || null),
w.organizationId === org.id &&
w.title === (org.title || null) &&
w.dateStart === (org.startDate || null) &&
w.dateEnd === (org.endDate || null),
)
Comment thread
ulemons marked this conversation as resolved.
) {
const newOrg = {
Expand Down Expand Up @@ -241,6 +243,10 @@ export class CommonMemberService extends LoggerBase {
organizationIds: string[],
syncToOpensearch = false,
): Promise<void> {
this.log.info(
{ memberId, organizationIds, syncToOpensearch },
Comment thread
ulemons marked this conversation as resolved.
'Starting affiliation recalculation workflow',
)
Comment thread
ulemons marked this conversation as resolved.
await this.temporal.workflow.start('memberUpdate', {
taskQueue: 'profiles',
workflowId: `${TemporalWorkflowId.MEMBER_UPDATE}/${DEFAULT_TENANT_ID}/${memberId}`,
Expand Down
Loading