Warn if uploading configuration will overwrite someone elses changes

[garethbowen]

Technical users use medic-conf to make configuration changes to instances and hopefully remember to commit the changes to git to track, back up, and share their work.

This can overwrite other configuration changes if...

1. a user has made changes through the admin app,
2. a user has made changes directly to the database,
3. a user made changes with medic-conf but neglected to commit their changes, or
4. a user forgets to pull updates from the repo before making their changes.

In any of these cases the configuration will be overwritten.

Instead medic-conf should warn when the configuration is different from when it was last executed. The user should be given the option to overwrite the changes (force) or update their configuration to be in sync (eg: git stash, export configuration, git stash pop).

[SCdF]

How would this work? Since presumably every time you update a config you're making changes (otherwise why are you updating it!).

Are you thinking we'd establish some kind of "authentic" changes line based on what is in git? I'm struggling to imagine how to solve this cleanly.

One way I thought of would be to store the last pushed _rev somewhere in git, presumably in a dotfile in the project directory. This way if you do a pull and the _rev is different to what is stored in git you can say that someone has changed. This would work for 1, 2 and 4, but maybe wouldn't work for 3, depending on the perspective I'm supposed to read that from.

So the flow would be:

• git pull to get the latest local medic-project changesz
• make your changes to the config file
• medic-conf whatever, which checks your .medic-conf file in the project, pulls whatever is in PROD and makes sure the rev is the same
• If it's bad, shows a diff, and gives you the option of writing out the config to a config.current.json file? Which is also in .gitignore so people don't think it's useful to commit it?
• If it's all good, uploads your new config, and then uses the returning _rev to update .medic-conf, and.... doesn't git commit it? Leaves that to you?

Is that your line of thinking @garethbowen?

[garethbowen]

Yeah that's exactly what I was thinking, I just don't like to put any implementation details in issues until they are ready to be worked on.

You would also need to store revs of forms and resources and so on.

I think it would work for number 3 because it would protect you from overwriting and prompt you to export it locally before making your changes and uploading again. You'd lose who made the original changes and why but at least you wouldn't lose the changes!

[garethbowen]

Related: medic/cht-core#1616

[garethbowen]

Ready for AT in 113-warn-config-overwrite

[newtewt]

@wtekeu I'm getting document update conflicts on app-settings when running the same command a second time.

Starting with a fresh webapp.
I run node /home/newtewt/dev/medic-conf/src/bin/medic-conf.js --accept-self-signed-certs --url=https://<user>:<pass>@localhost
Confirm y to all the prompts about not knowing if we are going to overwrite.
Once it completes, run the same command. The error below is shown.

INFO Starting action: upload-app-settings… 
{ StatusCodeError: 409 - "{\"error\":\"conflict\",\"reason\":\"Document update conflict.\"}\n"
    at new StatusCodeError (/home/newtewt/dev/medic-conf/node_modules/request-promise-core/lib/errors.js:32:15)
    at Request.plumbing.callback (/home/newtewt/dev/medic-conf/node_modules/request-promise-core/lib/plumbing.js:104:33)
    at Request.RP$callback [as _callback] (/home/newtewt/dev/medic-conf/node_modules/request-promise-core/lib/plumbing.js:46:31)
    at Request.self.callback (/home/newtewt/dev/medic-conf/node_modules/request/request.js:185:22)
    at emitTwo (events.js:126:13)
    at Request.emit (events.js:214:7)
    at Request.<anonymous> (/home/newtewt/dev/medic-conf/node_modules/request/request.js:1161:10)
    at emitOne (events.js:116:13)
    at Request.emit (events.js:211:7)
    at IncomingMessage.<anonymous> (/home/newtewt/dev/medic-conf/node_modules/request/request.js:1083:12)
  name: 'StatusCodeError',
  statusCode: 409,
  message: '409 - "{\\"error\\":\\"conflict\\",\\"reason\\":\\"Document update conflict.\\"}\\n"',
  error: '{"error":"conflict","reason":"Document update conflict."}\n',
  options: 
   { method: 'PUT',
     url: 'https://medic:8b30caf11dbd888b@localhost/medic/api/v1/settings?replace=1',
     headers: { 'Content-Type': 'application/json' },
     body: '{\n  "locales": [\n    {\n      "code": "en",\n      "name": "English"\n    },\n    {\n   

[newtewt]

Also, should we add a assume yes argument so the user doesn't have to press y a bunch when uploading a ton of forms?

[tookam]

@newtewt I have updated the branch. Also, I am not sure I understand the second comment. It is either you press y or Enter

[newtewt]

Thanks I'll review again.

As to the second comment. I was thinking similar to how apt-get has an assume yes option. So anytime a user would be prompted to select yes/ no it would just automatically answer yes. Not really apart of this ticket but might be a nice to have.

http://manpages.ubuntu.com/manpages/cosmic/man8/apt-get.8.html

[newtewt]

@garethbowen @SCdF There is an issue where the API updates app_settings after they've been uploaded. This means that if you were to run an upload from medic-conf 2 times in a row with no changes you'd be notified every time that you're going to overwrite changes because the rev has changed in couch. @wtekeu says this is because API doesn't delete existing fields it only overwrites existing and then saves. This seems to only occur if you're updating app_settings. Should we adjust API and how it saves or drop the app_settings check or something else?

[garethbowen]

Unfortunately we can't change that API without bumping the API version because other scripts rely on it operating the way it does. We could introduce a new option to make it overwrite completely which would solve this issue but it wouldn't be backwards compatible so old APIs would still be affected.

I wouldn't want to drop the rev check on the app_settings either because it's probably the most likely part to be updated outside of medic-conf (eg: through the admin UI).

@wtekeu How about we adjust the API to return the new rev. Then after it's called we can update the locally stored rev with the latest version?

[tookam]

@garethbowen It looks like the main issue is that the api listens to settings changes and then compare it the default settings (config.default.json) and then performs an update (which changes the rev one more time): https://github.com/medic/medic/blob/master/api/src/config.js#L59-L75

Which is why the rev do never match whenever an attempt to re-upload the settings is executed. And since there is a diff, it displays the prompt.

[garethbowen]

Ahh of course! That's a really old "feature" from back in the days when the config.default.json was actually useful. It is now so far away from any project's actual config that it's probably not helpful at all any more.

In addition, as part of 3.7.0 @kennsippell is working on medic/cht-core#5914 which will replace this config.default.json with the CHT reference app. We certainly wouldn't want to merge the reference app with whatever settings someone uploads.

Feel free to delete the config merging code in API. There's a chance that the code is relying on some property being defined in settings but if that happens we should fix the code to be null safe rather than updating the settings to have a default property.

[dianabarsan]

I think merging config.default.json into the app settings was useful when new features with configs were added, including the addition of new permissions. As far as I know, we can't add permissions in the admin UI, so users would be forced to use medic-conf to create the new permissions.

[garethbowen]

You're right - new permissions are why we need this... Thanks!

It still feels clunky to me to merge default every time the settings changes or api starts so I'd like to do something to fix it.

Firstly, I think we should reduce the default to just what's needed. I think this is just permissions but it's worth reviewing the file.

Secondly, should it be applied as it is now, or done as a migration when new permissions are added, or applied to permissions when accessed so the settings doc remains unchanged. I think I'm leaning towards doing it in a migration so it happens once and only once on upgrade, and adding a migration now to make sure everyone's caught up with the current default permissions.

What do you think @dianabarsan ?

[dianabarsan]

In theory, I think it's enough, in practice .. debatable.
I fear that, even with this warning feature in place, people will still overwrite configuration.
We should remember how many support requests were about people pushing the old format of permissions after such a migration.

[garethbowen]

Do you think we should keep the current merging behaviour, perhaps with just the permissions and nothing else?

[dianabarsan]

Just the permissions and only when necessary would be perfect!
In regards to the use of api/src/config.default.json, I think it also kind of filled the gap of us lacking a Schema document post 3.0. I know @SCdF is working on bringing that up to speed and re-adding it.

[tookam]

@garethbowen please note that this will not prevent the rev delta as the extra permissions will result in an update (which means new rev)

[garethbowen]

@wtekeu Yes. I think the other thing that should change is the default permissions should be applied in the /api/v1/settings endpoint controller so it gets modified before saving to the db. This way only one change happens and the rev should be all good. I think we should also keep the change listener around just in case someone updates the settings doc directly (not through the API) but it shouldn't save when there is no change to the doc. Does that make sense?

[tookam]

👍

[tookam]

@garethbowen I was reviewing it more carefully and it looks like it is impossible to know whether the change was the consequence of a direct update or that of an api request. Also, you say that it shouldn't save when there is no change to the doc but the listener is triggered only when the doc changed. Why do we need to worry about changes that result from a direct update?

[newtewt]

@wtekeu the app settings portion of this is working. The bad news is that I believe that now that we are generating forms server side. Forms are being edited and the rev is different on the server vs local. So every time you upload app forms you're being informed that you are overwriting a change. This feature seems like a whack-a-mole effort and I'm betting we'll run into this in future versions.

[garethbowen]

Sorry, I should've seen this coming...

I think Nick may be right and using the _rev may be too brittle for at least some document types. In the case of forms, we don't care about the generated model and html attachments but everything else in the doc should be identical. One solution would be to write a custom function for forms which compares the md5 of the xform attachment and the content of the doc (ignoring the rev) to determine if it's equivalent or not. That sounds like a reasonably large piece of work so we may need to consider deferring this feature to 3.8.0 to give us time to think it through carefully.

Alternatively there may be other simpler solutions that I haven't thought of...

[garethbowen]

I think this may also fix medic/cht-core#3050

[garethbowen]

This has been destabalised by other merges so we've decided to put it off to 3.8.0 to give us more time to come up with a good solution.

[garethbowen]

@wtekeu How are you getting on with this?

[garethbowen]

Ready for AT in
medic-conf: 113-warn-config-overwrite
medic-webapp: 113-warn-config-overwrite

[garethbowen]

This was already partially ATed by Nick so read through the history to see what changed. The big addition from what was previous version is now we correctly detect if you're overwriting enketo forms. Steps for testing the new part:

1. Upload a configuration to the server to get a known state. Check the _rev of the doc.
2. Upload the configuration again. Check the _rev of the doc is unchanged. Check that you weren't prompted to confirm as you weren't making a change.
3. Copy your configuration directory so you have two copies so you can pretend to be two different users.
4. In the first copy make a change to the form and upload it. Check the _rev changes and your modification is seen in the app. Check you weren't prompted to confirm because your original config was consistent with the server's config.
5. In the second copy make a different change to the form and upload it. Check that you are prompted because now you're overwriting someone else's change. Check that the three options (overwrite, cancel, view diff) all work as expected.

[ngaruko]

LTGM (warning displayed, changes in conf detected)

[1] Overwrite the changes
[2] Abort so that you can update the configuration
[3] View diff

You are trying to modify a configuration that has been modified since your last upload. Do you want to? 

[1, 2, 3]: 

Two issues however -

1. Option 2 (abort) throws an error - I think it should just skip the whole operation with a clean terminal or a aborted message.

You are trying to modify a configuration that has been modified since your last upload. Do you want to? [1, 2, 3]: 2
ERROR Error: configuration modified
    at Object.preUploadByRev (/Users/bede/Medic/medic-dev/medic-conf/src/lib/warn-upload-overwrite.js:82:15)
    at processTicksAndRejections (internal/process/task_queues.js:93:5)
    at async Object.execute (/Users/bede/Medic/medic-dev/medic-conf/src/fn/upload-resources.js:27:5)
    at async module.exports (/Users/bede/Medic/medic-dev/medic-conf/src/lib/main.js:188:5)
    at async /Users/bede/Medic/medic-dev/medic-conf/src/bin/medic-conf.js:11:18

2. Upon pressing 1 there is another prompt (so one has to enter 1 twice)
   
   Is this expected @wtekeu ?

[tookam]

@ngaruko

1. I wanted to be consistent with other medic-conf errors reporting
2. It is expected .. it checks each document being uploaded .. you will have to do that when (1) you have never used this version of medic-conf so doing it for the first time OR (2) there is a mismatch between remote and local docs