Join affiliates with owners #118

huyserp · 2021-05-17T15:35:16Z

Seeded my local db with 7 affiliates and 20 owners
- 9 owners get an affiliate code string in owners.affiliate column.
- 3 owners get an affiliate code that doesn’t correspond to an affiliate instance
  - To test the hypothetical that some owners may have content in the affiliate string but that content doesn't match an instance of an affiliate.
Ran migration - change column name in owners table from affiliate to affiliate_code
- After next migration to add a foreign key, owner.affiliate would refer to both the old string in the owners table AND the newly established association to an Affiliate object, which is problematic...
Ran migration - add foreign key affiliates to owners table.
- Also update associations in owner.rb and affiliate.rb
  - Owner belongs to affiliate (optional: true)
  - Affiliate has_many owners
Ran script to join data in tables. Used rails runner (this script is in /db/scripts/join_affiliate_to_owner.rb)
- Filter Owner.where(affiliate_code == true)
- Iterate over filtered owners
  - If the affiliate_code value corresponds to an affiliate instance (by affiliate.code) set the Owner.affiliate to this instance of affiliate.
Add class method to owner.rb to quickly identify owners that have a string in affiliate_code which doesn’t correspond to an affiliate instances.
- Test: Owner.non_associated_affiliates.each { |owner| puts owner.affiliate_code } (there should be 3, each string begins with 'a' based on my seed.rb)
Review models to update any code that gets an affiliate with find_by
- Changes:
  - Owner - use new association to get owner.affiliate
    - #affiliate_logo
    - #stripe_price_id

Testing was done in rails console.

…code in owners table

…ing affiliate code

…ith no corresponding affiliate

CLAassistant · 2021-05-17T15:35:22Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

bonflintstone

We need to make sure that for new owners we set the affiliate on creation. This probably needs to happen in the owners_controller, so that based on the affiliate code we associate the right model directly.
We should also consider how to handle this in the frontend. I think it might actually be nice if the backend returns an error if an affiliate code is sent be the frontend that does not exist in the database. But in that case we should make sure that the frontend handles that error gracefully and displays a helpful error message to the user, so that they can double check the affiliate link or simply create an account without affiliate link.

bonflintstone · 2021-05-19T15:38:50Z

app/models/owner.rb

@@ -50,8 +51,7 @@ def blocked?

  def affiliate_logo
    return unless affiliate
-
-    Affiliate.find_by(code: affiliate)&.logo_url
+    affiliate.logo_url


Suggested change

affiliate.logo_url

affiliate&.logo_url

In case you have not seen before, it's called the safe navigation operator and makes sure we do not get an exception when affiliate is nil here :)

bonflintstone · 2021-05-19T15:39:12Z

app/models/owner.rb

@@ -74,8 +74,7 @@ def stripe_price_id
    main_price_id = ENV['STRIPE_SUBSCRIPTION_PRICE_ID']

    raise "ENV['STRIPE_SUBSCRIPTION_PRICE_ID'] is empty" unless main_price_id.present?
-
-    Affiliate.find_by(code: affiliate)&.stripe_price_id_monthly || main_price_id
+    affiliate.stripe_price_id_monthly || main_price_id


Suggested change

affiliate.stripe_price_id_monthly || main_price_id

affiliate&.stripe_price_id_monthly || main_price_id

Elsewise this will also crash for empty affiliates

bonflintstone · 2021-05-19T15:41:41Z

app/models/owner.rb

@@ -106,4 +105,9 @@ def trial_end
    # There is not trial if the trial is blank or has already been passed, else it has to be at least two days in the future
    trial_ends_at? && trial_ends_at.future? ? [trial_ends_at, 50.hours.from_now].max.to_i : nil
  end
+
+  def self.non_associated_affiliates


This should be a scope instead of a class method, then one can also chain this, e.g.:

Owner.where('created_at > ?', 1.week.ago).non_associated_affiliates

Perhaps a better name for this scope would then also be "with_missing_affiliate" or similar

bonflintstone · 2021-05-19T15:42:33Z

app/models/owner.rb

+
+  def self.non_associated_affiliates
+    # To identify owners that have used an affiliate thats not properly registered in the affiliates table.
+    Owner.where(affiliate: nil).where.not(affiliate_code: nil)


You can actually leave the Owner., since where is also defined as an instance method. This would also enable chaining.

bonflintstone · 2021-05-19T15:43:30Z

db/scripts/join_affiliate_to_owner.rb

@@ -0,0 +1,9 @@
+owners_with_affiliate = Owner.all.select { |owner| owner.affiliate_code }


