Skip to content

Conversation

VaibhavSingh8
Copy link
Contributor

@VaibhavSingh8 VaibhavSingh8 commented Jun 17, 2025

Date: 17-Jun-2025

Developer Name: @VaibhavSingh8


Issue Ticket Number

Description

Tests for Authentication, missed in previous PR merged to develop

Documentation Updated?

  • Yes
  • No

Under Feature Flag

  • Yes
  • No

Database Changes

  • Yes
  • No

Breaking Changes

  • Yes
  • No

Development Tested?

  • Yes
  • No

Screenshots

Screenshot 1 Screenshot 2025-06-17 at 11 26 29 PM

Test Coverage

Screenshot 1 Screenshot 2025-06-17 at 11 27 09 PM

Additional Notes

Description by Korbit AI

What change is being made?

Add comprehensive support and testing for Google OAuth authentication in the Todo app.

Why are these changes being made?

These changes implement and ensure that OAuth-based authentication flows function correctly within the application, improve the middleware handling of JWTs, and include extensive tests to validate Google authentication processes, thereby enhancing the app's security and user reliability.

Is this description stale? Ask me to generate a new description by commenting /korbit-generate-pr-description

Copy link

coderabbitai bot commented Jun 17, 2025

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Summary by CodeRabbit

  • Refactor
    • Improved formatting and consistency of error and success response construction in authentication views and middleware. No changes to application logic or user-facing behavior.
  • Tests
    • Added comprehensive unit and integration tests for authentication middleware, user and OAuth services, user repository, user model, and authentication/task-related views.
    • Standardized authentication setup across test cases and updated error message assertions for consistency.
    • Introduced new test fixtures for user data.

Walkthrough

This update introduces comprehensive unit and integration tests for authentication and user management, including Google OAuth and RDS authentication. It refactors error response formatting in the JWT authentication middleware and authentication views for consistency. The test infrastructure is enhanced with authenticated test base classes and fixtures, and error message assertions are updated to match revised API responses.

Changes

File(s) Change Summary
.github/workflows/test.yml Added environment variables for test jobs; minor redundant Python version line.
todo/middlewares/jwt_auth.py Refactored error response construction for consistency and readability; no logic changes.
todo/views/auth.py Reformatted Response object construction for compactness and consistency; no logic or data changes.
todo/tests/fixtures/user.py Added new fixture with sample user data for tests.
todo/tests/integration/test_task_detail_api.py
todo/tests/integration/test_tasks_delete.py
Introduced AuthenticatedMongoTestCase base class to set up authenticated test clients; updated test classes to inherit from it and removed redundant client instantiation.
todo/tests/unit/middlewares/init.py Added empty init.py to mark the middlewares test directory as a package.
todo/tests/unit/middlewares/test_jwt_auth.py Added comprehensive unit tests for JWT authentication middleware and related utility functions.
todo/tests/unit/models/test_user.py Added unit tests for UserModel covering instantiation, validation, and default timestamp behavior.
todo/tests/unit/repositories/test_user_repository.py Added unit tests for UserRepository covering get_by_id, create_or_update, and error scenarios.
todo/tests/unit/services/test_google_oauth_service.py Added unit tests for GoogleOAuthService covering OAuth flow, token exchange, user info retrieval, and error handling.
todo/tests/unit/services/test_user_service.py Added unit tests for UserService covering user creation, update, retrieval, and validation logic.
todo/tests/unit/views/test_auth.py Added unit tests for all Google OAuth-related views, covering login, callback, status, refresh, and logout flows.
todo/tests/unit/views/test_task.py Refactored to introduce AuthenticatedTestCase for setting up authentication; updated error message assertions to match new API responses; updated all test classes to use the new base class.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant APIClient
    participant JWTAuthMiddleware
    participant GoogleOAuthService
    participant UserRepository
    participant TaskAPI

    User->>APIClient: Sends request to protected endpoint
    APIClient->>JWTAuthMiddleware: Forwards request
    alt Valid Google or RDS Token
        JWTAuthMiddleware->>UserRepository: Validate user/token
        UserRepository-->>JWTAuthMiddleware: User details
        JWTAuthMiddleware->>TaskAPI: Pass authenticated request
        TaskAPI-->>User: Returns data
    else Invalid or Missing Token
        JWTAuthMiddleware-->>User: Returns 401 Unauthorized with error response
    end
