O3-5253: Create flags domain in Initializer#309
O3-5253: Create flags domain in Initializer#309brandones wants to merge 13 commits intomekomsolutions:mainfrom
Conversation
|
ping @kdaud |
ibacher
left a comment
ibacher
left a comment
There was a problem hiding this comment.
This is mostly some minor points that should be fixed, but a few larger comments:
- Most module domains should go exclusively in the
apiproject unless they actually rely on platform APIs that aren't available. That doesn't seem to be the case here. - Please update the changelog part of the README as well.
| * Parses a list of DisplayPoint identifiers (UUID or name) and fetches the corresponding | ||
| * DisplayPoint entities. | ||
| */ | ||
| @Component("initializer.displayPointListParser") |
There was a problem hiding this comment.
Ideally the component name is namespaced, i.e., `initializer.flagDisplayPointListParser)
| @Override | ||
| public Flag bootstrap(CsvLine line) throws IllegalArgumentException { | ||
| String uuid = line.getUuid(); | ||
| Flag flag = flagService.getFlagByUuid(uuid); |
There was a problem hiding this comment.
It might be a good idea to have a check if UUID is blank here as well. This will probably run an unncessary database query at best and at worst error out.
There was a problem hiding this comment.
Fixed, but I would note that the other CsvParsers I looked at don't do this check. Hopefully getXByUuuid generally returns null fast for a blank uuid (but I suspect it might not).
| /** | ||
| * Loads Flags from CSV files. | ||
| */ | ||
| @Component |
There was a problem hiding this comment.
Component here should be given a prefixed name
| /** | ||
| * Parses CSV files for Priority entities. | ||
| */ | ||
| @Component |
There was a problem hiding this comment.
Component here should be given a prefixed name, especially for something like "Priority"
| // Optional roles | ||
| String rolesStr = line.getString(HEADER_ROLES, ""); | ||
| if (StringUtils.isNotBlank(rolesStr)) { | ||
| Set<Role> roles = new HashSet<Role>(roleListParser.parseList(rolesStr)); |
There was a problem hiding this comment.
| Set<Role> roles = new HashSet<Role>(roleListParser.parseList(rolesStr)); | |
| Set<Role> roles = new HashSet<>(roleListParser.parseList(rolesStr)); |
|
Sorry, looking at the wrong thing.
|
|
@ibacher Ok, yeah it's nonsense that it's in api-2.4. The patientflags module requires platform 2.2.0. But I assume that since the loader itself doesn't depend on platform 2.2 APIs it's okay if it's in the |
|
Ping @ibacher , when you have the chance |
pom.xml
Outdated
| <fhir2Version>1.6.0</fhir2Version> | ||
| <billingVersion>1.1.0</billingVersion> | ||
| <stockmanagementVersion>2.0.2</stockmanagementVersion> | ||
| <patientflagsVersion>3.0.9-SNAPSHOT</patientflagsVersion> |
There was a problem hiding this comment.
Can we cut a release of the flags module? That will make it easier to release Iniz with these chagnes.
There was a problem hiding this comment.
Yeah, anything I need to do before cutting a release in Bamboo?
README.md
Outdated
| * Bahmni I.e Apps 1.1.0 (*compatible*) | ||
| * Billing 1.1.0 (*compatible*) | ||
| * Data Filter 1.0.0 (*compatible*) | ||
| * Flags 3.0.8 (*compatible*) |
There was a problem hiding this comment.
This seems not to match the declaration in the POM.xml?
There was a problem hiding this comment.
Yeah I thought there was no API change (so it would be compatible) but then I changed the API. Changed to 3.0.9 (*compatible*)
3 participants
https://openmrs.atlassian.net/browse/O3-5253
This creates a domain for flags, provided by https://github.com/openmrs/openmrs-module-patientflags
Domains are
flags,flagpriorities, andflagtags.