[NAE 2441] Event permissions test#449
Open
SamuelPalaj wants to merge 4 commits into
Open
Conversation
- rework of case search query building in CaseSearchService - removed mongo search endpoints mongo_search and search2 from WorkflowController - added tests and test nets for create, delete and view case permissions in CasePermissionsTest - added tests and test nets for assign, cancel and delete task permissions in TaskPermissionsTest
|
Warning Review limit reached
More reviews will be available in 56 minutes and 35 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThis PR simplifies and consolidates permission query construction and domain permission resolution, removes two legacy case-search endpoints, adjusts importer default-role handling, and adds comprehensive case and task permission integration tests with many Petri net fixtures. ChangesPermission Logic Simplification and Query Refactoring
🎯 4 (Complex) | ⏱️ ~60 minutes
|
coderabbitai
Bot
added
improvement
![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
🤖 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/groovy/com/netgrif/application/engine/workflow/CasePermissionsTest.groovy`:
- Line 311: The FileInputStream created inline in the PetriNet import call is
never closed; update the code in CasePermissionsTest so that the stream passed
to petriNetService.importPetriNet is opened into a local InputStream variable
and closed reliably (use try-with-resources or try/finally) around the call to
petriNetService.importPetriNet(...) so each testNet file's stream is closed
after import; reference the petriNetService.importPetriNet(...) invocation and
ensure the InputStream is closed even when import throws.
In
`@src/test/groovy/com/netgrif/application/engine/workflow/TaskPermissionsTest.groovy`:
- Line 455: The current assertion in TaskPermissionsTest.groovy uses a compound
boolean that obscures which size comparison failed; change it to clearer
assertions by either replacing the single assertTrue(...) with two assertEquals
calls comparing presentInBoth.size() to actualTransitionIds.size() and
presentInBoth.size() to expectedTransitionIds.size(), or keep a single assertion
but add a descriptive message that states the expected and actual sizes
(referencing presentInBoth, actualTransitionIds, expectedTransitionIds) so
failures indicate which comparison broke.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 92b9850e-8337-43a9-86ea-d2f6d10d66d1
⛔ Files ignored due to path filters (4)
src/test/resources/csv/create case permissions - correct default disabled.csvis excluded by!**/*.csvsrc/test/resources/csv/create case permissions - correct.csvis excluded by!**/*.csvsrc/test/resources/csv/permissions - correct default disabled.csvis excluded by!**/*.csvsrc/test/resources/csv/permissions - correct.csvis excluded by!**/*.csv
📒 Files selected for processing (104)
src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskService.javasrc/main/java/com/netgrif/application/engine/elastic/service/ElasticViewPermissionService.javasrc/main/java/com/netgrif/application/engine/importer/service/Importer.javasrc/main/java/com/netgrif/application/engine/workflow/domain/Case.javasrc/main/java/com/netgrif/application/engine/workflow/domain/Task.javasrc/main/java/com/netgrif/application/engine/workflow/service/CaseSearchService.javasrc/main/java/com/netgrif/application/engine/workflow/service/TaskSearchService.javasrc/main/java/com/netgrif/application/engine/workflow/service/TaskService.javasrc/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.javasrc/main/java/com/netgrif/application/engine/workflow/web/WorkflowController.javasrc/test/groovy/com/netgrif/application/engine/workflow/CasePermissionsTest.groovysrc/test/groovy/com/netgrif/application/engine/workflow/TaskPermissionsTest.groovysrc/test/resources/petriNets/permissions/assign_permission_combinations.xmlsrc/test/resources/petriNets/permissions/assign_permission_combinations_no_default.xmlsrc/test/resources/petriNets/permissions/cancel_permission_combinations.xmlsrc/test/resources/petriNets/permissions/cancel_permission_combinations_no_default.xmlsrc/test/resources/petriNets/permissions/case/default/create/create_case_role_false_default_false.xmlsrc/test/resources/petriNets/permissions/case/default/create/create_case_role_false_default_true.xmlsrc/test/resources/petriNets/permissions/case/default/create/create_case_role_false_default_undefined.xmlsrc/test/resources/petriNets/permissions/case/default/create/create_case_role_true_default_false.xmlsrc/test/resources/petriNets/permissions/case/default/create/create_case_role_true_default_true.xmlsrc/test/resources/petriNets/permissions/case/default/create/create_case_role_true_default_undefined.xmlsrc/test/resources/petriNets/permissions/case/default/create/create_case_role_undefined_default_false.xmlsrc/test/resources/petriNets/permissions/case/default/create/create_case_role_undefined_default_true.xmlsrc/test/resources/petriNets/permissions/case/default/create/create_case_role_undefined_default_undefined.xmlsrc/test/resources/petriNets/permissions/case/default/delete/delete_case_role_false_default_false_user_false.xmlsrc/test/resources/petriNets/permissions/case/default/delete/delete_case_role_false_default_false_user_true.xmlsrc/test/resources/petriNets/permissions/case/default/delete/delete_case_role_false_default_false_user_undefined.xmlsrc/test/resources/petriNets/permissions/case/default/delete/delete_case_role_false_default_true_user_false.xmlsrc/test/resources/petriNets/permissions/case/default/delete/delete_case_role_false_default_true_user_true.xmlsrc/test/resources/petriNets/permissions/case/default/delete/delete_case_role_false_default_true_user_undefined.xmlsrc/test/resources/petriNets/permissions/case/default/delete/delete_case_role_false_default_undefined_user_false.xmlsrc/test/resources/petriNets/permissions/case/default/delete/delete_case_role_false_default_undefined_user_true.xmlsrc/test/resources/petriNets/permissions/case/default/delete/delete_case_role_false_default_undefined_user_undefined.xmlsrc/test/resources/petriNets/permissions/case/default/delete/delete_case_role_true_default_false_user_false.xmlsrc/test/resources/petriNets/permissions/case/default/delete/delete_case_role_true_default_false_user_true.xmlsrc/test/resources/petriNets/permissions/case/default/delete/delete_case_role_true_default_false_user_undefined.xmlsrc/test/resources/petriNets/permissions/case/default/delete/delete_case_role_true_default_true_user_false.xmlsrc/test/resources/petriNets/permissions/case/default/delete/delete_case_role_true_default_true_user_true.xmlsrc/test/resources/petriNets/permissions/case/default/delete/delete_case_role_true_default_true_user_undefined.xmlsrc/test/resources/petriNets/permissions/case/default/delete/delete_case_role_true_default_undefined_user_false.xmlsrc/test/resources/petriNets/permissions/case/default/delete/delete_case_role_true_default_undefined_user_true.xmlsrc/test/resources/petriNets/permissions/case/default/delete/delete_case_role_true_default_undefined_user_undefined.xmlsrc/test/resources/petriNets/permissions/case/default/delete/delete_case_role_undefined_default_false_user_false.xmlsrc/test/resources/petriNets/permissions/case/default/delete/delete_case_role_undefined_default_false_user_true.xmlsrc/test/resources/petriNets/permissions/case/default/delete/delete_case_role_undefined_default_false_user_undefined.xmlsrc/test/resources/petriNets/permissions/case/default/delete/delete_case_role_undefined_default_true_user_false.xmlsrc/test/resources/petriNets/permissions/case/default/delete/delete_case_role_undefined_default_true_user_true.xmlsrc/test/resources/petriNets/permissions/case/default/delete/delete_case_role_undefined_default_true_user_undefined.xmlsrc/test/resources/petriNets/permissions/case/default/delete/delete_case_role_undefined_default_undefined_user_false.xmlsrc/test/resources/petriNets/permissions/case/default/delete/delete_case_role_undefined_default_undefined_user_true.xmlsrc/test/resources/petriNets/permissions/case/default/delete/delete_case_role_undefined_default_undefined_user_undefined.xmlsrc/test/resources/petriNets/permissions/case/default/view/view_case_role_false_default_false_user_false.xmlsrc/test/resources/petriNets/permissions/case/default/view/view_case_role_false_default_false_user_true.xmlsrc/test/resources/petriNets/permissions/case/default/view/view_case_role_false_default_false_user_undefined.xmlsrc/test/resources/petriNets/permissions/case/default/view/view_case_role_false_default_true_user_false.xmlsrc/test/resources/petriNets/permissions/case/default/view/view_case_role_false_default_true_user_true.xmlsrc/test/resources/petriNets/permissions/case/default/view/view_case_role_false_default_true_user_undefined.xmlsrc/test/resources/petriNets/permissions/case/default/view/view_case_role_false_default_undefined_user_false.xmlsrc/test/resources/petriNets/permissions/case/default/view/view_case_role_false_default_undefined_user_true.xmlsrc/test/resources/petriNets/permissions/case/default/view/view_case_role_false_default_undefined_user_undefined.xmlsrc/test/resources/petriNets/permissions/case/default/view/view_case_role_true_default_false_user_false.xmlsrc/test/resources/petriNets/permissions/case/default/view/view_case_role_true_default_false_user_true.xmlsrc/test/resources/petriNets/permissions/case/default/view/view_case_role_true_default_false_user_undefined.xmlsrc/test/resources/petriNets/permissions/case/default/view/view_case_role_true_default_true_user_false.xmlsrc/test/resources/petriNets/permissions/case/default/view/view_case_role_true_default_true_user_true.xmlsrc/test/resources/petriNets/permissions/case/default/view/view_case_role_true_default_true_user_undefined.xmlsrc/test/resources/petriNets/permissions/case/default/view/view_case_role_true_default_undefined_user_false.xmlsrc/test/resources/petriNets/permissions/case/default/view/view_case_role_true_default_undefined_user_true.xmlsrc/test/resources/petriNets/permissions/case/default/view/view_case_role_true_default_undefined_user_undefined.xmlsrc/test/resources/petriNets/permissions/case/default/view/view_case_role_undefined_default_false_user_false.xmlsrc/test/resources/petriNets/permissions/case/default/view/view_case_role_undefined_default_false_user_true.xmlsrc/test/resources/petriNets/permissions/case/default/view/view_case_role_undefined_default_false_user_undefined.xmlsrc/test/resources/petriNets/permissions/case/default/view/view_case_role_undefined_default_true_user_false.xmlsrc/test/resources/petriNets/permissions/case/default/view/view_case_role_undefined_default_true_user_true.xmlsrc/test/resources/petriNets/permissions/case/default/view/view_case_role_undefined_default_true_user_undefined.xmlsrc/test/resources/petriNets/permissions/case/default/view/view_case_role_undefined_default_undefined_user_false.xmlsrc/test/resources/petriNets/permissions/case/default/view/view_case_role_undefined_default_undefined_user_true.xmlsrc/test/resources/petriNets/permissions/case/default/view/view_case_role_undefined_default_undefined_user_undefined.xmlsrc/test/resources/petriNets/permissions/case/defaultDisabled/create/create_case_role_false.xmlsrc/test/resources/petriNets/permissions/case/defaultDisabled/create/create_case_role_true.xmlsrc/test/resources/petriNets/permissions/case/defaultDisabled/create/create_case_role_undefined.xmlsrc/test/resources/petriNets/permissions/case/defaultDisabled/delete/delete_case_role_false_user_false.xmlsrc/test/resources/petriNets/permissions/case/defaultDisabled/delete/delete_case_role_false_user_true.xmlsrc/test/resources/petriNets/permissions/case/defaultDisabled/delete/delete_case_role_false_user_undefined.xmlsrc/test/resources/petriNets/permissions/case/defaultDisabled/delete/delete_case_role_true_user_false.xmlsrc/test/resources/petriNets/permissions/case/defaultDisabled/delete/delete_case_role_true_user_true.xmlsrc/test/resources/petriNets/permissions/case/defaultDisabled/delete/delete_case_role_true_user_undefined.xmlsrc/test/resources/petriNets/permissions/case/defaultDisabled/delete/delete_case_role_undefined_user_false.xmlsrc/test/resources/petriNets/permissions/case/defaultDisabled/delete/delete_case_role_undefined_user_true.xmlsrc/test/resources/petriNets/permissions/case/defaultDisabled/delete/delete_case_role_undefined_user_undefined.xmlsrc/test/resources/petriNets/permissions/case/defaultDisabled/view/view_case_role_false_user_false.xmlsrc/test/resources/petriNets/permissions/case/defaultDisabled/view/view_case_role_false_user_true.xmlsrc/test/resources/petriNets/permissions/case/defaultDisabled/view/view_case_role_false_user_undefined.xmlsrc/test/resources/petriNets/permissions/case/defaultDisabled/view/view_case_role_true_user_false.xmlsrc/test/resources/petriNets/permissions/case/defaultDisabled/view/view_case_role_true_user_true.xmlsrc/test/resources/petriNets/permissions/case/defaultDisabled/view/view_case_role_true_user_undefined.xmlsrc/test/resources/petriNets/permissions/case/defaultDisabled/view/view_case_role_undefined_user_false.xmlsrc/test/resources/petriNets/permissions/case/defaultDisabled/view/view_case_role_undefined_user_true.xmlsrc/test/resources/petriNets/permissions/case/defaultDisabled/view/view_case_role_undefined_user_undefined.xmlsrc/test/resources/petriNets/permissions/finish_permission_combinations.xmlsrc/test/resources/petriNets/permissions/finish_permission_combinations_no_default.xmlsrc/test/resources/petriNets/permissions/view_permission_combinations.xmlsrc/test/resources/petriNets/permissions/view_permission_combinations_no_default.xml
💤 Files with no reviewable changes (2)
- src/main/java/com/netgrif/application/engine/workflow/web/WorkflowController.java
- src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskService.java
| println("Present only in test results (${presentOnlyInMap.size()}): ${presentOnlyInMap}") | ||
| println("Present only in correct results (${presentOnlyInCorrectResultsWithDefaultRoleMap.size()}): ${presentOnlyInCorrectResultsWithDefaultRoleMap}") | ||
|
|
||
| assertTrue(presentInBoth.size() == actualTransitionIds.size() && presentInBoth.size() == expectedTransitionIds.size()) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider adding a descriptive assertion message.
The compound boolean condition makes it harder to diagnose failures. Consider using separate assertEquals calls or adding a descriptive message to explain which comparison failed.
♻️ Proposed improvement
- assertTrue(presentInBoth.size() == actualTransitionIds.size() && presentInBoth.size() == expectedTransitionIds.size())
+ assertEquals(expectedTransitionIds.size(), actualTransitionIds.size(),
+ "Mismatch in transition count for user ${userEmail} ${defaultRoleKey}: expected ${expectedTransitionIds.size()}, got ${actualTransitionIds.size()}")
+ assertEquals(expectedTransitionIds, actualTransitionIds,
+ "Transition ID mismatch for user ${userEmail} ${defaultRoleKey}")🤖 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/groovy/com/netgrif/application/engine/workflow/TaskPermissionsTest.groovy`
at line 455, The current assertion in TaskPermissionsTest.groovy uses a compound
boolean that obscures which size comparison failed; change it to clearer
assertions by either replacing the single assertTrue(...) with two assertEquals
calls comparing presentInBoth.size() to actualTransitionIds.size() and
presentInBoth.size() to expectedTransitionIds.size(), or keep a single assertion
but add a descriptive message that states the expected and actual sizes
(referencing presentInBoth, actualTransitionIds, expectedTransitionIds) so
failures indicate which comparison broke.
- stream closing fix
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/test/groovy/com/netgrif/application/engine/workflow/CasePermissionsTest.groovy (2)
327-329:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd null check to prevent cryptic NPE when role is missing.
If a test Petri net doesn't define a role with
importId == "process_role",findreturnsnulland the subsequent.stringIdcall throws an NPE without context.🛡️ Proposed fix
void assignRoleToUser(PetriNet net, IUser user) { - testUsers.put(user.email, userService.addRole(user, net.roles.values().find { role -> role.importId == "process_role" }.stringId)) + def role = net.roles.values().find { it.importId == "process_role" } + if (role == null) { + throw new IllegalStateException("PetriNet '${net.identifier}' is missing required role with importId='process_role'") + } + testUsers.put(user.email, userService.addRole(user, role.stringId)) }🤖 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/groovy/com/netgrif/application/engine/workflow/CasePermissionsTest.groovy` around lines 327 - 329, The method assignRoleToUser currently calls net.roles.values().find { role -> role.importId == "process_role" }.stringId which will NPE if no role is found; modify assignRoleToUser to first assign the result of the find to a local variable (e.g., foundRole), check for null, and either throw an informative IllegalStateException (or IllegalArgumentException) naming the missing "process_role" and the PetriNet id/name, or handle the missing role gracefully before calling userService.addRole and putting into testUsers; update references to use foundRole.stringId only after the null check.
440-440:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd descriptive assertion message for easier debugging.
When this assertion fails in CI, there's no indication of which user or scenario failed. The diagnostic
printlnstatements above help, but the assertion message itself should include context.🔧 Proposed fix
- assertTrue(presentInBoth.size() == actualNetIdentifiersIds.size() && presentInBoth.size() == expectedNetIdentifiers.size()) + assertTrue( + presentInBoth.size() == actualNetIdentifiersIds.size() && presentInBoth.size() == expectedNetIdentifiers.size(), + "Mismatch for user '${userEmail}' [${defaultRoleKey}]: unexpected=${presentOnlyInMap}, missing=${presentOnlyInCorrectResultsWithDefaultRoleMap}" + )🤖 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/groovy/com/netgrif/application/engine/workflow/CasePermissionsTest.groovy` at line 440, Replace the bare assertion on the three collections with one that includes a descriptive failure message: update the assertion that currently references presentInBoth, actualNetIdentifiersIds and expectedNetIdentifiers to include context (e.g., the tested user/role or test scenario) and the sizes/contents of those collections so CI failures show which user/scenario and what values mismatched; locate the assertion line using the variables presentInBoth, actualNetIdentifiersIds, and expectedNetIdentifiers in CasePermissionsTest and augment it with a clear message that prints the relevant identifiers and sizes.
🤖 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.
Outside diff comments:
In
`@src/test/groovy/com/netgrif/application/engine/workflow/CasePermissionsTest.groovy`:
- Around line 327-329: The method assignRoleToUser currently calls
net.roles.values().find { role -> role.importId == "process_role" }.stringId
which will NPE if no role is found; modify assignRoleToUser to first assign the
result of the find to a local variable (e.g., foundRole), check for null, and
either throw an informative IllegalStateException (or IllegalArgumentException)
naming the missing "process_role" and the PetriNet id/name, or handle the
missing role gracefully before calling userService.addRole and putting into
testUsers; update references to use foundRole.stringId only after the null
check.
- Line 440: Replace the bare assertion on the three collections with one that
includes a descriptive failure message: update the assertion that currently
references presentInBoth, actualNetIdentifiersIds and expectedNetIdentifiers to
include context (e.g., the tested user/role or test scenario) and the
sizes/contents of those collections so CI failures show which user/scenario and
what values mismatched; locate the assertion line using the variables
presentInBoth, actualNetIdentifiersIds, and expectedNetIdentifiers in
CasePermissionsTest and augment it with a clear message that prints the relevant
identifiers and sizes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4240f34a-643e-4fc0-a63f-506aabc4df7a
📒 Files selected for processing (1)
src/test/groovy/com/netgrif/application/engine/workflow/CasePermissionsTest.groovy
Contributor
|
there are test failures |
mazarijuraj
previously approved these changes
Jun 5, 2026
SamuelPalaj
dismissed
mazarijuraj’s stale review
The merge-base changed after approval.
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
3 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.
Description
Implements NAE-2441
Blocking Pull requests
Depends on #442
How Has Been This Tested?
Tests included in test files TaskPermissionsTest and CasePermissionsTest
Checklist:
Summary by CodeRabbit
Removals
Behavior Changes
Tests & Fixtures