Browse Source

[api] Refactor usage analytics API to DRF APIView and update tests (#4171)

- Replaces function-based analytics endpoints with a class-based
DRF view for improved permission handling, validation, and
error responses.
- Consolidates GET and PUT into a single endpoint,
removes redundant legacy routing, and updates tests to use
pytest and APIClient for modern, robust coverage.

Improves maintainability and aligns with REST API best practices.
Harsh Gupta 10 months ago
parent
commit
c2a44769bb

+ 43 - 59
apps/about/src/about/api.py

@@ -16,75 +16,59 @@
 # limitations under the License.
 
 import logging
+from typing import Any
 
 from rest_framework import status
+from rest_framework.request import Request
 from rest_framework.response import Response
+from rest_framework.views import APIView
 
-from desktop.auth.backend import is_admin
-from desktop.lib.conf import coerce_bool
+from about.serializer import UsageAnalyticsSerializer
+from desktop.auth.api_permissions import IsAdminUser
 from desktop.models import Settings
 
 LOG = logging.getLogger()
 
 
-def get_usage_analytics(request) -> Response:
+class UsageAnalyticsAPI(APIView):
   """
-  Retrieve the user preference for analytics settings.
+  Provides GET and PUT handlers for the usage analytics setting.
 
-  Args:
-    request (Request): The HTTP request object.
-
-  Returns:
-    Response: JSON response containing the analytics_enabled preference or an error message.
-
-  Raises:
-    403: If the user is not a Hue admin.
-    500: If there is an error retrieving preference.
-  """
-  if not is_admin(request.user):
-    return Response({'message': "You must be a Hue admin to access this endpoint."}, status=status.HTTP_403_FORBIDDEN)
-
-  try:
-    settings = Settings.get_settings()
-    return Response({'analytics_enabled': settings.collect_usage}, status=status.HTTP_200_OK)
-
-  except Exception as e:
-    message = f"Error retrieving usage analytics: {e}"
-    LOG.error(message)
-    return Response({'message': message}, status=status.HTTP_500_INTERNAL_SERVER_ERROR)
-
-
-def update_usage_analytics(request) -> Response:
+  This view allows authorized admin users to retrieve the current state of
+  the `collect_usage` setting and update it.
   """
-  Update the user preference for analytics settings.
-
-  Args:
-    request (Request): The HTTP request object containing 'analytics_enabled' parameter.
-
-  Returns:
-    Response: JSON response with the updated analytics_enabled preference or an error message.
-
-  Raises:
-    403: If the user is not a Hue admin.
-    400: If 'analytics_enabled' parameter is missing or invalid.
-    500: If there is an error updating preference.
-  """
-  if not is_admin(request.user):
-    return Response({'message': "You must be a Hue admin to access this endpoint."}, status=status.HTTP_403_FORBIDDEN)
-
-  try:
-    analytics_enabled = request.POST.get('analytics_enabled')
-
-    if analytics_enabled is None:
-      return Response({'message': 'Missing parameter: analytics_enabled is required.'}, status=status.HTTP_400_BAD_REQUEST)
-
-    settings = Settings.get_settings()
-    settings.collect_usage = coerce_bool(analytics_enabled)
-    settings.save()
-
-    return Response({'analytics_enabled': settings.collect_usage}, status=status.HTTP_200_OK)
 
-  except Exception as e:
-    message = f"Error updating usage analytics: {e}"
-    LOG.error(message)
-    return Response({'message': message}, status=status.HTTP_500_INTERNAL_SERVER_ERROR)
+  permission_classes = [IsAdminUser]
+
+  def get(self, request: Request, *args: Any, **kwargs: Any) -> Response:
+    """Handles GET requests to retrieve the current analytics setting."""
+    try:
+      settings = Settings.get_settings()
+      data = {"collect_usage": settings.collect_usage}
+
+      return Response(data, status=status.HTTP_200_OK)
+    except Exception as e:
+      LOG.error("Error retrieving usage analytics: %s", e)
+      return Response(
+        {"error": "A server error occurred while retrieving usage analytics settings."}, status=status.HTTP_500_INTERNAL_SERVER_ERROR
+      )
+
+  def put(self, request: Request, *args: Any, **kwargs: Any) -> Response:
+    """Handles PUT requests to update the analytics setting."""
+    try:
+      serializer = UsageAnalyticsSerializer(data=request.data)
+      if not serializer.is_valid():
+        return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST)
+
+      is_enabled = serializer.validated_data["collect_usage"]
+
+      settings = Settings.get_settings()
+      settings.collect_usage = is_enabled
+      settings.save()
+
+      return Response(serializer.data, status=status.HTTP_200_OK)
+    except Exception as e:
+      LOG.error("Error updating usage analytics: %s", e)
+      return Response(
+        {"error": "A server error occurred while saving the usage analytics settings."}, status=status.HTTP_500_INTERNAL_SERVER_ERROR
+      )

