ME-37: Support SystemTask Domain - #40
Conversation
Add SystemTaskDomainExporter and SystemTaskLineExporter for Initializer's systemtasks domain (tasks module) n
| // We currently don't have a ProviderRoleExporter in the module. This is because Initializer still only | ||
| // supports the ProviderRole from the providermanagement module, whereas on core 2.8+ provider roles have | ||
| // moved into core. Until an exporter exists, Selector drops the role returned here because no registered | ||
| // domain owns it. Keeping this here for future sake: it is returned anyway so the closure starts working | ||
| // the moment a provider roles domain is added. | ||
| ProviderRole role = SystemTaskLineExporter.resolveAssignee(instance); | ||
| return role == null ? Collections.emptyList() : Collections.singletonList(role); |
There was a problem hiding this comment.
@dkayiwa, @ibacher, we might need a decision on the Initializer side here. Initializer's providerroles domain and the system tasks "default assignee role" column both resolve provider roles through providermanagement only, so exports of core 2.8+ provider roles can't be imported yet.
There's a PR by @mseaton that adds core ProviderRole support, but it was paused: mekomsolutions/openmrs-module-initializer#304 (tracking issue: mekomsolutions/openmrs-module-initializer#303).
There was a problem hiding this comment.
Keep it as it is. Exporting the core role uuid is the only value that will still be right once #303/#304 lands, and dropping the column now would quietly lose the assignee from every config exported in the meantime. The README paragraph you added is the right place for the caveat, and none of this blocks the PR.
The follow-up that actually closes the gap is #304, so it is worth nudging @mseaton on it separately rather than working around it here.
| line.put(SystemTasksLineProcessor.HEADER_DEFAULT_ASSIGNEE_ROLE, providerRole.getUuid()); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Retired system tasks come out as uuid + void/retire only, and that row cannot be imported onto a server that does not already have the task, so this needs fixing before merge.
Initializer's shouldFill is !(voidOrRetire && instance.getId() != null), and SystemTask.getId() is a real Integer, so on a fresh target SystemTasksLineProcessor.fill does run. It just has no name or title to read, and tasks_systemtask declares both NOT NULL. I exported one live task and one retired task, then fed the CSV back through SystemTasksCsvParser in a context-sensitive test: the live row imports, the retired row fails with PropertyValueException: not-null property references a null or transient value : org.openmrs.module.tasks.SystemTask.name. A file that happens to hold only retired rows has no name column at all, and there line.getName(true) throws on the missing header instead.
Merged as is, replaying an exported configuration onto a server that doesn't already carry these tasks loses every retired one. Initializer records the line as failed and carries on, so the task never arrives.
IdentifierSourceLineExporter ran into this and re-dispatches to export, which is what I'd do here. I re-ran the round trip with the override in place and both directions work: on a fresh target the retired row is created and retired, and on a target that already has the row shouldFill is false, so the extra columns are ignored.
| @Override | |
| protected void writeRetiredDiscriminators(SystemTask instance, ExportLine line) { | |
| export(instance, line); | |
| } | |
SystemTaskLineExporterTest.retiredTaskEmitsUuidAndFlagOnly asserts the current shape, so it flips with this (it was the only test that failed). Dropping retired tasks from getAllInstances() the way CohortTypeDomainExporter does would also stop the failure, but it throws away the retirement, and unlike cohort types nothing forces that here.
| import static org.junit.jupiter.api.Assertions.assertNull; | ||
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
|
|
||
| class SystemTaskDomainExporterIntegrationTest extends BaseModuleContextSensitiveTest { |
There was a problem hiding this comment.
Nothing exercises getAllInstances() or an actual export-then-reimport, and both are reachable from this class, so they would be worth adding. Not blocking.
Context.getService(TasksService.class) does resolve in a context-sensitive test: tasks-api ships a moduleApplicationContext.xml, and at provided scope it lands on the test classpath, so the service bean is registered. SystemTask is an annotated @Entity under org.openmrs.**, so rows seed through sessionFactory and exporter.getAllInstances() can be asserted the way CohortTypeDomainExporterIntegrationTest does for cohort types. SystemTasksCsvParser's constructor is public too, so new SystemTasksCsvParser(Context.getService(TasksService.class), new SystemTasksLineProcessor()) plus setInputStream / getLines / process replays the exported CSV and reports what Initializer rejected. That round trip is the only thing that shows the file is loadable at all; the unit tests only check the shape of a line.
There was a problem hiding this comment.
i think we missing test coverage for getAllInstances()
|
@dkayiwa , @Bawanthathilan I've updated the PR. |
|
@dkayiwa is this ready to get merged? |
3 participants
Description of what I changed
Add SystemTaskDomainExporter and SystemTaskLineExporter for Initializer's systemtasks domain (tasks module) n
Issue I worked on
see https://openmrs.atlassian.net/browse/ME-37
Checklist: I completed these to help reviewers :)
My IDE is configured to follow the code style of this project.
No? Unsure? -> configure your IDE, format the code and add the changes with
git add . && git commit --amendI have added tests to cover my changes. (If you refactored
existing code that was well tested you do not have to add tests)
No? -> write tests and add them to this commit
git add . && git commit --amendI ran
mvn clean packageright before creating this pull request andadded all formatting changes to my commit.
No? -> execute above command
All new and existing tests passed.
No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.
My pull request is based on the latest changes of the master branch.
No? Unsure? -> execute command
git pull --rebase upstream master