Extend user model with total contributions field. #1209
Conversation
Summary by CodeRabbit
WalkthroughThis pull request introduces a new Changes
Assessment against linked issues
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 0
🧹 Nitpick comments (2)
backend/apps/github/models/user.py (2)
85-96: Consider moving contribution calculation to a separate method.The implementation correctly calculates the total contributions by summing the
contributions_countfrom all repository contributions for the user. However, this logic could be better encapsulated in a separate method to improve code organization and maintainability.Consider refactoring the code like this:
- if does_exist: - from apps.github.models.repository_contributor import RepositoryContributor - - contributions_count = ( - RepositoryContributor.objects.by_humans() - .filter(user=user) - .aggregate(total_contributions=Sum("contributions_count"))[ - "total_contributions" - ] - or 0 - ) - user.contributions_count = contributions_count + if does_exist: + user.update_contributions_count()And add this method to the User class:
def update_contributions_count(self): """Update the user's total contributions count.""" from apps.github.models.repository_contributor import RepositoryContributor self.contributions_count = ( RepositoryContributor.objects.by_humans() .filter(user=self) .aggregate(total_contributions=Sum("contributions_count"))[ "total_contributions" ] or 0 )
86-87: Consider moving import to the module level.Importing
RepositoryContributorinside the method prevents potential circular imports but could be moved to the top of the file for better code organization, as it's a standard practice in Python.from django.db import models from django.db.models import Sum from apps.common.models import TimestampedModel from apps.github.constants import GITHUB_GHOST_USER_LOGIN, OWASP_FOUNDATION_LOGIN from apps.github.models.common import GenericUserModel, NodeModel from apps.github.models.mixins.user import UserIndexMixin from apps.github.models.organization import Organization +from apps.github.models.repository_contributor import RepositoryContributor
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/apps/github/graphql/nodes/user.py(3 hunks)backend/apps/github/migrations/0020_user_contributions_count.py(1 hunks)backend/apps/github/models/mixins/user.py(1 hunks)backend/apps/github/models/user.py(3 hunks)frontend/src/api/queries/userQueries.ts(1 hunks)frontend/src/pages/UserDetails.tsx(2 hunks)frontend/src/types/user.ts(3 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
frontend/src/pages/UserDetails.tsx (1)
frontend/src/types/user.ts (1)
user(3-18)
backend/apps/github/models/user.py (2)
backend/apps/github/models/repository_contributor.py (2)
from_github(43-53)RepositoryContributor(12-78)backend/apps/github/models/managers/repository_contributor.py (2)
by_humans(12-16)by_humans(36-38)
backend/apps/github/graphql/nodes/user.py (1)
backend/apps/github/models/mixins/user.py (1)
idx_contributions_count(107-109)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
🔇 Additional comments (15)
frontend/src/api/queries/userQueries.ts (1)
48-48: Good addition to the user query!Adding the
contributionsCountfield to the GraphQL query allows the frontend to display this important statistic for users, enhancing the user profile data shown in the UI.backend/apps/github/migrations/0020_user_contributions_count.py (1)
11-16: Migration looks good for adding the contributions count field.The migration correctly adds an
IntegerFieldwith a default value of 0 and appropriate verbose name. This matches the PR's objective to extend the user model with a total contributions field.frontend/src/pages/UserDetails.tsx (2)
9-9: Good import of the code merge icon for contributions.The
faCodeMergeicon is semantically appropriate for representing user contributions.
218-222: Well-implemented contributions count display.The implementation follows the existing pattern for user stats, correctly using
millifyfor formatting large numbers andpluralizefor grammatical correctness. This enhances the user profile with valuable contribution information.backend/apps/github/models/mixins/user.py (1)
107-109:✅ Verification successful
Performance improvement by directly returning stored value.
This change simplifies the code and improves performance by directly returning the stored
contributions_countfield instead of calculating it on-the-fly with database queries. However, ensure that there's a mechanism elsewhere in the codebase to properly maintain and update this field when a user's contributions change.To verify that the
contributions_countfield is properly updated elsewhere in the codebase, run:
🏁 Script executed:
#!/bin/bash # Search for code that updates the contributions_count field rg -A 2 -B 2 "contributions_count\s*=" --type pyLength of output: 3398
Update Confirmed: Contributions Count Field Maintenance Verified
The code now directly returns the stored
contributions_countvalue, providing a clear performance benefit. Our investigation confirms that the field is properly maintained elsewhere in the codebase. Specifically:
- In backend/apps/github/models/user.py, the
contributions_countfield is updated (e.g., via the assignmentuser.contributions_count = contributions_countfollowed byuser.save()).- Tests in backend/tests/apps/github/models/repository_contributor_test.py verify that the contributions count is correctly maintained.
No further changes are required.
backend/apps/github/models/user.py (4)
4-4: Good addition of Sum import for aggregation.The import of
Sumfromdjango.db.modelsis appropriate for calculating the total contributions across repositories.
26-26: LGTM: New contributions_count field added properly.The
contributions_countfield is correctly defined as anIntegerFieldwith a default value of 0, which is appropriate for tracking user contributions.
73-76: Good explanation through comments.The comments clearly explain why you're creating the
does_existflag, which helps with understanding the implementation approach.
79-79: Properly setting the existence flag.Setting
does_existtoTruewhen a user is found in the database is correct and aligns with the implementation design.frontend/src/types/user.ts (3)
17-17: LGTM: Properly added contributions_count to user type.The
contributions_countproperty is correctly added to theusertype with the appropriate type ofnumber.
61-61: LGTM: Properly added contributions_count to User type.The
contributions_countproperty is correctly added to theUsertype with the appropriate type ofnumber.
82-82: LGTM: Properly added contributionsCount to UserDetailsProps interface.The
contributionsCountproperty is correctly added to theUserDetailsPropsinterface with the appropriate type ofnumber. The naming follows the camelCase convention used for other properties in this interface.backend/apps/github/graphql/nodes/user.py (3)
47-47: LGTM: Properly added contributions_count field to UserNode.The
contributions_countfield is correctly defined as agraphene.Int()type, which is appropriate for exposing the contributions count in the GraphQL schema.
63-63: LGTM: Properly added contributions_count to fields tuple.The
contributions_countfield is correctly added to thefieldstuple in theMetaclass, which makes it available for GraphQL queries.
94-96: LGTM: Proper implementation of resolve_contributions_count.The resolver method correctly returns
self.idx_contributions_count, which (based on the relevant code snippet) directly returns thecontributions_countvalue from the user model.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/tests/apps/github/graphql/nodes/user_test.py(1 hunks)backend/tests/apps/github/models/user_test.py(1 hunks)frontend/__tests__/e2e/pages/UserDetails.spec.ts(1 hunks)frontend/__tests__/unit/data/mockUserDetails.ts(1 hunks)frontend/__tests__/unit/pages/UserDetails.test.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (5)
frontend/__tests__/unit/data/mockUserDetails.ts (1)
15-15: LGTM: Properly added the new contributions count field to the mock data.The addition of
contributionsCount: 100to the mock user data correctly aligns with the new field being added to the user model. This change ensures that tests using this mock data will properly verify the display of contribution counts in the UI.backend/tests/apps/github/graphql/nodes/user_test.py (1)
37-37: LGTM: Test properly updated to include the new field.Adding
"contributions_count"to the expected fields ensures that the test verifies this new field is properly exposed through the GraphQL API. This is an appropriate and necessary test update.frontend/__tests__/e2e/pages/UserDetails.spec.ts (1)
30-30: LGTM: E2E test correctly validates the new contributions count UI element.The added assertion ensures that the text '100 Contributions' is visible on the page, validating that the UI properly displays the user's contribution count. This is a good addition to the test coverage.
frontend/__tests__/unit/pages/UserDetails.test.tsx (1)
176-178: LGTM: Unit test properly verifies the contributions count display.The added assertions correctly validate that the contributions count is displayed in the statistics section of the user details page. This complements the E2E test and ensures proper test coverage for the new feature.
backend/tests/apps/github/models/user_test.py (1)
35-35: Good addition of RepositoryContributor.objects mock.This correctly adds a mock for the RepositoryContributor objects manager, which aligns with the PR's objective of adding contribution tracking functionality to the User model.
| @patch("apps.github.models.user.User.objects.get") | ||
| @patch("apps.github.models.user.User.save") | ||
| def test_update_data(self, mock_save, mock_get): | ||
| def test_update_data(self, mock_save, mock_get, mock_repo_contributor): |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Test should utilize the mock_repo_contributor parameter.
While you've correctly added the parameter corresponding to the new mock, the test doesn't actually use this mock anywhere in its implementation. For proper test coverage of the new contribution counting functionality, you should:
- Set up appropriate return values for the mock
- Add assertions that verify the contributions_count is updated correctly
def test_update_data(self, mock_save, mock_get, mock_repo_contributor):
gh_user_mock = Mock()
gh_user_mock.raw_data = {"node_id": "12345"}
gh_user_mock.bio = "Bio"
gh_user_mock.hireable = True
gh_user_mock.twitter_username = "twitter"
mock_user = Mock(spec=User)
mock_user.node_id = "12345"
mock_get.return_value = mock_user
+ # Set up mock for repository contributor
+ mock_repo_contributor.filter.return_value.count.return_value = 5
updated_user = User.update_data(gh_user_mock, save=True)
mock_get.assert_called_once_with(node_id="12345")
assert updated_user.node_id == "12345"
+ # Verify the contributions count is updated
+ mock_repo_contributor.filter.assert_called_once_with(user=mock_user)
+ assert updated_user.contributions_count == 5📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_update_data(self, mock_save, mock_get, mock_repo_contributor): | |
| def test_update_data(self, mock_save, mock_get, mock_repo_contributor): | |
| gh_user_mock = Mock() | |
| gh_user_mock.raw_data = {"node_id": "12345"} | |
| gh_user_mock.bio = "Bio" | |
| gh_user_mock.hireable = True | |
| gh_user_mock.twitter_username = "twitter" | |
| mock_user = Mock(spec=User) | |
| mock_user.node_id = "12345" | |
| mock_get.return_value = mock_user | |
| # Set up mock for repository contributor | |
| mock_repo_contributor.filter.return_value.count.return_value = 5 | |
| updated_user = User.update_data(gh_user_mock, save=True) | |
| mock_get.assert_called_once_with(node_id="12345") | |
| assert updated_user.node_id == "12345" | |
| # Verify the contributions count is updated | |
| mock_repo_contributor.filter.assert_called_once_with(user=mock_user) | |
| assert updated_user.contributions_count == 5 |
|
Hello @arkid15r . It's been a week :). Could you please review it? |
I couldn't merge the migrations because of the permission error as I told you. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/__tests__/unit/data/mockUserDetails.ts(1 hunks)frontend/__tests__/unit/pages/UserDetails.test.tsx(1 hunks)frontend/src/app/community/users/[userKey]/page.tsx(2 hunks)frontend/src/server/queries/userQueries.ts(1 hunks)frontend/src/types/user.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/types/user.ts
- frontend/tests/unit/pages/UserDetails.test.tsx
- frontend/tests/unit/data/mockUserDetails.ts
🧰 Additional context used
🧬 Code Definitions (1)
frontend/src/app/community/users/[userKey]/page.tsx (1)
frontend/src/types/user.ts (1)
user(3-18)
🪛 GitHub Actions: Run CI/CD
frontend/src/app/community/users/[userKey]/page.tsx
[error] 203-203: Unstaged changes detected. Run make check and use git add to address it.
🔇 Additional comments (3)
frontend/src/server/queries/userQueries.ts (1)
49-49: Addition of contributionsCount field looks good!The field is properly placed within the user query section and will enable the frontend to display the user's total contribution count. This aligns with the PR objective of extending the user model with a total contributions field.
frontend/src/app/community/users/[userKey]/page.tsx (2)
9-9: Import of faCodeMerge icon is appropriate.The icon choice is well-suited for representing user contributions in the UI.
195-207: Implementation of the contributions count in userStats is well done.The implementation correctly follows the established pattern of other user statistics, displaying the count with an appropriate icon and label. The optional chaining with fallback to 0 ensures the UI handles cases where the data might be undefined.
🧰 Tools
🪛 GitHub Actions: Run CI/CD
[error] 203-203: Unstaged changes detected. Run
make checkand usegit addto address it.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/apps/github/migrations/0021_user_contributions_count.py (2)
9-9: Consider using double quotes for consistency.The static analysis tool suggests using double quotes instead of single quotes to maintain consistency with the codebase style guide.
- ('github', '0020_repositorycontributor_user_contrib_idx'), + ("github", "0020_repositorycontributor_user_contrib_idx"),🧰 Tools
🪛 Ruff (0.8.2)
9-9: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
9-9: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
14-16: Consider using double quotes for string literals.For consistency with the codebase style guide, consider using double quotes for these string literals.
- model_name='user', - name='contributions_count', - field=models.IntegerField(default=0, verbose_name='Contributions count'), + model_name="user", + name="contributions_count", + field=models.IntegerField(default=0, verbose_name="Contributions count"),🧰 Tools
🪛 Ruff (0.8.2)
14-14: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
15-15: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
16-16: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/apps/github/migrations/0021_user_contributions_count.py(1 hunks)frontend/src/app/community/users/[userKey]/page.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/app/community/users/[userKey]/page.tsx
🧰 Additional context used
🪛 Ruff (0.8.2)
backend/apps/github/migrations/0021_user_contributions_count.py
9-9: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
9-9: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
14-14: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
15-15: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
16-16: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (1)
backend/apps/github/migrations/0021_user_contributions_count.py (1)
1-18: LGTM! Migration adds contributions_count field to User model.The migration adds an IntegerField with default value 0 to track user contributions, which aligns with the PR objective of extending the user model with a total contributions field.
🧰 Tools
🪛 Ruff (0.8.2)
9-9: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
9-9: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
14-14: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
15-15: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
16-16: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
backend/apps/github/migrations/0021_user_contributions_count.py
Outdated
Show resolved
Hide resolved
Problem addressed! |
|
Resolves #1192