+ 82 - 61
apps/about/src/about/api_tests.py

@@ -14,89 +14,110 @@
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
-from unittest.mock import Mock, patch
+from unittest.mock import MagicMock, patch
 
+import pytest
+from django.core.exceptions import ObjectDoesNotExist
+from django.urls import reverse
 from rest_framework import status
+from rest_framework.test import APIClient
 
-from about.api import get_usage_analytics, update_usage_analytics
+from useradmin.models import User
 
 
-class TestUsageAnalyticsAPI:
-  def test_get_usage_analytics_success(self):
-    with patch('about.api.is_admin') as mock_is_admin:
-      with patch('about.api.Settings.get_settings') as mock_get_settings:
-        mock_is_admin.return_value = True
-        mock_get_settings.return_value = Mock(collect_usage=True)
+@pytest.mark.django_db
+class TestUsageAnalyticsSettingsAPI:
+  @pytest.fixture
+  def api_client(self) -> APIClient:
+    return APIClient()
 
-        request = Mock(method='GET', user=Mock())
-        response = get_usage_analytics(request)
+  @pytest.fixture
+  def regular_user(self, db) -> User:
+    return User.objects.create_user(username="testuser", password="")
 
-        assert response.status_code == status.HTTP_200_OK
-        assert response.data == {'analytics_enabled': True}
+  @pytest.fixture
+  def admin_user(self, db) -> User:
+    return User.objects.create_superuser(username="adminuser", password="")
 
-  def test_get_usage_analytics_unauthorized(self):
-    with patch('about.api.is_admin') as mock_is_admin:
-      mock_is_admin.return_value = False
+  @pytest.fixture
+  def analytics_settings_url(self) -> str:
+    return reverse("api:core_usage_analytics")
 
-      request = Mock(method='GET', user=Mock())
-      response = get_usage_analytics(request)
+  @patch("desktop.auth.api_permissions.is_admin", return_value=True)
+  @patch("about.api.Settings.get_settings")
+  def test_get_settings_as_admin_success(self, mock_get_settings, mock_is_admin, api_client, admin_user, analytics_settings_url):
+    mock_get_settings.return_value = MagicMock(collect_usage=True)
+    api_client.force_authenticate(user=admin_user)
 
-      assert response.status_code == status.HTTP_403_FORBIDDEN
-      assert response.data['message'] == "You must be a Hue admin to access this endpoint."
+    response = api_client.get(analytics_settings_url)
 
-  def test_get_usage_analytics_error(self):
-    with patch('about.api.is_admin') as mock_is_admin:
-      with patch('about.api.Settings.get_settings') as mock_get_settings:
-        mock_is_admin.return_value = True
-        mock_get_settings.side_effect = Exception("Test error")
+    assert response.status_code == status.HTTP_200_OK
+    assert response.data == {"collect_usage": True}
 
-        request = Mock(method='GET', user=Mock())
-        response = get_usage_analytics(request)
+  @patch("desktop.auth.api_permissions.is_admin", return_value=False)
+  def test_get_settings_as_non_admin_forbidden(self, mock_is_admin, api_client, regular_user, analytics_settings_url):
+    api_client.force_authenticate(user=regular_user)
 
-        assert response.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR
-        assert "Error retrieving usage analytics" in response.data['message']
+    response = api_client.get(analytics_settings_url)
 
-  def test_update_usage_analytics_success(self):
-    with patch('about.api.is_admin') as mock_is_admin:
-      with patch('about.api.Settings.get_settings') as mock_get_settings:
-        mock_is_admin.return_value = True
-        mock_get_settings.return_value = Mock(save=Mock())
+    assert response.status_code == status.HTTP_403_FORBIDDEN
+    assert response.data == {"detail": "You must be a Hue admin to perform this action."}
 
