Merge pull request #3061 from CarnegieLearningWeb/bugfix/mooclet-assign-error-fix

log mooclet assignment errors but do not throw, treat as default

Author: danoswaltCL

packages/backend/src/api/services/ExperimentAssignmentService.ts

@@ -1993,7 +1993,17 @@ export class ExperimentAssignmentService {
   private async getConditionFromMoocletProxy(experiment: Experiment, user: ExperimentUser, logger: UpgradeLogger) {
     const userId = user.id;
 
-    return await this.moocletExperimentService.getConditionFromMoocletProxy(experiment, userId, logger);
+    try {
+      return await this.moocletExperimentService.getConditionFromMoocletProxy(experiment, userId, logger);
+    } catch (err) {
+      logger.error({
+        message: 'Error getting condition from Mooclet proxy; experiment will return no condition for this user',
+        experimentId: experiment.id,
+        userId,
+        error: err,
+      });
+      return undefined;
+    }
   }
 
   private assignRandom(

packages/backend/src/api/services/MoocletDataService.ts

@@ -345,8 +345,6 @@ export class MoocletDataService {
     logger: UpgradeLogger
   ): Promise<any> {
     const { method, url, body } = requestParams;
-    let jsonResponse = '';
-
     if (method && url) {
       const headers: HeadersInit = {
         Authorization: this.apiToken,
@@ -366,18 +364,30 @@ export class MoocletDataService {
         };
 
         const res = await axios.request(options);
-
-        if (res?.status?.toString().startsWith('2')) {
-          jsonResponse = res.data;
-          return jsonResponse;
+        return res.data;
+      } catch (err) {
+        if (err instanceof MoocletError) {
+          throw err;
+        }
+        if (axios.isAxiosError(err)) {
+          logger.error({
+            message: 'Error fetching data from Mooclets API',
+            url,
+            method,
+            status: err.response?.status,
+            responseBody: err.response?.data,
+            error: err,
+          });
+          throw new MoocletError(`Mooclet server returned non-2xx status: ${err.response?.status}`);
         } else {
-          return {
-            error: res,
-          };
+          logger.error({
+            message: 'Error fetching data from Mooclets API',
+            url,
+            method,
+            error: err,
+          });
+          throw new MoocletError('Failed to communicate with Mooclet server');
         }
-      } catch (err) {
-        logger.error({ message: 'Error fetching data from Mooclets API', url, method });
-        throw new MoocletError('Failed to communicate with Mooclet server');
       }
     }
   }

packages/backend/src/api/services/MoocletExperimentService.ts

@@ -1303,6 +1303,11 @@ export class MoocletExperimentService extends ExperimentService {
     logger: UpgradeLogger
   ): Promise<ExperimentCondition> {
     const moocletExperimentRef = await this.getMoocletExperimentRefByUpgradeExperimentId(experiment.id);
+
+    if (!moocletExperimentRef) {
+      throw new MoocletError(`MoocletExperimentRef not found for experiment id ${experiment.id}`);
+    }
+
     const versionResponse = await this.moocletDataService.getVersionForNewLearner(
       moocletExperimentRef.moocletId,
       userId,

packages/backend/test/unit/services/ExperimentAssignmentService.test.ts

@@ -1729,4 +1729,17 @@ describe('Experiment Assignment Service Test', () => {
       });
     });
   });
+
+  it('[getConditionFromMoocletProxy] should return undefined and log error when mooclet proxy throws', async () => {
+    const userDoc = { id: 'user123', group: {}, workingGroup: {} };
+    const exp = structuredClone(simpleIndividualAssignmentExperiment);
+    const mockError = new Error('Mooclet proxy error');
+
+    moocletExperimentServiceMock.getConditionFromMoocletProxy.rejects(mockError);
+
+    const result = await (testedModule as any).getConditionFromMoocletProxy(exp, userDoc, loggerMock);
+
+    expect(result).toBeUndefined();
+    sinon.assert.calledOnce(loggerMock.error);
+  });
 });

packages/backend/test/unit/services/MoocletDataService.test.ts

@@ -249,8 +249,8 @@ describe('#MoocletDataService', () => {
     const mockPolicyParameters: MoocletTSConfigurablePolicyParametersDTO = {
       assignmentAlgorithm: ASSIGNMENT_ALGORITHM.MOOCLET_TS_CONFIGURABLE,
       prior: {
-        failure: 1,
-        success: 1,
+        1: { failure: 1, success: 1 },
+        2: { failure: 1, success: 1 },
       },
       batch_size: 1,
       max_rating: 1,
@@ -421,23 +421,28 @@ describe('#MoocletDataService', () => {
       expect(response).toEqual(mockResponse.data);
     });
 
-    it('should return an error object when the request fails with a non-2xx status', async () => {
+    it('should throw a MoocletError when the request fails with a non-2xx status', async () => {
       const mockRequestParams: MoocletProxyRequestParams = {
         method: 'POST',
         url: 'https://api.example.com/mooclet',
         apiToken: 'test-token',
         body: { ...mockRequest },
       };
 
-      const mockErrorResponse = {
-        status: 400,
-        data: { error: 'Bad Request' },
-      };
+      const mockAxiosError = Object.assign(new Error('Request failed with status code 400'), {
+        response: {
+          status: 400,
+          data: { error: 'Bad Request' },
+        },
+        isAxiosError: true,
+      });
 
-      (axios.request as jest.Mock).mockResolvedValue(mockErrorResponse);
+      (axios.request as jest.Mock).mockRejectedValue(mockAxiosError);
+      (axios as any).isAxiosError.mockReturnValueOnce(true);
 
-      const response = await moocletDataService.fetchExternalMoocletsData(mockRequestParams, logger);
-      expect(response).toEqual({ error: mockErrorResponse });
+      await expect(moocletDataService.fetchExternalMoocletsData(mockRequestParams, logger)).rejects.toThrow(
+        'Mooclet server returned non-2xx status: 400'
+      );
     });
 
     it('should handle and log errors when the request fails', async () => {

packages/backend/test/unit/services/MoocletExperimentService.test.ts

@@ -1491,6 +1491,14 @@ describe('#MoocletExperimentService', () => {
       );
     });
 
+    it('should throw MoocletError if moocletExperimentRef is not found', async () => {
+      jest.spyOn(moocletExperimentService, 'getMoocletExperimentRefByUpgradeExperimentId').mockResolvedValue(undefined);
+
+      await expect(
+        moocletExperimentService.getConditionFromMoocletProxy(mockExperiment, mockUserId, logger)
+      ).rejects.toThrow(`MoocletExperimentRef not found for experiment id ${mockExperiment.id}`);
+    });
+
     it('should throw error if version not found in maps', async () => {
       const mockVersionResponse = { id: 999, name: 'unknown' };