Updated Tock iconography based on Reporting Period Audit #769

rogeruiz · 2018-03-28T15:07:20Z

Description

This PR focuses on updating the iconography of Tock (HappyTock and AngryTock) from a UX perspective. If folks aren't late, they should be greeted with HappyTock in any use of the Tock icon. For now this functionality only exists in the favicon and the notifications. If folks are late, i.e. it's Monday, then they should be greeted with AngryTock in any use of the Tock icon.

Additional information

This may require some new functionality not built into the backend for Tock, but it could also just cover updating the icon to use the HappyTock instead of AngryTock.

Anyone can take on this work. I'm just opening the PR for visibility. Feel free to commit onto this branch directly.

tbaxter-18f · 2018-03-28T16:06:45Z

My thinking is that the first step would be to add an is_late() method to UserData. When we have that, we can pretty easily toggle other things based on the results. Looking at the audit view, the trick will be making it efficient, or maybe making it a cached property if it's going to be a non-trivial lift.

rogeruiz · 2018-03-28T16:07:42Z

Keep in mind for the JS refactor here, notifications.js, it would be better for the JS to pull the favicon form the <link> tag and not have a hard-coded string in the source.

tbaxter-18f · 2018-03-28T16:10:05Z

Yeah, what I'm seeing is something like

{% if user.is_late %}
   <link href="angry_tock">
{% else %}
  <link href="happy_tock">
{% endif %}

codecov-io · 2018-03-28T22:06:16Z

Codecov Report

Merging #769 into master will increase coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #769      +/-   ##
==========================================
+ Coverage   90.89%   90.94%   +0.05%     
==========================================
  Files          38       38              
  Lines        1691     1701      +10     
==========================================
+ Hits         1537     1547      +10     
  Misses        154      154

