Add foreign key option to Fields #807

iarie · 2017-03-31T17:19:40Z

Allows to specify foreign_key option in associative fields
Updates example_app to demonstrate this feature

houndci-bot · 2017-03-31T17:20:26Z

spec/lib/fields/deferred_spec.rb

-      deferred.permitted_attribute(:foo)
+    context "when not given a `foreign_key` option" do
+      it "delegates to the backing class" do
+        deferred = Administrate::Field::Deferred.new(Administrate::Field::String)


Line is too long. [81/80]

houndci-bot · 2017-03-31T17:20:26Z

spec/lib/fields/belongs_to_spec.rb

+      expect(field.permitted_attribute).to eq("customers_uuid")
+    end
+  end
+


Extra empty line detected at block body end.

houndci-bot · 2017-03-31T17:20:26Z

spec/example_app/db/seeds.rb

+  {code: 'DE', name: 'Germany'},
+  {code: 'CA', name: 'Canada'},
+  {code: 'BR', name: 'Brazil'},
+])


Indent the right bracket the same as the first position after the preceding left parenthesis.

houndci-bot · 2017-03-31T17:20:27Z

spec/example_app/db/seeds.rb

+  {code: 'RU', name: 'Russia'},
+  {code: 'DE', name: 'Germany'},
+  {code: 'CA', name: 'Canada'},
+  {code: 'BR', name: 'Brazil'},


Space inside { missing.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
Space inside } missing.

houndci-bot · 2017-03-31T17:20:27Z

spec/example_app/db/seeds.rb

+  {code: 'CN', name: 'China'},
+  {code: 'RU', name: 'Russia'},
+  {code: 'DE', name: 'Germany'},
+  {code: 'CA', name: 'Canada'},


Space inside { missing.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
Space inside } missing.

houndci-bot · 2017-03-31T17:20:27Z

spec/example_app/db/seeds.rb

