17 add timezone map #77

seanson · 2017-08-26T13:34:18Z

Relevant Issue:

Implements the changes requested in #17

Description of Changes

This commit adds two features:
- An enhancement to the existing celery task for gathering user information. We save a large chunk of the most common time zones for display purposes. We store a JSON record in the database so we can do historical tracking of timezone membership.
- A new partial template for the Slack module. This includes some Jinja2 html templating for the Leaflet map and a JS script that incldues timezone -> coordinate mapping and the Leaflet map builder.
This mapping is mostly generated from the public domain tzdata's "zone1970.tab" file, converted from iso6709 format and and dropped as a static mapping inside the javascript.

Screenshots, if applicable

Requirements

Tests
Documentation

seanson · 2017-08-26T13:47:43Z

If anyone would like to test this locally, the user capture script can be invoked manually with ./manage.py shell -c 'from pyslackers_website.slack.tasks import capture_snapshot_of_user_count; capture_snapshot_of_user_count.apply()'

mattrasband · 2017-08-26T14:14:49Z

docker-compose.yml

-  #   image:
-  #     context: .
-  #   command: celery -A config beat -l debug -S django
+  scheduler:


This shouldn't be necessary when using the devserver command, that command auto starts celery beat+worker when running locally

Ooh, nice. I'll drop this from the compose.

mattrasband · 2017-08-26T14:15:27Z

docker-compose.yml

-  #   image:
-  #     context: .
-  #   command: celery -A config worker -l debug
+  workers:


Same, this shouldn't be necessary here. The intend of the default compose file is just for run-time services locally

mattrasband · 2017-08-26T14:21:15Z

pyslackers_website/core/static/js/member_map.js

@@ -0,0 +1,401 @@
+const timezone_mapping = {


Since this is just vanilla JS+jquery, it's best to wrap this in an IIFE, or jquery is available put it on the document on ready handler

I need to do this for the small other bits of js...

mattrasband · 2017-08-26T14:31:11Z

pyslackers_website/slack/models.py

@@ -7,3 +7,4 @@ class Membership(models.Model):
    deleted_count = models.IntegerField()
    bot_count = models.IntegerField()
    timestamp = models.DateTimeField(auto_now_add=True)
+    tz_count_json = models.TextField(default="")


If the rationale of grabbing all this is some tracking and such, should we use the postgres json type so we can query data without loading it all into application memory ?

I haven't used the postgres JSON type in django before, but I see the edit to the migration but not this model - I believe that is required for it to act as we want

mattrasband · 2017-08-26T18:24:07Z

pyslackers_website/core/static/js/member_map.js

+        },
+        "type": "Feature",
+        "properties": {
+            "popupContent": `<h3>${timezone}: ${member_count} members</h3>`,


The key naming is inconsistent, is this required for the map or can we use snake_case?

Camelcase is required for the map, I'll switch the other counts to camel case for consistency.

mattrasband · 2017-08-26T18:34:31Z

pyslackers_website/slack/tasks.py

+                time_zones.append(tz)
+
+    counter = Counter(time_zones)
+    tz_count_json = dumps(dict(counter))


Nice usage of counter, however I would prefer to not manually dump the json and let the orm handle it. https://docs.djangoproject.com/en/1.11/ref/contrib/postgres/fields/#jsonfield

Secondly, from what I recall with the slack API and the Django cache - it should work with the object you're caching since it's a non-complex dict

(continued, since reviews on mobile are : 💩)

For item 2, any reason to manually dump vs allowing the cache abstraction to deal with it? One favorable reason is json vs pickle, but the context and cache accept Python types so I favor that

mattrasband · 2017-08-26T18:42:50Z

pyslackers_website/slack/templates/slack/partials/member_map.html

@@ -0,0 +1,14 @@
+{% load static %}
+<script src="https://cdnjs.cloudflare.com/ajax/libs/leaflet/1.2.0/leaflet.js"></script>


There should be extension points in the base template to put these in the right spot (on mobile, so double check, but should be extra_script I think).

mattrasband · 2017-08-26T18:44:38Z

pyslackers_website/slack/templates/slack/partials/member_map.html

+{% load static %}
+<script src="https://cdnjs.cloudflare.com/ajax/libs/leaflet/1.2.0/leaflet.js"></script>
+<!-- leaflet requires CSS for map applied before init --> 
+<link rel="stylesheet" href="{% static 'css/member_map.css' %}" />


Also they should be an extra_links template block

mattrasband · 2017-08-26T18:45:16Z

pyslackers_website/slack/templates/slack/partials/member_map.html

+<link rel="stylesheet" href="{% static 'css/member_map.css' %}" />
+
+<div class="ui container">
+  <div id='leaflet_member_map' />


Quoting is inconsistent with the file, please make these double quotes like the rest

mattrasband · 2017-08-26T18:46:34Z

pyslackers_website/static/css/member_map.css

@@ -0,0 +1,6 @@
+#leaflet_member_map {


This won't be very responsive, can the available classes from semantic work for this so it responds and collapses?

mattrasband · 2017-08-26T18:51:07Z

I'm out of town until tomorrow but will test it out when I'm back home.

seanson · 2017-08-28T01:29:01Z

Changes for all requests pushed!

mattrasband · 2017-08-28T01:30:32Z

pyslackers_website/core/static/js/member_map.js

+
+};
+
+console.log(slack_member_tz_count);


