OrderService prevents saving Order Frequencies
Description
Activity
Mike Seaton June 3, 2021 at 11:44 AM
, yes understood. This is generally the case across all of OpenMRS - maybe an oversight to the API. While the API has some safeguards in place for auditing of changes to data (voided, date changed, changed by, immutable orders, etc) it does not, as a rule, have any guards against changes to metadata. This means that across the data model - while it might not be possible to easily change the valueNumeric of an Obs without triggering some auditing, there is virtually nothing to prevent changing the name or meaning of the concept associated with the Obs. This is true across the data model.
Burke Mamlin June 3, 2021 at 3:19 AM
Well, the intent was to preserve the integrity of the medical record and not to cause problems like this.
Do these changes mean an order's frequency could change after it had been carried out and there might be no record of those changes?
Mike Seaton June 1, 2021 at 11:20 AM
Committed to 2.3.x here: https://github.com/openmrs/openmrs-core/commit/501662be72a2472b3f8a8c144b8a877dd7d5eda9
Committed to 2.4.x here: https://github.com/openmrs/openmrs-core/commit/79fa30a61e58dc157408be9b5151c62f9c278794
Committed to master here: https://github.com/openmrs/openmrs-core/commit/71209827fec07e9c92480ab0af95d7b91b83d315
Mark Goodrich May 28, 2021 at 1:59 PM
It's a very valid point that we don't do this level of validation with anything else, and I'm in favor of removing this validation.
At bare minimum, it shouldn't fail validation if the properties aren't dirty (but implementing that level of checking doesn't really seem worth it vs removing the validation).
Mike Seaton May 28, 2021 at 1:22 PM
PR against the 2.3.x branch issued here: https://github.com/openmrs/openmrs-core/pull/3790
If approved, I will port up to 2.4.x and master.
?
Details
Assignee
Saurabh KumarSaurabh KumarReporter
Mike SeatonMike SeatonComplexity
LowFix versions
Priority
TBD
Details
Details
Assignee
Saurabh KumarReporter
Mike SeatonComplexity
Fix versions
Priority
TBD
When the Order Frequency object was added to core, in , there was validation logic intentionally included to prohibit editing an existing Order Frequency that had any orders associated with it.
See comments in this ticket: https://openmrs.atlassian.net/browse/TRUNK-4188?focusedCommentId=25165&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel
This was implemented in the OrderServiceImpl as such (and this is the current implementation from the 2.3.x branch of OpenMRS core):
This has the ultimate effect of not allowing any kind of saving of Order Frequencies at all - whether they are dirty or not (any properties have actually changed) - and regardless of the nature of these changes.
Although the intent of this validation seem clear - to ensure that modifications at a later date are unable to change the meaning of historical data (see comments from on aforementioned ticket) - this is inconsistent with pretty much all other API-level behavior in OpenMRS, and most downstream tooling is unable to cope with this.
Specifically, tools like Initializer and MetadataDeploy will strive to save one's configuration that is loaded in, and trust the API to know whether data has been modified incorrectly or not.
For no other domain do we do this. One can change concepts at will, even though Obs refer to them. One can change drugs at will, even though Orders refer to them. One can change encounter types, programs, workflows, states, and pretty much any other metadata in the system as suits their needs, without this level of validation.
We should remove this validation check, or at the very least provide a mechanism that can be used to bypass it where required.
can you please weigh in on this, given you were opinionated on the original design?
and FYI, as this relates to the bug I filed with Initializer earlier today, and my hope is that this would resolve it without any changes needed to Initializer.