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

iambibhas · 2018-04-03T20:12:49Z

Anytime the site is loaded and there is -

a scroll event
a mouse pointer move event

the anon user session is set using a form submit.

Fixes #409.

jace · 2018-04-05T09:05:55Z

hasjob/templates/formlayout.html.jinja2

@@ -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() }}


This should be in layout.html.jinja2 only. We have a problem if we’re copy pasting an entire block between templates.

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?

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

@jace alright.

jace · 2018-04-05T09:09:08Z

hasjob/templates/macros.html.jinja2

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


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

jace · 2018-04-05T09:09:37Z

hasjob/templates/macros.html.jinja2

+      }
+      window.onscroll = function (e) {
+        if (!sniffed) {
+          sniff("scolll");


jace · 2018-04-05T09:25:36Z

hasjob/views/helper.py

@@ -28,14 +28,38 @@
 gif1x1 = 'R0lGODlhAQABAJAAAP8AAAAAACH5BAUQAAAALAAAAAABAAEAAAICBAEAOw=='.decode('base64')


-@app.route('/_sniffle.gif')
+@app.route('/_sniffle', methods=['POST'])


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

jace · 2018-04-05T09:35:42Z

hasjob/views/helper.py

+
+    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


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.

jace · 2018-04-05T09:38:12Z

hasjob/views/helper.py

+        session['au'] = g.anon_user.id
+        session.permanent = True
+
+    return Response("OK")


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

jace · 2018-04-05T09:41:25Z

hasjob/views/helper.py

+                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


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.

jace · 2018-04-05T09:42:11Z

hasjob/views/helper.py

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


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

jace · 2018-04-05T09:44:08Z

hasjob/views/helper.py

@@ -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:


When does g.esession not exist?

iambibhas · 2018-04-06T05:34:07Z

hasjob/templates/tablayout.html.jinja2

@@ -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>


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

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

jace

A few notes. Will need another review after the other PRs are merged.

jace · 2018-04-15T20:16:19Z

hasjob/templates/formlayout.html.jinja2

@@ -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() }}


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

jace · 2018-04-15T20:17:32Z

hasjob/templates/tablayout.html.jinja2

@@ -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>


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

jace · 2018-04-15T20:19:28Z

hasjob/views/api.py

+        db.session.commit()
+
+    if g.anon_user:
+        session['au'] = g.anon_user.id


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

I'm using session['au'] only to store anon user ID now. if it exists in cookie, it'll contain the anon user id.

jace · 2018-04-15T20:19:51Z

hasjob/views/helper.py

+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')


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

@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, {

jace · 2018-04-27T06:07:27Z

hasjob/templates/macros.html.jinja2

@@ -224,3 +224,37 @@
    </script>
  {%- endwith %}
 {%- endmacro -%}
+
+{%- macro anon_user_script() -%}


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

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.

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

jace · 2018-04-27T06:13:52Z

hasjob/views/api.py

+    """
+    Load anon user:
+
+    1. If client returns valid csrf token, create and set g.anon_user


Sends, not returns.

jace · 2018-04-27T06:14:31Z

hasjob/views/api.py

+    """
+    csrf_form = FlaskForm()
+    if csrf_form.validate_on_submit():
+        # This client sent us back valid csrf token


"sent us", not "sent us back"

jace · 2018-04-27T06:15:07Z

hasjob/views/api.py

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


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

jace · 2018-04-27T06:17:05Z

hasjob/views/api.py

+
+    if g.anon_user:
+        session['au'] = g.anon_user.id
+        g.esession.load_from_cache(session['au'], UserEvent)


What does this do? session['au'] contains a brand new id. How is it a cache reference?

jace · 2018-04-27T06:18:00Z

hasjob/views/helper.py

-    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 = {}


Keep the comments.

jace · 2018-04-27T06:20:27Z

hasjob/views/helper.py

-                else:
+                    if g.user:
+                        anon_user.user = g.user
+                        session.pop('au', None)


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

jace · 2018-04-27T06:22:59Z

hasjob/views/helper.py

+        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)


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.

jace · 2018-04-27T06:23:58Z

hasjob/views/helper.py

@@ -301,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'])


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

jace · 2018-04-27T07:03:39Z

@iambibhas Here is how this is supposed to work.

First, we recognise three states for a user's identity:

Unknown: We know nothing about this client. It's new, and it could be a bot or a user, but we don't know yet.
Anon User: The client is behaving like a user using a browser, so we believe this is a human.
User: The user has explicitly logged in.