-        request = Mock(method='POST', user=Mock(), POST={'analytics_enabled': 'true'})
-        response = update_usage_analytics(request)
+  @patch("desktop.auth.api_permissions.is_admin", return_value=True)
+  @patch("about.api.Settings.get_settings")
+  def test_get_settings_as_admin_error(self, mock_get_settings, mock_is_admin, api_client, admin_user, analytics_settings_url):
+    mock_get_settings.side_effect = ObjectDoesNotExist("Settings not found")
+    api_client.force_authenticate(user=admin_user)
 
-        assert response.status_code == status.HTTP_200_OK
-        assert mock_get_settings.return_value.save.called
-        assert response.data == {'analytics_enabled': True}
+    response = api_client.get(analytics_settings_url)
 
-  def test_update_usage_analytics_unauthorized(self):
-    with patch('about.api.is_admin') as mock_is_admin:
-      mock_is_admin.return_value = False
+    assert response.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR
+    assert response.data == {"error": "A server error occurred while retrieving usage analytics settings."}
 
-      request = Mock(method='POST', user=Mock(), data={'analytics_enabled': 'true'})
-      response = update_usage_analytics(request)
+  @patch("desktop.auth.api_permissions.is_admin", return_value=True)
+  @patch("about.api.Settings.get_settings")
+  def test_put_settings_as_admin_success(self, mock_get_settings, mock_is_admin, api_client, admin_user, analytics_settings_url):
+    mock_settings = MagicMock()
+    mock_get_settings.return_value = mock_settings
+    api_client.force_authenticate(user=admin_user)
+    payload = {"collect_usage": False}
 
-      assert response.status_code == status.HTTP_403_FORBIDDEN
-      assert response.data['message'] == "You must be a Hue admin to access this endpoint."
+    response = api_client.put(analytics_settings_url, data=payload, format="json")
 
-  def test_update_usage_analytics_missing_param(self):
-    with patch('about.api.is_admin') as mock_is_admin:
-      mock_is_admin.return_value = True
+    assert response.status_code == status.HTTP_200_OK
+    assert response.data == payload
+    assert mock_settings.collect_usage is False
+    mock_settings.save.assert_called_once()
 
-      request = Mock(method='POST', user=Mock(), POST={})
-      response = update_usage_analytics(request)
+  @patch("desktop.auth.api_permissions.is_admin", return_value=False)
+  def test_put_settings_as_non_admin_forbidden(self, mock_is_admin, api_client, regular_user, analytics_settings_url):
+    api_client.force_authenticate(user=regular_user)
 
-      assert response.status_code == status.HTTP_400_BAD_REQUEST
-      assert response.data['message'] == 'Missing parameter: analytics_enabled is required.'
+    response = api_client.put(analytics_settings_url)
 
-  def test_update_usage_analytics_error(self):
-    with patch('about.api.is_admin') as mock_is_admin:
-      with patch('about.api.Settings.get_settings') as mock_get_settings:
-        mock_is_admin.return_value = True
-        mock_get_settings.side_effect = Exception("Test error")
+    assert response.status_code == status.HTTP_403_FORBIDDEN
+    assert response.data == {"detail": "You must be a Hue admin to perform this action."}
 
-        request = Mock(method='POST', user=Mock(), POST={'analytics_enabled': 'true'})
-        response = update_usage_analytics(request)
+  @patch("desktop.auth.api_permissions.is_admin", return_value=True)
+  @patch("about.api.Settings.get_settings")
+  def test_put_settings_as_admin_error(self, mock_get_settings, mock_is_admin, api_client, admin_user, analytics_settings_url):
+    mock_get_settings.side_effect = ObjectDoesNotExist("Settings not found")
+    api_client.force_authenticate(user=admin_user)
+    payload = {"collect_usage": False}
 
-        assert response.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR
-        assert "Error updating usage analytics" in response.data['message']
+    response = api_client.put(analytics_settings_url, data=payload, format="json")
+
+    assert response.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR
+    assert response.data == {"error": "A server error occurred while saving the usage analytics settings."}
+
+  @patch("desktop.auth.api_permissions.is_admin", return_value=True)
+  @patch("about.api.Settings.get_settings")
+  def test_put_settings_as_admin_missing_field(self, mock_get_settings, mock_is_admin, api_client, admin_user, analytics_settings_url):
+    mock_get_settings.return_value = MagicMock(collect_usage=True)
+    api_client.force_authenticate(user=admin_user)
+    payload = {}
+
+    response = api_client.put(analytics_settings_url, data=payload, format="json")
+
+    assert response.status_code == status.HTTP_400_BAD_REQUEST
+    assert response.data == {"collect_usage": ["This field is required."]}

