feat: complete trademark matching for #1926 #5358
Conversation
github-actions
bot
added
migrations
|
👋 Hi @aaditya8979! This pull request needs a peer review before it can be merged. Please request a review from a team member who is not:
Once a valid peer review is submitted, this check will pass automatically. Thank you! |
WalkthroughThis PR introduces a comprehensive trademark matching system including a fuzzy-matching service, database models for storing trademark conflicts, automated signal-based checking when organizations and websites are created/updated, Django admin integration with bulk actions, API endpoints, and management commands for trademark verification and reporting. Changes
Sequence DiagramsequenceDiagram
actor Admin
participant Django as Django ORM
participant Signal as post_save Signal
participant Checker as _perform_trademark_check
participant Matcher as TrademarkMatcher
participant DB as Database
Admin->>Django: Create/Update Organization
Django->>Signal: Trigger post_save
Signal->>Checker: call _perform_trademark_check(name, org)
Checker->>Matcher: get_matches_for_website(name, threshold=85)
Matcher->>Matcher: normalize, compute similarity scores
Matcher-->>Checker: List[TrademarkCandidate]
Checker->>DB: Delete previous TrademarkMatch records
loop For each match
Checker->>Checker: Determine risk_level (high/medium/low)
Checker->>DB: Create TrademarkMatch record
end
DB-->>Checker: TrademarkMatch instances created
Checker-->>Signal: Complete (with logging)
Signal-->>Django: Return
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested labels
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
📊 Monthly LeaderboardHi @aaditya8979! Here's how you rank for December 2025:
Leaderboard based on contributions in December 2025. Keep up the great work! 🚀 |
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
| website = get_object_or_404(Website, id=website_id) | ||
|
|
||
| matches = TrademarkMatch.objects.filter(website=website).order_by("-similarity_score") |
There was a problem hiding this comment.
Bug: The website_trademark_check view will crash due to using the Website model without importing it and referencing a non-existent website field on the TrademarkMatch model.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
The website_trademark_check API view will crash when accessed. The function attempts to retrieve a Website object on line 75 using get_object_or_404(Website, id=website_id), but the Website model is never imported, which will raise a NameError. Even if the import were fixed, the code would still fail on line 77. The query TrademarkMatch.objects.filter(website=website) references the website field on the TrademarkMatch model, but this field has been explicitly commented out in the model's definition, which would cause a FieldError.
💡 Suggested Fix
Either remove the website_trademark_check view and its corresponding URL pattern if it is unused, or complete the implementation. This would involve importing the Website model and uncommenting or re-implementing the website foreign key field on the TrademarkMatch model.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: website/views/api/trademark.py#L75-L77
Potential issue: The `website_trademark_check` API view will crash when accessed. The
function attempts to retrieve a `Website` object on line 75 using
`get_object_or_404(Website, id=website_id)`, but the `Website` model is never imported,
which will raise a `NameError`. Even if the import were fixed, the code would still fail
on line 77. The query `TrademarkMatch.objects.filter(website=website)` references the
`website` field on the `TrademarkMatch` model, but this field has been explicitly
commented out in the model's definition, which would cause a `FieldError`.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 7826615
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (5)
website/models.py (1)
52-53: Remove commented-out code.Dead code comments like this add noise and reduce maintainability. If the
websitefield was intentionally removed, this comment should be deleted. If it's planned for future work, consider tracking it in an issue instead.🔎 Suggested fix
- # Remove the website field entirely for now - # website = models.ForeignKey(...) -website/signals.py (1)
81-85: Refactor: Move logger initialization to module level.The logger is imported and initialized inside the exception handler. While this works, it's inefficient if exceptions occur frequently and goes against Python conventions.
🔎 Proposed refactor
Move the import and logger initialization to the top of the file:
""" Django signals for automatic trademark checking when organizations/websites are created/updated. """ +import logging from django.db.models.signals import post_save from django.dispatch import receiver from django.utils import timezone from website.models import Organization, TrademarkMatch from website.services.trademark_integration import get_matches_for_website +logger = logging.getLogger(__name__)Then in the exception handler:
except Exception as e: - import logging - - logger = logging.getLogger(__name__) logger.error(f"Trademark check failed for {name}: {str(e)}")website/services/trademark_integration.py (1)
11-33: Refactor: Move SAMPLE_TRADEMARKS import to module level.Line 31 imports
SAMPLE_TRADEMARKSinside the function. While this works, it's unconventional and makes the dependency less obvious. Module-level imports are preferred in Python for clarity and to follow PEP 8 guidelines.The TODO comment on line 29 is noted and reasonable for the current implementation stage.
🔎 Proposed refactor
Move the import to the top of the file:
""" Integration module for trademark matching with BLT models. This provides Django-aware wrapper functions for the core matching service. """ from typing import List -from website.services.trademark_matching import TrademarkCandidate, TrademarkMatcher +from website.services.trademark_matching import ( + SAMPLE_TRADEMARKS, + TrademarkCandidate, + TrademarkMatcher, +)Then simplify the function:
def get_matches_for_website( website_name: str, threshold: float = 85.0, ) -> List[TrademarkCandidate]: """ Get trademark matches for a website/company. This is the primary API for integration with BLT models. Args: website_name: Name of the website or company to check. threshold: Match threshold (0-100). Default 85.0. Returns: List of TrademarkCandidate objects representing potential matches. """ matcher = TrademarkMatcher(threshold=threshold) # TODO: In production, fetch trademarks from USPTO API or database - # For now, using sample data - from website.services.trademark_matching import SAMPLE_TRADEMARKS return matcher.match(website_name, SAMPLE_TRADEMARKS)website/services/trademark_matching.py (2)
31-42: Consider: Aggressive normalization may over-match.The
normalizemethod removes all non-alphanumeric characters, which means "Bug-Hunt" and "BugHunt" become identical. While this enables fuzzy matching, it might create false positives for trademarks that specifically include punctuation or spacing as distinguishing features.This is likely intentional for catching similar names, but be aware that:
- "O'Reilly" and "OReilly" would match 100%
- "Inc." and "INC" would be identical
- Spacing differences are completely ignored
If this behavior causes issues in production, consider a less aggressive normalization that preserves some punctuation or word boundaries.
95-126: LGTM with scalability consideration.The matching logic is well-implemented with proper edge case handling (empty strings, normalization) and clear result processing (sorting, limiting, rounding).
For future reference: The current O(n × m) complexity (where n = number of trademarks, m = average string length) works well for the sample dataset but may need optimization if the trademark database grows significantly. Consider indexing strategies or approximate nearest neighbor algorithms if performance becomes an issue with thousands of trademarks.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (17)
.gitignoreblt/urls.pywebsite/admin.pywebsite/apps.pywebsite/management/commands/check_all_trademarks.pywebsite/management/commands/trademark_demo.pywebsite/migrations/0264_alter_activity_id_alter_activitylog_id_and_more.pywebsite/models.pywebsite/services/__init__.pywebsite/services/trademark_integration.pywebsite/services/trademark_matching.pywebsite/signals.pywebsite/tests/test_main.pywebsite/tests/test_trademark_matching.pywebsite/tests/test_trademark_models.pywebsite/views/api/trademark.pywebsite/views/trademark.py
🧰 Additional context used
🧬 Code graph analysis (9)
website/views/trademark.py (1)
website/services/trademark_integration.py (1)
get_trademark_report(54-86)
website/signals.py (3)
website/models.py (2)
Organization(251-359)TrademarkMatch(38-105)website/services/trademark_integration.py (1)
get_matches_for_website(11-33)website/services/trademark_matching.py (1)
match(95-126)
website/admin.py (1)
website/models.py (1)
TrademarkMatch(38-105)
website/services/trademark_integration.py (1)
website/services/trademark_matching.py (3)
TrademarkCandidate(11-15)TrademarkMatcher(18-126)match(95-126)
website/management/commands/trademark_demo.py (1)
website/services/trademark_matching.py (2)
get_trademark_matches(144-155)match(95-126)
website/tests/test_trademark_matching.py (1)
website/services/trademark_matching.py (7)
TrademarkCandidate(11-15)TrademarkMatcher(18-126)get_trademark_matches(144-155)normalize(31-42)_levenshtein_distance(44-73)_similarity_score(75-93)match(95-126)
website/tests/test_trademark_models.py (1)
website/models.py (3)
Organization(251-359)TrademarkMatch(38-105)is_high_risk(100-101)
website/management/commands/check_all_trademarks.py (2)
website/models.py (1)
Organization(251-359)website/signals.py (1)
_perform_trademark_check(55-85)
blt/urls.py (2)
website/views/api/trademark.py (2)
organization_trademark_check(35-64)trademark_report_api(15-30)website/views/trademark.py (1)
trademark_report_view(8-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run Tests
- GitHub Check: docker-test
🔇 Additional comments (19)
.gitignore (1)
28-28: Good housekeeping: macOS system files excluded.Adding
.DS_Storeto .gitignore prevents macOS system metadata files from being accidentally committed, which is a standard practice. No concerns.website/services/__init__.py (1)
1-4: LGTM!Clean package initialization with a clear docstring documenting the purpose. This properly establishes the services package for reusable business logic modules.
website/models.py (1)
38-106: Well-structured trademark match model.Good design choices:
- Appropriate field types and constraints
- Composite indexes align with expected query patterns (filtering by organization/score, status/date)
- The
__str__fallback logic handles the nullable organization FK gracefully- Properties for risk level thresholds provide clean abstractions
The model provides a solid foundation for tracking trademark conflicts.
website/apps.py (1)
7-12: LGTM!Good additions:
default_auto_field = "django.db.models.BigAutoField"aligns with Django 3.2+ best practices and ensures consistent primary key types across models- Importing
website.signalsinready()follows the established pattern in this file and ensures trademark-related signal handlers are registered at app startupwebsite/views/trademark.py (1)
7-22: Clean implementation of the trademark report endpoint.Good practices observed:
- Input validation with whitespace stripping
- Appropriate HTTP 400 response for missing required parameter
- Clear docstring with endpoint path
One consideration: This endpoint is publicly accessible without authentication. If trademark analysis is intended to be a protected feature, you may want to add
@login_requiredor similar access control.Verify if this endpoint should require authentication based on your security requirements.
website/tests/test_trademark_models.py (1)
1-86: LGTM! Well-structured model tests.The tests provide good coverage of the
TrademarkMatchmodel, including creation defaults, theis_high_riskproperty, string representation, and ordering behavior. The test cases align with the model definition and verify expected defaults correctly.website/admin.py (1)
182-245: LGTM! Well-implemented admin display methods and actions.The badge display methods use
format_html()correctly for XSS safety, color coding is appropriate for risk levels and statuses, and the batch actions properly update querysets with informative user feedback.website/tests/test_trademark_matching.py (1)
1-147: LGTM! Comprehensive test coverage for trademark matching.Excellent test suite covering:
- Normalization (special chars, lowercasing, empty strings)
- Levenshtein distance algorithm edge cases
- Similarity scoring
- Matching behavior with various thresholds
- Result limits and sorting
- Edge cases (empty inputs, empty trademark lists)
The tests are well-organized and provide good confidence in the matching logic.
website/migrations/0264_alter_activity_id_alter_activitylog_id_and_more.py (2)
468-537: LGTM! TrademarkMatch model migration is correct.The
CreateModeloperation properly defines all fields matching the model definition, with appropriate indexes on(organization, -similarity_score)and(status, -checked_at)for efficient querying by organization and status filtering.
12-467: BigAutoField migration is appropriate.The bulk
AlterFieldoperations convertingidfields toBigAutoFieldfollow Django 3.2+ best practices and future-proof the database for high-volume tables.website/signals.py (1)
13-21: LGTM!The organization trademark checking signal handler is properly registered and correctly validates the organization name before performing the check.
website/views/api/trademark.py (2)
14-30: LGTM!The API endpoint correctly validates the required
nameparameter and returns appropriate error responses. The implementation is clean and follows REST best practices.
33-64: LGTM!The organization trademark endpoint is properly secured with authentication, handles missing organizations gracefully with 404 responses, and provides a well-structured JSON response with useful metrics.
website/services/trademark_integration.py (2)
36-51: LGTM!The conflict checking function is straightforward and correctly returns a boolean indicating whether any matches exceed the threshold. The default threshold of 90.0 is appropriately conservative for high-confidence conflicts.
54-86: LGTM!The report generation function provides a well-structured response with useful metrics (highest score, total matches) and actionable recommendations. The data structure is clean and suitable for both API responses and internal use.
website/services/trademark_matching.py (4)
10-15: LGTM!The
TrademarkCandidatedataclass is a clean, type-annotated data structure that clearly represents a trademark match result.
44-73: LGTM!The Levenshtein distance implementation is correct and uses the classic dynamic programming approach with space optimization (storing only the previous row). The recursion on line 57 to swap strings when s1 is shorter is a nice optimization for the algorithm's efficiency.
75-93: LGTM!The similarity score calculation correctly converts Levenshtein distance to a percentage. The edge case handling for empty strings (returning 100.0) is appropriate since two empty strings are indeed identical.
129-155: LGTM!The sample trademark dataset provides reasonable test data, and the convenience function
get_trademark_matchesoffers a simple interface for common use cases with sensible defaults.
| path("api/trademark-report/", trademark_report_view, name="trademark_report"), | ||
| path("api/trademark-report/", trademark_report_api, name="trademark_report_api"), |
There was a problem hiding this comment.
Duplicate URL paths cause unreachable route.
Both lines define path("api/trademark-report/", ...) with different views. Django's URL resolver uses first-match-wins, so trademark_report_api on line 1257 will never be reached.
Looking at the view implementations in website/views/trademark.py and website/views/api/trademark.py, both trademark_report_view and trademark_report_api appear functionally identical (both return JSON trademark reports for a name query parameter).
Either:
- Remove one of the duplicate routes if they serve the same purpose
- Use distinct paths if both endpoints are needed (e.g.,
api/v1/trademark-report/vsapi/v2/trademark-report/)
🔎 Suggested fix: Remove the duplicate route
path("api/trademark-report/", trademark_report_view, name="trademark_report"),
- path("api/trademark-report/", trademark_report_api, name="trademark_report_api"),
path(
"api/organization/<int:org_id>/trademarks/", organization_trademark_check, name="organization_trademark_check"
),📝 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.
| path("api/trademark-report/", trademark_report_view, name="trademark_report"), | |
| path("api/trademark-report/", trademark_report_api, name="trademark_report_api"), | |
| path("api/trademark-report/", trademark_report_view, name="trademark_report"), |
🤖 Prompt for AI Agents
In blt/urls.py around lines 1256-1257 there are duplicate routes both using
path("api/trademark-report/", ...) which makes the second route unreachable;
remove the duplicate by either deleting the redundant entry (keep the correct
view) or, if both handlers are required, change one route to a distinct path
(e.g., "api/v2/trademark-report/" or "api/trademark-report/api/") and update any
callers/tests accordingly so each view has a unique URL.
| search_fields = [ | ||
| "checked_name", | ||
| "matched_trademark_name", | ||
| "organization__name", | ||
| "website__name", | ||
| ] |
There was a problem hiding this comment.
search_fields references non-existent website field.
The TrademarkMatch model does not have a website field (it's explicitly commented out in the model: "Remove the website field entirely for now"). This will cause a FieldError when searching in the admin.
🔎 Proposed fix
search_fields = [
"checked_name",
"matched_trademark_name",
"organization__name",
- "website__name",
]📝 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.
| search_fields = [ | |
| "checked_name", | |
| "matched_trademark_name", | |
| "organization__name", | |
| "website__name", | |
| ] | |
| search_fields = [ | |
| "checked_name", | |
| "matched_trademark_name", | |
| "organization__name", | |
| ] |
🤖 Prompt for AI Agents
In website/admin.py around lines 126 to 131, the admin search_fields includes
"website__name" which no longer exists on the TrademarkMatch model and will
raise a FieldError; remove "website__name" from the search_fields list (or
replace it with the correct existing relation/field if there is an alternative),
ensure the list only references valid model fields or related lookups, and run
the Django dev server/admin to verify the FieldError is resolved.
| ( | ||
| "Related Objects", | ||
| { | ||
| "fields": ("organization", "website"), | ||
| }, | ||
| ), |
There was a problem hiding this comment.
Fieldset references non-existent website field.
The TrademarkMatch model has no website field, so including it in the fieldset will cause an admin error when viewing/editing records.
🔎 Proposed fix
(
"Related Objects",
{
- "fields": ("organization", "website"),
+ "fields": ("organization",),
},
),🤖 Prompt for AI Agents
In website/admin.py around lines 155 to 160, the fieldset includes a
non-existent "website" field for the TrademarkMatch model which will raise an
admin error; remove "website" from the ("organization", "website") tuple or
replace it with the actual field name defined on TrademarkMatch (e.g., "site" or
"url") if intended—update the fieldset to only reference valid model fields and
run the Django admin to verify no errors.
| websites = Website.objects.filter(name__isnull=False).exclude(name="") | ||
| for website in websites: | ||
| try: | ||
| org = None | ||
| if hasattr(website, "organization"): | ||
| org = website.organization | ||
| _perform_trademark_check(name=website.name, website=website, organization=org) | ||
| website_count += 1 | ||
| self.stdout.write(f" ✓ {website.name}") | ||
| except Exception as e: | ||
| self.stdout.write(self.style.ERROR(f" ✗ {website.name}: {str(e)}")) |
There was a problem hiding this comment.
Missing import and invalid function parameter cause runtime errors.
Two issues will crash the command when processing websites:
-
Line 53:
Websiteis referenced but never imported - this will raise aNameError. -
Line 59:
_perform_trademark_check()is called withwebsite=website, but according towebsite/signals.py(lines 54-84), the function signature only acceptsnameandorganizationparameters.
🔎 Proposed fix
from django.core.management.base import BaseCommand
-from website.models import Organization
+from website.models import Organization, Website
from website.signals import _perform_trademark_checkAnd fix the function call (assuming website parameter support is added to _perform_trademark_check, or remove the website parameter):
- _perform_trademark_check(name=website.name, website=website, organization=org)
+ _perform_trademark_check(name=website.name, organization=org)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In website/management/commands/check_all_trademarks.py around lines 53 to 63,
the command refers to Website but never imports it and calls
_perform_trademark_check with an unexpected website= parameter; fix by adding
the proper import (from website.models import Website or the correct module
path) at the top of the file, and update the call to match the function
signature used in website/signals.py (call
_perform_trademark_check(name=website.name, organization=org) — remove the
website= argument — unless you deliberately change the trademark function to
accept a website parameter, in which case update that function signature and its
callers consistently).
| parser.add_argument( | ||
| "--threshold", | ||
| type=float, | ||
| default=85.0, | ||
| help="Match threshold 0-100 (default: 85.0)", | ||
| ) |
There was a problem hiding this comment.
Unused --threshold argument.
The --threshold argument is defined but never used. get_trademark_matches() internally creates a TrademarkMatcher(threshold=85.0) with a hardcoded threshold.
Either remove the unused argument or pass it to a custom matcher:
🔎 Option 1: Remove unused argument
def add_arguments(self, parser):
"""Add command arguments."""
parser.add_argument(
"--company",
type=str,
default="BugHeist",
help="Company name to search for (default: BugHeist)",
)
- parser.add_argument(
- "--threshold",
- type=float,
- default=85.0,
- help="Match threshold 0-100 (default: 85.0)",
- )🔎 Option 2: Use the threshold argument
+from website.services.trademark_matching import TrademarkMatcher, SAMPLE_TRADEMARKS
-from website.services.trademark_matching import get_trademark_matches
def handle(self, *args, **options):
"""Execute the command."""
company = options["company"]
+ threshold = options["threshold"]
self.stdout.write(self.style.SUCCESS(f"\n🔍 Searching for trademark matches for: '{company}'"))
self.stdout.write("=" * 60)
- matches = get_trademark_matches(company)
+ matcher = TrademarkMatcher(threshold=threshold)
+ matches = matcher.match(company, SAMPLE_TRADEMARKS)📝 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.
| parser.add_argument( | |
| "--threshold", | |
| type=float, | |
| default=85.0, | |
| help="Match threshold 0-100 (default: 85.0)", | |
| ) | |
| parser.add_argument( | |
| "--company", | |
| type=str, | |
| default="BugHeist", | |
| help="Company name to search for (default: BugHeist)", | |
| ) |
🤖 Prompt for AI Agents
In website/management/commands/trademark_demo.py around lines 24-29, the
--threshold argparse option is defined but never used; either remove the
argument or thread its value into the matcher creation. Preferred fix: add a
parameter to get_trademark_matches (or accept threshold in the call site) and
pass args.threshold through so TrademarkMatcher is constructed with
threshold=args.threshold instead of the hardcoded 85.0; alternatively, if
threshold is not needed, delete the parser.add_argument block and any references
to args.threshold.
| def check_website_trademarks(sender, instance, created, **kwargs): | ||
| """ | ||
| When a website is created or updated, automatically check for trademark conflicts. | ||
| """ | ||
| if not instance.name: | ||
| return | ||
|
|
||
| # Skip if already checked very recently | ||
| recent_check = ( | ||
| TrademarkMatch.objects.filter( | ||
| website=instance, | ||
| ) | ||
| .filter(checked_at__gte=timezone.now() - timezone.timedelta(days=1)) | ||
| .exists() | ||
| ) | ||
|
|
||
| if recent_check and not created: | ||
| return # Skip if updated but recently checked | ||
|
|
||
| # Run trademark check | ||
| org = None | ||
| if hasattr(instance, "organization"): | ||
| org = instance.organization | ||
|
|
||
| _perform_trademark_check( | ||
| name=instance.name, | ||
| organization=org, | ||
| website=instance, | ||
| ) |
There was a problem hiding this comment.
Critical: Missing signal registration and parameter mismatches.
This function has multiple critical issues:
-
Missing
@receiverdecorator: Line 24 is missing the decorator to register this signal handler. Without it, the function will never be called automatically when a Website is saved. -
Invalid field reference: Line 34 filters
TrademarkMatchbywebsite=instance, but according to the model definition (website/models.py lines 37-104), the website field was removed from TrademarkMatch. This will raise aFieldErrorat runtime. -
Parameter mismatch: Line 51 passes
website=instanceto_perform_trademark_check, but the function signature on line 55 only acceptsnameandorganizationparameters. This will raise aTypeError.
🔎 Proposed fixes
Add the missing decorator and remove references to the non-existent website field:
+@receiver(post_save, sender=Website)
def check_website_trademarks(sender, instance, created, **kwargs):
"""
When a website is created or updated, automatically check for trademark conflicts.
"""
if not instance.name:
return
- # Skip if already checked very recently
- recent_check = (
- TrademarkMatch.objects.filter(
- website=instance,
- )
- .filter(checked_at__gte=timezone.now() - timezone.timedelta(days=1))
- .exists()
- )
-
- if recent_check and not created:
- return # Skip if updated but recently checked
-
# Run trademark check
org = None
if hasattr(instance, "organization"):
org = instance.organization
_perform_trademark_check(
name=instance.name,
organization=org,
- website=instance,
)Note: The Website model import will also need to be added to the imports at the top of the file.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def check_website_trademarks(sender, instance, created, **kwargs): | |
| """ | |
| When a website is created or updated, automatically check for trademark conflicts. | |
| """ | |
| if not instance.name: | |
| return | |
| # Skip if already checked very recently | |
| recent_check = ( | |
| TrademarkMatch.objects.filter( | |
| website=instance, | |
| ) | |
| .filter(checked_at__gte=timezone.now() - timezone.timedelta(days=1)) | |
| .exists() | |
| ) | |
| if recent_check and not created: | |
| return # Skip if updated but recently checked | |
| # Run trademark check | |
| org = None | |
| if hasattr(instance, "organization"): | |
| org = instance.organization | |
| _perform_trademark_check( | |
| name=instance.name, | |
| organization=org, | |
| website=instance, | |
| ) | |
| @receiver(post_save, sender=Website) | |
| def check_website_trademarks(sender, instance, created, **kwargs): | |
| """ | |
| When a website is created or updated, automatically check for trademark conflicts. | |
| """ | |
| if not instance.name: | |
| return | |
| # Run trademark check | |
| org = None | |
| if hasattr(instance, "organization"): | |
| org = instance.organization | |
| _perform_trademark_check( | |
| name=instance.name, | |
| organization=org, | |
| ) |
🤖 Prompt for AI Agents
In website/signals.py around lines 24–52, register the handler and align its
behavior with the current TrademarkMatch model and _perform_trademark_check
signature: add the @receiver(post_save, sender=Website) decorator and import
receiver, post_save and Website at the top; replace the TrademarkMatch filter
that references the removed website field with a filter using existing fields
(e.g., filter(name=instance.name, organization=org) or whatever current unique
fields are on TrademarkMatch) to check recent_check; and call
_perform_trademark_check with only the accepted parameters (name=instance.name,
organization=org) — remove website=instance from that call. Ensure org is
derived safely from instance.organization before use.
| def _perform_trademark_check(name, organization=None): | ||
| """ | ||
| Core function to perform trademark matching and store results. | ||
| """ | ||
| try: | ||
| matches = get_matches_for_website(name, threshold=85.0) | ||
|
|
||
| if organization: | ||
| TrademarkMatch.objects.filter(organization=organization).delete() | ||
|
|
There was a problem hiding this comment.
Major: Unconditional deletion loses historical data and manual reviews.
Line 63 deletes all previous TrademarkMatch records for the organization every time a check is performed. This approach has significant downsides:
- Loses manual review data: If a maintainer has reviewed matches and set
is_reviewed=Trueor updated thestatusfield, this information is permanently lost. - No audit trail: Historical trademark checks and their evolution over time are erased.
- Race conditions: If multiple checks run concurrently (unlikely but possible with signals), they could interfere with each other.
Consider instead:
- Update existing matches if they still appear in the new results
- Mark old matches as stale rather than deleting them
- Only delete matches with
status='pending'andis_reviewed=False - Or maintain a complete history and add a "latest check" timestamp to Organization
🔎 Suggested approach for preserving reviewed matches
try:
matches = get_matches_for_website(name, threshold=85.0)
if organization:
- TrademarkMatch.objects.filter(organization=organization).delete()
+ # Only delete unreviewed pending matches
+ TrademarkMatch.objects.filter(
+ organization=organization,
+ is_reviewed=False,
+ status='pending'
+ ).delete()
for match in matches:🤖 Prompt for AI Agents
In website/signals.py around lines 55–64, the code unconditionally deletes all
TrademarkMatch records for the organization which loses reviewed data and
history; instead, fetch existing matches for the organization, compute which
incoming matches correspond to existing records (by trademark identifier/name),
update those existing records with refreshed scores/status/timestamps, mark
existing matches that no longer appear as stale (e.g., set status='stale' and
update a last_seen/updated_at), and only delete records that are both
status='pending' and is_reviewed=False if you must remove anything;
alternatively add/maintain a latest_check timestamp on Organization and append
new matches rather than blasting historical rows. Ensure operations are done in
a transaction to avoid race conditions.
| CHROME_PATH = "/Applications/Google Chrome.app/Contents/MacOS/Google Chrome" | ||
| NO_CHROME = not os.path.exists(CHROME_PATH) | ||
|
|
||
|
|
||
| @skipIf(NO_CHROME, "Chrome not installed locally; skipping Selenium UI tests") |
There was a problem hiding this comment.
Hardcoded macOS path will skip Selenium tests on Linux/Windows even when Chrome is available.
The path /Applications/Google Chrome.app/Contents/MacOS/Google Chrome is macOS-specific. This means Selenium tests will be unconditionally skipped on Linux CI environments (common for GitHub Actions) even when Chrome/Chromium is installed.
Consider using shutil.which to detect Chrome cross-platform, or use the chromedriver-autoinstaller's capabilities to detect Chrome availability:
🔎 Proposed cross-platform detection
+import shutil
+
-CHROME_PATH = "/Applications/Google Chrome.app/Contents/MacOS/Google Chrome"
-NO_CHROME = not os.path.exists(CHROME_PATH)
+def _chrome_available():
+ """Check if Chrome/Chromium is available on any platform."""
+ # Check common executable names
+ for name in ("google-chrome", "chromium", "chromium-browser", "chrome"):
+ if shutil.which(name):
+ return True
+ # macOS app bundle
+ macos_path = "/Applications/Google Chrome.app/Contents/MacOS/Google Chrome"
+ if os.path.exists(macos_path):
+ return True
+ return False
+
+NO_CHROME = not _chrome_available()📝 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.
| CHROME_PATH = "/Applications/Google Chrome.app/Contents/MacOS/Google Chrome" | |
| NO_CHROME = not os.path.exists(CHROME_PATH) | |
| @skipIf(NO_CHROME, "Chrome not installed locally; skipping Selenium UI tests") | |
| def _chrome_available(): | |
| """Check if Chrome/Chromium is available on any platform.""" | |
| # Check common executable names | |
| for name in ("google-chrome", "chromium", "chromium-browser", "chrome"): | |
| if shutil.which(name): | |
| return True | |
| # macOS app bundle | |
| macos_path = "/Applications/Google Chrome.app/Contents/MacOS/Google Chrome" | |
| if os.path.exists(macos_path): | |
| return True | |
| return False | |
| NO_CHROME = not _chrome_available() | |
| @skipIf(NO_CHROME, "Chrome not installed locally; skipping Selenium UI tests") |
🤖 Prompt for AI Agents
In website/tests/test_main.py around lines 38-42, the code uses a hardcoded
macOS Chrome path which causes tests to be skipped on Linux/Windows even when
Chrome/Chromium is present; replace the macOS-only check with a cross-platform
detection that sets NO_CHROME only when no browser executable can be found—use
shutil.which to look for common names ("google-chrome", "chrome", "chromium",
"chromium-browser") or leverage chromedriver-autoinstaller's detection API;
update the boolean accordingly so @skipIf only triggers when none of these are
available.
| @require_GET | ||
| @login_required | ||
| def website_trademark_check(request, website_id): | ||
| """ | ||
| JSON API to get trademark matches for a specific website. | ||
| GET /api/website/{website_id}/trademarks/ | ||
| """ | ||
| website = get_object_or_404(Website, id=website_id) | ||
|
|
||
| matches = TrademarkMatch.objects.filter(website=website).order_by("-similarity_score") | ||
|
|
||
| data = { | ||
| "website_id": website.id, | ||
| "website_name": website.name, | ||
| "total_matches": matches.count(), | ||
| "high_risk_count": matches.filter(risk_level="high").count(), | ||
| "matches": [ | ||
| { | ||
| "id": m.id, | ||
| "matched_trademark": m.matched_trademark_name, | ||
| "similarity_score": m.similarity_score, | ||
| "risk_level": m.risk_level, | ||
| "status": m.status, | ||
| "is_reviewed": m.is_reviewed, | ||
| "checked_at": m.checked_at.isoformat(), | ||
| } | ||
| for m in matches | ||
| ], | ||
| } | ||
|
|
||
| return JsonResponse(data) |
There was a problem hiding this comment.
Critical: Missing import and invalid field reference.
This endpoint has two critical issues that will cause runtime errors:
-
Missing import: Line 75 uses
Websitebut it's not imported. This will raise aNameError. The import statement on line 10 only includesOrganizationandTrademarkMatch. -
Invalid field reference: Line 77 filters
TrademarkMatch.objects.filter(website=website), but according to the model definition (website/models.py lines 37-104), the website field was removed from the TrademarkMatch model. This will raise aFieldError.
🔎 Proposed fixes
Add the missing import and update the logic if the Website relationship is needed:
from website.models import Organization, TrademarkMatch
+from website.models import Website
from website.services.trademark_integration import get_trademark_reportFor the field reference issue, you'll need to determine the correct relationship. If websites should be checked via their associated organization:
def website_trademark_check(request, website_id):
"""
JSON API to get trademark matches for a specific website.
GET /api/website/{website_id}/trademarks/
"""
website = get_object_or_404(Website, id=website_id)
- matches = TrademarkMatch.objects.filter(website=website).order_by("-similarity_score")
+ # If website has an organization relationship:
+ if website.organization:
+ matches = TrademarkMatch.objects.filter(organization=website.organization).order_by("-similarity_score")
+ else:
+ matches = TrademarkMatch.objects.none()Alternatively, if the website field should be added back to the TrademarkMatch model, that's a model-level decision that needs to be addressed there first.
Committable suggestion skipped: line range outside the PR's diff.
github-project-automation
bot
moved this from Backlog
to Ready
in 📌 OWASP BLT Project Board
Jayant2908
left a comment
Jayant2908
left a comment
There was a problem hiding this comment.
Do fix the coderabbitai issues and ask for re-review, I'll look it up!
|
@aaditya8979 Please resolve all the comments from code-rabbit, also it would be quite better to create an issue and get that verified from Donnie. |
github-actions
bot
added
last-active: 0d
|
💬 Reminder: Unresolved Conversations Hi @aaditya8979! This pull request has 10 unresolved conversations that need to be addressed. Please review and resolve the pending discussions so we can move forward with merging this PR. Thank you! 🙏 |
github-actions
bot
added
last-active: 1d
|
💬 Reminder: Unresolved Conversations Hi @aaditya8979! This pull request has 10 unresolved conversations that need to be addressed. Please review and resolve the pending discussions so we can move forward with merging this PR. Thank you! 🙏 |
github-actions
bot
added
last-active: 0d
4 participants
This pull request improves BLT’s trademark matching workflow and aligns the touched code with the project’s existing tooling and test expectations.
Summary
• Implements and refines reusable trademark matching logic in the services layer, so all consumers (APIs, commands, and tests) share a single, consistent implementation.
• Integrates this logic into the relevant API views and management commands that need to compute or display trademark matches.
• Updates and extends tests to cover successful matches, no‑match situations, and error‑handling paths (including mocked SMTP failures in weekly bug digests), while keeping the overall suite green.
• Applies automatic import sorting and formatting to all modified Python files so that style checks no longer fail on this code after a full pre‑commit run.
Key changes
• Centralized trademark matching behavior in dedicated service functions/classes, reducing duplication and making the matching rules easier to maintain.
• Ensured that trademark‑related endpoints and background jobs call the new services and correctly handle returned match data and edge cases.
• Clarified the behavior of weekly bug digest tests that intentionally raise SMTP exceptions via mocks, confirming that these logged errors are expected and fully caught inside the command.
• Cleaned up import order, unused imports, and formatting issues flagged by the project’s linters/formatters, and removed OS‑generated files from version control so they no longer interfere with checks.
Impact
• Functional behavior for end users remains compatible, but trademark processing is now more robust, test‑backed, and maintainable.
• Future contributors working in these areas can rely on consistent services, clearer tests, and stable automated checks when running the validation pipeline.
Summary by CodeRabbit
Release Notes
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.