When the user transitions from stage 2 to stage 3, we want to preserve their browsing history. This is why we do g.anon_user.user = g.user following which we set g.anon_user = None. The session is retained on the anon user account, because the browsing activity was anonymous, but we link the anon user account to the regular user account for future use (it's meant for keeping track of A/B test behaviour: if they were shown A or B before they logged in, we need to remember which one they were shown even after they logged in).

The transition from stage 1 to 2 is trickier. At stage 1, it could be a scraping bot causing a lot of traffic. We don't want to track session history for that, but we also don't want to lose the history of a client that turns out to be an anon user. Therefore:

Assign a probe cookie session['au'] = uuid4() to every incoming request.
Create a non-persistent EventSession and give it the same user id. Save it to Redis cache with a 5 minute timeout.
When the anonsession API endpoint is called (previously, the sniffle endpoint), create an AnonUser account with the same userid, retrieve the session from cache, and now save it to the database.

A bot will also create a Redis EventSession entry, but it will be auto-discarded in five minutes.

jace · 2018-05-08T09:27:42Z

hasjob/views/api.py

+        if 'impression' in session:
+            rq_save_impressions(g.esession.id, session.pop('impressions').values(), now, delay=False)
+
+    return Response({'status': 'ok'})


This needs to be jsonified.

jace · 2018-05-08T09:28:31Z

hasjob/views/api.py

+    if g.anon_user:
+        session['au'] = g.anon_user.id
+        if 'impression' in session:
+            rq_save_impressions(g.esession.id, session.pop('impressions').values(), now, delay=False)


Typo. impression or impressions?

Use the main save_impressions function.

jace · 2018-05-08T09:30:02Z

hasjob/views/helper.py

+def rq_save_impressions(session_id, impressions, viewed_time, delay=True):
+    func = save_impressions.delay if delay else save_impressions
+    print func, session_id
+    func(session_id, impressions, viewed_time)


You don't need this function. Call save_impressions or save_impressions.delay directly from wherever you need it.

jace · 2018-05-08T11:53:40Z

hasjob/views/helper.py

+        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)


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

jace · 2018-05-08T11:54:49Z

hasjob/views/helper.py

+        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.
+            rq_save_impressions(g.esession.id, session.pop('impressions').values(), now, delay=False)


Use regular save_impressions.

jace · 2018-05-08T12:21:15Z

hasjob/views/helper.py

@@ -192,7 +150,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:


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

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

CLAassistant · 2022-05-16T15:15:46Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

Moving anon user tracking from image element to form submit

12fdd70

iambibhas added enhancement in progress labels Apr 3, 2018

iambibhas requested a review from jace April 3, 2018 20:13

iambibhas self-assigned this Apr 3, 2018

fixes in handling anon user and associating with actual user upon login

6dd80be

iambibhas added the needs-review Need comment from the reviewers label Apr 4, 2018

Bibhas added 2 commits April 5, 2018 02:04

added comment

8648385

moved eventsession setting to per request

72139e2

jace requested changes Apr 5, 2018

View reviewed changes

Bibhas added 2 commits April 6, 2018 01:46

renamed sniffle and moved to api views

426e753

removed check for g.esession

ce31a8a

iambibhas commented Apr 6, 2018

View reviewed changes

fixed conflict

902e2dc

iambibhas removed the needs-review Need comment from the reviewers label Apr 10, 2018

Bibhas added 2 commits April 11, 2018 02:52

deleting session cookie if user is logged in

80ba99d

fetching g.esession from cache after anon user is saved in db

424e5cd

jace reviewed Apr 15, 2018

View reviewed changes

Bibhas added 3 commits April 18, 2018 13:13

fixed conflict

8c41e6c

session['au'] can only be integer now, so converting to string

507e6b3

removed anon macro import from templates where it's not needed anymore

24380f4

jace requested changes Apr 27, 2018

View reviewed changes

Bibhas added 2 commits May 7, 2018 14:46

Loading session details from cache when creating anon user

6f88b9d

saving and loading session from cache

b9234e9

jace requested changes May 8, 2018

View reviewed changes

Bibhas added 2 commits May 14, 2018 18:12

Fixed the eventsession flow

3c1fa76

No longer checking for dirty session before committing

6f34e5d

		@@ -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>

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

Are you sure you want to change the base?

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

Conversation

iambibhas commented Apr 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jace commented Apr 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CLAassistant commented May 16, 2022