[#272] Added Billing-related Domains. #269
Conversation
There was a problem hiding this comment.
Thanks @ODORA0. Since this is not connected to any Iniz GitHub issue, I'll ask you to provide a comprehensive description of what all this PR bring in, could you do that?
api-2.4/pom.xml
Outdated
| @@ -16,6 +16,7 @@ | |||
|
|
|||
| <properties> | |||
| <openmrsPlatformVersion>${openmrsVersion2.4}</openmrsPlatformVersion> | |||
| <billingVersion>1.1.0-SNAPSHOT</billingVersion> | |||
There was a problem hiding this comment.
We assume that there is plans for a v1.1.0 release?
.../src/main/java/org/openmrs/module/initializer/api/billing/BillableServicesLineProcessor.java
Outdated
Show resolved
Hide resolved
api-2.4/src/main/java/org/openmrs/module/initializer/api/billing/CashPointCsvParser.java
Show resolved
Hide resolved
api-2.4/src/main/java/org/openmrs/module/initializer/api/billing/CashPointLoader.java
Show resolved
Hide resolved
.../src/main/java/org/openmrs/module/initializer/api/billing/BillableServicesLineProcessor.java
Outdated
Show resolved
Hide resolved
...2.4/src/main/java/org/openmrs/module/initializer/api/billing/ServicePricesLineProcessor.java
Outdated
Show resolved
Hide resolved
...2.4/src/main/java/org/openmrs/module/initializer/api/billing/ServicePricesLineProcessor.java
Outdated
Show resolved
Hide resolved
...src/test/java/org/openmrs/module/initializer/api/BillableServicesLoaderIntergrationTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Let's make it work with Validator, this will prove that all is well.
See this as an example (with IDGen) of what's required to be done →
openmrs-module-initializer/validator/pom.xml
Lines 186 to 192 in 3b5a541
Then, you should validate that you can make a dry run of a piece of billing config, see → https://github.com/mekomsolutions/openmrs-module-initializer/blob/master/readme/validator.md#how-to-make-a-dry-run
api-2.4/src/main/java/org/openmrs/module/initializer/api/billing/CashPointCsvParser.java
Show resolved
Hide resolved
| public abstract class BillableServicesLoaderIntergrationTest extends DomainBaseModuleContextSensitive_2_3_Test { | ||
|
|
||
| public BillableServicesLoaderIntergrationTest() { | ||
| super(); | ||
| { | ||
| Module mod = new Module("", "billing", "", "", "", "1.1.0-SNAPSHOT"); | ||
| mod.setFile(new File("")); | ||
| ModuleFactory.getStartedModulesMap().put(mod.getModuleId(), mod); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public void updateSearchIndex() { | ||
| // to prevent Data Filter's 'Illegal Record Access' | ||
| } |
There was a problem hiding this comment.
Hi @ODORA0.
Thank you for the PR. We should include some integration tests here. You can refer to this example as a guide.
| { | ||
| Concept concept = new Concept(); | ||
| concept.setShortName(new ConceptName("Antenatal Services", Locale.ENGLISH)); | ||
| concept.setConceptClass(conceptService.getConceptClassByName("Misc")); | ||
| concept.setDatatype(conceptService.getConceptDatatypeByName("N/A")); | ||
| concept = conceptService.saveConcept(concept); | ||
| } | ||
| { | ||
| Concept concept = new Concept(); | ||
| concept.setShortName(new ConceptName("Orthopedic Services", Locale.ENGLISH)); | ||
| concept.setConceptClass(conceptService.getConceptClassByName("Misc")); | ||
| concept.setDatatype(conceptService.getConceptDatatypeByName("N/A")); | ||
| concept = conceptService.saveConcept(concept); | ||
| } |
There was a problem hiding this comment.
@ODORA0 did you need to create new test concepts, why not use some from the various test datasets?
There was a problem hiding this comment.
@mks-d I am picking this from the CSV.
There was a problem hiding this comment.
@ODORA0 you mean you're showing that loading the CSV will modify those concepts accordingly?
There was a problem hiding this comment.
@mks-d I don't follow, please explain. Which datasets did you mean earlier?
There was a problem hiding this comment.
Within OpenMRS' Spring tests you don't necessarily need to create concepts, you have access to this bunch if you need concepts.
You can even load and use the following extra dataset shipped with Iniz if you ever needed more:
| String uuid = line.getString(HEADER_UUID); | ||
| if (StringUtils.isNotBlank(uuid)) { | ||
| paymentMode.setUuid(uuid); | ||
| } |
There was a problem hiding this comment.
Is the UUID not required (like the name is)?
| if (line.containsHeader(HEADER_UUID)) { | ||
| String uuid = line.getString(HEADER_UUID); | ||
| if (StringUtils.isNotBlank(uuid)) { | ||
| cashPoint.setUuid(uuid); | ||
| } | ||
| } |
There was a problem hiding this comment.
Why not
cashPoint.setUuid(line.getString(HEADER_UUID));?
... if the UUID is not required of course.
| @Test | ||
| public void testBootstrapWithExistingService() { | ||
| String uuid = "44ebd6cd-04ad-4eba-8ce1-0de4564bfd17"; | ||
| CsvLine csvLine = new CsvLine(new String[] { "Uuid" }, new String[] { uuid }); | ||
|
|
||
| BillableService existingService = new BillableService(); | ||
| when(billableServiceResource.getByUniqueId(uuid)).thenReturn(existingService); | ||
|
|
||
| BillableService result = parser.bootstrap(csvLine); | ||
|
|
||
| assertNotNull(result); | ||
| assertEquals(existingService, result); | ||
| } |
There was a problem hiding this comment.
Starting with this one, divide all your test methods/cases as best as can following the given-when-then pattern. Eg.:
Also the method name follows a convention, see also the example above: bootstrap_shouldBootstrapObjectGivenUuidPresentAndObjectNotVoid.
As in: "method being tested" + "_" + "expected outcome".
| public void setup() { | ||
| { | ||
| Concept concept = conceptService.getConceptByUuid("a09ab2c5-878e-4905-b25d-5784167d0216"); | ||
| Assert.assertNotNull("Concept should not be null", concept); |
There was a problem hiding this comment.
If this concept is in the test dataset you don't need to assert this, IMO. Also you don't need to craft a special message. If somehow the test dataset is broken, then it's another story, it will be detected and fixed accordingly.
| } | ||
| { | ||
| Concept concept = conceptService.getConceptByUuid("4421da0d-42d0-410d-8ffd-47ec6f155d8f"); | ||
| Assert.assertNotNull("Concept should not be null", concept); |
...4/src/test/java/org/openmrs/module/initializer/api/BillableServiceLoaderIntegrationTest.java
Show resolved
Hide resolved
| { | ||
| BillableService service = billableServiceResource.getByUniqueId("44ebd6cd-04ad-4eba-8ce1-0de4564bfd17"); | ||
| Assert.assertNotNull(service); | ||
| Assert.assertEquals(conceptService.getConceptByUuid("a09ab2c5-878e-4905-b25d-5784167d0216"), service.getConcept()); | ||
| Assert.assertEquals(BillableServiceStatus.ENABLED, service.getServiceStatus()); | ||
| } | ||
| { | ||
| BillableService service = billableServiceResource.getByUniqueId("a0f7d8a1-4fa2-418c-aa8a-9b358f43d605"); | ||
| Assert.assertNotNull(service); | ||
| Assert.assertEquals(conceptService.getConceptByUuid("4421da0d-42d0-410d-8ffd-47ec6f155d8f"), service.getConcept()); | ||
| Assert.assertEquals(BillableServiceStatus.ENABLED, service.getServiceStatus()); | ||
| } | ||
| { | ||
| BillableService service = billableServiceResource.getByUniqueId("16435ab4-27c3-4d91-b21e-52819bd654d8"); | ||
| Assert.assertNotNull(service); | ||
| Assert.assertEquals("Updated Service", service.getName()); | ||
| Assert.assertEquals(BillableServiceStatus.DISABLED, service.getServiceStatus()); | ||
| } |
There was a problem hiding this comment.
Precedes this with "// verify":
// verify
...
|
@Ruhanga Please review |
| if (line.containsHeader(HEADER_LOCATION)) { | ||
| String location = line.getString(HEADER_LOCATION); | ||
| if (StringUtils.isNotBlank(location)) { | ||
| cashPoint.setLocation(Utils.fetchLocation(location, locationService)); | ||
| } else { | ||
| cashPoint.setLocation(null); | ||
| } | ||
| } |
There was a problem hiding this comment.
Are you sure you can't compactify all this into just
String location = line.getString(HEADER_LOCATION);
cashPoint.setLocation(Utils.fetchLocation(location, locationService));?
Did you check?
|
|
||
| @Override | ||
| public CashPoint fill(CashPoint cashPoint, CsvLine line) throws IllegalArgumentException { | ||
| // UUID is required |
There was a problem hiding this comment.
You may remove this line is in fact the UUID is not required.
| public CashPoint fill(CashPoint cashPoint, CsvLine line) throws IllegalArgumentException { | ||
| // UUID is required | ||
| cashPoint.setUuid(line.get(HEADER_UUID)); | ||
|
|
| // Process UUID (required) | ||
| String uuid = line.get(HEADER_UUID, true); | ||
| if (StringUtils.isNotBlank(uuid)) { | ||
| paymentMode.setUuid(uuid); | ||
| } |
There was a problem hiding this comment.
Why is the UUID required for payment modes but not for cash points?
There was a problem hiding this comment.
They are actually required, changing that for cash points
| protected static final String HEADER_UUID = "uuid"; | ||
|
|
||
| protected static final String HEADER_NAME = "name"; |
There was a problem hiding this comment.
Those two are already defined in BaseLineProcessor, you get them for free.
| // Process UUID (required) | ||
| paymentMode.setUuid(line.get(HEADER_UUID, true)); | ||
| // Process Name (required) | ||
| paymentMode.setName(line.get(HEADER_NAME, true)); | ||
| // Process other optional attributes | ||
| processAttribute(line, HEADER_PRICE, paymentMode); | ||
| processAttribute(line, HEADER_PAYMENT_MODE, paymentMode); | ||
| processAttribute(line, HEADER_ITEM, paymentMode); | ||
| processAttribute(line, HEADER_BILLABLE_SERVICE, paymentMode); |
There was a problem hiding this comment.
I think we don't need to document in the code which is optional and which is not, this is done in the READMEs and should be self-explanatory through the use of the throwable version or not.
So let's get rid of the comments, you didn't set them in CashPointsLineProcessor anyway.
mks-d
changed the title
There was a problem hiding this comment.
Thank you for adding tests, @ODORA0. Moving forward, you'll add more integration tests for each of the newly introduced domain loaders. Currently, we only have one, BillableServiceLoaderIntegrationTest. Here are a few other specific points to address:
| <dependency> | ||
| <groupId>org.openmrs.module</groupId> | ||
| <artifactId>billing-omod</artifactId> | ||
| <version>${billingVersion}</version> | ||
| <scope>provided</scope> | ||
| </dependency> |
There was a problem hiding this comment.
@ODORA0, we should rather depend on the sub-modules' api packages than the omod web api package.
There was a problem hiding this comment.
@Ruhanga Please advise, given the constraint that BillableServiceResource is only available in the billing-omod module and not in the billing-api module, transitioning to use billing-api will limit the direct operations we can perform on BillableService entities since these operations are currently done through the BillableServiceResource.
Also are you suggesting adding integration tests for all the other domain in this PR as well?
There was a problem hiding this comment.
@ODORA0 Iniz doesn't need any REST resources, it only needs the Java API of the modules it depends on. You can indeed switch to depending on the -api submodule and, if all your Spring tests pass, you're good.
Also are you suggesting adding integration tests for all the other domain in this PR as well?
Yes indeed. You need to bring as a test resource a typical, real-world looking, CSV file and demonstrate through your tests that the outcome of loading each CSV file is as expected and can be asserted.
Each *Loader class should have its corresponding Spring test (context sensitive) class.
| BillableService service = billableServiceResource.getByUniqueId("44ebd6cd-04ad-4eba-8ce1-0de4564bfd17"); | ||
| Assert.assertNotNull(service); | ||
| Assert.assertEquals(conceptService.getConceptByUuid("a09ab2c5-878e-4905-b25d-5784167d0216"), service.getConcept()); | ||
| Assert.assertEquals(BillableServiceStatus.ENABLED, service.getServiceStatus()); | ||
| } | ||
| { | ||
| BillableService service = billableServiceResource.getByUniqueId("a0f7d8a1-4fa2-418c-aa8a-9b358f43d605"); | ||
| Assert.assertNotNull(service); | ||
| Assert.assertEquals(conceptService.getConceptByUuid("4421da0d-42d0-410d-8ffd-47ec6f155d8f"), service.getConcept()); | ||
| Assert.assertEquals(BillableServiceStatus.ENABLED, service.getServiceStatus()); | ||
| } | ||
| { | ||
| BillableService service = billableServiceResource.getByUniqueId("16435ab4-27c3-4d91-b21e-52819bd654d8"); | ||
| Assert.assertNotNull(service); | ||
| Assert.assertEquals("Updated Service", service.getName()); | ||
| Assert.assertEquals(BillableServiceStatus.DISABLED, service.getServiceStatus()); |
There was a problem hiding this comment.
I don't think this is actually testing the loading of the billable services csv file located at resources/testAppDataDir/configuration/billableservices/services.csv since what's being retrieved is what's being saved inside setup() above. Else you'd have to remove setup() altogether or use it to save billable services to be edited or voided by contents of services.csv, which are valid update use-cases to test.
| <dependency> | ||
| <groupId>org.openmrs.module</groupId> | ||
| <artifactId>billing-omod</artifactId> | ||
| <version>${billingVersion}</version> | ||
| <scope>provided</scope> | ||
| </dependency> |
There was a problem hiding this comment.
@ODORA0 Iniz doesn't need any REST resources, it only needs the Java API of the modules it depends on. You can indeed switch to depending on the -api submodule and, if all your Spring tests pass, you're good.
Also are you suggesting adding integration tests for all the other domain in this PR as well?
Yes indeed. You need to bring as a test resource a typical, real-world looking, CSV file and demonstrate through your tests that the outcome of loading each CSV file is as expected and can be asserted.
Each *Loader class should have its corresponding Spring test (context sensitive) class.
This PR takes care of adding the OpenMRS Billing Module domains to iniz
BILLABLE_SERVICESthat represents services that can be billed within the billing module with attributes such as the name, short name, concept, service type, service category, service prices, and service status, handling creation of billable services in the billing module e.g. Covid Vaccination Service that might be a billable service in a facility under the Vaccination Services, Ultra Sound Scanning Services under the Antenatal Services, Complete Blood Count that can be under Clinical Consultation Services etc.CASH_POINTSwhich represents a locations such as OPD Clinic, ART Clinic within the OpenMRS billing module where bills can be created and paidBILLABLE_SERVICE_PRICESwhich are payment modes that represents different modes of payment (such as cash, mobile money or credit card) within the billing module which also allows for customisable attribute types, enabling users to define additional properties for each payment mode like the pricing of the different payments