carverauto/serviceradar

Fork 0

rulebuilder testing/integration work #2692

Merged

mfreeman451 merged 2 commits from refs/pull/2692/head into staging

2026-01-18 08:55:13 +00:00

mfreeman451 commented

2026-01-18 08:40:07 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #2342
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/2342
Original created: 2026-01-18T08:40:07Z
Original updated: 2026-01-18T08:55:15Z
Original head: carverauto/serviceradar:rulebuilder/wip
Original base: staging
Original merged: 2026-01-18T08:55:13Z by @mfreeman451

User description

IMPORTANT: Please sign the Developer Certificate of Origin

Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include
a DCO sign-off statement indicating the DCO acceptance in one commit message. Here
is an example DCO Signed-off-by line in a commit message:

Signed-off-by: J. Doe <j.doe@domain.com>

Describe your changes

Issue ticket number and link

Code checklist before requesting a review

I have signed the DCO?
The build completes without errors?
All tests are passing when running make test?

PR Type

Enhancement, Tests

Description

Add error handling for rule creation failures with user feedback
Link to Settings Rules page for advanced configuration options
Mark completed tasks in project tracking documentation

Diagram Walkthrough

flowchart LR
  A["Rule Creation"] --> B{"Success or Failure?"}
  B -->|Success| C["Show Success Flash"]
  B -->|Failure| D["Handle Error Message"]
  D --> E["Display Error Flash"]
  C --> F["Close Modal"]
  E --> F
  G["Rule Builder UI"] --> H["Add Settings Link"]
  H --> I["Navigate to Rules Settings"]

File Walkthrough

Relevant files

Enhancement

promotion_rule_builder.ex

Add settings link for advanced rule configuration

web-ng/lib/serviceradar_web_ng_web/components/promotion_rule_builder.ex

Add inline link to Settings Rules page for advanced configuration
Link navigates to /settings/rules?tab=events with primary styling
Improves user discoverability of advanced rule configuration options

+1/-0

Error handling

show.ex

Add error handling for failed rule creation

web-ng/lib/serviceradar_web_ng_web/live/log_live/show.ex

Implement handle_info/2 handler for {:rule_creation_failed, reason}
message
Display formatted error message to user via error flash notification
Gracefully handle rule creation failures with user-friendly feedback

+6/-0

Documentation

tasks.md

Mark completed rule builder implementation tasks

openspec/changes/add-log-to-event-ui/tasks.md

Mark task 3.7 as completed for rule creation error handling
Mark task 8.3 as completed for settings page link implementation
Update project tracking to reflect finished implementation work

+2/-2

Imported from GitHub pull request. Original GitHub pull request: #2342 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/2342 Original created: 2026-01-18T08:40:07Z Original updated: 2026-01-18T08:55:15Z Original head: carverauto/serviceradar:rulebuilder/wip Original base: staging Original merged: 2026-01-18T08:55:13Z by @mfreeman451 --- ### **User description** ## IMPORTANT: Please sign the Developer Certificate of Origin Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include a [DCO sign-off statement]( https://developercertificate.org/) indicating the DCO acceptance in one commit message. Here is an example DCO Signed-off-by line in a commit message: ``` Signed-off-by: J. Doe <j.doe@domain.com> ``` ## Describe your changes ## Issue ticket number and link ## Code checklist before requesting a review - [ ] I have signed the DCO? - [ ] The build completes without errors? - [ ] All tests are passing when running make test? ___ ### **PR Type** Enhancement, Tests ___ ### **Description** - Add error handling for rule creation failures with user feedback - Link to Settings Rules page for advanced configuration options - Mark completed tasks in project tracking documentation ___ ### Diagram Walkthrough ```mermaid flowchart LR A["Rule Creation"] --> B{"Success or Failure?"} B -->|Success| C["Show Success Flash"] B -->|Failure| D["Handle Error Message"] D --> E["Display Error Flash"] C --> F["Close Modal"] E --> F G["Rule Builder UI"] --> H["Add Settings Link"] H --> I["Navigate to Rules Settings"] ``` <details><summary><h3>File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Enhancement</strong></td><td><table> <tr> <td> <details> <summary><strong>promotion_rule_builder.ex</strong><dd><code>Add settings link for advanced rule configuration</code>                </dd></summary> <hr> web-ng/lib/serviceradar_web_ng_web/components/promotion_rule_builder.ex <ul><li>Add inline link to Settings Rules page for advanced configuration<br> <li> Link navigates to <code>/settings/rules?tab=events</code> with primary styling<br> <li> Improves user discoverability of advanced rule configuration options</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2342/files#diff-0226580d3777904915943339ececa4e0e314a03a7c43a0e9afec64fe2a8f9354">+1/-0</a>      </td> </tr> </table></td></tr><tr><td><strong>Error handling</strong></td><td><table> <tr> <td> <details> <summary><strong>show.ex</strong><dd><code>Add error handling for failed rule creation</code>                            </dd></summary> <hr> web-ng/lib/serviceradar_web_ng_web/live/log_live/show.ex <ul><li>Implement <code>handle_info/2</code> handler for <code>{:rule_creation_failed, reason}</code> <br>message<br> <li> Display formatted error message to user via error flash notification<br> <li> Gracefully handle rule creation failures with user-friendly feedback</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2342/files#diff-4f9769353c55928a0d382cd7510379444445aea116e1ecdf7b8eb892d249ff26">+6/-0</a>      </td> </tr> </table></td></tr><tr><td><strong>Documentation</strong></td><td><table> <tr> <td> <details> <summary><strong>tasks.md</strong><dd><code>Mark completed rule builder implementation tasks</code>                  </dd></summary> <hr> openspec/changes/add-log-to-event-ui/tasks.md <ul><li>Mark task 3.7 as completed for rule creation error handling<br> <li> Mark task 8.3 as completed for settings page link implementation<br> <li> Update project tracking to reflect finished implementation work</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2342/files#diff-ee81afe728f07f39c4cd4f0da61e541d8a818cb2f8f24c83b81c952dc1c85098">+2/-2</a>      </td> </tr> </table></td></tr></tbody></table> </details> ___

qodo-code-review[bot] commented

2026-01-18 08:40:33 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/2342#issuecomment-3765062758
Original created: 2026-01-18T08:40:33Z

ⓘ Your approaching your monthly quota for Qodo. Upgrade your plan

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Information disclosure Description: The new error flash message interpolates `format_error(reason)` directly into user-visible output, which could disclose sensitive internal details (e.g., database errors, stack traces, tokens, or configuration data) if `reason` contains such information; ensure `format_error/1` strictly sanitizes and maps errors to safe, non-sensitive messages. show.ex [77-81] Referred Code `def handle_info({:rule_creation_failed, reason}, socket) do {:noreply, socket \|> put_flash(:error, "Failed to create rule: #{format_error(reason)}")} end`
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Missing failure logging: The new `{:rule_creation_failed, reason}` handler displays an error to the user but does not clearly log the failure context for production debugging and monitoring. Referred Code `def handle_info({:rule_creation_failed, reason}, socket) do {:noreply, socket \|> put_flash(:error, "Failed to create rule: #{format_error(reason)}")} end` Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Error detail exposure: The user-facing flash message interpolates `format_error(reason)`, which may leak internal error details depending on how `reason` is constructed and formatted. Referred Code `def handle_info({:rule_creation_failed, reason}, socket) do {:noreply, socket \|> put_flash(:error, "Failed to create rule: #{format_error(reason)}")} end` Learn more about managing compliance generic rules or creating your own custom rules
Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2342#issuecomment-3765062758 Original created: 2026-01-18T08:40:33Z --- <pre>ⓘ Your approaching your monthly quota for Qodo. <a href="https://www.qodo.ai/pricing">Upgrade your plan</a></pre> ## PR Compliance Guide 🔍  Below is a summary of compliance checks for this PR:<br> <table><tbody><tr><td colspan='2'><strong>Security Compliance</strong></td></tr> <tr><td rowspan=1>⚪</td> <td><details><summary><strong>Information disclosure </strong></summary><br> <b>Description:</b> The new error flash message interpolates <code>format_error(reason)</code> directly into user-visible <br>output, which could disclose sensitive internal details (e.g., database errors, stack <br>traces, tokens, or configuration data) if <code>reason</code> contains such information; ensure <br><code>format_error/1</code> strictly sanitizes and maps errors to safe, non-sensitive messages.<br> <strong><a href='https://github.com/carverauto/serviceradar/pull/2342/files#diff-4f9769353c55928a0d382cd7510379444445aea116e1ecdf7b8eb892d249ff26R77-R81'>show.ex [77-81]</a></strong><br> <details open><summary>Referred Code</summary> ```elixir def handle_info({:rule_creation_failed, reason}, socket) do {:noreply, socket |> put_flash(:error, "Failed to create rule: #{format_error(reason)}")} end ``` </details></details></td></tr> <tr><td colspan='2'><strong>Ticket Compliance</strong></td></tr> <tr><td>⚪</td><td><details><summary>🎫 <strong>No ticket provided </strong></summary> - [ ] Create ticket/issue  </details></td></tr> <tr><td colspan='2'><strong>Codebase Duplication Compliance</strong></td></tr> <tr><td>⚪</td><td><details><summary><strong>Codebase context is not defined </strong></summary> Follow the <a href='https://qodo-merge-docs.qodo.ai/core-abilities/rag_context_enrichment/'>guide</a> to enable codebase context checks. </details></td></tr> <tr><td colspan='2'><strong>Custom Compliance</strong></td></tr> <tr><td rowspan=4>🟢</td><td> <details><summary><strong>Generic: Comprehensive Audit Trails</strong></summary><br> **Objective:** To create a detailed and reliable record of critical system actions for security analysis <br>and compliance.<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary><strong>Generic: Meaningful Naming and Self-Documenting Code</strong></summary><br> **Objective:** Ensure all identifiers clearly express their purpose and intent, making code <br>self-documenting<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary><strong>Generic: Secure Logging Practices</strong></summary><br> **Objective:** To ensure logs are useful for debugging and auditing without exposing sensitive <br>information like PII, PHI, or cardholder data.<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary><strong>Generic: Security-First Input Validation and Data Handling</strong></summary><br> **Objective:** Ensure all data inputs are validated, sanitized, and handled securely to prevent <br>vulnerabilities<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td rowspan=2>⚪</td> <td><details> <summary><strong>Generic: Robust Error Handling and Edge Case Management</strong></summary><br> **Objective:** Ensure comprehensive error handling that provides meaningful context and graceful <br>degradation<br> **Status:** <br><a href='https://github.com/carverauto/serviceradar/pull/2342/files#diff-4f9769353c55928a0d382cd7510379444445aea116e1ecdf7b8eb892d249ff26R77-R81'><strong>Missing failure logging</strong></a>: The new <code>{:rule_creation_failed, reason}</code> handler displays an error to the user but does not <br>clearly log the failure context for production debugging and monitoring.<br> <details open><summary>Referred Code</summary> ```elixir def handle_info({:rule_creation_failed, reason}, socket) do {:noreply, socket |> put_flash(:error, "Failed to create rule: #{format_error(reason)}")} end ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td><details> <summary><strong>Generic: Secure Error Handling</strong></summary><br> **Objective:** To prevent the leakage of sensitive system information through error messages while <br>providing sufficient detail for internal debugging.<br> **Status:** <br><a href='https://github.com/carverauto/serviceradar/pull/2342/files#diff-4f9769353c55928a0d382cd7510379444445aea116e1ecdf7b8eb892d249ff26R77-R81'><strong>Error detail exposure</strong></a>: The user-facing flash message interpolates <code>format_error(reason)</code>, which may leak internal <br>error details depending on how <code>reason</code> is constructed and formatted.<br> <details open><summary>Referred Code</summary> ```elixir def handle_info({:rule_creation_failed, reason}, socket) do {:noreply, socket |> put_flash(:error, "Failed to create rule: #{format_error(reason)}")} end ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td align="center" colspan="2"> - [ ] Update  </td></tr></tbody></table> <details><summary>Compliance status legend</summary> 🟢 - Fully Compliant<br> 🟡 - Partial Compliant<br> 🔴 - Not Compliant<br> ⚪ - Requires Further Human Verification<br> 🏷️ - Compliance label<br> </details>

qodo-code-review[bot] commented

2026-01-18 08:41:31 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/2342#issuecomment-3765063446
Original created: 2026-01-18T08:41:31Z

ⓘ Your approaching your monthly quota for Qodo. Upgrade your plan

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Close modal on rule creation failure In the `handle_info` function for a failed rule creation, close the rule builder modal by adding `assign(:show_rule_builder, false)` to provide a consistent user experience with the success case. web-ng/lib/serviceradar_web_ng_web/live/log_live/show.ex [77-81] `def handle_info({:rule_creation_failed, reason}, socket) do {:noreply, socket + \|> assign(:show_rule_builder, false) \|> put_flash(:error, "Failed to create rule: #{format_error(reason)}")} end` Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that the rule builder modal is not closed on failure, which is inconsistent with the success case and leads to a poor user experience.	Medium
General	Use verified routes for navigation links Replace the hardcoded URL string in the `<.link>` component's `navigate` attribute with a verified route using the `~p` sigil for improved maintainability. web-ng/lib/serviceradar_web_ng_web/components/promotion_rule_builder.ex [166] `-For advanced configuration, visit <.link navigate="/settings/rules?tab=events" class="link link-primary">Settings → Rules</.link>. +For advanced configuration, visit <.link navigate={~p"/settings/rules?tab=events"} class="link link-primary">Settings → Rules</.link>.` Apply / Chat Suggestion importance[1-10]: 5 __ Why: The suggestion correctly points out that using the `~p` sigil for verified routes is a best practice in Phoenix, which improves code maintainability and robustness against future route changes.	Low
Update

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2342#issuecomment-3765063446 Original created: 2026-01-18T08:41:31Z --- <pre>ⓘ Your approaching your monthly quota for Qodo. <a href="https://www.qodo.ai/pricing">Upgrade your plan</a></pre> ## PR Code Suggestions ✨  Explore these optional code suggestions: <table><thead><tr><td><strong>Category</strong></td><td align=left><strong>Suggestion                                                                                                                                    </strong></td><td align=center><strong>Impact</strong></td></tr><tbody><tr><td rowspan=1>Possible issue</td> <td> <details><summary>Close modal on rule creation failure</summary> ___ **In the <code>handle_info</code> function for a failed rule creation, close the rule builder <br>modal by adding <code>assign(:show_rule_builder, false)</code> to provide a consistent user <br>experience with the success case.** [web-ng/lib/serviceradar_web_ng_web/live/log_live/show.ex [77-81]](https://github.com/carverauto/serviceradar/pull/2342/files#diff-4f9769353c55928a0d382cd7510379444445aea116e1ecdf7b8eb892d249ff26R77-R81) ```diff def handle_info({:rule_creation_failed, reason}, socket) do {:noreply, socket + |> assign(:show_rule_builder, false) |> put_flash(:error, "Failed to create rule: #{format_error(reason)}")} end ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly identifies that the rule builder modal is not closed on failure, which is inconsistent with the success case and leads to a poor user experience. </details></details></td><td align=center>Medium </td></tr><tr><td rowspan=1>General</td> <td> <details><summary>Use verified routes for navigation links</summary> ___ **Replace the hardcoded URL string in the <code><.link></code> component's <code>navigate</code> attribute with a <br>verified route using the <code>~p</code> sigil for improved maintainability.** [web-ng/lib/serviceradar_web_ng_web/components/promotion_rule_builder.ex [166]](https://github.com/carverauto/serviceradar/pull/2342/files#diff-0226580d3777904915943339ececa4e0e314a03a7c43a0e9afec64fe2a8f9354R166-R166) ```diff -For advanced configuration, visit <.link navigate="/settings/rules?tab=events" class="link link-primary">Settings → Rules</.link>. +For advanced configuration, visit <.link navigate={~p"/settings/rules?tab=events"} class="link link-primary">Settings → Rules</.link>. ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 5</summary> __ Why: The suggestion correctly points out that using the `~p` sigil for verified routes is a best practice in Phoenix, which improves code maintainability and robustness against future route changes. </details></details></td><td align=center>Low </td></tr> <tr><td align="center" colspan="2"> - [ ] Update  </td><td></td></tr></tbody></table>