#4445 - Cache UserSettings() lookup of org.couchdb.user

[kennsippell]

Description

The contact's tab fetches the org.couchdb.user document three times just to render. Although it isn't a particularly expensive operation, it is done frequently. Since this document has very low churn, it should be safe to cache it in UserSettings() in a similar style to Settings().

#4445 (Suggestion 8)

In this change, I'm also caching the userCtx cookie in Session. I'm assuming this is low-risk since the cookie is symantically immutable. I did not attempt to measure the impact. I'm doing this because this is another very hot codepath and the ipCookie('userCtx') function does the following:

1. Split the document.cookie string by ;
2. Split each cookie within based on =
3. JSON.parse each cookie in a try{}

Impact

I timed how long the fetch for org.couchdb.user took while loading pages and while idle on my SM-G361H. An average range was between ~8-15ms when idle and ~80-130ms while the contacts page was loading. This is not a large savings by any means (3x savings for contacts page), but this is a frequent fetch across many user-actions and experiences.

Navigating directly to http://upstream:5988/muso/_design/medic/_rewrite/#/contacts loads the org.couchdb.user 7 times (4 for any pageload + 3 for contacts page).

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]

Nice one. I think it's probably worth doing.

We don't a good idea of what to do if the userCtx doc changes. We have code in the Inbox controller that listens for this doc changing and prompts the user to refresh so they get the new permissions. Given this I think you can safely remove the Cache invalidation function. It's not particularly expensive (string concat) but it will be run on every doc change so might be worth saving...

[kennsippell]

@garethbowen That makes sense - I didn't know about the existing watch. Updated.

[kennsippell]

@garethbowen Enough changes to tests that it'd be nice to have another review.

1. You mentioned a watch on the org.couchdb.user doc, but I am not actually able to find that specifically. Got a pointer for me to the code? I tested and I believe I can edit the doc of the current user, sync, and I am not prompted to reload.
2. Some e2e tests write to org.couchdb.user object during the test, so I've changed the pattern they use for that. Now I set a default empty user-contact for all tests and tests edit the user-contact object. Feels a bit sloppy maybe? Alternative is to cache bust or something.
3. Added caching to UserDistrict just for prosperity but after updating all tests, I don't think this is used anywhere in the product. May be better to just remove it.

[garethbowen]

You mentioned a watch on the org.couchdb.user doc, but I am not actually able to find that specifically. Got a pointer for me to the code? I tested and I believe I can edit the doc of the current user, sync, and I am not prompted to reload.

Right at the bottom of the InboxCtrl there's a Changes watch that I think is supposed to do this - it may be broken though!

Some e2e tests write to org.couchdb.user object during the test, so I've changed the pattern they use for that. Now I set a default empty user-contact for all tests and tests edit the user-contact object. Feels a bit sloppy maybe? Alternative is to cache bust or something.

I like it. This helps move us away from changing too much during the test run which is great because currently changing stuff requires a browser reload which is slow and flakey. I want us to get to a point where all the users, docs, forms, etc are created before the e2e test browser is even started and then all the tests run in one go.

Added caching to UserDistrict just for prosperity but after updating all tests, I don't think this is used anywhere in the product. May be better to just remove it.

Yeah it looks like you're right! Delete it! Good spotting.

[garethbowen]

@kennsippell Did you get the prompt to show when the user settings doc changed?

[kennsippell]

@garethbowen Yep. Only fires when you change roles. I was being too conservative with my changes to the document.

[garethbowen]

Nice.