This should be a rake task :)

bonflintstone · 2021-05-19T15:44:31Z

db/scripts/join_affiliate_to_owner.rb

@@ -0,0 +1,9 @@
+owners_with_affiliate = Owner.all.select { |owner| owner.affiliate_code }


This can be written as a db query. Right now you first load all owners from the database to an array, and then filter it. This operation can be done way more efficiently by the database using the active record query interface. In this case one might use:
Owner.where.not(affiliate_code: nil)

bonflintstone · 2021-05-19T15:45:22Z

db/seeds.rb

+Affiliate.destroy_all
+Owner.destroy_all
+
+7.times do
+  FactoryBot.create(:affiliate)
+end
+affiliates = Affiliate.all
+affiliates.each { |affiliate| puts "#{affiliate.name} #{affiliate.code}" }
+
+20.times do
+  FactoryBot.create(:owner)
+end
+owners = Owner.all
+owners.each { |owner| puts owner.name }
+
+
+# give 9 owners an affilate
+owners_with_affiliates = owners[0..8]
+owners_with_affiliates.each do |owner|
+  owner.update!(affiliate_code: affiliates.sample.code)
+end
+
+# give 3 owners affiliates which are not yet entities in the db
+owners_with_other_affiliates = owners[9..11]
+owners_with_other_affiliates.each do |owner|
+  owner.update!(affiliate_code: affiliates.sample.code.prepend('a').chop)
+end


This should go into a spec (probably spec/modles/owner_spec.rb)

bonflintstone · 2021-05-19T15:49:51Z

app/models/owner.rb

+  validates :password,
+    allow_nil: true,
+    length: { in: Devise.password_length },
+    confirmation: true,
    on: :update



Right now, when migrating, we are going through three stages:

The app as is right now, owner.affiliate will return a string

The apps db is migrated, but the data migration script has not been run yet. Since the code is updated, the app should expect .affiliate to return a model, but it's always empty. If payments are executed in this period, owners with affiliates might pay the full price, or similar weird behaviour might occur

The data migration has run, now .affilaite will return the affilaite model

So I think for the transition it might make sense to overwrite the owner.affiliate method so it always returns the affiliate model. Something like

Suggested change

def affiliate

return super if super.present? # make sure the association is used after the data migration

Affiliate.find_by(code: affiliate_code) # before the data migration is run, we try to find the affiliate "the old fashioned way"

end

huyserp added 12 commits May 12, 2021 17:39

remove comment, test on docker db

9026ae7

seed db with 20 owners and 7 affiliates

e156c5b

generate migration to change column name from affiliate to affiliate_…

5dc53eb

…code in owners table

run migration successful - affiliate is now affiliate_code

c438343

generate migration to add affiliate:reference on owners table

68ba3c9

run migration to add affiliate foreign key to owners table

fde8f2c

update associations in affiliate and owner models

05257c4

fix typo in owner model - owner can be assigned an affiliate instance!

6e2f677

run script to join owners table with correct affiliate based on match…

8e5a51c

…ing affiliate code

update owners association optional: true

cb7509c

update owner model to accomodate new affiliate association

fdcfc2a

add class method to owner.rb identifing owners with affiliate_codes w…

62ce883

…ith no corresponding affiliate

huyserp requested a review from bonflintstone May 17, 2021 15:35

bonflintstone requested changes May 19, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Join affiliates with owners #118

Join affiliates with owners #118

huyserp commented May 17, 2021

CLAassistant commented May 17, 2021

bonflintstone left a comment

bonflintstone May 19, 2021

bonflintstone May 19, 2021

bonflintstone May 19, 2021

bonflintstone May 19, 2021

bonflintstone May 19, 2021

bonflintstone May 19, 2021

bonflintstone May 19, 2021

bonflintstone May 19, 2021

	affiliate.stripe_price_id_monthly \|\| main_price_id
	affiliate&.stripe_price_id_monthly \|\| main_price_id

		@@ -0,0 +1,9 @@
		owners_with_affiliate = Owner.all.select { \|owner\| owner.affiliate_code }

+  def affiliate
+    return super if super.present? # make sure the association is used after the data migration
+    Affiliate.find_by(code: affiliate_code) # before the data migration is run, we try to find the affiliate "the old fashioned way"
+  end

Join affiliates with owners #118

Are you sure you want to change the base?

Join affiliates with owners #118

Conversation

huyserp commented May 17, 2021

CLAassistant commented May 17, 2021

bonflintstone 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment