Feature/signaturerelease #104
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughReplaces stringified responses with Gson JSON in EmployeeMasterController. Adds signature status enrichment in EmployeeMasterServiceImpl. Introduces a POST endpoint to activate/deactivate user signatures and adjusts file download headers in EmployeeSignatureController. Adds transient signatureStatus and persisted failedAttempt to M_User1. Updates M_User getters for null-safety and type alignment. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Controller as EmployeeSignatureController
participant Service as EmployeeSignatureServiceImpl
participant Repo as EmployeeSignatureRepo
Client->>Controller: POST /signature1/activateOrdeActivateSignature (JSON: userID, active)
Controller->>Service: updateUserSignatureStatus(json)
Service->>Service: Parse JSON (userID, active)
Service->>Repo: findOneByUserID(userID)
Repo-->>Service: EmployeeSignature or null
Service->>Service: Set isDeleted = !active (if entity exists)
Service->>Repo: save(entity)
Repo-->>Service: Updated entity
Service-->>Controller: EmployeeSignature
Controller-->>Client: OutputResponse (JSON)
note over Controller,Service: New/modified flow
sequenceDiagram
autonumber
actor Client
participant Ctl as EmployeeMasterController
participant Svc as EmployeeMasterServiceImpl
participant ERepo as EmployeeSignatureRepo
Client->>Ctl: GET /employee/byDesignation
Ctl->>Svc: getEmployeeByDesiganationID(...)
Svc->>Svc: Fetch List<M_User1>
loop for each user
Svc->>ERepo: findOneByUserID(user.userID)
ERepo-->>Svc: EmployeeSignature or null
Svc->>Svc: Set user.signatureStatus (Active/InActive/null)
end
Svc-->>Ctl: List<M_User1>
Ctl-->>Client: JSON via Gson (@Expose only)
note over Ctl,Svc: Serialization/enrichment updated
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (1 warning, 2 inconclusive)❌ Failed checks (1 warning, 2 inconclusive)
Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/com/iemr/admin/data/user/M_User.java (1)
86-118: Constructor bug: field never set (shadowed assignment)
userID = userID;assigns the parameter to itself; the field remains default. Set the instance field explicitly.Apply this diff:
- userID = userID; + this.userID = userID;src/main/java/com/iemr/admin/data/employeemaster/M_User1.java (1)
89-89: Sensitive data leakage via @expose
@Exposeon Aadhaar, PAN, Password, and AgentPassword will serialize these in API responses (see controller change to Gson withexcludeFieldsWithoutExposeAnnotation). Remove or setserialize=false.Apply this diff:
- @Expose + @Expose(deserialize = true, serialize = false) @Column(name="AadhaarNo") private String aadhaarNo; - @Expose + @Expose(deserialize = true, serialize = false) @Column(name="PAN") private String pAN; - @Expose + @Expose(deserialize = true, serialize = false) @Column(name="Password") private String password; - @Expose + @Expose(deserialize = true, serialize = false) @Column(name="AgentPassword") private String agentPassword;If other PII fields must be hidden (e.g.,
EmailID,ContactNo), extend accordingly.Also applies to: 92-92, 111-111, 120-120
🧹 Nitpick comments (4)
src/main/java/com/iemr/admin/data/user/M_User.java (1)
266-268: Null-safe wrapper return is fine; consider getter namingReturning
Booleanavoids NPEs. However,isIsSupervisor()is awkward for a wrapper; considergetIsSupervisor()orisSupervisor()for bean-compatibility.src/main/java/com/iemr/admin/data/employeemaster/M_User1.java (1)
205-208: signatureStatus as String invites drift; prefer enum or booleanAvoid magic strings ("Active"/"InActive"). Use an enum like
SignatureStatus { ACTIVE, INACTIVE }or aBoolean signatureActiveto make states type-safe and consistent.src/main/java/com/iemr/admin/service/employeemaster/EmployeeSignatureService.java (1)
32-33: Document input contract or accept a typed DTOMethod takes raw JSON
String. Add Javadoc describing expected schema ({"userID": long, "active": boolean}) or switch to a small DTO for validation.Would you like a small
ActivateSignatureRequestDTO and interface update?src/main/java/com/iemr/admin/service/employeemaster/EmployeeMasterServiceImpl.java (1)
1094-1109: Avoid N+1 queries and harden null checks
- Current loop does one query per user. Batch-fetch signatures by user IDs and map by
userID.- Use null-safe deleted check to avoid NPEs on legacy rows.
Apply this minimal safety tweak:
- if (null != signature) { - if (signature.getDeleted()) { + if (signature != null) { + if (Boolean.TRUE.equals(signature.getDeleted())) { user.setSignatureStatus("InActive"); } else { user.setSignatureStatus("Active"); } } else { user.setSignatureStatus(null); }Optional batching sketch (repo needs
findByUserIDIn):List<Integer> ids = getEmpByDesiganation.stream().map(M_User1::getUserID).toList(); Map<Integer, EmployeeSignature> byUser = employeeSignatureRepo.findByUserIDIn(ids) .stream().collect(Collectors.toMap(es -> es.getUserID().intValue(), es -> es)); for (M_User1 user : getEmpByDesiganation) { /* read from byUser */ }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/java/com/iemr/admin/controller/employeemaster/EmployeeMasterController.java(2 hunks)src/main/java/com/iemr/admin/controller/employeemaster/EmployeeSignatureController.java(4 hunks)src/main/java/com/iemr/admin/data/employeemaster/M_User1.java(1 hunks)src/main/java/com/iemr/admin/data/user/M_User.java(2 hunks)src/main/java/com/iemr/admin/service/employeemaster/EmployeeMasterServiceImpl.java(4 hunks)src/main/java/com/iemr/admin/service/employeemaster/EmployeeSignatureService.java(1 hunks)src/main/java/com/iemr/admin/service/employeemaster/EmployeeSignatureServiceImpl.java(2 hunks)
🔇 Additional comments (5)
src/main/java/com/iemr/admin/data/user/M_User.java (1)
274-276: Good null-safe deleted flag
Boolean.TRUE.equals(Deleted)avoids NPE; this is correct.src/main/java/com/iemr/admin/data/employeemaster/M_User1.java (1)
212-218: Accessors look fineGetters/setters align with the new transient field.
src/main/java/com/iemr/admin/service/employeemaster/EmployeeMasterServiceImpl.java (1)
195-197: Repo wiring LGTMAutowiring
EmployeeSignatureRepois consistent with usage below.src/main/java/com/iemr/admin/controller/employeemaster/EmployeeSignatureController.java (2)
104-107: Sensible media type fallbackFallback to
APPLICATION_OCTET_STREAMon parse errors is appropriate.
109-116: Byte-streaming change LGTMUsing the raw
byte[]with accuratecontentLengthis correct.
| Gson gson = new GsonBuilder() | ||
| .excludeFieldsWithoutExposeAnnotation() // Only serialize fields with @Expose | ||
| .create(); | ||
| String json = gson.toJson(employeeBydesiganation); | ||
| response.setResponse(json); |
There was a problem hiding this comment.
PII/credential exposure risk in JSON serialization
Using GsonBuilder().excludeFieldsWithoutExposeAnnotation() will serialize all @Expose fields in M_User1, including password, agentPassword, aadhaarNo, pAN (currently annotated). This will leak sensitive data in API responses.
Two safe options:
- Remove/mark
@Expose(serialize = false)on sensitive fields (preferred; see proposed diffs in M_User1). - Or locally exclude known-sensitive fields here:
- Gson gson = new GsonBuilder()
- .excludeFieldsWithoutExposeAnnotation()
- .create();
+ Gson gson = new GsonBuilder()
+ .excludeFieldsWithoutExposeAnnotation()
+ .addSerializationExclusionStrategy(new com.google.gson.ExclusionStrategy() {
+ public boolean shouldSkipField(com.google.gson.FieldAttributes f) {
+ String n = f.getName();
+ return "password".equals(n)
+ || "agentPassword".equals(n)
+ || "aadhaarNo".equals(n)
+ || "pAN".equals(n);
+ }
+ public boolean shouldSkipClass(Class<?> c) { return false; }
+ })
+ .create();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Gson gson = new GsonBuilder() | |
| .excludeFieldsWithoutExposeAnnotation() // Only serialize fields with @Expose | |
| .create(); | |
| String json = gson.toJson(employeeBydesiganation); | |
| response.setResponse(json); | |
| Gson gson = new GsonBuilder() | |
| .excludeFieldsWithoutExposeAnnotation() // Only serialize fields with @Expose | |
| .addSerializationExclusionStrategy(new com.google.gson.ExclusionStrategy() { | |
| @Override | |
| public boolean shouldSkipField(com.google.gson.FieldAttributes f) { | |
| String n = f.getName(); | |
| return "password".equals(n) | |
| || "agentPassword".equals(n) | |
| || "aadhaarNo".equals(n) | |
| || "pAN".equals(n); | |
| } | |
| @Override | |
| public boolean shouldSkipClass(Class<?> c) { | |
| return false; | |
| } | |
| }) | |
| .create(); | |
| String json = gson.toJson(employeeBydesiganation); | |
| response.setResponse(json); |
🤖 Prompt for AI Agents
In
src/main/java/com/iemr/admin/controller/employeemaster/EmployeeMasterController.java
around lines 1053-1057, the code uses
GsonBuilder().excludeFieldsWithoutExposeAnnotation() which will still serialize
any @Expose fields (including sensitive ones like password, agentPassword,
aadhaarNo, pAN) and leak PII; fix by either 1) updating the M_User1 model to
mark sensitive fields with @Expose(serialize = false) (preferred) so they are
never serialized, or 2) alter this controller to create a Gson instance with an
ExclusionStrategy (or use GsonBuilder().addSerializationExclusionStrategy(...))
that explicitly excludes those sensitive field names/types before calling
toJson; apply the change consistently and run related API tests to confirm
sensitive fields are omitted from responses.
| String fileName = URLEncoder.encode(userSignID.getFileName(), StandardCharsets.UTF_8); | ||
|
|
||
| responseHeaders.set(HttpHeaders.CONTENT_DISPOSITION, | ||
| "attachment; filename=\"" + fileName + "\"; filename*=UTF-8''" + fileName); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use RFC-5987-safe filename; avoid URLEncoder in headers
URLEncoder produces + for spaces (form encoding), which is invalid for filename*. Use ContentDisposition builder with charset to set both safely.
Apply this diff:
- String fileName = URLEncoder.encode(userSignID.getFileName(), StandardCharsets.UTF_8);
-
- responseHeaders.set(HttpHeaders.CONTENT_DISPOSITION,
- "attachment; filename=\"" + fileName + "\"; filename*=UTF-8''" + fileName);
+ ContentDisposition cd = ContentDisposition.attachment()
+ .filename(userSignID.getFileName(), StandardCharsets.UTF_8)
+ .build();
+ responseHeaders.setContentDisposition(cd);Also handle userSignID == null by returning 404.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
src/main/java/com/iemr/admin/controller/employeemaster/EmployeeSignatureController.java
around lines 97 to 101, replace the URLEncoder-based header construction and add
a null check: first, if userSignID == null return
ResponseEntity.status(HttpStatus.NOT_FOUND).build(); otherwise build a
ContentDisposition via
ContentDisposition.attachment().filename(userSignID.getFileName(),
StandardCharsets.UTF_8).build() and set it on
responseHeaders.setContentDisposition(contentDisposition) (or
responseHeaders.set(HttpHeaders.CONTENT_DISPOSITION,
contentDisposition.toString())); remove URLEncoder usage so filename* is
RFC-5987-safe and both plain and encoded filename forms are handled by the
ContentDisposition builder.
| @Operation(summary = "Active or DeActive user Signature") | ||
| @RequestMapping(value = "/activateOrdeActivateSignature", method = { RequestMethod.POST }, produces = { | ||
| "application/json" }) | ||
| public String ActivateUser(@RequestBody String activateUser, HttpServletRequest request) { | ||
| OutputResponse response = new OutputResponse(); | ||
| try { | ||
| EmployeeSignature empSignature = employeeSignatureServiceImpl.updateUserSignatureStatus(activateUser); | ||
| response.setResponse(empSignature.toString()); | ||
| } catch (Exception e) { | ||
| logger.error("Active or Deactivate User Signature failed with exception " + e.getMessage(), e); | ||
| response.setError(e); | ||
| } | ||
| return response.toString(); | ||
| } |
There was a problem hiding this comment.
Auth missing and overexposing response
- Endpoint lacks
headers = "Authorization"while other endpoints require it. This allows unauthenticated status toggles. - Returning full
EmployeeSignature(potentially including BLOB) is heavy and may leak data. Return a minimal DTO.
Apply this diff:
- @RequestMapping(value = "/activateOrdeActivateSignature", method = { RequestMethod.POST }, produces = {
- "application/json" })
+ @RequestMapping(value = "/activateOrdeActivateSignature", headers = "Authorization",
+ method = { RequestMethod.POST }, produces = { "application/json" })
public String ActivateUser(@RequestBody String activateUser, HttpServletRequest request) {
OutputResponse response = new OutputResponse();
try {
- EmployeeSignature empSignature = employeeSignatureServiceImpl.updateUserSignatureStatus(activateUser);
- response.setResponse(empSignature.toString());
+ EmployeeSignature empSignature = employeeSignatureServiceImpl.updateUserSignatureStatus(activateUser);
+ boolean active = empSignature.getDeleted() == null ? false : !empSignature.getDeleted();
+ response.setResponse("{\"userID\":" + empSignature.getUserID() + ",\"active\":" + active + "}");
} catch (Exception e) {
logger.error("Active or Deactivate User Signature failed with exception " + e.getMessage(), e);
response.setError(e);
}
return response.toString();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Operation(summary = "Active or DeActive user Signature") | |
| @RequestMapping(value = "/activateOrdeActivateSignature", method = { RequestMethod.POST }, produces = { | |
| "application/json" }) | |
| public String ActivateUser(@RequestBody String activateUser, HttpServletRequest request) { | |
| OutputResponse response = new OutputResponse(); | |
| try { | |
| EmployeeSignature empSignature = employeeSignatureServiceImpl.updateUserSignatureStatus(activateUser); | |
| response.setResponse(empSignature.toString()); | |
| } catch (Exception e) { | |
| logger.error("Active or Deactivate User Signature failed with exception " + e.getMessage(), e); | |
| response.setError(e); | |
| } | |
| return response.toString(); | |
| } | |
| @Operation(summary = "Active or DeActive user Signature") | |
| @RequestMapping( | |
| value = "/activateOrdeActivateSignature", | |
| headers = "Authorization", | |
| method = { RequestMethod.POST }, | |
| produces = { "application/json" } | |
| ) | |
| public String ActivateUser(@RequestBody String activateUser, HttpServletRequest request) { | |
| OutputResponse response = new OutputResponse(); | |
| try { | |
| EmployeeSignature empSignature = | |
| employeeSignatureServiceImpl.updateUserSignatureStatus(activateUser); | |
| boolean active = | |
| empSignature.getDeleted() == null | |
| ? false | |
| : !empSignature.getDeleted(); | |
| response.setResponse( | |
| "{\"userID\":" | |
| empSignature.getUserID() | |
| ",\"active\":" | |
| active | |
| "}" | |
| ); | |
| } catch (Exception e) { | |
| logger.error( | |
| "Active or Deactivate User Signature failed with exception " + e.getMessage(), | |
| e | |
| ); | |
| response.setError(e); | |
| } | |
| return response.toString(); | |
| } |
| @Override | ||
| public EmployeeSignature updateUserSignatureStatus(String activateUser) { | ||
| JSONObject obj = new JSONObject(activateUser); | ||
| Long userID = obj.getLong("userID"); | ||
| //String role = obj.getString("role"); | ||
| boolean active = obj.getBoolean("active"); | ||
| EmployeeSignature signature = employeeSignatureRepo.findOneByUserID(userID); | ||
| if (active) { | ||
| signature.setDeleted(false); | ||
| } else { | ||
| signature.setDeleted(true); | ||
| } | ||
| return employeeSignatureRepo.save(signature); | ||
| } |
There was a problem hiding this comment.
NPE when signature record is absent; add not-found handling
findOneByUserID may return null; subsequent setDeleted will NPE. Validate existence and fail fast (or upsert), and simplify flag toggle.
Apply this diff:
- EmployeeSignature signature = employeeSignatureRepo.findOneByUserID(userID);
- if (active) {
- signature.setDeleted(false);
- } else {
- signature.setDeleted(true);
- }
- return employeeSignatureRepo.save(signature);
+ EmployeeSignature signature = employeeSignatureRepo.findOneByUserID(userID);
+ if (signature == null) {
+ throw new IllegalArgumentException("No signature found for userID: " + userID);
+ }
+ signature.setDeleted(!active);
+ return employeeSignatureRepo.save(signature);Optional: accept a DTO and validate schema; consider auditing modifiedBy.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Override | |
| public EmployeeSignature updateUserSignatureStatus(String activateUser) { | |
| JSONObject obj = new JSONObject(activateUser); | |
| Long userID = obj.getLong("userID"); | |
| //String role = obj.getString("role"); | |
| boolean active = obj.getBoolean("active"); | |
| EmployeeSignature signature = employeeSignatureRepo.findOneByUserID(userID); | |
| if (active) { | |
| signature.setDeleted(false); | |
| } else { | |
| signature.setDeleted(true); | |
| } | |
| return employeeSignatureRepo.save(signature); | |
| } | |
| @Override | |
| public EmployeeSignature updateUserSignatureStatus(String activateUser) { | |
| JSONObject obj = new JSONObject(activateUser); | |
| Long userID = obj.getLong("userID"); | |
| //String role = obj.getString("role"); | |
| boolean active = obj.getBoolean("active"); | |
| EmployeeSignature signature = employeeSignatureRepo.findOneByUserID(userID); | |
| if (signature == null) { | |
| throw new IllegalArgumentException("No signature found for userID: " + userID); | |
| } | |
| signature.setDeleted(!active); | |
| return employeeSignatureRepo.save(signature); | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/com/iemr/admin/service/employeemaster/EmployeeSignatureServiceImpl.java
around lines 71 to 84, the code assumes
employeeSignatureRepo.findOneByUserID(userID) always returns a non-null
EmployeeSignature which causes an NPE when no record exists; modify the method
to check the repository result before using it and handle the not-found case
(either throw a clear exception like EntityNotFoundException with a helpful
message or create a new EmployeeSignature for an upsert), replace the redundant
if/else with a single setDeleted(!active) call, and then save and return the
entity; also validate the incoming JSON fields (userID and active) before use
and consider setting modifiedBy/modifiedAt if audit fields exist.
|
* Compile error resolved * fix: cherry pic the #101 pr * fix: cherry pic the #101 pr * Main branch changes missed * fix: cherry pic the #103 pr * fix: cherry pic the #104 pr * fix: cherry pic the #104 pr * fix: cherry pick the #104 pr * fix code rabbit comments --------- Co-authored-by: Ravi Shanigarapu <ravi.shanigarapu@wipro.com>
📋 Description
JIRA ID: AMM-1807
Please provide a summary of the change and the motivation behind it. Include relevant context and details.
✅ Type of Change
ℹ️ Additional Information
Please describe how the changes were tested, and include any relevant screenshots, logs, or other information that provides additional context.
Summary by CodeRabbit
New Features
Bug Fixes