copy cut paste reference modification and add ref with target infos - #1058
copy cut paste reference modification and add ref with target infos#1058souissimai wants to merge 23 commits into
Conversation
Signed-off-by: SOUISSI Maissa (Externe) <souissimai@gm0winl878.bureau.si.interne>
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: 1
🧹 Nitpick comments (1)
src/main/java/org/gridsuite/study/server/service/StudyService.java (1)
2167-2175: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd regression coverage for reference registration.
src/test/java/org/gridsuite/study/server/rootnetworks/ModificationToExcludeTest.javaverifies exclusion mappings but not the new directory-server side effect. Add a test that stubsnetworkModificationService.getReferences(...)and verifiesdirectoryService.createsReferencesToSharedComposites(...)receives the shared element UUID andtargetNodeUuid. Cover multiple references and an empty result.🤖 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 2167 - 2175, Add regression tests in ModificationToExcludeTest for the reference-registration flow around createReferencesToSharedComposites: stub networkModificationService.getReferences(...) and verify directoryService.createsReferencesToSharedComposites(...) is called with each shared element UUID and targetNodeUuid, covering multiple references and an empty result with no calls.
🤖 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 2161-2165: The workflow around
networkModificationService.duplicateModifications must not depend on the local
transaction for remote directory-server writes. Make
createReferencesToSharedComposites idempotent and retryable, or track each
completed reference and compensate both those references and the duplicated
modifications on failure before propagating the error.
---
Nitpick comments:
In `@src/main/java/org/gridsuite/study/server/service/StudyService.java`:
- Around line 2167-2175: Add regression tests in ModificationToExcludeTest for
the reference-registration flow around createReferencesToSharedComposites: stub
networkModificationService.getReferences(...) and verify
directoryService.createsReferencesToSharedComposites(...) is called with each
shared element UUID and targetNodeUuid, covering multiple references and an
empty result with no calls.
🪄 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: b24090fe-464b-40e8-9a61-63b234061fd4
📒 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
🤖 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/test/java/org/gridsuite/study/server/NetworkModificationTest.java`:
- Around line 2346-2350: Update the reference lookup stubs and verifications in
the affected duplication tests to match the expected original and copied UUID
query values explicitly, rather than accepting any uuids parameter. Verify each
duplication operation performs exactly one lookup with its correct UUIDs,
including the cases currently using aggregate counts, while preserving the
existing no-reference response behavior.
🪄 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: 172843bd-b608-4798-9bac-0691ad90f60c
📒 Files selected for processing (1)
src/test/java/org/gridsuite/study/server/NetworkModificationTest.java
| // no shared-composite reference among the duplicated modifications | ||
| wireMockServer.stubFor(WireMock.get(WireMock.urlPathEqualTo("/v1/references")) | ||
| .willReturn(WireMock.ok() | ||
| .withBody(mapper.writeValueAsString(Map.of())) | ||
| .withHeader(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_JSON_VALUE))); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Assert the UUIDs used in each reference lookup.
The /v1/references stubs match any query, and the verifications accept any uuids value. These tests can pass when the service omits an ID, sends the wrong original or copied ID, or sends an empty value. At Line 2405, the aggregate count across two duplications also does not prove that each operation performs one lookup. Match the expected UUIDs for each request and verify the count per duplication operation.
Also applies to: 2405-2405, 2452-2457, 2475-2475, 2512-2515, 2538-2538
🤖 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/test/java/org/gridsuite/study/server/NetworkModificationTest.java` around
lines 2346 - 2350, Update the reference lookup stubs and verifications in the
affected duplication tests to match the expected original and copied UUID query
values explicitly, rather than accepting any uuids parameter. Verify each
duplication operation performs exactly one lookup with its correct UUIDs,
including the cases currently using aggregate counts, while preserving the
existing no-reference response behavior.
Signed-off-by: SOUISSI Maissa (Externe) <souissimai@gm0winl878.bureau.si.interne>
Signed-off-by: SOUISSI Maissa (Externe) <souissimai@gm0winl878.bureau.si.interne>
Signed-off-by: SOUISSI Maissa (Externe) <souissimai@gm0winl878.bureau.si.interne>
Signed-off-by: SOUISSI Maissa (Externe) <souissimai@gm0winl878.bureau.si.interne>
Signed-off-by: SOUISSI Maissa (Externe) <souissimai@gm0winl878.bureau.si.interne>
|
Warning Your free Security trial is over. An organization admin can activate billing to continue. |
Signed-off-by: SOUISSI Maissa (Externe) <souissimai@gm0winl878.bureau.si.interne>
| * @param referenceId uuid of the referenced shared composite | ||
| * @param containerId uuid of the composite containing the reference, null if the modification-reference is at the root level | ||
| */ | ||
| public record ReferenceData(UUID modificationUuid, UUID referenceId, UUID containerId) { |
There was a problem hiding this comment.
I think this is clearer :
| * @param referenceId uuid of the referenced shared composite | |
| * @param containerId uuid of the composite containing the reference, null if the modification-reference is at the root level | |
| */ | |
| public record ReferenceData(UUID modificationUuid, UUID referenceId, UUID containerId) { | |
| * @param sharedCompositeId uuid of the referenced shared composite | |
| * @param containerId uuid of the composite containing the reference, null if the modification-reference is at the root level | |
| */ | |
| public record ReferenceData(UUID modificationUuid, UUID sharedCompositeId, UUID containerId) { |
Mathieu-Deharbe
left a comment
Mathieu-Deharbe
left a comment
There was a problem hiding this comment.
Only small remarks but I didn't validate yet because I need to test one last time after the conflict solving.
| List<UUID> targetChildrenUuids = networkModificationTreeService.getChildrenUuids(targetNodeUuid); | ||
| Map<UUID, List<UUID>> originChildrenUuidsByNode = originNodesTouched.stream() | ||
| .collect(Collectors.toMap(Function.identity(), networkModificationTreeService::getChildrenUuids, (a, b) -> a, LinkedHashMap::new)); | ||
|
|
||
| notificationService.emitStartModificationEquipmentNotification(studyUuid, targetNodeUuid, targetChildrenUuids, NotificationService.MODIFICATIONS_UPDATING_IN_PROGRESS); | ||
| originChildrenUuidsByNode.forEach((originNodeUuid, children) -> | ||
| notificationService.emitStartModificationEquipmentNotification(studyUuid, originNodeUuid, children, NotificationService.MODIFICATIONS_UPDATING_IN_PROGRESS)); |
There was a problem hiding this comment.
That part about notifications could probably be extracted for clarity, but we will see in the refacto.
| Collectors.mapping(ModificationMoveOrCopyInfos::modificationUuid, Collectors.toList()))); | ||
| } | ||
|
|
||
| private boolean hasModificationSource(ModificationContainerInfos source) { |
There was a problem hiding this comment.
Given that the cource may not be a modification and no data here involves a modification :
| private boolean hasModificationSource(ModificationContainerInfos source) { | |
| private boolean hasSource(ModificationContainerInfos source) { |
or
| private boolean hasModificationSource(ModificationContainerInfos source) { | |
| private boolean sourceDefined(ModificationContainerInfos source) { |
| private void createReferencesToSharedComposites(List<ReferenceData> referenceTargets, List<UUID> modificationsUuids, | ||
| Map<UUID, UUID> mappingModificationsUuids, String userId, UUID targetNodeUuid) { | ||
| Set<UUID> requestedUuids = new HashSet<>(modificationsUuids); |
There was a problem hiding this comment.
I don't think these are targets :
| private void createReferencesToSharedComposites(List<ReferenceData> referenceTargets, List<UUID> modificationsUuids, | |
| Map<UUID, UUID> mappingModificationsUuids, String userId, UUID targetNodeUuid) { | |
| Set<UUID> requestedUuids = new HashSet<>(modificationsUuids); | |
| private void createReferencesToSharedComposites(List<ReferenceData> references, List<UUID> modificationsUuids, | |
| Map<UUID, UUID> mappingModificationsUuids, String userId, UUID targetNodeUuid) { | |
| Set<UUID> requestedUuids = new HashSet<>(modificationsUuids); |
I understand the function but this is very convoluted and looks like the data could be stored in mappingModificationsUuids when buildModificationsUuidMapping is called. With a specific mapping structure we could probably simplify these functions and comments. modificationsUuids would be removed etc...
I am not asking you to do this here but could you reference this in the refacto ticket ?
There was a problem hiding this comment.
here we create refernce for list of copied modification , gourped by source container, exact !to refactor ! created ticket
| return mappingModificationsUuids; | ||
| } | ||
|
|
||
| private void copyModificationsToExclude(UUID originNodeUuid, UUID targetNodeUuid, Map<UUID, UUID> mappingModificationsUuids) { |
There was a problem hiding this comment.
You should remove the original copyModificationsToExclude function now useless; Anyway this will all be removed by Florent ticket about applicability.
Signed-off-by: SOUISSI Maissa (Externe) <souissimai@gm0winl878.bureau.si.interne>
Signed-off-by: SOUISSI Maissa (Externe) <souissimai@gm0winl878.bureau.si.interne>
|
2 participants
PR Summary