created dto for before stock taking report - #17726#18216
created dto for before stock taking report - #17726#18216RaveeshaPeiris wants to merge 1 commit intodevelopmentfrom
Conversation
WalkthroughThis pull request introduces a DTO-based approach for stock-taking reports in the pharmacy module. A new BeforeStockTakingDTO class is created, a corresponding retrieval method is added to StockFacade, and PharmacyController is refactored to use DTO projections via JPQL queries instead of loading full Stock entities, with conversion logic for backward compatibility. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Client/UI
participant PC as PharmacyController
participant SF as StockFacade
participant DB as Database (JPA)
UI->>PC: Request before stock-taking report<br/>(with filters: dept, category, qty)
PC->>PC: Build DTO-projection JPQL query<br/>with filters and parameters
PC->>SF: findBeforeStockTakingReport(sql, parameters)
SF->>DB: Execute JPA Query<br/>(bind parameters)
DB-->>SF: List<BeforeStockTakingDTO>
SF-->>PC: DTO list returned
PC->>PC: Convert DTOs to Stock objects<br/>(convertDtosToStocks)
PC->>UI: Return Stock list for UI display
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.40.5)src/main/java/com/divudi/bean/pharmacy/PharmacyController.javaThanks 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/main/java/com/divudi/core/facade/StockFacade.java`:
- Around line 37-45: Rename the method parameter `sql` to `jpql` for clarity and
update usages in findBeforeStockTakingReport; also add null-safety for
`parameters` by treating a null map as empty before iterating (e.g., check if
parameters is null and skip the loop or initialize an empty map), and continue
to set parameters on the Query created via em.createQuery(jpql) before returning
query.getResultList(); ensure you update the method signature and all internal
references (findBeforeStockTakingReport, em.createQuery, parameters)
accordingly.
🧹 Nitpick comments (2)
src/main/java/com/divudi/core/data/dto/BeforeStockTakingDTO.java (1)
28-55: Consider adding a no-arg constructor for serialization compatibility.While the full-argument constructor works well for JPQL constructor expressions, some serialization frameworks (e.g., Jackson, certain JAX-RS providers) require a no-arg constructor. If this DTO may be serialized/deserialized in the future, adding one would improve compatibility.
♻️ Suggested improvement
+ public BeforeStockTakingDTO() { + } + public BeforeStockTakingDTO( Long id, String itemCode,src/main/java/com/divudi/core/facade/StockFacade.java (1)
27-31: Empty if-block appears to be dead code.The
if (em == null) {}block does nothing. If this was intended to throw an exception or initialize something, the implementation is missing. If it's not needed, consider removing it.♻️ Suggested fix
`@Override` protected EntityManager getEntityManager() { - if (em == null) { - } return em; }
| public List<BeforeStockTakingDTO> findBeforeStockTakingReport(String sql, Map<String, Object> parameters) { | ||
| Query query = em.createQuery(sql); | ||
|
|
||
| for (Map.Entry<String, Object> entry : parameters.entrySet()) { | ||
| query.setParameter(entry.getKey(), entry.getValue()); | ||
| } | ||
|
|
||
| return query.getResultList(); | ||
| } |
There was a problem hiding this comment.
Add null safety for parameters and consider renaming sql to jpql.
Two observations:
- NullPointerException risk: If
parametersis null, the for-each loop will throw NPE. - Misleading parameter name:
em.createQuery()expects JPQL, not SQL. Naming the parameterjpqlwould be clearer.
🔧 Suggested fix
- public List<BeforeStockTakingDTO> findBeforeStockTakingReport(String sql, Map<String, Object> parameters) {
- Query query = em.createQuery(sql);
-
- for (Map.Entry<String, Object> entry : parameters.entrySet()) {
- query.setParameter(entry.getKey(), entry.getValue());
+ public List<BeforeStockTakingDTO> findBeforeStockTakingReport(String jpql, Map<String, Object> parameters) {
+ Query query = em.createQuery(jpql);
+
+ if (parameters != null) {
+ for (Map.Entry<String, Object> entry : parameters.entrySet()) {
+ query.setParameter(entry.getKey(), entry.getValue());
+ }
}
return query.getResultList();
}🤖 Prompt for AI Agents
In `@src/main/java/com/divudi/core/facade/StockFacade.java` around lines 37 - 45,
Rename the method parameter `sql` to `jpql` for clarity and update usages in
findBeforeStockTakingReport; also add null-safety for `parameters` by treating a
null map as empty before iterating (e.g., check if parameters is null and skip
the loop or initialize an empty map), and continue to set parameters on the
Query created via em.createQuery(jpql) before returning query.getResultList();
ensure you update the method signature and all internal references
(findBeforeStockTakingReport, em.createQuery, parameters) accordingly.
buddhika75
left a comment
buddhika75
left a comment
There was a problem hiding this comment.
PR Review: Before Stock Taking Report DTO Implementation
Overall Assessment
The approach of using DTO projection for the stock taking report is good for performance. However, there are several issues that need to be addressed before this can be merged.
🔴 Critical Issues
1. Variable Shadowing Bug (PharmacyController.java, line ~1734)
Department dept = new Department(); // Shadows class field 'dept'This shadows the class field dept which is used as a filter parameter. Use a different variable name like department or deptEntity.
2. Excessive Formatting Changes
The PR includes ~1500+ lines of formatting changes (4-space to 8-space indentation changes) that are unrelated to the feature. This:
- Makes the PR nearly impossible to review properly
- Pollutes git history/blame
- Could introduce unintended side effects
Please create a separate commit or PR for formatting changes, or revert them from this PR.
🟡 Medium Issues
3. Null Safety Missing (convertDtosToStocks method)
private List<Stock> convertDtosToStocks(List<BeforeStockTakingDTO> dtos) {
List<Stock> stocks = new ArrayList<>();
for (BeforeStockTakingDTO dto : dtos) { // No null check for dtosAdd null check: if (dtos == null) return stocks;
4. Potential Entity/Lazy Loading Issues
Creating detached entity objects (Item, Category, Department) and setting them on Stock may cause issues if the UI tries to access properties that weren't loaded in the DTO. The entities created are just shells with IDs and names - if any other property is accessed, it will be null instead of lazy-loading.
Consider either:
- Using the DTOs directly in the frontend
- Or ensuring all needed properties are included in the DTO
🟢 Minor Suggestions
5. StockFacade Method Could Be More Generic
The findBeforeStockTakingReport method is very specific. Consider renaming to something more generic or using the existing findByJpql pattern with proper typing.
6. DTO Could Benefit from a No-Args Constructor
For serialization compatibility, consider adding a no-args constructor to BeforeStockTakingDTO.
Summary
- ✅ Good: Using DTO projection for performance
- ✅ Good: Proper JPQL query with joins
- ❌ Fix: Variable shadowing bug
- ❌ Fix: Remove or separate formatting changes
⚠️ Consider: Null safety and lazy loading concerns
Please address the critical issues and resubmit. TIA!
3 participants
Summary by CodeRabbit
New Features
Performance
✏️ Tip: You can customize this high-level summary in your review settings.