Merge branch 'master' into dill21yu-patch-1

Author: dill21yu

.github/workflows/pr-comments.yml

@@ -1,13 +1,10 @@
 name: PR Comments
 
 on:
-  pull_request_target:
+  pull_request:
     branches:
       - master
 
-permissions:
-  pull-requests: write
-
 jobs:
   backend-tests-and-coverage:
     runs-on: ubuntu-latest
@@ -56,10 +53,8 @@ jobs:
         uses: MishaKav/pytest-coverage-comment@v1
         with:
           pytest-xml-coverage-path: ./reports/code-cov/coverage.xml
-          junitxml-path: ./reports/pytest/test_report.xml
-          junitxml-title: Pytest Report
-          title: Python Coverage Report
-          badge-title: Python Code Coverage
+          title: Backend Code Coverage Report
+          badge-title: Coverage
           report-only-changed-files: true
           xml-skip-covered: true
           remove-link-from-badge: true
@@ -121,7 +116,7 @@ jobs:
         uses: MishaKav/jest-coverage-comment@v1
         with:
           coverage-summary-path: ./reports/jest/coverage-summary.json
-          summary-title: UI Coverage Report
+          summary-title: UI Code Coverage Report
           badge-title: Coverage
           hide-comment: false
           create-new-comment: false

.gitignore

@@ -9,6 +9,14 @@
 # pip requirements file
 .requirements
 
+# Other generated build artifacts by generate_requirements.py
+desktop/core/requirements-arm64-*.txt
+desktop/core/requirements-aarch64-*.txt
+desktop/core/requirements-x86_64-*.txt
+
+# Python version file
+.python-version
+
 # Python compiled files
 *.pyc
 *.pyo
@@ -32,6 +40,13 @@ metastore_db
 VERSION_INFO
 npm_dist
 
+# Build marker files generated by Makefile
+.npm-install-lock
+desktop/core/ext-env-pip-install
+
+# Python version-specific directories
+desktop/core/3.*/
+
 # Generated configuration
 desktop/conf/*.ini
 

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
+      )

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."]}

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.")