Stratification Factor Feature

[Yagnik56]

This PR contain all changes of adding stratification factors feature from front-end and back-end. This PR also contains backend integration test cases. We will create separate PR's for backend unit test cases and frontend test cases. The details of the tickets covered in this PR are mentioned in the document.

[danoswaltCL]

If data comes back successfully but data[0] is unexpectedly falsey, what should happen? I think this would return whatever this unexpected data data is and not navigate anywhere, which is probably not wanted.

I guess what should happen would be same as the catchError scenarios, StratificationFactorsActions.actionDeleteStratificationFactorFailure(), but does failure also need to do router.navigate?

[Yagnik56]

Giving data a type there will resolve the first point you mentioned right?
Regarding navigation don't you think success or failure in both cases we should route back to stratification tab and not at home page?

[danoswaltCL]

what i'm saying is that this is an if block without an else. So in the case where this is "successful" response from the db call but data[0] is null or undefined (such as if the data is an empty array), there is no else block to handle what to do.

[danoswaltCL]

I tried running the unit tests for deleteStratificationFactor$ to see if this was covered, but they are not really working; try making them fail, they will keep passing. I'll put the reason why in the comments for tests.

[ppratikcr7]

Okay, got it. Will resolve this.

[danoswaltCL]

will this 'N/A' string for sure come this way? and never like n/a or n / a or whatever?

[amurphy-cl]

Good point about this string. I'd suggest if we make a default string value it should be just NA. We need to make sure this is described in the import UI and handled well for MVP.

[ppratikcr7]

This if condition should be removed. It's not used now.

[danoswaltCL]

I think we're close, I think I've given this as much a review as I can and most of the comments have been resolved, ty guys, just one or two questions left open.

[danoswaltCL]

Firstly, thank you for writing unit tests, that's great. I found a weird thing here though, I found that this test is not actually running the expectations in the subscribe block. If I change expectedAction to null or /participants to something else, it still passes.

This is because the observable never emits. I'd think that this would fail the test, but apparently it does not, which is very confusing but here's what I found is happening:

The reason the observable never emits is that you're passing in the data param like this when dispatching actionDeleteStratificationFactor:

{ factor: { ...mockData[0] }.factor }

Which makes no sense, mockData is an object, so.factor is undefined here, and thus it is getting filtered out. you want { factor: mockData.factor}.

When you pass the data in that way, the test will actually run the expect code in the subscribe block.

The reason I came into the unit tests in the first place is on the comment elsewhere about this, we should be able to use the unit tests here to test what happens when data[0] is undefined. When I test this, the navigation is not called and the action dispatched is undefined.

it('this should do something if the data coming back is unexpected or empty array, but it will neither navigate nor dispatch anything', fakeAsync(() => {
      stratificationFactorsDataService.deleteStratificationFactor = jest.fn().mockReturnValue(of([])); // <---return empty array from mock API response to simulate "success" but unexpected data.

      const expectedAction = StratificationFactorsActions.actionDeleteStratificationFactorSuccess({
        stratificationFactor: { ...mockData },
      });

      service.deleteStratificationFactor$.subscribe((result) => {
        expect(result).toEqual(expectedAction);
        expect(router.navigate).toHaveBeenCalledWith(['/participants']);
      });

      actions$.next(StratificationFactorsActions.actionDeleteStratificationFactor({ factor: mockData.factor }));

      tick(0);
    }));

[ppratikcr7]

Okay, got it. Will add failure actions and update the backend to return the object instead of array of object.