Improved NestBot /events command #657
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
abhayymishraa
force-pushed
the
feat/events
branch
from
53d9e26 to
eee4b37
Compare
After this commit:
Chnages Look Like:
|
|
|
hey @arkid15r , i removed the syncing lofgic for removing any data once it will be removed from the data then we can remove the fields that were previously used by them |
|
|
||
| def resolve_events(root, info): | ||
| """Resolve all events.""" | ||
| return Event.objects.all() |
There was a problem hiding this comment.
We are interested in current/upcoming events only
backend/apps/owasp/models/event.py
Outdated
| category = models.CharField(verbose_name="Category", max_length=100, default="", blank=True) | ||
| category_description = models.TextField( | ||
| verbose_name="Category Description", default="", blank=True | ||
| ) |
There was a problem hiding this comment.
This should be something like project level as they are shared for all events
backend/apps/owasp/models/event.py
Outdated
| level = models.CharField(verbose_name="Level", max_length=5, default="", blank=True) | ||
| optional_text = models.TextField(verbose_name="Additional Text", default="", blank=True) | ||
| start_date = models.DateField(verbose_name="Start Date", null=True, blank=True) |
There was a problem hiding this comment.
This can't be null, we need it for filtering/sorting
backend/apps/owasp/models/event.py
Outdated
| category_description = models.TextField( | ||
| verbose_name="Category Description", default="", blank=True | ||
| ) | ||
| dates = models.CharField(verbose_name="Dates", max_length=100, default="", blank=True) | ||
| level = models.CharField(verbose_name="Level", max_length=5, default="", blank=True) |
There was a problem hiding this comment.
It seems level can be removed.
There was a problem hiding this comment.
yeah i left there cause it required in the last data
backend/apps/owasp/models/event.py
Outdated
| category_description = models.TextField( | ||
| verbose_name="Category Description", default="", blank=True | ||
| ) | ||
| dates = models.CharField(verbose_name="Dates", max_length=100, default="", blank=True) |
There was a problem hiding this comment.
It'd be great to parse this and populate end_date field
|
|
||
| for event_data in category["events"]: | ||
| event_name_slug = slugify(event_data.get("name", "")) | ||
| key = f"www-event-{event_name_slug}" |
There was a problem hiding this comment.
We don't need the prefix.
| } | ||
|
|
||
| try: | ||
| event = Event.objects.get(name=fields["name"]) |
There was a problem hiding this comment.
Use a unique field (key)
backend/apps/owasp/models/event.py
Outdated
| level = models.CharField(verbose_name="Level", max_length=5, default="", blank=True) | ||
| optional_text = models.TextField(verbose_name="Additional Text", default="", blank=True) |
There was a problem hiding this comment.
| optional_text = models.TextField(verbose_name="Additional Text", default="", blank=True) | |
| desctiption = models.TextField(verbose_name="Additional Text", default="", blank=True) |
|
|
||
| def resolve_events(root, info): | ||
| """Resolve all events.""" | ||
| today = datetime.now(timezone.utc).date() |
There was a problem hiding this comment.
Use timezone.now() from django.utils
| """Resolve all events.""" | ||
| today = datetime.now(timezone.utc).date() | ||
|
|
||
| base_query = Event.objects.exclude(start_date__isnull=True).filter(start_date__gte=today) |
There was a problem hiding this comment.
start_date should not be nullable
backend/apps/owasp/models/event.py
Outdated
|
|
||
| owasp_repository = models.ForeignKey( | ||
| "github.Repository", on_delete=models.SET_NULL, blank=True, null=True | ||
| category_description = models.TextField( |
There was a problem hiding this comment.
You can remove this field. The description should belong to category entity, not to the event as it's always tied to the category.
| @staticmethod | ||
| def bulk_save(events, fields=None): | ||
| """Bulk save events.""" | ||
| BulkSaveModel.bulk_save(Event, events, fields=fields) | ||
|
|
There was a problem hiding this comment.
I believe you should keep this method but just update its logic based on the new requirements.
backend/apps/slack/utils.py
Outdated
| @@ -83,6 +84,32 @@ def get_staff_data(timeout=30): | |||
| logger.exception("Unable to parse OWASP staff data file", extra={"file_path": file_path}) | |||
|
|
|||
|
|
|||
| def get_events_data(): | |||
| """Get raw events data via GraphQL.""" | |||
| from apps.owasp.graphql.queries.event import EventQuery | |||
There was a problem hiding this comment.
NestBot goes directly to the DB
There was a problem hiding this comment.
sure :) Although it suggested by you to use graphql
Co-authored-by: Arkadii Yakovets <[email protected]>
|
After changes it will look like this:
|
Summary by CodeRabbit
WalkthroughThis pull request implements comprehensive modifications to event processing across the backend. A new Makefile target is added and integrated into data updates, while event handling is removed from some GitHub commands and the Django admin and API layers. GraphQL support for events is introduced, and a new management command imports events from a remote YAML file. The Event model and corresponding migration have been revised by removing and adding several fields and methods. Additionally, Slack commands now dynamically fetch and format events, tests are updated accordingly, and a utility function for slugification is added along with a new dependency. Changes
Suggested labels
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
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: 3
🔭 Outside diff range comments (1)
backend/tests/github/management/commands/github_update_owasp_organization_test.py (1)
71-73:⚠️ Potential issueInconsistency in test case
There's an inconsistency in this test case. The "event: 1" entry is still present in the expected_calls dictionary, but the Event model imports and related mock patches have been removed from the test file.
This will likely cause the test to fail since the code won't be checking for Event-related calls anymore.
- (None, 0, {"project": 1, "chapter": 1, "committee": 1, "event": 1}), - (None, 1, {"project": 0, "chapter": 1, "committee": 1, "event": 1}), + (None, 0, {"project": 1, "chapter": 1, "committee": 1}), + (None, 1, {"project": 0, "chapter": 1, "committee": 1}),
♻️ Duplicate comments (3)
backend/apps/owasp/graphql/queries/event.py (2)
17-23: 🛠️ Refactor suggestionFilter for events with non-null start_date.
Based on previous feedback, start_date should not be nullable. Add an additional filter to ensure we only retrieve events with valid start dates.
def resolve_events(root, info): """Resolve all events.""" today = timezone.now() - base_query = Event.objects.filter(start_date__gte=today) + base_query = Event.objects.filter(start_date__isnull=False, start_date__gte=today) return base_query.order_by("start_date")
3-4: 🛠️ Refactor suggestionUse Django's timezone instead of Python's datetime.timezone.
Django provides its own timezone utility that is aware of the project's timezone settings.
-from datetime import timezone +from django.utils import timezonebackend/apps/owasp/management/commands/owasp_update_events.py (1)
25-29: 🛠️ Refactor suggestionEnsure uniqueness with a more robust key.
Creating a key solely from the event name may lead to conflicts if different events have similar names. Consider including additional uniqueness factors like date or category.
event_name_slug = slugify(event_data.get("name", "")) - key = event_name_slug + # Create a more unique key by incorporating date information + date_info = event_data.get("start-date", "").replace("-", "") + key = f"{event_name_slug}-{date_info}" if date_info else event_name_slug end_date = parse_event_dates( event_data.get("dates", ""), event_data.get("start-date") )
🧹 Nitpick comments (11)
backend/apps/owasp/utils.py (1)
50-51: Consider logging parsing errors for debugging.Silently catching all exceptions may make debugging difficult when unexpected date formats appear.
- except (ValueError, IndexError, AttributeError): + except (ValueError, IndexError, AttributeError) as e: + import logging + logging.getLogger(__name__).debug(f"Error parsing date '{dates}': {e}") return Nonebackend/apps/owasp/management/commands/owasp_update_events.py (1)
54-55: Add error handling for bulk save operation.The bulk save operation could potentially fail if there are validation errors with any of the events.
if events: self.stdout.write(f"Saving {len(events)} events...") - Event.bulk_save(events, fields) - self.stdout.write(self.style.SUCCESS("Successfully saved events")) + try: + Event.bulk_save(events, fields) + self.stdout.write(self.style.SUCCESS("Successfully saved events")) + except Exception as e: + self.stdout.write(self.style.ERROR(f"Error saving events: {e}"))backend/tests/owasp/models/project_test.py (2)
80-91: Test setup could be clearer with separation of concerns.The test is now mixing concerns between mocking HTTP responses and testing project behavior. Consider extracting the mock setup to a separate method or fixture.
@patch("apps.owasp.models.project.Project.objects.get") @patch("apps.github.utils.requests.get") def test_update_data_project_does_not_exist(self, mock_requests_get, mock_get): """Test updating project data when the project doesn't exist.""" mock_get.side_effect = Project.DoesNotExist + # Setup mock response for GitHub API mock_response = Mock() mock_response.text = """ # Project Title Some project description """ mock_requests_get.return_value = mock_response - # Setup test data + # Setup repository test data gh_repository_mock = Mock() gh_repository_mock.name = "new_repo" repository_mock = Repository()
78-80: Test name doesn't fully match test behavior.The test now includes verification of GitHub API interaction but the name only suggests testing for non-existent projects.
@patch("apps.owasp.models.project.Project.objects.get") @patch("apps.github.utils.requests.get") - def test_update_data_project_does_not_exist(self, mock_requests_get, mock_get): + def test_update_data_project_does_not_exist_with_github_data(self, mock_requests_get, mock_get): """Test updating project data when the project doesn't exist."""backend/tests/slack/commands/events_test.py (2)
46-46: Remove parentheses from @pytest.fixture decorators.Ruff recommends removing parentheses, so prefer
@pytest.fixtureover@pytest.fixture().Here's a quick fix:
- @pytest.fixture() + @pytest.fixture(and similarly at line 52)
Also applies to: 52-52
🧰 Tools
🪛 Ruff (0.8.2)
46-46: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
58-80: Consider verifying stable sorting for identical start dates.Currently, the parameterized test checks sorting by date, but you may want to add a scenario with multiple events sharing the same
start_dateto ensure consistent ordering and stable sorting behavior.backend/apps/slack/commands/events.py (1)
20-23: Consider adding error handling for get_events_data failures.If
get_events_dataraises an unexpected exception, this command may fail silently. A try/except block or fallback error message could improve user experience.backend/tests/owasp/models/event_test.py (2)
19-19: Consider a more flexible date approach.
Using a fixed date is generally fine for testing, but you might use a dedicated fixture or a library likefreezegunto reduce potential coupling to a specific date.
23-25: Expand coverage for bulk save with varied dates.
You're assigning the same start date for both mock events. Consider adding variety to verify the model's handling of different dates.backend/apps/owasp/models/event.py (2)
24-29: Check future needs for category length.
A max_length of 20 is typically sufficient, but be prepared to expand if new categories arising from external sources exceed this length.
31-36: Validate uniqueness and date relationships.
Havingkeyuniquely identified is a good choice, but ensure collisions won’t occur if users manually set it. Also consider adding validation logic to guaranteestart_date <= end_datewhenend_dateis present.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
backend/Makefile(3 hunks)backend/apps/github/management/commands/github_update_owasp_organization.py(0 hunks)backend/apps/owasp/admin.py(0 hunks)backend/apps/owasp/api/event.py(0 hunks)backend/apps/owasp/graphql/nodes/event.py(1 hunks)backend/apps/owasp/graphql/queries/__init__.py(1 hunks)backend/apps/owasp/graphql/queries/event.py(1 hunks)backend/apps/owasp/management/commands/owasp_update_events.py(1 hunks)backend/apps/owasp/migrations/0016_remove_event_created_at_and_more.py(1 hunks)backend/apps/owasp/models/event.py(1 hunks)backend/apps/owasp/utils.py(1 hunks)backend/apps/slack/commands/events.py(2 hunks)backend/apps/slack/utils.py(2 hunks)backend/tests/github/management/commands/github_update_owasp_organization_test.py(1 hunks)backend/tests/owasp/api/event_test.py(0 hunks)backend/tests/owasp/models/event_test.py(2 hunks)backend/tests/owasp/models/project_test.py(1 hunks)backend/tests/slack/commands/events_test.py(1 hunks)
💤 Files with no reviewable changes (4)
- backend/apps/owasp/admin.py
- backend/tests/owasp/api/event_test.py
- backend/apps/owasp/api/event.py
- backend/apps/github/management/commands/github_update_owasp_organization.py
🧰 Additional context used
🪛 Ruff (0.8.2)
backend/tests/slack/commands/events_test.py
46-46: Use @pytest.fixture over @pytest.fixture()
Remove parentheses
(PT001)
52-52: Use @pytest.fixture over @pytest.fixture()
Remove parentheses
(PT001)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: CodeQL (python)
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
🔇 Additional comments (18)
backend/apps/owasp/graphql/nodes/event.py (1)
1-21: Clean and well-structured implementationThe EventNode implementation looks good - it properly defines a GraphQL node for the Event model with appropriate field selections. The structure follows the project conventions with proper docstrings and organization.
backend/apps/slack/utils.py (2)
11-11: Clean import additionThe timezone import is appropriately placed with the other imports.
87-96: Well-implemented database query functionThis function properly retrieves events from the database, filtering for current/future events and sorting them by start date. The exception handling with appropriate logging is a good practice.
Note: Based on past review comments, there was a discussion about using GraphQL vs direct database access. Either approach is valid, and this implementation works well.
backend/tests/github/management/commands/github_update_owasp_organization_test.py (4)
54-55: Event references removed correctlyThe removal of the "event" entry from the expected_calls dictionary aligns with the PR objective of removing event synchronization logic.
59-60: Event references removed correctlyThe removal of the "event" entry from the expected_calls dictionary aligns with the PR objective of removing event synchronization logic.
64-65: Event references removed correctlyThe removal of the "event" entry from the expected_calls dictionary aligns with the PR objective of removing event synchronization logic.
69-70: Event references removed correctlyThe removal of the "event" entry from the expected_calls dictionary aligns with the PR objective of removing event synchronization logic.
backend/apps/owasp/graphql/queries/__init__.py (2)
5-5: Good import additionThe EventQuery import is properly added.
10-10: EventQuery properly integratedThe EventQuery is now correctly included in the OwaspQuery inheritance chain, enabling event-related GraphQL queries in the application.
backend/tests/slack/commands/events_test.py (1)
82-124: Thorough test coverage for command responses.Verifying Slack message blocks for multiple event categories and scenarios is comprehensive and well-structured.
backend/apps/slack/commands/events.py (1)
38-64: Well-structured Slack block construction.The categorical grouping and block-building logic cleanly displays multiple events, creating a user-friendly experience.
backend/Makefile (3)
91-91: Shift to Poetry aligns with updated dependency management.Using Poetry for scraping tasks ensures consistency and controlled environments.
93-95: Excellent addition of the 'owasp-update-events' target.This new target smoothly integrates event updates into the existing workflow.
130-131: Including 'owasp-update-events' in 'update-data' maintains consistency.Tying event updates into the main data update process improves automation and thoroughness.
backend/tests/owasp/models/event_test.py (1)
1-1: Good use of thedateimport.
This import is properly utilized for setting the event start date in your tests.backend/apps/owasp/migrations/0016_remove_event_created_at_and_more.py (1)
1-86: Confirm data loss risks when removing fields.
This migration drops multiple fields, adding new ones with different semantics. Ensure that crucial data isn’t lost or, if it is irrelevant now, that removing it is intentional. Also verify whether any data migration is needed prior to dropping these fields, especially in production.backend/apps/owasp/models/event.py (2)
8-15: Clean and maintainable category choices.
UsingTextChoicesneatly encapsulates your categories and makes the code more readable.
17-17: DroppingRepositoryBasedEntityModelseems appropriate.
Removing GitHub-specific logic aligns with the revised event management approach.
backend/apps/owasp/utils.py
Outdated
| if start_date: | ||
| start_date_parsed = start_date | ||
| month = start_date_parsed.strftime("%B") # Full month name (e.g., "May") | ||
|
|
There was a problem hiding this comment.
Fix inconsistent type handling for start_date.
The function signature indicates start_date is a string or None, but this code assumes it's already a datetime object. This could cause AttributeError when strftime is called on a string.
# Use the start_date to determine the month if provided
if start_date:
- start_date_parsed = start_date
+ start_date_parsed = parser.parse(start_date).date() if isinstance(start_date, str) else start_date
month = start_date_parsed.strftime("%B") # Full month name (e.g., "May")📝 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.
| if start_date: | |
| start_date_parsed = start_date | |
| month = start_date_parsed.strftime("%B") # Full month name (e.g., "May") | |
| # Use the start_date to determine the month if provided | |
| if start_date: | |
| start_date_parsed = parser.parse(start_date).date() if isinstance(start_date, str) else start_date | |
| month = start_date_parsed.strftime("%B") # Full month name (e.g., "May") |
| "start_date": event_data.get("start-date", None), | ||
| "url": normalize_url(event_data.get("url", "")) or "", |
There was a problem hiding this comment.
❓ Verification inconclusive
Ensure start_date is properly parsed.
The start_date is directly set from the YAML data without parsing, but the Event model likely expects a datetime object.
"key": key,
"name": event_data.get("name", ""),
"description": event_data.get("optional-text", ""),
- "start_date": event_data.get("start-date", None),
+ "start_date": parser.parse(event_data.get("start-date")).date() if event_data.get("start-date") else None,
"url": normalize_url(event_data.get("url", "")) or "",🏁 Script executed:
#!/bin/bash
# Check the Event model definition to confirm start_date field type
rg "start_date" -A 1 -B 1 "apps/owasp/models/event.py"Length of output: 119
Attention: Confirm that the Event model accepts a date object
The change correctly converts the YAML string into a date object using parser.parse(...).date(). However, since we could not locate the Event model (the file apps/owasp/models/event.py wasn’t found), please manually verify that the start_date field in the Event model indeed expects a date object. If the field requires a full datetime, you may need to remove the .date() conversion.
- File & Lines:
backend/apps/owasp/management/commands/owasp_update_events.py(original lines 37-38) - Diff snippet:
"start_date": event_data.get("start-date", None), - "start_date": event_data.get("start-date", None), + "start_date": parser.parse(event_data.get("start-date")).date() if event_data.get("start-date") else None,
📝 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.
| "start_date": event_data.get("start-date", None), | |
| "url": normalize_url(event_data.get("url", "")) or "", | |
| "key": key, | |
| "name": event_data.get("name", ""), | |
| "description": event_data.get("optional-text", ""), | |
| "start_date": parser.parse(event_data.get("start-date")).date() if event_data.get("start-date") else None, | |
| "url": normalize_url(event_data.get("url", "")) or "", |
backend/Makefile
Outdated
| @@ -97,7 +101,7 @@ recreate-schema: | |||
| @CMD="psql -U nest_user_dev -d nest_db_dev -c \ | |||
| 'DROP SCHEMA public CASCADE; CREATE SCHEMA public; GRANT ALL ON SCHEMA public TO nest_user_dev'" \ | |||
| $(MAKE) exec-db-command 2>/dev/null | |||
| $(MAKE) migrate | |||
| $(MAKE) migrateNestBot goes directly to the DB | |||
There was a problem hiding this comment.
Potential issue with 'migrateNestBot goes directly to the DB'.
Check whether "migrateNestBot" is a valid target or a leftover comment, as this line might cause a Makefile error.
- $(MAKE) migrateNestBot goes directly to the DB
+ # Possibly intended:
+ $(MAKE) migrate📝 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.
| $(MAKE) migrateNestBot goes directly to the DB | |
| # Possibly intended: | |
| $(MAKE) migrate |
There was a problem hiding this comment.
This suggestion is actually useful
There was a problem hiding this comment.
@arkid15r I'm glad you found the suggestion helpful. Would you like me to implement this change for you? The current command $(MAKE) migrateNestBot goes directly to the DB would fail when executed since it's not a valid Makefile target. Changing it to $(MAKE) migrate would restore the correct behavior.
backend/Makefile
Outdated
| @CMD="poetry run python manage.py owasp_scrape_projects" $(MAKE) exec-backend-command | ||
|
|
||
| owasp-update-events: | ||
| @echo "Getting OWASP events data" | ||
| @CMD="poetry run python manage.py owasp_update_events" $(MAKE) exec-backend-command |
There was a problem hiding this comment.
Why changing this and adding poetry?
There was a problem hiding this comment.
i can fix that
backend/Makefile
Outdated
| @@ -97,7 +101,7 @@ recreate-schema: | |||
| @CMD="psql -U nest_user_dev -d nest_db_dev -c \ | |||
| 'DROP SCHEMA public CASCADE; CREATE SCHEMA public; GRANT ALL ON SCHEMA public TO nest_user_dev'" \ | |||
| $(MAKE) exec-db-command 2>/dev/null | |||
| $(MAKE) migrate | |||
| $(MAKE) migrateNestBot goes directly to the DB | |||
There was a problem hiding this comment.
This suggestion is actually useful
|
Done @arkid15r . |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
backend/apps/owasp/graphql/queries/event.py (1)
15-17: Update docstring to accurately reflect method functionalityThe current docstring says "Resolve all events" but the method is specifically resolving upcoming events.
def resolve_upcoming_events(root, info): - """Resolve all events.""" + """Resolve upcoming events.""" return Event.upcoming_eventsbackend/apps/owasp/models/event.py (2)
46-48: Consider including ongoing events in upcoming_events queryThe current implementation only returns events with a start date in the future. Consider also including events that have already started but haven't ended yet.
@staticmethod def upcoming_events(): """Get upcoming events.""" - return Event.objects.filter(start_date__gte=timezone.now().date()).order_by("start_date") + today = timezone.now().date() + return Event.objects.filter( + models.Q(start_date__gte=today) | + models.Q(end_date__gte=today) + ).order_by("start_date")
55-97: Consider simplifying date parsing logicThe current date parsing logic has hard-coded assumptions about date formats and only handles specific patterns. Consider using a more robust approach that can handle various date formats more generically.
While the current implementation works for the expected formats, a more maintainable solution would be to use dateutil's parser more directly with error handling for unexpected formats. Also, consider documenting the expected date formats in the docstring.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
backend/poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
backend/apps/common/utils.py(3 hunks)backend/apps/github/utils.py(1 hunks)backend/apps/owasp/graphql/queries/event.py(1 hunks)backend/apps/owasp/management/commands/owasp_update_events.py(1 hunks)backend/apps/owasp/migrations/0018_merge_20250302_1945.py(1 hunks)backend/apps/owasp/models/event.py(1 hunks)backend/pyproject.toml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- backend/apps/github/utils.py
- backend/apps/owasp/migrations/0018_merge_20250302_1945.py
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: CodeQL (python)
- GitHub Check: Run frontend e2e tests
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
🔇 Additional comments (4)
backend/apps/common/utils.py (3)
3-3: Import addition for regex operations.Added the
remodule which is required for the newslugifyfunction.
9-9: Well handled import renaming.The Django slugify function is appropriately aliased to avoid naming conflicts with the new utility function.
50-52: Good implementation of the slugify utility function.The function enhances Django's built-in slugify by ensuring there are no consecutive hyphens in the generated slug, which is useful for creating clean, readable URLs and identifiers for events.
backend/pyproject.toml (1)
42-42: Dependency addition is appropriate for date handlingThe addition of the
python-dateutildependency is necessary for the date parsing functionality used in the Event model'sparse_datesmethod. This is a widely-used library for handling date and time operations in Python.
| def handle(self, *args, **kwargs): | ||
| data = yaml.safe_load( | ||
| get_repository_file_content( | ||
| "https://raw.githubusercontent.com/OWASP/owasp.github.io/main/_data/events.yml" | ||
| ) | ||
| ) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for HTTP request failure
The command fetches a YAML file from GitHub but doesn't handle potential HTTP request failures. This could cause the command to crash if the request fails.
def handle(self, *args, **kwargs):
- data = yaml.safe_load(
- get_repository_file_content(
- "https://raw.githubusercontent.com/OWASP/owasp.github.io/main/_data/events.yml"
- )
- )
+ try:
+ content = get_repository_file_content(
+ "https://raw.githubusercontent.com/OWASP/owasp.github.io/main/_data/events.yml"
+ )
+ data = yaml.safe_load(content)
+ except Exception as e:
+ self.stderr.write(self.style.ERROR(f"Failed to fetch or parse events data: {e}"))
+ return📝 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 handle(self, *args, **kwargs): | |
| data = yaml.safe_load( | |
| get_repository_file_content( | |
| "https://raw.githubusercontent.com/OWASP/owasp.github.io/main/_data/events.yml" | |
| ) | |
| ) | |
| def handle(self, *args, **kwargs): | |
| - data = yaml.safe_load( | |
| - get_repository_file_content( | |
| - "https://raw.githubusercontent.com/OWASP/owasp.github.io/main/_data/events.yml" | |
| - ) | |
| - ) | |
| + try: | |
| + content = get_repository_file_content( | |
| + "https://raw.githubusercontent.com/OWASP/owasp.github.io/main/_data/events.yml" | |
| + ) | |
| + data = yaml.safe_load(content) | |
| + except Exception as e: | |
| + self.stderr.write(self.style.ERROR(f"Failed to fetch or parse events data: {e}")) | |
| + return |
backend/apps/owasp/models/event.py
Outdated
| "description": data.get("optional-text", ""), | ||
| "end_date": Event.parse_dates(data.get("dates", ""), data.get("start-date")), | ||
| "name": data["name"], | ||
| "start_date": data["start-date"], |
There was a problem hiding this comment.
Parse start_date string to date object
The start_date is assigned directly from data["start-date"] without any parsing. If the input is a string, this will cause issues when trying to assign it to a DateField.
- "start_date": data["start-date"],
+ "start_date": parser.parse(data["start-date"]).date() if isinstance(data["start-date"], str) else data["start-date"],📝 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.
| "start_date": data["start-date"], | |
| "start_date": parser.parse(data["start-date"]).date() if isinstance(data["start-date"], str) else data["start-date"], |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/apps/owasp/models/event.py (2)
55-97: Date parsing could benefit from refactoring.The parse_dates method handles specific date formats but is quite complex with nested conditionals. The TODO comment correctly acknowledges that this should be refactored.
Consider using a more structured approach with regex patterns or a dedicated date parsing library that can handle more formats consistently.
# An example of a more structured approach could use a library like dateparser: # pip install dateparser def parse_dates(dates, start_date): """Parse event dates with more consistent handling.""" import dateparser if not dates: return None # Try to directly parse the end date try: return dateparser.parse(dates).date() except (ValueError, AttributeError): # If direct parsing fails, try to extract end date from range try: if "-" in dates and "," in dates: # Handle date ranges like "May 26-30, 2025" date_parts = dates.split(", ") if len(date_parts) == 2: range_part, year = date_parts parts = range_part.split() if len(parts) == 2: month, days = parts end_day = days.split("-")[-1] end_date_str = f"{month} {end_day}, {year}" return dateparser.parse(end_date_str).date() except Exception: pass return None
114-133: Well-structured data mapping in from_dict.The from_dict method handles category mapping appropriately and includes proper date parsing. The specific issues with start_date parsing mentioned in previous review comments have been addressed.
One improvement to consider:
The category mapping is hardcoded which could be brittle if the source data changes. Consider a more flexible approach:
- "category": { - "AppSec Days": Event.Category.APPSEC_DAYS, - "Global": Event.Category.GLOBAL, - "Partner": Event.Category.PARTNER, - }.get(category, Event.Category.OTHER), + "category": next( + (cat for cat_value, cat_name in Event.Category.choices + if cat_name.lower() == category.lower()), + Event.Category.OTHER + ),This would match category names case-insensitively, making the mapping more robust to small variations in input data.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/apps/owasp/models/event.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: CodeQL (python)
- GitHub Check: Run frontend e2e tests
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
🔇 Additional comments (8)
backend/apps/owasp/models/event.py (8)
3-5: Well-structured imports.Good addition of dateutil parser for date parsing and timezone for datetime operations. These are needed for the new date handling functionality.
8-9: Appropriate utility imports.Adding the slugify utility for key generation and normalize_url for URL handling is a good practice for maintaining consistent data formats.
12-12: Model inheritance simplified.Removing
RepositoryBasedEntityModelfrom the inheritance chain is appropriate since events are no longer sourced from repository data as mentioned in previous comments.
19-26: Good use of TextChoices for category enumeration.Using Django's TextChoices for the Category enum is a clean approach. It provides good type safety, database representation, and human-readable labels.
27-32: Well-defined category field with appropriate defaults.The category field is properly configured with a reasonable max_length, choices from the TextChoices enum, and a sensible default value.
34-39: Well-structured model fields.The added fields follow good practices:
- end_date is nullable which is appropriate
- key has uniqueness constraint which ensures data integrity
- name has a reasonable length limit
- description allows empty values with blank=True
- start_date is correctly required (non-nullable)
- url field properly allows empty values
45-48: Efficient upcoming events query.The
upcoming_eventsmethod efficiently filters events by date and orders them properly. Using the database for filtering is more performant than filtering in Python.
99-112: Good approach to event updates using slugified keys.The update_data method now properly uses slugify for consistent key generation and has appropriate exception handling for the get operation.
|
Resolves #641
Add the PR description here.
Task Completed:
get_events_data()in utils to fetch data from the source filePreview/ScreenShots:
Slack:
TestcaseCoverage: