[GitHub] [fineract] galovics commented on a diff in pull request #3401: FINERACT-1760: Update office by external id

["galovics (via GitHub)" <gi...@apache.org>]

galovics commented on code in PR #3401:
URL: https://github.com/apache/fineract/pull/3401#discussion_r1307047821


##########
fineract-provider/src/main/java/org/apache/fineract/organisation/office/api/OfficesApiResource.java:
##########
@@ -176,6 +176,25 @@ public String updateOffice(@PathParam("officeId") @Parameter(description = "offi
         return toApiJsonSerializer.serialize(result);
     }
 
+    @PUT

Review Comment:
   Can't we use the new serializers to do this API? i.e. return the object directly from the method, etc so that you don't need this many swagger annotations.



##########
integration-tests/src/test/java/org/apache/fineract/integrationtests/common/OfficeHelper.java:
##########
@@ -82,13 +88,33 @@ public Integer updateOffice(int id, String name, String openingDate) {
                 new Gson().toJson(map), "resourceId");
     }
 
+    public Integer updateOfficeUsingExternalId(String externalId, String name, String openingDate) {

Review Comment:
   Can't we use the generated Office API?



##########
fineract-core/src/main/java/org/apache/fineract/organisation/office/service/OfficeReadPlatformService.java:
##########
@@ -31,6 +31,8 @@ public interface OfficeReadPlatformService {
 
     OfficeData retrieveOffice(Long officeId);
 
+    OfficeData retrieveOfficeWithExternalId(String externalId);

Review Comment:
   ExternalId class?



##########
fineract-provider/src/main/java/org/apache/fineract/organisation/office/service/OfficeReadPlatformServiceImpl.java:
##########
@@ -210,6 +210,22 @@ public OfficeData retrieveOffice(final Long officeId) {
         }
     }
 
+    @Override
+    public OfficeData retrieveOfficeWithExternalId(String externalId) {

Review Comment:
   Not to my liking to be honest. This should be solved by using JPQL + a projection. I wouldn't breed more native queries unnecessarily although I understand it was a copy paste.



##########
fineract-core/src/main/java/org/apache/fineract/organisation/office/exception/OfficeNotFoundException.java:
##########
@@ -33,4 +33,8 @@ public OfficeNotFoundException(final Long id) {
     public OfficeNotFoundException(Long id, EmptyResultDataAccessException e) {
         super("error.msg.office.id.invalid", "Office with identifier " + id + " does not exist", id, e);
     }
+
+    public OfficeNotFoundException(String externalId, EmptyResultDataAccessException e) {

Review Comment:
   Should be ExternalId class, not String.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org