Fix creating polymorphic associations by Drew-Goddyn · Pull Request #35 · clio/polymorphic_integer_type

Drew-Goddyn · 2020-11-06T20:37:28Z

We previously were only including the BelongsToPolymorphicAssociation module in specific activerecord versions. This module overides the replace_keys method to ensure we aren't sending the class's #polymorphic_name when creating associations. When this isn't included we send the polymorphic name which includes the class's namespace and can cause us to not find it in the mapping (assuming the mappings don't include namespaces).

By including it, we send the class object itself and let our fallback logic kick in to try several methods until we get a hit from the mapping.

It also cleans up a few other gotchas like creating a new ModuleGenerator. The generator defines an anonmyous module we can use for calling super since the previous implementation was all inline causing us to capture local variables in the blocks and behave oddly.

Drew-Goddyn · 2020-11-06T20:38:55Z

 require "polymorphic_integer_type/extensions"
 require "polymorphic_integer_type/mapping"
+require "polymorphic_integer_type/module_generator"
+require "polymorphic_integer_type/belongs_to_polymorphic_association_extension"


this require was the heart of the issue, as it was previously tied to an active record version < 5.2.0, but in reality we want it all the time

Drew-Goddyn · 2020-11-06T20:39:27Z

      private def replace_keys(record)
        super
-        owner[reflection.foreign_type] = record.class.base_class
+        owner[reflection.foreign_type] = record.class


send the class object instead of the base_class so we can try several methods like #polymorphic_name, #sti_name, #to_s etc

Drew-Goddyn · 2020-11-06T20:40:37Z

-          end
-
-          include(foreign_type_extension)
+          ModuleGenerator.generate_and_include(self, foreign_type, name)


previously, the define_method blocks that were abstracted would capture the mapping variable and could cause really odd behaviour when stubbing it in specs, this ensures that all the stuff it needs is encapsulated in a class and isn't affected by the outside world.

Drew-Goddyn · 2020-11-06T20:41:05Z

      @@mapping[as] || {}
    end

+    singleton_class.send(:alias_method, :[]=, :add)


thought it was odd we have an #add method that behaves canonically like []= so made an alias 🤷‍♂️

Drew-Goddyn · 2020-11-06T20:41:31Z

  spec.add_development_dependency "rspec"
  spec.add_development_dependency "sqlite3"
-  spec.add_development_dependency "byebug"
+  spec.add_development_dependency "pry-byebug"


cause we want choice! (I hate debugger since it has no colors)

Drew-Goddyn · 2020-11-06T20:41:56Z

  belongs_to :source, polymorphic: true, integer_type: true
  belongs_to :target, polymorphic: true, integer_type: true
-
-  def source=(val)


these appeared unused and didn't affect specs so bye bye

kamillus · 2020-11-10T20:32:41Z

@@ -0,0 +1,34 @@
+module PolymorphicIntegerType
+  class ModuleGenerator
+    def self.generate_and_include(klass,foreign_type, name)


Do you have a unit test for this method?

It's pretty difficult to unit test this in isolation since it involves setting up polymorphic models and then overriding some of the behaviour(which is basically what we do in the specs already). This functionality was already tested thoroughly in the specs and this just abstracts it away into class

Drew-Goddyn · 2020-11-17T18:23:07Z


          # Required way to dynamically define a class method on the model
-          singleton_class.__send__(:define_method, "#{foreign_type}_mapping") do
+          define_singleton_method("#{foreign_type}_mapping") do


just switching to more idiomatic way of defining singleton methods

IIRC, define_singleton_method didn't exist in earlier versions of Ruby when we first wrote this module, and __send__ was the only way to do it. This is much nicer to read.

Drew-Goddyn · 2020-11-17T18:25:42Z

+
+          link = Namespaced::Plant.create(name: "Oak").source_links.new
+          expect(link.source_type).to eq("Namespaced::Plant")
+          allow(Link).to receive(:source_type_mapping).and_return({1 => "Person", 2 => "Animal", 3 => "Plant"})


woops, don't think this is needed, gonna remove it

daniel-almeida

Good job with the comments! They made this much easier to review. 💯

fimmtiu · 2020-11-18T17:40:24Z

@@ -0,0 +1,11 @@
+module Namespaced
+  class Activity< ActiveRecord::Base


Suggested change

class Activity< ActiveRecord::Base

class Activity < ActiveRecord::Base

fimmtiu

Approved with one trivial suggestion.

Drew-Goddyn commented Nov 6, 2020

View reviewed changes

Drew-Goddyn requested review from a team, doliveirakn and fimmtiu and removed request for a team November 6, 2020 20:42

Drew-Goddyn changed the title ~~Fix creating polymorhic records~~ Fix creating polymorphic associations Nov 6, 2020

Drew-Goddyn requested review from daniel-almeida and kamillus and removed request for doliveirakn November 6, 2020 20:55

kamillus reviewed Nov 10, 2020

View reviewed changes

Drew-Goddyn commented Nov 17, 2020

View reviewed changes

daniel-almeida approved these changes Nov 17, 2020

View reviewed changes

Drew-Goddyn requested a review from kamillus November 17, 2020 21:25

fimmtiu reviewed Nov 18, 2020

View reviewed changes

fimmtiu approved these changes Nov 18, 2020

View reviewed changes

Drew-Goddyn added 3 commits November 18, 2020 12:37

fix creating assocations with specs and add module generator

6677535

remove extra useless stub

a733054

fix class sparing

9b62446

Drew-Goddyn force-pushed the fix-creating-polymorhic-records branch from bb77ff5 to 9b62446 Compare November 18, 2020 20:37

Drew-Goddyn merged commit 2ca2095 into clio:master Nov 18, 2020

Drew-Goddyn deleted the fix-creating-polymorhic-records branch November 18, 2020 20:38

		@@ -0,0 +1,11 @@
		module Namespaced
		class Activity< ActiveRecord::Base

	class Activity< ActiveRecord::Base
	class Activity < ActiveRecord::Base

Conversation

Drew-Goddyn commented Nov 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Drew-Goddyn Nov 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daniel-almeida left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fimmtiu left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Drew-Goddyn commented Nov 6, 2020 •

edited

Loading

Drew-Goddyn Nov 6, 2020 •

edited

Loading