api, ui: improve base view classes (bug 2039353)

- add BaseLandoViewMixin
- add mixin to base view classes
- use self.response where applicable
- replace usages of Http404 with helper (bug 2039353)

Author: zzzeid

src/lando/api/views.py

@@ -33,12 +33,13 @@
 )
 from lando.utils.landing_checks import LandingChecks
 from lando.utils.phabricator import PHABRICATOR_API_KEY_HEADER, get_phabricator_client
+from lando.utils.views import BaseLandoViewMixin
 
 
-class APIView(View):
+class APIView(View, BaseLandoViewMixin):
     """A base class for API views."""
 
-    pass
+    response_class = JsonResponse
 
 
 def phabricator_api_key_required(func: Callable) -> Callable:
@@ -47,14 +48,16 @@ def phabricator_api_key_required(func: Callable) -> Callable:
     @wraps(func)
     def _wrapper(self: View, request: WSGIRequest, *args, **kwargs) -> Callable:
         if PHABRICATOR_API_KEY_HEADER not in request.headers:
-            return JsonResponse(
+            return self.response(
                 {"error": f"{PHABRICATOR_API_KEY_HEADER} missing."}, status=400
             )
 
         api_key = request.headers[PHABRICATOR_API_KEY_HEADER]
         client = get_phabricator_client(api_key=api_key)
         if not client.verify_api_token():
-            return JsonResponse({"error": "Invalid Phabricator API token."}, status=401)
+            return self.response(
+                {"error": "Invalid Phabricator API token."}, status=401
+            )
 
         return func(self, request, *args, **kwargs)
 
@@ -93,7 +96,7 @@ def generate_warnings_and_blockers(
 
 
 @method_decorator(csrf_exempt, name="dispatch")
-class LegacyDiffWarningView(View):
+class LegacyDiffWarningView(APIView):
     """
     This class provides the API controllers for the legacy `DiffWarning` model.
 
@@ -131,20 +134,20 @@ def data_validator(data):
         if form.is_valid():
             data = form.cleaned_data
             warning = DiffWarning.objects.create(**data)
-            return JsonResponse(warning.serialize(), status=201)
+            return self.response(warning.serialize(), status=201)
 
-        return JsonResponse({"errors": dict(form.errors)}, status=400)
+        return self.response({"errors": dict(form.errors)}, status=400)
 
     @phabricator_api_key_required
     def delete(self, request: WSGIRequest, diff_warning_id: int) -> JsonResponse:
         """Archive a `DiffWarning` based on provided pk."""
         warning = DiffWarning.objects.get(pk=diff_warning_id)
         if not warning:
-            return JsonResponse({}, status=404)
+            return self.response({}, status=404)
 
         warning.status = DiffWarningStatus.ARCHIVED
         warning.save()
-        return JsonResponse(warning.serialize(), status=200)
+        return self.response(warning.serialize(), status=200)
 
     @phabricator_api_key_required
     def get(self, request: WSGIRequest, **kwargs) -> JsonResponse:
@@ -158,14 +161,14 @@ class Form(forms.Form):
         form = Form(request.GET)
         if form.is_valid():
             warnings = DiffWarning.objects.filter(**form.cleaned_data).all()
-            return JsonResponse(
+            return self.response(
                 [warning.serialize() for warning in warnings], status=200, safe=False
             )
 
-        return JsonResponse({"errors": dict(form.errors)}, status=400)
+        return self.response({"errors": dict(form.errors)}, status=400)
 
 
-class CommitMapBaseView(View):
+class CommitMapBaseView(APIView):
     """CommitMap base view to be extended for bidirectional git - hg mapping."""
 
     scm: str
@@ -177,16 +180,16 @@ def get(
             commit = CommitMap.map_hash_from(self.scm, git_repo_name, commit_hash)
         except CommitMap.DoesNotExist as exc:
             error_detail = f"No commit found in {self.scm} for {commit_hash} in {git_repo_name}: {exc}"
-            return JsonResponse(
+            return self.response(
                 {"error": "No commits found", "detail": error_detail}, status=404
             )
         except CommitMap.MultipleObjectsReturned as exc:
             error_detail = f"Multiple commits found in {self.scm} for {commit_hash} in {git_repo_name}: {exc}"
-            return JsonResponse(
+            return self.response(
                 {"error": "Multiple commits found", "detail": error_detail}, status=400
             )
 
-        return JsonResponse(commit.serialize(), status=200)
+        return self.response(commit.serialize(), status=200)
 
 
 @method_decorator(csrf_exempt, name="dispatch")
@@ -203,7 +206,7 @@ class hg2gitCommitMapView(CommitMapBaseView):
     scm = SCMType.HG
 
 
-class PullRequestAPIView(View):
+class PullRequestAPIView(APIView):
     """Set various common attributes for views that extend this one."""
 
     target_repo: Repo
@@ -245,7 +248,7 @@ def get(
                 status = str(_status).lower()
                 break
 
-        return JsonResponse({"status": status}, status=200)
+        return self.response({"status": status}, status=200)
 
     @method_decorator(require_authenticated_user)
     def post(
@@ -268,12 +271,12 @@ class Form(forms.Form):
 
         if blockers:
             # Pull request has blockers that prevent it from landing.
-            return JsonResponse({"errors": blockers}, status=400)
+            return self.response({"errors": blockers}, status=400)
 
         form = Form(json.loads(request.body))
 
         if not form.is_valid():
-            return JsonResponse(form.errors, 400)
+            return self.response(form.errors, 400)
 
         job = LandingJob.objects.create(
             target_repo=self.target_repo,
@@ -311,7 +314,7 @@ class Form(forms.Form):
         job.status = JobStatus.SUBMITTED
         job.save()
 
-        return JsonResponse({"id": job.id}, status=201)
+        return self.response({"id": job.id}, status=201)
 
 
 class PullRequestChecksAPIView(PullRequestAPIView):
@@ -324,5 +327,5 @@ def get(
             )
         except PullRequest.StaleMetadataException as exc:
             # The StaleMetadataException error message is safe for user consumption.
-            return JsonResponse({"errors": [str(exc)]}, status=500)
-        return JsonResponse(warnings_and_blockers)
+            return self.response({"errors": [str(exc)]}, status=500)
+        return self.response(warnings_and_blockers)

src/lando/ui/jobs.py

@@ -50,7 +50,7 @@ def get(self, request: WSGIRequest, job_id: int) -> TemplateResponse:
                     queue = queue[: queue.index(job)]
             context["queue"] = queue
 
-        return TemplateResponse(
+        return self.response(
             request=request,
             template="jobs/job.html",
             context=context,
@@ -108,7 +108,7 @@ def get(
                     queue = queue[: queue.index(landing_job)]
             context["queue"] = queue
 
-        return TemplateResponse(
+        return self.response(
             request=request,
             template="jobs/job.html",
             context=context,

src/lando/ui/legacy/pages.py

@@ -24,4 +24,4 @@ def get(self, request: WSGIRequest) -> TemplateResponse:
                 status__in=JobStatus.final(),
             ).order_by("-updated_at")[: self.MAX_JOBS_HISTORY]
 
-        return TemplateResponse(request=request, template="home.html", context=context)
+        return self.response(request=request, template="home.html", context=context)

src/lando/ui/legacy/revisions.py

@@ -293,7 +293,7 @@ def get(self, request: WSGIRequest) -> TemplateResponse:
             "assessment": assessment_instance,
         }
 
-        return TemplateResponse(
+        return self.response(
             request=request,
             template="uplift/request.html",
             context=context,
@@ -532,7 +532,7 @@ def get(
             ),
         }
 
-        return TemplateResponse(
+        return self.response(
             request=request,
             template="stack/stack.html",
             context=context,

src/lando/ui/pull_requests.py

@@ -1,7 +1,6 @@
 import logging
 
 from django.core.handlers.wsgi import WSGIRequest
-from django.http import Http404
 from django.template.response import TemplateResponse
 from requests import HTTPError
 
@@ -26,10 +25,10 @@ def get(
         try:
             target_repo = Repo.objects.get(name=repo_name)
         except Repo.DoesNotExist:
-            raise Http404(f"Repository {repo_name} doesn't exist.")
+            self.raise_http404(f"Repository {repo_name} doesn't exist.")
 
         if not target_repo.pr_enabled:
-            raise Http404(
+            self.raise_http404(
                 f"Pull Requests are not supported for repository {repo_name}."
             )
 
@@ -39,7 +38,9 @@ def get(
             pull_request = client.build_pull_request(number)
         except HTTPError as e:
             if e.response.status_code == 404:
-                raise Http404(f"Pull request {repo_name}#{number} doesn't exist") from e
+                self.raise_http404(
+                    f"Pull request {repo_name}#{number} doesn't exist", e
+                )
             raise e
 
         landing_jobs = get_jobs_for_pull(target_repo, number)
@@ -50,7 +51,7 @@ def get(
             "landing_jobs": landing_jobs,
         }
 
-        return TemplateResponse(
+        return self.response(
             request=request,
             template="stack/pull_request.html",
             context=context,

src/lando/ui/views.py

@@ -1,5 +1,10 @@
+from django.template.response import TemplateResponse
 from django.views import View
 
+from lando.utils.views import BaseLandoViewMixin
 
-class LandoView(View):
-    pass
+
+class LandoView(View, BaseLandoViewMixin):
+    """A base class for UI views."""
+
+    response_class = TemplateResponse

src/lando/utils/views.py

@@ -0,0 +1,29 @@
+from django.core.exceptions import PermissionDenied
+from django.http import Http404, JsonResponse
+from django.template.response import TemplateResponse
+
+
+class BaseLandoViewMixin:
+    """Provide helper methods for returning HTTP responses."""
+
+    def _raise_handled_exception(self, exception: Exception, original: Exception):
+        """Handle the special case of raising exceptions that are handled by middleware."""
+        if original:
+            raise exception from original
+        raise exception
+
+    def response(self, *args, **kwargs) -> TemplateResponse | JsonResponse:
+        """Return a response object based on the base response class."""
+        return self.response_class(*args, **kwargs)
+
+    def raise_http404(self, message: str = "", original: Exception | None = None):
+        """Raise django.http.Http404."""
+        # Note: this will use the 404 template available to Lando.
+        exception = Http404(message)
+        self._raise_handled_exception(exception, original)
+
+    def raise_permission_denied(self, message: str, original: Exception | None = None):
+        """Raise django.core.exceptions.PermissionDenied which is handled by middleware."""
+        # Note: this will use the 403 template available to Lando.
+        exception = PermissionDenied(message)
+        self._raise_handled_exception(exception, original)