[RHCLOUD-46428] Filter drawer entries from event log and behavior group apis#4280
Open
g-duval wants to merge 6 commits intoRedHatInsights:masterfrom
Open
[RHCLOUD-46428] Filter drawer entries from event log and behavior group apis#4280g-duval wants to merge 6 commits intoRedHatInsights:masterfrom
g-duval wants to merge 6 commits intoRedHatInsights:masterfrom
Conversation
Contributor
SC Environment Impact AssessmentOverall Impact: 🟢 LOW View full reportSummary
Detailed Findings🟢 LOW ImpactFeature flag change detected
Required Actions
This assessment was automatically generated. Please review carefully and consult with the ROSA Core team for critical/high impact changes. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a feature-flagged branch to behavior-group queries and conditional filtering of drawer-type endpoints: repository queries may fetch endpoints and prune DRAWER actions based on a new Unleash toggle; event mapping also omits DRAWER history entries unless drawer is enabled. Tests and BackendConfig were updated. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/src/test/java/com/redhat/cloud/notifications/routers/handlers/event/EventResourceTest.java (1)
255-270: Note: Mock state persists for remaining tests in the method.The
when(backendConfig.isDrawerEnabled()).thenReturn(true)at line 255 persists for all subsequent tests (#2through#36) withintestAllQueryParams. This appears intentional since later tests (e.g., Test#4,#5, etc.) assert on drawer actions likehistory6andhistory7.However, consider adding a comment clarifying this is intentional behavior, or reset the mock at the end for clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/test/java/com/redhat/cloud/notifications/routers/handlers/event/EventResourceTest.java` around lines 255 - 270, The mock setting when(backendConfig.isDrawerEnabled()).thenReturn(true) in testAllQueryParams currently persists for subsequent tests; either add a clear inline comment next to that line stating the persistence is intentional for tests `#2`–#36, or explicitly reset/restore the mock after the block (e.g., Mockito.reset(backendConfig) or when(backendConfig.isDrawerEnabled()).thenReturn(false)) so later assertions remain unambiguous; reference the test method testAllQueryParams and the backendConfig.isDrawerEnabled() mock when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@backend/src/test/java/com/redhat/cloud/notifications/db/repositories/BehaviorGroupRepositoryTest.java`:
- Around line 815-824: The helper updateAndFetchBehaviorGroupActions currently
performs the update again, causing duplicate calls; change it to only fetch
actions (remove the call to behaviorGroupRepository.updateBehaviorGroupActions
from updateAndFetchBehaviorGroupActions) so updateAndCheckBehaviorGroupActions
remains the single place that calls
behaviorGroupRepository.updateBehaviorGroupActions, and ensure the helper only
queries and returns the List<BehaviorGroupAction> (keeping entityManager.clear()
before the fetch if needed); update any references or rename the helper to
reflect it is a fetch-only method.
---
Nitpick comments:
In
`@backend/src/test/java/com/redhat/cloud/notifications/routers/handlers/event/EventResourceTest.java`:
- Around line 255-270: The mock setting
when(backendConfig.isDrawerEnabled()).thenReturn(true) in testAllQueryParams
currently persists for subsequent tests; either add a clear inline comment next
to that line stating the persistence is intentional for tests `#2`–#36, or
explicitly reset/restore the mock after the block (e.g.,
Mockito.reset(backendConfig) or
when(backendConfig.isDrawerEnabled()).thenReturn(false)) so later assertions
remain unambiguous; reference the test method testAllQueryParams and the
backendConfig.isDrawerEnabled() mock when making the change.
🪄 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: 01a4759f-54c9-4c48-8b24-c207f28a2264
📒 Files selected for processing (4)
backend/src/main/java/com/redhat/cloud/notifications/db/repositories/BehaviorGroupRepository.javabackend/src/main/java/com/redhat/cloud/notifications/routers/handlers/event/EventResource.javabackend/src/test/java/com/redhat/cloud/notifications/db/repositories/BehaviorGroupRepositoryTest.javabackend/src/test/java/com/redhat/cloud/notifications/routers/handlers/event/EventResourceTest.java
g-duval
force-pushed
the
RHCLOUD-46428_hideDrawerFromBackend
branch
from
b779c3a to
d7165e5
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/src/main/java/com/redhat/cloud/notifications/routers/handlers/event/EventResource.java (1)
160-175: Filter logic is correct; consider hoisting config check outside the stream.The filter condition correctly excludes DRAWER entries when the feature is disabled. However,
backendConfig.isDrawerEnabled()is evaluated on every history entry despite being invariant within the request. For readability and minor efficiency, consider evaluating it once before the stream.♻️ Suggested refactor
} else { + boolean drawerEnabled = backendConfig.isDrawerEnabled(); actions = event.getHistoryEntries().stream() - .filter(notificationHistory -> EndpointType.DRAWER != notificationHistory.getEndpointType() || backendConfig.isDrawerEnabled()) + .filter(notificationHistory -> EndpointType.DRAWER != notificationHistory.getEndpointType() || drawerEnabled) .map(historyEntry -> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/com/redhat/cloud/notifications/routers/handlers/event/EventResource.java` around lines 160 - 175, The stream filter is calling backendConfig.isDrawerEnabled() for every history entry; hoist that invariant out of the stream by evaluating boolean drawerEnabled = backendConfig.isDrawerEnabled() before mapping event.getHistoryEntries() and then use EndpointType.DRAWER != notificationHistory.getEndpointType() || drawerEnabled in the filter so the logic (in EventResource, actions construction) remains the same but avoids repeated calls for each historyEntry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@backend/src/main/java/com/redhat/cloud/notifications/routers/handlers/event/EventResource.java`:
- Around line 160-175: The stream filter is calling
backendConfig.isDrawerEnabled() for every history entry; hoist that invariant
out of the stream by evaluating boolean drawerEnabled =
backendConfig.isDrawerEnabled() before mapping event.getHistoryEntries() and
then use EndpointType.DRAWER != notificationHistory.getEndpointType() ||
drawerEnabled in the filter so the logic (in EventResource, actions
construction) remains the same but avoids repeated calls for each historyEntry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a1b27b0d-16b4-4bb0-9915-6bf12351f635
📒 Files selected for processing (4)
backend/src/main/java/com/redhat/cloud/notifications/db/repositories/BehaviorGroupRepository.javabackend/src/main/java/com/redhat/cloud/notifications/routers/handlers/event/EventResource.javabackend/src/test/java/com/redhat/cloud/notifications/db/repositories/BehaviorGroupRepositoryTest.javabackend/src/test/java/com/redhat/cloud/notifications/routers/handlers/event/EventResourceTest.java
✅ Files skipped from review due to trivial changes (1)
- backend/src/test/java/com/redhat/cloud/notifications/routers/handlers/event/EventResourceTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/src/main/java/com/redhat/cloud/notifications/db/repositories/BehaviorGroupRepository.java
- backend/src/test/java/com/redhat/cloud/notifications/db/repositories/BehaviorGroupRepositoryTest.java
gwenneg
reviewed
Apr 2, 2026
Comment on lines
+184
to
+189
| behaviorGroup.setActions( | ||
| behaviorGroup.getActions().stream() | ||
| .filter(action -> action.getEndpoint() == null | ||
| || EndpointType.DRAWER != action.getEndpoint().getType()) | ||
| .collect(Collectors.toList()) | ||
| ); |
Member
There was a problem hiding this comment.
Suggested change
| behaviorGroup.setActions( | |
| behaviorGroup.getActions().stream() | |
| .filter(action -> action.getEndpoint() == null | |
| || EndpointType.DRAWER != action.getEndpoint().getType()) | |
| .collect(Collectors.toList()) | |
| ); | |
| behaviorGroup.getActions().removeIf(action -> | |
| action.getEndpoint() != null && EndpointType.DRAWER == action.getEndpoint().getType()); |
Contributor
Author
There was a problem hiding this comment.
Tested, but actually we can't, because if we alter the collection, Hibernate will consider it as "dirty" state and needs to persit it.
Creating a new collection does not trigger this Hibernate behavior.
| * When PostgreSQL sorts a BOOLEAN column in DESC order, true comes first. That's not true for all DBMS. | ||
| */ | ||
| String query = "SELECT DISTINCT b FROM BehaviorGroup b LEFT JOIN FETCH b.actions a " + | ||
| "LEFT JOIN FETCH a.endpoint e " + |
Member
There was a problem hiding this comment.
From a DB perf perspective, it would be a bit better to have 2 distinct SQL queries (old one unchanged, new one that filters out drawer endpoint) and switch between them based on the feature toggle. This would remove the need for fetching endpoints data.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/src/test/java/com/redhat/cloud/notifications/db/repositories/BehaviorGroupRepositoryTest.java (1)
389-392: Make the drawer-disabled assumption explicit in this test.Line 391 currently relies on Mockito’s default
falseforbackendConfig.isDrawerEnabled(). Stubbing it explicitly makes intent and test stability clearer.Proposed test tweak
when(backendConfig.isUseDrawerfilteredQuery(eq(DEFAULT_ORG_ID))).thenReturn(true); + when(backendConfig.isDrawerEnabled()).thenReturn(false); bgActions = updateAndFetchBehaviorGroupActions(DEFAULT_ORG_ID, bundle.getId(), behaviorGroup.getId(), endpoint1.getId(), endpoint2.getId()); assertTrue(bgActions.isEmpty()); when(backendConfig.isDrawerEnabled()).thenReturn(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/test/java/com/redhat/cloud/notifications/db/repositories/BehaviorGroupRepositoryTest.java` around lines 389 - 392, The test currently relies on Mockito’s default false for backendConfig.isDrawerEnabled(); make this explicit by stubbing backendConfig.isDrawerEnabled() to return false before calling updateAndFetchBehaviorGroupActions(DEFAULT_ORG_ID, bundle.getId(), behaviorGroup.getId(), endpoint1.getId(), endpoint2.getId()) so that the behavior of isUseDrawerfilteredQuery(DEFAULT_ORG_ID) and the resulting bgActions assertion are deterministic and self-documenting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@backend/src/test/java/com/redhat/cloud/notifications/db/repositories/BehaviorGroupRepositoryTest.java`:
- Around line 389-392: The test currently relies on Mockito’s default false for
backendConfig.isDrawerEnabled(); make this explicit by stubbing
backendConfig.isDrawerEnabled() to return false before calling
updateAndFetchBehaviorGroupActions(DEFAULT_ORG_ID, bundle.getId(),
behaviorGroup.getId(), endpoint1.getId(), endpoint2.getId()) so that the
behavior of isUseDrawerfilteredQuery(DEFAULT_ORG_ID) and the resulting bgActions
assertion are deterministic and self-documenting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 92125ac9-4053-466f-ae63-5cabc17b219f
📒 Files selected for processing (3)
backend/src/main/java/com/redhat/cloud/notifications/config/BackendConfig.javabackend/src/main/java/com/redhat/cloud/notifications/db/repositories/BehaviorGroupRepository.javabackend/src/test/java/com/redhat/cloud/notifications/db/repositories/BehaviorGroupRepositoryTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/main/java/com/redhat/cloud/notifications/db/repositories/BehaviorGroupRepository.java
g-duval
force-pushed
the
RHCLOUD-46428_hideDrawerFromBackend
branch
from
0138aed to
fedb03d
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/src/main/java/com/redhat/cloud/notifications/config/BackendConfig.java (1)
54-54: Naming inconsistency:useDrawerfilteredQueryshould use proper camelCase.The field name has inconsistent casing -
filteredshould beFilteredto follow camelCase convention. This also affects the method name at line 326.✏️ Suggested fix
- private String useDrawerfilteredQuery; + private String useDrawerFilteredQuery;And update the corresponding method name:
- public boolean isUseDrawerfilteredQuery(String orgId) { + public boolean isUseDrawerFilteredQuery(String orgId) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/com/redhat/cloud/notifications/config/BackendConfig.java` at line 54, The field name useDrawerfilteredQuery violates camelCase; rename the field to useDrawerFilteredQuery and update all usages and accessors accordingly (e.g., rename any getter/setter methods such as getUseDrawerfilteredQuery/setUseDrawerfilteredQuery to getUseDrawerFilteredQuery/setUseDrawerFilteredQuery and adjust references in BackendConfig and consumers); ensure serialization/deserialization annotations (if any) and tests are updated to the new name so behavior remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@backend/src/main/java/com/redhat/cloud/notifications/config/BackendConfig.java`:
- Line 54: The field name useDrawerfilteredQuery violates camelCase; rename the
field to useDrawerFilteredQuery and update all usages and accessors accordingly
(e.g., rename any getter/setter methods such as
getUseDrawerfilteredQuery/setUseDrawerfilteredQuery to
getUseDrawerFilteredQuery/setUseDrawerFilteredQuery and adjust references in
BackendConfig and consumers); ensure serialization/deserialization annotations
(if any) and tests are updated to the new name so behavior remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 07424a37-cf1f-47f9-8a5f-d04445a7beb0
📒 Files selected for processing (5)
backend/src/main/java/com/redhat/cloud/notifications/config/BackendConfig.javabackend/src/main/java/com/redhat/cloud/notifications/db/repositories/BehaviorGroupRepository.javabackend/src/main/java/com/redhat/cloud/notifications/routers/handlers/event/EventResource.javabackend/src/test/java/com/redhat/cloud/notifications/db/repositories/BehaviorGroupRepositoryTest.javabackend/src/test/java/com/redhat/cloud/notifications/routers/handlers/event/EventResourceTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/main/java/com/redhat/cloud/notifications/routers/handlers/event/EventResource.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@backend/src/test/java/com/redhat/cloud/notifications/db/repositories/BehaviorGroupRepositoryTest.java`:
- Around line 831-837: The test currently hides a missing BehaviorGroup by
returning an empty list; change the behavior to fail fast: after calling
behaviorGroupRepository.findByBundleId(orgId, bundleId) and filtering for
behaviorGroupId, if the resulting list is empty, throw an assertion or explicit
exception (e.g., Assertions.fail or NoSuchElementException) instead of returning
new ArrayList<>() so that missing groups cause the test to fail; then return
behaviorGroups.get(0).getActions() as before.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: db5888c1-01d1-4387-a3bc-9c709d9da63f
📒 Files selected for processing (4)
backend/src/main/java/com/redhat/cloud/notifications/config/BackendConfig.javabackend/src/main/java/com/redhat/cloud/notifications/db/repositories/BehaviorGroupRepository.javabackend/src/main/java/com/redhat/cloud/notifications/routers/handlers/event/EventResource.javabackend/src/test/java/com/redhat/cloud/notifications/db/repositories/BehaviorGroupRepositoryTest.java
✅ Files skipped from review due to trivial changes (1)
- backend/src/main/java/com/redhat/cloud/notifications/routers/handlers/event/EventResource.java
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/src/main/java/com/redhat/cloud/notifications/config/BackendConfig.java
- backend/src/main/java/com/redhat/cloud/notifications/db/repositories/BehaviorGroupRepository.java
Comment on lines
+831
to
+837
| final List<BehaviorGroup> behaviorGroups = behaviorGroupRepository.findByBundleId(orgId, bundleId) | ||
| .stream().filter(behaviorGroup -> behaviorGroup.getId().equals(behaviorGroupId)).toList(); | ||
| if (behaviorGroups.isEmpty()) { | ||
| return new ArrayList<>(); | ||
| } else { | ||
| return behaviorGroups.get(0).getActions(); | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid silently returning empty when the behavior group is missing.
Returning new ArrayList<>() here can hide real regressions (missing behavior group) as a “valid empty actions” case. Fail fast when the group isn’t found.
Suggested change
private List<BehaviorGroupAction> findBehaviorGroupActions(String orgId, UUID bundleId, UUID behaviorGroupId) {
- final List<BehaviorGroup> behaviorGroups = behaviorGroupRepository.findByBundleId(orgId, bundleId)
- .stream().filter(behaviorGroup -> behaviorGroup.getId().equals(behaviorGroupId)).toList();
- if (behaviorGroups.isEmpty()) {
- return new ArrayList<>();
- } else {
- return behaviorGroups.get(0).getActions();
- }
+ return behaviorGroupRepository.findByBundleId(orgId, bundleId).stream()
+ .filter(behaviorGroup -> behaviorGroup.getId().equals(behaviorGroupId))
+ .findFirst()
+ .map(BehaviorGroup::getActions)
+ .orElseThrow(() -> new AssertionError(
+ "Expected behavior group " + behaviorGroupId + " in bundle " + bundleId + " for org " + orgId
+ ));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| final List<BehaviorGroup> behaviorGroups = behaviorGroupRepository.findByBundleId(orgId, bundleId) | |
| .stream().filter(behaviorGroup -> behaviorGroup.getId().equals(behaviorGroupId)).toList(); | |
| if (behaviorGroups.isEmpty()) { | |
| return new ArrayList<>(); | |
| } else { | |
| return behaviorGroups.get(0).getActions(); | |
| } | |
| private List<BehaviorGroupAction> findBehaviorGroupActions(String orgId, UUID bundleId, UUID behaviorGroupId) { | |
| return behaviorGroupRepository.findByBundleId(orgId, bundleId).stream() | |
| .filter(behaviorGroup -> behaviorGroup.getId().equals(behaviorGroupId)) | |
| .findFirst() | |
| .map(BehaviorGroup::getActions) | |
| .orElseThrow(() -> new AssertionError( | |
| "Expected behavior group " + behaviorGroupId + " in bundle " + bundleId + " for org " + orgId | |
| )); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@backend/src/test/java/com/redhat/cloud/notifications/db/repositories/BehaviorGroupRepositoryTest.java`
around lines 831 - 837, The test currently hides a missing BehaviorGroup by
returning an empty list; change the behavior to fail fast: after calling
behaviorGroupRepository.findByBundleId(orgId, bundleId) and filtering for
behaviorGroupId, if the resulting list is empty, throw an assertion or explicit
exception (e.g., Assertions.fail or NoSuchElementException) instead of returning
new ArrayList<>() so that missing groups cause the test to fail; then return
behaviorGroups.get(0).getActions() as before.
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.
We want to controle Drawer endpoint visibility from backend event if some integrations/System behavior groups/ notifications history exists.
Summary by CodeRabbit
New Features
Tests