Debug statement left around

mattrasband · 2017-08-28T01:31:33Z

CONTRIBUTING.md

@@ -52,6 +52,10 @@ Take note of the `client_id` and `client_secret` for each, and register them wit
 (.venv) $ ./manage.py createoauth2app --client-id $CLIENT_ID --client-secret $CLIENT_SECRET --provider {google,twitter}
 ``` 

+### Docker Environment Backend Services
+
+The default backend services can be run with `docker-compose up`, which will both expose ports locally as well as be available for inter-dependant links through the default docker networking (Celery -> Redis, Celery -> Postgres, etc.).


I believe this became less-relevant since the docker-compose revert. Line#28 should cover it, though maybe this is indicating we need more detail there. In any case, I think this can be reverted

mattrasband · 2017-08-28T01:32:31Z

pyslackers_website/core/static/js/member_map.js

+console.log(slack_member_tz_count);
+
+
+$(window).on('orientationchange pageshow resize', function () {


Interesting, I'm guessing this is a leaflet nuance?

Yup, this is required for us to support the responsive UI containers.

mattrasband · 2017-08-28T01:33:48Z

pyslackers_website/core/static/js/member_map.js

+
+
+$(window).on('orientationchange pageshow resize', function () {
+    if (!map) buildMap();


Note to self, I'll add a js check to the build later to enforce some standards. You don't need to change this, but I intend to make braces always required

Added JS linting and an .eslintrc to support failing this behaviour. I've grabbed airbnb as a default nice style guide.

mattrasband · 2017-08-28T01:35:00Z

pyslackers_website/slack/models.py

@@ -7,3 +7,4 @@ class Membership(models.Model):
    deleted_count = models.IntegerField()
    bot_count = models.IntegerField()
    timestamp = models.DateTimeField(auto_now_add=True)
+    tz_count_json = models.TextField(default="")


I haven't used the postgres JSON type in django before, but I see the edit to the migration but not this model - I believe that is required for it to act as we want

mattrasband · 2017-08-28T16:59:54Z

pyslackers_website/core/static/js/member_map.js

+            },
+            "type": "Feature",
+            "properties": {
+                "popupContent": `<h3>${timezone}: ${memberCount} members</h3>`,


Using this does mean not supporting IE at all (which is fine in my mind), I think it's a simple enough template literal that all modern major browsers support it at least.

Switched it out for a string concat because it's the only usage.

mattrasband · 2017-08-28T17:00:45Z

pyslackers_website/core/static/js/member_map.js

+
+    var features = slack_member_tz_count.map(([timezone, memberCount]) => (
+        {
+            "geometry": {


You don't have to fix this, but I would after merge if you don't: you're mixing quotes back and forth throughout this file - generally I like to see them remain consistent.

Sorted and now enforceable through linting.

mattrasband · 2017-08-28T17:04:06Z

pyslackers_website/core/static/js/member_map.js

+        onEachFeature: (feature, layer) => (layer.bindPopup(feature.properties.popupContent)),
+
+        pointToLayer: function (feature, latlng) {
+            console.log(feature.member_count);


debug statement left around

mattrasband · 2017-08-28T17:14:51Z

pyslackers_website/slack/templates/slack/partials/member_map.html

+  </div>
+</div>
+<script type="text/javascript">
+  const slack_member_tz_count = {{ slack_member_tz_count | safe }};


I get a bit nervous depending on a global variable to exist for a script to be able to run properly - but I don't have any great ideas other than making it an arg to an IIFE (As commented above).

I do need to validate what happens when a partial does these tags and the main template this is included into defines its own, I didn't think about that prior to recommending it. In jinja you have the super() call, django I don't think does.

The code itself won't get included if we don't have this variable defined, does that fit the required constraint?

mattrasband · 2017-08-28T17:25:13Z

pyslackers_website/slack/views.py

@@ -20,8 +21,10 @@ class SlackInvite(FormView):

    def get_context_data(self, **kwargs):
        context = super().get_context_data(**kwargs)
+        slack_member_tz_count = dumps(cache.get('slack_member_tz_count', []))


I believe that the controller/view should be dumb and not know how this data intends to be used (I think that is a display concern strictly). I think it would be best to make a new template tag that converts a python object to JSON for the view.

Along those lines, new additions require tests - if you can add something about context I'd like to see tests that operate like the blog/tests.py for just this new context item (I can back fill the other context I already have here).

Cool, I was hesitant to add a new template tag for this but it's doable.

I'll check out the tests as well.

This commit adds the following features: - An enhancement to the existing celery task for gathering user information. We save a large chunk of the most common time zones for display purposes. We store a JSON record in the database so we can do historical tracking of timezone membership. - A new partial template for the Slack module. This includes some Jinja2 html templating for the Leaflet map and a JS script that incldues timezone -> coordinate mapping and the Leaflet map builder. - A new tojson tag for converting objects to json string formatting. - A new package.json and associated eslint / jsbeautifyrc for managing future javascript implementation linting. This mapping is mostly generated from the public domain tzdata's "zone1970.tab" file, converted from iso6709 format and and dropped as a static mapping inside the javascript. Fixes: Specify addon version for postgresql to 9.6 to support JSON and match our Ansible playbook version

seanson requested a review from mattrasband August 26, 2017 13:34

mattrasband suggested changes Aug 26, 2017

View reviewed changes

seanson force-pushed the 17_add_timezone_map branch from f20bee9 to 58b241b Compare August 28, 2017 01:26

mattrasband suggested changes Aug 28, 2017

View reviewed changes

seanson force-pushed the 17_add_timezone_map branch from 91369e6 to 1141f00 Compare August 29, 2017 03:59

seanson force-pushed the 17_add_timezone_map branch from 1141f00 to 7320e76 Compare August 29, 2017 04:01

mattrasband approved these changes Aug 29, 2017

View reviewed changes

mattrasband merged commit a24ab1c into master Aug 29, 2017

mattrasband deleted the 17_add_timezone_map branch August 29, 2017 04:20

mattrasband mentioned this pull request Aug 29, 2017

Capture user timezones and create a visualization of user locations #17

Closed

seanson mentioned this pull request Aug 29, 2017

Accept ESLint style and enable Javascript ESLint in Travis builds #78

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

17 add timezone map #77

17 add timezone map #77

seanson commented Aug 26, 2017 •

edited

Loading

seanson commented Aug 26, 2017

mattrasband Aug 26, 2017

seanson Aug 27, 2017

mattrasband Aug 26, 2017

mattrasband Aug 26, 2017

mattrasband Aug 26, 2017

mattrasband Aug 26, 2017

mattrasband Aug 28, 2017

mattrasband Aug 26, 2017

seanson Aug 27, 2017

mattrasband Aug 26, 2017

mattrasband Aug 26, 2017

mattrasband Aug 26, 2017

mattrasband Aug 26, 2017

mattrasband Aug 26, 2017

mattrasband Aug 26, 2017

mattrasband commented Aug 26, 2017

seanson commented Aug 28, 2017

mattrasband Aug 28, 2017

mattrasband Aug 28, 2017

mattrasband Aug 28, 2017

seanson Aug 29, 2017

mattrasband Aug 28, 2017

seanson Aug 29, 2017

mattrasband Aug 28, 2017

mattrasband Aug 28, 2017

seanson Aug 29, 2017

mattrasband Aug 28, 2017

seanson Aug 29, 2017

mattrasband Aug 28, 2017

mattrasband Aug 28, 2017

seanson Aug 29, 2017

mattrasband Aug 28, 2017

seanson Aug 29, 2017

		@@ -0,0 +1,14 @@
		{% load static %}
		<script src="https://cdnjs.cloudflare.com/ajax/libs/leaflet/1.2.0/leaflet.js"></script>

		console.log(slack_member_tz_count);


		$(window).on('orientationchange pageshow resize', function () {



		$(window).on('orientationchange pageshow resize', function () {
		if (!map) buildMap();

17 add timezone map #77

17 add timezone map #77

Conversation

seanson commented Aug 26, 2017 • edited Loading

Relevant Issue:

Description of Changes

Screenshots, if applicable

Requirements

seanson commented Aug 26, 2017

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

mattrasband commented Aug 26, 2017

seanson commented Aug 28, 2017

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

seanson commented Aug 26, 2017 •

edited

Loading