From 917a36f4f3934ffaa01864aeeafaa85b52662d2e Mon Sep 17 00:00:00 2001
From: Andreas Kloeckner <inform@tiker.net>
Date: Tue, 15 Sep 2015 10:33:03 -0500
Subject: [PATCH] Various celery task improvements

---
 course/flow.py                            |  2 +-
 course/grades.py                          |  6 ++--
 course/tasks.py                           | 40 +++++------------------
 course/templates/course/course-base.html  |  2 +-
 course/templates/course/task-monitor.html |  8 ++---
 course/views.py                           |  7 +++-
 doc/misc.rst                              |  6 ++++
 local_settings.py.example                 | 12 ++++---
 relate/settings.py                        | 40 +++++++++++++++--------
 relate/urls.py                            |  6 ++--
 10 files changed, 68 insertions(+), 61 deletions(-)

diff --git a/course/flow.py b/course/flow.py
index 8457e09c..f9e6ca8e 100644
--- a/course/flow.py
+++ b/course/flow.py
@@ -1579,7 +1579,7 @@ class RegradeFlowForm(StyledForm):
 
 
 @course_view
-def regrade_not_for_credit_flows_view(pctx):
+def regrade_flows_view(pctx):
     if pctx.role != participation_role.instructor:
         raise PermissionDenied(_("must be instructor to regrade flows"))
 
diff --git a/course/grades.py b/course/grades.py
index e77604bb..735e0994 100644
--- a/course/grades.py
+++ b/course/grades.py
@@ -427,7 +427,7 @@ def view_grades_by_opportunity(pctx, opp_id):
                 from course.tasks import (
                         expire_in_progress_sessions,
                         finish_in_progress_sessions,
-                        regrade_ended_sessions,
+                        regrade_flow_sessions,
                         recalculate_ended_sessions)
 
                 if op == "expire":
@@ -447,9 +447,9 @@ def view_grades_by_opportunity(pctx, opp_id):
                     return redirect("relate-monitor_task", async_res.id)
 
                 elif op == "regrade":
-                    async_res = regrade_ended_sessions.delay(
+                    async_res = regrade_flow_sessions.delay(
                             pctx.course.id, opportunity.flow_id,
-                            rule_tag)
+                            rule_tag, inprog_value=False)
 
                     return redirect("relate-monitor_task", async_res.id)
 
diff --git a/course/tasks.py b/course/tasks.py
index 5e0030f4..4f4dfe99 100644
--- a/course/tasks.py
+++ b/course/tasks.py
@@ -27,6 +27,7 @@ THE SOFTWARE.
 from celery import shared_task
 
 from django.db import transaction
+from django.utils.translation import ugettext as _
 
 from course.models import (Course, FlowSession)
 from course.content import get_course_repo
