Remove "Primary Contacts" from Contact Tab LHS

[kennsippell]

Description

Remove "Primary Contacts" from Contact Tab LHS
#5084

Regarding removing "Primary Contacts" from RHS: I'd like to take a bit of time to double-check the database interactions and can either push in new PR or merge here.

To keep things simple at first, this creates a new hydrateDataRecords which defaults to true. I suspect we may want the interface to default to false, but looking for thoughts on that before I make any changes.

Review checklist

• Readable: Concise, well named, follows the style guide, documented if necessary.
• Documented: Announced in Changes.md in plain English. Configuration and user documentation on medic-docs
• Tested: Unit and/or e2e where appropriate
• Internationalised: All user facing text
• Backwards compatible: Works with existing data and configuration or includes a migration. Any breaking changes documented in Changes.md.

License

The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.

[garethbowen]

@kennsippell Is this ready for review?

[kennsippell]

@garethbowen Yep! Let me know what you think.

[garethbowen]

I think you're right - options.hydrateContactNames should default to false.

I've reviewed where this is called and I'm wondering if we can get rid of the HydrateContactNames service altogether. If we never show the primary contact name in contacts then I think we can use the lineage shared-lib to hydrate the lineage only.

[kennsippell]

I agree with the goal to consolidate the logic in HydrateContactNames. That said, HydrateContactNames was unique in that its input is summaries. lineage's input is docs.

An option is to update lineage to operate on summary inputs. However, this would require new lineage interfaces which perform similar operations but expect summary inputs. Not great for the clarity of lineage's already nuianced interfaces.

Unsure of the path forward for this. Open to suggestions!

For now, I've just defaulted hydrateContactNames to false and kept HydrateContactNames.

[garethbowen]

@kennsippell Sorry, travis didn't detect this commit so the build didn't run so I can't merge. I can't commit a file to kick off a build because it's in your repo. Can you commit a trivial change to get travis to pick this up?

[garethbowen]

@kennsippell Thanks! Now it looks like an e2e is legitimately failing for some reason...

[kennsippell]

@garethbowen This is the change I made to get the e2e test passing again... d2e508e.

This test passed alone, but didn't pass when run with other tests in the suite. I still don't fully understand why, but the fix addresses how we mutate the parameter of the function causing side effects to downstream calls. As discussed, these are tricky investigations which can potentially be avoided with a rule like https://eslint.org/docs/rules/no-param-reassign

[garethbowen]

Almost there...

[garethbowen]

IIRC calling done with err is better than calling done.fail with err - they both fail the test but the former gives a better error message.

[kennsippell]

I believe that is true on mocha but untrue in jasmine2. For example, did a quick test in protractor and done('foo') doesn't fail an e2e test.

[garethbowen]

What about done(new Error('foo'))?

[kennsippell]

Passes

[garethbowen]

I'm shocked. That's terrible!

Can you do a quick scan to see if we're doing this anywhere else?

[kennsippell]

Looking for this specific pattern catch(done) I don't see any other cases in the tests/e2e folder.

[garethbowen]

This is quite hard to read. What about using _.extend as below?

I think the ideal solution would be to destructure opts, eg:

const query = ({ limit=50, hydrateContactNames=true }={}) => {

This is easy to read and self documenting, however it does require working out all the possible options which isn't easy.

[kennsippell]

For destructuring, options actually has more properties (skip, silent) without explicit defaults and we send the object downstream to Search which takes more properties (include_docs, startkey, etc.). So we'd have to "restructure" awkwardly there.

I opted for the extend option which is a nice single command to shallow copy and provide select defaults.

[garethbowen]

Nice one.

[kennsippell]

The reason the test was failing is because of a bug in reports.js Changes callback. If a report is inserted into the database and it is at the end of the list, it will not appear. For example a scenario with list [A, B] insert [C] will result in [A, B] but [A,B,C] is expected.

To fix, I'm changing
query({ silent: true, limit: liveList.count() });
to
query({ silent: true, limit: Math.max(50, liveList.count()) });

In the case of this test failure, the list length was 0 so no items could be inserted. The test failed when run as part of the suite because the previous test ended on the Reports page - so the page was updated via Changes. This did not repro when the test was run individually because the page was updated through other code paths. The bug did not previously manifest because of options.limit = options.limit || 50; which handles the case properly for lists of length 0 only.