Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Join affiliates with owners #118

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions app/models/affiliate.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ class Affiliate < ApplicationRecord
EXPOSED_ATTRIBUTES = %i[name logo_link logo_url]

has_one_attached :logo
has_many :owners

validates :logo, blob: { content_type: ['image/png', 'image/jpg', 'image/jpeg'] }

Expand Down
32 changes: 18 additions & 14 deletions app/models/owner.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,22 +9,23 @@ class Owner < ApplicationRecord
:confirmable, :recoverable, jwt_revocation_strategy: JwtDenylist

validates :email, uniqueness: true, presence: true
validates :password,
presence: true,
length: { in: Devise.password_length },
confirmation: true,
on: :create

validates :password,
allow_nil: true,
length: { in: Devise.password_length },
confirmation: true,
validates :password,
presence: true,
length: { in: Devise.password_length },
confirmation: true,
on: :create

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. The app as is right now, owner.affiliate will return a string
  2. 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
  3. 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

scope :affiliate, -> { where.not(affiliate: [nil, '']).order(affiliate: :asc) }
scope :with_stripe_data, -> { where.not(stripe_subscription_id: nil) }

belongs_to :frontend
belongs_to :affiliate, optional: true

has_many :companies, dependent: :destroy
has_many :areas, through: :companies
Expand All @@ -50,8 +51,7 @@ def blocked?

def affiliate_logo
return unless affiliate

Affiliate.find_by(code: affiliate)&.logo_url
affiliate.logo_url
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

end

def frontend_url
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

end

def stripe_subscription
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

# 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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

end
end
5 changes: 5 additions & 0 deletions db/migrate/20210514123815_change_affiliate_column_name.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class ChangeAffiliateColumnName < ActiveRecord::Migration[6.1]
def change
rename_column :owners, :affiliate, :affiliate_code
end
end
5 changes: 5 additions & 0 deletions db/migrate/20210514152111_add_affiliate_ref_to_owners.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class AddAffiliateRefToOwners < ActiveRecord::Migration[6.1]
def change
add_reference :owners, :affiliate, foreign_key: true, type: :uuid
end
end
7 changes: 5 additions & 2 deletions db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema.define(version: 2021_05_05_084831) do
ActiveRecord::Schema.define(version: 2021_05_14_152111) do

# These are extensions that must be enabled in order to support this database
enable_extension "pgcrypto"
Expand Down Expand Up @@ -116,7 +116,7 @@
t.string "confirmation_token"
t.datetime "confirmed_at"
t.datetime "confirmation_sent_at"
t.string "affiliate"
t.string "affiliate_code"
t.string "stripe_customer_id"
t.string "stripe_subscription_id"
t.datetime "trial_ends_at"
Expand All @@ -131,6 +131,8 @@
t.string "street"
t.string "zip"
t.string "city"
t.uuid "affiliate_id"
t.index ["affiliate_id"], name: "index_owners_on_affiliate_id"
t.index ["confirmation_token"], name: "index_owners_on_confirmation_token", unique: true
t.index ["email"], name: "index_owners_on_email", unique: true
t.index ["frontend_id"], name: "index_owners_on_frontend_id"
Expand Down Expand Up @@ -165,6 +167,7 @@
add_foreign_key "areas", "companies"
add_foreign_key "companies", "owners"
add_foreign_key "data_requests", "companies"
add_foreign_key "owners", "affiliates"
add_foreign_key "owners", "frontends"
add_foreign_key "tickets", "areas"
end
9 changes: 9 additions & 0 deletions db/scripts/join_affiliate_to_owner.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
owners_with_affiliate = Owner.all.select { |owner| owner.affiliate_code }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a rake task :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)


owners_with_affiliate.each do |owner|
matching_affiliate = Affiliate.find_by(code: owner.affiliate_code)
if matching_affiliate
owner.affiliate = matching_affiliate
owner.save!
end
end
34 changes: 27 additions & 7 deletions db/seeds.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,27 @@
# This file should contain all the record creation needed to seed the database with its default values.
# The data can then be loaded with the rails db:seed command (or created alongside the database with db:setup).
#
# Examples:
#
# movies = Movie.create([{ name: 'Star Wars' }, { name: 'Lord of the Rings' }])
# Character.create(name: 'Luke', movie: movies.first)
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
Comment on lines +1 to +27
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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