Improved testing for backend/app/mentorship #3179
Conversation
WalkthroughAdds an Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (10)
backend/apps/github/models/__init__.py (1)
3-9: Consider maintaining alphabetical order for imports.The new
Issueimport breaks the alphabetical ordering of the existing imports. For consistency:🔎 Proposed fix
from .comment import Comment from .commit import Commit +from .issue import Issue from .label import Label from .milestone import Milestone from .pull_request import PullRequest from .user import User -from .issue import Issuebackend/tests/apps/mentorship/model/test_task_level.py (1)
17-26: Second test is redundant and has a misleading name.This test is identical in behavior to the first test. Setting
module.__str__.return_value = "ignored-str"has no effect becauseTaskLevel.__str__accessesself.module.namedirectly, notstr(self.module). The test name suggests it verifies special handling of custom__str__, but there's no such logic in the implementation.Consider either removing this test or renaming it to clarify what scenario it actually covers.
backend/tests/apps/mentorship/management/commands/test_mentorship_update_comments.py (1)
10-20: Consider extracting shared test utilities to a conftest.py.The
make_qshelper function is duplicated intest_mentorship_sync_module_issues.pyandtest_mentorship_sync_issue_levels.py. For DRY, consider moving it to a sharedconftest.pyin thebackend/tests/apps/mentorship/directory.backend/tests/apps/mentorship/model/test_start_end_range.py (2)
2-2: Remove unused import.The
patchimport is not used in this test file.🔎 Proposed fix
-from unittest.mock import MagicMock, patch +from unittest.mock import MagicMock
8-18: Test doesn't validate meaningful behavior.This test merely assigns attributes to a mock and reads them back, which doesn't actually verify StartEndRange model behavior. Since StartEndRange is an abstract model with only field definitions, consider either removing this test or converting it to an integration test that validates field behavior in a concrete model context.
Additional cleanup
Line 15 is redundant and can be removed:
mock_instance = MagicMock(spec=StartEndRange) mock_instance.started_at = start_time mock_instance.ended_at = end_time - instance = mock_instance - assert instance.started_at == start_time - assert instance.ended_at == end_time + assert mock_instance.started_at == start_time + assert mock_instance.ended_at == end_timebackend/tests/apps/mentorship/model/test_mentor_module.py (1)
3-4: Consider consolidating imports.The two imports from
apps.mentorship.modelscan be combined into a single line.🔎 Proposed fix
-from apps.mentorship.models import MentorModule -from apps.mentorship.models import Mentor, Module +from apps.mentorship.models import Mentor, MentorModule, Modulebackend/tests/apps/mentorship/model/test_issue_user_interest.py (2)
13-43: Consider integration tests in addition to mocked unit tests.While mocking
objects.createis valid for unit testing, it doesn't actually validate model behavior, database constraints, or Django ORM interactions. The test verifies that the mock was called correctly, not that the model works correctly.For comprehensive coverage, consider adding integration tests using pytest-django's database fixtures that create actual model instances.
Example integration test approach
@pytest.mark.django_db def test_create_issue_user_interest_integration(self): # Create real instances and test actual database behavior user = User.objects.create(login="testuser") issue = Issue.objects.create(title="Test Issue") module = Module.objects.create(name="Test Module") interest = IssueUserInterest.objects.create( user=user, issue=issue, module=module, ) assert interest.user == user assert interest.issue == issue assert interest.module == module assert "testuser" in str(interest)
46-68: Use Django's IntegrityError for more realistic testing.The test defines a custom
MockIntegrityErrorinstead of using Django's actualIntegrityError. This makes the test less realistic and doesn't verify that Django would actually raise the correct exception type.🔎 Proposed fix
+from django.db import IntegrityError + @patch("apps.mentorship.models.IssueUserInterest.objects.create") def test_unique_together_constraint(self, mock_create): - class MockIntegrityError(Exception): - pass - mock_create.side_effect = [ MagicMock(), - MockIntegrityError("Mock Integrity Error"), + IntegrityError("UNIQUE constraint failed"), ] IssueUserInterest.objects.create( user=self.user, issue=self.issue, module=self.module, ) - with pytest.raises(MockIntegrityError): + with pytest.raises(IntegrityError): IssueUserInterest.objects.create( user=self.user, issue=self.issue, module=self.module, ) assert mock_create.call_count == 2backend/tests/apps/mentorship/model/test_program.py (1)
6-10: Remove unused code.
MockIntegrityErroris defined but never used in this test file. The same applies to theMentorimport on line 9.🔎 Proposed cleanup
-class MockIntegrityError(Exception): - pass - -from apps.mentorship.models import Program, Mentor +from apps.mentorship.models import Program from apps.github.models import User as GithubUserbackend/tests/apps/mentorship/model/test_module.py (1)
6-8: Remove unused imports.
PublishedModuleManagerandIssueare imported but never used in this test file.🔎 Proposed cleanup
from apps.mentorship.models import Module -from apps.mentorship.models.managers import PublishedModuleManager from apps.mentorship.models import Program, Mentor -from apps.github.models import Issue, User as GithubUser +from apps.github.models import User as GithubUser from apps.owasp.models import Project
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
backend/apps/github/models/__init__.pybackend/tests/apps/mentorship/management/commands/test_mentorship_sync_issue_levels.pybackend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.pybackend/tests/apps/mentorship/management/commands/test_mentorship_update_comments.pybackend/tests/apps/mentorship/model/test_experience_level.pybackend/tests/apps/mentorship/model/test_issue_user_interest.pybackend/tests/apps/mentorship/model/test_mentee.pybackend/tests/apps/mentorship/model/test_mentee_module.pybackend/tests/apps/mentorship/model/test_mentee_program.pybackend/tests/apps/mentorship/model/test_mentor.pybackend/tests/apps/mentorship/model/test_mentor_module.pybackend/tests/apps/mentorship/model/test_module.pybackend/tests/apps/mentorship/model/test_program.pybackend/tests/apps/mentorship/model/test_start_end_range.pybackend/tests/apps/mentorship/model/test_task.pybackend/tests/apps/mentorship/model/test_task_level.py
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-12-31T05:17:39.659Z
Learnt from: kart-u
Repo: OWASP/Nest PR: 3101
File: backend/apps/common/extensions.py:92-98
Timestamp: 2025-12-31T05:17:39.659Z
Learning: In this codebase, import OperationType for GraphQL operations from the graphql-core package rather than from strawberry. Use 'from graphql import OperationType'. Strawberry re-exports via graphql-core internally, so relying on strawberry's API may be brittle. Apply this rule to all Python files that deal with GraphQL operation types; ensure imports come from graphql (graphql-core) and not from strawberry packages. This improves compatibility and avoids coupling to strawberry's internals.
Applied to files:
backend/tests/apps/mentorship/model/test_program.pybackend/tests/apps/mentorship/model/test_task.pybackend/tests/apps/mentorship/model/test_mentee_program.pybackend/tests/apps/mentorship/model/test_mentor.pybackend/tests/apps/mentorship/model/test_issue_user_interest.pybackend/tests/apps/mentorship/model/test_task_level.pybackend/tests/apps/mentorship/model/test_mentee.pybackend/apps/github/models/__init__.pybackend/tests/apps/mentorship/model/test_module.pybackend/tests/apps/mentorship/model/test_mentee_module.pybackend/tests/apps/mentorship/model/test_experience_level.pybackend/tests/apps/mentorship/model/test_mentor_module.pybackend/tests/apps/mentorship/model/test_start_end_range.pybackend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.pybackend/tests/apps/mentorship/management/commands/test_mentorship_update_comments.pybackend/tests/apps/mentorship/management/commands/test_mentorship_sync_issue_levels.py
📚 Learning: 2026-01-01T17:48:23.963Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:41-47
Timestamp: 2026-01-01T17:48:23.963Z
Learning: In Django code, be aware that a QuerySet's boolean evaluation (e.g., if not queryset) runs a database query to determine emptiness. While it is technically valid to use the queryset in a boolean context, use queryset.exists() for existence checks to avoid unnecessary queries and improve performance. Applicable broadly to Python/Django files rather than just this specific path.
Applied to files:
backend/tests/apps/mentorship/model/test_program.pybackend/tests/apps/mentorship/model/test_task.pybackend/tests/apps/mentorship/model/test_mentee_program.pybackend/tests/apps/mentorship/model/test_mentor.pybackend/tests/apps/mentorship/model/test_issue_user_interest.pybackend/tests/apps/mentorship/model/test_task_level.pybackend/tests/apps/mentorship/model/test_mentee.pybackend/apps/github/models/__init__.pybackend/tests/apps/mentorship/model/test_module.pybackend/tests/apps/mentorship/model/test_mentee_module.pybackend/tests/apps/mentorship/model/test_experience_level.pybackend/tests/apps/mentorship/model/test_mentor_module.pybackend/tests/apps/mentorship/model/test_start_end_range.pybackend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.pybackend/tests/apps/mentorship/management/commands/test_mentorship_update_comments.pybackend/tests/apps/mentorship/management/commands/test_mentorship_sync_issue_levels.py
📚 Learning: 2025-08-17T11:55:55.990Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2000
File: backend/apps/mentorship/management/commands/sync_module_issues.py:109-145
Timestamp: 2025-08-17T11:55:55.990Z
Learning: In the OWASP/Nest mentorship system, tasks are designed to be assigned to only one assignee per issue, even if GitHub issues can have multiple assignees. The sync_module_issues command correctly uses issue.assignees.first() to create one task per issue for the first assignee only.
Applied to files:
backend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.py
📚 Learning: 2025-12-18T05:39:42.678Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:40-40
Timestamp: 2025-12-18T05:39:42.678Z
Learning: In Django management commands, prefer using self.stdout.write(...) over print(...) for user-facing stdout output. This aligns with Django conventions and improves testability. When emitting messages, consider using self.stdout.write and, for styled messages, use self.style.SUCCESS/ERROR as appropriate to maintain consistent command output formatting. Apply this guideline to all Python files within any project's management/commands directory.
Applied to files:
backend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.pybackend/tests/apps/mentorship/management/commands/test_mentorship_update_comments.pybackend/tests/apps/mentorship/management/commands/test_mentorship_sync_issue_levels.py
🧬 Code graph analysis (16)
backend/tests/apps/mentorship/model/test_program.py (1)
backend/apps/mentorship/models/program.py (1)
ProgramStatus(25-28)
backend/tests/apps/mentorship/model/test_task.py (1)
backend/apps/mentorship/models/task.py (1)
Task(10-75)
backend/tests/apps/mentorship/model/test_mentee_program.py (2)
backend/apps/mentorship/models/mentee.py (1)
Mentee(11-49)backend/apps/mentorship/models/mentee_program.py (1)
MenteeProgram(11-39)
backend/tests/apps/mentorship/model/test_mentor.py (2)
backend/apps/mentorship/models/mentor.py (1)
Mentor(11-40)backend/tests/apps/mentorship/model/test_mentee.py (1)
test_str_returns_github_login(8-14)
backend/tests/apps/mentorship/model/test_issue_user_interest.py (3)
backend/apps/mentorship/models/issue_user_interest.py (1)
IssueUserInterest(6-36)backend/tests/apps/mentorship/model/test_module.py (1)
MockIntegrityError(12-13)backend/tests/apps/mentorship/model/test_program.py (1)
MockIntegrityError(6-7)
backend/tests/apps/mentorship/model/test_task_level.py (1)
backend/apps/mentorship/models/task_level.py (1)
TaskLevel(8-61)
backend/tests/apps/mentorship/model/test_mentee.py (3)
backend/apps/mentorship/models/mentee.py (1)
Mentee(11-49)backend/tests/apps/mentorship/model/test_mentor.py (1)
test_str_returns_github_login(8-14)backend/apps/mentorship/api/internal/nodes/mentor.py (1)
login(23-25)
backend/apps/github/models/__init__.py (3)
backend/tests/apps/github/models/issue_test.py (1)
issue(26-28)backend/apps/github/models/issue.py (1)
Issue(19-235)frontend/src/types/issue.ts (1)
Issue(4-24)
backend/tests/apps/mentorship/model/test_module.py (4)
backend/apps/mentorship/models/managers/module.py (1)
PublishedModuleManager(8-13)backend/tests/apps/mentorship/model/test_issue_user_interest.py (1)
MockIntegrityError(47-48)backend/tests/apps/mentorship/model/test_program.py (1)
MockIntegrityError(6-7)backend/apps/mentorship/models/program.py (1)
ProgramStatus(25-28)
backend/tests/apps/mentorship/model/test_mentee_module.py (1)
backend/apps/mentorship/models/mentee_module.py (1)
MenteeModule(11-45)
backend/tests/apps/mentorship/model/test_experience_level.py (1)
backend/apps/mentorship/models/common/experience_level.py (2)
ExperienceLevel(6-25)ExperienceLevelChoices(12-18)
backend/tests/apps/mentorship/model/test_mentor_module.py (2)
backend/apps/mentorship/models/mentor_module.py (1)
MentorModule(11-42)backend/apps/mentorship/models/mentor.py (1)
Mentor(11-40)
backend/tests/apps/mentorship/model/test_start_end_range.py (1)
backend/apps/mentorship/models/common/start_end_range.py (1)
StartEndRange(6-13)
backend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.py (1)
backend/apps/mentorship/management/commands/mentorship_sync_module_issues.py (5)
_extract_repo_full_name(28-40)_get_status(42-50)_get_last_assigned_date(52-74)_build_repo_label_to_issue_map(76-92)_process_module(94-203)
backend/tests/apps/mentorship/management/commands/test_mentorship_update_comments.py (1)
backend/apps/mentorship/management/commands/mentorship_update_comments.py (3)
process_mentorship_modules(31-47)process_module(49-81)process_issue_interests(83-152)
backend/tests/apps/mentorship/management/commands/test_mentorship_sync_issue_levels.py (1)
backend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.py (2)
command(22-30)make_qs(12-18)
🪛 Ruff (0.14.10)
backend/tests/apps/mentorship/model/test_program.py
1-1: File backend/tests/apps/mentorship/model/test_program.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
9-9: Module level import not at top of file
(E402)
10-10: Module level import not at top of file
(E402)
16-16: Unused lambda argument: self
(ARG005)
23-23: datetime.datetime() called without a tzinfo argument
(DTZ001)
24-24: datetime.datetime() called without a tzinfo argument
(DTZ001)
36-36: datetime.datetime() called without a tzinfo argument
(DTZ001)
37-37: datetime.datetime() called without a tzinfo argument
(DTZ001)
47-47: datetime.datetime() called without a tzinfo argument
(DTZ001)
48-48: datetime.datetime() called without a tzinfo argument
(DTZ001)
58-58: datetime.datetime() called without a tzinfo argument
(DTZ001)
59-59: datetime.datetime() called without a tzinfo argument
(DTZ001)
74-74: No newline at end of file
Add trailing newline
(W292)
backend/tests/apps/mentorship/model/test_task.py
1-1: File backend/tests/apps/mentorship/model/test_task.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
backend/tests/apps/mentorship/model/test_mentee_program.py
1-1: File backend/tests/apps/mentorship/model/test_mentee_program.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
19-19: Unused lambda argument: self
(ARG005)
25-25: Unused lambda argument: self
(ARG005)
30-30: datetime.datetime() called without a tzinfo argument
(DTZ001)
31-31: datetime.datetime() called without a tzinfo argument
(DTZ001)
75-75: datetime.datetime() called without a tzinfo argument
(DTZ001)
82-82: datetime.datetime() called without a tzinfo argument
(DTZ001)
backend/tests/apps/mentorship/model/test_mentor.py
1-1: File backend/tests/apps/mentorship/model/test_mentor.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
backend/tests/apps/mentorship/model/test_issue_user_interest.py
1-1: File backend/tests/apps/mentorship/model/test_issue_user_interest.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
backend/tests/apps/mentorship/model/test_task_level.py
1-1: File backend/tests/apps/mentorship/model/test_task_level.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
backend/tests/apps/mentorship/model/test_mentee.py
1-1: File backend/tests/apps/mentorship/model/test_mentee.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
backend/tests/apps/mentorship/model/test_module.py
1-1: File backend/tests/apps/mentorship/model/test_module.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
23-23: datetime.datetime() called without a tzinfo argument
(DTZ001)
24-24: datetime.datetime() called without a tzinfo argument
(DTZ001)
48-48: datetime.datetime() called without a tzinfo argument
(DTZ001)
49-49: datetime.datetime() called without a tzinfo argument
(DTZ001)
64-64: datetime.datetime() called without a tzinfo argument
(DTZ001)
65-65: datetime.datetime() called without a tzinfo argument
(DTZ001)
78-78: datetime.datetime() called without a tzinfo argument
(DTZ001)
79-79: datetime.datetime() called without a tzinfo argument
(DTZ001)
92-95: 1 blank line required between summary line and description
(D205)
92-95: Multi-line docstring summary should start at the first line
Remove whitespace after opening quotes
(D212)
107-111: 1 blank line required between summary line and description
(D205)
107-111: Multi-line docstring summary should start at the first line
Remove whitespace after opening quotes
(D212)
116-116: datetime.datetime() called without a tzinfo argument
(DTZ001)
117-117: datetime.datetime() called without a tzinfo argument
(DTZ001)
127-127: Unused function argument: args
(ARG001)
127-127: Unused function argument: kwargs
(ARG001)
141-141: datetime.datetime() called without a tzinfo argument
(DTZ001)
142-142: datetime.datetime() called without a tzinfo argument
(DTZ001)
144-144: datetime.datetime() called without a tzinfo argument
(DTZ001)
145-145: datetime.datetime() called without a tzinfo argument
(DTZ001)
156-156: No newline at end of file
Add trailing newline
(W292)
backend/tests/apps/mentorship/model/test_mentee_module.py
1-1: File backend/tests/apps/mentorship/model/test_mentee_module.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
16-16: Unused lambda argument: self
(ARG005)
18-18: Unused lambda argument: self
(ARG005)
22-22: datetime.datetime() called without a tzinfo argument
(DTZ001)
23-23: datetime.datetime() called without a tzinfo argument
(DTZ001)
62-62: datetime.datetime() called without a tzinfo argument
(DTZ001)
68-68: datetime.datetime() called without a tzinfo argument
(DTZ001)
backend/tests/apps/mentorship/model/test_experience_level.py
1-1: File backend/tests/apps/mentorship/model/test_experience_level.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
backend/tests/apps/mentorship/model/test_mentor_module.py
1-1: File backend/tests/apps/mentorship/model/test_mentor_module.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
backend/tests/apps/mentorship/model/test_start_end_range.py
1-1: File backend/tests/apps/mentorship/model/test_start_end_range.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
9-9: datetime.datetime() called without a tzinfo argument
(DTZ001)
10-10: datetime.datetime() called without a tzinfo argument
(DTZ001)
backend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.py
1-1: File backend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
12-12: Boolean default positional argument in function definition
(FBT002)
40-40: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
56-56: Multiple statements on one line (semicolon)
(E702)
69-69: Use datetime.UTC alias
Convert to datetime.UTC alias
(UP017)
70-70: Use datetime.UTC alias
Convert to datetime.UTC alias
(UP017)
71-71: Use datetime.UTC alias
Convert to datetime.UTC alias
(UP017)
72-72: Use datetime.UTC alias
Convert to datetime.UTC alias
(UP017)
76-76: Use datetime.UTC alias
Convert to datetime.UTC alias
(UP017)
97-97: Variable MockIssue in function should be lowercase
(N806)
115-115: Argument name MockIssue should be lowercase
(N803)
115-115: Argument name MockTask should be lowercase
(N803)
116-116: Multiple statements on one line (semicolon)
(E702)
116-116: Multiple statements on one line (semicolon)
(E702)
121-121: Multiple statements on one line (semicolon)
(E702)
121-121: Multiple statements on one line (semicolon)
(E702)
128-128: Multiple statements on one line (semicolon)
(E702)
152-152: Local variable num_linked is assigned to but never used
Remove assignment to unused variable num_linked
(F841)
185-185: Unused function argument: command
(ARG001)
185-185: Test function parameter mock_module has default argument
Remove default argument
(PT028)
185-185: Unused function argument: mock_repository
(ARG001)
185-185: Test function parameter mock_repository has default argument
Remove default argument
(PT028)
186-186: Multiple statements on one line (semicolon)
(E702)
186-186: Multiple statements on one line (semicolon)
(E702)
193-193: Variable MockTask in function should be lowercase
(N806)
backend/tests/apps/mentorship/management/commands/test_mentorship_update_comments.py
1-1: File backend/tests/apps/mentorship/management/commands/test_mentorship_update_comments.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
10-10: Boolean default positional argument in function definition
(FBT002)
73-73: Argument name MockModule should be lowercase
(N803)
81-81: Argument name MockModule should be lowercase
(N803)
93-93: Argument name MockIssue should be lowercase
(N803)
116-116: Argument name MockIssueUserInterest should be lowercase
(N803)
145-145: Argument name MockIssueUserInterest should be lowercase
(N803)
174-174: Argument name MockIssueUserInterest should be lowercase
(N803)
210-210: Argument name MockIssueUserInterest should be lowercase
(N803)
236-236: Argument name MockIssueUserInterest should be lowercase
(N803)
backend/tests/apps/mentorship/management/commands/test_mentorship_sync_issue_levels.py
1-1: File backend/tests/apps/mentorship/management/commands/test_mentorship_sync_issue_levels.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
17-17: Boolean default positional argument in function definition
(FBT002)
18-18: First line of docstring should be in imperative mood: "Helper: return a queryset-like MagicMock that is iterable and supports .exists()."
(D401)
27-27: Argument name MockTaskLevel should be lowercase
(N803)
28-28: No blank lines allowed after function docstring (found 1)
Remove blank line(s) after function docstring
(D202)
39-39: Argument name MockTaskLevel should be lowercase
(N803)
39-39: Argument name MockIssue should be lowercase
(N803)
40-40: No blank lines allowed after function docstring (found 1)
Remove blank line(s) after function docstring
(D202)
52-52: Multiple statements on one line (semicolon)
(E702)
53-53: Multiple statements on one line (semicolon)
(E702)
54-54: Multiple statements on one line (semicolon)
(E702)
97-97: Argument name MockTaskLevel should be lowercase
(N803)
97-97: Argument name MockIssue should be lowercase
(N803)
106-106: Multiple statements on one line (semicolon)
(E702)
🔇 Additional comments (15)
backend/tests/apps/mentorship/model/test_experience_level.py (1)
1-9: LGTM!The test correctly validates the string values of the
ExperienceLevelChoicesenum members. The assertions align with the source definition inexperience_level.py.backend/tests/apps/mentorship/management/commands/test_mentorship_update_comments.py (4)
89-112: LGTM!The
test_process_moduletest correctly verifies the orchestration of issue syncing and interest processing, with appropriate mocking of dependencies.
115-141: LGTM!The test correctly validates that new interests are created when a user comments with
/interested, with proper verification of the bulk_create call and stdout output.
144-169: LGTM!The test correctly verifies interest removal when a user's comment doesn't match interest patterns, including proper verification of the delete call and warning message.
235-280: LGTM!The multi-user test comprehensively validates mixed scenarios where some users express interest (user2, user3) while an existing interest is removed (user1). The assertions correctly verify the created and removed interests.
backend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.py (3)
39-52: LGTM!Good use of
pytest.mark.parametrizeto test multiple URL formats and edge cases for repository name extraction. The test cases cover valid GitHub URLs, invalid hosts, malformed URLs, and null input.
64-86: LGTM!Comprehensive test for
_get_last_assigned_datecovering:
- Finding the latest assigned event for a specific user
- Not finding events for the target user
- Handling
GithubExceptionwith stderr output
113-182: LGTM!The
test_process_module_links_and_creates_taskstest thoroughly validates:
- Linking issues to modules via
module.issues.set()- Creating tasks with correct status mapping (IN_PROGRESS for open+assigned, COMPLETED for closed)
- Appropriate stdout success messages
backend/tests/apps/mentorship/management/commands/test_mentorship_sync_issue_levels.py (3)
26-34: LGTM!Test correctly verifies early exit behavior when no TaskLevel objects exist, with appropriate warning message.
37-92: LGTM!Comprehensive test for issue level updates:
- Correctly sets up multiple issues with different label/level combinations
- Validates that issues are updated to matching levels
- Verifies
bulk_updateis called with the correct subset of changed issues- Confirms success message output
95-119: LGTM!Test correctly verifies that
bulk_updateis not called when all issues already have the correct level, with appropriate "up-to-date" message.backend/tests/apps/mentorship/model/test_mentee.py (1)
1-14: LGTM!The test correctly validates
Mentee.__str__returns the GitHub login. The approach is consistent with the existingtest_mentor.pytest pattern.backend/tests/apps/mentorship/model/test_mentor.py (1)
7-14: LGTM!The test correctly validates the Mentor.str implementation using appropriate mocking. The test is focused and verifies the expected string representation.
backend/tests/apps/mentorship/model/test_mentor_module.py (1)
7-19: LGTM!The test properly validates the MentorModule.str implementation by mocking the mentor and module components and verifying the expected format.
backend/tests/apps/mentorship/model/test_task.py (1)
7-37: LGTM!The test suite provides good coverage of Task model behavior:
- Both assigned and unassigned scenarios for string representation
- Status constants validation
The implementation is clean and well-structured.
backend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.py
Outdated
Show resolved
Hide resolved
backend/tests/apps/mentorship/management/commands/test_mentorship_update_comments.py
Outdated
Show resolved
Hide resolved
backend/tests/apps/mentorship/model/test_issue_user_interest.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
backend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.py (1)
185-208: Test function has incorrect signature with unused default parameters.The function signature includes an unused
commandfixture parameter and invalid default argumentsmock_module=Noneandmock_repository=None. Additionally, line 198 creates a newCommand()instance instead of using the provided fixture.🔎 Proposed fix
-def test_process_module_no_matches(command, mock_module=None, mock_repository=None): +def test_process_module_no_matches(): mock_repo = MagicMock(); mock_repo.id = 7; mock_repo.name = "r" mock_module = MagicMock()backend/tests/apps/mentorship/management/commands/test_mentorship_update_comments.py (1)
75-81: Remove test for non-existent command behavior.This test mocks
.exclude().select_related()and expects the message "No published mentorship modules with labels found. Exiting.", but the actualprocess_mentorship_modulesimplementation only checkspublished_modules.exists()and iterates over all published modules without any.exclude()or label-filtering logic. This test will never execute the asserted code path.Based on the actual command implementation in the relevant code snippets.
🧹 Nitpick comments (2)
backend/tests/apps/mentorship/management/commands/test_mentorship_update_comments.py (2)
5-5: Remove unused import.The
datetime as dtimport is unused throughout the file.🔎 Proposed fix
-import datetime as dt
159-185: Test validates interest removal correctly.The test verifies that an existing interest is removed when a user posts a regular comment without interest patterns. Note that this test is similar to
test_process_issue_interests_remove_interest(lines 134-156) and may provide redundant coverage.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.pybackend/tests/apps/mentorship/management/commands/test_mentorship_update_comments.py
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-08-17T11:55:55.990Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2000
File: backend/apps/mentorship/management/commands/sync_module_issues.py:109-145
Timestamp: 2025-08-17T11:55:55.990Z
Learning: In the OWASP/Nest mentorship system, tasks are designed to be assigned to only one assignee per issue, even if GitHub issues can have multiple assignees. The sync_module_issues command correctly uses issue.assignees.first() to create one task per issue for the first assignee only.
Applied to files:
backend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.py
📚 Learning: 2025-12-18T05:39:42.678Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:40-40
Timestamp: 2025-12-18T05:39:42.678Z
Learning: In Django management commands, prefer using self.stdout.write(...) over print(...) for user-facing stdout output. This aligns with Django conventions and improves testability. When emitting messages, consider using self.stdout.write and, for styled messages, use self.style.SUCCESS/ERROR as appropriate to maintain consistent command output formatting. Apply this guideline to all Python files within any project's management/commands directory.
Applied to files:
backend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.pybackend/tests/apps/mentorship/management/commands/test_mentorship_update_comments.py
📚 Learning: 2025-12-31T05:17:39.659Z
Learnt from: kart-u
Repo: OWASP/Nest PR: 3101
File: backend/apps/common/extensions.py:92-98
Timestamp: 2025-12-31T05:17:39.659Z
Learning: In this codebase, import OperationType for GraphQL operations from the graphql-core package rather than from strawberry. Use 'from graphql import OperationType'. Strawberry re-exports via graphql-core internally, so relying on strawberry's API may be brittle. Apply this rule to all Python files that deal with GraphQL operation types; ensure imports come from graphql (graphql-core) and not from strawberry packages. This improves compatibility and avoids coupling to strawberry's internals.
Applied to files:
backend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.pybackend/tests/apps/mentorship/management/commands/test_mentorship_update_comments.py
📚 Learning: 2026-01-01T17:48:23.963Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:41-47
Timestamp: 2026-01-01T17:48:23.963Z
Learning: In Django code, be aware that a QuerySet's boolean evaluation (e.g., if not queryset) runs a database query to determine emptiness. While it is technically valid to use the queryset in a boolean context, use queryset.exists() for existence checks to avoid unnecessary queries and improve performance. Applicable broadly to Python/Django files rather than just this specific path.
Applied to files:
backend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.pybackend/tests/apps/mentorship/management/commands/test_mentorship_update_comments.py
🧬 Code graph analysis (2)
backend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.py (1)
backend/apps/mentorship/management/commands/mentorship_sync_module_issues.py (5)
_extract_repo_full_name(28-40)_get_status(42-50)_get_last_assigned_date(52-74)_build_repo_label_to_issue_map(76-92)_process_module(94-203)
backend/tests/apps/mentorship/management/commands/test_mentorship_update_comments.py (1)
backend/apps/mentorship/management/commands/mentorship_update_comments.py (3)
process_mentorship_modules(31-47)process_module(49-81)process_issue_interests(83-152)
🪛 Ruff (0.14.10)
backend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.py
1-1: File backend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
12-12: Boolean default positional argument in function definition
(FBT002)
40-40: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
44-44: Duplicate of test case at index 0 in pytest.mark.parametrize
Remove duplicate test case
(PT014)
56-56: Multiple statements on one line (semicolon)
(E702)
69-69: Use datetime.UTC alias
Convert to datetime.UTC alias
(UP017)
70-70: Use datetime.UTC alias
Convert to datetime.UTC alias
(UP017)
71-71: Use datetime.UTC alias
Convert to datetime.UTC alias
(UP017)
72-72: Use datetime.UTC alias
Convert to datetime.UTC alias
(UP017)
76-76: Use datetime.UTC alias
Convert to datetime.UTC alias
(UP017)
97-97: Variable MockIssue in function should be lowercase
(N806)
115-115: Argument name MockIssue should be lowercase
(N803)
115-115: Argument name MockTask should be lowercase
(N803)
116-116: Multiple statements on one line (semicolon)
(E702)
116-116: Multiple statements on one line (semicolon)
(E702)
121-121: Multiple statements on one line (semicolon)
(E702)
121-121: Multiple statements on one line (semicolon)
(E702)
128-128: Multiple statements on one line (semicolon)
(E702)
152-152: Local variable num_linked is assigned to but never used
Remove assignment to unused variable num_linked
(F841)
185-185: Unused function argument: command
(ARG001)
185-185: Test function parameter mock_module has default argument
Remove default argument
(PT028)
185-185: Unused function argument: mock_repository
(ARG001)
185-185: Test function parameter mock_repository has default argument
Remove default argument
(PT028)
186-186: Multiple statements on one line (semicolon)
(E702)
186-186: Multiple statements on one line (semicolon)
(E702)
193-193: Variable MockTask in function should be lowercase
(N806)
backend/tests/apps/mentorship/management/commands/test_mentorship_update_comments.py
1-1: File backend/tests/apps/mentorship/management/commands/test_mentorship_update_comments.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
10-10: Boolean default positional argument in function definition
(FBT002)
68-68: Argument name MockModule should be lowercase
(N803)
76-76: Argument name MockModule should be lowercase
(N803)
88-88: Argument name MockIssue should be lowercase
(N803)
109-109: Argument name MockIssueUserInterest should be lowercase
(N803)
135-135: Argument name MockIssueUserInterest should be lowercase
(N803)
161-161: Argument name MockIssueUserInterest should be lowercase
(N803)
189-189: Argument name MockIssueUserInterest should be lowercase
(N803)
215-215: Argument name MockIssueUserInterest should be lowercase
(N803)
🔇 Additional comments (7)
backend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.py (1)
64-86: Comprehensive test coverage for assigned date retrieval.This test effectively covers the success path (finding the most recent assigned event), the not-found case, and exception handling with stderr reporting. The verification of stderr output on error aligns with Django management command best practices.
backend/tests/apps/mentorship/management/commands/test_mentorship_update_comments.py (6)
67-72: Test correctly validates empty module scenario.The test accurately reflects the command's behavior when no published modules exist.
84-106: Test comprehensively validates module processing orchestration.The test correctly verifies that
process_modulesyncs issue comments and processes interests for each issue.
108-131: Test accurately validates new interest registration.The test correctly verifies that when a user comments with "/interested", a new
IssueUserInterestis created viabulk_create.
134-156: Test correctly validates interest removal.The test accurately verifies that when a user with an existing interest posts a comment without the interest pattern, their interest is removed.
188-211: Test accurately validates single-user multi-comment aggregation.The test correctly verifies that multiple comments from a single user are aggregated, ensuring only one interest is created regardless of the number of comments.
214-259: Test comprehensively validates mixed multi-user scenarios.The test thoroughly verifies the command's behavior with multiple users exhibiting different interest patterns: creating new interests for user2 and user3, while removing user1's existing interest.
backend/tests/apps/mentorship/management/commands/mentorship_sync_module_issues_test.py
Show resolved
Hide resolved
backend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.py
Outdated
Show resolved
Hide resolved
kart-u
force-pushed
the
test_mentorship_backend
branch
from
13de343 to
c68231e
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI Agents
In
@backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_mentee.py:
- Line 61: The file ends without a trailing newline; update
backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_mentee.py to
add a single newline character at EOF so the final line (the assertion calling
mentee_node_no_exp.resolve_experience_level()) is terminated with a newline,
complying with PEP 8.
- Line 57: Replace the generic "# type: ignore" on the experience_level
assignment in test_api_internal_mentee.py with a specific mypy error code; since
this is ignoring assigning None to a non-Optional field, change it to "# type:
ignore[assignment]" on the experience_level=None line (preserve the comment
placement and nothing else).
In
@backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_mentor.py:
- Line 68: The test file ends without a trailing newline; add a single newline
character at the end of
backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_mentor.py so
the final line (the assertion using mock_mentor_node_no_github_user.login()) is
followed by a newline per PEP 8/POSIX conventions.
In
@backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_program.py:
- Line 129: The file ends without a terminating newline; add a single trailing
newline character at the end of
backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_program.py so
the file ends with a newline (preserve the existing content including the assert
UpdateProgramStatusInput.__annotations__["status"] == ProgramStatusEnum).
In @backend/tests/apps/mentorship/model/test_mentee_module.py:
- Around line 52-71: The test_unique_together_constraint currently patches
MenteeModule.objects.create and raises a MockIntegrityError, so it doesn't
verify the real DB constraint; change the test to be a Django TestCase (class
inherits from django.test.TestCase), remove the @patch and mock_create, create
the first MenteeModule via MenteeModule.objects.create(mentee=self.mentee,
module=self.module, started_at=...) using the real test DB, then assert that
creating the same record a second time raises django.db.IntegrityError (use
pytest.raises(django.db.IntegrityError) or self.assertRaises in TestCase);
ensure setUp provides valid self.mentee and self.module so the uniqueness
constraint on MenteeModule (unique_together = ("mentee", "module")) is actually
exercised.
- Around line 20-50: The test test_create_mentee_module incorrectly patches
MenteeModule.objects.create so it never exercises real model behavior; remove
the @patch decorator and call MenteeModule.objects.create directly in a Django
TestCase so the ORM and model validations run. Ensure the test class inherits
django.test.TestCase (or TransactionTestCase), create any required related
objects in setUp (e.g., self.mentee and self.module), call
MenteeModule.objects.create(…) without mocking, and assert the created instance
fields and __str__ value as before; also remove/mock-only external dependencies
if needed but do not mock the create method itself.
In @backend/tests/apps/mentorship/model/test_mentee_program.py:
- Around line 28-63: The test patches MenteeProgram.objects.create so it never
exercises the real Django model; remove the @patch decorator and the mock_create
usage, convert the test to a Django TestCase (or TransactionTestCase) that uses
the test database, ensure you create any required related objects (self.mentee,
self.program) within setUp or the test, call MenteeProgram.objects.create(...)
for a real DB record (or instantiate and save the model), then assert field
values and str(mentee_program) to validate model behavior and validations
instead of verifying mock interactions.
- Around line 65-85: The test currently mocks MenteeProgram.objects.create and
uses MockIntegrityError, so it doesn't verify the DB-level unique_together
constraint; replace the mocked test with a real DB test by converting it to a
Django TestCase (or TransactionTestCase) and create actual MenteeProgram
instances using MenteeProgram.objects.create, then assert that creating a
duplicate (same mentee and program) raises django.db.IntegrityError; remove use
of MockIntegrityError and the patch on MenteeProgram.objects.create so the real
ORM and DB constraint are exercised.
In @backend/tests/apps/mentorship/model/test_program.py:
- Around line 1-10: Move the MockIntegrityError declaration below the import
block so all import statements (datetime, MagicMock, patch, pytest, and from
apps.mentorship.models import Program, Mentor and from apps.github.models import
User as GithubUser) appear before any non-import code; specifically, relocate
the MockIntegrityError class definition to after those imports (e.g., after the
GithubUser import) to restore proper import organization.
In @backend/tests/apps/mentorship/model/test_start_end_range.py:
- Line 2: Remove the unused import by editing the import statement that
currently reads "from unittest.mock import MagicMock, patch" in the
test_start_end_range.py file and drop "patch" so only MagicMock is imported;
ensure no other references to "patch" exist in this file before committing the
change.
🧹 Nitpick comments (22)
backend/tests/apps/mentorship/management/commands/test_mentorship_sync_issue_levels.py (1)
7-14: Consider addingcmd.style.ERRORfor consistency.The
commandfixture is missing theERRORstyle mock present in similar fixtures across other mentorship command tests. While not required for the current tests, adding it maintains consistency:cmd.style.ERROR = lambda x: xbackend/tests/apps/mentorship/api/internal/queries/test_api_queries_program.py (3)
36-37: Escape regex metacharacter in match pattern.The
.at the end of the pattern is a regex metacharacter that matches any single character. Use a raw string and escape it to match a literal period.🔎 Proposed fix
- with pytest.raises(ObjectDoesNotExist, match="Program with key 'nonexistent' not found."): + with pytest.raises(ObjectDoesNotExist, match=r"Program with key 'nonexistent' not found\."):
65-77: Remove unused mock configurations.The mock configurations at lines 67-68 and 70-71 for
modules.return_value.mentors.return_value...are never accessed during test execution. Themy_programsimplementation usesprogram.admins.all()to determine user role, which is correctly mocked. These dead mock setups can be removed to improve test clarity.🔎 Proposed fix
mock_admin_program = MagicMock(spec=Program, nest_created_at="2023-01-01", id=1) mock_admin_program.admins.all.return_value = [mock_mentor] - mock_admin_program.modules.return_value.mentors.return_value.github_user.return_value = [] mock_mentor_program = MagicMock(spec=Program, nest_created_at="2023-01-02", id=2) mock_mentor_program.admins.all.return_value = [] - mock_mentor_program.modules.return_value.mentors.return_value = [mock_mentor]
142-144: Remove unused mock configuration (same pattern).Similar to the previous comment, lines 143-144 set up unused mock chains. The same applies to lines 169, 173, and 177 in
test_my_programs_pagination.backend/tests/apps/mentorship/api/internal/queries/test_api_queries_mentorship.py (3)
84-99: Add spaces around=in attribute assignments.The attribute assignments are missing spaces around
=(e.g.,mock_module.id=1). While this works, it's inconsistent with PEP 8 and the rest of the codebase.🔎 Proposed fix
mock_module = MagicMock(spec=Module) - mock_module.id=1 + mock_module.id = 1 mock_module_only.return_value.get.return_value = mock_module mock_github_user = MagicMock(spec=GithubUser) - mock_github_user.login="testmentee" - mock_github_user.name="Test_Mentee" - mock_github_user.avatar_url="url" - mock_github_user.bio="bio" + mock_github_user.login = "testmentee" + mock_github_user.name = "Test_Mentee" + mock_github_user.avatar_url = "url" + mock_github_user.bio = "bio" mock_github_user_only.return_value.get.return_value = mock_github_user mock_mentee = MagicMock(spec=Mentee) - mock_mentee.id=2 - mock_mentee.experience_level="Mid" - mock_mentee.domains=["Web"] - mock_mentee.tags=["Python"] + mock_mentee.id = 2 + mock_mentee.experience_level = "Mid" + mock_mentee.domains = ["Web"] + mock_mentee.tags = ["Python"]
348-352: Remove unused mock variables.
mock_issue1andmock_issue3are defined but never used. Onlymock_issue2is needed for this pagination test.🔎 Proposed fix
- mock_issue1 = MagicMock(id=1, number=1, title="Issue 1", state="open", created_at="", url="") mock_issue2 = MagicMock(id=2, number=2, title="Issue 2", state="closed", created_at="", url="") - mock_issue3 = MagicMock(id=3, number=3, title="Issue 3", state="open", created_at="", url="") mock_issues_qs_slice = [mock_issue2]
208-208: Minor style inconsistencies.A few spacing issues:
- Line 208:
mock_prefetch:MagicMock→mock_prefetch: MagicMock- Line 336, 386:
mock_prefetch.return_value=MagicMock()→mock_prefetch.return_value = MagicMock()Also applies to: 336-336, 386-386
backend/tests/apps/mentorship/api/internal/queries/test_api_queries_module.py (2)
86-89: Escape regex metacharacter in match pattern.The
.character in the match string is a regex metacharacter. Use a raw string with escaped period for exact matching.🔎 Proposed fix
with pytest.raises( - ObjectDoesNotExist, match="Module with key 'nonexistent' under program 'program1' not found." + ObjectDoesNotExist, match=r"Module with key 'nonexistent' under program 'program1' not found\." ):
96-97: Remove trailing blank lines.There are extra blank lines at the end of the file.
backend/tests/apps/mentorship/model/test_start_end_range.py (2)
9-10: Consider using timezone-aware datetime objects.The datetime objects are created without timezone information. Django typically uses timezone-aware datetimes when
USE_TZ=True. While this may work for mock tests, using timezone-aware datetimes is better practice.🔎 Suggested improvement
-from datetime import datetime +from datetime import datetime, timezone from unittest.mock import MagicMock, patch from apps.mentorship.models.common.start_end_range import StartEndRange class TestStartEndRange: def test_fields(self): - start_time = datetime(2023, 1, 1, 10, 0, 0) - end_time = datetime(2023, 1, 1, 11, 0, 0) + start_time = datetime(2023, 1, 1, 10, 0, 0, tzinfo=timezone.utc) + end_time = datetime(2023, 1, 1, 11, 0, 0, tzinfo=timezone.utc)
12-18: Simplify the test implementation.Line 15 (
instance = mock_instance) is redundant and can be removed. Additionally, the test could be more meaningful by testing actual model behavior rather than just mock attribute assignment.🔎 Simplified version
- mock_instance = MagicMock(spec=StartEndRange) - mock_instance.started_at = start_time - mock_instance.ended_at = end_time - instance = mock_instance - - assert instance.started_at == start_time - assert instance.ended_at == end_time + instance = MagicMock(spec=StartEndRange) + instance.started_at = start_time + instance.ended_at = end_time + + assert instance.started_at == start_time + assert instance.ended_at == end_timebackend/tests/apps/mentorship/api/internal/nodes/test_api_internal_mentee.py (1)
11-24: Consider simplifying the fixture return.The intermediate assignment to
mentee_nodebefore returning is unnecessary.🔎 Proposed simplification
@pytest.fixture def mock_mentee_node(): """Fixture for a mock MenteeNode instance.""" - mentee_node = MenteeNode( + return MenteeNode( id="1", login="testmentee", name="Test Mentee", avatar_url="https://example.com/avatar.jpg", bio="A test mentee", experience_level=ExperienceLevelEnum.BEGINNER, domains=["python"], tags=["backend"], ) - return mentee_nodebackend/tests/apps/mentorship/api/internal/nodes/test_api_internal_program.py (2)
36-38: Clarify the admins mock setup.The mock setup for
adminsis confusing. Line 38 reassignsmock_program.adminstomock_program.admins.all, which obscures the intent.🔎 Clearer approach
- # Mock properties/methods that access database - mock_program.admins.all.return_value = [MagicMock(), MagicMock()] # Return two mock mentors - mock_program.admins = mock_program.admins.all + # Mock the admins method to return two mock mentors + mock_program.admins.return_value = [MagicMock(), MagicMock()]This makes it clear that
admins()is a callable method that returns a list.
28-28: Consider adding timezone info to datetime objects.For better test reliability and to avoid potential timezone-related issues, consider using timezone-aware datetime objects. While acceptable for test fixtures, using naive datetimes can mask timezone handling bugs.
🔎 Suggested approach
from datetime import datetime, timezone # For fixed dates: datetime(2026, 6, 30, tzinfo=timezone.utc) # For current time: datetime.now(tz=timezone.utc)Also applies to: 31-31, 50-50, 55-55, 90-90, 92-92, 115-115, 117-117
backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_module.py (1)
288-289: Split multiple statements to separate lines.Using semicolons to place multiple statements on one line reduces readability.
🔎 Proposed fix
- mock_interest1 = MagicMock(); mock_interest1.user = MagicMock(login="user1") - mock_interest2 = MagicMock(); mock_interest2.user = MagicMock(login="user2") + mock_interest1 = MagicMock() + mock_interest1.user = MagicMock(login="user1") + mock_interest2 = MagicMock() + mock_interest2.user = MagicMock(login="user2")backend/tests/apps/mentorship/model/test_program.py (3)
23-24: Use timezone-aware datetimes in Django tests.Multiple
datetime()calls lack atzinfoargument. Django best practices recommend using timezone-aware datetimes to avoid subtle bugs in production. Usedjango.utils.timezoneor pass an explicit timezone.🔎 Proposed fix
from django.utils import timezone from datetime import datetime # Option 1: Use timezone.make_aware started_at = timezone.make_aware(datetime(2023, 1, 1, 10, 0, 0)) # Option 2: Use timezone.datetime with UTC from datetime import timezone as dt_timezone started_at = datetime(2023, 1, 1, 10, 0, 0, tzinfo=dt_timezone.utc)Also applies to: 36-37, 47-48, 58-59
16-16: Remove unused lambda parameter.The lambda defining
__str__has an unusedselfparameter. Either remove it or use a proper method reference.🔎 Proposed fix
- self.mentor = MagicMock(spec=Mentor, github_user=self.mentor_github_user, __str__=lambda self: "adminuser") + self.mentor = MagicMock(spec=Mentor, github_user=self.mentor_github_user, __str__=lambda: "adminuser")
74-74: Add trailing newline.The file is missing a newline at the end, which is a PEP 8 convention.
backend/tests/apps/mentorship/model/test_module.py (4)
23-24: Use timezone-aware datetimes in Django tests.Multiple
datetime()calls throughout this test file lacktzinfoarguments. For Django applications, timezone-aware datetimes prevent subtle bugs and align with Django's timezone support.🔎 Recommended approach
from django.utils import timezone from datetime import datetime # Use timezone.make_aware for existing datetime instances started_at = timezone.make_aware(datetime(2023, 1, 1, 10, 0, 0)) # Or use timezone.now() for current time created_at = timezone.now()Also applies to: 48-49, 64-65, 78-79, 116-117, 141-142, 144-145
92-95: Fix docstring formatting.Both docstrings violate PEP 257 conventions: they should start on the first line (not after opening quotes) and have a blank line between the summary and description.
🔎 Proposed fixes
def test_unique_module_key_in_program_constraint_mocked(self, mock_create_module): - """ - Pure-mock simulation of unique constraint: first create succeeds, + """Pure-mock simulation of unique constraint: first create succeeds, + second create raises an IntegrityError (mocked). """def test_module_save(self, mock_create_module): - """ - We cannot call the real Model.save without DB/app registry here, + """We cannot call the real Model.save without DB/app registry here, + so simulate the save behavior by attaching a side_effect to a mock instance's save method. This verifies the *expected* effects (key slugification and date inheritance) in a pure-mock way. """Also applies to: 107-111
127-132: Simplify the date inheritance logic.Lines 129-132 contain redundant logic. The condition checks
if not mock_module.started_at, so the subsequentorexpressions are unnecessary sincemock_module.started_atis guaranteed to be falsy.🔎 Proposed fix
def simulate_save_for_inheritance(*args, **kwargs): mock_module.key = "date-module" if not mock_module.started_at: - mock_module.started_at = mock_module.started_at or mock_module.program.started_at + mock_module.started_at = mock_module.program.started_at if not mock_module.ended_at: - mock_module.ended_at = mock_module.ended_at or mock_module.program.ended_at + mock_module.ended_at = mock_module.program.ended_atAlso, the unused
*args, **kwargsparameters can be removed:- def simulate_save_for_inheritance(*args, **kwargs): + def simulate_save_for_inheritance():
156-156: Add trailing newline.The file is missing a newline at the end, which is a PEP 8 convention.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
backend/apps/github/models/__init__.pybackend/tests/apps/mentorship/api/internal/nodes/test_api_internal_enum.pybackend/tests/apps/mentorship/api/internal/nodes/test_api_internal_mentee.pybackend/tests/apps/mentorship/api/internal/nodes/test_api_internal_mentor.pybackend/tests/apps/mentorship/api/internal/nodes/test_api_internal_module.pybackend/tests/apps/mentorship/api/internal/nodes/test_api_internal_program.pybackend/tests/apps/mentorship/api/internal/queries/test_api_queries_mentorship.pybackend/tests/apps/mentorship/api/internal/queries/test_api_queries_module.pybackend/tests/apps/mentorship/api/internal/queries/test_api_queries_program.pybackend/tests/apps/mentorship/management/commands/test_mentorship_sync_issue_levels.pybackend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.pybackend/tests/apps/mentorship/management/commands/test_mentorship_update_comments.pybackend/tests/apps/mentorship/model/test_experience_level.pybackend/tests/apps/mentorship/model/test_issue_user_interest.pybackend/tests/apps/mentorship/model/test_mentee.pybackend/tests/apps/mentorship/model/test_mentee_module.pybackend/tests/apps/mentorship/model/test_mentee_program.pybackend/tests/apps/mentorship/model/test_mentor.pybackend/tests/apps/mentorship/model/test_mentor_module.pybackend/tests/apps/mentorship/model/test_module.pybackend/tests/apps/mentorship/model/test_program.pybackend/tests/apps/mentorship/model/test_start_end_range.pybackend/tests/apps/mentorship/model/test_task.pybackend/tests/apps/mentorship/model/test_task_level.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/github/models/init.py
🧰 Additional context used
🧠 Learnings (9)
📚 Learning: 2025-12-31T05:17:39.659Z
Learnt from: kart-u
Repo: OWASP/Nest PR: 3101
File: backend/apps/common/extensions.py:92-98
Timestamp: 2025-12-31T05:17:39.659Z
Learning: In this codebase, import OperationType for GraphQL operations from the graphql-core package rather than from strawberry. Use 'from graphql import OperationType'. Strawberry re-exports via graphql-core internally, so relying on strawberry's API may be brittle. Apply this rule to all Python files that deal with GraphQL operation types; ensure imports come from graphql (graphql-core) and not from strawberry packages. This improves compatibility and avoids coupling to strawberry's internals.
Applied to files:
backend/tests/apps/mentorship/model/test_experience_level.pybackend/tests/apps/mentorship/model/test_start_end_range.pybackend/tests/apps/mentorship/api/internal/queries/test_api_queries_module.pybackend/tests/apps/mentorship/model/test_module.pybackend/tests/apps/mentorship/management/commands/test_mentorship_sync_issue_levels.pybackend/tests/apps/mentorship/api/internal/nodes/test_api_internal_mentor.pybackend/tests/apps/mentorship/model/test_mentor_module.pybackend/tests/apps/mentorship/api/internal/nodes/test_api_internal_enum.pybackend/tests/apps/mentorship/api/internal/queries/test_api_queries_mentorship.pybackend/tests/apps/mentorship/api/internal/nodes/test_api_internal_mentee.pybackend/tests/apps/mentorship/api/internal/nodes/test_api_internal_program.pybackend/tests/apps/mentorship/api/internal/queries/test_api_queries_program.pybackend/tests/apps/mentorship/api/internal/nodes/test_api_internal_module.pybackend/tests/apps/mentorship/model/test_issue_user_interest.pybackend/tests/apps/mentorship/model/test_mentee.pybackend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.pybackend/tests/apps/mentorship/management/commands/test_mentorship_update_comments.pybackend/tests/apps/mentorship/model/test_mentee_module.pybackend/tests/apps/mentorship/model/test_mentee_program.pybackend/tests/apps/mentorship/model/test_mentor.pybackend/tests/apps/mentorship/model/test_program.pybackend/tests/apps/mentorship/model/test_task.pybackend/tests/apps/mentorship/model/test_task_level.py
📚 Learning: 2026-01-01T17:48:23.963Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:41-47
Timestamp: 2026-01-01T17:48:23.963Z
Learning: In Django code, be aware that a QuerySet's boolean evaluation (e.g., if not queryset) runs a database query to determine emptiness. While it is technically valid to use the queryset in a boolean context, use queryset.exists() for existence checks to avoid unnecessary queries and improve performance. Applicable broadly to Python/Django files rather than just this specific path.
Applied to files:
backend/tests/apps/mentorship/model/test_experience_level.pybackend/tests/apps/mentorship/model/test_start_end_range.pybackend/tests/apps/mentorship/api/internal/queries/test_api_queries_module.pybackend/tests/apps/mentorship/model/test_module.pybackend/tests/apps/mentorship/management/commands/test_mentorship_sync_issue_levels.pybackend/tests/apps/mentorship/api/internal/nodes/test_api_internal_mentor.pybackend/tests/apps/mentorship/model/test_mentor_module.pybackend/tests/apps/mentorship/api/internal/nodes/test_api_internal_enum.pybackend/tests/apps/mentorship/api/internal/queries/test_api_queries_mentorship.pybackend/tests/apps/mentorship/api/internal/nodes/test_api_internal_mentee.pybackend/tests/apps/mentorship/api/internal/nodes/test_api_internal_program.pybackend/tests/apps/mentorship/api/internal/queries/test_api_queries_program.pybackend/tests/apps/mentorship/api/internal/nodes/test_api_internal_module.pybackend/tests/apps/mentorship/model/test_issue_user_interest.pybackend/tests/apps/mentorship/model/test_mentee.pybackend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.pybackend/tests/apps/mentorship/management/commands/test_mentorship_update_comments.pybackend/tests/apps/mentorship/model/test_mentee_module.pybackend/tests/apps/mentorship/model/test_mentee_program.pybackend/tests/apps/mentorship/model/test_mentor.pybackend/tests/apps/mentorship/model/test_program.pybackend/tests/apps/mentorship/model/test_task.pybackend/tests/apps/mentorship/model/test_task_level.py
📚 Learning: 2025-12-18T05:39:42.678Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:40-40
Timestamp: 2025-12-18T05:39:42.678Z
Learning: In Django management commands, prefer using self.stdout.write(...) over print(...) for user-facing stdout output. This aligns with Django conventions and improves testability. When emitting messages, consider using self.stdout.write and, for styled messages, use self.style.SUCCESS/ERROR as appropriate to maintain consistent command output formatting. Apply this guideline to all Python files within any project's management/commands directory.
Applied to files:
backend/tests/apps/mentorship/management/commands/test_mentorship_sync_issue_levels.pybackend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.pybackend/tests/apps/mentorship/management/commands/test_mentorship_update_comments.py
📚 Learning: 2025-07-16T13:49:58.648Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: backend/apps/mentorship/graphql/mutations/module.py:28-29
Timestamp: 2025-07-16T13:49:58.648Z
Learning: In the OWASP Nest mentorship system, mentors can be created with only github_user initially (without nest_user) when assigned to modules. This allows mentors to be assigned before they've signed into the system. When these users eventually sign in, the nest_user is automatically linked to the existing github_user, creating a complete mentor profile. This design provides flexibility in mentor assignment workflows.
Applied to files:
backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_mentor.py
📚 Learning: 2025-07-13T05:55:46.436Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: backend/apps/mentorship/graphql/mutations/program.py:166-166
Timestamp: 2025-07-13T05:55:46.436Z
Learning: In the OWASP Nest mentorship GraphQL mutations, Strawberry GraphQL automatically converts between Django Program instances and ProgramNode types, so mutations can return Program instances directly without manual conversion even when typed as ProgramNode, similar to the Module/ModuleNode pattern.
Applied to files:
backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_program.py
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS is actively used in frontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsx for program editing functionality and cannot be removed. It serves a different purpose than GET_PROGRAM_ADMIN_DETAILS, providing comprehensive program information needed for editing.
Applied to files:
backend/tests/apps/mentorship/api/internal/queries/test_api_queries_program.py
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS and GET_PROGRAM_ADMIN_DETAILS are two separate queries serving different purposes: GET_PROGRAM_DETAILS fetches comprehensive program information while GET_PROGRAM_ADMIN_DETAILS fetches only admin-related details. These queries cannot be removed or merged as they serve different use cases in the application.
Applied to files:
backend/tests/apps/mentorship/api/internal/queries/test_api_queries_program.py
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS and GET_PROGRAM_ADMIN_DETAILS are two separate queries serving different purposes: GET_PROGRAM_DETAILS fetches comprehensive program information while GET_PROGRAM_ADMIN_DETAILS fetches only admin-related details.
Applied to files:
backend/tests/apps/mentorship/api/internal/queries/test_api_queries_program.py
📚 Learning: 2025-08-17T11:55:55.990Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2000
File: backend/apps/mentorship/management/commands/sync_module_issues.py:109-145
Timestamp: 2025-08-17T11:55:55.990Z
Learning: In the OWASP/Nest mentorship system, tasks are designed to be assigned to only one assignee per issue, even if GitHub issues can have multiple assignees. The sync_module_issues command correctly uses issue.assignees.first() to create one task per issue for the first assignee only.
Applied to files:
backend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.py
🧬 Code graph analysis (23)
backend/tests/apps/mentorship/model/test_experience_level.py (1)
backend/apps/mentorship/models/common/experience_level.py (2)
ExperienceLevel(6-25)ExperienceLevelChoices(12-18)
backend/tests/apps/mentorship/model/test_start_end_range.py (1)
backend/apps/mentorship/models/common/start_end_range.py (1)
StartEndRange(6-13)
backend/tests/apps/mentorship/api/internal/queries/test_api_queries_module.py (1)
backend/apps/mentorship/api/internal/queries/module.py (3)
get_program_modules(19-26)get_project_modules(29-36)get_module(39-50)
backend/tests/apps/mentorship/model/test_module.py (2)
backend/apps/mentorship/models/managers/module.py (1)
PublishedModuleManager(8-13)backend/apps/mentorship/models/program.py (1)
ProgramStatus(25-28)
backend/tests/apps/mentorship/management/commands/test_mentorship_sync_issue_levels.py (2)
backend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.py (1)
command(22-30)backend/tests/apps/mentorship/management/commands/test_mentorship_update_comments.py (1)
command(38-45)
backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_mentor.py (1)
backend/apps/mentorship/api/internal/nodes/mentor.py (3)
avatar_url(13-15)name(18-20)login(23-25)
backend/tests/apps/mentorship/model/test_mentor_module.py (2)
backend/apps/mentorship/models/mentor_module.py (1)
MentorModule(11-42)backend/apps/mentorship/models/mentor.py (1)
Mentor(11-40)
backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_enum.py (3)
backend/apps/mentorship/api/internal/nodes/enum.py (2)
ExperienceLevelEnum(12-18)ProgramStatusEnum(22-27)backend/apps/mentorship/models/common/experience_level.py (2)
ExperienceLevel(6-25)ExperienceLevelChoices(12-18)backend/apps/mentorship/models/program.py (1)
ProgramStatus(25-28)
backend/tests/apps/mentorship/api/internal/queries/test_api_queries_mentorship.py (1)
backend/apps/mentorship/api/internal/queries/mentorship.py (3)
is_mentor(36-48)get_mentee_details(51-82)get_mentee_module_issues(85-124)
backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_mentee.py (2)
backend/apps/mentorship/api/internal/nodes/enum.py (1)
ExperienceLevelEnum(12-18)backend/apps/mentorship/api/internal/nodes/mentee.py (2)
resolve_avatar_url(23-25)resolve_experience_level(28-30)
backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_program.py (3)
backend/apps/mentorship/api/internal/nodes/enum.py (2)
ExperienceLevelEnum(12-18)ProgramStatusEnum(22-27)backend/apps/mentorship/api/internal/nodes/mentor.py (1)
name(18-20)backend/apps/mentorship/api/internal/nodes/program.py (1)
admins(32-34)
backend/tests/apps/mentorship/api/internal/queries/test_api_queries_program.py (4)
backend/apps/mentorship/api/internal/queries/program.py (2)
get_program(23-32)my_programs(35-80)backend/apps/mentorship/api/internal/nodes/mentor.py (1)
login(23-25)backend/apps/owasp/api/internal/nodes/member_snapshot.py (1)
github_user(32-34)backend/apps/mentorship/api/internal/nodes/program.py (1)
admins(32-34)
backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_module.py (4)
backend/apps/mentorship/api/internal/nodes/enum.py (1)
ExperienceLevelEnum(12-18)backend/apps/github/api/internal/nodes/issue.py (1)
labels(50-52)backend/apps/mentorship/models/task.py (1)
Task(10-75)backend/apps/mentorship/models/issue_user_interest.py (1)
IssueUserInterest(6-36)
backend/tests/apps/mentorship/model/test_issue_user_interest.py (1)
backend/apps/mentorship/models/issue_user_interest.py (1)
IssueUserInterest(6-36)
backend/tests/apps/mentorship/model/test_mentee.py (2)
backend/apps/mentorship/models/mentee.py (1)
Mentee(11-49)backend/tests/apps/mentorship/model/test_mentor.py (1)
test_str_returns_github_login(8-14)
backend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.py (2)
backend/apps/mentorship/models/task.py (1)
Task(10-75)backend/apps/mentorship/management/commands/mentorship_sync_module_issues.py (5)
_extract_repo_full_name(28-40)_get_status(42-50)_get_last_assigned_date(52-74)_build_repo_label_to_issue_map(76-92)_process_module(94-203)
backend/tests/apps/mentorship/management/commands/test_mentorship_update_comments.py (1)
backend/apps/mentorship/management/commands/mentorship_update_comments.py (3)
process_mentorship_modules(31-55)process_module(57-89)process_issue_interests(91-160)
backend/tests/apps/mentorship/model/test_mentee_module.py (2)
backend/apps/mentorship/models/mentee.py (1)
Mentee(11-49)backend/apps/mentorship/models/mentee_module.py (1)
MenteeModule(11-45)
backend/tests/apps/mentorship/model/test_mentee_program.py (3)
backend/apps/mentorship/models/mentee.py (1)
Mentee(11-49)backend/apps/mentorship/models/mentee_program.py (1)
MenteeProgram(11-39)backend/tests/apps/mentorship/model/test_mentee_module.py (2)
MockIntegrityError(9-10)test_unique_together_constraint(53-71)
backend/tests/apps/mentorship/model/test_mentor.py (3)
backend/apps/mentorship/models/mentor.py (1)
Mentor(11-40)backend/tests/apps/mentorship/model/test_mentee.py (1)
test_str_returns_github_login(8-14)backend/apps/mentorship/api/internal/nodes/mentor.py (1)
login(23-25)
backend/tests/apps/mentorship/model/test_program.py (3)
backend/apps/mentorship/models/mentor.py (1)
Mentor(11-40)backend/apps/mentorship/api/internal/nodes/mentor.py (2)
login(23-25)name(18-20)backend/apps/mentorship/models/program.py (1)
ProgramStatus(25-28)
backend/tests/apps/mentorship/model/test_task.py (1)
backend/apps/mentorship/models/task.py (1)
Task(10-75)
backend/tests/apps/mentorship/model/test_task_level.py (1)
backend/apps/mentorship/models/task_level.py (1)
TaskLevel(8-61)
🪛 Ruff (0.14.10)
backend/tests/apps/mentorship/model/test_experience_level.py
1-1: File backend/tests/apps/mentorship/model/test_experience_level.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
backend/tests/apps/mentorship/model/test_start_end_range.py
1-1: File backend/tests/apps/mentorship/model/test_start_end_range.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
9-9: datetime.datetime() called without a tzinfo argument
(DTZ001)
10-10: datetime.datetime() called without a tzinfo argument
(DTZ001)
backend/tests/apps/mentorship/api/internal/queries/test_api_queries_module.py
1-1: File backend/tests/apps/mentorship/api/internal/queries/test_api_queries_module.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
87-87: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
backend/tests/apps/mentorship/model/test_module.py
1-1: File backend/tests/apps/mentorship/model/test_module.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
23-23: datetime.datetime() called without a tzinfo argument
(DTZ001)
24-24: datetime.datetime() called without a tzinfo argument
(DTZ001)
48-48: datetime.datetime() called without a tzinfo argument
(DTZ001)
49-49: datetime.datetime() called without a tzinfo argument
(DTZ001)
64-64: datetime.datetime() called without a tzinfo argument
(DTZ001)
65-65: datetime.datetime() called without a tzinfo argument
(DTZ001)
78-78: datetime.datetime() called without a tzinfo argument
(DTZ001)
79-79: datetime.datetime() called without a tzinfo argument
(DTZ001)
92-95: 1 blank line required between summary line and description
(D205)
92-95: Multi-line docstring summary should start at the first line
Remove whitespace after opening quotes
(D212)
107-111: 1 blank line required between summary line and description
(D205)
107-111: Multi-line docstring summary should start at the first line
Remove whitespace after opening quotes
(D212)
116-116: datetime.datetime() called without a tzinfo argument
(DTZ001)
117-117: datetime.datetime() called without a tzinfo argument
(DTZ001)
127-127: Unused function argument: args
(ARG001)
127-127: Unused function argument: kwargs
(ARG001)
141-141: datetime.datetime() called without a tzinfo argument
(DTZ001)
142-142: datetime.datetime() called without a tzinfo argument
(DTZ001)
144-144: datetime.datetime() called without a tzinfo argument
(DTZ001)
145-145: datetime.datetime() called without a tzinfo argument
(DTZ001)
156-156: No newline at end of file
Add trailing newline
(W292)
backend/tests/apps/mentorship/management/commands/test_mentorship_sync_issue_levels.py
1-1: File backend/tests/apps/mentorship/management/commands/test_mentorship_sync_issue_levels.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
17-17: Boolean default positional argument in function definition
(FBT002)
18-18: First line of docstring should be in imperative mood: "Helper: return a queryset-like MagicMock that is iterable and supports .exists()."
(D401)
27-27: Argument name MockTaskLevel should be lowercase
(N803)
28-28: No blank lines allowed after function docstring (found 1)
Remove blank line(s) after function docstring
(D202)
39-39: Argument name MockTaskLevel should be lowercase
(N803)
39-39: Argument name MockIssue should be lowercase
(N803)
40-40: No blank lines allowed after function docstring (found 1)
Remove blank line(s) after function docstring
(D202)
52-52: Multiple statements on one line (semicolon)
(E702)
53-53: Multiple statements on one line (semicolon)
(E702)
54-54: Multiple statements on one line (semicolon)
(E702)
97-97: Argument name MockTaskLevel should be lowercase
(N803)
97-97: Argument name MockIssue should be lowercase
(N803)
106-106: Multiple statements on one line (semicolon)
(E702)
backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_mentor.py
1-1: File backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_mentor.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
68-68: No newline at end of file
Add trailing newline
(W292)
backend/tests/apps/mentorship/model/test_mentor_module.py
1-1: File backend/tests/apps/mentorship/model/test_mentor_module.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_enum.py
1-1: File backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_enum.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
backend/tests/apps/mentorship/api/internal/queries/test_api_queries_mentorship.py
1-1: File backend/tests/apps/mentorship/api/internal/queries/test_api_queries_mentorship.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
348-348: Local variable mock_issue1 is assigned to but never used
Remove assignment to unused variable mock_issue1
(F841)
350-350: Local variable mock_issue3 is assigned to but never used
Remove assignment to unused variable mock_issue3
(F841)
backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_mentee.py
1-1: File backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_mentee.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
24-24: Unnecessary assignment to mentee_node before return statement
Remove unnecessary assignment
(RET504)
57-57: Use specific rule codes when ignoring type issues
(PGH003)
61-61: No newline at end of file
Add trailing newline
(W292)
backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_program.py
1-1: File backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_program.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
28-28: datetime.datetime() called without a tzinfo argument
(DTZ001)
31-31: datetime.datetime() called without a tzinfo argument
(DTZ001)
50-50: datetime.datetime() called without a tzinfo argument
(DTZ001)
55-55: datetime.datetime() called without a tzinfo argument
(DTZ001)
79-79: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
80-80: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
83-83: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
90-90: datetime.datetime.now() called without a tz argument
(DTZ005)
92-92: datetime.datetime.now() called without a tz argument
(DTZ005)
100-100: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
101-101: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
102-102: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
106-106: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
115-115: datetime.datetime.now() called without a tz argument
(DTZ005)
117-117: datetime.datetime.now() called without a tz argument
(DTZ005)
127-127: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
128-128: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
129-129: No newline at end of file
Add trailing newline
(W292)
backend/tests/apps/mentorship/api/internal/queries/test_api_queries_program.py
1-1: File backend/tests/apps/mentorship/api/internal/queries/test_api_queries_program.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
36-36: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_module.py
1-1: File backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_module.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
1-2: 1 blank line required between summary line and description
(D205)
1-2: Multi-line docstring closing quotes should be on a separate line
Move closing quotes to new line
(D209)
15-16: 1 blank line required between summary line and description
(D205)
15-16: Multi-line docstring closing quotes should be on a separate line
Move closing quotes to new line
(D209)
25-25: datetime.datetime() called without a tzinfo argument
(DTZ001)
32-32: datetime.datetime() called without a tzinfo argument
(DTZ001)
184-184: datetime.datetime() called without a tzinfo argument
(DTZ001)
190-190: datetime.datetime() called without a tzinfo argument
(DTZ001)
288-288: Multiple statements on one line (semicolon)
(E702)
289-289: Multiple statements on one line (semicolon)
(E702)
313-313: datetime.datetime() called without a tzinfo argument
(DTZ001)
316-316: datetime.datetime() called without a tzinfo argument
(DTZ001)
332-332: datetime.datetime() called without a tzinfo argument
(DTZ001)
335-335: datetime.datetime() called without a tzinfo argument
(DTZ001)
352-352: datetime.datetime.now() called without a tz argument
(DTZ005)
354-354: datetime.datetime.now() called without a tz argument
(DTZ005)
362-362: datetime.datetime.now() called without a tz argument
(DTZ005)
363-363: datetime.datetime.now() called without a tz argument
(DTZ005)
backend/tests/apps/mentorship/model/test_issue_user_interest.py
1-1: File backend/tests/apps/mentorship/model/test_issue_user_interest.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
backend/tests/apps/mentorship/model/test_mentee.py
1-1: File backend/tests/apps/mentorship/model/test_mentee.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
backend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.py
1-1: File backend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
12-12: Boolean default positional argument in function definition
(FBT002)
40-40: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
44-44: Duplicate of test case at index 0 in pytest.mark.parametrize
Remove duplicate test case
(PT014)
56-56: Multiple statements on one line (semicolon)
(E702)
69-69: Use datetime.UTC alias
Convert to datetime.UTC alias
(UP017)
70-70: Use datetime.UTC alias
Convert to datetime.UTC alias
(UP017)
71-71: Use datetime.UTC alias
Convert to datetime.UTC alias
(UP017)
72-72: Use datetime.UTC alias
Convert to datetime.UTC alias
(UP017)
76-76: Use datetime.UTC alias
Convert to datetime.UTC alias
(UP017)
97-97: Variable MockIssue in function should be lowercase
(N806)
115-115: Argument name MockIssue should be lowercase
(N803)
115-115: Argument name MockTask should be lowercase
(N803)
116-116: Multiple statements on one line (semicolon)
(E702)
116-116: Multiple statements on one line (semicolon)
(E702)
121-121: Multiple statements on one line (semicolon)
(E702)
121-121: Multiple statements on one line (semicolon)
(E702)
128-128: Multiple statements on one line (semicolon)
(E702)
152-152: Local variable num_linked is assigned to but never used
Remove assignment to unused variable num_linked
(F841)
185-185: Unused function argument: command
(ARG001)
185-185: Test function parameter mock_module has default argument
Remove default argument
(PT028)
185-185: Unused function argument: mock_repository
(ARG001)
185-185: Test function parameter mock_repository has default argument
Remove default argument
(PT028)
186-186: Multiple statements on one line (semicolon)
(E702)
186-186: Multiple statements on one line (semicolon)
(E702)
193-193: Variable MockTask in function should be lowercase
(N806)
backend/tests/apps/mentorship/management/commands/test_mentorship_update_comments.py
1-1: File backend/tests/apps/mentorship/management/commands/test_mentorship_update_comments.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
10-10: Boolean default positional argument in function definition
(FBT002)
68-68: Argument name MockModule should be lowercase
(N803)
76-76: Argument name MockModule should be lowercase
(N803)
88-88: Argument name MockIssue should be lowercase
(N803)
109-109: Argument name MockIssueUserInterest should be lowercase
(N803)
135-135: Argument name MockIssueUserInterest should be lowercase
(N803)
161-161: Argument name MockIssueUserInterest should be lowercase
(N803)
189-189: Argument name MockIssueUserInterest should be lowercase
(N803)
215-215: Argument name MockIssueUserInterest should be lowercase
(N803)
backend/tests/apps/mentorship/model/test_mentee_module.py
1-1: File backend/tests/apps/mentorship/model/test_mentee_module.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
16-16: Unused lambda argument: self
(ARG005)
18-18: Unused lambda argument: self
(ARG005)
22-22: datetime.datetime() called without a tzinfo argument
(DTZ001)
23-23: datetime.datetime() called without a tzinfo argument
(DTZ001)
62-62: datetime.datetime() called without a tzinfo argument
(DTZ001)
68-68: datetime.datetime() called without a tzinfo argument
(DTZ001)
backend/tests/apps/mentorship/model/test_mentee_program.py
1-1: File backend/tests/apps/mentorship/model/test_mentee_program.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
19-19: Unused lambda argument: self
(ARG005)
25-25: Unused lambda argument: self
(ARG005)
30-30: datetime.datetime() called without a tzinfo argument
(DTZ001)
31-31: datetime.datetime() called without a tzinfo argument
(DTZ001)
75-75: datetime.datetime() called without a tzinfo argument
(DTZ001)
82-82: datetime.datetime() called without a tzinfo argument
(DTZ001)
backend/tests/apps/mentorship/model/test_mentor.py
1-1: File backend/tests/apps/mentorship/model/test_mentor.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
backend/tests/apps/mentorship/model/test_program.py
1-1: File backend/tests/apps/mentorship/model/test_program.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
9-9: Module level import not at top of file
(E402)
10-10: Module level import not at top of file
(E402)
16-16: Unused lambda argument: self
(ARG005)
23-23: datetime.datetime() called without a tzinfo argument
(DTZ001)
24-24: datetime.datetime() called without a tzinfo argument
(DTZ001)
36-36: datetime.datetime() called without a tzinfo argument
(DTZ001)
37-37: datetime.datetime() called without a tzinfo argument
(DTZ001)
47-47: datetime.datetime() called without a tzinfo argument
(DTZ001)
48-48: datetime.datetime() called without a tzinfo argument
(DTZ001)
58-58: datetime.datetime() called without a tzinfo argument
(DTZ001)
59-59: datetime.datetime() called without a tzinfo argument
(DTZ001)
74-74: No newline at end of file
Add trailing newline
(W292)
backend/tests/apps/mentorship/model/test_task.py
1-1: File backend/tests/apps/mentorship/model/test_task.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
backend/tests/apps/mentorship/model/test_task_level.py
1-1: File backend/tests/apps/mentorship/model/test_task_level.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_mentee.py
Outdated
Show resolved
Hide resolved
backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_mentee.py
Outdated
Show resolved
Hide resolved
backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_mentor.py
Outdated
Show resolved
Hide resolved
backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_program.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI Agents
In
@backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_mentee.py:
- Around line 44-46: The method resolve_experience_level currently returns
ExperienceLevelEnum when experience_level is set but a string when None; make it
consistently return a string by changing resolve_experience_level to return
self.experience_level.value if self.experience_level else "beginner" and keep
the return type annotation as str, and update the test
(test_mentee_node_resolve_experience_level) to expect the string "beginner"
instead of ExperienceLevelEnum.BEGINNER.
In
@backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_program.py:
- Around line 37-38: The test wrongly assigns mock_program.admins to the all
method itself; instead set admins to a mock relationship manager whose all()
returns the list of mentors. Replace the current lines with something like:
create a MagicMock() for mock_program.admins (e.g., mock_program.admins =
MagicMock()) and then set mock_program.admins.all.return_value = [MagicMock(),
MagicMock()] so that calls to mock_program.admins.all() behave like the real
relationship manager.
In @backend/tests/apps/mentorship/model/test_module.py:
- Around line 28-33: Replace the mock-based test with a Django TestCase that
creates a real Module instance and asserts its __str__ output: in
test_str_returns_name use Module.objects.create(...) to create a Module
(providing required relations like program and project or creating those
fixtures), then assert str(module) == "Security Basics" to validate the actual
Module.__str__ implementation.
- Around line 23-24: Replace naive datetimes used for started_at and ended_at
with timezone-aware datetimes: import django.utils.timezone and construct
datetimes with timezone.datetime(..., tzinfo=timezone.utc) or use
timezone.make_aware/timezone.now where appropriate; update every occurrence
referenced (started_at, ended_at and the other instances at the lines noted) so
tests use timezone-aware values and ensure the import for timezone is added to
the test module.
- Around line 36-87: The test currently patches Module.objects.create and
simulates Module.save via a MagicMock, so it doesn't validate the real
Module.save behavior; replace the mock-based test with real ORM-backed tests
using Django TestCase: remove the patching of Module.objects.create and
MagicMock usage, create a Program via Program.objects.create (with
timezone-aware started_at/ended_at), then create a Module via
Module.objects.create or instantiate and call module.save() to exercise the
actual Module.save method, and assert that module.key is slugified and
started_at/ended_at are inherited when None and preserved when explicitly
provided; update test names (e.g., test_module_save_inherits_dates_from_program
and test_module_save_respects_explicit_dates) to reflect the two behaviors being
verified.
In @backend/tests/apps/mentorship/model/test_program.py:
- Around line 13-22: Tests in TestProgram currently only assert enum values and
Program.__str__; convert them to real Django DB tests by subclassing
django.test.TestCase (replace the lightweight class TestProgram), create Program
instances via Program.objects.create in methods like
test_create_program_with_required_fields and
test_create_program_with_all_fields, assert created instance fields (id, name),
defaults and auto-generated values such as key (Program.key), timestamps
(started_at/ended_at), list fields (experience_levels, domains, tags) and
mentees_limit, and verify str(program) returns the name; also add tests for
relationships (create Mentor/Mentee linked to Program and assert relationship
queries) and ensure the file ends with a newline to fix W292.
- Around line 1-10: Remove unused imports and the unused helper from the test
file: delete the unused imports datetime, MagicMock, and patch, remove the
unused class MockIntegrityError, and drop unused model imports Mentor and
GithubUser if they are not referenced elsewhere in this test file; keep only the
necessary import(s) for Program (apps.mentorship.models.Program) and reorganize
the remaining imports into a clean, minimal import block.
🧹 Nitpick comments (10)
backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_program.py (1)
79-86: Consider usingisfor type comparisons.The tests check type annotations using
==, which works but is not idiomatic Python. The style guide recommends usingisfor type identity comparisons.🔎 Example refactor for one assertion
- assert CreateProgramInput.__annotations__["name"] == str + assert CreateProgramInput.__annotations__["name"] is strApply this pattern to all type annotation assertions throughout the three test functions.
Also applies to: 100-109, 127-129
backend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.py (6)
3-4: Consolidate datetime imports.The file imports both
from datetime import datetimeandimport datetime as dt, which creates redundancy. Later in the file,dt.timezone.utcis used. Consider consolidating to one approach:Option 1 (recommended): Import both
datetimeandtimezonedirectly:-from datetime import datetime -import datetime as dt +from datetime import datetime, timezoneThen update usages of
dt.timezone.utcto justtimezone.utc(lines 68-71, 75).Option 2: Use the module import consistently:
-from datetime import datetime import datetime as dtThen update bare
datetime(...)calls todt.datetime(...).
39-49: Use tuple for parametrize argument names.Line 40 should use a tuple for the parameter names to follow pytest conventions.
🔎 Proposed fix
@pytest.mark.parametrize( - "repo_url, expected", + ("repo_url", "expected"), [ ("https://github.com/owner/repo", "owner/repo"), ("https://www.github.com/owner/repo/sub/path", "owner/repo"),
54-60: Consider splitting statements for readability.Line 55 places multiple statements on one line using semicolons, which reduces readability.
🔎 Proposed fix
def test_get_status_variants(command): - issue = MagicMock(); issue.state = "closed" + issue = MagicMock() + issue.state = "closed" assert command._get_status(issue, MagicMock()) == Task.Status.COMPLETED
63-85: Consider usingdatetime.UTCfor timezone specification.Lines 68-71 and 75 use
dt.timezone.utc. Python 3.11+ introduceddatetime.UTCas a cleaner alias. If your project targets Python 3.11+, consider updating:- e1 = MagicMock(event="commented", created_at=datetime(2023, 1, 1, tzinfo=dt.timezone.utc)) + e1 = MagicMock(event="commented", created_at=datetime(2023, 1, 1, tzinfo=timezone.UTC))Apply similarly to lines 69-71 and 75. (This assumes you've consolidated imports as suggested earlier.)
Otherwise, the test logic is excellent—it thoroughly covers the found, not-found, and exception scenarios.
112-182: Excellent test coverage with comprehensive assertions.This test thoroughly validates the
_process_modulefunction:
- Issue linking via
module.issues.set- Task creation with correct status mapping (IN_PROGRESS for open+assigned, COMPLETED for closed)
- Return value verification (
num_linked)- Console output validation
Great job addressing the previous review comment by adding the
assert num_linked == 3assertion on line 158.Optional style improvements:
- Lines 115, 120, 127: Consider splitting semicolon-separated statements onto separate lines for better readability
- Lines 114: Mock parameter naming (
MockIssue,MockTask) could be lowercased per PEP 8, though PascalCase is common in test patches
185-208: Consider using thecommandfixture for consistency.The test instantiates a new
Command()instance directly (line 198) instead of using thecommandfixture like other tests. This means stdout/stderr aren't mocked, making the test inconsistent with the rest of the file.🔎 Proposed fix
-def test_process_module_no_matches(): - mock_repo = MagicMock(); mock_repo.id = 7; mock_repo.name = "r" +def test_process_module_no_matches(command): + mock_repo = MagicMock() + mock_repo.id = 7 + mock_repo.name = "r" mock_module = MagicMock() mock_module.project.repositories.all.return_value = [mock_repo] mock_module.labels = ["some-label"] repo_label_to_issue_ids = {} with patch("apps.mentorship.management.commands.mentorship_sync_module_issues.Task") as MockTask, \ patch("apps.mentorship.management.commands.mentorship_sync_module_issues.transaction.atomic") as mock_atomic: mock_atomic.return_value.__enter__.return_value = None mock_atomic.return_value.__exit__.return_value = None - num_linked = Command()._process_module( + num_linked = command._process_module( module=mock_module, repo_label_to_issue_ids=repo_label_to_issue_ids, gh_client=MagicMock(), repo_cache={}, verbosity=1, )This also splits the semicolon-separated statements on line 186 for better readability.
backend/tests/apps/mentorship/model/test_module.py (1)
38-42: Address minor formatting issues.Static analysis flags several style issues:
- Docstring formatting (lines 38-42): Multi-line docstring should start at the first line and have a blank line between summary and description.
- Unused arguments (line 58): The
argsandkwargsparameters insimulate_save_for_inheritanceare unused.- Missing newline (line 87): Add a trailing newline at the end of the file.
🔎 Formatting fixes
- def test_module_save(self, mock_create_module): - """ - We cannot call the real Model.save without DB/app registry here, - so simulate the save behavior by attaching a side_effect to a mock instance's save method. - This verifies the *expected* effects (key slugification and date inheritance) in a pure-mock way. - """ + def test_module_save(self, mock_create_module): + """We cannot call the real Model.save without DB/app registry here. + + Simulates the save behavior by attaching a side_effect to a mock instance's save method. + This verifies the *expected* effects (key slugification and date inheritance) in a pure-mock way. + """ - def simulate_save_for_inheritance(*args, **kwargs): + def simulate_save_for_inheritance():And add a newline at the end of the file (line 87).
Also applies to: 58-58, 87-87
backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_mentee.py (1)
23-24: Optional: Return directly without assignment.The assignment before return is unnecessary. Consider returning the
MenteeNodeconstructor directly.🔎 Proposed simplification
@pytest.fixture def mock_mentee_node(): """Fixture for a mock MenteeNode instance.""" - mentee_node = MenteeNode( + return MenteeNode( id="1", login="testmentee", name="Test Mentee", avatar_url="https://example.com/avatar.jpg", bio="A test mentee", experience_level=ExperienceLevelEnum.BEGINNER, domains=["python"], tags=["backend"], ) - return mentee_nodebackend/tests/apps/mentorship/api/internal/nodes/test_api_internal_mentor.py (1)
20-33: Optional: Return directly without intermediate assignment.Both
mock_mentor_nodeandmock_mentor_node_no_github_userfixtures assign to a variable before returning. Consider streamlining by removing the intermediate assignment.🔎 Proposed simplification
@pytest.fixture def mock_mentor_node(mock_github_user): """Fixture for a mock MentorNode instance.""" mentor_node = MentorNode(id="1") mentor_node.github_user = mock_github_user return mentor_node @pytest.fixture def mock_mentor_node_no_github_user(): """Fixture for a mock MentorNode instance without a GitHub user.""" mentor_node = MentorNode(id="2") mentor_node.github_user = None return mentor_nodeWhile this pattern is acceptable, if you prefer more concise fixtures, you could inline the attribute assignment. However, the current approach is clear and explicit, so this is truly optional.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_mentee.pybackend/tests/apps/mentorship/api/internal/nodes/test_api_internal_mentor.pybackend/tests/apps/mentorship/api/internal/nodes/test_api_internal_program.pybackend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.pybackend/tests/apps/mentorship/model/test_module.pybackend/tests/apps/mentorship/model/test_program.py
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-07-16T13:49:58.648Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: backend/apps/mentorship/graphql/mutations/module.py:28-29
Timestamp: 2025-07-16T13:49:58.648Z
Learning: In the OWASP Nest mentorship system, mentors can be created with only github_user initially (without nest_user) when assigned to modules. This allows mentors to be assigned before they've signed into the system. When these users eventually sign in, the nest_user is automatically linked to the existing github_user, creating a complete mentor profile. This design provides flexibility in mentor assignment workflows.
Applied to files:
backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_mentor.py
📚 Learning: 2025-12-31T05:17:39.659Z
Learnt from: kart-u
Repo: OWASP/Nest PR: 3101
File: backend/apps/common/extensions.py:92-98
Timestamp: 2025-12-31T05:17:39.659Z
Learning: In this codebase, import OperationType for GraphQL operations from the graphql-core package rather than from strawberry. Use 'from graphql import OperationType'. Strawberry re-exports via graphql-core internally, so relying on strawberry's API may be brittle. Apply this rule to all Python files that deal with GraphQL operation types; ensure imports come from graphql (graphql-core) and not from strawberry packages. This improves compatibility and avoids coupling to strawberry's internals.
Applied to files:
backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_mentor.pybackend/tests/apps/mentorship/model/test_module.pybackend/tests/apps/mentorship/api/internal/nodes/test_api_internal_program.pybackend/tests/apps/mentorship/api/internal/nodes/test_api_internal_mentee.pybackend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.pybackend/tests/apps/mentorship/model/test_program.py
📚 Learning: 2026-01-01T17:48:23.963Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:41-47
Timestamp: 2026-01-01T17:48:23.963Z
Learning: In Django code, be aware that a QuerySet's boolean evaluation (e.g., if not queryset) runs a database query to determine emptiness. While it is technically valid to use the queryset in a boolean context, use queryset.exists() for existence checks to avoid unnecessary queries and improve performance. Applicable broadly to Python/Django files rather than just this specific path.
Applied to files:
backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_mentor.pybackend/tests/apps/mentorship/model/test_module.pybackend/tests/apps/mentorship/api/internal/nodes/test_api_internal_program.pybackend/tests/apps/mentorship/api/internal/nodes/test_api_internal_mentee.pybackend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.pybackend/tests/apps/mentorship/model/test_program.py
📚 Learning: 2025-07-13T05:55:46.436Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: backend/apps/mentorship/graphql/mutations/program.py:166-166
Timestamp: 2025-07-13T05:55:46.436Z
Learning: In the OWASP Nest mentorship GraphQL mutations, Strawberry GraphQL automatically converts between Django Program instances and ProgramNode types, so mutations can return Program instances directly without manual conversion even when typed as ProgramNode, similar to the Module/ModuleNode pattern.
Applied to files:
backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_program.py
📚 Learning: 2025-09-29T06:02:35.566Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/components/SingleModuleCard.tsx:54-54
Timestamp: 2025-09-29T06:02:35.566Z
Learning: In the Module type from types/mentorship.ts, the experienceLevel field is required (experienceLevel: ExperienceLevelEnum), not optional, so null/undefined checks are not needed when accessing this field.
Applied to files:
backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_mentee.py
📚 Learning: 2025-08-17T11:55:55.990Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2000
File: backend/apps/mentorship/management/commands/sync_module_issues.py:109-145
Timestamp: 2025-08-17T11:55:55.990Z
Learning: In the OWASP/Nest mentorship system, tasks are designed to be assigned to only one assignee per issue, even if GitHub issues can have multiple assignees. The sync_module_issues command correctly uses issue.assignees.first() to create one task per issue for the first assignee only.
Applied to files:
backend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.py
📚 Learning: 2025-12-18T05:39:42.678Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:40-40
Timestamp: 2025-12-18T05:39:42.678Z
Learning: In Django management commands, prefer using self.stdout.write(...) over print(...) for user-facing stdout output. This aligns with Django conventions and improves testability. When emitting messages, consider using self.stdout.write and, for styled messages, use self.style.SUCCESS/ERROR as appropriate to maintain consistent command output formatting. Apply this guideline to all Python files within any project's management/commands directory.
Applied to files:
backend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.py
🧬 Code graph analysis (5)
backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_mentor.py (1)
backend/apps/mentorship/api/internal/nodes/mentor.py (3)
avatar_url(13-15)name(18-20)login(23-25)
backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_program.py (3)
backend/apps/mentorship/api/internal/nodes/enum.py (2)
ExperienceLevelEnum(12-18)ProgramStatusEnum(22-27)backend/apps/mentorship/api/internal/nodes/mentor.py (1)
name(18-20)backend/apps/mentorship/api/internal/nodes/program.py (1)
admins(32-34)
backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_mentee.py (2)
backend/apps/mentorship/api/internal/nodes/enum.py (1)
ExperienceLevelEnum(12-18)backend/apps/mentorship/api/internal/nodes/mentee.py (2)
resolve_avatar_url(23-25)resolve_experience_level(28-30)
backend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.py (2)
backend/apps/mentorship/models/task.py (1)
Task(10-75)backend/apps/mentorship/management/commands/mentorship_sync_module_issues.py (5)
_extract_repo_full_name(28-40)_get_status(42-50)_get_last_assigned_date(52-74)_build_repo_label_to_issue_map(76-92)_process_module(94-203)
backend/tests/apps/mentorship/model/test_program.py (2)
backend/apps/mentorship/models/program.py (1)
ProgramStatus(25-28)backend/apps/mentorship/api/internal/nodes/mentor.py (1)
name(18-20)
🪛 Ruff (0.14.10)
backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_mentor.py
1-1: File backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_mentor.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
backend/tests/apps/mentorship/model/test_module.py
1-1: File backend/tests/apps/mentorship/model/test_module.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
23-23: datetime.datetime() called without a tzinfo argument
(DTZ001)
24-24: datetime.datetime() called without a tzinfo argument
(DTZ001)
38-42: 1 blank line required between summary line and description
(D205)
38-42: Multi-line docstring summary should start at the first line
Remove whitespace after opening quotes
(D212)
47-47: datetime.datetime() called without a tzinfo argument
(DTZ001)
48-48: datetime.datetime() called without a tzinfo argument
(DTZ001)
58-58: Unused function argument: args
(ARG001)
58-58: Unused function argument: kwargs
(ARG001)
72-72: datetime.datetime() called without a tzinfo argument
(DTZ001)
73-73: datetime.datetime() called without a tzinfo argument
(DTZ001)
75-75: datetime.datetime() called without a tzinfo argument
(DTZ001)
76-76: datetime.datetime() called without a tzinfo argument
(DTZ001)
87-87: No newline at end of file
Add trailing newline
(W292)
backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_program.py
1-1: File backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_program.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
28-28: datetime.datetime() called without a tzinfo argument
(DTZ001)
31-31: datetime.datetime() called without a tzinfo argument
(DTZ001)
50-50: datetime.datetime() called without a tzinfo argument
(DTZ001)
55-55: datetime.datetime() called without a tzinfo argument
(DTZ001)
79-79: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
80-80: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
83-83: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
90-90: datetime.datetime.now() called without a tz argument
(DTZ005)
92-92: datetime.datetime.now() called without a tz argument
(DTZ005)
100-100: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
101-101: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
102-102: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
106-106: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
115-115: datetime.datetime.now() called without a tz argument
(DTZ005)
117-117: datetime.datetime.now() called without a tz argument
(DTZ005)
127-127: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
128-128: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_mentee.py
1-1: File backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_mentee.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
24-24: Unnecessary assignment to mentee_node before return statement
Remove unnecessary assignment
(RET504)
backend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.py
1-1: File backend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
12-12: Boolean default positional argument in function definition
(FBT002)
40-40: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
55-55: Multiple statements on one line (semicolon)
(E702)
68-68: Use datetime.UTC alias
Convert to datetime.UTC alias
(UP017)
69-69: Use datetime.UTC alias
Convert to datetime.UTC alias
(UP017)
70-70: Use datetime.UTC alias
Convert to datetime.UTC alias
(UP017)
71-71: Use datetime.UTC alias
Convert to datetime.UTC alias
(UP017)
75-75: Use datetime.UTC alias
Convert to datetime.UTC alias
(UP017)
96-96: Variable MockIssue in function should be lowercase
(N806)
114-114: Argument name MockIssue should be lowercase
(N803)
114-114: Argument name MockTask should be lowercase
(N803)
115-115: Multiple statements on one line (semicolon)
(E702)
115-115: Multiple statements on one line (semicolon)
(E702)
120-120: Multiple statements on one line (semicolon)
(E702)
120-120: Multiple statements on one line (semicolon)
(E702)
127-127: Multiple statements on one line (semicolon)
(E702)
186-186: Multiple statements on one line (semicolon)
(E702)
186-186: Multiple statements on one line (semicolon)
(E702)
193-193: Variable MockTask in function should be lowercase
(N806)
backend/tests/apps/mentorship/model/test_program.py
1-1: File backend/tests/apps/mentorship/model/test_program.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
9-9: Module level import not at top of file
(E402)
10-10: Module level import not at top of file
(E402)
22-22: No newline at end of file
Add trailing newline
(W292)
🔇 Additional comments (7)
backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_program.py (2)
43-58: LGTM!The field assertions correctly verify the mock fixture's attribute assignments.
67-74: LGTM!The test correctly verifies PaginatedPrograms field assignments.
backend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.py (4)
12-18: LGTM!The
make_qshelper provides a clean way to mock Django QuerySets for testing. The implementation correctly supports iteration,.exists(), and.all()methods.
21-30: LGTM!The fixture correctly sets up the Command instance with mocked stdout, stderr, and style for testing. This follows Django testing best practices.
33-36: LGTM!Clean test for extracting repository full name from an object with a
pathattribute.
88-109: LGTM!Excellent test coverage for the repo-label map building logic. The test correctly validates:
- Label normalization (case-insensitive grouping)
- Proper aggregation by (repo_id, normalized_label) key
- Correct issue ID deduplication
- Console output messages
The mock variable naming (
MockIssue) is a minor style convention that can be optionally lowercased if you prefer strict adherence to PEP 8.backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_mentor.py (1)
36-68: Excellent test coverage!The test suite comprehensively covers both the presence and absence of
github_user, validating the fallback behavior to empty strings. The test structure is clear and well-organized.
backend/tests/apps/mentorship/api/internal/nodes/api_internal_mentee_test.py
Show resolved
Hide resolved
backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_program.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @backend/tests/apps/mentorship/model/test_module.py:
- Around line 1-10: Remove the unused imports and class to clean up the test:
delete the imports PublishedModuleManager, Mentor, Issue, and GithubUser from
the top import block and remove the unused MockIntegrityError class definition;
keep only the imports actually referenced (e.g., Module, Program, datetime,
pytest, django.utils.timezone, MagicMock/patch) and run tests to ensure no
remaining references to those symbols.
- Around line 59-64: In simulate_save_for_inheritance, simplify the redundant
conditional assignments: inside the if not mock_module.started_at block set
mock_module.started_at directly from mock_module.program.started_at (drop the
"or" with mock_module.started_at), and likewise inside the if not
mock_module.ended_at block set mock_module.ended_at directly from
mock_module.program.ended_at; update the assignments in the
simulate_save_for_inheritance function to remove the always-falsy left-side of
the "or" expressions.
🧹 Nitpick comments (4)
backend/tests/apps/mentorship/model/test_start_end_range.py (1)
7-18: Test provides minimal coverage.The test simply assigns datetime values to a mock and asserts them back, which doesn't validate any actual
StartEndRangemodel behavior or logic. SinceStartEndRangeis an abstract model with field declarations, this mock-based approach exercises very little.That said, given the maintainer's preference for pure mock tests (based on learnings), this approach is acceptable if the goal is to verify field presence rather than database-level behavior.
backend/tests/apps/mentorship/model/test_program.py (1)
6-15: Add trailing newline to fix linting warning.The tests correctly validate the
ProgramStatusenum values and the__str__method using lightweight mock-based testing (consistent with maintainer preference).🔎 Proposed fix
def test_str_returns_name(self): mock_program_instance = Program(name="Security Program") assert Program.__str__(mock_program_instance) == "Security Program" +backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_program.py (1)
60-146: Comprehensive coverage of node and input types.The tests effectively validate:
- Field assignments on ProgramNode
- Resolver behavior (admins)
- Input type annotations and default values
- Pagination structure
The use of
==for type annotation checks (flagged by E721) is appropriate in this context—you're checking the__annotations__dictionary, not performing isinstance checks.Optional: Consider timezone-aware datetimes for Django consistency.
While the naive datetime objects work for these mock tests, using timezone-aware datetimes (
django.utils.timezone.datetime(...)withtzinfo=timezone.utc) would better align with Django's defaultUSE_TZ=Truesetting and avoid potential confusion.backend/tests/apps/mentorship/model/test_module.py (1)
17-88: Add trailing newline and consider cleaning up test complexity.The file is missing a trailing newline (W292). Additionally, the
simulate_save_for_inheritancefunction has unused*args, **kwargsparameters that can be removed.🔎 Proposed fix
- def simulate_save_for_inheritance(*args, **kwargs): + def simulate_save_for_inheritance(): mock_module.key = "date-module" if not mock_module.started_at: mock_module.started_at = mock_module.program.started_at if not mock_module.ended_at: mock_module.ended_at = mock_module.program.ended_at mock_module.save.side_effect = simulate_save_for_inheritance mock_create_module.return_value = mock_module module = Module.objects.create(name="Auto Date Module", program=program_with_dates, project=self.project) module.save() assert module.key == "date-module" assert module.started_at == django.utils.timezone.datetime(2024, 2, 1, tzinfo=django.utils.timezone.UTC) assert module.ended_at == django.utils.timezone.datetime(2024, 2, 28, tzinfo=django.utils.timezone.UTC) explicit_start = django.utils.timezone.datetime(2024, 3, 1, tzinfo=django.utils.timezone.UTC) explicit_end = django.utils.timezone.datetime(2024, 3, 31, tzinfo=django.utils.timezone.UTC) mock_module.started_at = explicit_start mock_module.ended_at = explicit_end mock_create_module.return_value = mock_module module = Module.objects.create(name="Auto Date Module", program=program_with_dates, project=self.project) module.save() assert module.started_at == explicit_start assert module.ended_at == explicit_end assert module.key == "date-module" +Based on learnings, the maintainer prefers pure mock tests for this test suite, so the mock-based approach is appropriate despite its complexity.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/apps/mentorship/api/internal/nodes/mentee.pybackend/tests/apps/mentorship/api/internal/nodes/test_api_internal_program.pybackend/tests/apps/mentorship/model/test_module.pybackend/tests/apps/mentorship/model/test_program.pybackend/tests/apps/mentorship/model/test_start_end_range.py
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: kart-u
Repo: OWASP/Nest PR: 3179
File: backend/tests/apps/mentorship/model/test_program.py:13-22
Timestamp: 2026-01-06T21:11:03.537Z
Learning: In the mentorship test suite (backend/tests/apps/mentorship/), the maintainer kart-u prefers pure mock tests over Django TestCase with database interactions for model tests.
📚 Learning: 2026-01-06T21:11:03.537Z
Learnt from: kart-u
Repo: OWASP/Nest PR: 3179
File: backend/tests/apps/mentorship/model/test_program.py:13-22
Timestamp: 2026-01-06T21:11:03.537Z
Learning: In backend/tests/apps/mentorship/, prefer pure mock tests (no real database interactions) for model tests. Avoid using Django TestCase with database access; implement tests with mocks/stubs to exercise logic and, when needed, lightweight fixtures without DB. This guideline targets all Python test files under the mentorship module.
Applied to files:
backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_program.pybackend/tests/apps/mentorship/model/test_module.pybackend/tests/apps/mentorship/model/test_program.pybackend/tests/apps/mentorship/model/test_start_end_range.py
📚 Learning: 2025-07-13T05:55:46.436Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: backend/apps/mentorship/graphql/mutations/program.py:166-166
Timestamp: 2025-07-13T05:55:46.436Z
Learning: In the OWASP Nest mentorship GraphQL mutations, Strawberry GraphQL automatically converts between Django Program instances and ProgramNode types, so mutations can return Program instances directly without manual conversion even when typed as ProgramNode, similar to the Module/ModuleNode pattern.
Applied to files:
backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_program.py
📚 Learning: 2025-12-31T05:17:39.659Z
Learnt from: kart-u
Repo: OWASP/Nest PR: 3101
File: backend/apps/common/extensions.py:92-98
Timestamp: 2025-12-31T05:17:39.659Z
Learning: In this codebase, import OperationType for GraphQL operations from the graphql-core package rather than from strawberry. Use 'from graphql import OperationType'. Strawberry re-exports via graphql-core internally, so relying on strawberry's API may be brittle. Apply this rule to all Python files that deal with GraphQL operation types; ensure imports come from graphql (graphql-core) and not from strawberry packages. This improves compatibility and avoids coupling to strawberry's internals.
Applied to files:
backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_program.pybackend/apps/mentorship/api/internal/nodes/mentee.pybackend/tests/apps/mentorship/model/test_module.pybackend/tests/apps/mentorship/model/test_program.pybackend/tests/apps/mentorship/model/test_start_end_range.py
📚 Learning: 2026-01-01T17:48:23.963Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:41-47
Timestamp: 2026-01-01T17:48:23.963Z
Learning: In Django code, be aware that a QuerySet's boolean evaluation (e.g., if not queryset) runs a database query to determine emptiness. While it is technically valid to use the queryset in a boolean context, use queryset.exists() for existence checks to avoid unnecessary queries and improve performance. Applicable broadly to Python/Django files rather than just this specific path.
Applied to files:
backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_program.pybackend/apps/mentorship/api/internal/nodes/mentee.pybackend/tests/apps/mentorship/model/test_module.pybackend/tests/apps/mentorship/model/test_program.pybackend/tests/apps/mentorship/model/test_start_end_range.py
📚 Learning: 2025-09-29T06:02:35.566Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/components/SingleModuleCard.tsx:54-54
Timestamp: 2025-09-29T06:02:35.566Z
Learning: In the Module type from types/mentorship.ts, the experienceLevel field is required (experienceLevel: ExperienceLevelEnum), not optional, so null/undefined checks are not needed when accessing this field.
Applied to files:
backend/apps/mentorship/api/internal/nodes/mentee.py
📚 Learning: 2025-09-10T15:32:12.688Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 2211
File: backend/apps/nest/controllers/calendar_events.py:0-0
Timestamp: 2025-09-10T15:32:12.688Z
Learning: In the backend/apps/nest/controllers/calendar_events.py file, the scheduled_time parameter in the schedule_reminder function is guaranteed to be timezone-aware, so explicit timezone awareness validation is not needed.
Applied to files:
backend/tests/apps/mentorship/model/test_module.py
📚 Learning: 2025-07-14T16:18:07.287Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/components/ModuleForm.tsx:112-134
Timestamp: 2025-07-14T16:18:07.287Z
Learning: In the OWASP Nest mentorship system, date validation for modules (ensuring start date precedes end date) is handled on the backend in the module GraphQL mutations via the `_validate_module_dates` helper function in backend/apps/mentorship/graphql/mutations/module.py, which prevents invalid date ranges from being stored in the database.
Applied to files:
backend/tests/apps/mentorship/model/test_module.py
📚 Learning: 2025-07-08T17:24:36.501Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: backend/apps/mentorship/graphql/mutations/program.py:41-44
Timestamp: 2025-07-08T17:24:36.501Z
Learning: In the mentorship program GraphQL mutations, date validation is handled at the GraphQL schema/node level in the input types (CreateProgramInput, UpdateProgramInput), preventing null values from reaching the mutation logic where date comparisons are performed.
Applied to files:
backend/tests/apps/mentorship/model/test_module.py
🧬 Code graph analysis (4)
backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_program.py (1)
backend/apps/mentorship/api/internal/nodes/enum.py (2)
ExperienceLevelEnum(12-18)ProgramStatusEnum(22-27)
backend/tests/apps/mentorship/model/test_module.py (2)
backend/apps/mentorship/models/managers/module.py (1)
PublishedModuleManager(8-13)backend/apps/mentorship/models/program.py (1)
ProgramStatus(25-28)
backend/tests/apps/mentorship/model/test_program.py (1)
backend/apps/mentorship/models/program.py (1)
ProgramStatus(25-28)
backend/tests/apps/mentorship/model/test_start_end_range.py (1)
backend/apps/mentorship/models/common/start_end_range.py (1)
StartEndRange(6-13)
🪛 Ruff (0.14.10)
backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_program.py
1-1: File backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_program.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
1-2: 1 blank line required between summary line and description
(D205)
1-2: Multi-line docstring closing quotes should be on a separate line
Move closing quotes to new line
(D209)
21-23: 1 blank line required between summary line and description
(D205)
31-31: datetime.datetime() called without a tzinfo argument
(DTZ001)
34-34: datetime.datetime() called without a tzinfo argument
(DTZ001)
67-67: datetime.datetime() called without a tzinfo argument
(DTZ001)
72-72: datetime.datetime() called without a tzinfo argument
(DTZ001)
96-96: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
97-97: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
100-100: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
107-107: datetime.datetime.now() called without a tz argument
(DTZ005)
109-109: datetime.datetime.now() called without a tz argument
(DTZ005)
117-117: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
118-118: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
119-119: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
123-123: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
132-132: datetime.datetime.now() called without a tz argument
(DTZ005)
134-134: datetime.datetime.now() called without a tz argument
(DTZ005)
144-144: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
145-145: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
backend/tests/apps/mentorship/model/test_module.py
1-1: File backend/tests/apps/mentorship/model/test_module.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
39-43: 1 blank line required between summary line and description
(D205)
39-43: Multi-line docstring summary should start at the first line
Remove whitespace after opening quotes
(D212)
59-59: Unused function argument: args
(ARG001)
59-59: Unused function argument: kwargs
(ARG001)
88-88: No newline at end of file
Add trailing newline
(W292)
backend/tests/apps/mentorship/model/test_program.py
1-1: File backend/tests/apps/mentorship/model/test_program.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
15-15: No newline at end of file
Add trailing newline
(W292)
backend/tests/apps/mentorship/model/test_start_end_range.py
1-1: File backend/tests/apps/mentorship/model/test_start_end_range.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
9-9: datetime.datetime() called without a tzinfo argument
(DTZ001)
10-10: datetime.datetime() called without a tzinfo argument
(DTZ001)
🔇 Additional comments (2)
backend/apps/mentorship/api/internal/nodes/mentee.py (1)
27-30: LGTM! Ensures consistent string output for experience level.The change to return
self.experience_level.valueensures the resolver consistently yields a string value instead of the enum member itself, which aligns with GraphQL string field expectations. The fallback to"beginner"when the field is not set is preserved.backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_program.py (1)
20-44: Well-designed test fixture for resolver testing.The
FakeProgramNodeclass provides a clean way to test the admins resolver pattern in isolation while maintaining control over the underlying manager. This approach effectively validates the resolver logic without requiring a full GraphQL or database setup.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @backend/apps/mentorship/api/internal/nodes/mentee.py:
- Line 30: The file ends without a trailing newline on the last line containing
the return expression for the experience level; open the file containing the
method/property that returns "self.experience_level if self.experience_level
else 'beginner'" and add a newline character at the end of the file so the final
line is terminated.
- Around line 27-30: The resolver resolve_experience_level currently declares a
return type of str but the GraphQL field (experienceLevel) is declared as
ExperienceLevelEnum; make them match by either changing the resolver to return
an ExperienceLevelEnum (import and return ExperienceLevelEnum.BEGINNER as the
fallback instead of "beginner") or changing the field declaration to type str;
update the resolver signature to -> ExperienceLevelEnum and ensure the returned
value is an enum member (or change the field's type to str if string returns are
intended).
🧹 Nitpick comments (1)
backend/apps/mentorship/api/internal/nodes/mentee.py (1)
27-30: Remove redundant null check on required field.Per the retrieved learnings,
experience_levelis a required field (non-optional). The conditional check at line 30 is unnecessary since the field cannot beNoneor falsy. Simplify to a direct return.🔎 Proposed simplification
@strawberry.field(name="experienceLevel") def resolve_experience_level(self) -> str: """Get the experience level of the mentee.""" - return self.experience_level if self.experience_level else "beginner" + return self.experience_level
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/apps/mentorship/api/internal/nodes/mentee.py
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: kart-u
Repo: OWASP/Nest PR: 3179
File: backend/tests/apps/mentorship/model/test_program.py:13-22
Timestamp: 2026-01-06T21:11:03.537Z
Learning: In the mentorship test suite (backend/tests/apps/mentorship/), the maintainer kart-u prefers pure mock tests over Django TestCase with database interactions for model tests.
📚 Learning: 2025-09-29T06:02:35.566Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/components/SingleModuleCard.tsx:54-54
Timestamp: 2025-09-29T06:02:35.566Z
Learning: In the Module type from types/mentorship.ts, the experienceLevel field is required (experienceLevel: ExperienceLevelEnum), not optional, so null/undefined checks are not needed when accessing this field.
Applied to files:
backend/apps/mentorship/api/internal/nodes/mentee.py
📚 Learning: 2025-12-31T05:17:39.659Z
Learnt from: kart-u
Repo: OWASP/Nest PR: 3101
File: backend/apps/common/extensions.py:92-98
Timestamp: 2025-12-31T05:17:39.659Z
Learning: In this codebase, import OperationType for GraphQL operations from the graphql-core package rather than from strawberry. Use 'from graphql import OperationType'. Strawberry re-exports via graphql-core internally, so relying on strawberry's API may be brittle. Apply this rule to all Python files that deal with GraphQL operation types; ensure imports come from graphql (graphql-core) and not from strawberry packages. This improves compatibility and avoids coupling to strawberry's internals.
Applied to files:
backend/apps/mentorship/api/internal/nodes/mentee.py
📚 Learning: 2026-01-01T17:48:23.963Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:41-47
Timestamp: 2026-01-01T17:48:23.963Z
Learning: In Django code, be aware that a QuerySet's boolean evaluation (e.g., if not queryset) runs a database query to determine emptiness. While it is technically valid to use the queryset in a boolean context, use queryset.exists() for existence checks to avoid unnecessary queries and improve performance. Applicable broadly to Python/Django files rather than just this specific path.
Applied to files:
backend/apps/mentorship/api/internal/nodes/mentee.py
🪛 Ruff (0.14.10)
backend/apps/mentorship/api/internal/nodes/mentee.py
30-30: No newline at end of file
Add trailing newline
(W292)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@backend/tests/apps/mentorship/api/internal/queries/test_api_queries_mentorship.py:
- Around line 1-11: Tests import symbols like MenteeNode, MentorshipQuery,
Mentee, MenteeModule, Module and Mentor from the mentorship test package but
several package directories are missing __init__.py so Python treats them as
non-packages; add an empty __init__.py to each package level in the mentorship
test package tree (so the test module that imports MenteeNode and
MentorshipQuery can be discovered and imports resolve) and re-run tests to
ensure the imports in test_api_queries_mentorship.py work.
🧹 Nitpick comments (7)
backend/tests/apps/mentorship/model/test_module.py (3)
53-53: Remove or prefix unused function arguments.The
argsandkwargsparameters are not used insimulate_save_for_inheritance. Either remove them or prefix with an underscore to indicate they're intentionally unused.✨ Proposed fix
- def simulate_save_for_inheritance(*args, **kwargs): + def simulate_save_for_inheritance(*_args, **_kwargs): mock_module.key = "date-module" if not mock_module.started_at: mock_module.started_at = mock_module.program.started_at if not mock_module.ended_at: mock_module.ended_at = mock_module.program.ended_atOr remove them entirely if not needed:
- def simulate_save_for_inheritance(*args, **kwargs): + def simulate_save_for_inheritance(): mock_module.key = "date-module" if not mock_module.started_at: mock_module.started_at = mock_module.program.started_at if not mock_module.ended_at: mock_module.ended_at = mock_module.program.ended_at
33-37: Fix docstring formatting.The docstring should follow PEP 257 conventions: summary line should start at the first line and be separated from the description by a blank line.
📝 Proposed fix
- def test_module_save(self, mock_create_module): - """ - We cannot call the real Model.save without DB/app registry here, - so simulate the save behavior by attaching a side_effect to a mock instance's save method. - This verifies the *expected* effects (key slugification and date inheritance) in a pure-mock way. - """ + def test_module_save(self, mock_create_module): + """Test Module.save behavior with pure mocks. + + We cannot call the real Model.save without DB/app registry here, + so simulate the save behavior by attaching a side_effect to a mock instance's save method. + This verifies the *expected* effects (key slugification and date inheritance) in a pure-mock way. + """
82-82: Add newline at end of file.The file is missing a trailing newline, which is a common Python convention.
📝 Proposed fix
assert module.started_at == explicit_start assert module.ended_at == explicit_end assert module.key == "date-module" +backend/tests/apps/mentorship/api/internal/queries/test_api_queries_mentorship.py (4)
81-98: Minor formatting: add spaces around=in attribute assignments.For consistency with PEP 8 style, add spaces around the assignment operator when setting mock attributes.
🔧 Suggested formatting
mock_module = MagicMock(spec=Module) - mock_module.id=1 + mock_module.id = 1 mock_module_only.return_value.get.return_value = mock_module mock_github_user = MagicMock(spec=GithubUser) - mock_github_user.login="testmentee" - mock_github_user.name="Test_Mentee" - mock_github_user.avatar_url="url" - mock_github_user.bio="bio" + mock_github_user.login = "testmentee" + mock_github_user.name = "Test_Mentee" + mock_github_user.avatar_url = "url" + mock_github_user.bio = "bio" mock_github_user_only.return_value.get.return_value = mock_github_user mock_mentee = MagicMock(spec=Mentee) - mock_mentee.id=2 - mock_mentee.experience_level="Mid" - mock_mentee.domains=["Web"] - mock_mentee.tags=["Python"] + mock_mentee.id = 2 + mock_mentee.experience_level = "Mid" + mock_mentee.domains = ["Web"] + mock_mentee.tags = ["Python"]
243-243: Consider usingassert_any_callor verifying call count for clarity.
GithubUser.objects.onlyis called twice inget_mentee_module_issues: once with("id")for the main lookup and once with("id", "login", "name", "avatar_url")in the Prefetch. Usingassert_called_withonly verifies the last call matched. For explicit verification of both calls, consider:# Option 1: Verify both calls explicitly assert mock_github_user_only.call_count == 2 mock_github_user_only.assert_any_call("id") mock_github_user_only.assert_any_call("id", "login", "name", "avatar_url")
346-348: Remove unused mock variables.Static analysis correctly identifies that
mock_issue1andmock_issue3are defined but never used in the test. Since onlymock_issue2appears in the slice, remove the unused variables.♻️ Proposed fix
- mock_issue1 = MagicMock(id=1, number=1, title="Issue 1", state="open", created_at="", url="") mock_issue2 = MagicMock(id=2, number=2, title="Issue 2", state="closed", created_at="", url="") - mock_issue3 = MagicMock(id=3, number=3, title="Issue 3", state="open", created_at="", url="")
206-206: Minor: add space after colon in type hint.- mock_prefetch:MagicMock + mock_prefetch: MagicMockThe same applies to lines 334 and 384 where
mock_prefetch.return_value=MagicMock()should have spaces around=.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/tests/apps/mentorship/api/internal/queries/test_api_queries_mentorship.pybackend/tests/apps/mentorship/model/test_module.py
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: kart-u
Repo: OWASP/Nest PR: 3179
File: backend/tests/apps/mentorship/model/test_program.py:13-22
Timestamp: 2026-01-06T21:11:03.537Z
Learning: In the mentorship test suite (backend/tests/apps/mentorship/), the maintainer kart-u prefers pure mock tests over Django TestCase with database interactions for model tests.
📚 Learning: 2026-01-06T21:11:03.537Z
Learnt from: kart-u
Repo: OWASP/Nest PR: 3179
File: backend/tests/apps/mentorship/model/test_program.py:13-22
Timestamp: 2026-01-06T21:11:03.537Z
Learning: In backend/tests/apps/mentorship/, prefer pure mock tests (no real database interactions) for model tests. Avoid using Django TestCase with database access; implement tests with mocks/stubs to exercise logic and, when needed, lightweight fixtures without DB. This guideline targets all Python test files under the mentorship module.
Applied to files:
backend/tests/apps/mentorship/api/internal/queries/test_api_queries_mentorship.pybackend/tests/apps/mentorship/model/test_module.py
📚 Learning: 2025-12-31T05:17:39.659Z
Learnt from: kart-u
Repo: OWASP/Nest PR: 3101
File: backend/apps/common/extensions.py:92-98
Timestamp: 2025-12-31T05:17:39.659Z
Learning: In this codebase, import OperationType for GraphQL operations from the graphql-core package rather than from strawberry. Use 'from graphql import OperationType'. Strawberry re-exports via graphql-core internally, so relying on strawberry's API may be brittle. Apply this rule to all Python files that deal with GraphQL operation types; ensure imports come from graphql (graphql-core) and not from strawberry packages. This improves compatibility and avoids coupling to strawberry's internals.
Applied to files:
backend/tests/apps/mentorship/api/internal/queries/test_api_queries_mentorship.pybackend/tests/apps/mentorship/model/test_module.py
📚 Learning: 2026-01-01T17:48:23.963Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:41-47
Timestamp: 2026-01-01T17:48:23.963Z
Learning: In Django code, be aware that a QuerySet's boolean evaluation (e.g., if not queryset) runs a database query to determine emptiness. While it is technically valid to use the queryset in a boolean context, use queryset.exists() for existence checks to avoid unnecessary queries and improve performance. Applicable broadly to Python/Django files rather than just this specific path.
Applied to files:
backend/tests/apps/mentorship/api/internal/queries/test_api_queries_mentorship.pybackend/tests/apps/mentorship/model/test_module.py
📚 Learning: 2025-09-10T15:32:12.688Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 2211
File: backend/apps/nest/controllers/calendar_events.py:0-0
Timestamp: 2025-09-10T15:32:12.688Z
Learning: In the backend/apps/nest/controllers/calendar_events.py file, the scheduled_time parameter in the schedule_reminder function is guaranteed to be timezone-aware, so explicit timezone awareness validation is not needed.
Applied to files:
backend/tests/apps/mentorship/model/test_module.py
📚 Learning: 2025-07-14T16:18:07.287Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/components/ModuleForm.tsx:112-134
Timestamp: 2025-07-14T16:18:07.287Z
Learning: In the OWASP Nest mentorship system, date validation for modules (ensuring start date precedes end date) is handled on the backend in the module GraphQL mutations via the `_validate_module_dates` helper function in backend/apps/mentorship/graphql/mutations/module.py, which prevents invalid date ranges from being stored in the database.
Applied to files:
backend/tests/apps/mentorship/model/test_module.py
📚 Learning: 2025-07-08T17:24:36.501Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: backend/apps/mentorship/graphql/mutations/program.py:41-44
Timestamp: 2025-07-08T17:24:36.501Z
Learning: In the mentorship program GraphQL mutations, date validation is handled at the GraphQL schema/node level in the input types (CreateProgramInput, UpdateProgramInput), preventing null values from reaching the mutation logic where date comparisons are performed.
Applied to files:
backend/tests/apps/mentorship/model/test_module.py
🧬 Code graph analysis (2)
backend/tests/apps/mentorship/api/internal/queries/test_api_queries_mentorship.py (1)
backend/apps/mentorship/api/internal/queries/mentorship.py (3)
is_mentor(36-48)get_mentee_details(51-82)get_mentee_module_issues(85-124)
backend/tests/apps/mentorship/model/test_module.py (2)
backend/apps/mentorship/models/program.py (1)
ProgramStatus(25-28)backend/tests/apps/mentorship/model/test_program.py (1)
test_str_returns_name(13-15)
🪛 Ruff (0.14.10)
backend/tests/apps/mentorship/api/internal/queries/test_api_queries_mentorship.py
1-1: File backend/tests/apps/mentorship/api/internal/queries/test_api_queries_mentorship.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
346-346: Local variable mock_issue1 is assigned to but never used
Remove assignment to unused variable mock_issue1
(F841)
348-348: Local variable mock_issue3 is assigned to but never used
Remove assignment to unused variable mock_issue3
(F841)
backend/tests/apps/mentorship/model/test_module.py
1-1: File backend/tests/apps/mentorship/model/test_module.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
33-37: 1 blank line required between summary line and description
(D205)
33-37: Multi-line docstring summary should start at the first line
Remove whitespace after opening quotes
(D212)
53-53: Unused function argument: args
(ARG001)
53-53: Unused function argument: kwargs
(ARG001)
82-82: No newline at end of file
Add trailing newline
(W292)
🔇 Additional comments (4)
backend/tests/apps/mentorship/model/test_module.py (2)
11-21: LGTM!The setup correctly uses timezone-aware datetimes and properly mocks the required models.
23-28: LGTM!The pure mock test correctly validates the
__str__implementation.backend/tests/apps/mentorship/api/internal/queries/test_api_queries_mentorship.py (2)
13-63: LGTM!The
TestIsMentorclass provides comprehensive coverage of theis_mentorquery, including edge cases for empty/whitespace/None logins. The parametrized test is a clean approach for testing multiple invalid inputs. Based on learnings, the pure mock approach aligns with the preferred testing style for this module.
192-408: Good test coverage forget_mentee_module_issues.The test class comprehensively covers the success path, all exception scenarios (module/user/mentee not found, not enrolled), and pagination edge cases including out-of-range offsets. The mock chain for
module.issues.filter().only().prefetch_related().order_by()correctly simulates the QuerySet behavior.
backend/tests/apps/mentorship/api/internal/queries/api_queries_mentorship_test.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (11)
backend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.py (1)
39-51: Fix the parametrize decorator to use a tuple.The first argument to
pytest.mark.parametrizeshould be a tuple of parameter names, not a comma-separated string. While pytest accepts strings for historical reasons, using a tuple is the recommended approach.♻️ Proposed fix
@pytest.mark.parametrize( - "repo_url, expected", + ("repo_url", "expected"), [ ("https://github.com/owner/repo", "owner/repo"),backend/tests/apps/mentorship/management/commands/test_mentorship_update_comments.py (6)
4-5: Remove unused datetime imports.Neither
datetimenordtare used in this file.♻️ Proposed fix
-from datetime import datetime -import datetime as dt -
10-10: Consider makingexistsa keyword-only parameter.Boolean flags as positional arguments reduce readability. Making it keyword-only (add
*before the parameter) ensures callers must explicitly writeexists=False, improving clarity.♻️ Proposed fix
-def make_qs(iterable, exists=True): +def make_qs(iterable, *, exists=True):
29-34: Consider using a realistic default forcreated_at.The fallback
"now"is a string, but the actual code likely expects a datetime object. Using a more realistic mock default (e.g.,datetime.now()or a fixed datetime) would make tests more robust.
88-88: Renamemock_issue_1to clarify it patches the Issue class.The name
mock_issue_1suggests a specific issue instance, but it patches theIssueclass (model). Combined with themock_issuefixture parameter, this creates confusion. Consider renaming tomock_issue_classormock_issue_model.♻️ Proposed fix
-def test_process_module(mock_issue_1, mock_process_issue_interests, mock_sync_issue_comments, mock_get_github_client, command, mock_module, mock_issue): +def test_process_module(mock_issue_class, mock_process_issue_interests, mock_sync_issue_comments, mock_get_github_client, command, mock_module, mock_issue): """Test process_module orchestrates issue syncing and interest processing.""" mock_issue.id = 1 mock_issue.title = "Test Issue 1" mock_issue.number = 123 issues_qs = make_qs([mock_issue], exists=True) - mock_issue_1.objects.filter.return_value.distinct.return_value = issues_qs + mock_issue_class.objects.filter.return_value.distinct.return_value = issues_qs
124-128: Simplify fragile assertion chain.The triple index
call_args[0][0][0]is hard to follow. Consider using intermediate variables orcall_args.args[0][0]for better clarity.♻️ Proposed fix
mock_issue_user_interest.objects.bulk_create.assert_called_once() - created_interest = mock_issue_user_interest.objects.bulk_create.call_args[0][0][0] + bulk_create_call_args = mock_issue_user_interest.objects.bulk_create.call_args.args + interests_list = bulk_create_call_args[0] + created_interest = interests_list[0] assert created_interest.module == mock_module
134-186: Consider consolidating duplicate test cases.Both
test_process_issue_interests_remove_interest(lines 134-156) andtest_process_issue_interests_existing_interest_removed(lines 159-186) test the same scenario: a user with an existing interest posts a comment that doesn't match interest patterns, resulting in interest removal. The only difference is the comment body text. Consider usingpytest.mark.parametrizeto avoid duplication.♻️ Proposed refactor using parametrize
+@pytest.mark.parametrize("comment_body", [ + "Not interested anymore", + "Just a regular comment", +]) @patch("apps.mentorship.management.commands.mentorship_update_comments.IssueUserInterest") -def test_process_issue_interests_remove_interest(mock_issue_user_interest, command, mock_issue, mock_module): - """Test process_issue_interests correctly removes interests.""" +def test_process_issue_interests_remove_interest(comment_body, mock_issue_user_interest, command, mock_issue, mock_module): + """Test process_issue_interests removes interests when user posts non-interested comment.""" user1 = make_user(1, "user1") - comment1 = make_comment(body="Not interested anymore", author=user1, created_at="2023-01-01") + comment1 = make_comment(body=comment_body, author=user1, created_at="2023-01-01") comments_qs = make_qs([comment1], exists=True) mock_issue.comments.select_related.return_value.filter.return_value.order_by.return_value = comments_qs mock_issue_user_interest.objects.filter.return_value.values_list.return_value = [1] mock_issue_user_interest.objects.filter.return_value.delete.return_value = (1, {}) mock_issue_user_interest.side_effect = lambda **kwargs: SimpleNamespace(**kwargs) mock_issue_user_interest.objects.bulk_create = MagicMock() command.process_issue_interests(mock_issue, mock_module) mock_issue_user_interest.objects.bulk_create.assert_not_called() - mock_issue_user_interest.objects.filter.assert_any_call(module=mock_module, issue=mock_issue, user_id__in=[1]) + + filter_calls = mock_issue_user_interest.objects.filter.call_args_list + found_delete_filter = any( + c.kwargs.get("user_id__in") == [1] and c.kwargs.get("module") == mock_module + for c in filter_calls + ) + assert found_delete_filter + mock_issue_user_interest.objects.filter.return_value.delete.assert_called_once() - command.stdout.write.assert_any_call( - command.style.WARNING("Unregistered 1 interest(s) for issue #123: user1") - ) - - -@patch("apps.mentorship.management.commands.mentorship_update_comments.IssueUserInterest") -def test_process_issue_interests_existing_interest_removed( - mock_issue_user_interest, command, mock_issue, mock_module -): - """Existing interest should be removed when user no longer shows interest.""" - mock_issue.number = 1 - - user1 = make_user(1, "user1") - comment1 = make_comment(body="Just a regular comment", author=user1, created_at="2023-01-01") - - comments_qs = make_qs([comment1], exists=True) - mock_issue.comments.select_related.return_value.filter.return_value.order_by.return_value = comments_qs - - mock_issue_user_interest.objects.filter.return_value.values_list.return_value = [1] - mock_issue_user_interest.objects.filter.return_value.delete.return_value = (1, {}) - - mock_issue_user_interest.side_effect = lambda **kwargs: SimpleNamespace(**kwargs) - mock_issue_user_interest.objects.bulk_create = MagicMock() - - command.process_issue_interests(mock_issue, mock_module) - - mock_issue_user_interest.objects.bulk_create.assert_not_called() - - mock_issue_user_interest.objects.filter.return_value.delete.assert_called_once() - command.stdout.write.assert_any_call( - command.style.WARNING("Unregistered 1 interest(s) for issue #1: user1") - )Note: You could then remove the duplicate test function.
backend/tests/apps/mentorship/management/commands/test_mentorship_sync_issue_levels.py (4)
17-23: Consider minor style improvements.The helper function works correctly but has two minor style issues flagged by static analysis:
- Boolean positional argument
exists=True— consider making it keyword-only- Docstring should use imperative mood
✨ Optional style refinements
-def make_qs(iterable, exists=True): - """Helper: return a queryset-like MagicMock that is iterable and supports .exists().""" +def make_qs(iterable, *, exists=True): + """Return a queryset-like MagicMock that is iterable and supports .exists().""" qs = MagicMock(name="QuerySet") qs.exists.return_value = exists qs.__iter__.return_value = iter(iterable) qs.all.return_value = list(iterable) return qs
26-34: Remove blank line after docstring.Static analysis flags an unnecessary blank line after the docstring on line 29.
✨ Style fix
@patch("apps.mentorship.management.commands.mentorship_sync_issue_levels.TaskLevel") def test_handle_no_task_levels(mock_task_level, command): """When no TaskLevel objects exist, command should exit early with a warning.""" - empty_qs = make_qs([], exists=False) mock_task_level.objects.select_related.return_value.order_by.return_value = empty_qs
52-54: Split multiple statements to separate lines.Multiple statements are combined on single lines using semicolons, which reduces readability.
✨ Style fix
- label_a = MagicMock(); label_a.name = "Label-A" - label_b = MagicMock(); label_b.name = "Label-B" - label_nomatch = MagicMock(); label_nomatch.name = "No-Match" + label_a = MagicMock() + label_a.name = "Label-A" + label_b = MagicMock() + label_b.name = "Label-B" + label_nomatch = MagicMock() + label_nomatch.name = "No-Match"And similarly for line 106:
- label_a = MagicMock(); label_a.name = "Label-A" + label_a = MagicMock() + label_a.name = "Label-A"Also applies to: 106-106
95-99: Remove blank line after docstring.Static analysis flags an unnecessary blank line after the docstring.
✨ Style fix
def test_handle_no_updates_needed(mock_task_level, mock_issue, command): """When all issues already have the correct level, bulk_update is not called.""" mock_module = MagicMock()
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/tests/apps/mentorship/api/internal/queries/test_api_queries_mentorship.pybackend/tests/apps/mentorship/management/__init__.pybackend/tests/apps/mentorship/management/commands/__init__.pybackend/tests/apps/mentorship/management/commands/test_mentorship_sync_issue_levels.pybackend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.pybackend/tests/apps/mentorship/management/commands/test_mentorship_update_comments.pybackend/tests/apps/mentorship/model/__init__.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/tests/apps/mentorship/api/internal/queries/test_api_queries_mentorship.py
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: kart-u
Repo: OWASP/Nest PR: 3179
File: backend/tests/apps/mentorship/model/test_program.py:13-22
Timestamp: 2026-01-06T21:11:08.745Z
Learning: In the mentorship test suite (backend/tests/apps/mentorship/), the maintainer kart-u prefers pure mock tests over Django TestCase with database interactions for model tests.
📚 Learning: 2026-01-06T21:11:03.537Z
Learnt from: kart-u
Repo: OWASP/Nest PR: 3179
File: backend/tests/apps/mentorship/model/test_program.py:13-22
Timestamp: 2026-01-06T21:11:03.537Z
Learning: In backend/tests/apps/mentorship/, prefer pure mock tests (no real database interactions) for model tests. Avoid using Django TestCase with database access; implement tests with mocks/stubs to exercise logic and, when needed, lightweight fixtures without DB. This guideline targets all Python test files under the mentorship module.
Applied to files:
backend/tests/apps/mentorship/management/commands/test_mentorship_update_comments.pybackend/tests/apps/mentorship/management/commands/test_mentorship_sync_issue_levels.pybackend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.py
📚 Learning: 2025-12-18T05:39:42.678Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:40-40
Timestamp: 2025-12-18T05:39:42.678Z
Learning: In Django management commands, prefer using self.stdout.write(...) over print(...) for user-facing stdout output. This aligns with Django conventions and improves testability. When emitting messages, consider using self.stdout.write and, for styled messages, use self.style.SUCCESS/ERROR as appropriate to maintain consistent command output formatting. Apply this guideline to all Python files within any project's management/commands directory.
Applied to files:
backend/tests/apps/mentorship/management/commands/test_mentorship_update_comments.pybackend/tests/apps/mentorship/management/commands/test_mentorship_sync_issue_levels.pybackend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.py
📚 Learning: 2025-12-31T05:17:39.659Z
Learnt from: kart-u
Repo: OWASP/Nest PR: 3101
File: backend/apps/common/extensions.py:92-98
Timestamp: 2025-12-31T05:17:39.659Z
Learning: In this codebase, import OperationType for GraphQL operations from the graphql-core package rather than from strawberry. Use 'from graphql import OperationType'. Strawberry re-exports via graphql-core internally, so relying on strawberry's API may be brittle. Apply this rule to all Python files that deal with GraphQL operation types; ensure imports come from graphql (graphql-core) and not from strawberry packages. This improves compatibility and avoids coupling to strawberry's internals.
Applied to files:
backend/tests/apps/mentorship/management/commands/test_mentorship_update_comments.pybackend/tests/apps/mentorship/management/commands/test_mentorship_sync_issue_levels.pybackend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.py
📚 Learning: 2026-01-01T17:48:23.963Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:41-47
Timestamp: 2026-01-01T17:48:23.963Z
Learning: In Django code, be aware that a QuerySet's boolean evaluation (e.g., if not queryset) runs a database query to determine emptiness. While it is technically valid to use the queryset in a boolean context, use queryset.exists() for existence checks to avoid unnecessary queries and improve performance. Applicable broadly to Python/Django files rather than just this specific path.
Applied to files:
backend/tests/apps/mentorship/management/commands/test_mentorship_update_comments.pybackend/tests/apps/mentorship/management/commands/test_mentorship_sync_issue_levels.pybackend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.py
📚 Learning: 2025-08-17T11:55:55.990Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2000
File: backend/apps/mentorship/management/commands/sync_module_issues.py:109-145
Timestamp: 2025-08-17T11:55:55.990Z
Learning: In the OWASP/Nest mentorship system, tasks are designed to be assigned to only one assignee per issue, even if GitHub issues can have multiple assignees. The sync_module_issues command correctly uses issue.assignees.first() to create one task per issue for the first assignee only.
Applied to files:
backend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.py
🧬 Code graph analysis (3)
backend/tests/apps/mentorship/management/commands/test_mentorship_update_comments.py (1)
backend/apps/mentorship/management/commands/mentorship_update_comments.py (3)
process_mentorship_modules(31-55)process_module(57-89)process_issue_interests(91-160)
backend/tests/apps/mentorship/management/commands/test_mentorship_sync_issue_levels.py (2)
backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py (3)
select_related(36-38)order_by(32-34)prefetch_related(40-42)backend/apps/github/api/internal/nodes/issue.py (1)
labels(50-52)
backend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.py (1)
backend/apps/mentorship/management/commands/mentorship_sync_module_issues.py (5)
_extract_repo_full_name(28-40)_get_status(42-50)_get_last_assigned_date(52-74)_build_repo_label_to_issue_map(76-92)_process_module(94-203)
🪛 Ruff (0.14.10)
backend/tests/apps/mentorship/management/commands/test_mentorship_update_comments.py
10-10: Boolean default positional argument in function definition
(FBT002)
backend/tests/apps/mentorship/management/commands/test_mentorship_sync_issue_levels.py
17-17: Boolean default positional argument in function definition
(FBT002)
18-18: First line of docstring should be in imperative mood: "Helper: return a queryset-like MagicMock that is iterable and supports .exists()."
(D401)
28-28: No blank lines allowed after function docstring (found 1)
Remove blank line(s) after function docstring
(D202)
40-40: No blank lines allowed after function docstring (found 1)
Remove blank line(s) after function docstring
(D202)
52-52: Multiple statements on one line (semicolon)
(E702)
53-53: Multiple statements on one line (semicolon)
(E702)
54-54: Multiple statements on one line (semicolon)
(E702)
106-106: Multiple statements on one line (semicolon)
(E702)
backend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.py
12-12: Boolean default positional argument in function definition
(FBT002)
40-40: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
55-55: Multiple statements on one line (semicolon)
(E702)
68-68: Use datetime.UTC alias
Convert to datetime.UTC alias
(UP017)
69-69: Use datetime.UTC alias
Convert to datetime.UTC alias
(UP017)
70-70: Use datetime.UTC alias
Convert to datetime.UTC alias
(UP017)
71-71: Use datetime.UTC alias
Convert to datetime.UTC alias
(UP017)
75-75: Use datetime.UTC alias
Convert to datetime.UTC alias
(UP017)
115-115: Multiple statements on one line (semicolon)
(E702)
115-115: Multiple statements on one line (semicolon)
(E702)
120-120: Multiple statements on one line (semicolon)
(E702)
120-120: Multiple statements on one line (semicolon)
(E702)
127-127: Multiple statements on one line (semicolon)
(E702)
186-186: Multiple statements on one line (semicolon)
(E702)
186-186: Multiple statements on one line (semicolon)
(E702)
🔇 Additional comments (8)
backend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.py (6)
21-30: LGTM!The command fixture is well-structured, properly mocking stdout, stderr, and style methods for isolated command testing.
54-60: LGTM!The test comprehensively covers all status mapping scenarios (closed, open with assignee, open without assignee).
63-85: LGTM!Excellent coverage of the assigned date retrieval logic, including the happy path (finding the latest assignment), not-found case, and exception handling with stderr output.
88-109: LGTM!This test effectively validates the repo-label map building logic, including case-insensitive label normalization and proper aggregation across multiple repositories.
112-182: LGTM!This comprehensive test validates the core module processing logic including:
- Issue linkage via
module.issues.set- Task creation with correct status mapping (IN_PROGRESS for open+assigned, COMPLETED for closed)
- Assigned date retrieval and updating
- Success message output
The assertion for
num_linked(line 158) correctly verifies the return value. All past review concerns have been addressed.
185-208: LGTM!This test correctly validates the edge case where no issues match the module's labels, ensuring that:
- An empty set is assigned to
module.issues- No tasks are created
- The return value correctly indicates zero linked issues
Past signature issues have been resolved.
backend/tests/apps/mentorship/management/commands/test_mentorship_update_comments.py (1)
214-259: Comprehensive multi-user interest test coverage.This test effectively validates the complex scenario of simultaneous interest creation and removal across multiple users. The assertions correctly verify both the bulk_create and delete operations, ensuring the command handles mixed interest states properly.
backend/tests/apps/mentorship/management/commands/test_mentorship_sync_issue_levels.py (1)
7-14: LGTM!The fixture correctly mocks the command's stdout and style attributes for testing Django management command output.
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @kart-u. * #3179 (comment) The following files were modified: * `backend/apps/mentorship/api/internal/nodes/mentee.py` * `backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_mentor.py` * `backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_module.py` * `backend/tests/apps/mentorship/api/internal/nodes/test_api_internal_program.py` * `backend/tests/apps/mentorship/api/internal/queries/test_api_queries_mentorship.py` * `backend/tests/apps/mentorship/api/internal/queries/test_api_queries_program.py` * `backend/tests/apps/mentorship/management/commands/test_mentorship_sync_issue_levels.py` * `backend/tests/apps/mentorship/management/commands/test_mentorship_sync_module_issues.py` * `backend/tests/apps/mentorship/management/commands/test_mentorship_update_comments.py` * `backend/tests/apps/mentorship/model/test_module.py`
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
@backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py:
- Around line 312-328: The test `test_module_node_interested_users_no_issue`
incorrectly contains unrelated `task_deadline` assertions; remove the block that
patches `apps.mentorship.models.task.Task.objects` and the subsequent `deadline`
assertions from that function, and create a new standalone test function named
`test_module_node_task_deadline` that contains the patched `Task.objects` setup,
the call to `mock_module_node.task_deadline(issue_number=101)`, the assert on
the returned datetime, and the
`mock_task_objects.filter.assert_called_once_with(...)` verification; keep the
same mock setup and assertions but located in the new function so
`test_module_node_interested_users_no_issue` only tests `interested_users`.
In
@backend/tests/apps/mentorship/management/commands/mentorship_update_comments_test.py:
- Around line 11-20: The docstring for make_qs is incomplete and cuts off
mid-sentence; update the docstring of the make_qs function to fully describe
what the mock queryset does (e.g., "Return a queryset-like MagicMock that is
iterable and supports exists(), all(), count(), distinct(), and iteration"), so
it clearly documents the behaviors implemented by the MagicMock (exists,
__iter__, all, count, distinct).
🧹 Nitpick comments (14)
backend/tests/apps/mentorship/model/task_level_test.py (1)
17-26: Consider whether this test adds sufficient value.This test verifies that
TaskLevel.__str__usesmodule.namedirectly rather thanstr(module). While the test is correct, it provides limited additional value since the implementation explicitly accesses the.nameattribute:def __str__(self) -> str: return f"{self.module.name} - {self.name}"The first test already covers the essential behavior. This test primarily documents implementation details that are unlikely to change or cause issues.
backend/tests/apps/mentorship/model/task_test.py (2)
9-21: Consider using idiomaticstr()for string representation tests.Directly calling
Task.__str__(task)works but is non-idiomatic. Usingstr(task)is more natural and tests the actual behavior users experience.♻️ Proposed refactor
- expected = f"Task: '{issue.title}' assigned to {assignee.login}" - assert Task.__str__(task) == expected + expected = f"Task: '{issue.title}' assigned to {assignee.login}" + assert str(task) == expected
23-32: Consider using idiomaticstr()for string representation tests.Same as the previous test method - using
str(task)instead ofTask.__str__(task)would be more idiomatic and natural.♻️ Proposed refactor
- expected = f"Task: {issue.title} (Unassigned)" - assert Task.__str__(task) == expected + expected = f"Task: {issue.title} (Unassigned)" + assert str(task) == expectedbackend/tests/apps/mentorship/model/start_end_range_test.py (1)
17-17: Remove redundant variable assignment.The assignment
instance = mock_instanceserves no purpose—both variables reference the same mock. Usemock_instancedirectly in the assertions below to simplify the code.♻️ Proposed fix
mock_instance = MagicMock(spec=StartEndRange) mock_instance.started_at = start_time mock_instance.ended_at = end_time - instance = mock_instance - assert instance.started_at == start_time - assert instance.ended_at == end_time + assert mock_instance.started_at == start_time + assert mock_instance.ended_at == end_timebackend/tests/apps/mentorship/model/module_test.py (1)
32-92: Consider splitting into two separate test methods for clarity.The test correctly verifies two scenarios:
- Default date inheritance from the program (lines 61-72)
- Explicit dates are preserved (lines 74-92)
Both scenarios work correctly within the current structure, but splitting them into separate test methods (
test_module_inherits_program_datesandtest_module_preserves_explicit_dates) would improve readability and follow the single-responsibility principle for unit tests.♻️ Optional refactor to split test scenarios
Split into two methods:
@patch("apps.mentorship.models.Module.objects.create") def test_module_inherits_program_dates_when_not_provided(self, mock_create_module): """Test that module inherits program dates when started_at/ended_at are None.""" program_with_dates = MagicMock( spec=Program, name="Program With Dates", key="program-with-dates", started_at=django.utils.timezone.datetime( 2024, 2, 1, tzinfo=django.utils.timezone.UTC ), ended_at=django.utils.timezone.datetime(2024, 2, 28, tzinfo=django.utils.timezone.UTC), ) mock_module = MagicMock(spec=Module) mock_module.name = "Auto Date Module" mock_module.program = program_with_dates mock_module.project = self.project mock_module.started_at = None mock_module.ended_at = None def simulate_save_for_inheritance(): mock_module.key = "date-module" if not mock_module.started_at: mock_module.started_at = mock_module.program.started_at if not mock_module.ended_at: mock_module.ended_at = mock_module.program.ended_at mock_module.save.side_effect = simulate_save_for_inheritance mock_create_module.return_value = mock_module module = Module.objects.create( name="Auto Date Module", program=program_with_dates, project=self.project ) module.save() assert module.key == "date-module" assert module.started_at == django.utils.timezone.datetime( 2024, 2, 1, tzinfo=django.utils.timezone.UTC ) assert module.ended_at == django.utils.timezone.datetime( 2024, 2, 28, tzinfo=django.utils.timezone.UTC ) @patch("apps.mentorship.models.Module.objects.create") def test_module_preserves_explicit_dates(self, mock_create_module): """Test that module preserves explicitly provided dates.""" program_with_dates = MagicMock( spec=Program, name="Program With Dates", key="program-with-dates", started_at=django.utils.timezone.datetime( 2024, 2, 1, tzinfo=django.utils.timezone.UTC ), ended_at=django.utils.timezone.datetime(2024, 2, 28, tzinfo=django.utils.timezone.UTC), ) explicit_start = django.utils.timezone.datetime( 2024, 3, 1, tzinfo=django.utils.timezone.UTC ) explicit_end = django.utils.timezone.datetime( 2024, 3, 31, tzinfo=django.utils.timezone.UTC ) mock_module = MagicMock(spec=Module) mock_module.name = "Auto Date Module" mock_module.program = program_with_dates mock_module.project = self.project mock_module.started_at = explicit_start mock_module.ended_at = explicit_end def simulate_save_for_inheritance(): mock_module.key = "date-module" if not mock_module.started_at: mock_module.started_at = mock_module.program.started_at if not mock_module.ended_at: mock_module.ended_at = mock_module.program.ended_at mock_module.save.side_effect = simulate_save_for_inheritance mock_create_module.return_value = mock_module module = Module.objects.create( name="Auto Date Module", program=program_with_dates, project=self.project ) module.save() assert module.started_at == explicit_start assert module.ended_at == explicit_end assert module.key == "date-module"backend/tests/apps/mentorship/management/commands/mentorship_sync_module_issues_test.py (2)
1-2: Consider consolidating datetime imports.Both the module (
import datetime as dt) and the class (from datetime import datetime) are imported. You could simplify by using one form consistently, e.g.,from datetime import datetime, UTCand replacingdt.UTCwithUTC.♻️ Proposed refactor
-import datetime as dt -from datetime import datetime +from datetime import UTC, datetimeThen update line 70:
- e1 = MagicMock(event="commented", created_at=datetime(2023, 1, 1, tzinfo=dt.UTC)) + e1 = MagicMock(event="commented", created_at=datetime(2023, 1, 1, tzinfo=UTC))
130-214: Consider splitting this complex test.This test validates multiple behaviors: issue linking, task creation, status mapping, and output messaging. While comprehensive, it might be easier to debug and maintain if split into focused tests (e.g., separate tests for status assignment vs. task creation vs. output messaging).
Based on learnings, this approach aligns with the preference for pure mock tests.
backend/tests/apps/mentorship/management/commands/mentorship_sync_issue_levels_test.py (2)
8-15: Consider adding stderr mock for completeness.The
commandfixture mocksstdoutandstylebut omitsstderr. For consistency with other management command tests in this module (seementorship_sync_module_issues_test.py), consider adding:cmd.stderr = MagicMock()While the current command may not use stderr, including it improves test robustness and follows the established pattern.
Proposed addition
@pytest.fixture def command(): cmd = Command() cmd.stdout = MagicMock() + cmd.stderr = MagicMock() cmd.style = MagicMock() cmd.style.WARNING = lambda x: x cmd.style.SUCCESS = lambda x: x return cmd
18-24: Consider extractingmake_qsto a shared test utilities module.The
make_qshelper is duplicated across multiple test files inbackend/tests/apps/mentorship/management/commands/(appears in at leastmentorship_sync_module_issues_test.pyandmentorship_update_comments_test.py). Extracting it to a shared test utilities module (e.g.,backend/tests/apps/mentorship/management/commands/test_utils.py) would improve maintainability and reduce duplication.backend/tests/apps/mentorship/management/commands/mentorship_update_comments_test.py (2)
92-126: Consider renaming parameter for clarity.The test correctly verifies the orchestration of issue syncing and interest processing. However, the parameter name
mock_issue_1(line 97) is potentially confusing since it represents the patchedIssueclass, not an issue instance. The actual issue instance ismock_issue(from the fixture).♻️ Optional clarity improvement
@patch("apps.mentorship.management.commands.mentorship_update_comments.get_github_client") @patch("apps.mentorship.management.commands.mentorship_update_comments.sync_issue_comments") @patch.object(Command, "process_issue_interests") @patch("apps.mentorship.management.commands.mentorship_update_comments.Issue") def test_process_module( - mock_issue_1, + mock_issue_class, mock_process_issue_interests, mock_sync_issue_comments, mock_get_github_client, command, mock_module, mock_issue, ): """Test process_module orchestrates issue syncing and interest processing.""" mock_issue.id = 1 mock_issue.title = "Test Issue 1" mock_issue.number = 123 issues_qs = make_qs([mock_issue], exist=True) - mock_issue_1.objects.filter.return_value.distinct.return_value = issues_qs + mock_issue_class.objects.filter.return_value.distinct.return_value = issues_qs
189-218: Consider consolidating similar tests.This test is very similar to
test_process_issue_interests_remove_interest(lines 158-187). Both verify interest removal when a user's comment doesn't match the interest pattern. The main difference is the comment body text ("Just a regular comment" vs "Not interested anymore"), but both should trigger the same removal logic.If these tests are intentionally covering different edge cases, consider adding a comment explaining the distinction. Otherwise, they could potentially be consolidated or parameterized.
backend/tests/apps/mentorship/model/program_test.py (1)
10-12: Consider using a proper mock and idiomatic str call.Two minor improvements:
- The variable name
mock_program_instanceis misleading—it's a realPrograminstance, not a mock. Based on learnings, pure mock tests are preferred for the mentorship module.- Calling
Program.__str__(mock_program_instance)is unconventional; usestr(mock_program_instance)instead.♻️ Proposed refactor using a proper mock
- def test_str_returns_name(self): - mock_program_instance = Program(name="Security Program") - assert Program.__str__(mock_program_instance) == "Security Program" + def test_str_returns_name(self): + from unittest.mock import MagicMock + mock_program = MagicMock(spec=Program) + mock_program.name = "Security Program" + assert Program.__str__(mock_program) == "Security Program"Or use the more idiomatic approach:
- def test_str_returns_name(self): - mock_program_instance = Program(name="Security Program") - assert Program.__str__(mock_program_instance) == "Security Program" + def test_str_returns_name(self): + from unittest.mock import MagicMock + mock_program = MagicMock(spec=Program) + mock_program.name = "Security Program" + mock_program.__str__ = lambda: mock_program.name + assert str(mock_program) == "Security Program"Based on learnings, pure mock tests are preferred in the mentorship test suite.
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py (1)
1-141: Consider simplifying FakeModuleNode to use only mock data.The
FakeModuleNodeclass contains actual Django ORM code in methods likementees(),issue_mentees(),available_labels(), etc. While these are fully mocked in the fixture, it would be cleaner to have these methods return mock data directly rather than containing real ORM code.Based on learnings, the maintainer prefers pure mock tests. The current implementation works because everything is mocked in the fixture, but having ORM code in the test double is a code smell.
backend/tests/apps/mentorship/api/internal/queries/api_queries_mentorship_test.py (1)
282-282: Consider more explicit assertion for multiple ORM calls.Line 282 uses
assert_called_withwhich verifies at least one call with those arguments, but doesn't check the call count. SinceGithubUser.objects.only()is called twice in the production code (once directly with("id")and once in the Prefetch with("id", "login", "name", "avatar_url")), consider usingassert_any_callfor clarity or verifying both calls explicitly.Current implementation works correctly but could be more explicit about the multiple calls to improve test clarity and maintainability.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
backend/apps/github/models/__init__.pybackend/tests/apps/mentorship/api/internal/nodes/api_internal_enum_test.pybackend/tests/apps/mentorship/api/internal/nodes/api_internal_mentee_test.pybackend/tests/apps/mentorship/api/internal/nodes/api_internal_mentor_test.pybackend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.pybackend/tests/apps/mentorship/api/internal/nodes/api_internal_program_test.pybackend/tests/apps/mentorship/api/internal/queries/api_queries_mentorship_test.pybackend/tests/apps/mentorship/api/internal/queries/api_queries_module_test.pybackend/tests/apps/mentorship/api/internal/queries/api_queries_program_test.pybackend/tests/apps/mentorship/management/commands/mentorship_sync_issue_levels_test.pybackend/tests/apps/mentorship/management/commands/mentorship_sync_module_issues_test.pybackend/tests/apps/mentorship/management/commands/mentorship_update_comments_test.pybackend/tests/apps/mentorship/model/experience_level_test.pybackend/tests/apps/mentorship/model/mentee_test.pybackend/tests/apps/mentorship/model/mentor_module_test.pybackend/tests/apps/mentorship/model/mentor_test.pybackend/tests/apps/mentorship/model/module_test.pybackend/tests/apps/mentorship/model/program_test.pybackend/tests/apps/mentorship/model/start_end_range_test.pybackend/tests/apps/mentorship/model/task_level_test.pybackend/tests/apps/mentorship/model/task_test.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/github/models/init.py
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: kart-u
Repo: OWASP/Nest PR: 3179
File: backend/tests/apps/mentorship/model/test_program.py:13-22
Timestamp: 2026-01-06T21:11:08.745Z
Learning: In the mentorship test suite (backend/tests/apps/mentorship/), the maintainer kart-u prefers pure mock tests over Django TestCase with database interactions for model tests.
📚 Learning: 2025-12-31T05:17:39.659Z
Learnt from: kart-u
Repo: OWASP/Nest PR: 3101
File: backend/apps/common/extensions.py:92-98
Timestamp: 2025-12-31T05:17:39.659Z
Learning: In this codebase, import OperationType for GraphQL operations from the graphql-core package rather than from strawberry. Use 'from graphql import OperationType'. Strawberry re-exports via graphql-core internally, so relying on strawberry's API may be brittle. Apply this rule to all Python files that deal with GraphQL operation types; ensure imports come from graphql (graphql-core) and not from strawberry packages. This improves compatibility and avoids coupling to strawberry's internals.
Applied to files:
backend/tests/apps/mentorship/api/internal/nodes/api_internal_enum_test.pybackend/tests/apps/mentorship/api/internal/nodes/api_internal_program_test.pybackend/tests/apps/mentorship/api/internal/nodes/api_internal_mentor_test.pybackend/tests/apps/mentorship/model/task_level_test.pybackend/tests/apps/mentorship/model/mentor_module_test.pybackend/tests/apps/mentorship/management/commands/mentorship_sync_issue_levels_test.pybackend/tests/apps/mentorship/model/start_end_range_test.pybackend/tests/apps/mentorship/model/mentee_test.pybackend/tests/apps/mentorship/api/internal/queries/api_queries_module_test.pybackend/tests/apps/mentorship/model/experience_level_test.pybackend/tests/apps/mentorship/management/commands/mentorship_update_comments_test.pybackend/tests/apps/mentorship/management/commands/mentorship_sync_module_issues_test.pybackend/tests/apps/mentorship/model/task_test.pybackend/tests/apps/mentorship/model/module_test.pybackend/tests/apps/mentorship/api/internal/nodes/api_internal_mentee_test.pybackend/tests/apps/mentorship/api/internal/queries/api_queries_mentorship_test.pybackend/tests/apps/mentorship/model/mentor_test.pybackend/tests/apps/mentorship/model/program_test.pybackend/tests/apps/mentorship/api/internal/queries/api_queries_program_test.pybackend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py
📚 Learning: 2026-01-01T17:48:23.963Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:41-47
Timestamp: 2026-01-01T17:48:23.963Z
Learning: In Django code, be aware that a QuerySet's boolean evaluation (e.g., if not queryset) runs a database query to determine emptiness. While it is technically valid to use the queryset in a boolean context, use queryset.exists() for existence checks to avoid unnecessary queries and improve performance. Applicable broadly to Python/Django files rather than just this specific path.
Applied to files:
backend/tests/apps/mentorship/api/internal/nodes/api_internal_enum_test.pybackend/tests/apps/mentorship/api/internal/nodes/api_internal_program_test.pybackend/tests/apps/mentorship/api/internal/nodes/api_internal_mentor_test.pybackend/tests/apps/mentorship/model/task_level_test.pybackend/tests/apps/mentorship/model/mentor_module_test.pybackend/tests/apps/mentorship/management/commands/mentorship_sync_issue_levels_test.pybackend/tests/apps/mentorship/model/start_end_range_test.pybackend/tests/apps/mentorship/model/mentee_test.pybackend/tests/apps/mentorship/api/internal/queries/api_queries_module_test.pybackend/tests/apps/mentorship/model/experience_level_test.pybackend/tests/apps/mentorship/management/commands/mentorship_update_comments_test.pybackend/tests/apps/mentorship/management/commands/mentorship_sync_module_issues_test.pybackend/tests/apps/mentorship/model/task_test.pybackend/tests/apps/mentorship/model/module_test.pybackend/tests/apps/mentorship/api/internal/nodes/api_internal_mentee_test.pybackend/tests/apps/mentorship/api/internal/queries/api_queries_mentorship_test.pybackend/tests/apps/mentorship/model/mentor_test.pybackend/tests/apps/mentorship/model/program_test.pybackend/tests/apps/mentorship/api/internal/queries/api_queries_program_test.pybackend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py
📚 Learning: 2026-01-06T21:11:03.537Z
Learnt from: kart-u
Repo: OWASP/Nest PR: 3179
File: backend/tests/apps/mentorship/model/test_program.py:13-22
Timestamp: 2026-01-06T21:11:03.537Z
Learning: In backend/tests/apps/mentorship/, prefer pure mock tests (no real database interactions) for model tests. Avoid using Django TestCase with database access; implement tests with mocks/stubs to exercise logic and, when needed, lightweight fixtures without DB. This guideline targets all Python test files under the mentorship module.
Applied to files:
backend/tests/apps/mentorship/api/internal/nodes/api_internal_enum_test.pybackend/tests/apps/mentorship/api/internal/nodes/api_internal_program_test.pybackend/tests/apps/mentorship/api/internal/nodes/api_internal_mentor_test.pybackend/tests/apps/mentorship/model/task_level_test.pybackend/tests/apps/mentorship/model/mentor_module_test.pybackend/tests/apps/mentorship/management/commands/mentorship_sync_issue_levels_test.pybackend/tests/apps/mentorship/model/start_end_range_test.pybackend/tests/apps/mentorship/model/mentee_test.pybackend/tests/apps/mentorship/api/internal/queries/api_queries_module_test.pybackend/tests/apps/mentorship/model/experience_level_test.pybackend/tests/apps/mentorship/management/commands/mentorship_update_comments_test.pybackend/tests/apps/mentorship/management/commands/mentorship_sync_module_issues_test.pybackend/tests/apps/mentorship/model/task_test.pybackend/tests/apps/mentorship/model/module_test.pybackend/tests/apps/mentorship/api/internal/nodes/api_internal_mentee_test.pybackend/tests/apps/mentorship/api/internal/queries/api_queries_mentorship_test.pybackend/tests/apps/mentorship/model/mentor_test.pybackend/tests/apps/mentorship/model/program_test.pybackend/tests/apps/mentorship/api/internal/queries/api_queries_program_test.pybackend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py
📚 Learning: 2025-07-13T05:55:46.436Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: backend/apps/mentorship/graphql/mutations/program.py:166-166
Timestamp: 2025-07-13T05:55:46.436Z
Learning: In the OWASP Nest mentorship GraphQL mutations, Strawberry GraphQL automatically converts between Django Program instances and ProgramNode types, so mutations can return Program instances directly without manual conversion even when typed as ProgramNode, similar to the Module/ModuleNode pattern.
Applied to files:
backend/tests/apps/mentorship/api/internal/nodes/api_internal_program_test.py
📚 Learning: 2025-07-16T13:49:58.648Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: backend/apps/mentorship/graphql/mutations/module.py:28-29
Timestamp: 2025-07-16T13:49:58.648Z
Learning: In the OWASP Nest mentorship system, mentors can be created with only github_user initially (without nest_user) when assigned to modules. This allows mentors to be assigned before they've signed into the system. When these users eventually sign in, the nest_user is automatically linked to the existing github_user, creating a complete mentor profile. This design provides flexibility in mentor assignment workflows.
Applied to files:
backend/tests/apps/mentorship/api/internal/nodes/api_internal_mentor_test.py
📚 Learning: 2025-12-18T05:39:42.678Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:40-40
Timestamp: 2025-12-18T05:39:42.678Z
Learning: In Django management commands, prefer using self.stdout.write(...) over print(...) for user-facing stdout output. This aligns with Django conventions and improves testability. When emitting messages, consider using self.stdout.write and, for styled messages, use self.style.SUCCESS/ERROR as appropriate to maintain consistent command output formatting. Apply this guideline to all Python files within any project's management/commands directory.
Applied to files:
backend/tests/apps/mentorship/management/commands/mentorship_sync_issue_levels_test.pybackend/tests/apps/mentorship/management/commands/mentorship_update_comments_test.pybackend/tests/apps/mentorship/management/commands/mentorship_sync_module_issues_test.py
📚 Learning: 2025-08-17T11:55:55.990Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2000
File: backend/apps/mentorship/management/commands/sync_module_issues.py:109-145
Timestamp: 2025-08-17T11:55:55.990Z
Learning: In the OWASP/Nest mentorship system, tasks are designed to be assigned to only one assignee per issue, even if GitHub issues can have multiple assignees. The sync_module_issues command correctly uses issue.assignees.first() to create one task per issue for the first assignee only.
Applied to files:
backend/tests/apps/mentorship/management/commands/mentorship_sync_module_issues_test.py
📚 Learning: 2025-09-29T06:02:35.566Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/components/SingleModuleCard.tsx:54-54
Timestamp: 2025-09-29T06:02:35.566Z
Learning: In the Module type from types/mentorship.ts, the experienceLevel field is required (experienceLevel: ExperienceLevelEnum), not optional, so null/undefined checks are not needed when accessing this field.
Applied to files:
backend/tests/apps/mentorship/api/internal/nodes/api_internal_mentee_test.py
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS is actively used in frontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsx for program editing functionality and cannot be removed. It serves a different purpose than GET_PROGRAM_ADMIN_DETAILS, providing comprehensive program information needed for editing.
Applied to files:
backend/tests/apps/mentorship/api/internal/queries/api_queries_program_test.py
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS and GET_PROGRAM_ADMIN_DETAILS are two separate queries serving different purposes: GET_PROGRAM_DETAILS fetches comprehensive program information while GET_PROGRAM_ADMIN_DETAILS fetches only admin-related details. These queries cannot be removed or merged as they serve different use cases in the application.
Applied to files:
backend/tests/apps/mentorship/api/internal/queries/api_queries_program_test.py
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS and GET_PROGRAM_ADMIN_DETAILS are two separate queries serving different purposes: GET_PROGRAM_DETAILS fetches comprehensive program information while GET_PROGRAM_ADMIN_DETAILS fetches only admin-related details.
Applied to files:
backend/tests/apps/mentorship/api/internal/queries/api_queries_program_test.py
🧬 Code graph analysis (18)
backend/tests/apps/mentorship/api/internal/nodes/api_internal_enum_test.py (3)
backend/apps/mentorship/api/internal/nodes/enum.py (2)
ExperienceLevelEnum(12-18)ProgramStatusEnum(22-27)backend/apps/mentorship/models/common/experience_level.py (2)
ExperienceLevel(6-25)ExperienceLevelChoices(12-18)backend/apps/mentorship/models/program.py (1)
ProgramStatus(25-28)
backend/tests/apps/mentorship/api/internal/nodes/api_internal_program_test.py (2)
backend/apps/mentorship/api/internal/nodes/enum.py (2)
ExperienceLevelEnum(12-18)ProgramStatusEnum(22-27)backend/apps/mentorship/api/internal/nodes/mentor.py (1)
name(18-20)
backend/tests/apps/mentorship/api/internal/nodes/api_internal_mentor_test.py (1)
backend/apps/mentorship/api/internal/nodes/mentor.py (3)
avatar_url(13-15)name(18-20)login(23-25)
backend/tests/apps/mentorship/model/task_level_test.py (1)
backend/apps/mentorship/models/task_level.py (1)
TaskLevel(8-61)
backend/tests/apps/mentorship/model/mentor_module_test.py (2)
backend/apps/mentorship/models/mentor.py (1)
Mentor(11-40)backend/apps/mentorship/models/mentor_module.py (1)
MentorModule(11-42)
backend/tests/apps/mentorship/management/commands/mentorship_sync_issue_levels_test.py (3)
backend/tests/apps/mentorship/management/commands/mentorship_sync_module_issues_test.py (2)
command(23-31)make_qs(13-19)backend/tests/apps/mentorship/management/commands/mentorship_update_comments_test.py (4)
command(39-46)make_qs(11-20)mock_issue(61-69)mock_module(50-57)backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py (3)
select_related(36-38)order_by(32-34)prefetch_related(40-42)
backend/tests/apps/mentorship/model/start_end_range_test.py (1)
backend/apps/mentorship/models/common/start_end_range.py (1)
StartEndRange(6-13)
backend/tests/apps/mentorship/model/mentee_test.py (1)
backend/apps/mentorship/models/mentee.py (1)
Mentee(11-49)
backend/tests/apps/mentorship/api/internal/queries/api_queries_module_test.py (1)
backend/apps/mentorship/api/internal/queries/module.py (4)
ModuleQuery(15-50)get_program_modules(19-26)get_project_modules(29-36)get_module(39-50)
backend/tests/apps/mentorship/model/experience_level_test.py (1)
backend/apps/mentorship/models/common/experience_level.py (2)
ExperienceLevel(6-25)ExperienceLevelChoices(12-18)
backend/tests/apps/mentorship/management/commands/mentorship_update_comments_test.py (1)
backend/apps/mentorship/management/commands/mentorship_update_comments.py (3)
process_mentorship_modules(31-55)process_module(57-89)process_issue_interests(91-160)
backend/tests/apps/mentorship/model/task_test.py (1)
backend/apps/mentorship/models/task.py (1)
Task(10-75)
backend/tests/apps/mentorship/model/module_test.py (2)
backend/apps/mentorship/models/program.py (1)
ProgramStatus(25-28)backend/tests/apps/mentorship/model/program_test.py (1)
test_str_returns_name(10-12)
backend/tests/apps/mentorship/api/internal/queries/api_queries_mentorship_test.py (1)
backend/apps/mentorship/api/internal/queries/mentorship.py (3)
is_mentor(36-48)get_mentee_details(51-82)get_mentee_module_issues(85-124)
backend/tests/apps/mentorship/model/mentor_test.py (2)
backend/apps/mentorship/models/mentor.py (1)
Mentor(11-40)backend/tests/apps/mentorship/model/mentee_test.py (1)
test_str_returns_github_login(8-14)
backend/tests/apps/mentorship/model/program_test.py (1)
backend/apps/mentorship/models/program.py (1)
ProgramStatus(25-28)
backend/tests/apps/mentorship/api/internal/queries/api_queries_program_test.py (1)
backend/apps/mentorship/api/internal/queries/program.py (2)
get_program(23-32)my_programs(35-80)
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py (6)
backend/apps/mentorship/api/internal/nodes/enum.py (1)
ExperienceLevelEnum(12-18)backend/apps/mentorship/api/internal/nodes/mentor.py (2)
name(18-20)login(23-25)backend/apps/github/api/internal/nodes/issue.py (1)
labels(50-52)backend/apps/mentorship/models/task.py (1)
Task(10-75)backend/apps/mentorship/models/issue_user_interest.py (1)
IssueUserInterest(6-36)backend/tests/apps/github/models/issue_test.py (1)
issue(26-28)
🔇 Additional comments (46)
backend/tests/apps/mentorship/model/task_level_test.py (2)
1-3: LGTM! Clean imports for pure mock tests.The imports are minimal and appropriate. Using
MagicMockfor pure unit tests aligns with the project's preference for mock-based model tests without database interactions.Based on learnings, pure mock tests are preferred for the mentorship module.
6-15: LGTM! Effective pure mock test.The test correctly verifies the
__str__method by calling the real implementation with mocked dependencies. UsingTaskLevel.__str__(task_level)with aMagicMockis the appropriate pattern to test the actual method logic while mockingself.backend/tests/apps/mentorship/model/task_test.py (2)
1-6: LGTM!The imports are clean and follow the learned preference for pure mock tests using MagicMock. Based on learnings, the approach aligns well with the maintainer's preference for mock-based tests in the mentorship module.
34-38: LGTM!The status constants test correctly verifies all Task.Status enum values and matches the model definition.
backend/tests/apps/mentorship/model/mentor_test.py (1)
1-14: LGTM! Consistent pure mock pattern.The test correctly verifies that
Mentor.__str__returns the GitHub login using pure mocks without database interactions, which aligns with the preferred testing approach for this module.backend/tests/apps/mentorship/model/mentee_test.py (1)
1-14: LGTM! Consistent with the mentor test pattern.The test correctly verifies that
Mentee.__str__returns the GitHub login using the same pure mock approach as the corresponding mentor test.backend/tests/apps/mentorship/model/mentor_module_test.py (1)
1-18: LGTM! Correctly tests composite string representation.The test properly mocks the dependencies'
__str__methods and verifies thatMentorModule.__str__returns the expected formatted string.backend/tests/apps/mentorship/model/module_test.py (1)
25-30: LGTM! Simple and clear.The test correctly verifies that
Module.__str__returns the module name.backend/tests/apps/mentorship/management/commands/mentorship_sync_module_issues_test.py (6)
13-19: Well-designed test helper.The
make_qshelper cleanly abstracts QuerySet mocking and improves test readability.
22-31: Solid fixture design.The fixture provides a well-configured Command instance with mocked I/O streams, enabling focused unit testing without side effects.
40-52: Excellent parametrized test coverage.The test cases comprehensively cover valid GitHub URLs, malformed inputs, non-GitHub domains, and None values.
65-99: Thorough testing of date retrieval logic.This test effectively covers the happy path (finding the latest assignment), the not-found case, and exception handling with proper stderr output verification.
102-127: Well-structured test for map building.The test verifies both the correctness of the case-insensitive label grouping and the user-facing stdout message. Good coverage.
217-248: Good edge case coverage.The test properly validates the no-match scenario, ensuring that empty results are handled correctly without creating spurious tasks.
backend/tests/apps/mentorship/management/commands/mentorship_sync_issue_levels_test.py (4)
27-34: LGTM!The early exit test correctly mocks an empty TaskLevel queryset and verifies the warning message is written to stdout.
78-93: Test structure is comprehensive (pending case sensitivity verification).The test thoroughly covers multiple update scenarios:
- Setting a level on an issue with none
- Updating an existing level to a different one
- Clearing level when no label matches
- Skipping already-correct issues
The assertions correctly verify the
bulk_updatecall and user-facing message. However, this approval is conditional on resolving the case sensitivity issue flagged above.
96-121: LGTM with case sensitivity caveat.The test correctly verifies that
bulk_updateis skipped when all issues already have the correct levels, and the appropriate "up-to-date" message is displayed. However, the same case sensitivity issue noted earlier (Line 107:"Label-A"vs. Line 103:"label-a") applies here as well.
45-56: Case sensitivity concern is not applicable — label matching is case-insensitive.The command implementation normalizes both
TaskLevellabels andLabelobject names usingnormalize_name()before comparison (lines 36 and 78 inmentorship_sync_issue_levels.py). Thenormalize_name()function appliescasefold()to convert strings to lowercase, ensuring case-insensitive matching.The test setup intentionally uses different cases (
"label-a"vs"Label-A") to verify this case-insensitive behavior works correctly. This is appropriate test design.backend/tests/apps/mentorship/management/commands/mentorship_update_comments_test.py (13)
1-9: LGTM! Clean test imports.The imports are well-organized and include all necessary dependencies for comprehensive mocking-based tests.
23-28: LGTM! Clean helper function.The
make_userhelper provides a clear, concise way to create user mocks for testing.
30-36: LGTM! Well-designed helper.The
make_commenthelper with optionalcreated_atparameter provides good flexibility for test scenarios.
38-46: LGTM! Proper command fixture setup.The fixture correctly mocks
stdoutandstylefor testing Django management commands. This aligns with Django conventions for testable command implementations.
49-57: LGTM! Appropriate ORM mock chaining.The fixture correctly mocks Django's queryset chaining pattern for
module.project.repositories.filter().values_list().distinct(), providing realistic behavior for tests.
60-69: LGTM! Clean baseline issue fixture.The fixture provides a sensible default mock issue with empty comments, allowing individual tests to override comment behavior as needed.
72-78: LGTM! Clear early-exit test.The test correctly verifies that the command exits gracefully with an appropriate message when no published modules exist.
80-90: LGTM! Thorough edge-case coverage.The test correctly handles the scenario where published modules exist but none have labels, verifying the appropriate warning message is displayed.
128-156: LGTM! Comprehensive interest registration test.The test thoroughly verifies new interest registration, including proper bulk_create usage, correct object attributes, and appropriate success messaging.
158-187: LGTM! Proper interest removal verification.The test correctly verifies that interests are removed when a user's comment no longer matches the interest pattern, including proper delete operation and warning message.
220-248: LGTM! Important aggregation test.This test correctly verifies that multiple comments from the same user are aggregated properly, ensuring only one interest is created even when a user posts multiple comments. This is crucial for preventing duplicate interest records.
250-300: LGTM! Excellent comprehensive scenario test.This test provides outstanding coverage of the most complex scenario: multiple users with mixed interest changes (some created, some removed) in a single processing run. The assertions thoroughly verify both the database operations and the corresponding user feedback messages.
1-300: Excellent test coverage with proper mocking patterns.This test suite demonstrates strong engineering practices:
- Pure mock-based tests without database dependencies (as per learnings from your previous feedback)
- Proper use of
stdout.writefor Django management command output- Comprehensive edge-case coverage including early exits, aggregation, and multi-user scenarios
- Clean helper functions and fixtures for test setup
The tests effectively validate the command's orchestration, interest tracking logic, and user feedback messaging.
Based on learnings, the pure mock approach without Django TestCase aligns with the mentorship test suite preferences.
backend/tests/apps/mentorship/model/experience_level_test.py (1)
4-9: LGTM!The test correctly validates that
ExperienceLevelChoicesconstants map to their expected string values. This is a clean, focused unit test with no database dependencies.backend/tests/apps/mentorship/model/program_test.py (1)
5-8: LGTM!The test correctly validates that
ProgramStatusconstants map to their expected string values.backend/tests/apps/mentorship/api/internal/nodes/api_internal_mentor_test.py (2)
10-33: LGTM!The fixtures are well-structured and follow best practices:
- Proper use of
MagicMockfor the GitHub user- Coverage of both scenarios (with and without
github_user)- Clear, descriptive fixture names and docstrings
36-68: LGTM!Excellent test coverage for
MentorNode:
- Tests validate all field resolvers (
avatar_url,name,login)- Edge cases handled properly (returns empty string when
github_userisNone)- Tests align with the actual resolver implementations
backend/tests/apps/mentorship/api/internal/nodes/api_internal_program_test.py (5)
17-37: LGTM!The
FakeProgramNodeclass is a reasonable approach for testing without database dependencies. It mimicsProgramNode's structure and behavior while maintaining isolation.
40-50: LGTM!The fixture properly initializes the fake node with mocked admins.
53-76: LGTM!The tests comprehensively validate all
ProgramNodefields and theadminsresolver.
78-85: LGTM!The test correctly validates the
PaginatedProgramsstructure.
88-140: LGTM!The input type tests validate both type annotations and default values. While testing
__annotations__directly is slightly fragile, it's an effective way to verify Strawberry Input type structures. The tests for default values (e.g.,domains=[],tags=[]) are particularly valuable.backend/tests/apps/mentorship/api/internal/nodes/api_internal_enum_test.py (1)
8-23: LGTM!These tests are critical for validating that the API enum layer correctly maps to the model choice layer. They ensure the contract between the GraphQL API and the database remains consistent.
backend/tests/apps/mentorship/api/internal/nodes/api_internal_mentee_test.py (1)
1-58: LGTM! Well-structured mentee node tests.The tests follow the mock-based testing pattern and provide good coverage of the MenteeNode functionality:
- Field initialization and access
- Resolver methods for avatar_url and experience_level
- Edge case handling for None experience_level with appropriate default
The use of
# type: ignore[assignment]on line 54 is appropriate for testing the None case.Based on learnings, these tests align with the preferred pure mock approach for the mentorship test suite.
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py (1)
143-390: Good mock-based test coverage.The tests properly mock all Django ORM interactions and cover various scenarios including success paths, empty results, and filtering. The use of patches ensures no database access occurs during test execution.
Based on learnings, these tests align with the preferred pure mock approach for the mentorship test suite.
backend/tests/apps/mentorship/api/internal/queries/api_queries_module_test.py (1)
1-114: LGTM! Comprehensive module query tests.The tests provide excellent coverage of the ModuleQuery class:
- Both program and project module retrieval (success + empty cases)
- Single module retrieval with proper exception handling
- All Django ORM interactions are properly mocked with patches
- Assertions verify both return values and method call signatures
Based on learnings, these tests follow the preferred pure mock approach without database interactions.
backend/tests/apps/mentorship/api/internal/queries/api_queries_program_test.py (1)
1-237: LGTM! Thorough program query tests.Excellent test coverage for ProgramQuery:
- get_program: success and DoesNotExist paths
- my_programs: comprehensive scenarios including admin/mentor roles, empty results, search filtering, and pagination
- All Django ORM calls properly mocked with patches
- Mock setup correctly handles complex relationships and user role assignment
The tests follow best practices with clear test names and proper assertion of both results and method calls.
Based on learnings, these tests follow the preferred pure mock approach for the mentorship module.
backend/tests/apps/mentorship/api/internal/queries/api_queries_mentorship_test.py (1)
1-467: LGTM! Comprehensive mentorship query tests.Excellent coverage across three query types:
- is_mentor: validates various edge cases including empty login, missing GitHub user, and mentor status
- get_mentee_details: covers success path and all exception scenarios (module, user, mentee not found, not enrolled)
- get_mentee_module_issues: includes success, error cases, and pagination scenarios
All Django ORM interactions are properly mocked. The use of
pytest.mark.parametrizeon line 76 is a good practice for testing multiple edge cases efficiently.Based on learnings, these tests follow the preferred pure mock approach for the mentorship test suite.
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py
Show resolved
Hide resolved
backend/tests/apps/mentorship/management/commands/mentorship_update_comments_test.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
backend/tests/apps/mentorship/management/commands/mentorship_update_comments_test.py (3)
11-20: Consider using a callable for__iter__to avoid iterator exhaustion.If a test iterates over the mock queryset multiple times, the current implementation will return an exhausted iterator on subsequent iterations because
iter(items)is evaluated once when settingreturn_value.This is low-risk since each test creates fresh mocks, but using
side_effectwould be more robust.♻️ Suggested fix
def make_qs(iterable, exist): """Return a queryset-like MagicMock that is iterable.""" qs = MagicMock(name="QuerySet") items = list(iterable) qs.exists.return_value = exist - qs.__iter__.return_value = iter(items) + qs.__iter__ = lambda self: iter(items) qs.all.return_value = items qs.count.return_value = len(items) qs.distinct.return_value = qs return qs
72-89: Parameter namemock_moduleshadows the fixture.The
@patchdecorator injects the mockedModuleclass asmock_module, which shares the same name as themock_modulefixture. While the fixture isn't used in these tests, this naming can confuse maintainers.Consider renaming the patch parameter to
mock_module_classorpatched_modulefor clarity.
92-125: Confusing parameter naming:mock_issue_1is the patchedIssueclass.The parameter
mock_issue_1(from@patch...Issue) is the patchedIssueclass, whilemock_issueis the fixture representing an issue instance. This naming is misleading—consider renaming tomock_issue_classorpatched_issuefor the patch parameter.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
backend/tests/apps/mentorship/api/internal/nodes/api_internal_enum_test.pybackend/tests/apps/mentorship/api/internal/nodes/api_internal_mentee_test.pybackend/tests/apps/mentorship/api/internal/nodes/api_internal_mentor_test.pybackend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.pybackend/tests/apps/mentorship/api/internal/queries/api_queries_mentorship_test.pybackend/tests/apps/mentorship/api/internal/queries/api_queries_module_test.pybackend/tests/apps/mentorship/api/internal/queries/api_queries_program_test.pybackend/tests/apps/mentorship/management/commands/mentorship_update_comments_test.py
🚧 Files skipped from review as they are similar to previous changes (6)
- backend/tests/apps/mentorship/api/internal/queries/api_queries_module_test.py
- backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py
- backend/tests/apps/mentorship/api/internal/nodes/api_internal_enum_test.py
- backend/tests/apps/mentorship/api/internal/nodes/api_internal_mentor_test.py
- backend/tests/apps/mentorship/api/internal/queries/api_queries_program_test.py
- backend/tests/apps/mentorship/api/internal/nodes/api_internal_mentee_test.py
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: kart-u
Repo: OWASP/Nest PR: 3179
File: backend/tests/apps/mentorship/model/test_program.py:13-22
Timestamp: 2026-01-06T21:11:08.745Z
Learning: In the mentorship test suite (backend/tests/apps/mentorship/), the maintainer kart-u prefers pure mock tests over Django TestCase with database interactions for model tests.
📚 Learning: 2026-01-06T21:11:03.537Z
Learnt from: kart-u
Repo: OWASP/Nest PR: 3179
File: backend/tests/apps/mentorship/model/test_program.py:13-22
Timestamp: 2026-01-06T21:11:03.537Z
Learning: In backend/tests/apps/mentorship/, prefer pure mock tests (no real database interactions) for model tests. Avoid using Django TestCase with database access; implement tests with mocks/stubs to exercise logic and, when needed, lightweight fixtures without DB. This guideline targets all Python test files under the mentorship module.
Applied to files:
backend/tests/apps/mentorship/api/internal/queries/api_queries_mentorship_test.pybackend/tests/apps/mentorship/management/commands/mentorship_update_comments_test.py
📚 Learning: 2025-12-31T05:17:39.659Z
Learnt from: kart-u
Repo: OWASP/Nest PR: 3101
File: backend/apps/common/extensions.py:92-98
Timestamp: 2025-12-31T05:17:39.659Z
Learning: In this codebase, import OperationType for GraphQL operations from the graphql-core package rather than from strawberry. Use 'from graphql import OperationType'. Strawberry re-exports via graphql-core internally, so relying on strawberry's API may be brittle. Apply this rule to all Python files that deal with GraphQL operation types; ensure imports come from graphql (graphql-core) and not from strawberry packages. This improves compatibility and avoids coupling to strawberry's internals.
Applied to files:
backend/tests/apps/mentorship/api/internal/queries/api_queries_mentorship_test.pybackend/tests/apps/mentorship/management/commands/mentorship_update_comments_test.py
📚 Learning: 2026-01-01T17:48:23.963Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:41-47
Timestamp: 2026-01-01T17:48:23.963Z
Learning: In Django code, be aware that a QuerySet's boolean evaluation (e.g., if not queryset) runs a database query to determine emptiness. While it is technically valid to use the queryset in a boolean context, use queryset.exists() for existence checks to avoid unnecessary queries and improve performance. Applicable broadly to Python/Django files rather than just this specific path.
Applied to files:
backend/tests/apps/mentorship/api/internal/queries/api_queries_mentorship_test.pybackend/tests/apps/mentorship/management/commands/mentorship_update_comments_test.py
📚 Learning: 2025-12-18T05:39:42.678Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:40-40
Timestamp: 2025-12-18T05:39:42.678Z
Learning: In Django management commands, prefer using self.stdout.write(...) over print(...) for user-facing stdout output. This aligns with Django conventions and improves testability. When emitting messages, consider using self.stdout.write and, for styled messages, use self.style.SUCCESS/ERROR as appropriate to maintain consistent command output formatting. Apply this guideline to all Python files within any project's management/commands directory.
Applied to files:
backend/tests/apps/mentorship/management/commands/mentorship_update_comments_test.py
🧬 Code graph analysis (2)
backend/tests/apps/mentorship/api/internal/queries/api_queries_mentorship_test.py (1)
backend/apps/mentorship/api/internal/queries/mentorship.py (3)
is_mentor(36-48)get_mentee_details(51-82)get_mentee_module_issues(85-124)
backend/tests/apps/mentorship/management/commands/mentorship_update_comments_test.py (1)
backend/apps/mentorship/management/commands/mentorship_update_comments.py (3)
process_mentorship_modules(31-55)process_module(57-89)process_issue_interests(91-160)
🔇 Additional comments (4)
backend/tests/apps/mentorship/management/commands/mentorship_update_comments_test.py (1)
128-300: LGTM!The
process_issue_intereststests comprehensively cover:
- New interest creation with
bulk_create- Interest removal with
delete- Multiple comments from a single user (aggregation)
- Mixed scenarios with multiple users
The assertions correctly verify both ORM interactions and stdout output messages. The pure mock approach aligns with the project's test guidelines. Based on learnings, this approach is preferred for the mentorship test suite.
backend/tests/apps/mentorship/api/internal/queries/api_queries_mentorship_test.py (3)
18-81: LGTM!The
TestIsMentorclass provides good coverage:
- Tests the happy path when user is a mentor
- Tests when user exists but is not a mentor
- Tests when GitHub user doesn't exist
- Parametrized tests for empty/whitespace login values
The assertions correctly verify both the return values and the expected ORM calls.
83-220: LGTM!The
TestGetMenteeDetailsclass thoroughly covers all code paths:
- Successful retrieval with correct field assertions
Module.DoesNotExistexception handlingGithubUser.DoesNotExistexception handlingMentee.DoesNotExistexception handling- Not-enrolled scenario with correct error message
The mock setup and assertions correctly mirror the production code's
only()field selections.
288-466: LGTM!The remaining tests in
TestGetMenteeModuleIssuesprovide good coverage:
- Error scenarios for missing module, GitHub user, and mentee
- Not-enrolled scenario with correct error message matching
- Pagination with custom
limitandoffsetparameters- Edge case for pagination beyond available results
backend/tests/apps/mentorship/api/internal/queries/api_queries_mentorship_test.py
Show resolved
Hide resolved
kart-u
force-pushed
the
test_mentorship_backend
branch
from
5316b0d to
359147c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @backend/tests/apps/mentorship/model/program_test.py:
- Around line 10-12: Replace the direct instantiation of the Django model in
test_str_returns_name with a pure mock: instead of Program(name="Security
Program"), create a MagicMock named mock_program_instance, set its name
attribute to "Security Program", and call Program.__str__(mock_program_instance)
to assert the returned string; this keeps the test consistent with other
mentorship model tests and avoids instantiating the real Program model.
🧹 Nitpick comments (7)
backend/tests/apps/mentorship/model/mentor_module_test.py (1)
8-18: Consider adding a docstring for test clarity.While the test is functionally correct, adding a docstring would improve readability and make the test's intent explicit, especially given the unconventional pattern of calling the unbound
__str__method on a mocked instance.📝 Proposed enhancement
class TestMentorModule: def test_str_returns_readable_identifier(self): + """Test that MentorModule.__str__ returns 'mentor of module' format.""" mentor = MagicMock(spec=Mentor) mentor.__str__.return_value = "test_mentor"backend/tests/apps/mentorship/management/commands/mentorship_sync_issue_levels_test.py (1)
18-24: Consider consolidating themake_qshelper.The
make_qshelper is duplicated across multiple test files (mentorship_update_comments_test.py,mentorship_sync_module_issues_test.py) with slight variations. The version inmentorship_update_comments_test.pyincludes acount()method that this one lacks.♻️ Refactor suggestion: Extract to shared test utility
Consider extracting
make_qsto a shared test utility module (e.g.,backend/tests/apps/mentorship/utils.pyorbackend/tests/apps/mentorship/conftest.py) to eliminate duplication and ensure consistency:def make_qs(iterable, exist): """Return a queryset-like MagicMock that is iterable.""" qs = MagicMock(name="QuerySet") items = list(iterable) qs.exists.return_value = exist qs.__iter__.return_value = iter(items) qs.all.return_value = items qs.count.return_value = len(items) qs.distinct.return_value = qs return qsThis would standardize the helper and include all necessary methods.
backend/tests/apps/mentorship/management/commands/mentorship_update_comments_test.py (1)
11-20: Consider consolidating themake_qshelper.This helper is duplicated across multiple test files (
mentorship_sync_issue_levels_test.py,mentorship_sync_module_issues_test.py) with slight variations. This version is more complete than others (includescount()anddistinct()methods).♻️ Refactor suggestion: Extract to shared test utility
Consider extracting
make_qsto a shared test utility module (e.g.,backend/tests/apps/mentorship/utils.pyorbackend/tests/apps/mentorship/conftest.py) to eliminate duplication and ensure consistency across all mentorship tests. This version appears to be the most complete and should serve as the canonical implementation.# In backend/tests/apps/mentorship/conftest.py or utils.py def make_qs(iterable, exist): """Return a queryset-like MagicMock that is iterable.""" qs = MagicMock(name="QuerySet") items = list(iterable) qs.exists.return_value = exist qs.__iter__.return_value = iter(items) qs.all.return_value = items qs.count.return_value = len(items) qs.distinct.return_value = qs return qsbackend/tests/apps/mentorship/api/internal/nodes/api_internal_program_test.py (1)
17-50: Consider using real ProgramNode instead of FakeProgramNode.The current approach tests a
FakeProgramNodeclass rather than the actualProgramNodeimplementation. This means the tests won't catch regressions if the realProgramNodestructure or behavior changes.Compare with
api_internal_mentor_test.py(lines 21-22), which instantiates the realMentorNodeclass and mocks only its dependencies. A similar approach here would:
- Instantiate real
ProgramNodeinstances- Mock the underlying model/queryset that
admins()depends on- Exercise actual resolver logic while maintaining pure mock tests
This ensures the tests validate production code behavior rather than a test double.
Based on learnings, prefer pure mock tests without database interactions.
♻️ Example refactor approach
@pytest.fixture def mock_program_node(): """Fixture for a mock ProgramNode instance with mocked dependencies.""" # Create a mock Django Program model instance mock_program_model = MagicMock() mock_program_model.id = 1 mock_program_model.key = "test-program" mock_program_model.name = "Test Program" # ... set other fields # Mock the admins queryset mock_program_model.admins.all.return_value = [ MagicMock(name="admin1"), MagicMock(name="admin2"), ] # Instantiate real ProgramNode with the mocked model program_node = ProgramNode(id=strawberry.ID("prog-1")) # Set the underlying model or mock the data source ProgramNode uses program_node._mock_model = mock_program_model # Adjust based on actual implementation return program_nodebackend/tests/apps/mentorship/model/task_level_test.py (1)
1-26: LGTM! Well-structured mock tests for TaskLevel.The tests correctly validate that
TaskLevel.__str__accessesmodule.namedirectly rather than callingstr(module). The use of pure mocks aligns with the project's testing preferences.♻️ Optional: Consider more idiomatic __str__ invocation
While calling
TaskLevel.__str__(task_level)as a class method works correctly, the more Pythonic approach is to use thestr()built-in:- assert TaskLevel.__str__(task_level) == "Test Module - Beginner Task" + assert str(task_level) == "Test Module - Beginner Task"Apply similarly to line 26. This is purely stylistic and doesn't affect functionality.
backend/tests/apps/mentorship/api/internal/queries/api_queries_program_test.py (2)
35-47: Remove unusedmock_infofixture parameter.The
get_programmethod doesn't accept aninfoparameter (per the implementation inprogram.py), somock_infois injected but never used. This applies to bothtest_get_program_successandtest_get_program_does_not_exist.♻️ Proposed fix
@patch("apps.mentorship.api.internal.queries.program.Program.objects.prefetch_related") def test_get_program_success( - self, mock_program_prefetch_related: MagicMock, mock_info: MagicMock, api_program_queries + self, mock_program_prefetch_related: MagicMock, api_program_queries ) -> None:@patch("apps.mentorship.api.internal.queries.program.Program.objects.prefetch_related") def test_get_program_does_not_exist( - self, mock_program_prefetch_related: MagicMock, mock_info: MagicMock, api_program_queries + self, mock_program_prefetch_related: MagicMock, api_program_queries ) -> None:
81-94: Remove dead mock setup for modules relationship.Lines 83, 87 set up
mock_program.modules.return_value.mentors.return_value...but this doesn't match actual code usage. Themy_programsmethod only accessesprogram.admins.all()for role checking—themodules__mentors__github_userpath is used only inprefetch_relatedfor the filter query, not for direct attribute access. This dead setup adds confusion.Same issue appears in lines 176, 211, 215, 219.
♻️ Proposed fix for lines 81-87
mock_admin_program = MagicMock(spec=Program, nest_created_at="2023-01-01", id=1) mock_admin_program.admins.all.return_value = [mock_mentor] - mock_admin_program.modules.return_value.mentors.return_value.github_user.return_value = [] mock_mentor_program = MagicMock(spec=Program, nest_created_at="2023-01-02", id=2) mock_mentor_program.admins.all.return_value = [] - mock_mentor_program.modules.return_value.mentors.return_value = [mock_mentor]
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
backend/apps/github/models/__init__.pybackend/pyproject.tomlbackend/tests/apps/mentorship/__init__.pybackend/tests/apps/mentorship/api/__init__.pybackend/tests/apps/mentorship/api/internal/__init__.pybackend/tests/apps/mentorship/api/internal/nodes/__init__.pybackend/tests/apps/mentorship/api/internal/nodes/api_internal_enum_test.pybackend/tests/apps/mentorship/api/internal/nodes/api_internal_mentee_test.pybackend/tests/apps/mentorship/api/internal/nodes/api_internal_mentor_test.pybackend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.pybackend/tests/apps/mentorship/api/internal/nodes/api_internal_program_test.pybackend/tests/apps/mentorship/api/internal/queries/__init__.pybackend/tests/apps/mentorship/api/internal/queries/api_queries_mentorship_test.pybackend/tests/apps/mentorship/api/internal/queries/api_queries_module_test.pybackend/tests/apps/mentorship/api/internal/queries/api_queries_program_test.pybackend/tests/apps/mentorship/management/__init__.pybackend/tests/apps/mentorship/management/commands/__init__.pybackend/tests/apps/mentorship/management/commands/mentorship_sync_issue_levels_test.pybackend/tests/apps/mentorship/management/commands/mentorship_sync_module_issues_test.pybackend/tests/apps/mentorship/management/commands/mentorship_update_comments_test.pybackend/tests/apps/mentorship/model/__init__.pybackend/tests/apps/mentorship/model/experience_level_test.pybackend/tests/apps/mentorship/model/mentee_test.pybackend/tests/apps/mentorship/model/mentor_module_test.pybackend/tests/apps/mentorship/model/mentor_test.pybackend/tests/apps/mentorship/model/module_test.pybackend/tests/apps/mentorship/model/program_test.pybackend/tests/apps/mentorship/model/task_level_test.pybackend/tests/apps/mentorship/model/task_test.py
💤 Files with no reviewable changes (1)
- backend/pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (8)
- backend/tests/apps/mentorship/model/mentor_test.py
- backend/tests/apps/mentorship/management/commands/mentorship_sync_module_issues_test.py
- backend/tests/apps/mentorship/model/module_test.py
- backend/tests/apps/mentorship/model/mentee_test.py
- backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py
- backend/tests/apps/mentorship/api/internal/nodes/api_internal_enum_test.py
- backend/tests/apps/mentorship/model/task_test.py
- backend/tests/apps/mentorship/api/internal/nodes/api_internal_mentee_test.py
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: kart-u
Repo: OWASP/Nest PR: 3179
File: backend/tests/apps/mentorship/model/test_program.py:13-22
Timestamp: 2026-01-06T21:11:08.745Z
Learning: In the mentorship test suite (backend/tests/apps/mentorship/), the maintainer kart-u prefers pure mock tests over Django TestCase with database interactions for model tests.
📚 Learning: 2026-01-06T21:11:03.537Z
Learnt from: kart-u
Repo: OWASP/Nest PR: 3179
File: backend/tests/apps/mentorship/model/test_program.py:13-22
Timestamp: 2026-01-06T21:11:03.537Z
Learning: In backend/tests/apps/mentorship/, prefer pure mock tests (no real database interactions) for model tests. Avoid using Django TestCase with database access; implement tests with mocks/stubs to exercise logic and, when needed, lightweight fixtures without DB. This guideline targets all Python test files under the mentorship module.
Applied to files:
backend/tests/apps/mentorship/api/internal/queries/api_queries_module_test.pybackend/tests/apps/mentorship/api/internal/nodes/api_internal_program_test.pybackend/tests/apps/mentorship/model/experience_level_test.pybackend/tests/apps/mentorship/management/commands/mentorship_update_comments_test.pybackend/tests/apps/mentorship/model/mentor_module_test.pybackend/tests/apps/mentorship/api/internal/nodes/api_internal_mentor_test.pybackend/tests/apps/mentorship/model/task_level_test.pybackend/tests/apps/mentorship/management/commands/mentorship_sync_issue_levels_test.pybackend/tests/apps/mentorship/model/program_test.pybackend/tests/apps/mentorship/api/internal/queries/api_queries_program_test.pybackend/tests/apps/mentorship/api/internal/queries/api_queries_mentorship_test.py
📚 Learning: 2025-12-31T05:17:39.659Z
Learnt from: kart-u
Repo: OWASP/Nest PR: 3101
File: backend/apps/common/extensions.py:92-98
Timestamp: 2025-12-31T05:17:39.659Z
Learning: In this codebase, import OperationType for GraphQL operations from the graphql-core package rather than from strawberry. Use 'from graphql import OperationType'. Strawberry re-exports via graphql-core internally, so relying on strawberry's API may be brittle. Apply this rule to all Python files that deal with GraphQL operation types; ensure imports come from graphql (graphql-core) and not from strawberry packages. This improves compatibility and avoids coupling to strawberry's internals.
Applied to files:
backend/tests/apps/mentorship/api/internal/queries/api_queries_module_test.pybackend/tests/apps/mentorship/api/internal/nodes/api_internal_program_test.pybackend/tests/apps/mentorship/model/experience_level_test.pybackend/tests/apps/mentorship/management/commands/mentorship_update_comments_test.pybackend/tests/apps/mentorship/model/mentor_module_test.pybackend/tests/apps/mentorship/api/internal/nodes/api_internal_mentor_test.pybackend/tests/apps/mentorship/model/task_level_test.pybackend/tests/apps/mentorship/management/commands/mentorship_sync_issue_levels_test.pybackend/tests/apps/mentorship/model/program_test.pybackend/tests/apps/mentorship/api/internal/queries/api_queries_program_test.pybackend/apps/github/models/__init__.pybackend/tests/apps/mentorship/api/internal/queries/api_queries_mentorship_test.py
📚 Learning: 2026-01-01T17:48:23.963Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:41-47
Timestamp: 2026-01-01T17:48:23.963Z
Learning: In Django code, be aware that a QuerySet's boolean evaluation (e.g., if not queryset) runs a database query to determine emptiness. While it is technically valid to use the queryset in a boolean context, use queryset.exists() for existence checks to avoid unnecessary queries and improve performance. Applicable broadly to Python/Django files rather than just this specific path.
Applied to files:
backend/tests/apps/mentorship/api/internal/queries/api_queries_module_test.pybackend/tests/apps/mentorship/api/internal/nodes/api_internal_program_test.pybackend/tests/apps/mentorship/model/experience_level_test.pybackend/tests/apps/mentorship/management/commands/mentorship_update_comments_test.pybackend/tests/apps/mentorship/model/mentor_module_test.pybackend/tests/apps/mentorship/api/internal/nodes/api_internal_mentor_test.pybackend/tests/apps/mentorship/model/task_level_test.pybackend/tests/apps/mentorship/management/commands/mentorship_sync_issue_levels_test.pybackend/tests/apps/mentorship/model/program_test.pybackend/tests/apps/mentorship/api/internal/queries/api_queries_program_test.pybackend/apps/github/models/__init__.pybackend/tests/apps/mentorship/api/internal/queries/api_queries_mentorship_test.py
📚 Learning: 2025-07-13T05:55:46.436Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: backend/apps/mentorship/graphql/mutations/program.py:166-166
Timestamp: 2025-07-13T05:55:46.436Z
Learning: In the OWASP Nest mentorship GraphQL mutations, Strawberry GraphQL automatically converts between Django Program instances and ProgramNode types, so mutations can return Program instances directly without manual conversion even when typed as ProgramNode, similar to the Module/ModuleNode pattern.
Applied to files:
backend/tests/apps/mentorship/api/internal/nodes/api_internal_program_test.py
📚 Learning: 2025-12-18T05:39:42.678Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:40-40
Timestamp: 2025-12-18T05:39:42.678Z
Learning: In Django management commands, prefer using self.stdout.write(...) over print(...) for user-facing stdout output. This aligns with Django conventions and improves testability. When emitting messages, consider using self.stdout.write and, for styled messages, use self.style.SUCCESS/ERROR as appropriate to maintain consistent command output formatting. Apply this guideline to all Python files within any project's management/commands directory.
Applied to files:
backend/tests/apps/mentorship/management/commands/mentorship_update_comments_test.pybackend/tests/apps/mentorship/management/commands/mentorship_sync_issue_levels_test.py
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS is actively used in frontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsx for program editing functionality and cannot be removed. It serves a different purpose than GET_PROGRAM_ADMIN_DETAILS, providing comprehensive program information needed for editing.
Applied to files:
backend/tests/apps/mentorship/api/internal/queries/api_queries_program_test.py
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS and GET_PROGRAM_ADMIN_DETAILS are two separate queries serving different purposes: GET_PROGRAM_DETAILS fetches comprehensive program information while GET_PROGRAM_ADMIN_DETAILS fetches only admin-related details. These queries cannot be removed or merged as they serve different use cases in the application.
Applied to files:
backend/tests/apps/mentorship/api/internal/queries/api_queries_program_test.py
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS and GET_PROGRAM_ADMIN_DETAILS are two separate queries serving different purposes: GET_PROGRAM_DETAILS fetches comprehensive program information while GET_PROGRAM_ADMIN_DETAILS fetches only admin-related details.
Applied to files:
backend/tests/apps/mentorship/api/internal/queries/api_queries_program_test.py
🧬 Code graph analysis (10)
backend/tests/apps/mentorship/api/internal/queries/api_queries_module_test.py (1)
backend/apps/mentorship/api/internal/queries/module.py (4)
ModuleQuery(15-50)get_program_modules(19-26)get_project_modules(29-36)get_module(39-50)
backend/tests/apps/mentorship/api/internal/nodes/api_internal_program_test.py (1)
backend/apps/mentorship/api/internal/nodes/enum.py (2)
ExperienceLevelEnum(12-18)ProgramStatusEnum(22-27)
backend/tests/apps/mentorship/model/experience_level_test.py (1)
backend/apps/mentorship/models/common/experience_level.py (2)
ExperienceLevel(6-25)ExperienceLevelChoices(12-18)
backend/tests/apps/mentorship/model/mentor_module_test.py (2)
backend/apps/mentorship/models/mentor.py (1)
Mentor(11-40)backend/apps/mentorship/models/mentor_module.py (1)
MentorModule(11-42)
backend/tests/apps/mentorship/api/internal/nodes/api_internal_mentor_test.py (1)
backend/apps/mentorship/api/internal/nodes/mentor.py (3)
avatar_url(13-15)name(18-20)login(23-25)
backend/tests/apps/mentorship/management/commands/mentorship_sync_issue_levels_test.py (2)
backend/tests/apps/mentorship/management/commands/mentorship_update_comments_test.py (2)
command(39-46)make_qs(11-20)backend/tests/apps/mentorship/management/commands/mentorship_sync_module_issues_test.py (2)
command(23-31)make_qs(13-19)
backend/tests/apps/mentorship/model/program_test.py (2)
backend/apps/mentorship/models/program.py (1)
ProgramStatus(25-28)backend/tests/apps/mentorship/model/module_test.py (1)
test_str_returns_name(25-30)
backend/tests/apps/mentorship/api/internal/queries/api_queries_program_test.py (1)
backend/apps/mentorship/api/internal/queries/program.py (2)
get_program(23-32)my_programs(35-80)
backend/apps/github/models/__init__.py (2)
backend/tests/apps/github/models/issue_test.py (1)
issue(26-28)backend/apps/github/models/issue.py (1)
Issue(19-235)
backend/tests/apps/mentorship/api/internal/queries/api_queries_mentorship_test.py (1)
backend/apps/mentorship/api/internal/queries/mentorship.py (4)
MentorshipQuery(32-124)is_mentor(36-48)get_mentee_details(51-82)get_mentee_module_issues(85-124)
🔇 Additional comments (20)
backend/tests/apps/mentorship/model/mentor_module_test.py (2)
1-3: LGTM! Imports are appropriate for mock-based testing.The imports correctly bring in
MagicMockfor creating test doubles and the necessary models for testing the__str__method.
6-18: Test logic is correct and follows codebase patterns.The test properly verifies that
MentorModule.__str__composes the mentor and module string representations. The use of pure mocks aligns with the project's testing approach for mentorship models.Based on learnings, pure mock tests are preferred for this module.
backend/apps/github/models/__init__.py (1)
5-5: LGTM! Issue model is now publicly accessible.The re-export of
Issuealigns with the existing pattern for other models (Comment, Commit, Label, etc.) and makes the model accessible to tests and other modules that import fromapps.github.models.backend/tests/apps/mentorship/management/commands/mentorship_sync_issue_levels_test.py (5)
1-5: LGTM! Standard test imports.The imports are appropriate for a Django management command test using pytest and mocking.
8-15: LGTM! Command fixture follows Django testing conventions.The fixture properly mocks
stdoutandstylemethods, aligning with Django management command testing patterns. Based on learnings, this approach avoids database interactions and uses pure mocks.
27-34: LGTM! Early exit test provides good coverage.The test correctly verifies that the command exits early with an appropriate message when no TaskLevel objects exist.
37-93: LGTM! Comprehensive test coverage for the update logic.This test thoroughly covers the main update scenarios:
- New level assignment (issue_with_label_a)
- Level update (issue_with_label_b)
- Level removal when no match (issue_no_match)
- No change when already correct (issue_already_up_to_date)
The assertions correctly verify bulk_update is called with only the modified issues and the success message is output.
96-121: LGTM! No-update scenario is properly tested.The test correctly verifies that
bulk_updateis not called when all issues already have the correct level, and the appropriate up-to-date message is displayed.backend/tests/apps/mentorship/management/commands/mentorship_update_comments_test.py (6)
1-9: LGTM! Imports are appropriate.The imports include all necessary modules for testing the management command with mocks and pytest.
23-35: LGTM! Helper factories provide clean test data construction.The
make_userandmake_commenthelpers create focused, readable test fixtures without database interactions, aligning with the maintainer's preference for pure mock tests.
38-69: LGTM! Fixtures are well-structured and reusable.The command, mock_module, and mock_issue fixtures provide consistent test setup with appropriate mocking of Django ORM behavior and stdout/styling for command output verification.
72-89: LGTM! Early exit conditions are properly tested.Both tests verify that the command exits gracefully with appropriate messages when preconditions aren't met (no published modules, no modules with labels).
92-125: LGTM! Orchestration test verifies the integration flow.The test confirms that
process_modulecorrectly coordinates the syncing of issue comments and processing of user interests, verifying both operations are called with the expected arguments.
128-300: LGTM! Comprehensive coverage of interest processing scenarios.The test suite thoroughly covers all scenarios for
process_issue_interests:
- New interest registration (lines 128-155)
- Interest removal (lines 158-186)
- Existing interest cleanup (lines 189-217)
- Multiple comments from single user (lines 220-247)
- Mixed scenarios with multiple users (lines 250-300)
The tests verify correct bulk operations, filtering, and appropriate stdout messages for each scenario. The assertions in the multi-user test (lines 250-300) are particularly thorough in validating the complex interaction between interest creation and removal.
backend/tests/apps/mentorship/model/program_test.py (1)
5-8: LGTM! Correct enum value assertions.The enum value checks correctly validate the ProgramStatus choices.
backend/tests/apps/mentorship/api/internal/queries/api_queries_module_test.py (1)
1-112: LGTM! Comprehensive and well-structured query tests.The test suite thoroughly covers ModuleQuery behaviors:
- Both success and empty-result scenarios for program/project module retrieval
- Single module lookup with proper exception handling
- Correct verification of ORM chain methods (filter, select_related, prefetch_related, order_by)
- Pure mock approach without database interactions
The tests align well with the project's testing standards.
backend/tests/apps/mentorship/model/experience_level_test.py (1)
1-9: LGTM! Clear and correct enum choice validation.The test straightforwardly validates the ExperienceLevel choice mappings, ensuring consistency between enum values and their string representations.
backend/tests/apps/mentorship/api/internal/queries/api_queries_mentorship_test.py (1)
1-465: LGTM! Excellent comprehensive test coverage for MentorshipQuery.This test suite is exceptionally thorough:
TestIsMentor:
- Validates mentor identification logic
- Handles edge cases (empty/whitespace/None login) using parametrize
- Tests GithubUser.DoesNotExist scenario
TestGetMenteeDetails:
- Covers successful mentee detail retrieval with proper field mapping
- Tests all failure paths (Module, GithubUser, Mentee DoesNotExist)
- Validates enrollment checking
TestGetMenteeModuleIssues:
- Tests issue retrieval with proper ORM chaining (filter, only, prefetch_related, order_by)
- Validates pagination with limit/offset parameters
- Covers out-of-range pagination gracefully
- Tests all failure paths comprehensively
The extensive use of mocks aligns perfectly with the project's preference for pure mock tests without database interactions.
backend/tests/apps/mentorship/api/internal/queries/api_queries_program_test.py (2)
1-29: LGTM!Imports are appropriate, and fixtures are well-structured with clear docstrings. The
mock_infofixture correctly sets up the nestedcontext.request.user.github_userstructure matching the actual code's access pattern.
117-235: LGTM!The remaining tests provide good coverage for:
- Mentor not found scenario with correct empty response
- No programs found with proper pagination defaults
- Search filtering with
name__icontainsverification- Pagination calculation across multiple pages
The test structure is clean and follows the pure mock approach per learnings. Based on learnings, this aligns with the preferred testing style for the mentorship module.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
backend/tests/apps/mentorship/model/program_test.py (1)
11-14: str test looks correct. Consider adding docstrings.The test correctly verifies the str method using a mock and follows the pattern from module_test.py. Consider adding docstrings to match the style shown in the relevant code snippets from module_test.py.
📝 Suggested docstrings
def test_program_status_choices(self): + """Unit test: verify ProgramStatus enum values map to expected strings.""" assert Program.ProgramStatus.DRAFT == "draft" assert Program.ProgramStatus.PUBLISHED == "published" assert Program.ProgramStatus.COMPLETED == "completed" def test_str_returns_name(self): + """Unit test: __str__ should return the program name (pure mock).""" mock_program_instance = MagicMock(spec=Program) mock_program_instance.name = "Security Program" assert Program.__str__(mock_program_instance) == "Security Program"Based on relevant code snippets showing the docstring pattern in module_test.py.
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py (3)
11-151: Consider testing the actualModuleNodeclass instead of a duplicate implementation.The
FakeModuleNodeclass replicates the resolver logic from the actualModuleNode. This means the tests verify the behavior ofFakeModuleNode, not the production code. If the actualModuleNodeimplementation changes, these tests may still pass while the production code is broken.Based on learnings, pure mock tests are preferred - this is achievable by importing the actual
ModuleNodeclass and mocking its ORM dependencies (e.g.,_mentors_manager,menteemodule_set,_issues_qs), rather than creating a separate test double with copied logic.Is there a specific reason the actual
ModuleNodecannot be instantiated for testing (e.g., Strawberry type initialization requirements)? If so, consider adding a comment explaining this constraint and perhaps a mechanism to ensureFakeModuleNodestays in sync with the actual implementation.
384-390: Missing docstring for consistency.This test function lacks a docstring, unlike all other test functions in the file.
📝 Add docstring for consistency
def test_module_node_task_assigned_at_none(mock_module_node): + """Test task_assigned_at when no assignment timestamp is found.""" with patch("apps.mentorship.models.task.Task.objects") as mock_task_objects:
37-151: Docstrings should use imperative mood per linting rules (D401).The static analysis flagged multiple docstrings starting with "Retrieves..." or "Returns..." instead of "Retrieve..." or "Return...". If D401 is enforced in CI, these will need adjustment.
Example fix for line 38:
def mentors(self): - """Retrieves all mentors associated with the module.""" + """Retrieve all mentors associated with the module.""" return self._mentors_manager.all()Apply similar pattern to other docstrings (lines 42, 53, 70, 74, 83, 90, 101, 110, 124, 139).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.pybackend/tests/apps/mentorship/model/program_test.py
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: kart-u
Repo: OWASP/Nest PR: 3179
File: backend/tests/apps/mentorship/model/test_program.py:13-22
Timestamp: 2026-01-06T21:11:08.745Z
Learning: In the mentorship test suite (backend/tests/apps/mentorship/), the maintainer kart-u prefers pure mock tests over Django TestCase with database interactions for model tests.
📚 Learning: 2026-01-06T21:11:03.537Z
Learnt from: kart-u
Repo: OWASP/Nest PR: 3179
File: backend/tests/apps/mentorship/model/test_program.py:13-22
Timestamp: 2026-01-06T21:11:03.537Z
Learning: In backend/tests/apps/mentorship/, prefer pure mock tests (no real database interactions) for model tests. Avoid using Django TestCase with database access; implement tests with mocks/stubs to exercise logic and, when needed, lightweight fixtures without DB. This guideline targets all Python test files under the mentorship module.
Applied to files:
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.pybackend/tests/apps/mentorship/model/program_test.py
📚 Learning: 2025-08-17T11:55:55.990Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2000
File: backend/apps/mentorship/management/commands/sync_module_issues.py:109-145
Timestamp: 2025-08-17T11:55:55.990Z
Learning: In the OWASP/Nest mentorship system, tasks are designed to be assigned to only one assignee per issue, even if GitHub issues can have multiple assignees. The sync_module_issues command correctly uses issue.assignees.first() to create one task per issue for the first assignee only.
Applied to files:
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py
📚 Learning: 2025-12-31T05:17:39.659Z
Learnt from: kart-u
Repo: OWASP/Nest PR: 3101
File: backend/apps/common/extensions.py:92-98
Timestamp: 2025-12-31T05:17:39.659Z
Learning: In this codebase, import OperationType for GraphQL operations from the graphql-core package rather than from strawberry. Use 'from graphql import OperationType'. Strawberry re-exports via graphql-core internally, so relying on strawberry's API may be brittle. Apply this rule to all Python files that deal with GraphQL operation types; ensure imports come from graphql (graphql-core) and not from strawberry packages. This improves compatibility and avoids coupling to strawberry's internals.
Applied to files:
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.pybackend/tests/apps/mentorship/model/program_test.py
📚 Learning: 2026-01-01T17:48:23.963Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:41-47
Timestamp: 2026-01-01T17:48:23.963Z
Learning: In Django code, be aware that a QuerySet's boolean evaluation (e.g., if not queryset) runs a database query to determine emptiness. While it is technically valid to use the queryset in a boolean context, use queryset.exists() for existence checks to avoid unnecessary queries and improve performance. Applicable broadly to Python/Django files rather than just this specific path.
Applied to files:
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.pybackend/tests/apps/mentorship/model/program_test.py
🧬 Code graph analysis (2)
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py (3)
backend/apps/mentorship/api/internal/nodes/enum.py (1)
ExperienceLevelEnum(12-18)backend/apps/mentorship/models/task.py (1)
Task(10-75)backend/apps/mentorship/models/issue_user_interest.py (1)
IssueUserInterest(6-36)
backend/tests/apps/mentorship/model/program_test.py (3)
backend/apps/mentorship/models/program.py (1)
ProgramStatus(25-28)backend/tests/apps/mentorship/model/module_test.py (1)
test_str_returns_name(25-30)backend/apps/mentorship/api/internal/nodes/mentor.py (1)
name(18-20)
🪛 Ruff (0.14.10)
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py
38-38: First line of docstring should be in imperative mood: "Retrieves all mentors associated with the module."
(D401)
42-42: First line of docstring should be in imperative mood: "Retrieves all mentees associated with the module."
(D401)
53-53: First line of docstring should be in imperative mood: "Retrieves mentees assigned to a specific issue within the module."
(D401)
70-70: First line of docstring should be in imperative mood: "Returns the name of the associated project."
(D401)
74-74: First line of docstring should be in imperative mood: "Retrieves issues associated with the module, with filtering and pagination."
(D401)
83-83: First line of docstring should be in imperative mood: "Returns the count of issues associated with the module."
(D401)
90-90: First line of docstring should be in imperative mood: "Retrieves a sorted list of unique labels used across issues in the module."
(D401)
101-101: First line of docstring should be in imperative mood: "Retrieves a single issue by its number within the module."
(D401)
110-110: First line of docstring should be in imperative mood: "Retrieves users who have expressed interest in a specific issue within the module."
(D401)
124-124: First line of docstring should be in imperative mood: "Retrieves the deadline for a task associated with a specific issue."
(D401)
139-139: First line of docstring should be in imperative mood: "Retrieves the assignment timestamp for a task associated with a specific issue."
(D401)
🔇 Additional comments (3)
backend/tests/apps/mentorship/model/program_test.py (1)
5-9: Enum value tests look correct.The test correctly verifies that ProgramStatus enum values map to their expected string representations.
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py (2)
154-171: Well-structured pytest fixture with comprehensive mock setup.The fixture properly configures return values for the mock managers and querysets. Each test function receives a fresh instance, avoiding state leakage between tests.
393-421: Good approach - testing actual input classes directly.This test correctly instantiates the actual
CreateModuleInputandUpdateModuleInputclasses and verifies their default values. This is the preferred pattern compared to testing a fake implementation.
kart-u
changed the title
kart-u
changed the title
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py:
- Around line 373-379: Add a descriptive docstring to the test function
test_module_node_task_assigned_at_none describing what scenario it verifies
(that task_assigned_at returns None when no task exists for the given
issue_number). Locate the test function named
test_module_node_task_assigned_at_none and insert a one-line docstring directly
under the def line to match the style of other tests in the file.
🧹 Nitpick comments (3)
backend/tests/apps/mentorship/model/program_test.py (1)
7-15: Optional: Consider adding docstrings for consistency.Other test files in the mentorship suite (e.g.,
module_test.py) include brief docstrings on test methods. Adding them here would maintain consistency across the test suite.📝 Example with docstrings
def test_program_status_choices(self): + """Verify ProgramStatus enum values map to expected strings.""" assert Program.ProgramStatus.DRAFT == "draft" assert Program.ProgramStatus.PUBLISHED == "published" assert Program.ProgramStatus.COMPLETED == "completed" def test_str_returns_name(self): + """Unit test: __str__ should return the program name (pure mock).""" mock_program_instance = MagicMock(spec=Program) mock_program_instance.name = "Security Program" assert Program.__str__(mock_program_instance) == "Security Program"backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py (2)
143-160: Consider more explicit mock setup for maintainability.The fixture uses chained method mocks (lines 149-155, 157-158) which can be fragile if the implementation's ORM call order changes. Additionally, the variable
mocked_query_prefetchis reused on lines 154 and 157, reducing clarity.Consider using more explicit mock configuration with named intermediates or
Mock(spec=...)to make the test setup more resilient to implementation changes.💡 Example refactor
@pytest.fixture def mock_module_node(): """Fixture for a mock ModuleNode instance.""" m = FakeModuleNode() # Mentors m._mentors_manager.all.return_value = [MagicMock(), MagicMock()] # Mentees mentee_chain = m.menteemodule_set.select_related.return_value.filter.return_value mentee_chain.values_list.return_value = ["github_user_id_1", "github_user_id_2"] # Issues queryset for issue_mentees and interested_users m._issues_qs.filter.return_value.values_list.return_value = ["issue_id_1"] # Issues with select_related and prefetch_related for issues() method issues_query_chain = m._issues_qs.select_related.return_value.prefetch_related.return_value issues_query_chain.filter.return_value.order_by.return_value = [MagicMock()] # Issues count m._issues_qs.count.return_value = 5 # Issue by number issue_by_num_chain = m._issues_qs.select_related.return_value.prefetch_related.return_value issue_by_num_chain.filter.return_value.first.return_value = MagicMock() return m
11-141: Consider maintenance implications of duplicating ModuleNode logic.The
FakeModuleNodeclass duplicates the business logic from the realModuleNodeimplementation. While this approach successfully tests the resolver logic without database access (consistent with the preference for pure mock tests), it creates a maintenance burden:
- Changes to the real
ModuleNoderequire manual updates toFakeModuleNode- No automated verification that
FakeModuleNodematches the real implementation- Risk of test/implementation drift over time
Alternative approaches to consider:
Test the real ModuleNode with patched ORM calls - Import and test the actual
ModuleNodeclass while patching database access, eliminating duplication.Extract business logic into testable functions - Move complex logic (like
mentees(),issue_mentees()) into separate utility functions that can be tested independently, leaving ModuleNode as a thin GraphQL resolver layer.Document the maintenance contract - If keeping
FakeModuleNode, add comments linking it to the real implementation and noting that both must be kept in sync.This is not a defect, but something to consider for long-term maintainability as the mentorship module evolves.
Based on learnings: In this test suite, pure mock tests are preferred over Django TestCase with database interactions for model tests.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.pybackend/tests/apps/mentorship/model/program_test.py
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: kart-u
Repo: OWASP/Nest PR: 3179
File: backend/tests/apps/mentorship/model/test_program.py:13-22
Timestamp: 2026-01-06T21:11:08.745Z
Learning: In the mentorship test suite (backend/tests/apps/mentorship/), the maintainer kart-u prefers pure mock tests over Django TestCase with database interactions for model tests.
📚 Learning: 2026-01-06T21:11:03.537Z
Learnt from: kart-u
Repo: OWASP/Nest PR: 3179
File: backend/tests/apps/mentorship/model/test_program.py:13-22
Timestamp: 2026-01-06T21:11:03.537Z
Learning: In backend/tests/apps/mentorship/, prefer pure mock tests (no real database interactions) for model tests. Avoid using Django TestCase with database access; implement tests with mocks/stubs to exercise logic and, when needed, lightweight fixtures without DB. This guideline targets all Python test files under the mentorship module.
Applied to files:
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.pybackend/tests/apps/mentorship/model/program_test.py
📚 Learning: 2025-08-17T11:55:55.990Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2000
File: backend/apps/mentorship/management/commands/sync_module_issues.py:109-145
Timestamp: 2025-08-17T11:55:55.990Z
Learning: In the OWASP/Nest mentorship system, tasks are designed to be assigned to only one assignee per issue, even if GitHub issues can have multiple assignees. The sync_module_issues command correctly uses issue.assignees.first() to create one task per issue for the first assignee only.
Applied to files:
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py
📚 Learning: 2025-12-31T05:17:39.659Z
Learnt from: kart-u
Repo: OWASP/Nest PR: 3101
File: backend/apps/common/extensions.py:92-98
Timestamp: 2025-12-31T05:17:39.659Z
Learning: In this codebase, import OperationType for GraphQL operations from the graphql-core package rather than from strawberry. Use 'from graphql import OperationType'. Strawberry re-exports via graphql-core internally, so relying on strawberry's API may be brittle. Apply this rule to all Python files that deal with GraphQL operation types; ensure imports come from graphql (graphql-core) and not from strawberry packages. This improves compatibility and avoids coupling to strawberry's internals.
Applied to files:
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.pybackend/tests/apps/mentorship/model/program_test.py
📚 Learning: 2026-01-01T17:48:23.963Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:41-47
Timestamp: 2026-01-01T17:48:23.963Z
Learning: In Django code, be aware that a QuerySet's boolean evaluation (e.g., if not queryset) runs a database query to determine emptiness. While it is technically valid to use the queryset in a boolean context, use queryset.exists() for existence checks to avoid unnecessary queries and improve performance. Applicable broadly to Python/Django files rather than just this specific path.
Applied to files:
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.pybackend/tests/apps/mentorship/model/program_test.py
🧬 Code graph analysis (2)
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py (3)
backend/apps/mentorship/api/internal/nodes/enum.py (1)
ExperienceLevelEnum(12-18)backend/apps/mentorship/models/task.py (1)
Task(10-75)backend/apps/mentorship/models/issue_user_interest.py (1)
IssueUserInterest(6-36)
backend/tests/apps/mentorship/model/program_test.py (2)
backend/apps/mentorship/models/program.py (1)
ProgramStatus(25-28)backend/tests/apps/mentorship/model/module_test.py (1)
test_str_returns_name(25-30)
🔇 Additional comments (1)
backend/tests/apps/mentorship/model/program_test.py (1)
1-15: LGTM! Tests are well-structured and follow project conventions.Both test methods are correctly implemented:
test_program_status_choicesverifies the ProgramStatus enum mappings match expected string values.test_str_returns_nameuses pure mocks to test the__str__method, aligning with the preferred testing approach for this module.The mock-based approach avoids database interactions while still testing the real implementation, which is exactly what's preferred in this test suite.
Based on learnings, pure mock tests are preferred for model tests in the mentorship module.
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py (2)
154-158: Redundant variable assignment.
mocked_query_prefetchis assigned twice on lines 154 and 157 with the same value. This appears to be copy-paste duplication.♻️ Suggested cleanup
mocked_query_prefetch = m._issues_qs.select_related.return_value.prefetch_related.return_value mocked_query_prefetch.filter.return_value.order_by.return_value = [MagicMock()] m._issues_qs.count.return_value = 5 - mocked_query_prefetch = m._issues_qs.select_related.return_value.prefetch_related.return_value mocked_query_prefetch.filter.return_value.first.return_value = MagicMock()
373-379: Missing docstring for consistency.This test function lacks a docstring, unlike all other test functions in this file.
♻️ Suggested fix
def test_module_node_task_assigned_at_none(mock_module_node): + """Test task_assigned_at when no assignment is found.""" with patch("apps.mentorship.models.task.Task.objects") as mock_task_objects:
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: kart-u
Repo: OWASP/Nest PR: 3179
File: backend/tests/apps/mentorship/model/test_program.py:13-22
Timestamp: 2026-01-06T21:11:08.745Z
Learning: In the mentorship test suite (backend/tests/apps/mentorship/), the maintainer kart-u prefers pure mock tests over Django TestCase with database interactions for model tests.
📚 Learning: 2026-01-06T21:11:03.537Z
Learnt from: kart-u
Repo: OWASP/Nest PR: 3179
File: backend/tests/apps/mentorship/model/test_program.py:13-22
Timestamp: 2026-01-06T21:11:03.537Z
Learning: In backend/tests/apps/mentorship/, prefer pure mock tests (no real database interactions) for model tests. Avoid using Django TestCase with database access; implement tests with mocks/stubs to exercise logic and, when needed, lightweight fixtures without DB. This guideline targets all Python test files under the mentorship module.
Applied to files:
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py
📚 Learning: 2025-08-17T11:55:55.990Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2000
File: backend/apps/mentorship/management/commands/sync_module_issues.py:109-145
Timestamp: 2025-08-17T11:55:55.990Z
Learning: In the OWASP/Nest mentorship system, tasks are designed to be assigned to only one assignee per issue, even if GitHub issues can have multiple assignees. The sync_module_issues command correctly uses issue.assignees.first() to create one task per issue for the first assignee only.
Applied to files:
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py
📚 Learning: 2025-12-31T05:17:39.659Z
Learnt from: kart-u
Repo: OWASP/Nest PR: 3101
File: backend/apps/common/extensions.py:92-98
Timestamp: 2025-12-31T05:17:39.659Z
Learning: In this codebase, import OperationType for GraphQL operations from the graphql-core package rather than from strawberry. Use 'from graphql import OperationType'. Strawberry re-exports via graphql-core internally, so relying on strawberry's API may be brittle. Apply this rule to all Python files that deal with GraphQL operation types; ensure imports come from graphql (graphql-core) and not from strawberry packages. This improves compatibility and avoids coupling to strawberry's internals.
Applied to files:
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py
📚 Learning: 2026-01-01T17:48:23.963Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:41-47
Timestamp: 2026-01-01T17:48:23.963Z
Learning: In Django code, be aware that a QuerySet's boolean evaluation (e.g., if not queryset) runs a database query to determine emptiness. While it is technically valid to use the queryset in a boolean context, use queryset.exists() for existence checks to avoid unnecessary queries and improve performance. Applicable broadly to Python/Django files rather than just this specific path.
Applied to files:
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py
🧬 Code graph analysis (1)
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py (3)
backend/apps/mentorship/api/internal/nodes/enum.py (1)
ExperienceLevelEnum(12-18)backend/apps/mentorship/models/task.py (1)
Task(10-75)backend/apps/mentorship/models/issue_user_interest.py (1)
IssueUserInterest(6-36)
🔇 Additional comments (1)
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py (1)
382-410: Good test coverage for input class defaults.This test correctly validates the actual
CreateModuleInputandUpdateModuleInputclasses from production code, ensuring thedomains,labels, andtagsfields properly default to empty lists.
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py:
- Line 8: The import line importing CreateModuleInput,
UpdateModuleInput,ModuleNode has a missing space after the comma; update the
statement that references CreateModuleInput, UpdateModuleInput, and ModuleNode
so there is a space after the comma (i.e., put a space between
UpdateModuleInput, and ModuleNode) to conform to style and avoid linter errors.
🧹 Nitpick comments (1)
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py (1)
37-70: Add missing spaces after commas for PEP 8 compliance.Several method calls are missing spaces after commas (lines 44, 50, 53, 59, 62, 66, 69). This affects readability and violates PEP 8 style guidelines.
✨ Proposed fixes
def mock_issue_mentees(self, issue_number: int): - return ModuleNode.issue_mentees(self,issue_number) + return ModuleNode.issue_mentees(self, issue_number) def mock_issues(self, limit: int = 20, offset: int = 0, label: str | None = None): - return ModuleNode.issues(self,limit=limit,offset=offset,label=label) + return ModuleNode.issues(self, limit=limit, offset=offset, label=label) def mock_issues_count(self, label: str | None = None): - return ModuleNode.issues_count(self,label=label) + return ModuleNode.issues_count(self, label=label) def mock_issue_by_number(self, number: int): - return ModuleNode.issue_by_number(self,number=number) + return ModuleNode.issue_by_number(self, number=number) def mock_interested_users(self, issue_number: int): - return ModuleNode.interested_users(self,issue_number=issue_number) + return ModuleNode.interested_users(self, issue_number=issue_number) def mock_task_deadline(self, issue_number: int): - return ModuleNode.task_deadline(self,issue_number=issue_number) + return ModuleNode.task_deadline(self, issue_number=issue_number) def mock_task_assigned_at(self, issue_number: int): - return ModuleNode.task_assigned_at(self,issue_number=issue_number) + return ModuleNode.task_assigned_at(self, issue_number=issue_number)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: kart-u
Repo: OWASP/Nest PR: 3179
File: backend/tests/apps/mentorship/model/test_program.py:13-22
Timestamp: 2026-01-06T21:11:08.745Z
Learning: In the mentorship test suite (backend/tests/apps/mentorship/), the maintainer kart-u prefers pure mock tests over Django TestCase with database interactions for model tests.
📚 Learning: 2026-01-06T21:11:03.537Z
Learnt from: kart-u
Repo: OWASP/Nest PR: 3179
File: backend/tests/apps/mentorship/model/test_program.py:13-22
Timestamp: 2026-01-06T21:11:03.537Z
Learning: In backend/tests/apps/mentorship/, prefer pure mock tests (no real database interactions) for model tests. Avoid using Django TestCase with database access; implement tests with mocks/stubs to exercise logic and, when needed, lightweight fixtures without DB. This guideline targets all Python test files under the mentorship module.
Applied to files:
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py
📚 Learning: 2025-08-17T11:55:55.990Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2000
File: backend/apps/mentorship/management/commands/sync_module_issues.py:109-145
Timestamp: 2025-08-17T11:55:55.990Z
Learning: In the OWASP/Nest mentorship system, tasks are designed to be assigned to only one assignee per issue, even if GitHub issues can have multiple assignees. The sync_module_issues command correctly uses issue.assignees.first() to create one task per issue for the first assignee only.
Applied to files:
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py
📚 Learning: 2025-12-31T05:17:39.659Z
Learnt from: kart-u
Repo: OWASP/Nest PR: 3101
File: backend/apps/common/extensions.py:92-98
Timestamp: 2025-12-31T05:17:39.659Z
Learning: In this codebase, import OperationType for GraphQL operations from the graphql-core package rather than from strawberry. Use 'from graphql import OperationType'. Strawberry re-exports via graphql-core internally, so relying on strawberry's API may be brittle. Apply this rule to all Python files that deal with GraphQL operation types; ensure imports come from graphql (graphql-core) and not from strawberry packages. This improves compatibility and avoids coupling to strawberry's internals.
Applied to files:
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py
📚 Learning: 2026-01-01T17:48:23.963Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:41-47
Timestamp: 2026-01-01T17:48:23.963Z
Learning: In Django code, be aware that a QuerySet's boolean evaluation (e.g., if not queryset) runs a database query to determine emptiness. While it is technically valid to use the queryset in a boolean context, use queryset.exists() for existence checks to avoid unnecessary queries and improve performance. Applicable broadly to Python/Django files rather than just this specific path.
Applied to files:
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py
🧬 Code graph analysis (1)
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py (4)
backend/apps/mentorship/api/internal/nodes/enum.py (1)
ExperienceLevelEnum(12-18)backend/apps/mentorship/api/internal/nodes/mentor.py (2)
name(18-20)login(23-25)backend/apps/github/api/internal/nodes/issue.py (1)
labels(50-52)backend/apps/mentorship/api/internal/nodes/module.py (7)
mentors(38-40)mentees(43-51)issue_mentees(54-68)available_labels(100-108)issue_by_number(111-118)task_deadline(134-145)task_assigned_at(148-159)
🔇 Additional comments (2)
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py (2)
92-310: Excellent test coverage with appropriate mocking strategy.The tests comprehensively cover ModuleNode behavior including edge cases (no issues, no project, no deadline, etc.) while using pure mocks to avoid database interactions. This approach aligns well with the project's testing preferences.
Based on learnings, the maintainer prefers pure mock tests over Django TestCase with database interactions for the mentorship module tests.
312-340: Good coverage of default values for input types.The test verifies that
CreateModuleInputandUpdateModuleInputproperly default domains, labels, and tags to empty lists, preventing potentialNone-related errors in downstream code.
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py
Outdated
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py (1)
264-287: Consider consistent mock variable naming.Tests correctly validate deadline retrieval logic. Minor suggestion: the variable naming for
mock_task_order_bydiffers between the two tests:
- Line 267: assigns the queryset result (
.order_by.return_value)- Line 282: assigns the order_by method (
.order_by)While both approaches work, using consistent naming would improve readability.
♻️ Proposed consistency improvement
def test_module_node_task_deadline_none(mock_module_node): """Test task_deadline when no deadline is found.""" with patch("apps.mentorship.models.task.Task.objects") as mock_task_objects: - mock_task_order_by = mock_task_objects.filter.return_value.order_by - mock_task_order_by.return_value.values_list.return_value.first.return_value = None + mock_task_order_by = mock_task_objects.filter.return_value.order_by.return_value + mock_task_order_by.values_list.return_value.first.return_value = None deadline = mock_module_node.mock_task_deadline(issue_number=101) assert deadline is None
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.pybackend/tests/apps/mentorship/api/internal/nodes/api_internal_program_test.py
✅ Files skipped from review due to trivial changes (1)
- backend/tests/apps/mentorship/api/internal/nodes/api_internal_program_test.py
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: kart-u
Repo: OWASP/Nest PR: 3179
File: backend/tests/apps/mentorship/model/test_program.py:13-22
Timestamp: 2026-01-06T21:11:08.745Z
Learning: In the mentorship test suite (backend/tests/apps/mentorship/), the maintainer kart-u prefers pure mock tests over Django TestCase with database interactions for model tests.
📚 Learning: 2026-01-06T21:11:03.537Z
Learnt from: kart-u
Repo: OWASP/Nest PR: 3179
File: backend/tests/apps/mentorship/model/test_program.py:13-22
Timestamp: 2026-01-06T21:11:03.537Z
Learning: In backend/tests/apps/mentorship/, prefer pure mock tests (no real database interactions) for model tests. Avoid using Django TestCase with database access; implement tests with mocks/stubs to exercise logic and, when needed, lightweight fixtures without DB. This guideline targets all Python test files under the mentorship module.
Applied to files:
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py
📚 Learning: 2025-08-17T11:55:55.990Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2000
File: backend/apps/mentorship/management/commands/sync_module_issues.py:109-145
Timestamp: 2025-08-17T11:55:55.990Z
Learning: In the OWASP/Nest mentorship system, tasks are designed to be assigned to only one assignee per issue, even if GitHub issues can have multiple assignees. The sync_module_issues command correctly uses issue.assignees.first() to create one task per issue for the first assignee only.
Applied to files:
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py
📚 Learning: 2025-07-11T15:57:56.648Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: backend/apps/mentorship/graphql/queries/module.py:39-50
Timestamp: 2025-07-11T15:57:56.648Z
Learning: In the OWASP Nest mentorship GraphQL queries, Strawberry GraphQL automatically converts between Django Module instances and ModuleNode types, so methods can return Module instances directly without manual conversion even when typed as ModuleNode.
Applied to files:
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py
📚 Learning: 2025-07-13T05:55:46.436Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: backend/apps/mentorship/graphql/mutations/program.py:166-166
Timestamp: 2025-07-13T05:55:46.436Z
Learning: In the OWASP Nest mentorship GraphQL mutations, Strawberry GraphQL automatically converts between Django Program instances and ProgramNode types, so mutations can return Program instances directly without manual conversion even when typed as ProgramNode, similar to the Module/ModuleNode pattern.
Applied to files:
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py
📚 Learning: 2025-12-31T05:17:39.659Z
Learnt from: kart-u
Repo: OWASP/Nest PR: 3101
File: backend/apps/common/extensions.py:92-98
Timestamp: 2025-12-31T05:17:39.659Z
Learning: In this codebase, import OperationType for GraphQL operations from the graphql-core package rather than from strawberry. Use 'from graphql import OperationType'. Strawberry re-exports via graphql-core internally, so relying on strawberry's API may be brittle. Apply this rule to all Python files that deal with GraphQL operation types; ensure imports come from graphql (graphql-core) and not from strawberry packages. This improves compatibility and avoids coupling to strawberry's internals.
Applied to files:
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py
📚 Learning: 2026-01-01T17:48:23.963Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:41-47
Timestamp: 2026-01-01T17:48:23.963Z
Learning: In Django code, be aware that a QuerySet's boolean evaluation (e.g., if not queryset) runs a database query to determine emptiness. While it is technically valid to use the queryset in a boolean context, use queryset.exists() for existence checks to avoid unnecessary queries and improve performance. Applicable broadly to Python/Django files rather than just this specific path.
Applied to files:
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py
🧬 Code graph analysis (1)
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py (4)
backend/apps/mentorship/api/internal/nodes/enum.py (1)
ExperienceLevelEnum(12-18)backend/apps/mentorship/api/internal/nodes/mentor.py (2)
name(18-20)login(23-25)backend/apps/github/api/internal/nodes/issue.py (1)
labels(50-52)backend/apps/mentorship/api/internal/nodes/module.py (7)
mentors(38-40)mentees(43-51)issue_mentees(54-68)available_labels(100-108)issue_by_number(111-118)task_deadline(134-145)task_assigned_at(148-159)
🔇 Additional comments (13)
backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py (13)
1-12: LGTM!Imports are well-organized and appropriate for the test suite.
15-72: Excellent alignment with testing preferences!The
FakeModuleNodetest double effectively enables testing actualModuleNoderesolver logic without database interactions. This approach correctly implements the pure-mock testing strategy preferred for the mentorship module.Based on learnings, the maintainer prefers pure mock tests over Django TestCase with database interactions for model tests.
74-91: LGTM!The fixture properly configures mock querysets with appropriate return values, supporting the comprehensive test scenarios that follow.
94-109: LGTM!Comprehensive validation of field assignments.
111-132: LGTM!Both tests correctly validate resolver behavior and queryset operations without database access.
134-161: LGTM!Thorough coverage of
issue_menteesbehavior including the edge case where no issue is found.
163-173: LGTM!Proper handling of both present and absent project relationships.
175-198: LGTM!Comprehensive coverage of issues querying with and without label filters.
200-213: LGTM!Correct validation of label filtering and retrieval logic.
215-229: LGTM!Thorough validation of queryset optimization with
select_relatedandprefetch_related.
231-262: LGTM!Proper coverage of interested users resolution including edge case handling.
289-312: LGTM!Proper coverage of assignment timestamp retrieval. The same optional naming consistency suggestion from the
task_deadlinetests applies here (lines 292 vs 307).
314-342: Default list fields correctly initialized to empty lists.The test correctly validates that
domains,labels, andtagsin bothCreateModuleInputandUpdateModuleInputdefault to empty lists viastrawberry.field(default_factory=list), preventing common bugs with None values.
1 participant
Proposed change
Resolves #2872
Added tests for app/mentorship
Left out tests for
backend/apps/mentorship/api/internal/mutationsintentionally due some ongoing work on permissions -- #3101 #3185 (waiting for their completion)Checklist
make check-testlocally and all tests passed