diff --git a/app/controllers/api/v1/customers_controller.rb b/app/controllers/api/v1/customers_controller.rb index 7e56ab6b28..f1fcdddbf3 100644 --- a/app/controllers/api/v1/customers_controller.rb +++ b/app/controllers/api/v1/customers_controller.rb @@ -5,9 +5,10 @@ require 'open_food_network/permissions' module Api module V1 class CustomersController < Api::V1::BaseController + include AddressTransformation + skip_authorization_check only: :index - before_action :set_customer, only: [:show, :update, :destroy] before_action :authorize_action, only: [:show, :update, :destroy] def index @@ -17,44 +18,44 @@ module Api end def show - render json: Api::V1::CustomerSerializer.new(@customer, include_options) + render json: Api::V1::CustomerSerializer.new(customer, include_options) end def create authorize! :update, Enterprise.find(customer_params[:enterprise_id]) - @customer = Customer.new(customer_params) + customer = Customer.new(customer_params) - if @customer.save - render json: Api::V1::CustomerSerializer.new(@customer), status: :created + if customer.save + render json: Api::V1::CustomerSerializer.new(customer), status: :created else - invalid_resource! @customer + invalid_resource! customer end end def update - if @customer.update(customer_params) - render json: Api::V1::CustomerSerializer.new(@customer) + if customer.update(customer_params) + render json: Api::V1::CustomerSerializer.new(customer) else - invalid_resource! @customer + invalid_resource! customer end end def destroy - if @customer.destroy - render json: Api::V1::CustomerSerializer.new(@customer) + if customer.destroy + render json: Api::V1::CustomerSerializer.new(customer) else - invalid_resource! @customer + invalid_resource! customer end end private - def set_customer - @customer = Customer.find(params[:id]) + def customer + @customer ||= Customer.find(params[:id]) end def authorize_action - authorize! action_name.to_sym, @customer + authorize! action_name.to_sym, customer end def search_customers @@ -77,7 +78,8 @@ module Api :phone, :latitude, :longitude, :first_name, :last_name, :street_address_1, :street_address_2, - :postal_code, :locality, :region, :country, + :postal_code, :locality, + { region: [:name, :code], country: [:name, :code] }, ] ).to_h @@ -89,39 +91,6 @@ module Api attributes end - def transform_address!(attributes, from, to) - return unless attributes.key?(from) - - address = attributes.delete(from) - - if address.nil? - attributes[to] = nil - return - end - - address.transform_keys! do |key| - { - phone: :phone, latitude: :latitude, longitude: :longitude, - first_name: :firstname, last_name: :lastname, - street_address_1: :address1, street_address_2: :address2, - postal_code: :zipcode, - locality: :city, - region: :state_name, - country: :country, - }.with_indifferent_access[key] - end - - if address[:state_name].present? - address[:state] = Spree::State.find_by(name: address[:state_name]) - end - - if address[:country].present? - address[:country] = Spree::Country.find_by(name: address[:country]) - end - - attributes["#{to}_attributes"] = address - end - def editable_enterprises OpenFoodNetwork::Permissions.new(current_api_user).editable_enterprises.select(:id) end diff --git a/app/controllers/concerns/address_transformation.rb b/app/controllers/concerns/address_transformation.rb new file mode 100644 index 0000000000..e38ed1a60d --- /dev/null +++ b/app/controllers/concerns/address_transformation.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +# Our internal data structures are different to the API data strurctures. +module AddressTransformation + extend ActiveSupport::Concern + + def transform_address!(attributes, from, to) + return unless attributes.key?(from) + + address = attributes.delete(from) + + if address.nil? + attributes[to] = nil + return + end + + address.transform_keys! do |key| + { + phone: :phone, latitude: :latitude, longitude: :longitude, + first_name: :firstname, last_name: :lastname, + street_address_1: :address1, street_address_2: :address2, + postal_code: :zipcode, + locality: :city, + region: :state, + country: :country, + }.with_indifferent_access[key] + end + + address[:state] = find_state(address) if address[:state].present? + address[:country] = find_country(address) if address[:country].present? + + attributes["#{to}_attributes"] = address + end + + private + + def find_state(address) + Spree::State.find_by("LOWER(abbr) = ? OR LOWER(name) = ?", + address.dig(:state, :code)&.downcase, + address.dig(:state, :name)&.downcase) + end + + def find_country(address) + Spree::Country.find_by("LOWER(iso) = ? OR LOWER(name) = ?", + address.dig(:country, :code)&.downcase, + address.dig(:country, :name)&.downcase) + end +end diff --git a/app/json_schemas/customer_schema.rb b/app/json_schemas/customer_schema.rb index 553f4cd522..ecced41793 100644 --- a/app/json_schemas/customer_schema.rb +++ b/app/json_schemas/customer_schema.rb @@ -41,8 +41,8 @@ class CustomerSchema < JsonApiSchema street_address_2: "", postal_code: "1234", locality: "Melbourne", - region: "Victoria", - country: "Australia", + region: { code: "Vic", name: "Victoria" }, + country: { code: "AU", name: "Australia" }, } end diff --git a/app/serializers/api/v1/address_serializer.rb b/app/serializers/api/v1/address_serializer.rb index da53af79f2..72b800e960 100644 --- a/app/serializers/api/v1/address_serializer.rb +++ b/app/serializers/api/v1/address_serializer.rb @@ -11,8 +11,18 @@ module Api attribute :street_address_2, &:address2 attribute :postal_code, &:zipcode attribute :locality, &:city - attribute :region, &:state_name - attribute :country, ->(object, _) { object.country.name } + attribute :region do |object| + { + name: object.state.name, + code: object.state.abbr, + } + end + attribute :country do |object| + { + name: object.country.name, + code: object.country.iso, + } + end end end end diff --git a/spec/controllers/concerns/address_transformation_spec.rb b/spec/controllers/concerns/address_transformation_spec.rb new file mode 100644 index 0000000000..d8f9cf74c2 --- /dev/null +++ b/spec/controllers/concerns/address_transformation_spec.rb @@ -0,0 +1,97 @@ +# frozen_string_literal: true + +require "spec_helper" + +describe AddressTransformation do + include AddressTransformation + + describe "#transform_address!" do + describe "default cases" do + let(:attributes) do + { shipping_address: CustomerSchema.address_example, + billing_address: CustomerSchema.address_example } + end + + let(:wanted_address) do + CustomerSchema.address_example. + except( :first_name, :last_name, :locality, + :postal_code, :region, :street_address_1, :street_address_2). + merge( + firstname: "Alice", + lastname: "Springs", + address1: "1 Flinders Street", + address2: "", + zipcode: "1234", + city: "Melbourne", + state: Spree::State.find_by(name: "Victoria"), + country: Spree::Country.find_by(name: "Australia"), + ) + end + + it "transforms the shipping_address and the billing_address" do + transformed_address = transform_address!(attributes, :shipping_address, :ship_address) + expect(transformed_address).to eq(wanted_address) + expect(attributes["ship_address_attributes"]).to eq(wanted_address) + + transformed_address = transform_address!(attributes, :billing_address, :bill_address) + expect(transformed_address).to eq( wanted_address) + expect(attributes["bill_address_attributes"]).to eq(wanted_address) + end + end + + describe "when the address attributes are nil" do + let(:attributes) do + { shipping_address: nil, + billing_address: nil } + end + + it "returns nil" do + transformed_address = transform_address!(attributes, :shipping_address, :ship_address) + expect(transformed_address).to be_nil + expect(attributes["ship_address_attributes"]).to be_nil + + transformed_address = transform_address!(attributes, :billing_address, :bill_address) + expect(transformed_address).to be_nil + expect(attributes["bill_address_attributes"]).to be_nil + end + end + + describe "when attributes doesn't contains the searched key" do + let(:attributes) do + { + address: CustomerSchema.address_example, + } + end + + it "returns nil" do + transformed_address = transform_address!(attributes, :shipping_address, :ship_address) + expect(transformed_address).to be_nil + expect(attributes["ship_address_attributes"]).to be_nil + end + end + end + + describe "#find_state" do + it "finds the state" do + state = find_state( + state: { + code: "VIC", + name: "Victoria", + }, + ) + expect(state).to eq(Spree::State.find_by(name: "Victoria")) + end + end + + describe "#find_country" do + it "finds the country" do + country = find_country( + country: { + code: "AU", + name: "Australia", + }, + ) + expect(country).to eq(Spree::Country.find_by(name: "Australia")) + end + end +end diff --git a/spec/requests/api/v1/customers_spec.rb b/spec/requests/api/v1/customers_spec.rb index 58ae41945d..047c5e0438 100644 --- a/spec/requests/api/v1/customers_spec.rb +++ b/spec/requests/api/v1/customers_spec.rb @@ -302,6 +302,59 @@ describe "Customers", type: :request do end end + describe "address" do + it "matches by country and state code" do + put "/api/v1/customers/#{customer1.id}", params: { + customer: { + shipping_address: CustomerSchema.address_example.merge( + region: { code: "Vic" }, + country: { code: "AU" }, + ), + } + } + + expect(response).to have_http_status :ok + expect(json_response.dig(:data, :attributes, :shipping_address)).to include( + region: { code: "Vic", name: "Victoria" }, + country: { code: "AU", name: "Australia" }, + ) + end + + it "matches by country and state name" do + put "/api/v1/customers/#{customer1.id}", params: { + customer: { + shipping_address: CustomerSchema.address_example.merge( + region: { name: "Victoria" }, + country: { name: "Australia" }, + ), + } + } + + expect(response).to have_http_status :ok + expect(json_response.dig(:data, :attributes, :shipping_address)).to include( + region: { code: "Vic", name: "Victoria" }, + country: { code: "AU", name: "Australia" }, + ) + end + + it "matches country and state case-insensitive" do + put "/api/v1/customers/#{customer1.id}", params: { + customer: { + shipping_address: CustomerSchema.address_example.merge( + region: { code: "VIC" }, + country: { name: "AUSTRALIA" }, + ), + } + } + + expect(response).to have_http_status :ok + expect(json_response.dig(:data, :attributes, :shipping_address)).to include( + region: { code: "Vic", name: "Victoria" }, + country: { code: "AU", name: "Australia" }, + ) + end + end + response "422", "Unprocessable entity" do param(:id) { customer1.id } param(:customer) { {} } diff --git a/swagger/v1/swagger.yaml b/swagger/v1/swagger.yaml index 911fa13e74..2909306e6e 100644 --- a/swagger/v1/swagger.yaml +++ b/swagger/v1/swagger.yaml @@ -91,8 +91,12 @@ components: street_address_2: '' postal_code: '1234' locality: Melbourne - region: Victoria - country: Australia + region: + code: Vic + name: Victoria + country: + code: AU + name: Australia required: - id - enterprise_id @@ -200,8 +204,12 @@ components: street_address_2: '' postal_code: '1234' locality: Melbourne - region: Victoria - country: Australia + region: + code: Vic + name: Victoria + country: + code: AU + name: Australia required: - id - enterprise_id @@ -372,8 +380,12 @@ paths: street_address_2: '' postal_code: '1234' locality: Melbourne - region: Victoria - country: Australia + region: + code: Vic + name: Victoria + country: + code: AU + name: Australia required: - enterprise_id - email @@ -478,8 +490,12 @@ paths: street_address_2: '' postal_code: '1234' locality: Melbourne - region: Victoria - country: Australia + region: + code: Vic + name: Victoria + country: + code: AU + name: Australia required: - enterprise_id - email