Refactor exception handling

websites/views.py

@@ -2,19 +2,21 @@
 
 import json
 import logging
-import os
+from pathlib import Path
 
 from django.conf import settings
 from django.contrib.auth.models import Group
 from django.contrib.postgres.search import SearchQuery, SearchVector
 from django.db.models import Case, CharField, F, OuterRef, Prefetch, Q, Value, When
 from django.utils.functional import cached_property
 from django.utils.text import slugify
+from github import GithubException
 from guardian.shortcuts import get_groups_with_perms, get_objects_for_user
 from mitol.common.utils.datetime import now_in_utc
+from requests import HTTPError
 from rest_framework import mixins, status, viewsets
 from rest_framework.decorators import action
-from rest_framework.exceptions import ValidationError
+from rest_framework.exceptions import PermissionDenied, ValidationError
 from rest_framework.generics import get_object_or_404
 from rest_framework.response import Response
 from rest_framework_extensions.mixins import NestedViewSetMixin
@@ -233,11 +235,34 @@ def publish_version(self, name, version, request):
                 )
             trigger_publish(website.name, version)
             return Response(status=200)
-        except ValidationError as ve:
-            return Response(data=ve.detail, status=status.HTTP_400_BAD_REQUEST)
-        except Exception as exc:  # pylint: disable=broad-except
-            log.exception("Error publishing %s version for %s", version, name)
-            return Response(status=500, data={"details": str(exc)})
+        except (ValidationError, PermissionDenied):
+            # Let DRF handle these exceptions
+            raise
+        except GithubException:
+            log.exception(
+                "GitHub error publishing %s version for %s (user: %s)",
+                version,
+                name,
+                request.user.username if request.user else "anonymous",
+            )
+            return Response(
+                status=status.HTTP_500_INTERNAL_SERVER_ERROR,
+                data={
+                    "error": "Failed to publish website",
+                    "error_type": "github_error",
+                },
+            )
+        except Exception:  # pylint: disable=broad-except
+            log.exception(
+                "Error publishing %s version for %s (user: %s)",
+                version,
+                name,
+                request.user.username if request.user else "anonymous",
+            )
+            return Response(
+                status=status.HTTP_500_INTERNAL_SERVER_ERROR,
+                data={"error": "Failed to publish website", "error_type": "unknown"},
+            )
 
     @action(
         detail=True, methods=["post"], permission_classes=[HasWebsitePreviewPermission]
@@ -311,11 +336,38 @@ def unpublish(self, request, name=None):
                     status=200,
                     data="The site has been submitted for unpublishing.",
                 )
-        except Exception as exc:  # pylint: disable=broad-except
-            log.exception("Error unpublishing %s", name)
-            return Response(status=500, data={"details": str(exc)})
+        except PermissionDenied:
+            # Let DRF handle permission exceptions
+            raise
+        except HTTPError:
+            log.exception(
+                "HTTP error unpublishing %s (user: %s)",
+                name,
+                request.user.username if request.user else "anonymous",
+            )
+            return Response(
+                status=status.HTTP_500_INTERNAL_SERVER_ERROR,
+                data={
+                    "error": "Failed to unpublish website",
+                    "error_type": "http_error",
+                },
+            )
+        except Exception:  # pylint: disable=broad-except
+            log.exception(
+                "Error unpublishing %s (user: %s)",
+                name,
+                request.user.username if request.user else "anonymous",
+            )
+            return Response(
+                status=status.HTTP_500_INTERNAL_SERVER_ERROR,
+                data={"error": "Failed to unpublish website", "error_type": "unknown"},
+            )
 
-    @action(detail=True, methods=["post"], permission_classes=[BearerTokenPermission])
+    @action(
+        detail=True,
+        methods=["post"],
+        permission_classes=[BearerTokenPermission],
+    )
     def pipeline_status(self, request, name=None):
         """Process webhook requests from concourse pipeline runs"""
         website = get_object_or_404(Website, name=name)
@@ -440,18 +492,40 @@ def site_configs(self, request):
                         for commit in data["commits"]
                     ]
                     for file in sublist
-                    if os.path.basename(file)  # noqa: PTH119
-                    == settings.OCW_STUDIO_SITE_CONFIG_FILE
+                    if Path(file).name == settings.OCW_STUDIO_SITE_CONFIG_FILE
                 ]
                 sync_github_website_starters(
                     data["repository"]["html_url"], files, commit=data.get("after")
                 )
