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
Allow test_track_visitor_id to be passed via headers #29
Allow test_track_visitor_id to be passed via headers #29
Conversation
Needs somebody from @jmileham, @samandmoore, and @aburgel to claim domain review. Use the shovel operator to claim, e.g.:
|
I think it should only be passed back when it changes, probably? Like a |
app/models/test_track/visitor.rb
Outdated
@@ -98,6 +98,10 @@ def loaded? | |||
!offline? && @remote_visitor.present? | |||
end | |||
|
|||
def id_changed? |
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.
Tried to follow rails naming conventions, but maybe this hides what this actually represents, the fact that the visitor id was overridden by the remote visitor?
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, I think you might want to go with a special name here. Remote won't have as much meaning to a browser of the controller mixin because to them the remote in mind is the client of the controller, not the upstream TT server, so some name that captures that the TT server changed our identity would be hot.
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.
id_overridden_by_existing_visitor?
?
bump |
subject.log_in!("identifier_type", "value") | ||
end | ||
|
||
expect(controller.response.headers['X-Set-TT-Visitor-ID']).to eq(nil) |
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.
not_to have_key or something?
<< domain tafn. Just come up with an awesome name and this is looking great. |
@betterzega needs to incorporate feedback from @jmileham. Bump when done. |
that'll work. it's a long enough name that it doesn't give you unwarranted confidence that you know what it's doing. domain lgtm! |
Ready to merge! 💫 🔩 👯 |
df57691
to
1bc57d4
Compare
/domain @jmileham @samandmoore @aburgel
From our conversation in the test_track channel, allowing the session to be read from the request header.
Still need to write back the test track visitor id in the response header. Should this only be passed back when the request has the TT visitor id in the header, or should it always be passed back?
Still need to do a proper integration test.