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
Show race interstitial 7 days after first log in #35857
Conversation
@Hamms @bethanyaconnor just tagging a couple foundations folks to take a quick peek / let y'all know I am doing this! |
@@ -201,6 +201,7 @@ class User < ActiveRecord::Base | |||
class_name: 'Pd::Application::ApplicationBase', | |||
dependent: :destroy | |||
|
|||
has_many :sign_ins |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we are just adding this now, will we get into issues of everyone seeing the race interstitial because their first sign in will be when we merge this code? Or has it been running already we just haven't accessed it explicitly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah exactly re: running already and not explicitly accessed, we've been logging to this table since early 2017 (289 million rows 😬 ). So, it's possible that a user would have no entries in this table (eg, account created 2016, hasn't signed in since), which I think I've accounted for 🤞.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh neat!
@@ -5,7 +5,7 @@ def self.show?(user) | |||
return false if user.races | |||
return false if user.teacher? | |||
return false if user.under_13? | |||
return false if user.account_age_days < 7 | |||
return false if user.days_since_first_sign_in && user.days_since_first_sign_in < 7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth adding a test to dashboard/test/helpers/race_interstitial_helper_test.rb
to test the case we're fixing here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did not realize we had tests there, which are now failing 😄 . Will update!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated, give it a look when you have a second!
Instead of showing an interstitial to ask students aged 13 and older for their race 7 days after account creation, we want to ask 7 days after their first sign in. We're trying to avoid the case where a teacher creates an account for their students, and when their student logs in for the first time two weeks later they're greeted with a message asking for their race. This change is also necessary such that we can show a new dialog thanking our donors when users log in for the first time.
This also is the first time we've made use of the
SignIn
model as far as I can tell in Rails code, but I think it's well set up to be used (ie, has an index on user ID so should be searchable quickly). It was originally added by an old engineer (Asher) for analytics purposes.Links
Testing story
Added new unit tests to cover new user methods. Also tested locally by creating a new user account, created a fake sign in from 10 days ago, and confirmed that I then saw the dialog. I had to disable some geocoding checks that we use to decide whether to ask a user for their race to get it to show up.
Reviewer Checklist: