Skip to content
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.

Sign up for GitHub

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

Govt StateID and DistrictId changes #58

Merged
merged 2 commits into from
Dec 16, 2024
Merged

Govt StateID and DistrictId changes #58

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6bc8808
GovtStateID,DistrictID changes
ravishanigarapu Dec 16, 2024
7126972
Http changes
ravishanigarapu Dec 16, 2024
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
13 changes: 13 additions & 0 deletions src/main/java/com/iemr/tm/data/location/Districts.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,13 @@ public Integer getStateID() {
public void setStateID(Integer stateID) {
this.stateID = stateID;
}
@Column(name = "GovtStateID")
@Expose
private Integer govtLGDStateID;

@Column(name = "GovtDistrictID")
@Expose
private Integer govtLGDDistrictID;
Comment on lines +89 to +95
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add getter/setter methods for new LGD ID fields.

The new government LGD ID fields are missing their accessor methods, which are needed for proper encapsulation and data access.

+    public Integer getGovtLGDStateID() {
+        return govtLGDStateID;
+    }
+
+    public void setGovtLGDStateID(Integer govtLGDStateID) {
+        this.govtLGDStateID = govtLGDStateID;
+    }
+
+    public Integer getGovtLGDDistrictID() {
+        return govtLGDDistrictID;
+    }
+
+    public void setGovtLGDDistrictID(Integer govtLGDDistrictID) {
+        this.govtLGDDistrictID = govtLGDDistrictID;
+    }
πŸ“ 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.

Suggested change
@Column(name = "GovtStateID")
@Expose
private Integer govtLGDStateID;
@Column(name = "GovtDistrictID")
@Expose
private Integer govtLGDDistrictID;
@Column(name = "GovtStateID")
@Expose
private Integer govtLGDStateID;
@Column(name = "GovtDistrictID")
@Expose
private Integer govtLGDDistrictID;
public Integer getGovtLGDStateID() {
return govtLGDStateID;
}
public void setGovtLGDStateID(Integer govtLGDStateID) {
this.govtLGDStateID = govtLGDStateID;
}
public Integer getGovtLGDDistrictID() {
return govtLGDDistrictID;
}
public void setGovtLGDDistrictID(Integer govtLGDDistrictID) {
this.govtLGDDistrictID = govtLGDDistrictID;
}

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 using constaructor 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.

@ravishanigarapu Understood. If you're using the constructor and don't require getter and setter methods for these fields, that's acceptable. Thanks for clarifying.


public Boolean getDeleted() {
return deleted;
Expand Down Expand Up @@ -133,6 +140,12 @@ public Districts(Integer DistrictID, String DistrictName, Integer stateId, Strin
this.districtID = DistrictID;
this.districtName = DistrictName;
}
public Districts(Integer districtID, String districtName, Integer govtLGDStateID, Integer govtLGDDistrictID) {
this.districtID = districtID;
this.districtName = districtName;
this.govtLGDStateID = govtLGDStateID;
this.govtLGDDistrictID = govtLGDDistrictID;
}

public int getDistrictID() {
return this.districtID.intValue();
Expand Down
11 changes: 10 additions & 1 deletion src/main/java/com/iemr/tm/data/location/States.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,10 @@ public class States {
@Column(name = "StateID")
@Expose
private Integer stateID;

@Column(name = "GovtStateID")
@Expose
private Integer govtLGDStateID;
Comment on lines +46 to +49
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add getter/setter methods for govtLGDStateID field

The new field lacks accessor methods which are needed for proper encapsulation. Also consider aligning the column name with the field name for consistency.

Add the following methods:

+ public Integer getGovtLGDStateID() {
+     return this.govtLGDStateID;
+ }
+
+ public void setGovtLGDStateID(Integer govtLGDStateID) {
+     this.govtLGDStateID = govtLGDStateID;
+ }

Consider renaming the column to match the field:

- @Column(name = "GovtStateID")
+ @Column(name = "GovtLGDStateID")
πŸ“ 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.

Suggested change
@Column(name = "GovtStateID")
@Expose
private Integer govtLGDStateID;
@Column(name = "GovtLGDStateID")
@Expose
private Integer govtLGDStateID;
public Integer getGovtLGDStateID() {
return this.govtLGDStateID;
}
public void setGovtLGDStateID(Integer govtLGDStateID) {
this.govtLGDStateID = govtLGDStateID;
}

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 In DB its mentioned same

Copy link
Contributor

Choose a reason for hiding this comment

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

@ravishanigarapu Understood, thank you for the clarification. Since the database column is named GovtStateID, it's appropriate to keep the annotation as @Column(name = "GovtStateID").

However, the field govtLGDStateID still lacks getter and setter methods. Consider adding them to ensure proper encapsulation and maintain consistency.

✏️ Learnings added 
Learnt from: ravishanigarapu
PR: PSMRI/TM-API#58
File: src/main/java/com/iemr/tm/data/location/States.java:46-49
Timestamp: 2024-12-16T11:43:56.194Z
Learning: In `src/main/java/com/iemr/tm/data/location/States.java`, the field `govtLGDStateID` corresponds to the database column `GovtStateID`, so the `@Column` annotation should be `@Column(name = "GovtStateID")`.

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.


@Column(name = "StateName")
@Expose
Expand Down Expand Up @@ -155,5 +159,10 @@ public String getStateName() {
public void setStateName(String stateName) {
this.stateName = stateName;
}


public States(int StateID, String StateName, Integer govtLGDStateID) {
this.stateID = StateID;
this.stateName = StateName;
this.govtLGDStateID = govtLGDStateID;
}
Comment on lines +163 to +167
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

πŸ› οΈ Refactor suggestion

Add input validation and deprecate old constructor

The new constructor should validate the govtLGDStateID parameter. Also, the old constructor should be marked as deprecated since it doesn't initialize all fields.

Apply these changes:

+ @Deprecated(since = "AMM-1166", forRemoval = false)
  public States(int StateID, String StateName) {
      this.stateID = StateID;
      this.stateName = StateName;
  }

  public States(int StateID, String StateName, Integer govtLGDStateID) {
      this.stateID = StateID;
      this.stateName = StateName;
+     if (govtLGDStateID != null && govtLGDStateID <= 0) {
+         throw new IllegalArgumentException("govtLGDStateID must be positive when provided");
+     }
      this.govtLGDStateID = govtLGDStateID;
  }

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 it is used for other methods its not deprecated

Copy link
Contributor

Choose a reason for hiding this comment

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

@ravishanigarapu, understood. Thank you for the clarification that the old constructor is used in other methods and should not be deprecated.

（ ・ω・）?

✏️ Learnings added 
Learnt from: ravishanigarapu
PR: PSMRI/TM-API#58
File: src/main/java/com/iemr/tm/data/location/States.java:163-167
Timestamp: 2024-12-16T11:44:54.815Z
Learning: The constructor 'public States(int StateID, String StateName)' in 'src/main/java/com/iemr/tm/data/location/States.java' is used in other methods and should not be deprecated.

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.

🧠 Learnings used 
Learnt from: ravishanigarapu
PR: PSMRI/TM-API#58
File: src/main/java/com/iemr/tm/data/location/States.java:46-49
Timestamp: 2024-12-16T11:43:56.377Z
Learning: In `src/main/java/com/iemr/tm/data/location/States.java`, the field `govtLGDStateID` corresponds to the database column `GovtStateID`, so the `@Column` annotation should be `@Column(name = "GovtStateID")`.

}
2 changes: 1 addition & 1 deletion src/main/java/com/iemr/tm/repo/location/DistrictMasterRepo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
@Repository

public interface DistrictMasterRepo extends CrudRepository<Districts, Integer> {
@Query(" SELECT d.districtID, d.districtName FROM Districts d " + " WHERE d.stateID = :stateID AND d.deleted != true ")
@Query(" SELECT d.districtID, d.districtName,d.govtLGDStateID,d.govtLGDDistrictID FROM Districts d " + " WHERE d.stateID = :stateID AND d.deleted != true ")
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

πŸ› οΈ Refactor suggestion

Improve query readability, type safety, and input validation

Similar to StateMasterRepo, this query would benefit from better type safety and readability improvements. Also add parameter validation.

Suggested improvements:

+ @NotNull(message = "stateID cannot be null")
+ @Positive(message = "stateID must be positive")
  @Param("stateID") Integer stateID

- @Query(" SELECT d.districtID, d.districtName,d.govtLGDStateID,d.govtLGDDistrictID FROM Districts d " + " WHERE d.stateID = :stateID AND d.deleted != true ")
+ @Query("SELECT d.districtID as id, " +
+        "       d.districtName as name, " +
+        "       d.govtLGDStateID as stateLgdId, " +
+        "       d.govtLGDDistrictID as districtLgdId " +
+        "FROM Districts d " +
+        "WHERE d.stateID = :stateID " +
+        "AND d.deleted != true")

Create a DTO for type-safe returns:

public class DistrictDTO {
    private Integer id;
    private String name;
    private Integer stateLgdId;
    private Integer districtLgdId;
    
    // Constructor matching query projection
    public DistrictDTO(Integer id, String name, 
                      Integer stateLgdId, Integer districtLgdId) {
        this.id = id;
        this.name = name;
        this.stateLgdId = stateLgdId;
        this.districtLgdId = districtLgdId;
    }
    // Getters
}

Update method signature:

- public ArrayList<Object[]> getDistrictMaster(@Param("stateID") Integer stateID);
+ public List<DistrictDTO> getDistrictMaster(@Param("stateID") Integer stateID);

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 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.

@ravishanigarapu, understood.

public ArrayList<Object[]> getDistrictMaster(@Param("stateID") Integer stateID);

}
2 changes: 1 addition & 1 deletion src/main/java/com/iemr/tm/repo/location/StateMasterRepo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,6 @@
@Repository

public interface StateMasterRepo extends CrudRepository<States, Integer> {
@Query(" SELECT stateID, stateName FROM States WHERE deleted != true ")
@Query(" SELECT stateID, stateName,govtLGDStateID FROM States WHERE deleted != true ")
public ArrayList<Object[]> getStateMaster();
}
5 changes: 3 additions & 2 deletions src/main/java/com/iemr/tm/service/location/LocationServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,8 @@ public String getDistrictList(Integer stateID) {
ArrayList<Object[]> districtMasterList = districtMasterRepo.getDistrictMaster(stateID);
if (districtMasterList != null && districtMasterList.size() > 0) {
for (Object[] objArr : districtMasterList) {
Districts districtMaster = new Districts((Integer) objArr[0], (String) objArr[1]);
Districts districtMaster = new Districts((Integer) objArr[0], (String) objArr[1], (Integer) objArr[2],
(Integer) objArr[3]);
Comment on lines +180 to +181
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add array bounds and null checking before accessing elements.

The code assumes the objArr array has at least 4 elements and that none are null. This could lead to ArrayIndexOutOfBoundsException or NullPointerException.

-    Districts districtMaster = new Districts((Integer) objArr[0], (String) objArr[1], (Integer) objArr[2],
-            (Integer) objArr[3]);
+    if (objArr.length < 4) {
+        throw new IllegalStateException("Insufficient data for district creation");
+    }
+    Integer districtId = (Integer) objArr[0];
+    String districtName = (String) objArr[1];
+    Integer govtLGDStateId = (Integer) objArr[2];
+    Integer govtLGDDistrictId = (Integer) objArr[3];
+    if (districtId == null || districtName == null || govtLGDStateId == null || govtLGDDistrictId == null) {
+        throw new IllegalStateException("Null values found in district data");
+    }
+    Districts districtMaster = new Districts(districtId, districtName, govtLGDStateId, govtLGDDistrictId);
πŸ“ 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.

Suggested change
Districts districtMaster = new Districts((Integer) objArr[0], (String) objArr[1], (Integer) objArr[2],
(Integer) objArr[3]);
if (objArr.length < 4) {
throw new IllegalStateException("Insufficient data for district creation");
}
Integer districtId = (Integer) objArr[0];
String districtName = (String) objArr[1];
Integer govtLGDStateId = (Integer) objArr[2];
Integer govtLGDDistrictId = (Integer) objArr[3];
if (districtId == null || districtName == null || govtLGDStateId == null || govtLGDDistrictId == null) {
throw new IllegalStateException("Null values found in district data");
}
Districts districtMaster = new Districts(districtId, districtName, govtLGDStateId, govtLGDDistrictId);

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 its mandatory to get those fields

Copy link
Contributor

Choose a reason for hiding this comment

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

@ravishanigarapu Understood, thank you for the clarification.

(_/)
( ._. )
/

✏️ Learnings added 
Learnt from: ravishanigarapu
PR: PSMRI/TM-API#58
File: src/main/java/com/iemr/tm/service/location/LocationServiceImpl.java:180-181
Timestamp: 2024-12-16T11:47:37.331Z
Learning: In `LocationServiceImpl.java`, within the `getDistrictList` method, it is guaranteed that `districtMasterList` will always contain arrays with at least four non-null elements (`objArr[0]` to `objArr[3]`), and it is mandatory to get those fields. Therefore, array bounds and null checks are not necessary in this context.

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.

districtList.add(districtMaster);
}
}
Expand Down Expand Up @@ -237,7 +238,7 @@ public String getLocDetailsNew(Integer vanID, Integer spPSMID) {
ArrayList<Object[]> stateMasterList = stateMasterRepo.getStateMaster();
if (stateMasterList != null && stateMasterList.size() > 0) {
for (Object[] objArr : stateMasterList) {
States states = new States((Integer) objArr[0], (String) objArr[1]);
States states = new States((Integer) objArr[0], (String) objArr[1], (Integer) objArr[2]);
stateList.add(states);
}
}
Expand Down
13 changes: 11 additions & 2 deletions src/main/java/com/iemr/tm/utils/http/HTTPRequestInterceptor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,15 +33,19 @@
import com.iemr.tm.utils.redis.RedisStorage;
import com.iemr.tm.utils.response.OutputResponse;
import com.iemr.tm.utils.sessionobject.SessionObject;
import com.iemr.tm.utils.validator.Validator;

import jakarta.servlet.http.HttpServletRequest;
import jakarta.servlet.http.HttpServletResponse;

@Component
public class HTTPRequestInterceptor implements HandlerInterceptor {

private Validator validator;
Logger logger = LoggerFactory.getLogger(this.getClass().getSimpleName());

@Autowired
public void setValidator(Validator validator) {
this.validator = validator;
}
private SessionObject sessionObject;

@Autowired
Expand Down Expand Up @@ -86,6 +90,11 @@ public boolean preHandle(HttpServletRequest request, HttpServletResponse respons
status = false;
break;
default:
String remoteAddress = request.getHeader("X-FORWARDED-FOR");
if (remoteAddress == null || remoteAddress.trim().length() == 0) {
remoteAddress = request.getRemoteAddr();
}
validator.checkKeyExists(authorization, remoteAddress);
Comment on lines +93 to +97
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add null check for validator before use.

The validator is used without checking if it's null, which could lead to a NullPointerException.

-    validator.checkKeyExists(authorization, remoteAddress);
+    if (validator == null) {
+        throw new IllegalStateException("Validator is not initialized");
+    }
+    validator.checkKeyExists(authorization, remoteAddress);
πŸ“ 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.

Suggested change
String remoteAddress = request.getHeader("X-FORWARDED-FOR");
if (remoteAddress == null || remoteAddress.trim().length() == 0) {
remoteAddress = request.getRemoteAddr();
}
validator.checkKeyExists(authorization, remoteAddress);
String remoteAddress = request.getHeader("X-FORWARDED-FOR");
if (remoteAddress == null || remoteAddress.trim().length() == 0) {
remoteAddress = request.getRemoteAddr();
}
if (validator == null) {
throw new IllegalStateException("Validator is not initialized");
}
validator.checkKeyExists(authorization, remoteAddress);

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 its mandatory thing

Copy link
Contributor

Choose a reason for hiding this comment

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

@ravishanigarapu, understood. If the validator is mandatory and always initialized, then a null check is not necessary.

(οΌ›οΈΏοΌ›)

✏️ Learnings added 
Learnt from: ravishanigarapu
PR: PSMRI/TM-API#58
File: src/main/java/com/iemr/tm/utils/http/HTTPRequestInterceptor.java:93-97
Timestamp: 2024-12-16T11:46:15.486Z
Learning: In the `HTTPRequestInterceptor` class (`src/main/java/com/iemr/tm/utils/http/HTTPRequestInterceptor.java`), the `validator` is mandatory and always initialized, so null checks are unnecessary.

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.

break;
}
} catch (Exception e) {
Expand Down
Loading