Extend Nest API chapter and project
#2606
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdded a new public Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 0
🧹 Nitpick comments (2)
backend/tests/apps/api/rest/v0/chapter_test.py (1)
35-35: Consider testing with populated leaders.The test only verifies the empty list case. Add a test case with actual leader names to ensure the resolver correctly maps
leaders_rawto theleadersfield.Example test data:
{ ... "leaders_raw": ["Alice Smith", "Bob Jones"], }Then assert:
assert chapter.leaders == ["Alice Smith", "Bob Jones"]backend/tests/apps/api/rest/v0/project_test.py (1)
35-35: Consider testing with populated leaders.Similar to the chapter test, consider adding a test case with actual leader names to ensure the resolver correctly maps
leaders_rawto theleadersfield.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/apps/api/rest/v0/chapter.py(1 hunks)backend/apps/api/rest/v0/project.py(1 hunks)backend/tests/apps/api/rest/v0/chapter_test.py(1 hunks)backend/tests/apps/api/rest/v0/project_test.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
backend/tests/apps/api/rest/v0/project_test.py (1)
backend/apps/api/rest/v0/project.py (1)
ProjectDetail(39-48)
backend/apps/api/rest/v0/chapter.py (2)
backend/apps/owasp/api/internal/nodes/common.py (1)
leaders(19-21)backend/apps/api/rest/v0/project.py (1)
resolve_leaders(46-48)
backend/apps/api/rest/v0/project.py (2)
backend/apps/owasp/api/internal/nodes/common.py (1)
leaders(19-21)backend/apps/api/rest/v0/chapter.py (1)
resolve_leaders(45-47)
backend/tests/apps/api/rest/v0/chapter_test.py (2)
backend/apps/api/rest/v0/chapter.py (1)
ChapterDetail(37-47)backend/apps/owasp/api/internal/nodes/common.py (1)
leaders(19-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (4)
backend/tests/apps/api/rest/v0/chapter_test.py (1)
42-42: LGTM!The assertion correctly validates that the
leadersfield is populated fromleaders_raw.backend/tests/apps/api/rest/v0/project_test.py (1)
42-42: LGTM!The assertion correctly validates that the
leadersfield is populated fromleaders_raw.backend/apps/api/rest/v0/chapter.py (1)
41-47: No issues found. Theleaders_rawattribute exists.The
ChapterDetail.resolve_leaders()method safely accessesobj.leaders_rawbecause the Chapter model inherits this attribute from its base classRepositoryBasedEntityModel. The attribute is defined as amodels.JSONFieldinbackend/apps/owasp/models/common.py(line 56), and multiple tests and management commands confirm it is properly accessible at runtime.backend/apps/api/rest/v0/project.py (1)
43-48: The code is correct—no changes needed.The
ProjectDetail.resolve_leaders()method correctly accessesobj.leaders_raw, which is a valid model field inherited fromRepositoryBasedEntityModel. TheProjectclass inherits fromRepositoryBasedEntityModel, whereleaders_rawis defined as amodels.JSONField()at line 56 ofbackend/apps/owasp/models/common.py. The resolver will not raise anAttributeErrorat runtime.Likely an incorrect or invalid review comment.
rudransh-shrivastava
requested review from
arkid15r and
kasya
as code owners
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/tests/apps/api/rest/v0/project_test.py (2)
30-32: Consider extracting MockEntityMember to a shared test utility.The
MockEntityMemberclass is duplicated in bothchapter_test.pyandproject_test.py. Extracting it to a common test fixture or utility module would reduce duplication and make future maintenance easier.Example approach:
Create
backend/tests/apps/api/rest/v0/conftest.pyor a test utilities module:class MockEntityMember: def __init__(self, name): self.member_name = nameThen import and use it in both test files.
39-39: Consider adding test case for empty leaders list.The current test only validates the happy path with two leaders. Adding a test case with an empty
entity_leaderslist would improve coverage and ensure the resolver handles edge cases correctly.Example:
def test_project_serializer_with_no_leaders(): class MockEntityMember: def __init__(self, name): self.member_name = name class MockProject: def __init__(self, data): for key, value in data.items(): setattr(self, key, value) self.nest_key = data["key"] self.entity_leaders = [] # Empty leaders list project_data = { "created_at": "2023-01-01T00:00:00Z", "description": "A test project", "key": "test-project", "level": "other", "name": "Test Project", "updated_at": "2023-01-02T00:00:00Z", } project = ProjectDetail.from_orm(MockProject(project_data)) assert project.leaders == []
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/apps/api/rest/v0/chapter.py(1 hunks)backend/apps/api/rest/v0/project.py(1 hunks)backend/tests/apps/api/rest/v0/chapter_test.py(2 hunks)backend/tests/apps/api/rest/v0/project_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/tests/apps/api/rest/v0/chapter_test.py
- backend/apps/api/rest/v0/chapter.py
- backend/apps/api/rest/v0/project.py
🧰 Additional context used
🧬 Code graph analysis (1)
backend/tests/apps/api/rest/v0/project_test.py (2)
backend/tests/apps/api/rest/v0/chapter_test.py (1)
MockEntityMember(34-36)backend/apps/api/rest/v0/project.py (1)
ProjectDetail(39-48)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (1)
backend/tests/apps/api/rest/v0/project_test.py (1)
46-46: LGTM!The assertion correctly validates that the
leadersfield is properly serialized from the mockentity_leadersdata, matching the expected behavior of theresolve_leadersmethod inProjectDetail.
| region: str | ||
|
|
||
| @staticmethod | ||
| def resolve_leaders(obj): |
There was a problem hiding this comment.
These are technically leader names (not leader objects).
I'm not sure how to handle compound objects within API yet (add minimum data + a separate endpoint vs adding all data).
There was a problem hiding this comment.
Yeah, I'll update it to leader_names for now. But I think it's wrong to name it leader_names. What do I do?
I found Stripe API Docs / Expand interesting.
The API has an Expand feature that allows you to retrieve linked objects in a single call, effectively replacing the object ID with all its properties and values. For example, say you wanted to access details on a customer tied to a given Checkout Session. You would retrieve the Checkout Session and pass the customer property to the expand array, which tells Stripe to include the entire Customer object in the response
There was a problem hiding this comment.
I'm not sure how to implement this, but I can look into it if we go down this path :)
There was a problem hiding this comment.
Let's keep it simple and use compact leader representation objects: rename it to leaders and provide objects with member key and name -- should be enough for now.
There was a problem hiding this comment.
Got it!
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
backend/apps/api/rest/v0/project.py (1)
45-48: Extract duplicated resolver logic to a shared helper.The
resolve_leader_namesmethod is identical to the one inbackend/apps/api/rest/v0/chapter.py(lines 47-49). This duplication violates DRY principles and increases maintenance burden.Consider extracting this logic to a shared utility function or mixin that both
ProjectDetailandChapterDetailcan use. For example:# In a shared utils module (e.g., apps/api/rest/v0/utils.py) def resolve_entity_leader_names(obj): """Resolve leader names from entity_leaders relationship.""" return [leader.member_name for leader in obj.entity_leaders]Then use it in both schemas:
@staticmethod def resolve_leader_names(obj): """Resolve leader names.""" - return [leader.member_name for leader in obj.entity_leaders] + from apps.api.rest.v0.utils import resolve_entity_leader_names + return resolve_entity_leader_names(obj)backend/tests/apps/api/rest/v0/project_test.py (2)
30-39: Extract duplicated mock class to a shared test fixture.The
MockEntityMemberclass is identical to the one inbackend/tests/apps/api/rest/v0/chapter_test.py(lines 33-35). This duplication increases maintenance overhead and violates DRY principles.Consider creating a shared test fixture or conftest.py:
# In backend/tests/apps/api/rest/v0/conftest.py or a shared fixtures module import pytest class MockEntityMember: """Mock for entity member used in API serializer tests.""" def __init__(self, name): self.member_name = name @pytest.fixture def mock_entity_members(): """Fixture providing sample entity members.""" return [MockEntityMember("Alice"), MockEntityMember("Bob")]Then simplify both test files:
def test_project_serializer_validation(project_data, mock_entity_members): class MockProject: def __init__(self, data): for key, value in data.items(): setattr(self, key, value) self.nest_key = data["key"] self.entity_leaders = mock_entity_members # ... rest of test
46-46: Add edge case tests for leader_names.The test only covers the happy path with two leaders. Consider adding test cases for:
- Empty
entity_leaderslist- Projects with no leaders assigned
- Leaders with missing or
Nonemember_namevaluesAdd additional test cases:
def test_project_serializer_with_no_leaders(): """Test serialization when project has no leaders.""" class MockProject: def __init__(self): self.created_at = "2023-01-01T00:00:00Z" self.description = "Test project" self.nest_key = "test-key" self.level = "incubator" self.name = "Test" self.updated_at = "2023-01-02T00:00:00Z" self.entity_leaders = [] project = ProjectDetail.from_orm(MockProject()) assert project.leader_names == []
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/apps/api/rest/v0/chapter.py(1 hunks)backend/apps/api/rest/v0/project.py(1 hunks)backend/tests/apps/api/rest/v0/chapter_test.py(2 hunks)backend/tests/apps/api/rest/v0/project_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/tests/apps/api/rest/v0/chapter_test.py
- backend/apps/api/rest/v0/chapter.py
🧰 Additional context used
🧬 Code graph analysis (2)
backend/tests/apps/api/rest/v0/project_test.py (2)
backend/tests/apps/api/rest/v0/chapter_test.py (1)
MockEntityMember(34-36)backend/apps/api/rest/v0/project.py (1)
ProjectDetail(39-48)
backend/apps/api/rest/v0/project.py (1)
backend/apps/api/rest/v0/chapter.py (1)
resolve_leader_names(48-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (1)
backend/apps/api/rest/v0/project.py (1)
46-48: Remove the review comment—the code is safe as-is.The
entity_leadersproperty returns aQuerySetfromEntityMember.objects.filter(...)(notNone), andmember_nameis a non-nullableCharField(max_length=255)with nonull=Trueorblank=Trueconstraint. EveryEntityMemberinstance is guaranteed to have amember_namevalue, and the resolver correctly iterates over the QuerySet without any risk ofAttributeErrororTypeError.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/apps/api/rest/v0/chapter.pybackend/apps/api/rest/v0/common.pybackend/apps/api/rest/v0/project.pybackend/tests/apps/api/rest/v0/chapter_test.pybackend/tests/apps/api/rest/v0/project_test.py
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/apps/api/rest/v0/chapter.py
- backend/tests/apps/api/rest/v0/project_test.py
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2223
File: backend/apps/owasp/models/common.py:213-232
Timestamp: 2025-09-06T19:03:01.985Z
Learning: In the OWASP Nest project, the get_leaders_emails() method in RepositoryBasedEntityModel is designed to only capture leaders with mailto: links from leaders.md files, intentionally ignoring plain text names without email addresses. The current regex implementation works correctly for the intended behavior as validated by comprehensive test cases.
📚 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/api/rest/v0/chapter_test.pybackend/apps/api/rest/v0/common.pybackend/apps/api/rest/v0/project.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/api/rest/v0/chapter_test.pybackend/apps/api/rest/v0/common.pybackend/apps/api/rest/v0/project.py
📚 Learning: 2025-11-23T11:37:26.253Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2606
File: backend/apps/api/rest/v0/project.py:43-48
Timestamp: 2025-11-23T11:37:26.253Z
Learning: In the OWASP Nest backend, `entity_leaders` is a `property` method defined in `RepositoryBasedEntityModel` (backend/apps/owasp/models/common.py) that returns a dynamically constructed QuerySet. It cannot be prefetched using standard `prefetch_related()` because Django's prefetch mechanism only works on model fields and relations, not property methods.
Applied to files:
backend/apps/api/rest/v0/project.py
🧬 Code graph analysis (2)
backend/tests/apps/api/rest/v0/chapter_test.py (2)
backend/tests/apps/api/rest/v0/project_test.py (2)
MockMember(30-32)MockEntityMember(34-37)backend/apps/owasp/api/internal/nodes/common.py (1)
leaders(19-21)
backend/apps/api/rest/v0/project.py (4)
backend/apps/api/rest/v0/common.py (1)
Leader(6-10)backend/apps/owasp/api/internal/nodes/common.py (1)
leaders(19-21)backend/apps/api/rest/v0/chapter.py (1)
resolve_leaders(48-53)backend/apps/mentorship/api/internal/nodes/mentor.py (2)
login(23-25)name(18-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
🔇 Additional comments (4)
backend/apps/api/rest/v0/common.py (1)
6-10: LGTM!The
Leaderschema is well-designed with an optionalkeyfield to handle leaders without GitHub logins and a requirednamefield to ensure every leader has an identifier.backend/tests/apps/api/rest/v0/chapter_test.py (2)
34-41: LGTM!The mock classes appropriately simulate the
EntityMemberandMembermodel relationships, correctly handling both scenarios where a leader has a login and where they don't.
60-64: LGTM!The assertions thoroughly validate the leader mapping logic, covering both cases: leaders with GitHub logins (key present) and leaders without (key is None).
backend/apps/api/rest/v0/project.py (1)
44-44: LGTM!The
leadersfield addition successfully extends the API response to include leader information, fulfilling the PR objective. The field type correctly references the sharedLeaderschema.
|
2 participants
Resolves #2569
Proposed change
leadersfield.Checklist
make check-testlocally; all checks and tests passed.