Fix has one class name #1028

dmitry-solomadin · 2017-11-20T02:00:01Z

Related issue: #951

Currently, when passing class_name option to has_one field it does not infer correct class name for the dashboard which results in exception.
This PR modifies has_one class to use associative to get the correct dashboard name.
Additionally, it fixes permitted_attributes method, which required me to modify the signature and pass options from Deferred attribute.

In scope of this I've also removed old locale keys for has_one.not_supported string.

/r @nickcharlton @tysongach

houndci-bot · 2017-11-20T02:00:11Z

lib/administrate/field/polymorphic.rb

@@ -11,7 +11,7 @@ def associated_resource_grouped_options
        end
      end

-      def self.permitted_attribute(attr)
+      def self.permitted_attribute(attr, options=nil)


Unused method argument - options. If it's necessary, use _ or _options as an argument name to indicate that it won't be used.
Surrounding space missing in default value assignment.

houndci-bot · 2017-11-20T02:00:11Z

lib/administrate/field/has_one.rb

+      private
+
+      def resolver
+        @resolver ||= Administrate::ResourceResolver.new("admin/#{associated_class_name}")


Line is too long. [90/80]

houndci-bot · 2017-11-20T02:00:11Z

lib/administrate/field/has_one.rb

+            options.fetch(:class_name, attr.to_s.singularize.camelcase)
+          else
+            attr
+          end  


Trailing whitespace detected.

houndci-bot · 2017-11-20T02:00:11Z

lib/administrate/field/has_one.rb

      end

-      def self.permitted_attribute(attr)
+      def self.permitted_attribute(attr, options=nil)


Surrounding space missing in default value assignment.

houndci-bot · 2017-11-20T02:00:11Z

lib/administrate/field/has_many.rb

@@ -7,8 +7,8 @@ module Field
    class HasMany < Associative
      DEFAULT_LIMIT = 5

-      def self.permitted_attribute(attribute)
-        { "#{attribute.to_s.singularize}_ids".to_sym => [] }
+      def self.permitted_attribute(attr, options=nil)


Unused method argument - options. If it's necessary, use _ or _options as an argument name to indicate that it won't be used.
Surrounding space missing in default value assignment.

houndci-bot · 2017-11-20T02:00:12Z

lib/administrate/field/deferred.rb

-      def permitted_attribute(attr)
-        options.fetch(:foreign_key, deferred_class.permitted_attribute(attr))
+      def permitted_attribute(attr, options=nil)
+        self.options.fetch(:foreign_key, deferred_class.permitted_attribute(attr, self.options))


Line is too long. [96/80]

houndci-bot · 2017-11-20T02:00:12Z

lib/administrate/field/deferred.rb

@@ -25,8 +25,8 @@ def searchable?
        options.fetch(:searchable, deferred_class.searchable?)
      end

-      def permitted_attribute(attr)
-        options.fetch(:foreign_key, deferred_class.permitted_attribute(attr))
+      def permitted_attribute(attr, options=nil)


Unused method argument - options. If it's necessary, use _ or _options as an argument name to indicate that it won't be used.
Surrounding space missing in default value assignment.

houndci-bot · 2017-11-20T02:00:12Z

lib/administrate/field/belongs_to.rb

@@ -3,7 +3,7 @@
 module Administrate
  module Field
    class BelongsTo < Associative
-      def self.permitted_attribute(attr)
+      def self.permitted_attribute(attr, options=nil)


Unused method argument - options. If it's necessary, use _ or _options as an argument name to indicate that it won't be used.
Surrounding space missing in default value assignment.

houndci-bot · 2017-11-20T02:00:12Z

lib/administrate/field/base.rb

@@ -24,7 +24,7 @@ def initialize(attribute, data, page, options = {})
        @options = options
      end

-      def self.permitted_attribute(attr)
+      def self.permitted_attribute(attr, options=nil)


Unused method argument - options. If it's necessary, use _ or _options as an argument name to indicate that it won't be used.
Surrounding space missing in default value assignment.

houndci-bot · 2017-11-20T02:00:12Z

lib/administrate/field/polymorphic.rb

@@ -11,7 +11,7 @@ def associated_resource_grouped_options
        end
      end

-      def self.permitted_attribute(attr)
+      def self.permitted_attribute(attr, options=nil)


Unused method argument - options. If it's necessary, use _ or _options as an argument name to indicate that it won't be used.
Surrounding space missing in default value assignment.

houndci-bot · 2017-11-20T02:00:12Z

lib/administrate/field/has_one.rb

+      private
+
+      def resolver
+        @resolver ||= Administrate::ResourceResolver.new("admin/#{associated_class_name}")


Line is too long. [90/80]

houndci-bot · 2017-11-20T02:00:12Z

lib/administrate/field/has_one.rb

+            options.fetch(:class_name, attr.to_s.singularize.camelcase)
+          else
+            attr
+          end  


Trailing whitespace detected.

