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.

Sign up for GitHub

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

Jump to bottom

Moving anon user tracking from image element to form submit #433

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Moving anon user tracking from image element to form submit #433

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
12fdd70
Moving anon user tracking from image element to form submit
Apr 3, 2018
6dd80be
fixes in handling anon user and associating with actual user upon login
Apr 4, 2018
8648385
added comment
Apr 4, 2018
72139e2
moved eventsession setting to per request
Apr 5, 2018
426e753
renamed sniffle and moved to api views
Apr 5, 2018
ce31a8a
removed check for g.esession
Apr 5, 2018
902e2dc
fixed conflict
Apr 9, 2018
80ba99d
deleting session cookie if user is logged in
Apr 10, 2018
424e5cd
fetching g.esession from cache after anon user is saved in db
Apr 11, 2018
8c41e6c
fixed conflict
Apr 18, 2018
507e6b3
session['au'] can only be integer now, so converting to string
Apr 18, 2018
24380f4
removed anon macro import from templates where it's not needed anymore
Apr 18, 2018
6f88b9d
Loading session details from cache when creating anon user
May 7, 2018
b9234e9
saving and loading session from cache
May 7, 2018
3c1fa76
Fixed the eventsession flow
May 14, 2018
6f34e5d
No longer checking for dirty session before committing
May 17, 2018
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
2 changes: 2 additions & 0 deletions hasjob/templates/formlayout.html.jinja2
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
{% from "macros.html.jinja2" import anon_user_script %}
{%- if current_view.tabs -%}
{%- extends "tablayout.html.jinja2" -%}
{%- else -%}
Expand Down Expand Up @@ -27,4 +28,5 @@
{%- if header_campaign %}{{ campaign_script() }}{% endif %}
{% assets "js_tinymce" %}<script src="{{ ASSET_URL }}" type="text/javascript"></script>{% endassets %}
{% block footerscripts %}{% endblock %}
{{ anon_user_script() }}
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 in layout.html.jinja2 only. We have a problem if we’re copy pasting an entire block between templates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i went through the templates, and they need some reorganization. Some blocks have been redefined in several places. I can fix them, but seems a little off topic for this PR. Maybe a separate PR for it?

Copy link
Member

Choose a reason for hiding this comment

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

This will change with @vidya-ram's PWA PR (#425). We should revisit this PR once that is merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jace alright.

{% endblock %}
8 changes: 3 additions & 5 deletions hasjob/templates/layout.html.jinja2
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{%- extends "baseframe.html.jinja2" -%}
{%- from "baseframe/components.html.jinja2" import hgnav -%}
{%- from "macros.html.jinja2" import campaign_header, campaign_script, filters_setup_script -%}
{%- from "macros.html.jinja2" import campaign_header, campaign_script, filters_setup_script, anon_user_script -%}

{%- block doctypehtml -%}
<!DOCTYPE html>
Expand Down Expand Up @@ -54,7 +54,7 @@
{{ filters_setup_script(job_filters, data_filters) }}
{%- else %}
{{ filters_setup_script(job_filters) }}
{%- endif %}
{%- endif %}
{%- endif %}
{%- endblock %}

Expand Down Expand Up @@ -225,13 +225,11 @@
to find out when new jobs are posted. Hosted by
<a href="http://e2enetworks.com/">E2E Networks</a>.
{%- endif %}
{%- if not g.user and not g.anon_user %}
<img src="{{ url_for('sniffle') }}" width="1" height="1" alt=""/>
{%- endif %}
</p>
{% endblock %}

{% block layoutscripts %}
{%- if header_campaign %}{{ campaign_script() }}{% endif %}
{% block footerscripts %}{% endblock %}
{{ anon_user_script() }}
{% endblock %}
34 changes: 34 additions & 0 deletions hasjob/templates/macros.html.jinja2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,3 +207,37 @@
</script>
{%- endwith %}
{%- endmacro -%}

{%- macro anon_user_script() -%}
Copy link
Member

Choose a reason for hiding this comment

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

