Allow producer to edit their products on hubs' orders #13113
Conversation
chahmedejaz
force-pushed
the
task/13031-allow-producers-to-edit-orders
branch
from
764ea78 to
ae86821
Compare
chahmedejaz
force-pushed
the
task/13031-allow-producers-to-edit-orders
branch
from
2e12ce7 to
a747c88
Compare
chahmedejaz
force-pushed
the
task/13031-allow-producers-to-edit-orders
branch
from
d9f0454 to
e4ef890
Compare
chahmedejaz
changed the title
|
|
||
| # Users can manage line items in orders if they have producer enterprise and | ||
| # any of order distributors allow them to edit their orders. | ||
| def can_manage_line_items_in_orders?(user) |
There was a problem hiding this comment.
Adding the same user method names here for the sake of writing ability specs to better show the assigned abilities
There was a problem hiding this comment.
Hi @chahmedejaz It's a good start, but I would like to see more unit test. It's a bit hard to follow if all changes are covered by some specs as you added them at the end. In general it's better to commit the specs whit the actual changes, at least the unit test.
I am wondering if we should also add a some system spec, at least for the order edit page.
| orders = Spree::Order. | ||
| where(managed_orders_where_values. | ||
| or(coordinated_orders_where_values)) | ||
| orders = if @user.can_manage_line_items_in_orders_only? |
There was a problem hiding this comment.
Shouldn't that be can_manage_line_items_in_orders?
There was a problem hiding this comment.
It can be, however, the _only method would make sure it won't be able to manage the orders and can only manage line items in the orders.
| def is_producer | ||
| is_primary_producer && sells == 'none' | ||
| end | ||
|
|
| def can_manage_line_items_in_orders?(user) | ||
| user.can_manage_line_items_in_orders? |
| # Users can manage orders if they have a sells own/any enterprise. or is admin | ||
| def can_manage_orders? | ||
| @can_manage_orders ||= (enterprises.pluck(:sells).intersect?(%w(own any)) or admin?) | ||
| end | ||
|
|
||
| # Users can manage line items in orders if they have producer enterprise and | ||
| # any of order distributors allow them to edit their orders. | ||
| def can_manage_line_items_in_orders? | ||
| @can_manage_line_items_in_orders ||= begin | ||
| has_any_producer = enterprises.any?(&:is_producer) | ||
| has_producer_editable_orders = Spree::Order.editable_by_producers(enterprises).exists? | ||
| has_any_producer && has_producer_editable_orders | ||
| end | ||
| end |
| def can_manage_line_items_in_orders_only? | ||
| !can_manage_orders? && can_manage_line_items_in_orders? | ||
| end |
| if @user.can_manage_line_items_in_orders_only? | ||
| Spree::LineItem.editable_by_producers( | ||
| @permissions.managed_enterprises.select("enterprises.id") | ||
| ) | ||
| else | ||
| Spree::LineItem.where(order_id: editable_orders.select(:id)) | ||
| end |
| @@ -20,13 +21,14 @@ def search | |||
| 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? | |||
| @@ -4,7 +4,7 @@ | |||
|
|
|||
| module Permissions | |||
| RSpec.describe Order do | |||
| let(:user) { double(:user) } | |||
| let(:user) { double(:user, can_manage_line_items_in_orders_only?: false) } | |||
There was a problem hiding this comment.
We might as well improve the spec here by using instance_doubleinstead of double . instance_double actually checks if the stubbed method would exist on an actual instance see: https://rspec.info/features/3-13/rspec-mocks/verifying-doubles/instance-doubles/
There was a problem hiding this comment.
Sure, will look into this improvement. 👍🏻
|
Thanks @rioug for the feedback. Let me incorporate and get back to you. |
What? Why?
What should we test?
Release notes
Changelog Category (reviewers may add a label for the release notes):