Impacted Files	Coverage Δ
employees/models.py	`98.55% <0%> (+0.24%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb7f9fa...0b361eb. Read the comment docs.

tbaxter-18f · 2018-03-28T22:07:34Z

tock/employees/models.py

@@ -128,6 +131,27 @@ def organization_name(self):

        return ''

+    def is_late(self):


consider this just a stub. There are no tests yet or anything. Just a way of knowing from the user object if they are late, so we can in turn flex the UI for them.

hmm. Thinking about this functionality again, what do you think about modifying this lookup so that the default is happy tock unless the following condition is met.

It’s the first weekday after the latest reporting period less than the current date and the user hasn’t submitted.

I’m just thinking that it would be better if the default wasn’t that the user was late but rather that the user is on time. But idk.

I like it! Most people are on time. Makes perfect sense.

One other quick thing here - would it make sense to have this behave as a property? It's a small semantic thing, but seeing as how this ultimately just returns True or False based on checking a couple of conditions and is not modifying anything or accepting any arguments, the @property decorator for the method seems appropriate to me.

I have no real opinion either way. I don't think there's any real advantage to doing so, but I don't think there's any downside at all, either.

tbaxter-18f · 2018-04-03T14:34:28Z

tock/tock/static/js/components/notifications.js

+    var links = document.getElementsByTagName('link');
+    var paths = [].slice.call(links).map(function (e) {
+      if (e.getAttribute('rel') === 'shortcut icon') {
+        return e.getAttribute('href');


Instead of iterating through all links, will this work?

iconPath = document.querySelector("[rel='shortcut icon']").href;

You'd still want to be sure that iconPath was found

thanks for the suggestion. I submitted an updated patch.

rogeruiz · 2018-04-04T17:17:40Z

The issue with recycle-production here is because of #766

https://developer.mozilla.org/en-US/docs/Web/API/Notification/requestPermission This also allows for the Notification code to pull from the HTML for the "shortcut icon" link tag rather than have that hardcoded in a JS file.

@tbaxter-18f

h/t @tbaxter-18f

The `is_not_late` test is failing though 😞

rogeruiz · 2018-04-06T17:10:25Z

~~Tests are passing and~~ it's working locally. You can verify it locally by creating a timecard for last week and then reloading the page. AngryTock should rear his head. Once you've submitted it, you'll get HappyTock again.

rogeruiz · 2018-04-06T17:14:45Z

Not sure what's happening with the tests here. This was working fine on an individual test level, but seems to be failing when running the whole suite. Looking into it.

rogeruiz · 2018-04-06T17:19:07Z

awww. dang. looks like this requires a refactor of all the tests, or possibly just where this lookup is occurring, since ReportingPeriods are now being checked all the time in the context_processor. Ugh 😞

tbaxter-18f · 2018-04-06T17:31:36Z

tock/tock/templates/base.html

@@ -7,7 +7,11 @@
    <title>Tock</title>

    {% load staticfiles %}
-    <link rel="shortcut icon" href="{% static 'img/favicon.ico' %}">
+    {% if x_tock_user_is_late %}


I don't think you need a context processor for this. I think you can just do {% if request.user.userdata.is_late %}

request.user should always be available, and I think it's more clear to see the check than the cp-supplied variable,, which are always a little less transparent than one would like. I think the only advantage of a context processor would be if you were doing the check a lot to avoid extra DB lookups.

That said, I could see you wanting to add a "late" body class as a simple hook for some styling changes.

Have you taken a look at the failing tests? Looks like not having timecard or reporting periods for all the users in all the tests is causing the tests to fail. But I’m not 100% sure either. But I do like that approach of checking the request object considering that’s where the user data lives in the first place.

I haven't. But would the combo of skipping the CP and flipping the is_late check polarity clear it up?

A user may not have all of the reporting periods or timecards associated with them though, so to me that seems to be an issue with the check itself. Should we have a bit more error handling or checks in the code to look for the existence of the reporting period first before using it?

Then again, you're calling .filter() with them, not .get(), so I'm also not entirely sure why the tests are failing with a message saying hours.models.DoesNotExist: ReportingPeriod matching query does not exist. Shouldn't that just be returning an empty QuerySet?

ccostino

Looking good so far, @rogeruiz! I have some other questions and additional feedback off of what @tbaxter-18f has pointed out already.

ccostino · 2018-04-06T20:27:25Z

tock/employees/models.py

@@ -128,6 +131,27 @@ def organization_name(self):

        return ''

+    def is_late(self):


One other quick thing here - would it make sense to have this behave as a property? It's a small semantic thing, but seeing as how this ultimately just returns True or False based on checking a couple of conditions and is not modifying anything or accepting any arguments, the @property decorator for the method seems appropriate to me.

ccostino · 2018-04-06T20:30:02Z

tock/employees/models.py

+        if timecard_count is 0:
+            return True
+        else:
+            return False


This looks good - filter out all timecards that match the user, reporting period and if they've been submitted; if none are returned, we know they're late. 👍

We don't have this logic elsewhere in the code, do we? It seems familiar, perhaps in one of the views. I wonder if this could serve to replace it, or if perhaps it should be abstracted out as a utility method that both this model method and the logic in the view call?

We'll revisit this as a later time, there's no need to hold things up here with it! One potential spot: https://github.com/18F/tock/blob/master/tock/api/views.py#L139-L162

We may also want to extract this in the future to provide support for crafting manager methods that allow us to filter query sets specifically around whether or not someone is late.

ccostino · 2018-04-06T20:32:09Z

tock/tock/context_processors.py

+    except UserData.DoesNotExist:
+        response['x_tock_user_is_late'] = False
+
+    return response


I agree with @tbaxter-18f - there should be a way to get this information off of request.user or request.user.user_data directly, which would keep things simpler and obviate the need for an extra context processor. :-)

ccostino · 2018-04-06T20:37:49Z

tock/tock/templates/base.html

@@ -7,7 +7,11 @@
    <title>Tock</title>

    {% load staticfiles %}
-    <link rel="shortcut icon" href="{% static 'img/favicon.ico' %}">
+    {% if x_tock_user_is_late %}


A user may not have all of the reporting periods or timecards associated with them though, so to me that seems to be an issue with the check itself. Should we have a bit more error handling or checks in the code to look for the existence of the reporting period first before using it?

Then again, you're calling .filter() with them, not .get(), so I'm also not entirely sure why the tests are failing with a message saying hours.models.DoesNotExist: ReportingPeriod matching query does not exist. Shouldn't that just be returning an empty QuerySet?

@ccostino

h/t @ccostino

tbaxter-18f · 2018-04-09T14:28:23Z

tock/tock/templates/base.html

@@ -7,7 +7,7 @@
    <title>Tock</title>

    {% load staticfiles %}
-    {% if request.user.user_data.is_late %}
+    {% if request.user.is_authenticated and request.user.user_data.is_late %}


I think a comment here mentioning that both checks are required might be prudent.

rogeruiz · 2018-04-10T14:08:33Z

I'm not going to bother updating the notification.js code on this PR. We can tackle it later if the UX leads to any issues. But this should clear up any of the feedback we've heard about AngryTock being the default for Tock iconography. 👍

This is possibly helpful for future iterations on the Tock notification work which is comprised of JS rather than Python. We'd have to really refactor the Notification code to be aware of users when they're late to properly swap the icon. But for now, the icon will default to whatever is on the page when the user first loads it.

tbaxter-18f · 2018-04-10T15:59:52Z

tock/tock/templates/base.html

@@ -12,6 +12,8 @@
    {% else %}
    <link rel="shortcut icon" href="{% static 'img/favicon-happy.png' %}">
    {% endif %}
+    <meta name="tock-icon-happy" content="{% static 'img/favicon-happy' %}">
+    <meta name="tock-icon-angry" content="{% static 'img/favicon-angry' %}">


curious: why are these custom meta elements needed?

from 0b361eb

Add meta tags with icons;
This is possibly helpful for future iterations on the Tock notification
work which is comprised of JS rather than Python. We'd have to really
refactor the Notification code to be aware of users when they're late to
properly swap the icon. But for now, the icon will default to whatever
is on the page when the user first loads it.

My original intention was to have these get pulled in from the JS code and leveraged whenever the notification timer would run, but the current implementation of the notification code relies on time rather than actually checking if the user is late or not. So without building an API endpoint for that now, I'm leaving the <meta> tags here in case someone wants to implement it in the future, myself or whomever. For now it's useful for the the notification code to pull from the <link> tag as it's mostly accurate. But by the time the notification actually goes out, the user may or may not be late.

This PR though I think achieves my original intent with these changes. It's all to make Tock less aggressive about reminding folks to Tock their time. AngryTock is a nice tool when it's needed, but a majority of folks aren't late and it's a bit overly aggressive to only have AngryTock be the iconography for the site. Even if it is just used in the Favicon and Notifications for now.

The notification code is intended as a reminder for later in the day on Friday so folks don't forget to Tock their time on time. If anything, this would be a slight enhancement to the feature to also notify the user that their timecard is actually late come the following week.

I think we're at a good place to stop here, and a future PR can address this tweak in functionality and add the necessary enhancements. :-)

/cc @mgwalker since he put the original notifications in place and may be interested to see all of this work and discussion!

ccostino

Everything looks good to go for me at this point, @rogeruiz, thank you very much for this!

rogeruiz added enhancement help wanted tech:javascript tech:html theme:ux code.gov code.gov Tag labels Mar 28, 2018

tbaxter-18f reviewed Mar 28, 2018

View reviewed changes

rogeruiz force-pushed the clearer-tock-notifications branch from e6393a4 to 0914743 Compare April 3, 2018 13:35

tbaxter-18f reviewed Apr 3, 2018

View reviewed changes

rogeruiz and others added 6 commits April 6, 2018 10:47

Add new favicons

1253115

Stubbed out is_late check

c9d2038

Circular import issues

7a6758c

Refactor notification to work in Safari 11;

3115526

https://developer.mozilla.org/en-US/docs/Web/API/Notification/requestPermission This also allows for the Notification code to pull from the HTML for the "shortcut icon" link tag rather than have that hardcoded in a JS file.

Clean up getting the iconPath;

8524bba

h/t @tbaxter-18f

Add tests;

0767d7f

The `is_not_late` test is failing though 😞

rogeruiz force-pushed the clearer-tock-notifications branch from 246c5ca to 0767d7f Compare April 6, 2018 14:47

rogeruiz added 2 commits April 6, 2018 13:06

Fix tests around Timecard submission

4f81419

Update favicon for Tock based on context

2676eee

rogeruiz changed the title ~~[wip] Updated Tock iconography based on Reporting Period Audit~~ Updated Tock iconography based on Reporting Period Audit Apr 6, 2018

rogeruiz requested a review from ccostino April 6, 2018 17:10

tbaxter-18f reviewed Apr 6, 2018

View reviewed changes

ccostino reviewed Apr 6, 2018

View reviewed changes

rogeruiz added 4 commits April 9, 2018 09:04

Make is_late a @Property;

6523243

h/t @ccostino

Remove the context processor & rely on the request

c282ea6

Check if user is logged in inside the is_late prop

3905a16

Check if the user is authenticated in the template

7d98c0e

tbaxter-18f reviewed Apr 9, 2018

View reviewed changes

rogeruiz added 2 commits April 9, 2018 11:47

Remove unused import

cbdfce9

Remove unused variable creation

28bb212

rogeruiz requested a review from ccostino April 10, 2018 14:08

rogeruiz added 2 commits April 10, 2018 11:48

Have clearer comment about time

9573436

tbaxter-18f reviewed Apr 10, 2018

View reviewed changes

rogeruiz mentioned this pull request Apr 10, 2018

Tock Notifications should be contextually aware #777

Closed

ccostino approved these changes Apr 13, 2018

View reviewed changes

ccostino merged commit 3760daf into master Apr 13, 2018

ccostino deleted the clearer-tock-notifications branch April 13, 2018 14:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated Tock iconography based on Reporting Period Audit #769

Updated Tock iconography based on Reporting Period Audit #769

rogeruiz commented Mar 28, 2018 •

edited

Loading

tbaxter-18f commented Mar 28, 2018 •

edited

Loading

rogeruiz commented Mar 28, 2018

tbaxter-18f commented Mar 28, 2018

codecov-io commented Mar 28, 2018 •

edited

Loading

tbaxter-18f Mar 28, 2018

rogeruiz Apr 6, 2018

tbaxter-18f Apr 6, 2018

ccostino Apr 6, 2018

tbaxter-18f Apr 6, 2018

tbaxter-18f Apr 3, 2018

rogeruiz Apr 3, 2018 •

edited

Loading

rogeruiz commented Apr 4, 2018

rogeruiz commented Apr 6, 2018 •

edited

Loading

rogeruiz commented Apr 6, 2018

rogeruiz commented Apr 6, 2018

tbaxter-18f Apr 6, 2018 •

edited

Loading

rogeruiz Apr 6, 2018

tbaxter-18f Apr 6, 2018

ccostino Apr 6, 2018

ccostino left a comment

ccostino Apr 6, 2018

ccostino Apr 6, 2018

ccostino Apr 6, 2018

ccostino Apr 13, 2018 •

edited

Loading

ccostino Apr 13, 2018

ccostino Apr 6, 2018

ccostino Apr 6, 2018

tbaxter-18f Apr 9, 2018

rogeruiz commented Apr 10, 2018

tbaxter-18f Apr 10, 2018

rogeruiz Apr 10, 2018

ccostino Apr 13, 2018

ccostino left a comment

		@@ -128,6 +131,27 @@ def organization_name(self):

		return ''

		def is_late(self):

Updated Tock iconography based on Reporting Period Audit #769

Updated Tock iconography based on Reporting Period Audit #769

Conversation

rogeruiz commented Mar 28, 2018 • edited Loading

Description

Additional information

tbaxter-18f commented Mar 28, 2018 • edited Loading

rogeruiz commented Mar 28, 2018

tbaxter-18f commented Mar 28, 2018

codecov-io commented Mar 28, 2018 • edited Loading

Codecov Report

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

rogeruiz Apr 3, 2018 • edited Loading

Choose a reason for hiding this comment

rogeruiz commented Apr 4, 2018

rogeruiz commented Apr 6, 2018 • edited Loading

rogeruiz commented Apr 6, 2018

rogeruiz commented Apr 6, 2018

tbaxter-18f Apr 6, 2018 • edited Loading

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

ccostino 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

ccostino Apr 13, 2018 • edited Loading

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

rogeruiz commented Apr 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccostino left a comment

Choose a reason for hiding this comment

rogeruiz commented Mar 28, 2018 •

edited

Loading

tbaxter-18f commented Mar 28, 2018 •

edited

Loading

codecov-io commented Mar 28, 2018 •

edited

Loading

rogeruiz Apr 3, 2018 •

edited

Loading

rogeruiz commented Apr 6, 2018 •

edited

Loading

tbaxter-18f Apr 6, 2018 •

edited

Loading

ccostino Apr 13, 2018 •

edited

Loading