Modify actions pool which update last modification date#982
Modify actions pool which update last modification date#982
Conversation
📝 WalkthroughWalkthroughMultiple mutating controller endpoints now accept a ChangesService + Controller + Tests
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Controller as StudyController
participant Service as StudyService
participant Repo as Repository
participant Notification as NotificationService
Client->>Controller: HTTP mutating request (Header: userId)
Controller->>Service: method(..., userId)
activate Service
Service->>Repo: persist/update/delete
Repo-->>Service: result
Service->>Notification: emitStudyChanged(...details)
Service->>Notification: emitElementUpdated(studyUuid, userId)
deactivate Service
Service-->>Controller: 200 OK
Controller-->>Client: 200 OK
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/gridsuite/study/server/service/StudyService.java (1)
3000-3008:⚠️ Potential issue | 🟠 MajorDon’t update
lastModificationDatewhen event CRUD fails.
emitElementUpdated(studyUuid, userId)is infinally, so it fires even ifsaveEvent(...)/deleteEvents(...)throws and no change was applied. That will mark the study as modified on failed requests.Suggested fix
try { dynamicSimulationEventService.saveEvent(nodeUuid, event); + notificationService.emitElementUpdated(studyUuid, userId); } finally { notificationService.emitEndEventCrudNotification(studyUuid, nodeUuid, childrenUuids); - notificationService.emitElementUpdated(studyUuid, userId); }Apply the same move in
updateDynamicSimulationEvent(...)anddeleteDynamicSimulationEvents(...).Also applies to: 3013-3021, 3026-3034
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/gridsuite/study/server/service/StudyService.java` around lines 3000 - 3008, The code currently calls notificationService.emitElementUpdated(studyUuid, userId) inside the finally block so it runs even when dynamicSimulationEventService.saveEvent/updateEvent/deleteEvents throws; move the emitElementUpdated call so it only runs on successful completion (i.e., after saveEvent/updateEvent/deleteEvents returns without exception) while keeping emitEndEventCrudNotification in the finally block; apply this change in createDynamicSimulationEvent, updateDynamicSimulationEvent and deleteDynamicSimulationEvents, referencing notificationService.emitElementUpdated, notificationService.emitEndEventCrudNotification and the dynamicSimulationEventService methods (saveEvent/updateEvent/deleteEvents).
🧹 Nitpick comments (1)
src/test/java/org/gridsuite/study/server/rootnetworks/RootNetworkTest.java (1)
838-838: Add coverage for the new last-modified side effect.Line 838 updates the new
userIdparameter, but this test still only checks ordering. A small assertion that the delete flow emits the generic element-updated notification would keep the PR’s actual behavior covered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/gridsuite/study/server/rootnetworks/RootNetworkTest.java` at line 838, The test calls studyService.deleteRootNetworks(studyEntity.getId(), List.of(result.get(1).rootNetworkUuid()), USER_ID) but only asserts ordering; add an assertion to verify the new last-modified/user-id side effect by asserting that the delete flow emitted the generic element-updated notification (e.g., check the notifier/mock was called with an ElementUpdated event for the affected study or root network and that the payload includes the updated lastModified/lastModifiedBy or USER_ID); locate the verification near the existing assertions after the delete call and use the same notifier/mock used elsewhere in this test class to assert the expected event was emitted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/main/java/org/gridsuite/study/server/service/StudyService.java`:
- Around line 3000-3008: The code currently calls
notificationService.emitElementUpdated(studyUuid, userId) inside the finally
block so it runs even when
dynamicSimulationEventService.saveEvent/updateEvent/deleteEvents throws; move
the emitElementUpdated call so it only runs on successful completion (i.e.,
after saveEvent/updateEvent/deleteEvents returns without exception) while
keeping emitEndEventCrudNotification in the finally block; apply this change in
createDynamicSimulationEvent, updateDynamicSimulationEvent and
deleteDynamicSimulationEvents, referencing
notificationService.emitElementUpdated,
notificationService.emitEndEventCrudNotification and the
dynamicSimulationEventService methods (saveEvent/updateEvent/deleteEvents).
---
Nitpick comments:
In `@src/test/java/org/gridsuite/study/server/rootnetworks/RootNetworkTest.java`:
- Line 838: The test calls studyService.deleteRootNetworks(studyEntity.getId(),
List.of(result.get(1).rootNetworkUuid()), USER_ID) but only asserts ordering;
add an assertion to verify the new last-modified/user-id side effect by
asserting that the delete flow emitted the generic element-updated notification
(e.g., check the notifier/mock was called with an ElementUpdated event for the
affected study or root network and that the payload includes the updated
lastModified/lastModifiedBy or USER_ID); locate the verification near the
existing assertions after the delete call and use the same notifier/mock used
elsewhere in this test class to assert the expected event was emitted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 05ff2696-7603-4477-8b29-29e9846d20ec
📒 Files selected for processing (3)
src/main/java/org/gridsuite/study/server/controller/StudyController.javasrc/main/java/org/gridsuite/study/server/service/StudyService.javasrc/test/java/org/gridsuite/study/server/rootnetworks/RootNetworkTest.java
AbdelHedhili
left a comment
AbdelHedhili
left a comment
There was a problem hiding this comment.
Can you add / modify the tests to check for the newly emitted msg
…t-modification-date
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/gridsuite/study/server/service/StudyService.java (1)
3000-3036:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
emitElementUpdatedfires on failure when placed inside thefinallyblock.All three dynamic simulation event methods (
createDynamicSimulationEvent,updateDynamicSimulationEvent,deleteDynamicSimulationEvents) putemitElementUpdatedinside thefinallyblock. WhensaveEvent/deleteEventsthrows an exception, Spring's@Transactionalrolls back the DB change, but theemitElementUpdatednotification is still dispatched — telling consumers that thelastModificationDatewas updated even though nothing was actually persisted.The established pattern everywhere else in this class (e.g.,
createNetworkModificationline 1750,deleteNetworkModificationsline 2124,stashNetworkModificationsline 2141) placesemitElementUpdatedafter the try-finally block so it is only reached on success.🐛 Proposed fix – move `emitElementUpdated` outside the `finally` blocks
try { dynamicSimulationEventService.saveEvent(nodeUuid, event); } finally { notificationService.emitEndEventCrudNotification(studyUuid, nodeUuid, childrenUuids); - notificationService.emitElementUpdated(studyUuid, userId); } + notificationService.emitElementUpdated(studyUuid, userId); postProcessEventCrud(studyUuid, nodeUuid); }Apply the same change to
updateDynamicSimulationEvent(line 3019–3020) anddeleteDynamicSimulationEvents(line 3032–3033).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/gridsuite/study/server/service/StudyService.java` around lines 3000 - 3036, Three methods (createDynamicSimulationEvent, updateDynamicSimulationEvent, deleteDynamicSimulationEvents) call notificationService.emitElementUpdated from inside the finally block so the "element updated" notification fires even on transaction rollback; move the emitElementUpdated(studyUuid, userId) call out of each finally block and place it after the try-finally (so only executed on success), keeping notificationService.emitEndEventCrudNotification(studyUuid, nodeUuid, childrenUuids) inside the finally and leaving postProcessEventCrud(studyUuid, nodeUuid) in its current position.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/main/java/org/gridsuite/study/server/service/StudyService.java`:
- Around line 3000-3036: Three methods (createDynamicSimulationEvent,
updateDynamicSimulationEvent, deleteDynamicSimulationEvents) call
notificationService.emitElementUpdated from inside the finally block so the
"element updated" notification fires even on transaction rollback; move the
emitElementUpdated(studyUuid, userId) call out of each finally block and place
it after the try-finally (so only executed on success), keeping
notificationService.emitEndEventCrudNotification(studyUuid, nodeUuid,
childrenUuids) inside the finally and leaving postProcessEventCrud(studyUuid,
nodeUuid) in its current position.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ab75fd57-bccb-43ef-8a82-448ec0c1cc8c
📒 Files selected for processing (1)
src/main/java/org/gridsuite/study/server/service/StudyService.java
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/gridsuite/study/server/service/StudyService.java (1)
1013-1032:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid double
emitElementUpdatedon non-security rerun path.On Line 1031,
rerunLoadflow(...)emitsemitElementUpdated(...), but on the non-security branch it already goes throughhandleLoadflowRequest(...), which emits again on Line 1084. This causes duplicate notifications for a single action.Suggested fix
@@ public void rerunLoadflow(UUID studyUuid, UUID nodeUuid, UUID rootNetworkUuid, UUID loadflowResultUuid, Boolean withRatioTapChangers, String userId) { - notificationService.emitElementUpdated(studyEntity.getId(), userId);Also applies to: 1072-1085
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/gridsuite/study/server/service/StudyService.java` around lines 1013 - 1032, The method rerunLoadflow currently calls notificationService.emitElementUpdated(...) unconditionally but the non-security path already triggers the same notification inside handleLoadflowRequest(...), causing duplicate notifications; remove the redundant emitElementUpdated call from rerunLoadflow (or conditionally call it only for the security branch) so that emitElementUpdated is only invoked once per action — update rerunLoadflow accordingly and ensure behavior for both branches (security branch uses invalidateNodeTree/buildNode then emits, non-security branch relies on handleLoadflowRequest to emit).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/org/gridsuite/study/server/service/StudyService.java`:
- Around line 3010-3014: The emitElementUpdated call must not run on failures;
remove or move notificationService.emitElementUpdated(studyUuid, userId) out of
the finally block and invoke it only after the CRUD operation completes
successfully (i.e. after dynamicSimulationEventService.saveEvent / update /
delete returns without throwing), while keeping
notificationService.emitEndEventCrudNotification(studyUuid, nodeUuid,
childrenUuids) in the finally block; apply the same change for the other event
CRUD blocks that call dynamicSimulationEventService (the create/update/delete
branches referenced) so emitElementUpdated is only called on success.
---
Outside diff comments:
In `@src/main/java/org/gridsuite/study/server/service/StudyService.java`:
- Around line 1013-1032: The method rerunLoadflow currently calls
notificationService.emitElementUpdated(...) unconditionally but the non-security
path already triggers the same notification inside handleLoadflowRequest(...),
causing duplicate notifications; remove the redundant emitElementUpdated call
from rerunLoadflow (or conditionally call it only for the security branch) so
that emitElementUpdated is only invoked once per action — update rerunLoadflow
accordingly and ensure behavior for both branches (security branch uses
invalidateNodeTree/buildNode then emits, non-security branch relies on
handleLoadflowRequest to emit).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c017758e-196b-42a3-b5f8-c32bcf60f2ca
📒 Files selected for processing (3)
src/main/java/org/gridsuite/study/server/service/StudyService.javasrc/test/java/org/gridsuite/study/server/NetworkModificationTreeTest.javasrc/test/java/org/gridsuite/study/server/studycontroller/StudyTest.java
|
|
My bad, I think initially it didn't require test changes and I forgot to add them as the scope grew |
2 participants
PR Summary
Add lastModificationDate update for following actions :