From d102652c038c0c2e1901fcd191430dd5a44a3ecf Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 18 Nov 2024 11:25:39 +1100 Subject: [PATCH] Fix Vine::VoucherValidatorService to handle exceptions --- .../voucher_adjustments_controller.rb | 9 +--- .../vine/voucher_validator_service.rb | 17 +++---- spec/requests/voucher_adjustments_spec.rb | 5 +- .../vine/voucher_validator_service_spec.rb | 49 +++++++------------ 4 files changed, 28 insertions(+), 52 deletions(-) diff --git a/app/controllers/voucher_adjustments_controller.rb b/app/controllers/voucher_adjustments_controller.rb index 3b774d46ea..cb9d0c83e4 100644 --- a/app/controllers/voucher_adjustments_controller.rb +++ b/app/controllers/voucher_adjustments_controller.rb @@ -91,9 +91,9 @@ class VoucherAdjustmentsController < BaseController ) voucher = vine_voucher_validator.validate - if vine_voucher_validator.errors.present? - return nil if ignored_vine_api_error(vine_voucher_validator) + return nil if vine_voucher_validator.errors[:not_found_voucher].present? + if vine_voucher_validator.errors.present? @order.errors.add(:voucher_code, I18n.t('checkout.errors.add_voucher_error')) return nil end @@ -101,11 +101,6 @@ class VoucherAdjustmentsController < BaseController voucher end - def ignored_vine_api_error(validator) - # Ignore distributor not set up for VINE or not found error - validator.errors[:vine_settings].present? || validator.errors[:not_found_voucher].present? - end - def update_payment_section render cable_ready: cable_car.replace( selector: "#checkout-payment-methods", diff --git a/app/services/vine/voucher_validator_service.rb b/app/services/vine/voucher_validator_service.rb index 0eba5ee3b3..8311a5fc9f 100644 --- a/app/services/vine/voucher_validator_service.rb +++ b/app/services/vine/voucher_validator_service.rb @@ -11,19 +11,14 @@ module Vine end def validate - if vine_settings.nil? - errors[:vine_settings] = I18n.t("vine_voucher_validator_service.errors.vine_settings") - return nil - end + return nil if vine_settings.nil? response = call_vine_api - if !response.success? - handle_errors(response) - return nil - end - save_voucher(response) + rescue Faraday::ClientError => e + handle_errors(e.response) + nil rescue Faraday::Error => e Rails.logger.error e.inspect Bugsnag.notify(e) @@ -47,9 +42,9 @@ module Vine end def handle_errors(response) - if response.status == 400 + if response[:status] == 400 errors[:invalid_voucher] = I18n.t("vine_voucher_validator_service.errors.invalid_voucher") - elsif response.status == 404 + elsif response[:status] == 404 errors[:not_found_voucher] = I18n.t("vine_voucher_validator_service.errors.not_found_voucher") else diff --git a/spec/requests/voucher_adjustments_spec.rb b/spec/requests/voucher_adjustments_spec.rb index 074b8e1a8a..e4b3712a46 100644 --- a/spec/requests/voucher_adjustments_spec.rb +++ b/spec/requests/voucher_adjustments_spec.rb @@ -118,10 +118,7 @@ RSpec.describe VoucherAdjustmentsController, type: :request do context "when coordinator is not connected to VINE" do it "returns 422 and an error message" do - mock_vine_voucher_validator( - voucher: nil, - errors: { vine_settings: "No Vine api settings for the given enterprise" } - ) + mock_vine_voucher_validator(voucher: nil) post("/voucher_adjustments", params:) diff --git a/spec/services/vine/voucher_validator_service_spec.rb b/spec/services/vine/voucher_validator_service_spec.rb index ad795d43c7..2c263d3f97 100644 --- a/spec/services/vine/voucher_validator_service_spec.rb +++ b/spec/services/vine/voucher_validator_service_spec.rb @@ -42,15 +42,15 @@ RSpec.describe Vine::VoucherValidatorService, feature: :connected_apps do let(:vine_voucher_set_id) { "9d24349c-1fe8-4090-988b-d7355ed32559" } it "verifies the voucher with VINE API" do - expect(vine_api_service).to receive(:voucher_validation) - .and_return(mock_api_response(success: true, data:)) + expect(vine_api_service).to receive(:voucher_validation).and_return( + mock_api_response(data:) + ) validate_voucher_service.validate end it "creates a new VINE voucher" do - allow(vine_api_service).to receive(:voucher_validation) - .and_return(mock_api_response(success: true, data:)) + allow(vine_api_service).to receive(:voucher_validation).and_return(mock_api_response(data:)) vine_voucher = validate_voucher_service.validate @@ -88,7 +88,7 @@ RSpec.describe Vine::VoucherValidatorService, feature: :connected_apps do external_voucher_id: vine_voucher_id, external_voucher_set_id: vine_voucher_set_id) allow(vine_api_service).to receive(:voucher_validation) - .and_return(mock_api_response(success: true, data:)) + .and_return(mock_api_response(data:)) vine_voucher = validate_voucher_service.validate @@ -129,7 +129,7 @@ RSpec.describe Vine::VoucherValidatorService, feature: :connected_apps do external_voucher_id: vine_voucher_id, external_voucher_set_id: vine_voucher_set_id) allow(vine_api_service).to receive(:voucher_validation) - .and_return(mock_api_response(success: true, data:)) + .and_return(mock_api_response(data:)) vine_voucher = validate_voucher_service.validate @@ -155,14 +155,6 @@ RSpec.describe Vine::VoucherValidatorService, feature: :connected_apps do validate_voucher_service.validate end - it "adds an error message" do - validate_voucher_service.validate - - expect(validate_voucher_service.errors).to include( - { vine_settings: "This shop is not enabled for VINE Vouchers." } - ) - end - it "doesn't creates a new VINE voucher" do expect_voucher_count_not_to_change end @@ -176,7 +168,7 @@ RSpec.describe Vine::VoucherValidatorService, feature: :connected_apps do } before do - allow(vine_api_service).to receive(:voucher_validation).and_raise(Faraday::ConnectionFailed) + mock_api_exception(type: Faraday::ConnectionFailed) end it "returns nil" do @@ -218,9 +210,7 @@ RSpec.describe Vine::VoucherValidatorService, feature: :connected_apps do } before do - allow(vine_api_service).to receive(:voucher_validation).and_return( - mock_api_response(success: false, status: 401, data: ) - ) + mock_api_exception(type: Faraday::UnauthorizedError, status: 401, body: data) end it "returns nil" do @@ -254,9 +244,7 @@ RSpec.describe Vine::VoucherValidatorService, feature: :connected_apps do } before do - allow(vine_api_service).to receive(:voucher_validation).and_return( - mock_api_response(success: false, status: 404, data: ) - ) + mock_api_exception(type: Faraday::ResourceNotFound, status: 404, body: data) end it "returns nil" do @@ -290,9 +278,7 @@ RSpec.describe Vine::VoucherValidatorService, feature: :connected_apps do } before do - allow(vine_api_service).to receive(:voucher_validation).and_return( - mock_api_response(success: false, status: 400, data: ) - ) + mock_api_exception(type: Faraday::BadRequestError, status: 400, body: data) end it "returns nil" do @@ -339,7 +325,7 @@ RSpec.describe Vine::VoucherValidatorService, feature: :connected_apps do before do allow(vine_api_service).to receive(:voucher_validation).and_return( - mock_api_response(success: true, status: 200, data: ) + mock_api_response(data: ) ) end @@ -384,13 +370,13 @@ RSpec.describe Vine::VoucherValidatorService, feature: :connected_apps do before do allow(vine_api_service).to receive(:voucher_validation).and_return( - mock_api_response(success: true, status: 200, data: ) + mock_api_response(data: ) ) end it "verify the voucher with VINE API" do expect(vine_api_service).to receive(:voucher_validation).and_return( - mock_api_response(success: true, status: 200, data: ) + mock_api_response(data: ) ) validate_voucher_service.validate @@ -445,13 +431,16 @@ RSpec.describe Vine::VoucherValidatorService, feature: :connected_apps do expect { validate_voucher_service.validate }.not_to change { Voucher.count } end - def mock_api_response(success:, data: nil, status: 200) + def mock_api_response(data: nil) mock_response = instance_double(Faraday::Response) - allow(mock_response).to receive(:success?).and_return(success) - allow(mock_response).to receive(:status).and_return(status) if data.present? allow(mock_response).to receive(:body).and_return(data) end mock_response end + + def mock_api_exception(type: Faraday::Error, status: 503, body: nil) + allow(vine_api_service).to receive(:voucher_validation).and_raise(type.new(nil, + { status:, body: }) ) + end end