diff --git a/websites/views.py b/websites/views.py index 9912f3196..1e9a3fe3c 100644 --- a/websites/views.py +++ b/websites/views.py @@ -2,7 +2,7 @@ import json import logging -import os +from pathlib import Path from django.conf import settings from django.contrib.auth.models import Group @@ -10,11 +10,13 @@ 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) diff --git a/websites/views_test.py b/websites/views_test.py index 37fc5f3cd..c308479ae 100644 --- a/websites/views_test.py +++ b/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])