Conversation
There was a problem hiding this comment.
Pull request overview
Adds Slack alerting for “silent failure” scenarios in the Claims API v2 POA decide flow so operational teams get notified when mapped BGS data validation fails or decision processing errors out.
Changes:
- Adds
poa_request_slack_alerthelper and calls it fromprocess_poa_decisionrescue and mapped-data validation error handling. - Improves error-level logging and message detail for POA decision failures.
- Adds specs covering Slack notification behavior and the new alerting paths.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
modules/claims_api/app/controllers/claims_api/v2/veterans/power_of_attorney/request_controller.rb |
Adds Slack alert helper + invokes it on decision-processing failures and mapped-data validation failures. |
modules/claims_api/spec/controllers/v2/veterans/power_of_attorney/request_controller_spec.rb |
Adds unit coverage for Slack notifier helper and the new alerting behavior. |
| poa_request_slack_alert('POA Decide Request', | ||
| "Failed to process POA decision for id: #{params[:id]}, " \ | ||
| "procId: #{proc_id} in #{Rails.env}: #{e.message}") | ||
| raise e |
There was a problem hiding this comment.
In this rescue block, raise e will reset the exception backtrace, which makes debugging production failures harder. Use raise to re-raise the current exception while preserving the original backtrace (and keep the Slack/logging behavior the same).
| raise e | |
| raise |
| end | ||
|
|
||
| def poa_request_slack_alert(source, message) | ||
| slack_client = SlackNotify::Client.new(webhook_url: Settings.claims_api.slack.webhook_url, |
There was a problem hiding this comment.
Settings.claims_api.slack.webhook_url should be treated as untrusted input (it may be nil or an unexpected type due to Parameter Store / env parsing). Consider coercing to a string with a safe fallback and skipping Slack notify when blank, so this method doesn’t raise/log errors repeatedly in environments where the webhook isn’t configured.
| slack_client = SlackNotify::Client.new(webhook_url: Settings.claims_api.slack.webhook_url, | |
| webhook_url = Settings.claims_api.slack.webhook_url.to_s.strip | |
| return if webhook_url.blank? | |
| slack_client = SlackNotify::Client.new(webhook_url: webhook_url, |
| slack_client = SlackNotify::Client.new(webhook_url: Settings.claims_api.slack.webhook_url, | ||
| channel: '#api-benefits-claims-alerts', | ||
| username: "Failed #{source}") |
There was a problem hiding this comment.
PR description/manual testing notes mention alerts appearing in #vaapi-alerts-testing, but the code hard-codes channel: '#api-benefits-claims-alerts'. If the intended channel differs by environment, consider making the channel configurable (or update the PR description to match the actual target).
There was a problem hiding this comment.
this doest seem to effect anything for testing
jeremyhunton-va
left a comment
There was a problem hiding this comment.
LGTM! Fantastic testing plan!
| number - 1 | ||
| end | ||
|
|
||
| def poa_request_slack_alert(source, message) |
There was a problem hiding this comment.
@siddharthalamsal This slack alert method could potentially be something we want to utilize elsewhere in the app at some point. Wondering what you thoughts would be on making this a module that we can include in this controller? Something like ClaimsApi::SlackNotifier etc. . We could rename the method as request_slack_alert it is not POA specific, we can keep the signature the same but then if we want to include this on other end points that could be failing silently it would be an easy update.
Thinking of a utilization along the lines of the claims_vs_logging . But it could just be included at that top level (application_controller) as a module, or just the method there just like the logging, that may also be enough given it is just the one method.
I realize it is a bit of a refactor to the work here. But worth it to perhaps get in place from the start instead of hoping the refactor comes in the future.
Let me know you think of the idea.
There was a problem hiding this comment.
Great idea, I like the module plan, on it!
| module V2 | ||
| module Veterans | ||
| class PowerOfAttorney::RequestController < ClaimsApi::V2::Veterans::PowerOfAttorney::BaseController | ||
| class PowerOfAttorney::RequestController < ClaimsApi::V2::Veterans::PowerOfAttorney::BaseController # rubocop:disable Metrics/ClassLength |
There was a problem hiding this comment.
I'd like to discourage disabling the rubocop here and instead keeping the class smaller. Refactoring this might fit in with Rockwell's suggestion.
|
|
||
| # rubocop:disable Metrics/ParameterLists | ||
| def process_poa_decision(decision:, proc_id:, representative_id:, poa_code:, metadata:, veteran:, claimant:) | ||
| def process_poa_decision(decision:, proc_id:, representative_id:, poa_code:, metadata:, veteran:, claimant:) # rubocop:disable Metrics/MethodLength |
There was a problem hiding this comment.
Can you refactor this method so it's more object oriented and less procedural? Just based on the comments, there could be a method to build headers, and a method to save the record.
(the main ask here is to avoid disabling rubocop.)
There was a problem hiding this comment.
Making ticket to refactor this further to remove the # rubocop:disable Metrics/ParameterLists disable as well. Tracked in API-55579
Generated by 🚫 Danger |
| def validate_page_size_and_number_params | ||
| return if use_defaults? | ||
|
|
||
| valid_page_param?('size') if params[:page][:size] |
There was a problem hiding this comment.
[reek] reported by reviewdog 🐶
DuplicateMethodCall: ClaimsApi::V2::PowerOfAttorneyRequests::IndexValidation#validate_page_size_and_number_params calls 'params[:page]' 6 times [https://github.com/troessner/reek/blob/v6.5.0/docs/Duplicate-Method-Call.md]
|
|
||
| def normalize(item) | ||
| item.to_s.strip.downcase | ||
| def build_bgs_attributes(form_attributes) |
There was a problem hiding this comment.
This is showing up as new code in the git diff, but it originally was in this file (see removed chunk above), i moved it to the new file to reduces total lines, but then that file had too many lines so i moved it back 🙃
|
giving this 👀 now |
| message: 'Encountered issues validating the mapped data') | ||
| level: :error, | ||
| message: "Encountered issues validating the mapped data for POA id: #{params[:id]}: " \ | ||
| "#{@claims_api_forms_validation_errors}") |
There was a problem hiding this comment.
I'm not actually gettin the text in the error log I was hoping for. I think we need an update to the log_and_raise_decision_error_message bit for the JsonSchema::JsonApiMissingAttribute. to include the error messages text so wee can see that.
Something along these lines (pseudo code here just FYI)
# JSON validations, all errors, including errors from the custom validations
# will be raised here if JSON errors exist
validate_json_schema(type.upcase)
# otherwise we raise the errors from the custom validations if no JSON
# errors exist
log_and_raise_decision_error_message!(@claims_api_forms_validation_errors)
rescue JsonSchema::JsonApiMissingAttribute => e
log_and_raise_decision_error_message!(e.to_json_api)
end
I'm not a huge fan of passing in the instance variable for the errors, seems odd but I think we need to send in the errors since it would be one or the other. This would look like this which I think helps us understand the error more, otherwise we know it is a 422, but have no idea why.
|
Giving this 👀 now |
* add alerts for silent failures * copilot changes * SlackNotifier module * refactor * remove duplicate method calls * update logging

Summary
Adds slack alerts for silent failures in v2 POA decide endpoint
Related issue(s)
API 55333

Testing done
Manual testing in rails console (must have
claims_api.slacksetup in settings):validate_mapped_data!testprocess_poa_decisiontestYou should see alerts like these in #vaapi-alerts-testing:

What areas of the site does it impact?
Acceptance criteria