Update owasp app event model with calendar attributes and add reminder models #2128
Conversation
Summary by CodeRabbit
WalkthroughAdds and iterates on OWASP Event model: introduces/removes fields via migrations, renames calendar_id to google_calendar_id, adjusts Category and upcoming_events, and alters Status choices. Separately, adds Nest Reminder and ReminderSchedule models with migrations and tests, and exports them in nest.models. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested labels
Suggested reviewers
✨ 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: 5
🧹 Nitpick comments (6)
backend/apps/owasp/models/event.py (4)
62-64: Clarify what “calendar_id” represents; consider adding event-scoped identifiers.Name suggests the Google Calendar “calendarId,” not the event id. For reliable sync and de-duplication you’ll likely need either the Google “id” and/or “iCalUID” per event.
Options:
- Keep calendar_id for the container and add:
- calendar_event_id = Google Events “id”
- ical_uid = Google Events “iCalUID”
- Make (“calendar_id”, “calendar_event_id”) unique to avoid duplicates.
Example additions:
calendar_id = models.CharField( - verbose_name="Calendar ID", max_length=1024, blank=True, default="" + verbose_name="Calendar ID", + help_text="Google Calendar identifier (calendarId), e.g. primary or [email protected]", + max_length=1024, blank=True, default="" ) +calendar_event_id = models.CharField( + verbose_name="Calendar Event ID", + help_text="Google Events resource id (per calendar).", + max_length=1024, blank=True, default="", +) +ical_uid = models.CharField( + verbose_name="iCalUID", + help_text="Stable event identifier across calendars.", + max_length=1024, blank=True, default="", +)
78-83: Index type/status for query performance; add a composite index for listing.upcoming_events now filters by type. Add db indexes to avoid table scans and consider an index optimized for the listing order.
status = models.CharField( verbose_name="Status", max_length=11, choices=Status.choices, default=Status.CONFIRMED, + db_index=True, ) type = models.CharField( verbose_name="Type", max_length=11, choices=Type.choices, default=Type.EXISTING, + db_index=True, )Add a composite index:
class Meta: db_table = "owasp_events" indexes = [ models.Index(fields=["-start_date"], name="event_start_date_desc_idx"), + models.Index(fields=["type", "-start_date"], name="event_type_start_date_desc_idx"), ]Also applies to: 88-93
111-114: Tighten upcoming_events: include “today”, filter out non‑confirmed.
- start_date is a DateField; comparing to timezone.now() is implicit casting. Use timezone.localdate() for clarity.
- Include events starting today (gte).
- Hide canceled/cancelled events from “upcoming”.
return ( Event.objects.filter( - start_date__gt=timezone.now(), - type=Event.Type.EXISTING, + start_date__gte=timezone.localdate(), + type=Event.Type.EXISTING, + status=Event.Status.CONFIRMED, )
332-346: Prevent costly AI/geocoding calls for synced calendar events.save() triggers OpenAI and geocoding every time if fields are empty. For bulk calendar syncs this will be slow, expensive, and brittle.
def save(self, *args, **kwargs): """Save the event instance.""" - if not self.suggested_location: - self.generate_suggested_location() - - if not self.latitude or not self.longitude: - self.generate_geo_location() + if self.type == Event.Type.EXISTING: + if not self.suggested_location: + self.generate_suggested_location() + if not self.latitude or not self.longitude: + self.generate_geo_location() super().save(*args, **kwargs)backend/apps/owasp/migrations/0049_event_calendar_id_event_channel_id_event_member_and_more.py (2)
51-60: Add indexes to support new query patterns.Given upcoming_events filters by type and orders by start_date, adding indexes in this migration will prevent regressions.
Add operations (after AddField ops):
+ migrations.AddIndex( + model_name="event", + index=models.Index(fields=["type", "-start_date"], name="event_type_start_date_desc_idx"), + ), + migrations.AddIndex( + model_name="event", + index=models.Index(fields=["status"], name="event_status_idx"), + ),
13-25: Consider constraints for calendar integrity (optional).If type="calendar", you likely want calendar_id present (and later calendar_event_id/ical_uid). A partial CHECK keeps data clean.
Example:
+ migrations.AddConstraint( + model_name="event", + constraint=models.CheckConstraint( + name="calendar_requires_calendar_id", + check=( + models.Q(type="calendar", calendar_id__gt="") | models.Q(~models.Q(type="calendar")) + ), + ), + ),
📜 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 (2)
backend/apps/owasp/migrations/0049_event_calendar_id_event_channel_id_event_member_and_more.py(1 hunks)backend/apps/owasp/models/event.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/apps/owasp/models/event.py (1)
backend/apps/slack/models/event.py (1)
Event(11-79)
⏰ 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). (3)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
🔇 Additional comments (2)
backend/apps/owasp/models/event.py (1)
65-66: channel_id length consistency verified – no changes needed
- Confirmed
backend/apps/slack/models/event.pydefineschannel_idwithmax_length=15.- Confirmed
backend/apps/owasp/models/event.pylikewise defineschannel_idwithmax_length=15.- No other
channel_id = models.CharField(definitions with differing lengths were found.All references are consistent. No further action required.
backend/apps/owasp/migrations/0049_event_calendar_id_event_channel_id_event_member_and_more.py (1)
1-12: Migration dependencies look correct.Depends on owasp.0048 and slack.0018; matches model reference to slack.Member.
backend/apps/owasp/migrations/0049_event_calendar_id_event_channel_id_event_member_and_more.py
Show resolved
Hide resolved
backend/apps/owasp/migrations/0049_event_calendar_id_event_channel_id_event_member_and_more.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/apps/owasp/models/event.py (2)
233-247: Ensure start_date is parsed before using it in parse_dates.from_dict passes start_date (possibly a str) into parse_dates, which expects a date and calls .strftime; this silently returns None due to the AttributeError catch, dropping end_date for ranges.
- start_date = data["start-date"] + start_date_raw = data["start-date"] + start_date_date = ( + parser.parse(start_date_raw).date() + if isinstance(start_date_raw, str) + else start_date_raw + ) @@ - "end_date": Event.parse_dates(data.get("dates", ""), start_date), - "name": data["name"], - "start_date": parser.parse(start_date).date() - if isinstance(start_date, str) - else start_date, + "end_date": Event.parse_dates(data.get("dates", ""), start_date_date), + "name": data["name"], + "start_date": start_date_date,Optionally, update parse_dates signature to accept date | str and normalize internally.
341-347: Gate expensive enrichments behind a flag and fix DateField filtering
- Add a
compute_extras: bool = Truekwarg toEvent.save()and only invoke LLM/geocoding whencompute_extrasis true andself.type == Event.Type.EXISTING.- Correct the
start_date__gt=timezone.now()filter (DateField vs datetime mismatch) at line 113 ofbackend/apps/owasp/models/event.pyto usetimezone.localdate()(ordate.today()) instead oftimezone.now().Proposed diffs:
--- a/backend/apps/owasp/models/event.py +++ b/backend/apps/owasp/models/event.py @@ -336,7 +336,8 @@ class Event(models.Model): - def save(self, *args, **kwargs): + def save(self, *args, compute_extras: bool = True, **kwargs): # only run expensive enrichments when explicitly enabled and for EXISTING events - if not self.suggested_location: - self.generate_suggested_location() - - if not self.latitude or not self.longitude: - self.generate_geo_location() + if compute_extras and self.type == Event.Type.EXISTING: + if not self.suggested_location: + self.generate_suggested_location() + if not self.latitude or not self.longitude: + self.generate_geo_location() super().save(*args, **kwargs)--- a/backend/apps/owasp/models/event.py +++ b/backend/apps/owasp/models/event.py @@ -110,7 +110,7 @@ class Event(models.Model): @classmethod def upcoming_existing(cls): return Event.objects.filter( - start_date__gt=timezone.now(), + start_date__gt=timezone.localdate(), type=Event.Type.EXISTING, )
♻️ Duplicate comments (2)
backend/apps/owasp/models/event.py (2)
206-211: Prevent key collisions; scope lookup by type and derive stable keys for calendar events.Key is unique and currently slugified from name only. Calendar feeds frequently repeat titles (e.g., “Weekly Meeting”), leading to IntegrityError. Also, lookup by key alone ignores the new type dimension.
Proposed minimal, backwards‑compatible changes:
- For type=CALENDAR, derive key from a stable external identifier (ical_uid, event_id, id) and calendar_id.
- Lookup by (type, key). Set event.type when creating.
- key = slugify(data["name"]) - try: - event = Event.objects.get(key=key) - except Event.DoesNotExist: - event = Event(key=key) + event_type = data.get("type", Event.Type.EXISTING) + if event_type == Event.Type.CALENDAR: + external_id = data.get("ical_uid") or data.get("event_id") or data.get("id") + calendar_id = data.get("calendar_id", "") + key = f"gcal:{calendar_id}:{external_id or slugify(data['name'])}" + else: + key = slugify(data["name"]) + + try: + event = Event.objects.get(type=event_type, key=key) + except Event.DoesNotExist: + event = Event(key=key, type=event_type) + # Optional: set calendar_id on first creation if provided + if event_type == Event.Type.CALENDAR and data.get("calendar_id"): + event.calendar_id = data["calendar_id"]Even stronger: change uniqueness to a composite constraint and drop unique=True on key (requires migration/backfill). See Meta suggestion below.
I can draft the safe migration and backfill if you want.
78-84: Option: move to composite uniqueness (type, key).If you adopt (type, key) uniqueness, drop unique=True here and add a UniqueConstraint in Meta. This avoids cross‑type collisions and reflects the domain model better.
- key = models.CharField(verbose_name="Key", max_length=100, unique=True) + key = models.CharField(verbose_name="Key", max_length=100)Add to Meta:
class Meta: db_table = "owasp_events" indexes = [ models.Index(fields=["-start_date"], name="event_start_date_desc_idx"), models.Index(fields=["type"], name="event_type_idx"), ] + constraints = [ + models.UniqueConstraint(fields=["type", "key"], name="event_type_key_uniq") + ]This change requires a migration and a backfill plan for existing rows.
I can generate the migration and a backfill script tailored to your data shape.
🧹 Nitpick comments (5)
backend/apps/owasp/migrations/0050_alter_event_status_event_event_type_idx.py (2)
27-30: Consider a composite index to match the primary query pattern.Since upcoming_events filters by type and orders by start_date, a composite index on (type, start_date) can outperform two separate indexes. Keep the existing ones if you prefer to defer, but consider adding in a follow‑up migration.
7-10: Minor: slack dependency here is unnecessary.This migration doesn’t touch slack models; the extra dependency can slow or complicate graph resolution. Safe to drop unless you intentionally need ordering.
backend/apps/owasp/models/event.py (3)
28-33: Index on type is good; consider adding a composite (type, start_date) index.Your primary read path filters by type and orders by start_date. A composite index typically beats two separate ones in Postgres. Keep current indexes if you prefer incremental changes.
Example:
class Meta: db_table = "owasp_events" indexes = [ models.Index(fields=["-start_date"], name="event_start_date_desc_idx"), models.Index(fields=["type"], name="event_type_idx"), + models.Index(fields=["type", "start_date"], name="event_type_start_date_idx"), ]
66-67: Make channel_id optional in forms.channel_id has a default but not blank=True. If you use ModelForms/Admin, it will be required. Add blank=True to match calendar_id/url semantics.
- channel_id = models.CharField(verbose_name="Channel ID", max_length=15, default="") + channel_id = models.CharField(verbose_name="Channel ID", max_length=15, default="", blank=True)
320-327: Avoid emitting “None” in the LLM context.When end_date is None, the context line reads “Dates: YYYY-MM-DD - None”. Minor but noisy for prompt quality. Prefer a single date when there’s no end_date.
- if include_dates: - context.append(f"Dates: {self.start_date} - {self.end_date}") + if include_dates: + if self.end_date: + context.append(f"Dates: {self.start_date} - {self.end_date}") + else: + context.append(f"Date: {self.start_date}")
📜 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 (2)
backend/apps/owasp/migrations/0050_alter_event_status_event_event_type_idx.py(1 hunks)backend/apps/owasp/models/event.py(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-26T01:50:54.008Z
Learnt from: ahmedxgouda
PR: OWASP/Nest#2128
File: backend/apps/owasp/migrations/0049_event_calendar_id_event_channel_id_event_member_and_more.py:27-36
Timestamp: 2025-08-26T01:50:54.008Z
Learning: In the OWASP Nest project, Event.member ForeignKey should use CASCADE deletion because events should be removed when the associated Slack member is deleted, as per the project's business requirements.
Applied to files:
backend/apps/owasp/models/event.py
🧬 Code graph analysis (1)
backend/apps/owasp/models/event.py (2)
backend/apps/slack/models/event.py (1)
Event(11-79)frontend/src/types/event.ts (1)
Event(1-10)
⏰ 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). (3)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
🔇 Additional comments (2)
backend/apps/owasp/models/event.py (2)
67-73: CASCADE on member aligns with project requirements.Per retrieved learnings for this repository, events should be removed when the associated Slack member is deleted. Keeping on_delete=CASCADE here is correct.
Note: I used your saved learning to validate this choice. If requirements change later, SET_NULL is the alternative.
50-56: Status = “cancelled” spelling fixed — good alignment with Google Calendar.This prevents future mapping churn and keeps API parity.
backend/apps/owasp/migrations/0050_alter_event_status_event_event_type_idx.py
Show resolved
Hide resolved
ahmedxgouda
changed the base branch from
main
to
feature/nestbot-calendar-events
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/apps/owasp/models/event.py (2)
185-199: Unique key collisions for recurring/same-named calendar events.
Key is derived from slugified name → collisions (“Weekly Meeting”) and IntegrityError.Apply a safer key strategy using external identifiers when available:
- key = slugify(data["name"]) + key = None + gcal_cal_id = data.get("calendar_id") + gcal_event_id = data.get("id") or data.get("event_id") + ical_uid = data.get("ical_uid") + if gcal_cal_id and (gcal_event_id or ical_uid): + key = f"gcal:{gcal_cal_id}:{gcal_event_id or ical_uid}" + elif ical_uid: + key = f"ical:{ical_uid}" + else: + key = slugify(data["name"])If you re-add
type, consider a composite uniqueness:class Meta: constraints = [ models.UniqueConstraint(fields=["type", "key"], name="event_type_key_uniq"), ] ]Add a data migration to backfill keys safely before enforcing changes.
151-167: parse_dates(): incorrect month override for explicit ranges.
For strings like “May 26-30, 2025” you overwrite the parsed month with start_date’s month → wrong end_date.Patch:
- # Extract month and day range - month = parts[0] + # Extract month and day range + month_token = parts[0] day_range = "".join(parts[1:]) @@ - # Use the start_date to determine the month if provided - if start_date: - start_date_parsed = start_date - month = start_date_parsed.strftime("%B") # Full month name (e.g., "May") + # Determine month: trust explicit token; otherwise fall back to start_date + if month_token.isalpha(): + month = month_token + elif start_date: + month = start_date.strftime("%B") + else: + return NoneAdd unit tests for both “May 26-30, 2025” and “26-30, 2025” with different start_date months.
♻️ Duplicate comments (1)
backend/apps/owasp/models/event.py (1)
91-97: DateField vs DateTime mismatch in upcoming_events().
start_date is a DateField; comparing to timezone.now() is incorrect.Apply:
- Event.objects.filter( - start_date__gt=timezone.now(), - ) + Event.objects.filter( + start_date__gt=timezone.localdate(), + )Optional: once CANCELLED is modeled, exclude it here to reduce noise.
🧹 Nitpick comments (4)
backend/apps/slack/models/remainder.py (3)
9-13: Scheduling performance: add ordering + indexes on remind_at and (channel, remind_at)Needed for “next reminder” queries and channel scans.
(Already included in the Meta diff above.)
28-28: Track lifecycleAdd timestamps for auditing and operations.
periodic = models.BooleanField(default=False, verbose_name="Is Periodic") + + created_at = models.DateTimeField(auto_now_add=True) + updated_at = models.DateTimeField(auto_now=True)
30-32: Don’t dump full message in str (noise, admin rendering); truncate- return f"Reminder for {self.member} in {self.channel}: {self.message} at {self.remind_at}" + msg = (self.message[:80] + "...") if len(self.message) > 80 else self.message + return f"Reminder for {self.member} in {self.channel}: {msg} at {self.remind_at}"backend/apps/owasp/models/event.py (1)
312-326: Heavy side-effects in save(); consider async/deferrable workflows.
Geocoding and LLM calls on every save impact latency and retry semantics.Suggestions:
- Gate with feature flags and persist “geocoded_at/generated_at” timestamps.
- Push generation to a task queue; compute on-demand if missing.
- Catch and log network errors without blocking DB writes.
📜 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 (3)
backend/apps/owasp/migrations/0051_remove_event_event_type_idx_remove_event_channel_id_and_more.py(1 hunks)backend/apps/owasp/models/event.py(2 hunks)backend/apps/slack/models/remainder.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-26T01:50:54.020Z
Learnt from: ahmedxgouda
PR: OWASP/Nest#2128
File: backend/apps/owasp/migrations/0049_event_calendar_id_event_channel_id_event_member_and_more.py:27-36
Timestamp: 2025-08-26T01:50:54.020Z
Learning: In the OWASP Nest project, Event.member ForeignKey should use CASCADE deletion because events should be removed when the associated Slack member is deleted, as per the project's business requirements.
Applied to files:
backend/apps/owasp/models/event.py
⏰ 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). (3)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
🔇 Additional comments (8)
backend/apps/slack/models/remainder.py (2)
1-33: Scope check: Slack model in a calendar-events PRThis model appears unrelated to Issue #2127 (event model + calendar attrs). Consider moving to a focused PR or justify coupling to calendar work.
1-33: Migrations and timezone config verified: no prior migrations referenceslack_remainders/Remainder, andUSE_TZis already set toTrue.backend/apps/owasp/models/event.py (2)
42-42: COMMUNITY category addition — LGTM.
Matches prior discussion to surface community events as a distinct bucket.
201-231: New calendar/status fields aren’t populated in from_dict().
No mapping for calendar/event IDs or status from the input data, so fields will remain default/blank.If your data dict includes these keys, extend mapping:
fields = { "category": { "AppSec Days": Event.Category.APPSEC_DAYS, "Global": Event.Category.GLOBAL, "Partner": Event.Category.PARTNER, }.get(category, Event.Category.OTHER), "description": data.get("optional-text", ""), "end_date": Event.parse_dates(data.get("dates", ""), start_date), "name": data["name"], "start_date": parser.parse(start_date).date() if isinstance(start_date, str) else start_date, "url": normalize_url(data.get("url", "")) or "", + # Optional calendar mappings (adjust keys to your payload): + "calendar_id": data.get("calendar_id", "") or data.get("ical_uid", "") or "", + "status": (Event.Status.CONFIRMED), }Confirm the exact payload shape for Google Calendar ingestion so we can wire this correctly and add tests.
backend/apps/owasp/migrations/0051_remove_event_event_type_idx_remove_event_channel_id_and_more.py (4)
12-15: Dropping event_type_idx is fine only if type stays removed.
If you retaintype, keep or recreate the index for filters and deduping.I can prep the migration either way—confirm desired direction.
17-23: Removing member/channel aligns with decoupling, but verify business rules.
Prior learning: Event.member FK used CASCADE per business requirement. With removal, ensure the replacement (e.g., Reminder model) preserves expected lifecycle behaviors.I can generate a follow-up migration and model stub for Reminder if needed.
31-43: Category change — LGTM.
Community added to choices; default remains “other”.
46-53: Status limited to confirmed — confirm ingestion strategy.
If Google Calendar feeds include cancelled/tentative items, either model them or filter them at import.If you decide to model them, I’ll extend Status and provide a small data migration (also update upcoming_events to exclude CANCELLED).
...d/apps/owasp/migrations/0051_remove_event_event_type_idx_remove_event_channel_id_and_more.py
Show resolved
Hide resolved
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
backend/apps/slack/models/__init__.py (1)
5-5: Re-export looks good; consider stabilizing the package API via all.Minor: define all to make public surface explicit and tooling-friendly.
Apply outside the selected range:
# At end of file __all__ = [ "Conversation", "Event", "Member", "Message", "Reminder", "Workspace", ]backend/tests/apps/slack/models/reminder_test.py (2)
3-4: Import from the package API for consistency.Since Reminder is re-exported, prefer
from apps.slack.models import Member, Reminderto avoid deep imports.-from apps.slack.models.member import Member -from apps.slack.models.reminder import Reminder +from apps.slack.models import Member, Reminder
25-33: Add a test for periodic default and channel_id non-empty.
- Validate default periodic=False when omitted.
- If we disallow empty channel_id (recommended), assert validation error or non-empty value.
I can add a parametric test covering defaults and basic validation once the model constraints are confirmed.
backend/apps/slack/models/reminder.py (2)
9-13: Add ordering and operational timestamps.To support common queries, add default ordering by remind_at and optional created/updated timestamps.
class Reminder(models.Model): @@ class Meta: db_table = "slack_reminders" verbose_name = "Slack Reminder" verbose_name_plural = "Slack Reminders" + ordering = ["remind_at", "id"] + + created_at = models.DateTimeField(auto_now_add=True) + updated_at = models.DateTimeField(auto_now=True)Note: requires a migration.
26-31: Stabilize str formatting and handle missing member.
- Use ISO 8601 for datetime to avoid locale-dependent str().
- Guard when member is None.
- Optionally truncate message in logs.
def __str__(self) -> str: """Reminder human readable representation.""" - return ( - f"Reminder for {self.member} in channel: " - f"{self.channel_id}: {self.message} at {self.remind_at}" - ) + who = str(self.member) if self.member else "Unknown member" + when = self.remind_at.isoformat() if hasattr(self.remind_at, "isoformat") else str(self.remind_at) + msg = (self.message[:60] + "…") if len(self.message) > 60 else self.message + return f"Reminder for {who} in channel: {self.channel_id}: {msg} at {when}"
📜 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 (4)
backend/apps/slack/migrations/0019_reminder.py(1 hunks)backend/apps/slack/models/__init__.py(1 hunks)backend/apps/slack/models/reminder.py(1 hunks)backend/tests/apps/slack/models/reminder_test.py(1 hunks)
⏰ 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 backend tests
- GitHub Check: Run frontend e2e tests
🔇 Additional comments (1)
backend/apps/slack/models/__init__.py (1)
5-5: Scope check: changes don’t match PR title/issue.This PR advertises Event model/calendar attributes, but this file (and others) add a Slack Reminder model. Confirm scope or split PRs to keep changes focused.
…update-event-model
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/apps/owasp/models/event.py (2)
89-97: Fix DateField vs DateTime comparison in upcoming_events.start_date is a DateField; compare to timezone.localdate() to avoid implicit casts and edge bugs.
- Event.objects.filter( - start_date__gt=timezone.now(), - ) + Event.objects.filter( + start_date__gt=timezone.localdate(), + )
211-229: Populate google_calendar_id from ingestion data.Field is never set in from_dict; add it so saves persist the ID.
fields = { "category": { "AppSec Days": Event.Category.APPSEC_DAYS, "Global": Event.Category.GLOBAL, "Partner": Event.Category.PARTNER, }.get(category, Event.Category.OTHER), "description": data.get("optional-text", ""), "end_date": Event.parse_dates(data.get("dates", ""), start_date), "name": data["name"], + "google_calendar_id": data.get("google_calendar_id", "").strip(), "start_date": parser.parse(start_date).date() if isinstance(start_date, str) else start_date, "url": normalize_url(data.get("url", "")) or "", }
♻️ Duplicate comments (3)
backend/apps/owasp/models/event.py (3)
185-199: Prevent key collisions; derive key from stable external ID when present.key=slugify(name) will collide for same-named/recurring calendar events. Use google_calendar_id when provided.
- key = slugify(data["name"]) + gcal_id = (data.get("google_calendar_id") or "").strip() + key = f"gcal:{gcal_id}" if gcal_id else slugify(data["name"])Also consider a composite uniqueness if/when type is added:
class Meta: constraints = [ models.UniqueConstraint( fields=["type", "key"], name="event_type_key_uniq" ) ]I can supply the migration/backfill if you want.
44-55: Reintroduce Event.Type discriminator required by Issue #2127.The model still lacks the type field (curated/existing vs calendar). Downstream ingestion, keying, and filters depend on it.
class Event(BulkSaveModel, TimestampedModel): @@ class Status(models.TextChoices): """Event status.""" CONFIRMED = "confirmed", "Confirmed" + + class Type(models.TextChoices): + """Event source/type.""" + EXISTING = "existing", "Existing" + CALENDAR = "calendar", "Calendar" @@ category = models.CharField( verbose_name="Category", max_length=11, choices=Category.choices, default=Category.OTHER, ) + type = models.CharField( + verbose_name="Type", + max_length=9, + choices=Type.choices, + default=Type.EXISTING, + db_index=True, + )Add a migration and backfill type=CALENDAR where google_calendar_id != "".
55-58: Clarify the identifier: field name vs. label vs. comment are inconsistent.Code comment says “event entity ID” but field is google_calendar_id with label “Google Calendar ID” (suggests calendar, not event). Align the label (minimal) or rename the field (preferred).
- # Google Calendar event entity ID - google_calendar_id = models.CharField( - verbose_name="Google Calendar ID", max_length=1024, blank=True, default="" - ) + # Google Calendar event entity ID (Event.id) + google_calendar_id = models.CharField( + verbose_name="Google Calendar Event ID", max_length=1024, blank=True, default="" + )If you actually need the calendar’s ID too, add a separate google_calendar_calendar_id field and index both. I can draft the migration if needed.
Also mirror this label change in migration 0052 (AlterField).
🧹 Nitpick comments (16)
backend/apps/owasp/migrations/0052_remove_event_calendar_id_event_google_calendar_id.py (1)
19-21: Consider indexing google_calendar_id for lookups and de-duplication.If you’ll query by this field, add an index now to avoid a follow-up migration.
operations = [ + migrations.AddIndex( + model_name="event", + index=models.Index(fields=["google_calendar_id"], name="event_gcal_id_idx"), + ), ]backend/apps/owasp/models/event.py (1)
28-33: Add an index for google_calendar_id in Meta to support ingestion/upserts.If you’ll match incoming events by this ID, index it in the model and DB.
class Meta: db_table = "owasp_events" indexes = [ models.Index(fields=["-start_date"], name="event_start_date_desc_idx"), + models.Index(fields=["google_calendar_id"], name="event_gcal_id_idx"), ]backend/apps/nest/models/__init__.py (1)
1-8: Make public API explicit via all.Export list is implicit and can trip linters. Define all to document the intended surface and avoid F401 nits.
Apply:
from .api_key import ApiKey from .badge import Badge from .google_account_authorization import GoogleAccountAuthorization from .reminder import Reminder from .reminder_schedule import ReminderSchedule from .user import User from .user_badge import UserBadge + +__all__ = [ + "ApiKey", + "Badge", + "GoogleAccountAuthorization", + "Reminder", + "ReminderSchedule", + "User", + "UserBadge", +]backend/tests/apps/nest/models/reminder_schedule_test.py (2)
19-23: Use timezone-aware datetimes instead of strings.Assigning ISO strings to DateTimeField bypasses tz handling. Use aware datetimes to mirror production behavior.
+from django.utils import timezone +from datetime import datetime, timezone as dt_timezone @@ - schedule = ReminderSchedule( - reminder=reminder, - scheduled_time="2023-01-01T12:00:00Z", + schedule = ReminderSchedule( + reminder=reminder, + scheduled_time=datetime(2023, 1, 1, 12, 0, tzinfo=dt_timezone.utc), recurrence=ReminderSchedule.Recurrence.DAILY, ) @@ - schedule = ReminderSchedule( - reminder=reminder, - scheduled_time="2023-01-01T12:00:00Z", + schedule = ReminderSchedule( + reminder=reminder, + scheduled_time=datetime(2023, 1, 1, 12, 0, tzinfo=dt_timezone.utc), recurrence=ReminderSchedule.Recurrence.DAILY, )Also applies to: 37-41
24-27: Future-proof expected string for display labels.If the model switches to display labels (see model comment), assert using get_recurrence_display() to avoid brittle coupling to stored values.
- == f"Schedule for {reminder} at {schedule.scheduled_time} ({schedule.recurrence})" + == f"Schedule for {reminder} at {schedule.scheduled_time} ({schedule.get_recurrence_display()})"backend/apps/nest/models/reminder.py (3)
14-21: Align null/blank semantics for member.Field allows null=True but not blank=True, which can be confusing in forms/admin. Consider permitting blank to match null.
- member = models.ForeignKey( + member = models.ForeignKey( "slack.Member", verbose_name="Slack Member", on_delete=models.SET_NULL, related_name="reminders", - null=True, + null=True, + blank=True, )
21-22: Avoid empty-string default for channel_id; add an index.Empty default can mask missing data. Also, channel lookups are common; add db_index.
- channel_id = models.CharField(verbose_name="Channel ID", max_length=15, default="") + channel_id = models.CharField(verbose_name="Channel ID", max_length=15, db_index=True)
24-26: Consider truncating message in str.Long messages will bloat logs/admin lists. Truncate for readability.
- return f"Reminder for {self.member} in channel: {self.channel_id}: {self.message}" + preview = (self.message[:50] + "…") if len(self.message) > 50 else self.message + return f"Reminder for {self.member} in channel: {self.channel_id}: {preview}"backend/apps/nest/models/reminder_schedule.py (5)
10-13: Add ordering for predictable schedules.Default ordering by scheduled_time improves admin lists and query consistency.
class Meta: db_table = "nest_reminder_schedules" verbose_name = "Nest Reminder Schedule" verbose_name_plural = "Nest Reminder Schedules" + ordering = ["scheduled_time"]
26-26: Index scheduled_time for common queries.Filtering by time is expected; add db_index for performance.
- scheduled_time = models.DateTimeField(verbose_name="Scheduled Time") + scheduled_time = models.DateTimeField(verbose_name="Scheduled Time", db_index=True)
34-36: Use display label for recurrence in str.Show human-friendly labels instead of raw values.
- return f"Schedule for {self.reminder} at {self.scheduled_time} ({self.recurrence})" + return f"Schedule for {self.reminder} at {self.scheduled_time} ({self.get_recurrence_display()})"
20-25: Optionally prevent duplicate schedules.If duplicates are undesirable, add a unique constraint across (reminder, scheduled_time, recurrence).
class ReminderSchedule(models.Model): @@ class Meta: db_table = "nest_reminder_schedules" verbose_name = "Nest Reminder Schedule" verbose_name_plural = "Nest Reminder Schedules" + constraints = [ + models.UniqueConstraint( + fields=["reminder", "scheduled_time", "recurrence"], + name="uniq_reminder_time_recurrence", + ) + ]Also applies to: 10-13
6-18: Scheduling engine integration and tz policy.Decide on the executor (Celery/Beat, APScheduler, Cloud scheduler) and define a single timezone policy (UTC stored; convert at edges). Add docs and signals/tasks wiring in follow-up PR.
backend/apps/nest/migrations/0008_reminder_reminderschedule.py (3)
3-5: Import Q for forthcoming constraintsIf you add a CheckConstraint (see below), import Q here.
import django.db.models.deletion from django.db import migrations, models +from django.db.models import Q
54-67: Index scheduler hot pathSchedulers typically filter by scheduled_time (and often recurrence). Add a composite index for efficient lookups.
migrations.CreateModel( name="ReminderSchedule", ... ), + migrations.AddIndex( + model_name="reminderschedule", + index=models.Index( + fields=["scheduled_time", "recurrence"], + name="idx_reminder_sched_time_recur", + ), + ),
56-67: Optional: add DB CHECK for recurrence choicesChoices enforce at model-level only. A DB check prevents bad data via raw inserts.
migrations.CreateModel( name="ReminderSchedule", ... ), + migrations.AddConstraint( + model_name="reminderschedule", + constraint=models.CheckConstraint( + check=Q(recurrence__in=["once", "daily", "weekly", "monthly"]), + name="reminderschedule_valid_recurrence", + ), + ),
📜 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 (8)
backend/apps/nest/migrations/0008_reminder_reminderschedule.py(1 hunks)backend/apps/nest/models/__init__.py(1 hunks)backend/apps/nest/models/reminder.py(1 hunks)backend/apps/nest/models/reminder_schedule.py(1 hunks)backend/apps/owasp/migrations/0052_remove_event_calendar_id_event_google_calendar_id.py(1 hunks)backend/apps/owasp/models/event.py(2 hunks)backend/tests/apps/nest/models/reminder_schedule_test.py(1 hunks)backend/tests/apps/nest/models/reminder_test.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-26T01:50:54.020Z
Learnt from: ahmedxgouda
PR: OWASP/Nest#2128
File: backend/apps/owasp/migrations/0049_event_calendar_id_event_channel_id_event_member_and_more.py:27-36
Timestamp: 2025-08-26T01:50:54.020Z
Learning: In the OWASP Nest project, Event.member ForeignKey should use CASCADE deletion because events should be removed when the associated Slack member is deleted, as per the project's business requirements.
Applied to files:
backend/apps/owasp/models/event.py
⏰ 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). (3)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
🔇 Additional comments (8)
backend/apps/owasp/models/event.py (2)
42-43: Category.COMMUNITY addition looks good.Matches prior discussion; length fits within max_length=11.
44-48: Status enum + field: OK given “confirmed-only” scope.Aligned with the “confirmed events” decision.
Also applies to: 64-69
backend/apps/nest/models/__init__.py (1)
4-5: Confirm PR scope vs exports
The PR title/description refers to updating the Event model for calendar attributes, but you’re exporting Reminder and ReminderSchedule in backend/apps/nest/models/init.py (lines 4–5). If reminders belong in this PR, update the description to include them; otherwise split out or remove these exports.backend/tests/apps/nest/models/reminder_test.py (2)
10-18: Ensure Member.str isn’t save-state dependent.If Member.str references PKs, using an unsaved instance could make the string unstable. Save the Member to be safe or confirm str only uses logical fields.
Optionally:
- member = Member(slack_user_id="U123456", username="Test User") + member = Member(slack_user_id="U123456", username="Test User") + member.save()
20-32: LGTM: metadata assertions match the model.The verbose_name and db_table checks align with Reminder.Meta.
backend/apps/nest/migrations/0008_reminder_reminderschedule.py (3)
70-77: Cascade delete from Reminder → ReminderSchedule looks correctChild schedules won’t dangle after parent deletion. LGTM.
79-84: Meta options/readability are fineClear verbose names and explicit db_table naming. LGTM.
1-11: Cross-app migration dependencies verified Bothnest.0007_alter_googleaccountauthorization_options_and_moreandslack.0018_conversation_sync_messagesexist and their dependency order is correct.
| operations = [ | ||
| migrations.RemoveField( | ||
| model_name="event", | ||
| name="calendar_id", | ||
| ), | ||
| migrations.AddField( | ||
| model_name="event", | ||
| name="google_calendar_id", | ||
| field=models.CharField( | ||
| blank=True, default="", max_length=1024, verbose_name="Google Calendar ID" | ||
| ), | ||
| ), | ||
| ] |
There was a problem hiding this comment.
Preserve data: use RenameField (then AlterField) instead of Remove/Add.
Removing then re-adding drops existing values in calendar_id. Prefer a rename to keep data, followed by an AlterField for the label.
- migrations.RemoveField(
- model_name="event",
- name="calendar_id",
- ),
- migrations.AddField(
- model_name="event",
- name="google_calendar_id",
- field=models.CharField(
- blank=True, default="", max_length=1024, verbose_name="Google Calendar ID"
- ),
- ),
+ migrations.RenameField(
+ model_name="event",
+ old_name="calendar_id",
+ new_name="google_calendar_id",
+ ),
+ migrations.AlterField(
+ model_name="event",
+ name="google_calendar_id",
+ field=models.CharField(
+ blank=True, default="", max_length=1024, verbose_name="Google Calendar Event ID"
+ ),
+ ),📝 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.
| operations = [ | |
| migrations.RemoveField( | |
| model_name="event", | |
| name="calendar_id", | |
| ), | |
| migrations.AddField( | |
| model_name="event", | |
| name="google_calendar_id", | |
| field=models.CharField( | |
| blank=True, default="", max_length=1024, verbose_name="Google Calendar ID" | |
| ), | |
| ), | |
| ] | |
| operations = [ | |
| migrations.RenameField( | |
| model_name="event", | |
| old_name="calendar_id", | |
| new_name="google_calendar_id", | |
| ), | |
| migrations.AlterField( | |
| model_name="event", | |
| name="google_calendar_id", | |
| field=models.CharField( | |
| blank=True, | |
| default="", | |
| max_length=1024, | |
| verbose_name="Google Calendar Event ID", | |
| ), | |
| ), | |
| ] |
🤖 Prompt for AI Agents
In
backend/apps/owasp/migrations/0052_remove_event_calendar_id_event_google_calendar_id.py
around lines 11 to 23, the migration removes the existing calendar_id field and
re-adds google_calendar_id which drops existing data; instead replace the
RemoveField+AddField with a migrations.RenameField from "calendar_id" to
"google_calendar_id" to preserve values, then add a separate
migrations.AlterField to adjust blank/default/verbose_name as needed.
There was a problem hiding this comment.
The old migrations are not migrated yet
There was a problem hiding this comment.
🧩 Analysis chain
@ahmedxgouda, thanks for the clarification! If the old migrations haven't been applied yet, you have a few cleaner options:
Option 1: Modify the original migration
Since migration 0049 hasn't been applied, you could edit it directly to use google_calendar_id instead of calendar_id, then delete migration 0052 entirely.
Option 2: Squash the migrations
Use Django's squashmigrations command to combine 0049-0052 into a single migration:
If you choose to keep the current approach, you're right that data preservation isn't a concern since no data exists yet. The RemoveField+AddField pattern will work fine in this case.
🏁 Script executed:
#!/bin/bash
# Show the current migration files to understand the sequence
find backend/apps/owasp/migrations/ -name "004*.py" -o -name "005*.py" | sort
Length of output: 1171
Perfect! Since migrations 0049-0052 haven't been applied yet, here's the cleanest approach:
Recommended: Squash the migrations
This will create a single, clean migration that:
- Adds only the final fields you actually want
- Avoids the back-and-forth of adding/removing fields
- Results in cleaner migration history
Alternative: Keep current approach
Since no data exists yet, your current RemoveField+AddField approach in 0052 is perfectly safe. The "data loss" concern only applies when there's actual data to lose.
Quick verification - check migration state:
If none show [X] (applied), then you're good to proceed with either approach!
🏁 Script executed:
#!/bin/bash
# Show what squashing would do (dry run)
cd backend
python manage.py squashmigrations owasp 0049 0052 --name=update_event_model_calendar --dry-run
Length of output: 334
🏁 Script executed:
#!/bin/bash
# Check if any of these migrations have been applied
cd backend
python manage.py showmigrations owasp | grep -E "(0049|0050|0051|0052)"
Length of output: 311
@ahmedxgouda, the sandbox environment doesn't have the Django dependencies installed, but I can still provide guidance based on your confirmation that the migrations haven't been applied yet.
Since migrations 0049-0052 haven't run, you have these options:
Option 1: Keep your current approach (simplest)
Your current migration 0052 with RemoveField+AddField is perfectly safe since there's no data to lose. This approach will work fine.
Option 2: Squash migrations (cleanest for production)
In your local environment, you can run:
cd backend
python manage.py squashmigrations owasp 0049 0052 --name=update_event_model_calendarThis creates a single migration replacing 0049-0052, avoiding the field churn.
Option 3: Manual edit (if you prefer)
Edit migration 0049 directly to use google_calendar_id instead of calendar_id, then delete migration 0052.
Recommendation: Since you've already written migration 0052 and confirmed no data exists, go ahead with your current approach. It's functionally equivalent to the alternatives and avoids additional work.
The data preservation concern in my original comment only applies when migrations have been applied and contain real data.
ahmedxgouda
changed the title
…update-event-model
|
arkid15r
merged commit 657ce95
into
OWASP:feature/nestbot-calendar-events
…r models (OWASP#2128) * Update event * Add slack member and channel id fields * Apply rabbit's suggestion * Apply suggestions * Add null possibility to channel in remainder * Apply rabbit suggestions * Use channel id and add tests * Apply suggestions and add reminder shedule --------- Co-authored-by: Kate Golovanova <[email protected]>
…r models (#2128) * Update event * Add slack member and channel id fields * Apply rabbit's suggestion * Apply suggestions * Add null possibility to channel in remainder * Apply rabbit suggestions * Use channel id and add tests * Apply suggestions and add reminder shedule --------- Co-authored-by: Kate Golovanova <[email protected]>
…r models (#2128) * Update event * Add slack member and channel id fields * Apply rabbit's suggestion * Apply suggestions * Add null possibility to channel in remainder * Apply rabbit suggestions * Use channel id and add tests * Apply suggestions and add reminder shedule --------- Co-authored-by: Kate Golovanova <[email protected]>
…r models (#2128) * Update event * Add slack member and channel id fields * Apply rabbit's suggestion * Apply suggestions * Add null possibility to channel in remainder * Apply rabbit suggestions * Use channel id and add tests * Apply suggestions and add reminder shedule --------- Co-authored-by: Kate Golovanova <[email protected]>
…r models (#2128) * Update event * Add slack member and channel id fields * Apply rabbit's suggestion * Apply suggestions * Add null possibility to channel in remainder * Apply rabbit suggestions * Use channel id and add tests * Apply suggestions and add reminder shedule --------- Co-authored-by: Kate Golovanova <[email protected]>
…r models (#2128) * Update event * Add slack member and channel id fields * Apply rabbit's suggestion * Apply suggestions * Add null possibility to channel in remainder * Apply rabbit suggestions * Use channel id and add tests * Apply suggestions and add reminder shedule --------- Co-authored-by: Kate Golovanova <[email protected]>
Proposed change
Resolves #2127
Checklist
make check-testlocally; all checks and tests passed.