Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ gem 'pg', '~> 1.0', require: false if dbs.match?(/all|postgres/)
gem 'fast_sqlite', require: false if dbs.match?(/all|sqlite/)
gem 'sqlite3', '>= 2.1', require: false if dbs.match?(/all|sqlite/)


gem 'database_cleaner', '~> 2.0', require: false
gem 'rspec-activemodel-mocks', '~> 1.1', require: false
gem 'rspec-rails', '~> 6.0.3', require: false
Expand Down
19 changes: 18 additions & 1 deletion api/app/controllers/spree/api/base_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ class BaseController < ActionController::Base
class_attribute :admin_line_item_attributes
self.admin_line_item_attributes = [:price, :variant_id, :sku]

class_attribute :admin_metadata_attributes
self.admin_metadata_attributes = [{ admin_metadata: {} }]

attr_accessor :current_api_user

before_action :load_user
Expand All @@ -35,15 +38,29 @@ class BaseController < ActionController::Base

private

Spree::Api::Config.metadata_permit_parameters.each do |resource|
define_method("permitted_#{resource.to_s.underscore}_attributes") do
if can?(:admin, "Spree::#{resource}".constantize)
super() + admin_metadata_attributes
else
super()
end
end
end

# users should be able to set price when importing orders via api
def permitted_line_item_attributes
if can?(:admin, Spree::LineItem)
super + admin_line_item_attributes
super + admin_line_item_attributes + admin_metadata_attributes
else
super
end
end

def permitted_user_attributes
can?(:admin, Spree.user_class) ? super + admin_metadata_attributes : super
end

def load_user
@current_api_user ||= Spree.user_class.find_by(spree_api_key: api_key.to_s)
end
Expand Down
17 changes: 15 additions & 2 deletions api/app/controllers/spree/api/line_items_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,10 @@ def create
@line_item = @order.contents.add(
variant,
params[:line_item][:quantity] || 1,
options: line_item_params[:options].to_h
options: line_item_params[:options].to_h,
**extract_metadata
)

respond_with(@line_item, status: 201, default_template: :show)
rescue ActiveRecord::RecordInvalid => error
invalid_resource!(error.record)
Expand Down Expand Up @@ -56,10 +58,21 @@ def line_items_attributes
{ line_items_attributes: {
id: params[:id],
quantity: params[:line_item][:quantity],
options: line_item_params[:options] || {}
options: line_item_params[:options] || {},
**extract_metadata
} }
end

def extract_metadata
metadata = { customer_metadata: line_item_params[:customer_metadata] }

if @current_user_roles&.include?("admin")
metadata[:admin_metadata] = line_item_params[:admin_metadata]
end

metadata
end

def line_item_params
params.require(:line_item).permit(permitted_line_item_attributes)
end
Expand Down
7 changes: 7 additions & 0 deletions api/app/controllers/spree/api/orders_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -112,12 +112,19 @@ def mine
def order_params
if params[:order]
normalize_params
prevent_customer_metadata_update
params.require(:order).permit(permitted_order_attributes)
else
{}
end
end

def prevent_customer_metadata_update
return unless @order&.completed? && cannot?(:admin, Spree::Order)

params[:order].delete(:customer_metadata) if params[:order]
end

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add specs for this and explain why this is necessary in the commit message

Copy link
Collaborator Author

@JustShah JustShah Feb 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have specs for this https://github.com/gms-electronics/solidus/pull/4/files#diff-2ef02110b947167ef3c10ad87001d8c36b1425578b4126fd5d39dd234e4906b7R132

Basically, This prevents users from updating customer_metadata once order it is completed

def normalize_params
if params[:order][:payments]
payments_params = params[:order].delete(:payments)
Expand Down
10 changes: 10 additions & 0 deletions api/app/helpers/spree/api/api_helpers.rb
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,16 @@ module ApiHelpers
end
end

Spree::Api::Config.metadata_api_parameters.each do |method_name, resource|
define_method("#{method_name}_attributes") do
authorized_attributes(resource, "#{method_name}_attributes")
end
end

def authorized_attributes(resource, config_attribute)
can?(:admin, resource) ? Spree::Api::Config.public_send(config_attribute) + [:admin_metadata] : Spree::Api::Config.public_send(config_attribute)
end
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to duplicate what already is done in the api base controller (this module is mixed into). Why was this change necessary?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shahmayur001 this one makes sense

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are adding the attribute to the params in API helpers, meanwhile, in the base controller, we are permitting the attribute.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also do not understand why we need this. Why do we need this here if we already set permitted__<resource\>_attributes?


def required_fields_for(model)
required_fields = model._validators.select do |_field, validations|
validations.any? { |validation| validation.is_a?(ActiveModel::Validations::PresenceValidator) }
Expand Down
38 changes: 30 additions & 8 deletions api/lib/spree/api_configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -36,22 +36,44 @@ class ApiConfiguration < Preferences::Configuration
:covered_by_store_credit, :display_total_applicable_store_credit,
:order_total_after_store_credit, :display_order_total_after_store_credit,
:total_applicable_store_credit, :display_total_available_store_credit,
:display_store_credit_remaining_after_capture, :canceler_id
:display_store_credit_remaining_after_capture, :canceler_id, :customer_metadata
]

preference :line_item_attributes, :array, default: [:id, :quantity, :price, :variant_id]
preference :line_item_attributes, :array, default: [:id, :quantity, :price, :variant_id, :customer_metadata]

# Spree::Api::Config.metadata_api_parameters contains the models
# to which the admin_metadata attribute is added
preference :metadata_api_parameters, :array, default: [
[:order, 'Spree::Order'],
[:customer_return, 'Spree::CustomerReturn'],
[:payment, 'Spree::Payment'],
[:return_authorization, 'Spree::ReturnAuthorization'],
[:shipment, 'Spree::Shipment'],
[:user, 'Spree.user_class'],
[:line_item, 'Spree::LineItem']
]

# Spree::Api::Config.metadata_permit_parameters contains the models
# to which the admin_metadata attribute is permitted
preference :metadata_permit_parameters, :array, default: [
:Order,
:CustomerReturn,
:Payment,
:ReturnAuthorization,
:Shipment
]

preference :option_type_attributes, :array, default: [:id, :name, :presentation, :position]

preference :payment_attributes, :array, default: [
:id, :source_type, :source_id, :amount, :display_amount,
:payment_method_id, :state, :avs_response, :created_at,
:updated_at
:updated_at, :customer_metadata
]

preference :payment_method_attributes, :array, default: [:id, :name, :description]

preference :shipment_attributes, :array, default: [:id, :tracking, :tracking_url, :number, :cost, :shipped_at, :state]
preference :shipment_attributes, :array, default: [:id, :tracking, :tracking_url, :number, :cost, :shipped_at, :state, :customer_metadata]

preference :taxonomy_attributes, :array, default: [:id, :name]

Expand Down Expand Up @@ -81,11 +103,11 @@ class ApiConfiguration < Preferences::Configuration
]

preference :customer_return_attributes, :array, default: [
:id, :number, :stock_location_id, :created_at, :updated_at
:id, :number, :stock_location_id, :created_at, :updated_at, :customer_metadata
]

preference :return_authorization_attributes, :array, default: [
:id, :number, :state, :order_id, :memo, :created_at, :updated_at
:id, :number, :state, :order_id, :memo, :created_at, :updated_at, :customer_metadata
]

preference :creditcard_attributes, :array, default: [
Expand All @@ -96,7 +118,7 @@ class ApiConfiguration < Preferences::Configuration
:id, :month, :year, :cc_type, :last_digits, :name
]

preference :user_attributes, :array, default: [:id, :email, :created_at, :updated_at]
preference :user_attributes, :array, default: [:id, :email, :created_at, :updated_at, :customer_metadata]

preference :property_attributes, :array, default: [:id, :name, :presentation]

Expand Down Expand Up @@ -132,7 +154,7 @@ def promotion_attributes=(value)

preference :store_credit_history_attributes, :array, default: [
:display_amount, :display_user_total_amount, :display_action,
:display_event_date, :display_remaining_amount
:display_event_date, :display_remaining_amount, :customer_metadata
]

preference :variant_property_attributes, :array, default: [
Expand Down
27 changes: 26 additions & 1 deletion api/spec/requests/spree/api/customer_returns_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,14 @@ module Spree::Api
expect(json_response).to have_attributes(attributes)
end

it "can view admin_metadata" do
customer_return = FactoryBot.create(:customer_return)

get spree.api_order_customer_return_path(customer_return.order, customer_return.id)

expect(json_response).to have_key('admin_metadata')
end

it "can get a list of customer returns" do
FactoryBot.create(:customer_return, shipped_order: order)
FactoryBot.create(:customer_return, shipped_order: order)
Expand Down Expand Up @@ -97,7 +105,7 @@ module Spree::Api
it "can learn how to create a new customer return" do
get spree.new_api_order_customer_return_path(order)

expect(json_response["attributes"]).to eq(["id", "number", "stock_location_id", "created_at", "updated_at"])
expect(json_response["attributes"]).to eq(["id", "number", "stock_location_id", "created_at", "updated_at", "customer_metadata", "admin_metadata"])
end

it "can update a customer return" do
Expand All @@ -112,6 +120,23 @@ module Spree::Api
expect(json_response["stock_location_id"]).to eq final_stock_location.id
end

it "can update a customer return admin_metadata" do
initial_stock_location = FactoryBot.create(:stock_location)
final_stock_location = FactoryBot.create(:stock_location)
customer_return = FactoryBot.create(:customer_return, stock_location: initial_stock_location)

put spree.api_order_customer_return_path(customer_return.order, customer_return.id),
params: {
order_id: customer_return.order.number,
customer_return: { stock_location_id: final_stock_location.id, admin_metadata: { 'order_number' => 'PN345678' } }
}

expect(response.status).to eq(200)
expect(json_response).to have_attributes(attributes)
expect(json_response["stock_location_id"]).to eq final_stock_location.id
expect(json_response["admin_metadata"]).to eq({ 'order_number' => 'PN345678' })
end

context "when creating new return items" do
it "can create a new customer return" do
stock_location = FactoryBot.create(:stock_location)
Expand Down
69 changes: 68 additions & 1 deletion api/spec/requests/spree/api/line_items_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ module Spree::Api

it "can learn how to create a new line item" do
get spree.new_api_order_line_item_path(order)
expect(json_response["attributes"]).to eq(["quantity", "price", "variant_id"])
expect(json_response["attributes"]).to eq(["quantity", "price", "variant_id", "customer_metadata"])
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the specs should be squashed into the same commit the change has been introduced in the controller

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved!

required_attributes = json_response["required_attributes"]
expect(required_attributes).to include("quantity", "variant_id")
end
Expand Down Expand Up @@ -95,6 +95,29 @@ module Spree::Api
expect(response.status).to eq(201)
end

it "cannot see admin_metadata" do
post spree.api_order_line_items_path(order),
params: {
line_item: { variant_id: product.master.to_param, quantity: 1 },
order_token: order.guest_token
}

expect(response.status).to eq(201)
expect(json_response).not_to have_key('admin_metadata')
end

it "allows creating line item with customer metadata but not admin metadata" do
post spree.api_order_line_items_path(order),
params: {
line_item: { variant_id: product.master.to_param,
quantity: 1,
customer_metadata: { "Company" => "Sample Company" } }
}

expect(json_response['customer_metadata']).to eq({ "Company" => "Sample Company" })
expect(json_response).not_to have_key('admin_metadata')
end

it '#create calls #invalid_resource! if adding a line item fails validation' do
allow_any_instance_of(Spree::LineItem).to receive(:valid?).and_return(false)
expect_any_instance_of(Spree::Api::BaseController).to receive(:invalid_resource!).once
Expand Down Expand Up @@ -128,6 +151,22 @@ module Spree::Api
expect(json_response["quantity"]).to eq(101)
end

it "can update a line item customer metadata on the order" do
line_item = order.line_items.first

put spree.api_order_line_item_path(order, line_item),
params: { line_item: { quantity: 101, customer_metadata: { "adding_quantity" => "true" } } }

expect(response.status).to eq(200)

order.reload

expect(order.total).to eq(1010) # 10 original due to factory, + 1000 in this test
expect(json_response).to have_attributes(attributes)
expect(json_response["quantity"]).to eq(101)
expect(json_response['customer_metadata']).to eq({ "adding_quantity" => "true" })
end

it "can update a line item's options on the order" do
without_partial_double_verification do
expect_any_instance_of(Spree::LineItem).to receive(:some_option=).with("foobar")
Expand Down Expand Up @@ -189,6 +228,34 @@ module Spree::Api
end
end

context "as an admin" do
sign_in_as_admin!

it "can see admin_metadata" do
post spree.api_order_line_items_path(order),
params: {
line_item: { variant_id: product.master.to_param, quantity: 1 },
order_token: order.guest_token
}

expect(response.status).to eq(201)
expect(json_response).to have_key('admin_metadata')
end

it "allows creating line item with customer metadata and admin metadata" do
post spree.api_order_line_items_path(order),
params: {
line_item: { variant_id: product.master.to_param,
quantity: 1,
customer_metadata: { "Company" => "Sample Company" },
admin_metadata: { "discount" => "not_applicable" } }
}

expect(json_response['customer_metadata']).to eq({ "Company" => "Sample Company" })
expect(json_response['admin_metadata']).to eq({ "discount" => "not_applicable" })
end
end

context "as just another user" do
before do
user = create(:user)
Expand Down
Loading
Loading