From 5e83563880e40035d503e17fe2e9d1133f42288e Mon Sep 17 00:00:00 2001 From: danoswaltCL Date: Tue, 10 Dec 2024 22:23:34 -0500 Subject: [PATCH] fixed delete rollback issues, better error response --- .../api/controllers/ExperimentController.ts | 1 + .../api/services/MoocletExperimentService.ts | 113 +++++++++--------- .../DecimalAssigmentWeight.ts | 2 +- .../DeleteAssignmentsOnExperimentDelete.ts | 2 +- .../onlyExperimentPoint/NoPartitionPoint.ts | 2 +- .../scheduleJob/CompleteStartExperiment.ts | 2 +- .../scheduleJob/DeleteEndExperiment.ts | 2 +- .../Experiment/update/ExperimentEndDate.ts | 2 +- .../Experiment/update/ExperimentStartDate.ts | 2 +- ...letePreviewAssignmentOnExperimentDelete.ts | 2 +- .../Upgrade/test/integration/utils/index.ts | 2 +- .../controllers/ExperimentController.test.ts | 45 ++++--- 12 files changed, 98 insertions(+), 79 deletions(-) diff --git a/backend/packages/Upgrade/src/api/controllers/ExperimentController.ts b/backend/packages/Upgrade/src/api/controllers/ExperimentController.ts index 4d42e19e1d..cf84c17ec4 100644 --- a/backend/packages/Upgrade/src/api/controllers/ExperimentController.ts +++ b/backend/packages/Upgrade/src/api/controllers/ExperimentController.ts @@ -1059,6 +1059,7 @@ export class ExperimentController { if (moocletExperimentRef) { return await this.moocletExperimentService.syncDelete({ moocletExperimentRef, + experimentId: id, currentUser, logger: request.logger, }); diff --git a/backend/packages/Upgrade/src/api/services/MoocletExperimentService.ts b/backend/packages/Upgrade/src/api/services/MoocletExperimentService.ts index 34602996a4..51358304c5 100644 --- a/backend/packages/Upgrade/src/api/services/MoocletExperimentService.ts +++ b/backend/packages/Upgrade/src/api/services/MoocletExperimentService.ts @@ -49,7 +49,7 @@ import { ConditionValidator } from '../DTO/ExperimentDTO'; import { UserDTO } from '../DTO/UserDTO'; import { Experiment } from '../models/Experiment'; import { UpgradeLogger } from '../../lib/logger/UpgradeLogger'; -import { ASSIGNMENT_ALGORITHM } from 'types/src'; +import { ASSIGNMENT_ALGORITHM } from 'upgrade_types'; export interface SyncCreateParams { experimentDTO: MoocletExperimentDTO; @@ -60,9 +60,9 @@ export interface SyncCreateParams { export interface SyncDeleteParams { moocletExperimentRef: MoocletExperimentRef; + experimentId: string; currentUser: UserDTO; logger: UpgradeLogger; - createType?: string; } const logger = new UpgradeLogger('MoocletExperimentService'); @@ -143,41 +143,29 @@ export class MoocletExperimentService extends ExperimentService { params: SyncCreateParams ): Promise { const moocletPolicyParameters = params.experimentDTO.moocletPolicyParameters; - let moocletExperimentRefResponse: MoocletExperimentRef; - let experimentResponse: MoocletExperimentDTO; - try { - experimentResponse = await this.createExperiment(manager, params); - moocletExperimentRefResponse = await this.orchestrateMoocletCreation( - experimentResponse, - moocletPolicyParameters, - logger - ); + const experimentResponse = await this.createExperiment(manager, params); + const moocletExperimentRefResponse = await this.orchestrateMoocletCreation( + experimentResponse, + moocletPolicyParameters, + params.currentUser, + logger + ); - logger.info({ - message: 'Mooclet experiment created successfully:', - moocletExperimentRef: JSON.stringify(moocletExperimentRefResponse), - }); + logger.info({ + message: 'Mooclet experiment created successfully:', + moocletExperimentRef: JSON.stringify(moocletExperimentRefResponse), + }); - await this.saveMoocletExperimentRef(manager, moocletExperimentRefResponse); - await this.createAndSaveVersionConditionMaps(manager, moocletExperimentRefResponse); - } catch (error) { - logger.error({ - message: 'Failed to sync experiment with Mooclet', - params: JSON.stringify(params), - }); - throw new UnprocessableEntityException('Failed to sync experiment with Mooclet'); - } + await this.saveMoocletExperimentRef(manager, moocletExperimentRefResponse); + await this.createAndSaveVersionConditionMaps(manager, moocletExperimentRefResponse); experimentResponse.moocletPolicyParameters = moocletPolicyParameters; return experimentResponse; } - private async createExperiment( - manager: EntityManager, - params: SyncCreateParams - ): Promise { + private async createExperiment(manager: EntityManager, params: SyncCreateParams): Promise { const { experimentDTO, currentUser, logger, createType } = params; return (await super.create(experimentDTO, currentUser, logger, { existingEntityManager: manager, @@ -187,14 +175,14 @@ export class MoocletExperimentService extends ExperimentService { private async saveMoocletExperimentRef( manager: EntityManager, - moocletExperimentRefResponse: MoocletExperimentRef, + moocletExperimentRefResponse: MoocletExperimentRef ): Promise { await manager.save(MoocletExperimentRef, moocletExperimentRefResponse); } private async createAndSaveVersionConditionMaps( manager: EntityManager, - moocletExperimentRefResponse: MoocletExperimentRef, + moocletExperimentRefResponse: MoocletExperimentRef ): Promise { if (moocletExperimentRefResponse.versionConditionMaps) { for (const versionConditionMap of moocletExperimentRefResponse.versionConditionMaps) { @@ -214,21 +202,25 @@ export class MoocletExperimentService extends ExperimentService { private async handleDeleteMoocletTransaction(manager: EntityManager, params: SyncDeleteParams): Promise { const { moocletExperimentRef, currentUser, logger } = params; - const { experiment } = moocletExperimentRef; let deleteResponse: Experiment | undefined; try { await this.orchestrateDeleteMoocletResources(moocletExperimentRef); - deleteResponse = await super.delete(moocletExperimentRef.experimentId, currentUser, { + deleteResponse = await super.delete(params.experimentId, currentUser, { existingEntityManager: manager, }); + logger.debug({ + message: 'Upgrade experiment deleted after Mooclet create failure.', + deleteResponse, + }); + return deleteResponse; } catch (error) { logger.error({ message: 'Failed to delete experiment with Mooclet', error: error, - experimentId: experiment.id, + experimentId: params.experimentId, user: currentUser, }); throw new UnprocessableEntityException('Failed to delete experiment with Mooclet'); @@ -249,6 +241,7 @@ export class MoocletExperimentService extends ExperimentService { public async orchestrateMoocletCreation( upgradeExperiment: MoocletExperimentDTO, moocletPolicyParameters: MoocletPolicyParameters, + currentUser: UserDTO, logger: UpgradeLogger ): Promise { const newMoocletRequest: MoocletRequestBody = { @@ -307,39 +300,47 @@ export class MoocletExperimentService extends ExperimentService { message: `[Mooclet Creation] 5. Variable created (if needed):`, moocletVariableResponse: JSON.stringify(moocletVariableResponse), }); + + moocletExperimentRef.variableId = moocletVariableResponse?.id; moocletExperimentRef.variableId = moocletVariableResponse?.id; } catch (err) { - console.log('>>>>>>>>>>>>>>> mooclet creation error, roll back', err) - await this.handleMoocletCreationError(err, moocletExperimentRef, logger); + await this.handleMoocletCreationError(err, { + moocletExperimentRef, + experimentId: upgradeExperiment.id, + logger, + currentUser, + }); + throw err; } moocletExperimentRef.id = uuid(); + return moocletExperimentRef; } private async handleMoocletCreationError( - err: any, - moocletExperimentRef: MoocletExperimentRef, - logger: UpgradeLogger + actualInternalError: any, + syncDeleteParams: SyncDeleteParams ): Promise { + logger.debug({ + message: `[Mooclet Creation] Error creating all Mooclet resources, beginning rollback to delete Mooclet resources and Upgade Experiment:`, + error: JSON.stringify(actualInternalError), + }); try { // Rollback all created resources. Null resource ids will be ignored. - return await this.orchestrateDeleteMoocletResources(moocletExperimentRef); - } catch (rollbackErr) { - logger.error({ - message: '[Mooclet Creation] Rollback failed', - error: rollbackErr, - moocletExperimentRef: moocletExperimentRef, + await this.syncDelete(syncDeleteParams); + logger.info({ + message: '[Mooclet Creation] Rollback complete, Upgrade experiment was not saved.', }); + throw new UnprocessableEntityException(actualInternalError); + } catch (error) { + logger.debug({ + message: 'Mooclet creation failure', + error, + syncDeleteParams, + }); + throw error; } - - throw new Error( - JSON.stringify({ - message: '[Mooclet Creation] Failed, see sequence of events in logs for more details.', - error: err, - moocletExperimentRef: moocletExperimentRef, - }) - ); } private createMoocletVersionConditionMaps( @@ -399,8 +400,9 @@ export class MoocletExperimentService extends ExperimentService { logger.error({ message: '[Mooclet Deletion]: Failed to delete Mooclet resources', error: err, - moocletExperimentRef: moocletExperimentRef.id, + moocletExperimentRef: moocletExperimentRef, }); + throw err; } } @@ -487,9 +489,11 @@ export class MoocletExperimentService extends ExperimentService { }; try { + let nothing: any; + nothing.id; return await this.moocletDataService.postNewPolicyParameters(policyParametersRequest); } catch (err) { - throw new Error(`Failed to create policy parameters: ${err}`); + throw new Error(`Failed to create Mooclet policy parameters: ${err}`); } } @@ -509,6 +513,7 @@ export class MoocletExperimentService extends ExperimentService { try { return await this.moocletDataService.postNewVariable(variableRequest); } catch (err) { + logger.error({ message: 'Failed to create Mooclet variable' }); throw new Error(`Failed to create variable: ${err}`); } } diff --git a/backend/packages/Upgrade/test/integration/Experiment/createWithDecimal/DecimalAssigmentWeight.ts b/backend/packages/Upgrade/test/integration/Experiment/createWithDecimal/DecimalAssigmentWeight.ts index cd6ace8652..51cd629306 100644 --- a/backend/packages/Upgrade/test/integration/Experiment/createWithDecimal/DecimalAssigmentWeight.ts +++ b/backend/packages/Upgrade/test/integration/Experiment/createWithDecimal/DecimalAssigmentWeight.ts @@ -140,7 +140,7 @@ export default async function DecimalAssignmentWeight(): Promise { expect(experimentDecisionPoint.length).toEqual(updatedExperimentDoc.partitions.length); // delete the experiment - await experimentService.delete(updatedExperimentDoc.id, user, new UpgradeLogger()); + await experimentService.delete(updatedExperimentDoc.id, user, { logger: new UpgradeLogger() }); const allExperiments = await experimentService.find(new UpgradeLogger()); expect(allExperiments.length).toEqual(0); diff --git a/backend/packages/Upgrade/test/integration/Experiment/delete/DeleteAssignmentsOnExperimentDelete.ts b/backend/packages/Upgrade/test/integration/Experiment/delete/DeleteAssignmentsOnExperimentDelete.ts index 4750ef8db8..4d4df95687 100644 --- a/backend/packages/Upgrade/test/integration/Experiment/delete/DeleteAssignmentsOnExperimentDelete.ts +++ b/backend/packages/Upgrade/test/integration/Experiment/delete/DeleteAssignmentsOnExperimentDelete.ts @@ -91,7 +91,7 @@ export default async function UpdateExperimentState(): Promise { let individualAssignments = await individualEnrollmentRepository.find(); expect(individualAssignments.length).toEqual(1); - await experimentService.delete(experimentId, user, new UpgradeLogger()); + await experimentService.delete(experimentId, user, { logger: new UpgradeLogger() }); individualAssignments = await individualEnrollmentRepository.find(); expect(individualAssignments.length).toEqual(0); diff --git a/backend/packages/Upgrade/test/integration/Experiment/onlyExperimentPoint/NoPartitionPoint.ts b/backend/packages/Upgrade/test/integration/Experiment/onlyExperimentPoint/NoPartitionPoint.ts index ea6ed6d932..af35f3c9b6 100644 --- a/backend/packages/Upgrade/test/integration/Experiment/onlyExperimentPoint/NoPartitionPoint.ts +++ b/backend/packages/Upgrade/test/integration/Experiment/onlyExperimentPoint/NoPartitionPoint.ts @@ -163,7 +163,7 @@ export default async function NoPartitionPoint(): Promise { expect(experimentDecisionPoint.length).toEqual(updatedExperimentDoc.partitions.length); // delete the experiment - await experimentService.delete(updatedExperimentDoc.id, user, new UpgradeLogger()); + await experimentService.delete(updatedExperimentDoc.id, user, { logger: new UpgradeLogger() }); const allExperiments = await experimentService.find(new UpgradeLogger()); expect(allExperiments.length).toEqual(0); diff --git a/backend/packages/Upgrade/test/integration/Experiment/scheduleJob/CompleteStartExperiment.ts b/backend/packages/Upgrade/test/integration/Experiment/scheduleJob/CompleteStartExperiment.ts index 80e09674c9..756fb18e17 100644 --- a/backend/packages/Upgrade/test/integration/Experiment/scheduleJob/CompleteStartExperiment.ts +++ b/backend/packages/Upgrade/test/integration/Experiment/scheduleJob/CompleteStartExperiment.ts @@ -46,7 +46,7 @@ export default async function DeleteStartExperiment(): Promise { ]) ); - await experimentService.delete(experiments[0].id, user, new UpgradeLogger()); + await experimentService.delete(experiments[0].id, user, { logger: new UpgradeLogger() }); startExperiment = await scheduledJobService.getAllStartExperiment(new UpgradeLogger()); expect(startExperiment.length).toEqual(0); diff --git a/backend/packages/Upgrade/test/integration/Experiment/scheduleJob/DeleteEndExperiment.ts b/backend/packages/Upgrade/test/integration/Experiment/scheduleJob/DeleteEndExperiment.ts index 24d7164a16..70d69b5ced 100644 --- a/backend/packages/Upgrade/test/integration/Experiment/scheduleJob/DeleteEndExperiment.ts +++ b/backend/packages/Upgrade/test/integration/Experiment/scheduleJob/DeleteEndExperiment.ts @@ -46,7 +46,7 @@ export default async function DeleteEndExperiment(): Promise { ]) ); - await experimentService.delete(experiments[0].id, user, new UpgradeLogger()); + await experimentService.delete(experiments[0].id, user, { logger: new UpgradeLogger() }); endExperiment = await scheduledJobService.getAllEndExperiment(new UpgradeLogger()); expect(endExperiment.length).toEqual(0); diff --git a/backend/packages/Upgrade/test/integration/Experiment/update/ExperimentEndDate.ts b/backend/packages/Upgrade/test/integration/Experiment/update/ExperimentEndDate.ts index 563bf46dd2..73c9ebf394 100644 --- a/backend/packages/Upgrade/test/integration/Experiment/update/ExperimentEndDate.ts +++ b/backend/packages/Upgrade/test/integration/Experiment/update/ExperimentEndDate.ts @@ -47,7 +47,7 @@ export default async function ExperimentEndDate(): Promise { .map((timelogs) => timelogs.timeLog) ).toHaveLength(1); - await experimentService.delete(experiment.id, user, new UpgradeLogger()); + await experimentService.delete(experiment.id, user, { logger: new UpgradeLogger() }); // with updated state await experimentService.create({ ...individualAssignmentExperiment } as any, user, new UpgradeLogger()); diff --git a/backend/packages/Upgrade/test/integration/Experiment/update/ExperimentStartDate.ts b/backend/packages/Upgrade/test/integration/Experiment/update/ExperimentStartDate.ts index 21698a54eb..cef0508afd 100644 --- a/backend/packages/Upgrade/test/integration/Experiment/update/ExperimentStartDate.ts +++ b/backend/packages/Upgrade/test/integration/Experiment/update/ExperimentStartDate.ts @@ -45,7 +45,7 @@ export default async function ExperimentEndDate(): Promise { .map((timelogs) => timelogs.timeLog) ).toHaveLength(1); - await experimentService.delete(experiment.id, user, new UpgradeLogger()); + await experimentService.delete(experiment.id, user, { logger: new UpgradeLogger() }); // with updated state await experimentService.create({ ...individualAssignmentExperiment } as any, user, new UpgradeLogger()); diff --git a/backend/packages/Upgrade/test/integration/PreviewExperiment/DeletePreviewAssignmentOnExperimentDelete.ts b/backend/packages/Upgrade/test/integration/PreviewExperiment/DeletePreviewAssignmentOnExperimentDelete.ts index c52ef53c32..8ee7b35e9e 100644 --- a/backend/packages/Upgrade/test/integration/PreviewExperiment/DeletePreviewAssignmentOnExperimentDelete.ts +++ b/backend/packages/Upgrade/test/integration/PreviewExperiment/DeletePreviewAssignmentOnExperimentDelete.ts @@ -67,7 +67,7 @@ export default async function testCase(): Promise { ]) ); - await experimentService.delete(experiments[0].id, user, new UpgradeLogger()); + await experimentService.delete(experiments[0].id, user, { logger: new UpgradeLogger() }); previewUsersData = await previewService.find(new UpgradeLogger()); expect(previewUsersData[0].assignments).toBeUndefined(); diff --git a/backend/packages/Upgrade/test/integration/utils/index.ts b/backend/packages/Upgrade/test/integration/utils/index.ts index ebaca7b6e5..e719272037 100644 --- a/backend/packages/Upgrade/test/integration/utils/index.ts +++ b/backend/packages/Upgrade/test/integration/utils/index.ts @@ -131,7 +131,7 @@ export async function markExperimentPoint( export async function checkDeletedExperiment(experimentId: string, user: User): Promise { const experimentService = Container.get(ExperimentService); // delete experiment and check assignments operations - await experimentService.delete(experimentId, user, new UpgradeLogger()); + await experimentService.delete(experimentId, user, { logger: new UpgradeLogger() }); // no individual assignments const individualEnrollmentRepository = tteContainer.getCustomRepository(IndividualEnrollmentRepository); diff --git a/backend/packages/Upgrade/test/unit/controllers/ExperimentController.test.ts b/backend/packages/Upgrade/test/unit/controllers/ExperimentController.test.ts index 3d4c881324..3ac9f5e9e1 100644 --- a/backend/packages/Upgrade/test/unit/controllers/ExperimentController.test.ts +++ b/backend/packages/Upgrade/test/unit/controllers/ExperimentController.test.ts @@ -12,7 +12,9 @@ import ExperimentAssignmentServiceMock from './mocks/ExperimentAssignmentService import { MoocletExperimentService } from '../../../src/api/services/MoocletExperimentService'; import MoocletExperimentServiceMock from './mocks/MoocletExperimentServiceMock'; import { env } from './../../../src/env'; -import { SUPPORTED_MOOCLET_POLICY_NAMES } from 'upgrade_types'; +import { ASSIGNMENT_ALGORITHM, ASSIGNMENT_UNIT, CONSISTENCY_RULE, EXPERIMENT_STATE, EXPERIMENT_TYPE, FILTER_MODE, POST_EXPERIMENT_RULE, SEGMENT_TYPE } from 'upgrade_types'; +import { MoocletExperimentDTO } from './../../../src/api/DTO/MoocletExperimentDTO'; +import { ExperimentDTO } from './../../../src/api/DTO/ExperimentDTO'; describe('Experiment Controller Testing', () => { beforeAll(() => { @@ -31,27 +33,27 @@ describe('Experiment Controller Testing', () => { //asdfasdf }); - const experimentData = { + const experimentData: ExperimentDTO = { id: uuid(), name: 'string', description: 'string', context: ['home'], - state: 'inactive', - startOn: '2021-08-11T05:41:51.655Z', - consistencyRule: 'individual', - assignmentUnit: 'individual', - postExperimentRule: 'continue', - assignmentAlgorithm: 'random', + state: EXPERIMENT_STATE.INACTIVE, + startOn: new Date('2021-08-11T05:41:51.655Z'), + consistencyRule: CONSISTENCY_RULE.EXPERIMENT, + assignmentUnit: ASSIGNMENT_UNIT.INDIVIDUAL, + postExperimentRule: POST_EXPERIMENT_RULE.CONTINUE, + assignmentAlgorithm: ASSIGNMENT_ALGORITHM.RANDOM, enrollmentCompleteCondition: { userCount: 0, groupCount: 0, }, - endOn: '2021-08-11T05:41:51.655Z', + endOn: new Date('2021-08-11T05:41:51.655Z'), revertTo: 'string', tags: ['string'], group: 'string', - filterMode: 'includeAll', - type: 'Simple', + filterMode: FILTER_MODE.INCLUDE_ALL, + type: EXPERIMENT_TYPE.SIMPLE, conditions: [ { id: 'string', @@ -77,7 +79,7 @@ describe('Experiment Controller Testing', () => { individualForSegment: [], groupForSegment: [], subSegments: [], - type: 'private', + type: SEGMENT_TYPE.PRIVATE, }, }, experimentSegmentExclusion: { @@ -85,16 +87,27 @@ describe('Experiment Controller Testing', () => { individualForSegment: [], groupForSegment: [], subSegments: [], - type: 'private', + type: SEGMENT_TYPE.PRIVATE, }, }, }; - const moocletExperimentData = { + const moocletExperimentData: MoocletExperimentDTO = { ...experimentData, id: uuid(), - moocletPolicyParameters: {}, - ASSIGNMENT_ALGORITHM: SUPPORTED_MOOCLET_POLICY_NAMES.TS_CONFIGURABLE, + moocletPolicyParameters: { + prior: { + success: 1, + failure: 1 + }, + batch_size: 1, + max_rating: 1, + min_rating: 0, + uniform_threshold: 0, + tspostdiff_thresh: 0, + outcome_variable_name: "TS_CONFIG_{{randomTitle}}" + }, + assignmentAlgorithm: ASSIGNMENT_ALGORITHM.MOOCLET_TS_CONFIGURABLE, }; //for future use where user will be mocked for all testcases