Skip to content
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

Fix rubocop Style/Send group #12703

Merged
merged 1 commit into from
Jul 29, 2024
Merged
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
33 changes: 0 additions & 33 deletions .rubocop_todo.yml
Original file line number Diff line number Diff line change
Expand Up @@ -872,39 +872,6 @@ Style/ReturnNilInPredicateMethodDefinition:
- 'app/serializers/api/admin/customer_serializer.rb'
- 'engines/order_management/app/services/order_management/subscriptions/validator.rb'

# Offense count: 207
Style/Send:
Exclude:
- 'spec/controllers/admin/subscriptions_controller_spec.rb'
- 'spec/controllers/payment_gateways/paypal_controller_spec.rb'
- 'spec/controllers/spree/admin/base_controller_spec.rb'
- 'spec/controllers/spree/orders_controller_spec.rb'
- 'spec/helpers/order_cycles_helper_spec.rb'
- 'spec/jobs/subscription_confirm_job_spec.rb'
- 'spec/jobs/subscription_placement_job_spec.rb'
- 'spec/lib/open_food_network/address_finder_spec.rb'
- 'spec/lib/open_food_network/enterprise_fee_applicator_spec.rb'
- 'spec/lib/open_food_network/enterprise_fee_calculator_spec.rb'
- 'spec/lib/open_food_network/order_cycle_form_applicator_spec.rb'
- 'spec/lib/open_food_network/permissions_spec.rb'
- 'spec/lib/open_food_network/tag_rule_applicator_spec.rb'
- 'spec/lib/reports/xero_invoices_report_spec.rb'
- 'spec/lib/stripe/webhook_handler_spec.rb'
- 'spec/models/calculator/weight_spec.rb'
- 'spec/models/enterprise_spec.rb'
- 'spec/models/exchange_spec.rb'
- 'spec/models/spree/order_inventory_spec.rb'
- 'spec/models/spree/payment_spec.rb'
- 'spec/models/spree/return_authorization_spec.rb'
- 'spec/models/tag_rule/filter_order_cycles_spec.rb'
- 'spec/models/tag_rule/filter_payment_methods_spec.rb'
- 'spec/models/tag_rule/filter_products_spec.rb'
- 'spec/models/tag_rule/filter_shipping_methods_spec.rb'
- 'spec/services/cart_service_spec.rb'
- 'spec/services/products_renderer_spec.rb'
- 'spec/services/variant_units/option_value_namer_spec.rb'
- 'spec/support/localized_number_helper.rb'

# Offense count: 3
# This cop supports unsafe autocorrection (--autocorrect-all).
Style/SlicingWithRange:
Expand Down
4 changes: 2 additions & 2 deletions spec/controllers/admin/subscriptions_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -754,7 +754,7 @@
end

it "assigns data to instance variables" do
controller.send(:load_form_data)
controller.__send__(:load_form_data)
Copy link
Collaborator

@chahmedejaz chahmedejaz Jul 23, 2024

Choose a reason for hiding this comment

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

Just my two cents here:

  • I believe the use of __send__ method should be avoided as it breaks the encapsulation principle of OOP. Private methods are meant to be used within the classes they are defined in, and this method just invokes the methods that were not intended to be used outside of the class.
  • I prefer to use its public counterpart (public_send) where there is a need to call the methods dynamically.
  • This PR can be the best opportunity to identify these private method calls and make them either public or see a better alternative.
  • What's your take on this, @openfoodfoundation/core-devs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey - thanks for the review. There were similar thoughts which occurred to me as well - I had a quick look at a few of the methods in question the ones I looked at seemed to be relatively short and straightforward (but also made sense as private methods eg. load_form_data which just sets a few instance variables for the controller class).
But really it made me wonder if these private methods need to be tested at all - as long as the public methods that call them are tested - but there's also an argument that it could make those tests more complex too. I'd be curious to hear others' opinions as well!

Copy link
Collaborator

@rioug rioug Jul 29, 2024

Choose a reason for hiding this comment

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

