From 3baed683b131a3f126c99d74db37180a5fa5b0ac Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 2 Mar 2022 11:54:31 +1100 Subject: [PATCH 01/11] Add allow_charges attribute to customers endpoint --- app/json_schemas/customer_schema.rb | 3 ++- app/serializers/api/v1/customer_serializer.rb | 3 ++- swagger/v1/swagger.yaml | 14 ++++++++++++++ 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/app/json_schemas/customer_schema.rb b/app/json_schemas/customer_schema.rb index 4652ae3bbf..8da8b67e2b 100644 --- a/app/json_schemas/customer_schema.rb +++ b/app/json_schemas/customer_schema.rb @@ -12,7 +12,8 @@ class CustomerSchema < JsonApiSchema first_name: { type: :string, nullable: true, example: "Alice" }, last_name: { type: :string, nullable: true, example: "Springs" }, code: { type: :string, nullable: true, example: "BUYER1" }, - email: { type: :string, example: "alice@example.com" } + email: { type: :string, example: "alice@example.com" }, + allow_charges: { type: :boolean, example: false }, } end diff --git a/app/serializers/api/v1/customer_serializer.rb b/app/serializers/api/v1/customer_serializer.rb index 6f71b39159..1cecc7ee23 100644 --- a/app/serializers/api/v1/customer_serializer.rb +++ b/app/serializers/api/v1/customer_serializer.rb @@ -3,7 +3,8 @@ module Api module V1 class CustomerSerializer < BaseSerializer - attributes :id, :enterprise_id, :first_name, :last_name, :code, :email + attributes :id, :enterprise_id, :first_name, :last_name, :code, :email, + :allow_charges belongs_to :enterprise, links: { related: ->(object) { diff --git a/swagger/v1/swagger.yaml b/swagger/v1/swagger.yaml index 9b2642959d..aec3ce9759 100644 --- a/swagger/v1/swagger.yaml +++ b/swagger/v1/swagger.yaml @@ -60,6 +60,9 @@ components: email: type: string example: alice@example.com + allow_charges: + type: boolean + example: false required: - id - enterprise_id @@ -67,6 +70,7 @@ components: - last_name - code - email + - allow_charges relationships: type: object properties: @@ -130,6 +134,9 @@ components: email: type: string example: alice@example.com + allow_charges: + type: boolean + example: false required: - id - enterprise_id @@ -137,6 +144,7 @@ components: - last_name - code - email + - allow_charges relationships: type: object properties: @@ -271,6 +279,9 @@ paths: email: type: string example: alice@example.com + allow_charges: + type: boolean + example: false required: - enterprise_id - email @@ -354,6 +365,9 @@ paths: email: type: string example: alice@example.com + allow_charges: + type: boolean + example: false required: - enterprise_id - email From 41746459fadfdd134f061d09dbce06b2985165b8 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Thu, 3 Mar 2022 12:20:45 +1100 Subject: [PATCH 02/11] Restrict allow_charges attribute to read-only We want people to use the UI to change this attribute. --- app/controllers/api/v1/base_controller.rb | 9 +++++ .../concerns/raising_parameters.rb | 23 +++++++++++ app/json_schemas/customer_schema.rb | 7 ++++ config/locales/en.yml | 1 + .../concerns/raising_parameters_spec.rb | 40 +++++++++++++++++++ spec/requests/api/v1/customers_spec.rb | 26 ++++++++++-- swagger/v1/swagger.yaml | 9 ----- 7 files changed, 102 insertions(+), 13 deletions(-) create mode 100644 app/controllers/concerns/raising_parameters.rb create mode 100644 spec/controllers/concerns/raising_parameters_spec.rb diff --git a/app/controllers/api/v1/base_controller.rb b/app/controllers/api/v1/base_controller.rb index aca1c09e55..4402873e69 100644 --- a/app/controllers/api/v1/base_controller.rb +++ b/app/controllers/api/v1/base_controller.rb @@ -7,6 +7,7 @@ module Api include RequestTimeouts include Pagy::Backend include JsonApiPagination + include RaisingParameters check_authorization @@ -18,6 +19,7 @@ module Api rescue_from CanCan::AccessDenied, with: :unauthorized rescue_from ActiveRecord::RecordNotFound, with: :not_found rescue_from Pagy::VariableError, with: :invalid_pagination + rescue_from ActionController::UnpermittedParameters, with: :unpermitted_parameters private @@ -60,6 +62,13 @@ module Api json: json_api_error(exception.message) end + def unpermitted_parameters(error) + message = I18n.t(:unpermitted_parameters, params: error.params.join(", "), scope: :api) + + render status: :unprocessable_entity, + json: json_api_error(message) + end + def invalid_resource!(resource = nil) render status: :unprocessable_entity, json: json_api_invalid( diff --git a/app/controllers/concerns/raising_parameters.rb b/app/controllers/concerns/raising_parameters.rb new file mode 100644 index 0000000000..d3f11c90c2 --- /dev/null +++ b/app/controllers/concerns/raising_parameters.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +# The API uses strict parameter checking. +# +# We want to raise errors when unused or unpermitted parameters are given +# to the API. You then know straight away when a parameter isn't used. +module RaisingParameters + extend ActiveSupport::Concern + + # ActionController manages this config on a per-class basis. The subclass + # enables us to raise errors only here and not in the rest of the app. + class Parameters < ActionController::Parameters + def self.action_on_unpermitted_parameters + :raise + end + end + + # We override the params method so that we always use the strict parameters. + # We could rename this method if we need access to the orginal as well. + def params + Parameters.new(super.to_unsafe_hash) + end +end diff --git a/app/json_schemas/customer_schema.rb b/app/json_schemas/customer_schema.rb index 8da8b67e2b..3281d12d90 100644 --- a/app/json_schemas/customer_schema.rb +++ b/app/json_schemas/customer_schema.rb @@ -21,6 +21,13 @@ class CustomerSchema < JsonApiSchema [:enterprise_id, :email] end + def self.writable_attributes + attributes.except( + :id, + :allow_charges, + ) + end + def self.relationships [:enterprise] end diff --git a/config/locales/en.yml b/config/locales/en.yml index 7bee8133c7..56dc356d3b 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1412,6 +1412,7 @@ en: unknown_error: "Something went wrong. Our team has been notified." invalid_api_key: "Invalid API key (%{key}) specified." unauthorized: "You are not authorized to perform that action." + unpermitted_parameters: "Parameters not allowed in this request: %{params}" invalid_resource: "Invalid resource. Please fix errors and try again." resource_not_found: "The resource you were looking for could not be found." enterprise_logo: diff --git a/spec/controllers/concerns/raising_parameters_spec.rb b/spec/controllers/concerns/raising_parameters_spec.rb new file mode 100644 index 0000000000..cf330221b7 --- /dev/null +++ b/spec/controllers/concerns/raising_parameters_spec.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe RaisingParameters do + describe "Parameters" do + let(:params) do + RaisingParameters::Parameters.new( + controller: "example", + action: "update", + data: { + id: "unique", + admin: true, + } + ) + end + + it "raises an error when a parameter is not permitted" do + expect { + params.require(:data).permit(:id) + }.to raise_error( + ActionController::UnpermittedParameters + ) + end + + it "raises no error when all parameters are permitted" do + expect { + params.require(:data).permit(:id, :admin) + }.to_not raise_error + end + + it "doesn't change standard parameter objects" do + original_params = ActionController::Parameters.new(one: 1, two: 2) + + expect { + original_params.permit(:one) + }.to_not raise_error + end + end +end diff --git a/spec/requests/api/v1/customers_spec.rb b/spec/requests/api/v1/customers_spec.rb index 7c8519a4b0..2167d1c953 100644 --- a/spec/requests/api/v1/customers_spec.rb +++ b/spec/requests/api/v1/customers_spec.rb @@ -101,7 +101,7 @@ describe "Customers", type: :request do parameter name: :customer, in: :body, schema: { type: :object, - properties: CustomerSchema.attributes.except(:id), + properties: CustomerSchema.writable_attributes, required: CustomerSchema.required_attributes } @@ -114,7 +114,26 @@ describe "Customers", type: :request do end schema "$ref": "#/components/schemas/resources/customer" - run_test! + run_test! do + expect(json_response[:data][:attributes]).to include( + allow_charges: false + ) + end + end + + response "422", "Unpermitted parameter" do + param(:customer) do + { + email: "test@example.com", + enterprise_id: enterprise1.id.to_s, + allow_charges: true, + } + end + schema "$ref": "#/components/schemas/error_response" + + run_test! do + expect(json_error_detail).to eq "Parameters not allowed in this request: allow_charges" + end end response "422", "Unprocessable entity" do @@ -187,7 +206,7 @@ describe "Customers", type: :request do parameter name: :customer, in: :body, schema: { type: :object, - properties: CustomerSchema.attributes, + properties: CustomerSchema.writable_attributes, required: CustomerSchema.required_attributes } @@ -195,7 +214,6 @@ describe "Customers", type: :request do param(:id) { customer1.id } param(:customer) do { - id: customer1.id.to_s, email: "test@example.com", enterprise_id: enterprise1.id.to_s } diff --git a/swagger/v1/swagger.yaml b/swagger/v1/swagger.yaml index aec3ce9759..207673ebf0 100644 --- a/swagger/v1/swagger.yaml +++ b/swagger/v1/swagger.yaml @@ -279,9 +279,6 @@ paths: email: type: string example: alice@example.com - allow_charges: - type: boolean - example: false required: - enterprise_id - email @@ -344,9 +341,6 @@ paths: schema: type: object properties: - id: - type: integer - example: 1 enterprise_id: type: integer example: 2 @@ -365,9 +359,6 @@ paths: email: type: string example: alice@example.com - allow_charges: - type: boolean - example: false required: - enterprise_id - email From 8d12c7a6929e5ea9d30bf99416a37e246049588d Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Mon, 7 Mar 2022 11:05:43 +1100 Subject: [PATCH 03/11] Permit more customer attributes for update --- .../api/v1/customers_controller.rb | 5 +++- spec/requests/api/v1/customers_spec.rb | 23 ++++++++++++++++++- swagger/v1/swagger.yaml | 2 +- 3 files changed, 27 insertions(+), 3 deletions(-) diff --git a/app/controllers/api/v1/customers_controller.rb b/app/controllers/api/v1/customers_controller.rb index 6c403ce2d4..bf3e035d1b 100644 --- a/app/controllers/api/v1/customers_controller.rb +++ b/app/controllers/api/v1/customers_controller.rb @@ -70,7 +70,10 @@ module Api end def customer_params - params.require(:customer).permit(:email, :enterprise_id) + params.require(:customer).permit( + :email, :enterprise_id, + :code, :first_name, :last_name, + ) end def editable_enterprises diff --git a/spec/requests/api/v1/customers_spec.rb b/spec/requests/api/v1/customers_spec.rb index 2167d1c953..d4d7a235b5 100644 --- a/spec/requests/api/v1/customers_spec.rb +++ b/spec/requests/api/v1/customers_spec.rb @@ -105,7 +105,7 @@ describe "Customers", type: :request do required: CustomerSchema.required_attributes } - response "201", "Customer created" do + response "201", "Minimal customer created" do param(:customer) do { email: "test@example.com", @@ -121,6 +121,27 @@ describe "Customers", type: :request do end end + response "201", "Example customer created" do + param(:customer) do + CustomerSchema.writable_attributes.transform_values do |attribute| + attribute[:example] + end.merge( + enterprise_id: enterprise1.id, + ) + end + schema "$ref": "#/components/schemas/resources/customer" + + run_test! do + expect(json_response[:data][:attributes]).to include( + first_name: "Alice", + last_name: "Springs", + code: "BUYER1", + email: "alice@example.com", + enterprise_id: enterprise1.id, + ) + end + end + response "422", "Unpermitted parameter" do param(:customer) do { diff --git a/swagger/v1/swagger.yaml b/swagger/v1/swagger.yaml index 207673ebf0..03365ac195 100644 --- a/swagger/v1/swagger.yaml +++ b/swagger/v1/swagger.yaml @@ -244,7 +244,7 @@ paths: parameters: [] responses: '201': - description: Customer created + description: Example customer created content: application/json: schema: From aa6e5ae79977e41f3a9fc9ccc98baeaec4660774 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Thu, 3 Mar 2022 14:07:04 +1100 Subject: [PATCH 04/11] Report useful error message in missing parameter --- app/controllers/api/v1/base_controller.rb | 8 ++++++++ config/locales/en.yml | 1 + spec/requests/api/v1/customers_spec.rb | 5 ++++- 3 files changed, 13 insertions(+), 1 deletion(-) diff --git a/app/controllers/api/v1/base_controller.rb b/app/controllers/api/v1/base_controller.rb index 4402873e69..39f9e1684b 100644 --- a/app/controllers/api/v1/base_controller.rb +++ b/app/controllers/api/v1/base_controller.rb @@ -19,6 +19,7 @@ module Api rescue_from CanCan::AccessDenied, with: :unauthorized rescue_from ActiveRecord::RecordNotFound, with: :not_found rescue_from Pagy::VariableError, with: :invalid_pagination + rescue_from ActionController::ParameterMissing, with: :missing_parameter rescue_from ActionController::UnpermittedParameters, with: :unpermitted_parameters private @@ -62,6 +63,13 @@ module Api json: json_api_error(exception.message) end + def missing_parameter(error) + message = I18n.t(:missing_parameter, param: error.param, scope: :api) + + render status: :unprocessable_entity, + json: json_api_error(message) + end + def unpermitted_parameters(error) message = I18n.t(:unpermitted_parameters, params: error.params.join(", "), scope: :api) diff --git a/config/locales/en.yml b/config/locales/en.yml index 56dc356d3b..7fea74e856 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1413,6 +1413,7 @@ en: invalid_api_key: "Invalid API key (%{key}) specified." unauthorized: "You are not authorized to perform that action." unpermitted_parameters: "Parameters not allowed in this request: %{params}" + missing_parameter: "A required parameter is missing or empty: %{param}" invalid_resource: "Invalid resource. Please fix errors and try again." resource_not_found: "The resource you were looking for could not be found." enterprise_logo: diff --git a/spec/requests/api/v1/customers_spec.rb b/spec/requests/api/v1/customers_spec.rb index d4d7a235b5..a6a620b398 100644 --- a/spec/requests/api/v1/customers_spec.rb +++ b/spec/requests/api/v1/customers_spec.rb @@ -161,7 +161,10 @@ describe "Customers", type: :request do param(:customer) { {} } schema "$ref": "#/components/schemas/error_response" - run_test! + run_test! do + expect(json_error_detail).to eq "A required parameter is missing or empty: customer" + expect(json_response[:meta]).to eq nil + end end end end From e5e8953a094a9e4a7abc853adefe14ec9269bbe4 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Thu, 3 Mar 2022 16:44:31 +1100 Subject: [PATCH 05/11] Add ToS acceptance to customer endpoint It's another read-only attribute. Please note that JSON:API specifies a format of `date-time` which we don't adhere to because it uses a `Z` in front of the timezone offset which doesn't seem to be included in the default json serialisation. So I didn't add the format and left it as simple `string`. Problem? --- app/json_schemas/customer_schema.rb | 5 +++++ app/serializers/api/v1/customer_serializer.rb | 2 +- spec/requests/api/v1/customers_spec.rb | 20 ++++++++++++++++--- swagger/v1/swagger.yaml | 12 +++++++++++ 4 files changed, 35 insertions(+), 4 deletions(-) diff --git a/app/json_schemas/customer_schema.rb b/app/json_schemas/customer_schema.rb index 3281d12d90..5c791e75a2 100644 --- a/app/json_schemas/customer_schema.rb +++ b/app/json_schemas/customer_schema.rb @@ -14,6 +14,10 @@ class CustomerSchema < JsonApiSchema code: { type: :string, nullable: true, example: "BUYER1" }, email: { type: :string, example: "alice@example.com" }, allow_charges: { type: :boolean, example: false }, + terms_and_conditions_accepted_at: { + type: :string, format: "date-time", nullable: true, + example: "2022-03-12T15:55:00.000+11:00", + }, } end @@ -25,6 +29,7 @@ class CustomerSchema < JsonApiSchema attributes.except( :id, :allow_charges, + :terms_and_conditions_accepted_at, ) end diff --git a/app/serializers/api/v1/customer_serializer.rb b/app/serializers/api/v1/customer_serializer.rb index 1cecc7ee23..1325d7c615 100644 --- a/app/serializers/api/v1/customer_serializer.rb +++ b/app/serializers/api/v1/customer_serializer.rb @@ -4,7 +4,7 @@ module Api module V1 class CustomerSerializer < BaseSerializer attributes :id, :enterprise_id, :first_name, :last_name, :code, :email, - :allow_charges + :allow_charges, :terms_and_conditions_accepted_at belongs_to :enterprise, links: { related: ->(object) { diff --git a/spec/requests/api/v1/customers_spec.rb b/spec/requests/api/v1/customers_spec.rb index a6a620b398..fa4332631c 100644 --- a/spec/requests/api/v1/customers_spec.rb +++ b/spec/requests/api/v1/customers_spec.rb @@ -5,7 +5,13 @@ require "swagger_helper" describe "Customers", type: :request do let!(:enterprise1) { create(:enterprise) } let!(:enterprise2) { create(:enterprise) } - let!(:customer1) { create(:customer, enterprise: enterprise1) } + let!(:customer1) { + create( + :customer, + enterprise: enterprise1, + terms_and_conditions_accepted_at: Time.zone.parse("2000-01-01"), + ) + } let!(:customer2) { create(:customer, enterprise: enterprise1) } let!(:customer3) { create(:customer, enterprise: enterprise2) } @@ -116,7 +122,8 @@ describe "Customers", type: :request do run_test! do expect(json_response[:data][:attributes]).to include( - allow_charges: false + allow_charges: false, + terms_and_conditions_accepted_at: nil, ) end end @@ -179,7 +186,14 @@ describe "Customers", type: :request do param(:id) { customer1.id } schema "$ref": "#/components/schemas/resources/customer" - run_test! + run_test! do + date_time_string = + json_response[:data][:attributes][:terms_and_conditions_accepted_at] + expect(date_time_string).to match /^2000-01-01T00:00:00.000[Z+-].*$/ + expect(DateTime.parse(date_time_string)).to eq( + customer1.terms_and_conditions_accepted_at + ) + end end response "404", "Not found" do diff --git a/swagger/v1/swagger.yaml b/swagger/v1/swagger.yaml index 03365ac195..d703156f03 100644 --- a/swagger/v1/swagger.yaml +++ b/swagger/v1/swagger.yaml @@ -63,6 +63,11 @@ components: allow_charges: type: boolean example: false + terms_and_conditions_accepted_at: + type: string + format: date-time + nullable: true + example: '2022-03-12T15:55:00.000+11:00' required: - id - enterprise_id @@ -71,6 +76,7 @@ components: - code - email - allow_charges + - terms_and_conditions_accepted_at relationships: type: object properties: @@ -137,6 +143,11 @@ components: allow_charges: type: boolean example: false + terms_and_conditions_accepted_at: + type: string + format: date-time + nullable: true + example: '2022-03-12T15:55:00.000+11:00' required: - id - enterprise_id @@ -145,6 +156,7 @@ components: - code - email - allow_charges + - terms_and_conditions_accepted_at relationships: type: object properties: From adc7e97e625c7f4f0fb39efe292bc3305ac26319 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Mon, 7 Mar 2022 12:38:53 +1100 Subject: [PATCH 06/11] Add tag list to customer endpoint --- .../api/v1/customers_controller.rb | 8 +++++-- app/json_schemas/customer_schema.rb | 1 + app/serializers/api/v1/customer_serializer.rb | 2 ++ spec/requests/api/v1/customers_spec.rb | 9 +++++++- swagger/v1/swagger.yaml | 22 +++++++++++++++++++ 5 files changed, 39 insertions(+), 3 deletions(-) diff --git a/app/controllers/api/v1/customers_controller.rb b/app/controllers/api/v1/customers_controller.rb index bf3e035d1b..7f25a3127a 100644 --- a/app/controllers/api/v1/customers_controller.rb +++ b/app/controllers/api/v1/customers_controller.rb @@ -70,10 +70,14 @@ module Api end def customer_params - params.require(:customer).permit( + attributes = params.require(:customer).permit( :email, :enterprise_id, :code, :first_name, :last_name, - ) + ).to_h + + attributes.merge!(tag_list: params[:tags]) if params.key?(:tags) + + attributes end def editable_enterprises diff --git a/app/json_schemas/customer_schema.rb b/app/json_schemas/customer_schema.rb index 5c791e75a2..185cb07f37 100644 --- a/app/json_schemas/customer_schema.rb +++ b/app/json_schemas/customer_schema.rb @@ -14,6 +14,7 @@ class CustomerSchema < JsonApiSchema code: { type: :string, nullable: true, example: "BUYER1" }, email: { type: :string, example: "alice@example.com" }, allow_charges: { type: :boolean, example: false }, + tags: { type: :array, example: ["staff", "discount"] }, terms_and_conditions_accepted_at: { type: :string, format: "date-time", nullable: true, example: "2022-03-12T15:55:00.000+11:00", diff --git a/app/serializers/api/v1/customer_serializer.rb b/app/serializers/api/v1/customer_serializer.rb index 1325d7c615..6e2edc8f04 100644 --- a/app/serializers/api/v1/customer_serializer.rb +++ b/app/serializers/api/v1/customer_serializer.rb @@ -6,6 +6,8 @@ module Api attributes :id, :enterprise_id, :first_name, :last_name, :code, :email, :allow_charges, :terms_and_conditions_accepted_at + attribute :tags, &:tag_list + belongs_to :enterprise, links: { related: ->(object) { url_helpers.api_v1_enterprise_url(id: object.enterprise_id) diff --git a/spec/requests/api/v1/customers_spec.rb b/spec/requests/api/v1/customers_spec.rb index fa4332631c..f87a299429 100644 --- a/spec/requests/api/v1/customers_spec.rb +++ b/spec/requests/api/v1/customers_spec.rb @@ -10,6 +10,7 @@ describe "Customers", type: :request do :customer, enterprise: enterprise1, terms_and_conditions_accepted_at: Time.zone.parse("2000-01-01"), + tag_list: ["long-term"], ) } let!(:customer2) { create(:customer, enterprise: enterprise1) } @@ -145,6 +146,7 @@ describe "Customers", type: :request do code: "BUYER1", email: "alice@example.com", enterprise_id: enterprise1.id, + tags: ["staff", "discount"], ) end end @@ -258,7 +260,12 @@ describe "Customers", type: :request do end schema "$ref": "#/components/schemas/resources/customer" - run_test! + run_test! do + # Tags should not be overridden when the param is missing: + expect(json_response[:data][:attributes]).to include( + tags: ["long-term"], + ) + end end response "422", "Unprocessable entity" do diff --git a/swagger/v1/swagger.yaml b/swagger/v1/swagger.yaml index d703156f03..9b7b0442c3 100644 --- a/swagger/v1/swagger.yaml +++ b/swagger/v1/swagger.yaml @@ -63,6 +63,11 @@ components: allow_charges: type: boolean example: false + tags: + type: array + example: + - staff + - discount terms_and_conditions_accepted_at: type: string format: date-time @@ -76,6 +81,7 @@ components: - code - email - allow_charges + - tags - terms_and_conditions_accepted_at relationships: type: object @@ -143,6 +149,11 @@ components: allow_charges: type: boolean example: false + tags: + type: array + example: + - staff + - discount terms_and_conditions_accepted_at: type: string format: date-time @@ -156,6 +167,7 @@ components: - code - email - allow_charges + - tags - terms_and_conditions_accepted_at relationships: type: object @@ -291,6 +303,11 @@ paths: email: type: string example: alice@example.com + tags: + type: array + example: + - staff + - discount required: - enterprise_id - email @@ -371,6 +388,11 @@ paths: email: type: string example: alice@example.com + tags: + type: array + example: + - staff + - discount required: - enterprise_id - email From 51420934f3bcbba30335946dbd50452ff57e9db9 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Mon, 7 Mar 2022 15:49:36 +1100 Subject: [PATCH 07/11] Add billing and shipping address to customer --- .rubocop_styleguide.yml | 5 +++ .../api/v1/customers_controller.rb | 2 +- app/json_schemas/customer_schema.rb | 25 +++++++++++ app/serializers/api/v1/address_serializer.rb | 18 ++++++++ app/serializers/api/v1/customer_serializer.rb | 12 ++++++ spec/requests/api/v1/customers_spec.rb | 1 + swagger/v1/swagger.yaml | 42 +++++++++++++++++++ 7 files changed, 104 insertions(+), 1 deletion(-) create mode 100644 app/serializers/api/v1/address_serializer.rb diff --git a/.rubocop_styleguide.yml b/.rubocop_styleguide.yml index 500b642c70..1cf9623b42 100644 --- a/.rubocop_styleguide.yml +++ b/.rubocop_styleguide.yml @@ -79,6 +79,11 @@ Lint/RaiseException: Lint/StructNewOverride: Enabled: true +Naming/VariableNumber: + AllowedIdentifiers: + - street_address_1 + - street_address_2 + Bundler/DuplicatedGem: Enabled: false diff --git a/app/controllers/api/v1/customers_controller.rb b/app/controllers/api/v1/customers_controller.rb index 7f25a3127a..9a2ee3e325 100644 --- a/app/controllers/api/v1/customers_controller.rb +++ b/app/controllers/api/v1/customers_controller.rb @@ -58,7 +58,7 @@ module Api end def search_customers - customers = visible_customers + customers = visible_customers.includes(:bill_address, :ship_address) customers = customers.where(enterprise_id: params[:enterprise_id]) if params[:enterprise_id] customers.ransack(params[:q]).result end diff --git a/app/json_schemas/customer_schema.rb b/app/json_schemas/customer_schema.rb index 185cb07f37..9f8b2a79f4 100644 --- a/app/json_schemas/customer_schema.rb +++ b/app/json_schemas/customer_schema.rb @@ -19,6 +19,30 @@ class CustomerSchema < JsonApiSchema type: :string, format: "date-time", nullable: true, example: "2022-03-12T15:55:00.000+11:00", }, + billing_address: { + type: :object, nullable: true, + example: nil, + }, + shipping_address: { + type: :object, nullable: true, + example: address_example, + }, + } + end + + def self.address_example + { + phone: "0404 333 222 111", + latitude: -37.817375100000, + longitude: 144.964803195704, + first_name: "Alice", + last_name: "Springs", + street_address_1: "1 Flinders Street", + street_address_2: "", + postal_code: "1234", + locality: "Melbourne", + region: "Victoria", + country: "Australia", } end @@ -31,6 +55,7 @@ class CustomerSchema < JsonApiSchema :id, :allow_charges, :terms_and_conditions_accepted_at, + :billing_address, :shipping_address, ) end diff --git a/app/serializers/api/v1/address_serializer.rb b/app/serializers/api/v1/address_serializer.rb new file mode 100644 index 0000000000..da53af79f2 --- /dev/null +++ b/app/serializers/api/v1/address_serializer.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +module Api + module V1 + class AddressSerializer < BaseSerializer + attributes :phone, :latitude, :longitude + + attribute :first_name, &:firstname + attribute :last_name, &:lastname + attribute :street_address_1, &:address1 + attribute :street_address_2, &:address2 + attribute :postal_code, &:zipcode + attribute :locality, &:city + attribute :region, &:state_name + attribute :country, ->(object, _) { object.country.name } + end + end +end diff --git a/app/serializers/api/v1/customer_serializer.rb b/app/serializers/api/v1/customer_serializer.rb index 6e2edc8f04..b5e59648b5 100644 --- a/app/serializers/api/v1/customer_serializer.rb +++ b/app/serializers/api/v1/customer_serializer.rb @@ -8,11 +8,23 @@ module Api attribute :tags, &:tag_list + attribute :billing_address do |object| + address(object.billing_address) + end + + attribute :shipping_address do |object| + address(object.shipping_address) + end + belongs_to :enterprise, links: { related: ->(object) { url_helpers.api_v1_enterprise_url(id: object.enterprise_id) } } + + def self.address(record) + AddressSerializer.new(record).serializable_hash.dig(:data, :attributes) + end end end end diff --git a/spec/requests/api/v1/customers_spec.rb b/spec/requests/api/v1/customers_spec.rb index f87a299429..3a47110e11 100644 --- a/spec/requests/api/v1/customers_spec.rb +++ b/spec/requests/api/v1/customers_spec.rb @@ -11,6 +11,7 @@ describe "Customers", type: :request do enterprise: enterprise1, terms_and_conditions_accepted_at: Time.zone.parse("2000-01-01"), tag_list: ["long-term"], + ship_address: create(:address), ) } let!(:customer2) { create(:customer, enterprise: enterprise1) } diff --git a/swagger/v1/swagger.yaml b/swagger/v1/swagger.yaml index 9b7b0442c3..84f42e1eed 100644 --- a/swagger/v1/swagger.yaml +++ b/swagger/v1/swagger.yaml @@ -73,6 +73,25 @@ components: format: date-time nullable: true example: '2022-03-12T15:55:00.000+11:00' + billing_address: + type: object + nullable: true + example: + shipping_address: + type: object + nullable: true + example: + phone: 0404 333 222 111 + latitude: -37.8173751 + longitude: 144.964803195704 + first_name: Alice + last_name: Springs + street_address_1: 1 Flinders Street + street_address_2: '' + postal_code: '1234' + locality: Melbourne + region: Victoria + country: Australia required: - id - enterprise_id @@ -83,6 +102,8 @@ components: - allow_charges - tags - terms_and_conditions_accepted_at + - billing_address + - shipping_address relationships: type: object properties: @@ -159,6 +180,25 @@ components: format: date-time nullable: true example: '2022-03-12T15:55:00.000+11:00' + billing_address: + type: object + nullable: true + example: + shipping_address: + type: object + nullable: true + example: + phone: 0404 333 222 111 + latitude: -37.8173751 + longitude: 144.964803195704 + first_name: Alice + last_name: Springs + street_address_1: 1 Flinders Street + street_address_2: '' + postal_code: '1234' + locality: Melbourne + region: Victoria + country: Australia required: - id - enterprise_id @@ -169,6 +209,8 @@ components: - allow_charges - tags - terms_and_conditions_accepted_at + - billing_address + - shipping_address relationships: type: object properties: From d789fb32e9fd7e6189bdc57dcec22f112347fe3c Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Thu, 24 Mar 2022 17:04:36 +1100 Subject: [PATCH 08/11] Make customer's address writable on API --- .../api/v1/customers_controller.rb | 42 +++++++++++++++++++ app/json_schemas/customer_schema.rb | 1 - spec/requests/api/v1/customers_spec.rb | 1 + swagger/v1/swagger.yaml | 38 +++++++++++++++++ 4 files changed, 81 insertions(+), 1 deletion(-) diff --git a/app/controllers/api/v1/customers_controller.rb b/app/controllers/api/v1/customers_controller.rb index 9a2ee3e325..36b1f65968 100644 --- a/app/controllers/api/v1/customers_controller.rb +++ b/app/controllers/api/v1/customers_controller.rb @@ -73,13 +73,55 @@ module Api attributes = params.require(:customer).permit( :email, :enterprise_id, :code, :first_name, :last_name, + :billing_address, shipping_address: [ + :phone, :latitude, :longitude, + :first_name, :last_name, + :street_address_1, :street_address_2, + :postal_code, :locality, :region, :country, + ] ).to_h attributes.merge!(tag_list: params[:tags]) if params.key?(:tags) + transform_address!(attributes, :billing_address, :bill_address) + transform_address!(attributes, :shipping_address, :ship_address) + 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/json_schemas/customer_schema.rb b/app/json_schemas/customer_schema.rb index 9f8b2a79f4..45529f19e3 100644 --- a/app/json_schemas/customer_schema.rb +++ b/app/json_schemas/customer_schema.rb @@ -55,7 +55,6 @@ class CustomerSchema < JsonApiSchema :id, :allow_charges, :terms_and_conditions_accepted_at, - :billing_address, :shipping_address, ) end diff --git a/spec/requests/api/v1/customers_spec.rb b/spec/requests/api/v1/customers_spec.rb index 3a47110e11..090c38545b 100644 --- a/spec/requests/api/v1/customers_spec.rb +++ b/spec/requests/api/v1/customers_spec.rb @@ -148,6 +148,7 @@ describe "Customers", type: :request do email: "alice@example.com", enterprise_id: enterprise1.id, tags: ["staff", "discount"], + shipping_address: CustomerSchema.address_example ) end end diff --git a/swagger/v1/swagger.yaml b/swagger/v1/swagger.yaml index 84f42e1eed..c67e71b8a9 100644 --- a/swagger/v1/swagger.yaml +++ b/swagger/v1/swagger.yaml @@ -350,6 +350,25 @@ paths: example: - staff - discount + billing_address: + type: object + nullable: true + example: + shipping_address: + type: object + nullable: true + example: + phone: 0404 333 222 111 + latitude: -37.8173751 + longitude: 144.964803195704 + first_name: Alice + last_name: Springs + street_address_1: 1 Flinders Street + street_address_2: '' + postal_code: '1234' + locality: Melbourne + region: Victoria + country: Australia required: - enterprise_id - email @@ -435,6 +454,25 @@ paths: example: - staff - discount + billing_address: + type: object + nullable: true + example: + shipping_address: + type: object + nullable: true + example: + phone: 0404 333 222 111 + latitude: -37.8173751 + longitude: 144.964803195704 + first_name: Alice + last_name: Springs + street_address_1: 1 Flinders Street + street_address_2: '' + postal_code: '1234' + locality: Melbourne + region: Victoria + country: Australia required: - enterprise_id - email From 5063f377c39d6494101b5b13c6d92b2a72c7c582 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Mon, 7 Mar 2022 16:30:58 +1100 Subject: [PATCH 09/11] Enable relationship inclusion for customers#show This will allow us to include address records. --- .../api/v1/customers_controller.rb | 8 ++++++- spec/requests/api/v1/customers_spec.rb | 24 ++++++++++++++++++- 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/app/controllers/api/v1/customers_controller.rb b/app/controllers/api/v1/customers_controller.rb index 36b1f65968..7e56ab6b28 100644 --- a/app/controllers/api/v1/customers_controller.rb +++ b/app/controllers/api/v1/customers_controller.rb @@ -17,7 +17,7 @@ module Api end def show - render json: Api::V1::CustomerSerializer.new(@customer) + render json: Api::V1::CustomerSerializer.new(@customer, include_options) end def create @@ -125,6 +125,12 @@ module Api def editable_enterprises OpenFoodNetwork::Permissions.new(current_api_user).editable_enterprises.select(:id) end + + def include_options + fields = [params.fetch(:include, [])].flatten + + { include: fields.map(&:to_s) } + end end end end diff --git a/spec/requests/api/v1/customers_spec.rb b/spec/requests/api/v1/customers_spec.rb index 090c38545b..0b7e1a599c 100644 --- a/spec/requests/api/v1/customers_spec.rb +++ b/spec/requests/api/v1/customers_spec.rb @@ -3,7 +3,7 @@ require "swagger_helper" describe "Customers", type: :request do - let!(:enterprise1) { create(:enterprise) } + let!(:enterprise1) { create(:enterprise, name: "The Farm") } let!(:enterprise2) { create(:enterprise) } let!(:customer1) { create( @@ -238,6 +238,28 @@ describe "Customers", type: :request do expect(json_response[:data][:relationships][:enterprise]).to eq(expected_enterprise_data) end end + + describe "including related records" do + it "doesn't include other records by default" do + get "/api/v1/customers/#{customer1.id}" + + expect(json_response[:included]).to eq nil + end + + it "includes enterprise data on request" do + get "/api/v1/customers/#{customer1.id}?include=enterprise" + + expect(json_response[:included].size).to eq 1 + expect(json_response[:included].first).to include( + id: enterprise1.id.to_s, + type: "enterprise", + attributes: { + id: enterprise1.id, + name: "The Farm", + } + ) + end + end end put "Update customer" do From 3e00ab261e72e519b872a55aa492576b2fc66f4f Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Thu, 24 Mar 2022 14:46:08 +1100 Subject: [PATCH 10/11] Spec customer relationships on API --- spec/requests/api/v1/customers_spec.rb | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/spec/requests/api/v1/customers_spec.rb b/spec/requests/api/v1/customers_spec.rb index 0b7e1a599c..002c1d5f39 100644 --- a/spec/requests/api/v1/customers_spec.rb +++ b/spec/requests/api/v1/customers_spec.rb @@ -223,6 +223,13 @@ describe "Customers", type: :request do end describe "related records" do + it "contains exactly the defined relationships" do + get "/api/v1/customers/#{customer1.id}" + + relationships = json_response[:data][:relationships].keys + expect(relationships).to match_array CustomerSchema.relationships.map(&:to_s) + end + it "serializes the enterprise relationship" do expected_enterprise_data = { "data" => { From 379eda7c412a1af320ec78865a70e8116ea51b3e Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 8 Mar 2022 10:44:33 +1100 Subject: [PATCH 11/11] Add feature toggle for API v1 use Dev, test and staging need to activate the feature toggle now before it's accessible. --- config/routes/api.rb | 10 ++++------ spec/requests/api/v1/customers_spec.rb | 5 ++++- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/config/routes/api.rb b/config/routes/api.rb index 9fa70e1d13..2a72d60155 100644 --- a/config/routes/api.rb +++ b/config/routes/api.rb @@ -87,13 +87,11 @@ Openfoodnetwork::Application.routes.draw do constraints: lambda { |_| Flipper.enabled?(:api_reports) } end - unless Rails.env.production? - namespace :v1 do - resources :customers + namespace :v1, constraints: ->(request) { Flipper.enabled?(:api_v1, request.env["warden"].user) } do + resources :customers - resources :enterprises do - resources :customers, only: :index - end + resources :enterprises do + resources :customers, only: :index end end diff --git a/spec/requests/api/v1/customers_spec.rb b/spec/requests/api/v1/customers_spec.rb index 002c1d5f39..d3a07b001e 100644 --- a/spec/requests/api/v1/customers_spec.rb +++ b/spec/requests/api/v1/customers_spec.rb @@ -17,7 +17,10 @@ describe "Customers", type: :request do let!(:customer2) { create(:customer, enterprise: enterprise1) } let!(:customer3) { create(:customer, enterprise: enterprise2) } - before { login_as enterprise1.owner } + before do + Flipper.enable(:api_v1) + login_as enterprise1.owner + end path "/api/v1/customers" do get "List customers" do