Skip to content

changelog: Improvements, In-Person Proofing, uses correct error handling#7192

Merged
allthesignals merged 1 commit intomainfrom
wmg/7718-usps-location-improved-error-handling
Oct 24, 2022
Merged

changelog: Improvements, In-Person Proofing, uses correct error handling#7192
allthesignals merged 1 commit intomainfrom
wmg/7718-usps-location-improved-error-handling

Conversation

@allthesignals
Copy link
Contributor

@allthesignals allthesignals commented Oct 22, 2022

🎫 Ticket

Link to the relevant ticket.

🛠 Summary of changes

Implements better error handling by passing the correct args (context).

📜 Testing Plan

Instead of "Faraday::ClientError: the server responded with status" we get a more help message:

 # Faraday::ClientError:
 #   received error code 400

Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +52 to +54
error_code = response_body.dig('error', 'code')

raise Faraday::ClientError.new(RuntimeError.new("received error code #{error_code}"), response_body)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add / update specs to clarify what we're expecting with these changes? It'd also help from a review perspective to understand the intended effect.

Something like this?

    it 'returns an error response body but with Status coded as 200' do
      stub_request_suggestions_error

      expect { subject.suggest('100 Main') }.to raise_error do |error|
        expect(error).to be_instance_of(Faraday::ClientError)
        expect(error.message).to eq('received error code 400')
        expect(error.response).to be_kind_of(Hash)
      end
    end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, @zachmargolis pointed out in a previous PR that I was using ClientError.new incorrectly. This change adds a more helpful message.

I'll add more assertions.

Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to adding specs

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +52 to +54
error_code = response_body.dig('error', 'code')

raise Faraday::ClientError.new(RuntimeError.new("received error code #{error_code}"), response_body)
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to adding specs

@allthesignals allthesignals force-pushed the wmg/7718-usps-location-improved-error-handling branch from ddd4275 to 0170282 Compare October 24, 2022 16:52
@allthesignals allthesignals merged commit 926df65 into main Oct 24, 2022
@allthesignals allthesignals deleted the wmg/7718-usps-location-improved-error-handling branch October 24, 2022 17:13
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.

3 participants