Can this be moved into app.js? It adds bulk to every page otherwise (to pages served to bots).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But if we move it to app.js then it'll load with all page load regardless of bots or humans? Also, will have to hardcode the api endpoint.

Copy link
Member

Choose a reason for hiding this comment

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

It will load only once because app.js is cached.

{%- if not g.user and not g.anon_user %}
<script type="text/javascript">
$(function () {
var sniffurl = "{{ url_for('sniffle') }}";
Copy link
Member

Choose a reason for hiding this comment

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

“Sniff” is the wrong term for this. We’re looking for an interactive session.

var sniffed = false;
var sniff = function (event) {
jQuery.post(
sniffurl,
{
event: event,
token: "{{ session['au'] }}"
},
function (data) {
console.log(data);
})
sniffed = true;
}
window.onscroll = function (e) {
if (!sniffed) {
sniff("scolll");
Copy link
Member

Choose a reason for hiding this comment

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

Spelling.

}
}
window.onpointermove = function (e) {
if (!sniffed) {
sniff("pointermove");
}
}
})
</script>
{%- endif %}
{%- endmacro -%}

3 changes: 2 additions & 1 deletion hasjob/templates/sheet.html.jinja2
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{%- extends "layout.html.jinja2" -%}
{%- from "macros.html.jinja2" import campaign_header, campaign_script -%}
{%- from "macros.html.jinja2" import campaign_header, campaign_script, anon_user_script -%}
{% block messages %}{% endblock %}
{% block basecontent %}
{%- if header_campaign %}<div id="header-campaign"></div>{% endif %}
Expand All @@ -24,4 +24,5 @@
{% block layoutscripts %}
{%- if header_campaign %}{{ campaign_script() }}{% endif %}
{% block footerscripts %}{% endblock %}
{{ anon_user_script() }}
{% endblock %}
2 changes: 2 additions & 0 deletions hasjob/templates/tablayout.html.jinja2
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
{% from "macros.html.jinja2" import anon_user_script %}
{%- extends "layout.html.jinja2" -%}
{% block pageheaders %}
<link href="//cdnjs.cloudflare.com/ajax/libs/c3/0.4.14/c3.min.css" rel="stylesheet"/>
Expand Down Expand Up @@ -31,4 +32,5 @@
<script type="text/javascript" src="//cdnjs.cloudflare.com/ajax/libs/d3/3.5.17/d3.min.js"></script>
<script type="text/javascript" src="//cdnjs.cloudflare.com/ajax/libs/c3/0.4.14/c3.min.js"></script>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jace should these static files be in tablayout template? Shouldn't they be in individual templates that inherit them and needs c3/d3?

Copy link
Member

Choose a reason for hiding this comment

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

We now use C3 enough that it should be in the top level requirements, and all these direct references should be removed.

{% block footerscripts %}{% endblock %}
{{ anon_user_script() }}
{% endblock %}
129 changes: 59 additions & 70 deletions hasjob/views/helper.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
from sqlalchemy import or_
from sqlalchemy.exc import IntegrityError
from geoip2.errors import AddressNotFoundError
from flask import Markup, request, g, session
from flask import Markup, request, g, session, Response
from flask_rq import job
from flask_lastuser import signal_user_looked_up
from coaster.sqlalchemy import failsafe_add
Expand All @@ -28,14 +28,38 @@
gif1x1 = 'R0lGODlhAQABAJAAAP8AAAAAACH5BAUQAAAALAAAAAABAAEAAAICBAEAOw=='.decode('base64')
Copy link
Member

Choose a reason for hiding this comment

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

This line can go. It's not needed anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jace there is a view_application_email_gif endpoint where it's being used

hasjob/hasjob/views/listing.py

Line 397 in 41450f7

return gif1x1, 200, {



@app.route('/_sniffle.gif')
@app.route('/_sniffle', methods=['POST'])
Copy link
Member

Choose a reason for hiding this comment

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

This will now be /api/1/anonsession. This should also move into an views/api.py.

def sniffle():
return gif1x1, 200, {
'Content-Type': 'image/gif',
'Cache-Control': 'no-cache, no-store, must-revalidate',
'Pragma': 'no-cache',
'Expires': '0'
}
"""
Load anon user:

1. If there's g.user and session['anon_user'], it loads that anon_user and tags with user=g.user, then removes anon
2. If there's no g.user and no session['anon_user'] and form submitted token matches session['au'], sets session['anon_user'] = 'test'
3. If there's no g.user and there is session['au'] != form['token'], loads g.anon_user
Copy link
Member

Choose a reason for hiding this comment

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

2 and 3 seem to be reversed. Also, you don’t need a test token in session['au'] anymore. A valid form CSRF token is enough. Our entire test token mechanism can be removed.

"""
if not g.user:
if unicode(session['au']).startswith('test') and unicode(session['au']) == request.form.get('token'):
# This client sent us back our test cookie, so set a real value now
g.anon_user = AnonUser()
db.session.add(g.anon_user)
g.esession = EventSession.new_from_request(request)
g.esession.anon_user = g.anon_user
db.session.add(g.esession)
g.esession.load_from_cache(session['au'], UserEvent)
# We'll update session['au'] below after database commit
else:
# form submitted token doesn't match the already set session['au']
# XXX: We got a fake value? This shouldn't happen
g.event_data['anon_cookie_test'] = session['au']
session['au'] = u'test-' + unicode(uuid4()) # Try again
g.esession = EventSessionBase.new_from_request(request)
db.session.commit()

if g.anon_user:
session['au'] = g.anon_user.id
session.permanent = True

return Response("OK")
Copy link
Member

Choose a reason for hiding this comment

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

Don’t reply with text. Return {'status': 'ok'} and wrap the view with render_with(json=True).



def index_is_paginated():
Expand Down Expand Up @@ -66,18 +90,13 @@ def load_user_data(user):
"""
All pre-request utilities, run after g.user becomes available.

Part 1: Load anon user:

1. If there's g.user and session['anon_user'], it loads that anon_user and tags with user=g.user, then removes anon
2. If there's no g.user and no session['anon_user'], sets session['anon_user'] = 'test'
3. If there's no g.user and there is session['anon_user'] = 'test', creates a new anon user, then saves to cookie
4. If there's no g.user and there is session['anon_user'] != 'test', loads g.anon_user

Part 1: If session['au'] exists, either set g.anon_user or set anon_user.user (if g.user exists).
If session['au'] does not exist, set it
Part 2: Are we in kiosk mode? Is there a preview campaign?
Part 3: Look up user's IP address location as geonameids for use in targeting.
"""
g.anon_user = None # Could change below
g.event_data = {} # Views can add data to the current pageview event
g.anon_user = None
g.event_data = {}
Copy link
Member

Choose a reason for hiding this comment

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

Keep the comments.

g.esession = None
g.viewcounts = {}
g.impressions = session.pop('impressions', {}) # Retrieve from cookie session if present there
Expand All @@ -87,65 +106,35 @@ def load_user_data(user):
now = datetime.utcnow()

if request.endpoint not in ('static', 'baseframe.static'):
# Loading an anon user only if we're not rendering static resources
if user:
if 'au' in session and session['au'] is not None and not unicode(session['au']).startswith(u'test'):
if 'au' in session and session['au'] is not None:
if not unicode(session['au']).startswith(u'test'):
# fetch anon user and set anon_user.user
anon_user = AnonUser.query.get(session['au'])
if anon_user:
anon_user.user = user
session.pop('au', None)
else:
if not session.get('au'):
session['au'] = u'test-' + unicode(uuid4())
g.esession = EventSessionBase.new_from_request(request)
g.event_data['anon_cookie_test'] = session['au']
# elif session['au'] == 'test': # Legacy test cookie, original request now lost
# g.anon_user = AnonUser()
# db.session.add(g.anon_user)
# g.esession = EventSession.new_from_request(request)
# g.esession.anon_user = g.anon_user
# db.session.add(g.esession)
# # We'll update session['au'] below after database commit
# elif unicode(session['au']).startswith('test-'): # Newer redis-backed test cookie
# # This client sent us back our test cookie, so set a real value now
# g.anon_user = AnonUser()
# db.session.add(g.anon_user)
# g.esession = EventSession.new_from_request(request)
# g.esession.anon_user = g.anon_user
# db.session.add(g.esession)
# g.esession.load_from_cache(session['au'], UserEvent)
# # We'll update session['au'] below after database commit
else:
anon_user = None # AnonUser.query.get(session['au'])
if not anon_user:
# XXX: We got a fake value? This shouldn't happen
g.event_data['anon_cookie_test'] = session['au']
session['au'] = u'test-' + unicode(uuid4()) # Try again
g.esession = EventSessionBase.new_from_request(request)
else:
if anon_user and g.user:
# we have anon user id in session['au'], set anon_user.user to current user
anon_user.user = g.user
g.db_commit_needed = True
Copy link
Member

Choose a reason for hiding this comment

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

Ugh, what is this? We should be doing automatic database commits if there’s a dirty session and no exception was raised. Better than such hacky flags.

session.pop('au', None)
elif anon_user and not g.user:
# set g.anon_user
g.anon_user = anon_user

# Prepare event session if it's not already present
if g.user or g.anon_user and not g.esession:
g.esession = EventSession.get_session(uuid=session.get('es'), user=g.user, anon_user=g.anon_user)
if g.esession:
session['es'] = g.esession.uuid

# Don't commit here. It flushes SQLAlchemy's session cache and forces
# fresh database hits. Let after_request commit. (Commented out 30-03-2016)
# db.session.commit()
g.db_commit_needed = True

if g.anon_user:
session['au'] = g.anon_user.id
session.permanent = True
if 'impressions' in session:
elif not g.user:
session['au'] = u'test-' + unicode(uuid4())
Copy link
Member

Choose a reason for hiding this comment

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

This is no longer needed since we’ll use the CSRF token cookie now.

g.esession = EventSessionBase.new_from_request(request)
g.event_data['anon_cookie_test'] = session['au']

# Prepare event session if it's not already present
if g.user or g.anon_user and not g.esession:
g.esession = EventSession.get_session(uuid=session.get('es'), user=g.user, anon_user=g.anon_user)
if g.esession:
session['es'] = g.esession.uuid

if g.anon_user and 'impressions' in session:
# Run this in the foreground since we need this later in the request for A/B display consistency.
# This is most likely being called from the UI-non-blocking sniffle.gif anyway.
save_impressions(g.esession.id, session.pop('impressions').values(), now)
Copy link
Member

Choose a reason for hiding this comment

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

Move this to a function that is called both here and in the the anonsession endpoint after setting g.anon_user. Comment needs to change as well.


# We have a user, now look up everything else

if session.get('kiosk'):
g.kiosk = True
else:
Expand Down Expand Up @@ -313,7 +302,7 @@ def session_jobpost_ab():
Returns the user's B-group assignment (NA, True, False) for all jobs shown to the user
in the current event session (impressions or views) as a dictionary of {id: bgroup}
"""
if not g.esession.persistent:
if g.esession and not g.esession.persistent:
Copy link
Member

Choose a reason for hiding this comment

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

When does g.esession not exist?

return {key: value[2] for key, value in session.get('impressions', {}).items()}
result = {ji.jobpost_id: ji.bgroup for ji in JobImpression.query.filter_by(event_session=g.esession)}
result.update({jvs.jobpost_id: jvs.bgroup for jvs in JobViewSession.query.filter_by(event_session=g.esession)})
Expand Down
1 change: 0 additions & 1 deletion hasjob/views/index.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,6 @@ def index(basequery=None, filters={}, md5sum=None, tag=None, domain=None, locati
BoardJobPost.board == g.board, JobPost.state.LISTED).options(
db.load_only('jobpost_id', 'pinned')).all()
}

else:
board_jobs = {}

Expand Down