Skip to content

Notification rework#913

Open
tompro wants to merge 7 commits into
masterfrom
notification-rework
Open

Notification rework#913
tompro wants to merge 7 commits into
masterfrom
notification-rework

Conversation

@tompro
Copy link
Copy Markdown
Collaborator

@tompro tompro commented May 5, 2026

📝 Description

Adjusts and reworks notifications with new spec as described in Google docs Notifications document.

  • Notifications have a new type (on the notification not the payload) and can be Actionable or Informal. Notifications can be filtered in the query by that flag as well
  • Notifications that are informal can no longer replace notifications where user action is required
  • Notifications for company events
  • Removed all CheckBill type of notifications so only required participants get notified about changes in Bills
  • New 'logical' notifications (to current client no Nostr publishing), is used in all cases where required
  • New global notification that is generated on account creation and should remind the user to backup his seed phrase.

Relates to #758


✅ Checklist

Please ensure the following tasks are completed before requesting a review:

  • My code adheres to the coding guidelines of this project.
  • I have run cargo fmt.
  • I have run cargo clippy.
  • I have added or updated tests (if applicable).
  • All CI/CD steps were successful.
  • I have updated the documentation (if applicable).
  • I have checked that there are no console errors or warnings.
  • I have verified that the application builds without errors.
  • I've described the changes made to the API. (modification, addition, deletion).

📋 Review Guidelines

Please focus on the following while reviewing:

  • Does the code follow the repository's contribution guidelines?
  • Are there any potential bugs or performance issues?
  • Are there any typos or grammatical errors in the code or comments?

@tompro tompro self-assigned this May 5, 2026
Copilot AI review requested due to automatic review settings May 5, 2026 14:36
@qodo-code-review
Copy link
Copy Markdown

ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 5, 2026

Codecov Report

❌ Patch coverage is 53.86703% with 340 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.49%. Comparing base (674673f) to head (2b9725c).

Files with missing lines Patch % Lines
...s/bcr-ebill-core/src/protocol/event/bill_events.rs 53.66% 139 Missing ⚠️
...sport/src/handler/company_chain_event_processor.rs 44.69% 73 Missing ⚠️
...r-ebill-core/src/protocol/blockchain/bill/chain.rs 45.56% 43 Missing ⚠️
...tes/bcr-ebill-core/src/application/notification.rs 24.00% 19 Missing ⚠️
.../bcr-ebill-transport/src/notification_transport.rs 70.00% 18 Missing ⚠️
.../bcr-ebill-api/src/service/bill_service/payment.rs 0.00% 16 Missing ⚠️
...transport/src/handler/bill_action_event_handler.rs 65.11% 15 Missing ⚠️
...r-ebill-core/src/protocol/blockchain/bill/block.rs 0.00% 6 Missing ⚠️
crates/bcr-ebill-wasm/src/data/notification.rs 0.00% 6 Missing ⚠️
...rates/bcr-ebill-persistence/src/db/notification.rs 50.00% 2 Missing ⚠️
... and 2 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #913      +/-   ##
==========================================
- Coverage   70.74%   70.49%   -0.25%     
==========================================
  Files         139      139              
  Lines       27713    28378     +665     
==========================================
+ Hits        19606    20006     +400     
- Misses       8107     8372     +265     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Reworks the notification pipeline across core/domain, persistence, transport, and WASM bindings to support the updated notification spec (action-required vs informational), richer bill notification payloads (sender info), and “local-only” notifications for certain flows (no Nostr publish).

Changes:

  • Adds NotificationLevel (ActionRequired / Informational) to notifications, exposes it through persistence + WASM DTOs, and enables filtering by level in notification queries.
  • Refactors bill transport event generation/dispatch (new BillChainEvent::generate_* helpers) and adjusts recipient/email logic to avoid unnecessary “check bill” notifications.
  • Introduces local (push-only) bill notifications for flows like “bill paid”, “bill sold”, and “offer-to-sell timeout”, and removes the send_bill_is_paid_event transport API.

