Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(fix) Update default valueSet UUIDs #140

Merged
merged 1 commit into from
Dec 17, 2024
Merged

Conversation

denniskigen
Copy link
Member

Requirements

  • This PR has a title that briefly describes the work done, including the ticket number if there is a ticket.
  • My work conforms to the OpenMRS 3.0 Styleguide and design documentation.
  • My work includes tests or is validated by existing tests.

Summary

Updates default UUIDs for valueSets in the config schema to match UUIDs that exist in the dev3 environment by default.

Screenshots

Related Issue

Other

Updates default UUIDs for valueSets in the config schema to match UUIDs that exist in the dev3 environment by default.
@denniskigen
Copy link
Member Author

denniskigen commented Dec 17, 2024

@mogoodrich @chibongho this is a breaking change for you. I assume that the default UUIDs that are currently set exist in the PIH EMR, so this change would require you to set those values via config instead for your distro.

@denniskigen denniskigen marked this pull request as draft December 17, 2024 13:48
@denniskigen
Copy link
Member Author

denniskigen commented Dec 17, 2024

Is it intentional that reasonForPause and reasonForClose valueSets use the same CIEL concept in the dispensing app config schema? See the descriptions for the respective properties in the config schema here and here.

@chibongho
Copy link

Deferring to @mogoodrich , but I think this change won't break us as we explicit set the config values here:

https://github.com/PIH/openmrs-config-pihemr/blob/d02efb1c445406900cb5e87a1bc6034a69a71868/configuration/frontend/base-config.json#L47

Copy link
Member

@mseaton mseaton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I generally disapprove of any and all default values that refer to specific metadata, and have long felt that the refapp needs to start explicitly configuring ESMs in it's config, and not expecting all ESMs to work in it by default. I want to break this existing (anti)pattern. That said, if we are going to insist on defaults, they should be what Mark added I think - which are the default values provided by CIEL - not by some arbitrarily different values in the refapp.

@ibacher
Copy link
Member

ibacher commented Dec 17, 2024

@mseaton The goal here is actually switching from what seem to be PIH-specific UUIDs to CIEL-provided UUIDs. We're not inventing new metadata for the RefApp.

@mseaton
Copy link
Member

mseaton commented Dec 17, 2024

The goal here is actually switching from what seem to be PIH-specific UUIDs to CIEL-provided UUIDs. We're not inventing new metadata for the RefApp.

If they are the CIEL UUIDs that makes sense to me if we need to have a default. I was making an assumption based on the existing description of those config properties in the existing code, all of which say something like Defaults to CIEL value set: XYZ. If they weren't actually defaulting to CIEL value sets, and this fixes that, that makes sense.

@mseaton
Copy link
Member

mseaton commented Dec 17, 2024

All this said, it would be nice if we could eliminate use of UUIDs like this entirely, and switch to mappings.

Copy link
Member

@mogoodrich mogoodrich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the intent here (as noted in the property descriptions) is for these default values to point to the default values provided by CIEL. This seems correct to me. Likely a bug/oversight on my part that these were set to PIH-specific uuids.

@denniskigen the reason (I believe) that "close" and "pause" point to the same concept is because while there was a request in our use case to have different reason sets (which we do in the PIH dictionary) the CIEL dictionary only had a single set.

So this LGTM!

@denniskigen denniskigen marked this pull request as ready for review December 17, 2024 16:56
@denniskigen
Copy link
Member Author

Merging this in shortly. Thanks all.

@denniskigen denniskigen merged commit c34c3b2 into main Dec 17, 2024
4 checks passed
@denniskigen denniskigen deleted the fix/value-set-uuids branch December 17, 2024 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants