Add a modification reference as a new modification type#187
Open
SlimaneAmar wants to merge 9 commits into
Open
Add a modification reference as a new modification type#187SlimaneAmar wants to merge 9 commits into
SlimaneAmar wants to merge 9 commits into
Conversation
This comment was marked as low quality.
This comment was marked as low quality.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/org/gridsuite/modification/dto/ModificationReferenceInfos.java`:
- Around line 55-63: ModificationReferenceInfos must validate its required
payloads before delegating: add non-null checks for referenceInfos and
referenceType in the DTO (e.g., at the start of toModification() and
createSubReportNode()) and throw a clear runtime exception
(IllegalStateException or Objects.requireNonNull with message) instead of
silently mapping null referenceType to DIRECTORY; then use the validated
referenceType to select the template (Type.SAMPLE ->
"network.modification.sample.reference.apply", otherwise
"network.modification.directory.reference.apply") and proceed to construct the
ModificationReference and ReportNode as before.
In
`@src/main/java/org/gridsuite/modification/modifications/ModificationReference.java`:
- Around line 24-25: ModificationReference is delegating to other modifications
but never stores the application context services, so filterService and
loadFlowService remain null; update ModificationReference's
initApplicationContext(...) to assign the passed IFilterService and
ILoadFlowService to the protected fields (filterService, loadFlowService) and
ensure any delegation path (the code that forwards to the wrapped/delegated
modification around the current forwarding at ~line 40) calls the delegated
modification's initApplicationContext(...) with the same services so the
delegate receives the captured context.
- Around line 45-46: The getName method in class ModificationReference returns
the misspelled string "ModificationRefence"; update the return value in the
getName() method to the correct "ModificationReference" so the method signature
and returned name match the class and other references.
In `@src/main/java/org/gridsuite/modification/ModificationType.java`:
- Line 14: The TabularBaseInfos formatting falls back to "equipments of unknown
type" for the newly added enum value ModificationType.MODIFICATION_REFERENCE;
update the equipment-type mapping in
src/main/java/org/gridsuite/modification/dto/tabular/TabularBaseInfos (the
method that maps ModificationType to display labels) to add a case for
MODIFICATION_REFERENCE and return the appropriate user-facing label (consistent
with other mappings) instead of the default unknown-type string so the new enum
renders correctly in tabular views.
In
`@src/test/java/org/gridsuite/modification/modifications/ModificationReferenceTest.java`:
- Around line 57-58: The testCreationModificationMessage currently calls
assertNotNull with ModificationType.MODIFICATION_REFERENCE.name() and
modificationInfos.getMessageType(), which doesn’t verify the actual value;
replace that assertion with
assertEquals(ModificationType.MODIFICATION_REFERENCE.name(),
modificationInfos.getMessageType()) in the testCreationModificationMessage
method so the test asserts the message type is exactly the expected value.
🪄 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: 14c0c97c-6d2a-46d6-a9b5-17df814d1948
📒 Files selected for processing (5)
src/main/java/org/gridsuite/modification/ModificationType.javasrc/main/java/org/gridsuite/modification/dto/ModificationInfos.javasrc/main/java/org/gridsuite/modification/dto/ModificationReferenceInfos.javasrc/main/java/org/gridsuite/modification/modifications/ModificationReference.javasrc/test/java/org/gridsuite/modification/modifications/ModificationReferenceTest.java
SlimaneAmar
force-pushed
the
shared-modification
branch
3 times, most recently
from
5b58389 to
244a439
Compare
SlimaneAmar
force-pushed
the
shared-modification
branch
from
244a439 to
c19317b
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/main/java/org/gridsuite/modification/dto/ModificationReferenceInfos.java (1)
62-64:⚠️ Potential issue | 🟠 MajorAvoid silent fallback to DIRECTORY when report template type is invalid/unvalidated.
Line 63 maps any non-
SAMPLEvalue (includingnull) to DIRECTORY. Please validate first (or callcheck()), then use an exhaustiveswitchto prevent accidental misreporting.Proposed fix
`@Override` public ReportNode createSubReportNode(ReportNode reportNode) { - return reportNode.newReportNode().withMessageTemplate(referenceType == Type.SAMPLE ? "network.modification.sample.reference.apply" : "network.modification.directory.reference.apply").add(); + check(); + String template = switch (referenceType) { + case SAMPLE -> "network.modification.sample.reference.apply"; + case DIRECTORY -> "network.modification.directory.reference.apply"; + }; + return reportNode.newReportNode().withMessageTemplate(template).add(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/gridsuite/modification/dto/ModificationReferenceInfos.java` around lines 62 - 64, createSubReportNode silently treats any non-SAMPLE referenceType (including null) as DIRECTORY; first validate referenceType (call or inline the existing check() or throw IllegalStateException on null/invalid) and then replace the ternary with an exhaustive switch on referenceType (switch over Type enum with cases SAMPLE and DIRECTORY and a default that throws) to avoid accidental fallback/misreporting; update createSubReportNode to perform validation before constructing the ReportNode and to use the switch so unknown values fail fast.
🧹 Nitpick comments (1)
src/main/java/org/gridsuite/modification/dto/ModificationReferenceInfos.java (1)
67-69: Prefer immutable empty map for no-value payloads.
new HashMap<>()allocates a mutable object each call;Map.of()is cheaper and expresses intent better.Proposed refactor
`@Override` public Map<String, String> getMapMessageValues() { - return new HashMap<>(); + return Map.of(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/gridsuite/modification/dto/ModificationReferenceInfos.java` around lines 67 - 69, The getMapMessageValues method in class ModificationReferenceInfos currently returns a newly allocated mutable HashMap on each call; replace that with an immutable empty map (e.g., return Map.of()) to avoid unnecessary allocations and make the intent explicit—update the body of ModificationReferenceInfos.getMapMessageValues() to return an immutable empty map instance instead of new HashMap<>().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@src/main/java/org/gridsuite/modification/dto/ModificationReferenceInfos.java`:
- Around line 62-64: createSubReportNode silently treats any non-SAMPLE
referenceType (including null) as DIRECTORY; first validate referenceType (call
or inline the existing check() or throw IllegalStateException on null/invalid)
and then replace the ternary with an exhaustive switch on referenceType (switch
over Type enum with cases SAMPLE and DIRECTORY and a default that throws) to
avoid accidental fallback/misreporting; update createSubReportNode to perform
validation before constructing the ReportNode and to use the switch so unknown
values fail fast.
---
Nitpick comments:
In
`@src/main/java/org/gridsuite/modification/dto/ModificationReferenceInfos.java`:
- Around line 67-69: The getMapMessageValues method in class
ModificationReferenceInfos currently returns a newly allocated mutable HashMap
on each call; replace that with an immutable empty map (e.g., return Map.of())
to avoid unnecessary allocations and make the intent explicit—update the body of
ModificationReferenceInfos.getMapMessageValues() to return an immutable empty
map instance instead of new HashMap<>().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d9d1c774-8a13-4bc0-b6a7-2b38aec5cb1d
📒 Files selected for processing (9)
src/main/java/org/gridsuite/modification/ModificationType.javasrc/main/java/org/gridsuite/modification/dto/ModificationInfos.javasrc/main/java/org/gridsuite/modification/dto/ModificationReferenceInfos.javasrc/main/java/org/gridsuite/modification/modifications/ModificationReference.javasrc/test/java/org/gridsuite/modification/modifications/AbstractNetworkModificationTest.javasrc/test/java/org/gridsuite/modification/modifications/CreateVoltageLevelSectionTest.javasrc/test/java/org/gridsuite/modification/modifications/LccCreationInNodeBreakerTest.javasrc/test/java/org/gridsuite/modification/modifications/ModificationReferenceTest.javasrc/test/java/org/gridsuite/modification/modifications/VoltageLevelTopologyModificationTest.java
✅ Files skipped from review due to trivial changes (5)
- src/main/java/org/gridsuite/modification/ModificationType.java
- src/test/java/org/gridsuite/modification/modifications/VoltageLevelTopologyModificationTest.java
- src/test/java/org/gridsuite/modification/modifications/LccCreationInNodeBreakerTest.java
- src/test/java/org/gridsuite/modification/modifications/CreateVoltageLevelSectionTest.java
- src/main/java/org/gridsuite/modification/modifications/ModificationReference.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/org/gridsuite/modification/modifications/ModificationReferenceTest.java
Contributor
There was a problem hiding this comment.
I only reported my test results in gridsuite/directory-server#247
So just a few questions and discussions about naming.
Comment on lines
+27
to
+29
| /** | ||
| * @author Slimane Amar <slimane.amar at rte-france.com> | ||
| */ |
Contributor
There was a problem hiding this comment.
It would be useful to precise somewhere what can be referenced. The jira precises only composites but it looks like anything can be referenced (of course if really any modification can and will be referenced, no need to precise anything, but should a modification reference be referenced by another modification reference ?)
|
|
||
| @Override | ||
| public Map<String, String> getMapMessageValues() { | ||
| return new HashMap<>(); |
Contributor
There was a problem hiding this comment.
Why are you overriding an already empty super function ? If I am reading right assignAttributes from ModificationReferenceEntity, this should never be used right ?
Contributor
Author
There was a problem hiding this comment.
We force the value as empty.
Because the default value may change later for ordinary modifications
| .referenceType(ModificationReferenceInfos.Type.SAMPLE) | ||
| .referenceId(UUID.randomUUID()) | ||
| .referenceInfos(modificationInfo) | ||
| .stashed(false) |
Contributor
There was a problem hiding this comment.
It is a default value :
Suggested change
| .stashed(false) |
Contributor
Author
There was a problem hiding this comment.
It's like the other tests
…twork-modification into shared-modification
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/resources/org/gridsuite/modification/reports_fr.properties`:
- Line 28: Replace the English values for the two localization keys with the
provided French translations: update network.modification.basic.reference.apply
to "Modification par référence basique" and update
network.modification.directory.reference.apply to "Modifications par référence
de répertoire"; ensure you edit these values in
src/main/resources/org/gridsuite/modification/reports_fr.properties so the keys
remain unchanged and only the right-hand-side strings are replaced.
🪄 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: 4449069f-aa71-488a-825e-08e6a17483b6
📒 Files selected for processing (5)
src/main/java/org/gridsuite/modification/dto/ModificationReferenceInfos.javasrc/main/java/org/gridsuite/modification/modifications/ModificationReference.javasrc/main/resources/org/gridsuite/modification/reports.propertiessrc/main/resources/org/gridsuite/modification/reports_fr.propertiessrc/test/java/org/gridsuite/modification/modifications/ModificationReferenceTest.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/org/gridsuite/modification/modifications/ModificationReference.java
- src/main/java/org/gridsuite/modification/dto/ModificationReferenceInfos.java
- src/test/java/org/gridsuite/modification/modifications/ModificationReferenceTest.java
|
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