-
Notifications
You must be signed in to change notification settings - Fork 30
Enhancement #89
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Enhancement #89
Changes from all commits
3c2135e
23cccac
83f693e
8ae4c09
cdd7b2a
128c8f9
a069382
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,92 @@ | ||
| package com.iemr.admin.controller.bulkRegistration; | ||
|
|
||
| import com.iemr.admin.repo.employeemaster.EmployeeMasterRepoo; | ||
| import com.iemr.admin.service.bulkRegistration.BulkRegistrationService; | ||
| import com.iemr.admin.service.bulkRegistration.BulkRegistrationServiceImpl; | ||
| import com.iemr.admin.service.bulkRegistration.EmployeeXmlService; | ||
| import com.iemr.admin.service.locationmaster.LocationMasterServiceInter; | ||
| import io.swagger.v3.oas.annotations.Operation; | ||
| import jakarta.servlet.http.HttpServletRequest; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
| import org.springframework.beans.factory.annotation.Autowired; | ||
| import org.springframework.core.io.ClassPathResource; | ||
| import org.springframework.http.HttpHeaders; | ||
| import org.springframework.http.MediaType; | ||
| import org.springframework.http.ResponseEntity; | ||
| import org.springframework.web.bind.annotation.*; | ||
|
|
||
| import java.lang.reflect.Method; | ||
| import java.util.HashMap; | ||
| import java.util.Map; | ||
|
|
||
| @RestController | ||
| public class BulkRegistrationController { | ||
| @Autowired | ||
| private EmployeeXmlService employeeXmlService; | ||
| @Autowired | ||
| BulkRegistrationServiceImpl bulkRegistrationServiceimpl; | ||
| private Logger logger = LoggerFactory.getLogger(this.getClass().getSimpleName()); | ||
|
|
||
|
|
||
| @Autowired | ||
| BulkRegistrationService bulkRegistrationService; | ||
|
|
||
| @Autowired | ||
| private EmployeeMasterRepoo employeeMasterRepoo; | ||
| private Map<String, Object> errorResponse = new HashMap<>(); | ||
| @Autowired | ||
| private LocationMasterServiceInter locationMasterServiceInter; | ||
| private Map<String, Object> response = new HashMap<>(); | ||
|
|
||
| @CrossOrigin() | ||
| @PostMapping(value = "/bulkRegistration", headers = "Authorization") | ||
| public ResponseEntity<Map<String, Object>> registerBulkUser(@RequestBody String m_user, @RequestHeader String authorization) { | ||
| bulkRegistrationServiceimpl.bulkRegistrationErrors.clear(); | ||
| logger.info("Bulk registration request received. Request payload is omitted from logs."); | ||
| try { | ||
| bulkRegistrationService.registerBulkUser(m_user, authorization); | ||
| response.put("status", "Success"); | ||
| response.put("statusCode", 200); | ||
| response.put("totalUser", bulkRegistrationServiceimpl.totalEmployeeListSize); | ||
| response.put("registeredUser", bulkRegistrationServiceimpl.m_bulkUser.size()); | ||
| response.put("error", bulkRegistrationServiceimpl.errorLogs.toString()); | ||
|
|
||
| bulkRegistrationServiceimpl.m_bulkUser.clear(); | ||
| bulkRegistrationServiceimpl.m_UserDemographics.clear(); | ||
| bulkRegistrationServiceimpl.errorLogs.clear(); | ||
| bulkRegistrationServiceimpl.totalEmployeeListSize=0; | ||
|
|
||
| } catch (Exception e) { | ||
| response.put("message", e.getMessage()); | ||
| response.put("statusCode", 500); | ||
|
|
||
| } | ||
| return ResponseEntity.ok(response); | ||
|
|
||
| } | ||
|
Comment on lines
+43
to
+67
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Improve error handling, input validation, and encapsulation. Several issues need to be addressed:
Apply this diff to improve the implementation: @PostMapping(value = "/bulkRegistration", headers = "Authorization")
public ResponseEntity<Map<String, Object>> registerBulkUser(@RequestBody String m_user, @RequestHeader String authorization) {
- bulkRegistrationServiceimpl.bulkRegistrationErrors.clear();
+ if (m_user == null || m_user.trim().isEmpty()) {
+ response.put("message", "Request body cannot be empty");
+ response.put("statusCode", 400);
+ return ResponseEntity.badRequest().body(response);
+ }
+
logger.info("Bulk registration request received. Request payload is omitted from logs.");
try {
+ bulkRegistrationService.clearPreviousErrors(); // Add this method to service
bulkRegistrationService.registerBulkUser(m_user, authorization);
// ... rest of success logic
- } catch (Exception e) {
+ } catch (IllegalArgumentException e) {
+ response.put("message", "Invalid input: " + e.getMessage());
+ response.put("statusCode", 400);
+ return ResponseEntity.badRequest().body(response);
+ } catch (Exception e) {
+ logger.error("Unexpected error during bulk registration", e);
response.put("message", e.getMessage());
response.put("statusCode", 500);
+ return ResponseEntity.status(500).body(response);
}
- return ResponseEntity.ok(response);
}
π€ Prompt for AI Agents |
||
|
|
||
| @CrossOrigin() | ||
| @GetMapping("/download-error-sheet") | ||
| public ResponseEntity<byte[]> downloadErrorSheet() { | ||
| try { | ||
| byte[] fileContent = bulkRegistrationServiceimpl.insertErrorLog(); | ||
|
|
||
| HttpHeaders headers = new HttpHeaders(); | ||
| headers.add("Content-Disposition", "attachment; filename=error_log.xlsx"); | ||
|
|
||
| if(!bulkRegistrationServiceimpl.bulkRegistrationErrors.isEmpty()){ | ||
| bulkRegistrationServiceimpl.bulkRegistrationErrors.clear(); | ||
| } | ||
| return ResponseEntity.ok() | ||
| .headers(headers) | ||
| .contentType(MediaType.APPLICATION_OCTET_STREAM) | ||
| .body(fileContent); | ||
|
|
||
| } catch (Exception e) { | ||
| return ResponseEntity.status(500).body(null); | ||
| } | ||
| } | ||
|
Comment on lines
+69
to
+89
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. π οΈ Refactor suggestion Improve error handling and security in error sheet download. The endpoint has several issues:
Apply this diff: @GetMapping("/download-error-sheet")
public ResponseEntity<byte[]> downloadErrorSheet() {
try {
byte[] fileContent = bulkRegistrationServiceimpl.insertErrorLog();
+
+ if (fileContent == null || fileContent.length == 0) {
+ logger.warn("No error data available for download");
+ return ResponseEntity.noContent().build();
+ }
HttpHeaders headers = new HttpHeaders();
headers.add("Content-Disposition", "attachment; filename=error_log.xlsx");
- if(!bulkRegistrationServiceimpl.bulkRegistrationErrors.isEmpty()){
- bulkRegistrationServiceimpl.bulkRegistrationErrors.clear();
- }
+ bulkRegistrationService.clearErrorsAfterDownload(); // Add this method to service
return ResponseEntity.ok()
.headers(headers)
.contentType(MediaType.APPLICATION_OCTET_STREAM)
.body(fileContent);
} catch (Exception e) {
+ logger.error("Error generating error sheet", e);
- return ResponseEntity.status(500).body(null);
+ return ResponseEntity.status(500)
+ .body("Error generating error sheet".getBytes());
}
}
π€ Prompt for AI Agents |
||
|
|
||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| package com.iemr.admin.data.bulkuser; | ||
|
|
||
| import lombok.Data; | ||
|
|
||
| import java.util.List; | ||
|
|
||
| @Data | ||
| public class BulkRegistrationError { | ||
| String userName; | ||
| Integer rowNumber; | ||
| List<String> error; | ||
| } | ||
|
Comment on lines
+7
to
+12
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. π οΈ Refactor suggestion Add field validation and improve encapsulation. The class fields lack validation and access control, which could lead to inconsistent data states. +import jakarta.validation.constraints.NotNull;
+import jakarta.validation.constraints.Positive;
+import jakarta.validation.constraints.NotEmpty;
@Data
public class BulkRegistrationError {
+ @NotEmpty(message = "Username cannot be empty")
- String userName;
+ private String userName;
+
+ @NotNull(message = "Row number cannot be null")
+ @Positive(message = "Row number must be positive")
- Integer rowNumber;
+ private Integer rowNumber;
+
+ @NotNull(message = "Error list cannot be null")
- List<String> error;
+ private List<String> errors;
}Also consider renaming π€ Prompt for AI Agents |
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,102 @@ | ||||||||||||||||||||||
| package com.iemr.admin.data.bulkuser; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| import com.fasterxml.jackson.dataformat.xml.annotation.JacksonXmlProperty; | ||||||||||||||||||||||
| import com.fasterxml.jackson.dataformat.xml.annotation.JacksonXmlRootElement; | ||||||||||||||||||||||
| import lombok.Data; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @Data | ||||||||||||||||||||||
| @JacksonXmlRootElement(localName = "Employee") | ||||||||||||||||||||||
| public class Employee { | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @JacksonXmlProperty(localName = "Title") | ||||||||||||||||||||||
| private String title=""; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @JacksonXmlProperty(localName = "FirstName") | ||||||||||||||||||||||
| private String firstName=""; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @JacksonXmlProperty(localName = "MiddleName") | ||||||||||||||||||||||
| private String middleName=""; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @JacksonXmlProperty(localName = "LastName") | ||||||||||||||||||||||
| private String lastName=""; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @JacksonXmlProperty(localName = "Gender") | ||||||||||||||||||||||
| private String gender=""; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @JacksonXmlProperty(localName = "ContactNo") | ||||||||||||||||||||||
| private String contactNo=""; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @JacksonXmlProperty(localName = "Designation") | ||||||||||||||||||||||
| private String designation=""; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @JacksonXmlProperty(localName = "EmergencyContactNo") | ||||||||||||||||||||||
| private String emergencyContactNo=""; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @JacksonXmlProperty(localName = "DateOfBirth") | ||||||||||||||||||||||
| private String dob=""; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @JacksonXmlProperty(localName = "Age") | ||||||||||||||||||||||
| private int age=0; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @JacksonXmlProperty(localName = "Email") | ||||||||||||||||||||||
| private String email=""; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @JacksonXmlProperty(localName = "MaritalStatus") | ||||||||||||||||||||||
| private String maritalStatus=""; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @JacksonXmlProperty(localName = "AadhaarNo") | ||||||||||||||||||||||
| private String aadhaarNo=""; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @JacksonXmlProperty(localName = "PAN") | ||||||||||||||||||||||
| private String pan=""; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @JacksonXmlProperty(localName = "Qualification") | ||||||||||||||||||||||
| private String qualification=""; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @JacksonXmlProperty(localName = "FatherName") | ||||||||||||||||||||||
| private String fatherName=""; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @JacksonXmlProperty(localName = "MotherName") | ||||||||||||||||||||||
| private String motherName=""; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @JacksonXmlProperty(localName = "Community") | ||||||||||||||||||||||
| private String community=""; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @JacksonXmlProperty(localName = "Religion") | ||||||||||||||||||||||
| private String religion=""; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @JacksonXmlProperty(localName = "CurrentAddressLine1") | ||||||||||||||||||||||
| private String addressLine1=""; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @JacksonXmlProperty(localName = "CurrentState") | ||||||||||||||||||||||
| private String state=""; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @JacksonXmlProperty(localName = "CurrentDistrict") | ||||||||||||||||||||||
| private String district=""; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @JacksonXmlProperty(localName = "CurrentPincode") | ||||||||||||||||||||||
| private String pincode=""; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @JacksonXmlProperty(localName = "PermanentAddressLine1") | ||||||||||||||||||||||
| private String permanentAddressLine1=""; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @JacksonXmlProperty(localName = "PermanentState") | ||||||||||||||||||||||
| private String permanentState=""; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @JacksonXmlProperty(localName = "PermanentDistrict") | ||||||||||||||||||||||
| private String permanentDistrict=""; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @JacksonXmlProperty(localName = "PermanentPincode") | ||||||||||||||||||||||
| private String permanentPincode=""; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @JacksonXmlProperty(localName = "DateOfJoining") | ||||||||||||||||||||||
| private String dateOfJoining=""; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @JacksonXmlProperty(localName = "UserName") | ||||||||||||||||||||||
| private String UserName=""; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @JacksonXmlProperty(localName = "Password") | ||||||||||||||||||||||
| private String Password=""; | ||||||||||||||||||||||
|
Comment on lines
+95
to
+99
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. π οΈ Refactor suggestion Fix field naming convention inconsistencies. The fields Apply this diff to fix the naming convention: - @JacksonXmlProperty(localName = "UserName")
- private String UserName="";
+ @JacksonXmlProperty(localName = "UserName")
+ private String userName="";
- @JacksonXmlProperty(localName = "Password")
- private String Password="";
+ @JacksonXmlProperty(localName = "Password")
+ private String password="";π Committable suggestion
Suggested change
π€ Prompt for AI Agents |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| } | ||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,17 @@ | ||||||||||||||||||||||||||||||||||||||||||||
| package com.iemr.admin.data.bulkuser; | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| import com.fasterxml.jackson.annotation.JsonProperty; | ||||||||||||||||||||||||||||||||||||||||||||
| import com.fasterxml.jackson.dataformat.xml.annotation.JacksonXmlElementWrapper; | ||||||||||||||||||||||||||||||||||||||||||||
| import com.fasterxml.jackson.dataformat.xml.annotation.JacksonXmlRootElement; | ||||||||||||||||||||||||||||||||||||||||||||
| import lombok.Data; | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| import java.util.List; | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| @Data | ||||||||||||||||||||||||||||||||||||||||||||
| @JacksonXmlRootElement(localName = "Employees") | ||||||||||||||||||||||||||||||||||||||||||||
| public class EmployeeList { | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| @JsonProperty("Employee") | ||||||||||||||||||||||||||||||||||||||||||||
| @JacksonXmlElementWrapper(useWrapping = false) // To avoid extra nested array in XML | ||||||||||||||||||||||||||||||||||||||||||||
| private List<Employee> employees; | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+10
to
+17
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. π οΈ Refactor suggestion Add validation and security considerations for XML processing. The class lacks validation constraints and security considerations for XML processing, which could lead to XML injection or processing issues. +import jakarta.validation.constraints.NotNull;
+import jakarta.validation.Valid;
@Data
@JacksonXmlRootElement(localName = "Employees")
public class EmployeeList {
+ @NotNull(message = "Employee list cannot be null")
+ @Valid
@JsonProperty("Employee")
@JacksonXmlElementWrapper(useWrapping = false) // To avoid extra nested array in XML
private List<Employee> employees;
}π Committable suggestion
Suggested change
π€ Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -40,9 +40,16 @@ public interface EmployeeMasterRepoo extends CrudRepository<M_User1, Integer> | |||||||||
| @Query("SELECT u FROM M_User1 u WHERE u.userID=:userID AND deleted=false") | ||||||||||
| M_User1 editEmployee(@Param("userID") Integer userID); | ||||||||||
|
|
||||||||||
| @Query("SELECT u FROM M_User1 u WHERE u.aadhaarNo=:aadhaar AND deleted=false ") | ||||||||||
| M_User1 findEmployeeAadhaarNo(@Param("aadhaar") String userName); | ||||||||||
|
Comment on lines
+43
to
+44
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix parameter naming inconsistency. The parameter is named @Query("SELECT u FROM M_User1 u WHERE u.aadhaarNo=:aadhaar AND deleted=false ")
-M_User1 findEmployeeAadhaarNo(@Param("aadhaar") String userName);
+M_User1 findEmployeeByAadhaar(@Param("aadhaar") String aadhaar);π€ Prompt for AI Agents |
||||||||||
|
|
||||||||||
| @Query("SELECT u FROM M_User1 u WHERE u.userName=:userName AND deleted=false ") | ||||||||||
| M_User1 findEmployeeByName(@Param("userName") String userName); | ||||||||||
|
|
||||||||||
|
|
||||||||||
| @Query("SELECT u FROM M_User1 u WHERE u.contactNo=:contactNo AND deleted=false ") | ||||||||||
| M_User1 findEmployeeByContact(@Param("contactNo") String userName); | ||||||||||
|
Comment on lines
+50
to
+51
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix parameter naming inconsistency. Similar to the Aadhaar method, the parameter should be named @Query("SELECT u FROM M_User1 u WHERE u.contactNo=:contactNo AND deleted=false ")
-M_User1 findEmployeeByContact(@Param("contactNo") String userName);
+M_User1 findEmployeeByContact(@Param("contactNo") String contactNo);π Committable suggestion
Suggested change
π€ Prompt for AI Agents |
||||||||||
|
|
||||||||||
| @Query("SELECT u FROM M_User1 u WHERE u.userName=:userName OR u.aadhaarNo=:aadhaarNo OR u.pAN=:getpAN OR u.employeeID=:employeeID OR u.healthProfessionalID=:healthProfessionalID AND deleted=false ") | ||||||||||
| M_User1 checkingEmpDetails(@Param("userName") String userName, @Param("aadhaarNo") String aadhaarNo, | ||||||||||
| @Param("getpAN") String getpAN,@Param("employeeID") String employeeID, @Param("healthProfessionalID") String healthProfessionalID); | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,5 @@ | ||||||||||||||||||||||||||||||
| package com.iemr.admin.service.bulkRegistration; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| public interface BulkRegistrationService { | ||||||||||||||||||||||||||||||
| void registerBulkUser(String user,String authorization); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
Comment on lines
+3
to
+5
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. π οΈ Refactor suggestion Enhance the interface design for better error handling and feedback. The current interface design has several limitations:
Consider this improved design: public interface BulkRegistrationService {
- void registerBulkUser(String user,String authorization);
+ /**
+ * Registers users in bulk from XML data
+ * @param xmlData The XML data containing user information
+ * @param authorizationToken The authorization token for the request
+ * @return Registration result with success count and error details
+ * @throws BulkRegistrationException if processing fails
+ */
+ BulkRegistrationResult registerBulkUser(String xmlData, String authorizationToken)
+ throws BulkRegistrationException;
}π Committable suggestion
Suggested change
π€ Prompt for AI Agents |
||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
π οΈ Refactor suggestion
β Verification inconclusive
Address dependency injection and security concerns.
Several issues with the controller structure:
@CrossOrigin()without restrictions allows all originsApply these changes:
Verify if rate limiting is implemented elsewhere:
π Script executed:
Length of output: 47
Address dependency injection and security concerns.
Several issues with the controller structure:
@CrossOrigin()without restrictions allows all originsApply these changes:
Verify if rate limiting is implemented elsewhere:
π€ Prompt for AI Agents