From a37e08c2fd27eb534025289d871d4244261c64d0 Mon Sep 17 00:00:00 2001 From: Ahmed Ejaz Date: Sun, 15 Jun 2025 18:09:02 +0500 Subject: [PATCH 01/14] Refactor order management permissions for producers Introduces granular permissions control for producers editing orders: - Adds new :edit_as_producer_only permission for suppliers - Refactors ability checks to clearly separate producer vs admin/distributor access - Updates order views to properly restrict actions based on user role - Prevents admins from being restricted by producer-only edit mode --- app/helpers/spree/admin/orders_helper.rb | 3 +- app/models/spree/ability.rb | 84 ++++++++++++++++--- app/views/spree/admin/orders/_form.html.haml | 2 +- .../spree/admin/orders/_table_row.html.haml | 4 +- app/views/spree/admin/orders/edit.html.haml | 11 ++- 5 files changed, 84 insertions(+), 20 deletions(-) diff --git a/app/helpers/spree/admin/orders_helper.rb b/app/helpers/spree/admin/orders_helper.rb index e80b6d9d13..976f97b948 100644 --- a/app/helpers/spree/admin/orders_helper.rb +++ b/app/helpers/spree/admin/orders_helper.rb @@ -155,8 +155,7 @@ module Spree end def filter_by_supplier?(order) - order.distributor&.enable_producers_to_edit_orders && - spree_current_user.can_manage_line_items_in_orders_only? + can? :edit_as_producer_only, order end def display_value_for_producer(order, value) diff --git a/app/models/spree/ability.rb b/app/models/spree/ability.rb index de867108e5..1a72d66980 100644 --- a/app/models/spree/ability.rb +++ b/app/models/spree/ability.rb @@ -20,6 +20,10 @@ module Spree if user.try(:admin?) can :manage, :all + + # this action was needed for restrictions for distributors and suppliers + # however, admins don't need to be restricted, so, bypassing it for admins + cannot :edit_as_producer_only, Spree::Order else can [:index, :read], Country can :create, Order @@ -257,8 +261,13 @@ module Spree end def add_order_cycle_management_abilities(user) + can [:admin, :index], OrderCycle do |order_cycle| + OrderCycle.visible_by(user).include?(order_cycle) || + order_cycle.orders.any? { |order| can_edit_as_producer(order, user) } + end + can [ - :admin, :index, :read, :edit, :update, :incoming, :outgoing, :checkout_options + :read, :edit, :update, :incoming, :outgoing, :checkout_options ], OrderCycle do |order_cycle| OrderCycle.visible_by(user).include? order_cycle end @@ -274,8 +283,37 @@ module Spree end def add_order_management_abilities(user) - can [:index, :create], Spree::Order - can [:read, :update, :fire, :resend, :invoice, :print], Spree::Order do |order| + can [:manage_order_sections], Spree::Order do |order| + user.admin? || + order.distributor.nil? || + user.enterprises.include?(order.distributor) || + order.order_cycle&.coordinated_by?(user) + end + + can [:edit_as_producer_only], Spree::Order do |order| + cannot?(:manage_order_sections, order) && can_edit_as_producer(order, user) + end + + can [:index], Spree::Order do |order| + user.admin? || + user.enterprises.any?(&:is_distributor) || + can_edit_as_producer(order, user) + end + + can [:create], Spree::Order + + can [:read, :update], Spree::Order do |order| + # We allow editing orders with a nil distributor as this state occurs + # during the order creation process from the admin backend + order.distributor.nil? || + # Enterprise User can access orders that they are a distributor for + user.enterprises.include?(order.distributor) || + # Enterprise User can access orders that are placed inside a OC they coordinate + order.order_cycle&.coordinated_by?(user) || + can_edit_as_producer(order, user) + end + + can [:fire, :resend, :invoice, :print], Spree::Order do |order| # We allow editing orders with a nil distributor as this state occurs # during the order creation process from the admin backend order.distributor.nil? || @@ -284,22 +322,39 @@ module Spree # Enterprise User can access orders that are placed inside a OC they coordinate order.order_cycle&.coordinated_by?(user) end - can [:admin, :bulk_management, :managed, :distribution], Spree::Order do + + can [:admin, :bulk_management], Spree::Order do |order| + user.admin? || + user.enterprises.any?(&:is_distributor) || + can_edit_as_producer(order, user) + end + + can [:managed, :distribution], Spree::Order do user.admin? || user.enterprises.any?(&:is_distributor) end can [:admin, :index, :create, :show, :poll, :generate], :invoice can [:admin, :visible], Enterprise can [:admin, :index, :create, :update, :destroy], :line_item - can [:admin, :index, :create], Spree::LineItem + can [:admin, :index, :create], Spree::LineItem do |item| + user.admin? || + user.enterprises.any?(&:is_distributor) || + can_edit_as_producer(item.order, user) + end can [:destroy, :update], Spree::LineItem do |item| order = item.order user.admin? || user.enterprises.include?(order.distributor) || - order.order_cycle&.coordinated_by?(user) + order.order_cycle&.coordinated_by?(user) || + can_edit_as_producer(order, user) + end + + can [:admin, :index, :read, :create, :edit, :update, :fire], Spree::Shipment do |shipment| + user.admin? || + user.enterprises.any?(&:is_distributor) || + can_edit_as_producer(shipment.order, user) end can [:admin, :index, :read, :create, :edit, :update, :fire], Spree::Payment - can [:admin, :index, :read, :create, :edit, :update, :fire], Spree::Shipment can [:admin, :index, :read, :create, :edit, :update, :fire], Spree::Adjustment can [:admin, :index, :read, :create, :edit, :update, :fire], Spree::ReturnAuthorization can [:destroy], Spree::Adjustment do |adjustment| @@ -350,26 +405,31 @@ module Spree can [:admin, :edit, :cancel, :resume], ProxyOrder do |proxy_order| user.enterprises.include?(proxy_order.subscription.shop) end + can [:visible], Enterprise end - def can_edit_order(order, user) + def can_edit_as_producer(order, user) return unless order.distributor&.enable_producers_to_edit_orders order.variants.any? { |variant| user.enterprises.ids.include?(variant.supplier_id) } end def add_manage_line_items_abilities(user) + can [:edit_as_producer_only], Spree::Order do |order| + can_edit_as_producer(order, user) + end + can [:admin, :read, :index, :edit, :update, :bulk_management], Spree::Order do |order| - can_edit_order(order, user) + can_edit_as_producer(order, user) end can [:admin, :index, :create, :destroy, :update], Spree::LineItem do |item| - can_edit_order(item.order, user) + can_edit_as_producer(item.order, user) end can [:index, :create, :add, :read, :edit, :update], Spree::Shipment do |shipment| - can_edit_order(shipment.order, user) + can_edit_as_producer(shipment.order, user) end can [:admin, :index], OrderCycle do |order_cycle| - can_edit_order(order_cycle.order, user) + can_edit_as_producer(order_cycle.order, user) end can [:visible], Enterprise end diff --git a/app/views/spree/admin/orders/_form.html.haml b/app/views/spree/admin/orders/_form.html.haml index 4f1866f330..f0f9a3434b 100644 --- a/app/views/spree/admin/orders/_form.html.haml +++ b/app/views/spree/admin/orders/_form.html.haml @@ -8,7 +8,7 @@ - if @order.shipments.any? = render :partial => "spree/admin/orders/shipment", :collection => @order.shipments, :locals => { :order => @order } - - if spree_current_user.can_manage_orders? + - if can?(:manage_order_sections, @order) - if @order.line_items.exists? = render partial: "spree/admin/orders/note", locals: { order: @order } diff --git a/app/views/spree/admin/orders/_table_row.html.haml b/app/views/spree/admin/orders/_table_row.html.haml index 6705719168..9ea6367a5f 100644 --- a/app/views/spree/admin/orders/_table_row.html.haml +++ b/app/views/spree/admin/orders/_table_row.html.haml @@ -47,10 +47,10 @@ - if local_assigns[:success] %i.success.icon-ok-sign{"data-controller": "ephemeral"} = render AdminTooltipComponent.new(text: t('spree.admin.orders.index.edit'), link_text: "", link: edit_admin_order_path(order), link_class: "icon_link with-tip icon-edit no-text") - - if spree_current_user.can_manage_orders? && order.ready_to_ship? + - if can?(:manage_order_sections, order) && order.ready_to_ship? %form = render ShipOrderComponent.new(order: order) = render partial: 'admin/shared/tooltip_button', locals: {button_class: "icon-road icon_link with-tip no-text", reflex_data_id: order.id.to_s, tooltip_text: t('spree.admin.orders.index.ship'), shipment: true} - - if can?(:update, Spree::Payment) && order.payment_required? && order.pending_payments.reject(&:requires_authorization?).any? + - if can?(:manage_order_sections, order) && can?(:update, Spree::Payment) && order.payment_required? && order.pending_payments.reject(&:requires_authorization?).any? = render partial: 'admin/shared/tooltip_button', locals: {button_class: "icon-capture icon_link no-text", button_reflex: "click->Admin::OrdersReflex#capture", reflex_data_id: order.id.to_s, tooltip_text: t('spree.admin.orders.index.capture')} diff --git a/app/views/spree/admin/orders/edit.html.haml b/app/views/spree/admin/orders/edit.html.haml index 8d9023581d..217eb6027c 100644 --- a/app/views/spree/admin/orders/edit.html.haml +++ b/app/views/spree/admin/orders/edit.html.haml @@ -6,7 +6,7 @@ - content_for :page_actions do - if can?(:fire, @order) %li= event_links(@order) - - if spree_current_user.can_manage_orders? + - if can?(:manage_order_sections, @order) = render partial: 'spree/admin/shared/order_links' - if can?(:admin, Spree::Order) %li @@ -14,7 +14,7 @@ = t(:back_to_orders_list) = render partial: "spree/admin/shared/order_page_title" -- if spree_current_user.can_manage_orders? +- if can?(:manage_order_sections, @order) = render partial: "spree/admin/shared/order_tabs", locals: { current: 'Order Details' } %div @@ -22,7 +22,12 @@ = admin_inject_shops(@shops, module: 'admin.orders') = admin_inject_order_cycles(@order_cycles) - %div{"ng-controller" => "orderCtrl", "ofn-distributor-id" => @order.distributor_id, "ofn-order-cycle-id" => @order.order_cycle_id} + %div{ + "ng-controller" => "orderCtrl", + "ofn-distributor-id" => @order.distributor_id, + "ofn-order-cycle-id" => @order.order_cycle_id, + "ofn-search-variants-as" => (can?(:manage_order_sections, @order) ? 'hub' : 'supplier'), + } = render :partial => 'add_product' if can?(:update, @order) From 8e8878e43a0fbba4fd5e5ed3ae1dfba731902270 Mon Sep 17 00:00:00 2001 From: Ahmed Ejaz Date: Sun, 15 Jun 2025 18:10:55 +0500 Subject: [PATCH 02/14] Add search_variants_as parameter to variant search functionality --- .../admin/orders/controllers/order_controller.js.coffee | 1 + .../admin/utils/directives/variant_autocomplete.js.coffee | 1 + app/controllers/spree/admin/variants_controller.rb | 2 +- lib/open_food_network/scope_variants_for_search.rb | 2 +- spec/controllers/spree/admin/variants_controller_spec.rb | 7 +++++++ .../open_food_network/scope_variants_for_search_spec.rb | 2 +- 6 files changed, 12 insertions(+), 3 deletions(-) diff --git a/app/assets/javascripts/admin/orders/controllers/order_controller.js.coffee b/app/assets/javascripts/admin/orders/controllers/order_controller.js.coffee index 6107ad2345..e99cbf3bbd 100644 --- a/app/assets/javascripts/admin/orders/controllers/order_controller.js.coffee +++ b/app/assets/javascripts/admin/orders/controllers/order_controller.js.coffee @@ -5,6 +5,7 @@ angular.module("admin.orders").controller "orderCtrl", ($scope, shops, orderCycl $scope.distributor_id = parseInt($attrs.ofnDistributorId) $scope.order_cycle_id = parseInt($attrs.ofnOrderCycleId) + $scope.search_variants_as = $attrs.ofnSearchVariantsAs $scope.validOrderCycle = (oc) -> $scope.orderCycleHasDistributor oc, parseInt($scope.distributor_id) diff --git a/app/assets/javascripts/admin/utils/directives/variant_autocomplete.js.coffee b/app/assets/javascripts/admin/utils/directives/variant_autocomplete.js.coffee index 49a5b0fe59..efc3b0042f 100644 --- a/app/assets/javascripts/admin/utils/directives/variant_autocomplete.js.coffee +++ b/app/assets/javascripts/admin/utils/directives/variant_autocomplete.js.coffee @@ -26,6 +26,7 @@ angular.module("admin.utils").directive "variantAutocomplete", ($timeout) -> order_cycle_id: scope.order_cycle_id eligible_for_subscriptions: scope.eligible_for_subscriptions include_out_of_stock: scope.include_out_of_stock + search_variants_as: scope.search_variants_as results: (data, page) -> window.variants = data # this is how spree auto complete JS code picks up variants results: data diff --git a/app/controllers/spree/admin/variants_controller.rb b/app/controllers/spree/admin/variants_controller.rb index deda271867..d78cb60427 100644 --- a/app/controllers/spree/admin/variants_controller.rb +++ b/app/controllers/spree/admin/variants_controller.rb @@ -117,7 +117,7 @@ module Spree def variant_search_params params.permit( :q, :distributor_id, :order_cycle_id, :schedule_id, :eligible_for_subscriptions, - :include_out_of_stock + :include_out_of_stock, :search_variants_as, ).to_h.with_indifferent_access end diff --git a/lib/open_food_network/scope_variants_for_search.rb b/lib/open_food_network/scope_variants_for_search.rb index 465e501c10..875b68ba5a 100644 --- a/lib/open_food_network/scope_variants_for_search.rb +++ b/lib/open_food_network/scope_variants_for_search.rb @@ -21,7 +21,7 @@ module OpenFoodNetwork scope_to_schedule if params[:schedule_id] scope_to_order_cycle if params[:order_cycle_id] scope_to_distributor if params[:distributor_id] - scope_to_supplier if spree_current_user.can_manage_line_items_in_orders_only? + scope_to_supplier if params[:search_variants_as] == 'supplier' @variants end diff --git a/spec/controllers/spree/admin/variants_controller_spec.rb b/spec/controllers/spree/admin/variants_controller_spec.rb index 21f0413bcd..0eb2350265 100644 --- a/spec/controllers/spree/admin/variants_controller_spec.rb +++ b/spec/controllers/spree/admin/variants_controller_spec.rb @@ -137,6 +137,13 @@ RSpec.describe Spree::Admin::VariantsController do expect(variant).to have_received(:destroy) end + describe "#search" do + it "filters by distributor and supplier1 products" do + spree_get :search, q: 'Prod', distributor_id: d.id.to_s, search_variants_as: 'supplier' + expect(assigns(:variants)).to eq([v1]) + end + end + it 'shows a success flash message' do spree_delete :destroy, id: variant.id, product_id: variant.product.id, format: 'html' diff --git a/spec/lib/open_food_network/scope_variants_for_search_spec.rb b/spec/lib/open_food_network/scope_variants_for_search_spec.rb index 35453826bf..943539c169 100644 --- a/spec/lib/open_food_network/scope_variants_for_search_spec.rb +++ b/spec/lib/open_food_network/scope_variants_for_search_spec.rb @@ -185,7 +185,7 @@ RSpec.describe OpenFoodNetwork::ScopeVariantsForSearch do end context "when search is done by the producer allowing to edit orders" do - let(:params) { { q: "product" } } + let(:params) { { q: "product", search_variants_as: 'supplier' } } let(:producer) { create(:supplier_enterprise) } let!(:spree_current_user) { instance_double('Spree::User', enterprises: Enterprise.where(id: producer.id), From 2f9c85664591bf3df94fabc638756b7353910d12 Mon Sep 17 00:00:00 2001 From: Ahmed Ejaz Date: Sun, 15 Jun 2025 18:12:30 +0500 Subject: [PATCH 03/14] Refactors order and line item permissions logic Simplifies permission checking by: - Extracting common managed/coordinated orders logic into separate method - Combining producer-editable and managed/coordinated order clauses - Merging producer and admin line item permission checks into single query --- app/services/permissions/order.rb | 34 ++++++++++++++++--------------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/app/services/permissions/order.rb b/app/services/permissions/order.rb index 29d63b5a2f..c9e043a8bd 100644 --- a/app/services/permissions/order.rb +++ b/app/services/permissions/order.rb @@ -21,18 +21,20 @@ module Permissions filtered_orders(orders) end + def managed_or_coordinated_orders_where_clause + Spree::Order.where( + managed_orders_where_values.or(coordinated_orders_where_values) + ) + end + # Any orders that the user can edit def editable_orders - orders = if @user.can_manage_line_items_in_orders_only? - Spree::Order.joins(:distributor).where( - id: produced_orders.select(:id), - distributor: { enable_producers_to_edit_orders: true } - ) - else - Spree::Order.where( - managed_orders_where_values.or(coordinated_orders_where_values) - ) - end + orders = Spree::Order.joins(:distributor).where( + id: produced_orders.select(:id), + distributor: { enable_producers_to_edit_orders: true } + ).or( + managed_or_coordinated_orders_where_clause + ) filtered_orders(orders) end @@ -43,13 +45,13 @@ module Permissions # Any line items that I can edit def editable_line_items - if @user.can_manage_line_items_in_orders_only? - Spree::LineItem.editable_by_producers( - @permissions.managed_enterprises.select("enterprises.id") + Spree::LineItem.editable_by_producers( + @permissions.managed_enterprises.select("enterprises.id") + ).or( + Spree::LineItem.where( + order_id: filtered_orders(managed_or_coordinated_orders_where_clause).select(:id) ) - else - Spree::LineItem.where(order_id: editable_orders.select(:id)) - end + ) end private From cd01a27bddf9e90743da83939b022b696a622298 Mon Sep 17 00:00:00 2001 From: Ahmed Ejaz Date: Sun, 15 Jun 2025 18:13:07 +0500 Subject: [PATCH 04/14] Add distributor_name_alias to searchable attributes and implement ransacker for filtering line items --- .../line_items/controllers/line_items_controller.js.coffee | 2 +- app/models/spree/order.rb | 7 ++++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/app/assets/javascripts/admin/line_items/controllers/line_items_controller.js.coffee b/app/assets/javascripts/admin/line_items/controllers/line_items_controller.js.coffee index d27787f764..1ebebf2bbb 100644 --- a/app/assets/javascripts/admin/line_items/controllers/line_items_controller.js.coffee +++ b/app/assets/javascripts/admin/line_items/controllers/line_items_controller.js.coffee @@ -19,7 +19,7 @@ angular.module("admin.lineItems").controller 'LineItemsCtrl', ($scope, $timeout, $scope.page = 1 $scope.per_page = $scope.per_page_options[0].id $scope.filterByVariantId = null - searchThrough = ["order_distributor_name", + searchThrough = ["order_distributor_name_alias", "order_bill_address_phone", "order_bill_address_firstname", "order_bill_address_lastname", diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb index 909cb1a529..6b9134ce4f 100644 --- a/app/models/spree/order.rb +++ b/app/models/spree/order.rb @@ -9,7 +9,7 @@ module Spree include SetUnusedAddressFields searchable_attributes :number, :state, :shipment_state, :payment_state, :distributor_id, - :order_cycle_id, :email, :total, :customer_id + :order_cycle_id, :email, :total, :customer_id, :distributor_name_alias searchable_associations :shipping_method, :bill_address, :distributor searchable_scopes :complete, :incomplete, :sort_by_billing_address_name_asc, :sort_by_billing_address_name_desc @@ -181,6 +181,11 @@ module Spree scope :by_state, lambda { |state| where(state:) } scope :not_state, lambda { |state| where.not(state:) } + # This is used to filter line items by the distributor name on BOM page + ransacker :distributor_name_alias do + Arel.sql("distributor.name") + end + def initialize(*_args) @checkout_processing = nil @manual_shipping_selection = nil From ade35f2fa2a872bfad4dcf5d1965805e5ac907a2 Mon Sep 17 00:00:00 2001 From: Ahmed Ejaz Date: Sun, 15 Jun 2025 18:14:57 +0500 Subject: [PATCH 05/14] Fixes specs and update code respectively --- app/models/spree/line_item.rb | 2 +- lib/reporting/line_items.rb | 2 +- .../scope_variants_for_search_spec.rb | 2 -- .../reports/orders_and_distributors_report_spec.rb | 1 - .../producer_actions_spec.rb | 4 ++-- spec/views/spree/admin/orders/edit.html.haml_spec.rb | 12 ++++++++---- 6 files changed, 12 insertions(+), 11 deletions(-) rename spec/system/admin/orders/{ => having_producer_products}/producer_actions_spec.rb (98%) diff --git a/app/models/spree/line_item.rb b/app/models/spree/line_item.rb index 92bca503f5..fed86cd802 100644 --- a/app/models/spree/line_item.rb +++ b/app/models/spree/line_item.rb @@ -109,7 +109,7 @@ module Spree } scope :editable_by_producers, ->(enterprises_ids) { - joins(:variant, order: :distributor).where( + joins(variant: :supplier, order: :distributor).where( distributor: { enable_producers_to_edit_orders: true }, spree_variants: { supplier_id: enterprises_ids } ) diff --git a/lib/reporting/line_items.rb b/lib/reporting/line_items.rb index 0073b66d50..6b0750ab0d 100644 --- a/lib/reporting/line_items.rb +++ b/lib/reporting/line_items.rb @@ -17,7 +17,7 @@ module Reporting def list(line_item_includes = [variant: [:supplier, :product]]) line_items = order_permissions.visible_line_items.in_orders(orders.result) - .order("supplier.name", "product.name", "variant.display_name", "variant.unit_description") + .order("supplier.name", "product.name", "spree_variants.display_name", "spree_variants.unit_description") if @params[:supplier_id_in].present? line_items = line_items.supplied_by_any(@params[:supplier_id_in]) diff --git a/spec/lib/open_food_network/scope_variants_for_search_spec.rb b/spec/lib/open_food_network/scope_variants_for_search_spec.rb index 943539c169..ccbed15dd0 100644 --- a/spec/lib/open_food_network/scope_variants_for_search_spec.rb +++ b/spec/lib/open_food_network/scope_variants_for_search_spec.rb @@ -70,8 +70,6 @@ RSpec.describe OpenFoodNetwork::ScopeVariantsForSearch do "Enterprise Load", "VariantOverride Load", "SQL", - "Enterprise Pluck", - "Enterprise Load" ] expect(result).to include v4 diff --git a/spec/lib/reports/orders_and_distributors_report_spec.rb b/spec/lib/reports/orders_and_distributors_report_spec.rb index 07c68b55ea..98f4943f29 100644 --- a/spec/lib/reports/orders_and_distributors_report_spec.rb +++ b/spec/lib/reports/orders_and_distributors_report_spec.rb @@ -151,7 +151,6 @@ RSpec.describe Reporting::Reports::OrdersAndDistributors::Base do subject # build context first expect { subject.table_rows }.to query_database [ - "Enterprise Pluck", "SQL", "Spree::LineItem Load", "Spree::PaymentMethod Load", diff --git a/spec/system/admin/orders/producer_actions_spec.rb b/spec/system/admin/orders/having_producer_products/producer_actions_spec.rb similarity index 98% rename from spec/system/admin/orders/producer_actions_spec.rb rename to spec/system/admin/orders/having_producer_products/producer_actions_spec.rb index 88e8d5bd34..d8e8a93dda 100644 --- a/spec/system/admin/orders/producer_actions_spec.rb +++ b/spec/system/admin/orders/having_producer_products/producer_actions_spec.rb @@ -19,7 +19,7 @@ RSpec.describe 'As a producer who have the ability to update orders' do o = create( :completed_order_with_totals, distributor:, order_cycle:, - user: supplier1_ent_user, line_items_count: 1 + line_items_count: 1 ) o.line_items.first.update_columns(variant_id: supplier1_v1.id) o @@ -28,7 +28,7 @@ RSpec.describe 'As a producer who have the ability to update orders' do o = create( :completed_order_with_totals, distributor:, order_cycle:, - user: supplier2_ent_user, line_items_count: 1 + line_items_count: 1 ) o.line_items.first.update_columns(variant_id: supplier2_v1.id) o diff --git a/spec/views/spree/admin/orders/edit.html.haml_spec.rb b/spec/views/spree/admin/orders/edit.html.haml_spec.rb index ca22a3c226..d53fa5abb7 100644 --- a/spec/views/spree/admin/orders/edit.html.haml_spec.rb +++ b/spec/views/spree/admin/orders/edit.html.haml_spec.rb @@ -14,14 +14,19 @@ RSpec.describe "spree/admin/orders/edit.html.haml" do Spree::Config[:enable_invoices?] = original_config end + let(:current_test_user) { create(:admin_user) } + before do controller.singleton_class.class_eval do + attr_accessor :current_test_user + def current_ability - Spree::Ability.new(Spree::User.new) + Spree::Ability.new(current_test_user) end end - allow(view).to receive_messages spree_current_user: create(:admin_user) + controller.current_test_user = current_test_user + allow(view).to receive_messages spree_current_user: current_test_user end context "when order is complete" do @@ -54,7 +59,6 @@ RSpec.describe "spree/admin/orders/edit.html.haml" do it "doesn't display a table of out of stock line items" do render - expect(rendered).not_to have_content "Out of Stock" expect(rendered).not_to have_selector ".insufficient-stock-items", text: out_of_stock_line_item.variant.display_name end @@ -96,7 +100,7 @@ RSpec.describe "spree/admin/orders/edit.html.haml" do it "doesn't display a table of out of stock line items" do render - expect(rendered).not_to have_content "Out of Stock" + expect(rendered).not_to have_selector ".insufficient-stock-items" end end From fe1b8aaab369e8d2d4e8da43023866afd8c1f5da Mon Sep 17 00:00:00 2001 From: Ahmed Ejaz Date: Sun, 15 Jun 2025 18:15:06 +0500 Subject: [PATCH 06/14] Add hub actions spec for producer order management functionality --- .../hub_actions_spec.rb | 150 ++++++++++++++++++ 1 file changed, 150 insertions(+) create mode 100644 spec/system/admin/orders/having_producer_products/hub_actions_spec.rb diff --git a/spec/system/admin/orders/having_producer_products/hub_actions_spec.rb b/spec/system/admin/orders/having_producer_products/hub_actions_spec.rb new file mode 100644 index 0000000000..fbba03a01e --- /dev/null +++ b/spec/system/admin/orders/having_producer_products/hub_actions_spec.rb @@ -0,0 +1,150 @@ +# frozen_string_literal: true + +require 'system_helper' + +RSpec.describe 'As a hub (producer seller) who have the ability to update orders having their products' do + include AdminHelper + include AuthenticationHelper + include WebHelper + + let!(:hub1) { create(:distributor_enterprise, name: 'My hub1') } + let!(:hub1_v1) { create(:variant, supplier_id: hub1.id) } + let!(:hub1_v2) { create(:variant, supplier_id: hub1.id) } + let(:order_cycle) do + create(:simple_order_cycle, distributors: [distributor], variants: [hub1_v1, hub1_v2], coordinator: distributor) + end + let!(:order_containing_hub1_products) do + o = create( + :completed_order_with_totals, + distributor:, order_cycle:, + line_items_count: 1 + ) + o.line_items.first.update_columns(variant_id: hub1_v1.id) + o + end + let(:hub1_ent_user) { create(:user, enterprises: [hub1]) } + + context "As hub1 enterprise user" do + before { login_as(hub1_ent_user) } + let(:order) { order_containing_hub1_products } + let(:user) { hub1_ent_user } + + describe 'orders index page' do + before { visit spree.admin_orders_path } + + context "when no distributor allow the producer to edit orders" do + let(:distributor) { create(:distributor_enterprise) } + + it "should not allow producer to view orders page" do + expect(page).to have_content 'NO ORDERS FOUND' + end + end + + context "when distributor allows the producer to edit orders" do + let(:distributor) { create(:distributor_enterprise, enable_producers_to_edit_orders: true) } + + context "when distributor doesn't allow to view customer details" do + it "should allow producer to view orders page with HIDDEN customer details" do + within('#listing_orders tbody') do + expect(page).to have_selector('tr', count: 1) # Only one order + # One for Email, one for Name + expect(page).to have_selector('td', text: '< Hidden >', count: 2) + end + end + end + + context "when distributor allows to view customer details" do + let(:distributor) do + create( + :distributor_enterprise, + enable_producers_to_edit_orders: true, + show_customer_names_to_suppliers: true + ) + end + it "should allow producer to view orders page with customer details" do + within('#listing_orders tbody') do + name = order.bill_address&.full_name_for_sorting + email = order.email + expect(page).to have_selector('tr', count: 1) # Only one order + expect(page).to have_selector('td', text: name, count: 1) + expect(page).to have_selector('td', text: email, count: 1) + within 'td.actions' do + # to have edit button + expect(page).to have_selector("a.icon-edit") + # not to have ship button + expect(page).not_to have_selector('button.icon-road') + end + end + end + end + end + end + + describe 'orders edit page' do + before { visit spree.edit_admin_order_path(order) } + + context "when no distributor allow the producer to edit orders" do + let(:distributor) { create(:distributor_enterprise) } + + it "should not allow producer to view orders page" do + expect(page).to have_content 'Unauthorized' + end + end + + context "when distributor allows to edit orders" do + let(:distributor) { create(:distributor_enterprise, enable_producers_to_edit_orders: true) } + let(:product) { hub1_v2.product } + + it "should allow me to manage my products in the order" do + expect(page).to have_content 'Add Product' + + # Add my product + add_product(product) + expect_product_change(product, :add) + + # Edit my product + edit_product(product) + expect_product_change(product, :update, 2) + + # Delete my product + delete_product(product) + expect_product_change(product, :remove) + end + end + + def expect_product_change(product, action, expected_qty = 0) + # JS for this page sometimes take more than 2 seconds (default timeout for cappybara) + timeout = 5 + + within('table.index tbody tr', wait: timeout) do + case action + when :add + expect(page).to have_text(product.name, wait: timeout) + when :update + expect(page).to have_text(expected_qty.to_s, wait: timeout) + when :remove + expect(page).not_to have_text(product.name, wait: timeout) + else + raise 'Invalid action' + end + end + end + + def add_product(product) + select2_select product.name, from: 'add_variant_id', search: true + find('button.add_variant').click + end + + def edit_product(product) + find('a.edit-item.icon_link.icon-edit.no-text.with-tip').click + fill_in 'quantity', with: 2 + find("a[data-variant-id='#{product.variants.last.id}'][data-action='save']").click + end + + def delete_product(product) + find("a[data-variant-id='#{product.variants.last.id}'][data-action='remove']").click + click_button 'OK' + end + end + end +end From 8d407b1dc971a021e88c0fc5e2edd8f2a03be429 Mon Sep 17 00:00:00 2001 From: Ahmed Ejaz Date: Sun, 15 Jun 2025 18:19:46 +0500 Subject: [PATCH 07/14] Fix lint issues --- lib/reporting/line_items.rb | 7 ++++++- .../having_producer_products/hub_actions_spec.rb | 12 ++++++++++-- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/lib/reporting/line_items.rb b/lib/reporting/line_items.rb index 6b0750ab0d..1fa5cb56c5 100644 --- a/lib/reporting/line_items.rb +++ b/lib/reporting/line_items.rb @@ -17,7 +17,12 @@ module Reporting def list(line_item_includes = [variant: [:supplier, :product]]) line_items = order_permissions.visible_line_items.in_orders(orders.result) - .order("supplier.name", "product.name", "spree_variants.display_name", "spree_variants.unit_description") + .order( + "supplier.name", + "product.name", + "spree_variants.display_name", + "spree_variants.unit_description" + ) if @params[:supplier_id_in].present? line_items = line_items.supplied_by_any(@params[:supplier_id_in]) diff --git a/spec/system/admin/orders/having_producer_products/hub_actions_spec.rb b/spec/system/admin/orders/having_producer_products/hub_actions_spec.rb index fbba03a01e..5358745720 100644 --- a/spec/system/admin/orders/having_producer_products/hub_actions_spec.rb +++ b/spec/system/admin/orders/having_producer_products/hub_actions_spec.rb @@ -2,7 +2,10 @@ require 'system_helper' -RSpec.describe 'As a hub (producer seller) who have the ability to update orders having their products' do +RSpec.describe ' + As a hub (producer seller) who have the ability to update + orders having their products +' do include AdminHelper include AuthenticationHelper include WebHelper @@ -11,7 +14,12 @@ RSpec.describe 'As a hub (producer seller) who have the ability to update orders let!(:hub1_v1) { create(:variant, supplier_id: hub1.id) } let!(:hub1_v2) { create(:variant, supplier_id: hub1.id) } let(:order_cycle) do - create(:simple_order_cycle, distributors: [distributor], variants: [hub1_v1, hub1_v2], coordinator: distributor) + create( + :simple_order_cycle, + distributors: [distributor], + variants: [hub1_v1, hub1_v2], + coordinator: distributor + ) end let!(:order_containing_hub1_products) do o = create( From 020d90b9579c3b6946aff5340212d4e2090ce5f0 Mon Sep 17 00:00:00 2001 From: Ahmed Ejaz Date: Sat, 21 Jun 2025 16:14:54 +0500 Subject: [PATCH 08/14] Enhance line item management abilities by consolidating permissions for Spree::Order --- app/models/spree/ability.rb | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/app/models/spree/ability.rb b/app/models/spree/ability.rb index 1a72d66980..c11e522852 100644 --- a/app/models/spree/ability.rb +++ b/app/models/spree/ability.rb @@ -415,11 +415,15 @@ module Spree end def add_manage_line_items_abilities(user) - can [:edit_as_producer_only], Spree::Order do |order| - can_edit_as_producer(order, user) - end - - can [:admin, :read, :index, :edit, :update, :bulk_management], Spree::Order do |order| + can [ + :admin, + :read, + :index, + :edit, + :update, + :bulk_management, + :edit_as_producer_only + ], Spree::Order do |order| can_edit_as_producer(order, user) end can [:admin, :index, :create, :destroy, :update], Spree::LineItem do |item| From 765ce68c119c8cd6e0432d6a609b677ee21391e0 Mon Sep 17 00:00:00 2001 From: Ahmed Ejaz Date: Sat, 21 Jun 2025 16:15:31 +0500 Subject: [PATCH 09/14] Add order_id to order controller, variant autocomplete, and search parameters for improved order management --- .../controllers/order_controller.js.coffee | 1 + .../directives/variant_autocomplete.js.coffee | 1 + .../spree/admin/variants_controller.rb | 2 +- app/views/spree/admin/orders/edit.html.haml | 1 + .../scope_variants_for_search.rb | 13 +++++++++++-- .../scope_variants_for_search_spec.rb | 17 +++++++++++++---- 6 files changed, 28 insertions(+), 7 deletions(-) diff --git a/app/assets/javascripts/admin/orders/controllers/order_controller.js.coffee b/app/assets/javascripts/admin/orders/controllers/order_controller.js.coffee index e99cbf3bbd..2624f27ae2 100644 --- a/app/assets/javascripts/admin/orders/controllers/order_controller.js.coffee +++ b/app/assets/javascripts/admin/orders/controllers/order_controller.js.coffee @@ -6,6 +6,7 @@ angular.module("admin.orders").controller "orderCtrl", ($scope, shops, orderCycl $scope.distributor_id = parseInt($attrs.ofnDistributorId) $scope.order_cycle_id = parseInt($attrs.ofnOrderCycleId) $scope.search_variants_as = $attrs.ofnSearchVariantsAs + $scope.order_id = $attrs.ofnOrderId $scope.validOrderCycle = (oc) -> $scope.orderCycleHasDistributor oc, parseInt($scope.distributor_id) diff --git a/app/assets/javascripts/admin/utils/directives/variant_autocomplete.js.coffee b/app/assets/javascripts/admin/utils/directives/variant_autocomplete.js.coffee index efc3b0042f..ea95e0c344 100644 --- a/app/assets/javascripts/admin/utils/directives/variant_autocomplete.js.coffee +++ b/app/assets/javascripts/admin/utils/directives/variant_autocomplete.js.coffee @@ -27,6 +27,7 @@ angular.module("admin.utils").directive "variantAutocomplete", ($timeout) -> eligible_for_subscriptions: scope.eligible_for_subscriptions include_out_of_stock: scope.include_out_of_stock search_variants_as: scope.search_variants_as + order_id: scope.order_id results: (data, page) -> window.variants = data # this is how spree auto complete JS code picks up variants results: data diff --git a/app/controllers/spree/admin/variants_controller.rb b/app/controllers/spree/admin/variants_controller.rb index d78cb60427..7ec309ea93 100644 --- a/app/controllers/spree/admin/variants_controller.rb +++ b/app/controllers/spree/admin/variants_controller.rb @@ -117,7 +117,7 @@ module Spree def variant_search_params params.permit( :q, :distributor_id, :order_cycle_id, :schedule_id, :eligible_for_subscriptions, - :include_out_of_stock, :search_variants_as, + :include_out_of_stock, :search_variants_as, :order_id ).to_h.with_indifferent_access end diff --git a/app/views/spree/admin/orders/edit.html.haml b/app/views/spree/admin/orders/edit.html.haml index 217eb6027c..a0e0d249c0 100644 --- a/app/views/spree/admin/orders/edit.html.haml +++ b/app/views/spree/admin/orders/edit.html.haml @@ -27,6 +27,7 @@ "ofn-distributor-id" => @order.distributor_id, "ofn-order-cycle-id" => @order.order_cycle_id, "ofn-search-variants-as" => (can?(:manage_order_sections, @order) ? 'hub' : 'supplier'), + "ofn-order-id" => @order.id, } = render :partial => 'add_product' if can?(:update, @order) diff --git a/lib/open_food_network/scope_variants_for_search.rb b/lib/open_food_network/scope_variants_for_search.rb index 875b68ba5a..d8e96b2a69 100644 --- a/lib/open_food_network/scope_variants_for_search.rb +++ b/lib/open_food_network/scope_variants_for_search.rb @@ -12,6 +12,7 @@ module OpenFoodNetwork def initialize(params, spree_current_user) @params = params @spree_current_user = spree_current_user + @ability = Spree::Ability.new(spree_current_user) end def search @@ -21,14 +22,14 @@ module OpenFoodNetwork scope_to_schedule if params[:schedule_id] scope_to_order_cycle if params[:order_cycle_id] scope_to_distributor if params[:distributor_id] - scope_to_supplier if params[:search_variants_as] == 'supplier' + scope_to_supplier if scope_to_supplier? @variants end private - attr_reader :params, :spree_current_user + attr_reader :params, :spree_current_user, :ability def search_params { product_name_cont: params[:q], sku_cont: params[:q], product_sku_cont: params[:q] } @@ -47,6 +48,10 @@ module OpenFoodNetwork @distributor ||= Enterprise.find params[:distributor_id] end + def order + @order ||= Spree::Order.find(params[:order_id]) + end + def scope_to_schedule @variants = @variants.in_schedule(params[:schedule_id]) end @@ -102,5 +107,9 @@ module OpenFoodNetwork def scope_to_supplier @variants = @variants.where(supplier_id: spree_current_user.enterprises.ids) end + + def scope_to_supplier? + params[:search_variants_as] == 'supplier' && ability.can?(:edit_as_producer_only, order) + end end end diff --git a/spec/lib/open_food_network/scope_variants_for_search_spec.rb b/spec/lib/open_food_network/scope_variants_for_search_spec.rb index ccbed15dd0..bbed2af712 100644 --- a/spec/lib/open_food_network/scope_variants_for_search_spec.rb +++ b/spec/lib/open_food_network/scope_variants_for_search_spec.rb @@ -67,9 +67,12 @@ RSpec.describe OpenFoodNetwork::ScopeVariantsForSearch do it "returns all products distributed through that distributor" do expect{ result }.to query_database [ + "Enterprise Load", + "EnterpriseGroup Load", + "OrderCycle Exists?", "Enterprise Load", "VariantOverride Load", - "SQL", + "SQL" ] expect(result).to include v4 @@ -183,13 +186,19 @@ RSpec.describe OpenFoodNetwork::ScopeVariantsForSearch do end context "when search is done by the producer allowing to edit orders" do - let(:params) { { q: "product", search_variants_as: 'supplier' } } + let(:order) { create(:order) } + let(:params) { { q: "product", search_variants_as: 'supplier', order_id: order.id } } let(:producer) { create(:supplier_enterprise) } + let(:ability) { instance_double('Spree::Ability', can?: true) } let!(:spree_current_user) { - instance_double('Spree::User', enterprises: Enterprise.where(id: producer.id), - can_manage_line_items_in_orders_only?: true) + instance_double('Spree::User', enterprises: Enterprise.where(id: producer.id)) } + before do + allow(Spree::Ability).to receive(:new).with(spree_current_user).and_return(ability) + allow(ability).to receive(:can?).with(:edit_as_producer_only, order).and_return(true) + end + it "returns products distributed by distributors allowing producers to edit orders" do v1.supplier_id = producer.id v2.supplier_id = producer.id From c648249160df0f73ff9d603dc4a0b54d59763247 Mon Sep 17 00:00:00 2001 From: Ahmed Ejaz Date: Sat, 21 Jun 2025 16:15:56 +0500 Subject: [PATCH 10/14] Refactor order view specs to improve clarity in expectations --- .../having_producer_products/hub_actions_spec.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/spec/system/admin/orders/having_producer_products/hub_actions_spec.rb b/spec/system/admin/orders/having_producer_products/hub_actions_spec.rb index 5358745720..b51b09d284 100644 --- a/spec/system/admin/orders/having_producer_products/hub_actions_spec.rb +++ b/spec/system/admin/orders/having_producer_products/hub_actions_spec.rb @@ -43,7 +43,7 @@ RSpec.describe ' context "when no distributor allow the producer to edit orders" do let(:distributor) { create(:distributor_enterprise) } - it "should not allow producer to view orders page" do + it "does not allow producer to view orders page" do expect(page).to have_content 'NO ORDERS FOUND' end end @@ -52,7 +52,7 @@ RSpec.describe ' let(:distributor) { create(:distributor_enterprise, enable_producers_to_edit_orders: true) } context "when distributor doesn't allow to view customer details" do - it "should allow producer to view orders page with HIDDEN customer details" do + it "allows producer to view orders page with HIDDEN customer details" do within('#listing_orders tbody') do expect(page).to have_selector('tr', count: 1) # Only one order # One for Email, one for Name @@ -69,7 +69,7 @@ RSpec.describe ' show_customer_names_to_suppliers: true ) end - it "should allow producer to view orders page with customer details" do + it "allows producer to view orders page with customer details" do within('#listing_orders tbody') do name = order.bill_address&.full_name_for_sorting email = order.email @@ -94,7 +94,7 @@ RSpec.describe ' context "when no distributor allow the producer to edit orders" do let(:distributor) { create(:distributor_enterprise) } - it "should not allow producer to view orders page" do + it "does not allow producer to view orders page" do expect(page).to have_content 'Unauthorized' end end @@ -103,7 +103,7 @@ RSpec.describe ' let(:distributor) { create(:distributor_enterprise, enable_producers_to_edit_orders: true) } let(:product) { hub1_v2.product } - it "should allow me to manage my products in the order" do + it "allows me to manage my products in the order" do expect(page).to have_content 'Add Product' # Add my product From 1b9d64ad5ea0f2bb4d78bfa523d38b2c1c76eda0 Mon Sep 17 00:00:00 2001 From: Ahmed Ejaz Date: Sat, 21 Jun 2025 16:39:15 +0500 Subject: [PATCH 11/14] Refactor search functionality in variants controller spec to include order_id for improved filtering --- .../spree/admin/variants_controller_spec.rb | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/spec/controllers/spree/admin/variants_controller_spec.rb b/spec/controllers/spree/admin/variants_controller_spec.rb index 0eb2350265..4e74f61278 100644 --- a/spec/controllers/spree/admin/variants_controller_spec.rb +++ b/spec/controllers/spree/admin/variants_controller_spec.rb @@ -137,13 +137,6 @@ RSpec.describe Spree::Admin::VariantsController do expect(variant).to have_received(:destroy) end - describe "#search" do - it "filters by distributor and supplier1 products" do - spree_get :search, q: 'Prod', distributor_id: d.id.to_s, search_variants_as: 'supplier' - expect(assigns(:variants)).to eq([v1]) - end - end - it 'shows a success flash message' do spree_delete :destroy, id: variant.id, product_id: variant.product.id, format: 'html' @@ -186,7 +179,12 @@ RSpec.describe Spree::Admin::VariantsController do describe "#search" do it "filters by distributor and supplier1 products" do - spree_get :search, q: 'Prod', distributor_id: d.id.to_s + order = d.distributed_orders.first + spree_get :search, + q: 'Prod', + distributor_id: d.id.to_s, + search_variants_as: 'supplier', + order_id: order.id.to_s expect(assigns(:variants)).to eq([v1]) end end From 7725fae99289e6f004e2dbfc64599a3b0aa7932a Mon Sep 17 00:00:00 2001 From: Ahmed Ejaz Date: Sun, 29 Jun 2025 19:13:31 +0500 Subject: [PATCH 12/14] Refactor order cycle and order management abilities to improve producer edit permissions --- app/models/spree/ability.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/spree/ability.rb b/app/models/spree/ability.rb index c11e522852..e0214eea66 100644 --- a/app/models/spree/ability.rb +++ b/app/models/spree/ability.rb @@ -263,7 +263,7 @@ module Spree def add_order_cycle_management_abilities(user) can [:admin, :index], OrderCycle do |order_cycle| OrderCycle.visible_by(user).include?(order_cycle) || - order_cycle.orders.any? { |order| can_edit_as_producer(order, user) } + order_cycle.orders.editable_by_producers(user.enterprises).exists? end can [ @@ -294,10 +294,10 @@ module Spree cannot?(:manage_order_sections, order) && can_edit_as_producer(order, user) end - can [:index], Spree::Order do |order| + can [:index], Spree::Order do user.admin? || user.enterprises.any?(&:is_distributor) || - can_edit_as_producer(order, user) + user.enterprises.distributors.where(enable_producers_to_edit_orders: true).exist? end can [:create], Spree::Order From 4b19d38c58a4ccaf8b7d5b122a1eeffdb7e0610e Mon Sep 17 00:00:00 2001 From: Ahmed Ejaz Date: Sun, 29 Jun 2025 19:13:44 +0500 Subject: [PATCH 13/14] Refactor variant creation in hub actions spec to use supplier association for clarity --- .../admin/orders/having_producer_products/hub_actions_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/system/admin/orders/having_producer_products/hub_actions_spec.rb b/spec/system/admin/orders/having_producer_products/hub_actions_spec.rb index b51b09d284..d5ca241d22 100644 --- a/spec/system/admin/orders/having_producer_products/hub_actions_spec.rb +++ b/spec/system/admin/orders/having_producer_products/hub_actions_spec.rb @@ -11,8 +11,8 @@ RSpec.describe ' include WebHelper let!(:hub1) { create(:distributor_enterprise, name: 'My hub1') } - let!(:hub1_v1) { create(:variant, supplier_id: hub1.id) } - let!(:hub1_v2) { create(:variant, supplier_id: hub1.id) } + let!(:hub1_v1) { create(:variant, supplier: hub1) } + let!(:hub1_v2) { create(:variant, supplier: hub1) } let(:order_cycle) do create( :simple_order_cycle, From 838e88a502741d89d128b93a44c3d3fd10005942 Mon Sep 17 00:00:00 2001 From: Ahmed Ejaz Date: Sun, 29 Jun 2025 19:41:41 +0500 Subject: [PATCH 14/14] Refactor display_value_for_producer method to use Spree::Ability for supplier edit permissions --- app/serializers/api/admin/order_serializer.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/app/serializers/api/admin/order_serializer.rb b/app/serializers/api/admin/order_serializer.rb index b1b4740786..e8fad05df9 100644 --- a/app/serializers/api/admin/order_serializer.rb +++ b/app/serializers/api/admin/order_serializer.rb @@ -102,9 +102,8 @@ module Api end def display_value_for_producer(order, value) - filter_by_supplier = - order.distributor&.enable_producers_to_edit_orders && - options[:current_user]&.can_manage_line_items_in_orders_only? + @ability ||= Spree::Ability.new(options[:current_user]) + filter_by_supplier = @ability.can?(:edit_as_producer_only, order) return value unless filter_by_supplier if order.distributor&.show_customer_names_to_suppliers