Sync OWASP Awards data and integrate with user profiles #2228
Conversation
Summary by CodeRabbit
WalkthroughAdds an Award model and admin, a management command to import OWASP awards from YAML and match users, integrates award sync and WASPY badge sync into Makefiles and the badge updater, and adds unit tests for the import command. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (9)
✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (9)
backend/apps/nest/management/commands/nest_update_badges.py (3)
93-97: Consider DB indexes for award lookups.Filters on awards by (category, is_reviewed) will be frequent here; add a composite index to Award to keep this fast at scale.
Example (in Award model):
indexes = [ models.Index(fields=["-year"], name="award_year_desc_idx"), models.Index(fields=["category", "is_reviewed"], name="award_cat_reviewed_idx"), ]
112-125: Avoid unnecessary writes when deactivating.Limit the UPDATE to active rows only.
- ).update(is_active=False) + , is_active=True).update(is_active=False)
77-128: Optional: make the method idempotent and efficient with bulk ops + transaction.Wrap in a transaction and use bulk_create + update() to cut N+1 queries.
Example outline:
from django.db import transaction @transaction.atomic def update_waspy_award_badge(self): ... user_ids = list(waspy_award_users.values_list("id", flat=True)) existing = set(UserBadge.objects.filter(badge=badge, user_id__in=user_ids).values_list("user_id", flat=True)) missing = [UserBadge(user_id=uid, badge=badge, is_active=True) for uid in user_ids if uid not in existing] if missing: UserBadge.objects.bulk_create(missing, ignore_conflicts=True) UserBadge.objects.filter(badge=badge, user_id__in=user_ids).update(is_active=True) ...backend/apps/owasp/admin/award.py (1)
8-16: Tighten admin UX and performance (select_related, ordering, quick review actions).Avoid N+1 on user, add sensible ordering/date drill-down, and provide actions to mark reviewed/unreviewed.
Apply:
@admin.register(Award) class AwardAdmin(admin.ModelAdmin): """Award admin.""" - list_display = ("name", "category", "year", "user", "is_reviewed", "created_at") - list_filter = ("category", "year", "is_reviewed", "created_at") - search_fields = ("name", "category", "user__name", "user__login") - readonly_fields = ("created_at", "updated_at") - raw_id_fields = ("user",) + list_display = ("name", "category", "year", "user", "is_reviewed", "created_at") + list_filter = ("category", "year", "is_reviewed", "created_at") + search_fields = ("name", "category", "user__name", "user__login") + readonly_fields = ("created_at", "updated_at") + raw_id_fields = ("user",) + list_select_related = ("user",) + ordering = ("-year", "category", "name") + date_hierarchy = "created_at" + actions = ("mark_reviewed", "mark_unreviewed") + + @admin.action(description="Mark selected awards as reviewed") + def mark_reviewed(self, request, queryset): + queryset.update(is_reviewed=True) + + @admin.action(description="Mark selected awards as unreviewed") + def mark_unreviewed(self, request, queryset): + queryset.update(is_reviewed=False)backend/tests/apps/owasp/management/commands/owasp_sync_awards_test.py (1)
14-38: Add edge-case and idempotency tests.Cover empty/invalid YAML, string-only winners, and double-run idempotency to prevent regressions.
You can extend with:
@patch("apps.owasp.management.commands.owasp_sync_awards.get_repository_file_content") def test_sync_awards_is_idempotent(self, mock_get_content): mock_get_content.return_value = """ - title: WASPY type: category - title: Project Person of the Year type: award category: WASPY year: 2024 winners: - name: Test Winner """ Command().handle() Command().handle() assert Award.objects.count() == 1 @patch("apps.owasp.management.commands.owasp_sync_awards.get_repository_file_content") def test_sync_awards_handles_string_winner(self, mock_get_content): mock_get_content.return_value = """ - title: WASPY type: category - title: Community Hero type: award category: WASPY year: 2023 winners: - Alice Example """ Command().handle() a = Award.objects.get(name="Community Hero - Alice Example (2023)") assert a.description == "" @patch("apps.owasp.management.commands.owasp_sync_awards.get_repository_file_content") def test_sync_awards_handles_empty_yaml(self, mock_get_content): mock_get_content.return_value = "" Command().handle() assert Award.objects.count() == 0backend/apps/owasp/models/award.py (4)
5-5: Makebulk_savea classmethod and coercefieldsto listRemoves the need for
# type: ignore[override], keeps API consistent, and ensuresbulk_updatereceives a list.Apply:
+from typing import Sequence @@ - @staticmethod - def bulk_save( # type: ignore[override] - awards: list, - fields: tuple[str, ...] | None = None, - ) -> None: + @classmethod + def bulk_save( + cls, + awards: list["Award"], + fields: Sequence[str] | None = None, + ) -> None: @@ - BulkSaveModel.bulk_save(Award, awards, fields=fields) + BulkSaveModel.bulk_save(cls, awards, fields=list(fields) if fields else None)Also applies to: 44-60
13-19: Set default ordering to match index and admin expectationsHandy for admin list and common queries.
Apply:
class Meta: db_table = "owasp_awards" indexes = [ models.Index(fields=["-year"], name="award_year_desc_idx"), ] + ordering = ["-year", "name"] verbose_name_plural = "Awards"
5-5: Validateyearand use a positive integer fieldPrevents invalid years (e.g., negatives, 0).
Apply:
+from django.core.validators import MinValueValidator @@ - year = models.IntegerField(verbose_name="Year") + year = models.PositiveSmallIntegerField(verbose_name="Year", validators=[MinValueValidator(1998)])Also applies to: 23-23
49-58: Document that the input list is cleared in-place
BulkSaveModel.bulk_saveclears the provided list. Callers should know.Apply:
"""Bulk save awards. Args: awards (list): A list of Award instances to be saved. fields (tuple, optional): A tuple of fields to update during the bulk save. Returns: None """ + # Note: This method clears the input `awards` list in-place. BulkSaveModel.bulk_save(Award, awards, fields=fields)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
backend/Makefile(2 hunks)backend/apps/nest/management/commands/nest_update_badges.py(3 hunks)backend/apps/owasp/Makefile(1 hunks)backend/apps/owasp/admin/__init__.py(1 hunks)backend/apps/owasp/admin/award.py(1 hunks)backend/apps/owasp/management/commands/owasp_sync_awards.py(1 hunks)backend/apps/owasp/models/__init__.py(1 hunks)backend/apps/owasp/models/award.py(1 hunks)backend/tests/apps/owasp/management/commands/owasp_sync_awards_test.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
backend/apps/owasp/models/__init__.py (1)
backend/apps/owasp/models/award.py (1)
Award(10-59)
backend/apps/owasp/admin/__init__.py (1)
backend/apps/owasp/admin/award.py (1)
AwardAdmin(9-16)
backend/apps/owasp/admin/award.py (1)
backend/apps/owasp/models/award.py (1)
Award(10-59)
backend/tests/apps/owasp/management/commands/owasp_sync_awards_test.py (1)
backend/apps/owasp/models/award.py (1)
Award(10-59)
backend/apps/nest/management/commands/nest_update_badges.py (3)
backend/apps/nest/models/badge.py (1)
Badge(10-41)backend/apps/github/models/user.py (1)
User(19-159)backend/apps/nest/models/user_badge.py (1)
UserBadge(10-50)
backend/apps/owasp/management/commands/owasp_sync_awards.py (2)
backend/apps/github/utils.py (1)
get_repository_file_content(60-79)backend/apps/owasp/models/award.py (1)
Award(10-59)
backend/apps/owasp/models/award.py (1)
backend/apps/common/models.py (2)
BulkSaveModel(10-34)TimestampedModel(37-46)
🔇 Additional comments (10)
backend/apps/nest/management/commands/nest_update_badges.py (3)
14-14: Constant name LGTM.Good, clear, and consistent with existing naming.
26-26: Include WASPY badge update in the flow — good addition.Keeps the job cohesive with other badge updates.
80-87: Badge creation defaults look reasonable.Name/description/icon/weight are sensible.
backend/apps/owasp/models/__init__.py (1)
1-1: Exporting Award at package level — good.Keeps imports uniform across the app.
backend/apps/owasp/admin/__init__.py (1)
7-7: Admin import OK.Assuming Award is registered in admin/award.py, this ensures it’s loaded.
backend/Makefile (2)
110-115: Good call: run badge updates after data enrichment but before indexing.This ordering ensures badges (including WASPY winners) are reflected in the search index. Looks correct given update-data already runs the awards sync first.
129-143: Add awards sync to update-data — sequencing looks right.Placing owasp-sync-awards before slack-sync-data aligns with the daily pipeline and keeps badge updates in sync later in sync-data.
backend/apps/owasp/Makefile (1)
67-70: Make target LGTM.Consistent with existing patterns and reuses exec-backend-command correctly.
backend/apps/owasp/models/award.py (2)
40-42: LGTM:__str__is concise and usefulGood human-readable representation.
15-17: Confirm DB support for descending indexes
Ensure your environments use Django ≥ 1.11 (addedDESCsupport), PostgreSQL ≥ 8.3 (per-column ASC/DESC indexes) and SQLite ≥ 3.7.10 (default DESC ordering; or legacy_file_format disabled) to guarantee true descending indexes.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
backend/apps/nest/management/commands/nest_update_badges.py (1)
46-54: Reactivation path fixed — good catch and implementation.Switching to exclude(..., badges__is_active=True) correctly includes users with inactive badge rows, so they’re reactivated. This addresses the prior review note.
Also applies to: 98-111
backend/apps/owasp/models/award.py (1)
18-23: Uniqueness fix looks right.Moving from name unique=True to a scoped UniqueConstraint on (category, name, year) matches repeated yearly awards.
backend/apps/owasp/management/commands/owasp_sync_awards.py (2)
22-36: YAML fetch/parse guards — good.Graceful handling for empty/invalid YAML and explicit bulk_save fields aligns with idempotent sync.
76-83: Don’t override human-reviewed records (even if user is None).If is_reviewed is True, skip any automated rematching to respect manual decisions.
- # Only set user if not already reviewed - if not (award.user and award.is_reviewed): - user = self._match_user(winner_name) + # Respect manual review: never modify user when reviewed + if not award.is_reviewed: + user = self._match_user(winner_name, winner_login=winner_login) if user: award.user = user else: logger.warning("Could not match user for award winner: %s", winner_name)
🧹 Nitpick comments (5)
backend/apps/nest/management/commands/nest_update_badges.py (2)
62-75: Avoid redundant writes when deactivating badges.Limit the “remove badge” queries to active badge rows to reduce unnecessary UPDATEs.
Apply:
- non_employees = User.objects.filter( - is_owasp_staff=False, - badges__badge=badge, - ).distinct() + non_employees = User.objects.filter( + is_owasp_staff=False, + badges__badge=badge, + badges__is_active=True, + ).distinct() @@ - ).update(is_active=False) + ).filter(is_active=True).update(is_active=False)- non_waspy_users = ( - User.objects.exclude(awards__category="WASPY", awards__is_reviewed=True) - .filter(badges__badge=badge) - .distinct() - ) + non_waspy_users = ( + User.objects.exclude(awards__category="WASPY", awards__is_reviewed=True) + .filter(badges__badge=badge, badges__is_active=True) + .distinct() + ) @@ - ).update(is_active=False) + ).filter(is_active=True).update(is_active=False)Also applies to: 114-127
22-27: Optional: wrap the sync in a single transaction.Prevents partial badge state if one step fails mid-run.
from django.core.management.base import BaseCommand +from django.db import transaction @@ - def handle(self, *args, **options): + def handle(self, *args, **options): """Execute the command.""" - self.stdout.write("Syncing user badges...") - self.update_owasp_staff_badge() - self.update_waspy_award_badge() + self.stdout.write("Syncing user badges...") + with transaction.atomic(): + self.update_owasp_staff_badge() + self.update_waspy_award_badge() self.stdout.write(self.style.SUCCESS("User badges sync completed"))backend/apps/owasp/models/award.py (2)
15-17: Add a composite index to speed badge sync queries.The badge job filters by category and is_reviewed; add an index to cut join/filter cost.
indexes = [ - models.Index(fields=["-year"], name="award_year_desc_idx"), + models.Index(fields=["-year"], name="award_year_desc_idx"), + models.Index(fields=["category", "is_reviewed", "-year"], name="award_cat_review_year_idx"), ]
29-34: Optional: constrain/validate year.Prevent bad data (e.g., 0 or far-future years).
-from django.db import models +from django.db import models +from django.db.models import Q @@ constraints = [ models.UniqueConstraint( fields=["category", "name", "year"], name="uniq_award_cat_name_year", ), + models.CheckConstraint( + check=Q(year__gte=1990), + name="award_year_gte_1990", + ), ]backend/apps/owasp/management/commands/owasp_sync_awards.py (1)
40-46: Optional: guard against missing winners field.Some award items might omit 'winners' or use None; your default empty list is fine. Consider logging when an award has no winners for visibility.
📜 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(4 hunks)backend/apps/owasp/management/commands/owasp_sync_awards.py(1 hunks)backend/apps/owasp/models/award.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
backend/apps/owasp/management/commands/owasp_sync_awards.py (3)
backend/apps/github/utils.py (1)
get_repository_file_content(60-79)backend/apps/owasp/models/award.py (2)
Award(10-65)bulk_save(51-65)backend/apps/nest/management/commands/nest_update_badges.py (2)
Command(17-129)handle(22-27)
backend/apps/owasp/models/award.py (1)
backend/apps/common/models.py (2)
BulkSaveModel(10-34)TimestampedModel(37-46)
backend/apps/nest/management/commands/nest_update_badges.py (3)
backend/apps/github/models/user.py (1)
User(19-159)backend/apps/nest/models/user_badge.py (1)
UserBadge(10-50)backend/apps/nest/models/badge.py (1)
Badge(10-41)
🔇 Additional comments (1)
backend/apps/owasp/management/commands/owasp_sync_awards.py (1)
38-49: Idempotent bulk upsert path looks good.Single bulk_save with explicit fields avoids per-row saves and is safe to re-run.
trucodd
force-pushed
the
feature/owasp-awards-syncprofiles
branch
from
d11dbec to
46ab72e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
backend/apps/nest/management/commands/nest_update_badges.py (6)
46-49: Minor: consider distinct() to avoid duplicate users in edge joins.
Not strictly necessary, but safer when excluding via reverse relations and then counting/iterating.Apply:
- employees_missing_or_inactive = User.objects.filter(is_owasp_staff=True).exclude( - badges__badge=badge, badges__is_active=True - ) + employees_missing_or_inactive = ( + User.objects.filter(is_owasp_staff=True) + .exclude(badges__badge=badge, badges__is_active=True) + .distinct() + )
52-54: Simplify activation with update_or_create.
One DB round-trip per user and less branching.- for user in employees_missing_or_inactive: - user_badge, _ = UserBadge.objects.get_or_create(user=user, badge=badge) - if not user_badge.is_active: - user_badge.is_active = True - user_badge.save(update_fields=["is_active"]) + for user in employees_missing_or_inactive: + UserBadge.objects.update_or_create( + user=user, badge=badge, defaults={"is_active": True} + )
98-103: Tiny optimization: compute count after distinct to reflect unique users.
Harmless as-is; optional polish.- users_missing_or_inactive = waspy_award_users.exclude( - badges__badge=badge, badges__is_active=True - ) + users_missing_or_inactive = waspy_award_users.exclude( + badges__badge=badge, badges__is_active=True + ).distinct() count = users_missing_or_inactive.count()
104-109: Mirror staff path simplification with update_or_create.
Keeps both badge updaters consistent and reduces queries.- if count: - for user in users_missing_or_inactive: - user_badge, _ = UserBadge.objects.get_or_create(user=user, badge=badge) - if not user_badge.is_active: - user_badge.is_active = True - user_badge.save(update_fields=["is_active"]) + if count: + for user in users_missing_or_inactive: + UserBadge.objects.update_or_create( + user=user, badge=badge, defaults={"is_active": True} + )
114-127: Deactivate only active badges and avoid materializing ID lists.
Improves correctness of “removed” counts and avoids large in-memory lists; also aligns with the staff path intent.- non_waspy_users = ( - User.objects.exclude(awards__category="WASPY", awards__is_reviewed=True) - .filter(badges__badge=badge) - .distinct() - ) - removed_count = non_waspy_users.count() + non_waspy_users = ( + User.objects.exclude(awards__category="WASPY", awards__is_reviewed=True) + .filter(badges__badge=badge, badges__is_active=True) + .distinct() + ) + removed_count = non_waspy_users.count() - if removed_count: - UserBadge.objects.filter( - user_id__in=non_waspy_users.values_list("id", flat=True), - badge=badge, - ).update(is_active=False) + if removed_count: + UserBadge.objects.filter( + user__in=non_waspy_users, badge=badge, is_active=True + ).update(is_active=False)Optional: apply the same active-only deactivation to update_owasp_staff_badge() for consistency.
77-129: Performance/ops: consider indexes for these query shapes.
For scale, add database indexes:
- UserBadge: composite index (badge_id, is_active, user_id)
- Award: (category, is_reviewed, user_id)
If desired, I can draft the migration.
📜 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(4 hunks)backend/apps/owasp/management/commands/owasp_sync_awards.py(1 hunks)backend/apps/owasp/models/award.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/apps/owasp/management/commands/owasp_sync_awards.py
- backend/apps/owasp/models/award.py
🧰 Additional context used
🧬 Code graph analysis (1)
backend/apps/nest/management/commands/nest_update_badges.py (3)
backend/apps/github/models/user.py (1)
User(19-159)backend/apps/nest/models/user_badge.py (1)
UserBadge(10-50)backend/apps/nest/models/badge.py (1)
Badge(10-41)
🔇 Additional comments (3)
backend/apps/nest/management/commands/nest_update_badges.py (3)
14-14: Good: dedicated constant for WASPY badge.
Keeps magic strings out of the logic and matches existing style.
77-129: Overall WASPY sync logic looks sound and idempotent.
Creation, activation, and deactivation paths are clear.
26-26: Confirm CI/Makefile ordering
Could not locate any invocations ofowasp_sync_awardsornest_update_badgesin Makefiles, GitHub workflows, or ops scripts; please manually verify thatowasp_sync_awardsexecutes beforenest_update_badgesin all daily job configurations.
trucodd
force-pushed
the
feature/owasp-awards-syncprofiles
branch
from
46ab72e to
6513f7b
Compare
|
Proposed change
Resolves #1766
Award Model: Added with category, name, description, year, user, and is_reviewed fields
Sync Command: owasp_sync_awards downloads/parses YAML, matches users, logs unmatched
Badge Integration: "WASPY Award Winner" badge assigned to users with reviewed WASPY awards
Admin Interface: Award management with review workflow
Build Integration: Added to daily sync pipeline