Feature/incentive for supporting doc#108
Conversation
WalkthroughThe pull request downgrades the Maven version and disables Swagger/OpenAPI endpoints across environment configurations. It introduces new incentive update functionality through a PendingActivity workflow, adding corresponding entities, DTOs, repositories, and service methods that support updating meetings when triggered via incentive updates with image handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant IncentiveController
participant IncentiveService
participant IncentivePendingActivityRepo
participant MaaMeetingService
participant Database
Client->>IncentiveController: POST /incentive/update<br/>(PendingActivityDTO)
IncentiveController->>IncentiveService: updateIncentive(pendingActivityDTO)
IncentiveService->>IncentivePendingActivityRepo: findByUserIdAndModuleNameActivityId<br/>(userId, moduleName, activityId)
IncentivePendingActivityRepo->>Database: Query incentive_pending_activity
Database-->>IncentivePendingActivityRepo: IncentivePendingActivity record
IncentivePendingActivityRepo-->>IncentiveService: Optional<IncentivePendingActivity>
alt Module is MAA_MEETING & Images exist
IncentiveService->>IncentiveService: Convert Images to Base64
IncentiveService->>MaaMeetingService: updateMeeting(MaaMeetingRequestDTO)
MaaMeetingService->>Database: Fetch existing MaaMeeting by id
Database-->>MaaMeetingService: MaaMeeting entity
MaaMeetingService->>Database: Update MaaMeeting with new data
Database-->>MaaMeetingService: Success
MaaMeetingService-->>IncentiveService: Updated MaaMeeting
else Error
IncentiveService-->>IncentiveService: Catch exception & store message
end
IncentiveService-->>IncentiveController: Error message or null
IncentiveController-->>Client: Response (200 OK or error)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (7)
src/main/environment/common_docker.properties (1)
18-19: Consistent Swagger disabling across environments.Removal of Swagger properties aligns with the pattern in other environment configs. The system will now rely on defaults from
application.properties(where Swagger is explicitly disabled). Minor: Using a single blank line per removed property may be more idiomatic, but this is not a functional issue.src/main/java/com/iemr/flw/domain/iemr/IncentiveActivityRecord.java (1)
50-54: Align DB schema with new eligibility/default flagsThe new
is_eligibleandis_default_activitycolumns onincentive_activity_recordlook fine, but make sure the database migration adds these columns with the expected nullability/defaults so JPA mapping and existing records don’t break.src/main/java/com/iemr/flw/controller/MaaMeetingController.java (1)
36-60: Redundantdto != nullcheck insaveMeeting
dtois always non-null because it is instantiated just above, so:if (dto != null) { service.saveMeeting(dto); }can be simplified to a direct
service.saveMeeting(dto);call inside thetryblock.src/main/java/com/iemr/flw/service/impl/IncentiveServiceImpl.java (2)
203-203: Extract hardcoded module name to a constant.The string
"MAA_MEETING"should be extracted to a constant (or use an enum) to avoid typos and improve maintainability, similar to howGroupNameenum is used elsewhere in this codebase.+private static final String MODULE_MAA_MEETING = "MAA_MEETING"; + // In updateIncentive method: -if(existingIncentivePendingActivity.getModuleName().equals("MAA_MEETING")){ +if(existingIncentivePendingActivity.getModuleName().equals(MODULE_MAA_MEETING)){
197-219: Clarify return value semantics for better API design.The method returns
nullin three different scenarios:
- Pending activity not found (line 217)
- Module name doesn't match "MAA_MEETING" (line 217)
- Success (implicitly, after update completes)
Consider returning distinct values or throwing appropriate exceptions to help callers distinguish between "success", "not found", and "not applicable" states.
src/main/java/com/iemr/flw/domain/iemr/IncentivePendingActivity.java (2)
34-38: Usejava.sql.Timestampfor date fields to maintain consistency.This entity uses
java.util.DateforcreatedDateandupdatedDate, but other entities in the codebase (e.g.,IncentiveActivityRecord,OrsDistribution) usejava.sql.Timestamp. Consider usingTimestampfor consistency and better precision.-import java.util.Date; +import java.sql.Timestamp; @Column(name = "created_date") -private Date createdDate; +private Timestamp createdDate; @Column(name = "updated_date") -private Date updatedDate; +private Timestamp updatedDate;
15-39: Consider adding audit fields for traceability.Other related entities like
IncentiveActivityRecordincludecreatedByandupdatedByfields. Adding these would improve traceability of who created/modified pending activity records.Additionally, consider adding
@PrePersistand@PreUpdatecallbacks to auto-populate the date fields:+@Column(name = "created_by") +private String createdBy; + +@Column(name = "updated_by") +private String updatedBy; + +@PrePersist +protected void onCreate() { + createdDate = new Timestamp(System.currentTimeMillis()); +} + +@PreUpdate +protected void onUpdate() { + updatedDate = new Timestamp(System.currentTimeMillis()); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
pom.xml(1 hunks)src/main/environment/common_ci.properties(1 hunks)src/main/environment/common_docker.properties(1 hunks)src/main/java/com/iemr/flw/controller/IncentiveController.java(6 hunks)src/main/java/com/iemr/flw/controller/MaaMeetingController.java(1 hunks)src/main/java/com/iemr/flw/domain/iemr/IncentiveActivityRecord.java(1 hunks)src/main/java/com/iemr/flw/domain/iemr/IncentivePendingActivity.java(1 hunks)src/main/java/com/iemr/flw/dto/iemr/MaaMeetingRequestDTO.java(1 hunks)src/main/java/com/iemr/flw/dto/iemr/PendingActivityDTO.java(1 hunks)src/main/java/com/iemr/flw/repo/iemr/IncentivePendingActivityRepository.java(1 hunks)src/main/java/com/iemr/flw/service/IncentiveService.java(2 hunks)src/main/java/com/iemr/flw/service/MaaMeetingService.java(5 hunks)src/main/java/com/iemr/flw/service/impl/IncentiveServiceImpl.java(3 hunks)src/main/resources/application.properties(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/main/java/com/iemr/flw/dto/iemr/PendingActivityDTO.java (1)
src/main/java/com/iemr/flw/dto/iemr/MaaMeetingRequestDTO.java (1)
Data(9-18)
src/main/java/com/iemr/flw/service/MaaMeetingService.java (1)
src/main/java/com/iemr/flw/service/impl/IncentiveServiceImpl.java (1)
Service(33-271)
src/main/java/com/iemr/flw/service/impl/IncentiveServiceImpl.java (1)
src/main/java/com/iemr/flw/service/MaaMeetingService.java (1)
Service(22-231)
src/main/java/com/iemr/flw/domain/iemr/IncentivePendingActivity.java (4)
src/main/java/com/iemr/flw/dto/iemr/MaaMeetingRequestDTO.java (1)
Data(9-18)src/main/java/com/iemr/flw/dto/iemr/PendingActivityDTO.java (1)
Data(8-14)src/main/java/com/iemr/flw/domain/iemr/IncentiveActivityRecord.java (1)
Entity(8-55)src/main/java/com/iemr/flw/domain/iemr/OrsDistribution.java (1)
Table(9-42)
🔇 Additional comments (14)
src/main/environment/common_ci.properties (1)
19-19: Consistent with broader Swagger disabling pattern.This change aligns with the Swagger disabling pattern in
application.propertiesandcommon_docker.properties. However, the business rationale for disabling API documentation across all environments remains unclear (see the comment onapplication.propertieslines 66–67 for context).pom.xml (1)
9-9: Confirm version 3.7.0 matches the target branch intent.Line 9 sets the version to 3.7.0, which appears inconsistent with the target branch name
sm/release-3.10.0_temp. Verify whether this version is intentional for this release or if it should align with the 3.10.0 branch context.src/main/resources/application.properties (1)
66-67: Clarify the rationale for disabling API documentation.Lines 66–67 disable SpringDoc API docs and Swagger UI generation. If this change accompanies new endpoint additions, it creates potential gaps in API reference material. Clarify whether this is a temporary security measure, a configuration oversight, a permanent choice, or environment-specific (e.g., production-only). If permanent, confirm that alternative documentation is in place.
src/main/java/com/iemr/flw/dto/iemr/MaaMeetingRequestDTO.java (1)
9-18:idfield addition is consistent with update requirementsAdding
idtoMaaMeetingRequestDTOis appropriate for update flows and aligns withMaaMeetingService.updateMeetingusage.src/main/java/com/iemr/flw/service/IncentiveService.java (1)
6-19: Interface extension for PendingActivity-based updates looks consistentAdding
updateIncentive(PendingActivityDTO pendingActivityDTO)cleanly exposes the new workflow and matches the controller usage.src/main/java/com/iemr/flw/controller/IncentiveController.java (2)
81-104: Enhanced error handling ingetAllIncentivesByUserIdlooks goodWrapping
getAllIncentivesByUserIdin a try/catch with null checks and logging, and usingOutputResponseconsistently, improves robustness without changing the contract.
106-127: Due to inability to access the repository, I cannot fully verify the specific claims about the class-level configuration or the exact type of theImagesfield inPendingActivityDTO. The technical concern about bindingMultipartFilefrom JSON is architecturally sound, but definitive verification of the implementation details is not possible at this time.src/main/java/com/iemr/flw/service/MaaMeetingService.java (2)
165-176:updateIncentiveincorrectly creates a new record instead of updating the existing oneIn
updateIncentive, the code finds an existing record but then immediately discards it and creates a new instance:IncentiveActivityRecord record = recordRepo .findRecordByActivityIdCreatedDateBenId(...); if (record != null) { record = new IncentiveActivityRecord(); record.setIsEligible(true); recordRepo.save(record); }This creates corrupt/invalid rows with unset fields and loses the linkage to the original activity/date/asha. Update the existing record instead:
- if (record!= null) { - record = new IncentiveActivityRecord(); - record.setIsEligible(true); - recordRepo.save(record); - } + if (record != null) { + record.setIsEligible(true); + // Optionally also update audit fields here (updatedDate/updatedBy) if needed. + recordRepo.save(record); + }Also consider marking the corresponding
IncentivePendingActivityas resolved when eligibility becomes true.Also applies to: 218-228
154-164: Remove redundant null check inupdatePendingActivityThe method instantiates
IncentivePendingActivityand immediately checksif(incentivePendingActivity!=null). This null check is always true and should be removed—the object was just created on the previous line.private void updatePendingActivity(Integer userId, Long recordId, Long activityId, String moduleName) { IncentivePendingActivity incentivePendingActivity = new IncentivePendingActivity(); incentivePendingActivity.setActivityId(activityId); incentivePendingActivity.setRecordId(recordId); incentivePendingActivity.setUserId(userId); incentivePendingActivity.setModuleName(moduleName); - if(incentivePendingActivity!=null){ - incentivePendingActivityRepository.save(incentivePendingActivity); - } + incentivePendingActivityRepository.save(incentivePendingActivity); }The claims regarding null IDs from unsaved entities and parameter swaps require verification against the actual calling code in the
addIncentivemethod, which could not be accessed for confirmation.src/main/java/com/iemr/flw/repo/iemr/IncentivePendingActivityRepository.java (1)
9-14: Spring Data derived query method name is invalid and will cause a startup errorThe method
findByUserIdAndModuleNameActivityIdwill be parsed by Spring Data as propertiesuserIdandmoduleNameActivityId. The missingAndseparator betweenmoduleNameandactivityIdviolates Spring Data JPA naming conventions.Correct the method name to:
Optional<IncentivePendingActivity> findByUserIdAndModuleNameAndActivityId(Integer userId, String moduleName, Long activityId);Update all call sites accordingly.
src/main/java/com/iemr/flw/controller/MaaMeetingController.java (1)
62-104:/updateendpoint missing id parameter; service update will fail without itThe
/maa-meetings/updateendpoint builds aMaaMeetingRequestDTObut does not set itsid, whileMaaMeetingService.updateMeetingexpects the id to locate the existing meeting. Any call through this controller will fail.You should:
- Accept an id from the client (e.g., as
@RequestPart("id") String idor@PathVariable Long id), and- Set it on the DTO before calling the service:
- public ResponseEntity<?> updateMeeting( + public ResponseEntity<?> updateMeeting( + @RequestPart("id") String id, @RequestPart("meetingDate") String meetingDate, ... ) { try { MaaMeetingRequestDTO dto = new MaaMeetingRequestDTO(); + dto.setId(Long.parseLong(id)); ...Additionally, consider marking optional parts as
required = falseif partial updates are intended, and update the response message to "Updated Successfully" for clarity.src/main/java/com/iemr/flw/dto/iemr/PendingActivityDTO.java (1)
1-14: MultipartFile in JSON DTO + capitalizedImagesfield are problematic
- Field name
Imagesstarting with a capital is non-idiomatic and will expose a JSON propertyImagesrather thanimages; consider renaming toimagesfor consistency.- More importantly, this DTO is used as an
@RequestBodyinIncentiveController.updateIncentivewhile the controller-level mapping declaresconsumes = "application/json". Spring MVC cannot deserializeList<MultipartFile>from JSON; file uploads requiremultipart/form-datawith@RequestPartparameters instead of@RequestBody.Either:
- Change this DTO to carry JSON-friendly representations (e.g.,
List<String> imagesBase64), or- Change the
/incentive/updateendpoint toconsumes = MediaType.MULTIPART_FORM_DATA_VALUEand use@RequestPartfor the file list.src/main/java/com/iemr/flw/service/impl/IncentiveServiceImpl.java (1)
9-9: LGTM on imports and dependency wiring.The new imports and autowired dependencies for
IncentivePendingActivity,IncentivePendingActivityRepository,MaaMeetingService, andMultipartFileare correctly added to support the newupdateIncentivefunctionality.Also applies to: 15-17, 24-24, 53-60
src/main/java/com/iemr/flw/domain/iemr/IncentivePendingActivity.java (1)
10-32: Entity structure looks good.The JPA entity is well-structured with proper annotations, Lombok for boilerplate reduction, explicit column mappings with appropriate constraints (
nullable = falseon required fields), and correct primary key generation strategy.
| MaaMeetingRequestDTO maaMeetingRequestDTO = new MaaMeetingRequestDTO(); | ||
| maaMeetingRequestDTO.setMeetingImages(pendingActivityDTO.getImages().toArray(new MultipartFile[0])); | ||
| maaMeetingService.updateMeeting(maaMeetingRequestDTO); |
There was a problem hiding this comment.
Critical: Meeting ID is never set, causing updateMeeting to fail.
The MaaMeetingRequestDTO is created with only meetingImages set, but id is never populated. Looking at MaaMeetingService.updateMeeting(), it immediately calls repository.findById(req.getId()), which will throw EntityNotFoundException because req.getId() is null.
The existingIncentivePendingActivity.getRecordId() likely should be used as the meeting ID.
Apply this diff to fix the missing meeting ID:
MaaMeetingRequestDTO maaMeetingRequestDTO = new MaaMeetingRequestDTO();
+maaMeetingRequestDTO.setId(existingIncentivePendingActivity.getRecordId());
maaMeetingRequestDTO.setMeetingImages(pendingActivityDTO.getImages().toArray(new MultipartFile[0]));
maaMeetingService.updateMeeting(maaMeetingRequestDTO);📝 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.
| MaaMeetingRequestDTO maaMeetingRequestDTO = new MaaMeetingRequestDTO(); | |
| maaMeetingRequestDTO.setMeetingImages(pendingActivityDTO.getImages().toArray(new MultipartFile[0])); | |
| maaMeetingService.updateMeeting(maaMeetingRequestDTO); | |
| MaaMeetingRequestDTO maaMeetingRequestDTO = new MaaMeetingRequestDTO(); | |
| maaMeetingRequestDTO.setId(existingIncentivePendingActivity.getRecordId()); | |
| maaMeetingRequestDTO.setMeetingImages(pendingActivityDTO.getImages().toArray(new MultipartFile[0])); | |
| maaMeetingService.updateMeeting(maaMeetingRequestDTO); |
🤖 Prompt for AI Agents
In src/main/java/com/iemr/flw/service/impl/IncentiveServiceImpl.java around
lines 206 to 208, the MaaMeetingRequestDTO is created with meetingImages but its
id is never set, causing MaaMeetingService.updateMeeting() to call
repository.findById(null) and throw EntityNotFoundException; set the DTO id
using existingIncentivePendingActivity.getRecordId() (or the appropriate
recordId getter on the current scope) before calling updateMeeting so
updateMeeting receives a valid meeting id and can find the entity.
| public MaaMeeting updateMeeting(MaaMeetingRequestDTO req) throws JsonProcessingException { | ||
| MaaMeeting existingMeeting = repository.findById(req.getId()) | ||
| .orElseThrow(() -> new EntityNotFoundException("Meeting not found: " + req.getId())); | ||
|
|
||
| // ✅ NULL CHECK | ||
| if (req.getMeetingDate() != null) { | ||
| existingMeeting.setMeetingDate(req.getMeetingDate()); | ||
| } | ||
| if (req.getPlace() != null) { | ||
| existingMeeting.setPlace(req.getPlace()); | ||
| } | ||
| if (req.getParticipants() != null) { | ||
| existingMeeting.setParticipants(req.getParticipants()); | ||
| } | ||
| if (req.getAshaId() != null) { | ||
| existingMeeting.setAshaId(req.getAshaId()); | ||
| } | ||
| if (req.getCreatedBY() != null) { // ✅ Typo fixed: CreatedBY → CreatedBy | ||
| existingMeeting.setCreatedBy(req.getCreatedBY()); | ||
| } | ||
|
|
||
| // Images - only if provided | ||
| if (req.getMeetingImages() != null && req.getMeetingImages().length > 0) { | ||
| List<String> base64Images = Arrays.stream(req.getMeetingImages()) | ||
| .filter(file -> file != null && !file.isEmpty()) | ||
| .map(this::convertToBase64) | ||
| .collect(Collectors.toList()); | ||
| existingMeeting.setMeetingImagesJson(objectMapper.writeValueAsString(base64Images)); | ||
| } | ||
|
|
||
| checkAndAddIncentive(existingMeeting); | ||
| if(existingMeeting.getMeetingImagesJson()!=null){ | ||
| checkAndUpdateIncentive(existingMeeting); | ||
|
|
||
| } | ||
| return repository.save(existingMeeting); | ||
| } |
There was a problem hiding this comment.
updateMeeting requires an id that the controller currently never sets
The partial-update logic and base64 handling here are fine, but updateMeeting calls:
MaaMeeting existingMeeting = repository.findById(req.getId())
.orElseThrow(() -> new EntityNotFoundException("Meeting not found: " + req.getId()));If req.getId() is null, this will fail before any update occurs. As implemented, MaaMeetingController.updateMeeting never sets id on MaaMeetingRequestDTO, so calls coming from that endpoint cannot succeed.
Consider one of:
- Accepting an
idin the controller (e.g.,@RequestPart("id")or@PathVariable Long id) and settingdto.setId(id)before invoking this service, or - Changing the service signature to take the id explicitly and updating callers accordingly.
Also applies to: 113-119
1 participant
📋 Description
JIRA ID: AMM-1994
In incentive screen option to attach activity missing supporting documents.
✅ 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
Chores
New Features
✏️ Tip: You can customize this high-level summary in your review settings.