I agree with what both of you are saying. We should really test the logic of these private methods via the public interface of the object. And I believe, it can reveal design issue in the code. We should remove these send but this is not really the scope of this PR.

expect(assigns(:customers)).to include customer1, customer2
expect(assigns(:schedules)).to eq [schedule]
expect(assigns(:order_cycles)).to eq [order_cycle]
Expand All @@ -769,7 +769,7 @@
}

it "only loads Stripe and Cash payment methods" do
controller.send(:load_form_data)
controller.__send__(:load_form_data)
expect(assigns(:payment_methods)).to include payment_method, stripe
expect(assigns(:payment_methods)).not_to include paypal
end
Expand Down
4 changes: 2 additions & 2 deletions spec/controllers/payment_gateways/paypal_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -109,11 +109,11 @@ module PaymentGateways
describe '#expire_current_order' do
it 'empties the order_id of the session' do
expect(session).to receive(:[]=).with(:order_id, nil)
controller.send(:expire_current_order)
controller.__send__(:expire_current_order)
end

it 'resets the @current_order ivar' do
controller.send(:expire_current_order)
controller.__send__(:expire_current_order)
expect(controller.instance_variable_get(:@current_order)).to be_nil
end
end
Expand Down
17 changes: 9 additions & 8 deletions spec/controllers/spree/admin/base_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ def index
it "passes a prefix to the serializer method and renders with serializer" do
expect(controller).to receive(:serializer).with(prefix) { "SerializerClass" }
expect(controller).to receive(:render).with({ json: data, serializer: "SerializerClass" })
controller.send(:render_as_json, data, ams_prefix: prefix)
controller.__send__(:render_as_json, data, ams_prefix: prefix)
end
end

Expand All @@ -35,7 +35,7 @@ def index
it "does not pass a prefix to the serializer method and renders with serializer" do
expect(controller).to receive(:serializer).with(prefix) { "SerializerClass" }
expect(controller).to receive(:render).with({ json: data, serializer: "SerializerClass" })
controller.send(:render_as_json, data, ams_prefix: prefix)
controller.__send__(:render_as_json, data, ams_prefix: prefix)
end
end
end
Expand All @@ -51,7 +51,7 @@ def index
expect(controller).to receive(:render).with(
{ json: data, each_serializer: "SerializerClass" }
)
controller.send(:render_as_json, data, ams_prefix: prefix)
controller.__send__(:render_as_json, data, ams_prefix: prefix)
end
end

Expand All @@ -63,7 +63,7 @@ def index
expect(controller).to receive(:render).with(
{ json: data, each_serializer: "SerializerClass" }
)
controller.send(:render_as_json, data, ams_prefix: prefix)
controller.__send__(:render_as_json, data, ams_prefix: prefix)
end
end
end
Expand All @@ -79,21 +79,22 @@ def index
context "when a prefix is passed in" do
context "and the prefix appears in the whitelist" do
it "returns the requested serializer" do
expect(controller.send(:serializer,
'allowed_prefix')).to eq Api::Admin::AllowedPrefixBaseSerializer
expect(
controller.__send__(:serializer, 'allowed_prefix')
).to eq Api::Admin::AllowedPrefixBaseSerializer
end
end

context "and the prefix does not appear in the whitelist" do
it "raises an error" do
expect{ controller.send(:serializer, 'other_prefix') }.to raise_error RuntimeError
expect{ controller.__send__(:serializer, 'other_prefix') }.to raise_error RuntimeError
end
end
end

context "when no prefix is passed in" do
it "returns the default serializer" do
expect(controller.send(:serializer, nil)).to eq Api::Admin::BaseSerializer
expect(controller.__send__(:serializer, nil)).to eq Api::Admin::BaseSerializer
end
end
end
Expand Down
10 changes: 5 additions & 5 deletions spec/controllers/spree/orders_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -389,7 +389,7 @@

context "when no order id is given in params" do
it "returns the current_order" do
expect(controller.send(:order_to_update)).to eq current_order
expect(controller.__send__(:order_to_update)).to eq current_order
end
end

Expand All @@ -402,7 +402,7 @@
let!(:order) { create(:order) }

it "returns nil" do
expect(controller.send(:order_to_update)).to eq nil
expect(controller.__send__(:order_to_update)).to eq nil
end
end

Expand All @@ -413,7 +413,7 @@
before { allow(controller).to receive(:can?).with(:update, order) { false } }

it "returns nil" do
expect(controller.send(:order_to_update)).to eq nil
expect(controller.__send__(:order_to_update)).to eq nil
end
end

Expand All @@ -422,7 +422,7 @@

context "and the order is not editable" do
it "returns nil" do
expect(controller.send(:order_to_update)).to eq nil
expect(controller.__send__(:order_to_update)).to eq nil
end
end

Expand All @@ -441,7 +441,7 @@
end

it "returns the order" do
expect(controller.send(:order_to_update)).to eq order
expect(controller.__send__(:order_to_update)).to eq order
end
end
end
Expand Down
12 changes: 8 additions & 4 deletions spec/helpers/order_cycles_helper_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -44,25 +44,29 @@

it "returns enterprises without shipping methods as disabled" do
create(:payment_method, distributors: [e])
expect(helper.send(:validated_enterprise_options, [e], shipping_and_payment_methods: true))
expect(helper.__send__(:validated_enterprise_options, [e],
shipping_and_payment_methods: true))
.to eq [['enterprise (no shipping methods)', e.id, { disabled: true }]]
end

it "returns enterprises without payment methods as disabled" do
create(:shipping_method, distributors: [e])
expect(helper.send(:validated_enterprise_options, [e], shipping_and_payment_methods: true))
expect(helper.__send__(:validated_enterprise_options, [e],
shipping_and_payment_methods: true))
.to eq [['enterprise (no payment methods)', e.id, { disabled: true }]]
end

it "returns enterprises with unavailable payment methods as disabled" do
create(:shipping_method, distributors: [e])
create(:payment_method, distributors: [e], active: false)
expect(helper.send(:validated_enterprise_options, [e], shipping_and_payment_methods: true))
expect(helper.__send__(:validated_enterprise_options, [e],
shipping_and_payment_methods: true))
.to eq [['enterprise (no payment methods)', e.id, { disabled: true }]]
end

it "returns enterprises with neither shipping nor payment methods as disabled" do
expect(helper.send(:validated_enterprise_options, [e], shipping_and_payment_methods: true))
expect(helper.__send__(:validated_enterprise_options, [e],
shipping_and_payment_methods: true))
.to eq [['enterprise (no shipping or payment methods)', e.id, { disabled: true }]]
end
end
Expand Down
20 changes: 10 additions & 10 deletions spec/jobs/subscription_confirm_job_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
placed_at: 5.minutes.ago)
end
let!(:order) { proxy_order.initialise_order! }
let(:proxy_orders) { job.send(:unconfirmed_proxy_orders) }
let(:proxy_orders) { job.__send__(:unconfirmed_proxy_orders) }

before do
Orders::WorkflowService.new(order).complete!
Expand Down Expand Up @@ -124,7 +124,7 @@

it "returns closed order cycles whose orders_close_at " \
"or updated_at date is within the last hour" do
order_cycles = job.send(:recently_closed_order_cycles)
order_cycles = job.__send__(:recently_closed_order_cycles)
expect(order_cycles).to include order_cycle3, order_cycle4
expect(order_cycles).not_to include order_cycle1, order_cycle2, order_cycle5
end
Expand Down Expand Up @@ -176,14 +176,14 @@
end

it "runs the charges in offline mode" do
job.send(:confirm_order!, order)
job.__send__(:confirm_order!, order)
expect(stripe_sca_payment_method.provider).to have_received(:purchase)
end

it "uses #capture if the payment is already authorized" do
allow(stripe_sca_payment).to receive(:preauthorized?) { true }
expect(stripe_sca_payment_method.provider).to receive(:capture)
job.send(:confirm_order!, order)
job.__send__(:confirm_order!, order)
end

it "authorizes the payment with Stripe" do
Expand All @@ -192,7 +192,7 @@
expect(OrderManagement::Order::StripeScaPaymentAuthorize).
to receive_message_chain(:new, :call!) { true }

job.send(:confirm_order!, order)
job.__send__(:confirm_order!, order)
end
end
end
Expand All @@ -216,7 +216,7 @@
it "sends a failed payment email" do
expect(job).to receive(:send_failed_payment_email)
expect(job).not_to receive(:send_confirmation_email)
job.send(:confirm_order!, order)
job.__send__(:confirm_order!, order)
end
end

Expand All @@ -233,7 +233,7 @@
expect(job).to receive(:send_failed_payment_email)
expect(job).not_to receive(:send_confirmation_email)
expect(job).not_to receive(:send_payment_authorization_emails)
job.send(:confirm_order!, order)
job.__send__(:confirm_order!, order)
end
end

Expand All @@ -247,7 +247,7 @@
end

it "sends only a subscription confirm email, no regular confirmation emails" do
expect{ job.send(:confirm_order!, order) }
expect{ job.__send__(:confirm_order!, order) }
.not_to have_enqueued_mail(Spree::OrderMailer, :confirm_email_for_customer)

expect(job).to have_received(:send_confirmation_email).once
Expand All @@ -268,7 +268,7 @@
it "records a success and sends the email" do
expect(order).to receive(:update_order!)
expect(job).to receive(:record_success).with(order).once
job.send(:send_confirmation_email, order)
job.__send__(:send_confirmation_email, order)
expect(SubscriptionMailer).to have_received(:confirmation_email).with(order)
expect(mail_mock).to have_received(:deliver_now)
end
Expand All @@ -285,7 +285,7 @@
it "records and logs an error and sends the email" do
expect(order).to receive(:update_order!)
expect(job).to receive(:record_and_log_error).with(:failed_payment, order, nil).once
job.send(:send_failed_payment_email, order)
job.__send__(:send_failed_payment_email, order)
expect(SubscriptionMailer).to have_received(:failed_payment_email).with(order)
expect(mail_mock).to have_received(:deliver_now)
end
Expand Down
20 changes: 10 additions & 10 deletions spec/jobs/subscription_placement_job_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,29 +25,29 @@
} # OK

it "ignores proxy orders where the OC has closed" do
expect(job.send(:proxy_orders)).to include proxy_order
expect(job.__send__(:proxy_orders)).to include proxy_order
proxy_order.update!(order_cycle_id: order_cycle2.id)
expect(job.send(:proxy_orders)).not_to include proxy_order
expect(job.__send__(:proxy_orders)).not_to include proxy_order
end

it "ignores proxy orders for paused or cancelled subscriptions" do
expect(job.send(:proxy_orders)).to include proxy_order
expect(job.__send__(:proxy_orders)).to include proxy_order
subscription.update!(paused_at: 1.minute.ago)
expect(job.send(:proxy_orders)).not_to include proxy_order
expect(job.__send__(:proxy_orders)).not_to include proxy_order
subscription.update!(paused_at: nil)
expect(job.send(:proxy_orders)).to include proxy_order
expect(job.__send__(:proxy_orders)).to include proxy_order
subscription.update!(canceled_at: 1.minute.ago)
expect(job.send(:proxy_orders)).not_to include proxy_order
expect(job.__send__(:proxy_orders)).not_to include proxy_order
end

it "ignores proxy orders that have been marked as cancelled or placed" do
expect(job.send(:proxy_orders)).to include proxy_order
expect(job.__send__(:proxy_orders)).to include proxy_order
proxy_order.update!(canceled_at: 5.minutes.ago)
expect(job.send(:proxy_orders)).not_to include proxy_order
expect(job.__send__(:proxy_orders)).not_to include proxy_order
proxy_order.update!(canceled_at: nil)
expect(job.send(:proxy_orders)).to include proxy_order
expect(job.__send__(:proxy_orders)).to include proxy_order
proxy_order.update!(placed_at: 5.minutes.ago)
expect(job.send(:proxy_orders)).not_to include proxy_order
expect(job.__send__(:proxy_orders)).not_to include proxy_order
end
end

Expand Down
Loading
Loading