UHC Visits count and last visited date contact default sort #4770

dianabarsan · 2018-08-06T17:24:01Z

Description

Adds visit count display in contacts list. Visit count display is conditioned by the same permission as displaying last visited date. Visits are counted based on the calendar month, with each month starting on either the 1st or on a configurable date. Visit counters may be color coded if a specific visit count goal configured.

Adds configuration option to enable contacts list to be sorted by last visited date by default.

Adds uhc configuration with options to enable/disable default sorting contacts lists by last visited date, visit counter settings for monthly count goal and the month reset/start date.

#4758
#4752

Review checklist

Readable: Concise, well named, follows the style guide, documented if necessary.
Documented: Announced in Changes.md in plain English. Configuration and user documentation on medic-docs
Tested: Unit and/or e2e where appropriate
Internationalised: All user facing text
Backwards compatible: Works with existing data and configuration or includes a migration. Any breaking changes documented in Changes.md.

Change integer filter from max digits to max number.

dianabarsan · 2018-08-06T18:37:02Z

Please review, @SCdF . Thanks!

SCdF

Nice!

Some changes, specifically I don't think you need to reimplement Math.max (understandable, I presume you tried to use it in the row list item but found out LiveList's terrible secret!)

Really happy to see we don't need to emit ~30 times for every document!

SCdF · 2018-08-14T12:48:44Z

static/css/variables.less

@@ -35,6 +35,10 @@
 @failed-state-color: #DA4548;
 @received-state-color: #E9AA22;

+/* Visit count progress colors */
+@visits-started-color: #C38813;


"started" doesn't make much sense to me, given that it's used in context of a warning. I think either the warning flag should change or this var name should.

Maybe visit-goal-started / visit-goal-incomplete? (and visit-goal-done to match).

SCdF · 2018-08-14T12:56:59Z

api/config.default.json

