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 MemberProfile fields (has_public_member_page, is_owasp_staff, contributions_count), migrations and a data-copy migration, model bulk_save helper, admin list/fieldset exposure, GraphQL resolvers and query eager-paths favoring owasp_profile, null-safe permission logic, bulk-update management command changes, and corresponding tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@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
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?? |
There was a problem hiding this comment.
Actionable comments posted: 0
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/nest/management/commands/nest_update_badges_test.py (1)
282-300: Fix undefined variable references.The variables were renamed to
mock_employees_with_badge(line 282) andmock_non_employees(line 285), but theside_effectlist (lines 295-296, 298-299) still references the old undefined namesmock_employeesandmock_non_employees_filter. This will cause aNameErrorat runtime.🔎 Proposed fix
# Configure filter side effects for two command runs mock_user_filter.side_effect = [ - mock_employees, - mock_non_employees_filter, + mock_employees_with_badge, + mock_non_employees, mock_leaders, - mock_employees, - mock_non_employees_filter, + mock_employees_with_badge, + mock_non_employees, mock_leaders, ]
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/apps/nest/management/commands/nest_update_badges.pybackend/tests/apps/nest/management/commands/nest_update_badges_test.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/nest/management/commands/nest_update_badges.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-18T05:39:42.678Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:40-40
Timestamp: 2025-12-18T05:39:42.678Z
Learning: In Django management commands, prefer using self.stdout.write(...) over print(...) for user-facing stdout output. This aligns with Django conventions and improves testability. When emitting messages, consider using self.stdout.write and, for styled messages, use self.style.SUCCESS/ERROR as appropriate to maintain consistent command output formatting. Apply this guideline to all Python files within any project's management/commands directory.
Applied to files:
backend/tests/apps/nest/management/commands/nest_update_badges_test.py
🔇 Additional comments (4)
backend/tests/apps/nest/management/commands/nest_update_badges_test.py (4)
47-62: LGTM! Dual-key support implemented correctly.The function properly handles both the new
owasp_profile__is_owasp_staffand legacyis_owasp_stafffield paths across Q objects, dicts, and tuples. This backward-compatible approach is appropriate for the migration period.
76-76: LGTM! Dual-key kwargs handling is correct.The fallback pattern
kwargs.get("owasp_profile__is_owasp_staff", kwargs.get("is_owasp_staff"))properly prioritizes the new field while maintaining backward compatibility.
257-258: LGTM! Assertion correctly verifies no badge removal on empty querysets.The assertion
mock_user_badge_filter.assert_not_called()properly validates that badge removal logic is not invoked when there are no former employees, making the test more explicit.
321-326: LGTM! Idempotency assertions are more explicit.The updated assertions correctly verify that both command runs produce identical zero-count outputs and that no update operations occur, making the idempotency test more precise.
|
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.