Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
package com.iemr.common.controller.abdmfacility;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.web.bind.annotation.CrossOrigin;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.PathVariable;
import org.springframework.web.bind.annotation.RequestHeader;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RestController;

import com.iemr.common.service.abdmfacility.AbdmFacilityService;
import com.iemr.common.utils.response.OutputResponse;

import io.swagger.v3.oas.annotations.Operation;
import io.swagger.v3.oas.annotations.parameters.RequestBody;

@RequestMapping({ "/facility" })
@RestController
public class AbdmFacilityController {

private final Logger logger = LoggerFactory.getLogger(this.getClass().getName());

@Autowired
private AbdmFacilityService abdmFacilityService;

@CrossOrigin
@Operation(summary = "Get Abdm facility mapped to worklocation")
@GetMapping(value = { "/getWorklocationMappedAbdmFacility/{workLocationId}" })
public String getAbdmFacilityDetails(@PathVariable int workLocationId, @RequestHeader(value = "Authorization") String Authorization) {

OutputResponse response = new OutputResponse();

try {

String resp = abdmFacilityService.getMappedAbdmFacility(workLocationId);

response.setResponse(resp);

} catch (Exception e) {

response.setError(5000, e.getMessage());
logger.error(e.getMessage());
}
logger.info("Get ABDM Registered facilities API response" + response.toString());
return response.toString();
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,14 @@ public class ProviderServiceAddressMapping
@Expose
@Column(name = "LocationName")
private String locationName;

@Expose
@Column(name = "AbdmFacilityID")
private String abdmFacilityID;

@Expose
@Column(name = "AbdmFacilityName")
private String abdmFacilityName;

// public ProviderServiceAddressMapping()
// {
Expand Down Expand Up @@ -149,4 +157,5 @@ public String getLocationName()
{
return locationName;
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
package com.iemr.common.repository.abdmfacility;

import org.springframework.data.jpa.repository.Query;
import org.springframework.data.repository.CrudRepository;
import org.springframework.data.repository.query.Param;
import org.springframework.stereotype.Repository;

import com.iemr.common.data.users.ProviderServiceAddressMapping;

@Repository
public interface AbdmFacilityRepository extends CrudRepository<ProviderServiceAddressMapping, Integer> {

@Query("SELECT v from ProviderServiceAddressMapping v where v.pSAddMapID = :pssmID")
ProviderServiceAddressMapping getAbdmFacility(@Param("pssmID") int pssmID);
Comment on lines +13 to +14
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

πŸ› οΈ Refactor suggestion

Enhance query robustness and naming

Based on previous learnings and best practices:

  1. Add ORDER BY clause as required per previous feedback
  2. Consider renaming method to findAbdmFacilityByPssmId to follow Spring Data naming conventions
  3. Add null handling
-    @Query("SELECT v from ProviderServiceAddressMapping v where v.pSAddMapID = :pssmID")
-    ProviderServiceAddressMapping getAbdmFacility(@Param("pssmID") int pssmID);
+    @Query("SELECT v from ProviderServiceAddressMapping v where v.pSAddMapID = :pssmID ORDER BY v.pSAddMapID")
+    ProviderServiceAddressMapping findAbdmFacilityByPssmId(@Param("pssmID") int pssmID);

Committable suggestion skipped: line range outside the PR's diff.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CodeRabbit That response contains only single object of list. Oder By is not required

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@helenKaryamsetty Understood. Since the query guarantees a single matching record, the ORDER BY clause isn’t beneficial. No changes needed regarding the ordering. If there's anything else you want to address (e.g., method naming or null checking), please let me know.


}

Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package com.iemr.common.service.abdmfacility;

public interface AbdmFacilityService {

public String getMappedAbdmFacility(int psmId);

Comment on lines +3 to +6
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

πŸ› οΈ Refactor suggestion

Consider improving the interface contract

The current interface could be enhanced in several ways:

  1. Consider returning a structured DTO instead of String for better type safety and data structure
  2. Add method documentation to explain the purpose and contract
  3. Define expected exceptions in the contract
 public interface AbdmFacilityService {
+    /**
+     * Retrieves the ABDM facility details for a given Provider Service Mapping ID
+     * 
+     * @param psmId Provider Service Mapping ID
+     * @return AbdmFacilityDTO containing the facility details
+     * @throws ResourceNotFoundException if facility not found
+     */
-    public String getMappedAbdmFacility(int psmId);
+    public AbdmFacilityDTO getMappedAbdmFacility(int psmId);
 }

Committable suggestion skipped: line range outside the PR's diff.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai i just need single value from that.. Creating a DTO for it is unnecessary so directly using String

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@helenKaryamsetty I understand. If all you need is a single value and there’s no additional context or complexity, returning it as a String is perfectly fine. Let me know if you need anything else.

(><) …
_(__)
/ … (sad rabbit)
/ \ …

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package com.iemr.common.service.abdmfacility;

import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.stereotype.Service;

import com.iemr.common.data.users.ProviderServiceAddressMapping;
import com.iemr.common.repository.abdmfacility.AbdmFacilityRepository;

@Service
public class AbdmFacilityServiceImpl implements AbdmFacilityService{

@Autowired
private AbdmFacilityRepository abdmFacilityRepo;

@Override
public String getMappedAbdmFacility(int psmId) {
ProviderServiceAddressMapping res = abdmFacilityRepo.getAbdmFacility(psmId);
return res.toString();

}
Comment on lines +16 to +20
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add proper error handling and null checks

The current implementation has several critical issues:

  1. No null checking before calling toString()
  2. No exception handling
  3. Direct toString() usage might not provide meaningful data
  4. Missing logging for debugging and monitoring
+    private static final Logger logger = LoggerFactory.getLogger(AbdmFacilityServiceImpl.class);
+
     @Override
     public String getMappedAbdmFacility(int psmId) {
+        logger.debug("Fetching ABDM facility for psmId: {}", psmId);
+        try {
             ProviderServiceAddressMapping res = abdmFacilityRepo.getAbdmFacility(psmId);
-            return res.toString();
+            if (res == null) {
+                throw new ResourceNotFoundException("ABDM facility not found for psmId: " + psmId);
+            }
+            return objectMapper.writeValueAsString(res);
+        } catch (Exception e) {
+            logger.error("Error fetching ABDM facility for psmId: {}", psmId, e);
+            throw new ServiceException("Error processing ABDM facility data", e);
+        }
     }

Committable suggestion skipped: line range outside the PR's diff.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai i want to return exact response. dont want to throw any error whatever it is... so not throwing any expections. Its handled in UI

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@helenKaryamsetty Understood! If the UI handles all error scenarios, returning the exact response from the service is perfectly valid. No worries about removing exception throwing logic. We can simply return the raw data and let the UI handle nulls, invalid states, or any other conditions.


✏️ Learnings added
Learnt from: helenKaryamsetty
PR: PSMRI/Common-API#145
File: src/main/java/com/iemr/common/service/abdmfacility/AbdmFacilityServiceImpl.java:16-20
Timestamp: 2024-12-18T08:53:22.632Z
Learning: In AbdmFacilityServiceImpl, no exceptions are thrown because the UI layer takes responsibility for handling all error scenarios and directly uses the raw response returned by the repository.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


}
Loading