+  ],
+  "uhc": {
+    "contacts_default_sort": {
+      "last_visited_date": false


for later migratory reasons, unless there is a strong reason for this, IMO this should be "contacts_defaults_sort": "last_visited_date" and then we just inject that directly in later on (instead of checking the boolean).

The reason I structured it like this is so it's self explanatory and people who configure don't need to use exact word matches to get the configuration to work.

I switched it to the way you indicated but did not set the default config to use "last_visited_date" as default sort order, so this feature will need to be documented.
Also, I changed the sort string from camelCase to _underscore so configuration values are consistent ("lastVisitedDate" -> "last_visited_date").

SCdF · 2018-08-14T12:57:46Z

static/js/controllers/contacts.js

+        if ($scope.lastVisitedDateExtras &&
+            uhcSettings.contacts_default_sort &&
+            uhcSettings.contacts_default_sort.last_visited_date) {
+          $scope.sortDirection = $scope.defaultSortDirection = 'lastVisitedDate';


.i.e. here we would say $scope.sortDirection = $scope.defaultSortDirection = uhcSettings.contacts_default_sort

SCdF · 2018-08-14T12:59:04Z

static/js/controllers/contacts.js

+            doc.fields.visited_contact_uuid &&
+            liveList.contains({ _id: doc.fields.visited_contact_uuid })
+        ) {
+          return true;


Maybe just return the boolean expression?

SCdF · 2018-08-14T13:15:05Z

static/js/filters/number.js

@@ -0,0 +1,18 @@
+angular.module('inboxFilters').filter('maxInteger', function() {


Why do we need this filter? AFAICT it's only used in JS code that could just use Math.max

SCdF · 2018-08-14T13:17:26Z

static/js/services/live-list.js

@@ -82,6 +83,23 @@ angular.module('inboxServices').factory('LiveListConfig',
                scope.overdue = contact.lastVisitedDate <= oneMonthAgo;
                scope.summary = $translate.instant('contact.last.visited.date', { date: relativeDayFilter(contact.lastVisitedDate, true) });
              }
+
+              scope.visits = {
+                count: $translate.instant('contacts.visits.count', { count: maxIntegerFilter(contact.visitCount, 99) }),


replace with Math.max?

SCdF · 2018-08-14T13:23:30Z

static/js/services/live-list.js

+                count: $translate.instant('contacts.visits.count', { count: maxIntegerFilter(contact.visitCount, 99) }),
+                summary: contact.visitCount === 1 ?
+                  $translate.instant('contacts.visits.count.visit') :
+                  $translate.instant('contacts.visits.count.visits'),


I haven't looked at what capabilities angular translate has, but a lot of them support plurals in the actual translation itself. This is useful as certain languages do plurals differently (I have no idea about the ones we support, but there are languages with no plurals, or 3-4 different states, etc)

This is correct! Thanks for pointing it out!

SCdF · 2018-08-14T13:25:08Z

static/js/services/search.js

+            var stats = visitStats[row.key] || { lastVisitedDate: -1, visitCount: 0 };
+
+            if (stats.lastVisitedDate < row.value) {
+              stats.lastVisitedDate = row.value;


IDK if you find this easier to read, but this can be just Math.min

SCdF · 2018-08-14T13:27:29Z

static/js/services/search.js

+              var lastVisitedDatePromise = getLastVisitedDates(searchResults, extensions.visitCountSettings);
+
+              result = $q
+                .all([ dataRecordsPromise, lastVisitedDatePromise ])


any reason for refactoring out passing a map to $q.all? IMO it's more readable, because you're not relying on an array matching up, and you get to actually name things.

I refactored it because the Q we used in Karma doesn't support mapping and I wanted to make the code testable. Promise.all doesn't support mapping either.

SCdF · 2018-08-14T13:30:47Z

tests/karma/unit/services/search.js

@@ -96,4 +98,177 @@ describe('Search service', function() {

  });

+  describe('dateLastVisited', () => {
+    beforeEach(() => {
+      clock = sinon.useFakeTimers(moment('2018-08-20 18:18:18').valueOf());


cool, I did not know about this!

dianabarsan · 2018-08-15T09:33:15Z

Please take another look, @SCdF . Thanks!

SCdF

LGTM!

Adds visit count display in contacts list. Visit count display is conditioned by the same permission as displaying last visited date. Visits are counted based on the calendar month, with each month starting on either the 1st or on a configurable date. Visit counters may be color coded if a specific visit count goal configured. Adds UHC app configuration section. Adds UHC configuration with options to select default sorting contacts lists by last visited date, visit counter settings for monthly count goal and the month reset/start date. #4758 #4752

dianabarsan added 12 commits August 3, 2018 18:01

Bump version for 2.18.0

94c251b

Just because it's Friday.

e9c3214

From target -> settings + colors & paddings.

be23d0a

Bump version for 2.18.0

f2dfc6b

Formatting.

2299ffe

Better interval calculation.

a962399

Rename stuff.

932993c

Refresh contacts list when visit report change is received.

318c848

Change integer filter from max digits to max number.

Default sort direction.

93e0277

Better stubs.

690154c

Internationalize visits count.

a866ddc

Minor refactors.

f978d39

dianabarsan requested a review from SCdF August 6, 2018 18:36

dianabarsan requested review from SCdF and removed request for SCdF August 13, 2018 06:45

SCdF suggested changes Aug 14, 2018

View reviewed changes

dianabarsan added 4 commits August 14, 2018 19:33

Code review feedback.

dcf299e

Adds valid sorting directions + unit test.

f3609ec

Support objects in Q.all

377b443

Switch to _underscore_ format from camelCase for config consistency

7a6c86c

dianabarsan requested a review from SCdF August 15, 2018 09:30

SCdF approved these changes Aug 15, 2018

View reviewed changes

dianabarsan merged commit 0600396 into 2.18.x Aug 15, 2018

dianabarsan deleted the 4758-visits-counter branch August 15, 2018 10:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UHC Visits count and last visited date contact default sort #4770

UHC Visits count and last visited date contact default sort #4770

dianabarsan commented Aug 6, 2018

dianabarsan commented Aug 6, 2018

SCdF left a comment

SCdF Aug 14, 2018

dianabarsan Aug 14, 2018

SCdF Aug 14, 2018

dianabarsan Aug 14, 2018

dianabarsan Aug 15, 2018 •

edited

SCdF Aug 14, 2018

SCdF Aug 14, 2018

SCdF Aug 14, 2018

SCdF Aug 14, 2018

SCdF Aug 14, 2018

dianabarsan Aug 14, 2018

SCdF Aug 14, 2018

SCdF Aug 14, 2018

dianabarsan Aug 14, 2018

SCdF Aug 14, 2018

dianabarsan commented Aug 15, 2018

SCdF left a comment

		@@ -0,0 +1,18 @@
		angular.module('inboxFilters').filter('maxInteger', function() {

UHC Visits count and last visited date contact default sort #4770

UHC Visits count and last visited date contact default sort #4770

Conversation

dianabarsan commented Aug 6, 2018

Description

Review checklist

dianabarsan commented Aug 6, 2018

SCdF 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

dianabarsan Aug 15, 2018 • edited

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

dianabarsan commented Aug 15, 2018

SCdF left a comment

Choose a reason for hiding this comment

dianabarsan Aug 15, 2018 •

edited