OCLOMRS-1053: Concept number mismatch between concept list and dictionary view

[merovingienne]

JIRA TICKET NAME:

OCLOMRS-1053: Concept number mismatch between concept list and dictionary view

Summary:

The number of total concepts shown in the dictionary view page does not match with the number in the concept list page.

[merovingienne]

@ibacher kindly take a look when possible.

[coveralls]

Coverage decreased (-0.5%) to 45.964% when pulling 6d9a0d7 on merovingienne:OCLOMRS-1053 into 0de0c31 on openmrs:master.

[ibacher]

@merovingienne Thanks for this! Looks great! Just a few small adjustments and I think this should be good to merge in.

[ibacher]

@merovingienne Playing around with this, I see it's generating an excessive number of requests to get the size of the concepts that seems to be why it's breaking the e2e test. Would you be able to have a look at that?

[merovingienne]

@ibacher Sure, I'll check why this is happening and what we can do about it. At first I was wondering if this was a random failure, but with two occurrences that seems unlikely.

[merovingienne]

@ibacher This has been a veritable rabbit hole. I found a couple of issues.

Tl;dr - Fixing the first problem caused some existing tests to fail.

1. In the AuthenticationRequired component, prop changes caused entire child component subtrees to mount, unmount and remount. This caused multiple API calls. Changed it so that children don't mount until profile data are loaded.

2. Concept counts API call URL lacked a trailing slash. This caused the API server to redirect the request, increasing call time.

Once these two were fixed, it seems some tests that unknowingly depended on the remount behaviour are now broken.

For future reference (mostly for myself):

Previously, loading profile data caused component remounts. This allowed Formik forms down the hierarchy to remount, initialising them with proper values from API calls. But without the remounts, the forms were initialised with empty values while fetching relevant data. This caused errors as Formik doesn't reinitialise by default, when the initial values variable changes.

eg: Edit Org privacy test

Fixed this by delaying Org Edit form render until org details are fetched.

Have a couple more tests to fix. Will inform once done.

[sherrif10]

cc @merovingienne , Is this a blocker?

[merovingienne]

@sherrif10 I'm debugging the tests currently. If I'm not mistaken, in the Add Concepts to Dictionary test group, the last 3 cases (tests 2-4) cannot succeed as they require a dictionary to exist first, but we are not enforcing that requirement in the tests. @ibacher could you kindly confirm this?

I'm testing now with a dictionary first created before the body of these tests. There's an API call failure as well (retrieveConcepts API call - URL excluding added concepts throws an HTTP 500).

Any help/tips would be appreciated!

[merovingienne]

Update: The retrieveConcepts API call succeeds when pointing to the OCL QA server. This means the dockerised server used for testing has a bug. I just saw that there's an updated image. Will test with that and update.

[sherrif10]

is there any good way how we can refactor these scenarios, Especially scenarios which look the same.

[ibacher]

I actually think this is fine. In general, I'm happy with the feature files being a little repetitive, as each feature should uniquely describe the steps it takes to use that feature, even if they are obvious or done everywhere.

[ibacher]

but we are not enforcing that requirement in the tests. @ibacher could you kindly confirm this?

Yes, that seems to be the case. Sorry for the difficulty!

[merovingienne]

I've fixed the integration tests now. Two unit tests are still giving me trouble though. Essentially, they need to wait until the page loads according to the defined API call sequence (component load state changes).

@ibacher I think it's time I seek your help. Could you kindly take a look at it?

[merovingienne]

@ibacher I've fixed the unit test failure by not delaying the content render in the View Concepts page. This (hopefully) allows us to close this ticket.

I'd feel better if you could read through the logic in that file and see if it's comprehensible. I'm worried if the component state machine is confusing.
File: https://github.com/openmrs/openmrs-ocl-client/pull/754/files#diff-60efd061211b613f4964e3afe828937492f8764f1f49790b47eb8cb19a778bbb

[merovingienne]

@ibacher @suruchee kind reminder on this.

[gracepotma]

Ping @suruchee, any thoughts/concerns on this?