[RHCLOUD-35781] Drawer feature enabling according to org#4307
Merged
g-duval merged 3 commits intoRedHatInsights:masterfrom Apr 8, 2026
Merged
Conversation
added 2 commits
April 2, 2026 16:56
…over changes Assisted-by: Claude Sonnet 4.5 (via Claude Code)
Assisted-by: Claude Sonnet 4.5 (via Claude Code)
Contributor
📝 WalkthroughWalkthroughDrawer enablement checks were made organization-aware by adding an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 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)
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 |
Contributor
SC Environment Impact AssessmentOverall Impact: 🟢 LOW View full reportSummary
Detailed Findings🟢 LOW ImpactFeature flag change detected
Feature 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 (1)
backend/src/main/java/com/redhat/cloud/notifications/routers/handlers/drawer/DrawerResource.java (1)
77-82:⚠️ Potential issue | 🟠 MajorApply the same org gate to
PUT /read.This GET path is now correctly org-scoped, but
updateNotificationReadStatus(...)below still updates rows unconditionally. That leaves drawer-disabled orgs able to mutate hidden notifications if they already know the IDs.Suggested change
public Integer updateNotificationReadStatus(`@Context` SecurityContext securityContext, UpdateNotificationDrawerStatus drawerStatus) { String orgId = getOrgId(securityContext); + if (!backendConfig.isDrawerEnabled(orgId)) { + return 0; + } String username = getUsername(securityContext); return drawerRepository.updateReadStatus(orgId, username, drawerStatus.getNotificationIds(), drawerStatus.getReadStatus()); }🤖 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/drawer/DrawerResource.java` around lines 77 - 82, The PUT /read handler (updateNotificationReadStatus) currently updates notifications regardless of org drawer enabled state; guard the updater with the same org gate used in the GET path by checking backendConfig.isDrawerEnabled(orgId) before performing any updates (e.g., before calling drawerRepository.update... / updateNotificationReadStatus logic) and return the appropriate no-op or error response when the drawer is disabled for that org; ensure you reference updateNotificationReadStatus and backendConfig.isDrawerEnabled(orgId) so the update path mirrors the GET path's org-scoping.
🧹 Nitpick comments (5)
engine/src/test/java/com/redhat/cloud/notifications/events/ConnectorReceiverTest.java (1)
299-343: Test name is misleading relative to assertions.At Line 299, the method name says “does not trigger drawer processing,” but the test verifies
drawerProcessor.manageConnectorDrawerReturnsIfNeeded(...)is called once. Consider renaming to reflect that the handler is invoked but receives a non-drawer type payload.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/test/java/com/redhat/cloud/notifications/events/ConnectorReceiverTest.java` around lines 299 - 343, The test method name testNonDrawerCallbackDoesNotTriggerDrawerProcessing is misleading because it asserts that drawerProcessor.manageConnectorDrawerReturnsIfNeeded(...) is called once; rename the test to accurately describe behavior (e.g., testHandlerInvokedForNonDrawerTypeOr testManagesConnectorReturnsForNonDrawerPayload) and update any references; ensure the method signature and any usages (test framework annotations) are updated to the new name so the assertions around drawerProcessor.manageConnectorDrawerReturnsIfNeeded(...) and the captured "details" type remain correct.engine/src/main/java/com/redhat/cloud/notifications/config/EngineConfig.java (1)
182-182: Avoid logging drawer toggle state using a null org context.At Line 182,
isDrawerEnabled(null)can produce a value that doesn’t represent real per-org behavior. Consider logging this toggle as “org-scoped” (or logging a representative org in non-prod diagnostics) to avoid misleading startup config output.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/main/java/com/redhat/cloud/notifications/config/EngineConfig.java` at line 182, The startup config currently calls isDrawerEnabled(null) and logs that value via config.put(drawerToggle, ...), which can be misleading because null does not represent per-org behavior; update the logic around config.put(drawerToggle, ...) to avoid invoking isDrawerEnabled(null) — instead store a sentinel like "org-scoped" (or, only in non-prod diagnostics, call isDrawerEnabled with a representative org id) so the startup output clearly indicates the toggle is org-scoped rather than a global boolean; reference the existing drawerToggle key and the isDrawerEnabled(...) method when making the change.backend/src/main/java/com/redhat/cloud/notifications/db/repositories/TemplateRepository.java (1)
359-361: Skip the drawer template lookup when the org is disabled.In the
DRAWERbranch,checkIfExistDrawerTemplateByEventType(...)still hits the DB before the org toggle is evaluated. Since this method runs while building the user-config schema, disabled orgs pay for that query on every event type only to returnfalse.Suggested change
case DRAWER: - checkIfExistDrawerTemplateByEventType(eventTypeId); - return backendConfig.isDrawerEnabled(orgId); + if (!backendConfig.isDrawerEnabled(orgId)) { + return false; + } + checkIfExistDrawerTemplateByEventType(eventTypeId); + return true;🤖 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/db/repositories/TemplateRepository.java` around lines 359 - 361, The DRAWER branch currently calls checkIfExistDrawerTemplateByEventType(eventTypeId) before verifying org toggles, causing unnecessary DB queries for disabled orgs; modify the logic in the switch-case handling DRAWER so it first calls backendConfig.isDrawerEnabled(orgId) and only calls checkIfExistDrawerTemplateByEventType(eventTypeId) if the org toggle returns true (i.e., short-circuit the DB lookup when isDrawerEnabled(orgId) is false).backend/src/test/java/com/redhat/cloud/notifications/routers/handlers/userconfig/UserConfigResourceTemplateModuleTest.java (1)
137-145: Keep the request helper data-driven.
createSettingsValue(...)now silently dropsDRAWERfrom the payload whenever the mock says the org is disabled. That makes disabled-org tests pass without actually exercising how the API behaves if a client still sendsDRAWER.One option is to let the caller decide whether the
DRAWERfield is present at all, instead of consultingbackendConfiginside the helper.🤖 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/userconfig/UserConfigResourceTemplateModuleTest.java` around lines 137 - 145, The helper createSettingsValue currently consults backendConfig.isDrawerEnabled(orgId) and omits the DRAWER entry when the org mock is disabled; change the helper so the caller controls whether the DRAWER field is included (e.g., add a parameter like includeDrawerField or accept drawer as a nullable/Optional) and remove the backendConfig.isDrawerEnabled(orgId) check from inside createSettingsValue; update callers/tests to pass the desired include/omit behavior so disabled-org tests can still exercise payloads that contain DRAWER and enabled-org tests can omit it as needed (refer to createSettingsValue and SettingsValuesByEventType.EventTypeSettingsValue to locate the helper and its emailSubscriptionTypes handling).backend/src/test/java/com/redhat/cloud/notifications/routers/handlers/userconfig/UserConfigResourceTest.java (1)
157-163: Keep the test payload and assertions independent fromisDrawerEnabled.These helpers consult the same mock as the resource, so the disabled-org path never sends a
DRAWERpayload and never checks thatDRAWERstays absent in the response. That leaves a blind spot where the write/read flow could still accept or return drawer subscriptions for a disabled org without breaking these tests. Prefer building the request from the helper arguments, then assertingDRAWERis absent whenever the org is disabled.Also applies to: 190-197, 727-773, 775-804
🤖 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/userconfig/UserConfigResourceTest.java` around lines 157 - 163, The helper createSettingsValue currently consults backendConfig.isDrawerEnabled(orgId) and omits the DRAWER key from eventTypeSettingsValue.emailSubscriptionTypes, making tests depend on the same mock as the resource; change createSettingsValue (and the other similar helpers mentioned) so it always sets eventTypeSettingsValue.emailSubscriptionTypes.put(DRAWER, drawer) based solely on the drawer argument (do not call backendConfig.isDrawerEnabled), and then update assertions in tests to explicitly assert that DRAWER is absent in responses when backendConfig.isDrawerEnabled(orgId) is false (and present/handled when true), using the unique symbols createSettingsValue, SettingsValuesByEventType.EventTypeSettingsValue, DRAWER, emailSubscriptionTypes, and backendConfig.isDrawerEnabled to locate and modify the relevant code paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@backend/src/main/java/com/redhat/cloud/notifications/routers/handlers/drawer/DrawerResource.java`:
- Around line 77-82: The PUT /read handler (updateNotificationReadStatus)
currently updates notifications regardless of org drawer enabled state; guard
the updater with the same org gate used in the GET path by checking
backendConfig.isDrawerEnabled(orgId) before performing any updates (e.g., before
calling drawerRepository.update... / updateNotificationReadStatus logic) and
return the appropriate no-op or error response when the drawer is disabled for
that org; ensure you reference updateNotificationReadStatus and
backendConfig.isDrawerEnabled(orgId) so the update path mirrors the GET path's
org-scoping.
---
Nitpick comments:
In
`@backend/src/main/java/com/redhat/cloud/notifications/db/repositories/TemplateRepository.java`:
- Around line 359-361: The DRAWER branch currently calls
checkIfExistDrawerTemplateByEventType(eventTypeId) before verifying org toggles,
causing unnecessary DB queries for disabled orgs; modify the logic in the
switch-case handling DRAWER so it first calls
backendConfig.isDrawerEnabled(orgId) and only calls
checkIfExistDrawerTemplateByEventType(eventTypeId) if the org toggle returns
true (i.e., short-circuit the DB lookup when isDrawerEnabled(orgId) is false).
In
`@backend/src/test/java/com/redhat/cloud/notifications/routers/handlers/userconfig/UserConfigResourceTemplateModuleTest.java`:
- Around line 137-145: The helper createSettingsValue currently consults
backendConfig.isDrawerEnabled(orgId) and omits the DRAWER entry when the org
mock is disabled; change the helper so the caller controls whether the DRAWER
field is included (e.g., add a parameter like includeDrawerField or accept
drawer as a nullable/Optional) and remove the
backendConfig.isDrawerEnabled(orgId) check from inside createSettingsValue;
update callers/tests to pass the desired include/omit behavior so disabled-org
tests can still exercise payloads that contain DRAWER and enabled-org tests can
omit it as needed (refer to createSettingsValue and
SettingsValuesByEventType.EventTypeSettingsValue to locate the helper and its
emailSubscriptionTypes handling).
In
`@backend/src/test/java/com/redhat/cloud/notifications/routers/handlers/userconfig/UserConfigResourceTest.java`:
- Around line 157-163: The helper createSettingsValue currently consults
backendConfig.isDrawerEnabled(orgId) and omits the DRAWER key from
eventTypeSettingsValue.emailSubscriptionTypes, making tests depend on the same
mock as the resource; change createSettingsValue (and the other similar helpers
mentioned) so it always sets
eventTypeSettingsValue.emailSubscriptionTypes.put(DRAWER, drawer) based solely
on the drawer argument (do not call backendConfig.isDrawerEnabled), and then
update assertions in tests to explicitly assert that DRAWER is absent in
responses when backendConfig.isDrawerEnabled(orgId) is false (and
present/handled when true), using the unique symbols createSettingsValue,
SettingsValuesByEventType.EventTypeSettingsValue, DRAWER,
emailSubscriptionTypes, and backendConfig.isDrawerEnabled to locate and modify
the relevant code paths.
In
`@engine/src/main/java/com/redhat/cloud/notifications/config/EngineConfig.java`:
- Line 182: The startup config currently calls isDrawerEnabled(null) and logs
that value via config.put(drawerToggle, ...), which can be misleading because
null does not represent per-org behavior; update the logic around
config.put(drawerToggle, ...) to avoid invoking isDrawerEnabled(null) — instead
store a sentinel like "org-scoped" (or, only in non-prod diagnostics, call
isDrawerEnabled with a representative org id) so the startup output clearly
indicates the toggle is org-scoped rather than a global boolean; reference the
existing drawerToggle key and the isDrawerEnabled(...) method when making the
change.
In
`@engine/src/test/java/com/redhat/cloud/notifications/events/ConnectorReceiverTest.java`:
- Around line 299-343: The test method name
testNonDrawerCallbackDoesNotTriggerDrawerProcessing is misleading because it
asserts that drawerProcessor.manageConnectorDrawerReturnsIfNeeded(...) is called
once; rename the test to accurately describe behavior (e.g.,
testHandlerInvokedForNonDrawerTypeOr
testManagesConnectorReturnsForNonDrawerPayload) and update any references;
ensure the method signature and any usages (test framework annotations) are
updated to the new name so the assertions around
drawerProcessor.manageConnectorDrawerReturnsIfNeeded(...) and the captured
"details" type remain correct.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ff91adfe-fb03-4a9a-a1d2-549f9e7a92a1
📒 Files selected for processing (14)
backend/src/main/java/com/redhat/cloud/notifications/config/BackendConfig.javabackend/src/main/java/com/redhat/cloud/notifications/db/repositories/SubscriptionRepository.javabackend/src/main/java/com/redhat/cloud/notifications/db/repositories/TemplateRepository.javabackend/src/main/java/com/redhat/cloud/notifications/routers/handlers/drawer/DrawerResource.javabackend/src/main/java/com/redhat/cloud/notifications/routers/handlers/userconfig/UserConfigResource.javabackend/src/test/java/com/redhat/cloud/notifications/db/repositories/TemplateRepositoryTest.javabackend/src/test/java/com/redhat/cloud/notifications/routers/handlers/drawer/DrawerResourceTest.javabackend/src/test/java/com/redhat/cloud/notifications/routers/handlers/userconfig/UserConfigResourceTemplateModuleTest.javabackend/src/test/java/com/redhat/cloud/notifications/routers/handlers/userconfig/UserConfigResourceTest.javaengine/src/main/java/com/redhat/cloud/notifications/config/EngineConfig.javaengine/src/main/java/com/redhat/cloud/notifications/events/ConnectorReceiver.javaengine/src/main/java/com/redhat/cloud/notifications/processors/drawer/DrawerProcessor.javaengine/src/test/java/com/redhat/cloud/notifications/events/ConnectorReceiverTest.javaengine/src/test/java/com/redhat/cloud/notifications/processors/drawer/DrawerProcessorTest.java
Assisted-by: Claude Sonnet 4.5 (via Claude Code)
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
engine/src/test/java/com/redhat/cloud/notifications/events/ConnectorReceiverTest.java (2)
238-248: Consider adding verification of the downstreamdrawerNotificationRepository.create()call.Since
DrawerProcessoris a spy (real implementation executes), the test could verify thatdrawerNotificationRepository.create()is called with the expected arguments when processing a drawer callback. This would provide more complete coverage of the drawer callback flow.💡 Optional enhancement for more complete verification
assertEquals(historyId, historyIdCaptor.getValue().toString()); Map<String, Object> capturedDetails = (Map<String, Object>) payloadCaptor.getValue().get("details"); assertEquals("com.redhat.console.notification.toCamel.drawer", capturedDetails.get("type")); + + verify(drawerNotificationRepository, times(1)).create(eq(event), eq("user1,user2")); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/test/java/com/redhat/cloud/notifications/events/ConnectorReceiverTest.java` around lines 238 - 248, Add a verify for drawerNotificationRepository.create(...) to the ConnectorReceiverTest so the test asserts the DrawerProcessor (used as a spy) actually persisted the drawer notification: locate the call to manageConnectorDrawerReturnsIfNeeded in ConnectorReceiverTest and after the existing verify/asserts add a verification of drawerNotificationRepository.create(...) (or capture its argument with ArgumentCaptor<DrawerNotification>) and assert expected fields (e.g., historyId, type from payload details, and any other expected properties). Ensure the test uses the same mock instance of drawerNotificationRepository that DrawerProcessor uses so the verify/captor observes the real invocation.
280-292: Minor inconsistency: missing error counter assertion.The
testDrawerCallbackProcessedtest asserts that the error counter is 0 (line 236), but this test only checks the processed counter. For consistency, consider adding the error counter assertion here as well.💡 Suggested fix for consistency
inMemoryConnector.source(FROMCAMEL_CHANNEL).send(payload); micrometerAssertionHelper.awaitAndAssertCounterIncrement(MESSAGES_PROCESSED_COUNTER_NAME, 1); + micrometerAssertionHelper.assertCounterIncrement(MESSAGES_ERROR_COUNTER_NAME, 0); ArgumentCaptor<Map<String, Object>> payloadCaptor = ArgumentCaptor.forClass(Map.class);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/test/java/com/redhat/cloud/notifications/events/ConnectorReceiverTest.java` around lines 280 - 292, In testDrawerCallbackProcessed add the missing error-counter assertion for consistency: after the MESSAGES_PROCESSED_COUNTER_NAME check, assert that the ERROR_COUNTER_NAME is 0 using the micrometerAssertionHelper (e.g., call the appropriate helper method to assert the error counter value is 0). Locate the testDrawerCallbackProcessed method and add the assertion referencing MESSAGES_PROCESSED_COUNTER_NAME, ERROR_COUNTER_NAME, and micrometerAssertionHelper so the test checks both processed and error counters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@engine/src/test/java/com/redhat/cloud/notifications/events/ConnectorReceiverTest.java`:
- Around line 238-248: Add a verify for drawerNotificationRepository.create(...)
to the ConnectorReceiverTest so the test asserts the DrawerProcessor (used as a
spy) actually persisted the drawer notification: locate the call to
manageConnectorDrawerReturnsIfNeeded in ConnectorReceiverTest and after the
existing verify/asserts add a verification of
drawerNotificationRepository.create(...) (or capture its argument with
ArgumentCaptor<DrawerNotification>) and assert expected fields (e.g., historyId,
type from payload details, and any other expected properties). Ensure the test
uses the same mock instance of drawerNotificationRepository that DrawerProcessor
uses so the verify/captor observes the real invocation.
- Around line 280-292: In testDrawerCallbackProcessed add the missing
error-counter assertion for consistency: after the
MESSAGES_PROCESSED_COUNTER_NAME check, assert that the ERROR_COUNTER_NAME is 0
using the micrometerAssertionHelper (e.g., call the appropriate helper method to
assert the error counter value is 0). Locate the testDrawerCallbackProcessed
method and add the assertion referencing MESSAGES_PROCESSED_COUNTER_NAME,
ERROR_COUNTER_NAME, and micrometerAssertionHelper so the test checks both
processed and error counters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f2afb592-07e4-4089-a127-bf8bc3ed5bdd
📒 Files selected for processing (2)
engine/src/main/java/com/redhat/cloud/notifications/events/ConnectorReceiver.javaengine/src/test/java/com/redhat/cloud/notifications/events/ConnectorReceiverTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- engine/src/main/java/com/redhat/cloud/notifications/events/ConnectorReceiver.java
Contributor
|
/retest |
Contributor
|
iqe test failures are unrelated |
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.
Summary by CodeRabbit
Improvements
Tests