Script to fix member affiliations for deleted work experiences #2679
Conversation
WalkthroughThis pull request introduces several changes across multiple files, primarily adding new functionalities related to member affiliations. A new asynchronous function, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
services/apps/script_executor_worker/src/activities/fix-member-affiliations/index.ts (1)
5-9: Add JSDoc documentation for the public function.Since this is a public function, please add JSDoc documentation describing:
- Purpose of the function
- Parameters and their constraints
- Return value
- Any potential errors that might be thrown
+/** + * Retrieves member IDs that have deleted work experiences for a given tenant. + * @param tenantId - The ID of the tenant + * @param limit - Maximum number of records to return + * @param offset - Number of records to skip + * @returns Array of member IDs + * @throws Error if database query fails + */ export async function getMemberIdsWithDeletedWorkexperiences(services/apps/script_executor_worker/src/types.ts (2)
30-34: LGTM! Consider adding JSDoc documentation.The interface structure is well-designed and consistent with other interfaces in the file. Consider adding JSDoc comments to document the purpose of each property, especially explaining what
testRundoes.+/** + * Arguments for fixing member affiliations after work experience deletion + */ export interface IFixMemberAffiliationsArgs { + /** The tenant ID to process */ tenantId: string + /** Starting offset for batch processing */ offset?: number + /** If true, runs in dry-run mode without making actual changes */ testRun?: boolean }
30-34: Consider adding a batch size parameter.Since this interface supports pagination through
offset, consider adding abatchSizeparameter to control how many records are processed in each batch. This would provide more flexibility in managing the processing load.export interface IFixMemberAffiliationsArgs { tenantId: string offset?: number testRun?: boolean + batchSize?: number }services/libs/data-access-layer/src/old/apps/script_executor_worker/member.repo.ts (2)
165-190: Add JSDoc documentation and input validation.The implementation looks good but could benefit from:
- JSDoc documentation describing the purpose, parameters, and return value
- Input validation for limit/offset parameters
Consider adding:
+/** + * Retrieves member IDs that have deleted work experiences + * @param tenantId - The tenant ID to filter by + * @param limit - Maximum number of results to return + * @param offset - Number of results to skip for pagination + * @returns Array of member IDs + */ async getMemberIdsWithDeletedWorkexperience(tenantId: string, limit: number, offset: number) { + if (limit < 0 || offset < 0) { + throw new Error('Limit and offset must be non-negative numbers'); + }
165-165: Consider consistent naming convention.The method name uses "workexperience" as one word, while it appears to be conventionally written as "work experience" in the codebase. Consider renaming for consistency.
- async getMemberIdsWithDeletedWorkexperience( + async getMemberIdsWithDeletedWorkExperience(services/apps/script_executor_worker/src/workflows/fixMemberAffiliations.ts (1)
30-30: Typo in log messageThere's a typo in your log message: "deleted worked experiences" should be "deleted work experiences".
Apply this diff to fix the typo:
- console.log(`No members found with deleted worked experiences!`) + console.log(`No members found with deleted work experiences!`)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
services/apps/script_executor_worker/src/activities.ts(2 hunks)services/apps/script_executor_worker/src/activities/fix-member-affiliations/index.ts(1 hunks)services/apps/script_executor_worker/src/types.ts(1 hunks)services/apps/script_executor_worker/src/workflows.ts(1 hunks)services/apps/script_executor_worker/src/workflows/fixMemberAffiliations.ts(1 hunks)services/libs/data-access-layer/src/old/apps/script_executor_worker/member.repo.ts(2 hunks)
🔇 Additional comments (8)
services/apps/script_executor_worker/src/activities/fix-member-affiliations/index.ts (2)
1-3: LGTM! Imports are appropriate and necessary.
The imports provide access to the required repository and service dependencies.
13-14: Verify repository method name consistency.
The function name and the repository method name slightly differ (Workexperiences vs Workexperience). Let's verify the correct method name in the repository.
services/apps/script_executor_worker/src/workflows.ts (2)
12-12: LGTM! Export statement is properly added.
The new function is correctly exported, maintaining consistency with the module's existing export pattern.
4-4: LGTM! Verify the imported module exists.
The import statement follows the established pattern and the function name clearly indicates its purpose.
Let's verify the imported module exists:
✅ Verification successful
Import statement is correctly implemented and module exists
The imported module exists at the correct location and exports the fixMemberAffiliations function as expected. The implementation is complete with proper error handling, pagination, and workflow execution.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of the fixMemberAffiliations module
# Expected: The file should exist and contain the exported function
fd -t f "fixMemberAffiliations.ts" services/apps/script_executor_worker/src/workflows/
Length of output: 162
Script:
#!/bin/bash
# Let's verify the content of the file to ensure it exports the function
ast-grep --pattern 'export async function fixMemberAffiliations'
Length of output: 432
Script:
#!/bin/bash
# Let's check the content of the file directly since ast-grep might not support all syntax
cat services/apps/script_executor_worker/src/workflows/fixMemberAffiliations.ts
Length of output: 1765
services/apps/script_executor_worker/src/activities.ts (2)
42-42: LGTM! Export statement maintains consistency.
The new export maintains alphabetical ordering and follows the module's pattern.
13-13: LGTM! Clean import statement.
The import follows the established pattern and the function name clearly describes its purpose.
Let's verify the imported function exists:
✅ Verification successful
Import statement is correctly implemented and function exists
The imported function getMemberIdsWithDeletedWorkexperiences exists in the correct location at services/apps/script_executor_worker/src/activities/fix-member-affiliations/index.ts and is properly exported. The implementation matches the expected functionality described in the context.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the imported function exists
ast-grep --pattern 'export function getMemberIdsWithDeletedWorkexperiences'
Length of output: 443
Script:
#!/bin/bash
# Let's check if the function exists in the specific file mentioned in the import
fd "fix-member-affiliations.ts" --exec cat {}
Length of output: 47
Script:
#!/bin/bash
# Let's try a broader search for the file and function
rg -l "getMemberIdsWithDeletedWorkexperiences"
Length of output: 262
Script:
#!/bin/bash
# Let's check the actual implementation in the source file
cat services/apps/script_executor_worker/src/activities/fix-member-affiliations/index.ts
Length of output: 627
services/libs/data-access-layer/src/old/apps/script_executor_worker/member.repo.ts (2)
5-5: LGTM!
The import of IMemberId is properly placed and follows the existing import pattern.
170-172: Verify query performance and index usage.
The query filters on tenantId and deletedAt columns. For optimal performance with pagination:
✅ Verification successful
Query performance is adequately supported by existing indexes
The verification reveals that the query has proper index coverage:
- There's an index on
organizationId(ix_memberOrganizations_organizationId) - Two unique indexes covering
memberId,organizationIdcombinations with date constraints - The query appears in batch processing contexts (profiles_worker, fix scripts) where some performance overhead is acceptable
The current indexing strategy is sufficient for the query's performance requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if appropriate indexes exist for the query
echo "Checking for indexes on memberOrganizations table..."
rg -A 5 'CREATE.*INDEX.*member.*organizations'
# Look for similar queries that might benefit from the same optimization
echo "Checking for similar queries..."
rg '"deletedAt".*null.*limit'
Length of output: 268
Script:
#!/bin/bash
# Let's try a different approach to find index definitions
# Check migration files for memberOrganizations table
echo "Checking migration files for memberOrganizations table..."
fd -e sql | xargs rg -l "memberOrganizations"
# Look for index definitions in TypeORM entity definitions
echo "Checking TypeORM entity definitions..."
ast-grep --pattern 'class $_ {
$$$
@Index($$$)
$$$
tenantId: $_
$$$
}'
# Check for the actual query usage
echo "Checking for query usage..."
rg -B 5 -A 5 'select.*distinct.*memberId.*from.*memberOrganizations'
Length of output: 5029
Script:
#!/bin/bash
# Let's check the specific migration files that might contain indexes for memberOrganizations
echo "Checking specific migration files for indexes..."
cat backend/src/database/migrations/V1682249232__missing-indexes.sql
cat backend/src/database/migrations/V1691569430__org_members_3rd_unique_index.sql
cat backend/src/database/migrations/V1691078055__add-unique-index-to-organization-members.sql
# Let's also check the entity definition for memberOrganizations
echo "Checking entity definition..."
fd -e ts | xargs rg -l "class.*MemberOrganization"
Length of output: 3524
services/apps/script_executor_worker/src/activities/fix-member-affiliations/index.ts
Outdated
Show resolved
Hide resolved
services/apps/script_executor_worker/src/activities/fix-member-affiliations/index.ts
Outdated
Show resolved
Hide resolved
services/apps/script_executor_worker/src/workflows/fixMemberAffiliations.ts
Show resolved
Hide resolved
…ev/crowd.dev into script/fix-member-affiliations
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
services/apps/script_executor_worker/src/workflows/fixMemberAffiliations.ts (2)
19-21: Add input validation and consider configurable page sizesThe function could benefit from input validation and configurable page sizes.
Consider these improvements:
+const DEFAULT_TEST_PAGE_SIZE = 10; +const DEFAULT_PROD_PAGE_SIZE = 100; + export async function fixMemberAffiliations(args: IFixMemberAffiliationsArgs) { - const MEMBER_PAGE_SIZE = args.testRun ? 10 : 100 + if (args.offset < 0) { + throw new Error('Offset must be non-negative'); + } + const MEMBER_PAGE_SIZE = args.testRun ? DEFAULT_TEST_PAGE_SIZE : DEFAULT_PROD_PAGE_SIZE const offset = args.offset || 0
23-25: Enhance logging for better observabilityThe test mode logging could be more informative and structured.
Consider enhancing the logging:
if (args.testRun) { - console.log(`Running in test mode with limit 10!`) + console.log({ + message: 'Running in test mode', + pageSize: MEMBER_PAGE_SIZE, + tenantId: args.tenantId, + level: 'INFO' + }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
services/apps/script_executor_worker/src/workflows/fixMemberAffiliations.ts(1 hunks)
🔇 Additional comments (3)
services/apps/script_executor_worker/src/workflows/fixMemberAffiliations.ts (3)
42-63:
Add concurrency control and fix ParentClosePolicy
The concurrent execution of child workflows needs better control, and the ParentClosePolicy needs to be fixed.
Apply these improvements:
+ const CONCURRENT_LIMIT = 10; // Process 10 members at a time
+ for (let i = 0; i < memberIds.length; i += CONCURRENT_LIMIT) {
+ const batch = memberIds.slice(i, i + CONCURRENT_LIMIT);
await Promise.all(
- memberIds.map((id) => {
+ batch.map((id) => {
return executeChild(memberUpdate, {
workflowId: `${TemporalWorkflowId.MEMBER_UPDATE}/${args.tenantId}/${id}`,
cancellationType: ChildWorkflowCancellationType.ABANDON,
- parentClosePolicy: ParentClosePolicy.PARENT_CLOSE_POLICY_ABANDON,
+ parentClosePolicy: ParentClosePolicy.ABANDON,
retry: {
maximumAttempts: 10,
},15-17: Consider increasing the activity timeout
The 60-second timeout might be insufficient when processing large datasets of member IDs with deleted work experiences.
Consider making the timeout configurable or increasing it:
const { getMemberIdsWithDeletedWorkexperiences } = proxyActivities<typeof activities>({
- startToCloseTimeout: '60 seconds',
+ startToCloseTimeout: '5 minutes',
})11-11: Verify the cross-service import path
The import path reaches into the profiles_worker service. This tight coupling between services might cause maintenance issues.
Consider moving shared workflows to a common location or creating a dedicated package for cross-service workflows.
services/apps/script_executor_worker/src/workflows/fixMemberAffiliations.ts
Outdated
Show resolved
Hide resolved
services/apps/script_executor_worker/src/workflows/fixMemberAffiliations.ts
Outdated
Show resolved
Hide resolved
Changes proposed ✍️
What
copilot:summary
copilot:poem
Why
How
copilot:walkthrough
Checklist ✅
Feature,Improvement, orBug.Summary by CodeRabbit
Release Notes
These changes aim to enhance the functionality and efficiency of member data handling within the application.