Loading

Assessment against linked issues

Objective Addressed Explanation
Implement authentication using credentials stored in RDS and Google OAuth (#26)
Issue secure JWT token on successful login (#26)
Unauthenticated users receive 401 Unauthorized on protected routes (#26)
Add comprehensive tests for authentication, user, and task APIs as required for robust authentication (#26)

Assessment against linked issues: Out-of-scope changes

No out-of-scope changes found.

Possibly related PRs

Suggested reviewers

  • iamitprakash

Poem

In the warren of code, a rabbit hops in,
With tokens and cookies, let testing begin!
Auth flows are guarded, responses refined,
New tests ensure bugs are left far behind.
🐇 With every hop, our backend grows strong,
Secure and robust—now hop along!


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 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.
    • Explain this complex logic.
    • 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 explain this code block.
    • @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 explain its main purpose.
    • @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.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

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 generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @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.

Copy link

@korbit-ai korbit-ai bot left a comment

Choose a reason for hiding this comment

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

I've completed my review and didn't find any issues... but I did find this squirrel.

         _.-"""-,
       .'  ..::. `\
      /  .::' `'` /
     / .::' .--.=;
     | ::' /  C ..\
     | :: |   \  _.)
      \ ':|   /  \
       '-, \./ \)\)
          `-|   );/
            '--'-'
Files scanned
File Path Reviewed
todo/middlewares/jwt_auth.py
todo/views/auth.py

Explore our documentation to understand the languages and file types we support and the files we ignore.

Check out our docs on how you can make Korbit work best for you and your team.

Loving Korbit!? Share us on LinkedIn Reddit and X

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: 17

🔭 Outside diff range comments (5)
todo/views/auth.py (2)

375-379: Cookie deletion omits domain / samesite – cookies may survive logout

set_cookie() uses the configured domain, samesite and secure flags, but the matching delete_cookie() calls only specify path="/".
If COOKIE_DOMAIN (or other attributes) is set, browsers will keep the cookies, resulting in a silent auth cache.

-        response.delete_cookie("ext-access", path="/")
-        response.delete_cookie("ext-refresh", path="/")
-        response.delete_cookie(settings.SESSION_COOKIE_NAME, path="/")
+        cookie_cfg = self._get_cookie_config()
+        response.delete_cookie("ext-access", **cookie_cfg)
+        response.delete_cookie("ext-refresh", **cookie_cfg)
+        response.delete_cookie(settings.SESSION_COOKIE_NAME, **cookie_cfg)

150-158: Repeated _get_cookie_config() logic – consider centralising

The same helper appears three times across the view classes. A small utility (e.g. todo.utils.cookie_utils.get_google_cookie_config()) would remove duplication and ensure future tweaks (like SameSite changes) are applied consistently.

Also applies to: 338-344, 382-389

todo/tests/unit/views/test_task.py (2)

77-96: Order-sensitive loop can cause flaky assertion

zip(response_data["errors"], expected_response["errors"]) assumes the API returns errors in a specific order. If validation order changes the test will fail while the API is still correct.

Prefer set-based or mapping comparison:

-        for actual_error, expected_error in zip(response_data["errors"], expected_response["errors"]):
-            self.assertEqual(actual_error["source"]["parameter"], expected_error["source"]["parameter"])
-            self.assertEqual(actual_error["detail"], expected_error["detail"])
+        self.assertCountEqual(
+            [e["source"]["parameter"] for e in response_data["errors"]],
+            [e["source"]["parameter"] for e in expected_response["errors"]],
+        )
+        self.assertCountEqual(
+            [e["detail"] for e in response_data["errors"]],
+            [e["detail"] for e in expected_response["errors"]],
+        )

316-324: try/except hides real failures – test will pass even if the view crashes

Catching a broad Exception inside the test means an unexpected server error is treated as success.

-        try:
-            response = self.client.post(self.url, data=self.valid_payload, format="json")
-            self.assertEqual(response.status_code, status.HTTP_500_INTERNAL_SERVER_ERROR)
-            self.assertEqual(response.data["message"], ApiErrors.INTERNAL_SERVER_ERROR)
-        except Exception as e:
-            self.assertEqual(str(e), "Database exploded")
+        response = self.client.post(self.url, data=self.valid_payload, format="json")
+        self.assertEqual(response.status_code, status.HTTP_500_INTERNAL_SERVER_ERROR)
+        self.assertEqual(response.data["message"], ApiErrors.INTERNAL_SERVER_ERROR)

If the view really raises an unhandled exception Django’s test client will propagate it and the test will naturally fail, which is what we want.

todo/tests/unit/services/test_user_service.py (1)

67-88: Consider parametrising with pytest for brevity

The unittest subTest loop is fine, but converting the suite to pytest with @pytest.mark.parametrize would make the intent shorter and clearer.

♻️ Duplicate comments (3)
todo/middlewares/jwt_auth.py (2)

118-136: …and in the specialised handlers
Both private handlers repeat the single-error list pattern. Once the helper exists, these become:

return self._json_unauthorized(str(exception))

52-60: Reuse the helper suggested above here as well
Same structure / same comments as previous block.

todo/tests/unit/services/test_user_service.py (1)

45-52: Assertion should target the repository instance

Same instance-vs-class issue as above.

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 5bb4ec1 and 1689c8c.

📒 Files selected for processing (15)
  • .github/workflows/test.yml (2 hunks)
  • todo/middlewares/jwt_auth.py (3 hunks)
  • todo/tests/fixtures/user.py (1 hunks)
  • todo/tests/integration/test_task_detail_api.py (1 hunks)
  • todo/tests/integration/test_tasks_delete.py (1 hunks)
  • todo/tests/unit/exceptions/test_exception_handler.py (2 hunks)
  • todo/tests/unit/middlewares/__init__.py (1 hunks)
  • todo/tests/unit/middlewares/test_jwt_auth.py (1 hunks)
  • todo/tests/unit/models/test_user.py (1 hunks)
  • todo/tests/unit/repositories/test_user_repository.py (1 hunks)
  • todo/tests/unit/services/test_google_oauth_service.py (1 hunks)
  • todo/tests/unit/services/test_user_service.py (1 hunks)
  • todo/tests/unit/views/test_auth.py (1 hunks)
  • todo/tests/unit/views/test_task.py (8 hunks)
  • todo/views/auth.py (5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
todo/middlewares/jwt_auth.py (2)
todo/dto/responses/error_response.py (2)
  • ApiErrorDetail (13-16)
  • ApiErrorResponse (19-23)
todo/constants/messages.py (2)
  • ApiErrors (19-42)
  • AuthErrorMessages (63-75)
todo/tests/unit/models/test_user.py (1)
todo/models/user.py (1)
  • UserModel (8-20)
todo/tests/unit/repositories/test_user_repository.py (5)
todo/repositories/user_repository.py (2)
  • UserRepository (12-56)
  • create_or_update (28-56)
todo/models/user.py (1)
  • UserModel (8-20)
todo/models/common/pyobjectid.py (1)
  • PyObjectId (4-15)
todo/exceptions/google_auth_exceptions.py (2)
  • GoogleUserNotFoundException (40-42)
  • GoogleAPIException (35-37)
todo/constants/messages.py (1)
  • RepositoryErrors (10-15)
todo/tests/unit/middlewares/test_jwt_auth.py (2)
todo/middlewares/jwt_auth.py (4)
  • JWTAuthenticationMiddleware (13-136)
  • is_google_user (139-140)
  • is_rds_user (143-144)
  • get_current_user_info (147-172)
todo/constants/messages.py (1)
  • AuthErrorMessages (63-75)
todo/views/auth.py (1)
todo/constants/messages.py (1)
  • AppMessages (2-6)
todo/tests/unit/views/test_task.py (4)
todo/utils/google_jwt_utils.py (1)
  • generate_google_token_pair (101-110)
todo/tests/integration/test_task_detail_api.py (3)
  • setUp (12-14)
  • setUp (29-37)
  • _setup_auth_cookies (16-25)
todo/tests/integration/test_tasks_delete.py (3)
  • setUp (12-14)
  • setUp (29-37)
  • _setup_auth_cookies (16-25)
todo/constants/messages.py (1)
  • ApiErrors (19-42)
🪛 Checkov (3.2.334)
.github/workflows/test.yml

[LOW] 15-16: Base64 High Entropy String

(CKV_SECRET_6)

🪛 actionlint (1.7.7)
.github/workflows/test.yml

32-32: key "python-version" is duplicated in "with" section. previously defined at line:31,col:11. note that this key is case insensitive

(syntax-check)

🪛 YAMLlint (1.37.1)
.github/workflows/test.yml

[error] 32-32: duplication of key "python-version" in mapping

(key-duplicates)

🪛 Pylint (3.3.7)
todo/tests/unit/services/test_user_service.py

[convention] 14-14: Line too long (114/100)

(C0301)


[convention] 42-42: Line too long (114/100)

(C0301)


[convention] 1-1: Missing module docstring

(C0114)


[error] 3-3: Unable to import 'rest_framework.exceptions'

(E0401)


[convention] 12-12: Missing class docstring

(C0115)


[convention] 18-18: Missing function or method docstring

(C0116)


[convention] 27-27: Missing function or method docstring

(C0116)


[convention] 37-37: Missing function or method docstring

(C0116)


[convention] 45-45: Missing function or method docstring

(C0116)


[convention] 54-54: Missing function or method docstring

(C0116)


[convention] 61-61: Missing function or method docstring

(C0116)


[warning] 63-63: Access to a protected member _validate_google_user_data of a client class

(W0212)


[convention] 67-67: Missing function or method docstring

(C0116)


[warning] 77-77: Access to a protected member _validate_google_user_data of a client class

(W0212)

todo/tests/fixtures/user.py

[convention] 1-1: Missing module docstring

(C0114)

todo/middlewares/jwt_auth.py

[convention] 41-41: Line too long (119/100)

(C0301)


[convention] 60-60: Line too long (115/100)

(C0301)


[convention] 122-122: Line too long (109/100)

(C0301)


[convention] 125-125: Line too long (111/100)

(C0301)


[convention] 132-132: Line too long (109/100)

(C0301)


[convention] 135-135: Line too long (111/100)

(C0301)

todo/tests/integration/test_task_detail_api.py

[error] 3-3: Unable to import 'bson'

(E0401)


[convention] 11-11: Missing class docstring

(C0115)


[convention] 28-28: Missing class docstring

(C0115)

todo/tests/unit/models/test_user.py

[convention] 1-1: Missing module docstring

(C0114)


[error] 3-3: Unable to import 'pydantic_core._pydantic_core'

(E0401)


[convention] 8-8: Missing class docstring

(C0115)


[convention] 12-12: Missing function or method docstring

(C0116)


[convention] 21-21: Missing function or method docstring

(C0116)


[convention] 35-35: Missing function or method docstring

(C0116)


[convention] 45-45: Missing function or method docstring

(C0116)

todo/tests/integration/test_tasks_delete.py

[error] 3-3: Unable to import 'bson'

(E0401)


[convention] 11-11: Missing class docstring

(C0115)


[convention] 28-28: Missing class docstring

(C0115)

todo/tests/unit/services/test_google_oauth_service.py

[convention] 21-21: Line too long (104/100)

(C0301)


[convention] 128-128: Line too long (103/100)

(C0301)


[convention] 1-1: Missing module docstring

(C0114)


[convention] 10-10: Missing class docstring

(C0115)


[convention] 25-25: Missing function or method docstring

(C0116)


[convention] 45-45: Missing function or method docstring

(C0116)


[convention] 55-55: Missing function or method docstring

(C0116)


[convention] 68-68: Missing function or method docstring

(C0116)


[convention] 77-77: Missing function or method docstring

(C0116)


[warning] 84-84: Access to a protected member _exchange_code_for_tokens of a client class

(W0212)


[convention] 95-95: Missing function or method docstring

(C0116)


[warning] 102-102: Access to a protected member _exchange_code_for_tokens of a client class

(W0212)


[convention] 106-106: Missing function or method docstring

(C0116)


[warning] 112-112: Access to a protected member _get_user_info of a client class

(W0212)


[convention] 120-120: Missing function or method docstring

(C0116)


[warning] 127-127: Access to a protected member _get_user_info of a client class

(W0212)


[convention] 131-131: Missing function or method docstring

(C0116)


[warning] 137-137: Access to a protected member _get_user_info of a client class

(W0212)

todo/tests/unit/repositories/test_user_repository.py

[convention] 15-15: Line too long (107/100)

(C0301)


[convention] 80-80: Line too long (114/100)

(C0301)


[convention] 1-1: Missing module docstring

(C0114)


[error] 3-3: Unable to import 'bson'

(E0401)


[convention] 13-13: Missing class docstring

(C0115)


[convention] 22-22: Missing function or method docstring

(C0116)


[convention] 34-34: Missing function or method docstring

(C0116)


[convention] 43-43: Missing function or method docstring

(C0116)


[convention] 52-52: Missing function or method docstring

(C0116)


[convention] 65-65: Missing function or method docstring

(C0116)


[convention] 74-74: Missing function or method docstring

(C0116)


[convention] 83-83: Missing function or method docstring

(C0116)

todo/tests/unit/middlewares/test_jwt_auth.py

[convention] 8-8: Line too long (117/100)

(C0301)


[convention] 1-1: Missing module docstring

(C0114)


[error] 3-3: Unable to import 'django.http'

(E0401)


[error] 4-4: Unable to import 'django.conf'

(E0401)


[error] 5-5: Unable to import 'rest_framework'

(E0401)


[convention] 12-12: Missing class docstring

(C0115)


[convention] 77-77: Missing class docstring

(C0115)


[convention] 6-6: standard import "json" should be placed before third party imports "django.http.HttpRequest", "django.conf.settings", "rest_framework.status"

(C0411)

todo/views/auth.py

[convention] 105-105: Line too long (106/100)

(C0301)


[convention] 325-325: Line too long (117/100)

(C0301)

todo/tests/unit/views/test_auth.py

[convention] 87-87: Line too long (108/100)

(C0301)


[convention] 1-1: Missing module docstring

(C0114)


[error] 1-1: Unable to import 'rest_framework.test'

(E0401)


[error] 2-2: Unable to import 'rest_framework.reverse'

(E0401)


[error] 3-3: Unable to import 'rest_framework'

(E0401)


[error] 5-5: Unable to import 'bson.objectid'

(E0401)


[convention] 17-17: Missing class docstring

(C0115)


[convention] 18-18: Missing function or method docstring

(C0116)


[convention] 18-18: Method name "setUp" doesn't conform to snake_case naming style

(C0103)


[convention] 24-24: Missing function or method docstring

(C0116)


[convention] 36-36: Missing function or method docstring

(C0116)


[convention] 49-49: Missing function or method docstring

(C0116)


[convention] 62-62: Missing class docstring

(C0115)


[convention] 63-63: Missing function or method docstring

(C0116)


[convention] 63-63: Method name "setUp" doesn't conform to snake_case naming style

(C0103)


[convention] 70-70: Missing function or method docstring

(C0116)


[convention] 80-80: Missing function or method docstring

(C0116)


[convention] 89-89: Missing function or method docstring

(C0116)


[convention] 101-101: Missing function or method docstring

(C0116)


[convention] 134-134: Missing class docstring

(C0115)


[convention] 135-135: Missing function or method docstring

(C0116)


[convention] 135-135: Method name "setUp" doesn't conform to snake_case naming style

(C0103)


[convention] 140-140: Missing function or method docstring

(C0116)


[convention] 150-150: Missing function or method docstring

(C0116)


[convention] 181-181: Missing class docstring

(C0115)


[convention] 182-182: Missing function or method docstring

(C0116)


[convention] 182-182: Method name "setUp" doesn't conform to snake_case naming style

(C0103)


[convention] 187-187: Missing function or method docstring

(C0116)


[convention] 196-196: Missing function or method docstring

(C0116)


[convention] 216-216: Missing class docstring

(C0115)


[convention] 217-217: Missing function or method docstring

(C0116)


[convention] 217-217: Method name "setUp" doesn't conform to snake_case naming style

(C0103)


[convention] 222-222: Missing function or method docstring

(C0116)


[convention] 241-241: Missing function or method docstring

(C0116)


[convention] 253-253: Missing function or method docstring

(C0116)


[convention] 4-4: standard import "unittest.mock.patch" should be placed before third party imports "rest_framework.test.APISimpleTestCase", "rest_framework.reverse.reverse", "rest_framework.status"

(C0411)

todo/tests/unit/views/test_task.py

[convention] 26-26: Missing class docstring

(C0115)


[convention] 27-27: Missing function or method docstring

(C0116)


[convention] 27-27: Method name "setUp" doesn't conform to snake_case naming style

(C0103)


[refactor] 26-26: Too few public methods (1/2)

(R0903)


[convention] 45-45: Missing class docstring

(C0115)


[convention] 23-23: Imports from package todo are not grouped

(C0412)


[convention] 216-216: Missing class docstring

(C0115)


[warning] 323-323: Catching too general exception Exception

(W0718)


[convention] 327-327: Missing class docstring

(C0115)


[convention] 357-357: Missing class docstring

(C0115)

🔇 Additional comments (2)
todo/tests/unit/middlewares/__init__.py (1)

1-1: Package marker looks good
Adding an empty __init__.py is the correct minimal change for test discovery.

todo/tests/integration/test_task_detail_api.py (1)

3-3: Ensure bson is available in CI
actionlint flagged the import. Verify that pymongo (which provides bson) is pinned in requirements.txt; otherwise tests will error in the workflow.

@yesyash yesyash changed the title Test for Authentication in Todo app test: adds test cases for authentication Jun 17, 2025
self.request.headers = {}
self.request.COOKIES = {}
self._original_public_paths = settings.PUBLIC_PATHS
settings.PUBLIC_PATHS = ["/v1/auth/google/login"]

Choose a reason for hiding this comment

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

This works, but the best practice is to use Django's reverse() utility. Hardcoding URL paths makes our tests brittle. If we ever decide to refactor our API routes (e.g., change /v1/ to /v2/), we would have to manually find and replace these strings in every single test file, which is error-prone.

@iamitprakash iamitprakash merged commit 3270fb2 into develop Jun 17, 2025
3 checks passed
@iamitprakash
Copy link
Member

as github is down, I am merging this PR

@prakashchoudhary07 prakashchoudhary07 deleted the auth-final branch June 17, 2025 19:54
Comment on lines +12 to +19
def test_user_model_instantiates_with_valid_data(self):
user = UserModel(**self.valid_user_data)

self.assertEqual(user.google_id, self.valid_user_data["google_id"])
self.assertEqual(user.email_id, self.valid_user_data["email_id"])
self.assertEqual(user.name, self.valid_user_data["name"])
self.assertEqual(user.created_at, self.valid_user_data["created_at"])
self.assertEqual(user.updated_at, self.valid_user_data["updated_at"])
Copy link
Contributor

Choose a reason for hiding this comment

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

  • I don't think we need to check class instantiation here. When we use the methods or properties of the class, we will know if the value was set correctly or not. Please make this change in the next pr.

Comment on lines +45 to +55
def test_user_model_sets_default_timestamps(self):
minimal_data = {
"google_id": self.valid_user_data["google_id"],
"email_id": self.valid_user_data["email_id"],
"name": self.valid_user_data["name"],
}
user = UserModel(**minimal_data)

self.assertIsInstance(user.created_at, datetime)
self.assertIsNone(user.updated_at)
self.assertLessEqual(user.created_at, datetime.now(timezone.utc))
Copy link
Contributor

Choose a reason for hiding this comment

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

  • i do not think we need to write test for default time stamp, please make this change in the next pr.


class UserRepositoryTests(TestCase):
def setUp(self) -> None:
self.valid_user_data = {"google_id": "123456789", "email": "[email protected]", "name": "Test User"}
Copy link
Contributor

Choose a reason for hiding this comment

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

  • let's get user data from users_db_data like we're doing in models/test_user.py.
self.valid_user_data = users_db_data[0]

Comment on lines +42 to +49
@patch("todo.repositories.user_repository.DatabaseManager")
def test_get_by_id_database_error(self, mock_db_manager):
mock_db_manager.return_value = self.mock_db_manager
user_id = str(ObjectId())
self.mock_collection.find_one.side_effect = Exception("Database error")

with self.assertRaises(GoogleUserNotFoundException):
UserRepository.get_by_id(user_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

  • as discussed we do not need to check this, as when getting a user by id there can be two cases, either we get the user or we do not and we are checking for both above.

Comment on lines +73 to +80
@patch("todo.repositories.user_repository.DatabaseManager")
def test_create_or_update_database_error(self, mock_db_manager):
mock_db_manager.return_value = self.mock_db_manager
self.mock_collection.find_one_and_update.side_effect = Exception("Database error")

with self.assertRaises(GoogleAPIException) as context:
UserRepository.create_or_update(self.valid_user_data)
self.assertIn(RepositoryErrors.USER_CREATE_UPDATE_FAILED.format("Database error"), str(context.exception))
Copy link
Contributor

Choose a reason for hiding this comment

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

  • same as above, please remove this.

Comment on lines +82 to +94
@patch("todo.repositories.user_repository.DatabaseManager")
def test_create_or_update_sets_timestamps(self, mock_db_manager):
mock_db_manager.return_value = self.mock_db_manager
self.mock_collection.find_one_and_update.return_value = users_db_data[0]

UserRepository.create_or_update(self.valid_user_data)

call_args = self.mock_collection.find_one_and_update.call_args[0]
update_doc = call_args[1]
self.assertIn("$set", update_doc)
self.assertIn("updated_at", update_doc["$set"])
self.assertIn("$setOnInsert", update_doc)
self.assertIn("created_at", update_doc["$setOnInsert"])
Copy link
Contributor

Choose a reason for hiding this comment

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

  • we can move the logic to check updated_at in the test_create_or_update_success test case.

"SCOPES": ["email", "profile"],
}
}
self.valid_user_info = {"id": "123456789", "email": "[email protected]", "name": "Test User"}
Copy link
Contributor

Choose a reason for hiding this comment

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

  • should we pick this from fixtures?

Comment on lines +119 to +131
@patch("todo.services.google_oauth_service.requests.get")
def test_get_user_info_missing_fields(self, mock_get):
mock_response = MagicMock()
mock_response.status_code = 200
mock_response.json.return_value = {"id": "123"}
mock_get.return_value = mock_response

with self.assertRaises(GoogleAPIException) as context:
GoogleOAuthService._get_user_info("test-token")
error_msg = str(context.exception)
self.assertIn(ApiErrors.MISSING_USER_INFO_FIELDS.split(":")[0], error_msg)
for field in ("email", "name"):
self.assertIn(field, error_msg)
Copy link
Contributor

Choose a reason for hiding this comment

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

  • please remove this

Comment on lines +12 to +19
self.mock_settings = {
"GOOGLE_OAUTH": {
"CLIENT_ID": "test-client-id",
"CLIENT_SECRET": "test-client-secret",
"REDIRECT_URI": "http://localhost:3000/auth/callback",
"SCOPES": ["email", "profile"],
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

  • pick this from fixtures


class UserServiceTests(TestCase):
def setUp(self) -> None:
self.valid_google_user_data = {"google_id": "123456789", "email": "[email protected]", "name": "Test User"}
Copy link
Contributor

Choose a reason for hiding this comment

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

  • please pick this from fixtures.

@yesyash
Copy link
Contributor

yesyash commented Jun 17, 2025

@VaibhavSingh8 please address the comments above in the next pr, also add a issue ticket for the next pr you will be raising here.

@VaibhavSingh8
Copy link
Contributor Author

VaibhavSingh8 commented Jun 18, 2025

@yesyash , I've created the issue ticket as asked: #85

Will incorporate the changes in the upcoming PR.

@VaibhavSingh8
Copy link
Contributor Author

@yesyash @Achintya-Chatterjee , addressed your comments in this PR #90 . Please review.

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.

Implement RDS and Google Authentication for Todo Backend (DRF)
4 participants