Reviewed changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
crates/bcr-ebill-wasm/src/data/notification.rs Adds level to WASM notification DTO and maps from core NotificationLevel.
crates/bcr-ebill-wasm/src/data/mod.rs Adds level filter field and maps it into persistence NotificationFilter.
crates/bcr-ebill-transport/src/transport_service.rs Switches to centralized bill event message generation and adjusts email recipient rules.
crates/bcr-ebill-transport/src/test_utils.rs Extends mocks for new notification transport API surface.
crates/bcr-ebill-transport/src/notification_transport.rs Adds local bill notification creation + push dispatch; updates timeout handling path.
crates/bcr-ebill-transport/src/lib.rs Wires push service into NotificationTransportService constructor.
crates/bcr-ebill-transport/src/handler/contact_share_handler.rs Marks contact-share notifications as ActionRequired.
crates/bcr-ebill-transport/src/handler/company_chain_event_processor.rs Adds company-event notifications (invite accepted/rejected, removal), with replacement rules and levels.
crates/bcr-ebill-transport/src/handler/bill_action_event_handler.rs Implements “informational must not replace actionable” rule and derives notification level from ActionType.
crates/bcr-ebill-persistence/src/notification.rs Adds level to NotificationFilter and includes it in query filters/bindings.
crates/bcr-ebill-persistence/src/db/notification.rs Persists NotificationLevel in SurrealDB and binds level filter in list queries.
crates/bcr-ebill-core/src/protocol/event/bill_events.rs Adds sender metadata into payload, new timeout event type, and centralized event-description + message generation helpers.
crates/bcr-ebill-core/src/protocol/blockchain/bill/chain.rs Adds APIs to get ident-only participants (excluding anon/bearer) for notification targeting.
crates/bcr-ebill-core/src/protocol/blockchain/bill/block.rs Adds helper predicates (is_anon / is_ident) on participant block data.
crates/bcr-ebill-core/src/application/notification.rs Adds NotificationLevel to the core Notification model and constructors.
crates/bcr-ebill-api/src/service/transport_service/transport.rs Removes send_bill_is_paid_event from the transport service trait.
crates/bcr-ebill-api/src/service/transport_service/notification_transport.rs Adds create_local_bill_notification to the notification transport trait.
crates/bcr-ebill-api/src/service/file_reference_helper.rs Updates test mock trait signature after removing send_bill_is_paid_event.
crates/bcr-ebill-api/src/service/bill_service/tests.rs Updates bill-service tests to expect local notifications for sell flows.
crates/bcr-ebill-api/src/service/bill_service/test_utils.rs Provides default mock expectations for the new notification transport methods.
crates/bcr-ebill-api/src/service/bill_service/payment.rs Replaces paid-event transport publish with local notification; adds local timeout notification for offer-to-sell.
crates/bcr-ebill-api/src/service/bill_service/blocks.rs Adds local notification when a sell block is created.

Comment thread crates/bcr-ebill-core/src/protocol/event/bill_events.rs
Comment thread crates/bcr-ebill-transport/src/handler/bill_action_event_handler.rs
Comment thread crates/bcr-ebill-core/src/application/notification.rs
Comment thread crates/bcr-ebill-persistence/src/db/notification.rs
Comment thread crates/bcr-ebill-transport/src/notification_transport.rs
Copy link
Copy Markdown
Collaborator

@zupzup zupzup left a comment

Choose a reason for hiding this comment

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

Very Nice, LGTM 👍

Comment thread crates/bcr-ebill-api/src/service/bill_service/payment.rs Outdated
Comment thread crates/bcr-ebill-core/src/protocol/blockchain/bill/chain.rs
Comment thread crates/bcr-ebill-core/src/protocol/blockchain/bill/chain.rs Outdated
Comment thread crates/bcr-ebill-core/src/protocol/blockchain/bill/chain.rs Outdated
Comment thread crates/bcr-ebill-core/src/protocol/event/bill_events.rs Outdated
CheckQuote,
}

impl BillEventType {
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@mtbitcr these are btw the text (bcs you mentioned text) translation keys to be used for the different event types. You need to add the corresponding translated text to the localisations to have meaningful messages. These keys are not new btw. they have been on the notifications since we started.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks I'll coordinate with @JulianVIE

Copy link
Copy Markdown
Collaborator Author

@tompro tompro May 6, 2026

Choose a reason for hiding this comment

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

@mtbitcr One more thing. Initially I forgot to add the new notification about seed phrase backup. This has been added with the recent commit. The payload looks like the following:

{
        "id": "b6522150-cb1c-4370-a4da-4e4f5fb2734b",
        "node_id": "bitcrt03ee419906bddd59fc4b331f410d130fb7b31811f66f03c289d74502089ee41104",
        "notification_type": "General",
        "reference_id": "seed_phrase",
        "description": "save_seed_phrase",
        "datetime": "2026-05-06T14:31:48.000Z",
        "active": true,
        "level": "ActionRequired"
}

The global type is General, which we had already, but this is I think the first notification that we built with that. Then there is a reference ID, which identifies the seed phrase here. Normally this contains a bill ID or something dynamic but in this case it's static (an app element). And then it has a description (like bill notifications have) with a translation key.

@tompro tompro requested a review from mtbitcr May 7, 2026 08:28
self.generate_action_messages(overrides, None, None)
}

pub fn generate_bill_sold_messages(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do we also generate system/local notification from blockchain by seller, that buyer bought bill ?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Comment thread crates/bcr-ebill-core/src/protocol/event/bill_events.rs Outdated
BillEventType::BillSellOffered => "bill_request_to_buy".to_string(),
BillEventType::BillSellOfferTimeout => "bill_sell_offer_timed_out".to_string(),
BillEventType::BillBuyingRejected => "bill_buying_rejected".to_string(),
BillEventType::BillPaid => "bill_paid".to_string(),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

its local notification, correct?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

BillEventType::BillSellOfferTimeout => "bill_sell_offer_timed_out".to_string(),
BillEventType::BillBuyingRejected => "bill_buying_rejected".to_string(),
BillEventType::BillPaid => "bill_paid".to_string(),
BillEventType::BillRecoursePaid => "bill_recourse_paid".to_string(),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

its local notification for recourser and standard for recoursee, correct?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Good catch this on is still missing the local notification 👁️

BillEventType::BillPaid => "bill_paid".to_string(),
BillEventType::BillRecoursePaid => "bill_recourse_paid".to_string(),
BillEventType::BillEndorsed => "bill_endorsed".to_string(),
BillEventType::BillSold => "bill_sold".to_string(),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

its local notification for seller and standard for buyer, correct?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes, seller local see above and buyer gets nostr + optional email

BillEventType::BillMintingRequested => "requested_to_mint".to_string(),
BillEventType::BillNewQuote => "new_quote".to_string(),
BillEventType::BillQuoteApproved => "quote_approved".to_string(),
BillEventType::BillBlock => "".to_string(),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

what is it?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This was a no action notification that was reserved just for bill block data. It's stemming from the time when we were still sending blocks via DM. It's still there for backwards compatibility.

BillEventType::BillSold => "bill_sold".to_string(),
BillEventType::BillMintingRequested => "requested_to_mint".to_string(),
BillEventType::BillNewQuote => "new_quote".to_string(),
BillEventType::BillQuoteApproved => "quote_approved".to_string(),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

how all mint notifications works now? api? or nostr?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I have no idea. I just cover all potential block cases with a notification type. But I never had a look in whether they are actually already in use and how they work. Don't forget these message types have been in code for about a year or so.

@tompro tompro force-pushed the notification-rework branch from a5da1e5 to 2b9725c Compare May 8, 2026 07:04
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.

4 participants