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

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 8 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 anonymized = false;
var csrf_token = $('meta[name="csrf-token"]').attr('content');
var anonymize = function (event) {
$.post(
"{{ url_for('anon_session') }}",
{
event: event,
csrf_token: csrf_token
},
function (data) {
console.log(data);
});
anonymized = true;
}
window.onscroll = function (e) {
if (!anonymized) {
anonymize("scroll");
}
}
window.onpointermove = function (e) {
if (!anonymized) {
anonymize("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 %}
37 changes: 34 additions & 3 deletions hasjob/views/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
# -*- coding: utf-8 -*-

from flask import jsonify
from coaster.views import requestargs
from ..models import Tag, Domain
from flask import jsonify, g, session, request, Response
from flask_wtf import FlaskForm
from coaster.views import requestargs, render_with

from ..models import db, Tag, Domain, AnonUser, EventSession, EventSessionBase, UserEvent
from .. import app, lastuser


Expand All @@ -18,3 +20,32 @@ def tag_autocomplete(q):
@requestargs('q')
def domain_autocomplete(q):
return jsonify({'domains': [d.name for d in Domain.autocomplete(q)]})


@app.route('/api/1/anonsession', methods=['POST'])
@render_with(json=True)
def anon_session():
"""
Load anon user:

1. If client returns valid csrf token, create and set 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.

Sends, not returns.

2. if g.anon_user exists, set session['au'] to anon_user.id
"""
csrf_form = FlaskForm()
if csrf_form.validate_on_submit():
Copy link
Member

Choose a reason for hiding this comment

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

if not g.user and not g.anon_user and csrf_form.validate_on_submit():

# This client sent us back valid csrf token
Copy link
Member

Choose a reason for hiding this comment

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

"sent us", not "sent us back"

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)
if 'au' in session:
g.esession.load_from_cache(session['au'], UserEvent)
# We'll update session['au'] below after database commit
db.session.commit()

if g.anon_user:
session['au'] = g.anon_user.id
Copy link
Member

Choose a reason for hiding this comment

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

Are we using session['au'] for two different sorts of content? That doesn't seem clean.

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'm using session['au'] only to store anon user ID now. if it exists in cookie, it'll contain the anon user id.

session.permanent = True

return Response({'status': 'ok'})
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be jsonified.

110 changes: 34 additions & 76 deletions hasjob/views/helper.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

from os import path
from datetime import datetime, timedelta
from uuid import uuid4
from urllib import quote, quote_plus
import hashlib
import bleach
Expand All @@ -19,25 +18,16 @@

from .. import app, redis_store, lastuser
from ..extapi import location_geodata
from ..models import (agelimit, newlimit, db, JobCategory, JobPost, JobType, POST_STATE, BoardJobPost, Tag, JobPostTag,
Campaign, CampaignView, CampaignAnonView, EventSessionBase, EventSession, UserEventBase, UserEvent, JobImpression,
JobViewSession, AnonUser, campaign_event_session_table, JobLocation, PAY_TYPE)
from ..models import (db, JobCategory, JobPost, JobType, BoardJobPost, Tag, JobPostTag,
Campaign, CampaignView, CampaignAnonView, EventSessionBase, EventSession, UserEventBase,
UserEvent, JobImpression, JobViewSession, AnonUser, campaign_event_session_table,
JobLocation, PAY_TYPE)
from ..utils import scrubemail, redactemail, cointoss


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')
def sniffle():
return gif1x1, 200, {
'Content-Type': 'image/gif',
'Cache-Control': 'no-cache, no-store, must-revalidate',
'Pragma': 'no-cache',
'Expires': '0'
}


def index_is_paginated():
return request.method == 'POST' and 'startdate' in request.values

Expand Down Expand Up @@ -66,18 +56,12 @@ 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).
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 +71,38 @@ 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 unicode(session['au']).startswith('test'):
# old test token that we no longer need
session.pop('au', None)
Copy link
Member

Choose a reason for hiding this comment

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

session.pop('au') since it's guaranteed present. If it's not, that's definitely an error worth raising for us to investigate.

else:
# fetch anon user and set anon_user.user if g.user exists
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 g.user:
anon_user.user = g.user
session.pop('au', None)
Copy link
Member

Choose a reason for hiding this comment

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

Under if g.user: add g.anon_user = None, and then an else clause before the following line. You don't want to have both g.user and 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:
else:
# the AnonUser record has been deleted for some reason,
# this should not happen.
session.pop('au', None)
elif not g.user:
# g.user, g.anon_user, session['au'], none of them are set
g.esession = EventSessionBase.new_from_request(request)

# 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 @@ -191,7 +148,7 @@ def load_user_data(user):

@app.after_request
def record_views_and_events(response):
if hasattr(g, 'db_commit_needed') and g.db_commit_needed:
if len(db.session.dirty) > 0 or len(db.session.new) > 0:
Copy link
Member

Choose a reason for hiding this comment

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

I'd move this into a generic function in Coaster so that this call can be commit_if_necessary(db.session)

Copy link
Member

Choose a reason for hiding this comment

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

Or we could just do db.session.commit() without an if condition. What is the impact of committing an empty transaction?

db.session.commit()

# We had a few error reports with g.* variables missing in this function, so now
Expand Down Expand Up @@ -300,7 +257,8 @@ def record_views_and_events(response):
jobpost_id=g.jobpost_viewed[0],
bgroup=g.jobpost_viewed[1])
else:
g.esession.save_to_cache(session['au'])
if 'au' in session:
g.esession.save_to_cache(session['au'])
Copy link
Member

Choose a reason for hiding this comment

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

Cache has to be saved before we have an anonymous user. It's not relevant after.

if g.impressions:
# Save impressions to user's cookie session to write to db later
session['impressions'] = g.impressions
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