Requirements Patches, Workflow Concurrency, Error Messaging, and Test Coverage (#346)

* Add acceptance, production to deployment workflow

* Install libcairo2-dev in dev container

* Comment runtime and requirements

* Revert "Add acceptance, production to deployment workflow"

This reverts commit aadc44a.

* Apply no-op requirement changes

* Update python-dateutil to 2.8.2

* Update simplejson to 3.17.6

* Update django-admin-rangefilter to 0.8.8

* Update gunicorn to 20.1.0

* Update to psycopg2-binary 2.8.6

* Fix note about py

* Update to python-decouple 3.4

* Only run initdb on an empty directory

* Add test_poll_state

* Fix formatting

* Refactor deployment workflow

* Add detailed error messages to views

* Refactor _poll_state_response

* Add err_msg parameter to error view

* Refactor poll_state

* Use valid PENDING state to updated percent

* Revert "Use valid PENDING state to updated percent"

This reverts commit 4c6df8c.

* Revert "Refactor poll_state"

This reverts commit e7287fd.

* Fix line length

* Revert "Install libcairo2-dev in dev container"

This reverts commit f183dcf.

* Reapply "Install libcairo2-dev in dev container"

This reverts commit 0fe85ea.

Author: aaron-lane

.devcontainer/Dockerfile

@@ -1,6 +1,6 @@
 FROM mcr.microsoft.com/devcontainers/python:3.9
 
 RUN apt-get update
-RUN	apt-get --yes install rabbitmq-server
+RUN	apt-get --yes install libcairo2-dev rabbitmq-server
 RUN	mkdir --parents /usr/local/var/postgres
 RUN chown vscode:vscode /usr/local/var/postgres

.github/workflows/deployment.yml

@@ -8,7 +8,45 @@ permissions:
   contents: read
 
 jobs:
-  commit:
+  commit-format:
+    runs-on: ubuntu-latest
+
+    steps:
+    - name: Checkout the Git repository
+      uses: actions/checkout@v3
+    - name: Set up Python 3.9
+      uses: actions/setup-python@v3
+      with:
+        python-version: "3.9"
+        cache: "pip"
+        cache-dependency-path: ./requirements-dev.txt
+    - name: Install dependencies
+      run: |
+        make env-python
+        make install-dev
+    - name: Format with autopep8
+      run: make format-check
+
+  commit-lint:
+    runs-on: ubuntu-latest
+
+    steps:
+    - name: Checkout the Git repository
+      uses: actions/checkout@v3
+    - name: Set up Python 3.9
+      uses: actions/setup-python@v3
+      with:
+        python-version: "3.9"
+        cache: "pip"
+        cache-dependency-path: ./requirements-dev.txt
+    - name: Install dependencies
+      run: |
+        make env-python
+        make install-dev
+    - name: Lint with ruff
+      run: make lint-check
+
+  commit-test:
     runs-on: ubuntu-latest
 
     env:
@@ -58,13 +96,9 @@ jobs:
           ./requirements.txt
     - name: Install dependencies
       run: |
+        make env-python
         make install
-    - name: Format with autopep8
-      run: |
-        make format-check
-    - name: Lint with ruff
-      run: |
-        make lint-check
+        make install-dev
     - name: Test with unittest
       run: |
         make celery &

Makefile

@@ -18,14 +18,15 @@ heroku:
 
 .PHONY: install
 install:
-	python3 -m venv venv
-	pip install -U pip
 	pip install -r requirements.txt
-	pip install -r requirements-dev.txt
 	make migrate
 	make groups
 	make static
 
+.PHONY: install-dev
+install-dev:
+	pip install -r requirements-dev.txt
+
 .PHONY: static
 static:
 	python3 manage.py collectstatic -c --no-input
@@ -45,6 +46,11 @@ else
 endif
 	@echo "RabbitMQ Status: Online"
 
+.PHONY: env-python
+env-python:
+	python3 -m venv venv
+	pip install -U pip
+
 .PHONY: stopenv
 stopenv:
 ifeq ($(USER),vscode)
@@ -75,12 +81,14 @@ groups:
 
 .PHONY: codespace
 codespace:
-	initdb /usr/local/var/postgres
+	find /usr/local/var/postgres -maxdepth 0 -empty -exec initdb {} \;
 	sudo cp ./rabbitmq-devcontainer.conf /etc/rabbitmq/rabbitmq.conf
 	make .env
 	make .git/hooks/pre-commit
 	make env
+	make env-python
 	make install
+	make install-dev
 	nohup bash -c 'make celery &'
 
 .PHONY: test

app/admin.py

@@ -4,7 +4,7 @@
 from django.forms import Textarea
 from django.http import HttpRequest, HttpResponseRedirect
 from django.utils import timezone as tz
-from rangefilter.filter import DateRangeFilter
+from rangefilter.filters import DateRangeFilter
 
 from app.constants.str import (
     EMPTY_DONATION,

app/templates/app/PollState.html

@@ -64,7 +64,7 @@
   }
 
   function setErrorPath() {
-    window.location.href = "/error";
+    window.location.href = "/error?err_msg=Failed%20to%20process%20the%20file.";
   }
 
   function poll() {

app/views/test_views.py

@@ -10,6 +10,7 @@
     download_receipt,
     export_csv,
     import_csv,
+    poll_state,
 )
 
 
@@ -69,30 +70,61 @@ def test_export_csv_post(self) -> None:
         self.assertEqual(first=response.status_code, second=302)
 
     def test_download_receipt(self) -> None:
-        request = self.request_factory.post(path="")
-        request.user = self.user
-        request.queryset = {}
+        download_receipt_request = self.request_factory.post(path="")
+        download_receipt_request.user = self.user
+        download_receipt_request.queryset = {}
 
-        post_response = download_receipt(request=request)
+        download_receipt_response = download_receipt(
+            request=download_receipt_request)
 
-        response = self.client.get(path=post_response.url)
+        get_receipt_response = self.client.get(
+            path=download_receipt_response.url)
 
-        self.assertContains(response=response,
+        self.assertContains(response=get_receipt_response,
                             text="""<div class="progress">
                                 <div class="bar" role="progressbar"></div>
                                 </div>""",
                             status_code=200, html=True)
 
-        location = post_response.get(header="Location")
+        location = download_receipt_response.get(header="Location")
         parsed_url = urlparse(url=location)
         query = parse_qs(qs=parsed_url.query)
         task_id = query["job"]
-        request = self.request_factory.get(path="", data={"task_id": task_id})
-        request.user = self.user
+        poll_state_request = self.request_factory.post(
+            path="", data={"task_id": task_id})
+        poll_state_request.user = self.user
+
+        poll_state_response = poll_state(request=poll_state_request)
+        self.assertContains(
+            response=poll_state_response,
+            text="SUCCESS",
+            status_code=200,
+            html=True)
 
-        response = download_file(request=request)
-        content_type = response["Content-Type"]
+        download_file_request = self.request_factory.get(
+            path="", data={"task_id": task_id})
+        download_file_request.user = self.user
+
+        download_file_response = download_file(request=download_file_request)
+        content_type = download_file_response["Content-Type"]
 
         self.assertEqual(first=content_type, second="application/zip", msg=(
             "The content type of the receipt response was unexpected. "
             "Is Celery running with a results backend enabled?"))
+
+    def test_poll_state(self) -> None:
+        request = self.request_factory.post(
+            path="", data={"task_id": "test-task-id"})
+        request.user = self.user
+
+        response = poll_state(request=request)
+
+        print(response.content)
+        self.assertEqual(first=response.status_code, second=200)
+        self.assertJSONEqual(
+            raw=response.content,
+            expected_data={
+                "state": "PENDING",
+                "process_percent": 0,
+                "status": 200},
+        )

app/views/views.py

@@ -36,58 +36,58 @@ def get_analytics(request: HttpRequest):
     return render(request, "app/analytics.html", _context("Analytics"))
 
 
-def import_view_template(request, importer, filetype, required_permission):
-    """A importer view template
+@require_http_methods(["GET", "POST"])
+@login_required(login_url="/login")
+def import_csv(request: HttpRequest):
+    """A view to redirect after task queuing csv importer
     """
-    if not request.user.has_perm(required_permission):
-        return _error(request, PERMISSION_DENIED)
+    filetype = ".csv"
+
+    if not request.user.has_perm("app.can_import_historical"):
+        return _error(request=request, err_msg=PERMISSION_DENIED)
 
     res = HttpResponseRedirect("/")
 
     if request.method == "GET":
-        if "job" in request.GET:
-            res = _poll_state_response(request, "import_csv")
-    elif request.method == "POST":
+        res = _poll_state_response(request, "import_csv")
+    # POST is the only other valid method
+    else:
         uploaded_file = request.FILES.get("uploaded_file", None)
         if uploaded_file and uploaded_file.name.endswith(filetype):
             raw_file = uploaded_file.read()
             decoded_file = str(raw_file, 'utf-8-sig',
                                errors='ignore').splitlines()
-            job = importer.s(decoded_file).delay()
+            job = historical_data_importer.s(decoded_file).delay()
             res = HttpResponseRedirect(f"{reverse('import_csv')}?job={job.id}")
         else:
-            res = _error(request)
-    return res
-
+            res = _error(
+                request=request,
+                err_msg="Uploaded file {uploaded_file.name} is not a "
+                "{filetype} file.")
 
-@require_http_methods(["GET", "POST"])
-@login_required(login_url="/login")
-def import_csv(request: HttpRequest):
-    """A view to redirect after task queuing csv importer
-    """
-    return import_view_template(
-        request, historical_data_importer, ".csv", "app.can_import_historical")
+    return res
 
 
 @require_http_methods(["GET", "POST"])
 @login_required(login_url="/login")
 def export_csv(request: HttpRequest):
     """Queue CSV exporter then redirect to poll state"""
     if not request.user.has_perm('app.can_export_data'):
-        return _error(request, PERMISSION_DENIED)
+        return _error(request=request, err_msg=PERMISSION_DENIED)
 
     res = HttpResponseRedirect("/")
 
     if request.method == "GET":
-        if "job" in request.GET:
-            return _poll_state_response(request, "export_csv")
-    elif request.method == "POST":
+        res = _poll_state_response(request, "export_csv")
+    # POST is the only other valid method
+    else:
         export_name = request.POST.get("export_name", "export")
         queryset = request.queryset if hasattr(request, 'queryset') \
             else Item.objects.all()
         rows = serializers.serialize("json", queryset)
         job = exporter.s(export_name, rows, len(queryset)).delay()
         res = HttpResponseRedirect(f"{reverse('export_csv')}?job={job.id}")
+
     return res
 
 
@@ -98,14 +98,12 @@ def download_receipt(request: HttpRequest):
     Takes request from admin which contains request.queryset
     """
     if not request.user.has_perm('app.generate_tax_receipt'):
-        return _error(request, PERMISSION_DENIED)
-
-    res = _error(request)
+        return _error(request=request, err_msg=PERMISSION_DENIED)
 
     if request.method == "GET":
-        if "job" in request.GET:
-            res = _poll_state_response(request, "download_receipt")
-    elif request.method == "POST":
+        res = _poll_state_response(request, "download_receipt")
+    # POST is the only other valid method
+    else:
         queryset = serializers.serialize("json", request.queryset)
         job = receiptor.s(queryset, len(request.queryset)).delay()
         res = HttpResponseRedirect(
@@ -119,7 +117,9 @@ def poll_state(request: HttpRequest):
     """A view to report the progress to the user"""
     task_id = request.POST.get("task_id", None)
     if task_id is None:
-        return _error(request)
+        return _error(
+            request=request,
+            err_msg="The task_id query parameter of the request was omitted.")
 
     task = AsyncResult(task_id)
     res = JsonResponse(_poll_state(PENDING, 0, 200))
@@ -153,15 +153,19 @@ def download_file(request: HttpRequest):
             except TimeoutError:
                 print(f"{task} {task_name} failed #{attempts}")
                 if (attempts >= ATTEMPT_LIMIT):
-                    return _error(request, "Download exceeded max attempts")
+                    return _error(
+                        request=request,
+                        err_msg="Download exceeded max attempts")
         return result
     except Exception as e:
-        return _error(request, "Something went wrong.", e)
+        return _error(request=request, err_msg=f"Failed to download file: {e}")
 
 
-def error(request):
+def error(request: HttpRequest):
     """Error page"""
-    return _error(request)
+    err_msg = request.GET.get("err_msg", "Something went wrong.")
+
+    return _error(request=request, err_msg=err_msg)
 
 
 """
@@ -170,10 +174,17 @@ def error(request):
 
 
 def _poll_state_response(request: HttpRequest, task_name):
+    job = request.GET.get("job", None)
+    if job is None:
+        return _error(
+            request=request,
+            err_msg="The job query parameter of the request was omitted.")
+
     context = _context("Poll State", {
-        "task_id": request.GET["job"],
+        "task_id": job,
         "task_name": task_name
     })
+
     return render(request, "app/PollState.html", context)
 
 
@@ -186,8 +197,7 @@ def _context(title, override={}):
     return context
 
 
-def _error(request: HttpRequest, err_msg="Something went wrong.", e=None):
-    # logger.exception(e)
+def _error(request: HttpRequest, err_msg="Something went wrong."):
     return render(request, "app/error.html", _context(err_msg))