houndci-bot · 2017-11-20T02:00:13Z

lib/administrate/field/has_one.rb

      end

-      def self.permitted_attribute(attr)
+      def self.permitted_attribute(attr, options=nil)


Surrounding space missing in default value assignment.

houndci-bot · 2017-11-20T02:00:13Z

lib/administrate/field/has_many.rb

@@ -7,8 +7,8 @@ module Field
    class HasMany < Associative
      DEFAULT_LIMIT = 5

-      def self.permitted_attribute(attribute)
-        { "#{attribute.to_s.singularize}_ids".to_sym => [] }
+      def self.permitted_attribute(attr, options=nil)


Unused method argument - options. If it's necessary, use _ or _options as an argument name to indicate that it won't be used.
Surrounding space missing in default value assignment.

houndci-bot · 2017-11-20T02:00:13Z

lib/administrate/field/deferred.rb

-      def permitted_attribute(attr)
-        options.fetch(:foreign_key, deferred_class.permitted_attribute(attr))
+      def permitted_attribute(attr, options=nil)
+        self.options.fetch(:foreign_key, deferred_class.permitted_attribute(attr, self.options))


Line is too long. [96/80]

houndci-bot · 2017-11-20T02:00:13Z

lib/administrate/field/deferred.rb

@@ -25,8 +25,8 @@ def searchable?
        options.fetch(:searchable, deferred_class.searchable?)
      end

-      def permitted_attribute(attr)
-        options.fetch(:foreign_key, deferred_class.permitted_attribute(attr))
+      def permitted_attribute(attr, options=nil)


Unused method argument - options. If it's necessary, use _ or _options as an argument name to indicate that it won't be used.
Surrounding space missing in default value assignment.

houndci-bot · 2017-11-20T02:00:13Z

lib/administrate/field/belongs_to.rb

@@ -3,7 +3,7 @@
 module Administrate
  module Field
    class BelongsTo < Associative
-      def self.permitted_attribute(attr)
+      def self.permitted_attribute(attr, options=nil)


Unused method argument - options. If it's necessary, use _ or _options as an argument name to indicate that it won't be used.
Surrounding space missing in default value assignment.

houndci-bot · 2017-11-20T02:00:13Z

lib/administrate/field/base.rb

@@ -24,7 +24,7 @@ def initialize(attribute, data, page, options = {})
        @options = options
      end

-      def self.permitted_attribute(attr)
+      def self.permitted_attribute(attr, options=nil)


Unused method argument - options. If it's necessary, use _ or _options as an argument name to indicate that it won't be used.
Surrounding space missing in default value assignment.

houndci-bot · 2017-11-25T20:49:55Z

lib/administrate/field/deferred.rb

-        options.fetch(:foreign_key, deferred_class.permitted_attribute(attr))
+      def permitted_attribute(attr, _options = nil)
+        self.options.fetch(:foreign_key,
+          deferred_class.permitted_attribute(attr, self.options))


Redundant self detected.

houndci-bot · 2017-11-25T20:49:55Z

lib/administrate/field/deferred.rb

-      def permitted_attribute(attr)
-        options.fetch(:foreign_key, deferred_class.permitted_attribute(attr))
+      def permitted_attribute(attr, _options = nil)
+        self.options.fetch(:foreign_key,


Redundant self detected.

houndci-bot · 2017-11-25T20:49:55Z

lib/administrate/field/deferred.rb

-        options.fetch(:foreign_key, deferred_class.permitted_attribute(attr))
+      def permitted_attribute(attr, _options = nil)
+        self.options.fetch(:foreign_key,
+          deferred_class.permitted_attribute(attr, self.options))


Redundant self detected.

houndci-bot · 2017-11-25T20:49:55Z

lib/administrate/field/deferred.rb

-      def permitted_attribute(attr)
-        options.fetch(:foreign_key, deferred_class.permitted_attribute(attr))
+      def permitted_attribute(attr, _options = nil)
+        self.options.fetch(:foreign_key,


Redundant self detected.

houndci-bot · 2017-11-25T22:30:25Z

spec/lib/fields/has_one_spec.rb

+        field_name = 'seo_meta_tag'
+        attributes = field.permitted_attribute(field_name)
+        expect(attributes[:"#{field_name}_attributes"]).
+          to eq([:meta_title, :meta_description, :id])


Use %i or %I for an array of symbols.

houndci-bot · 2017-11-25T22:30:25Z

spec/lib/fields/has_one_spec.rb

+        field = Administrate::Field::Deferred.
+          new(Administrate::Field::HasOne.with_options(class_name: :product_meta_tag))
+
+        field_name = 'seo_meta_tag'


Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2017-11-25T22:30:25Z

spec/lib/fields/has_one_spec.rb

+    context 'with custom class_name' do
+      it "returns attributes from correct dashboard" do
+        field = Administrate::Field::Deferred.
+          new(Administrate::Field::HasOne.with_options(class_name: :product_meta_tag))


Line is too long. [86/80]

houndci-bot · 2017-11-25T22:30:25Z

spec/lib/fields/has_one_spec.rb

+  end
+
+  describe ".permitted_attribute" do
+    context 'with custom class_name' do


Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

dmitry-solomadin · 2017-11-25T22:39:27Z

Added some tests. PR is ready for review
/cc @nickcharlton @tysongach

etagwerker

@dmitry-solomadin This is a great addition to the project. We are using it in one of our projects and it works. Thank you! 👍

nickcharlton · 2017-12-19T22:01:10Z

Hi @dmitry-solomadin! Thank you for opening this!

As I've taken a while to look at it, it's fallen behind a bit. Would you mind rebasing it (it's mostly translations, it seems…)?

nickcharlton · 2017-12-19T22:02:23Z

spec/lib/fields/deferred_spec.rb

@@ -20,7 +20,7 @@
        deferred.permitted_attribute(:foo)

        expect(Administrate::Field::String).
-          to have_received(:permitted_attribute).with(:foo)
+          to have_received(:permitted_attribute).with(:foo, {})


If the method is providing a default (nil), is it valuable for us to be providing an empty hash here?

Yes, however in that particular case we are testing this method on String field in scope of Deferred attribute, in deferred.rb the options that we pass are actually initialized with {} by default

dmitry-solomadin · 2017-12-20T02:32:21Z

@nickcharlton Hey Nick, rebased the branch off latest master, should be good now.
Let me know if you have any other concerns.

dmitry-solomadin · 2017-12-20T03:06:46Z

Looks like some tests on master are failing the same ones are failing on my branch

nickcharlton · 2017-12-20T14:11:07Z

Great, thanks! The failures are unrelated to your work, so I'm ignoring them until I get some time on Friday to look at them. I'll merge this now!

houndci-bot reviewed Nov 20, 2017

View reviewed changes

houndci-bot reviewed Nov 25, 2017

View reviewed changes

dmitry-solomadin force-pushed the ds-fix-has-one-class-name branch from cbb89ba to 8df9a57 Compare November 25, 2017 20:50

houndci-bot reviewed Nov 25, 2017

View reviewed changes

etagwerker approved these changes Nov 29, 2017

View reviewed changes

nickcharlton reviewed Dec 19, 2017

View reviewed changes

dmitry-solomadin added 6 commits December 19, 2017 21:26

Remove old has_one unsupported message from locales

3bfafd6

Fix has_one attribute when has class_name option

41d8242

Fix hound

c5bb544

Fix hound#2

5b7a7e2

Add tests

e2cfdea

Fix hound

4b83819

dmitry-solomadin force-pushed the ds-fix-has-one-class-name branch from 8c61b73 to 4b83819 Compare December 20, 2017 02:31

fix locales

6768d55

nickcharlton merged commit 9f693cc into thoughtbot:master Dec 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix has one class name #1028

Fix has one class name #1028

dmitry-solomadin commented Nov 20, 2017 •

edited

Loading

houndci-bot Nov 20, 2017

houndci-bot Nov 20, 2017

houndci-bot Nov 20, 2017

houndci-bot Nov 20, 2017

houndci-bot Nov 20, 2017

houndci-bot Nov 20, 2017

houndci-bot Nov 20, 2017

houndci-bot Nov 20, 2017

houndci-bot Nov 20, 2017

houndci-bot Nov 20, 2017

houndci-bot Nov 20, 2017

houndci-bot Nov 20, 2017

houndci-bot Nov 20, 2017

houndci-bot Nov 20, 2017

houndci-bot Nov 20, 2017

houndci-bot Nov 20, 2017

houndci-bot Nov 20, 2017

houndci-bot Nov 20, 2017

houndci-bot Nov 25, 2017

houndci-bot Nov 25, 2017

houndci-bot Nov 25, 2017

houndci-bot Nov 25, 2017

houndci-bot Nov 25, 2017

houndci-bot Nov 25, 2017

houndci-bot Nov 25, 2017

houndci-bot Nov 25, 2017

dmitry-solomadin commented Nov 25, 2017

etagwerker left a comment

nickcharlton commented Dec 19, 2017

nickcharlton Dec 19, 2017

dmitry-solomadin Dec 20, 2017

dmitry-solomadin commented Dec 20, 2017

dmitry-solomadin commented Dec 20, 2017

nickcharlton commented Dec 20, 2017

Fix has one class name #1028

Fix has one class name #1028

Conversation

dmitry-solomadin commented Nov 20, 2017 • edited Loading

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

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

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

dmitry-solomadin commented Nov 25, 2017

etagwerker left a comment

Choose a reason for hiding this comment

nickcharlton commented Dec 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitry-solomadin commented Dec 20, 2017

dmitry-solomadin commented Dec 20, 2017

nickcharlton commented Dec 20, 2017

dmitry-solomadin commented Nov 20, 2017 •

edited

Loading