Implement Milestones #1406
Implement Milestones #1406
Conversation
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughThis change introduces comprehensive support for GitHub milestones in the backend application. It adds a new Changes
Assessment against linked issues
Suggested reviewers
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
backend/apps/github/graphql/nodes/milestone.py (1)
1-21: 🛠️ Refactor suggestionMilestoneNode implementation looks good but is missing the
due_onfield.The GraphQL node implementation for Milestone is well-structured and correctly exposes key milestone attributes. However, the
due_onfield defined in the Milestone model is not included in the exposed fields, which could be important for clients who need milestone deadlines.Consider adding the
due_onfield to make milestone deadlines available through the GraphQL API:fields = ( "author", "created_at", "state", "title", "open_issues_count", "closed_issues_count", + "due_on", )
🧹 Nitpick comments (5)
backend/apps/github/graphql/queries/milestone.py (2)
29-56: Typo in docstring and unused parameter.There's a typo in the docstring: "maimum" should be "maximum". Also, as noted earlier, the
distinctparameter is accepted but not used in the implementation."""Resolve open milestones. Args: root (object): The root object. info (ResolveInfo): The GraphQL execution context. - limit (int): The maimum number of milestones to return. + limit (int): The maximum number of milestones to return. login (str, optional): Filter milestones by author login. organization (str, optional): Filter milestones by organization login. distinct (bool, optional): Whether to return distinct milestones.
57-88: Same issues in the closed milestones resolver.This resolver has the same issues as the open milestones resolver: a typo in the docstring and an unused
distinctparameter."""Resolve closed milestones. Args: root (object): The root object. info (ResolveInfo): The GraphQL execution context. - limit (int): The maimum number of milestones to return. + limit (int): The maximum number of milestones to return. login (str, optional): Filter milestones by author login. organization (str, optional): Filter milestones by organization login. distinct (bool, optional): Whether to return distinct milestones.Also implement the
distinctparameter usage as mentioned in the previous comment.backend/apps/github/models/milestone.py (3)
1-1: Fix typo in docstring.There's a typo in the module docstring: "Gitub" should be "GitHub".
-"""Gitub app Milestone model.""" +"""GitHub app Milestone model."""
79-81: Remove unnecessary save method override.The
savemethod doesn't add any functionality beyond what's provided by the parent class, making it redundant.- def save(self, *args, **kwargs): - """Save Milestone.""" - super().save(*args, **kwargs)
92-96: Fix parameter description in docstring.The docstring for
update_datarefers to "Author of the issue" and "Repository of the issue" instead of "milestone".- author (User, optional): Author of the issue. Defaults to None. - repository (Repository, optional): Repository of the issue. Defaults to None. + author (User, optional): Author of the milestone. Defaults to None. + repository (Repository, optional): Repository of the milestone. Defaults to None.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/apps/github/graphql/nodes/milestone.py(1 hunks)backend/apps/github/graphql/queries/milestone.py(1 hunks)backend/apps/github/migrations/0024_milestone.py(1 hunks)backend/apps/github/models/__init__.py(1 hunks)backend/apps/github/models/managers/milestone.py(1 hunks)backend/apps/github/models/milestone.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
backend/apps/github/models/__init__.py (1)
backend/apps/github/models/milestone.py (1)
Milestone(10-108)
backend/apps/github/graphql/nodes/milestone.py (2)
backend/apps/common/graphql/nodes.py (1)
BaseNode(6-10)backend/apps/github/models/milestone.py (2)
Milestone(10-108)Meta(17-20)
backend/apps/github/models/milestone.py (3)
backend/apps/common/models.py (1)
BulkSaveModel(8-30)backend/apps/github/models/generic_issue_model.py (1)
GenericIssueModel(10-77)backend/apps/github/models/managers/milestone.py (2)
ClosedMilestoneManager(14-19)OpenMilestoneManager(6-11)
backend/apps/github/graphql/queries/milestone.py (3)
backend/apps/github/graphql/nodes/milestone.py (1)
MilestoneNode(7-20)backend/apps/common/graphql/queries.py (1)
BaseQuery(6-7)backend/apps/github/models/milestone.py (1)
Milestone(10-108)
backend/apps/github/migrations/0024_milestone.py (1)
backend/apps/github/models/mixins/issue.py (1)
IssueIndexMixin(4-138)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
🔇 Additional comments (9)
backend/apps/github/models/__init__.py (1)
3-3: Import for new Milestone model added correctly.The Milestone model is now properly exposed through the package's
__init__.pyfile, making it available when importing from the github.models package. This addition correctly integrates the new model into the application's namespace.backend/apps/github/models/managers/milestone.py (1)
1-20: Custom milestone managers implemented correctly.The implementation of the Open and Closed milestone managers follows Django best practices for custom managers. These managers provide an elegant way to filter milestones by state and are properly used in the GraphQL queries.
backend/apps/github/models/milestone.py (6)
10-16: LGTM: Good manager organization.The model correctly uses three managers: the default manager and two custom managers for open and closed milestones. This enables convenient filtering by state.
17-20: LGTM: Appropriate Meta configuration.The Meta class correctly specifies the database table, verbose name plural, and default ordering by update time and state.
22-46: LGTM: Well-structured model fields.All fields are appropriately defined with correct types, constraints, and relationships. The model properly tracks issue counts, due date, and relationships to Repository, User (author), and Labels.
48-78: LGTM: Effective data population from GitHub API.The
from_githubmethod efficiently maps GitHub API fields to model fields using a dictionary mapping and handles null values appropriately.
83-86: LGTM: Efficient bulk save implementation.The
bulk_savemethod leverages the sharedBulkSaveModel.bulk_saveutility for efficient batch operations.
99-108: LGTM: Appropriate data sync logic.The
update_datamethod correctly handles retrieval or creation of milestone instances by node ID, updates fields from GitHub data, and provides options for saving behavior.backend/apps/github/migrations/0024_milestone.py (1)
9-83: LGTM: Correctly structured migration for Milestone model.The migration properly creates the Milestone model with all required fields, relationships, and meta options. The model inherits from IssueIndexMixin and includes appropriate database constraints and indexes.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/apps/github/common.py (2)
70-74: Consider adding a comment explaining the walrus operator usageThe walrus operator (
:=) is a Python 3.8+ feature that might not be immediately familiar to all developers. Consider adding a brief comment explaining its purpose here.
76-91: Consider implementing bulk save for milestonesThe code currently saves each milestone individually. Consider implementing a bulk save operation similar to what's done for releases and repository contributors (e.g.,
Release.bulk_saveon line 196,RepositoryContributor.bulk_saveon line 199) to improve performance, especially for repositories with many milestones.+ # Prepare milestones for bulk save + milestones = [] for gh_milestone in gh_repository.get_milestones(**kwargs): if gh_milestone.updated_at < until: break author = User.update_data(gh_milestone.user) - milestone = Milestone.update_data(gh_milestone, author=author, repository=repository) + milestone = Milestone.update_data(gh_milestone, author=author, repository=repository, save=False) + milestones.append(milestone) # Labels milestone.labels.clear() for gh_milestone_label in gh_milestone.get_labels(): try: milestone.labels.add(Label.update_data(gh_milestone_label)) except UnknownObjectException: logger.info("Couldn't get GitHub milestone label %s", milestone.url) + + # Bulk save milestones + Milestone.bulk_save(milestones)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/apps/github/common.py(4 hunks)backend/apps/github/migrations/0026_issue_milestone_pullrequest_milestone.py(1 hunks)backend/apps/github/models/issue.py(5 hunks)backend/apps/github/models/pull_request.py(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/apps/github/migrations/0026_issue_milestone_pullrequest_milestone.py
🧰 Additional context used
🧬 Code Graph Analysis (2)
backend/apps/github/models/pull_request.py (1)
backend/apps/github/models/issue.py (3)
from_github(84-123)update_data(186-210)save(163-172)
backend/apps/github/models/issue.py (1)
backend/apps/github/models/pull_request.py (3)
from_github(65-101)update_data(113-139)save(103-105)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (13)
backend/apps/github/common.py (3)
11-11: Good addition of Milestone model importThe import has been properly added to support the new milestone synchronization functionality.
63-91: Milestone synchronization implementation looks goodThe implementation follows the same pattern as other entity synchronizations in this file:
- Set up filtering parameters with sensible defaults
- Establish a time threshold using the latest updated entity or fallback to 30 days ago
- Process entities until reaching older ones
- Update related entities (author in this case)
- Handle labels with proper error handling
158-164: Check milestone author assignmentSimilar to the issue milestone handling, when updating a milestone associated with a pull request, the code passes the PR's author as the milestone's author. Please verify if this is the correct behavior.
backend/apps/github/models/issue.py (5)
62-68: Well-structured milestone relationshipThe foreign key field is properly defined with:
- Appropriate on_delete behavior (CASCADE)
- Nullable field (as not all issues have milestones)
- Clear related_name ("issues") for reverse lookups
84-93: Method signature and docstring properly updatedThe
from_githubmethod signature and documentation have been correctly updated to include the new milestone parameter.
122-124: Clean milestone assignment implementationSimple and clear assignment of the milestone relationship.
186-194: Method signature and docstring properly updatedThe
update_datastatic method signature and documentation have been correctly updated to include the new milestone parameter.
206-206: Appropriate parameter passingThe milestone parameter is correctly passed to the
from_githubmethod.backend/apps/github/models/pull_request.py (5)
43-49: Well-structured milestone relationshipThe foreign key field is properly defined with:
- Appropriate on_delete behavior (CASCADE)
- Nullable field (as not all pull requests have milestones)
- Clear related_name ("pull_requests") for reverse lookups
65-73: Method signature and docstring properly updatedThe
from_githubmethod signature and documentation have been correctly updated to include the new milestone parameter.
100-102: Clean milestone assignment implementationSimple and clear assignment of the milestone relationship.
113-121: Method signature and docstring properly updatedThe
update_datastatic method signature and documentation have been correctly updated to include the new milestone parameter.
133-135: Appropriate parameter passingThe milestone parameter is correctly passed to the
from_githubmethod, and the method call formatting has been properly adjusted to maintain readability with the added parameter.
…filters, and improve related data fetching
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
backend/apps/github/graphql/nodes/milestone.py (6)
10-12: Remove excessive blank lines.There are three consecutive blank lines here. Consider removing them to adhere to PEP 8 style guidelines.
from apps.github.graphql.nodes.pull_request import PullRequestNode - - - class MilestoneNode(BaseNode):
18-18: Remove whitespace from blank lines.Several blank lines contain whitespace characters, which violates PEP 8 style guidelines. This is also flagged by Ruff static analysis.
issues = graphene.List(IssueNode) pull_requests = graphene.List(PullRequestNode) - + class Meta:Also applies to: 35-35, 39-39
🧰 Tools
🪛 Ruff (0.8.2)
18-18: Blank line contains whitespace
Remove whitespace from blank line
(W293)
1-2: Enhance the module docstring with more details.The current docstring is very basic. Consider enhancing it to provide more context about the purpose of this module and how it fits into the overall GraphQL schema.
-"""Github Milestone Node.""" +"""Github Milestone Node. + +This module defines the GraphQL node type for GitHub milestones. +It exposes milestone data and relationships to issues and pull requests +to clients through the GraphQL API. +"""🧰 Tools
🪛 GitHub Actions: Run CI/CD
[error] 1-40: Ruff formatting errors fixed automatically by pre-commit hook. Please run 'pre-commit run --all-files' locally to apply formatting changes.
14-14: Fix inconsistent capitalization in docstring.The class docstring uses "Github" while the model and other files use "GitHub" (with uppercase "H"). Maintain consistent capitalization across the codebase.
- """Github Milestone Node.""" + """GitHub Milestone Node."""
5-8: Organize imports according to PEP 8 conventions.While the imports are functionally correct, they could be better organized following PEP 8 conventions: standard library imports, then third-party, then local application imports. Also consider alphabetical ordering within each group.
import graphene from apps.common.graphql.nodes import BaseNode -from apps.github.models.milestone import Milestone from apps.github.graphql.nodes.issue import IssueNode from apps.github.graphql.nodes.pull_request import PullRequestNode +from apps.github.models.milestone import Milestone
36-42: Enhance resolver docstrings with return type information.The resolver docstrings could be more informative by specifying what they return and any filtering that might be applied.
- def resolve_issues(self, info): - """Resolve issues.""" + def resolve_issues(self, info): + """Resolve issues associated with this milestone. + + Returns: + QuerySet: All issues linked to this milestone. + """ return self.issues.all() - def resolve_pull_requests(self, info): - """Resolve pull requests.""" + def resolve_pull_requests(self, info): + """Resolve pull requests associated with this milestone. + + Returns: + QuerySet: All pull requests linked to this milestone. + """ return self.pull_requests.all()🧰 Tools
🪛 Ruff (0.8.2)
39-39: Blank line contains whitespace
Remove whitespace from blank line
(W293)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/apps/github/graphql/nodes/milestone.py(1 hunks)backend/apps/github/graphql/queries/milestone.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/github/graphql/queries/milestone.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/apps/github/graphql/nodes/milestone.py (4)
backend/apps/common/graphql/nodes.py (1)
BaseNode(6-10)backend/apps/github/models/milestone.py (2)
Milestone(10-108)Meta(17-20)backend/apps/github/graphql/nodes/issue.py (1)
IssueNode(9-31)backend/apps/github/graphql/nodes/pull_request.py (1)
PullRequestNode(9-34)
🪛 Ruff (0.8.2)
backend/apps/github/graphql/nodes/milestone.py
18-18: Blank line contains whitespace
Remove whitespace from blank line
(W293)
35-35: Blank line contains whitespace
Remove whitespace from blank line
(W293)
39-39: Blank line contains whitespace
Remove whitespace from blank line
(W293)
🪛 GitHub Actions: Run CI/CD
backend/apps/github/graphql/nodes/milestone.py
[error] 1-40: Ruff formatting errors fixed automatically by pre-commit hook. Please run 'pre-commit run --all-files' locally to apply formatting changes.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
backend/apps/owasp/models/project.py (2)
154-161: Clarify docstring to specify "open" milestonesThe docstring currently says "Return milestones" but should be more specific since this property returns only open milestones.
@property def open_milestones(self): - """Return milestones.""" + """Return open milestones.""" return Milestone.open_milestones.filter( repository__in=self.repositories.all(), ).select_related( "repository", )
163-170: Clarify docstring to specify "closed" milestonesThe docstring currently says "Return milestones" but should be more specific since this property returns only closed milestones.
@property def closed_milestones(self): - """Return milestones.""" + """Return closed milestones.""" return Milestone.closed_milestones.filter( repository__in=self.repositories.all(), ).select_related( "repository", )backend/apps/github/graphql/queries/milestone.py (1)
124-127: Minor formatting inconsistencyThe formatting for the organization filter is different between the open and closed milestone resolvers. Consider standardizing for consistency.
if organization: - closed_milestones = closed_milestones.filter( - repository__organization__login=organization - ) + closed_milestones = closed_milestones.filter(repository__organization__login=organization)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/apps/github/graphql/nodes/milestone.py(1 hunks)backend/apps/github/graphql/queries/milestone.py(1 hunks)backend/apps/owasp/graphql/nodes/project.py(3 hunks)backend/apps/owasp/models/project.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/github/graphql/nodes/milestone.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/apps/owasp/models/project.py (1)
backend/apps/github/models/milestone.py (1)
Milestone(10-108)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
🔇 Additional comments (8)
backend/apps/owasp/graphql/nodes/project.py (4)
6-6: LGTM - Import added for MilestoneNodeThe import for MilestoneNode is properly added to support the new milestone-related fields.
25-26: LGTM - New GraphQL fields added for milestonesThe new fields
open_milestonesandclosed_milestonesare correctly defined as Lists of MilestoneNode, following the project's conventions.
67-70: LGTM - Well-implemented resolver for open milestonesThe resolver correctly returns open milestones with author data and appropriate ordering by creation date.
71-74: LGTM - Well-implemented resolver for closed milestonesThe resolver correctly returns closed milestones with author data and appropriate ordering by creation date.
backend/apps/github/graphql/queries/milestone.py (4)
1-13: LGTM - Good implementation of the MilestoneQuery classThe new MilestoneQuery class follows the project's architecture patterns and includes appropriate imports and documentation.
14-30: LGTM - Well-defined GraphQL query fieldsBoth query fields for open and closed milestones have appropriate parameters with sensible defaults. The parameters allow for flexible filtering of milestone data.
32-84: LGTM - Comprehensive resolver for open milestonesThe resolver for open milestones is well-implemented with:
- Proper database optimizations using select_related and prefetch_related
- Appropriate filtering based on query parameters
- Correct implementation of the distinct parameter using subqueries
- Clear and detailed docstring
85-139: LGTM - Comprehensive resolver for closed milestonesThe resolver for closed milestones follows the same pattern as the open milestones resolver with proper optimizations and filtering.
…ate RepositoryNode to resolve them
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/apps/github/graphql/nodes/repository.py (2)
95-101: Consider adding pagination for milestone lists.The resolver for closed milestones doesn't limit the number of returned items. If repositories have many closed milestones, this could impact performance. Consider adding pagination or limits similar to how
resolve_issuesandresolve_releasesuseRECENT_ISSUES_LIMITandRECENT_RELEASES_LIMIT.def resolve_closed_milestones(self, info): """Resolve closed milestones.""" return self.closed_milestones.select_related( "repository", - ).order_by( - "-created_at", - ) + ).order_by( + "-created_at", + )[:10] # Add a reasonable limit or make configurable
89-91: Potentially redundantselect_related.Using
select_related("repository")when we're already accessing milestones through a repository instance might be redundant. This doesn't cause issues but could be unnecessary optimization.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/apps/github/graphql/nodes/repository.py(3 hunks)backend/apps/github/models/repository.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/github/models/repository.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/apps/github/graphql/nodes/repository.py (2)
backend/apps/github/graphql/nodes/milestone.py (1)
MilestoneNode(11-39)backend/apps/github/models/repository.py (2)
open_milestones(183-185)closed_milestones(188-190)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (3)
backend/apps/github/graphql/nodes/repository.py (3)
7-7: Import added correctly.The import statement for
MilestoneNodeis correctly placed with other node imports, maintaining the code organization pattern.
20-21: New GraphQL fields properly defined.The
open_milestonesandclosed_milestonesfields are added to expose milestone data through the repository node, correctly typed as lists ofMilestoneNode.
87-93:❓ Verification inconclusive
Sorting discrepancy between resolver and model.
The resolver is sorting milestones by
-created_atwhile the repository model properties shown in the context sort by-due_on. Consider aligning the sorting logic for consistency.
🏁 Script executed:
#!/bin/bash # Check if other milestone-related code is consistently using the same sorting rg -l "order_by\(.*-due_on" --type python rg -l "order_by\(.*-created_at" --type python | grep milestoneLength of output: 170
Align milestone sorting in resolver with model
- The
resolve_open_milestonesresolver currently orders by-created_at, while the milestone model’s natural ordering usesdue_on.- We weren’t able to find other usages of either field in the codebase, so please verify the intended sort key:
- If milestones should be ordered by due date, change to:
.order_by("-due_on")- If sorting by creation date is correct, add a comment or adjust the model ordering to reflect this choice.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
backend/apps/github/graphql/queries/milestone.py (4)
14-22: Good implementation! Consider making parameter names more descriptive.The query parameters are well-structured, but a few parameter names could be more descriptive:
- The
closeparameter would be clearer asis_closedorstate='closed'|'open'- The
distinctparameter actually filters for uniqueness by author (not general distinctness), which isn't immediately obvious from the namemilestones = graphene.List( MilestoneNode, limit=graphene.Int(default_value=5), login=graphene.String(required=False), organization=graphene.String(required=False), repository=graphene.String(required=False), distinct=graphene.Boolean(default_value=False), - close=graphene.Boolean(default_value=True), + is_closed=graphene.Boolean(default_value=True), )Remember to update the resolver method signature as well if you implement this change.
24-33: Update method parameters to match query parameters if you apply the suggested changes.If you rename the
closeparameter tois_closedin the query field definition, remember to update the resolver method signature to match.def resolve_milestones( root, info, limit, login=None, organization=None, repository=None, distinct=False, - close=True, + is_closed=True, ):
34-49: Documentation looks good! Consider clarifying the meaning of thedistinctparameter.The docstring is comprehensive, but could clarify that the
distinctparameter specifically filters for uniqueness by author (returning only the latest milestone per author), rather than general distinctness across all fields."""Resolve milestones. Args: root (object): The root object. info (ResolveInfo): The GraphQL execution context. limit (int): The maximum number of milestones to return. login (str, optional): Filter milestones by author login. organization (str, optional): Filter milestones by organization login. repository (str, optional): Filter milestones by repository name. - distinct (bool, optional): Whether to return distinct milestones. + distinct (bool, optional): Whether to return only the latest milestone per author. close (bool, optional): Whether to return open or closed milestones. Returns: list: A list of milestones. """
50-78: Consider updating the variable name if parameter name is changed.If you rename the
closeparameter tois_closed, remember to update line 50 to use the new parameter name.- milestones = Milestone.closed_milestones if close else Milestone.open_milestones + milestones = Milestone.closed_milestones if is_closed else Milestone.open_milestones
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
backend/apps/github/graphql/queries/milestone.py(1 hunks)frontend/__tests__/e2e/data/mockHomeData.ts(1 hunks)frontend/__tests__/e2e/pages/Home.spec.ts(1 hunks)frontend/__tests__/e2e/pages/ProjectDetails.spec.ts(1 hunks)frontend/__tests__/e2e/pages/RepositoryDetails.spec.ts(1 hunks)frontend/__tests__/unit/data/mockHomeData.ts(1 hunks)frontend/__tests__/unit/data/mockProjectDetailsData.ts(1 hunks)frontend/__tests__/unit/data/mockRepositoryData.ts(1 hunks)frontend/__tests__/unit/pages/Home.test.tsx(1 hunks)frontend/__tests__/unit/pages/ProjectDetails.test.tsx(1 hunks)frontend/__tests__/unit/pages/RepositoryDetails.test.tsx(1 hunks)frontend/src/server/queries/homeQueries.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- frontend/tests/unit/data/mockProjectDetailsData.ts
- frontend/tests/unit/data/mockHomeData.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/server/queries/homeQueries.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/apps/github/graphql/queries/milestone.py (4)
backend/apps/common/graphql/queries.py (1)
BaseQuery(6-7)backend/apps/github/graphql/nodes/milestone.py (1)
MilestoneNode(9-33)backend/apps/github/models/milestone.py (1)
Milestone(10-108)backend/apps/owasp/models/project.py (2)
closed_milestones(164-170)open_milestones(155-161)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (10)
frontend/__tests__/e2e/data/mockHomeData.ts (1)
198-229: Mock data structure looks good and follows existing patterns.The addition of
openMilestonesandclosedMilestonesarrays correctly follows the established pattern of the mock data file. The milestone objects include all the necessary fields that would be expected from a GitHub milestone: author details, title, issue counts, repository information, creation date, and URL.frontend/__tests__/unit/pages/ProjectDetails.test.tsx (1)
225-244: Well-structured test for milestone rendering.This test thoroughly verifies both open and closed milestones are rendered correctly on the project details page. It checks all key data points: title, repository name, and issue counts (both open and closed).
frontend/__tests__/e2e/pages/ProjectDetails.spec.ts (1)
91-103: Good E2E tests for milestone sections.The tests for open and closed milestones cover the important UI elements that users would expect to see, including headings, milestone titles, dates, and repository names. These tests effectively verify that the milestone data is correctly displayed on the project details page.
frontend/__tests__/e2e/pages/RepositoryDetails.spec.ts (1)
87-99: Consistent milestone tests for repository details page.These tests mirror the milestone tests in the project details page, maintaining consistency in test coverage. They verify the same key elements (headings, titles, dates, repository names) ensuring milestone data is properly displayed in the repository view as well.
frontend/__tests__/unit/data/mockRepositoryData.ts (1)
48-79: Mock data properly structured for milestone testing.The mock data for open and closed milestones includes all necessary fields (author details, title, issue counts, repository information, timestamps, and URLs) that would be needed to properly test the milestone feature rendering and functionality.
frontend/__tests__/unit/pages/Home.test.tsx (1)
233-252: Well-structured test for milestone rendering.The test follows the established pattern of the test suite and thoroughly verifies that all milestone properties (title, repository name, open/closed issue counts) are correctly rendered for both open and closed milestones.
frontend/__tests__/unit/pages/RepositoryDetails.test.tsx (1)
182-201: Thorough milestone rendering verification.This test properly validates that milestone data is correctly rendered in the Repository Details page. It verifies all important milestone properties including titles, repository names, and issue counts for both open and closed milestones.
frontend/__tests__/e2e/pages/Home.spec.ts (1)
79-91: Comprehensive E2E tests for milestone sections.The end-to-end tests for both open and closed milestone sections follow the project's testing pattern and effectively verify the presence and content of milestone UI elements. The tests confirm proper rendering of milestone titles, dates, and repository names.
backend/apps/github/graphql/queries/milestone.py (2)
50-59: Good use of select_related and prefetch_related for query optimization.The code efficiently uses Django's select_related and prefetch_related to reduce the number of database queries, which is a best practice for optimizing GraphQL resolvers.
68-77: Effective implementation of distinct author filtering.The implementation of the
distinctparameter is well done, using a subquery to get the latest milestone per author. This is much more efficient than fetching all milestones and filtering in Python.
| @@ -16,6 +17,8 @@ class RepositoryNode(BaseNode): | |||
| """Repository node.""" | |||
|
|
|||
| issues = graphene.List(IssueNode) | |||
| open_milestones = graphene.List(MilestoneNode) | |||
There was a problem hiding this comment.
Why do we need both open/closed milestones instead of having just milestones filterable by their state?
There was a problem hiding this comment.
I think having conditional statements are more costly for the CPU rather than having one additional field and this will make the code simpler (just using the managers instead of if-else). I used the open/closed milestones in the frontend.
| open_milestones = graphene.List(MilestoneNode, limit=graphene.Int(default_value=5)) | ||
| closed_milestones = graphene.List(MilestoneNode, limit=graphene.Int(default_value=5)) |
There was a problem hiding this comment.
Same here, I don't see why we need both. Do you think we should use this approach instead of just milestones filterable by state?
There was a problem hiding this comment.
I explained above my opinion. For the home query, I made it one query with an option to filter to not violate the DRY principle.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/apps/github/models/milestone.py (1)
89-91: Fix docstring references to "issue".The docstring refers to "Author of the issue" and "Repository of the issue" which should be "milestone" instead.
- author (User, optional): Author of the issue. Defaults to None. - repository (Repository, optional): Repository of the issue. Defaults to None. + author (User, optional): Author of the milestone. Defaults to None. + repository (Repository, optional): Repository of the milestone. Defaults to None.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/apps/github/graphql/queries/milestone.py(1 hunks)backend/apps/github/migrations/0027_alter_milestone_table.py(1 hunks)backend/apps/github/models/issue.py(5 hunks)backend/apps/github/models/milestone.py(1 hunks)frontend/src/server/queries/homeQueries.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/apps/github/migrations/0027_alter_milestone_table.py
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/server/queries/homeQueries.ts
- backend/apps/github/models/issue.py
🧰 Additional context used
🧬 Code Graph Analysis (2)
backend/apps/github/graphql/queries/milestone.py (3)
backend/apps/common/graphql/queries.py (1)
BaseQuery(6-7)backend/apps/github/graphql/nodes/milestone.py (1)
MilestoneNode(9-33)backend/apps/github/models/milestone.py (1)
Milestone(10-104)
backend/apps/github/models/milestone.py (4)
backend/apps/common/models.py (1)
BulkSaveModel(8-30)backend/apps/github/models/generic_issue_model.py (1)
GenericIssueModel(10-77)backend/apps/github/models/managers/milestone.py (2)
ClosedMilestoneManager(14-19)OpenMilestoneManager(6-11)backend/apps/github/models/common.py (1)
get_node_id(77-79)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (7)
backend/apps/github/graphql/queries/milestone.py (4)
11-22: Well-designed query structure with a single parameterized endpoint.The approach of using a single
milestonesquery field with astateparameter (instead of separate fields for open and closed milestones) is clean and follows GraphQL best practices. This design provides flexibility while maintaining a simple API surface.
47-56: Good handling of state validation.The validation logic for the state parameter is well-implemented. By converting to lowercase first and raising a clear ValidationError with a helpful message for invalid states, you've made the API robust and user-friendly.
57-65: Excellent query optimization with select_related and prefetch_related.The use of both select_related and prefetch_related to optimize database access is a good practice that will help prevent N+1 query issues. This is particularly important for GraphQL endpoints where clients can request nested relationship data.
67-73: Efficient filtering implementation.The filtering logic is clean and efficient, applying filters conditionally based on provided parameters. The slicing at the end ensures the limit is respected regardless of the filters applied.
backend/apps/github/models/milestone.py (3)
13-15: Good use of custom managers.The implementation of custom managers for filtering open and closed milestones promotes code reusability and keeps queries clean throughout the codebase.
57-69: Clean mapping of GitHub API fields to model fields.The field mapping approach is robust and maintainable. Using a dictionary to map model fields to GitHub API fields makes it easier to modify or extend in the future.
95-104: Good implementation of the update_data method.The method efficiently handles both creation and update scenarios, reusing the from_github method for consistency. The optional save parameter provides flexibility for batch operations.
…s to reorder parameters for consistency
|
|
Migrations are merged. Branch is up to date. No conflicts. Let's finish this @arkid15r |
Resolves #1388