Skip to content

Conversation

@adityathebe
Copy link
Member

@adityathebe adityathebe commented Jan 23, 2026

Summary by CodeRabbit

  • Improvements
    • Notification history now supports an optional JSON payload field, allowing richer structured data to be stored and updated for unsent notifications.
  • Behavior
    • Duplicate-notification deduplication now updates stored body and JSON payload to the latest value while incrementing occurrence count.
  • Tests
    • Added and adjusted tests to validate JSON payload storage and update behavior.

✏️ Tip: You can customize this high-level summary in your review settings.

…tory

- Add p_body_payload jsonb parameter to stored procedure
- Write to body_payload column on INSERT and UPDATE
- Keep p_body for backward compatibility
@github-actions
Copy link

github-actions bot commented Jan 23, 2026

Benchstat

Base: a84cd33f33105fa015e8407f7c2fabb005d03fe0
Head: 2f9bc9720b173338324fc67ffa70511988a3edaa

goos: linux
goarch: amd64
pkg: github.com/flanksource/duty/bench
cpu: AMD EPYC 7763 64-Core Processor                
                                                │ bench-base.txt │          bench-head.txt           │
                                                │     sec/op     │   sec/op     vs base              │
Main/Sample-15000/catalog_changes/Without_RLS-4      5.093m ± 1%   5.146m ± 1%  +1.03% (p=0.004 n=6)
Main/Sample-15000/catalog_changes/With_RLS-4         127.8m ± 1%   127.9m ± 0%       ~ (p=0.394 n=6)
Main/Sample-15000/config_changes/Without_RLS-4       5.069m ± 1%   5.127m ± 1%  +1.13% (p=0.015 n=6)
Main/Sample-15000/config_changes/With_RLS-4          127.5m ± 0%   128.4m ± 1%  +0.71% (p=0.002 n=6)
Main/Sample-15000/config_detail/Without_RLS-4        3.826m ± 0%   3.862m ± 2%  +0.94% (p=0.002 n=6)
Main/Sample-15000/config_detail/With_RLS-4           123.9m ± 0%   124.1m ± 0%       ~ (p=0.485 n=6)
Main/Sample-15000/config_names/Without_RLS-4         12.46m ± 2%   12.43m ± 2%       ~ (p=0.699 n=6)
Main/Sample-15000/config_names/With_RLS-4            124.7m ± 1%   124.9m ± 2%       ~ (p=0.310 n=6)
Main/Sample-15000/config_summary/Without_RLS-4       59.93m ± 3%   61.33m ± 2%       ~ (p=0.065 n=6)
Main/Sample-15000/config_summary/With_RLS-4          739.5m ± 1%   735.0m ± 1%  -0.61% (p=0.041 n=6)
Main/Sample-15000/configs/Without_RLS-4              7.021m ± 7%   7.016m ± 0%       ~ (p=0.394 n=6)
Main/Sample-15000/configs/With_RLS-4                 124.4m ± 1%   122.8m ± 1%  -1.30% (p=0.002 n=6)
Main/Sample-15000/analysis_types/Without_RLS-4       3.781m ± 1%   3.780m ± 0%       ~ (p=0.589 n=6)
Main/Sample-15000/analysis_types/With_RLS-4          3.840m ± 3%   3.818m ± 2%  -0.57% (p=0.041 n=6)
Main/Sample-15000/analyzer_types/Without_RLS-4       3.670m ± 0%   3.662m ± 1%       ~ (p=0.485 n=6)
Main/Sample-15000/analyzer_types/With_RLS-4          3.680m ± 1%   3.685m ± 1%       ~ (p=0.699 n=6)
Main/Sample-15000/change_types/Without_RLS-4         5.123m ± 3%   5.095m ± 1%  -0.54% (p=0.041 n=6)
Main/Sample-15000/change_types/With_RLS-4            5.268m ± 4%   5.106m ± 2%  -3.07% (p=0.002 n=6)
Main/Sample-15000/config_classes/Without_RLS-4       3.243m ± 0%   3.241m ± 3%       ~ (p=0.937 n=6)
Main/Sample-15000/config_classes/With_RLS-4          123.5m ± 1%   122.6m ± 2%  -0.73% (p=0.041 n=6)
Main/Sample-15000/config_types/Without_RLS-4         3.851m ± 4%   3.894m ± 0%       ~ (p=0.065 n=6)
Main/Sample-15000/config_types/With_RLS-4            125.5m ± 1%   121.7m ± 1%  -3.03% (p=0.002 n=6)
geomean                                              18.91m        18.89m       -0.13%

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 23, 2026

Walkthrough

The insert_unsent_notification_to_history function in the notification SQL view is extended with an optional p_body_payload jsonb parameter, which is now persisted in the notification_send_history table during both insert and update operations.

Changes

Cohort / File(s) Summary
Notification function update
views/021_notification.sql
Added p_body_payload jsonb DEFAULT NULL to insert_unsent_notification_to_history signature; UPDATED path sets body_payload = p_body_payload; INSERT path includes body_payload column and value.
Tests: notification history behavior
tests/notification_test.go
Expanded/renamed tests to verify saving and deduplication of body_payload (JSONB) and updating existing history entries to the latest JSON payload; adjusted assertions to check BodyPayload and counts.

Possibly related PRs

  • clicky notification #1738 — Adds body_payload column and model field to notification history (related DB schema/model change that pairs with this function + tests update).

Suggested reviewers

  • moshloop
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: adding a new body_payload parameter to the insert_unsent_notification_to_history function, which is the primary focus of the changeset.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/notification-body-payload

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
views/021_notification.sql (1)

43-56: Drop the previous full signature to avoid callers hitting the old overload.

Adding a new optional parameter creates a new signature; if the old signature remains, calls with the previous argument list will still resolve to the old function and never populate body_payload. Explicitly drop the prior full signature (not just the 5‑arg one) so legacy calls route to the new function with the default value.

✅ Suggested fix
 -- Ensure the previous function is cleaned up.
 DROP FUNCTION IF EXISTS insert_unsent_notification_to_history(uuid, text, uuid, text, interval);
+DROP FUNCTION IF EXISTS insert_unsent_notification_to_history(
+  uuid, text, uuid, text, interval,
+  uuid, uuid, uuid, uuid, uuid, uuid, text
+);
🤖 Fix all issues with AI agents
In `@views/021_notification.sql`:
- Around line 86-89: The dedupe UPDATE is overwriting existing body_payload with
NULL when the caller omits p_body_payload; change the assignment for
body_payload in the UPDATE to preserve the stored value unless a new payload is
explicitly provided by using a conditional expression (e.g., COALESCE or CASE)
that sets body_payload = p_body_payload only when p_body_payload IS NOT NULL,
otherwise leaves the current body_payload unchanged; update the assignment
referencing body_payload and p_body_payload accordingly.

Drop the old 13-parameter function signature to prevent PostgreSQL
function overloading issues where callers might hit the old function
instead of the new one with body_payload parameter.
…ion_to_history

- Test saving body_payload for unsent notifications
- Test updating body_payload on duplicate notification (deduplication)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants