Skip to content
This repository is currently being migrated. It's locked while the migration is in progress.

API 55333 POA slack alerts #27776

Merged
siddharthalamsal merged 8 commits into from
Apr 21, 2026
Merged

API 55333 POA slack alerts #27776

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
158d641
add alerts for silent failures
siddharthalamsal Apr 15, 2026
55c1284
Merge branch 'master' into api-55333-POA-alerts
siddharthalamsal Apr 15, 2026
69330ae
copilot changes
siddharthalamsal Apr 15, 2026
4fa56a8
Merge branch 'master' into api-55333-POA-alerts
siddharthalamsal Apr 16, 2026
34ddfc8
SlackNotifier module
siddharthalamsal Apr 16, 2026
e523994
refactor
siddharthalamsal Apr 16, 2026
5fe4c3e
remove duplicate method calls
siddharthalamsal Apr 16, 2026
a895e73
update logging
siddharthalamsal Apr 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 27 additions & 4 deletions ...claims_api/app/controllers/claims_api/v2/veterans/power_of_attorney/request_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
module ClaimsApi
module V2
module Veterans
class PowerOfAttorney::RequestController < ClaimsApi::V2::Veterans::PowerOfAttorney::BaseController
class PowerOfAttorney::RequestController < ClaimsApi::V2::Veterans::PowerOfAttorney::BaseController # rubocop:disable Metrics/ClassLength
Copy link
Copy Markdown
Contributor

@rmtolmach rmtolmach Apr 16, 2026

Choose a reason for hiding this comment

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

I'd like to discourage disabling the rubocop here and instead keeping the class smaller. Refactoring this might fit in with Rockwell's suggestion.

FORM_NUMBER = 'POA_REQUEST'
MAX_PAGE_SIZE = 100
MAX_PAGE_NUMBER = 100
Expand Down Expand Up @@ -194,7 +194,7 @@ def get_dependent_name(icn)
end

# 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
Copy link
Copy Markdown
Contributor

@rmtolmach rmtolmach Apr 16, 2026

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

@rockwellwindsor-va rockwellwindsor-va Apr 16, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Making ticket to refactor this further to remove the # rubocop:disable Metrics/ParameterLists disable as well. Tracked in API-55579

result = ClaimsApi::PowerOfAttorneyRequestService::DecisionHandler.new(
decision:, proc_id:, registration_number: representative_id, poa_code:, metadata:, veteran:, claimant:
).call
Expand All @@ -217,8 +217,12 @@ def process_poa_decision(decision:, proc_id:, representative_id:, poa_code:, met
power_of_attorney # return to the decide method for the response
rescue => e
claims_v2_logging('process_poa_decision',
level: :error,
message: "Failed to save power of attorney record. Error: #{e}")
raise e
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
end
# rubocop:enable Metrics/ParameterLists

Expand All @@ -242,7 +246,12 @@ def validate_mapped_data!(veteran_participant_id, type, poa_code)

def log_and_raise_decision_error_message!
claims_v2_logging('process_poa_decision',
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}")
Copy link
Copy Markdown
Contributor

@rockwellwindsor-va rockwellwindsor-va Apr 20, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Image

poa_request_slack_alert('POA Decide Request',
"Validation errors in mapped data for POA id: #{params[:id]} in #{Rails.env}: " \
"#{@claims_api_forms_validation_errors}")

raise ::Common::Exceptions::UnprocessableEntity.new(
detail: 'An error occurred while processing this decision. Please try again later.'
Expand Down Expand Up @@ -486,6 +495,20 @@ def page_number_to_index(number)
number - 1
end

def poa_request_slack_alert(source, message)
Copy link
Copy Markdown
Contributor

@rockwellwindsor-va rockwellwindsor-va Apr 16, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Copy Markdown
Contributor Author

@siddharthalamsal siddharthalamsal Apr 16, 2026

Choose a reason for hiding this comment

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

Great idea, I like the module plan, on it!

webhook_url = Settings.claims_api.slack.webhook_url.to_s
return if webhook_url.blank?

slack_client = SlackNotify::Client.new(webhook_url:,
channel: '#api-benefits-claims-alerts',
username: "Failed #{source}")
slack_client.notify(message)
rescue => e
ClaimsApi::Logger.log('poa_request_slack_alert',
level: :error,
message: "Failed to send Slack alert: #{e.message}")
end

def normalize(item)
item.to_s.strip.downcase
end
Expand Down
115 changes: 115 additions & 0 deletions modules/claims_api/spec/controllers/v2/veterans/power_of_attorney/request_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1199,4 +1199,119 @@ def create_request_with(veteran_id:, form_attributes:, auth_header:)
params: { data: { attributes: form_attributes } }.to_json,
headers: auth_header.merge('Content-Type' => 'application/json')
end

describe '#poa_request_slack_alert' do
let(:slack_client) { instance_double(SlackNotify::Client) }

before do
allow(SlackNotify::Client).to receive(:new).and_return(slack_client)
allow(slack_client).to receive(:notify)
end

it 'sends a Slack notification with the correct parameters' do
subject.send(:poa_request_slack_alert, 'POA Decide Request', 'test message')

expect(SlackNotify::Client).to have_received(:new).with(
webhook_url: Settings.claims_api.slack.webhook_url.to_s,
channel: '#api-benefits-claims-alerts',
username: 'Failed POA Decide Request'
)
expect(slack_client).to have_received(:notify).with('test message')
end

context 'when webhook_url is blank' do
before do
allow(Settings.claims_api.slack).to receive(:webhook_url).and_return(nil)
end

it 'does not attempt to send a Slack notification' do
subject.send(:poa_request_slack_alert, 'POA Decide Request', 'test message')

expect(SlackNotify::Client).not_to have_received(:new)
end
end

context 'when Slack notification fails' do
before do
allow(slack_client).to receive(:notify).and_raise(StandardError.new('Slack down'))
end

it 'logs the failure and does not raise' do
expect(ClaimsApi::Logger).to receive(:log).with(
'poa_request_slack_alert',
level: :error,
message: 'Failed to send Slack alert: Slack down'
)

expect { subject.send(:poa_request_slack_alert, 'POA Decide Request', 'test') }.not_to raise_error
end
end
end

describe '#log_and_raise_decision_error_message!' do
let(:slack_client) { instance_double(SlackNotify::Client) }

before do
allow(SlackNotify::Client).to receive(:new).and_return(slack_client)
allow(slack_client).to receive(:notify)
allow_any_instance_of(described_class).to receive(:claims_v2_logging)
allow_any_instance_of(described_class).to receive(:params).and_return({ id: 'abc-123' })
end

it 'sends a Slack alert including the POA request id and validation errors before raising' do
subject.instance_variable_set(:@claims_api_forms_validation_errors, ['field X is invalid'])

expect(slack_client).to receive(:notify).with(
a_string_including('id: abc-123').and(a_string_including('field X is invalid'))
)

expect { subject.send(:log_and_raise_decision_error_message!) }.to raise_error(
Common::Exceptions::UnprocessableEntity
)
end

it 'logs at error level with the POA request id and validation errors' do
subject.instance_variable_set(:@claims_api_forms_validation_errors, ['field X is invalid'])

expect_any_instance_of(described_class).to receive(:claims_v2_logging).with(
'process_poa_decision',
level: :error,
message: a_string_including('id: abc-123').and(a_string_including('field X is invalid'))
)

expect { subject.send(:log_and_raise_decision_error_message!) }.to raise_error(
Common::Exceptions::UnprocessableEntity
)
end
end

describe '#process_poa_decision rescue' do
let(:slack_client) { instance_double(SlackNotify::Client) }
let(:params) do
{
decision: 'accepted', proc_id: '12345', representative_id: '999',
poa_code: '083', metadata: {}, veteran: nil, claimant: nil
}
end

before do
allow(SlackNotify::Client).to receive(:new).and_return(slack_client)
allow(slack_client).to receive(:notify)
allow_any_instance_of(described_class).to receive(:claims_v2_logging)
allow_any_instance_of(described_class).to receive(:params).and_return({ id: 'lh-id-456' })
allow_any_instance_of(
ClaimsApi::PowerOfAttorneyRequestService::DecisionHandler
).to receive(:call).and_raise(StandardError.new('BGS exploded'))
end

it 'sends a Slack alert with the lighthouse id, proc id, and error message' do
expect(slack_client).to receive(:notify).with(
a_string_including('id: lh-id-456')
.and(a_string_including('procId: 12345'))
.and(a_string_including('BGS exploded'))
)

expect { subject.send(:process_poa_decision, **params) }.to raise_error(StandardError, 'BGS exploded')
end
end
end
Loading