+Country.create([
+  {code: 'US', name: 'USA'},
+  {code: 'CN', name: 'China'},
+  {code: 'RU', name: 'Russia'},


Space inside { missing.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
Space inside } missing.

houndci-bot · 2017-03-31T17:20:27Z

spec/example_app/db/seeds.rb

+
+Country.create([
+  {code: 'US', name: 'USA'},
+  {code: 'CN', name: 'China'},


Space inside { missing.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
Space inside } missing.

houndci-bot · 2017-03-31T17:20:27Z

spec/example_app/db/seeds.rb

+Country.destroy_all
+
+Country.create([
+  {code: 'US', name: 'USA'},


Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.
Space inside { missing.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
Space inside } missing.

houndci-bot · 2017-03-31T17:20:27Z

spec/example_app/app/dashboards/customer_dashboard.rb

@@ -10,6 +10,7 @@ class CustomerDashboard < Administrate::BaseDashboard
    orders: Field::HasMany.with_options(limit: 2),
    updated_at: Field::DateTime,
    kind: Field::Select.with_options(collection: Customer::KINDS),
+    country: Field::BelongsTo.with_options(primary_key: :code, foreign_key: :country_code),


Line is too long. [91/80]

houndci-bot · 2017-03-31T17:20:27Z

spec/example_app/app/dashboards/country_dashboard.rb

+    :name,
+  ]
+
+  ATTRIBUTE_TYPES = {


Freeze mutable objects assigned to constants.

BenMorganIO · 2017-04-04T18:17:02Z

spec/example_app/app/models/country.rb

@@ -0,0 +1,4 @@
+class Country < ActiveRecord::Base
+  validates :code, presence: true
+  validates :name, presence: true


Maybe do:

validates :code, :name, presence: true

BenMorganIO · 2017-04-04T18:18:26Z

spec/example_app/db/migrate/20170331144723_create_countries.rb

+class CreateCountries < ActiveRecord::Migration
+  def change
+    create_table :countries do |t|
+      t.string :code


t.string :code, null: false, index: { unique: true }

That way you don't need the add_index at the bottom. Also, since you're validating at the application level, these should be null: false as well.

BenMorganIO · 2017-04-04T18:21:49Z

spec/example_app/db/seeds.rb

+Country.create(code: "CA", name: "Canada")
+Country.create(code: "CN", name: "China")
+Country.create(code: "RU", name: "Russia")
+Country.create(code: "AU", name: "Australia")


These could fail silently for a developer. Why not:

Country.create! [ { code: "US", name: "Trump Temptations: The Billionaire & The Bellboy" }, { code: "CA", name: "Canada" }, { code: "CN", name: "China" }, { code: "RU", name: "Russia" }, { code: "AU", name: "Australia" } ]

Ok, good point, but should we also add create-bang to other models in seeds.rb, i did create to preserve uniformity. Also i'll better stick with old definition of US 🤣

BenMorganIO · 2017-04-04T18:24:16Z

spec/example_app/db/seeds.rb


 100.times do
  name = "#{Faker::Name.first_name} #{Faker::Name.last_name}"
  Customer.create(
    name: name,
    email: Faker::Internet.safe_email(name),
+    country: Country.first,


This iterates 100 times. Maybe do:

country = Country.first customers = Array.new(100) do { name: Faker::Name.name, email: Faker::Internet.safe_email(name), country: country } end Customer.create!(customers)

Array.new is faster than Integer#times; best practice to always use Array.new. Also, you don't have to call Customer.create 100 times, just once with an array for it to iterate over. We don't need this blazing fast. It's not inserting 2,000 records. However, we nearly made it do an extra 100 queries with Customer.first instead of just one. We could actually make it even simpler with taking the data returned from our Country.create! and sampling from there.

houndci-bot · 2017-04-05T08:31:27Z

spec/example_app/db/seeds.rb

+  { code: "CA", name: "Canada" },
+  { code: "CN", name: "China" },
+  { code: "RU", name: "Russia" },
+  { code: "AU", name: "Australia" }


Put a comma after the last item of a multiline array.

iarie · 2017-04-05T13:34:56Z

@BenMorganIO let me know if there is something we need to make/improve

jcmorrow

I'm not sure of the best way to solve this, but I think this is in the right direction.

jcmorrow · 2017-04-14T17:12:40Z

spec/lib/fields/belongs_to_spec.rb

+          foreign_key: "customers_uuid",
+        )
+      field = association.new(:customers, [], :show)
+      expect(field.permitted_attribute).to eq("customers_uuid")


Could we break up the phases of this test with newlines? (https://robots.thoughtbot.com/four-phase-test)

jcmorrow · 2017-04-14T17:17:12Z

spec/lib/fields/belongs_to_spec.rb

+
+  describe "foreign_key option" do
+    it "determines what foreign key is used on the relationship for the form" do
+      association =


I think this line spacing is a bit awkward, what about

association = Administrate::Field::BelongsTo.with_options( foreign_key: "customers_uuid", )

jcmorrow · 2017-04-14T17:33:10Z

lib/administrate/field/belongs_to.rb

@@ -8,7 +8,7 @@ def self.permitted_attribute(attr)
      end

      def permitted_attribute
-        self.class.permitted_attribute(attribute)
+        foreign_key || self.class.permitted_attribute(attribute)


So, I found this whole permitted_attribute instance method calling a class method thing to be hard to follow and reason about. How would you feel about renaming class level implementations of permitted_attribute to like default_attribute or something like that? Or we could add an instance level default_attribute method and put that here instead of calling a class method as a default?

@jcmorrow i ended up with

def foreign_key options.fetch(:foreign_key, :"#{attribute}_id") end

on the instance level of BelongsTo, i found dangerous to rewrite permitted_attributes because it's used by BaseDashobard and Deffered classes.

jcmorrow · 2017-04-14T17:34:48Z

lib/administrate/field/associative.rb

@@ -24,6 +24,10 @@ def associated_class_name
      def primary_key
        options.fetch(:primary_key, :id)
      end
+
+      def foreign_key
+        options.fetch(:foreign_key, nil)


I'm not a huge fan of the way this reads in combination with line 25 above. Line 25 is handing you a default if not specified (which is what I would expect), but foreign_key is instead returning nil, and then allowing permitted_attribute to implement the default behavior for a foreign_key, but it's not clear from this line that that is what is going on. Does that make sense?

iarie · 2017-05-02T14:05:24Z

CircleCI fails with:

bundler: failed to load command: rake (/home/ubuntu/administrate/vendor/bundle/ruby/2.4.0/bin/rake)
Gem::LoadError: You have already activated bundler 1.13.7, but your Gemfile requires bundler 1.14.6. Prepending `bundle exec` to your command may solve this.

i don't understand why this happening...

BenMorganIO · 2017-04-14T17:30:20Z

spec/example_app/db/schema.rb


  # These are extensions that must be enabled in order to support this database
  enable_extension "plpgsql"

+  create_table "countries", force: :cascade do |t|
+    t.string   "code"


Didn't you make this null: false in your migration?

nickcharlton · 2017-06-13T15:51:06Z

Hi @iarie!

I think this is almost good to go. Do you think you'd be able to document this in the docs directory?

houndci-bot · 2017-06-17T13:50:50Z

lib/administrate/base_dashboard.rb

+        field = self.class::ATTRIBUTE_TYPES[key]
+
+        next key if association_classes.include?(field)
+        key if association_classes.include?(field.try :deferred_class)


Add parentheses to nested method call field.try :deferred_class.

houndci-bot · 2017-06-17T13:50:51Z

app/controllers/administrate/application_controller.rb

@@ -5,6 +5,7 @@ class ApplicationController < ActionController::Base
    def index
      search_term = params[:search].to_s.strip
      resources = Administrate::Search.new(resource_resolver, search_term).run
+      resources = resources.includes(*resource_includes) if resource_includes.any?


Line is too long. [82/80]

houndci-bot · 2017-06-17T13:50:51Z

app/assets/stylesheets/administrate/components/_buttons.scss

@@ -1,4 +1,7 @@
-#{$all-buttons},
+button,


Avoid qualifying attribute selectors with an element.

houndci-bot · 2017-06-17T13:50:51Z

lib/administrate/base_dashboard.rb

+        field = self.class::ATTRIBUTE_TYPES[key]
+
+        next key if association_classes.include?(field)
+        key if association_classes.include?(field.try :deferred_class)


Add parentheses to nested method call field.try :deferred_class.

houndci-bot · 2017-06-17T13:50:51Z

app/controllers/administrate/application_controller.rb

@@ -5,6 +5,7 @@ class ApplicationController < ActionController::Base
    def index
      search_term = params[:search].to_s.strip
      resources = Administrate::Search.new(resource_resolver, search_term).run
+      resources = resources.includes(*resource_includes) if resource_includes.any?


Line is too long. [82/80]

houndci-bot · 2017-06-17T13:50:51Z

app/assets/stylesheets/administrate/components/_buttons.scss

@@ -1,4 +1,7 @@
-#{$all-buttons},
+button,


Avoid qualifying attribute selectors with an element.

houndci-bot · 2017-06-17T13:59:44Z

spec/example_app/app/dashboards/country_dashboard.rb

+require "administrate/base_dashboard"
+
+class CountryDashboard < Administrate::BaseDashboard
+  ATTRIBUTES = [


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

houndci-bot · 2017-06-17T13:59:45Z

spec/example_app/app/dashboards/country_dashboard.rb

+require "administrate/base_dashboard"
+
+class CountryDashboard < Administrate::BaseDashboard
+  ATTRIBUTES = [


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

@nickcharlton done! And should i fix that dashboard attributes definition according to Hound?

iarie · 2017-07-10T08:39:42Z

rebased against master

nickcharlton · 2017-07-18T15:42:32Z

This looks great! Could I get you to fix up those Hound warnings? I'd done that on a recent one I'd merged in, so it'd be good to start doing that.

iarie · 2017-07-19T09:09:08Z

@nickcharlton fixing that syntax in example_app i thought should we also remake dashboard generators to rely on hound's syntax( add symbols %i array and freeze stuff)? Not here, in another PR? Not so important ofc :)

nickcharlton · 2017-07-20T11:27:22Z

Mmm. As they're not related to this PR, we should fix them up in another.

nickcharlton · 2017-07-20T11:28:40Z

gemfiles/.bundle/config

@@ -0,0 +1,2 @@
+---
+BUNDLE_RETRY: "1"


I didn't notice this before; is this important for this PR or do you think it's better excluded?

nickcharlton · 2017-07-20T12:29:41Z

spec/example_app/app/dashboards/country_dashboard.rb

+  ATTRIBUTES = %i(
+    code
+    name
+  ).freeze


Could we put this all on one line?

nickcharlton · 2017-07-20T12:30:17Z

spec/example_app/db/seeds.rb


-100.times do
+countries = Country.create! [


Should this be indented inside this block?

@nickcharlton Sorry, i don't understand what do you mean.
Something wrong with brackets?
I can separate attributes declaration from .create! like this:

country_attributes = [ { code: "US", name: "USA" }, { code: "CA", name: "Canada" }, { code: "CN", name: "China" }, { code: "RU", name: "Russia" }, { code: "AU", name: "Australia" }, ] countries = Country.create!(country_attributes)

Oh, oops! Apparently I didn't read the diff correctly.

nickcharlton · 2017-07-21T13:39:53Z

gemfiles/.bundle/config

@@ -0,0 +1 @@
+


Hmm, git still seems to think this file exists (but with just a blank line). Could you remove it completely?

Add country code to customers in example_app to show how it works

- add fetch default foreign_key to Field::BelongsTo - style fixes and specs

nickcharlton · 2017-07-31T13:26:22Z

Excellent, thank you! Merging now.

dan-ding · 2017-08-02T19:19:17Z

curious - why not use reflections to get the foreign key of a relation?

pablobm · 2020-02-07T12:48:46Z

@dan-ding - I think you are right, and we just didn't think of it at the time.

houndci-bot reviewed Mar 31, 2017

View reviewed changes

iarie force-pushed the add-foreign-key-option branch 3 times, most recently from 030f1bf to d0a1010 Compare April 1, 2017 09:06

BenMorganIO reviewed Apr 4, 2017

View reviewed changes

houndci-bot reviewed Apr 5, 2017

View reviewed changes

iarie force-pushed the add-foreign-key-option branch 2 times, most recently from 1445eb6 to 7e2d03f Compare April 5, 2017 08:38

iarie mentioned this pull request Apr 14, 2017

Allow belongs_to with primary_key option #805

Closed

jcmorrow reviewed Apr 14, 2017

View reviewed changes

iarie force-pushed the add-foreign-key-option branch 4 times, most recently from fc45f8f to cd0f445 Compare April 21, 2017 10:55

iarie force-pushed the add-foreign-key-option branch 2 times, most recently from 469a6dd to e866000 Compare May 2, 2017 13:00

BenMorganIO reviewed May 2, 2017

View reviewed changes

iarie force-pushed the add-foreign-key-option branch from 25f3e3c to 8a3d8a8 Compare May 3, 2017 07:12

iarie force-pushed the add-foreign-key-option branch from 8a3d8a8 to 14f93e0 Compare May 10, 2017 09:27

iarie force-pushed the add-foreign-key-option branch from 14f93e0 to 769aa38 Compare May 30, 2017 15:43

nickcharlton added the waiting-on-response label Jun 13, 2017

iarie force-pushed the add-foreign-key-option branch from f684c8f to 6f6c12c Compare June 17, 2017 13:50

houndci-bot reviewed Jun 17, 2017

View reviewed changes

iarie force-pushed the add-foreign-key-option branch from 6f6c12c to e640768 Compare June 17, 2017 13:59

houndci-bot reviewed Jun 17, 2017

View reviewed changes

iarie force-pushed the add-foreign-key-option branch 2 times, most recently from 3a570eb to 9c8580b Compare June 20, 2017 10:02

iarie force-pushed the add-foreign-key-option branch from 9c8580b to d715683 Compare July 10, 2017 08:26

iarie force-pushed the add-foreign-key-option branch from d715683 to 432a59e Compare July 17, 2017 09:23

nickcharlton reviewed Jul 20, 2017

View reviewed changes

nickcharlton reviewed Jul 21, 2017

View reviewed changes

iarie added 9 commits July 31, 2017 14:03

Add foreign key option to assosiations Fields

a97db29

Add country code to customers in example_app to show how it works

code refactor

052d6a2

Requested changes:

274be1d

- add fetch default foreign_key to Field::BelongsTo - style fixes and specs

update example_app's schema

42739d6

Rails release for migration

29121fe

add options description to docs

4f1d8b9

Attributes array via %i notation

56cd619

requested changes

e23613d

removed empty bundle/config file

7c5964a

iarie force-pushed the add-foreign-key-option branch from ada91e9 to 7c5964a Compare July 31, 2017 11:03

nickcharlton merged commit 736266d into thoughtbot:master Jul 31, 2017

pablobm mentioned this pull request Feb 7, 2020

Belongs_to form NoMethodError when association uses different foreign_key #354

Closed

		@@ -1,4 +1,7 @@
		#{$all-buttons},
		button,

		@@ -1,4 +1,7 @@
		#{$all-buttons},
		button,

Add foreign key option to Fields #807

Add foreign key option to Fields #807

Conversation

iarie commented Mar 31, 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

iarie commented Apr 5, 2017

jcmorrow left a 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

iarie commented May 2, 2017 • edited Loading

Choose a reason for hiding this comment

nickcharlton commented Jun 13, 2017

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

iarie commented Jul 10, 2017

nickcharlton commented Jul 18, 2017

iarie commented Jul 19, 2017 • edited Loading

nickcharlton commented Jul 20, 2017

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

nickcharlton commented Jul 31, 2017

dan-ding commented Aug 2, 2017

pablobm commented Feb 7, 2020

iarie commented Mar 31, 2017 •

edited

Loading

iarie commented May 2, 2017 •

edited

Loading

iarie commented Jul 19, 2017 •

edited

Loading