From 7cbe77668a7fd0439d2640140155f1f679c53ef5 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Tue, 8 Oct 2024 11:49:58 +1100 Subject: [PATCH 01/33] VineApiService, add voucher_validation It is used to validate a voucher using the given short code --- app/services/vine_api_service.rb | 38 ++++++-- .../returns_the_response.yml | 57 +++++++++++ spec/services/vine_api_service_spec.rb | 96 +++++++++++++++++-- 3 files changed, 173 insertions(+), 18 deletions(-) create mode 100644 spec/fixtures/vcr_cassettes/VineApiService/_voucher_validation/when_a_request_succeed/returns_the_response.yml diff --git a/app/services/vine_api_service.rb b/app/services/vine_api_service.rb index 8fa34ce48a..131e83b239 100644 --- a/app/services/vine_api_service.rb +++ b/app/services/vine_api_service.rb @@ -14,8 +14,32 @@ class VineApiService def my_team my_team_url = "#{@vine_api_url}/my-team" + response = connection.get(my_team_url) + + log_error("VineApiService#my_team", response) + + response + end + + def voucher_validation(voucher_short_code) + voucher_validation_url = "#{@vine_api_url}/voucher-validation" + + response = connection.post( + voucher_validation_url, + { type: "voucher_code", value: voucher_short_code }, + 'Content-Type': "application/json" + ) + + log_error("VineApiService#voucher_validation", response) + + response + end + + private + + def connection jwt = jwt_generator.generate_token - connection = Faraday.new( + Faraday.new( request: { timeout: 30 }, headers: { 'X-Authorization': "JWT #{jwt}", @@ -26,14 +50,12 @@ class VineApiService f.response :json f.request :authorization, 'Bearer', api_key end + end - response = connection.get(my_team_url) + def log_error(prefix, response) + return if response.success? - if !response.success? - Rails.logger.error "VineApiService#my_team -- response_status: #{response.status}" - Rails.logger.error "VineApiService#my_team -- response: #{response.body}" - end - - response + Rails.logger.error "#{prefix} -- response_status: #{response.status}" + Rails.logger.error "#{prefix} -- response: #{response.body}" end end diff --git a/spec/fixtures/vcr_cassettes/VineApiService/_voucher_validation/when_a_request_succeed/returns_the_response.yml b/spec/fixtures/vcr_cassettes/VineApiService/_voucher_validation/when_a_request_succeed/returns_the_response.yml new file mode 100644 index 0000000000..847f63ece3 --- /dev/null +++ b/spec/fixtures/vcr_cassettes/VineApiService/_voucher_validation/when_a_request_succeed/returns_the_response.yml @@ -0,0 +1,57 @@ +--- +http_interactions: +- request: + method: post + uri: https://vine-staging.openfoodnetwork.org.au/api/v1/voucher-validation + body: + encoding: UTF-8 + string: '{"type":"voucher_code","value":"CI3922"}' + headers: + X-Authorization: + - "" + Accept: + - application/json + User-Agent: + - Faraday v2.9.0 + Content-Type: + - application/json + Authorization: + - "" + Accept-Encoding: + - gzip;q=1.0,deflate;q=0.6,identity;q=0.3 + response: + status: + code: 200 + message: OK + headers: + Server: + - nginx + Content-Type: + - application/json + Transfer-Encoding: + - chunked + Connection: + - keep-alive + Vary: + - Accept-Encoding + Cache-Control: + - no-cache, private + Date: + - Mon, 07 Oct 2024 23:48:46 GMT + X-Ratelimit-Limit: + - '60' + X-Ratelimit-Remaining: + - '59' + Access-Control-Allow-Origin: + - "*" + X-Frame-Options: + - SAMEORIGIN + X-Xss-Protection: + - 1; mode=block + X-Content-Type-Options: + - nosniff + body: + encoding: ASCII-8BIT + string: '{"meta":{"responseCode":200,"limit":50,"offset":0,"message":""},"data":{"id":"9d2437c8-4559-4dda-802e-8d9c642a0c1d","voucher_short_code":"CI3922","voucher_set_id":"9d24349c-1fe8-4090-988b-d7355ed32559","is_test":1,"voucher_value_original":500,"voucher_value_remaining":500,"num_voucher_redemptions":0,"last_redemption_at":null,"created_at":"2024-10-01T13:20:02.000000Z","updated_at":"2024-10-01T13:20:02.000000Z","deleted_at":null}}' + recorded_at: Mon, 07 Oct 2024 23:48:46 GMT +recorded_with: VCR 6.2.0 diff --git a/spec/services/vine_api_service_spec.rb b/spec/services/vine_api_service_spec.rb index 0614931719..e9321fbb84 100644 --- a/spec/services/vine_api_service_spec.rb +++ b/spec/services/vine_api_service_spec.rb @@ -9,6 +9,7 @@ RSpec.describe VineApiService do let(:vine_api_key) { "12345" } let(:jwt_service) { VineJwtService.new(secret:) } let(:secret) { "my_secret" } + let(:token) { "some.jwt.token" } before do allow(ENV).to receive(:fetch).and_call_original @@ -33,22 +34,16 @@ RSpec.describe VineApiService do vine_api.my_team - expect(a_request(:get, "https://vine-staging.openfoodnetwork.org.au/api/v1/my-team").with( - headers: { Authorization: "Bearer #{vine_api_key}" } - )).to have_been_made + expect_request_with_api_key(:get, "https://vine-staging.openfoodnetwork.org.au/api/v1/my-team") end it "sends JWT token via a header" do - token = "some.jwt.token" stub_request(:get, my_team_url).to_return(status: 200) - - allow(jwt_service).to receive(:generate_token).and_return(token) + mock_jwt_service vine_api.my_team - expect(a_request(:get, "https://vine-staging.openfoodnetwork.org.au/api/v1/my-team").with( - headers: { 'X-Authorization': "JWT #{token}" } - )).to have_been_made + expect_request_with_jwt_token(:get, "https://vine-staging.openfoodnetwork.org.au/api/v1/my-team") end context "when a request succeed", :vcr do @@ -64,7 +59,7 @@ RSpec.describe VineApiService do it "logs the error" do stub_request(:get, my_team_url).to_return(body: "error", status: 401) - expect(Rails.logger).to receive(:error).twice + expect(Rails.logger).to receive(:error).with(match("VineApiService#my_team")).twice response = vine_api.my_team @@ -80,4 +75,85 @@ RSpec.describe VineApiService do end end end + + describe "#voucher_validation" do + let(:voucher_validation_url) { "#{vine_api_url}/voucher-validation" } + let(:voucher_short_code) { "123456" } + # let(:voucher_short_code) { "CI3922" } + + it "send a POST request to the team VINE api endpoint" do + stub_request(:post, voucher_validation_url).to_return(status: 200) + vine_api.voucher_validation(voucher_short_code) + + expect(a_request( + :post, "https://vine-staging.openfoodnetwork.org.au/api/v1/voucher-validation" + ).with(body: { type: "voucher_code", value: voucher_short_code } )).to have_been_made + end + + it "sends the VINE api key via a header" do + stub_request(:post, voucher_validation_url).to_return(status: 200) + + vine_api.voucher_validation(voucher_short_code) + + expect_request_with_api_key( + :post, "https://vine-staging.openfoodnetwork.org.au/api/v1/voucher-validation" + ) + end + + it "sends JWT token via a header" do + stub_request(:post, voucher_validation_url).to_return(status: 200) + mock_jwt_service + + vine_api.voucher_validation(voucher_short_code) + + expect_request_with_jwt_token( + :post, "https://vine-staging.openfoodnetwork.org.au/api/v1/voucher-validation" + ) + end + + context "when a request succeed", :vcr do + it "returns the response" do + response = vine_api.voucher_validation(voucher_short_code) + + expect(response.success?).to be(true) + expect(response.body).not_to be_empty + end + end + + context "when a request fails" do + it "logs the error" do + stub_request(:post, voucher_validation_url).to_return(body: "error", status: 401) + + expect(Rails.logger).to receive(:error).with( + match("VineApiService#voucher_validation") + ).twice + + response = vine_api.voucher_validation(voucher_short_code) + + expect(response.success?).to be(false) + end + + it "return the response" do + stub_request(:post, voucher_validation_url).to_return(body: "error", status: 401) + response = vine_api.voucher_validation(voucher_short_code) + + expect(response.success?).to be(false) + expect(response.body).not_to be_empty + end + end + end + + def expect_request_with_api_key(method, url) + expect(a_request(method, url).with( headers: { Authorization: "Bearer #{vine_api_key}" })) + .to have_been_made + end + + def expect_request_with_jwt_token(method, url) + expect(a_request(method, url).with( headers: { 'X-Authorization': "JWT #{token}" })) + .to have_been_made + end + + def mock_jwt_service + allow(jwt_service).to receive(:generate_token).and_return(token) + end end From 0f9b93311709067470139934b41f314190553f20 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Tue, 8 Oct 2024 16:48:16 +1100 Subject: [PATCH 02/33] Add extra column to Voucher They are used to store additional informations for VINE vouchers --- ..._id_external_voucher_set_id_voucher_type_to_vouchers.rb | 7 +++++++ db/schema.rb | 3 +++ 2 files changed, 10 insertions(+) create mode 100644 db/migrate/20241008053852_add_external_voucher_id_external_voucher_set_id_voucher_type_to_vouchers.rb diff --git a/db/migrate/20241008053852_add_external_voucher_id_external_voucher_set_id_voucher_type_to_vouchers.rb b/db/migrate/20241008053852_add_external_voucher_id_external_voucher_set_id_voucher_type_to_vouchers.rb new file mode 100644 index 0000000000..23981f5add --- /dev/null +++ b/db/migrate/20241008053852_add_external_voucher_id_external_voucher_set_id_voucher_type_to_vouchers.rb @@ -0,0 +1,7 @@ +class AddExternalVoucherIdExternalVoucherSetIdVoucherTypeToVouchers < ActiveRecord::Migration[7.0] + def change + add_column :vouchers, :external_voucher_id, :uuid + add_column :vouchers, :external_voucher_set_id, :uuid + add_column :vouchers, :voucher_type, :string + end +end diff --git a/db/schema.rb b/db/schema.rb index da955587b8..c9128ba17a 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1110,6 +1110,9 @@ ActiveRecord::Schema[7.0].define(version: 2024_10_30_033956) do t.datetime "deleted_at", precision: nil t.decimal "amount", precision: 10, scale: 2, default: "0.0", null: false t.string "type", limit: 255, default: "Vouchers::FlatRate", null: false + t.uuid "external_voucher_id" + t.uuid "external_voucher_set_id" + t.string "voucher_type" t.index ["code", "enterprise_id"], name: "index_vouchers_on_code_and_enterprise_id", unique: true t.index ["deleted_at"], name: "index_vouchers_on_deleted_at" t.index ["enterprise_id"], name: "index_vouchers_on_enterprise_id" From f9fb7bf3998f19517c14351e2768911c839ca921 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Wed, 16 Oct 2024 16:49:03 +1100 Subject: [PATCH 03/33] Add VineVoucherValidatorService and spec It handles validating and creating vine voucher --- .../vine_voucher_validator_service.rb | 81 ++++ config/locales/en.yml | 7 +- .../vine_voucher_validator_service_spec.rb | 365 ++++++++++++++++++ 3 files changed, 452 insertions(+), 1 deletion(-) create mode 100644 app/services/vine_voucher_validator_service.rb create mode 100644 spec/services/vine_voucher_validator_service_spec.rb diff --git a/app/services/vine_voucher_validator_service.rb b/app/services/vine_voucher_validator_service.rb new file mode 100644 index 0000000000..038ecf8fbc --- /dev/null +++ b/app/services/vine_voucher_validator_service.rb @@ -0,0 +1,81 @@ +# frozen_string_literal: true + +class VineVoucherValidatorService + attr_reader :voucher_code, :errors + + def initialize(voucher_code:, enterprise:) + @voucher_code = voucher_code + @enterprise = enterprise + @errors = {} + end + + def validate + if vine_settings.nil? + errors[:vine_settings] = I18n.t("vine_voucher_validator_service.errors.vine_settings") + return nil + end + + response = call_vine_api + + if !response.success? + handle_errors(response) + return nil + end + + save_voucher(response) + rescue Faraday::Error => e + Rails.logger.error e.inspect + Bugsnag.notify(e) + + # TODO do we need a more specific error ? + errors[:vine_api] = I18n.t("vine_voucher_validator_service.errors.vine_api") + nil + end + + private + + def vine_settings + ConnectedApps::Vine.find_by(enterprise: @enterprise)&.data + end + + def call_vine_api + # Check voucher is valid + jwt_service = VineJwtService.new(secret: vine_settings["secret"]) + vine_api = VineApiService.new(api_key: vine_settings["api_key"], jwt_generator: jwt_service) + + vine_api.voucher_validation(voucher_code) + end + + def handle_errors(response) + if response.status == 400 + errors[:invalid_voucher] = I18n.t("vine_voucher_validator_service.errors.invalid_voucher") + elsif response.status == 404 + errors[:not_found_voucher] = I18n.t("vine_voucher_validator_service.errors.not_found_voucher") + else + errors[:vine_api] = I18n.t("vine_voucher_validator_service.errors.vine_api") + end + end + + def save_voucher(response) + voucher_data = response.body["data"] + + # Check if voucher already exist + voucher = Voucher.vine.find_by(code: voucher_code, enterprise: @enterprise) + + amount = voucher_data["voucher_value_remaining"].to_f / 100 + if voucher.present? + voucher.update(amount: ) + else + voucher = Vouchers::FlatRate.create( + enterprise: @enterprise, + code: voucher_data["voucher_short_code"], + amount:, + external_voucher_id: voucher_data["id"], + external_voucher_set_id: voucher_data["voucher_set_id"], + voucher_type: "VINE" + ) + end + + voucher + end +end diff --git a/config/locales/en.yml b/config/locales/en.yml index 44834a3d6f..e8e87f0303 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -574,7 +574,12 @@ en: depth: "Depth" payment_could_not_process: "The payment could not be processed" payment_could_not_complete: "The payment could not be completed" - + vine_voucher_validator_service: + errors: + vine_settings: "No Vine api settings for the given enterprise" + vine_api: "There was an error communicating with the API" + invalid_voucher: "The voucher is not valid" + not_found_voucher: "The voucher doesn't exist" actions: create_and_add_another: "Create and Add Another" create: "Create" diff --git a/spec/services/vine_voucher_validator_service_spec.rb b/spec/services/vine_voucher_validator_service_spec.rb new file mode 100644 index 0000000000..2576713b37 --- /dev/null +++ b/spec/services/vine_voucher_validator_service_spec.rb @@ -0,0 +1,365 @@ +# frozen_string_literal: true + +require "spec_helper" + +RSpec.describe VineVoucherValidatorService, feature: :connected_apps do + subject(:validate_voucher_service) { described_class.new(voucher_code:, enterprise: distributor) } + + let(:voucher_code) { "good_code" } + let(:distributor) { create(:distributor_enterprise) } + let(:vine_api_service) { instance_double(VineApiService) } + + before do + allow(VineApiService).to receive(:new).and_return(vine_api_service) + end + + describe "#validate" do + context "with a valid voucher" do + let!(:vine_connected_app) { + ConnectedApps::Vine.create( + enterprise: distributor, data: { api_key: "1234568", secret: "my_secret" } + ) + } + let(:data) { + { + meta: { responseCode: 200, limit: 50, offset: 0, message: "" }, + data: { + id: "9d2437c8-4559-4dda-802e-8d9c642a0c1d", + voucher_short_code: voucher_code, + voucher_set_id: "9d24349c-1fe8-4090-988b-d7355ed32559", + is_test: 1, + voucher_value_original: 500, + voucher_value_remaining: 500, + num_voucher_redemptions: 0, + last_redemption_at: "null", + created_at: "2024-10-01T13:20:02.000000Z", + updated_at: "2024-10-01T13:20:02.000000Z", + deleted_at: "null" + } + }.deep_stringify_keys + } + + it "verifies the voucher with VINE API" do + expect(vine_api_service).to receive(:voucher_validation) + .and_return(mock_api_response( success: true, 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:)) + + vine_voucher = validate_voucher_service.validate + + expect(vine_voucher).not_to be_nil + expect(vine_voucher.code).to eq(voucher_code) + expect(vine_voucher.amount).to eq(5.00) + expect(vine_voucher.voucher_type).to eq("VINE") + expect(vine_voucher.external_voucher_id).to eq("9d2437c8-4559-4dda-802e-8d9c642a0c1d") + expect(vine_voucher.external_voucher_set_id).to eq( + "9d24349c-1fe8-4090-988b-d7355ed32559" + ) + end + end + + context "when distributor is not connected to VINE" do + it "returns nil" do + expect(validate_voucher_service.validate).to be_nil + end + + it "doesn't call the VINE API" do + expect(vine_api_service).not_to receive(:voucher_validation) + + validate_voucher_service.validate + end + + it "adds an error message" do + validate_voucher_service.validate + + expect(validate_voucher_service.errors).to include( + { vine_settings: "No Vine api settings for the given enterprise" } + ) + end + + it "doesn't creates a new VINE voucher" do + expect { validate_voucher_service.validate }.not_to change { Voucher.count } + end + end + + context "when there is an API error" do + let!(:vine_connected_app) { + ConnectedApps::Vine.create( + enterprise: distributor, data: { api_key: "1234567", secret: "my_secret" } + ) + } + + before do + allow(vine_api_service).to receive(:voucher_validation).and_raise(Faraday::Error) + end + + it "returns nil" do + expect(validate_voucher_service.validate).to be_nil + end + + it "adds an error message" do + validate_voucher_service.validate + + expect(validate_voucher_service.errors).to include( + { vine_api: "There was an error communicating with the API" } + ) + end + + it "doesn't creates a new VINE voucher" do + expect { validate_voucher_service.validate }.not_to change { Voucher.count } + end + + it "logs the error and notify bugsnag" do + expect(Rails.logger).to receive(:error) + expect(Bugsnag).to receive(:notify) + + validate_voucher_service.validate + end + end + + context "when there is an API authentication error" do + let!(:vine_connected_app) { + ConnectedApps::Vine.create( + enterprise: distributor, data: { api_key: "1234567", secret: "my_secret" } + ) + } + let(:data) { + { + meta: { numRecords: 0, totalRows: 0, responseCode: 401, + message: "Incorrect authorization signature." }, + data: [] + }.deep_stringify_keys + } + + before do + allow(vine_api_service).to receive(:voucher_validation).and_return( + mock_api_response(success: false, status: 401, data: ) + ) + end + + it "returns nil" do + expect(validate_voucher_service.validate).to be_nil + end + + it "adds an error message" do + validate_voucher_service.validate + + expect(validate_voucher_service.errors).to include( + { vine_api: "There was an error communicating with the API" } + ) + end + + it "doesn't creates a new VINE voucher" do + expect { validate_voucher_service.validate }.not_to change { Voucher.count } + end + end + + context "when the voucher doesn't exist" do + let!(:vine_connected_app) { + ConnectedApps::Vine.create( + enterprise: distributor, data: { api_key: "1234568", secret: "my_secret" } + ) + } + let(:data) { + { + meta: { responseCode: 404, limit: 50, offset: 0, message: "Not found" }, + data: [] + }.deep_stringify_keys + } + + before do + allow(vine_api_service).to receive(:voucher_validation).and_return( + mock_api_response(success: false, status: 404, data: ) + ) + end + + it "returns nil" do + expect(validate_voucher_service.validate).to be_nil + end + + it "adds an error message" do + validate_voucher_service.validate + + expect(validate_voucher_service.errors).to include( + { not_found_voucher: "The voucher doesn't exist" } + ) + end + + it "doesn't creates a new VINE voucher" do + expect { validate_voucher_service.validate }.not_to change { Voucher.count } + end + end + + context "when the voucher is an invalid voucher" do + let!(:vine_connected_app) { + ConnectedApps::Vine.create( + enterprise: distributor, data: { api_key: "1234568", secret: "my_secret" } + ) + } + let(:data) { + { + meta: { responseCode: 400, limit: 50, offset: 0, message: "Invalid merchant team." }, + data: [] + }.deep_stringify_keys + } + + before do + allow(vine_api_service).to receive(:voucher_validation).and_return( + mock_api_response(success: false, status: 400, data: ) + ) + end + + it "returns nil" do + expect(validate_voucher_service.validate).to be_nil + end + + it "adds an error message" do + validate_voucher_service.validate + + expect(validate_voucher_service.errors).to include( + { invalid_voucher: "The voucher is not valid" } + ) + end + + it "doesn't creates a new VINE voucher" do + expect { validate_voucher_service.validate }.not_to change { Voucher.count } + end + end + + context "when creating a new voucher fails" do + let!(:vine_connected_app) { + ConnectedApps::Vine.create( + enterprise: distributor, data: { api_key: "1234568", secret: "my_secret" } + ) + } + let(:data) { + { + meta: { responseCode: 200, limit: 50, offset: 0, message: "" }, + data: { + id: "9d2437c8-4559-4dda-802e-8d9c642a0c1d", + voucher_short_code: voucher_code, + voucher_set_id: "9d24349c-1fe8-4090-988b-d7355ed32559", + is_test: 1, + voucher_value_original: 500, + voucher_value_remaining: '', + num_voucher_redemptions: 0, + last_redemption_at: "null", + created_at: "2024-10-01T13:20:02.000000Z", + updated_at: "2024-10-01T13:20:02.000000Z", + deleted_at: "null" + } + }.deep_stringify_keys + } + + before do + allow(vine_api_service).to receive(:voucher_validation).and_return( + mock_api_response(success: true, status: 200, data: ) + ) + end + + it "returns an invalid voucher" do + voucher = validate_voucher_service.validate + expect(voucher).not_to be_valid + end + end + + context "with an existing voucher" do + let!(:vine_connected_app) { + ConnectedApps::Vine.create( + enterprise: distributor, data: { api_key: "1234567", secret: "my_secret" } + ) + } + let!(:voucher) { + create(:voucher_flat_rate, enterprise: distributor, code: voucher_code, + amount: 500, voucher_type: "VINE" ) + } + + let(:data) { + { + meta: { responseCode: 200, limit: 50, offset: 0, message: "" }, + data: { + id: "9d2437c8-4559-4dda-802e-8d9c642a0c1d", + voucher_short_code: voucher_code, + voucher_set_id: "9d24349c-1fe8-4090-988b-d7355ed32559", + is_test: 1, + voucher_value_original: 500, + voucher_value_remaining: 250, + num_voucher_redemptions: 1, + last_redemption_at: "2024-10-05T13:20:02.000000Z", + created_at: "2024-10-01T13:20:02.000000Z", + updated_at: "2024-10-01T13:20:02.000000Z", + deleted_at: "null" + } + }.deep_stringify_keys + } + + before do + allow(vine_api_service).to receive(:voucher_validation).and_return( + mock_api_response(success: true, status: 200, 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: ) + ) + + validate_voucher_service.validate + end + + it "updates the VINE voucher" do + vine_voucher = validate_voucher_service.validate + + expect(vine_voucher.id).to eq(voucher.id) + expect(vine_voucher.amount).to eq(2.50) + end + + context "when updating the voucher fails" do + let(:data) { + { + meta: { responseCode: 200, limit: 50, offset: 0, message: "" }, + data: { + id: "9d2437c8-4559-4dda-802e-8d9c642a0c1d", + voucher_short_code: voucher_code, + voucher_set_id: "9d24349c-1fe8-4090-988b-d7355ed32559", + is_test: 1, + voucher_value_original: 500, + voucher_value_remaining: '', + num_voucher_redemptions: 0, + last_redemption_at: "null", + created_at: "2024-10-01T13:20:02.000000Z", + updated_at: "2024-10-01T13:20:02.000000Z", + deleted_at: "null" + } + }.deep_stringify_keys + } + + it "returns an invalid voucher" do + vine_voucher = validate_voucher_service.validate + expect(vine_voucher).not_to be_valid + end + + it "doesn't update existing voucher" do + expect { + validate_voucher_service.validate + }.not_to change { voucher.reload.amount } + end + end + end + end + + def mock_api_response(success:, data: nil, status: 200) + 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 +end From 3a367ceb6e403cda9ffe5cff4d19456f2c7c7f80 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Wed, 16 Oct 2024 13:08:48 +1100 Subject: [PATCH 04/33] Handle adding a VINE voucher to an order Plus specs --- .../voucher_adjustments_controller.rb | 64 +++++++- app/models/voucher.rb | 3 + config/locales/en.yml | 1 + spec/requests/voucher_adjustments_spec.rb | 152 +++++++++++++++++- 4 files changed, 208 insertions(+), 12 deletions(-) diff --git a/app/controllers/voucher_adjustments_controller.rb b/app/controllers/voucher_adjustments_controller.rb index c4f5fdf5fb..b8f3a3f1e4 100644 --- a/app/controllers/voucher_adjustments_controller.rb +++ b/app/controllers/voucher_adjustments_controller.rb @@ -4,7 +4,16 @@ class VoucherAdjustmentsController < BaseController before_action :set_order def create - if add_voucher + if voucher_params[:voucher_code].blank? + @order.errors.add(:voucher_code, I18n.t('checkout.errors.voucher_not_found')) + return render_error + end + + voucher = load_voucher + + return render_error unless valid_voucher?(voucher) + + if add_voucher_to_order(voucher) update_payment_section elsif @order.errors.present? render_error @@ -30,19 +39,28 @@ class VoucherAdjustmentsController < BaseController @order = current_order end - def add_voucher - if voucher_params[:voucher_code].blank? - @order.errors.add(:voucher_code, I18n.t('checkout.errors.voucher_not_found')) - return false - end - - voucher = Voucher.find_by(code: voucher_params[:voucher_code], enterprise: @order.distributor) + def valid_voucher?(voucher) + return false if @order.errors.present? if voucher.nil? @order.errors.add(:voucher_code, I18n.t('checkout.errors.voucher_not_found')) return false end + if !voucher.valid? + @order.errors.add( + :voucher_code, + I18n.t( + 'checkout.errors.create_voucher_error', error: voucher.errors.full_messages.to_sentence + ) + ) + return false + end + + true + end + + def add_voucher_to_order(voucher) adjustment = voucher.create_adjustment(voucher.code, @order) unless adjustment.persisted? @@ -51,6 +69,7 @@ class VoucherAdjustmentsController < BaseController return false end + # calculate_voucher_adjustment clear_payments VoucherAdjustmentsService.new(@order).update @@ -59,6 +78,35 @@ class VoucherAdjustmentsController < BaseController true end + def load_voucher + voucher = Voucher.not_vine.find_by(code: voucher_params[:voucher_code], + enterprise: @order.distributor) + return voucher unless voucher.nil? + + vine_voucher + end + + def vine_voucher + vine_voucher_validator = VineVoucherValidatorService.new( + voucher_code: voucher_params[:voucher_code], enterprise: @order.distributor + ) + voucher = vine_voucher_validator.validate + + if vine_voucher_validator.errors.present? + return nil if ignored_vine_api_error(vine_voucher_validator) + + @order.errors.add(:voucher_code, I18n.t('checkout.errors.add_voucher_error')) + return nil + end + + 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/models/voucher.rb b/app/models/voucher.rb index 12ec455edf..eaa1d2273b 100644 --- a/app/models/voucher.rb +++ b/app/models/voucher.rb @@ -18,6 +18,9 @@ class Voucher < ApplicationRecord TYPES = ["Vouchers::FlatRate", "Vouchers::PercentageRate"].freeze + scope :vine, -> { where(voucher_type: "VINE") } + scope :not_vine, -> { where.not(voucher_type: "VINE").or(where(voucher_type: nil)) } + def code=(value) super(value.to_s.strip) end diff --git a/config/locales/en.yml b/config/locales/en.yml index e8e87f0303..cbb449f039 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -2128,6 +2128,7 @@ en: no_shipping_methods_available: Checkout is not possible due to absence of shipping options. Please contact the shop owner. voucher_not_found: Not found add_voucher_error: There was an error while adding the voucher + create_voucher_error: "There was an error while creating the voucher: %{error}" shops: hubs: show_closed_shops: "Show closed shops" diff --git a/spec/requests/voucher_adjustments_spec.rb b/spec/requests/voucher_adjustments_spec.rb index 78ae9c29d5..158911dd68 100644 --- a/spec/requests/voucher_adjustments_spec.rb +++ b/spec/requests/voucher_adjustments_spec.rb @@ -34,10 +34,10 @@ RSpec.describe VoucherAdjustmentsController, type: :request do let(:params) { { order: { voucher_code: voucher.code } } } it "adds a voucher to the user's current order" do - post("/voucher_adjustments", params:) - + expect { + post("/voucher_adjustments", params:) + }.to change { order.reload.voucher_adjustments.count }.by(1) expect(response).to be_successful - expect(order.reload.voucher_adjustments.length).to eq(1) end context "when voucher doesn't exist" do @@ -56,7 +56,7 @@ RSpec.describe VoucherAdjustmentsController, type: :request do # Create a non valid adjustment bad_adjustment = build(:adjustment, label: nil) allow(voucher).to receive(:create_adjustment).and_return(bad_adjustment) - allow(Voucher).to receive(:find_by).and_return(voucher) + allow(Voucher).to receive_message_chain(:not_vine, :find_by).and_return(voucher) post("/voucher_adjustments", params:) @@ -85,6 +85,145 @@ RSpec.describe VoucherAdjustmentsController, type: :request do end.to change { order.reload.all_adjustments.payment_fee.count }.from(1).to(0) end end + + context "with a VINE voucher", feature: :connected_apps do + let(:vine_voucher_validator) { instance_double(VineVoucherValidatorService) } + + before do + allow(VineVoucherValidatorService).to receive(:new).and_return(vine_voucher_validator) + end + + context "with a new voucher" do + let(:params) { { order: { voucher_code: vine_voucher_code } } } + let(:vine_voucher_code) { "PQ3187" } + + context "with a valid voucher" do + it "verifies the voucher with VINE API" do + expect(vine_voucher_validator).to receive(:validate) + allow(vine_voucher_validator).to receive(:errors).and_return({}) + + post "/voucher_adjustments", params: + end + + it "adds a voucher to the user's current order" do + vine_voucher = create(:voucher_flat_rate, code: vine_voucher_code) + mock_vine_voucher_validator(voucher: vine_voucher) + + post("/voucher_adjustments", params:) + + expect(response).to be_successful + expect(order.reload.voucher_adjustments.length).to eq(1) + end + end + + 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" } + ) + + post("/voucher_adjustments", params:) + + expect(response).to be_unprocessable + expect(flash[:error]).to match "Voucher code Not found" + end + end + + context "when there is an API error" do + it "returns 422 and an error message" do + mock_vine_voucher_validator( + voucher: nil, + errors: { vine_api: "There was an error communicating with the API" } + ) + + post("/voucher_adjustments", params:) + + expect(response).to be_unprocessable + expect(flash[:error]).to match "There was an error while adding the voucher" + end + end + + context "when the voucher doesn't exist" do + it "returns 422 and an error message" do + mock_vine_voucher_validator(voucher: nil, + errors: { not_found_voucher: "The voucher doesn't exist" }) + + post("/voucher_adjustments", params:) + + expect(response).to be_unprocessable + expect(flash[:error]).to match "Voucher code Not found" + end + end + + context "when the voucher is invalid voucher" do + it "returns 422 and an error message" do + mock_vine_voucher_validator(voucher: nil, + errors: { invalid_voucher: "The voucher is not valid" }) + + post("/voucher_adjustments", params:) + + expect(response).to be_unprocessable + expect(flash[:error]).to match "There was an error while adding the voucher" + end + end + + context "when creating a new voucher fails" do + it "returns 422 and an error message" do + vine_voucher = build(:voucher_flat_rate, code: vine_voucher_code, + enterprise: distributor, amount: "") + mock_vine_voucher_validator(voucher: vine_voucher) + + post("/voucher_adjustments", params:) + + expect(response).to be_unprocessable + expect(flash[:error]).to match( + "There was an error while creating the voucher: Amount can't be blank and " \ + "Amount is not a number" + ) + end + end + end + + context "with an existing voucher" do + let(:params) { { order: { voucher_code: vine_voucher_code } } } + let(:vine_voucher_code) { "PQ3187" } + + it "verify the voucher with VINE API" do + expect(vine_voucher_validator).to receive(:validate) + allow(vine_voucher_validator).to receive(:errors).and_return({}) + + post "/voucher_adjustments", params: + end + + it "adds a voucher to the user's current order" do + vine_voucher = create(:voucher_flat_rate, code: vine_voucher_code, + enterprise: distributor) + mock_vine_voucher_validator(voucher: vine_voucher) + + expect { + post("/voucher_adjustments", params:) + }.to change { order.reload.voucher_adjustments.count }.by(1) + expect(response).to be_successful + end + + context "when updating the voucher fails" do + it "returns 422 and an error message" do + vine_voucher = build(:voucher_flat_rate, code: vine_voucher_code, + enterprise: distributor, amount: "") + mock_vine_voucher_validator(voucher: vine_voucher) + + post("/voucher_adjustments", params:) + + expect(response).to be_unprocessable + expect(flash[:error]).to match( + "There was an error while creating the voucher: Amount can't be blank and " \ + "Amount is not a number" + ) + end + end + end + end end describe "DELETE voucher_adjustments/:id" do @@ -137,4 +276,9 @@ RSpec.describe VoucherAdjustmentsController, type: :request do end end end + + def mock_vine_voucher_validator(voucher:, errors: {}) + allow(vine_voucher_validator).to receive(:validate).and_return(voucher) + allow(vine_voucher_validator).to receive(:errors).and_return(errors) + end end From c89b4fb86be3d38944b369f24d6008b844cdfba2 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Sun, 20 Oct 2024 15:04:17 +1100 Subject: [PATCH 05/33] Add system spec fot adding VINE voucher to order --- .../adds_a_voucher_to_the_order.yml | 57 +++++++++++++++++++ .../with_an_invalid_voucher/show_an_error.yml | 50 ++++++++++++++++ spec/system/consumer/checkout/payment_spec.rb | 30 ++++++++++ 3 files changed, 137 insertions(+) create mode 100644 spec/fixtures/vcr_cassettes/As_a_consumer_I_want_to_checkout_my_order/as_a_logged_in_user/payment_step/vouchers/with_voucher_available/adding_voucher_to_the_order/with_a_VINE_voucher/adds_a_voucher_to_the_order.yml create mode 100644 spec/fixtures/vcr_cassettes/As_a_consumer_I_want_to_checkout_my_order/as_a_logged_in_user/payment_step/vouchers/with_voucher_available/adding_voucher_to_the_order/with_a_VINE_voucher/with_an_invalid_voucher/show_an_error.yml diff --git a/spec/fixtures/vcr_cassettes/As_a_consumer_I_want_to_checkout_my_order/as_a_logged_in_user/payment_step/vouchers/with_voucher_available/adding_voucher_to_the_order/with_a_VINE_voucher/adds_a_voucher_to_the_order.yml b/spec/fixtures/vcr_cassettes/As_a_consumer_I_want_to_checkout_my_order/as_a_logged_in_user/payment_step/vouchers/with_voucher_available/adding_voucher_to_the_order/with_a_VINE_voucher/adds_a_voucher_to_the_order.yml new file mode 100644 index 0000000000..a52e3c0cc2 --- /dev/null +++ b/spec/fixtures/vcr_cassettes/As_a_consumer_I_want_to_checkout_my_order/as_a_logged_in_user/payment_step/vouchers/with_voucher_available/adding_voucher_to_the_order/with_a_VINE_voucher/adds_a_voucher_to_the_order.yml @@ -0,0 +1,57 @@ +--- +http_interactions: +- request: + method: post + uri: https://vine-staging.openfoodnetwork.org.au/api/v1/voucher-validation + body: + encoding: UTF-8 + string: '{"type":"voucher_code","value":"CI3922"}' + headers: + X-Authorization: + - "" + Accept: + - application/json + User-Agent: + - Faraday v2.9.0 + Content-Type: + - application/json + Authorization: + - "" + Accept-Encoding: + - gzip;q=1.0,deflate;q=0.6,identity;q=0.3 + response: + status: + code: 200 + message: OK + headers: + Server: + - nginx + Content-Type: + - application/json + Transfer-Encoding: + - chunked + Connection: + - keep-alive + Vary: + - Accept-Encoding + Cache-Control: + - no-cache, private + Date: + - Sun, 20 Oct 2024 03:32:37 GMT + X-Ratelimit-Limit: + - '60' + X-Ratelimit-Remaining: + - '58' + Access-Control-Allow-Origin: + - "*" + X-Frame-Options: + - SAMEORIGIN + X-Xss-Protection: + - 1; mode=block + X-Content-Type-Options: + - nosniff + body: + encoding: ASCII-8BIT + string: '{"meta":{"responseCode":200,"limit":50,"offset":0,"message":""},"data":{"id":"9d2437c8-4559-4dda-802e-8d9c642a0c1d","voucher_short_code":"CI3922","voucher_set_id":"9d24349c-1fe8-4090-988b-d7355ed32559","is_test":1,"voucher_value_original":500,"voucher_value_remaining":500,"num_voucher_redemptions":0,"last_redemption_at":null,"created_at":"2024-10-01T13:20:02.000000Z","updated_at":"2024-10-01T13:20:02.000000Z","deleted_at":null}}' + recorded_at: Sun, 20 Oct 2024 03:32:37 GMT +recorded_with: VCR 6.2.0 diff --git a/spec/fixtures/vcr_cassettes/As_a_consumer_I_want_to_checkout_my_order/as_a_logged_in_user/payment_step/vouchers/with_voucher_available/adding_voucher_to_the_order/with_a_VINE_voucher/with_an_invalid_voucher/show_an_error.yml b/spec/fixtures/vcr_cassettes/As_a_consumer_I_want_to_checkout_my_order/as_a_logged_in_user/payment_step/vouchers/with_voucher_available/adding_voucher_to_the_order/with_a_VINE_voucher/with_an_invalid_voucher/show_an_error.yml new file mode 100644 index 0000000000..9448144724 --- /dev/null +++ b/spec/fixtures/vcr_cassettes/As_a_consumer_I_want_to_checkout_my_order/as_a_logged_in_user/payment_step/vouchers/with_voucher_available/adding_voucher_to_the_order/with_a_VINE_voucher/with_an_invalid_voucher/show_an_error.yml @@ -0,0 +1,50 @@ +--- +http_interactions: +- request: + method: post + uri: https://vine-staging.openfoodnetwork.org.au/api/v1/voucher-validation + body: + encoding: UTF-8 + string: '{"type":"voucher_code","value":"KM1891"}' + headers: + X-Authorization: + - "" + Accept: + - application/json + User-Agent: + - Faraday v2.9.0 + Content-Type: + - application/json + Authorization: + - "" + Accept-Encoding: + - gzip;q=1.0,deflate;q=0.6,identity;q=0.3 + response: + status: + code: 400 + message: Bad Request + headers: + Server: + - nginx + Content-Type: + - application/json + Transfer-Encoding: + - chunked + Connection: + - keep-alive + Cache-Control: + - no-cache, private + Date: + - Sun, 20 Oct 2024 03:42:25 GMT + X-Ratelimit-Limit: + - '60' + X-Ratelimit-Remaining: + - '59' + Access-Control-Allow-Origin: + - "*" + body: + encoding: UTF-8 + string: '{"meta":{"responseCode":400,"limit":50,"offset":0,"message":"Invalid + merchant team."},"data":[]}' + recorded_at: Sun, 20 Oct 2024 03:42:25 GMT +recorded_with: VCR 6.2.0 diff --git a/spec/system/consumer/checkout/payment_spec.rb b/spec/system/consumer/checkout/payment_spec.rb index b0bd281ca1..5c7c7ba1e7 100644 --- a/spec/system/consumer/checkout/payment_spec.rb +++ b/spec/system/consumer/checkout/payment_spec.rb @@ -172,6 +172,36 @@ RSpec.describe "As a consumer, I want to checkout my order" do expect(page).to have_content("Voucher code Not found") end end + + context "with a VINE voucher", :vcr, feature: :connected_apps do + let!(:vine_connected_app) { + ConnectedApps::Vine.create( + enterprise: distributor, data: { api_key: "1234568", secret: "my_secret" } + ) + } + before do + allow(ENV).to receive(:fetch).and_call_original + allow(ENV).to receive(:fetch).with("VINE_API_URL").and_return("https://vine-staging.openfoodnetwork.org.au/api/v1") + end + + it "adds a voucher to the order" do + apply_voucher "CI3922" + + expect(page).to have_content "$5.00 Voucher" + expect(order.reload.voucher_adjustments.length).to eq(1) + expect(Voucher.vine.find_by(code: "CI3922", enterprise: distributor)).not_to be_nil + end + + context "with an invalid voucher" do + it "show an error" do + fill_in "Enter voucher code", with: "KM1891" + click_button("Apply") + + expect(page).to have_content("There was an error while adding the voucher") + expect(Voucher.vine.find_by(code: "non_code", enterprise: distributor)).to be_nil + end + end + end end describe "removing voucher from order" do From b30096317c173fadd22ed2e73f459346bed6f17d Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 21 Oct 2024 14:14:28 +1100 Subject: [PATCH 06/33] VineApiService, add voucher_redemptions It is used to redeem a voucher --- app/services/vine_api_service.rb | 14 ++++ .../returns_the_response.yml | 55 ++++++++++++++ spec/services/vine_api_service_spec.rb | 71 ++++++++++++++++++- 3 files changed, 138 insertions(+), 2 deletions(-) create mode 100644 spec/fixtures/vcr_cassettes/VineApiService/_voucher_redemptions/when_a_request_succeed/returns_the_response.yml diff --git a/app/services/vine_api_service.rb b/app/services/vine_api_service.rb index 131e83b239..89b7b1a348 100644 --- a/app/services/vine_api_service.rb +++ b/app/services/vine_api_service.rb @@ -35,6 +35,20 @@ class VineApiService response end + def voucher_redemptions(voucher_id, voucher_set_id, amount) + voucher_redemptions_url = "#{@vine_api_url}/voucher-redemptions" + + response = connection.post( + voucher_redemptions_url, + { voucher_id:, voucher_set_id:, amount: amount.to_i }, + 'Content-Type': "application/json" + ) + + log_error("VineApiService#voucher_redemptions", response) + + response + end + private def connection diff --git a/spec/fixtures/vcr_cassettes/VineApiService/_voucher_redemptions/when_a_request_succeed/returns_the_response.yml b/spec/fixtures/vcr_cassettes/VineApiService/_voucher_redemptions/when_a_request_succeed/returns_the_response.yml new file mode 100644 index 0000000000..af1786e786 --- /dev/null +++ b/spec/fixtures/vcr_cassettes/VineApiService/_voucher_redemptions/when_a_request_succeed/returns_the_response.yml @@ -0,0 +1,55 @@ +--- +http_interactions: +- request: + method: post + uri: https://vine-staging.openfoodnetwork.org.au/api/v1/voucher-redemptions + body: + encoding: UTF-8 + string: '{"voucher_id":"9d316d27-0dad-411a-8953-316a1aaf7742","voucher_set_id":"9d314daa-0878-4b73-922d-698047640cf4","amount":1}' + headers: + X-Authorization: + - "" + Accept: + - application/json + User-Agent: + - Faraday v2.9.0 + Content-Type: + - application/json + Authorization: + - "" + Accept-Encoding: + - gzip;q=1.0,deflate;q=0.6,identity;q=0.3 + response: + status: + code: 200 + message: OK + headers: + Server: + - nginx + Content-Type: + - application/json + Transfer-Encoding: + - chunked + Connection: + - keep-alive + Vary: + - Accept-Encoding + Cache-Control: + - no-cache, private + Date: + - Mon, 21 Oct 2024 03:07:09 GMT + Access-Control-Allow-Origin: + - "*" + X-Frame-Options: + - SAMEORIGIN + X-Xss-Protection: + - 1; mode=block + X-Content-Type-Options: + - nosniff + body: + encoding: ASCII-8BIT + string: '{"meta":{"responseCode":200,"limit":50,"offset":0,"message":"Redemption + successful. This was a test redemption. Do NOT provide the person with goods + or services."},"data":{"voucher_id":"9d316d27-0dad-411a-8953-316a1aaf7742","voucher_set_id":"9d314daa-0878-4b73-922d-698047640cf4","redeemed_by_user_id":8,"redeemed_by_team_id":4,"redeemed_amount":1,"is_test":1,"updated_at":"2024-10-21T03:07:09.000000Z","created_at":"2024-10-21T03:07:09.000000Z","id":5}}' + recorded_at: Mon, 21 Oct 2024 03:07:09 GMT +recorded_with: VCR 6.2.0 diff --git a/spec/services/vine_api_service_spec.rb b/spec/services/vine_api_service_spec.rb index e9321fbb84..1a9f9977d4 100644 --- a/spec/services/vine_api_service_spec.rb +++ b/spec/services/vine_api_service_spec.rb @@ -78,8 +78,7 @@ RSpec.describe VineApiService do describe "#voucher_validation" do let(:voucher_validation_url) { "#{vine_api_url}/voucher-validation" } - let(:voucher_short_code) { "123456" } - # let(:voucher_short_code) { "CI3922" } + let(:voucher_short_code) { "CI3922" } it "send a POST request to the team VINE api endpoint" do stub_request(:post, voucher_validation_url).to_return(status: 200) @@ -143,6 +142,74 @@ RSpec.describe VineApiService do end end + describe "#voucher_redemptions" do + let(:voucher_redemptions_url) { "#{vine_api_url}/voucher-redemptions" } + let(:voucher_id) { "quia" } + let(:voucher_set_id) { "natus" } + let(:amount) { 500 } # $5.00 + + it "send a POST request to the voucher redemptions VINE api endpoint" do + stub_request(:post, voucher_redemptions_url).to_return(status: 200) + vine_api.voucher_redemptions(voucher_id, voucher_set_id, amount) + + expect(a_request( + :post, "https://vine-staging.openfoodnetwork.org.au/api/v1/voucher-redemptions" + ).with(body: { voucher_id:, voucher_set_id:, amount: })).to have_been_made + end + + it "sends the VINE api key via a header" do + stub_request(:post, voucher_redemptions_url).to_return(status: 200) + + vine_api.voucher_redemptions(voucher_id, voucher_set_id, amount) + + expect_request_with_api_key( + :post, "https://vine-staging.openfoodnetwork.org.au/api/v1/voucher-redemptions" + ) + end + + it "sends JWT token via a header" do + stub_request(:post, voucher_redemptions_url).to_return(status: 200) + mock_jwt_service + + vine_api.voucher_redemptions(voucher_id, voucher_set_id, amount) + + expect_request_with_jwt_token( + :post, "https://vine-staging.openfoodnetwork.org.au/api/v1/voucher-redemptions" + ) + end + + context "when a request succeed", :vcr do + it "returns the response" do + response = vine_api.voucher_redemptions(voucher_id, voucher_set_id, amount) + + expect(response.success?).to be(true) + expect(response.body).not_to be_empty + end + end + + context "when a request fails" do + it "logs the error" do + stub_request(:post, voucher_redemptions_url).to_return(body: "error", status: 401) + + expect(Rails.logger).to receive(:error).with( + match("VineApiService#voucher_redemptions") + ).twice + + response = vine_api.voucher_redemptions(voucher_id, voucher_set_id, amount) + + expect(response.success?).to be(false) + end + + it "return the response" do + stub_request(:post, voucher_redemptions_url).to_return(body: "error", status: 401) + response = vine_api.voucher_redemptions(voucher_id, voucher_set_id, amount) + + expect(response.success?).to be(false) + expect(response.body).not_to be_empty + end + end + end + def expect_request_with_api_key(method, url) expect(a_request(method, url).with( headers: { Authorization: "Bearer #{vine_api_key}" })) .to have_been_made From c17eddd69b8f4a914256fd9fc35dadfe4ffc7de7 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 21 Oct 2024 21:46:58 +1100 Subject: [PATCH 07/33] Add Voucher#vine? And small refactor --- app/models/voucher.rb | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/app/models/voucher.rb b/app/models/voucher.rb index eaa1d2273b..52ad6f2ef7 100644 --- a/app/models/voucher.rb +++ b/app/models/voucher.rb @@ -1,6 +1,8 @@ # frozen_string_literal: false class Voucher < ApplicationRecord + VINE_TYPE = "VINE".freeze + self.belongs_to_required_by_default = false acts_as_paranoid @@ -18,8 +20,8 @@ class Voucher < ApplicationRecord TYPES = ["Vouchers::FlatRate", "Vouchers::PercentageRate"].freeze - scope :vine, -> { where(voucher_type: "VINE") } - scope :not_vine, -> { where.not(voucher_type: "VINE").or(where(voucher_type: nil)) } + scope :vine, -> { where(voucher_type: VINE_TYPE) } + scope :not_vine, -> { where.not(voucher_type: VINE_TYPE).or(where(voucher_type: nil)) } def code=(value) super(value.to_s.strip) @@ -45,6 +47,10 @@ class Voucher < ApplicationRecord order.adjustments.create(adjustment_attributes) end + def vine? + voucher_type == VINE_TYPE + end + # The following method must be overriden in a concrete voucher. def display_value raise NotImplementedError, 'please use concrete voucher' From 9399c7e12931e207323d1d64850a26cd5c1f026d Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 21 Oct 2024 21:52:13 +1100 Subject: [PATCH 08/33] Add VineVoucherRedeemerService It handles redeeming a voucher --- app/services/vine_voucher_redeemer_service.rb | 64 ++++ config/locales/en.yml | 5 + .../vine_voucher_redeemer_service_spec.rb | 277 ++++++++++++++++++ 3 files changed, 346 insertions(+) create mode 100644 app/services/vine_voucher_redeemer_service.rb create mode 100644 spec/services/vine_voucher_redeemer_service_spec.rb diff --git a/app/services/vine_voucher_redeemer_service.rb b/app/services/vine_voucher_redeemer_service.rb new file mode 100644 index 0000000000..b616d8a798 --- /dev/null +++ b/app/services/vine_voucher_redeemer_service.rb @@ -0,0 +1,64 @@ +# frozen_string_literal: true + +class VineVoucherRedeemerService + attr_reader :order, :errors + + def initialize(order: ) + @order = order + @errors = {} + end + + def call + # Do nothing if we don't have a vine voucher added to the order + voucher_adjustment = order.voucher_adjustments.first + @voucher = voucher_adjustment&.originator + + return true if voucher_adjustment.nil? || !@voucher.vine? + + if vine_settings.nil? + errors[:vine_settings] = I18n.t("vine_voucher_redeemer_service.errors.vine_settings") + return false + end + + response = call_vine_api + + if !response.success? + handle_errors(response) + return false + end + + voucher_adjustment.close + + true + rescue Faraday::Error => e + Rails.logger.error e.inspect + Bugsnag.notify(e) + + errors[:vine_api] = I18n.t("vine_voucher_validator_service.errors.vine_api") + false + end + + private + + def vine_settings + ConnectedApps::Vine.find_by(enterprise: order.distributor)&.data + end + + def call_vine_api + jwt_service = VineJwtService.new(secret: vine_settings["secret"]) + vine_api = VineApiService.new(api_key: vine_settings["api_key"], jwt_generator: jwt_service) + + # Voucher amount is stored in dollars, VINE expect cents + vine_api.voucher_redemptions( + @voucher.external_voucher_id, @voucher.external_voucher_set_id, (@voucher.amount * 100) + ) + end + + def handle_errors(response) + if response.status == 400 + errors[:redeeming_failed] = I18n.t("vine_voucher_redeemer_service.errors.redeeming_failed") + else + errors[:vine_api] = I18n.t("vine_voucher_redeemer_service.errors.vine_api") + end + end +end diff --git a/config/locales/en.yml b/config/locales/en.yml index cbb449f039..abbbc23c4b 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -580,6 +580,11 @@ en: vine_api: "There was an error communicating with the API" invalid_voucher: "The voucher is not valid" not_found_voucher: "The voucher doesn't exist" + vine_voucher_redeemer_service: + errors: + vine_settings: "No Vine api settings for the given enterprise" + vine_api: "There was an error communicating with the API" + redeeming_failed: "Redeeming the voucher failed" actions: create_and_add_another: "Create and Add Another" create: "Create" diff --git a/spec/services/vine_voucher_redeemer_service_spec.rb b/spec/services/vine_voucher_redeemer_service_spec.rb new file mode 100644 index 0000000000..10d04d7f03 --- /dev/null +++ b/spec/services/vine_voucher_redeemer_service_spec.rb @@ -0,0 +1,277 @@ +# frozen_string_literal: true + +require "spec_helper" + +RSpec.describe VineVoucherRedeemerService, feature: :connected_apps do + subject(:voucher_redeemer_service) { described_class.new(order: ) } + + let(:user) { order.user } + let(:distributor) { create(:distributor_enterprise) } + let(:order_cycle) { create(:order_cycle, distributors: [distributor]) } + let(:order) { create(:order_with_line_items, line_items_count: 1, distributor:, order_cycle:) } + + let(:vine_voucher) { + create(:voucher_flat_rate, voucher_type: "VINE", code: 'some_code', enterprise: distributor, + amount: 6, external_voucher_id: voucher_id, + external_voucher_set_id: voucher_set_id ) + } + let(:voucher_id) { "9d316d27-0dad-411a-8953-316a1aaf7742" } + let(:voucher_set_id) { "9d314daa-0878-4b73-922d-698047640cf4" } + let(:vine_api_service) { instance_double(VineApiService) } + + before do + allow(VineApiService).to receive(:new).and_return(vine_api_service) + end + + describe "#call" do + context "with a valid voucher" do + let!(:vine_connected_app) { + ConnectedApps::Vine.create( + enterprise: distributor, data: { api_key: "1234568", secret: "my_secret" } + ) + } + let(:data) { + { + meta: { + responseCode: 200, + limit: 50, + offset: 0, + message: "Redemption successful. This was a test redemption. Do NOT provide " \ + "the person with goods or services." + }, + data: { + voucher_id: "9d316d27-0dad-411a-8953-316a1aaf7742", + voucher_set_id: "9d314daa-0878-4b73-922d-698047640cf4", + redeemed_by_user_id: 8, + redeemed_by_team_id: 4, + redeemed_amount: 1, + is_test: 1, + updated_at: "2024-10-21T03:07:09.000000Z", + created_at: "2024-10-21T03:07:09.000000Z", + id: 5 + } + }.deep_stringify_keys + } + + before { add_voucher(vine_voucher) } + + it "redeems the voucher with VINE" do + expect(vine_api_service).to receive(:voucher_redemptions) + .with(voucher_id, voucher_set_id, 600) + .and_return(mock_api_response(success: true, data:)) + + voucher_redeemer_service.call + end + + it "closes the linked assement" do + allow(vine_api_service).to receive(:voucher_redemptions) + .and_return(mock_api_response(success: true, data:)) + + expect { + voucher_redeemer_service.call + }.to change { order.voucher_adjustments.first.state }.to("closed") + end + + it "returns true" do + allow(vine_api_service).to receive(:voucher_redemptions) + .and_return(mock_api_response(success: true, data:)) + + expect(voucher_redeemer_service.call).to be(true) + end + + context "when redeeming fails" do + let(:data) { + { + meta: { responseCode: 400, limit: 50, offset: 0, message: "Invalid merchant team." }, + data: [] + }.deep_stringify_keys + } + before do + allow(vine_api_service).to receive(:voucher_redemptions) + .and_return(mock_api_response(success: false, data:, status: 400)) + end + + it "doesn't close the linked assement" do + expect { + voucher_redeemer_service.call + }.not_to change { order.voucher_adjustments.first.state } + end + + it "returns false" do + expect(voucher_redeemer_service.call).to be(false) + end + + it "adds an error message" do + voucher_redeemer_service.call + + expect(voucher_redeemer_service.errors).to include( + { redeeming_failed: "Redeeming the voucher failed" } + ) + end + end + end + + context "when distributor is not connected to VINE" do + before { add_voucher(vine_voucher) } + + it "returns false" do + expect(voucher_redeemer_service.call).to be(false) + end + + it "doesn't call the VINE API" do + expect(vine_api_service).not_to receive(:voucher_redemptions) + + voucher_redeemer_service.call + end + + it "adds an error message" do + voucher_redeemer_service.call + + expect(voucher_redeemer_service.errors).to include( + { vine_settings: "No Vine api settings for the given enterprise" } + ) + end + + it "doesn't close the linked assement" do + expect { + voucher_redeemer_service.call + }.not_to change { order.voucher_adjustments.first.state } + end + end + + # TODO should we set an error or just do nothing ? + context "when there are no voucher added to the order" do + let!(:vine_connected_app) { + ConnectedApps::Vine.create( + enterprise: distributor, data: { api_key: "1234568", secret: "my_secret" } + ) + } + + it "returns true" do + expect(voucher_redeemer_service.call).to be(true) + end + + it "doesn't call the VINE API" do + expect(vine_api_service).not_to receive(:voucher_redemptions) + + voucher_redeemer_service.call + end + end + + context "with a non vine voucher" do + let!(:vine_connected_app) { + ConnectedApps::Vine.create( + enterprise: distributor, data: { api_key: "1234568", secret: "my_secret" } + ) + } + let(:voucher) { create(:voucher_flat_rate, enterprise: distributor) } + + before { add_voucher(voucher) } + + it "returns true" do + expect(voucher_redeemer_service.call).to be(true) + end + + it "doesn't call the VINE API" do + expect(vine_api_service).not_to receive(:voucher_redemptions) + + voucher_redeemer_service.call + end + end + + context "when there is an API error" do + let!(:vine_connected_app) { + ConnectedApps::Vine.create( + enterprise: distributor, data: { api_key: "1234568", secret: "my_secret" } + ) + } + + before do + add_voucher(vine_voucher) + allow(vine_api_service).to receive(:voucher_redemptions).and_raise(Faraday::Error) + end + + it "returns false" do + expect(voucher_redeemer_service.call).to be(false) + end + + it "adds an error message" do + voucher_redeemer_service.call + + expect(voucher_redeemer_service.errors).to include( + { vine_api: "There was an error communicating with the API" } + ) + end + + it "doesn't close the linked assement" do + expect { + voucher_redeemer_service.call + }.not_to change { order.voucher_adjustments.first.state } + end + + it "logs the error and notify bugsnag" do + expect(Rails.logger).to receive(:error) + expect(Bugsnag).to receive(:notify) + + voucher_redeemer_service.call + end + end + + context "when there is an API authentication error" do + let!(:vine_connected_app) { + ConnectedApps::Vine.create( + enterprise: distributor, data: { api_key: "1234568", secret: "my_secret" } + ) + } + let(:data) { + { + meta: { numRecords: 0, totalRows: 0, responseCode: 401, + message: "Incorrect authorization signature." }, + data: [] + }.deep_stringify_keys + } + + before do + add_voucher(vine_voucher) + + allow(vine_api_service).to receive(:voucher_redemptions).and_return( + mock_api_response(success: false, status: 401, data: ) + ) + end + + it "returns false" do + expect(voucher_redeemer_service.call).to be(false) + end + + it "adds an error message" do + voucher_redeemer_service.call + + expect(voucher_redeemer_service.errors).to include( + { vine_api: "There was an error communicating with the API" } + ) + end + + it "doesn't close the linked assement" do + expect { + voucher_redeemer_service.call + }.not_to change { order.voucher_adjustments.first.state } + end + end + end + + def add_voucher(voucher) + voucher.create_adjustment(voucher.code, order) + VoucherAdjustmentsService.new(order).update + order.update_totals_and_states + end + + def mock_api_response(success:, data: nil, status: 200) + 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 +end From 129ccc33f83f25e24d24eadfce378c86918e3a2f Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Tue, 22 Oct 2024 13:48:17 +1100 Subject: [PATCH 09/33] CheckoutController, add VINE voucher redemption --- .rubocop_todo.yml | 33 ++++-------- app/controllers/checkout_controller.rb | 12 +++++ config/locales/en.yml | 1 + spec/controllers/checkout_controller_spec.rb | 55 ++++++++++++++++++++ 4 files changed, 78 insertions(+), 23 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index fd4b9ed7e7..f2132a4fe0 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -34,12 +34,6 @@ Lint/EmptyClass: Exclude: - 'spec/lib/reports/report_loader_spec.rb' -# Offense count: 1 -# Configuration parameters: AllowComments. -Lint/EmptyFile: - Exclude: - - 'spec/lib/open_food_network/enterprise_injection_data_spec.rb' - # Offense count: 2 Lint/FloatComparison: Exclude: @@ -85,14 +79,13 @@ Lint/UselessMethodDefinition: Exclude: - 'app/models/spree/gateway.rb' -# Offense count: 24 +# Offense count: 23 # Configuration parameters: AllowedMethods, AllowedPatterns, CountRepeatedAttributes, Max. Metrics/AbcSize: Exclude: - 'app/controllers/admin/enterprises_controller.rb' - 'app/controllers/payment_gateways/paypal_controller.rb' - 'app/controllers/spree/admin/payments_controller.rb' - - 'app/controllers/spree/admin/taxons_controller.rb' - 'app/controllers/spree/admin/variants_controller.rb' - 'app/controllers/spree/orders_controller.rb' - 'app/helpers/spree/admin/navigation_helper.rb' @@ -127,7 +120,7 @@ Metrics/BlockNesting: Exclude: - 'app/models/spree/payment/processing.rb' -# Offense count: 46 +# Offense count: 47 # Configuration parameters: CountComments, Max, CountAsOne. Metrics/ClassLength: Exclude: @@ -137,6 +130,7 @@ Metrics/ClassLength: - 'app/controllers/admin/resource_controller.rb' - 'app/controllers/admin/subscriptions_controller.rb' - 'app/controllers/application_controller.rb' + - 'app/controllers/checkout_controller.rb' - 'app/controllers/payment_gateways/paypal_controller.rb' - 'app/controllers/spree/admin/orders_controller.rb' - 'app/controllers/spree/admin/payment_methods_controller.rb' @@ -178,12 +172,11 @@ Metrics/ClassLength: - 'lib/reporting/reports/enterprise_fee_summary/scope.rb' - 'lib/reporting/reports/xero_invoices/base.rb' -# Offense count: 32 +# Offense count: 31 # Configuration parameters: AllowedMethods, AllowedPatterns, Max. Metrics/CyclomaticComplexity: Exclude: - 'app/controllers/admin/enterprises_controller.rb' - - 'app/controllers/spree/admin/taxons_controller.rb' - 'app/controllers/spree/orders_controller.rb' - 'app/helpers/checkout_helper.rb' - 'app/helpers/order_cycles_helper.rb' @@ -208,13 +201,12 @@ Metrics/CyclomaticComplexity: - 'lib/spree/localized_number.rb' - 'spec/models/product_importer_spec.rb' -# Offense count: 24 +# Offense count: 23 # Configuration parameters: CountComments, Max, CountAsOne, AllowedMethods, AllowedPatterns. Metrics/MethodLength: Exclude: - 'app/controllers/admin/enterprises_controller.rb' - 'app/controllers/payment_gateways/paypal_controller.rb' - - 'app/controllers/spree/admin/taxons_controller.rb' - 'app/controllers/spree/orders_controller.rb' - 'app/helpers/spree/admin/navigation_helper.rb' - 'app/models/spree/ability.rb' @@ -293,19 +285,17 @@ Metrics/ParameterLists: - 'spec/support/controller_requests_helper.rb' - 'spec/system/admin/reports_spec.rb' -# Offense count: 4 +# Offense count: 3 # Configuration parameters: AllowedMethods, AllowedPatterns, Max. Metrics/PerceivedComplexity: Exclude: - - 'app/controllers/spree/admin/taxons_controller.rb' - 'app/models/enterprise_relationship.rb' - 'app/models/spree/ability.rb' - 'app/models/spree/order/checkout.rb' -# Offense count: 8 +# Offense count: 7 Naming/AccessorMethodName: Exclude: - - 'app/controllers/spree/admin/taxonomies_controller.rb' - 'app/mailers/producer_mailer.rb' - 'app/models/spree/order.rb' - 'app/services/checkout/post_checkout_actions.rb' @@ -353,7 +343,7 @@ Naming/VariableNumber: - 'spec/models/spree/tax_rate_spec.rb' - 'spec/requests/api/orders_spec.rb' -# Offense count: 142 +# Offense count: 143 # This cop supports unsafe autocorrection (--autocorrect-all). # Configuration parameters: ResponseMethods. # ResponseMethods: response, last_response @@ -557,7 +547,7 @@ RSpecRails/InferredSpecType: - 'spec/requests/voucher_adjustments_spec.rb' - 'spec/routing/stripe_spec.rb' -# Offense count: 22 +# Offense count: 21 # Configuration parameters: IgnoreScopes, Include. # Include: app/models/**/*.rb Rails/InverseOf: @@ -572,7 +562,6 @@ Rails/InverseOf: - 'app/models/spree/price.rb' - 'app/models/spree/product.rb' - 'app/models/spree/stock_item.rb' - - 'app/models/spree/taxonomy.rb' - 'app/models/spree/variant.rb' - 'app/models/subscription_line_item.rb' @@ -720,7 +709,7 @@ Style/GlobalStdStream: - 'lib/tasks/subscriptions/debug.rake' - 'lib/tasks/subscriptions/test.rake' -# Offense count: 12 +# Offense count: 10 # This cop supports unsafe autocorrection (--autocorrect-all). # Configuration parameters: AllowSplatArgument. Style/HashConversion: @@ -728,9 +717,7 @@ Style/HashConversion: - 'app/controllers/admin/column_preferences_controller.rb' - 'app/controllers/admin/variant_overrides_controller.rb' - 'app/controllers/spree/admin/products_controller.rb' - - 'app/models/order_cycle.rb' - 'app/models/product_import/product_importer.rb' - - 'app/models/spree/shipping_method.rb' - 'app/serializers/api/admin/exchange_serializer.rb' - 'app/services/variants_stock_levels.rb' - 'spec/controllers/admin/inventory_items_controller_spec.rb' diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb index efd8e80eeb..c5bce3a5e1 100644 --- a/app/controllers/checkout_controller.rb +++ b/app/controllers/checkout_controller.rb @@ -78,6 +78,18 @@ class CheckoutController < BaseController return true if redirect_to_payment_gateway + # Redeem VINE voucher + vine_voucher_redeemer = VineVoucherRedeemerService.new(order: @order) + if vine_voucher_redeemer.call == false + # rubocop:disable Rails/DeprecatedActiveModelErrorsMethods + flash[:error] = if vine_voucher_redeemer.errors.keys.include?(:redeeming_failed) + vine_voucher_redeemer.errors[:redeeming_failed] + else + I18n.t('checkout.errors.voucher_redeeming_error') + end + return false + # rubocop:enable Rails/DeprecatedActiveModelErrorsMethods + end @order.process_payments! @order.confirm! order_completion_reset @order diff --git a/config/locales/en.yml b/config/locales/en.yml index abbbc23c4b..8e71f4b38b 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -2134,6 +2134,7 @@ en: voucher_not_found: Not found add_voucher_error: There was an error while adding the voucher create_voucher_error: "There was an error while creating the voucher: %{error}" + voucher_redeeming_error: There was an error while trying to redeem your voucher shops: hubs: show_closed_shops: "Show closed shops" diff --git a/spec/controllers/checkout_controller_spec.rb b/spec/controllers/checkout_controller_spec.rb index ed1da29653..37957c2ff9 100644 --- a/spec/controllers/checkout_controller_spec.rb +++ b/spec/controllers/checkout_controller_spec.rb @@ -433,6 +433,7 @@ RSpec.describe CheckoutController, type: :controller do context "summary step" do let(:step) { "summary" } + let(:checkout_params) { { confirm_order: "Complete order" } } before do order.bill_address = address @@ -496,6 +497,60 @@ RSpec.describe CheckoutController, type: :controller do end end + context "with a VINE voucher", feature: :connected_apps do + let(:vine_voucher) { + create(:voucher_flat_rate, voucher_type: "VINE", code: 'some_code', + enterprise: distributor, amount: 6) + } + let(:vine_voucher_redeemer) { instance_double(VineVoucherRedeemerService) } + + before do + # Adding voucher to the order + vine_voucher.create_adjustment(vine_voucher.code, order) + VoucherAdjustmentsService.new(order).update + order.update_totals_and_states + + allow(VineVoucherRedeemerService).to receive(:new).and_return(vine_voucher_redeemer) + end + + it "completes the order and redirects to order confirmation" do + expect(vine_voucher_redeemer).to receive(:call).and_return(true) + + put(:update, params:) + + expect(response).to redirect_to order_path(order, order_token: order.token) + expect(order.reload.state).to eq "complete" + end + + context "when redeeming the voucher fails" do + it "returns 422 and some error" do + allow(vine_voucher_redeemer).to receive(:call).and_return(false) + allow(vine_voucher_redeemer).to receive(:errors).and_return( + { redeeming_failed: "Redeeming the voucher failed" } + ) + + put(:update, params:) + + expect(response.status).to eq 422 + expect(flash[:error]).to match "Redeeming the voucher failed" + end + end + + context "when an other error happens" do + it "returns 422 and some error" do + allow(vine_voucher_redeemer).to receive(:call).and_return(false) + allow(vine_voucher_redeemer).to receive(:errors).and_return( + { vine_api: "There was an error communicating with the API" } + ) + + put(:update, params:) + + expect(response.status).to eq 422 + expect(flash[:error]).to match "There was an error while trying to redeem your voucher" + end + end + end + context "when an external payment gateway is used" do before do expect(Checkout::PaymentMethodFetcher). From 4906c19c8e77590fbf61f091a55a398edd33e61f Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Tue, 22 Oct 2024 15:54:52 +1100 Subject: [PATCH 10/33] Checkout Summary, remove shared example --- spec/system/consumer/checkout/summary_spec.rb | 44 +++++++++---------- 1 file changed, 21 insertions(+), 23 deletions(-) diff --git a/spec/system/consumer/checkout/summary_spec.rb b/spec/system/consumer/checkout/summary_spec.rb index cf5e8c4fe9..6fc9a5028e 100644 --- a/spec/system/consumer/checkout/summary_spec.rb +++ b/spec/system/consumer/checkout/summary_spec.rb @@ -393,49 +393,47 @@ RSpec.describe "As a consumer, I want to checkout my order" do } let(:payment) { completed_order.payments.first } - shared_examples "order confirmation page" do |paid_state, paid_amount| - it "displays the relevant information" do - expect(page).to have_content paid_state.to_s - expect(page).to have_selector('strong', text: "Amount Paid") - expect(page).to have_selector('strong', text: with_currency(paid_amount)) - end - end - context "an order with balance due" do before { set_up_order(-10, "balance_due") } - it_behaves_like "order confirmation page", "NOT PAID", "40" do - before do - expect(page).to have_selector('h5', text: "Balance Due") - expect(page).to have_selector('h5', text: with_currency(10)) - end + it "displays balance due and paid state" do + expect(page).to have_selector('h5', text: "Balance Due") + expect(page).to have_selector('h5', text: with_currency(10)) + + confirmation_page_expect_paid(paid_state: "NOT PAID" ,paid_amount: 40) end end context "an order with credit owed" do before { set_up_order(10, "credit_owed") } - it_behaves_like "order confirmation page", "PAID", "60" do - before do - expect(page).to have_selector('h5', text: "Credit Owed") - expect(page).to have_selector('h5', text: with_currency(-10)) - end + it "displays Credit owned and paid state" do + expect(page).to have_selector('h5', text: "Credit Owed") + expect(page).to have_selector('h5', text: with_currency(-10)) + + confirmation_page_expect_paid(paid_state: "PAID" ,paid_amount: 60) end end context "an order with no outstanding balance" do before { set_up_order(0, "paid") } - it_behaves_like "order confirmation page", "PAID", "50" do - before do - expect(page).not_to have_selector('h5', text: "Credit Owed") - expect(page).not_to have_selector('h5', text: "Balance Due") - end + it "displays paid state" do + expect(page).not_to have_selector('h5', text: "Credit Owed") + expect(page).not_to have_selector('h5', text: "Balance Due") + + confirmation_page_expect_paid(paid_state: "PAID" ,paid_amount: 50) end end end end + def confirmation_page_expect_paid(paid_state: ,paid_amount:) + expect(page).to have_content paid_state.to_s + expect(page).to have_selector('strong', text: "Amount Paid") + expect(page).to have_selector('strong', text: with_currency(paid_amount)) + end + def add_voucher_to_order(voucher, order) voucher.create_adjustment(voucher.code, order) VoucherAdjustmentsService.new(order).update From cf13dc2ff6a81e68eb50040d92df944cd0e8bec7 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Wed, 23 Oct 2024 14:53:05 +1100 Subject: [PATCH 11/33] Add system spec when completing order with VINE voucher --- .../completes_the_order.yml | 55 +++++++++++++++++++ spec/system/consumer/checkout/summary_spec.rb | 51 +++++++++++++++-- 2 files changed, 102 insertions(+), 4 deletions(-) create mode 100644 spec/fixtures/vcr_cassettes/As_a_consumer_I_want_to_checkout_my_order/as_a_logged_in_user/summary_step/with_a_VINE_voucher/when_placing_the_order/completes_the_order.yml diff --git a/spec/fixtures/vcr_cassettes/As_a_consumer_I_want_to_checkout_my_order/as_a_logged_in_user/summary_step/with_a_VINE_voucher/when_placing_the_order/completes_the_order.yml b/spec/fixtures/vcr_cassettes/As_a_consumer_I_want_to_checkout_my_order/as_a_logged_in_user/summary_step/with_a_VINE_voucher/when_placing_the_order/completes_the_order.yml new file mode 100644 index 0000000000..b3a5737dfb --- /dev/null +++ b/spec/fixtures/vcr_cassettes/As_a_consumer_I_want_to_checkout_my_order/as_a_logged_in_user/summary_step/with_a_VINE_voucher/when_placing_the_order/completes_the_order.yml @@ -0,0 +1,55 @@ +--- +http_interactions: +- request: + method: post + uri: https://vine-staging.openfoodnetwork.org.au/api/v1/voucher-redemptions + body: + encoding: UTF-8 + string: '{"voucher_id":"9d316d27-4e3c-4c8c-b3c8-8e23cc171f20","voucher_set_id":"9d314daa-0878-4b73-922d-698047640cf4","amount":1}' + headers: + X-Authorization: + - "" + Accept: + - application/json + User-Agent: + - Faraday v2.9.0 + Content-Type: + - application/json + Authorization: + - "" + Accept-Encoding: + - gzip;q=1.0,deflate;q=0.6,identity;q=0.3 + response: + status: + code: 200 + message: OK + headers: + Server: + - nginx + Content-Type: + - application/json + Transfer-Encoding: + - chunked + Connection: + - keep-alive + Vary: + - Accept-Encoding + Cache-Control: + - no-cache, private + Date: + - Wed, 23 Oct 2024 03:16:39 GMT + Access-Control-Allow-Origin: + - "*" + X-Frame-Options: + - SAMEORIGIN + X-Xss-Protection: + - 1; mode=block + X-Content-Type-Options: + - nosniff + body: + encoding: ASCII-8BIT + string: '{"meta":{"responseCode":200,"limit":50,"offset":0,"message":"Redemption + successful. This was a test redemption. Do NOT provide the person with goods + or services."},"data":{"voucher_id":"9d316d27-4e3c-4c8c-b3c8-8e23cc171f20","voucher_set_id":"9d314daa-0878-4b73-922d-698047640cf4","redeemed_by_user_id":8,"redeemed_by_team_id":4,"redeemed_amount":1,"is_test":1,"updated_at":"2024-10-23T03:16:39.000000Z","created_at":"2024-10-23T03:16:39.000000Z","id":7}}' + recorded_at: Wed, 23 Oct 2024 03:16:39 GMT +recorded_with: VCR 6.2.0 diff --git a/spec/system/consumer/checkout/summary_spec.rb b/spec/system/consumer/checkout/summary_spec.rb index 6fc9a5028e..313c133947 100644 --- a/spec/system/consumer/checkout/summary_spec.rb +++ b/spec/system/consumer/checkout/summary_spec.rb @@ -344,6 +344,49 @@ RSpec.describe "As a consumer, I want to checkout my order" do end end end + + context "with a VINE voucher", feature: :connected_apps do + let!(:vine_connected_app) { + ConnectedApps::Vine.create( + enterprise: distributor, data: { api_key: "1234568", secret: "my_secret" } + ) + } + let(:vine_voucher) { + create(:voucher_flat_rate, voucher_type: "VINE", code: 'some_vine_code', + enterprise: distributor, amount: 0.01) + } + + before do + allow(ENV).to receive(:fetch).and_call_original + allow(ENV).to receive(:fetch).with("VINE_API_URL") + .and_return("https://vine-staging.openfoodnetwork.org.au/api/v1") + + add_voucher_to_order(vine_voucher, order) + end + + it "shows the applied voucher" do + visit checkout_step_path(:summary) + + within ".summary-right" do + expect(page).to have_content "some_vine_code" + expect(page).to have_content "-0.01" + end + end + + context "when placing the order" do + it "completes the order", :vcr do + visit checkout_step_path(:summary) + + place_order + + within "#line-items" do + expect(page).to have_content "Voucher: some_vine_code" + expect(page).to have_content "$-0.01" + end + expect(order.reload.state).to eq "complete" + end + end + end end context "with previous open orders" do @@ -400,7 +443,7 @@ RSpec.describe "As a consumer, I want to checkout my order" do expect(page).to have_selector('h5', text: "Balance Due") expect(page).to have_selector('h5', text: with_currency(10)) - confirmation_page_expect_paid(paid_state: "NOT PAID" ,paid_amount: 40) + confirmation_page_expect_paid(paid_state: "NOT PAID", paid_amount: 40) end end @@ -411,7 +454,7 @@ RSpec.describe "As a consumer, I want to checkout my order" do expect(page).to have_selector('h5', text: "Credit Owed") expect(page).to have_selector('h5', text: with_currency(-10)) - confirmation_page_expect_paid(paid_state: "PAID" ,paid_amount: 60) + confirmation_page_expect_paid(paid_state: "PAID", paid_amount: 60) end end @@ -422,13 +465,13 @@ RSpec.describe "As a consumer, I want to checkout my order" do expect(page).not_to have_selector('h5', text: "Credit Owed") expect(page).not_to have_selector('h5', text: "Balance Due") - confirmation_page_expect_paid(paid_state: "PAID" ,paid_amount: 50) + confirmation_page_expect_paid(paid_state: "PAID", paid_amount: 50) end end end end - def confirmation_page_expect_paid(paid_state: ,paid_amount:) + def confirmation_page_expect_paid(paid_state:, paid_amount:) expect(page).to have_content paid_state.to_s expect(page).to have_selector('strong', text: "Amount Paid") expect(page).to have_selector('strong', text: with_currency(paid_amount)) From 6251814152b2b83668ca97a15e56ac463f24beb6 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Wed, 23 Oct 2024 16:17:53 +1100 Subject: [PATCH 12/33] Hide VINE voucher from admin voucher page --- app/views/admin/enterprises/form/_vouchers.html.haml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/admin/enterprises/form/_vouchers.html.haml b/app/views/admin/enterprises/form/_vouchers.html.haml index 2e3912475a..7f8f343671 100644 --- a/app/views/admin/enterprises/form/_vouchers.html.haml +++ b/app/views/admin/enterprises/form/_vouchers.html.haml @@ -3,7 +3,7 @@ = t('.add_new') %br -- if @enterprise.vouchers.with_deleted.present? +- if @enterprise.vouchers.not_vine.with_deleted.present? %table %thead %tr @@ -17,7 +17,7 @@ /%th= t('.customers') /%th= t('.net_value') %tbody - - @enterprise.vouchers.with_deleted.order(deleted_at: :desc, code: :asc).each do |voucher| + - @enterprise.vouchers.not_vine.with_deleted.order(deleted_at: :desc, code: :asc).each do |voucher| %tr %td= voucher.code %td= voucher.display_value From 724d5a2ca01ce1654a2a5865bb061cb808f0ce7a Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 28 Oct 2024 14:32:06 +1100 Subject: [PATCH 13/33] Add spec for creating a payment from admin page --- spec/requests/spree/admin/payments_spec.rb | 62 ++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/spec/requests/spree/admin/payments_spec.rb b/spec/requests/spree/admin/payments_spec.rb index 18a2a4f75c..70c9dd752b 100644 --- a/spec/requests/spree/admin/payments_spec.rb +++ b/spec/requests/spree/admin/payments_spec.rb @@ -10,6 +10,68 @@ RSpec.describe Spree::Admin::PaymentsController, type: :request do sign_in create(:admin_user) end + describe "POST /admin/orders/:order_number/payments.json" do + let(:params) do + { + payment: { + payment_method_id: payment_method.id, amount: order.total + } + } + end + let(:payment_method) do + create(:payment_method, distributors: [order.distributor]) + end + + it "creates a payment" do + expect { + post("/admin/orders/#{order.number}/payments.json", params:) + }.to change { order.payments.count }.by(1) + end + + it "redirect to payments page" do + post("/admin/orders/#{order.number}/payments.json", params:) + + expect(response).to redirect_to(spree.admin_order_payments_path(order)) + expect(flash[:success]).to eq "Payment has been successfully created!" + end + + context "when failing to create payment" do + it "redirects to payments page" do + payment_mock = instance_double(Spree::Payment) + allow(order.payments).to receive(:build).and_return(payment_mock) + allow(payment_mock).to receive(:save).and_return(false) + + post("/admin/orders/#{order.number}/payments.json", params:) + + expect(response).to redirect_to(spree.admin_order_payments_path(order)) + end + end + + context "when a getway error happens" do + let(:payment_method) do + create(:stripe_sca_payment_method, distributors: [order.distributor]) + end + + it "redirect to payments page" do + allow(Spree::Order).to receive(:find_by!).and_return(order) + + stripe_sca_payment_authorize = + instance_double(OrderManagement::Order::StripeScaPaymentAuthorize) + allow(OrderManagement::Order::StripeScaPaymentAuthorize).to receive(:new) + .and_return(stripe_sca_payment_authorize) + # Simulate an error + allow(stripe_sca_payment_authorize).to receive(:call!) do + order.errors.add(:base, "authorization_failure") + end + + post("/admin/orders/#{order.number}/payments.json", params:) + + expect(response).to redirect_to(spree.admin_order_payments_path(order)) + expect(flash[:error]).to eq("Authorization Failure") + end + end + end + describe "PUT /admin/orders/:order_number/payments/:id/fire" do let(:payment) do create( From 92c4cb9b7f8d9017e6fa9e2d15e66921e212e26b Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 28 Oct 2024 14:37:37 +1100 Subject: [PATCH 14/33] Redeem VINE voucher when creating a new payment Creating a new payment will try to complete the order, so we want to redeem any VINE voucher associated with the order first --- .../spree/admin/payments_controller.rb | 21 ++++++- spec/requests/spree/admin/payments_spec.rb | 56 +++++++++++++++++++ 2 files changed, 76 insertions(+), 1 deletion(-) diff --git a/app/controllers/spree/admin/payments_controller.rb b/app/controllers/spree/admin/payments_controller.rb index 0afaa5227c..075209e052 100644 --- a/app/controllers/spree/admin/payments_controller.rb +++ b/app/controllers/spree/admin/payments_controller.rb @@ -24,9 +24,12 @@ module Spree end def create + # Try to redeem VINE voucher first as we don't want to create a payment and complete + # the order if it fails + return redirect_to spree.admin_order_payments_path(@order) unless redeem_vine_voucher + @payment = @order.payments.build(object_params) load_payment_source - begin unless @payment.save redirect_to spree.admin_order_payments_path(@order) @@ -182,6 +185,22 @@ module Spree %w{capture void_transaction credit refund resend_authorization_email capture_and_complete_order} end + + def redeem_vine_voucher + vine_voucher_redeemer = VineVoucherRedeemerService.new(order: @order) + if vine_voucher_redeemer.call == false + # rubocop:disable Rails/DeprecatedActiveModelErrorsMethods + flash[:error] = if vine_voucher_redeemer.errors.keys.include?(:redeeming_failed) + vine_voucher_redeemer.errors[:redeeming_failed] + else + I18n.t('checkout.errors.voucher_redeeming_error') + end + # rubocop:enable Rails/DeprecatedActiveModelErrorsMethods + return false + end + + true + end end end end diff --git a/spec/requests/spree/admin/payments_spec.rb b/spec/requests/spree/admin/payments_spec.rb index 70c9dd752b..c0bd774d07 100644 --- a/spec/requests/spree/admin/payments_spec.rb +++ b/spec/requests/spree/admin/payments_spec.rb @@ -70,6 +70,62 @@ RSpec.describe Spree::Admin::PaymentsController, type: :request do expect(flash[:error]).to eq("Authorization Failure") end end + + context "with a VINE voucher", feature: :connected_apps do + let(:vine_voucher) { + create(:voucher_flat_rate, voucher_type: "VINE", code: 'some_code', + enterprise: order.distributor, amount: 6) + } + let(:vine_voucher_redeemer) { instance_double(VineVoucherRedeemerService) } + + before do + # Adding voucher to the order + vine_voucher.create_adjustment(vine_voucher.code, order) + VoucherAdjustmentsService.new(order).update + order.update_totals_and_states + + allow(VineVoucherRedeemerService).to receive(:new).and_return(vine_voucher_redeemer) + end + + it "completes the order and redirects to payment page" do + expect(vine_voucher_redeemer).to receive(:call).and_return(true) + + post("/admin/orders/#{order.number}/payments.json", params:) + + expect(response).to redirect_to(spree.admin_order_payments_path(order)) + expect(flash[:success]).to eq "Payment has been successfully created!" + + expect(order.reload.state).to eq "complete" + end + + context "when redeeming the voucher fails" do + it "redirect to payments page" do + allow(vine_voucher_redeemer).to receive(:call).and_return(false) + allow(vine_voucher_redeemer).to receive(:errors).and_return( + { redeeming_failed: "Redeeming the voucher failed" } + ) + + post("/admin/orders/#{order.number}/payments.json", params:) + + expect(response).to redirect_to(spree.admin_order_payments_path(order)) + expect(flash[:error]).to match "Redeeming the voucher failed" + end + end + + context "when an other error happens" do + it "redirect to payments page" do + allow(vine_voucher_redeemer).to receive(:call).and_return(false) + allow(vine_voucher_redeemer).to receive(:errors).and_return( + { vine_api: "There was an error communicating with the API" } + ) + + post("/admin/orders/#{order.number}/payments.json", params:) + + expect(response).to redirect_to(spree.admin_order_payments_path(order)) + expect(flash[:error]).to match "There was an error while trying to redeem your voucher" + end + end + end end describe "PUT /admin/orders/:order_number/payments/:id/fire" do From afb336d7895037c4ed9fdf416323bdd696e66592 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 28 Oct 2024 15:58:21 +1100 Subject: [PATCH 15/33] Add spec for fire event "capture_and_complete_order" --- spec/requests/spree/admin/payments_spec.rb | 49 +++++++++++++++++++++- 1 file changed, 48 insertions(+), 1 deletion(-) diff --git a/spec/requests/spree/admin/payments_spec.rb b/spec/requests/spree/admin/payments_spec.rb index c0bd774d07..b4627cc098 100644 --- a/spec/requests/spree/admin/payments_spec.rb +++ b/spec/requests/spree/admin/payments_spec.rb @@ -175,7 +175,7 @@ RSpec.describe Spree::Admin::PaymentsController, type: :request do end end - context "with 'void' parameter" do + context "with 'void' event" do before do allow(Spree::Payment).to receive(:find).and_return(payment) end @@ -219,6 +219,53 @@ RSpec.describe Spree::Admin::PaymentsController, type: :request do end end + context "with 'capture_and_complete_order' event" do + before do + allow(Spree::Payment).to receive(:find).and_return(payment) + end + + it "calls capture_and_complete_order! on payment" do + expect(payment).to receive(:capture_and_complete_order!) + + put( + "/admin/orders/#{order.number}/payments/#{order.payments.first.id}/" \ + "fire?e=capture_and_complete_order", + params: {}, + headers: + ) + end + + it "redirect to payments page" do + allow(payment).to receive(:capture_and_complete_order!).and_return(true) + + put( + "/admin/orders/#{order.number}/payments/#{order.payments.first.id}/" \ + "fire?e=capture_and_complete_order", + params: {}, + headers: + ) + + expect(response).to redirect_to(spree.admin_order_payments_url(order)) + expect(flash[:success]).to eq "Payment Updated" + end + + context "when capture_and_complete_order! fails" do + it "set an error flash message" do + allow(payment).to receive(:capture_and_complete_order!).and_return(false) + + put( + "/admin/orders/#{order.number}/payments/#{order.payments.first.id}/" \ + "fire?e=capture_and_complete_order", + params: {}, + headers: + ) + + expect(response).to redirect_to(spree.admin_order_payments_url(order)) + expect(flash[:error]).to eq "Could not update the payment" + end + end + end + context "when something unexpected happen" do before do allow(Spree::Payment).to receive(:find).and_return(payment) From 9f3da1af4f3377fefd370a8aac1eced1d9701169 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 28 Oct 2024 16:07:35 +1100 Subject: [PATCH 16/33] Reddeem VINE voucher when firing "capture_and_complete_order"o 'Spree::Payment#capture_and_complete!' will try to complete the order, so we want to redeem any VINE voucher associated with the order first. --- .rubocop_todo.yml | 5 +- .../spree/admin/payments_controller.rb | 4 + spec/requests/spree/admin/payments_spec.rb | 79 ++++++++++++++++++- 3 files changed, 82 insertions(+), 6 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index f2132a4fe0..92432aae32 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -79,7 +79,7 @@ Lint/UselessMethodDefinition: Exclude: - 'app/models/spree/gateway.rb' -# Offense count: 23 +# Offense count: 24 # Configuration parameters: AllowedMethods, AllowedPatterns, CountRepeatedAttributes, Max. Metrics/AbcSize: Exclude: @@ -172,11 +172,12 @@ Metrics/ClassLength: - 'lib/reporting/reports/enterprise_fee_summary/scope.rb' - 'lib/reporting/reports/xero_invoices/base.rb' -# Offense count: 31 +# Offense count: 32 # Configuration parameters: AllowedMethods, AllowedPatterns, Max. Metrics/CyclomaticComplexity: Exclude: - 'app/controllers/admin/enterprises_controller.rb' + - 'app/controllers/spree/admin/payments_controller.rb' - 'app/controllers/spree/orders_controller.rb' - 'app/helpers/checkout_helper.rb' - 'app/helpers/order_cycles_helper.rb' diff --git a/app/controllers/spree/admin/payments_controller.rb b/app/controllers/spree/admin/payments_controller.rb index 075209e052..bd60138d70 100644 --- a/app/controllers/spree/admin/payments_controller.rb +++ b/app/controllers/spree/admin/payments_controller.rb @@ -54,6 +54,10 @@ module Spree event = params[:e] return unless event && @payment.payment_source + # capture_and_complete_order will complete the order, so we want to try to redeem VINE + # voucher first and exit if it fails + return if event == "capture_and_complete_order" && !redeem_vine_voucher + # Because we have a transition method also called void, we do this to avoid conflicts. event = "void_transaction" if event == "void" if allowed_events.include?(event) && @payment.public_send("#{event}!") diff --git a/spec/requests/spree/admin/payments_spec.rb b/spec/requests/spree/admin/payments_spec.rb index b4627cc098..aeeef365c2 100644 --- a/spec/requests/spree/admin/payments_spec.rb +++ b/spec/requests/spree/admin/payments_spec.rb @@ -79,10 +79,7 @@ RSpec.describe Spree::Admin::PaymentsController, type: :request do let(:vine_voucher_redeemer) { instance_double(VineVoucherRedeemerService) } before do - # Adding voucher to the order - vine_voucher.create_adjustment(vine_voucher.code, order) - VoucherAdjustmentsService.new(order).update - order.update_totals_and_states + add_voucher_to_order(vine_voucher, order) allow(VineVoucherRedeemerService).to receive(:new).and_return(vine_voucher_redeemer) end @@ -264,6 +261,74 @@ RSpec.describe Spree::Admin::PaymentsController, type: :request do expect(flash[:error]).to eq "Could not update the payment" end end + + context "with a VINE voucher", feature: :connected_apps do + let(:vine_voucher) { + create(:voucher_flat_rate, voucher_type: "VINE", code: 'some_code', + enterprise: order.distributor, amount: 6) + } + let(:vine_voucher_redeemer) { instance_double(VineVoucherRedeemerService) } + + before do + add_voucher_to_order(vine_voucher, order) + + allow(VineVoucherRedeemerService).to receive(:new).and_return(vine_voucher_redeemer) + end + + it "completes the order and redirects to payment page" do + expect(vine_voucher_redeemer).to receive(:call).and_return(true) + + put( + "/admin/orders/#{order.number}/payments/#{order.payments.first.id}/" \ + "fire?e=capture_and_complete_order", + params: {}, + headers: + ) + + expect(response).to redirect_to(spree.admin_order_payments_url(order)) + expect(flash[:success]).to eq "Payment Updated" + + expect(order.reload.state).to eq "complete" + end + + context "when redeeming the voucher fails" do + it "redirect to payments page" do + allow(vine_voucher_redeemer).to receive(:call).and_return(false) + allow(vine_voucher_redeemer).to receive(:errors).and_return( + { redeeming_failed: "Redeeming the voucher failed" } + ) + + put( + "/admin/orders/#{order.number}/payments/#{order.payments.first.id}/" \ + "fire?e=capture_and_complete_order", + params: {}, + headers: + ) + + expect(response).to redirect_to(spree.admin_order_payments_url(order)) + expect(flash[:error]).to match "Redeeming the voucher failed" + end + end + + context "when an other error happens" do + it "redirect to payments page" do + allow(vine_voucher_redeemer).to receive(:call).and_return(false) + allow(vine_voucher_redeemer).to receive(:errors).and_return( + { vine_api: "There was an error communicating with the API" } + ) + + put( + "/admin/orders/#{order.number}/payments/#{order.payments.first.id}/" \ + "fire?e=capture_and_complete_order", + params: {}, + headers: + ) + + expect(response).to redirect_to(spree.admin_order_payments_url(order)) + expect(flash[:error]).to match "There was an error while trying to redeem your voucher" + end + end + end end context "when something unexpected happen" do @@ -298,4 +363,10 @@ RSpec.describe Spree::Admin::PaymentsController, type: :request do end end end + + def add_voucher_to_order(voucher, order) + voucher.create_adjustment(voucher.code, order) + VoucherAdjustmentsService.new(order).update + order.update_totals_and_states + end end From 0569b30e0d0df2d7bcff7e1d637d77ed79f58453 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Tue, 29 Oct 2024 10:45:05 +1100 Subject: [PATCH 17/33] Refactor Vine related services Move them under Vine module to keep the code nicely organised --- .../admin/connected_apps_controller.rb | 6 +- app/controllers/checkout_controller.rb | 4 +- .../spree/admin/payments_controller.rb | 4 +- .../voucher_adjustments_controller.rb | 2 +- app/services/vine/api_service.rb | 77 +++++++++++++++++ app/services/vine/jwt_service.rb | 23 +++++ app/services/vine/voucher_redeemer_service.rb | 66 +++++++++++++++ .../vine/voucher_validator_service.rb | 84 +++++++++++++++++++ app/services/vine_api_service.rb | 75 ----------------- app/services/vine_jwt_service.rb | 21 ----- app/services/vine_voucher_redeemer_service.rb | 64 -------------- .../vine_voucher_validator_service.rb | 81 ------------------ spec/controllers/checkout_controller_spec.rb | 10 +-- .../returns_the_response.yml | 0 .../returns_the_response.yml | 0 .../returns_the_response.yml | 0 spec/models/connected_apps/vine_spec.rb | 2 +- .../admin/connected_apps_controller_spec.rb | 8 +- spec/requests/spree/admin/payments_spec.rb | 20 ++--- spec/requests/voucher_adjustments_spec.rb | 4 +- .../api_service_spec.rb} | 10 +-- .../jwt_service_spec.rb} | 2 +- .../voucher_redeemer_service_spec.rb} | 56 ++++++------- .../voucher_validator_service_spec.rb} | 6 +- 24 files changed, 317 insertions(+), 308 deletions(-) create mode 100644 app/services/vine/api_service.rb create mode 100644 app/services/vine/jwt_service.rb create mode 100644 app/services/vine/voucher_redeemer_service.rb create mode 100644 app/services/vine/voucher_validator_service.rb delete mode 100644 app/services/vine_api_service.rb delete mode 100644 app/services/vine_jwt_service.rb delete mode 100644 app/services/vine_voucher_redeemer_service.rb delete mode 100644 app/services/vine_voucher_validator_service.rb rename spec/fixtures/vcr_cassettes/{VineApiService => Vine_ApiService}/_my_team/when_a_request_succeed/returns_the_response.yml (100%) rename spec/fixtures/vcr_cassettes/{VineApiService => Vine_ApiService}/_voucher_redemptions/when_a_request_succeed/returns_the_response.yml (100%) rename spec/fixtures/vcr_cassettes/{VineApiService => Vine_ApiService}/_voucher_validation/when_a_request_succeed/returns_the_response.yml (100%) rename spec/services/{vine_api_service_spec.rb => vine/api_service_spec.rb} (96%) rename spec/services/{vine_jwt_service_spec.rb => vine/jwt_service_spec.rb} (97%) rename spec/services/{vine_voucher_redeemer_service_spec.rb => vine/voucher_redeemer_service_spec.rb} (84%) rename spec/services/{vine_voucher_validator_service_spec.rb => vine/voucher_validator_service_spec.rb} (98%) diff --git a/app/controllers/admin/connected_apps_controller.rb b/app/controllers/admin/connected_apps_controller.rb index a5308d4984..083afd946d 100644 --- a/app/controllers/admin/connected_apps_controller.rb +++ b/app/controllers/admin/connected_apps_controller.rb @@ -44,9 +44,9 @@ module Admin create_connected_app - jwt_service = VineJwtService.new(secret: connected_app_params[:vine_secret]) - vine_api = VineApiService.new(api_key: connected_app_params[:vine_api_key], - jwt_generator: jwt_service) + jwt_service = Vine::JwtService.new(secret: connected_app_params[:vine_secret]) + vine_api = Vine::ApiService.new(api_key: connected_app_params[:vine_api_key], + jwt_generator: jwt_service) if !@app.connect(api_key: connected_app_params[:vine_api_key], secret: connected_app_params[:vine_secret], vine_api:) diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb index c5bce3a5e1..904d0d5b05 100644 --- a/app/controllers/checkout_controller.rb +++ b/app/controllers/checkout_controller.rb @@ -79,8 +79,8 @@ class CheckoutController < BaseController return true if redirect_to_payment_gateway # Redeem VINE voucher - vine_voucher_redeemer = VineVoucherRedeemerService.new(order: @order) - if vine_voucher_redeemer.call == false + vine_voucher_redeemer = Vine::VoucherRedeemerService.new(order: @order) + if vine_voucher_redeemer.redeem == false # rubocop:disable Rails/DeprecatedActiveModelErrorsMethods flash[:error] = if vine_voucher_redeemer.errors.keys.include?(:redeeming_failed) vine_voucher_redeemer.errors[:redeeming_failed] diff --git a/app/controllers/spree/admin/payments_controller.rb b/app/controllers/spree/admin/payments_controller.rb index bd60138d70..acbeabe4db 100644 --- a/app/controllers/spree/admin/payments_controller.rb +++ b/app/controllers/spree/admin/payments_controller.rb @@ -191,8 +191,8 @@ module Spree end def redeem_vine_voucher - vine_voucher_redeemer = VineVoucherRedeemerService.new(order: @order) - if vine_voucher_redeemer.call == false + vine_voucher_redeemer = Vine::VoucherRedeemerService.new(order: @order) + if vine_voucher_redeemer.redeem == false # rubocop:disable Rails/DeprecatedActiveModelErrorsMethods flash[:error] = if vine_voucher_redeemer.errors.keys.include?(:redeeming_failed) vine_voucher_redeemer.errors[:redeeming_failed] diff --git a/app/controllers/voucher_adjustments_controller.rb b/app/controllers/voucher_adjustments_controller.rb index b8f3a3f1e4..9fa62257b2 100644 --- a/app/controllers/voucher_adjustments_controller.rb +++ b/app/controllers/voucher_adjustments_controller.rb @@ -87,7 +87,7 @@ class VoucherAdjustmentsController < BaseController end def vine_voucher - vine_voucher_validator = VineVoucherValidatorService.new( + vine_voucher_validator = Vine::VoucherValidatorService.new( voucher_code: voucher_params[:voucher_code], enterprise: @order.distributor ) voucher = vine_voucher_validator.validate diff --git a/app/services/vine/api_service.rb b/app/services/vine/api_service.rb new file mode 100644 index 0000000000..cdf1326be3 --- /dev/null +++ b/app/services/vine/api_service.rb @@ -0,0 +1,77 @@ +# frozen_string_literal: true + +require "faraday" + +module Vine + class ApiService + attr_reader :api_key, :jwt_generator + + def initialize(api_key:, jwt_generator:) + @vine_api_url = ENV.fetch("VINE_API_URL") + @api_key = api_key + @jwt_generator = jwt_generator + end + + def my_team + my_team_url = "#{@vine_api_url}/my-team" + + response = connection.get(my_team_url) + + log_error("Vine::ApiService#my_team", response) + + response + end + + def voucher_validation(voucher_short_code) + voucher_validation_url = "#{@vine_api_url}/voucher-validation" + + response = connection.post( + voucher_validation_url, + { type: "voucher_code", value: voucher_short_code }, + 'Content-Type': "application/json" + ) + + log_error("Vine::ApiService#voucher_validation", response) + + response + end + + def voucher_redemptions(voucher_id, voucher_set_id, amount) + voucher_redemptions_url = "#{@vine_api_url}/voucher-redemptions" + + response = connection.post( + voucher_redemptions_url, + { voucher_id:, voucher_set_id:, amount: amount.to_i }, + 'Content-Type': "application/json" + ) + + log_error("Vine::ApiService#voucher_redemptions", response) + + response + end + + private + + def connection + jwt = jwt_generator.generate_token + Faraday.new( + request: { timeout: 30 }, + headers: { + 'X-Authorization': "JWT #{jwt}", + Accept: "application/json" + } + ) do |f| + f.request :json + f.response :json + f.request :authorization, 'Bearer', api_key + end + end + + def log_error(prefix, response) + return if response.success? + + Rails.logger.error "#{prefix} -- response_status: #{response.status}" + Rails.logger.error "#{prefix} -- response: #{response.body}" + end + end +end diff --git a/app/services/vine/jwt_service.rb b/app/services/vine/jwt_service.rb new file mode 100644 index 0000000000..acecc4f784 --- /dev/null +++ b/app/services/vine/jwt_service.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module Vine + class JwtService + ALGORITHM = "HS256" + ISSUER = "openfoodnetwork" + + def initialize(secret: ) + @secret = secret + end + + def generate_token + generation_time = Time.zone.now + payload = { + iss: ISSUER, + iat: generation_time.to_i, + exp: (generation_time + 1.minute).to_i, + } + + JWT.encode(payload, @secret, ALGORITHM) + end + end +end diff --git a/app/services/vine/voucher_redeemer_service.rb b/app/services/vine/voucher_redeemer_service.rb new file mode 100644 index 0000000000..6e03e0d2b8 --- /dev/null +++ b/app/services/vine/voucher_redeemer_service.rb @@ -0,0 +1,66 @@ +# frozen_string_literal: true + +module Vine + class VoucherRedeemerService + attr_reader :order, :errors + + def initialize(order: ) + @order = order + @errors = {} + end + + def redeem + # Do nothing if we don't have a vine voucher added to the order + voucher_adjustment = order.voucher_adjustments.first + @voucher = voucher_adjustment&.originator + + return true if voucher_adjustment.nil? || !@voucher.vine? + + if vine_settings.nil? + errors[:vine_settings] = I18n.t("vine_voucher_redeemer_service.errors.vine_settings") + return false + end + + response = call_vine_api + + if !response.success? + handle_errors(response) + return false + end + + voucher_adjustment.close + + true + rescue Faraday::Error => e + Rails.logger.error e.inspect + Bugsnag.notify(e) + + errors[:vine_api] = I18n.t("vine_voucher_validator_service.errors.vine_api") + false + end + + private + + def vine_settings + ConnectedApps::Vine.find_by(enterprise: order.distributor)&.data + end + + def call_vine_api + jwt_service = Vine::JwtService.new(secret: vine_settings["secret"]) + vine_api = Vine::ApiService.new(api_key: vine_settings["api_key"], jwt_generator: jwt_service) + + # Voucher amount is stored in dollars, VINE expect cents + vine_api.voucher_redemptions( + @voucher.external_voucher_id, @voucher.external_voucher_set_id, (@voucher.amount * 100) + ) + end + + def handle_errors(response) + if response.status == 400 + errors[:redeeming_failed] = I18n.t("vine_voucher_redeemer_service.errors.redeeming_failed") + else + errors[:vine_api] = I18n.t("vine_voucher_redeemer_service.errors.vine_api") + end + end + end +end diff --git a/app/services/vine/voucher_validator_service.rb b/app/services/vine/voucher_validator_service.rb new file mode 100644 index 0000000000..a2d085105a --- /dev/null +++ b/app/services/vine/voucher_validator_service.rb @@ -0,0 +1,84 @@ +# frozen_string_literal: true + +module Vine + class VoucherValidatorService + attr_reader :voucher_code, :errors + + def initialize(voucher_code:, enterprise:) + @voucher_code = voucher_code + @enterprise = enterprise + @errors = {} + end + + def validate + if vine_settings.nil? + errors[:vine_settings] = I18n.t("vine_voucher_validator_service.errors.vine_settings") + return nil + end + + response = call_vine_api + + if !response.success? + handle_errors(response) + return nil + end + + save_voucher(response) + rescue Faraday::Error => e + Rails.logger.error e.inspect + Bugsnag.notify(e) + + # TODO do we need a more specific error ? + errors[:vine_api] = I18n.t("vine_voucher_validator_service.errors.vine_api") + nil + end + + private + + def vine_settings + ConnectedApps::Vine.find_by(enterprise: @enterprise)&.data + end + + def call_vine_api + # Check voucher is valid + jwt_service = Vine::JwtService.new(secret: vine_settings["secret"]) + vine_api = Vine::ApiService.new(api_key: vine_settings["api_key"], jwt_generator: jwt_service) + + vine_api.voucher_validation(voucher_code) + end + + def handle_errors(response) + if response.status == 400 + errors[:invalid_voucher] = I18n.t("vine_voucher_validator_service.errors.invalid_voucher") + elsif response.status == 404 + errors[:not_found_voucher] = + I18n.t("vine_voucher_validator_service.errors.not_found_voucher") + else + errors[:vine_api] = I18n.t("vine_voucher_validator_service.errors.vine_api") + end + end + + def save_voucher(response) + voucher_data = response.body["data"] + + # Check if voucher already exist + voucher = Voucher.vine.find_by(code: voucher_code, enterprise: @enterprise) + + amount = voucher_data["voucher_value_remaining"].to_f / 100 + if voucher.present? + voucher.update(amount: ) + else + voucher = Vouchers::FlatRate.create( + enterprise: @enterprise, + code: voucher_data["voucher_short_code"], + amount:, + external_voucher_id: voucher_data["id"], + external_voucher_set_id: voucher_data["voucher_set_id"], + voucher_type: "VINE" + ) + end + + voucher + end + end +end diff --git a/app/services/vine_api_service.rb b/app/services/vine_api_service.rb deleted file mode 100644 index 89b7b1a348..0000000000 --- a/app/services/vine_api_service.rb +++ /dev/null @@ -1,75 +0,0 @@ -# frozen_string_literal: true - -require "faraday" - -class VineApiService - attr_reader :api_key, :jwt_generator - - def initialize(api_key:, jwt_generator:) - @vine_api_url = ENV.fetch("VINE_API_URL") - @api_key = api_key - @jwt_generator = jwt_generator - end - - def my_team - my_team_url = "#{@vine_api_url}/my-team" - - response = connection.get(my_team_url) - - log_error("VineApiService#my_team", response) - - response - end - - def voucher_validation(voucher_short_code) - voucher_validation_url = "#{@vine_api_url}/voucher-validation" - - response = connection.post( - voucher_validation_url, - { type: "voucher_code", value: voucher_short_code }, - 'Content-Type': "application/json" - ) - - log_error("VineApiService#voucher_validation", response) - - response - end - - def voucher_redemptions(voucher_id, voucher_set_id, amount) - voucher_redemptions_url = "#{@vine_api_url}/voucher-redemptions" - - response = connection.post( - voucher_redemptions_url, - { voucher_id:, voucher_set_id:, amount: amount.to_i }, - 'Content-Type': "application/json" - ) - - log_error("VineApiService#voucher_redemptions", response) - - response - end - - private - - def connection - jwt = jwt_generator.generate_token - Faraday.new( - request: { timeout: 30 }, - headers: { - 'X-Authorization': "JWT #{jwt}", - Accept: "application/json" - } - ) do |f| - f.request :json - f.response :json - f.request :authorization, 'Bearer', api_key - end - end - - def log_error(prefix, response) - return if response.success? - - Rails.logger.error "#{prefix} -- response_status: #{response.status}" - Rails.logger.error "#{prefix} -- response: #{response.body}" - end -end diff --git a/app/services/vine_jwt_service.rb b/app/services/vine_jwt_service.rb deleted file mode 100644 index f65f04a7f6..0000000000 --- a/app/services/vine_jwt_service.rb +++ /dev/null @@ -1,21 +0,0 @@ -# frozen_string_literal: true - -class VineJwtService - ALGORITHM = "HS256" - ISSUER = "openfoodnetwork" - - def initialize(secret: ) - @secret = secret - end - - def generate_token - generation_time = Time.zone.now - payload = { - iss: ISSUER, - iat: generation_time.to_i, - exp: (generation_time + 1.minute).to_i, - } - - JWT.encode(payload, @secret, ALGORITHM) - end -end diff --git a/app/services/vine_voucher_redeemer_service.rb b/app/services/vine_voucher_redeemer_service.rb deleted file mode 100644 index b616d8a798..0000000000 --- a/app/services/vine_voucher_redeemer_service.rb +++ /dev/null @@ -1,64 +0,0 @@ -# frozen_string_literal: true - -class VineVoucherRedeemerService - attr_reader :order, :errors - - def initialize(order: ) - @order = order - @errors = {} - end - - def call - # Do nothing if we don't have a vine voucher added to the order - voucher_adjustment = order.voucher_adjustments.first - @voucher = voucher_adjustment&.originator - - return true if voucher_adjustment.nil? || !@voucher.vine? - - if vine_settings.nil? - errors[:vine_settings] = I18n.t("vine_voucher_redeemer_service.errors.vine_settings") - return false - end - - response = call_vine_api - - if !response.success? - handle_errors(response) - return false - end - - voucher_adjustment.close - - true - rescue Faraday::Error => e - Rails.logger.error e.inspect - Bugsnag.notify(e) - - errors[:vine_api] = I18n.t("vine_voucher_validator_service.errors.vine_api") - false - end - - private - - def vine_settings - ConnectedApps::Vine.find_by(enterprise: order.distributor)&.data - end - - def call_vine_api - jwt_service = VineJwtService.new(secret: vine_settings["secret"]) - vine_api = VineApiService.new(api_key: vine_settings["api_key"], jwt_generator: jwt_service) - - # Voucher amount is stored in dollars, VINE expect cents - vine_api.voucher_redemptions( - @voucher.external_voucher_id, @voucher.external_voucher_set_id, (@voucher.amount * 100) - ) - end - - def handle_errors(response) - if response.status == 400 - errors[:redeeming_failed] = I18n.t("vine_voucher_redeemer_service.errors.redeeming_failed") - else - errors[:vine_api] = I18n.t("vine_voucher_redeemer_service.errors.vine_api") - end - end -end diff --git a/app/services/vine_voucher_validator_service.rb b/app/services/vine_voucher_validator_service.rb deleted file mode 100644 index 038ecf8fbc..0000000000 --- a/app/services/vine_voucher_validator_service.rb +++ /dev/null @@ -1,81 +0,0 @@ -# frozen_string_literal: true - -class VineVoucherValidatorService - attr_reader :voucher_code, :errors - - def initialize(voucher_code:, enterprise:) - @voucher_code = voucher_code - @enterprise = enterprise - @errors = {} - end - - def validate - if vine_settings.nil? - errors[:vine_settings] = I18n.t("vine_voucher_validator_service.errors.vine_settings") - return nil - end - - response = call_vine_api - - if !response.success? - handle_errors(response) - return nil - end - - save_voucher(response) - rescue Faraday::Error => e - Rails.logger.error e.inspect - Bugsnag.notify(e) - - # TODO do we need a more specific error ? - errors[:vine_api] = I18n.t("vine_voucher_validator_service.errors.vine_api") - nil - end - - private - - def vine_settings - ConnectedApps::Vine.find_by(enterprise: @enterprise)&.data - end - - def call_vine_api - # Check voucher is valid - jwt_service = VineJwtService.new(secret: vine_settings["secret"]) - vine_api = VineApiService.new(api_key: vine_settings["api_key"], jwt_generator: jwt_service) - - vine_api.voucher_validation(voucher_code) - end - - def handle_errors(response) - if response.status == 400 - errors[:invalid_voucher] = I18n.t("vine_voucher_validator_service.errors.invalid_voucher") - elsif response.status == 404 - errors[:not_found_voucher] = I18n.t("vine_voucher_validator_service.errors.not_found_voucher") - else - errors[:vine_api] = I18n.t("vine_voucher_validator_service.errors.vine_api") - end - end - - def save_voucher(response) - voucher_data = response.body["data"] - - # Check if voucher already exist - voucher = Voucher.vine.find_by(code: voucher_code, enterprise: @enterprise) - - amount = voucher_data["voucher_value_remaining"].to_f / 100 - if voucher.present? - voucher.update(amount: ) - else - voucher = Vouchers::FlatRate.create( - enterprise: @enterprise, - code: voucher_data["voucher_short_code"], - amount:, - external_voucher_id: voucher_data["id"], - external_voucher_set_id: voucher_data["voucher_set_id"], - voucher_type: "VINE" - ) - end - - voucher - end -end diff --git a/spec/controllers/checkout_controller_spec.rb b/spec/controllers/checkout_controller_spec.rb index 37957c2ff9..9ae4a16b43 100644 --- a/spec/controllers/checkout_controller_spec.rb +++ b/spec/controllers/checkout_controller_spec.rb @@ -502,7 +502,7 @@ RSpec.describe CheckoutController, type: :controller do create(:voucher_flat_rate, voucher_type: "VINE", code: 'some_code', enterprise: distributor, amount: 6) } - let(:vine_voucher_redeemer) { instance_double(VineVoucherRedeemerService) } + let(:vine_voucher_redeemer) { instance_double(Vine::VoucherRedeemerService) } before do # Adding voucher to the order @@ -510,11 +510,11 @@ RSpec.describe CheckoutController, type: :controller do VoucherAdjustmentsService.new(order).update order.update_totals_and_states - allow(VineVoucherRedeemerService).to receive(:new).and_return(vine_voucher_redeemer) + allow(Vine::VoucherRedeemerService).to receive(:new).and_return(vine_voucher_redeemer) end it "completes the order and redirects to order confirmation" do - expect(vine_voucher_redeemer).to receive(:call).and_return(true) + expect(vine_voucher_redeemer).to receive(:redeem).and_return(true) put(:update, params:) @@ -524,7 +524,7 @@ RSpec.describe CheckoutController, type: :controller do context "when redeeming the voucher fails" do it "returns 422 and some error" do - allow(vine_voucher_redeemer).to receive(:call).and_return(false) + allow(vine_voucher_redeemer).to receive(:redeem).and_return(false) allow(vine_voucher_redeemer).to receive(:errors).and_return( { redeeming_failed: "Redeeming the voucher failed" } ) @@ -538,7 +538,7 @@ RSpec.describe CheckoutController, type: :controller do context "when an other error happens" do it "returns 422 and some error" do - allow(vine_voucher_redeemer).to receive(:call).and_return(false) + allow(vine_voucher_redeemer).to receive(:redeem).and_return(false) allow(vine_voucher_redeemer).to receive(:errors).and_return( { vine_api: "There was an error communicating with the API" } ) diff --git a/spec/fixtures/vcr_cassettes/VineApiService/_my_team/when_a_request_succeed/returns_the_response.yml b/spec/fixtures/vcr_cassettes/Vine_ApiService/_my_team/when_a_request_succeed/returns_the_response.yml similarity index 100% rename from spec/fixtures/vcr_cassettes/VineApiService/_my_team/when_a_request_succeed/returns_the_response.yml rename to spec/fixtures/vcr_cassettes/Vine_ApiService/_my_team/when_a_request_succeed/returns_the_response.yml diff --git a/spec/fixtures/vcr_cassettes/VineApiService/_voucher_redemptions/when_a_request_succeed/returns_the_response.yml b/spec/fixtures/vcr_cassettes/Vine_ApiService/_voucher_redemptions/when_a_request_succeed/returns_the_response.yml similarity index 100% rename from spec/fixtures/vcr_cassettes/VineApiService/_voucher_redemptions/when_a_request_succeed/returns_the_response.yml rename to spec/fixtures/vcr_cassettes/Vine_ApiService/_voucher_redemptions/when_a_request_succeed/returns_the_response.yml diff --git a/spec/fixtures/vcr_cassettes/VineApiService/_voucher_validation/when_a_request_succeed/returns_the_response.yml b/spec/fixtures/vcr_cassettes/Vine_ApiService/_voucher_validation/when_a_request_succeed/returns_the_response.yml similarity index 100% rename from spec/fixtures/vcr_cassettes/VineApiService/_voucher_validation/when_a_request_succeed/returns_the_response.yml rename to spec/fixtures/vcr_cassettes/Vine_ApiService/_voucher_validation/when_a_request_succeed/returns_the_response.yml diff --git a/spec/models/connected_apps/vine_spec.rb b/spec/models/connected_apps/vine_spec.rb index 8b37afb0f5..c3a2e8cf44 100644 --- a/spec/models/connected_apps/vine_spec.rb +++ b/spec/models/connected_apps/vine_spec.rb @@ -7,7 +7,7 @@ RSpec.describe ConnectedApps::Vine do let(:vine_api_key) { "12345" } let(:secret) { "my_secret" } - let(:vine_api) { instance_double(VineApiService) } + let(:vine_api) { instance_double(Vine::ApiService) } describe "#connect" do it "send a request to VINE api" do diff --git a/spec/requests/admin/connected_apps_controller_spec.rb b/spec/requests/admin/connected_apps_controller_spec.rb index 05b7eefa0c..a39eaa52af 100644 --- a/spec/requests/admin/connected_apps_controller_spec.rb +++ b/spec/requests/admin/connected_apps_controller_spec.rb @@ -13,11 +13,11 @@ RSpec.describe "Admin ConnectedApp" do describe "POST /admin/enterprises/:enterprise_id/connected_apps" do context "with type ConnectedApps::Vine" do - let(:vine_api) { instance_double(VineApiService) } + let(:vine_api) { instance_double(Vine::ApiService) } before do - allow(VineJwtService).to receive(:new).and_return(instance_double(VineJwtService)) - allow(VineApiService).to receive(:new).and_return(vine_api) + allow(Vine::JwtService).to receive(:new).and_return(instance_double(Vine::JwtService)) + allow(Vine::ApiService).to receive(:new).and_return(vine_api) end it "creates a new connected app" do @@ -115,7 +115,7 @@ RSpec.describe "Admin ConnectedApp" do before do allow(ENV).to receive(:fetch).and_call_original allow(ENV).to receive(:fetch).with("VINE_API_URL").and_raise(KeyError) - allow(VineApiService).to receive(:new).and_call_original + allow(Vine::ApiService).to receive(:new).and_call_original end it "redirects to enterprise edit page, with an error" do diff --git a/spec/requests/spree/admin/payments_spec.rb b/spec/requests/spree/admin/payments_spec.rb index aeeef365c2..6fb898f520 100644 --- a/spec/requests/spree/admin/payments_spec.rb +++ b/spec/requests/spree/admin/payments_spec.rb @@ -76,16 +76,16 @@ RSpec.describe Spree::Admin::PaymentsController, type: :request do create(:voucher_flat_rate, voucher_type: "VINE", code: 'some_code', enterprise: order.distributor, amount: 6) } - let(:vine_voucher_redeemer) { instance_double(VineVoucherRedeemerService) } + let(:vine_voucher_redeemer) { instance_double(Vine::VoucherRedeemerService) } before do add_voucher_to_order(vine_voucher, order) - allow(VineVoucherRedeemerService).to receive(:new).and_return(vine_voucher_redeemer) + allow(Vine::VoucherRedeemerService).to receive(:new).and_return(vine_voucher_redeemer) end it "completes the order and redirects to payment page" do - expect(vine_voucher_redeemer).to receive(:call).and_return(true) + expect(vine_voucher_redeemer).to receive(:redeem).and_return(true) post("/admin/orders/#{order.number}/payments.json", params:) @@ -97,7 +97,7 @@ RSpec.describe Spree::Admin::PaymentsController, type: :request do context "when redeeming the voucher fails" do it "redirect to payments page" do - allow(vine_voucher_redeemer).to receive(:call).and_return(false) + allow(vine_voucher_redeemer).to receive(:redeem).and_return(false) allow(vine_voucher_redeemer).to receive(:errors).and_return( { redeeming_failed: "Redeeming the voucher failed" } ) @@ -111,7 +111,7 @@ RSpec.describe Spree::Admin::PaymentsController, type: :request do context "when an other error happens" do it "redirect to payments page" do - allow(vine_voucher_redeemer).to receive(:call).and_return(false) + allow(vine_voucher_redeemer).to receive(:redeem).and_return(false) allow(vine_voucher_redeemer).to receive(:errors).and_return( { vine_api: "There was an error communicating with the API" } ) @@ -267,16 +267,16 @@ RSpec.describe Spree::Admin::PaymentsController, type: :request do create(:voucher_flat_rate, voucher_type: "VINE", code: 'some_code', enterprise: order.distributor, amount: 6) } - let(:vine_voucher_redeemer) { instance_double(VineVoucherRedeemerService) } + let(:vine_voucher_redeemer) { instance_double(Vine::VoucherRedeemerService) } before do add_voucher_to_order(vine_voucher, order) - allow(VineVoucherRedeemerService).to receive(:new).and_return(vine_voucher_redeemer) + allow(Vine::VoucherRedeemerService).to receive(:new).and_return(vine_voucher_redeemer) end it "completes the order and redirects to payment page" do - expect(vine_voucher_redeemer).to receive(:call).and_return(true) + expect(vine_voucher_redeemer).to receive(:redeem).and_return(true) put( "/admin/orders/#{order.number}/payments/#{order.payments.first.id}/" \ @@ -293,7 +293,7 @@ RSpec.describe Spree::Admin::PaymentsController, type: :request do context "when redeeming the voucher fails" do it "redirect to payments page" do - allow(vine_voucher_redeemer).to receive(:call).and_return(false) + allow(vine_voucher_redeemer).to receive(:redeem).and_return(false) allow(vine_voucher_redeemer).to receive(:errors).and_return( { redeeming_failed: "Redeeming the voucher failed" } ) @@ -312,7 +312,7 @@ RSpec.describe Spree::Admin::PaymentsController, type: :request do context "when an other error happens" do it "redirect to payments page" do - allow(vine_voucher_redeemer).to receive(:call).and_return(false) + allow(vine_voucher_redeemer).to receive(:redeem).and_return(false) allow(vine_voucher_redeemer).to receive(:errors).and_return( { vine_api: "There was an error communicating with the API" } ) diff --git a/spec/requests/voucher_adjustments_spec.rb b/spec/requests/voucher_adjustments_spec.rb index 158911dd68..ae8dc8e4bf 100644 --- a/spec/requests/voucher_adjustments_spec.rb +++ b/spec/requests/voucher_adjustments_spec.rb @@ -87,10 +87,10 @@ RSpec.describe VoucherAdjustmentsController, type: :request do end context "with a VINE voucher", feature: :connected_apps do - let(:vine_voucher_validator) { instance_double(VineVoucherValidatorService) } + let(:vine_voucher_validator) { instance_double(Vine::VoucherValidatorService) } before do - allow(VineVoucherValidatorService).to receive(:new).and_return(vine_voucher_validator) + allow(Vine::VoucherValidatorService).to receive(:new).and_return(vine_voucher_validator) end context "with a new voucher" do diff --git a/spec/services/vine_api_service_spec.rb b/spec/services/vine/api_service_spec.rb similarity index 96% rename from spec/services/vine_api_service_spec.rb rename to spec/services/vine/api_service_spec.rb index 1a9f9977d4..33292ade65 100644 --- a/spec/services/vine_api_service_spec.rb +++ b/spec/services/vine/api_service_spec.rb @@ -2,12 +2,12 @@ require "spec_helper" -RSpec.describe VineApiService do +RSpec.describe Vine::ApiService do subject(:vine_api) { described_class.new(api_key: vine_api_key, jwt_generator: jwt_service) } let(:vine_api_url) { "https://vine-staging.openfoodnetwork.org.au/api/v1" } let(:vine_api_key) { "12345" } - let(:jwt_service) { VineJwtService.new(secret:) } + let(:jwt_service) { Vine::JwtService.new(secret:) } let(:secret) { "my_secret" } let(:token) { "some.jwt.token" } @@ -59,7 +59,7 @@ RSpec.describe VineApiService do it "logs the error" do stub_request(:get, my_team_url).to_return(body: "error", status: 401) - expect(Rails.logger).to receive(:error).with(match("VineApiService#my_team")).twice + expect(Rails.logger).to receive(:error).with(match("Vine::ApiService#my_team")).twice response = vine_api.my_team @@ -124,7 +124,7 @@ RSpec.describe VineApiService do stub_request(:post, voucher_validation_url).to_return(body: "error", status: 401) expect(Rails.logger).to receive(:error).with( - match("VineApiService#voucher_validation") + match("Vine::ApiService#voucher_validation") ).twice response = vine_api.voucher_validation(voucher_short_code) @@ -192,7 +192,7 @@ RSpec.describe VineApiService do stub_request(:post, voucher_redemptions_url).to_return(body: "error", status: 401) expect(Rails.logger).to receive(:error).with( - match("VineApiService#voucher_redemptions") + match("Vine::ApiService#voucher_redemptions") ).twice response = vine_api.voucher_redemptions(voucher_id, voucher_set_id, amount) diff --git a/spec/services/vine_jwt_service_spec.rb b/spec/services/vine/jwt_service_spec.rb similarity index 97% rename from spec/services/vine_jwt_service_spec.rb rename to spec/services/vine/jwt_service_spec.rb index 204dede96c..1a964b3666 100644 --- a/spec/services/vine_jwt_service_spec.rb +++ b/spec/services/vine/jwt_service_spec.rb @@ -2,7 +2,7 @@ require "spec_helper" -RSpec.describe VineJwtService do +RSpec.describe Vine::JwtService do describe "#generate_token" do subject { described_class.new(secret: vine_secret) } let(:vine_secret) { "some_secret" } diff --git a/spec/services/vine_voucher_redeemer_service_spec.rb b/spec/services/vine/voucher_redeemer_service_spec.rb similarity index 84% rename from spec/services/vine_voucher_redeemer_service_spec.rb rename to spec/services/vine/voucher_redeemer_service_spec.rb index 10d04d7f03..5dd83da12b 100644 --- a/spec/services/vine_voucher_redeemer_service_spec.rb +++ b/spec/services/vine/voucher_redeemer_service_spec.rb @@ -2,7 +2,7 @@ require "spec_helper" -RSpec.describe VineVoucherRedeemerService, feature: :connected_apps do +RSpec.describe Vine::VoucherRedeemerService, feature: :connected_apps do subject(:voucher_redeemer_service) { described_class.new(order: ) } let(:user) { order.user } @@ -17,13 +17,13 @@ RSpec.describe VineVoucherRedeemerService, feature: :connected_apps do } let(:voucher_id) { "9d316d27-0dad-411a-8953-316a1aaf7742" } let(:voucher_set_id) { "9d314daa-0878-4b73-922d-698047640cf4" } - let(:vine_api_service) { instance_double(VineApiService) } + let(:vine_api_service) { instance_double(Vine::ApiService) } before do - allow(VineApiService).to receive(:new).and_return(vine_api_service) + allow(Vine::ApiService).to receive(:new).and_return(vine_api_service) end - describe "#call" do + describe "#redeem" do context "with a valid voucher" do let!(:vine_connected_app) { ConnectedApps::Vine.create( @@ -60,7 +60,7 @@ RSpec.describe VineVoucherRedeemerService, feature: :connected_apps do .with(voucher_id, voucher_set_id, 600) .and_return(mock_api_response(success: true, data:)) - voucher_redeemer_service.call + voucher_redeemer_service.redeem end it "closes the linked assement" do @@ -68,7 +68,7 @@ RSpec.describe VineVoucherRedeemerService, feature: :connected_apps do .and_return(mock_api_response(success: true, data:)) expect { - voucher_redeemer_service.call + voucher_redeemer_service.redeem }.to change { order.voucher_adjustments.first.state }.to("closed") end @@ -76,7 +76,7 @@ RSpec.describe VineVoucherRedeemerService, feature: :connected_apps do allow(vine_api_service).to receive(:voucher_redemptions) .and_return(mock_api_response(success: true, data:)) - expect(voucher_redeemer_service.call).to be(true) + expect(voucher_redeemer_service.redeem).to be(true) end context "when redeeming fails" do @@ -93,16 +93,16 @@ RSpec.describe VineVoucherRedeemerService, feature: :connected_apps do it "doesn't close the linked assement" do expect { - voucher_redeemer_service.call + voucher_redeemer_service.redeem }.not_to change { order.voucher_adjustments.first.state } end it "returns false" do - expect(voucher_redeemer_service.call).to be(false) + expect(voucher_redeemer_service.redeem).to be(false) end it "adds an error message" do - voucher_redeemer_service.call + voucher_redeemer_service.redeem expect(voucher_redeemer_service.errors).to include( { redeeming_failed: "Redeeming the voucher failed" } @@ -115,17 +115,17 @@ RSpec.describe VineVoucherRedeemerService, feature: :connected_apps do before { add_voucher(vine_voucher) } it "returns false" do - expect(voucher_redeemer_service.call).to be(false) + expect(voucher_redeemer_service.redeem).to be(false) end - it "doesn't call the VINE API" do + it "doesn't redeem the VINE API" do expect(vine_api_service).not_to receive(:voucher_redemptions) - voucher_redeemer_service.call + voucher_redeemer_service.redeem end it "adds an error message" do - voucher_redeemer_service.call + voucher_redeemer_service.redeem expect(voucher_redeemer_service.errors).to include( { vine_settings: "No Vine api settings for the given enterprise" } @@ -134,7 +134,7 @@ RSpec.describe VineVoucherRedeemerService, feature: :connected_apps do it "doesn't close the linked assement" do expect { - voucher_redeemer_service.call + voucher_redeemer_service.redeem }.not_to change { order.voucher_adjustments.first.state } end end @@ -148,13 +148,13 @@ RSpec.describe VineVoucherRedeemerService, feature: :connected_apps do } it "returns true" do - expect(voucher_redeemer_service.call).to be(true) + expect(voucher_redeemer_service.redeem).to be(true) end - it "doesn't call the VINE API" do + it "doesn't redeem the VINE API" do expect(vine_api_service).not_to receive(:voucher_redemptions) - voucher_redeemer_service.call + voucher_redeemer_service.redeem end end @@ -169,13 +169,13 @@ RSpec.describe VineVoucherRedeemerService, feature: :connected_apps do before { add_voucher(voucher) } it "returns true" do - expect(voucher_redeemer_service.call).to be(true) + expect(voucher_redeemer_service.redeem).to be(true) end - it "doesn't call the VINE API" do + it "doesn't redeem the VINE API" do expect(vine_api_service).not_to receive(:voucher_redemptions) - voucher_redeemer_service.call + voucher_redeemer_service.redeem end end @@ -192,11 +192,11 @@ RSpec.describe VineVoucherRedeemerService, feature: :connected_apps do end it "returns false" do - expect(voucher_redeemer_service.call).to be(false) + expect(voucher_redeemer_service.redeem).to be(false) end it "adds an error message" do - voucher_redeemer_service.call + voucher_redeemer_service.redeem expect(voucher_redeemer_service.errors).to include( { vine_api: "There was an error communicating with the API" } @@ -205,7 +205,7 @@ RSpec.describe VineVoucherRedeemerService, feature: :connected_apps do it "doesn't close the linked assement" do expect { - voucher_redeemer_service.call + voucher_redeemer_service.redeem }.not_to change { order.voucher_adjustments.first.state } end @@ -213,7 +213,7 @@ RSpec.describe VineVoucherRedeemerService, feature: :connected_apps do expect(Rails.logger).to receive(:error) expect(Bugsnag).to receive(:notify) - voucher_redeemer_service.call + voucher_redeemer_service.redeem end end @@ -240,11 +240,11 @@ RSpec.describe VineVoucherRedeemerService, feature: :connected_apps do end it "returns false" do - expect(voucher_redeemer_service.call).to be(false) + expect(voucher_redeemer_service.redeem).to be(false) end it "adds an error message" do - voucher_redeemer_service.call + voucher_redeemer_service.redeem expect(voucher_redeemer_service.errors).to include( { vine_api: "There was an error communicating with the API" } @@ -253,7 +253,7 @@ RSpec.describe VineVoucherRedeemerService, feature: :connected_apps do it "doesn't close the linked assement" do expect { - voucher_redeemer_service.call + voucher_redeemer_service.redeem }.not_to change { order.voucher_adjustments.first.state } end end diff --git a/spec/services/vine_voucher_validator_service_spec.rb b/spec/services/vine/voucher_validator_service_spec.rb similarity index 98% rename from spec/services/vine_voucher_validator_service_spec.rb rename to spec/services/vine/voucher_validator_service_spec.rb index 2576713b37..a3175e24e9 100644 --- a/spec/services/vine_voucher_validator_service_spec.rb +++ b/spec/services/vine/voucher_validator_service_spec.rb @@ -2,15 +2,15 @@ require "spec_helper" -RSpec.describe VineVoucherValidatorService, feature: :connected_apps do +RSpec.describe Vine::VoucherValidatorService, feature: :connected_apps do subject(:validate_voucher_service) { described_class.new(voucher_code:, enterprise: distributor) } let(:voucher_code) { "good_code" } let(:distributor) { create(:distributor_enterprise) } - let(:vine_api_service) { instance_double(VineApiService) } + let(:vine_api_service) { instance_double(Vine::ApiService) } before do - allow(VineApiService).to receive(:new).and_return(vine_api_service) + allow(Vine::ApiService).to receive(:new).and_return(vine_api_service) end describe "#validate" do From 12cf62c2ff19035cf8d645965b22980145a115e7 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Tue, 29 Oct 2024 11:31:14 +1100 Subject: [PATCH 18/33] Refactor, add OrderManagement::Order::Updater#update_voucher Move the logic to update a voucher and associated order to `OrderManagement` --- app/controllers/spree/orders_controller.rb | 3 +-- .../voucher_adjustments_controller.rb | 3 +-- .../order_management/order/updater.rb | 5 +++++ .../order_management/order/updater_spec.rb | 20 +++++++++++++++++++ spec/controllers/checkout_controller_spec.rb | 3 +-- .../reports/sales_tax_totals_by_order_spec.rb | 3 +-- spec/requests/spree/admin/payments_spec.rb | 3 +-- .../vine/voucher_redeemer_service_spec.rb | 3 +-- .../admin/reports/revenues_by_hub_spec.rb | 4 +--- spec/system/consumer/checkout/payment_spec.rb | 3 +-- spec/system/consumer/checkout/summary_spec.rb | 3 +-- 11 files changed, 34 insertions(+), 19 deletions(-) diff --git a/app/controllers/spree/orders_controller.rb b/app/controllers/spree/orders_controller.rb index af0738b17e..a7a25700a2 100644 --- a/app/controllers/spree/orders_controller.rb +++ b/app/controllers/spree/orders_controller.rb @@ -70,8 +70,7 @@ module Spree @order.recreate_all_fees! # Enterprise fees on line items and on the order itself # Re apply the voucher - VoucherAdjustmentsService.new(@order).update - @order.update_totals_and_states + OrderManagement::Order::Updater.new(@order).update_voucher if @order.complete? @order.update_payment_fees! diff --git a/app/controllers/voucher_adjustments_controller.rb b/app/controllers/voucher_adjustments_controller.rb index 9fa62257b2..40001a7dd2 100644 --- a/app/controllers/voucher_adjustments_controller.rb +++ b/app/controllers/voucher_adjustments_controller.rb @@ -72,8 +72,7 @@ class VoucherAdjustmentsController < BaseController # calculate_voucher_adjustment clear_payments - VoucherAdjustmentsService.new(@order).update - @order.update_totals_and_states + OrderManagement::Order::Updater.new(@order).update_voucher true end diff --git a/engines/order_management/app/services/order_management/order/updater.rb b/engines/order_management/app/services/order_management/order/updater.rb index c31e2b929d..5fb1d59461 100644 --- a/engines/order_management/app/services/order_management/order/updater.rb +++ b/engines/order_management/app/services/order_management/order/updater.rb @@ -161,6 +161,11 @@ module OrderManagement persist_totals end + def update_voucher + VoucherAdjustmentsService.new(order).update + update_totals_and_states + end + private def cancel_payments_requiring_auth diff --git a/engines/order_management/spec/services/order_management/order/updater_spec.rb b/engines/order_management/spec/services/order_management/order/updater_spec.rb index 458bb785ce..17f4c7dc55 100644 --- a/engines/order_management/spec/services/order_management/order/updater_spec.rb +++ b/engines/order_management/spec/services/order_management/order/updater_spec.rb @@ -460,6 +460,26 @@ module OrderManagement end end + describe "#update_voucher" do + let(:voucher_service) { instance_double(VoucherAdjustmentsService) } + + it "calls VoucherAdjustmentsService" do + expect(VoucherAdjustmentsService).to receive(:new).and_return(voucher_service) + expect(voucher_service).to receive(:update) + + updater.update_voucher + end + + it "calls update_totals_and_states" do + allow(VoucherAdjustmentsService).to receive(:new).and_return(voucher_service) + allow(voucher_service).to receive(:update) + + expect(updater).to receive(:update_totals_and_states) + + updater.update_voucher + end + end + def update_order_quantity(order) order.line_items.first.update_attribute(:quantity, 2) end diff --git a/spec/controllers/checkout_controller_spec.rb b/spec/controllers/checkout_controller_spec.rb index 9ae4a16b43..2b29e58250 100644 --- a/spec/controllers/checkout_controller_spec.rb +++ b/spec/controllers/checkout_controller_spec.rb @@ -507,8 +507,7 @@ RSpec.describe CheckoutController, type: :controller do before do # Adding voucher to the order vine_voucher.create_adjustment(vine_voucher.code, order) - VoucherAdjustmentsService.new(order).update - order.update_totals_and_states + OrderManagement::Order::Updater.new(order).update_voucher allow(Vine::VoucherRedeemerService).to receive(:new).and_return(vine_voucher_redeemer) end diff --git a/spec/lib/reports/sales_tax_totals_by_order_spec.rb b/spec/lib/reports/sales_tax_totals_by_order_spec.rb index 603331788a..e8003da3a9 100644 --- a/spec/lib/reports/sales_tax_totals_by_order_spec.rb +++ b/spec/lib/reports/sales_tax_totals_by_order_spec.rb @@ -272,8 +272,7 @@ RSpec.describe "Reporting::Reports::SalesTax::SalesTaxTotalsByOrder" do def add_voucher(order, voucher) # Add voucher to the order voucher.create_adjustment(voucher.code, order) - VoucherAdjustmentsService.new(order).update - order.update_totals_and_states + OrderManagement::Order::Updater.new(order).update_voucher Orders::WorkflowService.new(order).complete! end diff --git a/spec/requests/spree/admin/payments_spec.rb b/spec/requests/spree/admin/payments_spec.rb index 6fb898f520..195611995a 100644 --- a/spec/requests/spree/admin/payments_spec.rb +++ b/spec/requests/spree/admin/payments_spec.rb @@ -366,7 +366,6 @@ RSpec.describe Spree::Admin::PaymentsController, type: :request do def add_voucher_to_order(voucher, order) voucher.create_adjustment(voucher.code, order) - VoucherAdjustmentsService.new(order).update - order.update_totals_and_states + OrderManagement::Order::Updater.new(order).update_voucher end end diff --git a/spec/services/vine/voucher_redeemer_service_spec.rb b/spec/services/vine/voucher_redeemer_service_spec.rb index 5dd83da12b..fbfc4c3df1 100644 --- a/spec/services/vine/voucher_redeemer_service_spec.rb +++ b/spec/services/vine/voucher_redeemer_service_spec.rb @@ -261,8 +261,7 @@ RSpec.describe Vine::VoucherRedeemerService, feature: :connected_apps do def add_voucher(voucher) voucher.create_adjustment(voucher.code, order) - VoucherAdjustmentsService.new(order).update - order.update_totals_and_states + OrderManagement::Order::Updater.new(order).update_voucher end def mock_api_response(success:, data: nil, status: 200) diff --git a/spec/system/admin/reports/revenues_by_hub_spec.rb b/spec/system/admin/reports/revenues_by_hub_spec.rb index 114ec2e411..079a94d2fa 100644 --- a/spec/system/admin/reports/revenues_by_hub_spec.rb +++ b/spec/system/admin/reports/revenues_by_hub_spec.rb @@ -154,8 +154,6 @@ RSpec.describe "Revenues By Hub Reports" do order.update_shipping_fees! order.update_order! - VoucherAdjustmentsService.new(order).update - - order.update_totals_and_states + OrderManagement::Order::Updater.new(order).update_voucher end end diff --git a/spec/system/consumer/checkout/payment_spec.rb b/spec/system/consumer/checkout/payment_spec.rb index 5c7c7ba1e7..d53e9c8908 100644 --- a/spec/system/consumer/checkout/payment_spec.rb +++ b/spec/system/consumer/checkout/payment_spec.rb @@ -383,7 +383,6 @@ RSpec.describe "As a consumer, I want to checkout my order" do def add_voucher_to_order(voucher, order) voucher.create_adjustment(voucher.code, order) - VoucherAdjustmentsService.new(order).update - order.update_totals_and_states + OrderManagement::Order::Updater.new(order).update_voucher end end diff --git a/spec/system/consumer/checkout/summary_spec.rb b/spec/system/consumer/checkout/summary_spec.rb index 313c133947..bd9192d4ad 100644 --- a/spec/system/consumer/checkout/summary_spec.rb +++ b/spec/system/consumer/checkout/summary_spec.rb @@ -479,8 +479,7 @@ RSpec.describe "As a consumer, I want to checkout my order" do def add_voucher_to_order(voucher, order) voucher.create_adjustment(voucher.code, order) - VoucherAdjustmentsService.new(order).update - order.update_totals_and_states + OrderManagement::Order::Updater.new(order).update_voucher end def set_up_order(balance, state) From d4d995851fc70bce66ae85231c4b8bd771c52a4c Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Tue, 29 Oct 2024 13:58:24 +1100 Subject: [PATCH 19/33] Display voucher section if connected to VINE --- app/views/checkout/_payment.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/checkout/_payment.html.haml b/app/views/checkout/_payment.html.haml index 516d2025bb..bb4e3f1ad3 100644 --- a/app/views/checkout/_payment.html.haml +++ b/app/views/checkout/_payment.html.haml @@ -1,5 +1,5 @@ .medium-6#checkout-payment-methods - - if @order.distributor.vouchers.present? + - if @order.distributor.vouchers.present? || @order.distributor.connected_apps.vine.present? %div.checkout-substep = render partial: "checkout/voucher_section", locals: { order: @order, voucher_adjustment: @order.voucher_adjustments.first } From 7726c7d1293cb5a987b46d4917666c9ca7c66cf4 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 4 Nov 2024 14:45:21 +1100 Subject: [PATCH 20/33] Per review, rename not_vine scope to local - use IS DISTINCT FROM instead of two conditions - added spec for scope --- .../voucher_adjustments_controller.rb | 4 +-- app/models/voucher.rb | 2 +- .../enterprises/form/_vouchers.html.haml | 4 +-- spec/models/voucher_spec.rb | 25 +++++++++++++++++++ spec/requests/voucher_adjustments_spec.rb | 2 +- 5 files changed, 31 insertions(+), 6 deletions(-) diff --git a/app/controllers/voucher_adjustments_controller.rb b/app/controllers/voucher_adjustments_controller.rb index 40001a7dd2..8f85798c2f 100644 --- a/app/controllers/voucher_adjustments_controller.rb +++ b/app/controllers/voucher_adjustments_controller.rb @@ -78,8 +78,8 @@ class VoucherAdjustmentsController < BaseController end def load_voucher - voucher = Voucher.not_vine.find_by(code: voucher_params[:voucher_code], - enterprise: @order.distributor) + voucher = Voucher.local.find_by(code: voucher_params[:voucher_code], + enterprise: @order.distributor) return voucher unless voucher.nil? vine_voucher diff --git a/app/models/voucher.rb b/app/models/voucher.rb index 52ad6f2ef7..b8ff92a81d 100644 --- a/app/models/voucher.rb +++ b/app/models/voucher.rb @@ -21,7 +21,7 @@ class Voucher < ApplicationRecord TYPES = ["Vouchers::FlatRate", "Vouchers::PercentageRate"].freeze scope :vine, -> { where(voucher_type: VINE_TYPE) } - scope :not_vine, -> { where.not(voucher_type: VINE_TYPE).or(where(voucher_type: nil)) } + scope :local, -> { where("voucher_type IS DISTINCT FROM ?", VINE_TYPE) } def code=(value) super(value.to_s.strip) diff --git a/app/views/admin/enterprises/form/_vouchers.html.haml b/app/views/admin/enterprises/form/_vouchers.html.haml index 7f8f343671..042d1e7470 100644 --- a/app/views/admin/enterprises/form/_vouchers.html.haml +++ b/app/views/admin/enterprises/form/_vouchers.html.haml @@ -3,7 +3,7 @@ = t('.add_new') %br -- if @enterprise.vouchers.not_vine.with_deleted.present? +- if @enterprise.vouchers.local.with_deleted.present? %table %thead %tr @@ -17,7 +17,7 @@ /%th= t('.customers') /%th= t('.net_value') %tbody - - @enterprise.vouchers.not_vine.with_deleted.order(deleted_at: :desc, code: :asc).each do |voucher| + - @enterprise.vouchers.local.with_deleted.order(deleted_at: :desc, code: :asc).each do |voucher| %tr %td= voucher.code %td= voucher.display_value diff --git a/spec/models/voucher_spec.rb b/spec/models/voucher_spec.rb index 6af5c6b5bd..c0043f42eb 100644 --- a/spec/models/voucher_spec.rb +++ b/spec/models/voucher_spec.rb @@ -15,6 +15,31 @@ RSpec.describe Voucher do it { is_expected.to have_many(:adjustments).dependent(nil) } end + describe "scope" do + let!(:vine_voucher) { create(:vine_voucher) } + + describe ".vine" do + it "returns only vine vouchers" do + create(:voucher) + expect(Voucher.vine.count).to eq(1) + end + end + + describe ".local" do + it "returns only local vouchers" do + create(:voucher) + expect(Voucher.local.count).to eq(1) + end + + context "with voucher type other than VINE" do + it "returns only local vouchers" do + create(:voucher, voucher_type: "other") + expect(Voucher.local.count).to eq(1) + end + end + end + end + describe '#code=' do it "removes leading and trailing whitespace" do voucher = build(:voucher, code: "\r\n\t new_code \r\n\t") diff --git a/spec/requests/voucher_adjustments_spec.rb b/spec/requests/voucher_adjustments_spec.rb index ae8dc8e4bf..cdfdebb899 100644 --- a/spec/requests/voucher_adjustments_spec.rb +++ b/spec/requests/voucher_adjustments_spec.rb @@ -56,7 +56,7 @@ RSpec.describe VoucherAdjustmentsController, type: :request do # Create a non valid adjustment bad_adjustment = build(:adjustment, label: nil) allow(voucher).to receive(:create_adjustment).and_return(bad_adjustment) - allow(Voucher).to receive_message_chain(:not_vine, :find_by).and_return(voucher) + allow(Voucher).to receive_message_chain(:local, :find_by).and_return(voucher) post("/voucher_adjustments", params:) From b42cba8c37ea88445d2dd8d07773c4871fa5f807 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 4 Nov 2024 14:47:00 +1100 Subject: [PATCH 21/33] Add vine_voucher factory --- spec/controllers/checkout_controller_spec.rb | 3 +-- spec/factories/voucher_factory.rb | 6 ++++++ spec/requests/spree/admin/payments_spec.rb | 6 ++---- spec/services/vine/voucher_redeemer_service_spec.rb | 6 +++--- spec/services/vine/voucher_validator_service_spec.rb | 3 +-- spec/system/consumer/checkout/summary_spec.rb | 3 +-- 6 files changed, 14 insertions(+), 13 deletions(-) diff --git a/spec/controllers/checkout_controller_spec.rb b/spec/controllers/checkout_controller_spec.rb index 2b29e58250..71d3a3d91c 100644 --- a/spec/controllers/checkout_controller_spec.rb +++ b/spec/controllers/checkout_controller_spec.rb @@ -499,8 +499,7 @@ RSpec.describe CheckoutController, type: :controller do context "with a VINE voucher", feature: :connected_apps do let(:vine_voucher) { - create(:voucher_flat_rate, voucher_type: "VINE", code: 'some_code', - enterprise: distributor, amount: 6) + create(:vine_voucher, code: 'some_code', enterprise: distributor, amount: 6) } let(:vine_voucher_redeemer) { instance_double(Vine::VoucherRedeemerService) } diff --git a/spec/factories/voucher_factory.rb b/spec/factories/voucher_factory.rb index 821728b09f..034fae6b77 100644 --- a/spec/factories/voucher_factory.rb +++ b/spec/factories/voucher_factory.rb @@ -14,4 +14,10 @@ FactoryBot.define do factory :voucher_percentage_rate, parent: :voucher, class: Vouchers::PercentageRate do amount { rand(1..100) } end + + factory :vine_voucher, parent: :voucher_flat_rate do + voucher_type { Voucher::VINE_TYPE } + external_voucher_id { SecureRandom.uuid } + external_voucher_set_id { SecureRandom.uuid } + end end diff --git a/spec/requests/spree/admin/payments_spec.rb b/spec/requests/spree/admin/payments_spec.rb index 195611995a..7992064410 100644 --- a/spec/requests/spree/admin/payments_spec.rb +++ b/spec/requests/spree/admin/payments_spec.rb @@ -73,8 +73,7 @@ RSpec.describe Spree::Admin::PaymentsController, type: :request do context "with a VINE voucher", feature: :connected_apps do let(:vine_voucher) { - create(:voucher_flat_rate, voucher_type: "VINE", code: 'some_code', - enterprise: order.distributor, amount: 6) + create(:vine_voucher, code: 'some_code', enterprise: order.distributor, amount: 6) } let(:vine_voucher_redeemer) { instance_double(Vine::VoucherRedeemerService) } @@ -264,8 +263,7 @@ RSpec.describe Spree::Admin::PaymentsController, type: :request do context "with a VINE voucher", feature: :connected_apps do let(:vine_voucher) { - create(:voucher_flat_rate, voucher_type: "VINE", code: 'some_code', - enterprise: order.distributor, amount: 6) + create(:vine_voucher, code: 'some_code', enterprise: order.distributor, amount: 6) } let(:vine_voucher_redeemer) { instance_double(Vine::VoucherRedeemerService) } diff --git a/spec/services/vine/voucher_redeemer_service_spec.rb b/spec/services/vine/voucher_redeemer_service_spec.rb index fbfc4c3df1..50c73f2a1c 100644 --- a/spec/services/vine/voucher_redeemer_service_spec.rb +++ b/spec/services/vine/voucher_redeemer_service_spec.rb @@ -11,9 +11,9 @@ RSpec.describe Vine::VoucherRedeemerService, feature: :connected_apps do let(:order) { create(:order_with_line_items, line_items_count: 1, distributor:, order_cycle:) } let(:vine_voucher) { - create(:voucher_flat_rate, voucher_type: "VINE", code: 'some_code', enterprise: distributor, - amount: 6, external_voucher_id: voucher_id, - external_voucher_set_id: voucher_set_id ) + create(:vine_voucher, code: 'some_code', enterprise: distributor, + amount: 6, external_voucher_id: voucher_id, + external_voucher_set_id: voucher_set_id ) } let(:voucher_id) { "9d316d27-0dad-411a-8953-316a1aaf7742" } let(:voucher_set_id) { "9d314daa-0878-4b73-922d-698047640cf4" } diff --git a/spec/services/vine/voucher_validator_service_spec.rb b/spec/services/vine/voucher_validator_service_spec.rb index a3175e24e9..5e46df2b2e 100644 --- a/spec/services/vine/voucher_validator_service_spec.rb +++ b/spec/services/vine/voucher_validator_service_spec.rb @@ -275,8 +275,7 @@ RSpec.describe Vine::VoucherValidatorService, feature: :connected_apps do ) } let!(:voucher) { - create(:voucher_flat_rate, enterprise: distributor, code: voucher_code, - amount: 500, voucher_type: "VINE" ) + create(:vine_voucher, enterprise: distributor, code: voucher_code, amount: 500) } let(:data) { diff --git a/spec/system/consumer/checkout/summary_spec.rb b/spec/system/consumer/checkout/summary_spec.rb index bd9192d4ad..4ad66f422e 100644 --- a/spec/system/consumer/checkout/summary_spec.rb +++ b/spec/system/consumer/checkout/summary_spec.rb @@ -352,8 +352,7 @@ RSpec.describe "As a consumer, I want to checkout my order" do ) } let(:vine_voucher) { - create(:voucher_flat_rate, voucher_type: "VINE", code: 'some_vine_code', - enterprise: distributor, amount: 0.01) + create(:vine_voucher, code: 'some_vine_code', enterprise: distributor, amount: 0.01) } before do From d7313ffec954e39bbe6c053ede7337a1b61ae9b2 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 4 Nov 2024 15:42:24 +1100 Subject: [PATCH 22/33] Per review, improve Vine::VoucherValidatorService plus specs --- .../vine/voucher_validator_service.rb | 5 ++- .../vine/voucher_validator_service_spec.rb | 35 ++++++++++++------- 2 files changed, 24 insertions(+), 16 deletions(-) diff --git a/app/services/vine/voucher_validator_service.rb b/app/services/vine/voucher_validator_service.rb index a2d085105a..9330ce80dc 100644 --- a/app/services/vine/voucher_validator_service.rb +++ b/app/services/vine/voucher_validator_service.rb @@ -28,7 +28,6 @@ module Vine Rails.logger.error e.inspect Bugsnag.notify(e) - # TODO do we need a more specific error ? errors[:vine_api] = I18n.t("vine_voucher_validator_service.errors.vine_api") nil end @@ -36,7 +35,7 @@ module Vine private def vine_settings - ConnectedApps::Vine.find_by(enterprise: @enterprise)&.data + @vine_settings ||= ConnectedApps::Vine.find_by(enterprise: @enterprise)&.data end def call_vine_api @@ -74,7 +73,7 @@ module Vine amount:, external_voucher_id: voucher_data["id"], external_voucher_set_id: voucher_data["voucher_set_id"], - voucher_type: "VINE" + voucher_type: Voucher::VINE ) end diff --git a/spec/services/vine/voucher_validator_service_spec.rb b/spec/services/vine/voucher_validator_service_spec.rb index 5e46df2b2e..1216e613f9 100644 --- a/spec/services/vine/voucher_validator_service_spec.rb +++ b/spec/services/vine/voucher_validator_service_spec.rb @@ -55,7 +55,7 @@ RSpec.describe Vine::VoucherValidatorService, feature: :connected_apps do expect(vine_voucher).not_to be_nil expect(vine_voucher.code).to eq(voucher_code) expect(vine_voucher.amount).to eq(5.00) - expect(vine_voucher.voucher_type).to eq("VINE") + expect(vine_voucher.vine?).to eq(true) expect(vine_voucher.external_voucher_id).to eq("9d2437c8-4559-4dda-802e-8d9c642a0c1d") expect(vine_voucher.external_voucher_set_id).to eq( "9d24349c-1fe8-4090-988b-d7355ed32559" @@ -65,7 +65,7 @@ RSpec.describe Vine::VoucherValidatorService, feature: :connected_apps do context "when distributor is not connected to VINE" do it "returns nil" do - expect(validate_voucher_service.validate).to be_nil + expect_validate_to_be_nil end it "doesn't call the VINE API" do @@ -83,7 +83,7 @@ RSpec.describe Vine::VoucherValidatorService, feature: :connected_apps do end it "doesn't creates a new VINE voucher" do - expect { validate_voucher_service.validate }.not_to change { Voucher.count } + expect_voucher_count_not_to_change end end @@ -95,11 +95,11 @@ RSpec.describe Vine::VoucherValidatorService, feature: :connected_apps do } before do - allow(vine_api_service).to receive(:voucher_validation).and_raise(Faraday::Error) + allow(vine_api_service).to receive(:voucher_validation).and_raise(Faraday::ConnectionFailed) end it "returns nil" do - expect(validate_voucher_service.validate).to be_nil + expect_validate_to_be_nil end it "adds an error message" do @@ -111,7 +111,7 @@ RSpec.describe Vine::VoucherValidatorService, feature: :connected_apps do end it "doesn't creates a new VINE voucher" do - expect { validate_voucher_service.validate }.not_to change { Voucher.count } + expect_voucher_count_not_to_change end it "logs the error and notify bugsnag" do @@ -143,7 +143,7 @@ RSpec.describe Vine::VoucherValidatorService, feature: :connected_apps do end it "returns nil" do - expect(validate_voucher_service.validate).to be_nil + expect_validate_to_be_nil end it "adds an error message" do @@ -155,7 +155,7 @@ RSpec.describe Vine::VoucherValidatorService, feature: :connected_apps do end it "doesn't creates a new VINE voucher" do - expect { validate_voucher_service.validate }.not_to change { Voucher.count } + expect_voucher_count_not_to_change end end @@ -179,7 +179,7 @@ RSpec.describe Vine::VoucherValidatorService, feature: :connected_apps do end it "returns nil" do - expect(validate_voucher_service.validate).to be_nil + expect_validate_to_be_nil end it "adds an error message" do @@ -191,7 +191,7 @@ RSpec.describe Vine::VoucherValidatorService, feature: :connected_apps do end it "doesn't creates a new VINE voucher" do - expect { validate_voucher_service.validate }.not_to change { Voucher.count } + expect_voucher_count_not_to_change end end @@ -215,7 +215,7 @@ RSpec.describe Vine::VoucherValidatorService, feature: :connected_apps do end it "returns nil" do - expect(validate_voucher_service.validate).to be_nil + expect_validate_to_be_nil end it "adds an error message" do @@ -227,7 +227,7 @@ RSpec.describe Vine::VoucherValidatorService, feature: :connected_apps do end it "doesn't creates a new VINE voucher" do - expect { validate_voucher_service.validate }.not_to change { Voucher.count } + expect_voucher_count_not_to_change end end @@ -265,6 +265,7 @@ RSpec.describe Vine::VoucherValidatorService, feature: :connected_apps do it "returns an invalid voucher" do voucher = validate_voucher_service.validate expect(voucher).not_to be_valid + expect(voucher.errors[:amount]).to include "must be greater than 0" end end @@ -315,7 +316,7 @@ RSpec.describe Vine::VoucherValidatorService, feature: :connected_apps do vine_voucher = validate_voucher_service.validate expect(vine_voucher.id).to eq(voucher.id) - expect(vine_voucher.amount).to eq(2.50) + expect(vine_voucher.reload.amount).to eq(2.50) end context "when updating the voucher fails" do @@ -352,6 +353,14 @@ RSpec.describe Vine::VoucherValidatorService, feature: :connected_apps do end end + def expect_validate_to_be_nil + expect(validate_voucher_service.validate).to be_nil + end + + def expect_voucher_count_not_to_change + expect { validate_voucher_service.validate }.not_to change { Voucher.count } + end + def mock_api_response(success:, data: nil, status: 200) mock_response = instance_double(Faraday::Response) allow(mock_response).to receive(:success?).and_return(success) From e7ece294cc222f7e9f4d3249f58967ca5d86b9e8 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou <40413322+rioug@users.noreply.github.com> Date: Mon, 4 Nov 2024 15:45:59 +1100 Subject: [PATCH 23/33] Better error for VineVoucherValidatorService Co-authored-by: David Cook --- config/locales/en.yml | 6 +++--- spec/services/vine/voucher_redeemer_service_spec.rb | 2 +- spec/services/vine/voucher_validator_service_spec.rb | 8 ++++---- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/config/locales/en.yml b/config/locales/en.yml index 8e71f4b38b..649f4eabc4 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -576,10 +576,10 @@ en: payment_could_not_complete: "The payment could not be completed" vine_voucher_validator_service: errors: - vine_settings: "No Vine api settings for the given enterprise" - vine_api: "There was an error communicating with the API" + vine_settings: "This shop is not enabled for VINE Vouchers." + vine_api: "There was an error communicating with the API, please try again later." invalid_voucher: "The voucher is not valid" - not_found_voucher: "The voucher doesn't exist" + not_found_voucher: "Sorry, we couldn't find that voucher, please check the code." vine_voucher_redeemer_service: errors: vine_settings: "No Vine api settings for the given enterprise" diff --git a/spec/services/vine/voucher_redeemer_service_spec.rb b/spec/services/vine/voucher_redeemer_service_spec.rb index 50c73f2a1c..77104c2352 100644 --- a/spec/services/vine/voucher_redeemer_service_spec.rb +++ b/spec/services/vine/voucher_redeemer_service_spec.rb @@ -199,7 +199,7 @@ RSpec.describe Vine::VoucherRedeemerService, feature: :connected_apps do voucher_redeemer_service.redeem expect(voucher_redeemer_service.errors).to include( - { vine_api: "There was an error communicating with the API" } + { vine_api: "There was an error communicating with the API, please try again later." } ) end diff --git a/spec/services/vine/voucher_validator_service_spec.rb b/spec/services/vine/voucher_validator_service_spec.rb index 1216e613f9..41b06b921b 100644 --- a/spec/services/vine/voucher_validator_service_spec.rb +++ b/spec/services/vine/voucher_validator_service_spec.rb @@ -78,7 +78,7 @@ RSpec.describe Vine::VoucherValidatorService, feature: :connected_apps do validate_voucher_service.validate expect(validate_voucher_service.errors).to include( - { vine_settings: "No Vine api settings for the given enterprise" } + { vine_settings: "This shop is not enabled for VINE Vouchers." } ) end @@ -106,7 +106,7 @@ RSpec.describe Vine::VoucherValidatorService, feature: :connected_apps do validate_voucher_service.validate expect(validate_voucher_service.errors).to include( - { vine_api: "There was an error communicating with the API" } + { vine_api: "There was an error communicating with the API, please try again later." } ) end @@ -150,7 +150,7 @@ RSpec.describe Vine::VoucherValidatorService, feature: :connected_apps do validate_voucher_service.validate expect(validate_voucher_service.errors).to include( - { vine_api: "There was an error communicating with the API" } + { vine_api: "There was an error communicating with the API, please try again later." } ) end @@ -186,7 +186,7 @@ RSpec.describe Vine::VoucherValidatorService, feature: :connected_apps do validate_voucher_service.validate expect(validate_voucher_service.errors).to include( - { not_found_voucher: "The voucher doesn't exist" } + { not_found_voucher: "Sorry, we couldn't find that voucher, please check the code." } ) end From a2c4c44eead4f72a3b9739b90618799082b4645c Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Wed, 6 Nov 2024 14:49:03 +1100 Subject: [PATCH 24/33] Move Vine voucher to Vouchers::Vine A Vine voucher is really a specific type of FlatRate voucher but because a Vine voucher can be used by mutiple enterprise, it can be considered different enough to warrant it's own class. It still share a lot of the behaviour of a FlatRate voucher, so to avoid duplication, all the shared functionality have been moved to a Vouchers::FlatRatable concern. --- app/models/concerns/vouchers/flat_ratable.rb | 31 +++++++ app/models/voucher.rb | 11 +-- app/models/vouchers/flat_rate.rb | 20 +---- app/models/vouchers/percentage_rate.rb | 1 + app/models/vouchers/vine.rb | 13 +++ ...ex_and_remove_voucher_type_from_voucher.rb | 9 ++ db/schema.rb | 6 +- spec/factories/voucher_factory.rb | 4 +- spec/models/voucher_spec.rb | 26 ------ spec/models/vouchers/flat_rate_spec.rb | 1 + spec/models/vouchers/percentage_rate_spec.rb | 1 + spec/models/vouchers/vine_spec.rb | 83 +++++++++++++++++++ 12 files changed, 147 insertions(+), 59 deletions(-) create mode 100644 app/models/concerns/vouchers/flat_ratable.rb create mode 100644 app/models/vouchers/vine.rb create mode 100644 db/migrate/20241112230401_update_index_and_remove_voucher_type_from_voucher.rb create mode 100644 spec/models/vouchers/vine_spec.rb diff --git a/app/models/concerns/vouchers/flat_ratable.rb b/app/models/concerns/vouchers/flat_ratable.rb new file mode 100644 index 0000000000..21f4882cf7 --- /dev/null +++ b/app/models/concerns/vouchers/flat_ratable.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +require "active_support/concern" + +module Vouchers + module FlatRatable + extend ActiveSupport::Concern + + included do + validates :amount, + presence: true, + numericality: { greater_than: 0 } + end + + def display_value + Spree::Money.new(amount) + end + + # We limit adjustment to the maximum amount needed to cover the order, ie if the voucher + # covers more than the order.total we only need to create an adjustment covering the order.total + def compute_amount(order) + -amount.clamp(0, order.pre_discount_total) + end + + def rate(order) + amount = compute_amount(order) + + amount / order.pre_discount_total + end + end +end diff --git a/app/models/voucher.rb b/app/models/voucher.rb index b8ff92a81d..5be186d4d9 100644 --- a/app/models/voucher.rb +++ b/app/models/voucher.rb @@ -1,8 +1,6 @@ # frozen_string_literal: false class Voucher < ApplicationRecord - VINE_TYPE = "VINE".freeze - self.belongs_to_required_by_default = false acts_as_paranoid @@ -16,13 +14,10 @@ class Voucher < ApplicationRecord class_name: 'Spree::Adjustment', dependent: nil - validates :code, presence: true, uniqueness: { scope: :enterprise_id } + validates :code, presence: true TYPES = ["Vouchers::FlatRate", "Vouchers::PercentageRate"].freeze - scope :vine, -> { where(voucher_type: VINE_TYPE) } - scope :local, -> { where("voucher_type IS DISTINCT FROM ?", VINE_TYPE) } - def code=(value) super(value.to_s.strip) end @@ -47,10 +42,6 @@ class Voucher < ApplicationRecord order.adjustments.create(adjustment_attributes) end - def vine? - voucher_type == VINE_TYPE - end - # The following method must be overriden in a concrete voucher. def display_value raise NotImplementedError, 'please use concrete voucher' diff --git a/app/models/vouchers/flat_rate.rb b/app/models/vouchers/flat_rate.rb index 9ad0d3de89..45b85e26ab 100644 --- a/app/models/vouchers/flat_rate.rb +++ b/app/models/vouchers/flat_rate.rb @@ -2,24 +2,8 @@ module Vouchers class FlatRate < Voucher - validates :amount, - presence: true, - numericality: { greater_than: 0 } + include FlatRatable - def display_value - Spree::Money.new(amount) - end - - # We limit adjustment to the maximum amount needed to cover the order, ie if the voucher - # covers more than the order.total we only need to create an adjustment covering the order.total - def compute_amount(order) - -amount.clamp(0, order.pre_discount_total) - end - - def rate(order) - amount = compute_amount(order) - - amount / order.pre_discount_total - end + validates :code, uniqueness: { scope: :enterprise_id } end end diff --git a/app/models/vouchers/percentage_rate.rb b/app/models/vouchers/percentage_rate.rb index 41b17e77f6..06d0263280 100644 --- a/app/models/vouchers/percentage_rate.rb +++ b/app/models/vouchers/percentage_rate.rb @@ -5,6 +5,7 @@ module Vouchers validates :amount, presence: true, numericality: { greater_than: 0, less_than_or_equal_to: 100 } + validates :code, uniqueness: { scope: :enterprise_id } def display_value ActionController::Base.helpers.number_to_percentage(amount, precision: 2) diff --git a/app/models/vouchers/vine.rb b/app/models/vouchers/vine.rb new file mode 100644 index 0000000000..3e87047a54 --- /dev/null +++ b/app/models/vouchers/vine.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: false + +module Vouchers + class Vine < Voucher + include FlatRatable + + # a VINE voucher : + # - can potentially be associated with mutiple enterprise + # - code ( "short code" in VINE ) can be recycled, but they shouldn't be linked to the same + # voucher_id + validates :code, uniqueness: { scope: [:enterprise_id, :external_voucher_id] } + end +end diff --git a/db/migrate/20241112230401_update_index_and_remove_voucher_type_from_voucher.rb b/db/migrate/20241112230401_update_index_and_remove_voucher_type_from_voucher.rb new file mode 100644 index 0000000000..df967c3e19 --- /dev/null +++ b/db/migrate/20241112230401_update_index_and_remove_voucher_type_from_voucher.rb @@ -0,0 +1,9 @@ +class UpdateIndexAndRemoveVoucherTypeFromVoucher < ActiveRecord::Migration[7.0] + def change + remove_column :vouchers, :voucher_type + remove_index :vouchers, [:code, :enterprise_id], unique: true + + add_index :vouchers, [:code, :enterprise_id] + add_index :vouchers, [:code, :enterprise_id, :external_voucher_id], name: "index_vouchers_on_code_and_enterprise_id_and_ext_voucher_id" + end +end diff --git a/db/schema.rb b/db/schema.rb index c9128ba17a..be371a9499 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.0].define(version: 2024_10_30_033956) do +ActiveRecord::Schema[7.0].define(version: 2024_11_12_230401) do # These are extensions that must be enabled in order to support this database enable_extension "pg_stat_statements" enable_extension "plpgsql" @@ -1112,8 +1112,8 @@ ActiveRecord::Schema[7.0].define(version: 2024_10_30_033956) do t.string "type", limit: 255, default: "Vouchers::FlatRate", null: false t.uuid "external_voucher_id" t.uuid "external_voucher_set_id" - t.string "voucher_type" - t.index ["code", "enterprise_id"], name: "index_vouchers_on_code_and_enterprise_id", unique: true + t.index ["code", "enterprise_id", "external_voucher_id"], name: "index_vouchers_on_code_and_enterprise_id_and_ext_voucher_id" + t.index ["code", "enterprise_id"], name: "index_vouchers_on_code_and_enterprise_id" t.index ["deleted_at"], name: "index_vouchers_on_deleted_at" t.index ["enterprise_id"], name: "index_vouchers_on_enterprise_id" end diff --git a/spec/factories/voucher_factory.rb b/spec/factories/voucher_factory.rb index 034fae6b77..df6835e61e 100644 --- a/spec/factories/voucher_factory.rb +++ b/spec/factories/voucher_factory.rb @@ -15,8 +15,8 @@ FactoryBot.define do amount { rand(1..100) } end - factory :vine_voucher, parent: :voucher_flat_rate do - voucher_type { Voucher::VINE_TYPE } + factory :vine_voucher, parent: :voucher, class: Vouchers::Vine do + amount { 20 } external_voucher_id { SecureRandom.uuid } external_voucher_set_id { SecureRandom.uuid } end diff --git a/spec/models/voucher_spec.rb b/spec/models/voucher_spec.rb index c0043f42eb..aff8f19eef 100644 --- a/spec/models/voucher_spec.rb +++ b/spec/models/voucher_spec.rb @@ -15,31 +15,6 @@ RSpec.describe Voucher do it { is_expected.to have_many(:adjustments).dependent(nil) } end - describe "scope" do - let!(:vine_voucher) { create(:vine_voucher) } - - describe ".vine" do - it "returns only vine vouchers" do - create(:voucher) - expect(Voucher.vine.count).to eq(1) - end - end - - describe ".local" do - it "returns only local vouchers" do - create(:voucher) - expect(Voucher.local.count).to eq(1) - end - - context "with voucher type other than VINE" do - it "returns only local vouchers" do - create(:voucher, voucher_type: "other") - expect(Voucher.local.count).to eq(1) - end - end - end - end - describe '#code=' do it "removes leading and trailing whitespace" do voucher = build(:voucher, code: "\r\n\t new_code \r\n\t") @@ -52,7 +27,6 @@ RSpec.describe Voucher do subject { build(:voucher_flat_rate, code: 'new_code', enterprise:) } it { is_expected.to validate_presence_of(:code) } - it { is_expected.to validate_uniqueness_of(:code).scoped_to(:enterprise_id) } end describe '#display_value' do diff --git a/spec/models/vouchers/flat_rate_spec.rb b/spec/models/vouchers/flat_rate_spec.rb index 9294784746..10c0f56a46 100644 --- a/spec/models/vouchers/flat_rate_spec.rb +++ b/spec/models/vouchers/flat_rate_spec.rb @@ -8,6 +8,7 @@ RSpec.describe Vouchers::FlatRate do it { is_expected.to validate_presence_of(:amount) } it { is_expected.to validate_numericality_of(:amount).is_greater_than(0) } + it { is_expected.to validate_uniqueness_of(:code).scoped_to(:enterprise_id) } end describe '#compute_amount' do diff --git a/spec/models/vouchers/percentage_rate_spec.rb b/spec/models/vouchers/percentage_rate_spec.rb index b172baf016..116751d126 100644 --- a/spec/models/vouchers/percentage_rate_spec.rb +++ b/spec/models/vouchers/percentage_rate_spec.rb @@ -12,6 +12,7 @@ RSpec.describe Vouchers::PercentageRate do .is_greater_than(0) .is_less_than_or_equal_to(100) end + it { is_expected.to validate_uniqueness_of(:code).scoped_to(:enterprise_id) } end describe '#compute_amount' do diff --git a/spec/models/vouchers/vine_spec.rb b/spec/models/vouchers/vine_spec.rb new file mode 100644 index 0000000000..157aac3776 --- /dev/null +++ b/spec/models/vouchers/vine_spec.rb @@ -0,0 +1,83 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Vouchers::Vine do + describe 'validations' do + subject { build(:vine_voucher) } + + it { is_expected.to validate_presence_of(:amount) } + it { is_expected.to validate_numericality_of(:amount).is_greater_than(0) } + + describe "#code" do + subject { build(:vine_voucher, code: 'vine_code', enterprise:, external_voucher_id: ) } + + let(:external_voucher_id) { SecureRandom.uuid } + let(:enterprise) { create(:enterprise) } + + it { + is_expected.to validate_uniqueness_of(:code).scoped_to( + [:enterprise_id, :external_voucher_id] + ) + } + + it "can be reused within the same enterprise" do + subject.save! + # Voucher with the same code but different external_voucher_id, it is mapped to a + # different voucher in VINE + voucher = build(:vine_voucher, code: 'vine_code', enterprise: ) + expect(voucher.valid?).to be(true) + end + + it "can be used by mutiple enterprises" do + subject.save! + # Voucher with the same code and external_voucher_id, ie exiting VINE voucher used by + # another enterprise + voucher = build(:vine_voucher, code: 'vine_code', enterprise: build(:enterprise), + external_voucher_id: ) + expect(voucher.valid?).to be(true) + end + end + end + + describe '#compute_amount' do + let(:order) { create(:order_with_totals) } + + before do + order.update_columns(item_total: 15) + end + + context 'when order total is more than the voucher' do + subject { create(:vine_voucher, amount: 5) } + + it 'uses the voucher total' do + expect(subject.compute_amount(order).to_f).to eq(-5) + end + end + + context 'when order total is less than the voucher' do + subject { create(:vine_voucher, amount: 20) } + + it 'matches the order total' do + expect(subject.compute_amount(order).to_f).to eq(-15) + end + end + end + + describe "#rate" do + subject do + create(:vine_voucher, code: 'new_code', amount: 5) + end + let(:order) { create(:order_with_totals) } + + before do + order.update_columns(item_total: 10) + end + + it "returns the voucher rate" do + # rate = -voucher_amount / order.pre_discount_total + # -5 / 10 = -0.5 + expect(subject.rate(order).to_f).to eq(-0.5) + end + end +end From 48ad7ed8a09255d6033c79884e39585fae134de0 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 11 Nov 2024 15:03:17 +1100 Subject: [PATCH 25/33] Add voucher used by multiple enterprise and recycle code scenario Plus optimise code with `find_or_initialize_by` as suggested in review --- .../vine/voucher_validator_service.rb | 23 ++-- .../vine/voucher_validator_service_spec.rb | 104 ++++++++++++++++-- 2 files changed, 102 insertions(+), 25 deletions(-) diff --git a/app/services/vine/voucher_validator_service.rb b/app/services/vine/voucher_validator_service.rb index 9330ce80dc..c8f9aa628d 100644 --- a/app/services/vine/voucher_validator_service.rb +++ b/app/services/vine/voucher_validator_service.rb @@ -61,21 +61,14 @@ module Vine voucher_data = response.body["data"] # Check if voucher already exist - voucher = Voucher.vine.find_by(code: voucher_code, enterprise: @enterprise) - - amount = voucher_data["voucher_value_remaining"].to_f / 100 - if voucher.present? - voucher.update(amount: ) - else - voucher = Vouchers::FlatRate.create( - enterprise: @enterprise, - code: voucher_data["voucher_short_code"], - amount:, - external_voucher_id: voucher_data["id"], - external_voucher_set_id: voucher_data["voucher_set_id"], - voucher_type: Voucher::VINE - ) - end + voucher = Voucher.vine.find_or_initialize_by( + code: voucher_code, + enterprise: @enterprise, + external_voucher_id: voucher_data["id"], + external_voucher_set_id: voucher_data["voucher_set_id"] + ) + voucher.amount = voucher_data["voucher_value_remaining"].to_f / 100 + voucher.save voucher end diff --git a/spec/services/vine/voucher_validator_service_spec.rb b/spec/services/vine/voucher_validator_service_spec.rb index 41b06b921b..5d95262c00 100644 --- a/spec/services/vine/voucher_validator_service_spec.rb +++ b/spec/services/vine/voucher_validator_service_spec.rb @@ -24,9 +24,9 @@ RSpec.describe Vine::VoucherValidatorService, feature: :connected_apps do { meta: { responseCode: 200, limit: 50, offset: 0, message: "" }, data: { - id: "9d2437c8-4559-4dda-802e-8d9c642a0c1d", + id: vine_voucher_id, voucher_short_code: voucher_code, - voucher_set_id: "9d24349c-1fe8-4090-988b-d7355ed32559", + voucher_set_id: vine_voucher_set_id, is_test: 1, voucher_value_original: 500, voucher_value_remaining: 500, @@ -38,17 +38,19 @@ RSpec.describe Vine::VoucherValidatorService, feature: :connected_apps do } }.deep_stringify_keys } + let(:vine_voucher_id) { "9d2437c8-4559-4dda-802e-8d9c642a0c1d" } + 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:)) + .and_return(mock_api_response(success: true, 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:)) + .and_return(mock_api_response(success: true, data:)) vine_voucher = validate_voucher_service.validate @@ -56,10 +58,89 @@ RSpec.describe Vine::VoucherValidatorService, feature: :connected_apps do expect(vine_voucher.code).to eq(voucher_code) expect(vine_voucher.amount).to eq(5.00) expect(vine_voucher.vine?).to eq(true) - expect(vine_voucher.external_voucher_id).to eq("9d2437c8-4559-4dda-802e-8d9c642a0c1d") - expect(vine_voucher.external_voucher_set_id).to eq( - "9d24349c-1fe8-4090-988b-d7355ed32559" - ) + expect(vine_voucher.external_voucher_id).to eq(vine_voucher_id) + expect(vine_voucher.external_voucher_set_id).to eq(vine_voucher_set_id) + end + + context "when the VINE voucher has already been used by another enterprise" do + let(:data) { + { + meta: { responseCode: 200, limit: 50, offset: 0, message: "" }, + data: { + id: vine_voucher_id, + voucher_short_code: voucher_code, + voucher_set_id: vine_voucher_set_id, + is_test: 1, + voucher_value_original: 500, + voucher_value_remaining: 250, + num_voucher_redemptions: 0, + last_redemption_at: "null", + created_at: "2024-10-01T13:20:02.000000Z", + updated_at: "2024-10-01T13:20:02.000000Z", + deleted_at: "null" + } + }.deep_stringify_keys + } + + it "creates a new voucher" do + existing_voucher = create(:vine_voucher, enterprise: create(:enterprise), + code: voucher_code, + 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:)) + + vine_voucher = validate_voucher_service.validate + + expect(vine_voucher.id).not_to eq(existing_voucher.id) + expect(vine_voucher.enterprise).to eq(distributor) + expect(vine_voucher.code).to eq(voucher_code) + expect(vine_voucher.amount).to eq(2.50) + expect(vine_voucher.vine?).to eq(true) + expect(vine_voucher.external_voucher_id).to eq(vine_voucher_id) + expect(vine_voucher.external_voucher_set_id).to eq(vine_voucher_set_id) + end + end + + context "with a recycled code" do + let(:data) { + { + meta: { responseCode: 200, limit: 50, offset: 0, message: "" }, + data: { + id: new_vine_voucher_id, + voucher_short_code: voucher_code, + voucher_set_id: new_vine_voucher_set_id, + is_test: 1, + voucher_value_original: 500, + voucher_value_remaining: 140, + num_voucher_redemptions: 0, + last_redemption_at: "null", + created_at: "2024-10-01T13:20:02.000000Z", + updated_at: "2024-10-01T13:20:02.000000Z", + deleted_at: "null" + } + }.deep_stringify_keys + } + let(:new_vine_voucher_id) { "9d2437c8-4559-4dda-802e-8d9c642a0c5e" } + let(:new_vine_voucher_set_id) { "9d24349c-1fe8-4090-988b-d7355ed32590" } + + it "creates a new voucher" do + existing_voucher = create(:vine_voucher, enterprise: distributor, code: voucher_code, + 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:)) + + vine_voucher = validate_voucher_service.validate + + expect(vine_voucher.id).not_to eq(existing_voucher.id) + expect(vine_voucher.enterprise).to eq(distributor) + expect(vine_voucher.code).to eq(voucher_code) + expect(vine_voucher.amount).to eq(1.40) + expect(vine_voucher.vine?).to eq(true) + expect(vine_voucher.external_voucher_id).to eq(new_vine_voucher_id) + expect(vine_voucher.external_voucher_set_id).to eq(new_vine_voucher_set_id) + end end end @@ -276,14 +357,17 @@ RSpec.describe Vine::VoucherValidatorService, feature: :connected_apps do ) } let!(:voucher) { - create(:vine_voucher, enterprise: distributor, code: voucher_code, amount: 500) + create(:vine_voucher, enterprise: distributor, code: voucher_code, amount: 500, + external_voucher_id: vine_voucher_id, + external_voucher_set_id: "9d24349c-1fe8-4090-988b-d7355ed32559") } + let(:vine_voucher_id) { "9d2437c8-4559-4dda-802e-8d9c642a0c1d" } let(:data) { { meta: { responseCode: 200, limit: 50, offset: 0, message: "" }, data: { - id: "9d2437c8-4559-4dda-802e-8d9c642a0c1d", + id: vine_voucher_id, voucher_short_code: voucher_code, voucher_set_id: "9d24349c-1fe8-4090-988b-d7355ed32559", is_test: 1, From d413a142c990ae2a578e53191a792f5269792c93 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Wed, 13 Nov 2024 09:55:03 +1100 Subject: [PATCH 26/33] Update various voucher related file to use the new Vouchers::Vine --- .../voucher_adjustments_controller.rb | 6 +++--- app/services/vine/voucher_redeemer_service.rb | 2 +- app/services/vine/voucher_validator_service.rb | 2 +- .../admin/enterprises/form/_vouchers.html.haml | 4 ++-- spec/requests/voucher_adjustments_spec.rb | 16 ++++++++-------- .../vine/voucher_validator_service_spec.rb | 6 +++--- spec/system/admin/vouchers_spec.rb | 4 ++++ spec/system/consumer/checkout/payment_spec.rb | 5 +++-- 8 files changed, 25 insertions(+), 20 deletions(-) diff --git a/app/controllers/voucher_adjustments_controller.rb b/app/controllers/voucher_adjustments_controller.rb index 8f85798c2f..3b774d46ea 100644 --- a/app/controllers/voucher_adjustments_controller.rb +++ b/app/controllers/voucher_adjustments_controller.rb @@ -78,9 +78,9 @@ class VoucherAdjustmentsController < BaseController end def load_voucher - voucher = Voucher.local.find_by(code: voucher_params[:voucher_code], - enterprise: @order.distributor) - return voucher unless voucher.nil? + voucher = Voucher.find_by(code: voucher_params[:voucher_code], + enterprise: @order.distributor) + return voucher unless voucher.nil? || voucher.is_a?(Vouchers::Vine) vine_voucher end diff --git a/app/services/vine/voucher_redeemer_service.rb b/app/services/vine/voucher_redeemer_service.rb index 6e03e0d2b8..25fb26b505 100644 --- a/app/services/vine/voucher_redeemer_service.rb +++ b/app/services/vine/voucher_redeemer_service.rb @@ -14,7 +14,7 @@ module Vine voucher_adjustment = order.voucher_adjustments.first @voucher = voucher_adjustment&.originator - return true if voucher_adjustment.nil? || !@voucher.vine? + return true if voucher_adjustment.nil? || !@voucher.is_a?(Vouchers::Vine) if vine_settings.nil? errors[:vine_settings] = I18n.t("vine_voucher_redeemer_service.errors.vine_settings") diff --git a/app/services/vine/voucher_validator_service.rb b/app/services/vine/voucher_validator_service.rb index c8f9aa628d..0eba5ee3b3 100644 --- a/app/services/vine/voucher_validator_service.rb +++ b/app/services/vine/voucher_validator_service.rb @@ -61,7 +61,7 @@ module Vine voucher_data = response.body["data"] # Check if voucher already exist - voucher = Voucher.vine.find_or_initialize_by( + voucher = Vouchers::Vine.find_or_initialize_by( code: voucher_code, enterprise: @enterprise, external_voucher_id: voucher_data["id"], diff --git a/app/views/admin/enterprises/form/_vouchers.html.haml b/app/views/admin/enterprises/form/_vouchers.html.haml index 042d1e7470..faea0baa58 100644 --- a/app/views/admin/enterprises/form/_vouchers.html.haml +++ b/app/views/admin/enterprises/form/_vouchers.html.haml @@ -3,7 +3,7 @@ = t('.add_new') %br -- if @enterprise.vouchers.local.with_deleted.present? +- if @enterprise.vouchers.where.not(type: "Vouchers::Vine").with_deleted.present? %table %thead %tr @@ -17,7 +17,7 @@ /%th= t('.customers') /%th= t('.net_value') %tbody - - @enterprise.vouchers.local.with_deleted.order(deleted_at: :desc, code: :asc).each do |voucher| + - @enterprise.vouchers.where.not(type: "Vouchers::Vine").with_deleted.order(deleted_at: :desc, code: :asc).each do |voucher| %tr %td= voucher.code %td= voucher.display_value diff --git a/spec/requests/voucher_adjustments_spec.rb b/spec/requests/voucher_adjustments_spec.rb index cdfdebb899..074b8e1a8a 100644 --- a/spec/requests/voucher_adjustments_spec.rb +++ b/spec/requests/voucher_adjustments_spec.rb @@ -56,7 +56,7 @@ RSpec.describe VoucherAdjustmentsController, type: :request do # Create a non valid adjustment bad_adjustment = build(:adjustment, label: nil) allow(voucher).to receive(:create_adjustment).and_return(bad_adjustment) - allow(Voucher).to receive_message_chain(:local, :find_by).and_return(voucher) + allow(Voucher).to receive(:find_by).and_return(voucher) post("/voucher_adjustments", params:) @@ -106,7 +106,7 @@ RSpec.describe VoucherAdjustmentsController, type: :request do end it "adds a voucher to the user's current order" do - vine_voucher = create(:voucher_flat_rate, code: vine_voucher_code) + vine_voucher = create(:vine_voucher, code: vine_voucher_code) mock_vine_voucher_validator(voucher: vine_voucher) post("/voucher_adjustments", params:) @@ -170,8 +170,8 @@ RSpec.describe VoucherAdjustmentsController, type: :request do context "when creating a new voucher fails" do it "returns 422 and an error message" do - vine_voucher = build(:voucher_flat_rate, code: vine_voucher_code, - enterprise: distributor, amount: "") + vine_voucher = build(:vine_voucher, code: vine_voucher_code, + enterprise: distributor, amount: "") mock_vine_voucher_validator(voucher: vine_voucher) post("/voucher_adjustments", params:) @@ -197,8 +197,8 @@ RSpec.describe VoucherAdjustmentsController, type: :request do end it "adds a voucher to the user's current order" do - vine_voucher = create(:voucher_flat_rate, code: vine_voucher_code, - enterprise: distributor) + vine_voucher = create(:vine_voucher, code: vine_voucher_code, + enterprise: distributor) mock_vine_voucher_validator(voucher: vine_voucher) expect { @@ -209,8 +209,8 @@ RSpec.describe VoucherAdjustmentsController, type: :request do context "when updating the voucher fails" do it "returns 422 and an error message" do - vine_voucher = build(:voucher_flat_rate, code: vine_voucher_code, - enterprise: distributor, amount: "") + vine_voucher = build(:vine_voucher, code: vine_voucher_code, + enterprise: distributor, amount: "") mock_vine_voucher_validator(voucher: vine_voucher) 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 5d95262c00..ad795d43c7 100644 --- a/spec/services/vine/voucher_validator_service_spec.rb +++ b/spec/services/vine/voucher_validator_service_spec.rb @@ -55,9 +55,9 @@ RSpec.describe Vine::VoucherValidatorService, feature: :connected_apps do vine_voucher = validate_voucher_service.validate expect(vine_voucher).not_to be_nil + expect(vine_voucher).to be_a(Vouchers::Vine) expect(vine_voucher.code).to eq(voucher_code) expect(vine_voucher.amount).to eq(5.00) - expect(vine_voucher.vine?).to eq(true) expect(vine_voucher.external_voucher_id).to eq(vine_voucher_id) expect(vine_voucher.external_voucher_set_id).to eq(vine_voucher_set_id) end @@ -96,7 +96,7 @@ RSpec.describe Vine::VoucherValidatorService, feature: :connected_apps do expect(vine_voucher.enterprise).to eq(distributor) expect(vine_voucher.code).to eq(voucher_code) expect(vine_voucher.amount).to eq(2.50) - expect(vine_voucher.vine?).to eq(true) + expect(vine_voucher).to be_a(Vouchers::Vine) expect(vine_voucher.external_voucher_id).to eq(vine_voucher_id) expect(vine_voucher.external_voucher_set_id).to eq(vine_voucher_set_id) end @@ -137,7 +137,7 @@ RSpec.describe Vine::VoucherValidatorService, feature: :connected_apps do expect(vine_voucher.enterprise).to eq(distributor) expect(vine_voucher.code).to eq(voucher_code) expect(vine_voucher.amount).to eq(1.40) - expect(vine_voucher.vine?).to eq(true) + expect(vine_voucher).to be_a(Vouchers::Vine) expect(vine_voucher.external_voucher_id).to eq(new_vine_voucher_id) expect(vine_voucher.external_voucher_set_id).to eq(new_vine_voucher_set_id) end diff --git a/spec/system/admin/vouchers_spec.rb b/spec/system/admin/vouchers_spec.rb index 2673cb2403..3a6fb74a79 100644 --- a/spec/system/admin/vouchers_spec.rb +++ b/spec/system/admin/vouchers_spec.rb @@ -11,6 +11,7 @@ RSpec.describe ' let(:enterprise) { create(:supplier_enterprise, name: 'Feedme', sells: 'own') } let(:voucher_code) { 'awesomevoucher' } + let(:vine_voucher_code) { 'vine_voucher' } let(:amount) { 25 } let(:enterprise_user) { create(:user, enterprise_limit: 1) } @@ -22,6 +23,7 @@ RSpec.describe ' it 'lists enterprise vouchers' do # Given an enterprise with vouchers create(:voucher_flat_rate, enterprise:, code: voucher_code, amount:) + create(:vine_voucher, enterprise:, code: vine_voucher_code, amount:) # When I go to the enterprise voucher tab visit edit_admin_enterprise_path(enterprise) @@ -31,6 +33,8 @@ RSpec.describe ' # Then I see a list of vouchers expect(page).to have_content voucher_code expect(page).to have_content amount + + expect(page).not_to have_content vine_voucher_code end describe "adding voucher" do diff --git a/spec/system/consumer/checkout/payment_spec.rb b/spec/system/consumer/checkout/payment_spec.rb index d53e9c8908..0f3caddea8 100644 --- a/spec/system/consumer/checkout/payment_spec.rb +++ b/spec/system/consumer/checkout/payment_spec.rb @@ -189,7 +189,8 @@ RSpec.describe "As a consumer, I want to checkout my order" do expect(page).to have_content "$5.00 Voucher" expect(order.reload.voucher_adjustments.length).to eq(1) - expect(Voucher.vine.find_by(code: "CI3922", enterprise: distributor)).not_to be_nil + expect(Vouchers::Vine.find_by(code: "CI3922", + enterprise: distributor)).not_to be_nil end context "with an invalid voucher" do @@ -198,7 +199,7 @@ RSpec.describe "As a consumer, I want to checkout my order" do click_button("Apply") expect(page).to have_content("There was an error while adding the voucher") - expect(Voucher.vine.find_by(code: "non_code", enterprise: distributor)).to be_nil + expect(Vouchers::Vine.find_by(code: "KM1891", enterprise: distributor)).to be_nil end end end From 9ab2a3ae3df5e0342b6fb030c183012e23e972f2 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 11 Nov 2024 16:19:54 +1100 Subject: [PATCH 27/33] Per review, fix a some minor issues --- app/controllers/checkout_controller.rb | 2 +- spec/requests/spree/admin/payments_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb index 904d0d5b05..b41eeb5265 100644 --- a/app/controllers/checkout_controller.rb +++ b/app/controllers/checkout_controller.rb @@ -80,7 +80,7 @@ class CheckoutController < BaseController # Redeem VINE voucher vine_voucher_redeemer = Vine::VoucherRedeemerService.new(order: @order) - if vine_voucher_redeemer.redeem == false + unless vine_voucher_redeemer.redeem # rubocop:disable Rails/DeprecatedActiveModelErrorsMethods flash[:error] = if vine_voucher_redeemer.errors.keys.include?(:redeeming_failed) vine_voucher_redeemer.errors[:redeeming_failed] diff --git a/spec/requests/spree/admin/payments_spec.rb b/spec/requests/spree/admin/payments_spec.rb index 7992064410..f8e9f6e3c6 100644 --- a/spec/requests/spree/admin/payments_spec.rb +++ b/spec/requests/spree/admin/payments_spec.rb @@ -47,7 +47,7 @@ RSpec.describe Spree::Admin::PaymentsController, type: :request do end end - context "when a getway error happens" do + context "when a gateway error happens" do let(:payment_method) do create(:stripe_sca_payment_method, distributors: [order.distributor]) end From 73819a4638ca94390faaa6b46cfb5d383854d4d3 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Wed, 13 Nov 2024 14:01:56 +1100 Subject: [PATCH 28/33] Fix unique validator for vouche code Paranoia doesn't support unique validation including deleted records: https://github.com/rubysherpas/paranoia/pull/333 We use a custom validator, ScopedUniquenessValidator to avoid the issue --- app/models/vouchers/flat_rate.rb | 2 +- app/models/vouchers/percentage_rate.rb | 2 +- .../vouchers/scoped_uniqueness_validator.rb | 25 ++++++++++++ spec/models/vouchers/flat_rate_spec.rb | 2 +- spec/models/vouchers/percentage_rate_spec.rb | 2 +- spec/support/voucher_uniqueness_helper.rb | 39 +++++++++++++++++++ 6 files changed, 68 insertions(+), 4 deletions(-) create mode 100644 app/validators/vouchers/scoped_uniqueness_validator.rb create mode 100644 spec/support/voucher_uniqueness_helper.rb diff --git a/app/models/vouchers/flat_rate.rb b/app/models/vouchers/flat_rate.rb index 45b85e26ab..b09013d11e 100644 --- a/app/models/vouchers/flat_rate.rb +++ b/app/models/vouchers/flat_rate.rb @@ -4,6 +4,6 @@ module Vouchers class FlatRate < Voucher include FlatRatable - validates :code, uniqueness: { scope: :enterprise_id } + validates_with ScopedUniquenessValidator end end diff --git a/app/models/vouchers/percentage_rate.rb b/app/models/vouchers/percentage_rate.rb index 06d0263280..833aeaaa35 100644 --- a/app/models/vouchers/percentage_rate.rb +++ b/app/models/vouchers/percentage_rate.rb @@ -5,7 +5,7 @@ module Vouchers validates :amount, presence: true, numericality: { greater_than: 0, less_than_or_equal_to: 100 } - validates :code, uniqueness: { scope: :enterprise_id } + validates_with ScopedUniquenessValidator def display_value ActionController::Base.helpers.number_to_percentage(amount, precision: 2) diff --git a/app/validators/vouchers/scoped_uniqueness_validator.rb b/app/validators/vouchers/scoped_uniqueness_validator.rb new file mode 100644 index 0000000000..c537f0a194 --- /dev/null +++ b/app/validators/vouchers/scoped_uniqueness_validator.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: false + +# paranoia doesn't support unique validation including deleted records: +# https://github.com/rubysherpas/paranoia/pull/333 +# We use a custom validator to fix the issue, so we don't need to fork/patch the gem +module Vouchers + class ScopedUniquenessValidator < ActiveModel::Validator + def validate(record) + @record = record + + return unless unique_voucher_code_per_enterprise? + + record.errors.add :code, :taken, value: @record.code + end + + private + + def unique_voucher_code_per_enterprise? + query = Voucher.with_deleted.where(code: @record.code, enterprise_id: @record.enterprise_id) + query = query.where.not(id: @record.id) unless @record.id.nil? + + query.present? + end + end +end diff --git a/spec/models/vouchers/flat_rate_spec.rb b/spec/models/vouchers/flat_rate_spec.rb index 10c0f56a46..b9e85d9fbb 100644 --- a/spec/models/vouchers/flat_rate_spec.rb +++ b/spec/models/vouchers/flat_rate_spec.rb @@ -8,7 +8,7 @@ RSpec.describe Vouchers::FlatRate do it { is_expected.to validate_presence_of(:amount) } it { is_expected.to validate_numericality_of(:amount).is_greater_than(0) } - it { is_expected.to validate_uniqueness_of(:code).scoped_to(:enterprise_id) } + it_behaves_like 'has a unique code per enterprise', "voucher_flat_rate" end describe '#compute_amount' do diff --git a/spec/models/vouchers/percentage_rate_spec.rb b/spec/models/vouchers/percentage_rate_spec.rb index 116751d126..89c4834da3 100644 --- a/spec/models/vouchers/percentage_rate_spec.rb +++ b/spec/models/vouchers/percentage_rate_spec.rb @@ -12,7 +12,7 @@ RSpec.describe Vouchers::PercentageRate do .is_greater_than(0) .is_less_than_or_equal_to(100) end - it { is_expected.to validate_uniqueness_of(:code).scoped_to(:enterprise_id) } + it_behaves_like 'has a unique code per enterprise', "voucher_percentage_rate" end describe '#compute_amount' do diff --git a/spec/support/voucher_uniqueness_helper.rb b/spec/support/voucher_uniqueness_helper.rb new file mode 100644 index 0000000000..7e8ddd1cf3 --- /dev/null +++ b/spec/support/voucher_uniqueness_helper.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +shared_examples_for 'has a unique code per enterprise' do |voucher_type| + describe "code" do + let(:code) { "super_code" } + let(:enterprise) { create(:enterprise) } + + it "is unique per enterprise" do + voucher = create(voucher_type, code:, enterprise:) + expect(voucher).to be_valid + + expect_voucher_with_same_enterprise_to_be_invalid(voucher_type) + + expect_voucher_with_other_enterprise_to_be_valid(voucher_type) + end + + context "with deleted voucher" do + it "is unique per enterprise" do + create(voucher_type, code:, enterprise:).destroy! + + expect_voucher_with_same_enterprise_to_be_invalid(voucher_type) + + expect_voucher_with_other_enterprise_to_be_valid(voucher_type) + end + end + end + + def expect_voucher_with_same_enterprise_to_be_invalid(voucher_type) + new_voucher = build(voucher_type, code:, enterprise: ) + + expect(new_voucher).not_to be_valid + expect(new_voucher.errors.full_messages).to include("Code has already been taken") + end + + def expect_voucher_with_other_enterprise_to_be_valid(voucher_type) + other_voucher = build(voucher_type, code:, enterprise: create(:enterprise) ) + expect(other_voucher).to be_valid + end +end From 1b50217242f011465fb68565a1c39183f06ac01d Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 18 Nov 2024 11:13:40 +1100 Subject: [PATCH 29/33] Re worked the Vine::ApiService to raise exception on error Log Client and Server error, and re raise exception for the caller to handle --- app/services/vine/api_service.rb | 58 ++++++++++++++------------ spec/services/vine/api_service_spec.rb | 44 ++++--------------- 2 files changed, 39 insertions(+), 63 deletions(-) diff --git a/app/services/vine/api_service.rb b/app/services/vine/api_service.rb index cdf1326be3..2a016a7400 100644 --- a/app/services/vine/api_service.rb +++ b/app/services/vine/api_service.rb @@ -15,39 +15,33 @@ module Vine def my_team my_team_url = "#{@vine_api_url}/my-team" - response = connection.get(my_team_url) - - log_error("Vine::ApiService#my_team", response) - - response + call_with_logging do + connection.get(my_team_url) + end end def voucher_validation(voucher_short_code) voucher_validation_url = "#{@vine_api_url}/voucher-validation" - response = connection.post( - voucher_validation_url, - { type: "voucher_code", value: voucher_short_code }, - 'Content-Type': "application/json" - ) - - log_error("Vine::ApiService#voucher_validation", response) - - response + call_with_logging do + connection.post( + voucher_validation_url, + { type: "voucher_code", value: voucher_short_code }, + 'Content-Type': "application/json" + ) + end end def voucher_redemptions(voucher_id, voucher_set_id, amount) voucher_redemptions_url = "#{@vine_api_url}/voucher-redemptions" - response = connection.post( - voucher_redemptions_url, - { voucher_id:, voucher_set_id:, amount: amount.to_i }, - 'Content-Type': "application/json" - ) - - log_error("Vine::ApiService#voucher_redemptions", response) - - response + call_with_logging do + connection.post( + voucher_redemptions_url, + { voucher_id:, voucher_set_id:, amount: amount.to_i }, + 'Content-Type': "application/json" + ) + end end private @@ -64,14 +58,24 @@ module Vine f.request :json f.response :json f.request :authorization, 'Bearer', api_key + f.use Faraday::Response::RaiseError end end - def log_error(prefix, response) - return if response.success? + def call_with_logging + yield + rescue Faraday::ClientError, Faraday::ServerError => e + # caller_location(2,1) gets us the second entry in the stacktrace, + # ie the method where `call_with_logging` is called from + log_error("#{self.class}##{caller_locations(2, 1)[0].label}", e.response) - Rails.logger.error "#{prefix} -- response_status: #{response.status}" - Rails.logger.error "#{prefix} -- response: #{response.body}" + # Re raise the same exception + raise + end + + def log_error(prefix, response) + Rails.logger.error "#{prefix} -- response_status: #{response[:status]}" + Rails.logger.error "#{prefix} -- response: #{response[:body]}" end end end diff --git a/spec/services/vine/api_service_spec.rb b/spec/services/vine/api_service_spec.rb index 33292ade65..6ca76ece16 100644 --- a/spec/services/vine/api_service_spec.rb +++ b/spec/services/vine/api_service_spec.rb @@ -60,18 +60,7 @@ RSpec.describe Vine::ApiService do stub_request(:get, my_team_url).to_return(body: "error", status: 401) expect(Rails.logger).to receive(:error).with(match("Vine::ApiService#my_team")).twice - - response = vine_api.my_team - - expect(response.success?).to be(false) - end - - it "return the response" do - stub_request(:get, my_team_url).to_return(body: "error", status: 401) - response = vine_api.my_team - - expect(response.success?).to be(false) - expect(response.body).not_to be_empty + expect { vine_api.my_team }.to raise_error(Faraday::UnauthorizedError) end end end @@ -126,18 +115,9 @@ RSpec.describe Vine::ApiService do expect(Rails.logger).to receive(:error).with( match("Vine::ApiService#voucher_validation") ).twice - - response = vine_api.voucher_validation(voucher_short_code) - - expect(response.success?).to be(false) - end - - it "return the response" do - stub_request(:post, voucher_validation_url).to_return(body: "error", status: 401) - response = vine_api.voucher_validation(voucher_short_code) - - expect(response.success?).to be(false) - expect(response.body).not_to be_empty + expect { + vine_api.voucher_validation(voucher_short_code) + }.to raise_error(Faraday::UnauthorizedError) end end end @@ -193,19 +173,11 @@ RSpec.describe Vine::ApiService do expect(Rails.logger).to receive(:error).with( match("Vine::ApiService#voucher_redemptions") - ).twice + ).twice.and_call_original - response = vine_api.voucher_redemptions(voucher_id, voucher_set_id, amount) - - expect(response.success?).to be(false) - end - - it "return the response" do - stub_request(:post, voucher_redemptions_url).to_return(body: "error", status: 401) - response = vine_api.voucher_redemptions(voucher_id, voucher_set_id, amount) - - expect(response.success?).to be(false) - expect(response.body).not_to be_empty + expect { + vine_api.voucher_redemptions(voucher_id, voucher_set_id, amount) + }.to raise_error(Faraday::UnauthorizedError) end end end From d102652c038c0c2e1901fcd191430dd5a44a3ecf Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 18 Nov 2024 11:25:39 +1100 Subject: [PATCH 30/33] 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 From 1e6fbadd8bba1fc7aeb781e93f161cc6825994b1 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 18 Nov 2024 11:39:30 +1100 Subject: [PATCH 31/33] Fix Vine::VoucherRedeemerService to handle exceptions --- app/services/vine/voucher_redeemer_service.rb | 19 ++++------- .../vine/voucher_redeemer_service_spec.rb | 34 +++++++------------ 2 files changed, 20 insertions(+), 33 deletions(-) diff --git a/app/services/vine/voucher_redeemer_service.rb b/app/services/vine/voucher_redeemer_service.rb index 25fb26b505..c9e0fa27ce 100644 --- a/app/services/vine/voucher_redeemer_service.rb +++ b/app/services/vine/voucher_redeemer_service.rb @@ -16,21 +16,16 @@ module Vine return true if voucher_adjustment.nil? || !@voucher.is_a?(Vouchers::Vine) - if vine_settings.nil? - errors[:vine_settings] = I18n.t("vine_voucher_redeemer_service.errors.vine_settings") - return false - end + return false if vine_settings.nil? - response = call_vine_api - - if !response.success? - handle_errors(response) - return false - end + call_vine_api voucher_adjustment.close true + rescue Faraday::ClientError => e + handle_errors(e.response) + false rescue Faraday::Error => e Rails.logger.error e.inspect Bugsnag.notify(e) @@ -42,7 +37,7 @@ module Vine private def vine_settings - ConnectedApps::Vine.find_by(enterprise: order.distributor)&.data + @vine_settings ||= ConnectedApps::Vine.find_by(enterprise: order.distributor)&.data end def call_vine_api @@ -56,7 +51,7 @@ module Vine end def handle_errors(response) - if response.status == 400 + if response[:status] == 400 errors[:redeeming_failed] = I18n.t("vine_voucher_redeemer_service.errors.redeeming_failed") else errors[:vine_api] = I18n.t("vine_voucher_redeemer_service.errors.vine_api") diff --git a/spec/services/vine/voucher_redeemer_service_spec.rb b/spec/services/vine/voucher_redeemer_service_spec.rb index 77104c2352..4152e3a4b6 100644 --- a/spec/services/vine/voucher_redeemer_service_spec.rb +++ b/spec/services/vine/voucher_redeemer_service_spec.rb @@ -58,14 +58,14 @@ RSpec.describe Vine::VoucherRedeemerService, feature: :connected_apps do it "redeems the voucher with VINE" do expect(vine_api_service).to receive(:voucher_redemptions) .with(voucher_id, voucher_set_id, 600) - .and_return(mock_api_response(success: true, data:)) + .and_return(mock_api_response(data:)) voucher_redeemer_service.redeem end it "closes the linked assement" do allow(vine_api_service).to receive(:voucher_redemptions) - .and_return(mock_api_response(success: true, data:)) + .and_return(mock_api_response(data:)) expect { voucher_redeemer_service.redeem @@ -74,7 +74,7 @@ RSpec.describe Vine::VoucherRedeemerService, feature: :connected_apps do it "returns true" do allow(vine_api_service).to receive(:voucher_redemptions) - .and_return(mock_api_response(success: true, data:)) + .and_return(mock_api_response(data:)) expect(voucher_redeemer_service.redeem).to be(true) end @@ -87,8 +87,7 @@ RSpec.describe Vine::VoucherRedeemerService, feature: :connected_apps do }.deep_stringify_keys } before do - allow(vine_api_service).to receive(:voucher_redemptions) - .and_return(mock_api_response(success: false, data:, status: 400)) + mock_api_exception(type: Faraday::BadRequestError, status: 400, body: data) end it "doesn't close the linked assement" do @@ -124,14 +123,6 @@ RSpec.describe Vine::VoucherRedeemerService, feature: :connected_apps do voucher_redeemer_service.redeem end - it "adds an error message" do - voucher_redeemer_service.redeem - - expect(voucher_redeemer_service.errors).to include( - { vine_settings: "No Vine api settings for the given enterprise" } - ) - end - it "doesn't close the linked assement" do expect { voucher_redeemer_service.redeem @@ -139,7 +130,6 @@ RSpec.describe Vine::VoucherRedeemerService, feature: :connected_apps do end end - # TODO should we set an error or just do nothing ? context "when there are no voucher added to the order" do let!(:vine_connected_app) { ConnectedApps::Vine.create( @@ -188,7 +178,7 @@ RSpec.describe Vine::VoucherRedeemerService, feature: :connected_apps do before do add_voucher(vine_voucher) - allow(vine_api_service).to receive(:voucher_redemptions).and_raise(Faraday::Error) + mock_api_exception(type: Faraday::ConnectionFailed) end it "returns false" do @@ -234,9 +224,7 @@ RSpec.describe Vine::VoucherRedeemerService, feature: :connected_apps do before do add_voucher(vine_voucher) - allow(vine_api_service).to receive(:voucher_redemptions).and_return( - mock_api_response(success: false, status: 401, data: ) - ) + mock_api_exception(type: Faraday::UnauthorizedError, status: 401, body: data) end it "returns false" do @@ -264,13 +252,17 @@ RSpec.describe Vine::VoucherRedeemerService, feature: :connected_apps do OrderManagement::Order::Updater.new(order).update_voucher 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_redemptions).and_raise(type.new(nil, + { status:, + body: }) ) + end end From 16d6e1f9355379119678780e1574ffb9b26dcdeb Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 18 Nov 2024 11:40:51 +1100 Subject: [PATCH 32/33] Remove unused error translation --- config/locales/en.yml | 2 -- 1 file changed, 2 deletions(-) diff --git a/config/locales/en.yml b/config/locales/en.yml index 649f4eabc4..360965ef4e 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -576,13 +576,11 @@ en: payment_could_not_complete: "The payment could not be completed" vine_voucher_validator_service: errors: - vine_settings: "This shop is not enabled for VINE Vouchers." vine_api: "There was an error communicating with the API, please try again later." invalid_voucher: "The voucher is not valid" not_found_voucher: "Sorry, we couldn't find that voucher, please check the code." vine_voucher_redeemer_service: errors: - vine_settings: "No Vine api settings for the given enterprise" vine_api: "There was an error communicating with the API" redeeming_failed: "Redeeming the voucher failed" actions: From f5b9ca361c44a78616b03b5ea05e0ebde924f886 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 18 Nov 2024 15:09:52 +1100 Subject: [PATCH 33/33] Use the voucher adjustment amount for redeeming --- app/services/vine/voucher_redeemer_service.rb | 13 +++++++------ spec/services/vine/voucher_redeemer_service_spec.rb | 5 +++-- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/app/services/vine/voucher_redeemer_service.rb b/app/services/vine/voucher_redeemer_service.rb index c9e0fa27ce..45aaa71244 100644 --- a/app/services/vine/voucher_redeemer_service.rb +++ b/app/services/vine/voucher_redeemer_service.rb @@ -11,16 +11,16 @@ module Vine def redeem # Do nothing if we don't have a vine voucher added to the order - voucher_adjustment = order.voucher_adjustments.first - @voucher = voucher_adjustment&.originator + @voucher_adjustment = order.voucher_adjustments.first + @voucher = @voucher_adjustment&.originator - return true if voucher_adjustment.nil? || !@voucher.is_a?(Vouchers::Vine) + return true if @voucher_adjustment.nil? || !@voucher.is_a?(Vouchers::Vine) return false if vine_settings.nil? call_vine_api - voucher_adjustment.close + @voucher_adjustment.close true rescue Faraday::ClientError => e @@ -44,9 +44,10 @@ module Vine jwt_service = Vine::JwtService.new(secret: vine_settings["secret"]) vine_api = Vine::ApiService.new(api_key: vine_settings["api_key"], jwt_generator: jwt_service) - # Voucher amount is stored in dollars, VINE expect cents + # Voucher adjustment amount is stored in dollars and negative, VINE expect cents + amount = -1 * @voucher_adjustment.amount * 100 vine_api.voucher_redemptions( - @voucher.external_voucher_id, @voucher.external_voucher_set_id, (@voucher.amount * 100) + @voucher.external_voucher_id, @voucher.external_voucher_set_id, amount ) end diff --git a/spec/services/vine/voucher_redeemer_service_spec.rb b/spec/services/vine/voucher_redeemer_service_spec.rb index 4152e3a4b6..3b12ad8d59 100644 --- a/spec/services/vine/voucher_redeemer_service_spec.rb +++ b/spec/services/vine/voucher_redeemer_service_spec.rb @@ -12,7 +12,7 @@ RSpec.describe Vine::VoucherRedeemerService, feature: :connected_apps do let(:vine_voucher) { create(:vine_voucher, code: 'some_code', enterprise: distributor, - amount: 6, external_voucher_id: voucher_id, + amount: 50, external_voucher_id: voucher_id, external_voucher_set_id: voucher_set_id ) } let(:voucher_id) { "9d316d27-0dad-411a-8953-316a1aaf7742" } @@ -56,8 +56,9 @@ RSpec.describe Vine::VoucherRedeemerService, feature: :connected_apps do before { add_voucher(vine_voucher) } it "redeems the voucher with VINE" do + # Order pre discount total is $10, so we expect to redeen 1000 cents expect(vine_api_service).to receive(:voucher_redemptions) - .with(voucher_id, voucher_set_id, 600) + .with(voucher_id, voucher_set_id, 1000) .and_return(mock_api_response(data:)) voucher_redeemer_service.redeem