add user data for cookies for marketing #42634
Conversation
| private | ||
|
|
||
| def account_age_in_years |
There was a problem hiding this comment.
Just checking that we always want to round down here?
There was a problem hiding this comment.
I was on the fence about this, I'll change it to .round
dashboard/app/models/user.rb
Outdated
| end | ||
|
|
||
| # Returns a list of all courses that the teacher currently has sections for | ||
| def courses_being_taught |
There was a problem hiding this comment.
Should we call this something like curriculums_being_taught or something similar? (I'm worried that course means something specific in the model.)
dashboard/app/models/user.rb
Outdated
| end | ||
|
|
||
| def self.marketing_segment_data_keys | ||
| %w(locale account_age_in_years grades courses has_attended_pd within_usschool_percent_frl school_title_i) |
There was a problem hiding this comment.
Should we add a test that checks that this list is in sync with the data returned above?
| @@ -23,7 +23,7 @@ def setup | |||
|
|
|||
| sign_in_as section.teacher | |||
|
|
|||
| assert_queries 11 do | |||
| assert_queries 15 do | |||
There was a problem hiding this comment.
Are we worried about all of these query counts going up by so much? Do we incur this cost on every request or just login?
There was a problem hiding this comment.
These requests are made in after_set_user, so it should just be when the user logs in. It follows the pattern for setting cookies at login like the experiments cookie. It's not ideal that I've added extra queries, but I didn't see a great alternative.
There was a problem hiding this comment.
I moved where the cookies get set to the homepage which eliminated the extra queries.
maureensturgeon
force-pushed
the
maureen/LP-2039-segment-teacher-banner
branch
from
161b57b
to
a1a2775
Compare
|
Very interesting integration with Optimizely! Just curious about a couple things:
|
Before I added these cookies, I confirmed with Travis that there are no data privacy issues with adding these cookies.
The benefits of having the data in separate cookies is it makes it easy for marketing to set up their own custom audiences, but I think that you're right, it'd be possible to combine these into one cookie, it would just take a bit of additional work/guidance for marketing to set up new audiences. |
The changes here add cookies which can be used by marketing to segment the user experience on Optimizely. The cookies will be added when the teacher visits the homepage and will be removed on log-out. I was able to verify that the cookies were being set through tests and by running locally.
Links
Testing story
Tested through unit tests that set-cookies header had expected values
PR Checklist: