filter distributors before listing on checkout options #11079
Conversation
abdellani
force-pushed
the
fix-distributors-listing-on-checkout-options
branch
4 times, most recently
from
090203e
to
aa58562
Compare
|
I think we should add a couple more tests for the two cases mentioned in the issue that explicitly set up this scenario where there are multiple suppliers in the OC and the current user manages a supplier that's assigned to the OC but isn't the coordinator. |
abdellani
force-pushed
the
fix-distributors-listing-on-checkout-options
branch
from
aa58562
to
b072da1
Compare
app/helpers/order_cycles_helper.rb
Outdated
| order_cycle.distributors.select do |distributor| | ||
| distributor.in? Enterprise.managed_by(spree_current_user) | ||
| end |
There was a problem hiding this comment.
I would hope that Enterprise.managed_by(spree_current_user) is cached but it wouldn't hurt to store it in a variable first. It could also be represented as a union:
| order_cycle.distributors.select do |distributor| | |
| distributor.in? Enterprise.managed_by(spree_current_user) | |
| end | |
| order_cycle.distributors & Enterprise.managed_by(spree_current_user) |
But super-admin can see all enterprises which would make this list very long. So we could add a condition to shortcut super admin.
Oh, hang on, I'm just realising that we completely missed the greatest feature of ActiveRecord scopes: chaining!
| order_cycle.distributors.select do |distributor| | |
| distributor.in? Enterprise.managed_by(spree_current_user) | |
| end | |
| order_cycle.distributors.managed_by(spree_current_user) |
I reckon that this change is worth doing before merging.
There was a problem hiding this comment.
Okay, tried that and it turns out that order_cycle.distributors is an array, not a scope. Oh, hang on, just in the spec...
There was a problem hiding this comment.
I added a few commits. That's my suggestion but I may have overlooked something. This needs another review by someone else, including @abdellani. If you are happy with this, then move it to Test Ready.
It's more realistic and doesn't break with the next change.
mkllnk
force-pushed
the
fix-distributors-listing-on-checkout-options
branch
from
d89483f
to
c3d49bf
Compare
| @@ -94,24 +94,27 @@ | |||
| end | |||
|
|
|||
| describe "distibutors that have editable shipping/payment methods" do | |||
| let(:result) { | |||
There was a problem hiding this comment.
I would use subject here :
| let(:result) { | |
| subject(:result) { | |
| helper.distributors_with_editable_shipping_and_payment_methods(order_cycle) | |
| } |
|
Hi @abdellani , it looks like we just need to you confirm Maikel's latest changes and move to Test Ready if you're happy with them. |
|
Ok, it looks good to me :) |
|
Hey @abdellani , I've checked now, that supplier enterprises which do not coordinated the OC cannot view/edit payment/shipping methods from the other enterprises:
Also, I've checked that a distributor which is not the coordinator of the OC, can only see/edit their own shipping/payment methods:
This looks great!! |
What? Why?
4. Checkout Options) Multi-supplier OC: non-coordinator suppliers see info on other suppliers #10173What should we test?
Release notes
Changelog Category: Technical changes
The title of the pull request will be included in the release notes.
Dependencies
Documentation updates