@@ -64,6 +65,8 @@ def expire_in_progress_sessions(self, course_id, flow_id, rule_tag, now_datetime
 
     repo.close()
 
+    return {"message": _("%d sessions expired.") % count}
+
 
 @shared_task(bind=True)
 @transaction.atomic
@@ -96,37 +99,7 @@ def finish_in_progress_sessions(self, course_id, flow_id, rule_tag, now_datetime
 
     repo.close()
 
-    return count
-
-
-@shared_task(bind=True)
-@transaction.atomic
-def regrade_ended_sessions(self, course_id, flow_id, rule_tag):
-    course = Course.objects.get(id=course_id)
-    repo = get_course_repo(course)
-
-    sessions = (FlowSession.objects
-            .filter(
-                course=course,
-                flow_id=flow_id,
-                participation__isnull=False,
-                access_rules_tag=rule_tag,
-                in_progress=False,
-                ))
-
-    nsessions = sessions.count()
-    count = 0
-
-    from course.flow import regrade_session
-    for session in sessions:
-        regrade_session(repo, course, session)
-        count += 1
-
-        self.update_state(
-                state='PROGRESS',
-                meta={'current': count, 'total': nsessions})
-
-    repo.close()
+    return {"message": _("%d sessions ended.") % count}
 
 
 @shared_task(bind=True)
@@ -158,6 +131,8 @@ def recalculate_ended_sessions(self, course_id, flow_id, rule_tag):
 
     repo.close()
 
+    return {"message": _("Grades recalculated for %d sessions.") % count}
+
 
 @shared_task(bind=True)
 @transaction.atomic
@@ -168,6 +143,7 @@ def regrade_flow_sessions(self, course_id, flow_id, access_rules_tag, inprog_val
     sessions = (FlowSession.objects
             .filter(
                 course=course,
+                participation__isnull=False,
                 flow_id=flow_id))
 
     if access_rules_tag is not None:
@@ -190,5 +166,7 @@ def regrade_flow_sessions(self, course_id, flow_id, access_rules_tag, inprog_val
 
     repo.close()
 
+    return {"message": _("%d sessions regraded.") % count}
+
 
 # vim: foldmethod=marker
diff --git a/course/templates/course/course-base.html b/course/templates/course/course-base.html
index 35611a35..d8fb1436 100644
--- a/course/templates/course/course-base.html
+++ b/course/templates/course/course-base.html
@@ -58,7 +58,7 @@
       {% if role == pr.instructor %}
         <li><a href="{% url "relate-import_grades" course.identifier %}">{% trans "Import Grades" %}</a></li>
       {% endif %}
-      <li><a href="{% url "relate-regrade_not_for_credit_flows_view" course.identifier %}">{% trans "Regrade not-for-credit flow" %}</a></li>
+      <li><a href="{% url "relate-regrade_flows_view" course.identifier %}">{% trans "Regrade flows" %}</a></li>
 
       {% if role == pr.instructor or role == pr.teaching_assistant %}
         <li role="presentation" class="divider"></li>
diff --git a/course/templates/course/task-monitor.html b/course/templates/course/task-monitor.html
index b8b3a33d..fa5abe9a 100644
--- a/course/templates/course/task-monitor.html
+++ b/course/templates/course/task-monitor.html
@@ -28,9 +28,9 @@
   {% if progress_percent != None %}
     <div class="progress">
       <div class="progress-bar" role="progressbar"
-        aria-valuenow="{{ progress_percentage }}" aria-valuemin="0" aria-valuemax="100"
-        style="width: {{ progress_percentage|stringformat:".9f" }}%;">
-        {{ progress_percentage|floatformat:0 }}%
+        aria-valuenow="{{ progress_percent }}" aria-valuemin="0" aria-valuemax="100"
+        style="width: {{ progress_percent|stringformat:".9f" }}%;">
+        {{ progress_percent|floatformat:0 }}%
       </div>
     </div>
   {% else %}
@@ -45,7 +45,7 @@
       {% endif %}"
       role="progressbar"
         aria-valuenow="100" aria-valuemin="0" aria-valuemax="100"
-        style="width: 100%;">
+        style="width: 100%; min-width:4em;">
       </div>
     </div>
   {% endif %}
diff --git a/course/views.py b/course/views.py
index 01fe765f..fc0a4a73 100644
--- a/course/views.py
+++ b/course/views.py
@@ -1038,12 +1038,17 @@ def monitor_task(request, task_id):
         current = meta["current"]
         total = meta["total"]
         if total > 0:
-            progress_percent = current / total
+            progress_percent = 100 * (current / total)
 
         progress_statement = (
                 _("%d out of %d items processed.")
                 % (current, total))
 
+    if async_res.state == "SUCCESS":
+        if (isinstance(async_res.result, dict)
+                and "message" in async_res.result):
+            progress_statement = async_res.result["message"]
+
     traceback = None
     if request.user.is_staff and async_res.state == "FAILURE":
         traceback = async_res.traceback
diff --git a/doc/misc.rst b/doc/misc.rst
index 7476c4d2..41b17bee 100644
--- a/doc/misc.rst
+++ b/doc/misc.rst
@@ -53,6 +53,12 @@ those long-running tasks. Start a worker by running::
 
     celery worker -A relate
 
+Note that, due to limitations of the demo configuration (i.e. due to not having
+out-of-process caches available), long-running tasks can only show
+"PENDING/STARTED/SUCCESS/FAILURE" as their progress, but no more detailed
+information. This will be better as soon as you provide actual caches (the "CACHES"
+option :file:`local_settings.py`).
+
 Additional setup steps for Docker
 ---------------------------------
 
diff --git a/local_settings.py.example b/local_settings.py.example
index 7ec6b911..b4611f9d 100644
--- a/local_settings.py.example
+++ b/local_settings.py.example
@@ -22,11 +22,15 @@ ALLOWED_HOSTS = [
 
 # Recommended, because dulwich is kind of slow in retrieving stuff.
 #
+# Also, progress bars for long-running operations will only work
+# properly if you enable this. (or a similar out-of-process cache
+# backend)
+#
 # CACHES = {
-#   'default': {
-#     'BACKEND': 'django.core.cache.backends.memcached.MemcachedCache',
-#     'LOCATION': '127.0.0.1:11211',
-#   }
+#     'default': {
+#         'BACKEND': 'django.core.cache.backends.memcached.MemcachedCache',
+#         'LOCATION': '127.0.0.1:11211',
+#     }
 # }
 
 # SECURITY WARNING: don't run with debug turned on in production!
diff --git a/relate/settings.py b/relate/settings.py
index bd06e97e..1c490a00 100644
--- a/relate/settings.py
+++ b/relate/settings.py
@@ -78,19 +78,6 @@ TEMPLATE_CONTEXT_PROCESSORS = (
         + RELATE_EXTRA_CONTEXT_PROCESSORS
         )
 
-# {{{ celery config
-
-BROKER_URL = 'django://'
-
-CELERY_ACCEPT_CONTENT = ['pickle']
-CELERY_TASK_SERIALIZER = 'pickle'
-CELERY_RESULT_SERIALIZER = 'pickle'
-CELERY_TRACK_STARTED = True
-
-CELERY_RESULT_BACKEND = 'djcelery.backends.database:DatabaseBackend'
-
-# }}}
-
 # {{{ bower packages
 
 BOWER_COMPONENTS_ROOT = os.path.join(BASE_DIR, "components")
@@ -175,6 +162,33 @@ for name, val in local_settings.items():
     if not name.startswith("_"):
         globals()[name] = val
 
+# {{{ celery config
+
+BROKER_URL = 'django://'
+
+CELERY_ACCEPT_CONTENT = ['pickle']
+CELERY_TASK_SERIALIZER = 'pickle'
+CELERY_RESULT_SERIALIZER = 'pickle'
+CELERY_TRACK_STARTED = True
+
+if "CELERY_RESULT_BACKEND" not in globals():
+    if ("CACHES" in globals()
+            and "LocMem" not in CACHES["default"]["BACKEND"]  # noqa
+            and "Dummy" not in CACHES["default"]["BACKEND"]  # noqa
+            ):
+        # If possible, we would like to use an external cache as a
+        # result backend--because then the progress bars work, because
+        # the writes realizing them arent't stuck inside of an ongoing
+        # transaction. But if we're using the in-memory cache, using
+        # cache as a results backend doesn't make much sense.
+
+        CELERY_RESULT_BACKEND = 'djcelery.backends.cache:CacheBackend'
+
+    else:
+        CELERY_RESULT_BACKEND = 'djcelery.backends.database:DatabaseBackend'
+
+# }}}
+
 TEMPLATES = [
     {
         "BACKEND": "django.template.backends.django.DjangoTemplates",
diff --git a/relate/urls.py b/relate/urls.py
index cd9cb87b..bbe00ad6 100644
--- a/relate/urls.py
+++ b/relate/urls.py
@@ -337,10 +337,10 @@ urlpatterns = [
 
     url(r"^course"
         "/" + COURSE_ID_REGEX +
-        "/regrade-not-for-credit-flows"
+        "/regrade-flows"
         "/$",
-        course.flow.regrade_not_for_credit_flows_view,
-        name="relate-regrade_not_for_credit_flows_view"),
+        course.flow.regrade_flows_view,
+        name="relate-regrade_flows_view"),
 
     url(r"^course"
         "/" + COURSE_ID_REGEX +
-- 
GitLab