Dictionary migration - Part 2 - Execution migration#183
Dictionary migration - Part 2 - Execution migration#183leoraba wants to merge 13 commits intofeat/dictionary_migration_dbfrom
Conversation
leoraba
changed the base branch from
feat/dictionary_migration
to
feat/dictionary_migration_db
| migrationId: | ||
| type: string | ||
| description: ID of the Migration if applicable | ||
| allowEmptyValue: true |
There was a problem hiding this comment.
New optional field in the response registering a new dictionary.
| data: Partial<SubmittedData>; | ||
| audit: { | ||
| dataDiff: DataDiff; | ||
| errors?: DictionaryValidationRecordErrorDetails[]; | ||
| isMigration: boolean; | ||
| oldIsValid: boolean; | ||
| submissionId: number; | ||
| }; |
There was a problem hiding this comment.
Adding 2 new properties errors and isMigration in the args, grouping values used for auditing
| // This ensures the main thread is not affected by worker errors and can continue processing other tasks. | ||
| } | ||
| }, | ||
| dictionaryMigration: async (input) => { |
There was a problem hiding this comment.
this is the entry point function to be used to start a working thread to handle the dictionary migration.
| * **This function is designed to be executed in a worker thread.** | ||
| * Performs the Submitted data validation for a given migration, | ||
| * it iterates over all organizations and validates the submitted data for each of them. | ||
| * @param migrationId The ID of the migration to perform | ||
| * @param userName The name of the user that initiated the migration (for audit purposes) | ||
| * @returns void | ||
| */ | ||
| const performMigrationValidation = async ({ |
There was a problem hiding this comment.
added a tsdoc to indicate the reason why this function is intended to by called on a worker thread
leoraba
changed the title
leoraba
changed the title
joneubank
left a comment
joneubank
left a comment
There was a problem hiding this comment.
Generally looks good, have a couple small items to look at. Thanks!
| if (submisionUpdateData) { | ||
| logger.info(LOG_MODULE, `Updating submittedData system ID '${data.systemId}' in entity '${entityName}'`); | ||
| inputUpdate.data = data.data; | ||
| logger.info( |
There was a problem hiding this comment.
Should probably be debug level, we expect a lot of records to be updated in a migration.
There was a problem hiding this comment.
I was having a hard time reviewing the many changes to performCommitSubmissionAsync, the submission process is long with many steps and since its all inline with many checks for empty values or errors there is a lot of error handling mixed in with actual actions. Its likely that the processes could be separated into functions for each step, or more comments added to outline the process that is performed. This is likely a refactoring process separate from this PR.
| migrationService.finalizeMigration({ | ||
| migrationId, | ||
| status: 'FAILED', | ||
| userName, | ||
| }); | ||
| } |
There was a problem hiding this comment.
This finalize will likely fail for the same reason that caused it to throw an error the first time, but there is no error handling in place for this call. Will this crash the server? What is the expected outcome here?
| /** | ||
| * Update the status of the migration to COMPLETED or FAILED | ||
| * @param param0 | ||
| * @returns The ID of the finalized migration | ||
| */ | ||
| const finalizeMigration = async ({ | ||
| migrationId, | ||
| status, | ||
| userName, | ||
| }: { | ||
| migrationId: number; | ||
| status: Extract<MigrationStatus, 'COMPLETED' | 'FAILED'>; | ||
| userName: string; | ||
| }): Promise<number> => { |
There was a problem hiding this comment.
Why return the migrationId?, thats one of the input parameters. Also, the only place that calls this does not use the returned value.
This could easily return Promise<void>.
Should this return a Result instead of throwing?
| const migrationId = await initiateMigration({ | ||
| categoryId: updatedCategory.id, | ||
| fromDictionaryId: foundCategory.activeDictionaryId, | ||
| toDictionaryId: savedDictionary.id, | ||
| userName: username || '', | ||
| }); |
There was a problem hiding this comment.
initiateMigration throws on failure. Should this be in a try/catch? Maybe it should return a Result?
3 participants
Summary
Dictionary migration implementation, PR 2 of #175
This PR depends on: