-
-
Notifications
You must be signed in to change notification settings - Fork 719
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
filter distributors before listing on checkout options #11079
filter distributors before listing on checkout options #11079
Conversation
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. |
aa58562
to
b072da1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
d89483f
to
c3d49bf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it's good, but I must admit I don't know much about coordinator and distributor.
@@ -94,24 +94,27 @@ | |||
end | |||
|
|||
describe "distibutors that have editable shipping/payment methods" do | |||
let(:result) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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