+ 23 - 0
apps/about/src/about/serializer.py

@@ -0,0 +1,23 @@
+#!/usr/bin/env python
+# Licensed to Cloudera, Inc. under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  Cloudera, Inc. licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+from rest_framework import serializers
+
+
+class UsageAnalyticsSerializer(serializers.Serializer):
+  """Serializes and validates the usage analytics settings data."""
+
+  collect_usage = serializers.BooleanField(required=True, help_text="Indicates whether usage analytics collection is enabled.")

+ 1 - 14
desktop/core/src/desktop/api_public.py

@@ -19,10 +19,9 @@ import json
 import logging
 
 from django.http import HttpResponse, QueryDict
-from rest_framework.decorators import api_view, authentication_classes, permission_classes
+from rest_framework.decorators import api_view, permission_classes
 from rest_framework.permissions import AllowAny
 
-from about import api as about_api
 from beeswax import api as beeswax_api
 from desktop import api2 as desktop_api
 from desktop.auth.backend import rewrite_user
@@ -90,18 +89,6 @@ def available_app_examples(request):
   return desktop_api.available_app_examples(django_request)
 
 
-@api_view(["GET"])
-def get_usage_analytics(request):
-  django_request = get_django_request(request)
-  return about_api.get_usage_analytics(django_request)
-
-
-@api_view(["POST"])
-def update_usage_analytics(request):
-  django_request = get_django_request(request)
-  return about_api.update_usage_analytics(django_request)
-
-
 # Editor
 
 

+ 2 - 2
desktop/core/src/desktop/api_public_urls_v1.py

@@ -17,6 +17,7 @@
 
 from django.urls import re_path
 
+from about import api as about_api
 from desktop import api_public
 from desktop.lib.botserver import api as botserver_api
 from desktop.lib.importer import api as importer_api
@@ -36,11 +37,10 @@ urlpatterns += [
   re_path(r'^logs/download/?$', api_public.download_hue_logs, name='core_download_hue_logs'),
   re_path(r'^install_app_examples/?$', api_public.install_app_examples, name='core_install_app_examples'),
   re_path(r'^available_app_examples/?$', api_public.available_app_examples, name='core_available_app_examples'),
-  re_path(r'^usage_analytics/?$', api_public.get_usage_analytics, name='core_get_usage_analytics'),
-  re_path(r'^usage_analytics/update/?$', api_public.update_usage_analytics, name='core_update_usage_analytics'),
   re_path(r'^get_config/?$', api_public.get_config),
   re_path(r'^check_config/?$', api_public.check_config, name='core_check_config'),
   re_path(r'^get_namespaces/(?P<interface>[\w\-]+)/?$', api_public.get_context_namespaces),  # To remove
+  re_path(r'^usage_analytics/?$', about_api.UsageAnalyticsAPI.as_view(), name='core_usage_analytics'),
 ]
 
 urlpatterns += [

+ 47 - 0
desktop/core/src/desktop/auth/api_permissions.py

@@ -0,0 +1,47 @@
+#!/usr/bin/env python
+# Licensed to Cloudera, Inc. under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  Cloudera, Inc. licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+import logging
+from typing import TYPE_CHECKING
+
+from rest_framework import permissions
+from rest_framework.request import Request
+
+from desktop.auth.backend import is_admin
+
+# TYPE_CHECKING is True only during static type checking, avoids circular imports.
+if TYPE_CHECKING:
+  from rest_framework.views import APIView
+
+LOG = logging.getLogger()
+
+
+class IsAdminUser(permissions.BasePermission):
+  """A DRF permission class that only allows access to admin users."""
+
+  message = "You must be a Hue admin to perform this action."
+
+  def has_permission(self, request: Request, view: "APIView") -> bool:
+    """Checks if the request's user is authenticated and is an admin."""
+
+    if not request.user or not request.user.is_authenticated:
+      return False
+
+    try:
+      return is_admin(request.user)
+    except Exception:
+      LOG.exception("Permission check failed: Could not validate user %s.", request.user)
+      return False