add max allowed computations#102
Merged
Merged
Conversation
Signed-off-by: Etienne LESOT <etienne.lesot@rte-france.com>
|
Warning Review limit reached
More reviews will be available in 39 minutes and 42 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughEleven new analysis-type quota fields are added to the user profile system. A Liquibase migration extends ChangesNew maxAllowed quota dimensions for user profiles
🚥 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/gridsuite/useradmin/server/service/UserProfileService.java (1)
119-135:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
updateProfileignores the 11 newly introduced quota fields.This method updates only
maxAllowedCases/maxAllowedBuilds; incoming values for the new quota dimensions are never persisted.Suggested fix
profile.setMaxAllowedCases(userProfile.maxAllowedCases()); profile.setMaxAllowedBuilds(userProfile.maxAllowedBuilds()); + profile.setMaxAllowedLoadflow(userProfile.maxAllowedLoadflow()); + profile.setMaxAllowedSecurity(userProfile.maxAllowedSecurity()); + profile.setMaxAllowedSensitivity(userProfile.maxAllowedSensitivity()); + profile.setMaxAllowedShortCircuit(userProfile.maxAllowedShortCircuit()); + profile.setMaxAllowedVoltageInit(userProfile.maxAllowedVoltageInit()); + profile.setMaxAllowedPccMin(userProfile.maxAllowedPccMin()); + profile.setMaxAllowedStateEstimation(userProfile.maxAllowedStateEstimation()); + profile.setMaxAllowedBalanceAdjustement(userProfile.maxAllowedBalanceAdjustement()); + profile.setMaxAllowedDynamicSimulation(userProfile.maxAllowedDynamicSimulation()); + profile.setMaxAllowedDynamicSecurity(userProfile.maxAllowedDynamicSecurity()); + profile.setMaxAllowedDynamicMargin(userProfile.maxAllowedDynamicMargin()); profile.setSpreadsheetConfigCollectionId(userProfile.spreadsheetConfigCollectionId());🤖 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/useradmin/server/service/UserProfileService.java` around lines 119 - 135, The updateProfile method in UserProfileService is not persisting the 11 newly introduced quota fields from the incoming userProfile object. Add setter calls to the profile entity for each of the missing quota dimension fields, following the same pattern as the existing setters like setMaxAllowedCases and setMaxAllowedBuilds. Extract each quota field value from the userProfile parameter and call the corresponding setter method on the profile entity to ensure all new quota fields are properly persisted.
🤖 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/useradmin/server/controller/UserAdminController.java`:
- Around line 151-237: The endpoints getUserProfileMaxAllowedLoadflows,
getUserProfileMaxAllowedSecurity, getUserProfileMaxAllowedSensitivity,
getUserProfileMaxAllowedShortCircuit, getUserProfileMaxAllowedVoltageInit,
getUserProfileMaxAllowedPccMin, getUserProfileMaxAllowedStateEstimation,
getUserProfileMaxAllowedBalanceAdjustement,
getUserProfileMaxAllowedDynamicSimulation,
getUserProfileMaxAllowedDynamicSecurity, and
getUserProfileMaxAllowedDynamicMargin all document 404 responses in their
ApiResponse annotations, but they always return ResponseEntity.ok() which means
they never actually return 404. Resolve this inconsistency by either removing
the 404 ApiResponse annotations from all these endpoints if the service never
returns not-found errors, or if the service can return null or throw an
exception for unknown users, modify these handlers to check the result and
return ResponseEntity.notFound().build() when appropriate.
In `@src/main/resources/db/changelog/changesets/changelog_20260616T125854Z.xml`:
- Around line 5-55: The Liquibase changeset column names use snake_case
(max_allowed_*) but the UserProfileEntity JPA entity maps these fields to
camelCase column names (maxAllowed*). Update all column name attributes in the
addColumn elements across all changeSet entries (ids 1781614744540-2 through
1781614744540-12) to change from the snake_case format
(max_allowed_balance_adjustement, max_allowed_dynamic_margin,
max_allowed_dynamic_security, max_allowed_dynamic_simulation,
max_allowed_loadflow, max_allowed_pcc_min, max_allowed_security,
max_allowed_sensitivity, max_allowed_short_circuit,
max_allowed_state_estimation, max_allowed_voltage_init, etc.) to the camelCase
format that matches the entity mapping (maxAllowedBalanceAdjustement,
maxAllowedDynamicMargin, maxAllowedDynamicSecurity, maxAllowedDynamicSimulation,
maxAllowedLoadflow, maxAllowedPccMin, maxAllowedSecurity, maxAllowedSensitivity,
maxAllowedShortCircuit, maxAllowedStateEstimation, maxAllowedVoltageInit, etc.)
to ensure the database schema aligns with the JPA entity column name mappings.
---
Outside diff comments:
In
`@src/main/java/org/gridsuite/useradmin/server/service/UserProfileService.java`:
- Around line 119-135: The updateProfile method in UserProfileService is not
persisting the 11 newly introduced quota fields from the incoming userProfile
object. Add setter calls to the profile entity for each of the missing quota
dimension fields, following the same pattern as the existing setters like
setMaxAllowedCases and setMaxAllowedBuilds. Extract each quota field value from
the userProfile parameter and call the corresponding setter method on the
profile entity to ensure all new quota fields are properly persisted.
🪄 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: 1970dc4b-649a-4c36-8255-1a8515762f0e
📒 Files selected for processing (18)
src/main/java/org/gridsuite/useradmin/server/UserAdminApplicationProps.javasrc/main/java/org/gridsuite/useradmin/server/controller/UserAdminController.javasrc/main/java/org/gridsuite/useradmin/server/dto/UserInfos.javasrc/main/java/org/gridsuite/useradmin/server/dto/UserProfile.javasrc/main/java/org/gridsuite/useradmin/server/entity/UserInfosEntity.javasrc/main/java/org/gridsuite/useradmin/server/entity/UserProfileEntity.javasrc/main/java/org/gridsuite/useradmin/server/service/UserAdminService.javasrc/main/java/org/gridsuite/useradmin/server/service/UserIdentityService.javasrc/main/java/org/gridsuite/useradmin/server/service/UserInfosService.javasrc/main/java/org/gridsuite/useradmin/server/service/UserProfileService.javasrc/main/resources/db/changelog/changesets/changelog_20260616T125854Z.xmlsrc/main/resources/db/changelog/db.changelog-master.yamlsrc/test/java/org/gridsuite/useradmin/server/DtoConverterTest.javasrc/test/java/org/gridsuite/useradmin/server/NoQuotaTest.javasrc/test/java/org/gridsuite/useradmin/server/UserAdminTest.javasrc/test/java/org/gridsuite/useradmin/server/UserProfileTest.javasrc/test/java/org/gridsuite/useradmin/server/controller/UserInfosControllerTest.javasrc/test/java/org/gridsuite/useradmin/server/service/UserInfosServiceTest.java
Comment on lines
+151
to
+237
| @GetMapping(value = "/users/{sub}/profile/max-loadflow") | ||
| @Operation(summary = "Get the user's max allowed loadflows") | ||
| @ApiResponse(responseCode = "200", description = "The user max allowed loadflows") | ||
| @ApiResponse(responseCode = "404", description = "The user doesn't exist") | ||
| public ResponseEntity<Integer> getUserProfileMaxAllowedLoadflows(@PathVariable String sub) { | ||
| return ResponseEntity.ok().body(service.getUserProfileMaxAllowedLoadflows(sub)); | ||
| } | ||
|
|
||
| @GetMapping(value = "/users/{sub}/profile/max-security") | ||
| @Operation(summary = "Get the user's max allowed security analysis") | ||
| @ApiResponse(responseCode = "200", description = "The user max allowed security analysis") | ||
| @ApiResponse(responseCode = "404", description = "The user doesn't exist") | ||
| public ResponseEntity<Integer> getUserProfileMaxAllowedSecurity(@PathVariable String sub) { | ||
| return ResponseEntity.ok().body(service.getUserProfileMaxAllowedSecurity(sub)); | ||
| } | ||
|
|
||
| @GetMapping(value = "/users/{sub}/profile/max-sensitivity") | ||
| @Operation(summary = "Get the user's max allowed sensitivity analysis") | ||
| @ApiResponse(responseCode = "200", description = "The user max allowed sensitivity analysis") | ||
| @ApiResponse(responseCode = "404", description = "The user doesn't exist") | ||
| public ResponseEntity<Integer> getUserProfileMaxAllowedSensitivity(@PathVariable String sub) { | ||
| return ResponseEntity.ok().body(service.getUserProfileMaxAllowedSensitivity(sub)); | ||
| } | ||
|
|
||
| @GetMapping(value = "/users/{sub}/profile/max-shortcircuit") | ||
| @Operation(summary = "Get the user's max allowed short circuit analysis") | ||
| @ApiResponse(responseCode = "200", description = "The user max allowed short circuit analysis") | ||
| @ApiResponse(responseCode = "404", description = "The user doesn't exist") | ||
| public ResponseEntity<Integer> getUserProfileMaxAllowedShortCircuit(@PathVariable String sub) { | ||
| return ResponseEntity.ok().body(service.getUserProfileMaxAllowedShortCircuit(sub)); | ||
| } | ||
|
|
||
| @GetMapping(value = "/users/{sub}/profile/max-voltage-init") | ||
| @Operation(summary = "Get the user's max allowed voltage init analysis") | ||
| @ApiResponse(responseCode = "200", description = "The user max allowed voltage init analysis") | ||
| @ApiResponse(responseCode = "404", description = "The user doesn't exist") | ||
| public ResponseEntity<Integer> getUserProfileMaxAllowedVoltageInit(@PathVariable String sub) { | ||
| return ResponseEntity.ok().body(service.getUserProfileMaxAllowedVoltageInit(sub)); | ||
| } | ||
|
|
||
| @GetMapping(value = "/users/{sub}/profile/max-pcc-min") | ||
| @Operation(summary = "Get the user's max allowed pcc min analysis") | ||
| @ApiResponse(responseCode = "200", description = "The user max allowed pcc min analysis") | ||
| @ApiResponse(responseCode = "404", description = "The user doesn't exist") | ||
| public ResponseEntity<Integer> getUserProfileMaxAllowedPccMin(@PathVariable String sub) { | ||
| return ResponseEntity.ok().body(service.getUserProfileMaxAllowedPccMin(sub)); | ||
| } | ||
|
|
||
| @GetMapping(value = "/users/{sub}/profile/max-state-estimation") | ||
| @Operation(summary = "Get the user's max allowed state estimation analysis") | ||
| @ApiResponse(responseCode = "200", description = "The user max allowed state estimation analysis") | ||
| @ApiResponse(responseCode = "404", description = "The user doesn't exist") | ||
| public ResponseEntity<Integer> getUserProfileMaxAllowedStateEstimation(@PathVariable String sub) { | ||
| return ResponseEntity.ok().body(service.getUserProfileMaxAllowedStateEstimation(sub)); | ||
| } | ||
|
|
||
| @GetMapping(value = "/users/{sub}/profile/max-balance-adjustement") | ||
| @Operation(summary = "Get the user's max allowed balance adjustement analysis") | ||
| @ApiResponse(responseCode = "200", description = "The user max allowed balance adjustement analysis") | ||
| @ApiResponse(responseCode = "404", description = "The user doesn't exist") | ||
| public ResponseEntity<Integer> getUserProfileMaxAllowedBalanceAdjustement(@PathVariable String sub) { | ||
| return ResponseEntity.ok().body(service.getUserProfileMaxAllowedBalanceAdjustement(sub)); | ||
| } | ||
|
|
||
| @GetMapping(value = "/users/{sub}/profile/max-dynamic-simulation") | ||
| @Operation(summary = "Get the user's max allowed dynamic simulation analysis") | ||
| @ApiResponse(responseCode = "200", description = "The user max allowed dynamic simulation analysis") | ||
| @ApiResponse(responseCode = "404", description = "The user doesn't exist") | ||
| public ResponseEntity<Integer> getUserProfileMaxAllowedDynamicSimulation(@PathVariable String sub) { | ||
| return ResponseEntity.ok().body(service.getUserProfileMaxAllowedDynamicSimulation(sub)); | ||
| } | ||
|
|
||
| @GetMapping(value = "/users/{sub}/profile/max-dynamic-security") | ||
| @Operation(summary = "Get the user's max allowed dynamic security analysis") | ||
| @ApiResponse(responseCode = "200", description = "The user max allowed dynamic security analysis") | ||
| @ApiResponse(responseCode = "404", description = "The user doesn't exist") | ||
| public ResponseEntity<Integer> getUserProfileMaxAllowedDynamicSecurity(@PathVariable String sub) { | ||
| return ResponseEntity.ok().body(service.getUserProfileMaxAllowedDynamicSecurity(sub)); | ||
| } | ||
|
|
||
| @GetMapping(value = "/users/{sub}/profile/max-dynamic-margin") | ||
| @Operation(summary = "Get the user's max allowed dynamic margin analysis") | ||
| @ApiResponse(responseCode = "200", description = "The user max allowed dynamic margin analysis") | ||
| @ApiResponse(responseCode = "404", description = "The user doesn't exist") | ||
| public ResponseEntity<Integer> getUserProfileMaxAllowedDynamicMargin(@PathVariable String sub) { | ||
| return ResponseEntity.ok().body(service.getUserProfileMaxAllowedDynamicMargin(sub)); | ||
| } |
There was a problem hiding this comment.
404 response is documented but unreachable on the new quota endpoints.
At Lines 151-237, each handler always returns ResponseEntity.ok(...), and the delegated service path falls back to defaults instead of signaling not found. The @ApiResponse(responseCode = "404") on these endpoints is therefore inconsistent with actual behavior. Align the contract by either removing 404 from docs or explicitly returning 404 for unknown users/profiles.
🤖 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/useradmin/server/controller/UserAdminController.java`
around lines 151 - 237, The endpoints getUserProfileMaxAllowedLoadflows,
getUserProfileMaxAllowedSecurity, getUserProfileMaxAllowedSensitivity,
getUserProfileMaxAllowedShortCircuit, getUserProfileMaxAllowedVoltageInit,
getUserProfileMaxAllowedPccMin, getUserProfileMaxAllowedStateEstimation,
getUserProfileMaxAllowedBalanceAdjustement,
getUserProfileMaxAllowedDynamicSimulation,
getUserProfileMaxAllowedDynamicSecurity, and
getUserProfileMaxAllowedDynamicMargin all document 404 responses in their
ApiResponse annotations, but they always return ResponseEntity.ok() which means
they never actually return 404. Resolve this inconsistency by either removing
the 404 ApiResponse annotations from all these endpoints if the service never
returns not-found errors, or if the service can return null or throw an
exception for unknown users, modify these handlers to check the result and
return ResponseEntity.notFound().build() when appropriate.
Comment on lines
+5
to
+55
| <column name="max_allowed_balance_adjustement" type="INTEGER"/> | ||
| </addColumn> | ||
| </changeSet> | ||
| <changeSet author="lesoteti (generated)" id="1781614744540-3"> | ||
| <addColumn tableName="user_profile"> | ||
| <column name="max_allowed_dynamic_margin" type="INTEGER"/> | ||
| </addColumn> | ||
| </changeSet> | ||
| <changeSet author="lesoteti (generated)" id="1781614744540-4"> | ||
| <addColumn tableName="user_profile"> | ||
| <column name="max_allowed_dynamic_security" type="INTEGER"/> | ||
| </addColumn> | ||
| </changeSet> | ||
| <changeSet author="lesoteti (generated)" id="1781614744540-5"> | ||
| <addColumn tableName="user_profile"> | ||
| <column name="max_allowed_dynamic_simulation" type="INTEGER"/> | ||
| </addColumn> | ||
| </changeSet> | ||
| <changeSet author="lesoteti (generated)" id="1781614744540-6"> | ||
| <addColumn tableName="user_profile"> | ||
| <column name="max_allowed_loadflow" type="INTEGER"/> | ||
| </addColumn> | ||
| </changeSet> | ||
| <changeSet author="lesoteti (generated)" id="1781614744540-7"> | ||
| <addColumn tableName="user_profile"> | ||
| <column name="max_allowed_pcc_min" type="INTEGER"/> | ||
| </addColumn> | ||
| </changeSet> | ||
| <changeSet author="lesoteti (generated)" id="1781614744540-8"> | ||
| <addColumn tableName="user_profile"> | ||
| <column name="max_allowed_security" type="INTEGER"/> | ||
| </addColumn> | ||
| </changeSet> | ||
| <changeSet author="lesoteti (generated)" id="1781614744540-9"> | ||
| <addColumn tableName="user_profile"> | ||
| <column name="max_allowed_sensitivity" type="INTEGER"/> | ||
| </addColumn> | ||
| </changeSet> | ||
| <changeSet author="lesoteti (generated)" id="1781614744540-10"> | ||
| <addColumn tableName="user_profile"> | ||
| <column name="max_allowed_short_circuit" type="INTEGER"/> | ||
| </addColumn> | ||
| </changeSet> | ||
| <changeSet author="lesoteti (generated)" id="1781614744540-11"> | ||
| <addColumn tableName="user_profile"> | ||
| <column name="max_allowed_state_estimation" type="INTEGER"/> | ||
| </addColumn> | ||
| </changeSet> | ||
| <changeSet author="lesoteti (generated)" id="1781614744540-12"> | ||
| <addColumn tableName="user_profile"> | ||
| <column name="max_allowed_voltage_init" type="INTEGER"/> |
There was a problem hiding this comment.
Align new Liquibase column names with JPA @Column names.
The added DB columns use max_allowed_*, while UserProfileEntity maps these fields to maxAllowed* column names. This schema/mapping mismatch will break persistence for the new quotas.
Suggested fix (make migration names match entity mappings)
- <column name="max_allowed_balance_adjustement" type="INTEGER"/>
+ <column name="maxAllowedBalanceAdjustement" type="INTEGER"/>
...
- <column name="max_allowed_dynamic_margin" type="INTEGER"/>
+ <column name="maxAllowedDynamicMargin" type="INTEGER"/>
...
- <column name="max_allowed_dynamic_security" type="INTEGER"/>
+ <column name="maxAllowedDynamicSecurity" type="INTEGER"/>
...
- <column name="max_allowed_dynamic_simulation" type="INTEGER"/>
+ <column name="maxAllowedDynamicSimulation" type="INTEGER"/>
...
- <column name="max_allowed_loadflow" type="INTEGER"/>
+ <column name="maxAllowedLoadflow" type="INTEGER"/>
...
- <column name="max_allowed_pcc_min" type="INTEGER"/>
+ <column name="maxAllowedPccMin" type="INTEGER"/>
...
- <column name="max_allowed_security" type="INTEGER"/>
+ <column name="maxAllowedSecurity" type="INTEGER"/>
...
- <column name="max_allowed_sensitivity" type="INTEGER"/>
+ <column name="maxAllowedSensitivity" type="INTEGER"/>
...
- <column name="max_allowed_short_circuit" type="INTEGER"/>
+ <column name="maxAllowedShortCircuit" type="INTEGER"/>
...
- <column name="max_allowed_state_estimation" type="INTEGER"/>
+ <column name="maxAllowedStateEstimation" type="INTEGER"/>
...
- <column name="max_allowed_voltage_init" type="INTEGER"/>
+ <column name="maxAllowedVoltageInit" type="INTEGER"/>🤖 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/resources/db/changelog/changesets/changelog_20260616T125854Z.xml`
around lines 5 - 55, The Liquibase changeset column names use snake_case
(max_allowed_*) but the UserProfileEntity JPA entity maps these fields to
camelCase column names (maxAllowed*). Update all column name attributes in the
addColumn elements across all changeSet entries (ids 1781614744540-2 through
1781614744540-12) to change from the snake_case format
(max_allowed_balance_adjustement, max_allowed_dynamic_margin,
max_allowed_dynamic_security, max_allowed_dynamic_simulation,
max_allowed_loadflow, max_allowed_pcc_min, max_allowed_security,
max_allowed_sensitivity, max_allowed_short_circuit,
max_allowed_state_estimation, max_allowed_voltage_init, etc.) to the camelCase
format that matches the entity mapping (maxAllowedBalanceAdjustement,
maxAllowedDynamicMargin, maxAllowedDynamicSecurity, maxAllowedDynamicSimulation,
maxAllowedLoadflow, maxAllowedPccMin, maxAllowedSecurity, maxAllowedSensitivity,
maxAllowedShortCircuit, maxAllowedStateEstimation, maxAllowedVoltageInit, etc.)
to ensure the database schema aligns with the JPA entity column name mappings.
GhilesA
reviewed
Jun 25, 2026
| UUID pccMinParameterId, | ||
| UUID voltageInitParameterId, | ||
| Boolean allLinksValid, | ||
| Map<String, Integer> maxAllowValuesMap, |
There was a problem hiding this comment.
maxAllowValuesMap is not copied and is returned/accessed as is without modification protection
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/test/java/org/gridsuite/useradmin/server/UserProfileTest.java (1)
401-408: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore the
MAX_ALLOWED_CASESround-trip assertion.This helper updates both quota entries on Lines 279-280, but the verification only checks
MAX_ALLOWED_BUILD. A regression in the cases mapping would now pass unnoticed.Suggested fix
assertEquals(newData.getVoltageInitParameterId(), updatedProfile.getVoltageInitParameterId()); + assertEquals(newData.getMaxAllowValuesMap().get(MAX_ALLOWED_CASES), updatedProfile.getMaxAllowValuesMap().get(MAX_ALLOWED_CASES)); assertEquals(newData.getMaxAllowValuesMap().get(MAX_ALLOWED_BUILD), updatedProfile.getMaxAllowValuesMap().get(MAX_ALLOWED_BUILD)); assertNull(updatedProfile.getAllLinksValid()); // validity not set in this case🤖 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/useradmin/server/UserProfileTest.java` around lines 401 - 408, The round-trip verification in UserProfileTest is missing the MAX_ALLOWED_CASES assertion, so a regression in the cases quota mapping could slip through. Update the test’s updatedProfile checks to also compare the MAX_ALLOWED_CASES entry from newData.getMaxAllowValuesMap() against updatedProfile.getMaxAllowValuesMap(), alongside the existing MAX_ALLOWED_BUILD assertion.src/test/java/org/gridsuite/useradmin/server/NoQuotaTest.java (2)
88-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the no-profile branch for the new
max-*endpoints.Lines 92-93 only verify
max-buildsandmax-caseswhen the user has no associated profile. The new quota accessors are only exercised later for a user linked to an empty profile, which is a different branch. Adding the remainingmax-*assertions here would close that gap.Suggested change
`@Test` void testUserCreationWithoutProfile() throws Exception { createUser(USER_SUB); assertTrue(getMaxAllowedBuilds(USER_SUB).isEmpty()); assertTrue(createMaxAllowedCases(USER_SUB).isEmpty()); + assertTrue(getMaxAllowedLoadflow(USER_SUB).isEmpty()); + assertTrue(getMaxAllowedSecurity(USER_SUB).isEmpty()); + assertTrue(getMaxAllowedSensitivity(USER_SUB).isEmpty()); + assertTrue(getMaxAllowedShortCircuit(USER_SUB).isEmpty()); + assertTrue(getMaxAllowedVoltageInit(USER_SUB).isEmpty()); + assertTrue(getMaxAllowedPccMin(USER_SUB).isEmpty()); + assertTrue(getMaxAllowedStateEstimation(USER_SUB).isEmpty()); + assertTrue(getMaxAllowedBalanceAdjustement(USER_SUB).isEmpty()); + assertTrue(getMaxAllowedDynamicSimulation(USER_SUB).isEmpty()); + assertTrue(getMaxAllowedDynamicSecurity(USER_SUB).isEmpty()); + assertTrue(getMaxAllowedDynamicMargin(USER_SUB).isEmpty()); }🤖 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/useradmin/server/NoQuotaTest.java` around lines 88 - 94, The no-profile test currently only covers the existing max-builds and max-cases endpoints, leaving the new max-* accessors untested for the branch where a user has no associated profile. Update testUserCreationWithoutProfile in NoQuotaTest to also assert the new quota endpoints return empty results for USER_SUB, using the existing helper methods on the same test flow so this branch is fully covered.
117-185: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCollapse the quota assertions and endpoint wrappers into one table-driven helper.
Lines 124-136, 173-185, and 214-264 manually mirror every quota key and endpoint, and the same helper pattern is duplicated in
src/test/java/org/gridsuite/useradmin/server/UserAdminTest.java. With 13 quota dimensions now, this is getting expensive to keep in sync.Also applies to: 214-264
🤖 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/useradmin/server/NoQuotaTest.java` around lines 117 - 185, The quota checks in NoQuotaTest are duplicated across many individual assertions and endpoint helper methods, making them hard to maintain as quota dimensions grow. Refactor the repeated max-allowed assertions in createProfile and the per-quota endpoint wrappers into a single table-driven helper that iterates over quota keys and expected values, using the existing symbols createMaxAllowedCases, createProfile(String, Map<String, Integer>), and the getMaxAllowed* methods as the consolidation point. Mirror the same helper pattern in UserAdminTest so both test classes share the same maintainable structure instead of manually repeating each quota dimension.
🤖 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/useradmin/server/dto/UserProfile.java`:
- Around line 18-50: The UserProfile DTO change breaks the existing /profiles
JSON contract by exposing maxAllowValuesMap instead of the top-level maxAllowed*
fields. Update UserProfile so Jackson still serializes/deserializes the legacy
property names while keeping the internal map, using the UserProfile class and
createDefaultProfile builder path as the main locations to adjust. Preserve
backward compatibility with existing clients by adding aliases/accessors or
another compatibility layer rather than changing the payload shape outright.
In `@src/main/resources/db/changelog/changesets/changelog_20260616T125854Z.xml`:
- Around line 5-55: Remove the hard-coded defaultValue settings from the new
quota columns in the Liquibase changeset so the schema no longer assigns 1/2
automatically. Update the user_profile addColumn entries in
changelog_20260616T125854Z.xml to keep these fields nullable and let
UserProfileService.toEntity(...) apply UserAdminApplicationProps fallbacks when
values are absent. Ensure the affected changeSet entries for the max_allowed_*
columns preserve the config-driven behavior instead of persisting defaults at
the DB level.
---
Nitpick comments:
In `@src/test/java/org/gridsuite/useradmin/server/NoQuotaTest.java`:
- Around line 88-94: The no-profile test currently only covers the existing
max-builds and max-cases endpoints, leaving the new max-* accessors untested for
the branch where a user has no associated profile. Update
testUserCreationWithoutProfile in NoQuotaTest to also assert the new quota
endpoints return empty results for USER_SUB, using the existing helper methods
on the same test flow so this branch is fully covered.
- Around line 117-185: The quota checks in NoQuotaTest are duplicated across
many individual assertions and endpoint helper methods, making them hard to
maintain as quota dimensions grow. Refactor the repeated max-allowed assertions
in createProfile and the per-quota endpoint wrappers into a single table-driven
helper that iterates over quota keys and expected values, using the existing
symbols createMaxAllowedCases, createProfile(String, Map<String, Integer>), and
the getMaxAllowed* methods as the consolidation point. Mirror the same helper
pattern in UserAdminTest so both test classes share the same maintainable
structure instead of manually repeating each quota dimension.
In `@src/test/java/org/gridsuite/useradmin/server/UserProfileTest.java`:
- Around line 401-408: The round-trip verification in UserProfileTest is missing
the MAX_ALLOWED_CASES assertion, so a regression in the cases quota mapping
could slip through. Update the test’s updatedProfile checks to also compare the
MAX_ALLOWED_CASES entry from newData.getMaxAllowValuesMap() against
updatedProfile.getMaxAllowValuesMap(), alongside the existing MAX_ALLOWED_BUILD
assertion.
🪄 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: e66604c6-a68a-4240-95d4-66046e18881c
📒 Files selected for processing (10)
src/main/java/org/gridsuite/useradmin/server/dto/UserProfile.javasrc/main/java/org/gridsuite/useradmin/server/service/UserAdminService.javasrc/main/java/org/gridsuite/useradmin/server/service/UserProfileService.javasrc/main/resources/config/application.yamlsrc/main/resources/db/changelog/changesets/changelog_20260616T125854Z.xmlsrc/test/java/org/gridsuite/useradmin/server/NoQuotaTest.javasrc/test/java/org/gridsuite/useradmin/server/UserAdminTest.javasrc/test/java/org/gridsuite/useradmin/server/UserProfileTest.javasrc/test/resources/application-default.ymlsrc/test/resources/application-noquota.yml
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/org/gridsuite/useradmin/server/service/UserProfileService.java
- src/test/java/org/gridsuite/useradmin/server/UserAdminTest.java
- src/main/java/org/gridsuite/useradmin/server/service/UserAdminService.java
Comment on lines
+18
to
+50
| @Getter | ||
| @Builder | ||
| public final class UserProfile { | ||
| public static final String DEFAULT_PROFILE_NAME = "default profile"; | ||
| public static final String MAX_ALLOWED_CASES = "maxAllowedCases"; | ||
| public static final String MAX_ALLOWED_BUILD = "maxAllowedBuilds"; | ||
| public static final String MAX_ALLOWED_LOADFLOW = "maxAllowedLoadflow"; | ||
| public static final String MAX_ALLOWED_SECURITY = "maxAllowedSecurity"; | ||
| public static final String MAX_ALLOWED_SENSITIVITY = "maxAllowedSensitivity"; | ||
| public static final String MAX_ALLOWED_SHORT_CIRCUIT = "maxAllowedShortCircuit"; | ||
| public static final String MAX_ALLOWED_VOLTAGE_INIT = "maxAllowedVoltageInit"; | ||
| public static final String MAX_ALLOWED_PCC_MIN = "maxAllowedPccMin"; | ||
| public static final String MAX_ALLOWED_STATE_ESTIMATION = "maxAllowedStateEstimation"; | ||
| public static final String MAX_ALLOWED_BALANCE_ADJUSTEMENT = "maxAllowedBalanceAdjustement"; | ||
| public static final String MAX_ALLOWED_DYNAMIC_SIMULATION = "maxAllowedDynamicSimulation"; | ||
| public static final String MAX_ALLOWED_DYNAMIC_SECURITY = "maxAllowedDynamicSecurity"; | ||
| public static final String MAX_ALLOWED_DYNAMIC_MARGIN = "maxAllowedDynamicMargin"; | ||
| private final UUID id; | ||
| private final String name; | ||
| private final UUID loadFlowParameterId; | ||
| private final UUID securityAnalysisParameterId; | ||
| private final UUID sensitivityAnalysisParameterId; | ||
| private final UUID shortcircuitParameterId; | ||
| private final UUID pccMinParameterId; | ||
| private final UUID voltageInitParameterId; | ||
| private final Boolean allLinksValid; | ||
| private final Map<String, Integer> maxAllowValuesMap; | ||
| private final UUID spreadsheetConfigCollectionId; | ||
| private final UUID networkVisualizationParameterId; | ||
| private final UUID workspaceId; | ||
|
|
||
| public static UserProfile createDefaultProfile(Integer maxAllowedCases, Integer maxAllowedBuilds) { | ||
| return new UserProfile( | ||
| null, | ||
| DEFAULT_PROFILE_NAME, | ||
| null, | ||
| null, | ||
| null, | ||
| null, | ||
| null, | ||
| null, | ||
| null, | ||
| maxAllowedCases, | ||
| maxAllowedBuilds, | ||
| null, | ||
| null, | ||
| null | ||
| ); | ||
| public static UserProfile createDefaultProfile(Map<String, Integer> maxAllowValues) { | ||
| return UserProfile.builder().name(DEFAULT_PROFILE_NAME).maxAllowValuesMap(maxAllowValues).build(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
This breaks the existing /profiles JSON contract.
The DTO no longer exposes top-level maxAllowedCases / maxAllowedBuilds; callers now have to send and read maxAllowValuesMap instead. UserProfileTest was updated to post the new map shape, so existing clients of the current API version will break unless you keep backward-compatible aliases/accessors or version the endpoint.
🤖 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/useradmin/server/dto/UserProfile.java` around
lines 18 - 50, The UserProfile DTO change breaks the existing /profiles JSON
contract by exposing maxAllowValuesMap instead of the top-level maxAllowed*
fields. Update UserProfile so Jackson still serializes/deserializes the legacy
property names while keeping the internal map, using the UserProfile class and
createDefaultProfile builder path as the main locations to adjust. Preserve
backward compatibility with existing clients by adding aliases/accessors or
another compatibility layer rather than changing the payload shape outright.
Comment on lines
+5
to
+55
| <column name="max_allowed_loadflow" type="INTEGER" defaultValue="2"/> | ||
| </addColumn> | ||
| </changeSet> | ||
| <changeSet author="lesoteti (generated)" id="1781614744540-3"> | ||
| <addColumn tableName="user_profile"> | ||
| <column name="max_allowed_security" type="INTEGER" defaultValue="2"/> | ||
| </addColumn> | ||
| </changeSet> | ||
| <changeSet author="lesoteti (generated)" id="1781614744540-4"> | ||
| <addColumn tableName="user_profile"> | ||
| <column name="max_allowed_sensitivity" type="INTEGER" defaultValue="1"/> | ||
| </addColumn> | ||
| </changeSet> | ||
| <changeSet author="lesoteti (generated)" id="1781614744540-5"> | ||
| <addColumn tableName="user_profile"> | ||
| <column name="max_allowed_short_circuit" type="INTEGER" defaultValue="1"/> | ||
| </addColumn> | ||
| </changeSet> | ||
| <changeSet author="lesoteti (generated)" id="1781614744540-6"> | ||
| <addColumn tableName="user_profile"> | ||
| <column name="max_allowed_voltage_init" type="INTEGER" defaultValue="2"/> | ||
| </addColumn> | ||
| </changeSet> | ||
| <changeSet author="lesoteti (generated)" id="1781614744540-7"> | ||
| <addColumn tableName="user_profile"> | ||
| <column name="max_allowed_pcc_min" type="INTEGER" defaultValue="1"/> | ||
| </addColumn> | ||
| </changeSet> | ||
| <changeSet author="lesoteti (generated)" id="1781614744540-8"> | ||
| <addColumn tableName="user_profile"> | ||
| <column name="max_allowed_state_estimation" type="INTEGER" defaultValue="1"/> | ||
| </addColumn> | ||
| </changeSet> | ||
| <changeSet author="lesoteti (generated)" id="1781614744540-9"> | ||
| <addColumn tableName="user_profile"> | ||
| <column name="max_allowed_balance_adjustement" type="INTEGER" defaultValue="1"/> | ||
| </addColumn> | ||
| </changeSet> | ||
| <changeSet author="lesoteti (generated)" id="1781614744540-10"> | ||
| <addColumn tableName="user_profile"> | ||
| <column name="max_allowed_dynamic_simulation" type="INTEGER" defaultValue="1"/> | ||
| </addColumn> | ||
| </changeSet> | ||
| <changeSet author="lesoteti (generated)" id="1781614744540-11"> | ||
| <addColumn tableName="user_profile"> | ||
| <column name="max_allowed_dynamic_security" type="INTEGER" defaultValue="1"/> | ||
| </addColumn> | ||
| </changeSet> | ||
| <changeSet author="lesoteti (generated)" id="1781614744540-12"> | ||
| <addColumn tableName="user_profile"> | ||
| <column name="max_allowed_dynamic_margin" type="INTEGER" defaultValue="1"/> |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Remove the hard-coded DB defaults for these new quota columns.
These defaultValues bypass the config contract: application-noquota.yml makes the quotas nullable, and UserProfileService.toEntity(...) already applies UserAdminApplicationProps fallbacks when values are missing. Keeping 1/2 at the schema level will persist the wrong quotas for migrated rows and any insert path that omits these columns.
Suggested fix
- <column name="max_allowed_loadflow" type="INTEGER" defaultValue="2"/>
+ <column name="max_allowed_loadflow" type="INTEGER"/>
...
- <column name="max_allowed_dynamic_margin" type="INTEGER" defaultValue="1"/>
+ <column name="max_allowed_dynamic_margin" type="INTEGER"/>🤖 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/resources/db/changelog/changesets/changelog_20260616T125854Z.xml`
around lines 5 - 55, Remove the hard-coded defaultValue settings from the new
quota columns in the Liquibase changeset so the schema no longer assigns 1/2
automatically. Update the user_profile addColumn entries in
changelog_20260616T125854Z.xml to keep these fields nullable and let
UserProfileService.toEntity(...) apply UserAdminApplicationProps fallbacks when
values are absent. Ensure the affected changeSet entries for the max_allowed_*
columns preserve the config-driven behavior instead of persisting defaults at
the DB level.
Signed-off-by: Etienne LESOT <etienne.lesot@rte-france.com>
GhilesA
approved these changes
Jun 26, 2026
|
GhilesA
approved these changes
Jun 29, 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
add all quotas for each computation service