O3-5661: Migrate PaymentModeAttributeTypeService to OpenMRS Service - #187
O3-5661: Migrate PaymentModeAttributeTypeService to OpenMRS Service#187Josephkagimu1 wants to merge 17 commits into
Conversation
|
CC : @NethmiRodrigo , @wikumChamith |
|
@claude review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| description.addProperty("retired"); | ||
| if (rep instanceof DefaultRepresentation || rep instanceof FullRepresentation) { | ||
| description.addProperty("retireReason"); | ||
| description.addProperty("format"); |
There was a problem hiding this comment.
This needs fixing before merge: the new representation description only adds uuid, name, description, retired, retireReason, format, and regExp, dropping attributeOrder, foreignKey, and required that the old BaseRestAttributeTypeResource-based resource exposed (it added those on top of BaseRestMetadataResource's properties). Since MetadataDelegatingCrudResource uses this same description to decide which properties are settable on create/update, a client that creates or updates a payment mode attribute type with required: true, foreignKey: <conceptId>, or attributeOrder: <n> will now have those values silently dropped (falling back to null/false on the entity), and GET responses will stop returning them at all. Worth restoring these three properties in the description.
There was a problem hiding this comment.
The settable set doesn't come from getRepresentationDescription. MetadataDelegatingCrudResource.getCreatableProperties() hardcodes name (required) plus description, and BaseDelegatingResource.getUpdatableProperties() just delegates to it, so create and update never consult the description above. The mechanism described in this thread is off, but the create/update half of the problem is real and still open, and it fails harder than a silent drop: setConvertedProperties throws for any submitted property outside the description, and ConversionException carries @ResponseStatus(BAD_REQUEST).
So POST /ws/rest/v1/billing/paymentModeAttributeType/{uuid} with a format (or regExp, attributeOrder, foreignKey, required) returns 400 instead of updating the row. Editing an existing attribute type is the path that breaks, and format is the field that tells clients how to read the attribute's value.
Measured on both refs by instantiating the resource:
- main (77de297): creatable and updatable are both
[name, description, retired, attributeOrder, format, foreignKey, regExp, required], and settingformat/required/regExp/attributeOrdersucceeds. - 62fa8b2: both are
[name, description], and the same call throwsConversionException: Some properties are not allowed to be set: format, required, regExp, attributeOrder.
An override restores it, the way CashPointResource does for location:
@Override
public DelegatingResourceDescription getCreatableProperties() {
DelegatingResourceDescription description = super.getCreatableProperties();
description.addProperty("format");
description.addProperty("regExp");
description.addProperty("attributeOrder");
description.addProperty("foreignKey");
description.addProperty("required");
return description;
}There was a problem hiding this comment.
getCreatableProperties() is still not overridden at cca125e, so this half is unchanged. I re-measured by instantiating the resource on both refs and setting one property at a time:
- main (1eaa8f1): creatable and updatable are both
[name, description, retired, attributeOrder, format, foreignKey, regExp, required], and each of those properties sets successfully. - cca125e: both are
[name, description], andformat,regExp,attributeOrder,foreignKey,requiredandretiredare each rejected withConversionException: Some properties are not allowed to be set: <property>, which carries@ResponseStatus(BAD_REQUEST).
Merged as-is, POST /ws/rest/v1/billing/paymentModeAttributeType returns 400 whenever the body carries format, on create and on update alike, and the same for the other five. A create that avoids the 400 can only send {name, description}, which leaves format null, so nothing tells a client what datatype the attribute's value holds. The override in the comment above closes it.
One thing worth knowing if you check this by hand: on update, setConvertedProperties discards unchanged values before it applies the allowed-list check, so re-posting a value the row already holds looks like it succeeds.
There was a problem hiding this comment.
getCreatableProperties() is still not overridden at f8eade3, so this half is unchanged. I re-measured on both refs by instantiating the resource and setting one property at a time:
- main (77de297): creatable and updatable are both
[name, description, retired, attributeOrder, format, foreignKey, regExp, required], and each of those eight sets successfully. - f8eade3: both are
[name, description], andformat,regExp,attributeOrder,foreignKeyandrequiredare each rejected withConversionException: Some properties are not allowed to be set: <property>, which carries@ResponseStatus(BAD_REQUEST).
Merged as-is, POST /ws/rest/v1/billing/paymentModeAttributeType/{uuid} returns 400 whenever the body carries any of those five, so nobody can change an existing attribute type's format any more, and format is the field that tells a client how to read the attribute's value. Editing is the whole of the damage here: owner is not-null="true" in Bill.hbm.xml and has never been settable through the description, so a create was already impossible on main. retired is not in the rejected set either, since DelegatingCrudResource.update adds it to the description itself when isRetirable(), so my list of six above was one too many.
The override in my earlier comment closes it. Worth landing a test with it: all ten tests in the new PaymentModeAttributeTypeResourceTest go through getRepresentationDescription, none of them touch the creatable set, so the override coming back out later wouldn't fail anything.
@Test
public void getCreatableProperties_shouldIncludeAttributeFields() {
DelegatingResourceDescription description = resource.getCreatableProperties();
assertNotNull(description.getProperties().get("format"));
assertNotNull(description.getProperties().get("regExp"));
assertNotNull(description.getProperties().get("attributeOrder"));
assertNotNull(description.getProperties().get("foreignKey"));
assertNotNull(description.getProperties().get("required"));
}There was a problem hiding this comment.
Nothing fails if getCreatableProperties() comes back out again. I deleted the whole override in a local checkout of 20907c2 and all 59 tests in the omod module passed, including the ten in PaymentModeAttributeTypeResourceTest, while creatable and updatable dropped back to [name, description]. That is the same silence as before, nothing throws in the module, the cost is a 400 on any update carrying format.
The test from my comment above is still the guard. With the override deleted it is the one failure out of eleven:
@Test
public void getCreatableProperties_shouldIncludeAttributeFields() {
DelegatingResourceDescription description = resource.getCreatableProperties();
assertNotNull(description.getProperties().get("format"));
assertNotNull(description.getProperties().get("regExp"));
assertNotNull(description.getProperties().get("attributeOrder"));
assertNotNull(description.getProperties().get("foreignKey"));
assertNotNull(description.getProperties().get("required"));
}Not a blocker.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #187 +/- ##
============================================
+ Coverage 34.45% 35.06% +0.61%
- Complexity 809 831 +22
============================================
Files 213 213
Lines 5274 5298 +24
Branches 646 647 +1
============================================
+ Hits 1817 1858 +41
+ Misses 3252 3233 -19
- Partials 205 207 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@claude review |
1 similar comment
|
@claude review |
| private final PaymentModeAttributeTypeService service = Context.getService(PaymentModeAttributeTypeService.class); | ||
|
|
||
| @Override | ||
| public DelegatingResourceDescription getRepresentationDescription(Representation rep) { |
There was a problem hiding this comment.
There's no test file for PaymentModeAttributeTypeResource at all (Codecov reports 0% patch coverage on it, 20 lines missing), and this exact class already lost attributeOrder/foreignKey/required from the representation once earlier in this PR, caught only by manual review rather than a test. Worth adding a unit test mirroring PaymentResourceTest's pattern (mockStatic(Context.class) plus a mocked PaymentModeAttributeTypeService) that asserts getRepresentationDescription(new FullRepresentation()) includes all seven properties, and that doGetAll/getByUniqueId/save/purge delegate to the service. That would catch a repeat of that regression automatically instead of relying on someone noticing it in review.
| */ | ||
| package org.openmrs.module.billing.web.rest.resource; | ||
|
|
||
| import org.openmrs.module.billing.web.base.resource.BaseRestAttributeTypeResource; |
There was a problem hiding this comment.
BaseRestAttributeTypeResource had only this one subclass, so after this change it's unused anywhere in the codebase. Worth deleting it in this PR or a follow-up so it doesn't linger as dead code.
|
@claude review |
| * Copyright (C) OpenMRS Inc. OpenMRS is a registered trademark and the OpenMRS | ||
| * graphic logo is a trademark of OpenMRS Inc. | ||
| */ | ||
| package org.openmrs.module.billing.web.rest.resource; |
There was a problem hiding this comment.
This needs fixing before merge: this test class lives under omod/src/main/java/... instead of omod/src/test/java/.... The omod module's mockito-inline/JUnit Jupiter dependencies are declared with <scope>test</scope> in omod/pom.xml, but Maven's default-compile execution (which compiles src/main/java) only has compile/provided/system scope dependencies on its classpath. As soon as this file is compiled as main source, org.junit.jupiter.api.Test, org.mockito.Mockito and org.mockito.MockedStatic won't resolve, and the omod module fails to build. Even if that were somehow avoided, Surefire only picks up tests from src/test/java by default, so none of these assertions (including the regression check for attributeOrder/foreignKey/required added earlier in this PR) would actually run in CI. Please move this file to omod/src/test/java/org/openmrs/module/billing/web/rest/resource/PaymentModeAttributeTypeResourceTest.java.
9d5a60e to
19764fe
Compare
|
@claude review |
19764fe to
6bbfb58
Compare
|
@claude review |
| description.addProperty("name"); | ||
| description.addProperty("description"); | ||
| description.addProperty("retired"); | ||
| if (rep instanceof DefaultRepresentation || rep instanceof FullRepresentation) { |
There was a problem hiding this comment.
The two resources already migrated to this pattern, PaymentModeResource and CashPointResource, both return null here when the representation is a CustomRepresentation, and this one doesn't. That difference has a real effect. BaseDelegatingResource.asRepresentation uses whatever getRepresentationDescription returns as soon as it is non-null, and only falls through to parsing the custom:(...) field list when the description comes back null. Because this method always returns a non-null description, a request like GET .../paymentModeAttributeType/{uuid}?v=custom:(uuid,format) responds with {uuid, name, description, retired} rather than the fields the caller asked for, and without any error.
This isn't a regression (the old BaseRestAttributeTypeResource had the same gap), so I don't consider it a merge blocker. But since you're rewriting this method specifically to mirror the two siblings, it's worth matching them: add an else if (rep instanceof CustomRepresentation) { return null; } branch, along with the CustomRepresentation import the siblings already carry, so custom representations are honored for this resource too.
…seRestAttributeTypeResource
6bbfb58 to
714dcec
Compare
|
@claude review |
| description.addProperty("name"); | ||
| description.addProperty("description"); | ||
| description.addProperty("retired"); | ||
| if (rep instanceof DefaultRepresentation || rep instanceof FullRepresentation) { |
There was a problem hiding this comment.
Gating these five properties behind Default/Full means a RefRepresentation now gets only uuid, name, description, retired. BaseRestAttributeTypeResource added attributeOrder, format, foreignKey, regExp and required unconditionally, so refs carried them, and refs are how the rest of the module hands attribute types out: PaymentModeResource exposes attributeTypes at Representation.REF, and BaseRestAttributeDataResource (so PaymentAttributeResource) exposes attributeType at Representation.REF, which BillResource reaches through payments at FULL and then attributes.
Merged as-is, GET /ws/rest/v1/billing/bill/{uuid} and GET /ws/rest/v1/billing/paymentMode stop returning format, required, regExp, attributeOrder and foreignKey on every embedded attribute type, so a client reading a payment can no longer tell what datatype the attribute's value holds, or which attributes were required. Nothing throws, the keys are simply absent, which is why CI stays green.
I ran the same PaymentModeAttributeType through ConversionUtil.convertToRepresentation(t, Representation.REF) on both refs, with this resource registered as the converter for the class:
- main (77de297):
{uuid, name, description, retired, attributeOrder=1, format=java.lang.String, foreignKey=42, regExp=.*, required=true} - 62fa8b2:
{uuid, name, description, retired}
Moving those five addProperty calls above the if restores it for every representation.
Smaller thing in the same method: the old resource added auditInfo for FullRepresentation and this one doesn't, so ?v=full loses it. Worth putting back while you're in here, but I wouldn't hold the PR on it.
| public void getAllPaymentModeAttributeTypes_shouldReturnAllIncludingRetired() { | ||
| List<PaymentModeAttributeType> types = service.getAllPaymentModeAttributeTypes(true); | ||
| assertNotNull(types); | ||
| assertEquals(3, types.size()); |
There was a problem hiding this comment.
Both includeRetired cases assert 3, because PaymentModeTest.xml has three cashier_payment_mode_attribute_type rows and every one of them is retired="false". Nothing here distinguishes getAllPaymentModeAttributeTypes(true) from (false): drop the where retired = false clause from the new DAO method, or invert the flag, and both tests stay green.
PaymentModeServiceImplTest does exercise this for payment modes (3 vs 4), because the dataset includes a retired cashier_payment_mode row. One retired attribute-type row plus expecting 4 here would do the same. Nothing else asserts attribute-type counts (PaymentModeServiceImplTest only checks the collection is non-empty), so the dataset change is safe. Not a blocker, just a cheap gap to close.
|
CC : @dkayiwa hope I fixed everything, thanks for review. |
|
|
||
| @Override | ||
| public List<PaymentModeAttributeType> getAllPaymentModeAttributeTypes(boolean includeRetired) { | ||
| String hql = "from PaymentModeAttributeType" + (includeRetired ? "" : " where retired = false"); |
There was a problem hiding this comment.
GET /ws/rest/v1/billing/paymentModeAttributeType used to come back sorted by name and no longer does. The old path ran through BaseRestMetadataResource.doGetAll into getAll(includeRetired, pagingInfo), and BaseMetadataDataServiceImpl.getDefaultSort() put Order.asc("name") on that criteria. This HQL has no order by, so rows arrive in whatever order the database picks.
I saved three attribute types named "Zulu probe", "Mike probe" and "Alpha probe", in that order, against payment mode 0, then listed them:
- main (1eaa8f1):
[Alpha probe, Mike probe, Test 1 Attribute Type, Test 1 Attribute Type, Test 1 Attribute Type, Zulu probe] - 62fa8b2:
[Test 1 Attribute Type, Test 1 Attribute Type, Test 1 Attribute Type, Zulu probe, Mike probe, Alpha probe]
Nothing throws, and all three cashier_payment_mode_attribute_type rows in PaymentModeTest.xml carry the same name, so no test can see the difference. The resource also slices the whole list in memory now (NeedsPaging), which leaves startIndex/limit paging resting on a query with no defined order.
One line brings it back. Spotless leaves it unwrapped, and the probe then returns main's ordering again:
| String hql = "from PaymentModeAttributeType" + (includeRetired ? "" : " where retired = false"); | |
| String hql = "from PaymentModeAttributeType" + (includeRetired ? "" : " where retired = false") + " order by name"; |
HibernatePaymentModeDAOImpl lost the same sort when payment modes were migrated, so you could equally close both later. Ordering is cosmetic next to the other open comments here, so your call whether it belongs in this PR.
There was a problem hiding this comment.
Nothing asserts the order, so the order by name can come back out without failing anything. I deleted it in a local checkout of f8eade3 and all ten tests in PaymentModeAttributeTypeServiceImplTest stayed green, while the list flipped from [Retired Attribute Type, Test 1 Attribute Type, Test 1 Attribute Type, Test 1 Attribute Type] to [Test 1 Attribute Type, Test 1 Attribute Type, Test 1 Attribute Type, Retired Attribute Type].
The retired row that PaymentModeAttributeTypeTest.xml now adds makes it a one-liner, since "Retired Attribute Type" sorts ahead of the three non-retired rows, and those three all carry the same name so the includeRetired=false test cannot see order either way. In getAllPaymentModeAttributeTypes_shouldReturnAllIncludingRetired:
assertEquals("Retired Attribute Type", types.get(0).getName());That is the one assertion I could get to flip when the order by name goes.
|
CC : @dkayiwa |
| assertNotNull(description.getProperties().get("attributeOrder")); | ||
| assertNotNull(description.getProperties().get("foreignKey")); | ||
| assertNotNull(description.getProperties().get("required")); | ||
| } |
There was a problem hiding this comment.
These two tests pass whether the five attribute properties are added unconditionally or gated behind DefaultRepresentation || FullRepresentation, so neither one guards the change that moved them out of that branch. I re-gated them in a local checkout, reverting that fix exactly, and all nine tests in this file still went green while getRepresentationDescription for a ref dropped back to [uuid, name, description, retired].
Ref is the representation that regressed, and it is the one the rest of the module hands attribute types out through: PaymentModeResource exposes attributeTypes at REF, and BaseRestAttributeDataResource exposes attributeType at REF. A third case closes it, and it is the only one of the resulting ten that fails when I re-gate the properties:
@Test
public void getRepresentationDescription_shouldIncludeAttributeFieldsForRefRepresentation() {
DelegatingResourceDescription description = resource.getRepresentationDescription(new RefRepresentation());
assertNotNull(description);
assertNotNull(description.getProperties().get("format"));
assertNotNull(description.getProperties().get("regExp"));
assertNotNull(description.getProperties().get("attributeOrder"));
assertNotNull(description.getProperties().get("foreignKey"));
assertNotNull(description.getProperties().get("required"));
}That needs import org.openmrs.module.webservices.rest.web.representation.RefRepresentation; alongside the other representation imports. Not a merge blocker, just the cheapest guard against this coming back.
cca125e to
f8eade3
Compare
|
Hi @dkayiwa , is there any thing else to fix ? |
dkayiwa
left a comment
dkayiwa
left a comment
There was a problem hiding this comment.
Yes, a few. The getCreatableProperties() comment on PaymentModeAttributeTypeResource is the one I would hold the merge for. Also still open, though yours to take or leave: a DAO test for a uuid that matches no row, a save round-trip test, and an ordering assertion on getAllPaymentModeAttributeTypes(true).
…PaymentModeAttributeTypeTest.xml Co-authored-by: dkayiwa <dkayiwa@openmrs.org>
|
|
Hi @dkayiwa , I worked on the requested changes |
3 participants
Summary
This PR replaces the custom
IMetadataDataService-based service layer forPaymentModeAttributeTypewith the standard OpenMRS service pattern (
OpenmrsService+BaseOpenmrsService+ Hibernate DAO).This removes the dependency on the generic
BaseMetadataDataServiceImplandgenericRepositoryDao,giving the service explicit CRUD methods with proper authorization, transaction management, and
null-safety.
Ticket : https://openmrs.atlassian.net/browse/O3-5661