Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add statistics to Articles #3798

Merged
merged 8 commits into from
Mar 26, 2025
Merged

Add statistics to Articles #3798

merged 8 commits into from
Mar 26, 2025

Conversation

itsisak
Copy link
Contributor

@itsisak itsisak commented Mar 21, 2025

Description

Create endpoint for getting statistics from plausible for articles. Also move logic for fetching from plausible to own function. Mercilessly "kokt" from events.

Testing

Wrote some tests mocking a response from Plausible, but there are Nno fixtures has data on plausible so should probably test this in staging.

Resolves ABA-1375

@itsisak itsisak added new-feature Pull requests that introduce or issues that suggest a new feature review-needed Pull requests that need review labels Mar 21, 2025
@itsisak itsisak self-assigned this Mar 21, 2025
Copy link

linear bot commented Mar 21, 2025

Copy link

coderabbitai bot commented Mar 21, 2025

📝 Walkthrough

Walkthrough

The changes introduce a new endpoint for retrieving article statistics with appropriate authentication and permission checks in the ArticlesViewSet. Testing capabilities for this feature are enhanced with the addition of helper functions and a dedicated test case class. The statistics method in the EventViewSet is refactored to utilize a centralized utility function, request_plausible_statistics, which simplifies the process of fetching statistics data. This utility function is also newly defined to handle API requests for both articles and events.

Changes

File(s) Change Summary
lego/.../articles/tests/test_articles_api.py Added functions (get_statistics_url, mock_plausible_response) and a test case class (ArticleStatisticsCase) to validate the article statistics endpoint under various authentication and permission scenarios.
lego/.../articles/views.py Updated ArticlesViewSet to change inheritance and add a new statistics action method that checks user authentication and permissions, calling request_plausible_statistics to retrieve article statistics.
lego/.../events/views.py Refactored the statistics method in EventViewSet by replacing manual API request construction with a call to request_plausible_statistics, simplifying the control flow.
lego/.../utils/functions.py Introduced the request_plausible_statistics function that constructs an API URL based on object attributes, sets up authorization headers, and performs a GET request to fetch statistics data.

Assessment against linked issues

Objective Addressed Explanation
Create statistics API endpoint (ABA-1375)
✨ Finishing Touches
  • 📝 Generate Docstrings

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai plan to trigger planning for file edits and PR creation.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

coderabbitai[bot]
coderabbitai bot previously requested changes Mar 21, 2025
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 94cf036 and b057452.

📒 Files selected for processing (4)
  • lego/apps/articles/tests/test_articles_api.py (3 hunks)
  • lego/apps/articles/views.py (3 hunks)
  • lego/apps/events/views.py (2 hunks)
  • lego/utils/functions.py (2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
lego/apps/events/views.py (1)
lego/utils/functions.py (2) (2)
  • request_plausible_statistics (48-68)
  • verify_captcha (17-31)
🪛 Ruff (0.8.2)
lego/apps/articles/views.py

62-62: Missing return type annotation for public function statistics

(ANN201)


62-62: Missing type annotation for function argument request

(ANN001)


62-62: Missing type annotation for function argument pk

(ANN001)


62-62: Missing type annotation for *args

(ANN002)


62-62: Unused method argument: args

(ARG002)


62-62: Missing type annotation for **kwargs

(ANN003)


62-62: Unused method argument: kwargs

(ARG002)


71-71: Trailing comma missing

Add trailing comma

(COM812)

lego/utils/functions.py

50-50: datetime.datetime.now() called without a tz argument

(DTZ005)


55-61: Use f-string instead of format call

Convert to f-string

(UP032)


68-68: Probable use of requests call without timeout

(S113)

lego/apps/articles/tests/test_articles_api.py

6-6: rest_framework.response.Response imported but unused

Remove unused import: rest_framework.response.Response

(F401)


22-22: Missing return type annotation for public function get_statistics_url

(ANN201)


22-22: Missing type annotation for function argument pk

(ANN001)


26-26: Missing return type annotation for public function create_user

(ANN201)


30-30: Missing return type annotation for public function create_other_user

(ANN201)


322-322: Missing return type annotation for public function generate_plausible_item

(ANN201)


322-322: Missing type annotation for function argument i

(ANN001)


325-325: datetime.datetime.now() called without a tz argument

(DTZ005)


335-335: Missing return type annotation for public function mocked_plausible_request

(ANN201)


335-335: Missing type annotation for *args

(ANN002)


335-335: Unused function argument: args

(ARG001)


335-335: Missing type annotation for **kwargs

(ANN003)


335-335: Unused function argument: kwargs

(ARG001)


337-337: Missing return type annotation for special method __init__

Add return type annotation: None

(ANN204)


337-337: Missing type annotation for *args

(ANN002)


337-337: Unused method argument: args

(ARG002)


337-337: Missing type annotation for **kwargs

(ANN003)


337-337: Unused method argument: kwargs

(ARG002)


343-343: Missing return type annotation for private function json

(ANN202)


354-354: Missing return type annotation for public function setUp

Add return type annotation: None

(ANN201)


367-367: Missing return type annotation for public function test_unauthenticated

Add return type annotation: None

(ANN201)


367-367: Missing type annotation for *args

(ANN002)


367-367: Unused method argument: args

(ARG002)


369-369: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


371-371: Missing return type annotation for public function test_authenticated_view

Add return type annotation: None

(ANN201)


371-371: Missing type annotation for *args

(ANN002)


371-371: Unused method argument: args

(ARG002)


374-374: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


376-376: Missing return type annotation for public function test_authenticated_edit

Add return type annotation: None

(ANN201)


376-376: Missing type annotation for *args

(ANN002)


376-376: Unused method argument: args

(ARG002)


379-379: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


380-380: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


381-381: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)

🔇 Additional comments (4)
lego/apps/events/views.py (2)

75-75: Good refactoring to use the shared utility function.

The import of request_plausible_statistics from the centralized utility simplifies the code and promotes code reuse.


265-266: Clean implementation using the shared utility.

The refactored code is more concise and maintainable by delegating the responsibility of constructing and making the external request to a dedicated function.

lego/utils/functions.py (1)

1-12: Appropriate imports for the new functionality.

The added imports properly support the new function's requirements.

lego/apps/articles/tests/test_articles_api.py (1)

349-382: Well-structured test cases for the statistics endpoint.

The test cases appropriately verify:

  1. Unauthenticated access is rejected
  2. Authenticated users without proper permissions cannot access statistics
  3. Users with edit permissions can access statistics

This provides good coverage for the new functionality.

🧰 Tools
🪛 Ruff (0.8.2)

354-354: Missing return type annotation for public function setUp

Add return type annotation: None

(ANN201)


367-367: Missing return type annotation for public function test_unauthenticated

Add return type annotation: None

(ANN201)


367-367: Missing type annotation for *args

(ANN002)


367-367: Unused method argument: args

(ARG002)


369-369: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


371-371: Missing return type annotation for public function test_authenticated_view

Add return type annotation: None

(ANN201)


371-371: Missing type annotation for *args

(ANN002)


371-371: Unused method argument: args

(ARG002)


374-374: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


376-376: Missing return type annotation for public function test_authenticated_edit

Add return type annotation: None

(ANN201)


376-376: Missing type annotation for *args

(ANN002)


376-376: Unused method argument: args

(ARG002)


379-379: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


380-380: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


381-381: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
lego/apps/articles/views.py (2)

3-7: Update imports to remove unused functions.

The import get_permission_handler from lego.apps.permissions.utils is not used in the file and should be removed to avoid confusion and keep the code clean.

from lego.apps.permissions.constants import OBJECT_PERMISSIONS_FIELDS
-from lego.apps.permissions.utils import get_permission_handler
from lego.utils.functions import request_plausible_statistics

Also applies to: 18-19


