Add endpoint to determine if there are active notifications

[zupzup]

User description

📝 Description

• Add endpoint to determine if there are active notifications

Relates to #589

---

✅ 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).

---

🚀 Changes Made

See above.

---

💡 How to Test

Please provide clear instructions on how reviewers can test your changes:

1. cargo test
2. Test the new endpoint with several identities and active and inactive notifications

---

🤝 Related Issues

List any related issues, pull requests, or discussions:

• Add endpoint to determine if there are active notifications #589

---

📋 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?

---

PR Type

Enhancement

---

Description

• Add endpoint to check active notification status for node IDs

• Implement database query for active notification lookup

• Add WASM API binding for frontend integration

• Include comprehensive test coverage for new functionality

---

Diagram Walkthrough

flowchart LR
  A["Client Request"] --> B["WASM API"]
  B --> C["Notification Service"]
  C --> D["Database Store"]
  D --> E["Active Status Query"]
  E --> F["HashMap Response"]
  F --> G["Web Interface"]

Loading

File Walkthrough

	Relevant files
Enhancement	8 files default_service.rs Add service method for active notification status                +11/-0    notification.rs Implement database query for active notification lookup    +119/-1  notification.rs Add trait method for active status retrieval                          +5/-0      notification_service.rs Add transport trait method definition                                        +5/-0      notification.rs Add WASM API endpoint for notification status                        +23/-2    notification.rs Add web data structure for notification status                      +8/-0      main.js Add frontend button for testing new endpoint                          +8/-0      index.html Add UI button for notification status testing                        +1/-0
Tests	2 files mod.rs Update mock traits with new method signature                          +8/-0      mod.rs Update transport mock with new method                                        +4/-0
Documentation	1 files CHANGELOG.md Document new API call functionality                                            +2/-0

---

[qodo-code-review]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 589 - Partially compliant Compliant requirements: • Add an endpoint active_notifications_for_node_ids that takes a Vec • Returns { "node_id_1": true, "node_id_2": false ...} • If the Vec is set, return status only for the given node ids Non-compliant requirements: • If the Vec is empty, return status for personal identity and all local companies Requires further human verification: • Verification that empty Vec returns only personal identity and local companies (not all node IDs with active notifications)

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 Security concerns SQL injection: The code uses string formatting to construct SQL queries with the node_id_filter variable in line 58. While the current implementation appears safe since node_id_filter is a hardcoded string, this pattern could be dangerous if modified in the future. Consider using parameterized queries or prepared statements consistently.

⚡ Recommended focus areas for review Error Handling The method uses unwrap_or_default() which silently swallows database errors and returns an empty HashMap, potentially masking important failures Ok(self .notification_store .get_active_status_for_node_ids(node_ids) .await .unwrap_or_default()) Typo Method name has a typo: get_active_notificattion_status_for_node_ids should be get_active_notification_status_for_node_ids async fn get_active_notificattion_status_for_node_ids( &self, node_ids: &[NodeId], ) -> Result<HashMap<NodeId, bool>>; SQL Injection String formatting is used to construct SQL query with node_id_filter variable, which could potentially lead to SQL injection if not properly handled let result: Vec<NodeIdDb> = self.db.query(&format!("SELECT node_id from notifications where active = true {node_id_filter} GROUP BY node_id"), bindings).await?; let mut res: HashMap<NodeId, bool> = HashMap::with_capacity(node_ids.len());

[Copilot]

Pull Request Overview

Add a new endpoint and related plumbing to fetch active notification statuses for a set of node IDs.

• Introduce NotificationStatusWeb and a Wasm-binding active_notifications_for_node_ids method in the frontend crate.
• Declare and implement transport and persistence APIs for fetching active-status by node IDs, including DB query logic and unit tests.
• Update JS UI and CHANGELOG to expose and document the new endpoint.

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
crates/bcr-ebill-wasm/src/data/notification.rs	Added NotificationStatusWeb struct for the new endpoint
crates/bcr-ebill-wasm/src/api/notification.rs	Added active_notifications_for_node_ids Wasm API method
crates/bcr-ebill-wasm/main.js	Registered getActiveNotif handler for the new button
crates/bcr-ebill-wasm/index.html	Added button to trigger the new endpoint
crates/bcr-ebill-transport/src/notification_service.rs	Declared transport trait method for active-status lookup
crates/bcr-ebill-transport/src/handler/mod.rs	Added store API signature for get_active_status_for_node_ids
crates/bcr-ebill-persistence/src/notification.rs	Extended persistence trait with active-status method
crates/bcr-ebill-persistence/src/db/notification.rs	Implemented DB query for active-status and added tests
crates/bcr-ebill-api/src/tests/mod.rs	Updated API mock to include new store method
crates/bcr-ebill-api/src/service/notification_service/default_service.rs	Implemented the new service method get_active_notificattion_status_for_node_ids
CHANGELOG.md	Documented the new API endpoint

Comments suppressed due to low confidence (2)

crates/bcr-ebill-api/src/service/notification_service/default_service.rs:753

• There are no unit tests for this service-level method. Add tests to verify that it correctly forwards results from the store and handles errors to prevent regressions.

    async fn get_active_notificattion_status_for_node_ids(

crates/bcr-ebill-persistence/src/db/notification.rs:58

• The format! macro includes a {node_id_filter} placeholder without a corresponding argument, causing a compile error. Use format!("... {} ...", node_id_filter) or named arguments ({node_id_filter} = node_id_filter).

        let result: Vec<NodeIdDb> = self.db.query(&format!("SELECT node_id from notifications where active = true {node_id_filter} GROUP BY node_id"), bindings).await?;

[qodo-code-review]

PR Code Suggestions ✨

Latest suggestions up to 2bb3d99

Category	Suggestion	Impact
Incremental [*]	Improve error handling propagation Replace unwrap_or_default() with proper error propagation using the ? operator to avoid silently swallowing errors from the notification store. crates/bcr-ebill-api/src/service/notification_service/default_service.rs [753-762] async fn get_active_notification_status_for_node_ids( &self, node_ids: &[NodeId], ) -> Result<HashMap<NodeId, bool>> { - Ok(self - .notification_store + self.notification_store .get_active_status_for_node_ids(node_ids) .await - .unwrap_or_default()) } Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies that using unwrap_or_default() swallows potential errors from the data store, which is a significant issue as the function is designed to propagate Results.	Medium
	Optimize lookup performance complexity Optimize the algorithm by converting the result to a HashSet first to avoid O(n*m) complexity when checking if node_ids exist in the result. crates/bcr-ebill-persistence/src/db/notification.rs [59-72] let mut res: HashMap<NodeId, bool> = HashMap::with_capacity(node_ids.len()); if node_ids.is_empty() { for node_id_db in result { res.insert(node_id_db.node_id.to_owned(), true); } } else { + let active_node_ids: std::collections::HashSet<_> = result.iter().map(|n| &n.node_id).collect(); for node_id in node_ids { res.insert( node_id.to_owned(), - result.iter().any(|n| n.node_id == *node_id), + active_node_ids.contains(node_id), ); } } Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a performance issue with O(n*m) complexity and proposes an effective optimization using a HashSet for O(1) average time lookups, which significantly improves efficiency.	Medium
General	Fix HashMap capacity allocation The capacity allocation is incorrect when node_ids is empty, as the result size will be based on active notifications count, not input size. This could lead to unnecessary reallocations or wasted memory. crates/bcr-ebill-persistence/src/db/notification.rs [59-72] -let mut res: HashMap<NodeId, bool> = HashMap::with_capacity(node_ids.len()); +let mut res: HashMap<NodeId, bool> = if node_ids.is_empty() { + HashMap::with_capacity(result.len()) +} else { + HashMap::with_capacity(node_ids.len()) +}; if node_ids.is_empty() { for node_id_db in result { res.insert(node_id_db.node_id.to_owned(), true); } } else { for node_id in node_ids { res.insert( node_id.to_owned(), result.iter().any(|n| n.node_id == *node_id), ); } } Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that when node_ids is empty, the HashMap capacity is incorrectly initialized to 0, potentially causing reallocations, and provides a correct fix.	Low
Security	Use parameterized queries instead Using format! to construct SQL queries with user input creates a potential SQL injection vulnerability. Even though node_id_filter is controlled internally, this pattern is dangerous and should use parameterized queries instead. crates/bcr-ebill-persistence/src/db/notification.rs [58] -let result: Vec<NodeIdDb> = self.db.query(&format!("SELECT node_id from notifications where active = true {node_id_filter} GROUP BY node_id"), bindings).await?; +let query = if node_ids.is_empty() { + "SELECT node_id from notifications where active = true GROUP BY node_id" +} else { + "SELECT node_id from notifications where active = true and node_id in $node_ids GROUP BY node_id" +}; +let result: Vec<NodeIdDb> = self.db.query(query, bindings).await?; Apply / Chat Suggestion importance[1-10]: 5 __ Why: While there is no actual SQL injection vulnerability as the formatted string is not from user input, the suggestion correctly points out a bad practice and the improved code is a safer and cleaner pattern.	Low
Update

---

Previous suggestions

Suggestions up to commit d62a355

Category	Suggestion	Impact
General	Handle errors properly instead of unwrapping Using unwrap_or_default() silently swallows errors from the notification store, which could hide important failures. Consider properly handling the error or at least logging it before returning the default value. crates/bcr-ebill-api/src/service/notification_service/default_service.rs [757-761] -Ok(self - .notification_store +self.notification_store .get_active_status_for_node_ids(node_ids) .await - .unwrap_or_default()) Suggestion importance[1-10]: 8 __ Why: The suggestion correctly points out that unwrap_or_default() hides potential errors from the database layer, which should be propagated up to the caller for proper handling.	Medium
	Fix method name typo The method name has a typo - "notificattion" should be "notification". This typo appears consistently across multiple files in the PR and should be corrected to maintain code consistency and readability. crates/bcr-ebill-api/src/service/notification_service/default_service.rs [753-762] -async fn get_active_notificattion_status_for_node_ids( +async fn get_active_notification_status_for_node_ids( &self, node_ids: &[NodeId], ) -> Result<HashMap<NodeId, bool>> { Ok(self .notification_store .get_active_status_for_node_ids(node_ids) .await .unwrap_or_default()) } Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies a typo in the function name get_active_notificattion_status_for_node_ids, and fixing it improves code readability and maintainability.	Low

[codecov]

Codecov Report

Attention: Patch coverage is 45.23810% with 23 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
crates/bcr-ebill-wasm/src/api/notification.rs	0.00%	16 Missing ⚠️
...rc/service/notification_service/default_service.rs	0.00%	7 Missing ⚠️

📢 Thoughts on this report? Let us know!