Merge pull request #839 from ministryofjustice/fix-datetime-compare

Fix datetime compare

controlpanel/frontend/jinja2/user-list.html

@@ -29,20 +29,17 @@ <h1 class="govuk-heading-xl">{{ page_title }}</h1>
       </td>
       <td class="govuk-table__cell">{{ user.email }}</td>
       <td class="govuk-table__cell">
-        {%- with login_details=last_login.get(user.auth0_id) -%}
-          {%- if login_details -%}
-          {%- with login = login_details.get("last_login") -%}
-          <span data-last-login="{{login}}"
-                title="{{login.strftime("%Y/%m/%d %H:%M:%S")}}">
-            {{ timesince(login) }} ago
-          </span>
-          {%- if login_details.get("unused") -%}
-          (<strong><a href="https://github.com/orgs/moj-analytical-services/people/{{ user.username }}" target="_blank">unused?</a></strong>)
-          {%- endif -%}
-          {%- endwith -%}
+          {%- if user.last_login -%}
+              <span data-last-login="{{user.last_login}}"
+                    title="{{user.last_login.strftime("%Y/%m/%d %H:%M:%S")}}">
+                {{ timesince(user.last_login) }} ago
+              </span>
           {%- else -%}
+              <span>Never logged in.</span>
+          {%- endif -%}
+          {%- if user in unused_users -%}
+              (<strong><a href="https://github.com/orgs/moj-analytical-services/people/{{ user.username }}" target="_blank">unused?</a></strong>)
           {%- endif -%}
-        {%- endwith -%}
       </td>
       <td class="govuk-table__cell">
         <a href="{{ url('manage-user', kwargs={ "pk": user.auth0_id }) }}"

controlpanel/frontend/views/user.py

@@ -19,7 +19,7 @@ def ninety_days_ago():
     past, from the current date. The assumption made here is that a month is
     roughly 30 days (so the result is always 90 days in the past).
     """
-    return datetime.now() - timedelta(days=90)
+    return datetime.now().date() - timedelta(days=90)
 
 
 class UserList(LoginRequiredMixin, PermissionRequiredMixin, ListView):
@@ -30,68 +30,15 @@ class UserList(LoginRequiredMixin, PermissionRequiredMixin, ListView):
     template_name = "user-list.html"
 
     def get_context_data(self, **kwargs):
-        # TODO: Refactor this. I've added annotations for future reference.
         context = super().get_context_data(**kwargs)
-
-        # This returns a list of dictionary objects from Auth0. Each dictionary
-        # represents a user.
-        users = auth0.ManagementAPI().list_users(
-            params={
-                'q': 'identities.connection:"github"'
-            },
-        )
-
-        # We also need the django.contrib.auth.User instances for each user
-        # too. The auth0_mapper dictionary stores a User instance for each user
-        # against their Auth0 ID (so we can connect them to the dictionary
-        # related record from Auth0 -- see above). These objects are used as a
-        # fallback for when we don't have a last_login for the user from Auth0.
-        auth0_mapper = {}
-        for db_user in self.queryset:
-            if db_user.auth0_id:
-                auth0_mapper[db_user.auth0_id] = db_user
-
-        # This dictionary will store details of last logins for those users for
-        # whom such data is available (apparently, not all users may have
-        # logged in). TODO: Investigate if this is actually the case. Surely
-        # all users must have logged in to set up their account and get
-        # registered via Auth0.
-        last_logins = {}
-        for user in users:
-            auth0_id = user['user_id']
-            # Apparently some Auth0 users won't have a "user_id". TODO: WAT?
-            # Check if this is the case.
-            if auth0_id == '':
-                continue
-            # This next block is a confusing mess. TODO: Investigate is we
-            # can't just use Django's User model's last_login field.
-            last_login = None
-            try:
-                # First check the Auth0 based record / dictionary.
-                auth0_last_login = user.get("last_login")
-                if auth0_last_login:
-                    last_login = dateutil.parser.parse(user.get('last_login'))
-                else: 
-                    # Fall back to Django's User model's last_login field.
-                    db_user = auth0_mapper.get(auth0_id, None)
-                    if db_user:
-                        last_login = db_user.last_login
-            except (TypeError, ValueError):
-                last_login = None
-            # Only add a last_login record to the context if there's something
-            # to add.
-            if last_login:
-                last_logins[auth0_id] = {
-                    # The datetime of last login to display in the template.
-                    "last_login": last_login,
-                    # Not logged in for 90 days? Perhaps the account is unused
-                    # so set this flag to indicate the account should be
-                    # checked by a human for legitimate inactivity and thus
-                    # removal from the system.
-                    "unused": last_login.utcnow() < ninety_days_ago(),
-                }
-
-        context['last_login'] = last_logins
+        unused_users = []
+        for user in self.queryset:
+            if user.last_login:
+                if user.last_login.date() < ninety_days_ago():
+                    unused_users.append(user)
+            else:
+                unused_users.append(user)
+        context['unused_users'] = unused_users
         return context
 
 
@@ -113,8 +60,13 @@ class UserDetail(LoginRequiredMixin, PermissionRequiredMixin, DetailView):
     def get_context_data(self, **kwargs):
         context = super().get_context_data(**kwargs)
         # A flag to indicate if the user hasn't logged in for 90 days - so the
-        # account maybe unused and so a candidate for removal.
-        context["unused"] = self.object.last_login.utcnow() < ninety_days_ago()
+        # account maybe unused and thus a candidate for removal.
+        context["unused"] = None
+        if self.object.last_login:
+            context["unused"] = self.object.last_login.date() < ninety_days_ago()
+        else:
+            # No last login? They've never logged into the site.
+            context["unused"] = True
         return context