Sync OWASP Awards data and integrate with user profiles #2024
Conversation
Summary by CodeRabbit
WalkthroughAdds an Award model and migration, admin and tests; adds two management commands (awards sync and badge updater) and wires them into Makefiles; removes a prior badge migration and updates a merge migration; simplifies Badge admin UI. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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: 8
🧹 Nitpick comments (23)
backend/apps/owasp/models/__init__.py (1)
1-1: Consider exposing the public API explicitly with allImporting
Awardhere is fine. To make the intended export explicit, consider adding__all__.from .award import Award + +__all__ = ["Award"]backend/apps/nest/Makefile (1)
1-3: Add .PHONY and optional no-op targets to satisfy checkmake.The target is fine. To address checkmake’s minphony warnings and improve Makefile robustness:
- Mark the target phony.
- Optionally add no-op
all,clean,testto satisfy the linter without altering behavior.Apply:
+.PHONY: nest-update-user-badges all clean test + +all: + @: + +clean: + @: + +test: + @: + nest-update-user-badges: @echo "Updating user badges" @CMD="python manage.py update_user_badges" $(MAKE) exec-backend-commandIf you prefer to keep this file minimal, confirm whether checkmake is enforced on per-app Makefiles in CI. If it is, the above will resolve the warnings.
backend/apps/nest/models/badge.py (3)
108-110: Prefer award year from metadata over earned_at.year in display_nameearned_at is when the badge record was created, not necessarily the award year. If metadata contains the award year, display that and fall back to earned_at.
Apply this small change:
- return f"{self.badge_type.name} ({self.earned_at.year})" + year = None + try: + year = self.metadata.get("year") + except Exception: + # metadata may not be a dict; be defensive + year = None + return f"{self.badge_type.name} ({year or self.earned_at.year})"
87-89: Consider recording awarded_at in addition to earned_atIf you plan to surface when the award was actually received (vs when the badge row was created), adding an optional awarded_at DateField/DateTimeField improves fidelity and sorting by award history.
I can add an awarded_at field and wire the management command to populate it from Award.year (e.g., Jan 1 of the year) while keeping existing earned_at semantics. Want a PR snippet?
38-44: Optional: validate color formatIf you expect hex colors, add a RegexValidator to catch invalid inputs at save time. Otherwise, keep as-is for CSS names too.
Additional code outside this range for context:
from django.core.validators import RegexValidator
- color = models.CharField( + color = models.CharField( verbose_name="Color", max_length=20, blank=True, default="#FFD700", - help_text="Badge color (hex code or CSS color name)", + help_text="Badge color (hex code or CSS color name)", + validators=[RegexValidator(regex=r'^#([0-9a-fA-F]{3}|[0-9a-fA-F]{6})$|^[a-zA-Z]+$')], )backend/tests/apps/nest/management/commands/update_user_badges_test.py (4)
42-48: Use TestCase assertions for clearer failuresSwitch bare asserts to unittest/Django assertions for better messages and consistency.
- assert badge_type.name == "WASPY Award Winner" + self.assertEqual(badge_type.name, "WASPY Award Winner") ... - assert user_badge.user == self.user - assert "Test Person of the Year" in user_badge.reason + self.assertEqual(user_badge.user, self.user) + self.assertIn("Test Person of the Year", user_badge.reason)
55-56: Prefer assertTrue/assertFalse/assertEqual in remaining testsMinor consistency/readability improvement across tests.
- assert UserBadge.objects.filter(user=self.user, badge_type=badge_type).exists() + self.assertTrue(UserBadge.objects.filter(user=self.user, badge_type=badge_type).exists()) ... - assert not UserBadge.objects.filter(user=self.user, badge_type=badge_type).exists() + self.assertFalse(UserBadge.objects.filter(user=self.user, badge_type=badge_type).exists()) ... - assert not BadgeType.objects.filter(name="WASPY Award Winner").exists() - assert not UserBadge.objects.filter(user=self.user).exists() + self.assertFalse(BadgeType.objects.filter(name="WASPY Award Winner").exists()) + self.assertFalse(UserBadge.objects.filter(user=self.user).exists()) ... - assert user_badge.user == self.user + self.assertEqual(user_badge.user, self.user)Also applies to: 65-66, 73-74, 83-84
79-80: Strengthen single-user scope testConsider adding a second user with an award and assert that user_login limits processing to the target user only.
I can extend this test with a second user+award and assertions if you want a patch.
35-39: Add idempotency test (optional)Running the command twice should not create duplicate UserBadge rows. Add an assertion on count after two runs.
call_command("update_user_badges", verbosity=0) call_command("update_user_badges", verbosity=0) badge_type = BadgeType.objects.get(name="WASPY Award Winner") self.assertEqual( UserBadge.objects.filter(user=self.user, badge_type=badge_type).count(), 1 )backend/apps/nest/admin/badge.py (1)
46-56: Admin config looks solid; consider small optimizationsGood use of autocomplete and select_related. Two optional tweaks:
- Use list_select_related = ("user", "badge_type") instead of overriding get_queryset for brevity.
- Add date_hierarchy = "earned_at" to enable date drill-down.
Additional code outside the selected range for context:
class UserBadgeAdmin(admin.ModelAdmin): ... list_select_related = ("user", "badge_type") date_hierarchy = "earned_at" # You can then drop get_queryset if desired.Also applies to: 57-61, 69-70, 91-93
backend/apps/owasp/admin/award.py (2)
27-33: Add user fields to search for better admin UXInclude user login/name in
search_fieldsto quickly find awards by recipient.search_fields = ( "name", "category", "winner_name", "description", "winner_info", + "user__login", + "user__name", )
61-63: Prefer list_select_related over overriding get_querysetUsing
list_select_related = ("user",)gives the list view optimization without affecting other admin queries. Optional but cleaner.- def get_queryset(self, request): - """Optimize queryset with select_related.""" - return super().get_queryset(request).select_related("user") + # Optimize list view queries without overriding get_queryset + list_select_related = ("user",)backend/apps/owasp/models/award.py (3)
46-52: Add basic year validatorsConstrain
yearto a sane range to avoid bad data.from django.db import models +from django.core.validators import MinValueValidator, MaxValueValidator ... year = models.IntegerField( verbose_name="Year", null=True, blank=True, help_text="Year the award was given (null for category definitions)", + validators=[MinValueValidator(1990), MaxValueValidator(2100)], )
98-106: Guard against None in str for safetyAvoid rendering "(None)" if data inconsistent.
def __str__(self) -> str: """Return string representation of the award.""" if self.award_type == "category": return f"{self.name} (Category)" if self.winner_name: - return f"{self.name} - {self.winner_name} ({self.year})" + return ( + f"{self.name} - {self.winner_name} ({self.year})" + if self.year is not None + else f"{self.name} - {self.winner_name}" + ) - return f"{self.name} ({self.year})" + return f"{self.name} ({self.year})" if self.year is not None else self.name
75-85: Consider TextChoices for award_typeUsing
TextChoicesimproves type-safety and avoids magic strings across the codebase.backend/apps/nest/management/commands/update_user_badges.py (6)
11-11: Hoist badge name to a constantAvoid magic strings and typos by centralizing the badge name.
logger = logging.getLogger(__name__) +BADGE_WASPY_AWARD_WINNER = "WASPY Award Winner"
120-121: Prefetch badge_type to avoid per-user queriesUse
badges__badge_typeto prevent N+1 when resolving existing badges.- users = User.objects.filter(id__in=all_user_ids).prefetch_related("awards", "badges") + users = ( + User.objects.filter(id__in=all_user_ids) + .prefetch_related("awards", "badges__badge_type") + )
129-130: Apply the same prefetch optimization in single-user path- user = User.objects.prefetch_related("awards", "badges").get(login=user_login) + user = ( + User.objects.prefetch_related("awards", "badges__badge_type") + .get(login=user_login) + )
155-159: Don’t log a stack trace for expected missing badge typesUse
warninginstead ofexceptionto avoid noisy logs.- except BadgeType.DoesNotExist: - logger.exception("Badge type '%s' not found", badge_name) + except BadgeType.DoesNotExist: + logger.warning("Badge type '%s' not found", badge_name) return
161-163: Use prefetched badges to avoid extra query per userLeverage
user.badges.all()to find the existing badge in-memory.- existing_badge = UserBadge.objects.filter(user=user, badge_type=badge_type).first() + existing_badge = next( + (b for b in user.badges.all() if b.badge_type_id == badge_type.id), + None, + )
221-231: Sort dry-run output for readability and parity- elif has_waspy_award: - award_names = list(waspy_awards.values_list("name", "year")) + elif has_waspy_award: + award_names = [(a.name, a.year) for a in awards_list]backend/apps/owasp/management/commands/owasp_sync_awards.py (2)
53-55: Use a constant for the source URLCentralize the YAML URL for easier maintenance and testing.
+SOURCE_URL = "https://raw.githubusercontent.com/OWASP/owasp.github.io/main/_data/awards.yml" ... - yaml_content = get_repository_file_content( - "https://raw.githubusercontent.com/OWASP/owasp.github.io/main/_data/awards.yml" - ) + yaml_content = get_repository_file_content(SOURCE_URL)
288-300: Harden GitHub username extraction to avoid org/team URLs and false positivesThe current regex may extract “OWASP” from
github.com/OWASP/teams/.... Tighten the pattern and ensure word boundaries.- github_url_pattern = r"github\.com/([a-zA-Z0-9\-_]+)" - match = re.search(github_url_pattern, text, re.IGNORECASE) + github_url_pattern = ( + r"(?:https?://)?(?:www\.)?github\.com/" + r"(?!orgs/|topics/|enterprise/|features/|apps/)" + r"([A-Za-z0-9](?:[A-Za-z0-9-]{0,38}[A-Za-z0-9])?)\b" + ) + match = re.search(github_url_pattern, text, re.IGNORECASE) if match: return match.group(1) # Pattern 2: @username mentions - mention_pattern = r"@([a-zA-Z0-9\-_]+)" + mention_pattern = r"@([A-Za-z0-9](?:[A-Za-z0-9-]{0,38}[A-Za-z0-9])?)\b" match = re.search(mention_pattern, text) if match: return match.group(1)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
backend/Makefile(3 hunks)backend/apps/nest/Makefile(1 hunks)backend/apps/nest/admin/__init__.py(1 hunks)backend/apps/nest/admin/badge.py(1 hunks)backend/apps/nest/management/__init__.py(1 hunks)backend/apps/nest/management/commands/__init__.py(1 hunks)backend/apps/nest/management/commands/update_user_badges.py(1 hunks)backend/apps/nest/migrations/0003_badgetype_userbadge.py(1 hunks)backend/apps/nest/models/__init__.py(1 hunks)backend/apps/nest/models/badge.py(1 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/migrations/0045_award.py(1 hunks)backend/apps/owasp/models/__init__.py(1 hunks)backend/apps/owasp/models/award.py(1 hunks)backend/tests/apps/nest/management/__init__.py(1 hunks)backend/tests/apps/nest/management/commands/__init__.py(1 hunks)backend/tests/apps/nest/management/commands/update_user_badges_test.py(1 hunks)
🧰 Additional context used
🪛 checkmake (0.2.2)
backend/apps/nest/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🔇 Additional comments (14)
backend/apps/nest/management/__init__.py (1)
1-1: Good addition for Django command discoveryAdding an
__init__.pyhere ensures the management package is recognized. The docstring is fine.backend/tests/apps/nest/management/__init__.py (1)
1-1: Test package initialization looks goodHaving this
__init__.pyhelps keep tests organized as a package. No issues.backend/apps/nest/management/commands/__init__.py (1)
1-1: Required initializer for commands packageThis enables Django to discover commands under
management/commands. Looks good.backend/tests/apps/nest/management/commands/__init__.py (1)
1-1: OK to include for test commands structureDocstring-only initializer is fine and consistent with the rest of the test modules.
backend/apps/owasp/admin/__init__.py (1)
7-7: LGTM. Ensure registration is decorator-based.Importing
AwardAdminhere is consistent with the existing pattern. Just confirm thatbackend/apps/owasp/admin/award.pyuses@admin.register(Award)(or explicitly callsadmin.site.register) so the import triggers registration.backend/apps/nest/admin/__init__.py (1)
4-4: Badge admin imports look good.Consistent with the module import pattern used elsewhere to register admin classes.
backend/apps/nest/models/__init__.py (1)
2-2: Model exports added correctly.Re-exporting
BadgeTypeandUserBadgehere is consistent and helpful for imports elsewhere.backend/apps/owasp/Makefile (1)
25-27: owasp-sync-awards target integration verifiedThe
owasp-sync-awardstarget is included in the main backend workflow (backend/Makefile:139), so it’s wired up correctly. LGTM.backend/Makefile (3)
3-3: Good integration of Nest Makefile includeIncluding backend/apps/nest/Makefile cleanly exposes the new badge target to the root Makefile.
139-139: Awards sync wired into update-data at the right positionPlacing owasp-sync-awards within update-data (prior to sync-data’s nest-update-user-badges) ensures award data is fresh before badges are computed.
113-113: Badge update correctly sequenced after data updateVerified that the
nest-update-user-badgestarget is defined inbackend/apps/nest/Makefileand invokes:python manage.py update_user_badgesThis ensures it runs after
update-dataandenrich-datain thesync-datarecipe, preserving the intended ordering with the awards sync. ✅backend/apps/nest/models/badge.py (1)
63-66: Confirm uniqueness semantics for multiple awards per userThe uniqueness constraint user + badge_type enforces a single badge per type. This fits “assign a WASPY Award Winner badge to users who have received WASPY awards.” If we ever need one badge per year (multiple WASPY badges), this constraint will block it.
Would you like a single “winner” badge per user (aggregating multiple years in metadata), or distinct badges per year? If the latter, we should either add year to the uniqueness or introduce an awarded_year field. I can provide a migration if needed.
backend/tests/apps/nest/management/commands/update_user_badges_test.py (1)
21-23: Verify field types for created_at/updated_atIf apps.github.models.user.User uses DateTimeField for created_at/updated_at, pass timezone-aware datetimes instead of strings to avoid implicit casting issues.
from django.utils import timezone self.user = User.objects.create( login="testuser", name="Test User", email="[email protected]", created_at=timezone.now(), updated_at=timezone.now(), )backend/apps/nest/migrations/0003_badgetype_userbadge.py (1)
14-78: Migration aligns with models and requirementsTables, constraints (unique user+badge_type), and indexes mirror the model definitions. Dependencies are correct. No issues found.
Also applies to: 79-154
| badge_name = "WASPY Award Winner" | ||
|
|
||
| # Check if user has any WASPY awards | ||
| waspy_awards = user.awards.filter(category="WASPY", award_type="award") | ||
|
|
||
| has_waspy_award = waspy_awards.exists() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Stabilize award ordering to keep badge metadata deterministic
Sorting awards avoids unnecessary badge updates caused by non-deterministic queryset ordering.
- badge_name = "WASPY Award Winner"
+ badge_name = BADGE_WASPY_AWARD_WINNER
...
- waspy_awards = user.awards.filter(category="WASPY", award_type="award")
-
- has_waspy_award = waspy_awards.exists()
+ waspy_awards = (
+ user.awards.filter(category="WASPY", award_type="award")
+ .order_by("year", "name", "winner_name")
+ )
+ awards_list = list(waspy_awards)
+ has_waspy_award = bool(awards_list)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| badge_name = "WASPY Award Winner" | |
| # Check if user has any WASPY awards | |
| waspy_awards = user.awards.filter(category="WASPY", award_type="award") | |
| has_waspy_award = waspy_awards.exists() | |
| badge_name = BADGE_WASPY_AWARD_WINNER | |
| # Check if user has any WASPY awards | |
| waspy_awards = ( | |
| user.awards.filter(category="WASPY", award_type="award") | |
| .order_by("year", "name", "winner_name") | |
| ) | |
| awards_list = list(waspy_awards) | |
| has_waspy_award = bool(awards_list) |
🤖 Prompt for AI Agents
In backend/apps/nest/management/commands/update_user_badges.py around lines 146
to 152, the queryset for fetching WASPY awards is not ordered, which can cause
non-deterministic ordering and unnecessary badge updates. Fix this by adding an
explicit order_by clause to the waspy_awards queryset to ensure stable and
deterministic ordering of awards.
| award_details = [ | ||
| { | ||
| "award_name": award.name, | ||
| "year": award.year, | ||
| "winner_name": award.winner_name, | ||
| } | ||
| for award in waspy_awards | ||
| ] | ||
|
|
||
| award_names_str = ", ".join( | ||
| [f"{a['award_name']} ({a['year']})" for a in award_details] | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Build metadata from the sorted list to keep it stable
- award_details = [
- {
- "award_name": award.name,
- "year": award.year,
- "winner_name": award.winner_name,
- }
- for award in waspy_awards
- ]
+ award_details = [
+ {"award_name": a.name, "year": a.year, "winner_name": a.winner_name}
+ for a in awards_list
+ ]
award_names_str = ", ".join(
[f"{a['award_name']} ({a['year']})" for a in award_details]
)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In backend/apps/nest/management/commands/update_user_badges.py around lines 165
to 176, the award_details list is built directly from waspy_awards without
sorting, which can cause instability in the output order. To fix this, sort
waspy_awards by a consistent key such as award name or year before constructing
award_details to ensure stable and predictable metadata ordering.
| award_details = [] | ||
| for award in waspy_awards: | ||
| award_details.append( | ||
| { | ||
| "award_name": award.name, | ||
| "year": award.year, | ||
| "winner_name": award.winner_name, | ||
| } | ||
| ) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Keep update path consistent with create path and ordering
- award_details = []
- for award in waspy_awards:
- award_details.append(
- {
- "award_name": award.name,
- "year": award.year,
- "winner_name": award.winner_name,
- }
- )
+ award_details = [
+ {"award_name": a.name, "year": a.year, "winner_name": a.winner_name}
+ for a in awards_list
+ ]Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In backend/apps/nest/management/commands/update_user_badges.py between lines 197
and 206, the update logic for award_details should follow the same path and
ordering as the create logic to maintain consistency. Review the create path for
award_details and adjust the update loop to match the same structure and order
of fields, ensuring the data is processed and appended in the same sequence and
format.
backend/apps/owasp/admin/award.py
Outdated
| fieldsets = ( | ||
| ( | ||
| "Basic Information", | ||
| {"fields": ("name", "category", "award_type", "year", "description")}, | ||
| ), | ||
| ( | ||
| "Winner Information", | ||
| { | ||
| "fields": ("winner_name", "winner_info", "winner_image", "user"), | ||
| "classes": ("collapse",), | ||
| }, | ||
| ), | ||
| ( | ||
| "Timestamps", | ||
| { | ||
| "fields": ("nest_created_at", "nest_updated_at"), | ||
| "classes": ("collapse",), | ||
| }, | ||
| ), | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enforce data integrity between award_type and fields
Admin currently allows inconsistent edits (e.g., award_type='category' with non-null year or winner fields). Add model validation (clean) to enforce:
- category entries:
year is Noneand winner fields empty - award entries:
year is not None
I can add a clean() method in the model and wire an admin form to surface validation errors in the UI.
🤖 Prompt for AI Agents
In backend/apps/owasp/admin/award.py around lines 38 to 57, the admin form
allows inconsistent data between award_type and related fields. To fix this, add
a clean() method in the Award model that validates: if award_type is 'category',
then year must be None and winner fields must be empty; if award_type is
'award', then year must not be None. Then, create a custom ModelForm for the
admin that calls this clean() method and surfaces validation errors in the UI by
wiring this form to the AwardAdmin class.
| award, created = Award.objects.get_or_create( | ||
| name=award_name, | ||
| category=category, | ||
| year=year, | ||
| winner_name=winner_name, | ||
| defaults={ | ||
| "award_type": "award", | ||
| "description": "", | ||
| "winner_info": winner_info, | ||
| "winner_image": winner_image, | ||
| "user": matched_user, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Potential IntegrityError due to unique constraint not including winner_name
get_or_create uses (name, category, year, winner_name). If the DB has a unique constraint only on (name, year, category), inserting a second winner for the same award/year will fail.
Action:
- Update model and migration to include
winner_namein the unique constraint (see model/migration comments). With that in place, this block is correct and idempotent.
🤖 Prompt for AI Agents
In backend/apps/owasp/management/commands/owasp_sync_awards.py around lines 168
to 180, the get_or_create call uses a unique constraint including winner_name,
but the database unique constraint only covers name, year, and category. To fix
this, update the Award model and its migration files to include winner_name in
the unique_together or UniqueConstraint definition so that the database enforces
uniqueness on (name, category, year, winner_name). After applying this
migration, the current get_or_create usage will be correct and idempotent.
| models.UniqueConstraint( | ||
| fields=("name", "year", "category"), name="unique_award_name_year_category" | ||
| ) |
There was a problem hiding this comment.
Migration unique constraint must include winner_name
Align the DB constraint with the model to allow multiple winners per award/year. Without this, inserts will fail when there are multiple winners.
- models.UniqueConstraint(
- fields=("name", "year", "category"), name="unique_award_name_year_category"
- )
+ models.UniqueConstraint(
+ fields=("name", "year", "category", "winner_name"),
+ name="unique_award_name_year_category_winner",
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| models.UniqueConstraint( | |
| fields=("name", "year", "category"), name="unique_award_name_year_category" | |
| ) | |
| models.UniqueConstraint( | |
| fields=("name", "year", "category", "winner_name"), | |
| name="unique_award_name_year_category_winner", | |
| ) |
🤖 Prompt for AI Agents
In backend/apps/owasp/migrations/0045_award.py around lines 121 to 123, the
UniqueConstraint on the award model currently includes only "name", "year", and
"category" fields, which prevents multiple winners for the same
award/year/category. To fix this, add "winner_name" to the fields tuple in the
UniqueConstraint so that the database allows multiple winners per
award/year/category without insert failures.
backend/apps/owasp/models/award.py
Outdated
| constraints = [ | ||
| models.UniqueConstraint( | ||
| fields=["name", "year", "category"], name="unique_award_name_year_category" | ||
| ) | ||
| ] |
There was a problem hiding this comment.
Uniqueness is too coarse; blocks multiple winners for the same award/year
Current unique constraint on (name, year, category) prevents multiple winners for a single award/year. The YAML contains multiple winners per award in some cases. This will raise IntegrityError when inserting the second winner.
Update the constraint to include winner_name:
- constraints = [
- models.UniqueConstraint(
- fields=["name", "year", "category"], name="unique_award_name_year_category"
- )
- ]
+ constraints = [
+ models.UniqueConstraint(
+ fields=["name", "year", "category", "winner_name"],
+ name="unique_award_name_year_category_winner",
+ )
+ ]Also update the migration accordingly (see migration comment).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| constraints = [ | |
| models.UniqueConstraint( | |
| fields=["name", "year", "category"], name="unique_award_name_year_category" | |
| ) | |
| ] | |
| constraints = [ | |
| models.UniqueConstraint( | |
| fields=["name", "year", "category", "winner_name"], | |
| name="unique_award_name_year_category_winner", | |
| ) | |
| ] |
🤖 Prompt for AI Agents
In backend/apps/owasp/models/award.py around lines 26 to 30, the unique
constraint on (name, year, category) is too broad and prevents multiple winners
for the same award and year. Modify the UniqueConstraint to include the field
winner_name so that uniqueness is enforced per winner as well. After updating
the model, generate and update the corresponding migration file to reflect this
change in the database schema.
There was a problem hiding this comment.
This is a decent code 👍
Moreover, I like some of your ideas around badges implementation.
Let's do the following: remove badge related code from the PR while focusing on syncing the awards and matching them to users.
We use badges separately just for displaying some user attributes to visitors
backend/apps/nest/models/badge.py
Outdated
| from apps.common.models import TimestampedModel | ||
|
|
||
|
|
||
| class BadgeType(TimestampedModel): |
There was a problem hiding this comment.
The Badge model already exists at owasp/models/badge.py
backend/apps/owasp/models/award.py
Outdated
| award_type = models.CharField( | ||
| verbose_name="Award Type", | ||
| max_length=20, | ||
| choices=[ | ||
| ("category", "Category"), | ||
| ("award", "Award"), | ||
| ], | ||
| default="award", | ||
| help_text="Type of entry: category definition or individual award", | ||
| ) |
There was a problem hiding this comment.
This doesn't look right, it seems we need Distinguished Lifetime Memberships and WASPY -- please double check.
backend/apps/owasp/models/award.py
Outdated
| verbose_name="Award Type", | ||
| max_length=20, | ||
| choices=[ | ||
| ("category", "Category"), |
There was a problem hiding this comment.
Please also make the categories a separate subclass (see an example)
backend/apps/owasp/models/award.py
Outdated
| verbose_name="Year", | ||
| null=True, | ||
| blank=True, | ||
| help_text="Year the award was given (null for category definitions)", |
There was a problem hiding this comment.
The category part needs to be hardcoded. I don't think we should create a separate table for them as they rarely change.
There was a problem hiding this comment.
After removing that you can focus completely on award entities.
backend/apps/owasp/models/award.py
Outdated
| """ | ||
| from apps.github.models.user import User | ||
|
|
||
| return User.objects.filter(awards__category="WASPY", awards__award_type="award").distinct() |
There was a problem hiding this comment.
Reuse WASPY value after defining a separate choice field for categories.
backend/apps/owasp/models/award.py
Outdated
| category = models.CharField( | ||
| verbose_name="Category", | ||
| max_length=100, | ||
| help_text="Award category (e.g., 'WASPY', 'Lifetime Achievement')", | ||
| ) |
There was a problem hiding this comment.
| category = models.CharField( | |
| verbose_name="Category", | |
| max_length=100, | |
| help_text="Award category (e.g., 'WASPY', 'Lifetime Achievement')", | |
| ) |
|
The badge model was not there when I started so I created I will refactor to use the existing model and make the other requested changes. |
trucodd
force-pushed
the
feature/owasp-awards-profilesync
branch
from
04edc7d to
082c701
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
backend/apps/owasp/models/award.py (1)
33-37: Allow multiple winners per award/year by including winner_name in the unique constraintAs-is, inserting a second winner for the same award/year/category will raise IntegrityError. Include winner_name in the uniqueness constraint.
- constraints = [ - models.UniqueConstraint( - fields=["name", "year", "category"], name="unique_award_name_year_category" - ) - ] + constraints = [ + models.UniqueConstraint( + fields=["name", "year", "category", "winner_name"], + name="unique_award_name_year_category_winner", + ) + ]backend/apps/owasp/migrations/0045_award.py (1)
121-123: Migration unique constraint must include winner_name to allow multiple winnersKeep the DB schema aligned with the model and the YAML reality (multiple winners per award/year).
- models.UniqueConstraint( - fields=("name", "year", "category"), name="unique_award_name_year_category" - ) + models.UniqueConstraint( + fields=("name", "year", "category", "winner_name"), + name="unique_award_name_year_category_winner", + )
🧹 Nitpick comments (8)
backend/tests/apps/owasp/models/award_test.py (5)
26-26: Use enum for category instead of magic stringsPrefer Award.Category.WASPY for consistency and refactor safety.
- category="WASPY", + category=Award.Category.WASPY,Also applies to: 43-43, 57-57
33-37: Prefer unittest assertions for clearer failuresUse TestCase assertions to improve failure messages and consistency.
- assert award.name == "Test Award" - assert award.category == "WASPY" - assert award.year == 2024 - assert award.winner_name == "Test User" - assert award.user == self.user + self.assertEqual(award.name, "Test Award") + self.assertEqual(award.category, Award.Category.WASPY) + self.assertEqual(award.year, 2024) + self.assertEqual(award.winner_name, "Test User") + self.assertEqual(award.user, self.user)
50-52: Prefer unittest assertions for membership checksUse assertIn for clarity.
- assert self.user in winners + self.assertIn(self.user, winners)
64-65: Prefer unittest assertions for membership checksUse assertIn for clarity.
- assert award in user_awards + self.assertIn(award, user_awards)
14-20: Ensure field types match the User modelcreated_at/updated_at literals are ISO strings; if the User model uses DateTimeField, parsing may vary. Prefer timezone-aware datetimes to avoid surprises.
Proposed change (outside current range): add an import and use timezone.now():
from django.utils import timezone ... self.user = User.objects.create( login="testuser", name="Test User", email="[email protected]", created_at=timezone.now(), updated_at=timezone.now(), )Additionally, consider adding a test that asserts multiple winners for the same award/year are allowed (to guard the DB uniqueness fix).
backend/apps/owasp/models/award.py (3)
148-151: Normalize parsed strings from YAML (strip) to avoid accidental duplicatesTrim title and category to prevent whitespace differences from creating distinct rows.
- award_name = award_data.get("title", "") - category = award_data.get("category", "") + award_name = (award_data.get("title") or "").strip() + category = (award_data.get("category") or "").strip()
136-142: Correct the return type in the docstringupdate_data returns a list of Award instances (or None), not a single instance.
- Returns: - Award instance or list of Award instances + Returns: + list[Award]: when award_data['type'] == 'award' + None: otherwise
115-126: Optional: provide deterministic ordering for per-user listingsAdding a default ordering improves UX and test determinism.
- return cls.objects.filter(user=user, category=cls.Category.WASPY) + return cls.objects.filter(user=user, category=cls.Category.WASPY).order_by("-year", "name")
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
backend/Makefile(1 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/migrations/0045_award.py(1 hunks)backend/apps/owasp/migrations/0045_badge.py(0 hunks)backend/apps/owasp/models/__init__.py(1 hunks)backend/apps/owasp/models/award.py(1 hunks)backend/tests/apps/nest/management/__init__.py(1 hunks)backend/tests/apps/nest/management/commands/__init__.py(1 hunks)backend/tests/apps/owasp/models/award_test.py(1 hunks)
💤 Files with no reviewable changes (1)
- backend/apps/owasp/migrations/0045_badge.py
✅ Files skipped from review due to trivial changes (1)
- backend/tests/apps/nest/management/init.py
🚧 Files skipped from review as they are similar to previous changes (7)
- backend/apps/owasp/admin/award.py
- backend/apps/owasp/admin/init.py
- backend/apps/owasp/models/init.py
- backend/Makefile
- backend/tests/apps/nest/management/commands/init.py
- backend/apps/owasp/Makefile
- backend/apps/owasp/management/commands/owasp_sync_awards.py
🧰 Additional context used
🧬 Code Graph Analysis (2)
backend/tests/apps/owasp/models/award_test.py (1)
backend/apps/owasp/models/award.py (3)
Award(10-188)get_waspy_award_winners(104-113)get_user_waspy_awards(116-126)
backend/apps/owasp/models/award.py (2)
backend/apps/common/models.py (1)
TimestampedModel(37-46)backend/apps/github/api/internal/queries/user.py (1)
user(40-56)
🔇 Additional comments (1)
backend/apps/owasp/migrations/0045_award.py (1)
51-58: Model/migration mismatch: year is nullable here but not in the modelEnsure the model matches this (blank=True, null=True) or adjust the migration to null=False to avoid drift. The model change is recommended given category-type entries.
| ( | ||
| "award_type", | ||
| models.CharField( | ||
| choices=[("category", "Category"), ("award", "Award")], | ||
| default="award", | ||
| help_text="Type of entry: category definition or individual award", | ||
| max_length=20, | ||
| verbose_name="Award Type", | ||
| ), | ||
| ), |
There was a problem hiding this comment.
Model/migration mismatch: award_type exists in migration but not in the model
Add award_type to the model (with choices and default) as suggested in the model review to avoid runtime errors when passing award_type in code/tests.
🤖 Prompt for AI Agents
In backend/apps/owasp/migrations/0045_award.py around lines 88 to 97, the
migration adds an "award_type" CharField with choices
[("category","Category"),("award","Award")] and default "award", but the model
class lacks this field; add an award_type = models.CharField(...) to the Award
model in the app's models.py using the same max_length, choices, default,
help_text and verbose_name as the migration so the model and migration match
(then run tests and, if necessary, makemigrations to ensure no further
mismatches).
| class Category(models.TextChoices): | ||
| WASPY = "WASPY", "WASPY" | ||
| DISTINGUISHED_LIFETIME = ( | ||
| "Distinguished Lifetime Memberships", | ||
| "Distinguished Lifetime Memberships", | ||
| ) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add EntryType choices and the award_type field to match migration and tests
The migration and tests expect award_type with choices ("category", "award"). Define choices and the field.
Add the EntryType choices class near Category:
class Award(TimestampedModel):
@@
class Category(models.TextChoices):
WASPY = "WASPY", "WASPY"
DISTINGUISHED_LIFETIME = (
"Distinguished Lifetime Memberships",
"Distinguished Lifetime Memberships",
)
+
+ class EntryType(models.TextChoices):
+ CATEGORY = "category", "Category"
+ AWARD = "award", "Award"Add the award_type field after winner_image:
winner_image = models.CharField(
verbose_name="Winner Image",
max_length=500,
blank=True,
default="",
help_text="Path to winner's image",
)
+ award_type = models.CharField(
+ verbose_name="Award Type",
+ max_length=20,
+ choices=EntryType.choices,
+ default=EntryType.AWARD,
+ help_text="Type of entry: category definition or individual award",
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class Category(models.TextChoices): | |
| WASPY = "WASPY", "WASPY" | |
| DISTINGUISHED_LIFETIME = ( | |
| "Distinguished Lifetime Memberships", | |
| "Distinguished Lifetime Memberships", | |
| ) | |
| class Award(TimestampedModel): | |
| class Category(models.TextChoices): | |
| WASPY = "WASPY", "WASPY" | |
| DISTINGUISHED_LIFETIME = ( | |
| "Distinguished Lifetime Memberships", | |
| "Distinguished Lifetime Memberships", | |
| ) | |
| class EntryType(models.TextChoices): | |
| CATEGORY = "category", "Category" | |
| AWARD = "award", "Award" | |
| winner_image = models.CharField( | |
| verbose_name="Winner Image", | |
| max_length=500, | |
| blank=True, | |
| default="", | |
| help_text="Path to winner's image", | |
| ) | |
| award_type = models.CharField( | |
| verbose_name="Award Type", | |
| max_length=20, | |
| choices=EntryType.choices, | |
| default=EntryType.AWARD, | |
| help_text="Type of entry: category definition or individual award", | |
| ) |
🤖 Prompt for AI Agents
In backend/apps/owasp/models/award.py around lines 17-23, add an EntryType
TextChoices class defining the two choices ("category", "award") and then add an
award_type CharField immediately after the winner_image field; implement
EntryType with human-readable labels (e.g., CATEGORY="category","Category" and
AWARD="award","Award") and add award_type = models.CharField(max_length=20,
choices=EntryType.choices, default=EntryType.AWARD) so the model matches the
migration and tests.
| year = models.IntegerField( | ||
| verbose_name="Year", | ||
| help_text="Year the award was given", | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align model field nullability with migration and data shape
The migration makes year nullable (blank=True, null=True). The model currently does not. Align these to avoid a follow-up migration and to support category-type rows if ever stored.
- year = models.IntegerField(
- verbose_name="Year",
- help_text="Year the award was given",
- )
+ year = models.IntegerField(
+ verbose_name="Year",
+ help_text="Year the award was given (null for category definitions)",
+ blank=True,
+ null=True,
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| year = models.IntegerField( | |
| verbose_name="Year", | |
| help_text="Year the award was given", | |
| ) | |
| year = models.IntegerField( | |
| verbose_name="Year", | |
| help_text="Year the award was given (null for category definitions)", | |
| blank=True, | |
| null=True, | |
| ) |
🤖 Prompt for AI Agents
In backend/apps/owasp/models/award.py around lines 54 to 57, the model's year
field is currently defined as a non-nullable IntegerField but the migration made
it nullable; update the field to include null=True and blank=True so the model
definition matches the migration and supports nullable year values (e.g.,
category-type rows).
| award = Award.objects.create( | ||
| name="Test Award", | ||
| category="WASPY", | ||
| year=2024, | ||
| award_type="award", | ||
| winner_name="Test User", | ||
| user=self.user, | ||
| ) |
There was a problem hiding this comment.
Fix model/test mismatch: award_type is referenced but not defined on the model
Award.objects.create(..., award_type="award") will raise TypeError because Award currently lacks an award_type field. The migration includes this field, so the model must define it to stay in sync.
Apply the model-side fixes suggested in backend/apps/owasp/models/award.py (add award_type field and its choices) rather than removing it from the tests.
🤖 Prompt for AI Agents
In backend/tests/apps/owasp/models/award_test.py around lines 24 to 31, the test
sets award_type="award" but the Award model lacks an award_type field; update
the model (backend/apps/owasp/models/award.py) to add an award_type CharField
with the same choices used in migrations (include the choice tuples and a
reasonable default or allow null if migration expects it), import/define the
choices constant, and run makemigrations/migrate if needed so the model and
tests are in sync.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/apps/owasp/migrations/0045_award.py (1)
116-118: UniqueConstraint should include winner_name to allow multiple winners per award/year/categoryAs-is, multiple recipients for the same award in the same year will violate the uniqueness constraint. Include winner_name in the constraint to support multiple winners.
Apply this diff:
- models.UniqueConstraint( - fields=("name", "year", "category"), name="unique_award_name_year_category" - ) + models.UniqueConstraint( + fields=("name", "year", "category", "winner_name"), + name="unique_award_name_year_category_winner", + )
🧹 Nitpick comments (3)
backend/apps/owasp/management/commands/owasp_update_badges.py (1)
31-36: Optional micro-optimization: avoid re-evaluating QuerySet for countIf keeping the current approach, calling waspy_winners.count() after iterating re-queries the DB. Cache IDs or compute len() on a list once.
backend/apps/owasp/migrations/0045_award.py (2)
84-91: Consider URLField for winner_imageIf winner_image stores URLs from awards.yml, URLField with validation would be more appropriate than CharField.
Proposed field change (requires data migration if existing rows):
- Replace CharField(max_length=500) with models.URLField(max_length=500, blank=True, default="", ...)
1-11: Minor: Migration headers show mixed Django versionsThis migration says “Generated by Django 5.2.5” while 0046 says 5.2.4. Not harmful, but standardizing generation version helps avoid unnecessary diffs across environments.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/Makefile(2 hunks)backend/apps/owasp/Makefile(2 hunks)backend/apps/owasp/admin/award.py(1 hunks)backend/apps/owasp/management/commands/owasp_update_badges.py(1 hunks)backend/apps/owasp/migrations/0045_award.py(1 hunks)backend/apps/owasp/migrations/0046_merge_0045_badge_0045_project_audience.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/apps/owasp/Makefile
- backend/apps/owasp/admin/award.py
🧰 Additional context used
🧬 Code Graph Analysis (2)
backend/apps/owasp/management/commands/owasp_update_badges.py (2)
backend/apps/owasp/models/award.py (2)
get_waspy_award_winners(104-113)get_user_waspy_awards(116-126)backend/apps/owasp/models/badge.py (1)
Badge(10-41)
backend/apps/owasp/migrations/0045_award.py (1)
backend/apps/owasp/migrations/0046_merge_0045_badge_0045_project_audience.py (1)
Migration(6-12)
🔇 Additional comments (5)
backend/Makefile (2)
109-114: Good wiring: update badges after enrich and before indexingPlacing owasp-update-badges after enrich-data and before index-data ensures badges reflect freshly synced data before search indexing. This ordering looks correct.
128-142: Include awards sync in update-data pipelineAdding owasp-sync-awards into update-data is appropriate and sequenced after github-update-users, which should improve recipient matching. Nice.
backend/apps/owasp/migrations/0046_merge_0045_badge_0045_project_audience.py (1)
7-10: 0045_badge migration not found—safe to remove dependency
A scan ofbackend/apps/owasp/migrationsshows no file or references for0045_badge.py. The only existing migrations are0045_award.pyand0045_project_audience.py, which match the updated dependencies in the merge file. Removing the nonexistent0045_badgedependency is therefore safe.backend/apps/owasp/management/commands/owasp_update_badges.py (1)
18-25: Idempotent badge creation is correctUsing get_or_create with sensible defaults for the WASPY badge is good and keeps the command idempotent.
backend/apps/owasp/migrations/0045_award.py (1)
27-38: Confirm category choices cover all values present in awards.ymlHard-coding only two choices risks rejecting future categories from the canonical source. If awards.yml contains other categories, migrations or parsing will fail. Consider a looser validation model-level (Enum that you extend) or map unknown categories to a generic bucket.
Would you confirm the set of categories present in awards.yml and whether we expect more over time? If dynamic, we should adjust the choices or parsing strategy to avoid breaking on new categories.
trucodd
force-pushed
the
feature/owasp-awards-profilesync
branch
2 times, most recently
from
309b84a to
867a047
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/apps/nest/admin/badge.py (1)
8-8: Confirm single source of truth for Badge modelA previous review noted another Badge model at owasp/models/badge.py. Ensure we’re using a single, canonical Badge model across apps to avoid divergence.
Run this script to locate any duplicate Badge model definitions and imports:
#!/bin/bash set -euo pipefail echo "Listing potential badge model files:" fd -H -i -t f 'badge.py' | sort echo echo "Searching for class Badge definitions:" ast-grep --pattern $'class Badge($_):\n $$$' || true echo echo "Searching for Badge imports:" rg -n -t py $'from\\s+apps\\.(owasp|nest)\\.models(\\.badge)?\\s+import\\s+Badge' -A 1 || true
🧹 Nitpick comments (5)
backend/apps/nest/models/badge.py (1)
41-41: Add trailing newline to satisfy Ruff W292Line 41 lacks a trailing newline. Many tools expect POSIX-compliant files to end with a newline; Ruff flags this as W292.
Apply this diff:
- return self.name + return self.name +backend/apps/owasp/models/__init__.py (1)
1-3: Define all to make the public API explicitExplicitly declare the public surface to avoid accidental re-exports and stabilize external imports.
Apply this diff:
from .award import Award from .project import Project + +__all__ = ["Project", "Award"]backend/apps/owasp/admin/award.py (2)
5-5: Prefer importing from the models package for consistencySince Award is re-exported in apps.owasp.models, import from the package rather than the module path.
Apply this diff:
-from apps.owasp.models.award import Award +from apps.owasp.models import Award
59-61: Use list_select_related instead of overriding get_querysetDjango’s list_select_related property provides the same optimization with less code and better readability.
Apply this diff to remove the method:
- def get_queryset(self, request): - """Optimize queryset with select_related.""" - return super().get_queryset(request).select_related("user")Then add this attribute near the other admin attributes:
# Add within AwardAdmin list_select_related = ("user",)backend/apps/nest/admin/badge.py (1)
15-15: Add trailing newline to satisfy Ruff W292File is missing a trailing newline on Line 15.
Apply this diff:
- ordering = ("weight", "name") + ordering = ("weight", "name") +
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
backend/Makefile(2 hunks)backend/apps/nest/admin/badge.py(1 hunks)backend/apps/nest/models/badge.py(1 hunks)backend/apps/owasp/Makefile(2 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/management/commands/owasp_update_badges.py(1 hunks)backend/apps/owasp/migrations/0045_award.py(1 hunks)backend/apps/owasp/migrations/0046_merge_0045_badge_0045_project_audience.py(1 hunks)backend/apps/owasp/models/__init__.py(1 hunks)backend/apps/owasp/models/award.py(1 hunks)backend/tests/apps/nest/management/__init__.py(1 hunks)backend/tests/apps/nest/management/commands/__init__.py(1 hunks)backend/tests/apps/owasp/models/award_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- backend/tests/apps/nest/management/commands/init.py
- backend/tests/apps/nest/management/init.py
- backend/Makefile
- backend/apps/owasp/management/commands/owasp_sync_awards.py
- backend/tests/apps/owasp/models/award_test.py
- backend/apps/owasp/migrations/0046_merge_0045_badge_0045_project_audience.py
- backend/apps/owasp/Makefile
- backend/apps/owasp/models/award.py
- backend/apps/owasp/migrations/0045_award.py
- backend/apps/owasp/admin/init.py
- backend/apps/owasp/management/commands/owasp_update_badges.py
🧰 Additional context used
🧬 Code Graph Analysis (3)
backend/apps/owasp/admin/award.py (1)
backend/apps/owasp/models/award.py (1)
Award(10-188)
backend/apps/owasp/models/__init__.py (1)
backend/apps/owasp/models/award.py (1)
Award(10-188)
backend/apps/nest/admin/badge.py (1)
backend/apps/nest/models/badge.py (1)
Badge(10-41)
🪛 Ruff (0.12.2)
backend/apps/nest/admin/badge.py
15-15: No newline at end of file
Add trailing newline
(W292)
backend/apps/nest/models/badge.py
41-41: No newline at end of file
Add trailing newline
(W292)
🔇 Additional comments (3)
backend/apps/owasp/models/__init__.py (1)
1-1: Good addition: re-exporting Award at the package levelMaking Award importable via apps.owasp.models aligns with how Project is exposed and simplifies imports elsewhere.
backend/apps/owasp/admin/award.py (1)
8-57: Admin configuration is solid and comprehensiveThe list_display/filter/search organization looks good, and autocomplete_fields for user is appropriate. Fieldsets/read-only timestamp fields are clear.
backend/apps/nest/admin/badge.py (1)
8-8: Decorator-based registration looks goodSwitching to @admin.register(Badge) is consistent with modern Django admin patterns and other admins in this PR.
trucodd
force-pushed
the
feature/owasp-awards-profilesync
branch
from
867a047 to
bc6244d
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
backend/apps/owasp/models/award.py (2)
24-33: Add a composite UniqueConstraint to allow multiple years and winners while preventing duplicatesEnforce uniqueness at the tuple level: (name, year, category, winner_name). This allows the same award each year and multiple winners per year without collisions.
Apply:
class Meta: db_table = "owasp_awards" indexes = [ models.Index(fields=["category", "year"], name="owasp_award_category_year"), models.Index(fields=["-year"], name="owasp_award_year_desc"), models.Index(fields=["name"], name="owasp_award_name"), ] verbose_name = "Award" verbose_name_plural = "Awards" + constraints = [ + models.UniqueConstraint( + fields=["name", "year", "category", "winner_name"], + name="unique_award_name_year_category_winner", + ) + ]
154-158: Persist description and sanitize winner fields; avoid None writes
- The YAML description is dropped; store it.
- Coalesce winner fields to empty strings and strip them to avoid writing None into non-nullable fields.
Apply:
- award_defaults = { - "description": "", - "winner_info": winner_data.get("info", ""), - "winner_image": winner_data.get("image", ""), - } + award_defaults = { + "description": (award_data.get("description") or "").strip(), + "winner_info": (winner_data.get("info") or "").strip(), + "winner_image": (winner_data.get("image") or "").strip(), + }backend/apps/owasp/migrations/0045_award.py (1)
107-116: Add a composite UniqueConstraint on (name, year, category, winner_name)Align DB constraints with intended semantics and the model.
Apply:
options={ "verbose_name": "Award", "verbose_name_plural": "Awards", "db_table": "owasp_awards", - "indexes": [ + "indexes": [ models.Index(fields=["category", "year"], name="owasp_award_category_year"), models.Index(fields=["-year"], name="owasp_award_year_desc"), models.Index(fields=["name"], name="owasp_award_name"), ], + "constraints": [ + models.UniqueConstraint( + fields=("name", "year", "category", "winner_name"), + name="unique_award_name_year_category_winner", + ) + ], },
🧹 Nitpick comments (2)
backend/apps/owasp/models/award.py (2)
141-148: Normalize parsed strings to reduce duplicate rows caused by whitespace/caseTrim award title and category and validate category against choices to prevent accidental duplicates due to spacing.
Apply:
- award_name = award_data.get("title", "") - category = award_data.get("category", "") + award_name = (award_data.get("title") or "").strip() + category = (award_data.get("category") or "").strip() + # Optional: validate category + if category not in dict(Award.Category.choices): + # If YAML differs only in case/spacing, try a tolerant match + normalized = {k.lower(): k for k, _ in Award.Category.choices} + key = category.lower() + category = normalized.get(key, category)
50-53: Optional: Make year nullable if you plan to store category definition entriesIf you ever persist “category” type rows from YAML (no year), year should be nullable; if not, keep as-is.
- year = models.IntegerField( + year = models.IntegerField( verbose_name="Year", - help_text="Year the award was given", + help_text="Year the award was given", + null=True, + blank=True, )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
backend/Makefile(2 hunks)backend/apps/nest/admin/badge.py(1 hunks)backend/apps/owasp/Makefile(2 hunks)backend/apps/owasp/admin/award.py(1 hunks)backend/apps/owasp/management/commands/owasp_update_badges.py(1 hunks)backend/apps/owasp/migrations/0045_award.py(1 hunks)backend/apps/owasp/migrations/0046_merge_0045_badge_0045_project_audience.py(1 hunks)backend/apps/owasp/models/award.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- backend/apps/owasp/admin/award.py
- backend/apps/owasp/migrations/0046_merge_0045_badge_0045_project_audience.py
- backend/Makefile
- backend/apps/owasp/management/commands/owasp_update_badges.py
- backend/apps/nest/admin/badge.py
🧰 Additional context used
🧬 Code Graph Analysis (2)
backend/apps/owasp/models/award.py (1)
backend/apps/common/models.py (1)
TimestampedModel(37-46)
backend/apps/owasp/migrations/0045_award.py (1)
backend/apps/owasp/migrations/0046_merge_0045_badge_0045_project_audience.py (1)
Migration(6-12)
🔇 Additional comments (3)
backend/apps/owasp/models/award.py (2)
17-23: Categories defined via TextChoices look correctThe two categories requested (“WASPY”, “Distinguished Lifetime Memberships”) are present and typed as choices.
99-123: Query helpers are concise and appropriate
- get_waspy_award_winners: distinct user set via reverse relation is efficient and readable.
- get_user_waspy_awards: straightforward filter; consider adding .only(...) in call sites if needed.
backend/apps/owasp/migrations/0045_award.py (1)
111-114: No issues with descending index configurationThe project’s default database engine is PostgreSQL (
django.db.backends.postgresql), which fully supports descending indexes. Themodels.Index(fields=["-year"], …)declaration will be applied correctly—no changes to the migration are needed.
| owasp-sync-awards: | ||
| @echo "Syncing OWASP awards data" | ||
| @CMD="python manage.py owasp_sync_awards" $(MAKE) exec-backend-command |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add a PHONY declaration and verify command existence
- Recommend declaring this target as phony to avoid file/dir name collisions.
- Please verify the management command exists and is wired into your Python path in the container/runtime.
You can run this script to verify the command files exist and basic callable structure:
Optionally add a .PHONY entry:
Makefile snippet (outside the changed hunk):
.PHONY: owasp-sync-awards
🏁 Script executed:
#!/bin/bash
# Verify management command modules exist and include a Command class
set -euo pipefail
echo "Searching for owasp_sync_awards command..."
fd -a 'owasp_sync_awards.py'
rg -n --glob '**/owasp_sync_awards.py' $'class Command\\(' -A 3 || {
echo "ERROR: Command class not found in owasp_sync_awards.py" >&2; exit 1;
}
echo "Attempting to list management commands..."
# This ensures Django can discover the command (does not run it)
python - <<'PY'
import os
os.environ.setdefault("DJANGO_SETTINGS_MODULE", "backend.settings")
from django.core import management
commands = management.get_commands()
assert "owasp_sync_awards" in commands, "owasp_sync_awards not registered"
print("Found owasp_sync_awards in Django command registry")
PYLength of output: 954
🏁 Script executed:
#!/bin/bash
# Verify import of BaseCommand in the management command file
set -euo pipefail
# Locate the command file
CMD_FILE=$(fd -a 'owasp_sync_awards.py')
echo "Found command file at: $CMD_FILE"
# Check for proper BaseCommand import
rg -n "from django.core.management.base import BaseCommand" "$CMD_FILE" || {
echo "ERROR: Missing BaseCommand import in $CMD_FILE" >&2
exit 1
}
# Show the top of the file for context
echo "---- Top of $CMD_FILE ----"
head -n 20 "$CMD_FILE"
echo "--------------------------"Length of output: 1470
Add .PHONY for the owasp-sync-awards target
We’ve confirmed that
- The file
backend/apps/owasp/management/commands/owasp_sync_awards.pyexists - It imports
BaseCommandand definesclass Command(BaseCommand)
To avoid Make interpreting owasp-sync-awards as a real file/directory, please declare it as phony before your target:
Makefile (backend/apps/owasp/Makefile) – add above your target:
.PHONY: owasp-sync-awards
owasp-sync-awards:
@echo "Syncing OWASP awards data"
@CMD="python manage.py owasp_sync_awards" $(MAKE) exec-backend-command
🤖 Prompt for AI Agents
In backend/apps/owasp/Makefile around lines 29 to 31, the owasp-sync-awards
target is not declared phony which can cause Make to treat it as a real file;
add a .PHONY declaration for that target above the target definition (i.e.,
insert a line ".PHONY: owasp-sync-awards" immediately before the
owasp-sync-awards recipe) so Make always runs the recipe instead of checking the
filesystem.
25e47e3 to
02f6581
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/tests/apps/owasp/models/award_test.py (1)
1-1: Re-check model/test sync for award_type and add a targeted test if it exists.Prior review flagged award_type present in migration but missing on the model. If award_type remains, add a simple test to assert the field exists and choices align with migration; else, remove it from migrations to keep parity.
Run this script to verify the current state and locate any award_type discrepancies:
#!/bin/bash set -euo pipefail echo "Searching for 'award_type' across the repo..." rg -n --hidden 'award_type' || true echo echo "Inspect Award model(s):" fd -a 'award.py' backend | xargs -I{} bash -lc 'echo "==> {}"; rg -n "class Award|award_type|TextChoices|Meta|db_table" "{}" || true' echo echo "Inspect OWASP migrations referencing Award:" fd -a 'migrations' backend/apps/owasp | xargs -I{} rg -n --heading 'award_type|class Award|migrations\.CreateModel|migrations\.AddField' {} || true
🧹 Nitpick comments (5)
backend/tests/apps/owasp/models/award_test.py (5)
6-8: DRY imports with setUpTestData to reduce repetition.Centralize the Award import once for reuse across tests.
Apply this diff:
class AwardModelTest(TestCase): """Test cases for Award model.""" - + @classmethod + def setUpTestData(cls): + from apps.owasp.models.award import Award + cls.Award = Award
9-14: Prefer unittest assertions over bare asserts.Bare asserts can be optimized away and provide poorer failure messages.
Apply this diff:
def test_award_import(self): """Test that Award model can be imported.""" from apps.owasp.models.award import Award - assert Award is not None + self.assertIsNotNone(Award)
15-25: Use class-cached model and unittest assertions.Removes redundant import and uses assertIn for clearer failures.
Apply this diff:
def test_award_category_choices(self): """Test Award category choices.""" - from apps.owasp.models.award import Award - - choices = Award.Category.choices - assert ("WASPY", "WASPY") in choices - assert ( - "Distinguished Lifetime Memberships", - "Distinguished Lifetime Memberships", - ) in choices + choices = self.Award.Category.choices + self.assertIn(("WASPY", "WASPY"), choices) + self.assertIn(("Distinguished Lifetime Memberships", "Distinguished Lifetime Memberships"), choices)
26-31: Use unittest assertions and verify verbose_name_plural as well.This adds a useful extra assertion and improves assertion clarity.
Apply this diff:
def test_award_meta(self): """Test Award model meta.""" - from apps.owasp.models.award import Award - - assert Award._meta.db_table == "owasp_awards" - assert Award._meta.verbose_name == "Award" + self.assertEqual(self.Award._meta.db_table, "owasp_awards") + self.assertEqual(self.Award._meta.verbose_name, "Award") + self.assertEqual(self.Award._meta.verbose_name_plural, "Awards")
6-7: Consider adding tests for unique constraints and model dunder methods.
- Validate that name is unique (creating a second Award with the same name should raise IntegrityError).
- Add a str test (commonly returns name) if implemented.
These will exercise real persistence concerns beyond metadata.I can draft these tests once you confirm required fields on Award (beyond category, name, description, year).
📜 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 settings in your CodeRabbit configuration.
📒 Files selected for processing (9)
backend/Makefile(2 hunks)backend/apps/nest/admin/badge.py(1 hunks)backend/apps/owasp/Makefile(2 hunks)backend/apps/owasp/admin/award.py(1 hunks)backend/apps/owasp/management/commands/owasp_update_badges.py(1 hunks)backend/apps/owasp/migrations/0045_award.py(1 hunks)backend/apps/owasp/migrations/0046_merge_0045_badge_0045_project_audience.py(1 hunks)backend/apps/owasp/models/award.py(1 hunks)backend/tests/apps/owasp/models/award_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- backend/apps/owasp/migrations/0046_merge_0045_badge_0045_project_audience.py
- backend/Makefile
- backend/apps/owasp/management/commands/owasp_update_badges.py
- backend/apps/nest/admin/badge.py
- backend/apps/owasp/migrations/0045_award.py
- backend/apps/owasp/models/award.py
- backend/apps/owasp/Makefile
- backend/apps/owasp/admin/award.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/tests/apps/owasp/models/award_test.py (1)
backend/apps/owasp/models/award.py (2)
Award(10-183)Category(17-22)
🔇 Additional comments (1)
backend/tests/apps/owasp/models/award_test.py (1)
1-5: Solid baseline smoke tests for Award model metadata and choices.This covers importability, Category.choices, and essential Meta attributes. Good starting coverage.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (6)
backend/apps/owasp/admin/award.py (1)
8-10: Enforce data integrity between award_type/year/winner fields.Add model-level validation (clean) to ensure category entries have year=None and no winner fields; award entries must have non-null year. Surface these errors in admin forms.
backend/apps/owasp/models/award.py (4)
49-52: Make year nullable to support category rows.Category entries don't have a year; current non-null constraint will fail saves.
- year = models.IntegerField( - verbose_name="Year", - help_text="Year the award was given", - ) + year = models.IntegerField( + verbose_name="Year", + help_text="Year the award was given (null for category definitions)", + blank=True, + null=True, + )
24-33: Add unique constraint to ensure idempotent inserts per winner.Prevents duplicates and aligns with get_or_create(name, category, year, winner_name).
class Meta: db_table = "owasp_awards" indexes = [ models.Index(fields=["category", "year"], name="owasp_award_category_year"), models.Index(fields=["-year"], name="owasp_award_year_desc"), models.Index(fields=["name"], name="owasp_award_name"), ] verbose_name = "Award" verbose_name_plural = "Awards" + constraints = [ + models.UniqueConstraint( + fields=["name", "category", "year", "winner_name"], + name="unique_award_name_category_year_winner", + ) + ]
153-157: Persist YAML description and award_type; normalize strings.Currently description is dropped and award_type is not set on created/updated rows.
- award_defaults = { - "description": "", - "winner_info": winner_data.get("info", ""), - "winner_image": winner_data.get("image", ""), - } + award_defaults = { + "description": (award_data.get("description") or "").strip(), + "winner_info": (winner_data.get("info") or "").strip(), + "winner_image": (winner_data.get("image") or "").strip(), + "award_type": Award.EntryType.AWARD, + }
17-23: Critical: add missingaward_typefield to Award model — management command depends on itThe management command backend/apps/owasp/management/commands/owasp_sync_awards.py calls get_or_create with award_type (seen at lines with award_type="category" and "award"), but backend/apps/owasp/models/award.py currently has no
award_typefield — this will raise FieldError/TypeError at runtime. Add the EntryType choices and the award_type CharField, and create/apply a migration.Files to update:
- backend/apps/owasp/models/award.py — add EntryType TextChoices and award_type CharField near the Category/TextChoices and model fields.
- backend/apps/owasp/management/commands/owasp_sync_awards.py — references found at:
- backend/apps/owasp/management/commands/owasp_sync_awards.py:103 (award_type="category")
- backend/apps/owasp/management/commands/owasp_sync_awards.py:174 ( "award_type": "award")
Suggested diff to apply in backend/apps/owasp/models/award.py:
@@ class Category(models.TextChoices): WASPY = "WASPY", "WASPY" DISTINGUISHED_LIFETIME = ( "Distinguished Lifetime Memberships", "Distinguished Lifetime Memberships", ) + + class EntryType(models.TextChoices): + CATEGORY = "category", "Category" + AWARD = "award", "Award" @@ winner_image = models.CharField( verbose_name="Winner Image", max_length=500, blank=True, default="", help_text="Path to winner's image", ) + + award_type = models.CharField( + verbose_name="Entry Type", + max_length=20, + choices=EntryType.choices, + default=EntryType.AWARD, + help_text="Type of entry: 'category' rows define a category; 'award' rows represent a winner", + )backend/apps/owasp/management/commands/owasp_sync_awards.py (1)
259-271: Fuzzy matching is too permissive; risks mis-assigning users.Require at least two tokens and both to match; avoid single-token partial hits.
- def _try_fuzzy_name_match(self, clean_name: str, min_name_parts: int) -> User | None: - """Try fuzzy name matching with partial matches.""" - name_parts = clean_name.split() - if len(name_parts) >= min_name_parts: - # Try "FirstName LastName" variations - for i in range(len(name_parts)): - for j in range(i + 1, len(name_parts) + 1): - partial_name = " ".join(name_parts[i:j]) - try: - return User.objects.get(name__icontains=partial_name) - except (User.DoesNotExist, User.MultipleObjectsReturned): - continue - return None + def _try_fuzzy_name_match(self, clean_name: str, min_name_parts: int) -> User | None: + """Fuzzy match by requiring at least two tokens present in name.""" + parts = [p for p in clean_name.split() if p] + if len(parts) < max(min_name_parts, 2): + return None + first, last = parts[0], parts[-1] + qs = User.objects.filter(name__icontains=first).filter(name__icontains=last) + count = qs.count() + if count == 1: + return qs.first() + return None
🧹 Nitpick comments (2)
backend/apps/owasp/admin/award.py (1)
34-35: Prefer list_select_related over overriding get_queryset.Cleaner and declarative; avoids method override.
@@ - autocomplete_fields = ("user",) + autocomplete_fields = ("user",) + list_select_related = ("user",) @@ - def get_queryset(self, request): - """Optimize queryset with select_related.""" - return super().get_queryset(request).select_related("user") + # get_queryset override not needed with list_select_relatedAlso applies to: 59-61
backend/apps/owasp/management/commands/owasp_sync_awards.py (1)
51-66: Handle YAML parse errors explicitly (optional).Catching yaml.YAMLError gives clearer diagnostics than a broad Exception.
- awards_data = yaml.safe_load(yaml_content) + try: + awards_data = yaml.safe_load(yaml_content) + except yaml.YAMLError as ye: + logger.exception("YAML parsing error for awards.yml") + self.stdout.write(self.style.ERROR(f"Failed to parse awards.yml: {ye!s}")) + return
📜 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 settings in your CodeRabbit configuration.
📒 Files selected for processing (15)
backend/Makefile(2 hunks)backend/apps/nest/admin/badge.py(1 hunks)backend/apps/owasp/Makefile(2 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/management/commands/owasp_update_badges.py(1 hunks)backend/apps/owasp/migrations/0045_award.py(1 hunks)backend/apps/owasp/migrations/0045_badge.py(0 hunks)backend/apps/owasp/migrations/0046_merge_0045_badge_0045_project_audience.py(1 hunks)backend/apps/owasp/models/__init__.py(1 hunks)backend/apps/owasp/models/award.py(1 hunks)backend/tests/apps/nest/management/__init__.py(1 hunks)backend/tests/apps/nest/management/commands/__init__.py(1 hunks)backend/tests/apps/owasp/models/award_test.py(1 hunks)
💤 Files with no reviewable changes (1)
- backend/apps/owasp/migrations/0045_badge.py
✅ Files skipped from review due to trivial changes (1)
- backend/Makefile
🚧 Files skipped from review as they are similar to previous changes (10)
- backend/tests/apps/nest/management/init.py
- backend/apps/owasp/migrations/0046_merge_0045_badge_0045_project_audience.py
- backend/tests/apps/nest/management/commands/init.py
- backend/tests/apps/owasp/models/award_test.py
- backend/apps/owasp/admin/init.py
- backend/apps/owasp/management/commands/owasp_update_badges.py
- backend/apps/owasp/models/init.py
- backend/apps/nest/admin/badge.py
- backend/apps/owasp/Makefile
- backend/apps/owasp/migrations/0045_award.py
🧰 Additional context used
🧬 Code Graph Analysis (3)
backend/apps/owasp/admin/award.py (1)
backend/apps/owasp/models/award.py (1)
Award(10-183)
backend/apps/owasp/models/award.py (1)
backend/apps/common/models.py (1)
TimestampedModel(37-46)
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-183)
backend/apps/owasp/admin/award.py
Outdated
| list_display = ( | ||
| "name", | ||
| "category", | ||
| "year", | ||
| "winner_name", | ||
| "user", | ||
| "nest_created_at", | ||
| "nest_updated_at", | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Expose and filter by award_type in admin (once model field is added).
Admin needs visibility into entry type to distinguish category rows vs actual awards.
Apply these diffs:
@@
list_display = (
"name",
"category",
+ "award_type",
"year",
"winner_name",
"user",
"nest_created_at",
"nest_updated_at",
)
@@
list_filter = (
"category",
+ "award_type",
"year",
)
@@
(
"Basic Information",
- {"fields": ("name", "category", "year", "description")},
+ {"fields": ("name", "category", "award_type", "year", "description")},
),Also applies to: 21-24, 38-40
🤖 Prompt for AI Agents
In backend/apps/owasp/admin/award.py around lines 12-20, 21-24, and 38-40, add
the new model field "award_type" to the admin configuration so the admin can see
and filter by entry type: include "award_type" in the list_display tuple at
lines 12-20 and add it to the appropriate list_filter (and any other admin
tuples for visible columns or default ordering shown at lines 21-24 and 38-40)
so that category rows and actual awards are distinguishable once the model field
exists.
| # Create or update category record | ||
| if not dry_run: | ||
| award, created = Award.objects.get_or_create( | ||
| name=category_name, | ||
| award_type="category", | ||
| defaults={ | ||
| "category": category_name, | ||
| "description": description, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use model enum instead of string literals for award_type.
Prevents typos and centralizes truth in the model.
- award, created = Award.objects.get_or_create(
- name=category_name,
- award_type="category",
+ award, created = Award.objects.get_or_create(
+ name=category_name,
+ award_type=Award.EntryType.CATEGORY,
defaults={
"category": category_name,
"description": description,
},
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Create or update category record | |
| if not dry_run: | |
| award, created = Award.objects.get_or_create( | |
| name=category_name, | |
| award_type="category", | |
| defaults={ | |
| "category": category_name, | |
| "description": description, | |
| }, | |
| ) | |
| # Create or update category record | |
| if not dry_run: | |
| award, created = Award.objects.get_or_create( | |
| name=category_name, | |
| award_type=Award.EntryType.CATEGORY, | |
| defaults={ | |
| "category": category_name, | |
| "description": description, | |
| }, | |
| ) |
🤖 Prompt for AI Agents
In backend/apps/owasp/management/commands/owasp_sync_awards.py around lines 99
to 108, the code passes the literal "category" into award_type; replace that
string with the model's enum/constant (for example Award.AwardType.CATEGORY or
the specific Award class constant your model defines) by importing/referencing
the enum from the Award model and using it in the get_or_create call so
award_type uses the canonical enum value instead of a raw string.
| def _process_award(self, award_data: dict, *, dry_run: bool = False): | ||
| """Process an individual award.""" | ||
| award_name = award_data.get("title", "") | ||
| category = award_data.get("category", "") | ||
| year = award_data.get("year") | ||
| winners = award_data.get("winners", []) | ||
|
|
||
| if not award_name or not category or not year: | ||
| logger.warning("Skipping incomplete award: %s", award_data) | ||
| return | ||
|
|
||
| # Process each winner | ||
| for winner_data in winners: | ||
| self._process_winner(award_name, category, year, winner_data, dry_run=dry_run) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Thread description through to _process_winner.
award_description is needed for defaults above.
def _process_award(self, award_data: dict, *, dry_run: bool = False):
"""Process an individual award."""
award_name = award_data.get("title", "")
category = award_data.get("category", "")
year = award_data.get("year")
+ award_description = award_data.get("description", "") or ""
winners = award_data.get("winners", [])
@@
# Process each winner
for winner_data in winners:
- self._process_winner(award_name, category, year, winner_data, dry_run=dry_run)
+ self._process_winner(
+ award_name, category, year, winner_data, award_description=award_description, dry_run=dry_run
+ )
@@
- def _process_winner(
+ def _process_winner(
self,
award_name: str,
category: str,
year: int,
winner_data: dict,
*,
+ award_description: str = "",
dry_run: bool = False,
):Also applies to: 137-145
🤖 Prompt for AI Agents
In backend/apps/owasp/management/commands/owasp_sync_awards.py around lines 122
to 136 (and likewise for 137-145), the award processing is missing
award_description defaults and is not passing the description through to
_process_winner; extract award_description = award_data.get("description", "")
(or a sensible default), use it when validating/skipping incomplete awards if
required, and pass award_description as an additional argument to
self._process_winner for each winner (and update _process_winner/_process_winner
signature/consumers accordingly) so winners receive the award description
defaults.
| if not dry_run: | ||
| # Create or update award record | ||
| award, created = Award.objects.get_or_create( | ||
| name=award_name, | ||
| category=category, | ||
| year=year, | ||
| winner_name=winner_name, | ||
| defaults={ | ||
| "award_type": "award", | ||
| "description": "", | ||
| "winner_info": winner_info, | ||
| "winner_image": winner_image, | ||
| "user": matched_user, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use model enum and carry description through to award rows.
Propagate award description to each winner row; avoid hardcoded strings.
Consider passing description down from _process_award (see next comment).
award, created = Award.objects.get_or_create(
name=award_name,
category=category,
year=year,
winner_name=winner_name,
defaults={
- "award_type": "award",
- "description": "",
+ "award_type": Award.EntryType.AWARD,
+ "description": award_description,
"winner_info": winner_info,
"winner_image": winner_image,
"user": matched_user,
},
)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In backend/apps/owasp/management/commands/owasp_sync_awards.py around lines 166
to 180, replace the hardcoded award_type string and empty description with the
model enum and the actual description value passed down from _process_award: use
the Award model's enum constant for the award_type (e.g. Award.AwardType.AWARD
or the project's actual enum attribute) instead of "award", pass the real
description variable into defaults for get_or_create, and ensure that when an
existing award is found you also update its description (and award_type if
needed) so the winner rows carry the propagated description; adjust
_process_award to accept/forward the description if it doesn’t already.
backend/apps/owasp/models/award.py
Outdated
| indexes = [ | ||
| models.Index(fields=["category", "year"], name="owasp_award_category_year"), | ||
| models.Index(fields=["-year"], name="owasp_award_year_desc"), | ||
| models.Index(fields=["name"], name="owasp_award_name"), |
There was a problem hiding this comment.
I'd prefer to introduce indexes later.
backend/apps/owasp/models/award.py
Outdated
| default="", | ||
| help_text="Detailed information about the winner", | ||
| ) | ||
| winner_image = models.CharField( |
There was a problem hiding this comment.
It probably could be winner_image_url with proper URL type?
| verbose_name="User", | ||
| help_text="Associated GitHub user (if matched)", | ||
| ) | ||
|
|
There was a problem hiding this comment.
I think the matching process needs to be verified by a human. Could you introduce is_reviewed flag for this model?
backend/apps/owasp/models/award.py
Outdated
| return None | ||
|
|
||
| @staticmethod | ||
| def _create_awards_from_winners(award_data: dict, *, save: bool = True): |
There was a problem hiding this comment.
This code doesn't look consistent with the rest of models where update_data is responsible for a single instance creation/update logic.
backend/apps/owasp/models/award.py
Outdated
| return cls.objects.filter(user=user, category=cls.Category.WASPY) | ||
|
|
||
| @staticmethod | ||
| def update_data(award_data: dict, *, save: bool = True): |
There was a problem hiding this comment.
This one should wait until the initial badge processing job is implemented in another PR. Then we'll just update it with additional logic.
trucodd
force-pushed
the
feature/owasp-awards-profilesync
branch
from
02f6581 to
1a25b2d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
backend/apps/owasp/management/commands/owasp_update_badges.py (2)
3-7: Add transaction import for atomic badge updatesWe’ll wrap handle() in a transaction to make add/remove operations consistent.
from django.core.management.base import BaseCommand +from django.db import transaction from apps.github.models.user import User from apps.nest.models.badge import Badge from apps.owasp.models.award import Award
15-47: Avoid N+1 queries; diff sets and report actual adds/removesCompute set differences and only touch users whose badge membership actually changes. This also produces accurate logs.
def handle(self, *args, **options): """Handle the command execution.""" - # Get or create WASPY badge - waspy_badge, created = Badge.objects.get_or_create( - name="WASPY Award Winner", - defaults={ - "description": "Recipient of WASPY award from OWASP", - "css_class": "badge-waspy", - "weight": 10, - }, - ) - - if created: - self.stdout.write(f"Created badge: {waspy_badge.name}") - - # Get users with reviewed WASPY awards only - waspy_winners = User.objects.filter( - awards__category=Award.Category.WASPY, awards__is_reviewed=True - ).distinct() - - # Add badge to WASPY winners - for user in waspy_winners: - user.badges.add(waspy_badge) - - # Remove badge from users without reviewed WASPY awards - users_with_badge = User.objects.filter(badges=waspy_badge) - for user in users_with_badge: - if not Award.objects.filter( - user=user, category=Award.Category.WASPY, is_reviewed=True - ).exists(): - user.badges.remove(waspy_badge) - - self.stdout.write(f"Updated badges for {waspy_winners.count()} WASPY winners") + with transaction.atomic(): + # Get or create WASPY badge + waspy_badge, created = Badge.objects.get_or_create( + name="WASPY Award Winner", + defaults={ + "description": "Recipient of WASPY award from OWASP", + "css_class": "badge-waspy", + "weight": 10, + }, + ) + if created: + self.stdout.write(f"Created badge: {waspy_badge.name}") + + # Compute desired vs current badge holders + winner_ids = set( + User.objects.filter( + awards__category=Award.Category.WASPY, awards__is_reviewed=True + ) + .distinct() + .values_list("id", flat=True) + ) + current_ids = set( + User.objects.filter(badges=waspy_badge).values_list("id", flat=True) + ) + + to_add_ids = winner_ids - current_ids + to_remove_ids = current_ids - winner_ids + + if to_add_ids: + for user in User.objects.filter(id__in=to_add_ids): + user.badges.add(waspy_badge) + if to_remove_ids: + for user in User.objects.filter(id__in=to_remove_ids): + user.badges.remove(waspy_badge) + + self.stdout.write( + f"WASPY badge updated: +{len(to_add_ids)} added, -{len(to_remove_ids)} removed, total_winners={len(winner_ids)}" + )backend/apps/owasp/models/award.py (1)
24-28: Enforce idempotency with a DB-level unique constraintWithout a uniqueness constraint, duplicates can slip in (or a manual duplicate can break update_data via MultipleObjectsReturned). Constrain by (name, category, year, winner_name).
class Meta: db_table = "owasp_awards" verbose_name = "Award" verbose_name_plural = "Awards" + constraints = [ + models.UniqueConstraint( + fields=["name", "category", "year", "winner_name"], + name="unique_award_name_year_category_winner", + ) + ]backend/apps/owasp/migrations/0045_award.py (1)
113-119: Add a unique constraint to back the model’s idempotency contractMatch the model change and ensure DB-level protection against duplicates of the same award/winner/year/category.
options={ "verbose_name": "Award", "verbose_name_plural": "Awards", "db_table": "owasp_awards", + "constraints": [ + models.UniqueConstraint( + fields=("name", "year", "category", "winner_name"), + name="unique_award_name_year_category_winner", + ) + ], },backend/apps/owasp/management/commands/owasp_sync_awards.py (2)
133-141: Verify DB uniqueness includes winner_name to prevent IntegrityErrorBoth the pre-check and update_data identify rows by (name, category, year, winner_name). Ensure the model/migration UniqueConstraint uses these four fields so get-or-create behavior is truly idempotent.
Run to verify constraints reference winner_name:
#!/usr/bin/env bash set -euo pipefail echo "Model constraints referencing winner_name:" rg -nP -C2 'UniqueConstraint|unique_together|constraints' backend/apps/owasp/models/award.py | rg -nP 'winner_name|UniqueConstraint|unique_together' -n -C2 || true echo echo "Migrations that define unique constraints for Award:" fd -t f award.py backend/apps/owasp/migrations | while read -r f; do echo "---- $f" rg -nP -C2 'UniqueConstraint|unique_together|constraints|AlterUniqueTogether' "$f" | rg -nP 'winner_name|UniqueConstraint|unique_together|AlterUniqueTogether' -n -C2 || true doneExpected: a UniqueConstraint on (name, category, year, winner_name).
217-229: Fuzzy matching is too permissive; restrict to two-token match (first+last) to avoid mis-assignmentsSingle-token partial matches can link awards to the wrong user (e.g., “John”). Restrict matching to require at least two distinct tokens and both must be present.
- def _try_fuzzy_name_match(self, clean_name: str, min_name_parts: int) -> User | None: - """Try fuzzy name matching with partial matches.""" - name_parts = clean_name.split() - if len(name_parts) >= min_name_parts: - # Try "FirstName LastName" variations - for i in range(len(name_parts)): - for j in range(i + 1, len(name_parts) + 1): - partial_name = " ".join(name_parts[i:j]) - try: - return User.objects.get(name__icontains=partial_name) - except (User.DoesNotExist, User.MultipleObjectsReturned): - continue - return None + def _try_fuzzy_name_match(self, clean_name: str, min_name_parts: int) -> User | None: + """Require at least two tokens and match on first AND last tokens.""" + parts = [p for p in re.split(r"\s+", clean_name) if p] + if len(parts) < max(2, min_name_parts): + return None + first, last = parts[0], parts[-1] + qs = User.objects.filter(name__icontains=first).filter(name__icontains=last) + if qs.count() == 1: + return qs.first() + return None
🧹 Nitpick comments (8)
backend/tests/apps/owasp/models/award_test.py (2)
11-11: Import via the public models package to validate API surfaceImporting from apps.owasp.models ensures the public export is wired (via init.py) and avoids coupling tests to module layout.
- from apps.owasp.models.award import Award + from apps.owasp.models import Award @@ - from apps.owasp.models.award import Award + from apps.owasp.models import Award @@ - from apps.owasp.models.award import Award + from apps.owasp.models import AwardAlso applies to: 17-17, 28-28
6-32: Broaden coverage: add idempotency and parsing tests for update_data()Current tests validate choices and meta only. Given the sync relies on Award.update_data(), add:
- A test proving update_data is idempotent (no duplicate rows on repeated calls).
- A test verifying mapping of YAML keys (title/category/year/name/description/info/image) to model fields.
I can draft these tests using factories or minimal dicts; say the word and I’ll open a follow-up PR with them.
backend/apps/owasp/admin/award.py (1)
36-69: Add admin actions to mark awards reviewed/unreviewed in bulkUseful for triaging matched users quickly.
autocomplete_fields = ("user",) + actions = ("mark_reviewed", "mark_unreviewed") @@ readonly_fields = ("nest_created_at", "nest_updated_at") + def mark_reviewed(self, request, queryset): + updated = queryset.update(is_reviewed=True) + self.message_user(request, f"Marked {updated} award(s) as reviewed.") + + mark_reviewed.short_description = "Mark selected awards as reviewed" + + def mark_unreviewed(self, request, queryset): + updated = queryset.update(is_reviewed=False) + self.message_user(request, f"Marked {updated} award(s) as unreviewed.") + + mark_unreviewed.short_description = "Mark selected awards as unreviewed" + def get_queryset(self, request): """Optimize queryset with select_related.""" return super().get_queryset(request).select_related("user")backend/apps/owasp/models/award.py (1)
145-158: Use get_or_create to simplify and avoid racey get/except flowsThis tightens the idempotent path and reduces boilerplate. Still relies on the unique constraint above to prevent duplicates.
- try: - award = Award.objects.get( - name=name, - category=category, - year=year, - winner_name=winner_name, - ) - except Award.DoesNotExist: - award = Award( - name=name, - category=category, - year=year, - winner_name=winner_name, - ) + award, _ = Award.objects.get_or_create( + name=name, + category=category, + year=year, + winner_name=winner_name, + )backend/apps/owasp/management/commands/owasp_sync_awards.py (4)
61-66: Catch yaml.YAMLError to surface parse failures preciselysafe_load can raise yaml.YAMLError. Catching it yields a clearer failure mode than the generic exception handler.
- awards_data = yaml.safe_load(yaml_content) - - if not awards_data: - self.stdout.write(self.style.ERROR("Failed to parse awards.yml content")) - return + try: + awards_data = yaml.safe_load(yaml_content) + except yaml.YAMLError as e: + self.stdout.write(self.style.ERROR(f"Failed to parse awards.yml: {e}")) + return + + if not awards_data: + self.stdout.write(self.style.ERROR("Failed to parse awards.yml content")) + return
133-161: Avoid double DB lookups when determining created vs updatedYou first query to determine is_new, then call Award.update_data which internally does its own get/create. This doubles DB hits per winner.
Option A (leave as-is for clarity) or Option B (preferred): make update_data return (award, created) and use that here.
Proposed change in this file if update_data returns a tuple:
- # Check if award exists before update - try: - Award.objects.get( - name=award_name, - category=winner_data.get("category", ""), - year=winner_data.get("year"), - winner_name=winner_name, - ) - is_new = False - except Award.DoesNotExist: - is_new = True - - # Use the model's update_data method - award = Award.update_data(winner_data, save=True) + # Create or update in one call; update_data returns (award, created) + award, is_new = Award.update_data(winner_data, save=True)Outside this file (model change) you can update Award.update_data to:
def update_data(award_data: dict, *, save: bool = True) -> tuple["Award", bool]: # return (award, created)I can draft the model changes if you want to pursue this.
287-295: Prune impossible login variants (underscores aren’t valid GitHub usernames)This reduces unnecessary queries while keeping your 10-variant cap.
unique_logins = [] for login in potential_logins: - if login and login not in seen and len(login) >= min_login_length: + # Skip invalid characters for GitHub logins + if "_" in login: + continue + if login and login not in seen and len(login) >= min_login_length: seen.add(login) unique_logins.append(login)
44-60: Nit: surface HTTP fetch timeouts and source URL as optionsMinor: get_repository_file_content supports a timeout; exposing --timeout and/or --source-url can help with debugging and self-hosted mirrors. Not required for this PR.
Example:
def add_arguments(self, parser): """Add command arguments.""" parser.add_argument( "--dry-run", action="store_true", help="Run without making changes to the database", ) + parser.add_argument( + "--timeout", + type=float, + default=30, + help="HTTP timeout when fetching awards.yml (seconds)", + ) @@ - yaml_content = get_repository_file_content( - "https://raw.githubusercontent.com/OWASP/owasp.github.io/main/_data/awards.yml" - ) + source_url = "https://raw.githubusercontent.com/OWASP/owasp.github.io/main/_data/awards.yml" + yaml_content = get_repository_file_content(source_url, timeout=options.get("timeout", 30))
📜 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 (15)
backend/Makefile(2 hunks)backend/apps/nest/admin/badge.py(1 hunks)backend/apps/owasp/Makefile(2 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/management/commands/owasp_update_badges.py(1 hunks)backend/apps/owasp/migrations/0045_award.py(1 hunks)backend/apps/owasp/migrations/0045_badge.py(0 hunks)backend/apps/owasp/migrations/0046_merge_0045_badge_0045_project_audience.py(1 hunks)backend/apps/owasp/models/__init__.py(1 hunks)backend/apps/owasp/models/award.py(1 hunks)backend/tests/apps/nest/management/__init__.py(1 hunks)backend/tests/apps/nest/management/commands/__init__.py(1 hunks)backend/tests/apps/owasp/models/award_test.py(1 hunks)
💤 Files with no reviewable changes (1)
- backend/apps/owasp/migrations/0045_badge.py
🚧 Files skipped from review as they are similar to previous changes (8)
- backend/tests/apps/nest/management/init.py
- backend/tests/apps/nest/management/commands/init.py
- backend/apps/owasp/models/init.py
- backend/apps/owasp/admin/init.py
- backend/apps/owasp/migrations/0046_merge_0045_badge_0045_project_audience.py
- backend/Makefile
- backend/apps/owasp/Makefile
- backend/apps/nest/admin/badge.py
🧰 Additional context used
🧬 Code Graph Analysis (6)
backend/apps/owasp/management/commands/owasp_update_badges.py (2)
backend/apps/nest/models/badge.py (1)
Badge(10-41)backend/apps/owasp/models/award.py (2)
Award(10-180)Category(17-22)
backend/tests/apps/owasp/models/award_test.py (1)
backend/apps/owasp/models/award.py (2)
Award(10-180)Category(17-22)
backend/apps/owasp/admin/award.py (1)
backend/apps/owasp/models/award.py (1)
Award(10-180)
backend/apps/owasp/migrations/0045_award.py (1)
backend/apps/owasp/migrations/0046_merge_0045_badge_0045_project_audience.py (1)
Migration(6-12)
backend/apps/owasp/models/award.py (1)
backend/apps/common/models.py (2)
BulkSaveModel(10-34)TimestampedModel(37-46)
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 (2)
Award(10-180)update_data(128-164)
🔇 Additional comments (3)
backend/apps/owasp/admin/award.py (1)
12-27: Admin config looks solidGood defaults, useful filters, search, ordering, and select_related("user") optimization.
backend/apps/owasp/management/commands/owasp_sync_awards.py (2)
67-77: Transactional writes + dry-run separation look solidGood use of transaction.atomic() for real runs and a clear dry-run branch. This guards against partial syncs and makes the command safe to iterate on.
82-87: Filtering by “award” is correct
Fetched the canonical awards.yml shows only two types—52 award and 2 category. The code’sif item.get("type") == "award"correctly processes only award entries; category items are grouping headers and should be skipped. No change required.
| def _process_award(self, award_data: dict, *, dry_run: bool = False): | ||
| """Process an individual award.""" | ||
| award_name = award_data.get("title", "") | ||
| category = award_data.get("category", "") | ||
| year = award_data.get("year") | ||
| winners = award_data.get("winners", []) | ||
|
|
||
| if not award_name or not category or not year: | ||
| logger.warning("Skipping incomplete award: %s", award_data) | ||
| return | ||
|
|
||
| # Process each winner using the model's update_data method | ||
| for winner_data in winners: | ||
| # Prepare winner data with award context | ||
| winner_with_context = { | ||
| "title": award_name, | ||
| "category": category, | ||
| "year": year, | ||
| "name": winner_data.get("name", ""), | ||
| "info": winner_data.get("info", ""), | ||
| "image": winner_data.get("image", ""), | ||
| } | ||
|
|
||
| self._process_winner(winner_with_context, dry_run=dry_run) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Propagate award description into winner rows to satisfy model completeness
Winners created via update_data currently get an empty description because description isn’t carried into winner_with_context. The Award model requires description to mirror awards.yml.
def _process_award(self, award_data: dict, *, dry_run: bool = False):
"""Process an individual award."""
award_name = award_data.get("title", "")
category = award_data.get("category", "")
year = award_data.get("year")
+ award_description = award_data.get("description", "") or ""
winners = award_data.get("winners", [])
@@
winner_with_context = {
"title": award_name,
"category": category,
"year": year,
"name": winner_data.get("name", ""),
"info": winner_data.get("info", ""),
"image": winner_data.get("image", ""),
+ "description": award_description,
}This also aligns with the PR objective: “Award data is stored and accessible via an Award model,” including description.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _process_award(self, award_data: dict, *, dry_run: bool = False): | |
| """Process an individual award.""" | |
| award_name = award_data.get("title", "") | |
| category = award_data.get("category", "") | |
| year = award_data.get("year") | |
| winners = award_data.get("winners", []) | |
| if not award_name or not category or not year: | |
| logger.warning("Skipping incomplete award: %s", award_data) | |
| return | |
| # Process each winner using the model's update_data method | |
| for winner_data in winners: | |
| # Prepare winner data with award context | |
| winner_with_context = { | |
| "title": award_name, | |
| "category": category, | |
| "year": year, | |
| "name": winner_data.get("name", ""), | |
| "info": winner_data.get("info", ""), | |
| "image": winner_data.get("image", ""), | |
| } | |
| self._process_winner(winner_with_context, dry_run=dry_run) | |
| def _process_award(self, award_data: dict, *, dry_run: bool = False): | |
| """Process an individual award.""" | |
| award_name = award_data.get("title", "") | |
| category = award_data.get("category", "") | |
| year = award_data.get("year") | |
| award_description = award_data.get("description", "") or "" | |
| winners = award_data.get("winners", []) | |
| if not award_name or not category or not year: | |
| logger.warning("Skipping incomplete award: %s", award_data) | |
| return | |
| # Process each winner using the model's update_data method | |
| for winner_data in winners: | |
| # Prepare winner data with award context | |
| winner_with_context = { | |
| "title": award_name, | |
| "category": category, | |
| "year": year, | |
| "name": winner_data.get("name", ""), | |
| "info": winner_data.get("info", ""), | |
| "image": winner_data.get("image", ""), | |
| "description": award_description, | |
| } | |
| self._process_winner(winner_with_context, dry_run=dry_run) |
🤖 Prompt for AI Agents
In backend/apps/owasp/management/commands/owasp_sync_awards.py around lines 88
to 111, winner_with_context is missing the award's description so winners
created via update_data get an empty description; include the award description
by adding a "description": award_data.get("description", "") entry to the
winner_with_context dict (keeping existing keys and defaults) so the Award model
receives the same description as the award row.
| def _extract_github_username(self, text: str) -> str | None: | ||
| """Extract GitHub username from text using various patterns.""" | ||
| if not text: | ||
| return None | ||
|
|
||
| # Pattern 1: github.com/username | ||
| github_url_pattern = r"github\.com/([a-zA-Z0-9\-_]+)" | ||
| match = re.search(github_url_pattern, text, re.IGNORECASE) | ||
| if match: | ||
| return match.group(1) | ||
|
|
||
| # Pattern 2: @username mentions | ||
| mention_pattern = r"@([a-zA-Z0-9\-_]+)" | ||
| match = re.search(mention_pattern, text) | ||
| if match: | ||
| return match.group(1) | ||
|
|
||
| return None |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden GitHub username extraction; avoid emails and reserved paths
Current patterns will match “example” from an email ([email protected]) and can capture “orgs” from URLs like github.com/orgs/OWASP. Tighten both URL and mention patterns and exclude reserved path segments.
def _extract_github_username(self, text: str) -> str | None:
"""Extract GitHub username from text using various patterns."""
if not text:
return None
- # Pattern 1: github.com/username
- github_url_pattern = r"github\.com/([a-zA-Z0-9\-_]+)"
- match = re.search(github_url_pattern, text, re.IGNORECASE)
- if match:
- return match.group(1)
+ # Pattern 1: github.com/<username> (exclude known non-user segments)
+ excluded = {"orgs", "organizations", "topics", "enterprise", "marketplace", "settings", "apps", "features", "pricing", "sponsors"}
+ github_url_pattern = r"(?:https?://)?(?:www\.)?github\.com/([A-Za-z0-9-]+)(?=[/\s]|$)"
+ match = re.search(github_url_pattern, text, re.IGNORECASE)
+ if match:
+ candidate = match.group(1)
+ if candidate.lower() not in excluded:
+ return candidate
- # Pattern 2: @username mentions
- mention_pattern = r"@([a-zA-Z0-9\-_]+)"
- match = re.search(mention_pattern, text)
+ # Pattern 2: @username mentions (avoid emails/local-parts)
+ mention_pattern = r"(?<![A-Za-z0-9._%+-])@([A-Za-z0-9-]+)\b"
+ match = re.search(mention_pattern, text, re.IGNORECASE)
if match:
return match.group(1)
return NoneNote: GitHub usernames cannot contain underscores, so the character class now excludes “_”.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _extract_github_username(self, text: str) -> str | None: | |
| """Extract GitHub username from text using various patterns.""" | |
| if not text: | |
| return None | |
| # Pattern 1: github.com/username | |
| github_url_pattern = r"github\.com/([a-zA-Z0-9\-_]+)" | |
| match = re.search(github_url_pattern, text, re.IGNORECASE) | |
| if match: | |
| return match.group(1) | |
| # Pattern 2: @username mentions | |
| mention_pattern = r"@([a-zA-Z0-9\-_]+)" | |
| match = re.search(mention_pattern, text) | |
| if match: | |
| return match.group(1) | |
| return None | |
| def _extract_github_username(self, text: str) -> str | None: | |
| """Extract GitHub username from text using various patterns.""" | |
| if not text: | |
| return None | |
| # Pattern 1: github.com/<username> (exclude known non-user segments) | |
| excluded = { | |
| "orgs", "organizations", "topics", "enterprise", "marketplace", | |
| "settings", "apps", "features", "pricing", "sponsors" | |
| } | |
| github_url_pattern = r"(?:https?://)?(?:www\.)?github\.com/([A-Za-z0-9-]+)(?=[/\s]|$)" | |
| match = re.search(github_url_pattern, text, re.IGNORECASE) | |
| if match: | |
| candidate = match.group(1) | |
| if candidate.lower() not in excluded: | |
| return candidate | |
| # Pattern 2: @username mentions (avoid emails/local-parts) | |
| mention_pattern = r"(?<![A-Za-z0-9._%+-])@([A-Za-z0-9-]+)\b" | |
| match = re.search(mention_pattern, text, re.IGNORECASE) | |
| if match: | |
| return match.group(1) | |
| return None |
trucodd
force-pushed
the
feature/owasp-awards-profilesync
branch
from
1a25b2d to
db4dbfc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
backend/apps/owasp/Makefile (1)
29-31: Add .PHONY for owasp-sync-awards to avoid target/file collisionsMark the new target as phony so Make doesn’t skip it if a file named owasp-sync-awards exists.
Apply this addition just above the target:
+.PHONY: owasp-sync-awards owasp-sync-awards: @echo "Syncing OWASP awards data" @CMD="python manage.py owasp_sync_awards" $(MAKE) exec-backend-commandbackend/apps/owasp/models/award.py (4)
44-47: Consider making year nullable to support non-award “category” entries (verify against migration)If you store “type: category” rows (no year), year must be nullable. Prior feedback/migrations suggested this might be nullable.
Proposed change:
- year = models.IntegerField( - verbose_name="Year", - help_text="Year the award was given", - ) + year = models.IntegerField( + verbose_name="Year", + help_text="Year the award was given (null for category definitions)", + blank=True, + null=True, + )Run this script to confirm what the migration declares for year and whether null=True is expected:
#!/bin/bash set -euo pipefail rg -n -C2 "CreateModel\(['\"]Award['\"]" backend/apps/owasp/migrations rg -n -A5 -B2 "('year', models\.IntegerField" backend/apps/owasp/migrations
86-95: Avoid “(None)” in string display when year is missingGuard year in str and display_name to keep admin/UI clean.
- def __str__(self) -> str: - """Return string representation of the award.""" - if self.winner_name: - return f"{self.name} - {self.winner_name} ({self.year})" - return f"{self.name} ({self.year})" + def __str__(self) -> str: + """Return string representation of the award.""" + parts = [self.name] + if self.winner_name: + parts.append(f"- {self.winner_name}") + if self.year is not None: + parts.append(f"({self.year})") + return " ".join(parts) @@ - def display_name(self) -> str: - """Get display name for the award.""" - return f"{self.name} ({self.year})" + def display_name(self) -> str: + """Get display name for the award.""" + return f"{self.name} ({self.year})" if self.year is not None else self.name
17-23: If your migration/tests expect EntryType/award_type, add them (model/migration alignment)Some prior feedback/migrations referenced an award_type with choices (“category”, “award”). If that’s still expected, add EntryType and the award_type field; otherwise ignore.
class Award(BulkSaveModel, TimestampedModel): @@ class Category(models.TextChoices): WASPY = "WASPY", "WASPY" DISTINGUISHED_LIFETIME = ( "Distinguished Lifetime Memberships", "Distinguished Lifetime Memberships", ) + + class EntryType(models.TextChoices): + CATEGORY = "category", "Category" + AWARD = "award", "Award"Add the field (after winner_image_url):
winner_image_url = models.URLField( verbose_name="Winner Image URL", blank=True, default="", help_text="URL to winner's image", ) + + award_type = models.CharField( + verbose_name="Award Type", + max_length=20, + choices=EntryType.choices, + default="award", + help_text="Type of entry: category definition or individual award", + )Check the migration contents to confirm whether award_type is required:
#!/bin/bash set -euo pipefail rg -n "('award_type'|EntryType|TextChoices)" backend/apps/owasp/migrations -C2 || true
24-28: Add a composite uniqueness constraint to prevent duplicate rows per winnerThere’s no DB-level uniqueness, so duplicate rows can be created for the same award/year/winner. Enforce uniqueness on (name, year, category, winner_name).
class Meta: db_table = "owasp_awards" verbose_name = "Award" verbose_name_plural = "Awards" + constraints = [ + models.UniqueConstraint( + fields=["name", "year", "category", "winner_name"], + name="unique_award_name_year_category_winner", + ) + ]
🧹 Nitpick comments (2)
backend/apps/owasp/models/award.py (2)
166-181: Minor: strip incoming strings and accept alternate keys to reduce whitespace/field driftTrimming inputs and accepting alternate keys prevents stray whitespace and supports both flattened and raw shapes.
- fields = { - "description": data.get("description", ""), - "winner_info": data.get("info", ""), - "winner_image_url": data.get("image", ""), - } + fields = { + "description": (data.get("description") or "").strip(), + "winner_info": (data.get("info") or data.get("winner_info") or "").strip(), + "winner_image_url": (data.get("image") or data.get("winner_image_url") or "").strip(), + }
127-158: Normalize and trim award data in update_dataTo ensure consistent handling of incoming data, strip whitespace on the title, category, and winner name in update_data. Since the sync command already flattens the YAML winners into dicts with explicit “title” and “category” keys, we don’t need to fall back on raw “type” or “winner_name” keys here—those mappings belong in the importer if you ever call update_data on unflattened YAML entries.
• File: backend/apps/owasp/models/award.py
• Method: update_data (around lines 127–158)Recommended diff:
- name = award_data.get("title", "") - category = award_data.get("category", "") + name = (award_data.get("title") or "").strip() + category = (award_data.get("category") or "").strip() year = award_data.get("year") - winner_name = award_data.get("name", "").strip() + winner_name = (award_data.get("name") or "").strip()If you later decide to call update_data directly on raw YAML entries, handle the “type”→category and “winners”→“name” mappings in the importer (e.g. owasp_sync_awards.py) before invoking update_data.
📜 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/Makefile(1 hunks)backend/apps/owasp/Makefile(1 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/Makefile
- backend/apps/owasp/management/commands/owasp_sync_awards.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/apps/owasp/models/award.py (1)
backend/apps/common/models.py (2)
BulkSaveModel(10-34)TimestampedModel(37-46)
🔇 Additional comments (5)
backend/apps/owasp/models/award.py (5)
17-35: Hardcoded categories implemented correctly and aligned with guidanceUsing TextChoices for WASPY and Distinguished Lifetime Memberships and constraining category via choices is consistent with the “hardcode categories” direction. The field wiring and help_texts look good.
63-68: Good: winner_image_url uses URLField and matches naming feedbackRenaming to winner_image_url and using a URLField addresses earlier review feedback.
80-84: Good: is_reviewed enables human verification gateThis aligns with the requirement to allow a human review of automated matching.
97-107: WASPY winners helper looks goodQuerying Users via awards__category with distinct() is correct; import inside method avoids cycles. Keep as-is.
122-126: Static bulk_save passthrough is fineDelegating to BulkSaveModel.bulk_save maintains consistency with existing patterns.
trucodd
force-pushed
the
feature/owasp-awards-profilesync
branch
from
db4dbfc to
f831803
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/apps/owasp/models/award.py (1)
1-181: Add a unique constraint on (name, category, year, winner_name) in a follow-up migrationNo existing migration enforces uniqueness for the
Awardmodel on those four fields. Without this,update_data’s get-or-create logic can create duplicates or crash if data drifts. Please add a new Django migration, for example:from django.db import migrations, models class Migration(migrations.Migration): dependencies = [ ("owasp", "0045_award"), ] operations = [ migrations.AddConstraint( model_name="award", constraint=models.UniqueConstraint( fields=["name", "category", "year", "winner_name"], name="owasp_award_unique_fields", ), ), ] ]– Ensures the combination of name, category, year, and winner_name is enforced at the database level
– Prevents duplicate award records and future data drift inupdate_databackend/apps/owasp/management/commands/owasp_sync_awards.py (1)
1-313: Missing WASPY Award Winner badge integrationI confirmed there are no references to “WASPY Award Winner” or any badge assignment logic tied to OWASP awards:
- Badge model is defined in
backend/apps/nest/models/badge.py, but never used for OWASP awards.- No occurrences of “WASPY Award Winner” or similar in the codebase.
- No scheduled job or management command assigning/removing this badge.
Per the PR objective, we need to hook into the daily badge sync (e.g., your existing
sync_badgescommand or Celery/cron job) to:
- Call
Award.get_waspy_award_winners()- Assign the “WASPY Award Winner” badge to matching users
- Remove it from users no longer on the list
Let me know if you’d like a draft of the hook and any necessary migration/fixture.
♻️ Duplicate comments (4)
backend/apps/owasp/models/award.py (1)
24-28: Enforce idempotency with a DB-level unique constraintWithout a DB constraint, concurrent runs or external inserts can create duplicates, and your get() in update_data will crash with MultipleObjectsReturned. Add a unique constraint spanning the natural key.
class Meta: db_table = "owasp_awards" verbose_name = "Award" verbose_name_plural = "Awards" + constraints = [ + models.UniqueConstraint( + fields=["name", "category", "year", "winner_name"], + name="uniq_award_name_category_year_winner", + ) + ]backend/apps/owasp/management/commands/owasp_sync_awards.py (3)
88-111: Propagate award description to winner rowsWinners currently get an empty description, despite YAML having an award-level description. Thread it through so Award.from_dict populates description.
def _process_award(self, award_data: dict, *, dry_run: bool = False): """Process an individual award.""" award_name = award_data.get("title", "") - category = award_data.get("category", "") + category = award_data.get("category", "") year = award_data.get("year") + award_description = (award_data.get("description") or "").strip() winners = award_data.get("winners", []) @@ winner_with_context = { "title": award_name, "category": category, "year": year, "name": winner_data.get("name", ""), "info": winner_data.get("info", ""), "image": winner_data.get("image", ""), + "description": award_description, }
217-229: Fuzzy matching is too permissive; risks wrong user assignmentsSingle-token and arbitrary substrings can easily attach awards to the wrong user. Require at least first and last tokens to both match, and only proceed on an unambiguous single result.
- def _try_fuzzy_name_match(self, clean_name: str, min_name_parts: int) -> User | None: - """Try fuzzy name matching with partial matches.""" - name_parts = clean_name.split() - if len(name_parts) >= min_name_parts: - # Try "FirstName LastName" variations - for i in range(len(name_parts)): - for j in range(i + 1, len(name_parts) + 1): - partial_name = " ".join(name_parts[i:j]) - try: - return User.objects.get(name__icontains=partial_name) - except (User.DoesNotExist, User.MultipleObjectsReturned): - continue - return None + def _try_fuzzy_name_match(self, clean_name: str, min_name_parts: int) -> User | None: + """Try fuzzy match by requiring at least two tokens present in name.""" + parts = [p for p in clean_name.split() if p] + if len(parts) < max(2, min_name_parts): + return None + first, last = parts[0], parts[-1] + qs = User.objects.filter(name__icontains=first).filter(name__icontains=last) + count = qs.count() + if count == 1: + return qs.first() + return None
241-258: Tighten GitHub username extraction; avoid emails and reserved pathsCurrent regex can match underscores (invalid for GitHub usernames) and reserved URL segments (e.g., orgs). Harden patterns.
def _extract_github_username(self, text: str) -> str | None: """Extract GitHub username from text using various patterns.""" if not text: return None - # Pattern 1: github.com/username - github_url_pattern = r"github\.com/([a-zA-Z0-9\-_]+)" - match = re.search(github_url_pattern, text, re.IGNORECASE) - if match: - return match.group(1) + # Pattern 1: github.com/<username> (exclude known non-user segments) + excluded = {"orgs", "organizations", "topics", "enterprise", "marketplace", "settings", "apps", "features", "pricing", "sponsors"} + github_url_pattern = r"(?:https?://)?(?:www\.)?github\.com/([A-Za-z0-9-]+)(?=[/\s]|$)" + match = re.search(github_url_pattern, text, re.IGNORECASE) + if match: + candidate = match.group(1) + if candidate.lower() not in excluded: + return candidate - # Pattern 2: @username mentions - mention_pattern = r"@([a-zA-Z0-9\-_]+)" - match = re.search(mention_pattern, text) + # Pattern 2: @username mentions (avoid emails/local-parts; no underscores) + mention_pattern = r"(?<![A-Za-z0-9._%+-])@([A-Za-z0-9-]+)\b" + match = re.search(mention_pattern, text, re.IGNORECASE) if match: return match.group(1) return None
🧹 Nitpick comments (3)
backend/apps/owasp/admin/award.py (1)
27-33: Trim category from search_fields to avoid unnecessary LIKE scansSince category is a choices field and already present in list_filter, searching it via LIKE is redundant and can be slower on larger tables.
search_fields = ( "name", - "category", "winner_name", "description", "winner_info", )backend/apps/owasp/management/commands/owasp_sync_awards.py (2)
133-145: Treat duplicate rows as updates when counting (avoid crashes)If duplicates exist, Award.objects.get(...) will raise MultipleObjectsReturned and abort the run. Count them as updates.
if not dry_run: # Check if award exists before update try: Award.objects.get( name=award_name, category=winner_data.get("category", ""), year=winner_data.get("year"), winner_name=winner_name, ) is_new = False except Award.DoesNotExist: is_new = True + except Award.MultipleObjectsReturned: + is_new = False
268-285: Avoid generating invalid username variants with underscoresGitHub usernames cannot contain underscores. Generating them adds noise to queries without benefit.
- clean_name = re.sub(r"[^a-zA-Z0-9\s\-_]", "", name).strip() + clean_name = re.sub(r"[^a-zA-Z0-9\s\-]", "", name).strip() @@ base_variations = [ clean_name.lower().replace(" ", ""), clean_name.lower().replace(" ", "-"), - clean_name.lower().replace(" ", "_"), ] @@ - potential_logins.extend( - [ - variation, - variation.replace("-", ""), - variation.replace("_", ""), - variation.replace("-", "_"), - variation.replace("_", "-"), - ] - ) + potential_logins.extend([variation, variation.replace("-", "")])
📜 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 (14)
backend/Makefile(1 hunks)backend/apps/nest/admin/badge.py(1 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/migrations/0045_award.py(1 hunks)backend/apps/owasp/migrations/0045_badge.py(0 hunks)backend/apps/owasp/migrations/0046_merge_0045_badge_0045_project_audience.py(1 hunks)backend/apps/owasp/models/__init__.py(1 hunks)backend/apps/owasp/models/award.py(1 hunks)backend/tests/apps/nest/management/__init__.py(1 hunks)backend/tests/apps/nest/management/commands/__init__.py(1 hunks)backend/tests/apps/owasp/models/award_test.py(1 hunks)
💤 Files with no reviewable changes (1)
- backend/apps/owasp/migrations/0045_badge.py
✅ Files skipped from review due to trivial changes (1)
- backend/apps/owasp/migrations/0046_merge_0045_badge_0045_project_audience.py
🚧 Files skipped from review as they are similar to previous changes (8)
- backend/tests/apps/nest/management/commands/init.py
- backend/tests/apps/nest/management/init.py
- backend/apps/owasp/Makefile
- backend/apps/owasp/admin/init.py
- backend/tests/apps/owasp/models/award_test.py
- backend/apps/owasp/migrations/0045_award.py
- backend/Makefile
- backend/apps/nest/admin/badge.py
🧰 Additional context used
🧬 Code Graph Analysis (4)
backend/apps/owasp/models/__init__.py (1)
backend/apps/owasp/models/award.py (1)
Award(10-180)
backend/apps/owasp/admin/award.py (1)
backend/apps/owasp/models/award.py (1)
Award(10-180)
backend/apps/owasp/models/award.py (1)
backend/apps/common/models.py (2)
BulkSaveModel(10-34)TimestampedModel(37-46)
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 (2)
Award(10-180)update_data(128-164)
🔇 Additional comments (2)
backend/apps/owasp/models/__init__.py (1)
1-1: Export surface extended with Award: LGTMImporting Award here is correct and keeps the models package consistent for external imports.
backend/apps/owasp/admin/award.py (1)
8-21: Admin configuration is solid: LGTMGood field coverage, includes is_reviewed, and shows timestamps. select_related in get_queryset mitigates N+1 on user.
backend/apps/owasp/models/award.py
Outdated
| @staticmethod | ||
| def update_data(award_data: dict, *, save: bool = True) -> Award: | ||
| """Update award data. | ||
|
|
||
| Args: | ||
| award_data: Dictionary containing single award winner data | ||
| save: Whether to save the award instance | ||
|
|
||
| Returns: | ||
| Award instance | ||
|
|
||
| """ | ||
| # Create unique identifier for get_or_create | ||
| name = award_data.get("title", "") | ||
| category = award_data.get("category", "") | ||
| year = award_data.get("year") | ||
| winner_name = award_data.get("name", "").strip() | ||
|
|
||
| try: | ||
| award = Award.objects.get( | ||
| name=name, | ||
| category=category, | ||
| year=year, | ||
| winner_name=winner_name, | ||
| ) | ||
| except Award.DoesNotExist: | ||
| award = Award( | ||
| name=name, | ||
| category=category, | ||
| year=year, | ||
| winner_name=winner_name, | ||
| ) | ||
|
|
||
| award.from_dict(award_data) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden update_data against duplicates (MultipleObjectsReturned)
update_data assumes uniqueness and only handles DoesNotExist. If duplicates ever exist, this will raise and break the sync job. Gracefully select the first match.
try:
award = Award.objects.get(
name=name,
category=category,
year=year,
winner_name=winner_name,
)
- except Award.DoesNotExist:
+ except Award.DoesNotExist:
award = Award(
name=name,
category=category,
year=year,
winner_name=winner_name,
)
+ except Award.MultipleObjectsReturned:
+ award = (
+ Award.objects.filter(
+ name=name,
+ category=category,
+ year=year,
+ winner_name=winner_name,
+ )
+ .order_by("id")
+ .first()
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @staticmethod | |
| def update_data(award_data: dict, *, save: bool = True) -> Award: | |
| """Update award data. | |
| Args: | |
| award_data: Dictionary containing single award winner data | |
| save: Whether to save the award instance | |
| Returns: | |
| Award instance | |
| """ | |
| # Create unique identifier for get_or_create | |
| name = award_data.get("title", "") | |
| category = award_data.get("category", "") | |
| year = award_data.get("year") | |
| winner_name = award_data.get("name", "").strip() | |
| try: | |
| award = Award.objects.get( | |
| name=name, | |
| category=category, | |
| year=year, | |
| winner_name=winner_name, | |
| ) | |
| except Award.DoesNotExist: | |
| award = Award( | |
| name=name, | |
| category=category, | |
| year=year, | |
| winner_name=winner_name, | |
| ) | |
| award.from_dict(award_data) | |
| @staticmethod | |
| def update_data(award_data: dict, *, save: bool = True) -> Award: | |
| """Update award data. | |
| Args: | |
| award_data: Dictionary containing single award winner data | |
| save: Whether to save the award instance | |
| Returns: | |
| Award instance | |
| """ | |
| # Create unique identifier for get_or_create | |
| name = award_data.get("title", "") | |
| category = award_data.get("category", "") | |
| year = award_data.get("year") | |
| winner_name = award_data.get("name", "").strip() | |
| try: | |
| award = Award.objects.get( | |
| name=name, | |
| category=category, | |
| year=year, | |
| winner_name=winner_name, | |
| ) | |
| except Award.DoesNotExist: | |
| award = Award( | |
| name=name, | |
| category=category, | |
| year=year, | |
| winner_name=winner_name, | |
| ) | |
| except Award.MultipleObjectsReturned: | |
| award = ( | |
| Award.objects.filter( | |
| name=name, | |
| category=category, | |
| year=year, | |
| winner_name=winner_name, | |
| ) | |
| .order_by("id") | |
| .first() | |
| ) | |
| award.from_dict(award_data) |
🤖 Prompt for AI Agents
In backend/apps/owasp/models/award.py around lines 127 to 160, update
update_data to handle MultipleObjectsReturned by catching
Award.MultipleObjectsReturned (or by using Award.objects.filter(...).first())
instead of only DoesNotExist; if multiple matches exist pick the first result
(optionally ordered) and proceed, and if no match create a new Award instance;
keep the existing award.from_dict(award_data) and respect the save flag as
before.
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/apps/owasp/management/commands/owasp_update_badges.py (1)
30-45: Avoid N+1 operations and report accurate changes; compute add/remove diffs within a transactionCurrent flow iterates all winners to add the badge (even if they already have it) and then iterates all users-with-badge to remove if not a winner. This causes avoidable queries/writes and the final message reports total winners, not the delta. Compute set differences and wrap in a single atomic transaction for a consistent snapshot and fewer round-trips.
Apply this diff to streamline updates and improve logging:
- # Get users with WASPY awards using the model method - waspy_winners = Award.get_waspy_award_winners() - - # Add badge to WASPY winners - for user in waspy_winners: - user.badges.add(waspy_badge) - - # Remove badge from users no longer on the WASPY winners list - users_with_badge = User.objects.filter(badges=waspy_badge) - waspy_winner_ids = set(waspy_winners.values_list("id", flat=True)) - - for user in users_with_badge: - if user.id not in waspy_winner_ids: - user.badges.remove(waspy_badge) - - self.stdout.write(f"Updated badges for {waspy_winners.count()} WASPY winners") + with transaction.atomic(): + # Compute desired and current holders + winner_ids = set( + Award.get_waspy_award_winners().values_list("id", flat=True) + ) + current_ids = set( + User.objects.filter(badges=waspy_badge).values_list("id", flat=True) + ) + + to_add_ids = winner_ids - current_ids + to_remove_ids = current_ids - winner_ids + + if to_add_ids: + for user in User.objects.filter(id__in=to_add_ids): + user.badges.add(waspy_badge) + if to_remove_ids: + for user in User.objects.filter(id__in=to_remove_ids): + user.badges.remove(waspy_badge) + + self.stdout.write( + f"WASPY badge updated: +{len(to_add_ids)} added, -{len(to_remove_ids)} removed, total_winners={len(winner_ids)}" + )Add the missing import at the top of the file:
from django.db import transaction
🧹 Nitpick comments (1)
backend/apps/owasp/management/commands/owasp_update_badges.py (1)
10-16: Add focused tests for badge updates (add/remove) via management commandRecommend a small test ensuring:
- Users with reviewed WASPY awards gain the badge.
- Users who no longer qualify lose the badge.
- Idempotency (running twice doesn’t create duplicates).
I can draft a test like this (adjust paths/model factories to your project setup):
from django.core.management import call_command from django.test import TestCase from apps.github.models.user import User from apps.nest.models.badge import Badge from apps.owasp.models.award import Award class TestOwaspUpdateBadges(TestCase): def setUp(self): self.user1 = User.objects.create(username="winner1") self.user2 = User.objects.create(username="not_winner") # Reviewed WASPY award -> should receive badge Award.objects.create( category=Award.Category.WASPY, name="Event Person of the Year - Winner One (2024)", description="", year=2024, user=self.user1, is_reviewed=True, ) def test_award_badge_add_and_remove(self): call_command("owasp_update_badges") waspy_badge = Badge.objects.get(name="WASPY Award Winner") self.assertTrue(self.user1.badges.filter(pk=waspy_badge.pk).exists()) self.assertFalse(self.user2.badges.filter(pk=waspy_badge.pk).exists()) # Make user1 no longer eligible Award.objects.filter(user=self.user1).update(is_reviewed=False) call_command("owasp_update_badges") self.user1.refresh_from_db() self.assertFalse(self.user1.badges.filter(pk=waspy_badge.pk).exists()) def test_idempotency(self): call_command("owasp_update_badges") call_command("owasp_update_badges") # run twice without side effects waspy_badge = Badge.objects.get(name="WASPY Award Winner") self.assertEqual( self.user1.badges.filter(pk=waspy_badge.pk).count(), 1 )
📜 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 (6)
backend/Makefile(2 hunks)backend/apps/owasp/Makefile(2 hunks)backend/apps/owasp/management/commands/owasp_sync_awards.py(1 hunks)backend/apps/owasp/management/commands/owasp_update_badges.py(1 hunks)backend/apps/owasp/migrations/0045_award.py(1 hunks)backend/apps/owasp/models/award.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- backend/apps/owasp/Makefile
- backend/apps/owasp/models/award.py
- backend/Makefile
- backend/apps/owasp/migrations/0045_award.py
- backend/apps/owasp/management/commands/owasp_sync_awards.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/apps/owasp/management/commands/owasp_update_badges.py (2)
backend/apps/nest/models/badge.py (1)
Badge(10-41)backend/apps/owasp/models/award.py (2)
Award(10-181)get_waspy_award_winners(99-110)
🔇 Additional comments (1)
backend/apps/owasp/management/commands/owasp_update_badges.py (1)
1-14: Good structure and imports — clear, idiomatic Django management command setupModule docstring, imports, and Command skeleton look clean and consistent with the rest of the codebase.
Proposed change
Resolves #1766
Awards are synced from canonical source
Users are matched to awards automatically
Data is stored and accessible via Award model
Integrated into daily sync pipeline