69-71: Good permission handling but consider using permissions framework consistently.

Using user.has_perm() directly works, but for consistency with the rest of the codebase, consider using the permission handler approach as seen in other parts of the application.

-    has_perm = user.has_perm("statistics", article) or user.has_perm(
-        "edit", article
-    )
-    if not has_perm:
+    permission_handler = get_permission_handler(Article)
+    user_has_perm = permission_handler.has_object_permissions(
+        user, "statistics", article
+    ) or permission_handler.has_object_permissions(
+        user, "edit", article
+    )
+    if not user_has_perm:
        return Response(status=status.HTTP_403_FORBIDDEN)
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8dddb93 and d0dd1ee.

📒 Files selected for processing (1)
  • lego/apps/articles/views.py (3 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
lego/apps/articles/views.py

18-18: lego.apps.permissions.utils.get_permission_handler imported but unused

Remove unused import: lego.apps.permissions.utils.get_permission_handler

(F401)

🔇 Additional comments (2)
lego/apps/articles/views.py (2)

22-22: LGTM: ViewSet class updated to use direct import.

Changed from using viewsets.ModelViewSet to a direct import of ModelViewSet, which is a good practice for cleaner imports.


61-76: Add error handling and type annotations to the statistics endpoint.

The implementation of the statistics endpoint is good, but it could be improved with better error handling and type annotations as mentioned in previous reviews.

  1. Add error handling for article retrieval and the Plausible request
  2. Add type annotations for the request parameter
  3. Consider error handling for edge cases
@action(detail=True, methods=["GET"])
-def statistics(self, request, pk=None, *args, **kwargs) -> Response:
+def statistics(self, request, pk=None, *args, **kwargs) -> Response:
-    article = Article.objects.get(id=pk)
+    try:
+        article = Article.objects.get(id=pk)
+    except Article.DoesNotExist:
+        return Response(
+            {"error": "Article not found"},
+            status=status.HTTP_404_NOT_FOUND
+        )
    user = request.user

    if not user or not user.is_authenticated:
        return Response(status=status.HTTP_401_UNAUTHORIZED)

    has_perm = user.has_perm("statistics", article) or user.has_perm(
        "edit", article
    )
    if not has_perm:
        return Response(status=status.HTTP_403_FORBIDDEN)

-    response = request_plausible_statistics(article)
-    return Response(response.json())
+    try:
+        response = request_plausible_statistics(article)
+        return Response(response.json())
+    except Exception as e:
+        import logging
+        log = logging.getLogger(__name__)
+        log.error("plausible_statistics_error", exc_info=True, extra={
+            "article_id": article.id
+        })
+        return Response(
+            {"error": "Failed to retrieve statistics"},
+            status=status.HTTP_503_SERVICE_UNAVAILABLE
+        )

@itsisak itsisak force-pushed the article-statistics branch from d0dd1ee to 8351c0c Compare March 23, 2025 16:30
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
lego/apps/articles/views.py (1)

61-76: 🛠️ Refactor suggestion

Add error handling for the statistics endpoint

The statistics endpoint properly checks authentication and permissions, but it lacks error handling for potential exceptions from the Plausible request. Also, adding type annotations would improve code quality.

    @action(detail=True, methods=["GET"])
-   def statistics(self, request, pk=None, *args, **kwargs) -> Response:
+   def statistics(self, request, pk=None, *args, **kwargs) -> Response:
        article = Article.objects.get(id=pk)
        user = request.user

        if not user or not user.is_authenticated:
            return Response(status=status.HTTP_401_UNAUTHORIZED)

        has_perm = user.has_perm("statistics", article) or user.has_perm(
            "edit", article
        )
        if not has_perm:
            return Response(status=status.HTTP_403_FORBIDDEN)

-       response = request_plausible_statistics(article)
-       return Response(response.json())
+       try:
+           response = request_plausible_statistics(article)
+           return Response(response.json())
+       except Exception as e:
+           # Log the error
+           return Response(
+               {"error": "Failed to retrieve statistics"},
+               status=status.HTTP_503_SERVICE_UNAVAILABLE
+           )
🧹 Nitpick comments (2)
lego/apps/articles/views.py (1)

18-18: Remove unused import

The get_permission_handler import is not being used in this file. It should be removed to keep the imports clean.

-from lego.apps.permissions.utils import get_permission_handler
🧰 Tools
🪛 Ruff (0.8.2)

18-18: lego.apps.permissions.utils.get_permission_handler imported but unused

Remove unused import: lego.apps.permissions.utils.get_permission_handler

(F401)

lego/apps/articles/tests/test_articles_api.py (1)

6-6: Remove unused import

The Response import is not being used in this file. It should be removed to keep the imports clean.

-from rest_framework.response import Response
🧰 Tools
🪛 Ruff (0.8.2)

6-6: rest_framework.response.Response imported but unused

Remove unused import: rest_framework.response.Response

(F401)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d0dd1ee and 8351c0c.

📒 Files selected for processing (2)
  • lego/apps/articles/tests/test_articles_api.py (3 hunks)
  • lego/apps/articles/views.py (3 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
lego/apps/articles/tests/test_articles_api.py

6-6: rest_framework.response.Response imported but unused

Remove unused import: rest_framework.response.Response

(F401)

lego/apps/articles/views.py

18-18: lego.apps.permissions.utils.get_permission_handler imported but unused

Remove unused import: lego.apps.permissions.utils.get_permission_handler

(F401)

🔇 Additional comments (5)
lego/apps/articles/views.py (1)

69-71: Good permission implementation

The permission check correctly verifies if the user has either "statistics" or "edit" permissions on the article.

lego/apps/articles/tests/test_articles_api.py (4)

22-24: Good implementation of the statistics URL helper function

This helper function is well implemented and follows the same pattern as the other URL helper functions in the file.


321-342: Use timezone-aware datetime and add type annotations

The helper functions for testing are well-implemented, but there are small improvements for timezone consistency and code quality:

-def mock_plausible_response(*args, **kwargs):
+def mock_plausible_response(*args, **kwargs) -> object:
    class MockPlausibleResponse:
        def __init__(self, *args, **kwargs):
            self.status_code = status.HTTP_200_OK
            self.data = [
                self.generate_plausible_item(i)
                for i in range(number_of_plausible_items)
            ]

-        def generate_plausible_item(self, i):
+        def generate_plausible_item(self, i: int) -> dict:
            return {
                "bounce_rate": str(30),
-               "date": (datetime.now() - timedelta(days=i)).strftime("%Y-%m-%d"),
+               "date": (datetime.now().replace(tzinfo=timezone.utc) - timedelta(days=i)).strftime("%Y-%m-%d"),
                "pageviews": str(i * 2),
                "visit_duration": str(i * 10),
                "visitors": str(i),
            }

-        def json(self):
+        def json(self) -> list:
            return self.data

    return MockPlausibleResponse()

Don't forget to add the timezone import:

-from datetime import datetime, timedelta
+from datetime import datetime, timedelta, timezone

345-348: Good use of mock for testing

Using mock.patch to replace the real API call with a mock response is a good testing practice. This approach isolates the tests from the external Plausible service.


349-409: Comprehensive test cases for the statistics endpoint

The test cases cover all important permission scenarios:

  1. Unauthenticated users cannot access statistics
  2. Users with only view permission cannot access statistics
  3. Users with edit permission can access statistics
  4. Users with appropriate keyword permissions can access statistics
  5. Article creators can access their own statistics

This is a thorough and well-structured test suite.

Copy link
Member

@jonasdeluna jonasdeluna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8351c0c and 7dd4b3c.

📒 Files selected for processing (2)
  • lego/apps/articles/tests/test_articles_api.py (3 hunks)
  • lego/apps/articles/views.py (3 hunks)
🔇 Additional comments (13)
lego/apps/articles/views.py (3)

3-7: LGTM! Appropriate imports for the new functionality.

The new imports from rest_framework and the model view change are appropriate for implementing the statistics endpoint.


18-18: LGTM! Proper import for the Plausible statistics utility.

Importing the request_plausible_statistics function from the utils package is appropriate for fetching statistics data.


21-21: LGTM! Good refactoring of the import usage.

Simplifying the import usage by directly using ModelViewSet rather than viewsets.ModelViewSet is a good practice.

lego/apps/articles/tests/test_articles_api.py (10)

21-22: LGTM! Well-defined helper function.

Good practice to have a dedicated helper function for the statistics URL consistent with existing patterns.


317-317: LGTM! Good constant definition.

Defining a constant for the number of plausible items is a good practice for maintainability.


320-341: Add type annotations and use timezone-aware datetime.

The mock response implementation needs some improvements:

  1. Use timezone-aware datetime to ensure consistent test results
  2. Add type annotations for better code quality

Apply this diff:

def mock_plausible_response(*args, **kwargs):
    class MockPlausibleResponse:
        def __init__(self, *args, **kwargs):
            self.status_code = status.HTTP_200_OK
            self.data = [
                self.generate_plausible_item(i)
                for i in range(number_of_plausible_items)
            ]

-        def generate_plausible_item(self, i):
+        def generate_plausible_item(self, i: int) -> dict:
            return {
                "bounce_rate": str(30),
-                "date": (datetime.now() - timedelta(days=i)).strftime("%Y-%m-%d"),
+                "date": (datetime.now(timezone.utc) - timedelta(days=i)).strftime("%Y-%m-%d"),
                "pageviews": str(i * 2),
                "visit_duration": str(i * 10),
                "visitors": str(i),
            }

-        def json(self):
+        def json(self) -> list:
            return self.data

    return MockPlausibleResponse()

Don't forget to import timezone from django.utils.


344-348: LGTM! Good mocking approach.

The mock patching of the request_plausible_statistics function is well implemented.


349-357: LGTM! Good test setup.

The test setup properly creates the necessary user groups and articles with appropriate permissions.


358-362: LGTM! Good test for unauthenticated access.

The test correctly verifies that unauthenticated users cannot access the statistics endpoint.


363-369: LGTM! Good test for view group access.

The test properly verifies that users who can only view the article (but not edit) cannot access statistics.


370-380: LGTM! Well-structured test for edit group access.

The test correctly verifies that users with edit permissions can access statistics and checks the response structure.


381-396: LGTM! Comprehensive test for keyword permissions.

The test thoroughly checks that users with the appropriate keyword permission can access statistics. Good comments explaining the requirement to add the user to the view group.


397-407: LGTM! Good test for article creators.

The test verifies that the original creator of an article can access its statistics, which is an important use case.

Copy link
Member

@ivarnakken ivarnakken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool!

Copy link

codecov bot commented Mar 24, 2025

Codecov Report

Attention: Patch coverage is 94.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 89.97%. Comparing base (94cf036) to head (40b944c).
Report is 10 commits behind head on master.

Files with missing lines Patch % Lines
lego/utils/functions.py 80.00% 5 Missing ⚠️
lego/apps/events/views.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3798      +/-   ##
==========================================
+ Coverage   89.92%   89.97%   +0.04%     
==========================================
  Files         741      741              
  Lines       23985    24070      +85     
==========================================
+ Hits        21569    21656      +87     
+ Misses       2416     2414       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (3)
lego/utils/functions.py (3)

50-53: Add timezone awareness to datetime.now().

The current implementation uses a naive datetime object for the current time. For consistency and accuracy, you should use a timezone-aware datetime.

-    now = datetime.now().strftime("%Y-%m-%d")
+    from django.utils import timezone
+    now = datetime.now(timezone.utc).strftime("%Y-%m-%d")

56-56: Simplify redundant default value in kwargs.get().

The .get() method already returns None by default, so the explicit None is unnecessary.

-    url_root = kwargs.get("url_root", None)
+    url_root = kwargs.get("url_root")
🧰 Tools
🪛 Ruff (0.8.2)

56-56: Use kwargs.get("url_root") instead of kwargs.get("url_root", None)

Replace kwargs.get("url_root", None) with kwargs.get("url_root")

(SIM910)


50-90: Add docstring to document function parameters and return value.

This function would benefit from a docstring explaining its purpose, parameters, and return value to assist other developers who might use it.

 def request_plausible_statistics(obj: BasisModel, **kwargs) -> Response:
+    """
+    Fetch statistics from Plausible analytics for a given model instance.
+    
+    Args:
+        obj: The model instance to fetch statistics for
+        **kwargs: Additional arguments
+            url_root: Optional custom URL root to use instead of model name
+            
+    Returns:
+        Response: The HTTP response from Plausible
+        
+    Raises:
+        ValueError: If neither model_name nor url_root is available
+        requests.exceptions.RequestException: If the request to Plausible fails
+    """
🧰 Tools
🪛 Ruff (0.8.2)

56-56: Use kwargs.get("url_root") instead of kwargs.get("url_root", None)

Replace kwargs.get("url_root", None) with kwargs.get("url_root")

(SIM910)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a06efc4 and 2d2e5c6.

📒 Files selected for processing (2)
  • lego/apps/articles/tests/test_articles_api.py (3 hunks)
  • lego/utils/functions.py (2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
lego/utils/functions.py

56-56: Use kwargs.get("url_root") instead of kwargs.get("url_root", None)

Replace kwargs.get("url_root", None) with kwargs.get("url_root")

(SIM910)

🔇 Additional comments (12)
lego/utils/functions.py (3)

47-48: LGTM: Good separation for testability.

Separating the external API call into its own function facilitates easier mocking in tests, which is good practice.


72-75: LGTM: Safe handling of settings key.

The code correctly checks if the setting exists before trying to access it, preventing potential attribute errors.


81-89: LGTM: Good error handling with proper logging.

The implementation properly handles request exceptions and logs detailed error information, which is essential for diagnosing issues with external API calls.

lego/apps/articles/tests/test_articles_api.py (9)

317-318: LGTM: Well-defined constant for test configuration.

Defining the number of test items as a constant makes it easy to adjust the test data size and keeps the value consistent throughout the tests.


332-332: Add timezone awareness to datetime.now().

Similar to the recommendation in the main function, you should use a timezone-aware datetime here to ensure consistency and prevent potential timezone-related issues.

-                "date": (datetime.now() - timedelta(days=i)).strftime("%Y-%m-%d"),
+                from django.utils import timezone
+                "date": (datetime.now(timezone.utc) - timedelta(days=i)).strftime("%Y-%m-%d"),

320-342: Add type annotations to functions and methods.

For better code quality and to help with IDE auto-completion and type checking, consider adding type annotations to the mock functions and methods.

-def mock_plausible_response(*args, **kwargs):
+def mock_plausible_response(*args, **kwargs) -> 'MockPlausibleResponse':

class MockPlausibleResponse:
-    def __init__(self, *args, **kwargs):
+    def __init__(self, *args, **kwargs) -> None:

-    def generate_plausible_item(self, i):
+    def generate_plausible_item(self, i: int) -> dict:

-    def json(self):
+    def json(self) -> list[dict]:

344-348: LGTM: Good use of mock patching at the class level.

Using the mock.patch decorator at the class level is an efficient way to apply the mock to all test methods, reducing code duplication.


358-362: LGTM: Comprehensive test for unauthenticated users.

This test properly verifies that unauthenticated users cannot access the statistics endpoint.


363-369: LGTM: Accurate testing of permission restrictions.

This test correctly verifies that users who can only view an article cannot view its statistics, as they need edit permissions.


370-380: LGTM: Thorough testing of edit permissions.

This test correctly validates that users with edit permissions can access statistics and verifies the response data structure.


381-396: LGTM: Good testing of keyword permissions with clear comments.

The test includes helpful comments explaining why additional setup (adding the user to view_group) is needed for the ObjectPermissionModel, making the code more maintainable.


397-407: LGTM: Proper testing of content creator permissions.

This test correctly verifies that the creator of an article can view its statistics, which is an important permission case to validate.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (3)
lego/utils/functions.py (3)

47-49: Add timeout parameter to improve robustness in _request_plausible

The helper function correctly extracts the API call for easier mocking, but it should include a timeout parameter to prevent hanging requests.

def _request_plausible(url: str, headers: dict) -> Response:
-    return requests.get(url, headers=headers)
+    return requests.get(url, headers=headers, timeout=10)

58-58: Simplify kwargs.get call

Since None is already the default return value for dict.get(), specifying it explicitly is redundant.

-    url_root = kwargs.get("url_root", None)
+    url_root = kwargs.get("url_root")
🧰 Tools
🪛 Ruff (0.8.2)

58-58: Use kwargs.get("url_root") instead of kwargs.get("url_root", None)

Replace kwargs.get("url_root", None) with kwargs.get("url_root")

(SIM910)


74-81: Add validation for missing PLAUSIBLE_KEY

The current implementation sets the API key to an empty string if not available in settings, which might cause API calls to fail silently. Consider adding explicit validation or fallback behavior.

-    key = None
-    if hasattr(settings, "PLAUSIBLE_KEY"):
-        key = settings.PLAUSIBLE_KEY
-
-    headers = {
-        "Authorization": f"Bearer {key or ''}",
-        "Content-Type": "application/json",
-    }
+    if not hasattr(settings, "PLAUSIBLE_KEY") or not settings.PLAUSIBLE_KEY:
+        log.warning("plausible_key_missing", message="PLAUSIBLE_KEY not set in settings")
+        # Consider what behavior you want here - fail early or continue with empty token?
+    
+    headers = {
+        "Authorization": f"Bearer {getattr(settings, 'PLAUSIBLE_KEY', '')}",
+        "Content-Type": "application/json",
+    }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2d2e5c6 and 40b944c.

📒 Files selected for processing (1)
  • lego/utils/functions.py (2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
lego/utils/functions.py

58-58: Use kwargs.get("url_root") instead of kwargs.get("url_root", None)

Replace kwargs.get("url_root", None) with kwargs.get("url_root")

(SIM910)

🔇 Additional comments (2)
lego/utils/functions.py (2)

54-55: Use timezone-aware datetime

The current implementation uses a timezone-naive datetime, which could lead to incorrect date ranges if the server's timezone differs from UTC.

-    now = datetime.now().strftime("%Y-%m-%d")
+    from django.utils import timezone
+    now = datetime.now(timezone.utc).strftime("%Y-%m-%d")

52-92: Overall the implementation looks good

The function correctly handles fetching statistics from Plausible with proper error handling and URL construction. The suggestions above will further improve its robustness, but the current implementation is already a good improvement over the previous approach.

🧰 Tools
🪛 Ruff (0.8.2)

58-58: Use kwargs.get("url_root") instead of kwargs.get("url_root", None)

Replace kwargs.get("url_root", None) with kwargs.get("url_root")

(SIM910)

Copy link
Contributor

@ch0rizo ch0rizo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🪨

@itsisak itsisak merged commit 9d12668 into master Mar 26, 2025
3 checks passed
@itsisak itsisak deleted the article-statistics branch March 26, 2025 12:43
Copy link

sentry-io bot commented Mar 26, 2025

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ ValueError: Field 'id' expected a number but got '562-weekly-uke-13'. /api/v1/articles/{pk}/statistics/ View Issue

Did you find this useful? React with a 👍 or 👎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-feature Pull requests that introduce or issues that suggest a new feature review-needed Pull requests that need review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants