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

Add association between referral and referrer #773

Merged
merged 4 commits into from Feb 12, 2023

Conversation

PatrickMcSweeny
Copy link
Contributor

@PatrickMcSweeny PatrickMcSweeny commented Feb 5, 2023

Closes #736

Pull request checklist

  • My code contains tests covering the code I modified
  • I linted and tested the project with bin/check
  • I added significant changes and product updates to the changelog

Copy link
Owner

@joemasilotti joemasilotti left a comment

Choose a reason for hiding this comment

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

I don't want any referral-based columns to exist on the users table, so I think we still need another model. The diagram in the issue outlined a ReferralCode which belongs_to :user and I think we still want that.

This will rework the associations on the User model to has_one :referral_code, has_one :referrer, and has_many :referrals with the necessary through: options.

We can also add a unique constraint to the code on the new table in the database, along with the existing Rails validation.

@PatrickMcSweeny
Copy link
Contributor Author

Currently there is a has_one :referral association on the User model so I think it would be confusing to have a has_many :referrals association.

@joemasilotti
Copy link
Owner

Good point. I think that it should actually be has_one :referral and has_many :referees.

@PatrickMcSweeny
Copy link
Contributor Author

@joemasilotti I updated the commit

@joemasilotti
Copy link
Owner

Thanks for the updates! I pulled down the code and I think I glossed over a major fallacy with this approach: it is terribly hard to reason around what is going on! Not your code, but the data modeling that I proposed.

Thanks to your initial commit I'm leaning back towards getting rid of the ReferralCode model entirely. Instead, a Referral can have an optional relationship with the referrer. I think that should simplify things quite a bit.

I also think that I haven't given the naming conventions of this enough though. Here's what I'd love the naming to be:

  • A user has one referrer - the person who referred them
  • A user has many referrals - the people they've referred
  • A referral belongs to a referee (of class "User")
  • A referral belongs to an optional referrer (of class "User")

Which I think means we need to add where clauses to the relationship references on the user model. Otherwise, it would always include records where the user was a referrer or referee.


Does that make sense? Or am I overthinking this?

Let me know if you'd like to give it another pass. And no worries if not – I understand I've jumped around a lot with this one!

@PatrickMcSweeny
Copy link
Contributor Author

That comes pretty close to the commit I originally made. The main difference is that the relationship I used for the referrals made the user is referrals_made.

@PatrickMcSweeny
Copy link
Contributor Author

I pushed the original commit to the remote branch. Take a look and let me know if it needs any adjustments.

@joemasilotti
Copy link
Owner

Awesome, thank you again!

I pulled out the referral-related code to a module and tweaked the names a bit more. I think that in theory "referrer" and "referee" make sense but in practice I have a hard time wrapping my head around which is which.

I like where this Referrable model ended up, especially the alias.

module Referrable
  extend ActiveSupport::Concern

  included do
    has_one :referral, foreign_key: "referred_user_id", dependent: :destroy
    has_one :referring_user, through: :referral
    alias_method :referred_by, :referring_user

    has_many :referrals, foreign_key: "referring_user_id", dependent: :nullify
    has_many :referred_users, through: :referrals

    validates :referral_code, uniqueness: true, allow_nil: true
  end
end

user.referred_by # => User
user.referred_users # => [User]

@joemasilotti joemasilotti merged commit ed7756a into joemasilotti:main Feb 12, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rework modeling of referral program
2 participants