Migrate User fields to MemberProfile (PR 1: Schema & Sync) #2686
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds three fields (has_public_member_page, is_owasp_staff, contributions_count) to owasp.MemberProfile with migrations and a data-copy migration; updates model API (bulk_save), admin list/fieldsets, GraphQL resolvers to prefer owasp_profile values, permission checks to be null-safe, management commands to use/bulk-save MemberProfile, and adjusts tests accordingly. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
Tip ✨ Issue Enrichment is now available for GitHub issues!CodeRabbit can now help you manage issues more effectively:
Disable automatic issue enrichmentTo disable automatic issue enrichment, add the following to your issue_enrichment:
auto_enrich:
enabled: falseThanks 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 |
|
@arkid15r Should I raise a second PR with the tests and field removal now, or wait for this one to be reviewed first? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/tests/apps/github/management/commands/github_update_users_test.py (1)
33-82: Add assertions to verify MemberProfile synchronization behavior.The test mocks
MemberProfilebut doesn't verify that:
MemberProfile.objects.get_or_createwas called with correct arguments- Profile fields (
contributions_count,is_owasp_staff,has_public_member_page) were set correctlyMemberProfile.bulk_savewas called with the correct field listThis means the MemberProfile synchronization logic introduced in the command isn't actually being tested.
Add assertions after line 81 to verify MemberProfile behavior:
# Verify MemberProfile synchronization assert mock_member_profile.objects.get_or_create.call_count == 3 mock_member_profile.objects.get_or_create.assert_any_call(github_user_id=1) mock_member_profile.objects.get_or_create.assert_any_call(github_user_id=2) mock_member_profile.objects.get_or_create.assert_any_call(github_user_id=3) # Verify MemberProfile.bulk_save was called assert mock_member_profile.bulk_save.call_count == 2 # Verify bulk_save was called with correct fields call_args = mock_member_profile.bulk_save.call_args_list[-1] assert call_args[1]['fields'] == ('contributions_count', 'is_owasp_staff', 'has_public_member_page')Apply similar assertions to the other test methods (lines 87-124, 130-160, 166-194, 200-222, 228-261).
🧹 Nitpick comments (3)
backend/apps/owasp/admin/member_profile.py (1)
24-29: Consider addingis_owasp_stafftolist_filter.Since
is_owasp_staffis a boolean flag that distinguishes a specific user category, adding it tolist_filterwould allow admins to quickly filter and view only staff members, similar to the existing filters for board members, former staff, and GSoC mentors.list_filter = ( "is_owasp_board_member", + "is_owasp_staff", "is_former_owasp_staff", "is_gsoc_mentor", "nest_created_at", )backend/apps/owasp/models/member_profile.py (2)
74-74: Addverbose_nameandhelp_texttohas_public_member_page.For consistency with the other fields in this model (
is_owasp_staff,contributions_count, and existing fields likeis_owasp_board_member),has_public_member_pageshould includeverbose_nameandhelp_textattributes to improve clarity in the admin interface and API documentation.- has_public_member_page = models.BooleanField(default=True) + has_public_member_page = models.BooleanField( + default=True, + verbose_name="Has Public Member Page", + help_text="Indicates if the member has a public profile page on the OWASP website.", + )
83-83: Remove extra blank line for style consistency.There's an extra blank line at line 83 that's inconsistent with the code style used elsewhere in the model.
contributions_count = models.PositiveIntegerField( verbose_name="Contributions count", default=0 ) - - + def __str__(self) -> str:
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/apps/github/management/commands/github_update_users.py(2 hunks)backend/apps/owasp/admin/member_profile.py(1 hunks)backend/apps/owasp/migrations/0066_memberprofile_contributions_count_and_more.py(1 hunks)backend/apps/owasp/migrations/0067_memberprofile_backward_compatibility.py(1 hunks)backend/apps/owasp/models/member_profile.py(2 hunks)backend/tests/apps/github/management/commands/github_update_users_test.py(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2223
File: backend/apps/owasp/models/entity_member.py:50-56
Timestamp: 2025-09-06T19:28:14.297Z
Learning: In the OWASP/Nest project, when migrating scraper logic to GitHub .md file parsing, the sync_leaders method uses member_name as the primary identifier for finding and updating existing EntityMember records, not member_email. This approach is chosen because names are more stable identifiers in markdown files, while emails might be added/updated over time.
🧬 Code graph analysis (5)
backend/apps/owasp/models/member_profile.py (4)
backend/apps/common/models.py (2)
BulkSaveModel(10-34)TimestampedModel(37-46)backend/apps/nest/api/internal/nodes/user.py (1)
is_owasp_staff(14-16)backend/apps/github/models/user.py (1)
bulk_save(167-169)backend/apps/github/models/commit.py (1)
bulk_save(67-75)
backend/apps/github/management/commands/github_update_users.py (2)
backend/apps/owasp/models/member_profile.py (2)
MemberProfile(12-92)bulk_save(90-92)backend/apps/github/models/user.py (2)
User(28-216)bulk_save(167-169)
backend/tests/apps/github/management/commands/github_update_users_test.py (1)
backend/tests/apps/github/management/commands/github_sync_user_test.py (1)
mock_member_profile(84-86)
backend/apps/owasp/migrations/0066_memberprofile_contributions_count_and_more.py (1)
backend/apps/owasp/migrations/0067_memberprofile_backward_compatibility.py (1)
Migration(15-24)
backend/apps/owasp/migrations/0067_memberprofile_backward_compatibility.py (2)
backend/apps/owasp/models/member_profile.py (1)
MemberProfile(12-92)backend/apps/owasp/migrations/0066_memberprofile_contributions_count_and_more.py (1)
Migration(6-28)
🪛 Ruff (0.14.5)
backend/apps/owasp/migrations/0067_memberprofile_backward_compatibility.py
5-5: Missing docstring in public function
(D103)
5-5: Unused function argument: schema_editor
(ARG001)
🔇 Additional comments (4)
backend/apps/github/management/commands/github_update_users.py (2)
58-64: LGTM! MemberProfile synchronization logic is correct.The implementation properly creates or retrieves a MemberProfile for each user and synchronizes the migrated fields. The conditional assignment of
github_useron line 59-60 is correct becauseget_or_create(github_user_id=user.id)only sets the foreign key ID when creating, not the actual relationship object.
68-75: Verification confirms code is working as intended—no issues found.The
BulkSaveModel.bulk_saveimplementation inbackend/apps/common/models.py(line 34) does callobjects.clear()after saving. BothUser.bulk_saveandMemberProfile.bulk_savedelegate to this method, ensuring lists are cleared properly after each batch. The original review's assessment is correct.backend/apps/owasp/migrations/0066_memberprofile_contributions_count_and_more.py (1)
13-27: LGTM! Schema migration is correct.The field definitions are consistent with the model and the migration dependencies are properly set.
backend/apps/owasp/models/member_profile.py (1)
89-92: LGTM! Bulk save method correctly delegates to BulkSaveModel.The static method properly delegates to
BulkSaveModel.bulk_savewith the correct model class and parameters, consistent with the pattern used in other models likeUserandCommit.
backend/apps/owasp/migrations/0067_memberprofile_backward_compatibility.py
Outdated
Show resolved
Hide resolved
backend/tests/apps/github/management/commands/github_update_users_test.py
Outdated
Show resolved
Hide resolved
Sorry, this was a mistake on my part. I only ran |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/tests/apps/github/management/commands/github_update_users_test.py (1)
42-81: Add mock attributes and verify MemberProfile operations.The test has two gaps:
- Mock users lack
is_owasp_staffandhas_public_member_pageattributes that the command accesses (see backend/apps/github/management/commands/github_update_users.py lines 54-56).- The test doesn't verify that
MemberProfile.bulk_saveis called with the correct arguments.Apply this diff:
- mock_user1 = MagicMock(id=1, title="User 1", contributions_count=0) - mock_user2 = MagicMock(id=2, title="User 2", contributions_count=0) - mock_user3 = MagicMock(id=3, title="User 3", contributions_count=0) + mock_user1 = MagicMock(id=1, title="User 1", contributions_count=0, + is_owasp_staff=False, has_public_member_page=True) + mock_user2 = MagicMock(id=2, title="User 2", contributions_count=0, + is_owasp_staff=True, has_public_member_page=False) + mock_user3 = MagicMock(id=3, title="User 3", contributions_count=0, + is_owasp_staff=False, has_public_member_page=True)And add this assertion after line 81:
assert mock_member_profile.bulk_save.call_count == 2 mock_member_profile.bulk_save.assert_called_with( mock_member_profile.objects.get_or_create.return_value[0], fields=("contributions_count", "is_owasp_staff", "has_public_member_page"), )
♻️ Duplicate comments (4)
backend/tests/apps/github/management/commands/github_update_users_test.py (4)
92-124: Add mock attributes and verify MemberProfile operations.Same issues as the previous test: mock users need
is_owasp_staffandhas_public_member_pageattributes, and the test should verifyMemberProfile.bulk_saveis called.Apply similar changes as suggested for test_handle_with_default_offset.
135-160: Add mock attributes and verify MemberProfile operations.Same issues: mock users need
is_owasp_staffandhas_public_member_pageattributes, and verifyMemberProfile.bulk_saveis called.
171-194: Add mock attributes and verify MemberProfile operations.Same issues: mock user needs
is_owasp_staffandhas_public_member_pageattributes, and verifyMemberProfile.bulk_saveis called.
233-261: Add mock attributes and verify MemberProfile operations.Same issues: mock users need
is_owasp_staffandhas_public_member_pageattributes, and verifyMemberProfile.bulk_saveis called.
🧹 Nitpick comments (2)
backend/apps/owasp/models/member_profile.py (1)
80-82: Consider adding help_text for clarity.While the field name is self-explanatory, adding
help_textwould improve consistency with other fields in the model and clarify what counts as a "contribution."contributions_count = models.PositiveIntegerField( - verbose_name="Contributions count", default=0 + verbose_name="Contributions count", + default=0, + help_text="Total number of contributions to OWASP repositories", )backend/tests/apps/github/management/commands/github_update_users_test.py (1)
263-299: Good test, but enhance bulk_save verification.This test properly addresses the previous review comment about testing the
created=Truebranch. However, the bulk_save assertion should verify the fields parameter to ensure the correct fields are being updated.Enhance the assertion at line 299:
- mock_member_profile.bulk_save.assert_called_once() + mock_member_profile.bulk_save.assert_called_once() + call_args = mock_member_profile.bulk_save.call_args + assert call_args[1]["fields"] == ( + "contributions_count", + "is_owasp_staff", + "has_public_member_page", + )
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/apps/owasp/admin/member_profile.py(2 hunks)backend/apps/owasp/migrations/0066_memberprofile_contributions_count_and_more.py(1 hunks)backend/apps/owasp/migrations/0067_memberprofile_backward_compatibility.py(1 hunks)backend/apps/owasp/models/member_profile.py(2 hunks)backend/tests/apps/github/management/commands/github_update_users_test.py(7 hunks)backend/tests/apps/owasp/admin/member_profile_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/apps/owasp/migrations/0067_memberprofile_backward_compatibility.py
- backend/apps/owasp/admin/member_profile.py
- backend/apps/owasp/migrations/0066_memberprofile_contributions_count_and_more.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2223
File: backend/apps/owasp/models/entity_member.py:50-56
Timestamp: 2025-09-06T19:28:14.297Z
Learning: In the OWASP/Nest project, when migrating scraper logic to GitHub .md file parsing, the sync_leaders method uses member_name as the primary identifier for finding and updating existing EntityMember records, not member_email. This approach is chosen because names are more stable identifiers in markdown files, while emails might be added/updated over time.
🧬 Code graph analysis (2)
backend/apps/owasp/models/member_profile.py (3)
backend/apps/common/models.py (1)
BulkSaveModel(10-34)backend/apps/github/models/user.py (1)
bulk_save(167-169)backend/apps/github/models/commit.py (1)
bulk_save(67-75)
backend/tests/apps/github/management/commands/github_update_users_test.py (2)
backend/apps/github/management/commands/github_update_users.py (2)
Command(16-81)handle(28-81)backend/apps/owasp/models/member_profile.py (1)
bulk_save(89-91)
🔇 Additional comments (3)
backend/tests/apps/owasp/admin/member_profile_test.py (1)
15-17: LGTM!The test correctly reflects the addition of the three migrated fields to the admin list display.
backend/apps/owasp/models/member_profile.py (2)
8-8: LGTM!The import supports the bulk_save delegation pattern, consistent with how User and Commit models handle bulk operations.
88-91: LGTM!The bulk_save method correctly follows the delegation pattern used by User and Commit models, enabling efficient batch operations.
backend/tests/apps/github/management/commands/github_update_users_test.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/apps/owasp/models/member_profile.py (1)
74-86: Consider adding help_text forcontributions_countfor admin clarityThe boolean fields here all provide
verbose_nameandhelp_text, which makes the admin clearer.contributions_countonly has averbose_name; adding a briefhelp_text(e.g., what counts as a “contribution”) would keep this model’s metadata consistent and more self-documenting.backend/tests/apps/github/management/commands/github_update_users_test.py (1)
265-301: New test nicely covers thecreated=TrueMemberProfile branch
test_handle_member_profile_createdverifies the previously untested path where a profile is newly created, including linkinggithub_userand syncingcontributions_count,is_owasp_staff, andhas_public_member_page. As a small optional improvement, you could also patchbuiltins.printhere (for consistency with the other tests) and/or assert the args passed tomock_member_profile.bulk_saveif you want even stricter coverage of the batch-save call.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/apps/owasp/models/member_profile.py(2 hunks)backend/tests/apps/github/management/commands/github_update_users_test.py(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2223
File: backend/apps/owasp/models/entity_member.py:50-56
Timestamp: 2025-09-06T19:28:14.297Z
Learning: In the OWASP/Nest project, when migrating scraper logic to GitHub .md file parsing, the sync_leaders method uses member_name as the primary identifier for finding and updating existing EntityMember records, not member_email. This approach is chosen because names are more stable identifiers in markdown files, while emails might be added/updated over time.
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.
🧬 Code graph analysis (2)
backend/apps/owasp/models/member_profile.py (2)
backend/apps/common/models.py (1)
BulkSaveModel(10-34)backend/apps/github/models/user.py (1)
bulk_save(167-169)
backend/tests/apps/github/management/commands/github_update_users_test.py (3)
backend/apps/owasp/models/member_profile.py (1)
bulk_save(93-95)backend/apps/github/models/repository_contributor.py (1)
bulk_save(77-79)backend/apps/github/management/commands/github_update_users.py (2)
Command(16-81)handle(28-81)
🔇 Additional comments (3)
backend/apps/owasp/models/member_profile.py (1)
8-8: Bulk-save wrapper matches existing pattern and looks correctImporting
BulkSaveModeland delegating viaMemberProfile.bulk_save()is consistent with theUser.bulk_save/RepositoryContributor.bulk_savehelpers and should integrate cleanly with existing batch-save infrastructure. No issues from a correctness or API-usage standpoint.Also applies to: 92-95
backend/tests/apps/github/management/commands/github_update_users_test.py (2)
33-41: MemberProfile mocking correctly wires tests to the new sync pathPatching
MemberProfileat the command module path and standardizingobjects.get_or_create.return_valueacross these tests keeps the existing scenarios working while exercising the new profile-sync behavior. This looks consistent and correct; no functional issues spotted.Also applies to: 83-92, 126-135, 162-171, 196-205, 226-235
221-224: Good addition: assert MemberProfile.bulk_save is called on empty inputThe new assertions mirror the existing
User.bulk_savechecks and ensure the command always callsMemberProfile.bulk_save, even when there are no users. This tightens coverage of the new persistence path without changing behavior.
ahmedxgouda
left a comment
ahmedxgouda
left a comment
There was a problem hiding this comment.
Good job. Left a few comments 👇 👇
There was a problem hiding this comment.
This command was originally made for updating the GitHub.User contributions count. So, I suggest to move this command to Owasp app and only update the contributions count of MemberProfile.
There was a problem hiding this comment.
As you asked, I created a new member profile update in the owasp folder only updating contributions_count,
so far a few changes are still pending. I’ll have this PR ready for review soon. (I have exams going on, so my time is a bit limited, but I’ll do my best to finish this in the next 24 hour)
| user.contributions_count = user_contributions.get(user.id, 0) | ||
| users.append(user) | ||
|
|
||
| profile, created = MemberProfile.objects.get_or_create(github_user_id=user.id) | ||
| if created: | ||
| profile.github_user = user | ||
| profile.contributions_count = user.contributions_count | ||
| profile.is_owasp_staff = user.is_owasp_staff | ||
| profile.has_public_member_page = user.has_public_member_page |
There was a problem hiding this comment.
We only need to update the contribution_count of the MemberProfile. No other fields. Also, you should update the field directly from the calculated value not by accessing the contribution count field of the GitHub.User. You will remove this field from GitHub.User in another PR, right?
| MemberProfile.bulk_save( | ||
| profiles, | ||
| fields=( | ||
| "contributions_count", | ||
| "is_owasp_staff", | ||
| "has_public_member_page", | ||
| ), | ||
| ) | ||
|
|
||
| User.bulk_save(users, fields=("contributions_count",)) | ||
| MemberProfile.bulk_save( | ||
| profiles, | ||
| fields=("contributions_count", "is_owasp_staff", "has_public_member_page"), | ||
| ) |
There was a problem hiding this comment.
Same as above, just update the contributions_count
|
@kart-u Also, I suggest to consider your branch naming approach. The branch name is too long. Although, this comment is not related to your changes and is not required for approval. Just a suggestion :) |
kart-u
deleted the
migrate-non-github-field-from-githubuser-owasp-memberprofile
branch
kart-u
restored the
migrate-non-github-field-from-githubuser-owasp-memberprofile
branch
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/apps/owasp/management/commands/owasp_update_member_profiles.py (2)
3-3: Remove unused logger import and declaration.The logger is imported and declared but never used in this command.
Apply this diff:
-import logging - from django.core.management.base import BaseCommand from django.db.models import Q, Sum from apps.common.models import BATCH_SIZE from apps.github.models.repository_contributor import RepositoryContributor from apps.github.models.user import User from apps.owasp.models.member_profile import MemberProfile -logger = logging.getLogger(__name__) -Also applies to: 13-13
54-58: Consider setting github_user consistently for clarity.The
profile.github_user = userassignment is only set for newly created profiles (line 56). While this doesn't affect correctness since onlycontributions_countis bulk-saved, setting it for all profiles would improve consistency and avoid potential confusion.Apply this diff:
profile, created = MemberProfile.objects.get_or_create(github_user_id=user.id) - if created: - profile.github_user = user + profile.github_user = user profile.contributions_count = user_contributions.get(user.id, 0) profiles.append(profile)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/apps/owasp/management/commands/owasp_update_member_profiles.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2223
File: backend/apps/owasp/models/entity_member.py:50-56
Timestamp: 2025-09-06T19:28:14.297Z
Learning: In the OWASP/Nest project, when migrating scraper logic to GitHub .md file parsing, the sync_leaders method uses member_name as the primary identifier for finding and updating existing EntityMember records, not member_email. This approach is chosen because names are more stable identifiers in markdown files, while emails might be added/updated over time.
🧬 Code graph analysis (1)
backend/apps/owasp/management/commands/owasp_update_member_profiles.py (4)
backend/apps/owasp/models/member_profile.py (1)
MemberProfile(12-95)backend/apps/github/management/commands/github_update_users.py (3)
Command(15-59)add_arguments(18-25)handle(27-59)backend/apps/github/models/user.py (1)
get_non_indexable_logins(172-184)backend/apps/github/models/common.py (1)
title(40-44)
| .values("user_id") | ||
| .annotate(total_contributions=Sum("contributions_count")) | ||
| } | ||
| profiles = [] |
There was a problem hiding this comment.
🧩 Analysis chain
Profiles list should be cleared after each batch save to avoid redundant writes.
The profiles list is never cleared after batch saves, causing the same records to be written multiple times:
- At BATCH_SIZE (e.g., 100): saves 100 profiles
- At 2×BATCH_SIZE (200): saves all 200 profiles (including the first 100 again)
- Final save: saves all accumulated profiles regardless of previous batch saves
While bulk_save should be idempotent, this causes unnecessary database writes and degrades performance.
Apply this diff to clear the list after each batch:
if not len(profiles) % BATCH_SIZE:
MemberProfile.bulk_save(
profiles,
fields=("contributions_count",),
)
+ profiles = []
MemberProfile.bulk_save(
profiles,
fields=("contributions_count",),
)Note: The same pattern exists in github_update_users.py (lines 39-45). Consider fixing both commands or verifying if this pattern is intentional.
Run this script to check if the same pattern exists in the related command:
Also applies to: 60-69
🏁 Script executed:
#!/bin/bash
# Check if github_update_users.py has the same issue with not clearing the list
rg -A2 -B2 'if not len\(users\) % BATCH_SIZE' backend/apps/github/management/commands/github_update_users.pyLength of output: 147
---
🏁 Script executed:
```shell
#!/bin/bash
# Check full batch save pattern in github_update_users.py, including final save after loop
rg -A10 -B5 'if not len\(users\) % BATCH_SIZE' backend/apps/github/management/commands/github_update_users.py
Length of output: 333
🏁 Script executed:
#!/bin/bash
# Look for final save statement after the main loop in github_update_users.py
rg -n 'User\.bulk_save' backend/apps/github/management/commands/github_update_users.pyLength of output: 139
---
🏁 Script executed:
```shell
#!/bin/bash
# Verify the batch save pattern in owasp_update_member_profiles.py at lines 60-69
sed -n '55,70p' backend/apps/owasp/management/commands/owasp_update_member_profiles.py
Length of output: 481
Profiles list should be cleared after each batch save to avoid redundant writes.
The profiles list is never cleared after batch saves, causing the same records to be written multiple times:
- At BATCH_SIZE (e.g., 100): saves 100 profiles
- At 2×BATCH_SIZE (200): saves all 200 profiles (including the first 100 again)
- Final save: saves all accumulated profiles regardless of previous batch saves
While bulk_save should be idempotent, this causes unnecessary database writes and degrades performance.
Apply this diff to clear the list after each batch (lines 60-64):
if not len(profiles) % BATCH_SIZE:
MemberProfile.bulk_save(
profiles,
fields=("contributions_count",),
)
+ profiles = []
MemberProfile.bulk_save(
profiles,
fields=("contributions_count",),
)Note: The same pattern exists in github_update_users.py (lines 57, 59). Consider fixing both commands or verifying if this pattern is intentional.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In backend/apps/owasp/management/commands/owasp_update_member_profiles.py around
line 49, the profiles list is never cleared after each bulk save causing
repeated writes; after each bulk_save(...) call inside the batching loop add a
profiles.clear() (and ensure you also clear it after any final bulk_save) so
that once a batch is persisted the list is emptied before collecting the next
batch; also inspect github_update_users.py (around lines 57–59) and apply the
same change if it uses the same accumulation pattern.
changed backend/apps/github/api/internal/nodes/user.py changed backend/apps/github/api/internal/queries/user.py changed backend/apps/github/management/commands/github_update_users.py changed backend/apps/github/models/mixins/user.py
|
Hi there @kart-u |
|
@ahmedxgouda Yes, most of the work has been done. I have also changed the logic so that the member profile is used for the stated fields. I’m extremely sorry for the delay. Could you please give me one more day so that I can wrap up testing? |
kart-u
force-pushed
the
migrate-non-github-field-from-githubuser-owasp-memberprofile
branch
from
90b9e8a to
e895d6e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
backend/apps/nest/api/internal/nodes/user.py (1)
14-18: Consider using try/except for better performance.The current implementation is functionally correct, but using
hasattr()to check for the reverse OneToOne relation may cause two descriptor evaluations—one in thehasattr()check and another when accessing the field on line 17. This could result in redundant database queries.Apply this diff to use a more efficient try/except pattern:
@strawberry_django.field def is_owasp_staff(self) -> bool: """Check if the user is an OWASP staff member.""" - if self.github_user and hasattr(self.github_user, "owasp_profile"): - return self.github_user.owasp_profile.is_owasp_staff - return False + try: + return self.github_user.owasp_profile.is_owasp_staff + except (AttributeError, ObjectDoesNotExist): + return FalseDon't forget to add the import at the top of the file:
+from django.core.exceptions import ObjectDoesNotExist + from apps.nest.models import Userbackend/apps/github/management/commands/github_update_users.py (1)
11-12: Command now updatesMemberProfile.contributions_count; consider clearing batch listSwitching from
UsertoMemberProfileand settingcontributions_countfrom the aggregated contributions dict matches the migration goals and prior review feedback (only this field is updated, and it’s derived fromRepositoryContributor).One small improvement:
profileskeeps growing and everybulk_savere-sends all accumulated profiles, then the final call re-sends them again. That’s correct but does extra work and uses more memory than necessary. You can clear the list after each flush:- profiles = [] + profiles = [] for idx, user in enumerate(active_users[offset:]): @@ - profiles.append(profile) - - if not len(profiles) % BATCH_SIZE: - MemberProfile.bulk_save( - profiles, - fields=("contributions_count",), - ) - - MemberProfile.bulk_save( - profiles, - fields=("contributions_count",), - ) + profiles.append(profile) + + if len(profiles) >= BATCH_SIZE: + MemberProfile.bulk_save( + profiles, + fields=("contributions_count",), + ) + profiles.clear() + + if profiles: + MemberProfile.bulk_save( + profiles, + fields=("contributions_count",), + )Also applies to: 49-69
backend/apps/github/api/internal/nodes/user.py (1)
29-35: Use safer access pattern forowasp_profileto avoid potentialDoesNotExisterrorsBoth
contributions_countandis_owasp_staffusehasattr(self, "owasp_profile")and then accessself.owasp_profile.... With Django one-to-one relations this can still raiseDoesNotExistif the relatedMemberProfileis missing, even though the descriptor exists on the model.Given this is a resolver that should fail soft, consider a safer pattern that:
- avoids exceptions when a profile is missing
- keeps the same defaults (0 / False)
- is consistent between the two fields
For example:
@strawberry.field def contributions_count(self) -> int: """Resolve contributions count.""" - if hasattr(self, "owasp_profile"): - return self.owasp_profile.contributions_count - return 0 + profile = getattr(self, "owasp_profile", None) + return getattr(profile, "contributions_count", 0) @@ @strawberry.field def is_owasp_staff(self) -> bool: """Resolve if the user is an OWASP staff member.""" - if hasattr(self, "owasp_profile"): - return self.owasp_profile.is_owasp_staff - return False + profile = getattr(self, "owasp_profile", None) + return bool(getattr(profile, "is_owasp_staff", False))You could optionally apply the same pattern to other
owasp_profile-based resolvers in this node in a follow-up for consistency.Also applies to: 91-97
backend/tests/apps/github/management/commands/github_update_users_test.py (2)
204-240: Minor duplication in single-user test setupThe single-user test correctly checks:
- That contributions are written to a single
MemberProfile.- That
bulk_saveis called twice (once inside the loop due toBATCH_SIZE = 1, and once after the loop).There is a tiny redundancy in the queryset setup:
mock_users_queryset.count.return_value = 1 mock_users_queryset.__getitem__.return_value = [mock_user1] mock_users_queryset.count.return_value = 1 mock_users_queryset.__getitem__.return_value = [mock_user1]You can safely drop the second pair of assignments without changing behavior:
- mock_users_queryset.count.return_value = 1 - mock_users_queryset.__getitem__.return_value = [mock_user1] - mock_users_queryset.count.return_value = 1 - mock_users_queryset.__getitem__.return_value = [mock_user1] + mock_users_queryset.count.return_value = 1 + mock_users_queryset.__getitem__.return_value = [mock_user1]Purely cosmetic, non-blocking.
321-352: New MemberProfile-creation test looks good; optional stronger assertionThe new
test_handle_member_profile_creatednicely covers the previously untestedcreated=Truebranch:
- Verifies
profile.github_useris set.- Verifies
profile.contributions_countis set from aggregated data.- Ensures
MemberProfile.bulk_saveis called once.If you’d like to tighten it further, you could also assert the exact call signature:
- mock_member_profile.bulk_save.assert_called_once() + mock_member_profile.bulk_save.assert_called_once_with( + [mock_profile], + fields=("contributions_count",), + )Not required, but it would lock in the expected usage of
bulk_save.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
backend/apps/github/api/internal/nodes/user.py(2 hunks)backend/apps/github/api/internal/queries/user.py(1 hunks)backend/apps/github/management/commands/github_update_users.py(2 hunks)backend/apps/github/models/mixins/user.py(1 hunks)backend/apps/nest/api/internal/nodes/user.py(1 hunks)backend/apps/nest/management/commands/nest_update_badges.py(2 hunks)backend/apps/owasp/api/internal/permissions/project_health_metrics.py(1 hunks)backend/apps/owasp/api/internal/views/permissions.py(1 hunks)backend/apps/owasp/migrations/0068_alter_memberprofile_has_public_member_page.py(1 hunks)backend/tests/apps/github/api/internal/queries/user_test.py(1 hunks)backend/tests/apps/github/management/commands/github_update_users_test.py(7 hunks)backend/tests/apps/nest/management/commands/nest_update_badges_test.py(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2223
File: backend/apps/owasp/models/entity_member.py:50-56
Timestamp: 2025-09-06T19:28:14.297Z
Learning: In the OWASP/Nest project, when migrating scraper logic to GitHub .md file parsing, the sync_leaders method uses member_name as the primary identifier for finding and updating existing EntityMember records, not member_email. This approach is chosen because names are more stable identifiers in markdown files, while emails might be added/updated over time.
📚 Learning: 2025-08-01T04:15:32.151Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1823
File: frontend/__tests__/e2e/pages/Login.spec.ts:28-34
Timestamp: 2025-08-01T04:15:32.151Z
Learning: In the OWASP Nest project, the login page (/auth/login) handles only authentication (GitHub OAuth) and does not differentiate between OWASP staff and non-staff users. The role-based access control using the is_owasp_staff field happens after authentication in downstream components like DashboardWrapper and ProjectsWrapper, not during the login process itself.
Applied to files:
backend/apps/nest/api/internal/nodes/user.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/apps/nest/api/internal/nodes/user.py
🧬 Code graph analysis (7)
backend/apps/github/management/commands/github_update_users.py (2)
backend/apps/owasp/models/member_profile.py (2)
MemberProfile(12-95)bulk_save(93-95)backend/apps/github/models/user.py (1)
bulk_save(167-169)
backend/apps/github/models/mixins/user.py (1)
backend/apps/github/api/internal/nodes/user.py (1)
contributions_count(30-34)
backend/tests/apps/github/api/internal/queries/user_test.py (1)
backend/apps/github/api/internal/queries/user.py (2)
UserQuery(12-57)user(40-57)
backend/apps/nest/api/internal/nodes/user.py (2)
backend/apps/owasp/api/internal/nodes/member_snapshot.py (1)
github_user(32-34)backend/apps/github/api/internal/nodes/user.py (1)
is_owasp_staff(92-96)
backend/apps/github/api/internal/nodes/user.py (1)
backend/apps/nest/api/internal/nodes/user.py (1)
is_owasp_staff(14-18)
backend/tests/apps/github/management/commands/github_update_users_test.py (2)
backend/apps/owasp/models/member_profile.py (1)
bulk_save(93-95)backend/apps/github/management/commands/github_update_users.py (1)
handle(28-69)
backend/apps/owasp/api/internal/views/permissions.py (3)
backend/apps/owasp/api/internal/nodes/member_snapshot.py (1)
github_user(32-34)backend/apps/github/api/internal/nodes/user.py (1)
is_owasp_staff(92-96)backend/apps/nest/api/internal/nodes/user.py (1)
is_owasp_staff(14-18)
🔇 Additional comments (11)
backend/apps/github/api/internal/queries/user.py (1)
53-57: User resolver correctly migrated to useMemberProfile.has_public_member_pageUsing
select_related("owasp_profile")and filtering onowasp_profile__has_public_member_page=Truewithloginaligns with the new schema and keeps the query efficient.backend/tests/apps/nest/management/commands/nest_update_badges_test.py (1)
32-47: Tests correctly handle both new and legacyis_owasp_staffkeysThe updated helpers cover Q objects, dicts, and tuples and prefer
owasp_profile__is_owasp_staffwhile still supporting the legacyis_owasp_staffkey, which matches the production filter behavior.Also applies to: 61-62
backend/apps/owasp/migrations/0068_alter_memberprofile_has_public_member_page.py (1)
1-18: Migration 0068 cleanly alignshas_public_member_pagewith the modelDependency chain and field definition (default, verbose_name, help_text) match the
MemberProfilemodel, so this migration cleanly brings the DB schema in sync.backend/apps/nest/management/commands/nest_update_badges.py (1)
44-46: Badge sync correctly migrated toowasp_profile__is_owasp_staffBoth assignment and removal queries now use
owasp_profile__is_owasp_staff, which matches the new data model and the updated tests innest_update_badges_test.py. Behavior is preserved assuming every staff user has a correspondingMemberProfile.Also applies to: 62-64
backend/tests/apps/github/api/internal/queries/user_test.py (1)
17-31: UserQuery tests correctly exercise the newselect_related+ filter chainThe tests now mock
User.objects.select_related("owasp_profile")and assert the expected chained calls (filter(owasp_profile__has_public_member_page=True, login=...).first()), which keeps them aligned with the resolver implementation.Also applies to: 32-46, 47-60, 62-73
backend/apps/owasp/api/internal/permissions/project_health_metrics.py (1)
11-20: I encountered repeated failures attempting to clone and analyze the repository. Without access to the codebase, I cannot verify the specific concerns raised in the review comment through direct examination.Unable to verify the review comment at this time due to repository access failures.
The review comment raises concerns about potential
AttributeErrorexceptions when:
info.context.requestis not available- User lacks a
github_userrelationshipThe proposed solution using
getattr()for defensive access is a standard Python best practice for handling optional object attributes. However, I cannot definitively confirm whether these scenarios actually occur in this specific codebase without access to:
- The actual implementation context
- Related Django model definitions
- Existing test coverage and execution results
- Similar permission implementations for pattern comparison
The review comment should be manually verified by:
- Running the existing test suite to confirm current behavior
- Checking Django model relationships
- Testing edge cases with missing
info.context.requestbackend/tests/apps/github/management/commands/github_update_users_test.py (5)
33-99: Tests correctly exercise MemberProfile batching and contributions mapping (default offset)This test now accurately reflects the new behavior:
MemberProfile.objects.get_or_createreturns per-user profiles.contributions_countis set from the aggregated RepositoryContributor data.MemberProfile.bulk_saveis called twice, with the final call including all profiles[mock_profile1, mock_profile2, mock_profile3], matching the command’s current batching semantics.The setup and assertions look solid and give good confidence in the migration of contributions_count from
UsertoMemberProfile.
100-155: Custom offset scenario is well coveredThe custom-offset test cleanly verifies that:
- Slicing via
offset=1only processes the expected users.- Contributions are mapped to the correct
MemberProfileinstances.bulk_saveis invoked twice, with the last call containing just the two processed profiles.This aligns with the command’s offset handling and batching logic.
156-203: No-contributions scenario is correctly handledThis test confirms that when RepositoryContributor aggregation returns an empty list:
contributions_countremains at the default (0) on both profiles.- A single
MemberProfile.bulk_savecall is made with both profiles.That matches the command’s
dict.get(user.id, 0)behavior and ensures we don’t regress the “no contributions” edge case.
241-268: Empty user list behavior and MemberProfile bulk_save are well assertedThe empty-user-list test now asserts:
- No output is printed.
MemberProfile.bulk_saveis called exactly once with an empty list.This is a good regression guard to keep the command idempotent and predictable when there are no active users.
269-320: Exact batch-size scenario matches current batching semanticsThis test ensures that when the number of users equals
BATCH_SIZE:
- Each user’s contributions are written to the correct
MemberProfile.bulk_saveis invoked twice, and the final call includes both profiles.This mirrors the command’s current implementation where the final
bulk_savere-sends the full list, which is what your assertions encode. If you later refactor the command to clear the list after each batch, this test will correctly force you to update expectations.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/apps/owasp/api/internal/views/permissions.py (1)
14-17: Consider catchingAttributeErrorfor extra defense.While
ObjectDoesNotExistcorrectly handles missing related objects, catchingAttributeErroras well would guard against edge cases where thegithub_userattribute doesn't exist on the model at all.try: github_user = user.github_user - except ObjectDoesNotExist: + except (AttributeError, ObjectDoesNotExist): return FalseThis is defensive coding—the current implementation is sufficient for normal operation.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/apps/github/models/mixins/user.py(1 hunks)backend/apps/owasp/api/internal/views/permissions.py(1 hunks)backend/apps/owasp/migrations/0068_alter_memberprofile_has_public_member_page.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/apps/github/models/mixins/user.py
- backend/apps/owasp/migrations/0068_alter_memberprofile_has_public_member_page.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2223
File: backend/apps/owasp/models/entity_member.py:50-56
Timestamp: 2025-09-06T19:28:14.297Z
Learning: In the OWASP/Nest project, when migrating scraper logic to GitHub .md file parsing, the sync_leaders method uses member_name as the primary identifier for finding and updating existing EntityMember records, not member_email. This approach is chosen because names are more stable identifiers in markdown files, while emails might be added/updated over time.
📚 Learning: 2025-07-31T07:05:25.056Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 0
File: :0-0
Timestamp: 2025-07-31T07:05:25.056Z
Learning: In the OWASP Nest project, Django views may not properly access authenticated users from sessions created by Strawberry GraphQL mutations. The issue occurs because Django's AuthenticationMiddleware doesn't always populate request.user from session data that GraphQL context successfully uses via info.context.request.user. The solution is to manually resolve users from session data using request.session.get('_auth_user_id') and User.objects.select_related('github_user').get(pk=user_id) to match the same authentication mechanism used by GraphQL context.
Applied to files:
backend/apps/owasp/api/internal/views/permissions.py
🧬 Code graph analysis (1)
backend/apps/owasp/api/internal/views/permissions.py (2)
backend/apps/owasp/api/internal/nodes/member_snapshot.py (1)
github_user(32-34)frontend/src/app/api/auth/[...nextauth]/route.ts (1)
profile(46-54)
🔇 Additional comments (2)
backend/apps/owasp/api/internal/views/permissions.py (2)
5-5: LGTM - Correct import for defensive relation access.The
ObjectDoesNotExistimport is necessary and correct for the try/except blocks that safely navigate OneToOne relations.
9-24: Excellent migration to MemberProfile-based staff check.The refactored
has_dashboard_permissionsuccessfully migrates theis_owasp_staffcheck fromgithub_usertoowasp_profile(MemberProfile), aligning perfectly with the PR objectives. The defensive navigation pattern usingtry/exceptblocks for relation access is the correct approach and addresses the concerns raised in the previous bot review.The implementation correctly:
- Uses
getattrfor safe attribute access- Employs
try/except ObjectDoesNotExistblocks for OneToOne relations (the recommended Django pattern)- Verifies the entire chain:
request→user→github_user→owasp_profile- Safely extracts
is_owasp_stafffrom MemberProfileBased on learnings from previous reviews regarding Django OneToOne relation safety.
ahmedxgouda
left a comment
ahmedxgouda
left a comment
There was a problem hiding this comment.
Good job. Please ensure backward compatibility in this PR. In the next PR you will remove the User old fields and update the code to not use them.
| @strawberry.field | ||
| def contributions_count(self) -> int: | ||
| """Resolve contributions count.""" | ||
| if hasattr(self, "owasp_profile"): | ||
| return self.owasp_profile.contributions_count | ||
| return 0 |
There was a problem hiding this comment.
You should ensure backward compatibility. You should return the User old field contribution_count if you found that contribution_count of owasp_profile is the default value i.e. 0.
| @strawberry.field | ||
| def is_owasp_staff(self) -> bool: | ||
| """Resolve if the user is an OWASP staff member.""" | ||
| if hasattr(self, "owasp_profile"): | ||
| return self.owasp_profile.is_owasp_staff | ||
| return False | ||
|
|
There was a problem hiding this comment.
Same as above. Return the old field.
| return ( | ||
| User.objects.select_related("owasp_profile") | ||
| .filter(owasp_profile__has_public_member_page=True, login=login) | ||
| .first() | ||
| ) |
| user.contributions_count = user_contributions.get(user.id, 0) | ||
| users.append(user) | ||
|
|
||
| profile, created = MemberProfile.objects.get_or_create(github_user_id=user.id) |
There was a problem hiding this comment.
Keep the update of the user field for now. In a separate PR you should remove it.
| if not hasattr(self, "owasp_profile"): | ||
| return 0 | ||
| return int(self.owasp_profile.contributions_count) |
There was a problem hiding this comment.
Same as above. The old value.
| owasp_profile__is_owasp_staff=False, | ||
| user_badges__badge=badge, | ||
| ).distinct() | ||
| removed_count = non_employees.count() |
| return ( | ||
| (user := info.context.request.user) | ||
| and user.is_authenticated | ||
| and user.github_user.is_owasp_staff | ||
| hasattr(user.github_user, "owasp_profile") | ||
| and user.github_user.owasp_profile.is_owasp_staff |
| user = getattr(request, "user", None) | ||
| if not (user and getattr(user, "is_authenticated", False)): | ||
| return False | ||
| try: | ||
| github_user = user.github_user | ||
| except ObjectDoesNotExist: | ||
| return False | ||
|
|
||
| try: | ||
| profile = github_user.owasp_profile | ||
| except ObjectDoesNotExist: | ||
| return False | ||
|
|
||
| return bool(getattr(profile, "is_owasp_staff", False)) |
There was a problem hiding this comment.
These try statements are not necessary. If there is no github_user, the field will exist but it will be None. Same as owasp_profile. So, I suggest to make it similar to the original code, but add the new statement that will check for is_owasp_staff of owasp_profile. Also, make sure to add backward compatibility (see the above suggestions).
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
backend/tests/apps/github/api/internal/queries/user_test.py (1)
50-57: Consider adding.first()assertion for consistency.This test omits the
.first()assertion that other tests include. While it's testing filter arguments specifically, adding the assertion would maintain consistency.UserQuery().user(login="test-user") mock_filter.assert_called_once_with(has_public_member_page=True, login="test-user") + mock_filter.return_value.first.assert_called_once()backend/apps/github/management/commands/github_update_users.py (1)
55-57: Remove redundant assignment afterget_or_create.When
get_or_create(github_user=user)creates a new record,github_useris already set. The assignment on line 57 is unnecessary.profile, created = MemberProfile.objects.get_or_create(github_user=user) - if created: - profile.github_user = user contributions = user_contributions.get(user.id, 0)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/apps/github/api/internal/nodes/user.py(2 hunks)backend/apps/github/management/commands/github_update_users.py(2 hunks)backend/apps/github/models/mixins/user.py(1 hunks)backend/apps/nest/api/internal/nodes/user.py(1 hunks)backend/apps/owasp/api/internal/permissions/project_health_metrics.py(1 hunks)backend/apps/owasp/api/internal/views/permissions.py(1 hunks)backend/tests/apps/github/api/internal/queries/user_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/apps/owasp/api/internal/permissions/project_health_metrics.py
- backend/apps/nest/api/internal/nodes/user.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2223
File: backend/apps/owasp/models/entity_member.py:50-56
Timestamp: 2025-09-06T19:28:14.297Z
Learning: In the OWASP/Nest project, when migrating scraper logic to GitHub .md file parsing, the sync_leaders method uses member_name as the primary identifier for finding and updating existing EntityMember records, not member_email. This approach is chosen because names are more stable identifiers in markdown files, while emails might be added/updated over time.
📚 Learning: 2025-07-31T07:05:25.056Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 0
File: :0-0
Timestamp: 2025-07-31T07:05:25.056Z
Learning: In the OWASP Nest project, Django views may not properly access authenticated users from sessions created by Strawberry GraphQL mutations. The issue occurs because Django's AuthenticationMiddleware doesn't always populate request.user from session data that GraphQL context successfully uses via info.context.request.user. The solution is to manually resolve users from session data using request.session.get('_auth_user_id') and User.objects.select_related('github_user').get(pk=user_id) to match the same authentication mechanism used by GraphQL context.
Applied to files:
backend/apps/owasp/api/internal/views/permissions.py
🧬 Code graph analysis (5)
backend/apps/owasp/api/internal/views/permissions.py (4)
backend/apps/github/api/internal/queries/user.py (1)
user(40-53)backend/apps/owasp/api/internal/nodes/member_snapshot.py (1)
github_user(32-34)backend/apps/github/api/internal/nodes/user.py (1)
is_owasp_staff(92-96)backend/apps/nest/api/internal/nodes/user.py (1)
is_owasp_staff(14-18)
backend/apps/github/models/mixins/user.py (1)
backend/apps/github/api/internal/nodes/user.py (1)
contributions_count(30-34)
backend/apps/github/api/internal/nodes/user.py (1)
backend/apps/nest/api/internal/nodes/user.py (1)
is_owasp_staff(14-18)
backend/apps/github/management/commands/github_update_users.py (3)
backend/apps/owasp/models/member_profile.py (2)
MemberProfile(12-95)bulk_save(93-95)backend/apps/github/api/internal/nodes/user.py (1)
contributions_count(30-34)backend/apps/github/models/user.py (2)
bulk_save(167-169)User(28-216)
backend/tests/apps/github/api/internal/queries/user_test.py (1)
backend/apps/github/api/internal/queries/user.py (1)
user(40-53)
🔇 Additional comments (3)
backend/apps/github/models/mixins/user.py (1)
124-129: LGTM!The guarded access with
hasattr()and fallback to the originalcontributions_countfield ensures backward compatibility. The truthiness check oncontributions_countcorrectly falls back to the legacy field when the profile value is0(default), which aligns with the reviewer's past feedback on backward compatibility.backend/tests/apps/github/api/internal/queries/user_test.py (1)
17-48: LGTM!The test refactoring simplifies the mock setup by directly chaining
mock_filter.return_value.first.return_value, which is cleaner and more accurately reflects the actual query patternUser.objects.filter(...).first(). The assertions properly verify both the filter parameters and the.first()call.backend/apps/owasp/api/internal/views/permissions.py (1)
8-18: LGTM with a minor observation.The permission check correctly prioritizes
owasp_profile.is_owasp_staffwith a fallback togithub_user.is_owasp_stafffor backward compatibility. This aligns with the pattern used inbackend/apps/nest/api/internal/nodes/user.py(lines 13-17) andbackend/apps/github/api/internal/nodes/user.py(lines 91-96).The
hasattr(user, "github_user")check on line 11 is technically redundant sinceuser.github_userwill beNone(not raise) if the relation doesn't exist, and the subsequent truthy check handles that. However, this was addressed in past reviews and the explicit check adds clarity.
| if not len(profiles) % BATCH_SIZE: | ||
| MemberProfile.bulk_save( | ||
| profiles, | ||
| fields=("contributions_count",), | ||
| ) | ||
| User.bulk_save( | ||
| users, | ||
| fields=("contributions_count",), | ||
| ) | ||
|
|
||
| User.bulk_save(users, fields=("contributions_count",)) | ||
| MemberProfile.bulk_save( | ||
| profiles, | ||
| fields=("contributions_count",), | ||
| ) | ||
| User.bulk_save( | ||
| users, | ||
| fields=("contributions_count",), | ||
| ) |
There was a problem hiding this comment.
Batch logic causes duplicate saves and accumulates memory.
Two issues with the current batching approach:
-
Duplicate saves: The batch save inside the loop (lines 66-73) saves all accumulated items, not just the current batch. Then the final save (lines 75-82) re-saves everything again.
-
Memory: Lists grow unbounded; only final batch should be saved at the end.
Apply this diff to fix the batching:
- if not len(profiles) % BATCH_SIZE:
+ if len(profiles) >= BATCH_SIZE:
MemberProfile.bulk_save(
profiles,
fields=("contributions_count",),
)
User.bulk_save(
users,
fields=("contributions_count",),
)
+ profiles = []
+ users = []
- MemberProfile.bulk_save(
- profiles,
- fields=("contributions_count",),
- )
- User.bulk_save(
- users,
- fields=("contributions_count",),
- )
+ if profiles:
+ MemberProfile.bulk_save(
+ profiles,
+ fields=("contributions_count",),
+ )
+ if users:
+ User.bulk_save(
+ users,
+ fields=("contributions_count",),
+ )This clears the lists after each batch save to prevent duplicate saves and control memory usage, and only saves remaining items if any exist.
📝 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.
| if not len(profiles) % BATCH_SIZE: | |
| MemberProfile.bulk_save( | |
| profiles, | |
| fields=("contributions_count",), | |
| ) | |
| User.bulk_save( | |
| users, | |
| fields=("contributions_count",), | |
| ) | |
| User.bulk_save(users, fields=("contributions_count",)) | |
| MemberProfile.bulk_save( | |
| profiles, | |
| fields=("contributions_count",), | |
| ) | |
| User.bulk_save( | |
| users, | |
| fields=("contributions_count",), | |
| ) | |
| if len(profiles) >= BATCH_SIZE: | |
| MemberProfile.bulk_save( | |
| profiles, | |
| fields=("contributions_count",), | |
| ) | |
| User.bulk_save( | |
| users, | |
| fields=("contributions_count",), | |
| ) | |
| profiles = [] | |
| users = [] | |
| if profiles: | |
| MemberProfile.bulk_save( | |
| profiles, | |
| fields=("contributions_count",), | |
| ) | |
| if users: | |
| User.bulk_save( | |
| users, | |
| fields=("contributions_count",), | |
| ) |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
backend/apps/github/api/internal/nodes/user.py (1)
29-35: Consider fallback when owasp_profile has default value (0).The resolver returns
owasp_profile.contributions_countwheneverowasp_profileexists, even if the value is 0 (the default). During migration, if the data sync hasn't run yet, users with existing contributions on the User model would show 0.A previous review suggested falling back to the User field when
owasp_profile.contributions_countis the default value:def _resolve_contributions_count(self) -> int: """Resolve contributions count.""" - if hasattr(self, "owasp_profile"): + if hasattr(self, "owasp_profile") and self.owasp_profile.contributions_count: return self.owasp_profile.contributions_count return super().__getattribute__("contributions_count")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/apps/github/api/internal/nodes/user.py(2 hunks)backend/apps/github/api/internal/queries/user.py(1 hunks)backend/apps/github/management/commands/github_update_users.py(2 hunks)backend/apps/nest/management/commands/nest_update_badges.py(2 hunks)backend/tests/apps/github/api/internal/queries/user_test.py(1 hunks)backend/tests/apps/github/management/commands/github_update_users_test.py(7 hunks)backend/tests/apps/nest/management/commands/nest_update_badges_test.py(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/tests/apps/nest/management/commands/nest_update_badges_test.py
- backend/apps/github/management/commands/github_update_users.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2223
File: backend/apps/owasp/models/entity_member.py:50-56
Timestamp: 2025-09-06T19:28:14.297Z
Learning: In the OWASP/Nest project, when migrating scraper logic to GitHub .md file parsing, the sync_leaders method uses member_name as the primary identifier for finding and updating existing EntityMember records, not member_email. This approach is chosen because names are more stable identifiers in markdown files, while emails might be added/updated over time.
📚 Learning: 2025-07-31T07:05:25.056Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 0
File: :0-0
Timestamp: 2025-07-31T07:05:25.056Z
Learning: In the OWASP Nest project, Django views may not properly access authenticated users from sessions created by Strawberry GraphQL mutations. The issue occurs because Django's AuthenticationMiddleware doesn't always populate request.user from session data that GraphQL context successfully uses via info.context.request.user. The solution is to manually resolve users from session data using request.session.get('_auth_user_id') and User.objects.select_related('github_user').get(pk=user_id) to match the same authentication mechanism used by GraphQL context.
Applied to files:
backend/apps/github/api/internal/nodes/user.py
🧬 Code graph analysis (3)
backend/apps/nest/management/commands/nest_update_badges.py (2)
backend/apps/github/models/user.py (1)
User(28-216)backend/apps/nest/api/internal/nodes/user.py (1)
is_owasp_staff(14-18)
backend/tests/apps/github/api/internal/queries/user_test.py (1)
backend/apps/github/api/internal/queries/user.py (2)
UserQuery(12-62)user(40-62)
backend/apps/github/api/internal/nodes/user.py (1)
backend/apps/nest/api/internal/nodes/user.py (1)
is_owasp_staff(14-18)
🔇 Additional comments (4)
backend/apps/github/api/internal/queries/user.py (1)
53-62: Verify intended precedence behavior for has_public_member_page.The fallback query runs whenever the first query returns no result. This means if a user has
owasp_profile.has_public_member_page=False(intentionally hiding their page), butUser.has_public_member_page=True(legacy value), the user will still be returned via the fallback.Is this the intended behavior during migration, or should
owasp_profiletake strict precedence when present?If
owasp_profileshould take precedence when it exists:user = ( User.objects.select_related("owasp_profile") - .filter(owasp_profile__has_public_member_page=True, login=login) + .filter(login=login) .first() ) - if user: + if user and hasattr(user, "owasp_profile") and user.owasp_profile: + if user.owasp_profile.has_public_member_page: + return user + return None # owasp_profile exists and says no + elif user and user.has_public_member_page: return user - return User.objects.filter(has_public_member_page=True, login=login).first() + return Nonebackend/tests/apps/github/api/internal/queries/user_test.py (1)
17-62: LGTM!The tests correctly cover all three code paths for the two-stage user resolution: found on first query (no fallback), found on second query (fallback invoked), and not found (both queries attempted). Mock chaining is properly structured.
backend/apps/github/api/internal/nodes/user.py (1)
92-98: Resolver pattern looks correct; verify boolean precedence is intentional.The pattern correctly avoids recursion by using
super().__getattribute__(). Note that whenowasp_profileexists, its value takes precedence over the User model, meaning a user withowasp_profile.is_owasp_staff=FalsebutUser.is_owasp_staff=Truewill resolve toFalse. Ensure this is the intended behavior during migration.backend/tests/apps/github/management/commands/github_update_users_test.py (1)
321-352: Good addition of test coverage for MemberProfile creation.This test properly exercises the
created=Truebranch where a new MemberProfile is created and linked to the GitHub user. The assertions correctly verify thatgithub_useris set andcontributions_countis propagated.
backend/tests/apps/github/management/commands/github_update_users_test.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
backend/apps/nest/management/commands/nest_update_badges.py (1)
44-51: Composite staff filter fixes the migration fallback gapSwitching to
Q(owasp_profile__is_owasp_staff=True) | Q(is_owasp_staff=True, owasp_profile__isnull=True)correctly:
- Prefers
owasp_profile.is_owasp_staffwhen a profile exists.- Still includes legacy
is_owasp_staff=Trueusers who don’t yet have anowasp_profile.
This removes the earlier “only if count is zero” fallback bug and aligns with the staged migration semantics.One optional improvement: because of the join introduced by
.exclude(user_badges__badge=badge), you may wantdistinct()onemployees_without_badgeto avoid duplicateUserrows when a user has multiple other badges (mostly a perf/readability tweak, not correctness).backend/tests/apps/nest/management/commands/nest_update_badges_test.py (4)
34-46: Dual-key handling inextract_is_owasp_staffis consistent and clearThe helper correctly supports both the new
"owasp_profile__is_owasp_staff"and legacy"is_owasp_staff"keys across objects-with-children, dicts, and 2-tuples, which aligns well with the staged migration strategy. If you later end up dealing with more complex nested structures, a small optional enhancement would be to make thechildrenscan recursive instead of single-level.
60-69:user_filter_side_effect_factorycorrectly routes both new and legacy flagsUsing kwargs lookup for
"owasp_profile__is_owasp_staff"with a fallback to"is_owasp_staff", plus the argument scan viaextract_is_owasp_staff, gives good coverage for both code paths while keeping the side-effect logic simple. To avoid string drift between this factory andextract_is_owasp_staff, you might optionally centralize the key names (e.g., module-level constants) so future field renames only need to be updated in one place.
126-168:test_badge_creationaccurately models the “no staff users” scenarioThe decorator ordering and argument list for the three patches are correct, and the detailed
mock_empty_queryset(withcount,exclude,distinct,values_list) mirrors queryset behavior closely enough for this command. Asserting thatUserBadge.objects.filteris never called is a good guard that no removals happen when both employee and non-employee sets are empty.One minor future-proofing option: instead of a fixed-length
side_effectlist of identicalmock_empty_querysetobjects, you could use a callable side effect that always returnsmock_empty_queryset, which would decouple the test from the exact number ofUser.objects.filtercalls.
169-206:test_command_idempotencygives solid coverage of no-op repeated runsThe mocks for
mock_employees_with_badgeandmock_non_employees(driving.exclude().count()and.distinct().count()to zero) combined withuser_filter_side_effect_factorycorrectly simulate a state where no changes are needed. Running the command twice and asserting the “0 employees / 0 non-employees” messages plusUserBadge.objects.filter(...).updatenever being called is a good practical idempotency check.If you want even stronger guarantees, you could also assert the call counts for
Badge.objects.get_or_createandUser.objects.filteracross both runs, but that’s strictly optional.backend/tests/apps/github/management/commands/github_update_users_test.py (2)
239-265: Optionally assertget_or_createis not called for empty user listThe expectations around no prints and no
bulk_savecalls are good. Sincecount()is mocked to return 0, you could strengthen this test slightly by asserting that noMemberProfilelookups happen at all:@@ - mock_print.assert_not_called() - - mock_member_profile.bulk_save.assert_not_called() - mock_user.bulk_save.assert_not_called() + mock_print.assert_not_called() + + mock_member_profile.objects.get_or_create.assert_not_called() + mock_member_profile.bulk_save.assert_not_called() + mock_user.bulk_save.assert_not_called()This would guard against accidental work being done when there are no users.
319-350: Tighten assertion onbulk_savearguments for created profileThis test nicely covers the
created=Truebranch and checks thatgithub_userandcontributions_countare set. To align with the other tests and more directly assert that the new profile is what gets flushed, you could make the final assertion stricter:- assert mock_profile.github_user == mock_user1 - assert mock_profile.contributions_count == 5 - mock_member_profile.bulk_save.assert_called_once() + assert mock_profile.github_user == mock_user1 + assert mock_profile.contributions_count == 5 + mock_member_profile.bulk_save.assert_called_once_with([mock_profile])This would fail if the command ever called
bulk_savewith the wrong list (or an empty one) while still mutatingmock_profile.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/apps/nest/management/commands/nest_update_badges.py(3 hunks)backend/tests/apps/github/management/commands/github_update_users_test.py(7 hunks)backend/tests/apps/nest/management/commands/nest_update_badges_test.py(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2223
File: backend/apps/owasp/models/entity_member.py:50-56
Timestamp: 2025-09-06T19:28:14.297Z
Learning: In the OWASP/Nest project, when migrating scraper logic to GitHub .md file parsing, the sync_leaders method uses member_name as the primary identifier for finding and updating existing EntityMember records, not member_email. This approach is chosen because names are more stable identifiers in markdown files, while emails might be added/updated over time.
📚 Learning: 2025-07-31T07:05:25.056Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 0
File: :0-0
Timestamp: 2025-07-31T07:05:25.056Z
Learning: In the OWASP Nest project, Django views may not properly access authenticated users from sessions created by Strawberry GraphQL mutations. The issue occurs because Django's AuthenticationMiddleware doesn't always populate request.user from session data that GraphQL context successfully uses via info.context.request.user. The solution is to manually resolve users from session data using request.session.get('_auth_user_id') and User.objects.select_related('github_user').get(pk=user_id) to match the same authentication mechanism used by GraphQL context.
Applied to files:
backend/apps/nest/management/commands/nest_update_badges.py
🧬 Code graph analysis (1)
backend/apps/nest/management/commands/nest_update_badges.py (1)
backend/apps/nest/api/internal/nodes/user.py (1)
is_owasp_staff(14-18)
🔇 Additional comments (7)
backend/apps/nest/management/commands/nest_update_badges.py (2)
5-7: Q import is appropriate and scoped correctly
from django.db.models import Qis needed for the new composite filters and is only used in this module, so this change is correct and self-contained.
63-69: Non‑employee filter now mirrors assignment logic and covers legacy dataUsing
Q(owasp_profile__is_owasp_staff=False) | Q(is_owasp_staff=False, owasp_profile__isnull=True)ensures:
- Users with profiles rely solely on the new
owasp_profileflag.- Legacy users without profiles still have badges removed when
is_owasp_staff=False.- The prior fallback gap (skipping users without profiles once some profiles exist) is eliminated.
distinct()is appropriate here given theuser_badgesjoin, so the updated removal query looks solid. Ifis_owasp_staffcan ever beNULL, consider whether “NULL = non‑staff” should also be captured (e.g., viaisnull=True) — otherwise current behavior is consistent but treats NULL as “unknown”.backend/tests/apps/github/management/commands/github_update_users_test.py (5)
33-98: Default-offset path coverage looks solidThe test wiring for
MemberProfile.objects.get_or_create, the contributions aggregation, and thebulk_savebatching assertions all line up with the intended behavior for the default-offset path. No issues spotted here.
100-155: Custom-offset test correctly exercises slicing and batchingThis test cleanly verifies offset slicing, printed progress lines, per-profile
contributions_count, and finalbulk_savecontents. The setup and expectations are consistent with the default-offset case.
156-202: Good coverage for “no contributions” behaviorThis case nicely ensures that when the contributions queryset is empty, profiles still end up with
contributions_count == 0and are flushed via a singlebulk_savecall. That matches the expected “reset to zero” semantics.
204-238: Single-user / batch-size-1 behavior is well specifiedThe test captures both the printed output and the double
bulk_savecalls that arise from batch-size 1 plus a final flush, and confirms the profile’scontributions_count. This is a clear specification of the batching behavior.
267-317: Exact-batch-size test cleanly validates batching semanticsThe setup and assertions correctly cover the case where user count equals
BATCH_SIZE, including per-profile contributions and the twobulk_savecalls with the expected profiles list.
ahmedxgouda
left a comment
ahmedxgouda
left a comment
There was a problem hiding this comment.
Nice. We are close! Just a couple of comments.
| User.objects.select_related("owasp_profile") | ||
| .filter(owasp_profile__has_public_member_page=True, login=login) | ||
| .first() | ||
| ) | ||
|
|
||
| if user: | ||
| return user |
There was a problem hiding this comment.
Use walrus operator for this.
| return 0 | ||
| return int(self.owasp_profile.contributions_count) | ||
| if hasattr(self, "owasp_profile") and self.owasp_profile.contributions_count: | ||
| return int(self.owasp_profile.contributions_count) |
There was a problem hiding this comment.
Why are we converting this to int?
There was a problem hiding this comment.
Sorry my bad it is already defined as int i will simply correct it
…thubuser-owasp-memberprofile' into migrate-non-github-field-from-githubuser-owasp-memberprofile
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/apps/github/models/mixins/user.py (1)
125-129: Prefer MemberProfile value whenever a profile exists (avoid truthiness check).Right now,
idx_contributions_countonly usesowasp_profile.contributions_countwhen it is truthy; if a profile exists but itscontributions_countis legitimately0(orNone), we silently fall back to the legacyself.contributions_count. This is OK while both fields are kept perfectly in sync, but it makes the staged migration brittle and risks surfacing stale data oncegithub.User.contributions_countstops being authoritative.I’d recommend treating the existence of
owasp_profileas the switch, and using its field even when it’s zero:@property def idx_contributions_count(self) -> int: """Return contributions count for indexing.""" profile = getattr(self, "owasp_profile", None) if profile is not None and profile.contributions_count is not None: return profile.contributions_count return self.contributions_countThis keeps the compatibility fallback while ensuring MemberProfile remains the single source of truth whenever present.
backend/apps/github/api/internal/queries/user.py (1)
53-60: User resolver correctly prefers MemberProfile flag; consider select_related on fallback (optional).The new early-return path that filters on
owasp_profile__has_public_member_page=Trueand eagerly loadsowasp_profilelooks correct and aligns with the migration strategy, while the fallback preserves the legacyhas_public_member_pagebehavior onUser.If you want to keep downstream resolvers consistently efficient, you could optionally mirror the eager load in the fallback as well:
return ( User.objects.select_related("owasp_profile") .filter(has_public_member_page=True, login=login) .first() )Not blocking, just a small consistency/perf tweak.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/apps/github/api/internal/queries/user.py(1 hunks)backend/apps/github/models/mixins/user.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2223
File: backend/apps/owasp/models/entity_member.py:50-56
Timestamp: 2025-09-06T19:28:14.297Z
Learning: In the OWASP/Nest project, when migrating scraper logic to GitHub .md file parsing, the sync_leaders method uses member_name as the primary identifier for finding and updating existing EntityMember records, not member_email. This approach is chosen because names are more stable identifiers in markdown files, while emails might be added/updated over time.
|
@ahmedxgouda should I raise second PR now?? |
2 participants
Proposed change
Resolves #2623
This is 1st PR
Migrated the fields
has_public_member_page,is_owasp_staff, andcontributions_countfrombackend/apps/github/models/user.pytobackend/apps/owasp/models/member_profile.py.Added these fields, created the migration
backend/apps/owasp/migrations/0067_memberprofile_backward_compatibility.pyfor the initial data sync, and updated the code paths to write toMemberProfilealong withUserfor subsequent updates inbackend/apps/github/management/commands/github_update_users.py.After running
make github-update-users, the values are being updated successfully.Checklist
make check-testlocally; all checks and tests passed.