(fix) O3-2025: Visit enabled system setting should be referenced using "visit.enabled" #1099
Conversation
There was a problem hiding this comment.
LGTM, though it would be good for @ibacher to glance at it quickly since he's closer to this than me.
|
This request is currently failing on dev3:
|
|
Potentially a similar situation to #900 |
|
@ibacher @denniskigen probably because that GP doesn't exist on dev3 (and that might be okay, as long as it just goes with the default, which I believe is 'true', if it isn't present?) |
|
@ibacher, @mogoodrich, @denniskigen, @Jexsie, could we rather use the existing visits.enabled GP? Otherwise we update it to |
|
Shouldn't this change be done from the config schema itself, instead of writing it directly. |
|
The UUID in the configuration should be replaced in the config schema to |
|
@vasharma05 For system settings, the name is the identifier for the setting, so this isn't exactly the same as hard-coding an implementation-changeable variable. (In essence UUIDs are changeable across implementations because we don't always hard-code them; names for system settings, however, are properly a unique identifier for the setting), so this implementation is correct). PS "We shouldn't hardcode anything I believe, it should be done via configuration." is, I think, not the right approach. We want to allow reasonable customisation, but it's a balancing act: if too many things are configurable, the configuration schemas become harder to use for non-developers. For the most part, the goal of configuration schemas should be to enable non-developer users to tweak how things work in their systems in ways we can reasonably anticipate they will need to do so. Of course, what qualifies as "reasonable" varies depending on the component in question. Registration, for example, is a beast configuration-wise, but that's because the core functionality is basically the same, but the details of what people need to capture can vary widely. On the other hand, for things like widgets in the patient chart, I'd err on the side of creating a new MF rather than adding too many configuration options. There are technical reasons why we sometimes need to encode UUIDs in configuration schemas (because UUIDs can vary across installations), but when this happens, it should be regarded as a regrettable circumstance and where we can avoid exposing them in implementer-facing interfaces, we should always prefer to do so. |
@Ruhanga That was the intention. The mistake is in the ticket itself. |
|
Yeah, I wrote the ticketed based on this line from the config description: "_description: "UUID for the system setting's property 'visit.enabled'""... looks like there was typo there, if the global property is really called "visits.enabled", that's what we should be using. @vasharma05 agree with @ibacher Also in this case "visits.enabled" is really a configuration property defined on the back-end (as a global property, which is the way configuration properties are defined on the back end), so having a configuration property in the front-end config to point to "visits.enabled" is kind of duplicatively: having a configuration property that points to another configuration property. Implementers have full control of what to set the 'visits.enabled" global property to. |
Requirements
Summary
Screenshots
Related Issue
Other