Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new interface ModifyingProcessConfig with modificationUuids(), removes that method from ProcessConfig, updates SecurityAnalysisConfig to implement ModifyingProcessConfig, narrows ApplyModificationsStep generics to ModifyingProcessConfig, and updates tests to cover empty and non-empty modificationUuids cases. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 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🧪 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 |
KoloMenek
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/processes/commons/steps/ApplyModificationsStepTest.java (1)
27-27: Redundant explicit import ofMockito.The static wildcard import on line 35 (
import static org.mockito.Mockito.*) already covers all static methods. You can replaceMockito.mock(ProcessConfig.class)on line 89 with justmock(ProcessConfig.class)and drop this import.Proposed fix
-import org.mockito.Mockito;And on line 89:
- ProcessConfig processConfigMock = Mockito.mock(ProcessConfig.class); + ProcessConfig processConfigMock = mock(ProcessConfig.class);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/processes/commons/steps/ApplyModificationsStepTest.java` at line 27, Remove the redundant explicit import of org.mockito.Mockito and use the existing static import of org.mockito.Mockito.*; replace usages like Mockito.mock(ProcessConfig.class) in ApplyModificationsStepTest (e.g., the call creating the ProcessConfig mock) with the shorter mock(ProcessConfig.class) so the static import is used consistently and the explicit import can be deleted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/processes/commons/steps/ApplyModificationsStepTest.java`:
- Line 27: Remove the redundant explicit import of org.mockito.Mockito and use
the existing static import of org.mockito.Mockito.*; replace usages like
Mockito.mock(ProcessConfig.class) in ApplyModificationsStepTest (e.g., the call
creating the ProcessConfig mock) with the shorter mock(ProcessConfig.class) so
the static import is used consistently and the explicit import can be deleted.
22145fd to
9a1cc12
Compare
antoinebhs
reviewed
Feb 24, 2026
| @@ -0,0 +1,14 @@ | |||
| package org.gridsuite.monitor.commons; | |||
antoinebhs
reviewed
Feb 24, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/processes/commons/steps/ApplyModificationsStepTest.java`:
- Around line 85-95: Rename the test method to reflect the actual behavior under
test: change the method name
executeDoesNothingWhenConfigNotModifyingProcessConfig to
executeDoesNothingWhenModificationUuidsEmpty in ApplyModificationsStepTest;
ensure the test still creates a ModifyingProcessConfig mock, stubs
stepContext.getConfig(), calls applyModificationsStep.execute(stepContext), and
verifies no interactions with networkModificationService,
networkModificationRestService, and filterService, and update any references
(e.g., test runner or IDE-run configurations) to the new method name so the test
suite remains discoverable; this aligns the name with the guard in
ApplyModificationsStep.execute() that checks modificationUuids().
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/processes/commons/steps/ApplyModificationsStep.javamonitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/processes/commons/steps/ApplyModificationsStepTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/processes/commons/steps/ApplyModificationsStep.java
KoloMenek
force-pushed
the
marutk/refactor/make_process_config_generic
branch
from
169bcd9 to
b3e7fe2
Compare
Member
Author
|
Rebased and made requested changes. Let me know if the PR looks good to you ;) |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
monitor-commons/src/main/java/org/gridsuite/monitor/commons/ModifyingProcessConfig.java (1)
12-16: Nit: Remove the blank line between the Javadoc block and the interface declaration.Javadoc must immediately precede the element it documents; the blank line at line 15 breaks that association and is non-idiomatic Java.
♻️ Proposed fix
/** * `@author` Kamil MARUT {`@literal` <kamil.marut at rte-france.com>} */ - public interface ModifyingProcessConfig extends ProcessConfig {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor-commons/src/main/java/org/gridsuite/monitor/commons/ModifyingProcessConfig.java` around lines 12 - 16, Remove the blank line between the Javadoc block and the interface declaration so the Javadoc immediately precedes the ModifyingProcessConfig interface; place the /** ... */ comment directly above the "public interface ModifyingProcessConfig extends ProcessConfig {" line to restore proper Java doc association.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@monitor-commons/src/main/java/org/gridsuite/monitor/commons/ModifyingProcessConfig.java`:
- Around line 12-16: Remove the blank line between the Javadoc block and the
interface declaration so the Javadoc immediately precedes the
ModifyingProcessConfig interface; place the /** ... */ comment directly above
the "public interface ModifyingProcessConfig extends ProcessConfig {" line to
restore proper Java doc association.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
monitor-commons/src/main/java/org/gridsuite/monitor/commons/ModifyingProcessConfig.javamonitor-commons/src/main/java/org/gridsuite/monitor/commons/ProcessConfig.javamonitor-commons/src/main/java/org/gridsuite/monitor/commons/SecurityAnalysisConfig.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/processes/commons/steps/ApplyModificationsStep.javamonitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/processes/commons/steps/ApplyModificationsStepTest.java
💤 Files with no reviewable changes (1)
- monitor-commons/src/main/java/org/gridsuite/monitor/commons/ProcessConfig.java
🚧 Files skipped from review as they are similar to previous changes (2)
- monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/processes/commons/steps/ApplyModificationsStepTest.java
- monitor-commons/src/main/java/org/gridsuite/monitor/commons/SecurityAnalysisConfig.java
KoloMenek
force-pushed
the
marutk/refactor/make_process_config_generic
branch
from
b3e7fe2 to
b505604
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/processes/commons/steps/ApplyModificationsStepTest.java`:
- Around line 100-103: The test executeDoesNothingWhenModificationUuidsEmpty is
missing an assertion that S3 is untouched; add a no-interaction verification for
the s3Service mock (e.g., verifyNoInteractions(s3Service) or equivalent)
alongside the existing verifyNoInteractions(networkModificationService),
verifyNoInteractions(networkModificationRestService), and
verifyNoInteractions(filterService) to ensure the test fully asserts there are
no S3 side effects.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
monitor-commons/src/main/java/org/gridsuite/monitor/commons/ModifyingProcessConfig.javamonitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/processes/commons/steps/ApplyModificationsStepTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- monitor-commons/src/main/java/org/gridsuite/monitor/commons/ModifyingProcessConfig.java
Comment on lines
+100
to
+103
| verifyNoInteractions(networkModificationService); | ||
| verifyNoInteractions(networkModificationRestService); | ||
| verifyNoInteractions(filterService); | ||
| } |
There was a problem hiding this comment.
executeDoesNothingWhenModificationUuidsEmpty should also assert no S3 side effects.
The test says “does nothing”, but it currently does not guard s3Service. Add a no-interaction assertion to fully lock the contract.
Suggested patch
verifyNoInteractions(networkModificationService);
verifyNoInteractions(networkModificationRestService);
verifyNoInteractions(filterService);
+ verifyNoInteractions(s3Service);📝 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
| verifyNoInteractions(networkModificationService); | |
| verifyNoInteractions(networkModificationRestService); | |
| verifyNoInteractions(filterService); | |
| } | |
| verifyNoInteractions(networkModificationService); | |
| verifyNoInteractions(networkModificationRestService); | |
| verifyNoInteractions(filterService); | |
| verifyNoInteractions(s3Service); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/processes/commons/steps/ApplyModificationsStepTest.java`
around lines 100 - 103, The test executeDoesNothingWhenModificationUuidsEmpty is
missing an assertion that S3 is untouched; add a no-interaction verification for
the s3Service mock (e.g., verifyNoInteractions(s3Service) or equivalent)
alongside the existing verifyNoInteractions(networkModificationService),
verifyNoInteractions(networkModificationRestService), and
verifyNoInteractions(filterService) to ensure the test fully asserts there are
no S3 side effects.
b505604 to
44ee6df
Compare
…ssConfig interface Moved modificationUuids from ProcessConfig to ModifyingProcessConfig interface in order to make the ProcessConfig interface more generic. It is required for further developments related to SnapshotRefinerServer's connection to monitor-server Signed-off-by: Kamil MARUT <kamil.marut@rte-france.com>
KoloMenek
force-pushed
the
marutk/refactor/make_process_config_generic
branch
from
44ee6df to
8989ebc
Compare
|
This was referenced Feb 26, 2026
Closed
benrejebmoh
pushed a commit
that referenced
this pull request
Mar 4, 2026
…ssConfig interface (#43) Moved modificationUuids from ProcessConfig to ModifyingProcessConfig interface in order to make the ProcessConfig interface more generic. It is required for further developments related to SnapshotRefinerServer's connection to monitor-server Signed-off-by: Kamil MARUT <kamil.marut@rte-france.com>
This was referenced Mar 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Summary
Moved modificationUuids from ProcessConfig to ModifyingProcessConfig interface in order to make the ProcessConfig interface more generic. It is required for further developments related to SnapshotRefinerServer's connection to monitor-server