removed leader and suggested leader, introduced entity_member model #2063
Conversation
Summary by CodeRabbit
WalkthroughReplaces per-model leaders M2M with a new generic EntityMember model and migration, adds GenericRelation on Project/Chapter/Committee, introduces EntityMember admin and inlines, refactors admin mixins, and provides new/updated management commands (owasp_update_leaders / updated github_match_users) with exact+fuzzy matching and bulk-create; tests and Makefiles updated. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Assessment against linked issues
Out-of-scope changes
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🔭 Outside diff range comments (2)
backend/apps/owasp/admin/mixins.py (2)
34-41: Fix get_base_list_display: tuple() call will raise TypeErrortuple() accepts a single iterable; passing multiple positional args via star-unpacking will crash at runtime.
Apply this diff:
- def get_base_list_display(self, *additional_fields): - """Get base list display with additional fields.""" - return tuple( - ("name",) if hasattr(self.model, "name") else (), - *additional_fields, - *self.list_display_field_names, - ) + def get_base_list_display(self, *additional_fields): + """Get base list display with additional fields.""" + base = ("name",) if hasattr(self.model, "name") else () + return base + tuple(additional_fields) + self.list_display_field_names
50-53: Guard prefetch_related in GenericEntityAdminMixin.get_querysetWrap the call to
.prefetch_related("repositories")in atry/exceptto avoid aFieldErrorwhen this mixin is used on models without arepositoriesrelation.• File:
backend/apps/owasp/admin/mixins.py
- In the
GenericEntityAdminMixin.get_querysetmethod- Add import of
FieldErrorat the top+ from django.core.exceptions import FieldError @@ class GenericEntityAdminMixin(BaseOwaspAdminMixin): - def get_queryset(self, request): - """Get queryset with optimized relations.""" - return super().get_queryset(request).prefetch_related("repositories") + def get_queryset(self, request): + """Get queryset with optimized relations.""" + qs = super().get_queryset(request) + try: + return qs.prefetch_related("repositories") + except FieldError: + return qs
🧹 Nitpick comments (13)
backend/apps/owasp/models/project.py (1)
94-96: Add related_query_name for better reverse filtering from EntityMemberDefining a related_query_name enables ergonomic queries like EntityMember.objects.filter(projects__key=...).
Apply this diff:
- members = GenericRelation("owasp.EntityMember") + members = GenericRelation("owasp.EntityMember", related_query_name="projects")backend/apps/owasp/models/chapter.py (1)
67-69: Prefer setting related_query_name for cross-model filtering ergonomicsThis improves discoverability and makes filters from EntityMember more natural (e.g., EntityMember.objects.filter(chapters__key=...)).
Apply this diff:
- members = GenericRelation("owasp.EntityMember") + members = GenericRelation("owasp.EntityMember", related_query_name="chapters")backend/apps/owasp/models/committee.py (1)
31-33: Add related_query_name for better reverse filtering from EntityMemberKeeps query naming consistent across entities and enables concise lookups.
Apply this diff:
- members = GenericRelation("owasp.EntityMember") + members = GenericRelation("owasp.EntityMember", related_query_name="committees")backend/apps/owasp/admin/chapter.py (1)
14-14: Expose membership search in admin (optional but useful)With members now managed via EntityMemberInline, searching by member info can help admins. Consider extending search_fields.
Apply this diff:
- search_fields = ( - "name", - "key", - ) + search_fields = ( + "name", + "key", + "members__member__login", + "members__member__name", + )backend/apps/owasp/admin/entity_member.py (1)
11-16: Show the related entity in list_display for contextExpose the linked entity to make rows self-explanatory in the changelist.
list_display = ( + "entity", "member", "kind", "is_reviewed", "order", )backend/apps/owasp/models/common.py (1)
90-98: Optional: Preserve display order when exposing leadersEntityMember has an order field; consider returning leaders ordered by that priority.
@property def leaders(self) -> models.QuerySet[User]: """Return entity's leaders.""" - return User.objects.filter( - pk__in=self.members.filter(kind=EntityMember.MemberKind.LEADER).values_list( - "member_id", flat=True - ) - ) + from django.contrib.contenttypes.models import ContentType + ct = ContentType.objects.get_for_model(self, for_concrete_model=False) + return ( + User.objects.filter( + entity_memberships__content_type=ct, + entity_memberships__object_id=self.pk, + entity_memberships__kind=EntityMember.MemberKind.LEADER, + ) + .order_by("entity_memberships__order", "login") + .distinct() + )backend/apps/owasp/models/entity_member.py (3)
16-22: Add default ordering for predictable iteration and admin displayOrdering by (content_type, object_id, order, member__login) helps both admin and API consumers.
class Meta: db_table = "owasp_entity_members" unique_together = ("content_type", "object_id", "member", "kind") indexes = [ models.Index(fields=["content_type", "object_id"]), ] verbose_name_plural = "Entity Members" + ordering = ("content_type", "object_id", "order", "member__login")
24-33: Consider adding db index on kind if frequently filteredIf most lookups filter by kind="leader", an index on kind can help. This can be deferred until profiling confirms need.
37-39: Minor: str can cause extra queriesUsing self.entity in str dereferences the GenericForeignKey and may trigger queries in bulk contexts (admin list). This is acceptable but be aware; prefer showing fields (member, kind, order) in admin list_display which you already do.
backend/apps/owasp/admin/mixins.py (2)
10-17: Inline looks good; consider admin UX improvementsOptional:
- Add show_change_link = True to quickly navigate to member pages.
- Consider autocomplete_fields = ("member",) if you have admin search configured for github.User (preferable to raw_id_fields for large tables).
For example:
class EntityMemberInline(GenericTabularInline): @@ raw_id_fields = ("member",) + show_change_link = TrueNote: Do not set autocomplete_fields together with raw_id_fields.
109-111: Ensure list_filter concatenation is type-safeIf extra_list_filters is a list, tuple + list will raise TypeError. Cast to tuple.
Apply this diff:
- if extra_list_filters: - config["list_filter"] = self.list_filter_field_names + extra_list_filters + if extra_list_filters: + config["list_filter"] = ( + self.list_filter_field_names + tuple(extra_list_filters) + )backend/apps/github/management/commands/github_match_users.py (2)
34-35: Atomic transaction scope may be too broadDecorating handle() with @transaction.atomic wraps the entire multi-model run in a single transaction. For large datasets (“all”), this increases lock duration and the risk of massive rollbacks.
Consider:
- Moving the atomic boundary into _process_entities (per model).
- Or batching EntityMember.objects.bulk_create in chunks within an atomic block.
I can provide a patch if you confirm the preferred scope.
109-145: Optional performance: accelerate exact matches before fuzzy scoringYou can precompute dictionaries for O(1) exact matches and only run fuzzy scoring on the remaining names. This reduces overall token_sort_ratio calls.
Example approach (sketch):
- Build:
- by_login = {u["login"].lower(): u}
- by_name = {u["name"].lower(): u for u in users_list if u["name"]}
- For each leader_lower:
- if hit in by_login/by_name: append and continue
- else: run the fuzzy loop
Happy to send a concrete patch if desired.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
backend/apps/github/management/commands/github_match_users.py(2 hunks)backend/apps/owasp/admin/__init__.py(1 hunks)backend/apps/owasp/admin/chapter.py(1 hunks)backend/apps/owasp/admin/committee.py(1 hunks)backend/apps/owasp/admin/entity_member.py(1 hunks)backend/apps/owasp/admin/mixins.py(1 hunks)backend/apps/owasp/admin/project.py(2 hunks)backend/apps/owasp/migrations/0047_remove_chapter_leaders_and_more.py(1 hunks)backend/apps/owasp/models/chapter.py(2 hunks)backend/apps/owasp/models/committee.py(2 hunks)backend/apps/owasp/models/common.py(2 hunks)backend/apps/owasp/models/entity_member.py(1 hunks)backend/apps/owasp/models/project.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (9)
backend/apps/owasp/admin/entity_member.py (1)
backend/apps/owasp/models/entity_member.py (1)
EntityMember(10-39)
backend/apps/owasp/admin/__init__.py (1)
backend/apps/owasp/admin/entity_member.py (1)
EntityMemberAdmin(8-28)
backend/apps/owasp/admin/chapter.py (1)
backend/apps/owasp/admin/mixins.py (2)
EntityMemberInline(10-17)GenericEntityAdminMixin(47-91)
backend/apps/owasp/admin/committee.py (1)
backend/apps/owasp/admin/mixins.py (2)
EntityMemberInline(10-17)GenericEntityAdminMixin(47-91)
backend/apps/owasp/admin/project.py (1)
backend/apps/owasp/admin/mixins.py (2)
EntityMemberInline(10-17)GenericEntityAdminMixin(47-91)
backend/apps/owasp/models/common.py (3)
backend/apps/github/api/internal/queries/user.py (1)
user(40-56)backend/apps/github/models/user.py (1)
User(19-159)backend/apps/owasp/models/entity_member.py (2)
EntityMember(10-39)MemberKind(13-14)
backend/apps/owasp/models/entity_member.py (4)
backend/apps/owasp/models/chapter.py (1)
Meta(32-38)backend/apps/owasp/models/committee.py (1)
Meta(27-29)backend/apps/owasp/models/common.py (1)
Meta(28-29)backend/apps/owasp/models/project.py (1)
Meta(42-48)
backend/apps/github/management/commands/github_match_users.py (4)
backend/apps/github/api/internal/queries/user.py (1)
user(40-56)backend/apps/github/models/user.py (1)
User(19-159)backend/apps/owasp/models/chapter.py (1)
Chapter(21-217)backend/apps/owasp/models/entity_member.py (2)
EntityMember(10-39)MemberKind(13-14)
backend/apps/owasp/admin/mixins.py (1)
backend/apps/owasp/models/entity_member.py (1)
EntityMember(10-39)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: CodeQL (python)
🔇 Additional comments (10)
backend/apps/owasp/models/project.py (1)
8-8: Import aligns with GenericRelation usage — LGTMRequired import for the new GenericRelation is correct.
backend/apps/owasp/admin/__init__.py (1)
10-10: EntityMemberAdmin registration confirmed
TheEntityMembermodel is already registered with its admin class inbackend/apps/owasp/admin/entity_member.py:31viaadmin.site.register(EntityMember, EntityMemberAdmin)No further action needed.
backend/apps/owasp/models/chapter.py (1)
7-7: Import addition is correctGenericRelation import is required and correct.
backend/apps/owasp/models/committee.py (1)
5-5: Import addition is correctGenericRelation import is required and correct.
backend/apps/owasp/admin/chapter.py (2)
7-7: Imports updated to new mixin/inline — LGTMImporting EntityMemberInline and GenericEntityAdminMixin aligns with the new EntityMember model.
10-10: Switch away from LeaderAdminMixin — good refactorRemoving the legacy mixin in favor of GenericEntityAdminMixin is aligned with the model changes.
backend/apps/owasp/admin/project.py (1)
19-19: Using EntityMemberInline is the right directionInline management aligns with the new EntityMember model and simplifies membership editing in-place.
backend/apps/owasp/admin/committee.py (1)
13-14: LGTM on switching to inline membership editingAutocomplete on owasp_repository and inlines for EntityMember align with the new model and simplify admin UX.
backend/apps/owasp/models/common.py (1)
90-98: All OWASP entity models define themembersGenericRelationI verified that
Chapter,Committee, andProjecteach declaremembers = GenericRelation("owasp.EntityMember")and there are no remaining many-to-many fields or
.add()calls forleadersorsuggested_leaders. No further action is needed.backend/apps/github/management/commands/github_match_users.py (1)
90-104: self.style.NOTICE is not a standard Django styleUse self.style.WARNING or plain write. NOTICE is not present in Django’s color style and will raise AttributeError.
Apply this diff:
- else: - self.stdout.write( - self.style.NOTICE(f" -> No new leader records to create for {model_label}.") - ) + else: + self.stdout.write( + self.style.WARNING(f" -> No new leader records to create for {model_label}.") + )Likely an incorrect or invalid review comment.
backend/apps/owasp/migrations/0047_remove_chapter_leaders_and_more.py
Outdated
Show resolved
Hide resolved
backend/apps/owasp/migrations/0047_remove_chapter_leaders_and_more.py
Outdated
Show resolved
Hide resolved
backend/apps/owasp/migrations/0047_remove_chapter_leaders_and_more.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
backend/tests/apps/github/management/commands/github_match_users_test.py (6)
23-27: Make help-text assertion less brittle; also consider adding an integration test for EntityMember creation semanticsAsserting full equality on help text can cause spurious failures if the help string is edited for clarity. Prefer checking key phrases.
Additionally, since the help mentions creating EntityMember records, consider adding a focused integration test that exercises handle() to assert it:
- creates EntityMember records with is_reviewed=False,
- respects uniqueness (entity, leader),
- supports the "all" model_name path.
Proposed change for the assertion:
- assert ( - command.help - == "Matches entity leader names with GitHub Users and creates EntityMember records." - ) + assert "GitHub Users" in command.help + assert "EntityMember" in command.helpI can scaffold an integration test that sets up ContentTypes and a minimal set of model instances, runs the command with model_name="all", and verifies EntityMember rows (including is_reviewed default) were created. Want me to add it?
34-50: Avoid asserting exact add_argument call count to reduce brittlenessAsserting the total number of add_argument calls makes the test fragile as optional flags evolve. Keep the specific assertions you already have and drop the count check.
- assert parser.add_argument.call_count == 2 + # Avoid asserting total call count to keep the test resilient to new optional args.
76-82: Docstring says "dictionary" but fixture returns a listFix the docstring to reflect the actual return type.
- def mock_users(self): - """Return a dictionary of mock users.""" + def mock_users(self): + """Return a list of mock users."""
85-92: Strengthen exact match case to also assert case-insensitive name matchingChanging the name input to uppercase ensures the case-insensitive path is exercised.
- leaders_raw = ["john_doe", "Jane Doe"] + leaders_raw = ["john_doe", "JANE DOE"]
104-107: Stabilize the unmatched test by patching fuzz to return low scoresRelying on real fuzz scores introduces brittle coupling to an external algorithm. Stub fuzz to force low scores for clarity.
+ @patch("apps.github.management.commands.github_match_users.fuzz") - def test_unmatched_leader(self, command, mock_users): + def test_unmatched_leader(self, mock_fuzz, command, mock_users): """Test that an unknown leader returns no matches.""" + mock_fuzz.token_sort_ratio.return_value = 0 leaders_raw = ["unknown_leader"] matches = command._find_user_matches(leaders_raw, mock_users, 100) assert matches == []
78-82: Add a test to verify duplicates are deduped and empty inputs are ignoredhelpers dedupe leaders and skip falsey values; add a small test to lock this in.
Add this test (outside the current block):
def test_duplicates_and_empty_ignored(self, command, mock_users): leaders_raw = ["john_doe", "", None, "john_doe"] matches = command._find_user_matches(leaders_raw, mock_users, 90) assert len(matches) == 1 assert matches[0]["id"] == 1I can open a follow-up PR to include this test along with coverage for threshold edge cases if you'd like.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/apps/github/management/commands/github_match_users.py(2 hunks)backend/tests/apps/github/management/commands/github_match_users_test.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/github/management/commands/github_match_users.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/tests/apps/github/management/commands/github_match_users_test.py (1)
backend/apps/github/management/commands/github_match_users.py (3)
Command(16-142)add_arguments(19-31)_find_user_matches(107-142)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (5)
backend/tests/apps/github/management/commands/github_match_users_test.py (5)
13-16: Good fixture: command instance with mocked stdout is set up correctlyInstantiating the command and mocking stdout makes downstream I/O-safe assertions possible. Looks clean.
20-20: Informative test class docstringClear scope definition for the suite. No issues.
42-44: Choices include 'all' with clear help text—good alignment with CLI designThe parser choices and help text match the updated command API.
72-74: Focused tests for _find_user_matches are a good refactorSeparating helper coverage from command orchestration keeps tests fast and targeted.
96-101: Deterministic fuzzy matching via patch is solidStubbing fuzz.token_sort_ratio to steer the match keeps the test deterministic and intent-revealing. Looks good.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
backend/apps/github/management/commands/github_match_users.py (3)
60-62: Fix model label: currently prints “Type” instead of the class nameUse the model class’ name, not its metaclass.
- model_label = model_class.__class__.__name__.capitalize() + model_label = model_class.__name__.capitalize()
51-53: Relax user validation to not require a nameRequiring both login and name length ≥ 2 reduces recall; GitHub users often have empty names. Validate only the login.
- valid_users = [u for u in all_users if self.is_valid_user(u["login"], u["name"])] + valid_users = [u for u in all_users if self.is_valid_user(u["login"], u["name"])]And in is_valid_user:
- def is_valid_user(self, login, name): - """Check if GitHub user meets minimum requirements.""" - return len(login) >= ID_MIN_LENGTH and len(name or "") >= ID_MIN_LENGTH + def is_valid_user(self, login, name): + """Check if GitHub user meets minimum requirements.""" + return isinstance(login, str) and len(login) >= ID_MIN_LENGTH
67-75: leaders_raw may be missing; use getattr to avoid AttributeErrorSafely handle entities without leaders_raw by using getattr and passing the value through.
- for entity in model_class.objects.all(): - if not entity.leaders_raw: - continue - - matched_users = self.find_user_matches(entity.leaders_raw, users_list, threshold) + for entity in model_class.objects.all(): + leaders_raw = getattr(entity, "leaders_raw", None) + if not leaders_raw: + continue + + matched_users = self.find_user_matches(leaders_raw, users_list, threshold)
🧹 Nitpick comments (7)
backend/tests/apps/github/management/commands/github_match_users_test.py (2)
107-125: Prefer stronger assertions on bulk_create callsYou can tighten expectations:
- Assert bulk_create was called once and with ignore_conflicts=True.
- Assert content_type was passed through (even as a MagicMock) if you want to ensure the command wires the generic relation correctly.
Example diff:
- mock_bulk_create = self.mock_entity_member.objects.bulk_create - # FIX: Use plain assert - assert mock_bulk_create.called + mock_bulk_create = self.mock_entity_member.objects.bulk_create + mock_bulk_create.assert_called_once() + _, kwargs = mock_bulk_create.call_args + assert kwargs.get("ignore_conflicts") is TrueAlso, consider dropping the “FIX:” comment as it looks like a leftover note.
126-138: Negative assertion could be stricterCurrently asserting that (4, 2) is not present doesn’t guarantee that no record for entity 4 was created. If the intent is to ensure no fuzzy match was created for entity 4 under a high threshold, assert that no tuple with object_id == 4 exists.
- assert (4, 2) not in created_members + assert all(obj_id != 4 for obj_id, _ in created_members)backend/apps/github/management/commands/github_match_users.py (5)
43-50: Redundant invalid-model check; argparse choices already enforce thisSince add_arguments declares choices=("chapter","committee","project"), argparse will raise CommandError before handle() runs. You can simplify by removing the manual check and use a clearer variable name.
- if model_name not in model_map: - self.stdout.write( - self.style.ERROR("Invalid model name! Choose from: chapter, committee, project") - ) - return - - models_to_process = model_map[model_name] + model_class = model_map[model_name]And update the call below:
- self.process_entities(models_to_process, valid_users, threshold) + self.process_entities(model_class, valid_users, threshold)
118-121: Trim whitespace before matchingStripping leader names reduces false negatives from trailing/leading spaces.
- leader_lower = leader_name.lower() + leader_lower = leader_name.strip().lower()
122-131: Minor optimization: break after exact match foundYou set exact_match_found and continue; you can also break out of the user loop once you append a match. Dedupe makes this non-functional, but it saves unnecessary comparisons.
- for user in users_list: + for user in users_list: if user["login"].lower() == leader_lower or ( user["name"] and user["name"].lower() == leader_lower ): matched_users.append(user) exact_match_found = True + break
91-99: Consider transaction wrapping bulk_createIf the process is interrupted mid-insert, you may end up with partial creations for a given model. Wrapping the bulk_create in an atomic block improves consistency.
- if new_members_to_create: - created_records = EntityMember.objects.bulk_create( - new_members_to_create, - ignore_conflicts=True, - ) + if new_members_to_create: + from django.db import transaction + with transaction.atomic(): + created_records = EntityMember.objects.bulk_create( + new_members_to_create, + ignore_conflicts=True, + )
51-56: Pre-index users for exact lookups to reduce complexityCurrent exact matching is O(E * U). Precompute dicts for login->user and name->user once, then only perform fuzzy matching for not-found entries. This will materially improve performance with large user tables.
If you agree, I can propose a focused refactor that:
- builds users_by_login and users_by_name in handle(),
- passes those to find_user_matches,
- attempts exact lookups first from the dicts,
- falls back to scanning users_list only for fuzzy matches.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/apps/github/management/commands/github_match_users.py(2 hunks)backend/apps/owasp/migrations/0047_remove_chapter_leaders_and_more.py(1 hunks)backend/apps/owasp/models/entity_member.py(1 hunks)backend/tests/apps/github/management/commands/github_match_users_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/apps/owasp/migrations/0047_remove_chapter_leaders_and_more.py
- backend/apps/owasp/models/entity_member.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/apps/github/management/commands/github_match_users.py (3)
backend/apps/owasp/models/chapter.py (1)
Chapter(21-217)backend/apps/owasp/models/entity_member.py (2)
EntityMember(10-40)MemberKind(13-14)backend/apps/owasp/models/project.py (1)
Project(31-373)
🔇 Additional comments (6)
backend/tests/apps/github/management/commands/github_match_users_test.py (5)
12-38: Solid patching strategy and isolated tests via SimpleTestCaseClass-level patchers with stopall at tearDownClass effectively isolate the command’s dependencies. Good use of call_command to exercise the command end-to-end with mocks.
39-63: Mock configuration is robust and avoids DB accessResetting mocks per test and stubbing EntityMember() as a call-through constructor plus bulk_create return behavior keeps tests deterministic and database-free. Nicely done.
64-71: User fixtures cover key name/login combinationsIncluding users with login-only vs name-based variants allows you to exercise exact vs fuzzy and the short-length filter paths.
139-149: Good guard for short identifiersThis test validates the user-length filtering behavior and the “no-op” message path. It’s effective for catching regressions in the validation predicate.
150-159: Good verification that exact match bypasses fuzzyPatching fuzz and asserting token_sort_ratio is not called confirms the intended short-circuit for exact matches.
backend/apps/github/management/commands/github_match_users.py (1)
78-89: EntityMember payload aligns with the new model and issue requirementsUsing content_type/object_id + member_id, setting kind=LEADER and is_reviewed=False, and bulk_create(ignore_conflicts=True) correctly supports the new EntityMember model and uniqueness constraint.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
backend/apps/github/management/commands/github_match_users.py (3)
101-104: Fix invalid ColorStyle usage: NOTICE isn’t available; use a safe fallbackDjango’s ColorStyle doesn’t provide NOTICE; this will raise an AttributeError on many versions. Use a fallback (e.g., WARNING).
- else: - self.stdout.write( - self.style.NOTICE(f" -> No new leader records to create for {model_label}.") - ) + else: + style_notice = getattr(self.style, "NOTICE", self.style.WARNING) + self.stdout.write( + style_notice(f" -> No new leader records to create for {model_label}.") + )
67-74: Avoid AttributeError on entities without leaders_rawAccessing entity.leaders_raw directly can fail when absent. Use getattr and pass the local variable to the matcher.
- for entity in model_class.objects.all(): - if not entity.leaders_raw: - continue - - matched_users = self.find_user_matches(entity.leaders_raw, users_list, threshold) + for entity in model_class.objects.all(): + leaders_raw = getattr(entity, "leaders_raw", None) + if not leaders_raw: + continue + + matched_users = self.find_user_matches(leaders_raw, users_list, threshold)To confirm whether leaders_raw exists consistently on Chapter/Committee/Project models, run:
#!/bin/bash # Find declarations and usages of leaders_raw across the repo rg -n --iglob '!**/node_modules/**' --iglob '!**/dist/**' --iglob '!**/build/**' $'leaders_raw'Also applies to: 71-72
106-109: Relax user validation to not require a non-empty nameRequiring a name excludes valid GitHub accounts and reduces recall. Validate login only.
- def is_valid_user(self, login, name): - """Check if GitHub user meets minimum requirements.""" - return len(login) >= ID_MIN_LENGTH and len(name or "") >= ID_MIN_LENGTH + def is_valid_user(self, login, name): + """Check if GitHub user meets minimum requirements.""" + return isinstance(login, str) and len(login) >= ID_MIN_LENGTH
🧹 Nitpick comments (5)
backend/apps/github/management/commands/github_match_users.py (5)
92-95: Use batch_size with bulk_create to bound memory and round-tripsFor large inserts, specify batch_size to avoid building oversized INSERTs and to limit memory.
if new_members_to_create: created_records = EntityMember.objects.bulk_create( new_members_to_create, - ignore_conflicts=True, + ignore_conflicts=True, + batch_size=1000, )
56-56: Remove leading newline in the final success messageMinor UX nit: BaseCommand adds newlines automatically; a leading newline renders an extra blank line.
- self.stdout.write(self.style.SUCCESS("\nCommand finished successfully.")) + self.stdout.write(self.style.SUCCESS("Command finished successfully."))
98-99: Clarify log wording: “EntityMember records” instead of “leader records”Since you’re creating EntityMember entries with kind=LEADER, this wording is clearer.
- f" -> Created {len(created_records)} new leader records for {model_label}." + f" -> Created {len(created_records)} new EntityMember records for {model_label}."
1-1: Update module docstring to reflect the new behaviorCurrent text mentions “slack members”. Align with EntityMember creation.
-"""A command to perform fuzzy and exact matching of leaders/slack members with User model.""" +"""Matches entity leader names with GitHub Users and creates EntityMember records."""
43-47: Redundant invalid model guard (choices already restricts input)Given argparse choices, this branch is unreachable in normal use. You can keep it as a defensive guard or remove it to reduce noise.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/apps/github/management/commands/github_match_users.py(2 hunks)backend/apps/owasp/admin/__init__.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/owasp/admin/init.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/apps/github/management/commands/github_match_users.py (3)
backend/apps/owasp/models/chapter.py (1)
Chapter(21-217)backend/apps/owasp/models/entity_member.py (2)
EntityMember(10-40)MemberKind(13-14)backend/apps/owasp/models/project.py (1)
Project(31-373)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
🔇 Additional comments (2)
backend/apps/github/management/commands/github_match_users.py (2)
60-62: Correct use of model_class.name for label looks goodThis fixes the earlier bug that printed "Type". Using name is correct here.
78-87: EntityMember creation flags align with requirementsSetting kind=LEADER and is_reviewed=False matches the migration plan and review workflow.
There was a problem hiding this comment.
I updated the code and after testing it locally have 2 requests:
- when adding entity members we need to set
ordervalue based on leader's name position inleaders_raw, e.g. Arkadii Yakovets (10), Kate Golovanova (20), Starr Brown (30) - show entity name (not just entity_type and entity_id) in Django admin
- implement entity member search to support entity key/name
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (2)
backend/apps/owasp/admin/mixins.py (2)
24-31: Bug: tuple construction is invalid;tuple()doesn’t accept multiple iterables
tuple(("name",), *additional_fields, *self.list_display_field_names)raisesTypeError. Build the tuple via concatenation.Apply this diff:
- def get_base_list_display(self, *additional_fields): - """Get base list display with additional fields.""" - return tuple( - ("name",) if hasattr(self.model, "name") else (), - *additional_fields, - *self.list_display_field_names, - ) + def get_base_list_display(self, *additional_fields): + """Get base list display with additional fields.""" + base = ("name",) if hasattr(self.model, "name") else tuple() + return base + tuple(additional_fields) + tuple(self.list_display_field_names)
61-64: Guard prefetch for models withoutrepositories; consider selectingowasp_repositoryinsteadUnconditionally calling
prefetch_related("repositories")will error for models that don’t define that relation (e.g., Chapter, Committee). Guard by checking model attributes and useselect_relatedfor single FK.Apply this diff:
- def get_queryset(self, request): - """Get queryset with optimized relations.""" - return super().get_queryset(request).prefetch_related("repositories") + def get_queryset(self, request): + """Get queryset with optimized relations.""" + qs = super().get_queryset(request) + # Prefetch M2M repositories when present; otherwise, select FK owasp_repository if available. + if hasattr(self.model, "repositories"): + qs = qs.prefetch_related("repositories") + elif hasattr(self.model, "owasp_repository"): + qs = qs.select_related("owasp_repository") + return qs
♻️ Duplicate comments (1)
backend/apps/owasp/models/entity_member.py (1)
7-7: Import path aligned with project style (resolves prior comment).Good switch to the concrete module path for User. This matches usage elsewhere and avoids relying on package re-exports.
🧹 Nitpick comments (5)
backend/apps/owasp/migrations/0048_entitymember.py (1)
68-74: Consider adding a composite index including kind to speed leader lookupsYou already have an index on
(entity_type, entity_id). If most queries filter by kind (e.g., leaders), a composite index(entity_type, entity_id, kind)can shave I/O off list/detail pages and admin inlines.Apply this diff to the indexes list:
"indexes": [ models.Index( fields=["entity_type", "entity_id"], name="owasp_entit_entity__dda41b_idx" ), models.Index(fields=["member"], name="owasp_entit_member__6e516f_idx"), + models.Index( + fields=["entity_type", "entity_id", "kind"], + name="owasp_entit_entity_kind_idx", + ), ],backend/apps/owasp/management/commands/owasp_update_leaders.py (3)
1-1: Docstring mentions Slack members but command only handles leadersTighten the docstring to match behavior and avoid confusion.
Apply this diff:
-"""A command to perform fuzzy and exact matching of leaders/slack members with User model.""" +"""A command to perform fuzzy and exact matching of entity leaders with the User model."""
110-146: Optional: reduce matching complexity with precomputed lookups and fewer passesCurrent matching loops over all users twice per leader name. For large datasets, this is O(E * L * U). You can pre-index users by normalized login and name and short-circuit exact matches in O(1), then fall back to fuzzy across a smaller candidate set.
Example drop-in refactor:
- def find_user_matches(self, leaders_raw, users_list, threshold): + def find_user_matches(self, leaders_raw, users_list, threshold): """Find user matches for a list of raw leader names.""" - matched_users = [] - - for leader_name in set(leaders_raw): + matched_users = [] + + # Precompute normalized maps for O(1) exact lookups. + by_login = {u["login"].lower(): u for u in users_list if u.get("login")} + by_name = {u["name"].lower(): u for u in users_list if u.get("name")} + + for leader_name in set(leaders_raw): if not leader_name: continue - - leader_lower = leader_name.lower() - best_fuzzy_match = None - highest_score = 0 - - exact_match_found = False - for user in users_list: - if user["login"].lower() == leader_lower or ( - user["name"] and user["name"].lower() == leader_lower - ): - matched_users.append(user) - exact_match_found = True - - if exact_match_found: - continue - - for user in users_list: - score = fuzz.token_sort_ratio(leader_lower, user["login"].lower()) - if user["name"]: - score = max(score, fuzz.token_sort_ratio(leader_lower, user["name"].lower())) - - if score > highest_score: - highest_score = score - best_fuzzy_match = user - - if highest_score >= threshold: - matched_users.append(best_fuzzy_match) - - return list({user["id"]: user for user in matched_users}.values()) + leader_lower = leader_name.lower() + + # Fast exact match + exact = by_login.get(leader_lower) or by_name.get(leader_lower) + if exact: + matched_users.append(exact) + continue + + # Fuzzy fallback across all valid users + best_user = None + best_score = -1 + for user in users_list: + score = fuzz.token_sort_ratio(leader_lower, user["login"].lower()) + if user.get("name"): + score = max(score, fuzz.token_sort_ratio(leader_lower, user["name"].lower())) + if score > best_score: + best_score = score + best_user = user + + if best_score >= threshold and best_user: + matched_users.append(best_user) + + # Deduplicate by user id + return list({u["id"]: u for u in matched_users}.values())
63-91: Consider streaming and chunked writes for large importsIf the dataset is large, consider iterating with
iterator()and chunkingbulk_createto avoid high memory usage.Sketch:
- Iterate:
for entity in model_class.objects.iterator(chunk_size=1000):- Accumulate to a buffer (e.g., 5k instances) and
bulk_create(buffer, ignore_conflicts=True), then clear.backend/apps/owasp/models/entity_member.py (1)
42-46: Future-proofkindlength to allow additional roles.
max_length=6is tight (only fits "leader"). If you add roles like "co-leader" or "maintainer", this will cause validation/migration churn. Bump to something reasonable (e.g., 16).- kind = models.CharField( - max_length=6, + kind = models.CharField( + max_length=16, choices=MemberKind.choices, default=MemberKind.LEADER, )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
backend/apps/owasp/admin/chapter.py(1 hunks)backend/apps/owasp/admin/committee.py(1 hunks)backend/apps/owasp/admin/entity_member.py(1 hunks)backend/apps/owasp/admin/mixins.py(2 hunks)backend/apps/owasp/admin/project.py(2 hunks)backend/apps/owasp/management/commands/owasp_update_leaders.py(1 hunks)backend/apps/owasp/migrations/0048_entitymember.py(1 hunks)backend/apps/owasp/models/chapter.py(2 hunks)backend/apps/owasp/models/committee.py(2 hunks)backend/apps/owasp/models/common.py(2 hunks)backend/apps/owasp/models/entity_member.py(1 hunks)backend/apps/owasp/models/project.py(2 hunks)backend/tests/apps/owasp/management/commands/owasp_update_leaders_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- backend/apps/owasp/models/project.py
- backend/apps/owasp/models/chapter.py
- backend/apps/owasp/models/common.py
- backend/apps/owasp/admin/entity_member.py
- backend/apps/owasp/admin/project.py
- backend/apps/owasp/models/committee.py
- backend/apps/owasp/admin/committee.py
🧰 Additional context used
🧬 Code Graph Analysis (5)
backend/apps/owasp/migrations/0048_entitymember.py (1)
backend/apps/mentorship/api/internal/nodes/mentor.py (1)
name(18-20)
backend/apps/owasp/management/commands/owasp_update_leaders.py (5)
backend/apps/github/api/internal/queries/user.py (1)
user(40-56)backend/apps/owasp/models/chapter.py (1)
Chapter(21-217)backend/apps/owasp/models/committee.py (1)
Committee(16-90)backend/apps/owasp/models/entity_member.py (2)
EntityMember(10-59)MemberKind(13-14)backend/apps/owasp/models/project.py (1)
Project(31-373)
backend/apps/owasp/admin/chapter.py (1)
backend/apps/owasp/admin/mixins.py (2)
EntityMemberInline(37-55)GenericEntityAdminMixin(58-102)
backend/apps/owasp/models/entity_member.py (1)
backend/apps/github/api/internal/queries/user.py (1)
user(40-56)
backend/apps/owasp/admin/mixins.py (1)
backend/apps/owasp/models/entity_member.py (1)
EntityMember(10-59)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
🔇 Additional comments (5)
backend/apps/owasp/admin/chapter.py (1)
14-14: LGTM: Inline for EntityMemberUsing
EntityMemberInlinehere aligns with the new generic membership model and provides a clear editing surface.backend/tests/apps/owasp/management/commands/owasp_update_leaders_test.py (2)
128-140: Good coverage of threshold behavior and fuzzy vs. exact preferenceThese assertions exercise the intended matching logic and ensure fuzzy matching respects the threshold and is skipped when exact matches exist.
105-109: Expect SystemExit in the test; remove redundant runtime validation in the commandArgparse enforces the "choices" for the positional model_name, so call_command("owasp_update_leaders", "invalid_model") fails during parsing (SystemExit), not in handle() (CommandError). Update the test and remove the redundant runtime check.
Files to change:
- backend/tests/apps/owasp/management/commands/owasp_update_leaders_test.py
- Replace the pytest expectation for CommandError with SystemExit.
- Suggested patch:
- with pytest.raises(CommandError): + with pytest.raises(SystemExit): call_command("owasp_update_leaders", "invalid_model")- backend/apps/owasp/management/commands/owasp_update_leaders.py
- Remove the redundant runtime validation block that checks model_name against model_map and returns an error, since argparse choices already prevent invalid values:
- if model_name not in model_map: - self.stdout.write( - self.style.ERROR("Invalid model name! Choose from: chapter, committee, project") - ) - returnLikely an incorrect or invalid review comment.
backend/apps/owasp/admin/mixins.py (1)
37-56: Inline configuration looks solid
ct_field,ct_fk_field, ordering, andraw_id_fieldsare correctly set for a performant generic inline.backend/apps/owasp/models/entity_member.py (1)
16-29: Confirm uniqueness semantics & migrate to UniqueConstraint; add ordering and stronger indexesI inspected backend/apps/owasp/models/entity_member.py and the migration backend/apps/owasp/migrations/0048_entitymember.py. Both currently use unique_together = ("entity_type", "entity_id", "member", "kind"). Note MemberKind currently only defines LEADER, so today that is effectively unique on (entity_type, entity_id, member), but adding new kinds would change the semantics.
Recommendation: switch to explicit UniqueConstraint(s), add deterministic ordering and stronger indexes, and create a new migration (do not edit 0048) to apply the constraint/index changes. If the intent is to allow multiple roles per member per entity, keep kind in the uniqueness but still convert to UniqueConstraint for clarity.
Files to update:
- backend/apps/owasp/models/entity_member.py (Meta)
- add a new migration to alter constraints/indexes (refer to backend/apps/owasp/migrations/0048_entitymember.py for current state — do not modify historical migration)
Suggested model Meta diff:
class Meta: - db_table = "owasp_entity_members" - unique_together = ( - "entity_type", - "entity_id", - "member", - "kind", - ) - indexes = [ - models.Index(fields=["entity_type", "entity_id"]), - models.Index(fields=["member"]), - ] - verbose_name_plural = "Entity members" + db_table = "owasp_entity_members" + verbose_name = "Entity member" + verbose_name_plural = "Entity members" + ordering = ["entity_type", "entity_id", "order", "id"] + constraints = [ + models.UniqueConstraint( + fields=["entity_type", "entity_id", "member"], + name="uq_owasp_entity_member", + ), + ] + indexes = [ + models.Index(fields=["entity_type", "entity_id", "kind"], name="idx_entity_kind"), + models.Index(fields=["member"], name="idx_member"), + ]If multiple roles per member per entity are intended, keep kind in the uniqueness but convert to UniqueConstraint:
- unique_together = ( - "entity_type", - "entity_id", - "member", - "kind", - ) + constraints = [ + models.UniqueConstraint( + fields=["entity_type", "entity_id", "member", "kind"], + name="uq_owasp_entity_member_kind", + ), + ]Question for you (per #2017): Should duplicates across kinds be allowed (i.e., permit same member multiple times for the same entity with different kinds), or should membership be unique per (entity_type, entity_id, member)?
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
backend/apps/owasp/admin/entity_member.py (2)
18-19: Preload FKs and use a more useful default orderingSelect related FKs to reduce N+1 on changelist; order by entity then member for predictable browsing.
class EntityMemberAdmin(admin.ModelAdmin): """Admin for EntityMember records (generic link to any OWASP entity).""" autocomplete_fields = ("member",) + list_select_related = ("member", "entity_type") @@ - ordering = ("-id",) + ordering = ("entity_type", "entity_id", "order", "member__login")Also applies to: 46-46
19-27: Restrict editable ContentTypes to OWASP entitiesPrevents accidentally linking to unrelated models and reduces admin footguns.
class EntityMemberAdmin(admin.ModelAdmin): @@ fields = ( - "entity_type", + "entity_type", "entity_id", "member", "kind", "order", "is_reviewed", "description", ) @@ - raw_id_fields = ("member",) + # raw_id_fields not needed when using autocomplete_fields @@ @admin.display(description="Entity", ordering="entity_type") def entity(self, obj): """Return entity link.""" if obj.entity: link = reverse( f"admin:{obj.entity_type.app_label}_{obj.entity_type.model}_change", args=[obj.entity_id], ) return format_html('<a href="{}">{}</a>', link, obj.entity) return "— No Associated Entity —" + + def formfield_for_foreignkey(self, db_field, request, **kwargs): + """Limit content type choices to OWASP entities.""" + if db_field.name == "entity_type": + kwargs.setdefault( + "queryset", + ContentType.objects.filter( + app_label="owasp", + model__in=("project", "chapter", "committee"), + ), + ) + return super().formfield_for_foreignkey(db_field, request, **kwargs)Also applies to: 35-41, 48-58
backend/apps/owasp/management/commands/owasp_update_leaders.py (2)
43-49: Remove unreachable validation; argparse choices already enforce thisThe
choices=on the argument rejects invalid values beforehandle()runs. Keep a single source of truth for errors.- if model_name not in model_map: - self.stdout.write( - self.style.ERROR("Invalid model name! Choose from: chapter, committee, project") - ) - return - models_to_process = model_map[model_name]
101-103: Use a supported output style;self.style.NOTICEmay not existDjango guarantees SUCCESS, WARNING, ERROR. Using NOTICE risks AttributeError.
- self.stdout.write( - self.style.NOTICE(f" -> No new leader records to create for {model_label}.") - ) + self.stdout.write( + self.style.WARNING(f" -> No new leader records to create for {model_label}.") + )
🧹 Nitpick comments (3)
backend/apps/owasp/Makefile (1)
29-31: Add an echo for consistency with other targetsMost OWASP targets emit a helpful echo line. Mirror that here for discoverability.
owasp-update-leaders: - @CMD="python manage.py owasp_update_leaders $(MATCH_MODEL)" $(MAKE) exec-backend-command + @echo "Updating leaders (EntityMember) for $(MATCH_MODEL)" + @CMD="python manage.py owasp_update_leaders $(MATCH_MODEL)" $(MAKE) exec-backend-commandbackend/apps/owasp/admin/entity_member.py (1)
41-46: Allow searching by entity_idHandy when you know the numeric ID. Use exact match for integer field.
search_fields = ( "member__login", "member__name", "description", + "=entity_id", )backend/apps/owasp/management/commands/owasp_update_leaders.py (1)
51-56: Optional: reduce memory and I/O with lazy iterationFor large user tables, avoid materializing all rows and precompute lowercase keys to reduce repeated .lower() calls in matching.
- all_users = list(User.objects.values("id", "login", "name")) - valid_users = [u for u in all_users if self.is_valid_user(u["login"], u["name"])] + valid_users = ( + { + "id": u["id"], + "login": u["login"], + "name": u["name"], + "_login_l": (u["login"] or "").lower(), + "_name_l": (u["name"] or "").lower(), + } + for u in User.objects.values("id", "login", "name").iterator() + if self.is_valid_user(u["login"], u["name"]) + )And in
find_single_user_matches, use the precomputed lowercase fields:- for user in users_list: - if user["login"].lower() == leader_lower or ( - user["name"] and user["name"].lower() == leader_lower - ): + for user in users_list: + if user["_login_l"] == leader_lower or (user["_name_l"] == leader_lower): return user @@ - for user in users_list: - score = fuzz.token_sort_ratio(leader_lower, user["login"].lower()) - if user["name"]: - score = max(score, fuzz.token_sort_ratio(leader_lower, user["name"].lower())) + for user in users_list: + score = fuzz.token_sort_ratio(leader_lower, user["_login_l"]) + if user["_name_l"]: + score = max(score, fuzz.token_sort_ratio(leader_lower, user["_name_l"]))
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
backend/apps/github/Makefile(0 hunks)backend/apps/owasp/Makefile(1 hunks)backend/apps/owasp/admin/entity_member.py(1 hunks)backend/apps/owasp/management/commands/owasp_update_leaders.py(1 hunks)backend/tests/apps/owasp/management/commands/owasp_update_leaders_test.py(1 hunks)
💤 Files with no reviewable changes (1)
- backend/apps/github/Makefile
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/tests/apps/owasp/management/commands/owasp_update_leaders_test.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/apps/owasp/admin/entity_member.py (6)
backend/apps/owasp/api/internal/queries/chapter.py (1)
chapter(14-19)backend/apps/owasp/models/chapter.py (1)
Chapter(21-217)backend/apps/owasp/api/internal/queries/committee.py (1)
committee(14-28)backend/apps/owasp/models/committee.py (1)
Committee(16-90)backend/apps/owasp/models/entity_member.py (1)
EntityMember(10-59)backend/apps/owasp/models/project.py (1)
Project(31-373)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (2)
backend/apps/owasp/management/commands/owasp_update_leaders.py (2)
5-5: No action needed:thefuzzis already declared
Confirmed thatthefuzz = "^0.22.1"is listed in backend/pyproject.toml (and present in poetry.lock), so the import in owasp_update_leaders.py will resolve successfully.
67-76: leaders_raw is defined and safely guarded in all target models
All three models—Chapter, Committee, and Project—inherit from RepositoryBasedEntityModel (in backend/apps/owasp/models/common.py), which declares:
leaders_raw = models.JSONField(verbose_name="Entity leaders list", default=list, blank=True, null=True)The loop’s precondition
if not entity.leaders_raw: continuecovers both
Noneand empty‐list cases, so no AttributeError can occur. No changes needed.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/apps/owasp/admin/entity_member.py (1)
15-46: Preload related FKs and restrict ContentTypes in the admin form
- Reduce N+1s on the changelist via list_select_related for member and entity_type.
- Limit content_type choices to allowed OWASP models (project, chapter, committee) to prevent accidental linkage to unrelated content types.
class EntityMemberAdmin(admin.ModelAdmin): """Admin for EntityMember records (generic link to any OWASP entity).""" + # Reduce N+1 queries on changelist + list_select_related = ("member", "entity_type") + @@ - ordering = ("member__name", "order",) + ordering = ("member__name", "order") + + def formfield_for_foreignkey(self, db_field, request, **kwargs): + if db_field.name == "entity_type": + kwargs.setdefault( + "queryset", + ContentType.objects.filter( + app_label="owasp", + model__in=("project", "chapter", "committee"), + ), + ) + return super().formfield_for_foreignkey(db_field, request, **kwargs)
🧹 Nitpick comments (3)
backend/apps/owasp/admin/entity_member.py (3)
18-19: Avoid conflicting widgets: don't use both autocomplete_fields and raw_id_fields for the same FKDjango will prioritize raw_id_fields over autocomplete on the same field, defeating the purpose of autocomplete. Pick one; given you already configured autocomplete, remove raw_id_fields for member.
- raw_id_fields = ("member",)Also applies to: 39-39
47-56: Harden entity link: guard against missing admin URL with NoReverseMatchIf a target model isn’t registered in admin (or the pattern differs), reverse can raise NoReverseMatch and break the changelist. Handle gracefully.
-from django.urls import reverse +from django.urls import NoReverseMatch, reverse @@ - if obj.entity: - link = reverse( - f"admin:{obj.entity_type.app_label}_{obj.entity_type.model}_change", - args=[obj.entity_id], - ) - return format_html('<a href="{}">{}</a>', link, obj.entity) + if obj.entity: + try: + link = reverse( + f"admin:{obj.entity_type.app_label}_{obj.entity_type.model}_change", + args=[obj.entity_id], + ) + return format_html('<a href="{}">{}</a>', link, obj.entity) + except NoReverseMatch: + # Fallback: just show the entity label without a link + return str(obj.entity) return "— No Associated Entity —"
62-86: Streamline get_search_results: fewer queries and less materialization
- Use get_for_models to fetch ContentTypes in one DB hit.
- Avoid wrapping values_list querysets in list(); Django can handle iterables for __in.
- Only set use_distinct when the OR filter contributes results.
- project_ids = Project.objects.filter( - Q(name__icontains=search_term) | Q(key__icontains=search_term) - ).values_list("pk", flat=True) + project_ids = Project.objects.filter( + Q(name__icontains=search_term) | Q(key__icontains=search_term) + ).values_list("pk", flat=True) - chapter_ids = Chapter.objects.filter( - Q(name__icontains=search_term) | Q(key__icontains=search_term) - ).values_list("pk", flat=True) + chapter_ids = Chapter.objects.filter( + Q(name__icontains=search_term) | Q(key__icontains=search_term) + ).values_list("pk", flat=True) - committee_ids = Committee.objects.filter( - Q(name__icontains=search_term) | Q(key__icontains=search_term) - ).values_list("pk", flat=True) + committee_ids = Committee.objects.filter( + Q(name__icontains=search_term) | Q(key__icontains=search_term) + ).values_list("pk", flat=True) - project_ct = ContentType.objects.get_for_model(Project) - chapter_ct = ContentType.objects.get_for_model(Chapter) - committee_ct = ContentType.objects.get_for_model(Committee) + cts = ContentType.objects.get_for_models(Project, Chapter, Committee) + project_ct = cts[Project] + chapter_ct = cts[Chapter] + committee_ct = cts[Committee] entity_match_query = ( - Q(entity_type=project_ct, entity_id__in=list(project_ids)) - | Q(entity_type=chapter_ct, entity_id__in=list(chapter_ids)) - | Q(entity_type=committee_ct, entity_id__in=list(committee_ids)) + Q(entity_type=project_ct, entity_id__in=project_ids) + | Q(entity_type=chapter_ct, entity_id__in=chapter_ids) + | Q(entity_type=committee_ct, entity_id__in=committee_ids) ) - queryset |= self.model.objects.filter(entity_match_query) - use_distinct = True + extra = self.model.objects.filter(entity_match_query) + if extra.exists(): + queryset |= extra + use_distinct = True
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
backend/apps/owasp/admin/entity_member.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/apps/owasp/admin/entity_member.py (3)
backend/apps/owasp/api/internal/queries/chapter.py (1)
chapter(14-19)backend/apps/owasp/api/internal/queries/committee.py (1)
committee(14-28)backend/apps/owasp/models/entity_member.py (1)
EntityMember(10-59)
🪛 Ruff (0.12.2)
backend/apps/owasp/admin/entity_member.py
45-45: Trailing comma prohibited
Remove trailing comma
(COM819)
🪛 GitHub Actions: Run CI/CD
backend/apps/owasp/admin/entity_member.py
[error] 42-42: Ruff: pre-commit hook failed (exit code 1); fixed by removing trailing comma in the ordering tuple in backend/apps/owasp/admin/entity_member.py.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CodeQL (javascript-typescript)
| models_to_process = model_map[model_name] | ||
|
|
||
| all_users = list(User.objects.values("id", "login", "name")) | ||
| valid_users = [u for u in all_users if self.is_valid_user(u["login"], u["name"])] |
There was a problem hiding this comment.
What if there is more than one person with the same name?
|
|
||
| for user in users_list: | ||
| score = fuzz.token_sort_ratio(leader_lower, user["login"].lower()) | ||
| if user["name"]: |
There was a problem hiding this comment.
Same here -- you don't handle cases if more than one person with the same name exists in the database. You always return the first match that may or may not be the right one (the name is matched, but the real leader with the same name could be the next one).
There was a problem hiding this comment.
Please restore bulk approval action functionality.
| entity_type = models.ForeignKey(ContentType, on_delete=models.CASCADE) | ||
| is_reviewed = models.BooleanField( | ||
| default=False, | ||
| help_text="Indicates if the membership is verified", |
There was a problem hiding this comment.
| help_text="Indicates if the membership is verified", | |
| help_text="Indicates if the membership is reviwed", |
| default=False, | ||
| help_text="Indicates if the membership is verified", | ||
| ) | ||
| kind = models.CharField( |
There was a problem hiding this comment.
Let's also add is_active boolean field to separate the leader status from the relation review status (e.g. a reviewed relation cannot be approved if it's false positive fuzzy match).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
backend/apps/owasp/admin/entity_member.py (4)
18-18: Prefetch related FKs in changelist to avoid N+1 queries.Select member and entity_type to cut per-row lookups on the changelist. This mirrors the earlier suggestion.
autocomplete_fields = ("member",) + list_select_related = ("member", "entity_type")
90-92: Restrict editable ContentTypes to OWASP entities (Project/Chapter/Committee).Prevents accidental linking to unrelated models in the admin. Aligns with earlier recommendation.
- admin.site.register(EntityMember, EntityMemberAdmin) + + def formfield_for_foreignkey(self, db_field, request, **kwargs): + if db_field.name == "entity_type": + kwargs.setdefault( + "queryset", + ContentType.objects.filter( + app_label="owasp", + model__in=("project", "chapter", "committee"), + ), + ) + return super().formfield_for_foreignkey(db_field, request, **kwargs) + +admin.site.register(EntityMember, EntityMemberAdmin)
45-45: Resolved: trailing comma linter issue (COM819) already fixed.The earlier Ruff complaint is addressed; no trailing comma remains.
45-46: Restore bulk approval admin actions (requested).Bring back the bulk “mark reviewed/unreviewed” actions for is_reviewed to match previous admin behavior.
- ordering = ("member__name", "order") + ordering = ("entity_type", "entity_id", "order", "member__login") + actions = ("mark_reviewed", "mark_unreviewed")Add the actions:
def get_search_results(self, request, queryset, search_term): """Get search results from entity name or key.""" queryset, use_distinct = super().get_search_results(request, queryset, search_term) @@ return queryset, use_distinct + + @admin.action(description="Mark selected members as reviewed") + def mark_reviewed(self, request, queryset): + updated = queryset.update(is_reviewed=True) + self.message_user(request, f"{updated} membership(s) marked as reviewed.") + + @admin.action(description="Mark selected members as unreviewed") + def mark_unreviewed(self, request, queryset): + updated = queryset.update(is_reviewed=False) + self.message_user(request, f"{updated} membership(s) marked as unreviewed.")
🧹 Nitpick comments (3)
backend/apps/owasp/admin/entity_member.py (3)
45-45: Group changelist by entity for better UX; stabilize member ordering.Sort by entity first, then order, then member login. This makes the list more coherent when browsing memberships per entity.
- ordering = ("member__name", "order") + ordering = ("entity_type", "entity_id", "order", "member__login")
40-44: Enable direct ID search by entity_id.Small usability win to quickly jump to memberships by known entity PK.
search_fields = ( "member__login", "member__name", "description", + "entity_id", )
47-56: Harden entity link: guard against NoReverseMatch and unregistered admins.If a ContentType slips through (or admin isn’t registered), reverse() will raise NoReverseMatch. Fall back to plain text.
- @admin.display(description="Entity", ordering="entity_type") - def entity(self, obj): + @admin.display(description="Entity", ordering="entity_type") + def entity(self, obj): """Return entity link.""" if obj.entity: - link = reverse( - f"admin:{obj.entity_type.app_label}_{obj.entity_type.model}_change", - args=[obj.entity_id], - ) - return format_html('<a href="{}">{}</a>', link, obj.entity) + try: + link = reverse( + f"admin:{obj.entity_type.app_label}_{obj.entity_type.model}_change", + args=[obj.entity_id], + ) + return format_html('<a href="{}">{}</a>', link, obj.entity) + except NoReverseMatch: + return str(obj.entity) return "— No Associated Entity —"Also update imports:
-from django.urls import reverse +from django.urls import NoReverseMatch, reverse
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
backend/apps/owasp/admin/entity_member.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/apps/owasp/admin/entity_member.py (3)
backend/apps/owasp/api/internal/queries/chapter.py (1)
chapter(14-19)backend/apps/owasp/api/internal/queries/committee.py (1)
committee(14-28)backend/apps/owasp/models/entity_member.py (1)
EntityMember(10-59)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
backend/apps/owasp/migrations/0048_entitymember.py (2)
14-82: Backfill existing leaders and drop the old M2M field (required by issue #2017).This migration only adds EntityMember. Per the PR objective, the old leaders M2M must be removed and existing data should be preserved via a data migration. Without a backfill + RemoveField, you’ll have two parallel sources of truth and lingering runtime usages.
Run this to confirm the old M2M removal and detect any lingering usages:
#!/bin/bash set -euo pipefail echo "Searching for RemoveField on 'leaders' in migrations..." rg -nP -C2 'migrations\.RemoveField\([^)]*name\s*=\s*[\'"]leaders[\'"]' backend/apps/owasp/migrations || true echo echo "Searching for runtime usages of '.leaders' (excluding migrations/tests)..." rg -nP -n -C2 --glob '!**/migrations/**' --glob '!**/test*/**' '\.leaders\b' backend || trueSuggested approach:
- Add a data migration to copy each existing leaders relation to owasp.EntityMember with:
- entity_type from ContentType for Chapter/Committee/Project
- entity_id as the entity PK
- member_id as the user PK
- kind=leader
- is_reviewed=True (recommended, since these were curated M2M relations)
- order as a simple incremental sequence (e.g., (idx+1)*10)
- Add a follow-up schema migration to RemoveField(name='leaders') from Chapter, Committee, and Project.
Example data migration (new file, 0049_backfill_leaders_to_entity_member.py):
from django.db import migrations def backfill_leaders(apps, schema_editor): ContentType = apps.get_model("contenttypes", "ContentType") EntityMember = apps.get_model("owasp", "EntityMember") Chapter = apps.get_model("owasp", "Chapter") Committee = apps.get_model("owasp", "Committee") Project = apps.get_model("owasp", "Project") def backfill_for(model): ct = ContentType.objects.get_for_model(model) to_create = [] for entity in model.objects.all().iterator(): # If the field has already been removed in some env, skip gracefully. if not hasattr(entity, "leaders"): continue # No guaranteed ordering on M2M; just use insertion order. for idx, user in enumerate(entity.leaders.all()): to_create.append( EntityMember( entity_type=ct, entity_id=entity.pk, member_id=user.pk, kind="leader", is_reviewed=True, order=(idx + 1) * 10, ) ) if to_create: EntityMember.objects.bulk_create(to_create, ignore_conflicts=True) for m in (Chapter, Committee, Project): backfill_for(m) class Migration(migrations.Migration): dependencies = [ ("owasp", "0048_entitymember"), ("contenttypes", "0002_remove_content_type_name"), ] operations = [ migrations.RunPython(backfill_leaders, migrations.RunPython.noop), ]Then a schema migration (0050_remove_leaders.py):
from django.db import migrations class Migration(migrations.Migration): dependencies = [ ("owasp", "0049_backfill_leaders_to_entity_member"), ] operations = [ migrations.RemoveField(model_name="chapter", name="leaders"), migrations.RemoveField(model_name="committee", name="leaders"), migrations.RemoveField(model_name="project", name="leaders"), ]
30-31: Align entity_id with BigAutoField PKs: use PositiveBigIntegerField.If your entities use BigAutoField (Django default), PositiveIntegerField can overflow. Switch to PositiveBigIntegerField here (and in the model) to avoid type mismatch.
Apply:
- ("entity_id", models.PositiveIntegerField()), + ("entity_id", models.PositiveBigIntegerField()),Note: If this migration has already been applied somewhere, don’t edit it in place—add a new migration with AlterField instead to keep environments consistent.
backend/apps/owasp/management/commands/owasp_update_leaders.py (2)
43-47: Remove unreachable validation; argparse choices already enforce this.The choices= argument rejects invalid model_name before handle() runs. This block won’t execute.
Apply:
- if model_name not in model_map: - self.stdout.write( - self.style.ERROR("Invalid model name! Choose from: chapter, committee, project") - ) - return
100-102: Use a supported style; self.style.NOTICE can raise AttributeError at runtime.Replace with WARNING or SUCCESS to avoid a crash.
Apply:
- self.stdout.write( - self.style.NOTICE(f" -> No new leader records to create for {model_label}.") - ) + self.stdout.write( + self.style.WARNING(f" -> No new leader records to create for {model_label}.") + )
🧹 Nitpick comments (4)
backend/apps/owasp/management/commands/owasp_update_leaders.py (4)
104-106: Don’t exclude users without a display name.Many GitHub users omit the name field; current logic filters them out even when login matches. Allow login-only users.
Apply:
- return len(login) >= ID_MIN_LENGTH and len(name or "") >= ID_MIN_LENGTH + return len(login) >= ID_MIN_LENGTH
113-114: Trim whitespace before lowercasing the leader name.Avoid false negatives due to stray spaces.
Apply:
- leader_lower = leader_name.lower() + leader_lower = leader_name.strip().lower()
71-87: Confirm intended behavior for ambiguous matches (multiple users per leader).find_all_user_matches can return multiple users; you then create one EntityMember per match. If these are meant as “candidates” for manual review (is_reviewed=False), this is fine. If not, consider selecting a single best match (e.g., highest fuzz score) to avoid noisy memberships.
If you want to keep multiple suggestions but help reviewers, consider writing the match strategy to the description, e.g., “Matched ‘{leader_name}’ by fuzzy ({score})”.
1-1: Docstring nit: this command doesn’t handle “slack members.”Tighten the wording to reflect current behavior.
Apply:
-"""A command to perform fuzzy and exact matching of leaders/slack members with User model.""" +"""A command to perform fuzzy and exact matching of leader names to GitHub Users and create EntityMember records."""
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
backend/apps/owasp/management/commands/owasp_update_leaders.py(1 hunks)backend/apps/owasp/migrations/0048_entitymember.py(1 hunks)backend/apps/owasp/models/entity_member.py(1 hunks)backend/tests/apps/owasp/management/commands/owasp_update_leaders_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/tests/apps/owasp/management/commands/owasp_update_leaders_test.py
- backend/apps/owasp/models/entity_member.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/apps/owasp/management/commands/owasp_update_leaders.py (5)
backend/apps/github/api/internal/queries/user.py (1)
user(40-56)backend/apps/owasp/api/internal/queries/chapter.py (1)
chapter(14-19)backend/apps/owasp/models/chapter.py (1)
Chapter(21-217)backend/apps/owasp/models/entity_member.py (2)
EntityMember(10-63)MemberKind(13-14)backend/apps/owasp/models/project.py (1)
Project(31-373)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (1)
backend/apps/owasp/management/commands/owasp_update_leaders.py (1)
89-97: LGTM: bulk_create with ignore_conflicts is appropriate here.Efficient insert plus idempotency via unique_together is a good fit for repeated runs.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
backend/apps/owasp/admin/entity_member.py (2)
18-19: Optimize changelist queries and restrict editable ContentTypes
- Add list_select_related to cut N+1 queries for member/content type on the changelist.
- Restrict entity_type choices to OWASP entities to avoid accidental linking to unrelated models.
Apply these diffs:
actions = ("approve_members",) autocomplete_fields = ("member",) + list_select_related = ("member", "entity_type")ordering = ("member__name", "order") + def formfield_for_foreignkey(self, db_field, request, **kwargs): + if db_field.name == "entity_type": + kwargs.setdefault( + "queryset", + ContentType.objects.filter( + app_label="owasp", model__in=("project", "chapter", "committee") + ), + ) + return super().formfield_for_foreignkey(db_field, request, **kwargs)Note: The field name is entity_type (not content_type) in this model. Also, computing obj.entity in list_display still triggers a per-row fetch; this change won’t eliminate that, but it does reduce two other FKs. If needed, we can switch the display to a non-dereferencing label to avoid the GFK hit.
Also applies to: 46-47
40-40: Remove raw_id_fields: conflicts with autocomplete_fields and will break adminDjango forbids using raw_id_fields and autocomplete_fields on the same field. This will raise ImproperlyConfigured at startup.
Apply this diff:
- raw_id_fields = ("member",)
🧹 Nitpick comments (2)
backend/apps/owasp/admin/entity_member.py (2)
73-96: Avoid materializing PK lists; use subqueries and get_for_models for minor efficiency gains
- Using list() forces evaluation and builds large in-memory lists. Passing the QuerySet directly to __in uses a subquery and scales better.
- get_for_models can resolve multiple ContentTypes in one shot (and benefits from caching).
Apply this diff:
- project_ids = Project.objects.filter( - Q(name__icontains=search_term) | Q(key__icontains=search_term) - ).values_list("pk", flat=True) + project_ids = Project.objects.filter( + Q(name__icontains=search_term) | Q(key__icontains=search_term) + ).values_list("pk", flat=True) - chapter_ids = Chapter.objects.filter( - Q(name__icontains=search_term) | Q(key__icontains=search_term) - ).values_list("pk", flat=True) + chapter_ids = Chapter.objects.filter( + Q(name__icontains=search_term) | Q(key__icontains=search_term) + ).values_list("pk", flat=True) - committee_ids = Committee.objects.filter( - Q(name__icontains=search_term) | Q(key__icontains=search_term) - ).values_list("pk", flat=True) + committee_ids = Committee.objects.filter( + Q(name__icontains=search_term) | Q(key__icontains=search_term) + ).values_list("pk", flat=True) - project_ct = ContentType.objects.get_for_model(Project) - chapter_ct = ContentType.objects.get_for_model(Chapter) - committee_ct = ContentType.objects.get_for_model(Committee) + cts = ContentType.objects.get_for_models(Project, Chapter, Committee) + project_ct = cts[Project] + chapter_ct = cts[Chapter] + committee_ct = cts[Committee] entity_match_query = ( - Q(entity_type=project_ct, entity_id__in=list(project_ids)) - | Q(entity_type=chapter_ct, entity_id__in=list(chapter_ids)) - | Q(entity_type=committee_ct, entity_id__in=list(committee_ids)) + Q(entity_type=project_ct, entity_id__in=project_ids) + | Q(entity_type=chapter_ct, entity_id__in=chapter_ids) + | Q(entity_type=committee_ct, entity_id__in=committee_ids) )
29-35: Surface is_active and add filters to improve moderation workflowAdding is_active to the list and filters gives quick visibility into approval status. Filtering by entity_type also helps when triaging members per entity kind.
Apply these diffs:
list_display = ( "member", "entity", "kind", "is_reviewed", + "is_active", "order", )list_filter = ( "kind", "is_reviewed", + "is_active", + "entity_type", )Also applies to: 36-39
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
backend/apps/owasp/admin/entity_member.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/apps/owasp/admin/entity_member.py (6)
backend/apps/owasp/api/internal/queries/chapter.py (1)
chapter(14-19)backend/apps/owasp/models/chapter.py (1)
Chapter(21-217)backend/apps/owasp/api/internal/queries/committee.py (1)
committee(14-28)backend/apps/owasp/models/committee.py (1)
Committee(16-90)backend/apps/owasp/models/entity_member.py (1)
EntityMember(10-63)backend/apps/owasp/models/project.py (1)
Project(31-373)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run frontend e2e tests
🔇 Additional comments (1)
backend/apps/owasp/admin/entity_member.py (1)
48-56: LGTM: bulk approval action restoredapprove_members cleanly bulk-updates is_reviewed and is_active and provides clear feedback. Matches the requested functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
backend/apps/owasp/migrations/0048_entitymember.py (1)
14-87: Backfill existing leaders and remove the old M2M in follow-up migrations.This migration creates EntityMember but does not migrate existing M2M leaders data nor remove the old field(s). Requirement in #2017: “cleanly remove the old leaders M2M field” and preserve existing associations.
Proposed plan:
- Add a data migration (RunPython) to copy existing leaders into EntityMember with role='leader', is_reviewed=False, is_active=False, order derived from existing ordering if available.
- Add a subsequent schema migration to RemoveField(name='leaders') from all affected models (Chapter, Committee, Project), after the backfill.
Example data migration (new file, e.g., 0049_backfill_leaders_to_entity_member.py):
from django.db import migrations from django.contrib.contenttypes.models import ContentType def backfill(apps, schema_editor): EntityMember = apps.get_model("owasp", "EntityMember") Chapter = apps.get_model("owasp", "Chapter") Committee = apps.get_model("owasp", "Committee") Project = apps.get_model("owasp", "Project") def migrate_for(model_cls): ct = ContentType.objects.get_for_model(model_cls) batch = [] for entity in model_cls.objects.all().iterator(): # If there was an explicit order, replicate it; else enumerate for idx, user in enumerate(entity.leaders.all()): # old M2M batch.append(EntityMember( entity_type=ct, entity_id=entity.pk, member_id=user.pk, role="leader", is_reviewed=False, is_active=False, order=(idx + 1) * 10, )) if batch: EntityMember.objects.bulk_create(batch, ignore_conflicts=True) migrate_for(Chapter) migrate_for(Committee) migrate_for(Project) def noop(apps, schema_editor): pass class Migration(migrations.Migration): dependencies = [ ("owasp", "0048_entitymember"), ] operations = [ migrations.RunPython(backfill, noop), ]Then remove old fields (new file, e.g., 0050_remove_leaders_m2m.py):
from django.db import migrations class Migration(migrations.Migration): dependencies = [ ("owasp", "0049_backfill_leaders_to_entity_member"), ] operations = [ migrations.RemoveField(model_name="chapter", name="leaders"), migrations.RemoveField(model_name="committee", name="leaders"), migrations.RemoveField(model_name="project", name="leaders"), ]I can draft these migrations precisely once the model module paths are confirmed.
backend/apps/owasp/admin/entity_member.py (1)
44-44: Remove raw_id_fields; conflicts with autocomplete_fields and breaks admin.Django forbids using raw_id_fields and autocomplete_fields on the same field; this will raise ImproperlyConfigured.
- raw_id_fields = ("member",)backend/apps/owasp/management/commands/owasp_update_leaders.py (2)
43-47: Redundant validation: argparse choices already enforce valid model_name.The block is unreachable and can be removed.
- if model_name not in model_map: - self.stdout.write( - self.style.ERROR("Invalid model name! Choose from: chapter, committee, project") - ) - return
98-100: Use a supported style; NOTICE is not standard in Django.Replace self.style.NOTICE with WARNING or SUCCESS to avoid AttributeError.
- self.stdout.write( - self.style.NOTICE(f" -> No member records found for {model_label}.") - ) + self.stdout.write( + self.style.WARNING(f" -> No member records found for {model_label}.") + )
🧹 Nitpick comments (8)
docker-compose/local.yaml (2)
68-68: Renaming the DB volume will create a fresh Postgres data dir (likely unintended).Switching to db-data-leaders will cause docker-compose to provision a brand-new volume, wiping local data/state. If that’s not intentional, keep the original volume name.
Apply this diff to retain the existing DB volume and avoid data loss:
- - db-data-leaders:/var/lib/postgresql/data + - db-data:/var/lib/postgresql/dataIf the intent is to force a fresh DB for local dev, please add a note to docs/CHANGELOG so contributors aren’t surprised. Also consider removing the old volume definition to avoid confusion (see below).
117-117: Unused volume definition (db-data-leaders) after reverting mapping.If you keep using db-data for the DB, remove the unused db-data-leaders declaration to prevent confusion.
db-data: - db-data-leaders:backend/apps/owasp/migrations/0048_entitymember.py (1)
75-85: Consider adding a composite index including role for common leader queries.Queries will often filter by (entity_type, entity_id, role='leader') and order by order. The current index on (entity_type, entity_id) helps, but adding role (and optionally order) can reduce filtering/ordering work.
options={ "verbose_name_plural": "Entity members", "db_table": "owasp_entity_members", "indexes": [ models.Index( fields=["entity_type", "entity_id"], name="owasp_entit_entity__dda41b_idx" ), models.Index(fields=["member"], name="owasp_entit_member__6e516f_idx"), + models.Index(fields=["entity_type", "entity_id", "role", "order"], name="owasp_entit_entity_role_order_idx"), ], "unique_together": {("entity_type", "entity_id", "member", "role")}, },backend/apps/owasp/admin/entity_member.py (1)
15-39: Reduce N+1 queries and restrict selectable ContentTypes.
- Select related FKs for the changelist to cut queries.
- Limit entity_type choices to OWASP models (project, chapter, committee) to prevent linking arbitrary content types.
class EntityMemberAdmin(admin.ModelAdmin): """Admin for EntityMember records (generic link to any OWASP entity).""" + list_select_related = ("member", "entity_type") actions = ("approve_members",) autocomplete_fields = ("member",) fields = ( "entity_type", "entity_id", "member", "role", "order", "is_active", "is_reviewed", "description", ) list_display = ( "member", "entity", "owasp_url", "role", "is_active", "is_reviewed", "order", ) list_filter = ( "role", "is_active", "is_reviewed", ) @@ ordering = ("member__name", "order") + + def formfield_for_foreignkey(self, db_field, request, **kwargs): + if db_field.name == "entity_type": + kwargs.setdefault( + "queryset", + ContentType.objects.filter(app_label="owasp", model__in=("project", "chapter", "committee")), + ) + return super().formfield_for_foreignkey(db_field, request, **kwargs) + + def get_queryset(self, request): + qs = super().get_queryset(request) + return qs.select_related("member", "entity_type")backend/apps/owasp/models/entity_member.py (2)
37-40: Declare GenericForeignKey after its backing fields for clarity and lint-friendliness.No functional change, but it aligns with Django docs and improves readability.
- entity = GenericForeignKey("entity_type", "entity_id") - entity_id = models.PositiveBigIntegerField() - entity_type = models.ForeignKey(ContentType, on_delete=models.CASCADE) + entity_type = models.ForeignKey(ContentType, on_delete=models.CASCADE) + entity_id = models.PositiveBigIntegerField() + entity = GenericForeignKey("entity_type", "entity_id")
25-28: Optional: add composite index including role and order to speed typical leader lookups.If you frequently query leaders for an entity and order them, this helps.
indexes = [ models.Index(fields=["entity_type", "entity_id"]), models.Index(fields=["member"]), + models.Index(fields=["entity_type", "entity_id", "role", "order"]), ]backend/apps/owasp/management/commands/owasp_update_leaders.py (2)
102-104: Loosen user validity: allow matching by login even if name is missing.Requiring both login and name ≥ 2 chars drops legitimate users without a name. Prefer login-based validity and treat name as optional.
def is_valid_user(self, login, name): """Check if GitHub user meets minimum requirements.""" - return len(login) >= ID_MIN_LENGTH and len(name or "") >= ID_MIN_LENGTH + return len(login or "") >= ID_MIN_LENGTH
106-129: Limit to the best match to avoid duplicate member entries per leader.Currently, all matches above threshold are created, which can add multiple members for a single leader name. Prefer the single best match by score (keep exact matches prioritized). This still sets is_reviewed=False, allowing manual approval.
def find_all_user_matches(self, leader_name, users_list, threshold): """Find user matches for a list of raw leader names.""" if not leader_name: return [] leader_lower = leader_name.lower() - matches = [ - user - for user in users_list - if user["login"].lower() == leader_lower - or (user["name"] and user["name"].lower() == leader_lower) - ] + # Prefer exact matches (login or name) + exact_matches = [ + user + for user in users_list + if user["login"].lower() == leader_lower + or (user.get("name") and user["name"].lower() == leader_lower) + ] - if not matches: - for user in users_list: - score = fuzz.token_sort_ratio(leader_lower, user["login"].lower()) - if user["name"]: - score = max(score, fuzz.token_sort_ratio(leader_lower, user["name"].lower())) - - if score >= threshold: - matches.append(user) - - return list({user["id"]: user for user in matches}.values()) + if exact_matches: + # If there are multiple exact matches on name, just return one to avoid duplicates. + return [exact_matches[0]] + + # Fuzzy match: pick the single best scoring user >= threshold + best_user = None + best_score = -1 + for user in users_list: + score = fuzz.token_sort_ratio(leader_lower, user["login"].lower()) + if user.get("name"): + score = max(score, fuzz.token_sort_ratio(leader_lower, user["name"].lower())) + if score >= threshold and score > best_score: + best_score = score + best_user = user + + return [best_user] if best_user else []If multiple legitimate leaders share the same name, relying on manual review (is_reviewed=False, is_active defaults False) keeps safety. If you prefer keeping all candidates, consider limiting to top-N and surfacing that count in the output.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
backend/apps/owasp/admin/entity_member.py(1 hunks)backend/apps/owasp/admin/mixins.py(2 hunks)backend/apps/owasp/management/commands/owasp_update_leaders.py(1 hunks)backend/apps/owasp/migrations/0048_entitymember.py(1 hunks)backend/apps/owasp/models/common.py(2 hunks)backend/apps/owasp/models/entity_member.py(1 hunks)backend/tests/apps/owasp/management/commands/owasp_update_leaders_test.py(1 hunks)docker-compose/local.yaml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/tests/apps/owasp/management/commands/owasp_update_leaders_test.py
- backend/apps/owasp/models/common.py
- backend/apps/owasp/admin/mixins.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
|
…WASP#2063) * removed leader and suggested leader, introduced entity_member model * fixed tests * updated code * code rabbit suggestion * updated tests * Update code * udpated code * Update code * Run pre-commit * added fields updated tests * added approve_members * Update code * Update docker-compose/local.yaml --------- Co-authored-by: Arkadii Yakovets <[email protected]>
Proposed change
Resolves #2017
Add the PR description here.
Checklist
make check-testlocally; all checks and tests passed.