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

feat: Add Pagination Support to GET /api/v1/activity-log Endpoint #1197 #1223

Open
wants to merge 15 commits into
base: dev
Choose a base branch
from

Conversation

Dafinci01
Copy link

@Dafinci01 Dafinci01 commented Mar 2, 2025

Description

The current GET /api/v1/activity-logs endpoint returns all activity logs in a single response, which can lead to performance issues and overly large payloads as the number of logs grows. This enhancement introduces pagination support, allowing clients to fetch logs in manageable chunks.

Related Issue (Link to issue ticket)

[Issue #1115](#1115)

Motivation and Context

  • Improves performance by reducing response payload size.
  • Enhances API usability by providing paginated responses.
  • Prevents memory and latency issues with large datasets.

How Has This Been Tested?

Manual API Testing

Default Pagination:

Sent a GET request to /api/v1/activity-logs without any parameters.

Verified that the response included 20 logs (if available) and the correct pagination metadata.

Custom Pagination:

Sent a GET request to /api/v1/activity-logs?page=2&limit=10.

Confirmed that the response included the correct subset of logs corresponding to page 2 and limit 10, along with accurate pagination metadata.

Edge Case Testing:

Tested scenarios such as negative page values, excessively high limits, and requests for pages beyond the available data.

Verified that appropriate validation errors or empty results were returned.

Testing Steps

  1. Default Pagination:

    • Send a GET request to /api/v1/activity-logs without any parameters.
    • Verify that the response includes 20 logs (if available) and the correct pagination metadata.
  2. Custom Pagination:

    • Send a GET request to /api/v1/activity-logs?page=2&limit=10.

    • Confirm that the response includes the correct subset of logs corresponding to page 2 and limit 10, along with accurate pagination metadata.

Screenshots (if appropriate - Postman, etc):

Screenshot 2025-03-01 15130

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Proposed Solution

The endpoint should support pagination with the following changes:

Query Parameters:

  • page (optional, default: 1) - Specifies the page number.
  • limit (optional, default: 20) - Specifies the number of logs per page.

Response Structure:

The response should include a pagination object containing:

  • page: The current page number.
  • limit: The number of logs per page.
  • total_items: The total number of activity logs.
  • total_pages: The total number of pages based on the limit.

Default Behavior:

  • If no parameters are provided, return page 1 with 20 logs.
  • Maintain consistency with existing response structures.

Expected JSON Response Format:

{
  "status": "success",
  "status_code": 200,
  "message": "Activity logs retrieved successfully",
  "data": {
    "logs": [ /* array of activity logs */ ],
    "pagination": {
      "page": 1,
      "limit": 20,
      "total_items": 100,
      "total_pages": 5
    }
  }
}

Acceptance Criteria

  • The endpoint accepts optional page and limit query parameters.
  • Returns the default subset (page 1, 20 logs) when no query parameters are provided.
  • Includes a pagination object with page, limit, total_items, and total_pages in the response.
  • Maintains existing functionality and adheres to the standard JSON response format.

@phurhard
Copy link

phurhard commented Mar 2, 2025

Resolve the merge conflict

Copy link

@phurhard phurhard left a comment

Choose a reason for hiding this comment

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

Implement the changes requested

.gitignore Outdated
Comment on lines 177 to 207
# Ignore environment files
.env
.env.*
.ven
.venv
env
alembic/env.py

# Ignore environment and virtual environment files
.env
.env.*
.ven
.venv
env
alembic/env.py

# Ignore environment and virtual environment files
.env
.env.*
.ven
.venv
env
alembic/env.py

# Ignore environment and virtual environment files
.env
.env.*
.ven
.venv
env
alembic/env.py
Copy link

Choose a reason for hiding this comment

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

why the repitions??

README.md Outdated
@@ -45,7 +45,7 @@ GRANT ALL PRIVILEGES ON DATABASE hng_fast_api TO user;

**Starting the database**
after cloning the database, dont run
`alembic revision --autogenerate -m 'initial migration'`
`embic revision --autogenerate -m al'initial migration'`
Copy link

Choose a reason for hiding this comment

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

Revert this

Comment on lines +42 to +43
"""Get paginated activity logs"""
activity_logs = activity_log_service.fetch_all(db=db, page=page, limit=limit)
Copy link

Choose a reason for hiding this comment

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

just passing the page and page limits doesn't mean it's reflected in the implementation.

.env.sample Outdated
Copy link

Choose a reason for hiding this comment

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

why is this deleted, revert it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants