Add applicability per root network tag on network modifications - #219
Add applicability per root network tag on network modifications#219flomillot wants to merge 12 commits into
Conversation
Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesRoot-network applicability
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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.
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/java/org/gridsuite/modification/dto/ModificationInfos.java`:
- Around line 165-167: Update the applicability check in ModificationInfos so a
null value for the rootNetworkTag entry is treated as applicable, with only an
explicit Boolean false disabling it; avoid returning a nullable Boolean that can
be unboxed. Add a regression test covering an applicabilityByRootNetworkTag
entry whose value is null.
🪄 Autofix
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 Plus
Run ID: 9a164320-11d9-46ea-8088-e0310a976005
📒 Files selected for processing (1)
src/main/java/org/gridsuite/modification/dto/ModificationInfos.java
…fication-applicability
The method combines the global activation flag and the per root network tag applicability, while being applicable refers to the tag alone. Signed-off-by: Florent MILLOT <florent.millot_externe@rte-france.com>
A stashed modification is not activated on any root network: the stash is now part of the activation rule instead of being checked separately by every caller. An undefined stash flag keeps the default of the field, not stashed. Signed-off-by: Florent MILLOT <florent.millot_externe@rte-france.com>
….java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
A tag mapped to a null value reaches the DTO through deserialization. The map lookup now tells it apart from an explicit false, and the test locks that in. Signed-off-by: Florent MILLOT <florent.millot_externe@rte-france.com>
…string The test compares a hardcoded toString, which lombok now renders with applicabilityByRootNetworkTag between activated and description. Signed-off-by: Florent MILLOT <florent.millot_externe@rte-france.com>
| assertEquals( | ||
| "TwoWindingsTransformerCreationInfos(super=BranchCreationInfos(super=EquipmentCreationInfos(super=EquipmentModificationInfos(super=ModificationInfos(uuid=null, " | ||
| + "type=TWO_WINDINGS_TRANSFORMER_CREATION, date=null, stashed=false, messageType=null, messageValues=null, activated=true, description=a dummy description), " | ||
| + "type=TWO_WINDINGS_TRANSFORMER_CREATION, date=null, stashed=false, messageType=null, messageValues=null, activated=true, applicabilityByRootNetworkTag=null, " |
There was a problem hiding this comment.
can applicabilityByRootNetworkTag be null ? for a modification stored in gridexplore for example ?
There was a problem hiding this comment.
| if (Boolean.TRUE.equals(stashed) || !Boolean.TRUE.equals(activated)) { | ||
| return false; | ||
| } | ||
| return rootNetworkTag == null | ||
| || applicabilityByRootNetworkTag == null | ||
| || !Boolean.FALSE.equals(applicabilityByRootNetworkTag.get(rootNetworkTag)); |
There was a problem hiding this comment.
I think the '!' is unecessary, especially for the second case :
| if (Boolean.TRUE.equals(stashed) || !Boolean.TRUE.equals(activated)) { | |
| return false; | |
| } | |
| return rootNetworkTag == null | |
| || applicabilityByRootNetworkTag == null | |
| || !Boolean.FALSE.equals(applicabilityByRootNetworkTag.get(rootNetworkTag)); | |
| if (Boolean.TRUE.equals(stashed) || Boolean.FALSE.equals(activated)) { | |
| return false; | |
| } | |
| return rootNetworkTag == null | |
| || applicabilityByRootNetworkTag == null | |
| || Boolean.TRUE.equals(applicabilityByRootNetworkTag.get(rootNetworkTag)); |
Edit : My bad I was wrong about the null. Still a bit convulated to read.
| ModificationInfos modificationInfos = modificationInfos(true, Map.of(TAG, true)); | ||
| modificationInfos.setStashed(true); | ||
| assertFalse(modificationInfos.isActivatedOn(TAG), | ||
| "A stashed modification is activated on no root network, whatever its applicabilities"); |
There was a problem hiding this comment.
I think this is clearer english but you may want to remain closer to the function name, so do as you wish :
| "A stashed modification is activated on no root network, whatever its applicabilities"); | |
| "A stashed modification should never activate, whatever its applicabilities"); |
There was a problem hiding this comment.
should never be activated is more correct no ?
| void testActivatedWhenTagEntryIsNull() { | ||
| Map<String, Boolean> applicabilityByRootNetworkTag = new HashMap<>(); | ||
| applicabilityByRootNetworkTag.put(TAG, null); | ||
| assertTrue(modificationInfos(true, applicabilityByRootNetworkTag).isActivatedOn(TAG), |
There was a problem hiding this comment.
You could write this on one line instead of three like in the previous test.
There was a problem hiding this comment.
Yes it's because Map of doesn't accept null values. I found an alternative.
…fication-applicability
…Test.java Co-authored-by: Mathieu Deharbe <148252167+Mathieu-Deharbe@users.noreply.github.com>
|
Warning Your free Security trial is over. An organization admin can activate billing to continue. |
…Test.java Co-authored-by: Mathieu Deharbe <148252167+Mathieu-Deharbe@users.noreply.github.com>
Map.of rejects a null value, singletonMap does not. Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>
…plicability' into florent/GRD-4941-modification-applicability
|
3 participants
A network modification now carries its applicability per root network tag, next to its global
activatedflag:Map<String, Boolean> applicabilityByRootNetworkTag, where a tag without an entry is applicable.isActivatedOn(rootNetworkTag)gathers the rule used by the servers when applying modifications:!stashed && activated && applicability.getOrDefault(tag, true). The stash is part of the rule so that every caller shares a single predicate; a stashed modification can no longer be applied to a network.