-            except Exception as exc:  # pylint: disable=broad-except
-                log.exception("Error syncing config files")
-                return Response(status=500, data={"details": str(exc)})
+            except KeyError:
+                log.exception(
+                    "Key error syncing config files from repo %s, commit: %s",
+                    data.get("repository", {}).get("html_url"),
+                    data.get("after"),
+                )
+                return Response(
+                    status=status.HTTP_500_INTERNAL_SERVER_ERROR,
+                    data={
+                        "error": "Failed to sync site configuration files",
+                        "error_type": "key_error",
+                    },
+                )
+            except Exception:  # pylint: disable=broad-except
+                log.exception(
+                    "Error syncing config files from repo %s, files: %s, commit: %s",
+                    data.get("repository", {}).get("html_url"),
+                    files if "files" in locals() else [],
+                    data.get("after"),
+                )
+                return Response(
+                    status=status.HTTP_500_INTERNAL_SERVER_ERROR,
+                    data={
+                        "error": "Failed to sync site configuration files",
+                        "error_type": "unknown",
+                    },
+                )
             return Response(status=status.HTTP_202_ACCEPTED)
         else:
-            # Only github webhooks are currently supported
             return Response(status=status.HTTP_400_BAD_REQUEST)
 
 

websites/views_test.py

@@ -309,7 +309,8 @@ def test_websites_endpoint_preview_error(mocker, drf_client):
         reverse("websites_api-preview", kwargs={"name": website.name})
     )
     assert resp.status_code == 500
-    assert resp.data == {"details": "422 {}"}
+    assert resp.data["error"] == "Failed to publish website"
+    assert resp.data["error_type"] == "github_error"
 
 
 @pytest.mark.parametrize("unpublish_status", [PUBLISH_STATUS_NOT_STARTED, None])
@@ -351,10 +352,8 @@ def test_websites_endpoint_publish_denied(mocker, drf_client):
     resp = drf_client.post(
         reverse("websites_api-publish", kwargs={"name": website.name})
     )
-    assert resp.status_code == 500
-    assert resp.data == {
-        "details": "You do not have permission to perform this action."
-    }
+    assert resp.status_code == 403
+    assert "detail" in resp.data or "details" in resp.data
 
 
 def test_websites_endpoint_publish_error(mocker, drf_client):
@@ -371,7 +370,8 @@ def test_websites_endpoint_publish_error(mocker, drf_client):
         reverse("websites_api-publish", kwargs={"name": website.name})
     )
     assert resp.status_code == 500
-    assert resp.data == {"details": "422 {}"}
+    assert resp.data["error"] == "Failed to publish website"
+    assert resp.data["error_type"] == "github_error"
 
 
 @pytest.mark.parametrize("is_published", [True, False])
@@ -470,10 +470,8 @@ def test_websites_endpoint_unpublish_denied(drf_client, mocker):
     resp = drf_client.post(
         reverse("websites_api-unpublish", kwargs={"name": website.name})
     )
-    assert resp.status_code == 500
-    assert resp.data == {
-        "details": "You do not have permission to perform this action."
-    }
+    assert resp.status_code == 403
+    assert "detail" in resp.data or "details" in resp.data
 
 
 def test_websites_endpoint_unpublish_error(drf_client, mocker):
@@ -491,7 +489,8 @@ def test_websites_endpoint_unpublish_error(drf_client, mocker):
         reverse("websites_api-unpublish", kwargs={"name": website.name})
     )
     assert resp.status_code == 500
-    assert resp.data == {"details": "oops"}
+    assert resp.data["error"] == "Failed to unpublish website"
+    assert resp.data["error_type"] == "http_error"
 
 
 def test_websites_endpoint_detail_update_denied(drf_client):
@@ -709,7 +708,8 @@ def test_website_starters_site_configs_exception(mocker, drf_client):
         reverse("website_starters_api-site-configs"), data=MOCK_GITHUB_DATA
     )
     assert resp.status_code == 500
-    assert resp.data == {"details": "'Key not found'"}
+    assert resp.data["error"] == "Failed to sync site configuration files"
+    assert resp.data["error_type"] == "key_error"
 
 
 @pytest.mark.parametrize("detailed_list", [True, False])