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/base_controller.rb b/app/controllers/api/v1/base_controller.rb index aca1c09e55..39f9e1684b 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,8 @@ 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 @@ -60,6 +63,20 @@ 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) + + 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/api/v1/customers_controller.rb b/app/controllers/api/v1/customers_controller.rb index 6c403ce2d4..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 @@ -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 @@ -70,12 +70,67 @@ module Api end def customer_params - params.require(:customer).permit(:email, :enterprise_id) + 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 + + def include_options + fields = [params.fetch(:include, [])].flatten + + { include: fields.map(&:to_s) } + end end end end 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 4652ae3bbf..45529f19e3 100644 --- a/app/json_schemas/customer_schema.rb +++ b/app/json_schemas/customer_schema.rb @@ -12,7 +12,37 @@ 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 }, + 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", + }, + 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 @@ -20,6 +50,14 @@ class CustomerSchema < JsonApiSchema [:enterprise_id, :email] end + def self.writable_attributes + attributes.except( + :id, + :allow_charges, + :terms_and_conditions_accepted_at, + ) + end + def self.relationships [:enterprise] 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 6f71b39159..b5e59648b5 100644 --- a/app/serializers/api/v1/customer_serializer.rb +++ b/app/serializers/api/v1/customer_serializer.rb @@ -3,13 +3,28 @@ 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, :terms_and_conditions_accepted_at + + 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/config/locales/en.yml b/config/locales/en.yml index d973822da5..fd9af5a234 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1414,6 +1414,8 @@ 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}" + 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/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/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..d3a07b001e 100644 --- a/spec/requests/api/v1/customers_spec.rb +++ b/spec/requests/api/v1/customers_spec.rb @@ -3,13 +3,24 @@ 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(:customer, enterprise: enterprise1) } + let!(:customer1) { + create( + :customer, + 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) } 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 @@ -101,11 +112,11 @@ 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 } - response "201", "Customer created" do + response "201", "Minimal customer created" do param(:customer) do { email: "test@example.com", @@ -114,14 +125,60 @@ 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, + terms_and_conditions_accepted_at: nil, + ) + 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, + tags: ["staff", "discount"], + shipping_address: CustomerSchema.address_example + ) + 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 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 @@ -136,7 +193,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 @@ -162,6 +226,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" => { @@ -177,6 +248,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 @@ -187,7 +280,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,14 +288,18 @@ 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 } 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 9b2642959d..c67e71b8a9 100644 --- a/swagger/v1/swagger.yaml +++ b/swagger/v1/swagger.yaml @@ -60,6 +60,38 @@ components: 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' + 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 @@ -67,6 +99,11 @@ components: - last_name - code - email + - allow_charges + - tags + - terms_and_conditions_accepted_at + - billing_address + - shipping_address relationships: type: object properties: @@ -130,6 +167,38 @@ components: 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' + 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 @@ -137,6 +206,11 @@ components: - last_name - code - email + - allow_charges + - tags + - terms_and_conditions_accepted_at + - billing_address + - shipping_address relationships: type: object properties: @@ -236,7 +310,7 @@ paths: parameters: [] responses: '201': - description: Customer created + description: Example customer created content: application/json: schema: @@ -271,6 +345,30 @@ paths: email: type: string example: alice@example.com + tags: + type: array + 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 @@ -333,9 +431,6 @@ paths: schema: type: object properties: - id: - type: integer - example: 1 enterprise_id: type: integer example: 2 @@ -354,6 +449,30 @@ paths: email: type: string example: alice@example.com + tags: + type: array + 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