Skip to content

fix: keep temporary limits in REPLACE mode when nothing is applied#191

Open
flomillot wants to merge 5 commits into
mainfrom
fix/temporary-limits-replace-no-modification
Open

fix: keep temporary limits in REPLACE mode when nothing is applied#191
flomillot wants to merge 5 commits into
mainfrom
fix/temporary-limits-replace-no-modification

Conversation

@flomillot
Copy link
Copy Markdown
Contributor

@flomillot flomillot commented May 22, 2026

Problem

In REPLACE mode, the existing temporary limits of a limit set were dropped unconditionally, even when every provided input had been rejected by validation (missing name/duration, duplicate name, no match). The limit set ended up empty although no modification had actually been performed.

Changes

  • modifyTemporaryLimits now tracks whether at least one input actually applied a change. In REPLACE mode the untouched limits are dropped only when at least one input was applied; otherwise the existing temporary limits are kept intact.
  • The "no match" check — a MODIFY input targeting a non-existing temporary limit — is extracted into isModifyWithoutMatch and moved before the duplicate check.
  • temporaryLimitsNoMatch log message reworded for consistency with the other "ignored" messages.
  • Validation logs split per failed field: temporaryLimitsMissingInfo becomes temporaryLimitsMissingName / temporaryLimitsMissingDuration, and temporaryLimitsDuplicate becomes temporaryLimitsDuplicateName / temporaryLimitsDuplicateDuration. preModificationCheck and wouldCreateDuplicate now aggregate problems instead of returning on the first one, so a row that fails several checks reports every individual issue.
  • Refactor: the add-back/REPLACE block is extracted into handleUnmodifiedTemporaryLimits, and preModificationCheck is split into checkTemporaryLimitModificationType (modificationType consistency) and checkTemporaryLimitMandatoryFields (presence of name and duration).

In REPLACE mode the existing temporary limits were dropped unconditionally
as soon as any limit was left untouched, even when every provided input was
rejected by validation (missing field, duplicate name, no match). The limit
set was emptied although no modification had actually been performed.

The existing temporary limits are now kept untouched unless at least one
input actually created, modified or deleted a temporary limit.

The "no match" check (a MODIFY input targeting a non-existing temporary
limit) is also moved before the duplicate check, so an ignored input has no
side effect on the limit set.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 22, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: dd1bbbd8-afaf-48cc-a98a-199ff4b9dae4

📥 Commits

Reviewing files that changed from the base of the PR and between 2e3d4ce and 74cd5b0.

📒 Files selected for processing (1)
  • src/main/java/org/gridsuite/modification/modifications/olg/OperationalLimitsGroupModification.java

📝 Walkthrough

Walkthrough

Refactors temporary-limit processing to track whether inputs applied changes, adds validation to skip MODIFY rows with no matching duration, splits missing/duplicate report keys into name vs duration variants, changes applyTemporaryLimitModification to return boolean, and updates tests and messages to match the new behavior.

Changes

Temporary Limit Modification Tracking and Validation

Layer / File(s) Summary
Loop tracking & detail-log condition
src/main/java/org/gridsuite/modification/modifications/olg/OperationalLimitsGroupModification.java
Introduce atLeastOneLimitApplied and require it together with areLimitsReplaced before emitting the temporaryLimitsReplaced detail log.
preModificationCheck refactor
src/main/java/org/gridsuite/modification/modifications/olg/OperationalLimitsGroupModification.java
Refactor preModificationCheck(...) to compute and return a valid boolean based on missing/invalid temporary-limit fields.
Mandatory fields & isModifyWithoutMatch validation
src/main/java/org/gridsuite/modification/modifications/olg/OperationalLimitsGroupModification.java
Add checkTemporaryLimitMandatoryFields(...) to emit per-field warnings for missing name/acceptableDuration, and add isModifyWithoutMatch(...) that logs a warning and skips MODIFY rows with no matching existing temporary limit by acceptable duration.
Duplicate detection logging
src/main/java/org/gridsuite/modification/modifications/olg/OperationalLimitsGroupModification.java
Emit distinct warnings/report keys for duplicate acceptable duration vs duplicate name while preserving duplicate-detection skip behavior.
applyTemporaryLimitModification boolean refactor
src/main/java/org/gridsuite/modification/modifications/olg/OperationalLimitsGroupModification.java
Change applyTemporaryLimitModification(...) to return boolean; refactor to early-return false for pre-check failures, MODIFY-without-match, or duplicates, and return true when a create/modify/delete is applied; remove modified entries from unmodifiedTemporaryLimits and update the working set.
modifyTemporaryLimit reporting
src/main/java/org/gridsuite/modification/modifications/olg/OperationalLimitsGroupModification.java
Compute finalValueToReport earlier and reuse it in both value-only and general modification logging branches for consistent messages.
Reports properties
src/main/resources/org/gridsuite/modification/reports.properties
Split temporaryLimitsMissingInfo into temporaryLimitsMissingName and temporaryLimitsMissingDuration; split temporaryLimitsDuplicate into temporaryLimitsDuplicateName and temporaryLimitsDuplicateDuration; rewrite temporaryLimitsNoMatch formatting to use acceptableDuration=${limitAcceptableDuration} and end with : ignored.
Tests updated
src/test/java/org/gridsuite/modification/modifications/tabularmodifications/LimitSetModificationsTest.java
Parameterize missing-field cases to assert specific missing-key/messages and always retain existing temporary limits; update duplicate and no-match WARN assertions to expect the new, more specific messages and report keys.

