RHCLOUD-45800: Send notifications for V2 role operations#2604
RHCLOUD-45800: Send notifications for V2 role operations#2604randomnetcat merged 1 commit intomasterfrom
Conversation
Reviewer's GuideAdds notification sending for custom V2 role create/update/delete operations and updates tests to verify Kafka messages are produced when notifications are enabled. Sequence diagram for custom V2 role change notificationssequenceDiagram
actor User
participant RoleV2View
participant custom_v2_role_obj_change_notification_handler
participant NotificationService
participant Kafka
rect rgb(230, 255, 230)
User->>RoleV2View: POST /v2/roles (create)
RoleV2View->>RoleV2View: create RoleV2 role
RoleV2View->>custom_v2_role_obj_change_notification_handler: role, created, request_user
custom_v2_role_obj_change_notification_handler->>custom_v2_role_obj_change_notification_handler: check NOTIFICATIONS_ENABLED
custom_v2_role_obj_change_notification_handler->>custom_v2_role_obj_change_notification_handler: validate role.type is CUSTOM
custom_v2_role_obj_change_notification_handler->>custom_v2_role_obj_change_notification_handler: payload_builder(username, role)
custom_v2_role_obj_change_notification_handler->>NotificationService: notify(custom-v2-role-created, payload, org_id)
NotificationService->>Kafka: produce message
end
rect rgb(230, 240, 255)
User->>RoleV2View: PATCH /v2/roles/{id} (update)
RoleV2View->>RoleV2View: update RoleV2 role
RoleV2View->>custom_v2_role_obj_change_notification_handler: role, updated, request_user
custom_v2_role_obj_change_notification_handler->>NotificationService: notify(custom-v2-role-updated, payload, org_id)
NotificationService->>Kafka: produce message
end
rect rgb(255, 235, 230)
User->>RoleV2View: DELETE /v2/roles (bulk_destroy)
RoleV2View->>RoleV2View: remove roles, collect removed_roles
loop for each role in removed_roles
RoleV2View->>custom_v2_role_obj_change_notification_handler: role, deleted, request_user
custom_v2_role_obj_change_notification_handler-->>NotificationService: notify(custom-v2-role-deleted, payload, org_id)
NotificationService-->>Kafka: produce message
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
custom_v2_role_obj_change_notification_handler, raising aValueErrorwhen the role is not custom or the operation is unknown will surface as a 500 from normal view code; consider returning early (or handling this as a no-op) to avoid runtime errors if new role types/operations are introduced or if the handler is accidentally reused. - The string literals for event types (
custom-v2-role-created/updated/deleted) are duplicated between the handler and tests; consider centralizing them as constants (e.g., in the notifications module) to reduce the risk of drift if these values change.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `custom_v2_role_obj_change_notification_handler`, raising a `ValueError` when the role is not custom or the operation is unknown will surface as a 500 from normal view code; consider returning early (or handling this as a no-op) to avoid runtime errors if new role types/operations are introduced or if the handler is accidentally reused.
- The string literals for event types (`custom-v2-role-created/updated/deleted`) are duplicated between the handler and tests; consider centralizing them as constants (e.g., in the notifications module) to reduce the risk of drift if these values change.
## Individual Comments
### Comment 1
<location path="tests/management/role/test_v2_view.py" line_range="989-998" />
<code_context>
self._assert_audit_log(action=AuditLog.CREATE, description=f"Created V2 role: {data['name']}")
+ send_kafka_message.assert_called_with(
+ settings.NOTIFICATIONS_TOPIC,
+ {
+ "bundle": "console",
+ "application": "rbac",
+ "event_type": "custom-v2-role-created",
+ "timestamp": ANY,
+ "events": [
+ {
+ "metadata": {},
+ "payload": {
+ "name": data["name"],
+ "username": self.user_data["username"],
+ "uuid": response.data["id"],
+ },
+ }
+ ],
+ "org_id": self.tenant.org_id,
+ },
+ ANY,
+ )
+
def test_create_role_multiple_permissions(self):
</code_context>
<issue_to_address>
**suggestion (testing):** Consider using assert_called_once_with to also verify call count for the notification producer
Currently the test only checks the call arguments, not that `send_kafka_message` is called exactly once. Using `send_kafka_message.assert_called_once_with(...)` would also guard against accidental duplicate notifications; you can apply this to the create, update, and delete notification checks.
Suggested implementation:
```python
self._assert_audit_log(action=AuditLog.CREATE, description=f"Created V2 role: {data['name']}")
send_kafka_message.assert_called_once_with(
```
```python
ANY,
)
```
To fully apply your suggestion, similar changes should be made for the update and delete notification tests in this file:
1. In `test_update_role_success` (and any other update-related tests using the same mock), replace `send_kafka_message.assert_called_with(...)` with `send_kafka_message.assert_called_once_with(...)`.
2. In the corresponding delete test(s) (e.g., `test_delete_role_success` or similar), also replace `assert_called_with` with `assert_called_once_with` for the `send_kafka_message` mock.
3. Ensure all such tests that validate notification payloads also verify the call count via `assert_called_once_with`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| send_kafka_message.assert_called_with( | ||
| settings.NOTIFICATIONS_TOPIC, | ||
| { | ||
| "bundle": "console", | ||
| "application": "rbac", | ||
| "event_type": "custom-v2-role-created", | ||
| "timestamp": ANY, | ||
| "events": [ | ||
| { | ||
| "metadata": {}, |
There was a problem hiding this comment.
suggestion (testing): Consider using assert_called_once_with to also verify call count for the notification producer
Currently the test only checks the call arguments, not that send_kafka_message is called exactly once. Using send_kafka_message.assert_called_once_with(...) would also guard against accidental duplicate notifications; you can apply this to the create, update, and delete notification checks.
Suggested implementation:
self._assert_audit_log(action=AuditLog.CREATE, description=f"Created V2 role: {data['name']}")
send_kafka_message.assert_called_once_with( ANY,
)To fully apply your suggestion, similar changes should be made for the update and delete notification tests in this file:
- In
test_update_role_success(and any other update-related tests using the same mock), replacesend_kafka_message.assert_called_with(...)withsend_kafka_message.assert_called_once_with(...). - In the corresponding delete test(s) (e.g.,
test_delete_role_successor similar), also replaceassert_called_withwithassert_called_once_withfor thesend_kafka_messagemock. - Ensure all such tests that validate notification payloads also verify the call count via
assert_called_once_with.
|
@randomnetcat can solve merge conflict please ? 🙏 |
b9c606b to
51e7b7d
Compare
|
Done. |
|
Thanks @randomnetcat , can you please again rebase ? |
685b28d to
cfebeb7
Compare
cfebeb7 to
69de2db
Compare
Link(s) to Jira
RHCLOUD-45800
Description of Intent of Change(s)
Send notifications for V2 role changes.
Summary by Sourcery
Send notifications when custom V2 roles are created, updated, or deleted and verify this behavior in API tests.
New Features:
Enhancements:
Tests: