Cht-core: Warn when replicating too many docs

Created on 8 Feb 2019  路  19Comments  路  Source: medic/cht-core

Users are trying to replicate more docs sometimes on purpose and sometimes due to misconfiguration. This leads to poor user experience and can impact other users by monopolising server resources. We need to do a better job of communicating the maximum number of recommended docs and also warning when users have been configured incorrectly.

  1. @SCdF Has made a start on an API to return the number of docs a user will replicate - take his PR and get it production ready and and merged. Consider caching to reduce load.
  2. When creating a user through medic-conf or the admin app query this API and warn (but don't block) the configurer if the doc count is too high.
  3. When a user replication request is received log a warning in the API log if the doc count is too high. Consider how to not spam the logs, possibly batching the writes together so it's written once per hour.
  4. When a user does an initial replication show a confirm dialog so they are aware that their doc count is unusually high.

The definition of "too high" is up for debate but I think we should warn at 10,000 docs for now. This can be raised when we improve scalability.

2 - Medium Performance

All 19 comments

@derickl This looks like the solution for malabawc on CBHIPP

@newtewt is working on a proposal to work out what how we scale with different numbers of docs which would be useful for working out when this warning should be shown.

This is somewhat related to _Server Side Purge_ #5443

Ready for AT on 5362-offline-user-info

There are conflicts on the branch....can you please have a look and resolve them before we merge @dianabarsan ?

@ngaruko conflicts have been resolved.

LGTM
Offline user replication:
image

Creating user:
image

Based on the commit comment it seems as though the warning for creating a user is shown after clicking the _Submit_ button, and then _Submit_ would need to be clicked again to proceed. This doesn't seem to be UX pattern we've used elsewhere in the app... how could we reuse a more familiar pattern to users?

It seems as though having a warning modal would have been the obvious choice -- but the add/edit user is already in a modal and we don't want to layer modals. We could move the add/edit user modal into the main panel, but that is beyond the scope of this issue.

Some other ideas:
1) Create a new page within the modal. This could work, but seems like a heavy lift and also out of scope for this issue.
1) Make the text appear with a warning styling. We'd want to differentiate this from the validation errors since you could proceed despite the warning. We could use the "warning" style from our theme.

image

I would go with option 2, and replace the Do you wish to proceed? text since there is no Yes/No option. Here is some suggested text: Warning! This user would replicate 10499 docs, which exceeds the recommended limit. Edit the Role or Place to make changes as needed, then press Submit to proceed.

Also, since the number of docs is affected by server side purge it would be worth adding a note about that to the user. For instance If there are too many docs for a user consider adjusting the doc purge rules.

_[edit: clarified wording after getting confirmation that server side purge is considered in the doc count]_

@abbyad

Also, if the number of docs affected by server side purge it would be worth adding a note about that to the user.

The plan is to update server-side-purge PR so the user-docs API would exclude purged docs and reflect the actual number of docs the user would download - but that required for this PR to be merged first.
It's a "todo" at the moment (https://github.com/medic/medic/pull/5878/files#diff-1cfd7b5c0942df518e43b5cc0bf0ded8R138)

To summarize, you are requesting that the message should be:

"Warning! This user would replicate 10499 docs, which exceeds the recommended limit. Edit the Role or Place to make changes as needed, then press Submit to proceed. If there are too many docs for a user consider adjusting the doc purge rules."

in orange warning style?

Yes, that is great, thanks.

The styling changes are available for AT on 5362-styling.
I have 2 more PR-s related to this issue:

  • one in medic (https://github.com/medic/medic/pull/5929) that implements a change in the users-info endpoint so it would accept multiple roles - related to: https://github.com/medic/medic/issues/5903
  • one in medic-conf (https://github.com/medic/medic-conf/pull/227) that adds calls to users-info endpoint prior to creating users via create-users

Do we want to branch them out to their separate issues so this one would be closed after the styling changes are ATed?

I think it is less about whether it should be done with the separate issue, and more about if you think they should be included with the same release (3.7). If you think it should, then let's include them. If you think they are high risk for regressions, or extensive time to AT, then we should consider putting them in a future release.

I think it's ok to use the same issue number and have them all ATed at the same time. Those two PRs look like they're ready for AT, just make sure you document any additional things to test, then we can merge all PRs in one fell swoop!

The styling changes requested are available for AT on 5362-styling

Further, to accomplish point nbr 2 from the issue description (When creating a user through medic-conf query this API and warn (but don't block) the configurer if the doc count is too high):
In light of how roles are used in production, I've changed the endpoint to accept multiple roles. This change is available here: 5362-array-roles.
The medic-conf update is available on this branch 5362-doc-count-warning. This requires the change in 5362-array-roles to work correctly, so please AT them together.

Styling UI
image
On user creation
image

Nice work, looks great!

One more edge case scenario to clarify @dianabarsan . Is this supposed to cover the case of an existing user being updated and assigned to a place with too many docs via an API call like so:
POST /api/v1/users/tester Content-Type: application/json { "place": "8606a91a-f454-56e3-a089-0b686af3c6b7" }

@ngaruko If you're updating the user via the Admin app, then yes. If you're cURL-ing api/v1/users, then no.
The endpoint that this feature adds is documented here: https://github.com/medic/medic/blob/5362-array-roles/api/README.md#get-apiv1users-info
It's completely separate from /api/v1/users/, and Admin app & medic-conf call it before actually creating or updating the user.

LGTM -
medic-conf create-users also has the warning.
image

Was this page helpful?
0 / 5 - 0 ratings