Suggested reviewers

  • Meklo
  • basseche
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 73.68% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main fix: preventing temporary limits from being dropped in REPLACE mode when no inputs are successfully applied.
Description check ✅ Passed The description is well-related to the changeset, explaining the problem, changes made, and how the fix addresses the issue of unconditionally dropping temporary limits.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/test/java/org/gridsuite/modification/modifications/tabularmodifications/LimitSetModificationsTest.java (1)

440-458: ⚡ Quick win

Add an explicit REPLACE + no-match regression case.

This now covers missing-field rejection in REPLACE mode, but not the MODIFY-without-match rejection path that was also part of the bug scenario. Adding that case here would better lock the isModifyWithoutMatch + atLeastOneLimitApplied behavior.

🤖 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/modification/modifications/tabularmodifications/LimitSetModificationsTest.java`
around lines 440 - 458, The test currently checks missing-field rejection for
one path but lacks a regression case for a MODIFY-without-match scenario; update
the MethodSource "missingTemporaryLimitFieldCases" (the provider used by
testTemporaryLimitMissingFieldIsSkipped) to include an explicit case where
TemporaryLimitModificationType is MODIFY and the modification targets a
non-matching limit (simulate "no-match"), so the code exercises the
isModifyWithoutMatch + atLeastOneLimitApplied logic; ensure the new case still
results in no changes to existing temporary limits (assertEquals(2,
limits.getTemporaryLimits().size()) remains valid) and reuse the existing
buildMissingFieldModification/ModificationInfos setup so the test asserts the
missing-field rejection path for MODIFY-without-match.
🤖 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.

Nitpick comments:
In
`@src/test/java/org/gridsuite/modification/modifications/tabularmodifications/LimitSetModificationsTest.java`:
- Around line 440-458: The test currently checks missing-field rejection for one
path but lacks a regression case for a MODIFY-without-match scenario; update the
MethodSource "missingTemporaryLimitFieldCases" (the provider used by
testTemporaryLimitMissingFieldIsSkipped) to include an explicit case where
TemporaryLimitModificationType is MODIFY and the modification targets a
non-matching limit (simulate "no-match"), so the code exercises the
isModifyWithoutMatch + atLeastOneLimitApplied logic; ensure the new case still
results in no changes to existing temporary limits (assertEquals(2,
limits.getTemporaryLimits().size()) remains valid) and reuse the existing
buildMissingFieldModification/ModificationInfos setup so the test asserts the
missing-field rejection path for MODIFY-without-match.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 130d6292-3b57-4a45-ae91-64f1355c7319

📥 Commits

Reviewing files that changed from the base of the PR and between 286f4c9 and 801a07f.

📒 Files selected for processing (3)
  • src/main/java/org/gridsuite/modification/modifications/olg/OperationalLimitsGroupModification.java
  • src/main/resources/org/gridsuite/modification/reports.properties
  • src/test/java/org/gridsuite/modification/modifications/tabularmodifications/LimitSetModificationsTest.java

flomillot added 4 commits May 25, 2026 11:45
Missing-info and duplicate-conflict checks each emitted a single log
covering both fields. Split into per-field templates
(temporaryLimitsMissingName / temporaryLimitsMissingDuration,
temporaryLimitsDuplicateName / temporaryLimitsDuplicateDuration) and
aggregate the validity result so every problem on a row is reported.

Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>
Remove unnecessary value change check to ensure logs are emitted only when the name is modified, avoiding redundant log entries.

Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>
Simplified condition to ensure logging occurs only when the name is modified, improving clarity and avoiding redundant code.

Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>
…cationCheck

- `handleUnmodifiedTemporaryLimits` factored out of `modifyTemporaryLimits`.
- `preModificationCheck` split into `checkTemporaryLimitModificationType`
  (per-limit modificationType consistency) and `checkTemporaryLimitMandatoryFields`
  (presence of name and duration); both are always evaluated so every problem
  on a limit is reported.

Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>
@sonarqubecloud
Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant