Favor #public_send over #send using Rubocop's cop #2618

sauloperez · 2018-08-30T16:53:26Z

What? Why?

Devs keep using #send although that method does not preserve
private/protected visibility. Watching after this turned out to be quite
time-consuming while doing code review.

Currently, the Style/Send cop doesn't enforce #public_send however
(that's what we want). It simply discourages the use of #send. See
rubocop/rubocop#2081 (comment)
for details. So a new entry on the Code Conventions doc has been added
to overcome this limitation:
https://github.com/openfoodfoundation/openfoodnetwork/wiki/Code-Conventions#prefer-public_send-over-send

What should we test?

A smoke test, including:

Product import
Subscriptions
Uploading a new logo from Configuration > Contents
Inventory items (from admin/variant_overrides URL)
Capture (or any other modification to the payment) an order's payment (from admin/orders/:order_id/payments URL)
Enterprise fees

Although very little, these features are affected by this change.

Release notes

Ensure devs use #public_send over #send by enabling a Rubocop cop.

Changelog Category: Added

Devs keep using `#send` although that method does not preserve private/protected visibility. Watching after this turned out to be quite time-consuming while doing code review. Currently, the Style/Send cop doesn't enforce `#public_send` however (that's what we want). It simply discourages the use of #send. See rubocop/rubocop#2081 (comment) for details. So a new entry on the Code Conventions doc has been added to overcome this limitation: https://github.com/openfoodfoundation/openfoodnetwork/wiki/Code-Conventions#prefer-public_send-over-send

luisramos0

Thank you!

luisramos0 · 2018-08-30T18:58:15Z

There are 50 warnings, this means that when a dev touches one of these files in a PR, they will have to fix it to the new convention. Correct?

sauloperez · 2018-08-31T08:51:29Z

AFAIK that Cop doesn't have autocorrect but I can try to fix them

sauloperez · 2018-08-31T10:14:22Z

app/controllers/shop_controller.rb

@@ -44,7 +44,7 @@ def changeable_orders_alert
  private

  def filtered_json(products_json)
-    if applicator.send(:rules).any?
+    if applicator.rules.any?


I made this one public. If we use it from outside, it's part of applicator's public API

sauloperez · 2018-08-31T10:15:07Z

app/controllers/spree/admin/payment_methods_controller_decorator.rb

@@ -10,7 +10,7 @@ module Admin
      # Only show payment methods that user has access to and sort by distributor name
      # ! Redundant code copied from Spree::Admin::ResourceController with modifications marked
      def collection
-        return parent.send(controller_name) if parent_data.present?
+        return parent.controller_name if parent_data.present?


controller_name is a public method

The original did not call send(:controller_name). Doesn't it call a method with the same name as the controller?

💩 yes, I went too fast on this one. Thanks for the heads-up!

sauloperez · 2018-08-31T10:15:56Z

app/helpers/application_helper.rb

@@ -15,7 +15,7 @@ def ng_form_for(name, *args, &block)
  # spree.foo_path in any view rendered from non-spree-namespaced controllers.
  def method_missing(method, *args, &block)
    if (method.to_s.end_with?('_path') || method.to_s.end_with?('_url')) && spree.respond_to?(method)
-      spree.send(method, *args)
+      spree.public_send(method, *args)


Rails' URL helpers are public

sauloperez · 2018-08-31T10:18:09Z

app/models/model_set.rb

@@ -12,7 +12,7 @@ def initialize(klass, collection, attributes={}, reject_if=nil, delete_if=nil)
    @collection = attributes[:collection] if attributes[:collection]

    attributes.each do |name, value|
-      send("#{name}=", value)
+      public_send("#{name}=", value)


the setters of a model are meant to be public

sauloperez · 2018-08-31T10:21:23Z

app/models/proxy_order.rb

@@ -41,7 +41,7 @@ def resume
    return false unless order_cycle.orders_close_at.andand > Time.zone.now
    transaction do
      update_column(:canceled_at, nil)
-      order.send('resume') if order
+      order.resume if order


these two methods are public

sauloperez · 2018-08-31T10:23:31Z

app/services/order_syncer.rb

@@ -83,7 +83,7 @@ def relevant_address_attrs

  def addresses_match?(order_address, subscription_address)
    relevant_address_attrs.all? do |attr|
-      order_address[attr] == subscription_address.send("#{attr}_was") ||
+      order_address[attr] == subscription_address.public_send("#{attr}_was") ||


AR's dirty attributes API is public

sauloperez · 2018-08-31T10:27:11Z

lib/discourse/single_sign_on.rb

@@ -42,7 +42,7 @@ def self.parse(payload, sso_secret = nil)
        if BOOLS.include? k
          val = ["true", "false"].include?(val) ? val == "true" : nil
        end
-        sso.send("#{k}=", val)
+        sso.public_send("#{k}=", val)


each of these K are defined as public attr_accessor in line 11

sauloperez · 2018-08-31T10:29:42Z

lib/open_food_network/paperclippable.rb

+      base.extend(ActiveModel::Naming)
+      base.extend(ActiveModel::Callbacks)
+      base.include(ActiveModel::Validations)
+      base.include(Paperclip::Glue)


extend and include are public Module methods in Ruby 2.1.5 https://ruby-doc.org/core-2.1.5/Module.html#method-i-include

sauloperez · 2018-08-31T10:30:47Z

lib/open_food_network/paperclippable.rb

-      base.send :define_model_callbacks, :save, only: [:after]
-      base.send :define_model_callbacks, :commit, only: [:after]
-      base.send :define_model_callbacks, :destroy, only: [:before, :after]
+      base.define_model_callbacks(:save, only: [:after])


it is public https://apidock.com/rails/ActiveModel/Callbacks/define_model_callbacks

luisramos0

What is the test pan? Do you think these changes are validated by automatic tests?

sauloperez · 2018-09-01T14:32:00Z

Thanks for the heads-up. I think this now deserves a smoke test.

Matt-Yorkley · 2018-09-01T14:39:16Z

I think this now deserves a smoke test.

Looks like the tests are smoking already...

sauloperez · 2018-09-03T09:20:22Z

lol @Matt-Yorkley

luisramos0

That's a lot of changes...
Great stuff.

@sauloperez What parts of the application should be tested more carefully in staging?

mkllnk

I spotted one change that doesn't look right to me. But I might be wrong?

mkllnk · 2018-09-05T09:39:09Z

app/controllers/spree/admin/payment_methods_controller_decorator.rb

@@ -10,7 +10,7 @@ module Admin
      # Only show payment methods that user has access to and sort by distributor name
      # ! Redundant code copied from Spree::Admin::ResourceController with modifications marked
      def collection
-        return parent.send(controller_name) if parent_data.present?
+        return parent.controller_name if parent_data.present?


The original did not call send(:controller_name). Doesn't it call a method with the same name as the controller?

sauloperez · 2018-09-05T10:34:02Z

I have extended the list of features to test. Thanks @luisramos0 !

sauloperez · 2018-09-12T12:24:19Z

Staged on https://staging.katuma.org/

RachL · 2018-09-20T15:20:28Z

FINALLY!
Sorry it took me so long ...
Everything looks good:
https://docs.google.com/document/d/1FLZVVyGaH6ubjeM-bpXHzeN4GxUTsTVC6fADeCO1xGk/edit

sauloperez mentioned this pull request Aug 30, 2018

Support removal of enterprise logo and promo image #2512

Merged

luisramos0 approved these changes Aug 30, 2018

View reviewed changes

luisramos0 assigned sauloperez Aug 30, 2018

sauloperez force-pushed the enable-style-send-cop branch 4 times, most recently from c8b0d0f to e99fee1 Compare August 31, 2018 10:22

sauloperez commented Aug 31, 2018

View reviewed changes

sauloperez force-pushed the enable-style-send-cop branch 2 times, most recently from c376f70 to 7346ed4 Compare August 31, 2018 13:26

luisramos0 reviewed Aug 31, 2018

View reviewed changes

HugsDaniel approved these changes Sep 2, 2018

View reviewed changes

sauloperez force-pushed the enable-style-send-cop branch from 7346ed4 to 74bf97a Compare September 3, 2018 09:48

luisramos0 approved these changes Sep 3, 2018

View reviewed changes

mkllnk requested changes Sep 5, 2018

View reviewed changes

sauloperez force-pushed the enable-style-send-cop branch from 74bf97a to 5a06e82 Compare September 5, 2018 10:02

Fix current violations of Style/Send cop

b23cb55

sauloperez force-pushed the enable-style-send-cop branch from 5a06e82 to b23cb55 Compare September 5, 2018 10:05

mkllnk approved these changes Sep 5, 2018

View reviewed changes

daniellemoorhead added the tech debt label Sep 7, 2018

sauloperez added the pr-staged-es label Sep 12, 2018

RachL self-assigned this Sep 18, 2018

RachL removed the pr-staged-es label Sep 20, 2018

sauloperez merged commit 20077c9 into openfoodfoundation:master Sep 20, 2018

sauloperez deleted the enable-style-send-cop branch September 20, 2018 17:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Favor #public_send over #send using Rubocop's cop #2618

Favor #public_send over #send using Rubocop's cop #2618

sauloperez commented Aug 30, 2018 •

edited

Loading

luisramos0 left a comment

luisramos0 commented Aug 30, 2018

sauloperez commented Aug 31, 2018

sauloperez Aug 31, 2018

sauloperez Aug 31, 2018

mkllnk Sep 5, 2018

sauloperez Sep 5, 2018

sauloperez Aug 31, 2018

sauloperez Aug 31, 2018

sauloperez Aug 31, 2018

sauloperez Aug 31, 2018

sauloperez Aug 31, 2018

sauloperez Aug 31, 2018

sauloperez Aug 31, 2018

luisramos0 left a comment

sauloperez commented Sep 1, 2018

Matt-Yorkley commented Sep 1, 2018 •

edited

Loading

sauloperez commented Sep 3, 2018

luisramos0 left a comment

mkllnk left a comment

mkllnk Sep 5, 2018

sauloperez commented Sep 5, 2018 •

edited

Loading

sauloperez commented Sep 12, 2018

RachL commented Sep 20, 2018

Favor #public_send over #send using Rubocop's cop #2618

Favor #public_send over #send using Rubocop's cop #2618

Conversation

sauloperez commented Aug 30, 2018 • edited Loading

What? Why?

What should we test?

Release notes

luisramos0 left a comment

Choose a reason for hiding this comment

luisramos0 commented Aug 30, 2018

sauloperez commented Aug 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luisramos0 left a comment

Choose a reason for hiding this comment

sauloperez commented Sep 1, 2018

Matt-Yorkley commented Sep 1, 2018 • edited Loading

sauloperez commented Sep 3, 2018

luisramos0 left a comment

Choose a reason for hiding this comment

mkllnk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sauloperez commented Sep 5, 2018 • edited Loading

sauloperez commented Sep 12, 2018

RachL commented Sep 20, 2018

sauloperez commented Aug 30, 2018 •

edited

Loading

Matt-Yorkley commented Sep 1, 2018 •

edited

Loading

sauloperez commented Sep 5, 2018 •

edited

Loading