Sync OWASP Awards data and integrate with user profiles #2024
Conversation
Summary by CodeRabbit
WalkthroughAdds an Award model with migrations, admin, tests, and two management commands (awards sync and badge updater); integrates new commands into Makefiles; removes a prior badge migration and updates a merge migration; simplifies the 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="test@example.com", 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="test@example.com", 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.
trucodd
force-pushed
the
feature/owasp-awards-profilesync
branch
2 times, most recently
from
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.
| 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.
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.
trucodd
force-pushed
the
feature/owasp-awards-profilesync
branch
from
aebe403 to
e224251
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
backend/apps/owasp/Makefile (2)
29-33: PHONY decl + wiring for awards sync looks good.Target correctly declared as phony and uses the existing exec-backend-command pattern to invoke the Django command.
73-77: Badge updater target is properly declared and consistent with the pipeline.PHONY added and invocation mirrors other targets. No additional changes needed here.
backend/apps/owasp/management/commands/owasp_sync_awards.py (2)
108-121: Good: description is propagated into winner contextThis addresses earlier feedback about carrying the award description to each winner row. Nice.
225-237: Fuzzy match still allows single-token matches; restrict to first+last containmentCurrent logic tries all contiguous token slices, including single tokens, which can misassign common first names. Require both first and last tokens to 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: + """Try fuzzy match requiring at least two tokens (e.g., first and last) to reduce false positives.""" + 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 (8)
backend/tests/apps/owasp/management/commands/owasp_update_badges_test.py (1)
35-36: Optional: de-duplicate hardcoded badge name.Using a module-level constant reduces string drift across tests and the command.
@@ - waspy_badge = Badge.objects.get(name="WASPY Award Winner") + WASPY_BADGE_NAME = "WASPY Award Winner" + waspy_badge = Badge.objects.get(name=WASPY_BADGE_NAME) @@ - waspy_badge = Badge.objects.get(name="WASPY Award Winner") + waspy_badge = Badge.objects.get(name=WASPY_BADGE_NAME) @@ - Badge.objects.filter(name="WASPY Award Winner").delete() + Badge.objects.filter(name=WASPY_BADGE_NAME).delete() @@ - assert Badge.objects.filter(name="WASPY Award Winner").exists() + assert Badge.objects.filter(name=WASPY_BADGE_NAME).exists() @@ - waspy_badge = Badge.objects.get(name="WASPY Award Winner") + waspy_badge = Badge.objects.get(name=WASPY_BADGE_NAME) @@ - waspy_badge = Badge.objects.get(name="WASPY Award Winner") + waspy_badge = Badge.objects.get(name=WASPY_BADGE_NAME)Also applies to: 52-54, 59-65, 67-68, 85-88
backend/apps/owasp/admin/award.py (2)
27-33: Adduser__loginto search fields for admin usability.Makes it easier to find awards by GitHub username.
search_fields = ( "name", "winner_name", "description", "winner_info", + "user__login", )
68-80: Prefer@admin.actiondecorator over settingshort_descriptionattributes.Modern, less error-prone, and avoids deprecated patterns.
- def mark_reviewed(self, request, queryset): + @admin.action(description="Mark selected awards as reviewed") + def mark_reviewed(self, request, queryset): """Mark selected awards as reviewed.""" 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): + @admin.action(description="Mark selected awards as unreviewed") + def mark_unreviewed(self, request, queryset): """Mark selected awards as unreviewed.""" 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"backend/tests/apps/owasp/models/award_test.py (3)
15-24: Prefer unittest assertions over bare assertsUsing unittest assertions gives better failure messages and avoids being skipped under Python optimizations.
Apply this diff:
- choices = Award.Category.choices - assert ("WASPY", "WASPY") in choices - assert ( - "Distinguished Lifetime Memberships", - "Distinguished Lifetime Memberships", - ) in choices + choices = Award.Category.choices + self.assertIn(("WASPY", "WASPY"), choices) + self.assertIn( + ("Distinguished Lifetime Memberships", "Distinguished Lifetime Memberships"), + choices, + )
26-31: Use unittest assertions for meta checksSmall consistency/readability win.
- assert Award._meta.db_table == "owasp_awards" - assert Award._meta.verbose_name == "Award" + self.assertEqual(Award._meta.db_table, "owasp_awards") + self.assertEqual(Award._meta.verbose_name, "Award")
6-32: Optional: add behavior tests for str and display_nameWould lock in the formatting contract used by admin/UI and other call sites.
If helpful, I can add a follow-up test verifying:
- str(Award(name="X - Y (2024)", year=2024, winner_name="Y")) == "X - Y (2024)"
- award.display_name == "X - Y (2024)"
backend/apps/owasp/management/commands/owasp_sync_awards.py (2)
61-70: Harden YAML type validation to avoid runtime errorsSome YAML parsers return dicts when anchors/aliases are used. Guard for a non-list root to prevent iteration/type errors downstream.
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: + if not awards_data: self.stdout.write(self.style.ERROR("Failed to parse awards.yml content")) return + + if not isinstance(awards_data, list): + self.stdout.write(self.style.ERROR("awards.yml root must be a list of entries")) + return
282-311: Broaden login variations with common initials patternsConsider adding jdoe (first initial + last) and johnd (first + last initial). These are frequent GitHub login styles and relatively low-risk.
- # Add variations with different cases + # Add variations with different cases for variation in base_variations: potential_logins.extend([variation, variation.replace("-", "")]) + # Add initials-based patterns: jdoe, johnD, j-doe + parts = [p for p in clean_name.lower().split() if p] + if len(parts) >= 2: + first, last = parts[0], parts[-1] + fi = first[0] + li = last[0] + initials_variations = [ + f"{fi}{last}", + f"{first}{li}", + f"{fi}-{last}", + ] + potential_logins.extend(initials_variations)
📜 Review details
Configuration used: Path: .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 (20)
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/migrations/0049_award_unique_constraint.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/management/__init__.py(1 hunks)backend/tests/apps/owasp/management/commands/__init__.py(1 hunks)backend/tests/apps/owasp/management/commands/owasp_update_badges_test.py(1 hunks)backend/tests/apps/owasp/models/award_duplicate_handling_test.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 (2)
- backend/tests/apps/owasp/management/init.py
- backend/tests/apps/owasp/management/commands/init.py
🚧 Files skipped from review as they are similar to previous changes (10)
- backend/apps/owasp/models/init.py
- backend/tests/apps/nest/management/init.py
- backend/apps/owasp/admin/init.py
- backend/Makefile
- backend/apps/owasp/management/commands/owasp_update_badges.py
- backend/tests/apps/nest/management/commands/init.py
- backend/apps/owasp/migrations/0046_merge_0045_badge_0045_project_audience.py
- backend/apps/nest/admin/badge.py
- backend/apps/owasp/migrations/0045_award.py
- backend/apps/owasp/models/award.py
🧰 Additional context used
🧬 Code graph analysis (5)
backend/tests/apps/owasp/models/award_duplicate_handling_test.py (1)
backend/apps/owasp/models/award.py (3)
Award(10-183)Category(17-22)update_data(131-167)
backend/tests/apps/owasp/management/commands/owasp_update_badges_test.py (2)
backend/apps/nest/models/badge.py (1)
Badge(10-41)backend/apps/owasp/models/award.py (2)
Award(10-183)Category(17-22)
backend/apps/owasp/admin/award.py (1)
backend/apps/owasp/models/award.py (1)
Award(10-183)
backend/tests/apps/owasp/models/award_test.py (1)
backend/apps/owasp/models/award.py (2)
Award(10-183)Category(17-22)
backend/apps/owasp/management/commands/owasp_sync_awards.py (3)
backend/apps/github/utils.py (1)
get_repository_file_content(60-79)backend/apps/owasp/models/award.py (2)
Award(10-183)update_data(131-167)backend/apps/owasp/management/commands/owasp_update_badges.py (2)
Command(10-46)handle(15-46)
🪛 checkmake (0.2.2)
backend/apps/owasp/Makefile
[warning] 73-73: Missing required phony target "all"
(minphony)
[warning] 73-73: Missing required phony target "clean"
(minphony)
[warning] 73-73: Missing required phony target "test"
(minphony)
🔇 Additional comments (5)
backend/tests/apps/owasp/management/commands/owasp_update_badges_test.py (1)
30-46: End-to-end coverage for add/remove flow looks solid.Covers creation of the badge when missing, assignment, removal on unreview, and idempotency. Nice.
backend/tests/apps/owasp/models/award_test.py (1)
9-14: Import test looks goodThe import check is clear and scoped. No issues.
backend/apps/owasp/management/commands/owasp_sync_awards.py (3)
249-281: Nice hardening of GitHub username extractionExcluding reserved path segments and avoiding email local-part matches reduces false positives. Looks solid.
44-89: Overall command flow is solid and transactional
- Safe YAML loading, dry-run support, atomic writes, and a clear summary are all in good shape.
- Post-sync badge refresh via call_command is appropriate and decoupled.
143-161: Ignore removal of nest_updated_at—field is defined on Award
Thenest_updated_atfield is declared inapps/common/models.pyand inherited byAward(which subclassesTimestampedModel). Keepingupdate_fields=["user", "nest_updated_at"]is correct.• apps/common/models.py:46 defines
nest_updated_at = models.DateTimeField(auto_now=True)
• apps/owasp/models/award.py:10 declaresclass Award(BulkSaveModel, TimestampedModel)No changes needed here.
Likely an incorrect or invalid review comment.
backend/tests/apps/owasp/models/award_duplicate_handling_test.py
Outdated
Show resolved
Hide resolved
trucodd
force-pushed
the
feature/owasp-awards-profilesync
branch
from
e224251 to
289c8c5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
backend/apps/owasp/Makefile (2)
29-33: Good addition: PHONY target and wiring look correctThe owasp-sync-awards target mirrors existing patterns and avoids filename collisions via .PHONY. Nice.
73-77: Good addition: PHONY target for badges updateThe owasp-update-badges target is consistent with the rest of the Makefile, and the .PHONY guard is in place.
backend/apps/owasp/models/award.py (4)
36-41: Do not bake winner/year into name; remove unique=True and use a composite unique constraint insteadCurrent strategy builds name like "Title - Winner (Year)" to satisfy unique=True. This breaks semantics, causes duplication in str/display_name, and makes updates fragile. Model the domain explicitly.
Apply:
- name = models.CharField( + name = models.CharField( verbose_name="Name", max_length=200, - unique=True, help_text="Award name/title (e.g., 'Event Person of the Year')", )And add a UniqueConstraint (see next comment).
24-28: Enforce proper uniqueness at the DB layerAllow recurring titles but ensure per-award/year/winner uniqueness.
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_category_year_winner", + ) + ]
87-96: Fix duplicated winner/year in str and display_nameWith the current ingestion, str becomes "Title - Winner (Year) - Winner (Year)" and display_name doubles the year. Return clean strings.
- 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
141-161: Refactor update_data to use explicit keys instead of a synthesized “unique name”Querying by synthesized name is brittle. Use the proper fields; this aligns with the composite UniqueConstraint and keeps name clean.
- # Create unique name for each winner to satisfy unique constraint - award_title = (award_data.get("title") or "").strip() - category = (award_data.get("category") or "").strip() - year = award_data.get("year") - winner_name = (award_data.get("name") or "").strip() - - # Create unique name combining award title, winner, and year - unique_name = f"{award_title} - {winner_name} ({year})" - - try: - award = Award.objects.get(name=unique_name) - except Award.DoesNotExist: - award = Award( - name=unique_name, - category=category, - year=year, - winner_name=winner_name, - ) - except Award.MultipleObjectsReturned: - award = Award.objects.filter(name=unique_name).order_by("id").first() + # Normalize fields + award_title = (award_data.get("title") or "").strip() + category = (award_data.get("category") or "").strip() + year = award_data.get("year") + winner_name = (award_data.get("name") or "").strip() + + # Look up by domain fields (not a synthesized name) + try: + award = Award.objects.get( + name=award_title, + category=category, + year=year, + winner_name=winner_name, + ) + except Award.DoesNotExist: + award = Award( + name=award_title, + category=category, + year=year, + winner_name=winner_name, + ) + except Award.MultipleObjectsReturned: + award = ( + Award.objects.filter( + name=award_title, + category=category, + year=year, + winner_name=winner_name, + ) + .order_by("id") + .first() + )backend/apps/owasp/management/commands/owasp_sync_awards.py (1)
225-237: Fuzzy matching is too permissive; risk of wrong user assignmentSingle-token substring matches can collide easily (“John”, “Alex”). Require at least two name tokens and both to be present to consider a unique match.
- 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 conservative fuzzy match: require first and last token to be present.""" + 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() + return qs.first() if count == 1 else None
🧹 Nitpick comments (5)
backend/apps/owasp/Makefile (1)
73-73: Silence checkmake “minphony” warnings (optional)Static analysis flags missing phony targets all/clean/test for this Makefile. If checkmake runs against sub-app Makefiles, either add stub targets or configure the linter to skip minphony here.
Example stubs near the top of this file:
+.PHONY: all clean test +all: + @echo "noop (backend/apps/owasp/Makefile)" +clean: + @echo "noop (backend/apps/owasp/Makefile)" +test: + @echo "noop (backend/apps/owasp/Makefile)"backend/tests/apps/owasp/models/award_test.py (1)
6-32: Increase coverage: idempotency, multi-winner per award/year, and string/display formattingCurrent tests don’t cover update_data behavior, handling multiple winners for the same award/year, or str/display_name. Adding these will guard against regressions in ingestion and presentation.
Sample additions:
@@ class AwardModelTest(TestCase): @@ + def test_update_data_idempotent_and_multi_winner(self): + from apps.owasp.models import Award + payload_common = {"title": "Community Member of the Year", "category": "WASPY", "year": 2024, "description": "Desc"} + w1 = {**payload_common, "name": "Alice", "info": "github.com/alice", "image": "/assets/a.png"} + w2 = {**payload_common, "name": "Bob", "info": "github.com/bob", "image": "/assets/b.png"} + a1 = Award.update_data(w1) + a1_again = Award.update_data(w1) + a2 = Award.update_data(w2) + assert a1.id == a1_again.id, "update_data must be idempotent for same winner" + assert {a1.winner_name, a2.winner_name} == {"Alice", "Bob"}, "multiple winners per award/year should be allowed" + + def test_str_and_display_do_not_duplicate_winner_or_year(self): + from apps.owasp.models import Award + a = Award.update_data({"title": "Event Person of the Year", "category": "WASPY", "year": 2024, "name": "Charlie"}) + # After saving, __str__ and display_name should not repeat (winner) or (year) info redundantly + s = str(a) + disp = a.display_name + assert "Charlie" in s and s.count("(2024)") == 1 + assert disp == "Event Person of the Year (2024)"backend/apps/owasp/admin/award.py (2)
27-33: Improve search by enabling user login searchSearch often starts with a GitHub handle. Add user__login to search_fields.
search_fields = ( "name", "winner_name", "description", "winner_info", + "user__login", )
68-80: Use @admin.action decorator and update timestamps on bulk status changesThe decorator is cleaner than setting short_description. Also consider updating nest_updated_at, since bulk update won’t touch auto_now.
+from django.utils import timezone @@ - def mark_reviewed(self, request, queryset): + @admin.action(description="Mark selected awards as reviewed") + def mark_reviewed(self, request, queryset): """Mark selected awards as reviewed.""" - updated = queryset.update(is_reviewed=True) + updated = queryset.update(is_reviewed=True, nest_updated_at=timezone.now()) 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): + @admin.action(description="Mark selected awards as unreviewed") + def mark_unreviewed(self, request, queryset): """Mark selected awards as unreviewed.""" - updated = queryset.update(is_reviewed=False) + updated = queryset.update(is_reviewed=False, nest_updated_at=timezone.now()) self.message_user(request, f"Marked {updated} award(s) as unreviewed.") - - mark_unreviewed.short_description = "Mark selected awards as unreviewed"backend/apps/owasp/models/award.py (1)
64-69: Relative image paths from YAML may fail URLField validation in adminawards.yml often uses “/assets/…”. URLField form validation expects absolute URLs. Consider a CharField here or a custom admin form field/validator that accepts absolute or site-relative paths.
If you want to keep URLField in the DB but relax admin validation only:
# admin form (place in admin/award.py) from django import forms from .award import Award class AwardAdminForm(forms.ModelForm): class Meta: model = Award fields = "__all__" def clean_winner_image_url(self): val = self.cleaned_data.get("winner_image_url", "") # Accept site-relative paths like "/assets/.." return val # then wire it: class AwardAdmin(admin.ModelAdmin): form = AwardAdminForm
📜 Review details
Configuration used: Path: .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 (11)
backend/apps/owasp/Makefile(2 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/0049_award_unique_constraint.py(1 hunks)backend/apps/owasp/models/award.py(1 hunks)backend/tests/apps/owasp/management/__init__.py(1 hunks)backend/tests/apps/owasp/management/commands/__init__.py(1 hunks)backend/tests/apps/owasp/management/commands/owasp_update_badges_test.py(1 hunks)backend/tests/apps/owasp/models/award_duplicate_handling_test.py(1 hunks)backend/tests/apps/owasp/models/award_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- backend/tests/apps/owasp/management/init.py
- backend/apps/owasp/migrations/0049_award_unique_constraint.py
- backend/tests/apps/owasp/management/commands/owasp_update_badges_test.py
- backend/tests/apps/owasp/models/award_duplicate_handling_test.py
- backend/apps/owasp/management/commands/owasp_update_badges.py
- backend/tests/apps/owasp/management/commands/init.py
🧰 Additional context used
🧬 Code graph analysis (4)
backend/apps/owasp/admin/award.py (1)
backend/apps/owasp/models/award.py (1)
Award(10-183)
backend/tests/apps/owasp/models/award_test.py (1)
backend/apps/owasp/models/award.py (2)
Award(10-183)Category(17-22)
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 (3)
backend/apps/github/utils.py (1)
get_repository_file_content(60-79)backend/apps/owasp/models/award.py (2)
Award(10-183)update_data(131-167)backend/apps/owasp/management/commands/owasp_update_badges.py (2)
Command(10-46)handle(15-46)
🪛 checkmake (0.2.2)
backend/apps/owasp/Makefile
[warning] 73-73: Missing required phony target "all"
(minphony)
[warning] 73-73: Missing required phony target "clean"
(minphony)
[warning] 73-73: Missing required phony target "test"
(minphony)
🔇 Additional comments (8)
backend/apps/owasp/Makefile (1)
29-33: Confirmed correct ordering of OWASP targetsThe
backend/Makefiledeclares:
sync-data: update-data \ enrich-data \ owasp-update-badges \ index-dataupdate-data: … \ owasp-sync-awards \ …This ensures that
owasp-sync-awards(viaupdate-data) runs beforeowasp-update-badges. No changes are required.backend/tests/apps/owasp/models/award_test.py (2)
19-24: LGTM: Category choices assertions are aligned with the modelThe checks for "WASPY" and "Distinguished Lifetime Memberships" match Award.Category.choices.
26-31: LGTM: Meta checks are precisedb_table and verbose_name assertions are correct and valuable for regression protection.
backend/apps/owasp/admin/award.py (1)
12-21: Solid admin list configurationThe chosen columns surface the most relevant fields (incl. review status and timestamps) and select_related/user autocomplete is a good touch for performance/UX.
backend/apps/owasp/models/award.py (1)
98-111: Confirm intended gating: badges only for reviewed awardsget_waspy_award_winners filters by is_reviewed=True. This means badges won’t be assigned until manual review. If this is intentional, all good; if not, consider making the gating conditional.
Would you like badges to reflect unreviewed matches too (e.g., behind a management command flag), or keep the current conservative behavior?
backend/apps/owasp/management/commands/owasp_sync_awards.py (3)
254-276: Nice hardening for GitHub handle extractionExcluding reserved paths and avoiding email local-parts reduces false positives. Character class aligns with GitHub username rules.
330-340: Confirm post-sync badge update policyBadges are updated after every successful sync (non-dry-run). Given get_waspy_award_winners filters reviewed=True, this is safe. If you later choose to include unreviewed matches, consider adding a --include-unreviewed flag to owasp_update_badges and pass it here.
Would you like me to add an optional flag path now, or keep the reviewed-only default?
96-105: Category field mapping verified as correct
Theaward_data.get("category")call aligns with the OWASP awards.yml schema, where award entries indeed use thecategorykey for grouping (e.g., “WASPY”). No changes required.
trucodd
force-pushed
the
feature/owasp-awards-profilesync
branch
2 times, most recently
from
cce7323 to
3decd96
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
backend/tests/apps/owasp/models/award_duplicate_handling_test.py (1)
23-34: Good fix: simulate duplicates via patch instead of violating uniqueness.Switching to patching
Award.objects.getto raiseMultipleObjectsReturnedis the right way to exercise the branch without breaking the unique constraint. Matches the earlier guidance.backend/apps/owasp/models/award.py (3)
36-41: Removeunique=Trueonname; enforce uniqueness on domain fields instead.
nameshould hold the award title (e.g., “Event Person of the Year”). Making it unique prevents valid inserts across years and winners, and forced the code to embed winner/year intoname. Move uniqueness to a composite includingname,category,year, andwinner_name.- name = models.CharField( + name = models.CharField( verbose_name="Name", max_length=200, - unique=True, help_text="Award name/title (e.g., 'Event Person of the Year')", )
24-28: Add a composite unique constraint to allow multiple years and winners.This aligns persistence with the data model (award title repeats each year and can have multiple winners).
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", + ) + ]
87-96: Fix double-print and avoid “(None)” in string representations.Current
__str__duplicates winner/year ifnamealready embeds them; anddisplay_nameshows(None)if year is missing in any future cases. Build strings from fields, not preformattedname.- 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.namebackend/apps/owasp/management/commands/owasp_sync_awards.py (2)
147-159: Detect create/update using domain fields, not synthesizedunique_name.This logic should mirror the model’s uniqueness (name, category, year, winner_name). It also prevents divergence if string formatting changes.
- # Check if award exists before update using unique name - unique_name = f"{award_name} - {winner_name} ({winner_data.get('year')})" - try: - Award.objects.get(name=unique_name) - is_new = False - except Award.DoesNotExist: - is_new = True - except Award.MultipleObjectsReturned: - is_new = False + # Check existence using domain fields (mirrors model uniqueness) + is_new = not Award.objects.filter( + name=award_name, + category=winner_data.get("category", ""), + year=winner_data.get("year"), + winner_name=winner_name, + ).exists()
229-241: Fuzzy matching is too permissive; tighten to reduce false positives.Single-token containment can easily misassign users (e.g., “John”). Require at least two tokens and ensure both are 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: + """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
🧹 Nitpick comments (6)
backend/tests/apps/owasp/models/award_duplicate_handling_test.py (1)
35-36: Prefer unittest assertions over bareassert.Use
self.assertEqualfor clearer failure messages and consistency with Django’s TestCase.- assert result_award.id == award1.id + self.assertEqual(result_award.id, award1.id)backend/apps/owasp/Makefile (1)
73-73: Optional: Satisfy checkmake minphony (all/clean/test) or suppress locally.Static analysis warns about missing
all,clean,test. If this app-level Makefile is linted independently, consider stubbing these or relaxing the rule for this path.If you want to appease checkmake without changing the top-level workflow, add no-op stubs:
.PHONY: all clean test all: @echo "Use top-level Makefile targets" clean: @echo "Use top-level Makefile clean" test: @echo "Use top-level Makefile test"backend/apps/owasp/admin/award.py (2)
12-21: Admin config is solid; consider minor UX tweaks.
- Keep as-is functionally. Optional improvements:
- Add
list_display_links = ("name", "winner_name")to make rows easier to open.- Add
list_per_page = 50to keep pages lighter on large datasets.class AwardAdmin(admin.ModelAdmin): @@ - list_display = ( + list_display = ( "name", "category", "year", "winner_name", "user", "is_reviewed", "nest_created_at", "nest_updated_at", ) + list_display_links = ("name", "winner_name") + list_per_page = 50
40-57: Heads-up:winner_image_urlmay be relative; admin edits can fail URL validation.
awards.ymloften uses paths like/assets/.... Django admin will validateURLFieldand may reject these on save. Normalize to an absolute URL in the model setter to prevent admin friction. See model comment for a concrete patch tofrom_dict.backend/apps/owasp/models/award.py (2)
176-180: Normalize relative image paths to absolute URLs for admin compatibility.
awards.ymloften uses/assets/..., which failsURLFieldvalidation in admin. Prefix with the site domain when path-like.- 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(), - } + raw_image = (data.get("image") or data.get("winner_image_url") or "").strip() + if raw_image and raw_image.startswith("/"): + # Convert to absolute URL for admin/form validation + raw_image = f"https://owasp.org{raw_image}" + fields = { + "description": (data.get("description") or "").strip(), + "winner_info": (data.get("info") or data.get("winner_info") or "").strip(), + "winner_image_url": raw_image, + }
45-48: Confirm whetheryearneeds to be nullable.If you plan to persist “category” rows (no year) later, align with a nullable
year(null=True, blank=True) to avoid future migrations. If awards-only, current non-null is fine.
📜 Review details
Configuration used: Path: .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 (11)
backend/apps/owasp/Makefile(2 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/0049_award_unique_constraint.py(1 hunks)backend/apps/owasp/models/award.py(1 hunks)backend/tests/apps/owasp/management/__init__.py(1 hunks)backend/tests/apps/owasp/management/commands/__init__.py(1 hunks)backend/tests/apps/owasp/management/commands/owasp_update_badges_test.py(1 hunks)backend/tests/apps/owasp/models/award_duplicate_handling_test.py(1 hunks)backend/tests/apps/owasp/models/award_test.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/tests/apps/owasp/management/commands/init.py
🚧 Files skipped from review as they are similar to previous changes (5)
- backend/tests/apps/owasp/management/init.py
- backend/apps/owasp/management/commands/owasp_update_badges.py
- backend/tests/apps/owasp/models/award_test.py
- backend/apps/owasp/migrations/0049_award_unique_constraint.py
- backend/tests/apps/owasp/management/commands/owasp_update_badges_test.py
🧰 Additional context used
🧬 Code graph analysis (4)
backend/apps/owasp/models/award.py (1)
backend/apps/common/models.py (2)
BulkSaveModel(10-34)TimestampedModel(37-46)
backend/tests/apps/owasp/models/award_duplicate_handling_test.py (1)
backend/apps/owasp/models/award.py (3)
Award(10-183)Category(17-22)update_data(131-167)
backend/apps/owasp/admin/award.py (1)
backend/apps/owasp/models/award.py (1)
Award(10-183)
backend/apps/owasp/management/commands/owasp_sync_awards.py (3)
backend/apps/github/utils.py (1)
get_repository_file_content(60-79)backend/apps/owasp/models/award.py (2)
Award(10-183)update_data(131-167)backend/apps/owasp/management/commands/owasp_update_badges.py (2)
Command(10-46)handle(15-46)
🪛 checkmake (0.2.2)
backend/apps/owasp/Makefile
[warning] 73-73: Missing required phony target "all"
(minphony)
[warning] 73-73: Missing required phony target "clean"
(minphony)
[warning] 73-73: Missing required phony target "test"
(minphony)
🔇 Additional comments (5)
backend/apps/owasp/Makefile (2)
29-34: PHONY target and wiring look good.
owasp-sync-awardsis correctly marked.PHONYand calls the Django command through the existing exec wrapper.
73-77: PHONY target for badges update looks good.
owasp-update-badgesis correctly marked.PHONYand delegates tomanage.py owasp_update_badges.backend/apps/owasp/models/award.py (1)
1-184: AI summary mismatch: it claimsunique=Truewas removed, but code still has it.The AI summary mentions removing
unique=Truefromname; the code still declares it. Please reconcile to avoid schema drift.backend/apps/owasp/management/commands/owasp_sync_awards.py (2)
100-111: YAML key mapping LGTM; continue skipping non-award entries.Reading
title/category/yearand skipping where incomplete is correct; limiting totype == "award"matches the upstream structure.
334-344: Badge update hook is appropriate and isolated.Calling
owasp_update_badgesafter a successful sync (outside dry-run) is correct and keeps concerns separated.
trucodd
force-pushed
the
feature/owasp-awards-profilesync
branch
from
3decd96 to
c2eb7a1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
backend/apps/owasp/Makefile (1)
29-34: PHONY + wiring for owasp-sync-awards look goodTarget is correctly declared phony and wired through the existing exec-backend-command mechanism. No further action from my side.
backend/apps/owasp/models/award.py (2)
24-41: Fix uniqueness: remove unique=True on name and enforce composite uniquenessA single award title repeats across years and can have multiple winners. Storing a synthesized “unique name” in name is brittle and breaks admin display; relying on unique=True here either blocks valid data or forces the current workaround. Prefer domain-level uniqueness at the DB: (name, category, year, winner_name).
Apply:
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", + ) + ] @@ - name = models.CharField( + name = models.CharField( verbose_name="Name", max_length=200, - unique=True, help_text="Award name/title (e.g., 'Event Person of the Year')", )Follow-up: generate a migration for the new constraint and dropping the unique index on name.
130-162: Stop synthesizing name; select/update by domain fieldsBuilding a “unique_name = '{title} - {winner} ({year})'” leaks presentation into storage, breaks admin display, and complicates lookups. Use the real title in name and select/update by (name, category, year, winner_name). This also aligns with the composite DB constraint above.
- # Create unique name for each winner to satisfy unique constraint - award_title = (award_data.get("title") or "").strip() - category = (award_data.get("category") or "").strip() - year = award_data.get("year") - winner_name = (award_data.get("name") or "").strip() - - # Create unique name combining award title, winner, and year - unique_name = f"{award_title} - {winner_name} ({year})" - - try: - award = Award.objects.get(name=unique_name) - except Award.DoesNotExist: - award = Award( - name=unique_name, - category=category, - year=year, - winner_name=winner_name, - ) - except Award.MultipleObjectsReturned: - award = Award.objects.filter(name=unique_name).order_by("id").first() + award_title = (award_data.get("title") or "").strip() + category = (award_data.get("category") or "").strip() + year = award_data.get("year") + winner_name = (award_data.get("name") or "").strip() + + # Select by domain fields; tolerate duplicates defensively + award = ( + Award.objects.filter( + name=award_title, + category=category, + year=year, + winner_name=winner_name, + ) + .order_by("id") + .first() + ) + if not award: + award = Award( + name=award_title, + category=category, + year=year, + winner_name=winner_name, + )No behavioral change to from_dict/save below; they remain correct.
backend/apps/owasp/management/commands/owasp_sync_awards.py (2)
147-157: Existence check should use domain fields (not synthesized name)Relying on a formatted string is fragile and diverges from the model’s intended uniqueness. Detect on (name, category, year, winner_name) so stats are accurate and logic remains stable if formatting changes.
- # Check if award exists before update using unique name - unique_name = f"{award_name} - {winner_name} ({winner_data.get('year')})" - try: - Award.objects.get(name=unique_name) - is_new = False - except Award.DoesNotExist: - is_new = True - except Award.MultipleObjectsReturned: - is_new = False + # Check existence using domain fields + exists = Award.objects.filter( + name=award_name, + category=winner_data.get("category", ""), + year=winner_data.get("year"), + winner_name=winner_name, + ).exists() + is_new = not exists
229-241: Constrain fuzzy matching to reduce false-positive user assignmentsSingle-token contains() matches are risky (e.g., “John” will hit many). Require at least two tokens (first+last) and ensure both are present in the name. This materially reduces mis-assignments.
- 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) + if qs.count() == 1: + return qs.first() + return None
🧹 Nitpick comments (1)
backend/apps/owasp/models/award.py (1)
87-96: Avoid duplicate winner/year in string representationBecause update_data currently writes winner and year into name, str and display_name end up duplicating them. Even after fixing update_data, this is a safer implementation and prevents “(None)” when year is absent in future.
- 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
📜 Review details
Configuration used: Path: .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 (11)
backend/apps/owasp/Makefile(2 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/0049_award_unique_constraint.py(1 hunks)backend/apps/owasp/models/award.py(1 hunks)backend/tests/apps/owasp/management/__init__.py(1 hunks)backend/tests/apps/owasp/management/commands/__init__.py(1 hunks)backend/tests/apps/owasp/management/commands/owasp_update_badges_test.py(1 hunks)backend/tests/apps/owasp/models/award_duplicate_handling_test.py(1 hunks)backend/tests/apps/owasp/models/award_test.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/tests/apps/owasp/management/commands/init.py
🚧 Files skipped from review as they are similar to previous changes (7)
- backend/tests/apps/owasp/management/init.py
- backend/tests/apps/owasp/management/commands/owasp_update_badges_test.py
- backend/apps/owasp/management/commands/owasp_update_badges.py
- backend/tests/apps/owasp/models/award_test.py
- backend/apps/owasp/admin/award.py
- backend/apps/owasp/migrations/0049_award_unique_constraint.py
- backend/tests/apps/owasp/models/award_duplicate_handling_test.py
🧰 Additional context used
🧬 Code graph analysis (2)
backend/apps/owasp/management/commands/owasp_sync_awards.py (3)
backend/apps/github/utils.py (1)
get_repository_file_content(60-79)backend/apps/owasp/models/award.py (2)
Award(10-187)update_data(131-167)backend/apps/owasp/management/commands/owasp_update_badges.py (2)
Command(10-46)handle(15-46)
backend/apps/owasp/models/award.py (1)
backend/apps/common/models.py (2)
BulkSaveModel(10-34)TimestampedModel(37-46)
🪛 checkmake (0.2.2)
backend/apps/owasp/Makefile
[warning] 73-73: Missing required phony target "all"
(minphony)
[warning] 73-73: Missing required phony target "clean"
(minphony)
[warning] 73-73: Missing required phony target "test"
(minphony)
🔇 Additional comments (6)
backend/apps/owasp/Makefile (1)
73-77: Orchestration confirmed:owasp-update-badgesruns after awards syncVerified that in
backend/Makefile, thesync-datatarget depends first onupdate-data(which invokesowasp-sync-awards), then onenrich-data, and only afterward onowasp-update-badges. The intended ordering is correct—no further changes are needed.backend/apps/owasp/models/award.py (2)
98-111: WASPY winners query is concise and correctUsing the reverse relation and distinct() is efficient and clear. Good call on gating by is_reviewed to reflect human validation.
45-48: Year nullability confirmed aligned with migrationsThe
yearfield in the initial CreateModel migration (0045_award.py) is defined withoutnull=True(i.e. non-nullable), matching the current model declaration.
- If every Award row will always have a year, no changes are required.
- If you intend to persist “category” entries without a year, you’ll need to update both the model and create a new migration to add
null=True(andblank=Trueif desired) onyearto avoid schema drift.backend/apps/owasp/management/commands/owasp_sync_awards.py (3)
100-109: YAML field mapping looks correctaward_name from "title", category from "category", and year from "year" match the current awards.yml structure for items where type: award. Verified against the canonical file on August 23, 2025. (raw.githubusercontent.com)
112-126: Good: winner context carries description/info/imageThreading description into each winner ensures Award.description mirrors upstream and avoids empty descriptions on created rows. Nice.
334-345: Post-sync badge update is a nice touchUpdating badges after a successful sync keeps UX coherent. Wrapped in try/except with logging — LGTM.
trucodd
force-pushed
the
feature/owasp-awards-profilesync
branch
from
c2eb7a1 to
84f3b1f
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
backend/apps/owasp/management/commands/owasp_sync_awards.py (2)
229-241: Tighten fuzzy matching to reduce false positivesRequire at least two tokens and ensure both appear in the candidate’s name; avoid single-token substring matches that can misassign users.
- 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 the same 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) + return qs.first() if qs.count() == 1 else None
147-156: Detect created/updated via composite fields, not synthesized unique_nameOnce the model enforces composite uniqueness, use the same fields here to decide is_new. This removes the brittle unique_name coupling.
- if not dry_run: - # Check if award exists before update using unique name - unique_name = f"{award_name} - {winner_name} ({winner_data.get('year')})" - try: - Award.objects.get(name=unique_name) - is_new = False - except Award.DoesNotExist: - is_new = True - except Award.MultipleObjectsReturned: - is_new = False + if not dry_run: + # Detect existence using domain fields + composite_lookup = { + "name": award_name, + "category": winner_data.get("category", ""), + "year": winner_data.get("year"), + "winner_name": winner_name, + } + is_new = not Award.objects.filter(**composite_lookup).exists()
🧹 Nitpick comments (2)
backend/tests/apps/owasp/models/award_test.py (1)
13-14: Prefer unittest assertions over bare assertsUse TestCase helpers for clearer failures and consistency with Django’s test runner.
- assert Award is not None + self.assertIsNotNone(Award) - choices = Award.Category.choices - assert ("WASPY", "WASPY") in choices - assert ( - "Distinguished Lifetime Memberships", - "Distinguished Lifetime Memberships", - ) in choices + choices = Award.Category.choices + self.assertIn(("WASPY", "WASPY"), choices) + self.assertIn( + ("Distinguished Lifetime Memberships", "Distinguished Lifetime Memberships"), + choices, + ) - assert Award._meta.db_table == "owasp_awards" - assert Award._meta.verbose_name == "Award" + self.assertEqual(Award._meta.db_table, "owasp_awards") + self.assertEqual(Award._meta.verbose_name, "Award")Also applies to: 19-24, 30-31
backend/apps/owasp/Makefile (1)
73-73: Optional: add common stub targets if checkmake is gating your CIStatic analysis hints flagged missing “all/clean/test”. If your repo enforces these per-Makefile, add minimal stubs; otherwise ignore.
If needed, add at the end of this Makefile (example):
.PHONY: all clean test all: @echo "backend/apps/owasp: no default target" clean: @echo "backend/apps/owasp: nothing to clean" test: @echo "backend/apps/owasp: no tests bound here"
📜 Review details
Configuration used: Path: .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 (11)
backend/apps/owasp/Makefile(2 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/0049_award_unique_constraint.py(1 hunks)backend/apps/owasp/models/award.py(1 hunks)backend/tests/apps/owasp/management/__init__.py(1 hunks)backend/tests/apps/owasp/management/commands/__init__.py(1 hunks)backend/tests/apps/owasp/management/commands/owasp_update_badges_test.py(1 hunks)backend/tests/apps/owasp/models/award_duplicate_handling_test.py(1 hunks)backend/tests/apps/owasp/models/award_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- backend/tests/apps/owasp/management/init.py
- backend/tests/apps/owasp/management/commands/init.py
- backend/apps/owasp/migrations/0049_award_unique_constraint.py
- backend/apps/owasp/management/commands/owasp_update_badges.py
- backend/tests/apps/owasp/management/commands/owasp_update_badges_test.py
- backend/tests/apps/owasp/models/award_duplicate_handling_test.py
- backend/apps/owasp/admin/award.py
🧰 Additional context used
🧬 Code graph analysis (3)
backend/tests/apps/owasp/models/award_test.py (1)
backend/apps/owasp/models/award.py (2)
Award(10-190)Category(17-22)
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-190)update_data(134-170)
backend/apps/owasp/models/award.py (1)
backend/apps/common/models.py (2)
BulkSaveModel(10-34)TimestampedModel(37-46)
🪛 checkmake (0.2.2)
backend/apps/owasp/Makefile
[warning] 73-73: Missing required phony target "all"
(minphony)
[warning] 73-73: Missing required phony target "clean"
(minphony)
[warning] 73-73: Missing required phony target "test"
(minphony)
🔇 Additional comments (3)
backend/apps/owasp/Makefile (1)
29-34: New PHONY targets are wired correctly to management commandsTargets and echo lines look good; discovery will work via the existing exec-backend-command pattern.
Also applies to: 73-77
backend/apps/owasp/models/award.py (1)
45-48: No action needed onyearnullabilityThe
0045_award.pymigration defines theyearfield withoutnull=True, making it non-nullable by default—this aligns with the model’s currentIntegerField(which is alsonull=Falseby default). There’s no drift between the model and migrations here, so you can leave the field as-is.backend/apps/owasp/management/commands/owasp_sync_awards.py (1)
258-283: Good hardening of GitHub username extractionNice exclusion of reserved paths and removal of underscores; avoids matching emails and org URLs.
| name = models.CharField( | ||
| verbose_name="Name", | ||
| max_length=200, | ||
| unique=True, | ||
| help_text="Award name/title (e.g., 'Event Person of the Year')", | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix schema: remove unique=True from name and enforce composite uniqueness; stop synthesizing name
Current design stores “title - winner (year)” in name to satisfy a single-field unique constraint, which fights the domain model and complicates queries. Use the real award title in name and enforce uniqueness across (name, year, category, winner_name). Update update_data accordingly.
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",
+ )
+ ]
@@
- name = models.CharField(
- verbose_name="Name",
- max_length=200,
- unique=True,
- help_text="Award name/title (e.g., 'Event Person of the Year')",
- )
+ name = models.CharField(
+ verbose_name="Name",
+ max_length=200,
+ help_text="Award name/title (e.g., 'Event Person of the Year')",
+ )
@@
- # Create unique name for each winner to satisfy unique constraint
- award_title = (award_data.get("title") or "").strip()
- category = (award_data.get("category") or "").strip()
- year = award_data.get("year")
- winner_name = (award_data.get("name") or "").strip()
-
- # Create unique name combining award title, winner, and year
- unique_name = f"{award_title} - {winner_name} ({year})"
-
- try:
- award = Award.objects.get(name=unique_name)
- except Award.DoesNotExist:
- award = Award(
- name=unique_name,
- category=category,
- year=year,
- winner_name=winner_name,
- )
- except Award.MultipleObjectsReturned:
- award = Award.objects.filter(name=unique_name).order_by("id").first()
+ award_title = (award_data.get("title") or "").strip()
+ category = (award_data.get("category") or "").strip()
+ year = award_data.get("year")
+ winner_name = (award_data.get("name") or "").strip()
+
+ try:
+ award = Award.objects.get(
+ name=award_title,
+ category=category,
+ year=year,
+ winner_name=winner_name,
+ )
+ except Award.DoesNotExist:
+ award = Award(
+ name=award_title,
+ category=category,
+ year=year,
+ winner_name=winner_name,
+ )
+ except Award.MultipleObjectsReturned:
+ award = (
+ Award.objects.filter(
+ name=award_title,
+ category=category,
+ year=year,
+ winner_name=winner_name,
+ )
+ .order_by("id")
+ .first()
+ )Follow-up migration required: drop the unique index on name and add the composite UniqueConstraint.
Want me to generate the migration diff as well?
Also applies to: 24-28, 145-165
🤖 Prompt for AI Agents
In backend/apps/owasp/models/award.py around lines 24-28, 36-41 and 145-165, the
model currently synthesizes a unique "name" value ("title - winner (year)") and
sets unique=True on name; instead, revert name to store only the real award
title (remove unique=True), stop synthesizing name in model
methods/serializers/update_data, and add a composite UniqueConstraint on (name,
year, category, winner_name) in the model's Meta; update update_data logic to
populate separate fields (title/name, year, category, winner_name) rather than
composing a unique name, and plan a migration that drops the unique index on
name and adds the composite UniqueConstraint.
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