Field updater methods should create changelogs

[lache-melvin]

The suggestion

Add insert_changelog to field updater methods.

CONTEXT: https://github.com/msupply-foundation/open-msupply/pull/4570#discussion_r1716022457

Most of the time, to update an entity, we use the repository upsert method, which updates and then inserts a changelog, so the changes are synced to where they need to go. However, we've also implemented a number of specific field update methods, e.g. update_tax. Many of these field updaters don't create changelogs.

Please change to a bug if you think its needs! I made this a refactor and not a bug issue because I don't think it's actively causing any problems. The places where we are usually using these updater methods are in conjunction with a regular upsert on the same entity, so there is a changelog created, and the field updates get included in sync. i.e. regular invoice line update, then update_tax, tax changes are also synced.

However, I recently mimicked this pattern, but to create an updater method for a entity that was not also getting a full upsert (#4570 - just wanted to updated approved_quantity field on requisition). Copy pasting, I almost missed inserting the changelog.

Why should we invest time in this?

Given we never know where those updater methods might be reused (or copied like I did) I think its pretty dangerous to assume they'll be called in the same transaction as an upsert statement on the same entity. Changes might get missed from sync.

Assuming the changes would have used to have been captured by the old changelog triggers, and these methods got missed in the move to managing changelogs in rust.

The only place where it might currently be an issue is here, I don't have cold chain set up to test it right now...:

https://github.com/msupply-foundation/open-msupply/blob/047c0ca9c0b928b2b313c9f8189b229ca30e6f7a/server/repository/src/db_diesel/temperature_log_row.rs#L77-L86

I did find a case where this was intentional:

https://github.com/msupply-foundation/open-msupply/blob/047c0ca9c0b928b2b313c9f8189b229ca30e6f7a/server/repository/src/db_diesel/sync_file_reference_row.rs#L171-L179

But if not annotated like this explicitly, I think we should insert a changelog?

Are there any risks associated with this change?

Don't think so? Changelogs are already deduped so creating multiple if we do something like the upsert and then update_tax would be fine?

How much effort is required?

Will be a quick code change, there aren't many of these updaters.. yet 😁 An hour?

Agreed Solution

[andreievg]

Triage, normal priority as I think that most granular updates happen at the same time as atomic upsert

[andreievg]

Refinement, try to find other places where we don't create change log when updating records and make sure change log is created