Implement calendar events scheduling #2211
Conversation
Summary by CodeRabbit
WalkthroughAdds Google Calendar integration, Slack UI/commands for browsing and scheduling reminders, django-rq scheduling (workers/scheduler), model fields and migrations for reminders/events/job IDs, many utility/helpers, extensive unit tests, and Docker/compose/settings updates to run background workers. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 inconclusive)
✅ Passed checks (3 passed)
✨ 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. Comment |
ahmedxgouda
changed the title
![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 (13)
backend/apps/slack/blocks.py (1)
98-107: Optional: use constants for action_ids to avoid drift.Consider importing VIEW_CALENDAR_EVENTS_ACTION and using it here (and, ideally, unify other buttons in a follow-up).
backend/tests/apps/common/utils_test.py (1)
145-155: Expand cases to cover invalid/empty/None inputs (per util docstring).Add a couple of negative cases so tests lock the contract and prevent regressions.
@pytest.mark.parametrize( ("date_str", "expected"), [ ("2025-05-26T09:00:00-07:00", timezone.datetime(2025, 5, 26, 16, 0, 0, tzinfo=UTC)), ("2025-12-31T23:59:59+00:00", timezone.datetime(2025, 12, 31, 23, 59, 59, tzinfo=UTC)), ("2025-01-01T00:00:00Z", timezone.datetime(2025, 1, 1, 0, 0, 0, tzinfo=UTC)), + ("", None), + ("not-a-date", None), + (None, None), ], ) def test_parse_date_and_convert_to_local(self, date_str, expected): assert parse_date_and_convert_to_local(date_str) == expectedbackend/tests/apps/owasp/models/event_test.py (1)
162-181: LGTM: Non-confirmed events are excludedCoverage for tentative events is appropriate. Consider adding a case for canceled events, too.
backend/apps/slack/actions/home.py (1)
93-102: Harden error handling around fetching calendar blocks
calendar_events.get_blockscan raise on auth/validation; mirror the Google sign-in branch to return a friendly message instead of failing the whole publish.Apply within this case:
- blocks = calendar_events.get_blocks( - slack_user_id=user_id, - page=page, - presentation=home_presentation, - ) + try: + blocks = calendar_events.get_blocks( + slack_user_id=user_id, + page=page, + presentation=home_presentation, + ) + except (ValueError, ValidationError, PermissionError): + blocks = [markdown("Unable to fetch your calendar events. Please sign in with Google and try again.")] + logger.exception("Calendar events fetch error for user %s", user_id)backend/apps/nest/models/reminder.py (1)
14-21: Channel ID field: consider indexing and headroomSlack IDs are short but future-proofing helps, and channel lookups are common.
- channel_id = models.CharField(verbose_name="Channel ID", max_length=15, default="") + channel_id = models.CharField(verbose_name="Channel ID", max_length=20, default="", db_index=True)Also confirm migration 0009 covers the new FK and that reverse names (
reminders) on Member and Event are intentional; having the same related_name on different targets is fine.backend/tests/apps/nest/clients/google_calendar_test.py (2)
16-16: Add autospec to tighten the mock signatureKeeps the method signature honest and enables asserting the bound instance.
- @patch.object(GoogleCalendarClient, "get_events") + @patch.object(GoogleCalendarClient, "get_events", autospec=True)
35-38: Assert the client method was invokedSmall assertion improves confidence that the instance method was called.
events = client.get_events() assert len(events) == 1 assert events[0]["id"] == "12345" assert events[0]["summary"] == "Test Event" + mock_get_events.assert_called_once_with(client)backend/apps/nest/clients/google_calendar.py (2)
3-6: Add logging and HttpError import for resilience.Apply:
+import logging from django.utils import timezone from googleapiclient.discovery import build +from googleapiclient.errors import HttpError
12-17: Keep a reference to creds to persist refreshes.Apply:
- def __init__(self, google_account_authorization: GoogleAccountAuthorization): + def __init__(self, google_account_authorization: GoogleAccountAuthorization): """Initialize the Google Calendar API Client.""" - self.google_account_authorization = google_account_authorization - self.service = build( - "calendar", "v3", credentials=google_account_authorization.credentials - ) + self.google_account_authorization = google_account_authorization + self._creds = google_account_authorization.credentials + self.service = build("calendar", "v3", credentials=self._creds)backend/apps/nest/models/google_account_authorization.py (2)
158-173: Credentials property: no need for naive expiry; add None-guard.Apply:
- return Credentials( + return Credentials( token=self.access_token, refresh_token=self.refresh_token, scopes=self.scope, - # Google expects naive date in the request - expiry=self.naive_expires_at, + # google-auth accepts aware or naive; guard None safely. + expiry=self.naive_expires_at, token_uri=settings.GOOGLE_AUTH_TOKEN_URI, client_id=settings.GOOGLE_AUTH_CLIENT_ID, client_secret=settings.GOOGLE_AUTH_CLIENT_SECRET, )And adjust naive_expires_at to avoid errors if expires_at is ever null (defensive):
def naive_expires_at(self): """Get the naive datetime of token expiry.""" - return timezone.make_naive(self.expires_at) + return timezone.make_naive(self.expires_at) if self.expires_at else None
187-222: Refresh flow: log cause and keep behavior unchanged.Consider including the underlying message in the ValidationError for easier triage (without leaking secrets):
- except RefreshError as e: - error_message = "Error refreshing Google OAuth token" + except RefreshError as e: + error_message = f"Error refreshing Google OAuth token: {e.__class__.__name__}" logger.exception(error_message) raise ValidationError(error_message) from ebackend/apps/slack/common/handlers/calendar_events.py (2)
35-39: Don’t hardcode GMT; show the actual timezone.Apply:
- f"{NL}- Starts at: {event.start_date.strftime('%Y-%m-%d %H:%M')} GMT" - f"{NL}- Ends at: {event.end_date.strftime('%Y-%m-%d %H:%M')} GMT" + f"{NL}- Starts at: {event.start_date.strftime('%Y-%m-%d %H:%M %Z')}" + f"{NL}- Ends at: {event.end_date.strftime('%Y-%m-%d %H:%M %Z')}"
19-21: Optional: page bounds and day-window UX.Clamp page to >=1 and consider anchoring the window to calendar days (start/end of day) for predictability.
📜 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 (14)
backend/apps/common/utils.py(2 hunks)backend/apps/nest/clients/google_calendar.py(1 hunks)backend/apps/nest/migrations/0009_reminder_event.py(1 hunks)backend/apps/nest/models/google_account_authorization.py(1 hunks)backend/apps/nest/models/reminder.py(1 hunks)backend/apps/owasp/models/event.py(2 hunks)backend/apps/slack/actions/home.py(3 hunks)backend/apps/slack/blocks.py(1 hunks)backend/apps/slack/common/handlers/calendar_events.py(1 hunks)backend/apps/slack/constants.py(1 hunks)backend/tests/apps/common/utils_test.py(3 hunks)backend/tests/apps/nest/clients/google_calendar_test.py(1 hunks)backend/tests/apps/owasp/models/event_test.py(2 hunks)cspell/custom-dict.txt(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). (3)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
🔇 Additional comments (12)
cspell/custom-dict.txt (1)
64-64: Add dictionary entry looks good.No issues. Placement near other "g*" entries is consistent.
backend/apps/slack/blocks.py (1)
98-107: Add “View Calendar Events” button — looks good.Matches the new action_id and existing button structure.
backend/tests/apps/common/utils_test.py (1)
6-6: Imports are fine.timezone is used in expectations; new util import is correct.
Also applies to: 16-16
backend/apps/slack/constants.py (1)
29-31: Approve new calendar action constants
Handlers for these action IDs are registered in blocks.py (action_id on line 106) and handled in actions/home.py (case at lines 94–96).backend/tests/apps/owasp/models/event_test.py (2)
1-1: LGTM: UTC import for explicit tz-aware assertionsImporting UTC improves clarity in time comparisons.
5-5: LGTM: timezone usage in assertionsConsistent with Django’s timezone utilities.
backend/apps/slack/actions/home.py (3)
11-18: LGTM: Calendar events handler import wired into home actionsImport grouping is consistent with existing pattern.
22-24: LGTM: New action IDs integratedConstants are imported alongside existing actions; naming is consistent.
140-142: LGTM: Action registration includes calendar events (prev/next/root)Registration ordering and reuse of the same handler match the existing pattern.
backend/apps/owasp/models/event.py (1)
21-21: LGTM: Utility import
parse_date_and_convert_to_localimport is correct and used.backend/tests/apps/nest/clients/google_calendar_test.py (1)
15-27: LGTM: Test isolates external dependency via patch and settings overrideSolid unit boundary; avoids real HTTP/KMS.
backend/apps/nest/migrations/0009_reminder_event.py (1)
14-24: Reverse accessor “reminders” is unique on owasp.Event—no other ForeignKey on owasp.Event uses related_name="reminders".
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
backend/apps/slack/common/handlers/calendar_events.py (2)
16-19: Return a clickable Google sign-in button instead of plain text.Improves UX and matches prior suggestion.
- auth = GoogleAccountAuthorization.authorize(slack_user_id) - if not isinstance(auth, GoogleAccountAuthorization): - return [markdown(f"*Please sign in with Google first through this <{auth[0]}|link>*")] - client = GoogleCalendarClient(auth) + auth_result = GoogleAccountAuthorization.authorize(slack_user_id) + if not isinstance(auth_result, GoogleAccountAuthorization): + authorization_url, _state = auth_result + return [ + markdown("*Please sign in with Google first.*"), + { + "type": "actions", + "elements": [ + { + "type": "button", + "text": {"type": "plain_text", "text": "Sign in with Google"}, + "url": authorization_url, + } + ], + }, + ] + auth = auth_result + client = GoogleCalendarClient(auth)
64-67: Sign-in UX parity for reminder path.Apply the same “Sign in with Google” button pattern here for consistency.
- auth = GoogleAccountAuthorization.authorize(slack_user_id) - if not isinstance(auth, GoogleAccountAuthorization): - return [markdown(f"*Please sign in with Google first through this <{auth[0]}|link>*")] - client = GoogleCalendarClient(auth) + auth_result = GoogleAccountAuthorization.authorize(slack_user_id) + if not isinstance(auth_result, GoogleAccountAuthorization): + authorization_url, _state = auth_result + return [ + markdown("*Please sign in with Google first.*"), + { + "type": "actions", + "elements": [ + { + "type": "button", + "text": {"type": "plain_text", "text": "Sign in with Google"}, + "url": authorization_url, + } + ], + }, + ] + auth = auth_result + client = GoogleCalendarClient(auth)
🧹 Nitpick comments (1)
backend/apps/slack/common/handlers/calendar_events.py (1)
75-79: Harden event retrieval error handling.Network/API errors will raise and bubble to the generic handler. Return a friendly block here.
- event = Event.parse_google_calendar_event(client.get_event(google_calendar_id)) + try: + raw_event = client.get_event(google_calendar_id) + except Exception: + return [markdown("*Unable to fetch the event from Google Calendar. Please try again.*")] + event = Event.parse_google_calendar_event(raw_event)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/apps/nest/clients/google_calendar.py(1 hunks)backend/apps/nest/models/reminder.py(2 hunks)backend/apps/owasp/models/event.py(2 hunks)backend/apps/slack/commands/set_reminder.py(1 hunks)backend/apps/slack/common/handlers/calendar_events.py(1 hunks)backend/apps/slack/utils.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/apps/nest/models/reminder.py
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/apps/owasp/models/event.py
- backend/apps/nest/clients/google_calendar.py
🧰 Additional context used
🧬 Code graph analysis (3)
backend/apps/slack/commands/set_reminder.py (3)
backend/apps/slack/commands/command.py (1)
CommandBase(20-157)backend/apps/slack/common/handlers/calendar_events.py (1)
get_reminder_blocks(57-100)backend/apps/slack/utils.py (1)
parse_slack_reminder_args(220-256)
backend/apps/slack/utils.py (1)
backend/apps/slack/models/message.py (1)
text(83-85)
backend/apps/slack/common/handlers/calendar_events.py (5)
backend/apps/slack/blocks.py (2)
get_pagination_buttons(113-149)markdown(21-34)backend/apps/nest/clients/google_calendar.py (3)
GoogleCalendarClient(9-42)get_events(19-36)get_event(38-42)backend/apps/nest/models/google_account_authorization.py (2)
GoogleAccountAuthorization(28-225)authorize(55-93)backend/apps/owasp/models/event.py (3)
Event(25-369)parse_google_calendar_events(173-183)parse_google_calendar_event(186-213)backend/apps/nest/models/reminder.py (2)
Reminder(10-60)set_reminder(36-56)
ahmedxgouda
force-pushed
the
nestbot-calendar/schedule-event
branch
from
d9aabbb to
20c8f9f
Compare
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 (1)
backend/apps/nest/controllers/calendar_events.py (1)
1-78: Enforce uniqueness on Event.google_calendar_id at the database level
Thegoogle_calendar_idfield inbackend/apps/owasp/models/event.py(currently declared withoutunique=True) has no DB constraint to prevent duplicates and the existing migration (0052) doesn’t add one. Addunique=Trueto the field (or aUniqueConstraintinMeta.constraints) and generate a new migration to enforce it.
🧹 Nitpick comments (3)
backend/apps/nest/controllers/calendar_events.py (3)
42-46: Validate minutes_before is non-negative.Prevents confusing “minutes before” semantics with negative values.
def set_reminder( @@ ) -> Reminder: """Set a reminder for a user.""" + if minutes_before < 0: + raise ValidationError("minutes_before must be >= 0.") auth = GoogleAccountAuthorization.authorize(slack_user_id)
24-26: Consider accepting ReminderSchedule.Recurrence members in addition to raw strings.If callers pass the enum member (e.g., Recurrence.DAILY), this check will fail. Normalize via
str(recurrence)when comparing.- if recurrence not in ReminderSchedule.Recurrence.values: + if str(recurrence) not in ReminderSchedule.Recurrence.values: message = "Invalid recurrence value." raise ValidationError(message)
66-71: Message formatting: avoid duplicating event name when message already includes it.Minor UX: if message already starts with event name, current code duplicates it.
- reminder = Reminder.objects.create( + base_message = message.strip() + final_message = ( + f"{event.name} - {base_message}" if base_message and not base_message.startswith(event.name) else (base_message or event.name) + ) + reminder = Reminder.objects.create( channel_id=channel, event=event, member=member, - message=f"{event.name} - {message}" if message else event.name, + message=final_message, )
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/apps/nest/controllers/calendar_events.py(1 hunks)backend/apps/nest/models/reminder.py(1 hunks)backend/apps/slack/common/handlers/calendar_events.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/apps/slack/common/handlers/calendar_events.py
- backend/apps/nest/models/reminder.py
🧰 Additional context used
🧬 Code graph analysis (1)
backend/apps/nest/controllers/calendar_events.py (6)
backend/apps/nest/clients/google_calendar.py (2)
GoogleCalendarClient(9-42)get_event(38-42)backend/apps/nest/models/google_account_authorization.py (2)
GoogleAccountAuthorization(28-225)authorize(55-93)backend/apps/nest/models/reminder.py (1)
Reminder(6-33)backend/apps/nest/models/reminder_schedule.py (2)
ReminderSchedule(6-36)Recurrence(14-18)backend/apps/slack/models/member.py (1)
Member(10-78)backend/apps/owasp/models/event.py (1)
parse_google_calendar_event(186-213)
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
backend/apps/slack/common/handlers/calendar_events.py (2)
17-20: Return a clickable Google sign-in button instead of a plain link.Improves UX and aligns with prior feedback.
- if not isinstance(auth, GoogleAccountAuthorization): - return [markdown(f"*Please sign in with Google first through this <{auth[0]}|link>*")] + if not isinstance(auth, GoogleAccountAuthorization): + authorization_url = auth[0] + return [ + markdown("*Please sign in to Google first.*"), + { + "type": "actions", + "elements": [ + { + "type": "button", + "text": {"type": "plain_text", "text": "Sign in with Google"}, + "url": authorization_url, + } + ], + }, + ]
39-44: Replace hardcoded “GMT” with the actual timezone.Localize event datetimes and include %Z.
- f"{NL}- Starts at: {event.start_date.strftime('%Y-%m-%d %H:%M')} GMT" - f"{NL}- Ends at: {event.end_date.strftime('%Y-%m-%d %H:%M')} GMT" + f"{NL}- Starts at: {timezone.localtime(event.start_date).strftime('%Y-%m-%d %H:%M %Z')}" + f"{NL}- Ends at: {timezone.localtime(event.end_date).strftime('%Y-%m-%d %H:%M %Z')}"
🧹 Nitpick comments (5)
backend/apps/nest/controllers/calendar_events.py (2)
27-31: Return the created schedule (useful for callers/tests).Returning the object improves debuggability and testability.
- ReminderSchedule.objects.create( + schedule = ReminderSchedule.objects.create( reminder=reminder, scheduled_time=scheduled_time, recurrence=recurrence, ) + return schedule
48-53: Consider scoping the cache key to the date/page to avoid collisions.
f"{slack_user_id}_{event_number}"can be overwritten when the user pages through days; include a cursor (e.g., date) in the key.If you want, I can propose a consistent key format across both writer (handlers.get_blocks) and reader (set_reminder), e.g.,
f"{slack_user_id}:{event_window}:{event_number}".backend/apps/slack/common/handlers/calendar_events.py (3)
28-33: Show correct timezone label in header dates.Don’t omit TZ; localize and include %Z.
- markdown( - f"*Your upcoming calendar events from {min_time.strftime('%Y-%m-%d %H:%M')}" - f" to {max_time.strftime('%Y-%m-%d %H:%M')}*" + markdown( + f"*Your upcoming calendar events from " + f"{timezone.localtime(min_time).strftime('%Y-%m-%d %H:%M %Z')}" + f" to {timezone.localtime(max_time).strftime('%Y-%m-%d %H:%M %Z')}*"
26-27: Handle the case where all events are filtered out by parsing.If parsing drops unconfirmed/invalid events, show a friendly message.
- parsed_events = Event.parse_google_calendar_events(events) + parsed_events = Event.parse_google_calendar_events(events) + if not parsed_events: + return [markdown("*No upcoming calendar events found.*")]
37-37: Cache key may collide across pages/days.Include a stable window identifier (e.g., the UTC date) in the key to avoid overwriting event 1 across pages.
I can align this with
set_reminderby adding a lightweight cursor (likemin_time.date().isoformat()) to both the cache key here and the lookup key there.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/apps/nest/controllers/calendar_events.py(1 hunks)backend/apps/nest/models/reminder.py(1 hunks)backend/apps/slack/common/handlers/calendar_events.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/nest/models/reminder.py
🧰 Additional context used
🧬 Code graph analysis (2)
backend/apps/nest/controllers/calendar_events.py (6)
backend/apps/nest/clients/google_calendar.py (2)
GoogleCalendarClient(9-42)get_event(38-42)backend/apps/nest/models/google_account_authorization.py (2)
GoogleAccountAuthorization(28-225)authorize(55-93)backend/apps/nest/models/reminder.py (1)
Reminder(6-33)backend/apps/nest/models/reminder_schedule.py (2)
ReminderSchedule(6-36)Recurrence(14-18)backend/apps/slack/models/member.py (1)
Member(10-78)backend/apps/owasp/models/event.py (1)
parse_google_calendar_event(186-213)
backend/apps/slack/common/handlers/calendar_events.py (5)
backend/apps/slack/blocks.py (2)
get_pagination_buttons(113-149)markdown(21-34)backend/apps/nest/clients/google_calendar.py (2)
GoogleCalendarClient(9-42)get_events(19-36)backend/apps/nest/models/google_account_authorization.py (2)
GoogleAccountAuthorization(28-225)authorize(55-93)backend/apps/owasp/models/event.py (2)
Event(25-369)parse_google_calendar_events(173-183)backend/apps/nest/controllers/calendar_events.py (1)
set_reminder(34-77)
🔇 Additional comments (1)
backend/apps/slack/common/handlers/calendar_events.py (1)
11-55: Overall structure and pagination flow look good.Inline imports minimize coupling; day-by-day pagination is simple and readable.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (3)
backend/apps/nest/controllers/calendar_events.py (1)
44-47: Improve authorization flow with clickable sign-in URL.When authorization fails, the function returns a generic error message. Based on the
GoogleAccountAuthorization.authorizemethod's return type, you could provide the OAuth URL to help users complete authentication.Consider enhancing the error handling to include the authorization URL when available:
auth = GoogleAccountAuthorization.authorize(slack_user_id) if not isinstance(auth, GoogleAccountAuthorization): - message = "User is not authorized with Google. Please sign in first." + if isinstance(auth, tuple): + authorization_url, _ = auth + message = f"User is not authorized with Google. Please sign in at: {authorization_url}" + else: + message = "User is not authorized with Google. Please sign in first." raise ValidationError(message)backend/apps/slack/common/handlers/calendar_events.py (2)
17-19: Enhance the sign-in experience with a proper button.Currently returns a hyperlink in markdown, but a proper Slack button would provide a better user experience.
Apply this diff to return a clickable button:
auth = GoogleAccountAuthorization.authorize(slack_user_id) if not isinstance(auth, GoogleAccountAuthorization): - return [markdown(f"*Please sign in with Google first through this <{auth[0]}|link>*")] + authorization_url, _state = auth + return [ + markdown("*Please sign in to Google first.*"), + { + "type": "actions", + "elements": [ + { + "type": "button", + "text": {"type": "plain_text", "text": "Sign in with Google"}, + "url": authorization_url, + } + ], + }, + ]
42-43: Fix incorrect timezone labeling.The code formats dates with hardcoded "GMT" label, which may be incorrect if the datetime objects are already localized to a different timezone.
Apply this diff to properly display timezone information:
- f"{NL}- Starts at: {event.start_date.strftime('%Y-%m-%d %H:%M')} GMT" - f"{NL}- Ends at: {event.end_date.strftime('%Y-%m-%d %H:%M')} GMT" + f"{NL}- Starts at: {timezone.localtime(event.start_date).strftime('%Y-%m-%d %H:%M %Z')}" + f"{NL}- Ends at: {timezone.localtime(event.end_date).strftime('%Y-%m-%d %H:%M %Z')}"
🧹 Nitpick comments (1)
backend/apps/slack/common/handlers/calendar_events.py (1)
76-76: Improve reminder confirmation message format.The current format
"{minutes}-minutes Reminder"is grammatically awkward. Consider a more natural phrasing.Apply this diff for better readability:
- markdown(f"*{args.minutes_before}-minutes Reminder set for event '{reminder.event.name}'*") + markdown(f"*Reminder set for '{reminder.event.name}' - {args.minutes_before} minutes before start*")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/apps/nest/controllers/calendar_events.py(1 hunks)backend/apps/nest/models/reminder.py(1 hunks)backend/apps/slack/common/handlers/calendar_events.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/nest/models/reminder.py
🧰 Additional context used
🧬 Code graph analysis (2)
backend/apps/nest/controllers/calendar_events.py (6)
backend/apps/nest/clients/google_calendar.py (2)
GoogleCalendarClient(9-42)get_event(38-42)backend/apps/nest/models/google_account_authorization.py (2)
GoogleAccountAuthorization(28-225)authorize(55-93)backend/apps/nest/models/reminder.py (1)
Reminder(6-33)backend/apps/nest/models/reminder_schedule.py (1)
Recurrence(14-18)backend/apps/slack/models/member.py (1)
Member(10-78)backend/apps/owasp/models/event.py (1)
parse_google_calendar_event(186-213)
backend/apps/slack/common/handlers/calendar_events.py (5)
backend/apps/slack/blocks.py (2)
get_pagination_buttons(113-149)markdown(21-34)backend/apps/nest/clients/google_calendar.py (2)
GoogleCalendarClient(9-42)get_events(19-36)backend/apps/nest/models/google_account_authorization.py (2)
GoogleAccountAuthorization(28-225)authorize(55-93)backend/apps/owasp/models/event.py (2)
Event(25-369)parse_google_calendar_events(173-183)backend/apps/nest/controllers/calendar_events.py (1)
set_reminder(34-78)
🔇 Additional comments (1)
backend/apps/slack/common/handlers/calendar_events.py (1)
36-37: Consider TTL implications for event number cache.The cache TTL of 3600 seconds (1 hour) means users have only 1 hour to set a reminder after viewing events. Consider if this is sufficient for the use case.
Consider increasing the cache timeout or implementing a more persistent storage mechanism if users need more time to set reminders:
- cache.set(f"{slack_user_id}_{event_number}", event.google_calendar_id, timeout=3600) + cache.set(f"{slack_user_id}_{event_number}", event.google_calendar_id, timeout=86400) # 24 hours
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
backend/apps/slack/utils.py (2)
15-16: Import missing types for robust parsing.Add
ArgumentTypeErrorand useshlex.splitto preserve quoted messages.-from argparse import ArgumentParser +from argparse import ArgumentParser, ArgumentTypeError +from shlex import split as shlex_split
220-263: Arg parsing breaks on quoted messages; numeric args allow invalid values.Use shell-like splitting and enforce positive integers.
def parse_slack_reminder_args(text: str): @@ - parser.add_argument( - "--event_number", type=int, help="The event number to set the reminder for.", required=True - ) + def positive_int(value: str) -> int: + try: + iv = int(value) + except ValueError as e: + raise ArgumentTypeError("must be an integer") from e + if iv <= 0: + raise ArgumentTypeError("must be a positive integer") + return iv + + parser.add_argument( + "--event_number", + type=positive_int, + help="The event number to set the reminder for.", + required=True, + ) @@ - "--minutes_before", - type=int, + "--minutes_before", + type=positive_int, @@ - return parser.parse_args(text.split()) + return parser.parse_args(shlex_split(text))backend/apps/owasp/models/event.py (2)
172-184: Parsing helper is fine but consider all-day events and type consistency.Support
start.date/end.dateand prefer the Status enum.- def parse_google_calendar_events(events: list[dict]) -> list[Event]: + def parse_google_calendar_events(events: list[dict]) -> list[Event]: @@ - return [event for event in (Event.parse_google_calendar_event(e) for e in events) if event] + return [e for e in (Event.parse_google_calendar_event(ev) for ev in events) if e]
185-213: Handle all-day events, use Status enum, and decide on DateTime vs Date.Current code assigns tz-aware datetimes to DateField and ignores all-day events. Pick one:
- Option A (recommended): migrate start_date/end_date to DateTimeField and keep datetimes.
- Option B: keep DateField and coerce to .date(), while parsing all-day events.
Minimal code changes if staying with DateField:
- start = event.get("start", {}).get("dateTime", {}) - end = event.get("end", {}).get("dateTime", {}) - if not start: + start_info = event.get("start") or {} + end_info = event.get("end") or {} + start_dt = start_info.get("dateTime") + start_d = start_info.get("date") + if not (start_dt or start_d): return None - - if event.get("status") != "confirmed": + if event.get("status") != Event.Status.CONFIRMED: return None - - return Event( + # Timed vs all-day + if start_d: + try: + from dateutil import parser as _p + start_date = _p.parse(start_d).date() + end_raw = end_info.get("date") + end_date = _p.parse(end_raw).date() if end_raw else None + except (ValueError, TypeError): + return None + else: + start_dt_parsed = parse_date_and_convert_to_local(start_dt) + end_dt_parsed = parse_date_and_convert_to_local(end_info.get("dateTime", "")) + if not start_dt_parsed: + return None + start_date = start_dt_parsed.date() + end_date = end_dt_parsed.date() if end_dt_parsed else None + + return Event( name=event.get("summary", ""), description=event.get("description", ""), url=event.get("htmlLink", ""), - start_date=parse_date_and_convert_to_local(start), - end_date=parse_date_and_convert_to_local(end), + start_date=start_date, + end_date=end_date, google_calendar_id=event.get("id", ""), category=Event.Category.COMMUNITY, suggested_location=event.get("location", ""), )If you choose Option A, I can draft the model/migration updates.
backend/apps/nest/controllers/calendar_events.py (2)
15-19: Use correct datetime type and enforce timezone awareness.Replace
timezone.datetimeannotation and guard against naive datetimes.-from django.utils import timezone +from django.utils import timezone +from datetime import datetime @@ -def schedule_reminder( - reminder: Reminder, - scheduled_time: timezone.datetime, - recurrence=ReminderSchedule.Recurrence.ONCE, -) -> ReminderSchedule: +def schedule_reminder( + reminder: Reminder, + scheduled_time: datetime, + recurrence=ReminderSchedule.Recurrence.ONCE, +) -> ReminderSchedule: @@ - if scheduled_time < timezone.now(): + if timezone.is_naive(scheduled_time): + raise ValidationError("scheduled_time must be timezone-aware.") + if scheduled_time < timezone.now(): message = "Scheduled time must be in the future." raise ValidationError(message)
42-77: Harden reminder creation: validate inputs, handle API/DB errors, and ensure atomicity.Covers negative minutes, Google API failures, member lookup, and transactional integrity.
@@ - auth = GoogleAccountAuthorization.authorize(slack_user_id) + auth = GoogleAccountAuthorization.authorize(slack_user_id) if not isinstance(auth, GoogleAccountAuthorization): message = "User is not authorized with Google. Please sign in first." raise ValidationError(message) client = GoogleCalendarClient(auth) @@ - event = Event.parse_google_calendar_event(client.get_event(google_calendar_id)) + from googleapiclient.errors import HttpError + try: + raw = client.get_event(google_calendar_id) + except HttpError as e: + raise ValidationError("Could not retrieve the event details. Please try again later.") from e + event = Event.parse_google_calendar_event(raw) if not event: message = "Could not retrieve the event details. Please try again later." raise ValidationError(message) - reminder_time = event.start_date - timezone.timedelta(minutes=minutes_before) + if minutes_before < 0: + raise ValidationError("Minutes before must be zero or a positive integer.") + reminder_time = event.start_date - timezone.timedelta(minutes=minutes_before) if reminder_time < timezone.now(): message = "Reminder time must be in the future. Please adjust the minutes before." raise ValidationError(message) - if recurrence and recurrence not in ReminderSchedule.Recurrence.values: + if recurrence: + recurrence = recurrence.lower() + if recurrence and recurrence not in ReminderSchedule.Recurrence.values: message = "Invalid recurrence value." raise ValidationError(message) - event.save() - member = Member.objects.get(slack_user_id=slack_user_id) - reminder = Reminder.objects.create( - channel_id=channel, - event=event, - member=member, - message=f"{event.name} - {message}" if message else event.name, - ) - return schedule_reminder( - reminder=reminder, - scheduled_time=reminder_time, - recurrence=recurrence or ReminderSchedule.Recurrence.ONCE, - ) + from django.db import transaction + try: + member = Member.objects.get(slack_user_id=slack_user_id) + except Member.DoesNotExist as e: + raise ValidationError(f"Member with Slack ID {slack_user_id} does not exist.") from e + + # Persist or update the Event before referencing it from Reminder. + event, _ = Event.objects.update_or_create( + google_calendar_id=event.google_calendar_id, + defaults={ + "name": event.name, + "description": event.description, + "url": event.url, + "start_date": event.start_date, + "end_date": event.end_date, + "category": event.category, + "suggested_location": event.suggested_location, + }, + ) + + with transaction.atomic(): + reminder = Reminder.objects.create( + channel_id=channel, + event=event, + member=member, + message=f"{event.name} - {message}" if message else event.name, + ) + return schedule_reminder( + reminder=reminder, + scheduled_time=reminder_time, + recurrence=recurrence or ReminderSchedule.Recurrence.ONCE, + )backend/apps/slack/common/handlers/calendar_events.py (1)
73-76: Use proper message extraction from ValidationError.
.messageis not reliable; use.messagesorstr(e).- except ValidationError as e: - return [markdown(f"*{e.message}*")] + except ValidationError as e: + msg = e.messages[0] if hasattr(e, "messages") else str(e) + return [markdown(f"*{msg}*")]
🧹 Nitpick comments (4)
backend/apps/slack/MANIFEST.yaml (1)
64-68: Align usage hint with flags and include recurrence.Reflect the actual parser contract and advise quoting for messages.
- usage_hint: --channel <#channel_name> --event_number <event_number> --minutes_before <minutes_before_event> --message <optional_message> + usage_hint: --channel <#channel> --event_number <N> --minutes_before <M> [--message "optional text"] [--recurrence once|daily|weekly|monthly]backend/apps/slack/common/handlers/calendar_events.py (3)
19-23: Return a clickable sign-in button when auth is missing.Better UX than a raw link and uses the URL from
authorize(...).- auth = GoogleAccountAuthorization.authorize(slack_user_id) - if not isinstance(auth, GoogleAccountAuthorization): - return [markdown(f"*Please sign in with Google first through this <{auth[0]}|link>*")] + auth = GoogleAccountAuthorization.authorize(slack_user_id) + if not isinstance(auth, GoogleAccountAuthorization): + authorization_url, _state = auth + return [ + markdown("*Please sign in with Google first.*"), + { + "type": "actions", + "elements": [ + { + "type": "button", + "text": {"type": "plain_text", "text": "Sign in with Google"}, + "url": authorization_url, + } + ], + }, + ]
31-35: Update instructions to match the flag-based CLI.The hint still shows positional args; switch to flags.
- f"{NL}Set a reminder for an event with `/set-reminder [channel]" - " [event number] [minutes before] [optional message] [recurrence]`" + f"{NL}Set a reminder with:" + " `/set-reminder --channel <#channel> --event_number <N> --minutes_before <M>" + ' [--message "text"] [--recurrence once|daily|weekly|monthly]`'
44-45: Fix timezone label and handle missing end time.Avoid hardcoded GMT and guard
end_dateNone.- f"{NL}- Starts at: {event.start_date.strftime('%Y-%m-%d %H:%M')} GMT" - f"{NL}- Ends at: {event.end_date.strftime('%Y-%m-%d %H:%M')} GMT" + f"{NL}- Starts at: {timezone.localtime(event.start_date).strftime('%Y-%m-%d %H:%M %Z')}" + f"{NL}- Ends at: " + f"{timezone.localtime(event.end_date).strftime('%Y-%m-%d %H:%M %Z') if event.end_date else 'N/A'}"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/apps/nest/controllers/calendar_events.py(1 hunks)backend/apps/owasp/migrations/0053_alter_event_key.py(1 hunks)backend/apps/owasp/models/event.py(4 hunks)backend/apps/slack/MANIFEST.yaml(1 hunks)backend/apps/slack/commands/__init__.py(1 hunks)backend/apps/slack/common/handlers/calendar_events.py(1 hunks)backend/apps/slack/utils.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
backend/apps/slack/commands/__init__.py (1)
backend/apps/nest/controllers/calendar_events.py (1)
set_reminder(34-77)
backend/apps/slack/utils.py (1)
backend/apps/slack/models/message.py (1)
text(83-85)
backend/apps/nest/controllers/calendar_events.py (6)
backend/apps/nest/clients/google_calendar.py (2)
GoogleCalendarClient(9-42)get_event(38-42)backend/apps/nest/models/google_account_authorization.py (2)
GoogleAccountAuthorization(28-225)authorize(55-93)backend/apps/nest/models/reminder.py (1)
Reminder(6-33)backend/apps/nest/models/reminder_schedule.py (2)
ReminderSchedule(6-36)Recurrence(14-18)backend/apps/owasp/models/event.py (3)
Event(25-370)parse_google_calendar_event(186-213)save(355-370)backend/apps/slack/models/member.py (1)
Member(10-78)
backend/apps/slack/common/handlers/calendar_events.py (5)
backend/apps/slack/blocks.py (2)
get_pagination_buttons(113-149)markdown(21-34)backend/apps/nest/clients/google_calendar.py (2)
GoogleCalendarClient(9-42)get_events(19-36)backend/apps/nest/models/google_account_authorization.py (2)
GoogleAccountAuthorization(28-225)authorize(55-93)backend/apps/owasp/models/event.py (2)
Event(25-370)parse_google_calendar_events(173-183)backend/apps/nest/controllers/calendar_events.py (1)
set_reminder(34-77)
backend/apps/owasp/models/event.py (2)
backend/apps/common/utils.py (1)
parse_date_and_convert_to_local(37-47)backend/apps/owasp/models/chapter.py (2)
generate_suggested_location(128-149)generate_geo_location(116-126)
🪛 Ruff (0.12.2)
backend/apps/owasp/models/event.py
63-63: Avoid using null=True on string-based fields such as CharField
(DJ001)
🪛 GitHub Actions: Run CI/CD
backend/apps/owasp/models/event.py
[error] 63-63: DJ001: Avoid using null=True on string-based fields such as CharField.
🔇 Additional comments (2)
backend/apps/slack/commands/__init__.py (1)
21-21: Good addition: exports the new command.Including
set_reminderensures auto-registration withCommandBase.configure_commands(). No further issues.backend/apps/owasp/models/event.py (1)
363-369: Save hook logic looks good.Skipping geo/suggested location for Community reduces unnecessary API calls.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/apps/owasp/models/event.py (1)
249-273: Normalize URLs and dates—prepare for DateTimeField migration
normalize_url(...) or ""is fine. Oncestart_date/end_dateare converted toDateTimeFields, update all callers (notablybackend/tests/apps/slack/commands/events_test.py) that assumedateobjects or invoke.date().
♻️ Duplicate comments (1)
backend/apps/owasp/models/event.py (1)
201-202: Avoid magic string for status; use the enum.
UseEvent.Status.CONFIRMEDinstead of"confirmed". Already flagged earlier.
🧹 Nitpick comments (2)
backend/apps/nest/models/reminder_schedule.py (1)
10-15: DB-level uniqueness looks good; add time-based index and confirm 'monthly' support.
- For schedulers that scan due items by time, add an index on scheduled_time to avoid table scans.
- Recurrence includes "monthly" while the issue scope lists once/daily/weekly—confirm downstream handling or drop for now.
Minimal change for the index (adds a migration):
- scheduled_time = models.DateTimeField(verbose_name="Scheduled Time") + scheduled_time = models.DateTimeField(verbose_name="Scheduled Time", db_index=True)Optional: keep lists ordered chronologically by default:
# inside class Meta ordering = ["scheduled_time"]backend/apps/owasp/migrations/0053_alter_event_end_date_alter_event_start_date.py (1)
11-22: Follow-up migrations required
- Add a conditional unique constraint on
google_calendar_id(only when non-empty) to prevent duplicate imports;keyis already declaredunique=True.- Legacy migration 0016 used
DateFieldforstart_date/end_date; verify rows created pre-0053 (with default midnight timestamps) behave correctly in downstream logic.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/apps/nest/migrations/0010_reminderschedule_unique_reminder_schedule.py(1 hunks)backend/apps/nest/models/reminder_schedule.py(1 hunks)backend/apps/owasp/migrations/0053_alter_event_end_date_alter_event_start_date.py(1 hunks)backend/apps/owasp/models/event.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/apps/owasp/models/event.py (2)
backend/apps/common/utils.py (1)
parse_date_and_convert_to_local(37-47)backend/apps/owasp/models/chapter.py (2)
generate_suggested_location(128-149)generate_geo_location(116-126)
🔇 Additional comments (3)
backend/apps/owasp/models/event.py (3)
21-21: Import changes look good.
Bringing inparse_date_and_convert_to_localandslugifyaligns with the new parsing paths.
172-184: Aggregator is fine.
The generator + filter is concise and correct.
367-373: Good call to skip AI/geocoding for COMMUNITY events.
Prevents unnecessary external calls on imported calendar items.
backend/apps/nest/migrations/0010_reminderschedule_unique_reminder_schedule.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/settings/base.py (1)
157-166: Composing the Redis URL with secrets at class definition is unsafe and likely broken.
- The f-string embeds
values.SecretValueobjects before django-configurations resolves them, yielding an invalid URL at runtime. Values should be used as settings attributes, not interpolated into other attributes at class creation. (django-configurations.readthedocs.io)- Avoid placing the password in the URL; django-redis supports passing
OPTIONS["PASSWORD"], which also sidesteps URL-encoding pitfalls for special characters. (pypi.org)Apply this diff, then finalize values in
post_setup(below):- "LOCATION": f"redis://:{REDIS_PASSWORD}@{REDIS_HOST}:6379", + # Set to a neutral placeholder; we’ll assign the real host/password in post_setup(). + "LOCATION": "redis://localhost:6379/0", "OPTIONS": { - "CLIENT_CLASS": "django_redis.client.DefaultClient", + "CLIENT_CLASS": "django_redis.client.DefaultClient", + "PASSWORD": "", # populated in post_setup() },Add this method to compute settings after env values are resolved:
# Place inside Base(Configuration) @classmethod def post_setup(cls): super().post_setup() # Compose cache settings from resolved env without leaking secrets in URLs cls.CACHES["default"]["LOCATION"] = f"redis://{cls.REDIS_HOST}:6379/0" cls.CACHES["default"]["OPTIONS"]["PASSWORD"] = cls.REDIS_PASSWORD # Keep RQ in sync with cache (if you didn’t adopt USE_REDIS_CACHE in RQ_QUEUES) # cls.RQ_QUEUES = { # "default": { # "HOST": cls.REDIS_HOST, # "PORT": 6379, # "DB": 0, # "PASSWORD": cls.REDIS_PASSWORD, # "DEFAULT_TIMEOUT": 360, # } # }backend/apps/owasp/models/event.py (1)
90-101: exclude() logic is wrong (ANDed); split into two excludes.Currently excludes only COMMUNITY events that also have empty name/url. You likely intended to exclude any empty name/url and also exclude COMMUNITY.
- return ( - Event.objects.filter( - start_date__gt=timezone.now(), - ) - .exclude( - Q(name__exact="") | Q(url__exact=""), - category=Event.Category.COMMUNITY, - ) - .order_by( - "start_date", - ) - ) + return ( + Event.objects.filter(start_date__gt=timezone.now()) + .exclude(Q(name__exact="") | Q(url__exact="")) + .exclude(category=Event.Category.COMMUNITY) + .order_by("start_date") + )
♻️ Duplicate comments (13)
backend/apps/slack/commands/set_reminder.py (1)
17-24: Return a clear usage block and avoid KeyError when 'text' is missing.Guard access to command["text"] and show actionable usage on parse failure.
- def render_blocks(self, command): + def render_blocks(self, command): """Render the blocks for the command.""" - try: - args = parse_reminder_args(command["text"]) + try: + text = (command.get("text") or "").strip() + args = parse_reminder_args(text) except SystemExit: - return [markdown("*Invalid command format. Please check your input and try again.*")] + usage = ( + ":warning: Invalid command format.\n" + "*Usage:*\n" + "`/set-reminder --channel <#channel-or-id> --event_number <n> " + "[--minutes_before <m>] [--message \"text\"] " + "[--recurrence once|daily|weekly|monthly]`" + "\n*Example:* `/set-reminder --channel #eng --event_number 2 " + "--minutes_before 15 --message \"Bring slides\" --recurrence weekly`" + ) + return [markdown(usage)] else: return get_reminder_blocks(args, self.get_user_id(command))backend/apps/nest/controllers/calendar_events.py (7)
66-69: Normalize recurrence input to match TextChoices values.Lower-case user input before validating.
- if recurrence and recurrence not in ReminderSchedule.Recurrence.values: + if recurrence: + recurrence = recurrence.lower() + if recurrence and recurrence not in ReminderSchedule.Recurrence.values: message = "Invalid recurrence value." raise ValidationError(message)
3-12: Add missing imports and a module logger; use correct exception classes.Needed for robust API error handling, atomic DB writes, and proper type annotations.
from django.core.cache import cache from django.core.exceptions import ValidationError from django.utils import timezone +import logging +from datetime import datetime +from django.db import transaction, IntegrityError +from googleapiclient.errors import HttpError + +logger = logging.getLogger(__name__)
15-19: Fix type annotation for scheduled_time.Use datetime, not timezone.datetime.
def schedule_reminder( reminder: Reminder, - scheduled_time: timezone.datetime, + scheduled_time: datetime, recurrence=ReminderSchedule.Recurrence.ONCE, ) -> ReminderSchedule:
21-23: Guard against naive datetimes before comparing to timezone.now().Comparing naive to aware datetimes raises TypeError under USE_TZ.
- if scheduled_time < timezone.now(): + if timezone.is_naive(scheduled_time): + raise ValidationError("scheduled_time must be timezone-aware.") + if scheduled_time < timezone.now(): message = "Scheduled time must be in the future." raise ValidationError(message)
55-60: Handle Google API failures and avoid assigning unsaved/invalid events.Wrap get_event() with HttpError handling and parse into a separate variable.
- client = GoogleCalendarClient(auth) - event = Event.parse_google_calendar_event(client.get_event(google_calendar_id)) - if not event: + client = GoogleCalendarClient(auth) + try: + gc_event = client.get_event(google_calendar_id) + except HttpError as e: + logger.exception("Google Calendar get_event failed for id=%s", google_calendar_id) + raise ValidationError("Could not retrieve the event details. Please try again later.") from e + + parsed_event = Event.parse_google_calendar_event(gc_event) + if not parsed_event: message = "Could not retrieve the event details. Please try again later." raise ValidationError(message)
61-65: Validate minutes_before; disallow negatives and non-integers.Prevents surprising future shifts when users pass negative values.
- reminder_time = event.start_date - timezone.timedelta(minutes=minutes_before) + try: + minutes_before = int(minutes_before) + except (TypeError, ValueError): + raise ValidationError("minutes_before must be an integer.") + if minutes_before < 0: + raise ValidationError("minutes_before must be zero or a positive integer.") + reminder_time = parsed_event.start_date - timezone.timedelta(minutes=minutes_before)
70-84: Persist/update Event, handle missing Member, and make DB writes atomic.Avoid partial writes and ensure FK targets exist/are updated.
- # Saving event to the database after validation - event.save() - - member = Member.objects.get(slack_user_id=user_id) - reminder = Reminder.objects.create( - channel_id=channel, - event=event, - member=member, - message=f"{event.name} - {message}" if message else event.name, - ) - return schedule_reminder( - reminder=reminder, - scheduled_time=reminder_time, - recurrence=recurrence or ReminderSchedule.Recurrence.ONCE, - ) + with transaction.atomic(): + # Upsert Event to keep details fresh. + event, _ = Event.objects.update_or_create( + google_calendar_id=parsed_event.google_calendar_id, + defaults={ + "name": parsed_event.name, + "description": parsed_event.description, + "url": parsed_event.url, + "start_date": parsed_event.start_date, + "end_date": parsed_event.end_date, + "category": parsed_event.category, + "suggested_location": parsed_event.suggested_location, + }, + ) + try: + member = Member.objects.get(slack_user_id=user_id) + except Member.DoesNotExist as e: + raise ValidationError(f"Member with Slack ID {user_id} does not exist.") from e + + reminder = Reminder.objects.create( + channel_id=channel, + event=event, + member=member, + message=f"{event.name} - {message}" if message else event.name, + ) + return schedule_reminder( + reminder=reminder, + scheduled_time=reminder_time, + recurrence=recurrence or ReminderSchedule.Recurrence.ONCE, + )backend/apps/slack/common/handlers/calendar_events.py (5)
19-22: Return a clickable Google sign‑in button when not authorized.Better UX than a plain link.
- auth = GoogleAccountAuthorization.authorize(slack_user_id) - if not isinstance(auth, GoogleAccountAuthorization): - return [markdown(f"*Please sign in with Google first through this <{auth[0]}|link>*")] - client = GoogleCalendarClient(auth) + auth_result = GoogleAccountAuthorization.authorize(slack_user_id) + if not isinstance(auth_result, GoogleAccountAuthorization): + authorization_url, _ = auth_result + return [ + markdown("*Please sign in to Google first.*"), + { + "type": "actions", + "elements": [ + { + "type": "button", + "text": {"type": "plain_text", "text": "Sign in with Google"}, + "url": authorization_url, + } + ], + }, + ] + auth = auth_result + client = GoogleCalendarClient(auth)
31-36: Format header times with timezone.localtime and include zone for both bounds.Ensures correct local zone display.
- f"*Your upcoming calendar events from {min_time.strftime('%Y-%m-%d %H:%M')}" - f" to {max_time.strftime('%Y-%m-%d %H:%M %Z')}*" + f"*Your upcoming calendar events from {timezone.localtime(min_time).strftime('%Y-%m-%d %H:%M %Z')}" + f" to {timezone.localtime(max_time).strftime('%Y-%m-%d %H:%M %Z')}*"
45-46: Localize event times before formatting.Safer if datetimes are aware but not in the desired local tz.
- f"{NL}- Starts at: {event.start_date.strftime('%Y-%m-%d %H:%M %Z')}" - f"{NL}- Ends at: {event.end_date.strftime('%Y-%m-%d %H:%M %Z')}" + f"{NL}- Starts at: {timezone.localtime(event.start_date).strftime('%Y-%m-%d %H:%M %Z')}" + f"{NL}- Ends at: {timezone.localtime(event.end_date).strftime('%Y-%m-%d %H:%M %Z')}"
84-84: Localize scheduled_time in success message.- f"{reminder_schedule.scheduled_time.strftime('%Y-%m-%d %H:%M %Z')}" + f"{timezone.localtime(reminder_schedule.scheduled_time).strftime('%Y-%m-%d %H:%M %Z')}"
74-77: Fix ValidationError message extraction.Use messages[0] or str(e); .message is not reliable.
- except ValidationError as e: - return [markdown(f"*{e.message}*")] + except ValidationError as e: + msg = e.messages[0] if hasattr(e, "messages") else str(e) + return [markdown(f"*{msg}*")]
🧹 Nitpick comments (4)
backend/pyproject.toml (1)
53-53: If you’ll use scheduled jobs, plan the worker processes.To actually execute jobs and schedules you’ll need to run
manage.py rqworker --with-scheduler(orrqscheduler) in your deployment. Consider adding worker Procfiles/manifests and health checks. (github.com)backend/settings/base.py (2)
64-65: App registration is correct.Adding
django_rqtoTHIRD_PARTY_APPSis required. If you want the built-in dashboard, also includedjango_rq.urlsinurls.pyunder a protected path. (github.com)
111-122: Optional: prefer a single REDIS_URL over splitting host/password.Using
CacheURLValue(or aREDIS_URLstring) simplifies setup and makes local/dev overrides trivial. If you go this route, also switch RQ toURLorUSE_REDIS_CACHE. (django-configurations.readthedocs.io, github.com)Also applies to: 157-166
backend/apps/owasp/models/event.py (1)
28-35: Add index/uniqueness on google_calendar_id (optional, improves lookups and integrity).Lookups use google_calendar_id; add an index and a conditional UniqueConstraint when non-empty.
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"), ] verbose_name_plural = "Events" + constraints = [ + models.UniqueConstraint( + fields=("google_calendar_id",), + condition=~Q(google_calendar_id=""), + name="uniq_event_gcal_id_when_non_empty", + ) + ]
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
backend/poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
backend/apps/nest/controllers/calendar_events.py(1 hunks)backend/apps/nest/migrations/0011_merge_20250908_0548.py(1 hunks)backend/apps/nest/utils/calendar_events.py(1 hunks)backend/apps/owasp/migrations/0054_merge_20250908_0549.py(1 hunks)backend/apps/owasp/models/event.py(4 hunks)backend/apps/slack/commands/set_reminder.py(1 hunks)backend/apps/slack/common/handlers/calendar_events.py(1 hunks)backend/apps/slack/utils.py(1 hunks)backend/pyproject.toml(1 hunks)backend/settings/base.py(2 hunks)backend/tests/apps/owasp/models/event_test.py(2 hunks)
✅ Files skipped from review due to trivial changes (3)
- backend/apps/owasp/migrations/0054_merge_20250908_0549.py
- backend/apps/nest/migrations/0011_merge_20250908_0548.py
- backend/apps/slack/utils.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/tests/apps/owasp/models/event_test.py
🧰 Additional context used
🧬 Code graph analysis (5)
backend/apps/nest/utils/calendar_events.py (1)
backend/apps/slack/models/message.py (1)
text(83-85)
backend/apps/nest/controllers/calendar_events.py (6)
backend/apps/nest/clients/google_calendar.py (2)
GoogleCalendarClient(9-42)get_event(38-42)backend/apps/nest/models/google_account_authorization.py (2)
GoogleAccountAuthorization(28-225)authorize(55-93)backend/apps/nest/models/reminder.py (1)
Reminder(6-33)backend/apps/nest/models/reminder_schedule.py (2)
ReminderSchedule(6-42)Recurrence(20-24)backend/apps/owasp/models/event.py (3)
Event(25-375)parse_google_calendar_event(186-218)save(360-375)backend/apps/slack/models/member.py (1)
Member(10-78)
backend/apps/slack/common/handlers/calendar_events.py (5)
backend/apps/slack/blocks.py (2)
get_pagination_buttons(113-149)markdown(21-34)backend/apps/nest/clients/google_calendar.py (2)
GoogleCalendarClient(9-42)get_events(19-36)backend/apps/nest/models/google_account_authorization.py (2)
GoogleAccountAuthorization(28-225)authorize(55-93)backend/apps/owasp/models/event.py (2)
Event(25-375)parse_google_calendar_events(173-183)backend/apps/nest/controllers/calendar_events.py (1)
set_reminder(34-84)
backend/apps/slack/commands/set_reminder.py (4)
backend/apps/nest/utils/calendar_events.py (1)
parse_reminder_args(6-45)backend/apps/slack/blocks.py (1)
markdown(21-34)backend/apps/slack/commands/command.py (1)
CommandBase(20-157)backend/apps/slack/common/handlers/calendar_events.py (1)
get_reminder_blocks(61-88)
backend/apps/owasp/models/event.py (2)
backend/apps/common/utils.py (1)
parse_date_and_convert_to_local(37-47)backend/apps/owasp/models/chapter.py (2)
generate_suggested_location(128-149)generate_geo_location(116-126)
⏰ 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 backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
🔇 Additional comments (4)
backend/pyproject.toml (1)
53-53: Dependency addition looks compatible with our stack.
django-rq = "^3.1"supports Django 3.2+ and was released on Aug 2, 2025 with wheels built on CPython 3.13.2, so it should work with Django 5.1 and Python 3.13. (pypi.org)backend/apps/nest/utils/calendar_events.py (1)
37-43: Monthly recurrence is fully supported. TheReminderSchedule.Recurrenceenum, migrations and controller validation include"monthly", so exposing it in the CLI is safe.backend/apps/slack/commands/set_reminder.py (1)
12-16: Overriding command_name is appropriate here.The base would yield "/setreminder"; keeping "/set-reminder" matches Slack manifest and UX.
backend/apps/owasp/models/event.py (1)
368-374: LGTM: Skip AI/geocoding for COMMUNITY events.This avoids unnecessary external calls for imported calendar events.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (6)
backend/apps/nest/controllers/calendar_events.py (6)
16-33: Cross-check: multiple earlier bot suggestions still unaddressedPast suggestions about timezone-aware datetimes, transactions, error handling, and idempotency remain. The diffs above consolidate those changes.
Also applies to: 35-90
25-32: Make scheduling idempotent to honor the unique constraintApply:
- return ReminderSchedule.objects.create( - reminder=reminder, - scheduled_time=scheduled_time, - recurrence=recurrence, - ) + schedule, created = ReminderSchedule.objects.get_or_create( + reminder=reminder, + scheduled_time=scheduled_time, + defaults={"recurrence": recurrence}, + ) + if not created and schedule.recurrence != recurrence: + schedule.recurrence = recurrence + schedule.save(update_fields=["recurrence"]) + return schedule
62-66: Validate minutes_before and guard against past remindersApply:
- reminder_time = event.start_date - timezone.timedelta(minutes=minutes_before) + if not isinstance(minutes_before, int) or minutes_before < 0: + raise ValidationError("minutes_before must be a non-negative integer.") + reminder_time = event.start_date - timezone.timedelta(minutes=minutes_before) if reminder_time < timezone.now(): message = "Reminder time must be in the future. Please adjust the minutes before." raise ValidationError(message)
74-74: Handle missing Slack member gracefullyApply:
- member = Member.objects.get(slack_user_id=user_id) + try: + member = Member.objects.get(slack_user_id=user_id) + except Member.DoesNotExist as e: + raise ValidationError(f"Member with Slack ID {user_id} does not exist.") from e
56-60: Handle Google API errors explicitly and log themApply:
- client = GoogleCalendarClient(auth) - event = Event.parse_google_calendar_event(client.get_event(google_calendar_id)) + client = GoogleCalendarClient(auth) + try: + raw_event = client.get_event(google_calendar_id) + except HttpError as e: + logger.exception("Google Calendar get_event failed for id=%s", google_calendar_id) + raise ValidationError("Could not retrieve the event details. Please try again later.") from e + event = Event.parse_google_calendar_event(raw_event)
16-20: Use stdlib datetime in annotations and enforce timezone-awarenessApply:
def schedule_reminder( reminder: Reminder, - scheduled_time: timezone.datetime, + scheduled_time: datetime, recurrence=ReminderSchedule.Recurrence.ONCE, ) -> ReminderSchedule: """Schedule a reminder.""" - if scheduled_time < timezone.now(): + if timezone.is_naive(scheduled_time): + raise ValidationError("scheduled_time must be timezone-aware.") + if scheduled_time < timezone.now(): message = "Scheduled time must be in the future." raise ValidationError(message)
🧹 Nitpick comments (3)
backend/apps/nest/controllers/calendar_events.py (3)
3-6: Add missing imports for typing, transactions, logging, and Google API errorsApply:
from django.core.cache import cache from django.core.exceptions import ValidationError from django.utils import timezone +import logging +from datetime import datetime +from django.db import transaction +from googleapiclient.errors import HttpError + +logger = logging.getLogger(__name__)
67-69: Normalize recurrence input to be case-insensitiveApply:
- if recurrence and recurrence not in ReminderSchedule.Recurrence.values: + if recurrence: + recurrence = recurrence.lower() + if recurrence and recurrence not in ReminderSchedule.Recurrence.values: message = "Invalid recurrence value." raise ValidationError(message)
88-88: Use Slack user ID mentions, not @usernameSlack mentions should be
<@USERID>to reliably notify the user.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
backend/poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
backend/apps/nest/controllers/calendar_events.py(1 hunks)backend/apps/nest/models/reminder_schedule.py(2 hunks)backend/apps/nest/schedulers/calendar_events/base.py(1 hunks)backend/apps/nest/schedulers/calendar_events/slack.py(1 hunks)backend/pyproject.toml(1 hunks)backend/settings/base.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/pyproject.toml
- backend/apps/nest/models/reminder_schedule.py
- backend/settings/base.py
🧰 Additional context used
🧬 Code graph analysis (3)
backend/apps/nest/schedulers/calendar_events/slack.py (1)
backend/apps/nest/schedulers/calendar_events/base.py (2)
BaseScheduler(8-41)send_message(34-37)
backend/apps/nest/schedulers/calendar_events/base.py (2)
backend/apps/nest/models/reminder_schedule.py (3)
ReminderSchedule(6-57)Recurrence(20-24)cron_expression(41-53)backend/apps/nest/schedulers/calendar_events/slack.py (1)
send_message(9-11)
backend/apps/nest/controllers/calendar_events.py (8)
backend/apps/nest/clients/google_calendar.py (2)
GoogleCalendarClient(9-42)get_event(38-42)backend/apps/nest/models/google_account_authorization.py (2)
GoogleAccountAuthorization(28-225)authorize(55-93)backend/apps/nest/models/reminder.py (1)
Reminder(6-33)backend/apps/nest/models/reminder_schedule.py (2)
ReminderSchedule(6-57)Recurrence(20-24)backend/apps/nest/schedulers/calendar_events/slack.py (1)
SlackScheduler(6-11)backend/apps/owasp/models/event.py (3)
Event(25-375)parse_google_calendar_event(186-218)save(360-375)backend/apps/slack/models/member.py (1)
Member(10-78)backend/apps/nest/schedulers/calendar_events/base.py (2)
schedule(17-32)send_notification_message(39-41)
⏰ 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 CI Denendencies Scan
- GitHub Check: Run Code Scan
🔇 Additional comments (1)
backend/apps/nest/controllers/calendar_events.py (1)
1-90: Verification complete – no unsafe bound-method enqueues detected and the onlytimezone.datetimeannotation is in schedule_reminder (backend/apps/nest/controllers/calendar_events.py:18). No changes required.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
backend/apps/nest/controllers/calendar_events.py (2)
73-73: Handle missing Member as a validation error.Uncaught DoesNotExist will bubble up as a 500 in Slack handler flow.
- member = Member.objects.get(slack_user_id=user_id) + try: + member = Member.objects.get(slack_user_id=user_id) + except Member.DoesNotExist as e: + raise ValidationError(f"Member with Slack ID {user_id} does not exist.") from e
15-23: Fix type annotation and guard against naive datetimes.timezone.datetime isn’t a valid type; comparing naive vs aware datetimes can raise TypeError.
-from django.utils import timezone +from django.utils import timezone +from datetime import datetime @@ -def schedule_reminder( +def schedule_reminder( reminder: Reminder, - scheduled_time: timezone.datetime, + scheduled_time: datetime, recurrence=ReminderSchedule.Recurrence.ONCE, ) -> ReminderSchedule: """Schedule a reminder.""" + if timezone.is_naive(scheduled_time): + raise ValidationError("scheduled_time must be timezone-aware.") if scheduled_time < timezone.now(): message = "Scheduled time must be in the future." raise ValidationError(message)
🧹 Nitpick comments (10)
backend/settings/urls.py (1)
28-28: Restrict django-rq UI under admin and gate via feature flag
backend/settings/urls.py: replace- path("django-rq/", include("django_rq.urls")), + path("a/rq/", include("django_rq.urls")),Optionally wrap in a feature flag (defaults to DEBUG):
if getattr(settings, "ENABLE_DJANGO_RQ_UI", settings.DEBUG): urlpatterns += [path("a/rq/", include("django_rq.urls"))]backend/apps/nest/schedulers/calendar_events/base.py (1)
11-14: Allow scheduler name injection for flexibility and tests.Enables directing jobs to non-default queues in different environments.
-def __init__(self, reminder_schedule: ReminderSchedule): +def __init__(self, reminder_schedule: ReminderSchedule, scheduler_name: str = "default"): """Initialize the BaseScheduler with a ReminderSchedule instance.""" self.reminder_schedule = reminder_schedule - self.scheduler = get_scheduler("default") + self.scheduler = get_scheduler(scheduler_name)backend/apps/nest/schedulers/calendar_events/slack.py (2)
3-5: Import timezone to ensure UTC scheduling for one-off jobs.You’ll need timezone to normalize scheduled_time below.
from apps.nest.models.reminder_schedule import ReminderSchedule from apps.nest.schedulers.calendar_events.base import BaseScheduler from apps.nest.schedulers.tasks.slack import send_message +from django.utils import timezone
22-31: Prefer kwargs and stable job id for cron; reduces arg-order coupling and prevents duplicates.Passing kwargs is clearer, and a stable id avoids duplicate cron registrations across deploys.
- self.scheduler.cron( - self.reminder_schedule.cron_expression, - func=send_message, - args=( - self.reminder_schedule.reminder.message, - self.reminder_schedule.reminder.channel_id, - ), - queue_name="default", - use_local_timezone=True, - ) + self.scheduler.cron( + self.reminder_schedule.cron_expression, + func=send_message, + kwargs={ + "message": self.reminder_schedule.reminder.message, + "channel_id": self.reminder_schedule.reminder.channel_id, + }, + queue_name="default", + use_local_timezone=True, + # Optional: if supported by your rq-scheduler version + # id=f"reminder-cron:{self.reminder_schedule.id}", + )backend/apps/nest/controllers/calendar_events.py (4)
66-68: Normalize recurrence input before validating.Lowercasing prevents user input mismatches.
- if recurrence and recurrence not in ReminderSchedule.Recurrence.values: + if recurrence: + recurrence = recurrence.lower() + if recurrence and recurrence not in ReminderSchedule.Recurrence.values: message = "Invalid recurrence value." raise ValidationError(message)
70-72: Persist event idempotently to avoid duplicates on concurrent requests.Using save() on a transient instance can race; prefer update_or_create.
- # Saving event to the database after validation - event.save() + # Persist or update the Event before referencing it from Reminder. + event, _ = Event.objects.update_or_create( + google_calendar_id=event.google_calendar_id, + defaults={ + "name": event.name, + "description": event.description, + "url": event.url, + "start_date": event.start_date, + "end_date": event.end_date, + "category": event.category, + "suggested_location": event.suggested_location, + }, + )
61-64: Validate minutes_before is non-negative.Negative inputs produce “minutes after start,” which is counterintuitive.
- reminder_time = event.start_date - timezone.timedelta(minutes=minutes_before) + if minutes_before < 0: + raise ValidationError("minutes_before must be zero or a positive integer.") + reminder_time = event.start_date - timezone.timedelta(minutes=minutes_before)
27-31: Make schedule creation idempotent under the unique constraint.Avoid IntegrityError on duplicates; update recurrence if needed.
- return ReminderSchedule.objects.create( - reminder=reminder, - scheduled_time=scheduled_time, - recurrence=recurrence, - ) + schedule, created = ReminderSchedule.objects.get_or_create( + reminder=reminder, + scheduled_time=scheduled_time, + defaults={"recurrence": recurrence}, + ) + if not created and schedule.recurrence != recurrence: + schedule.recurrence = recurrence + schedule.save(update_fields=["recurrence"]) + return schedulebackend/apps/slack/common/handlers/calendar_events.py (2)
31-36: Display correct timezone labels consistently.Include %Z for both from/to and use timezone.localtime for clarity.
- f"*Your upcoming calendar events from {min_time.strftime('%Y-%m-%d %H:%M')}" - f" to {max_time.strftime('%Y-%m-%d %H:%M %Z')}*" + f"*Your upcoming calendar events from " + f"{timezone.localtime(min_time).strftime('%Y-%m-%d %H:%M %Z')}" + f" to {timezone.localtime(max_time).strftime('%Y-%m-%d %H:%M %Z')}*" @@ - f"{NL}- Starts at: {event.start_date.strftime('%Y-%m-%d %H:%M %Z')}" - f"{NL}- Ends at: {event.end_date.strftime('%Y-%m-%d %H:%M %Z')}" + f"{NL}- Starts at: {timezone.localtime(event.start_date).strftime('%Y-%m-%d %H:%M %Z')}" + f"{NL}- Ends at: {timezone.localtime(event.end_date).strftime('%Y-%m-%d %H:%M %Z')}" @@ - f"{NL} Scheduled Time: " - f"{reminder_schedule.scheduled_time.strftime('%Y-%m-%d %H:%M %Z')}" + f"{NL} Scheduled Time: " + f"{timezone.localtime(reminder_schedule.scheduled_time).strftime('%Y-%m-%d %H:%M %Z')}"Also applies to: 45-47, 93-95
76-83: Consider letting controllers schedule after commit instead of here.To avoid double-scheduling and ensure external side effects only run after DB commit, move scheduling/notification into controllers via transaction.on_commit, and keep this handler purely presentational.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/apps/nest/controllers/calendar_events.py(1 hunks)backend/apps/nest/models/reminder_schedule.py(2 hunks)backend/apps/nest/schedulers/calendar_events/base.py(1 hunks)backend/apps/nest/schedulers/calendar_events/slack.py(1 hunks)backend/apps/nest/schedulers/tasks/slack.py(1 hunks)backend/apps/slack/common/handlers/calendar_events.py(1 hunks)backend/settings/urls.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/nest/models/reminder_schedule.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-08T07:14:17.031Z
Learnt from: ahmedxgouda
PR: OWASP/Nest#2211
File: backend/apps/slack/common/handlers/calendar_events.py:75-75
Timestamp: 2025-09-08T07:14:17.031Z
Learning: In this Django codebase, ValidationError objects have a .message attribute that can be accessed directly (e.g., e.message) for error handling, as evidenced by consistent usage in backend/apps/core/api/internal/algolia.py and backend/apps/slack/common/handlers/calendar_events.py.
Applied to files:
backend/apps/slack/common/handlers/calendar_events.py
🧬 Code graph analysis (5)
backend/apps/nest/schedulers/tasks/slack.py (1)
backend/apps/slack/apps.py (1)
SlackConfig(13-26)
backend/apps/nest/controllers/calendar_events.py (6)
backend/apps/nest/clients/google_calendar.py (2)
GoogleCalendarClient(9-42)get_event(38-42)backend/apps/nest/models/google_account_authorization.py (2)
GoogleAccountAuthorization(28-225)authorize(55-93)backend/apps/nest/models/reminder.py (1)
Reminder(6-33)backend/apps/nest/models/reminder_schedule.py (2)
ReminderSchedule(6-59)Recurrence(20-24)backend/apps/owasp/models/event.py (3)
Event(25-375)parse_google_calendar_event(186-218)save(360-375)backend/apps/slack/models/member.py (1)
Member(10-78)
backend/apps/nest/schedulers/calendar_events/base.py (1)
backend/apps/nest/models/reminder_schedule.py (1)
ReminderSchedule(6-59)
backend/apps/slack/common/handlers/calendar_events.py (1)
backend/apps/slack/blocks.py (2)
get_pagination_buttons(113-149)markdown(21-34)
backend/apps/nest/schedulers/calendar_events/slack.py (3)
backend/apps/nest/models/reminder_schedule.py (3)
ReminderSchedule(6-59)Recurrence(20-24)cron_expression(41-55)backend/apps/nest/schedulers/calendar_events/base.py (1)
BaseScheduler(8-14)backend/apps/nest/schedulers/tasks/slack.py (1)
send_message(6-12)
⏰ 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 backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docker-compose/production.yaml (1)
90-91: Healthcheck shouldn’t use hardcoded names; use the container’s env vars via a shell form.- test: [CMD, pg_isready, -U, nest-user-production, -d, production-nest-db] + test: ["CMD-SHELL", "pg_isready -U $POSTGRES_USER -d $POSTGRES_DB"]Ensure
.env.dbsuppliesPOSTGRES_USERandPOSTGRES_DB.
♻️ Duplicate comments (1)
backend/apps/slack/common/handlers/calendar_events.py (1)
19-23: Return a Slack “Sign in with Google” button instead of a raw link.Improves UX and matches Slack UI patterns.
- auth = GoogleAccountAuthorization.authorize(slack_user_id) - if not isinstance(auth, GoogleAccountAuthorization): - return [markdown(f"*Please sign in with Google first through this <{auth[0]}|link>*")] - client = GoogleCalendarClient(auth) + auth_result = GoogleAccountAuthorization.authorize(slack_user_id) + if not isinstance(auth_result, GoogleAccountAuthorization): + authorization_url, _state = auth_result + return [ + markdown("*Please sign in to Google first.*"), + { + "type": "actions", + "elements": [ + { + "type": "button", + "text": {"type": "plain_text", "text": "Sign in with Google"}, + "url": authorization_url, + } + ], + }, + ] + auth = auth_result + client = GoogleCalendarClient(auth)
🧹 Nitpick comments (17)
docker-compose/staging.yaml (4)
19-27: Add a healthcheck or startup guard for the worker.Without a healthcheck, Compose can't restart on internal failures and
depends_on: condition: service_healthyon other services doesn't guarantee app readiness. Consider a lightweightmanage.py checkor a Redis ping wrapper.
25-26: Fix spellcheck failures: add “rqworker” to cspell dictionary.CI fails on cspell for “rqworker”. Add it to
.cspell.json(words) or your custom dictionary.
40-48: Add a healthcheck for the scheduler.Same rationale as the worker; a tiny script that pings Redis or lists scheduled jobs prevents silent failures.
46-47: Fix spellcheck failures: add “rqscheduler” to cspell dictionary.CI fails on cspell for “rqscheduler”. Add it to the cspell dictionary.
docker-compose/production.yaml (2)
27-27: Fix spellcheck failures: add “rqworker” to cspell dictionary.Add to
.cspell.jsonor your custom words list to unblock CI.
48-48: Fix spellcheck failures: add “rqscheduler” to cspell dictionary.Add to cspell to satisfy CI.
docker-compose/local.yaml (4)
58-58: Fix spellcheck failures: add “rqworker” to cspell dictionary.Unblocks CI on the worker command.
67-67: Fix spellcheck failures: add “rqscheduler” to cspell dictionary.Unblocks CI on the scheduler command.
34-41: Optional: add restart policies for worker/scheduler in local.For smoother DX, consider
restart: unless-stoppedso workers recover after Redis restarts.backend-worker: container_name: nest-backend-worker + restart: unless-stopped @@ backend-scheduler: container_name: nest-backend-scheduler + restart: unless-stoppedAlso applies to: 63-71
34-62: Optional: dedupe repeated env blocks with YAML anchors.Reduces drift across backend/worker/scheduler.
Also applies to: 63-91
backend/apps/slack/common/handlers/calendar_events.py (5)
31-33: Use timezone-aware display consistently in the header.Include zone for both endpoints and localize explicitly.
- f"*Your upcoming calendar events from {min_time.strftime('%Y-%m-%d %H:%M')}" - f" to {max_time.strftime('%Y-%m-%d %H:%M %Z')}*" + f"*Your upcoming calendar events from " + f"{timezone.localtime(min_time).strftime('%Y-%m-%d %H:%M %Z')}" + f" to {timezone.localtime(max_time).strftime('%Y-%m-%d %H:%M %Z')}*"
49-56: Pagination “total_pages” misuse.Passing page + 1 forces a Next button even when there may be no further pages. Either compute a real total or expose an explicit “show_next=True” in get_pagination_buttons.
38-41: Cache key may collide across pages/refreshes.Keying by “{slack_user_id}_{event_number}” can point to a different event if the user opens another page/home refresh before running the command. Consider scoping keys to a session/token or clearing old keys before setting new ones.
83-86: Log exceptions and keep user-friendly responses.Add logging to aid debugging while preserving concise Slack messages.
+import logging +logger = logging.getLogger(__name__) @@ - except ValidationError as e: - return [markdown(f"*{e.message}*")] + except ValidationError as e: + logger.exception("ValidationError while setting reminder for %s", slack_user_id) + return [markdown(f"*{e.message}*")] - except (ServerNotFoundError, Error): - return [markdown("*An unexpected error occurred. Please try again later*")] + except (ServerNotFoundError, Error): + logger.exception("Unexpected error while setting reminder for %s", slack_user_id) + return [markdown("*An unexpected error occurred. Please try again later*")]
77-82: Localize reminder times in confirmation messages.Match event list formatting and avoid confusion if worker runs in a different TZ.
- f" at {reminder_schedule.scheduled_time.strftime('%Y-%m-%d %H:%M %Z')}", + f" at {timezone.localtime(reminder_schedule.scheduled_time).strftime('%Y-%m-%d %H:%M %Z')}", @@ - f"{reminder_schedule.scheduled_time.strftime('%Y-%m-%d %H:%M %Z')}" + f"{timezone.localtime(reminder_schedule.scheduled_time).strftime('%Y-%m-%d %H:%M %Z')}"Also applies to: 92-93
backend/apps/nest/schedulers/calendar_events/slack.py (1)
10-17: Harden Slack send: handle unconfigured app and API failures.Prevents silent no-ops and surfaces errors in logs.
+import logging +logger = logging.getLogger(__name__) @@ - def send_message(message: str, channel_id: str): + def send_message(message: str, channel_id: str): """Send message to the specified Slack channel.""" - if app := SlackConfig.app: - app.client.chat_postMessage( - channel=channel_id, - text=message, - ) + app = SlackConfig.app + if not app: + logger.error("Slack app is not configured; cannot send message to %s", channel_id) + return + try: + app.client.chat_postMessage(channel=channel_id, text=message) + except Exception: + logger.exception("Failed to post Slack message to channel %s", channel_id)backend/apps/nest/schedulers/calendar_events/base.py (1)
27-37: Optional: provide a stable job_id and consider kwargs for parity.Helps dedupe one-off schedules and keeps arg-passing consistent with enqueue_at.
self.scheduler.cron( self.reminder_schedule.cron_expression, func=self.__class__.send_message, - args=( - self.reminder_schedule.reminder.message, - self.reminder_schedule.reminder.channel_id, - ), + args=( + self.reminder_schedule.reminder.message, + self.reminder_schedule.reminder.channel_id, + ), queue_name="default", use_local_timezone=True, result_ttl=500, + # job_id=f"reminder:{self.reminder_schedule.id}", )
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/apps/nest/schedulers/calendar_events/base.py(1 hunks)backend/apps/nest/schedulers/calendar_events/slack.py(1 hunks)backend/apps/slack/common/handlers/calendar_events.py(1 hunks)backend/docker/Dockerfile(0 hunks)docker-compose/local.yaml(1 hunks)docker-compose/production.yaml(1 hunks)docker-compose/staging.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- backend/docker/Dockerfile
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-08T07:14:17.031Z
Learnt from: ahmedxgouda
PR: OWASP/Nest#2211
File: backend/apps/slack/common/handlers/calendar_events.py:75-75
Timestamp: 2025-09-08T07:14:17.031Z
Learning: In this Django codebase, ValidationError objects have a .message attribute that can be accessed directly (e.g., e.message) for error handling, as evidenced by consistent usage in backend/apps/core/api/internal/algolia.py and backend/apps/slack/common/handlers/calendar_events.py.
Applied to files:
backend/apps/slack/common/handlers/calendar_events.py
🧬 Code graph analysis (3)
backend/apps/nest/schedulers/calendar_events/base.py (2)
backend/apps/nest/models/reminder_schedule.py (3)
ReminderSchedule(6-59)Recurrence(20-24)cron_expression(41-55)backend/apps/nest/schedulers/calendar_events/slack.py (1)
send_message(11-17)
backend/apps/slack/common/handlers/calendar_events.py (7)
backend/apps/slack/blocks.py (2)
get_pagination_buttons(113-149)markdown(21-34)backend/apps/nest/clients/google_calendar.py (2)
GoogleCalendarClient(9-42)get_events(19-36)backend/apps/nest/models/google_account_authorization.py (2)
GoogleAccountAuthorization(28-225)authorize(55-93)backend/apps/owasp/models/event.py (2)
Event(25-375)parse_google_calendar_events(173-183)backend/apps/nest/controllers/calendar_events.py (1)
set_reminder(34-84)backend/apps/nest/schedulers/calendar_events/slack.py (2)
SlackScheduler(7-17)send_message(11-17)backend/apps/nest/schedulers/calendar_events/base.py (2)
schedule(16-37)send_message(40-43)
backend/apps/nest/schedulers/calendar_events/slack.py (2)
backend/apps/nest/schedulers/calendar_events/base.py (2)
BaseScheduler(8-43)send_message(40-43)backend/apps/slack/apps.py (1)
SlackConfig(13-26)
🪛 GitHub Actions: Run CI/CD
docker-compose/staging.yaml
[error] 25-25: cspell: Unknown word (rqworker) detected during 'make check-spelling'.
[error] 46-46: cspell: Unknown word (rqscheduler) detected during 'make check-spelling'.
docker-compose/local.yaml
[error] 38-38: cspell: Unknown word (rqworker) detected during 'make check-spelling'.
[error] 67-67: cspell: Unknown word (rqscheduler) detected during 'make check-spelling'.
docker-compose/production.yaml
[error] 27-27: cspell: Unknown word (rqworker) detected during 'make check-spelling'.
[error] 48-48: cspell: Unknown word (rqscheduler) detected during 'make check-spelling'.
🔇 Additional comments (1)
docker-compose/production.yaml (1)
6-6: Verify entrypoint path exists in the image.Ensure
/home/owasp/entrypoint-production.shis present and executable inowasp/nest:backend-production.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
backend/tests/apps/nest/utils/calendar_events_test.py (1)
22-30: Add edge-case coverage for negatives/zero and missing requireds.Tests don’t exercise invalid ints or missing required flags; add cases expecting SystemExit. This aligns with earlier feedback on argument validation.
class TestCalendarEventsUtils: @@ def test_parse_reminder_args_missing_optional_args(self): @@ assert args.recurrence == "once" # Default value + + def test_parse_reminder_args_invalid_numbers(self): + # minutes_before must be positive int + text = "--channel C123456 --event_number 2 --minutes_before 0" + try: + parse_reminder_args(text) + assert False, "Expected SystemExit for invalid minutes_before" + except SystemExit: + pass + + def test_parse_reminder_args_missing_required(self): + # Missing --event_number + text = "--channel C123456" + try: + parse_reminder_args(text) + assert False, "Expected SystemExit for missing required args" + except SystemExit: + passbackend/apps/nest/utils/calendar_events.py (1)
7-45: Validate positive integers for event_number and minutes_before.Argparse accepts negatives/zero today. Add a small validator for > 0. Also keeps behavior consistent across surfaces.
-import shlex -from argparse import ArgumentParser +import shlex +from argparse import ArgumentParser, ArgumentTypeError @@ def parse_reminder_args(text: str): @@ - parser = ArgumentParser(prog="/set-reminder", description="Set a reminder for a Slack event.") + parser = ArgumentParser(prog="/set-reminder", description="Set a reminder for a Slack event.") + + def positive_int(v: str) -> int: + try: + iv = int(v) + except ValueError as e: + raise ArgumentTypeError("must be an integer") from e + if iv <= 0: + raise ArgumentTypeError("must be > 0") + return iv @@ - parser.add_argument( - "--event_number", type=int, help="The event number to set the reminder for.", required=True - ) + parser.add_argument( + "--event_number", + type=positive_int, + help="The event number to set the reminder for.", + required=True, + ) @@ - type=int, + type=positive_int, help="Minutes before the event to send the reminder.", default=10, )backend/apps/slack/common/handlers/calendar_events.py (1)
18-21: Optional UX: render a clickable “Sign in with Google” button.Current markdown link works and matches tests; consider adding an actions block with a button for better UX in a follow-up (don’t change now to avoid test breakage).
🧹 Nitpick comments (26)
backend/tests/apps/nest/controllers/calendar_events_test.py (8)
20-38: Strengthen success-path assertions to prevent silent regressions (message formatting, schedule args, and Google client calls).Right now the test doesn’t verify:
- formatted reminder message ("{event.name} - {message}"),
- that schedule_reminder is invoked with the computed scheduled_time,
- that GoogleCalendarClient.get_event is called with the cached event id.
Also, the mocked Reminder uses message=message, which could mask a formatting bug. Suggest the following tweaks.
@@ def test_set_reminder_success( @@ - mock_reminder = Reminder( + expected_message = f"{mock_event.name} - {message}" + mock_reminder = Reminder( member=mock_member, event=mock_event, - message=message, + message=expected_message, channel_id=channel, ) @@ # Call the function result = set_reminder( @@ ) # Assertions assert result.reminder.member == mock_member assert result.reminder.event == mock_event assert result.recurrence == recurrence + expected_scheduled_time = mock_event.start_date - timezone.timedelta(minutes=minutes_before) + mock_reminder_create.assert_called_once_with( + channel_id=channel, + event=mock_event, + member=mock_member, + message=expected_message, + ) + mock_schedule_reminder.assert_called_once_with( + reminder=mock_reminder, + scheduled_time=expected_scheduled_time, + recurrence=recurrence, + ) mock_member_get.assert_called_once_with(slack_user_id=user_id) mock_authorize.assert_called_once_with(user_id) mock_cache.get.assert_called_once_with(f"{user_id}_{event_number}") mock_parse_event.assert_called_once() mock_event_save.assert_called_once() - mock_google_client.assert_called_once_with(mock_auth) + mock_google_client.assert_called_once_with(mock_auth) + mock_google_client.return_value.get_event.assert_called_once_with("google_calendar_event_id")Also applies to: 48-79, 80-99
154-190: Assert Google client is used with the cached event id.Verifies the integration surface between cache, Google client, and parser.
@@ mock_parse_event.assert_called_once() - mock_google_client.assert_called_once() + mock_google_client.assert_called_once() + mock_google_client.return_value.get_event.assert_called_once_with("google_calendar_event_id")
191-235: Do the same get_event id assertion here for past-time path.@@ mock_parse_event.assert_called_once() - mock_google_client.assert_called_once() + mock_google_client.assert_called_once() + mock_google_client.return_value.get_event.assert_called_once_with("google_calendar_event_id")
236-277: And for invalid recurrence path as well.@@ mock_parse_event.assert_called_once() - mock_google_client.assert_called_once() + mock_google_client.assert_called_once() + mock_google_client.return_value.get_event.assert_called_once_with("google_calendar_event_id")
100-112: Use robust ValidationError assertions.Accessing
.messageis brittle across Django versions; preferstr(excinfo.value)which is stable for single-message errors.- assert excinfo.value.message == "Minutes before must be a positive integer." + assert str(excinfo.value) == "Minutes before must be a positive integer." @@ - assert excinfo.value.message == "Scheduled time must be in the future." + assert str(excinfo.value) == "Scheduled time must be in the future." @@ - assert excinfo.value.message == "Invalid recurrence value." + assert str(excinfo.value) == "Invalid recurrence value."Also applies to: 317-336, 337-357
20-38: Optional: avoid potential Manager descriptor pitfalls when mocking .objects.get.Patching
"…Member.objects.get"usually works, butpatch.object(Member.objects, "get")is more explicit and less fragile.Example:
with patch.object(Member.objects, "get") as mock_member_get: mock_member_get.return_value = mock_member ...
358-358: Add a test for default recurrence (ONCE) and message fallback when no custom message is provided.Covers two important defaults in one go.
+ @patch("apps.nest.controllers.calendar_events.GoogleCalendarClient") + @patch("apps.nest.controllers.calendar_events.cache") + @patch("apps.nest.controllers.calendar_events.GoogleAccountAuthorization.authorize") + @patch("apps.nest.controllers.calendar_events.Event.parse_google_calendar_event") + @patch("apps.nest.controllers.calendar_events.Reminder.objects.create") + @patch("apps.nest.controllers.calendar_events.schedule_reminder") + def test_set_reminder_defaults_once_and_message_fallback( + self, + mock_schedule_reminder, + mock_reminder_create, + mock_parse_event, + mock_authorize, + mock_cache, + mock_google_client, + ): + user_id = "U123456" + event_number = "7" + channel = "C123456" + minutes_before = 30 + mock_auth = GoogleAccountAuthorization( + access_token="t", refresh_token="r", expires_at=timezone.now() + timezone.timedelta(hours=1) + ) + mock_authorize.return_value = mock_auth + mock_cache.get.return_value = "google_calendar_event_id" + mock_event = Event( + google_calendar_id="event_id", + key="event_key", + name="Test Event", + start_date=timezone.now() + timezone.timedelta(hours=2), + end_date=timezone.now() + timezone.timedelta(hours=3), + description="Event Description", + ) + mock_parse_event.return_value = mock_event + mock_reminder = Reminder(member=Member(slack_user_id=user_id), event=mock_event, message=mock_event.name, channel_id=channel) + mock_reminder_create.return_value = mock_reminder + expected_scheduled_time = mock_event.start_date - timezone.timedelta(minutes=minutes_before) + mock_schedule_reminder.return_value = ReminderSchedule( + reminder=mock_reminder, scheduled_time=expected_scheduled_time, recurrence=ReminderSchedule.Recurrence.ONCE + ) + + result = set_reminder( + channel=channel, + event_number=event_number, + user_id=user_id, + minutes_before=minutes_before, + recurrence=None, + message="", + ) + + assert result.recurrence == ReminderSchedule.Recurrence.ONCE + mock_reminder_create.assert_called_once_with( + channel_id=channel, event=mock_event, member=mock_reminder.member, message=mock_event.name + ) + mock_schedule_reminder.assert_called_once_with( + reminder=mock_reminder, scheduled_time=expected_scheduled_time, recurrence=ReminderSchedule.Recurrence.ONCE + ) + mock_google_client.assert_called_once_with(mock_auth) + mock_google_client.return_value.get_event.assert_called_once_with("google_calendar_event_id")
40-66: Minor stability: capture a single 'now' per test to avoid microsecond drift in constructed datetimes.Helps reduce flakiness in time-comparison assertions.
now = timezone.now() start = now + timezone.timedelta(days=1) end = start + timezone.timedelta(hours=1)Also applies to: 208-216, 258-261, 282-295, 321-351
backend/tests/apps/nest/models/reminder_schedule_test.py (1)
51-72: Cron mapping looks correct; consider parametrizing to reduce duplication.Assertions for DAILY/WEEKLY/MONTHLY/ONCE match the model’s logic and DOW mapping. As a light improvement, parametrize over (recurrence, expected) to shrink the test and make it easier to extend.
backend/apps/common/utils.py (1)
37-58: Solid parser + TZ handling; clarify naive semantics and prefer app TZ for awareness.Currently naive datetimes are assumed UTC before converting to local. If you’d rather treat naive input as the project’s default timezone, switch to timezone.get_default_timezone(). Also mention this in the docstring to avoid ambiguity.
Apply:
def parse_date_and_convert_to_local(date_string: str | None) -> datetime | None: @@ - Returns: - datetime | None: The converted datetime object in the local timezone, - or None if parsing fails. + Returns: + datetime | None: The converted datetime in the current local timezone, + or None if parsing fails. Naive inputs are assumed to be in the + project’s default timezone. @@ - if timezone.is_naive(parsed): - parsed = timezone.make_aware(parsed, timezone=UTC) + if timezone.is_naive(parsed): + parsed = timezone.make_aware(parsed, timezone=timezone.get_default_timezone()) return timezone.localtime(parsed)backend/tests/apps/nest/schedulers/calendar_events/slack_test.py (1)
11-21: Add a no-app case to prevent false positives.Also verify no call when SlackConfig.app is None.
class TestSlackScheduler: @@ @patch("apps.nest.schedulers.calendar_events.slack.SlackConfig") def test_send_message(self, mock_slack_config): @@ mock_client.chat_postMessage.assert_called_once_with( channel="C123456", text="Test Message", ) + + @patch("apps.nest.schedulers.calendar_events.slack.SlackConfig") + def test_send_message_no_app(self, mock_slack_config): + """Should no-op when Slack app is not configured.""" + mock_slack_config.app = None + SlackScheduler.send_message("Test Message", "C123456") + # Nothing to assert; absence of exceptions is success.backend/tests/apps/nest/schedulers/calendar_events/base_test.py (2)
27-46: Use an aware datetime to mirror production and catch type issues.enqueue_at expects a datetime; using a string masks type errors.
-from unittest.mock import MagicMock, patch +from unittest.mock import MagicMock, patch +from django.utils import timezone @@ - mock_reminder_schedule.scheduled_time = "2024-10-10 10:00:00" + mock_reminder_schedule.scheduled_time = timezone.make_aware( + timezone.datetime(2024, 10, 10, 10, 0, 0) + ) @@ - scheduler_instance.enqueue_at.assert_called_once_with( - "2024-10-10 10:00:00", + scheduler_instance.enqueue_at.assert_called_once_with( + mock_reminder_schedule.scheduled_time, BaseScheduler.send_message, message="Test Message", channel_id="C123456", )
49-70: Prefer enum for recurrence to reflect real values.Set recurrence to ReminderSchedule.Recurrence.DAILY for clarity.
-from apps.nest.schedulers.calendar_events.base import BaseScheduler +from apps.nest.schedulers.calendar_events.base import BaseScheduler +from apps.nest.models.reminder_schedule import ReminderSchedule @@ - mock_reminder_schedule.recurrence = "daily" + mock_reminder_schedule.recurrence = ReminderSchedule.Recurrence.DAILYbackend/tests/apps/nest/clients/google_calendar_test.py (2)
28-52: Avoid patching the SUT; mock the Google service to verify request wiring.Patching GoogleCalendarClient.get_events makes the test tautological. Instead, patch build to return a service mock and assert events().list(...).execute() is called with expected params and default time window.
- @override_settings(IS_GOOGLE_AUTH_ENABLED=True, IS_AWS_KMS_ENABLED=True) - @patch.object(GoogleCalendarClient, "get_events") - def test_get_events(self, mock_get_events): + @override_settings(IS_GOOGLE_AUTH_ENABLED=True, IS_AWS_KMS_ENABLED=True) + @patch("apps.nest.clients.google_calendar.build") + def test_get_events(self, mock_build): """Test the retrieval of events from Google Calendar.""" - mock_get_events.return_value = [ - { - "id": "12345", - "summary": "Test Event", - "start": {"dateTime": "2025-05-26T09:00:00-07:00"}, - "end": {"dateTime": "2025-05-30T17:00:00-07:00"}, - } - ] - - client = GoogleCalendarClient( + service = MagicMock() + events = service.events.return_value + list_call = events.list.return_value + list_call.execute.return_value = {"items": [{"id": "12345", "summary": "Test Event"}]} + mock_build.return_value = service + + client = GoogleCalendarClient( google_account_authorization=GoogleAccountAuthorization( access_token="mock_access_token", # noqa: S106 refresh_token="mock_refresh_token", # noqa: S106 expires_at=timezone.now() + timezone.timedelta(hours=1), ) ) - events = client.get_events() - assert len(events) == 1 - assert events[0]["id"] == "12345" - assert events[0]["summary"] == "Test Event" + result = client.get_events() + assert result == [{"id": "12345", "summary": "Test Event"}] + events.list.assert_called_once() + list_call.execute.assert_called_once()
53-77: Refactortest_get_eventto patch build and use the correctgoogle_calendar_id
- Patch
apps.nest.clients.google_calendar.buildinstead ofGoogleCalendarClient.get_event.- In the test, invoke
client.get_event(google_calendar_id="12345")(per implementation signature) and simulate:
service = MagicMock(); mock_build.return_value = serviceevents = service.events.return_valueget_call = events.get.return_valueget_call.execute.return_value = {...}- Assert:
events.get.assert_called_once_with(calendarId="primary", eventId="12345")get_call.execute.assert_called_once()backend/tests/apps/slack/common/handlers/calendar_events_test.py (4)
66-76: Test is brittle due to assuming a fixed actions block count.len(blocks) == 4 relies on “Next” always being present. If pagination logic changes, this will break. Prefer asserting the presence of header + two event sections and optionally checking actions type when present.
- assert len(blocks) == 4 # 1 header + 2 events + 1 pagination + # 1 header + 2 events + assert len(blocks) >= 3 + if len(blocks) > 3: + assert blocks[-1]["type"] == "actions"
108-124: Add a test for “no parsable events after filtering.”When Google returns items but parsing filters them all out, UI currently shows only a header (see handler comment). Add a case where events contain non-confirmed/invalid entries and assert the “No upcoming…” block.
125-162: Assert the Slack confirmation message contents minimally.You already assert schedule/send calls; add a light assertion on the composed message including event name and scheduled time prefix to guard regressions.
- mock_slack_scheduler.send_message.assert_called_once() + mock_slack_scheduler.send_message.assert_called_once() + args, kwargs = mock_slack_scheduler.send_message.call_args + assert "set a reminder" in args[0] + assert "Test Event" in args[0]
19-21: Clear cache between tests to avoid cross-test interference.get_blocks() writes to cache; while unlikely, keys could collide across tests. Clear cache in setup/teardown.
+from django.core.cache import cache + class TestCalendarEvents: """Slack calendar events handlers test cases.""" + def setup_method(self): + cache.clear() + + def teardown_method(self): + cache.clear()Also applies to: 22-26, 77-90, 91-107, 111-124, 163-177, 178-191
backend/apps/slack/common/handlers/calendar_events.py (5)
28-31: Handle “no parsable events” (filtered out) explicitly.If events is non-empty but all items are filtered out, the UI shows only a header. Check parsed_events alone.
- if not (events or parsed_events): + if not parsed_events: return [markdown("*No upcoming calendar events found.*")]
33-38: Fix timezone display in header; include zone for both bounds and localize.min_time lacks %Z and both should be localized.
- blocks = [ - markdown( - f"*Your upcoming calendar events from {min_time.strftime('%Y-%m-%d %H:%M')}" - f" to {max_time.strftime('%Y-%m-%d %H:%M %Z')}*" + local_min_time = timezone.localtime(min_time) + local_max_time = timezone.localtime(max_time) + blocks = [ + markdown( + f"*Your upcoming calendar events from {local_min_time.strftime('%Y-%m-%d %H:%M %Z')}" + f" to {local_max_time.strftime('%Y-%m-%d %H:%M %Z')}*" f"{NL}You can set a reminder for an event with `/set-reminder --channel [channel]" " --event_number [event number] --minutes_before [minutes before]" " --message [optional message] --recurrence [recurrence]`" ) ]
45-49: Localize event times before formatting.Ensures correct zone label regardless of stored tz.
- f"{NL}- Starts at: {event.start_date.strftime('%Y-%m-%d %H:%M %Z')}" - f"{NL}- Ends at: {event.end_date.strftime('%Y-%m-%d %H:%M %Z')}" + f"{NL}- Starts at: {timezone.localtime(event.start_date).strftime('%Y-%m-%d %H:%M %Z')}" + f"{NL}- Ends at: {timezone.localtime(event.end_date).strftime('%Y-%m-%d %H:%M %Z')}"
21-27: Catch Google API HttpError as a service failure as well.Network/HTTP failures from googleapiclient should map to the same user-facing message.
-from httplib2.error import ServerNotFoundError +from httplib2.error import ServerNotFoundError +from googleapiclient.errors import HttpError @@ - except ServerNotFoundError: + except (ServerNotFoundError, HttpError): return [markdown("*Please check your internet connection.*")]
77-83: Localize scheduled_time in user-facing strings.scheduled_time is tz-aware (per controllers), but localizing ensures correct label in Slack.
- f" at {reminder_schedule.scheduled_time.strftime('%Y-%m-%d %H:%M %Z')}", + f" at {timezone.localtime(reminder_schedule.scheduled_time).strftime('%Y-%m-%d %H:%M %Z')}", @@ - f"{NL} Scheduled Time: " - f"{reminder_schedule.scheduled_time.strftime('%Y-%m-%d %H:%M %Z')}" + f"{NL} Scheduled Time: " + f"{timezone.localtime(reminder_schedule.scheduled_time).strftime('%Y-%m-%d %H:%M %Z')}"Also applies to: 88-97
backend/tests/apps/slack/commands/set_reminder_test.py (2)
10-22: Decouple the happy-path test from the real parser and assert call args.Patching parse_reminder_args here improves isolation and lets you validate both dependencies were invoked correctly.
Apply this diff:
- @patch("apps.slack.commands.set_reminder.get_reminder_blocks") - def test_render_blocks_valid_args(self, mock_get_reminder_blocks): + @patch("apps.slack.commands.set_reminder.get_reminder_blocks") + @patch("apps.slack.commands.set_reminder.parse_reminder_args") + def test_render_blocks_valid_args(self, mock_parse_reminder_args, mock_get_reminder_blocks): """Test render_blocks with valid arguments.""" - mock_get_reminder_blocks.return_value = [markdown("Reminder set!")] + mock_get_reminder_blocks.return_value = [markdown("Reminder set!")] + parsed_args = object() + mock_parse_reminder_args.return_value = parsed_args command = { "text": "--channel C123456 --event_number 1 --minutes_before 10", "user_id": "U123456", } set_reminder_cmd = SetReminder() blocks = set_reminder_cmd.render_blocks(command) assert blocks == [markdown("Reminder set!")] - mock_get_reminder_blocks.assert_called_once() + mock_parse_reminder_args.assert_called_once_with(command["text"]) + mock_get_reminder_blocks.assert_called_once_with(parsed_args, "U123456")
23-35: Tighten assertions on the error block structure.Validate Slack block shape in addition to the message for better signal.
Apply this diff:
set_reminder_cmd = SetReminder() blocks = set_reminder_cmd.render_blocks(command) assert len(blocks) == 1 + assert blocks[0]["type"] == "section" + assert blocks[0]["text"]["type"] == "mrkdwn" assert ( "*Invalid command format. Please check your input and try again.*" in blocks[0]["text"]["text"] ) mock_parse_reminder_args.assert_called_once()
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
backend/apps/common/utils.py(2 hunks)backend/apps/nest/controllers/calendar_events.py(1 hunks)backend/apps/nest/utils/calendar_events.py(1 hunks)backend/apps/slack/common/handlers/calendar_events.py(1 hunks)backend/tests/apps/nest/clients/google_calendar_test.py(1 hunks)backend/tests/apps/nest/controllers/calendar_events_test.py(1 hunks)backend/tests/apps/nest/models/reminder_schedule_test.py(2 hunks)backend/tests/apps/nest/schedulers/calendar_events/base_test.py(1 hunks)backend/tests/apps/nest/schedulers/calendar_events/slack_test.py(1 hunks)backend/tests/apps/nest/utils/calendar_events_test.py(1 hunks)backend/tests/apps/slack/commands/set_reminder_test.py(1 hunks)backend/tests/apps/slack/common/handlers/calendar_events_test.py(1 hunks)docker-compose/local.yaml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/apps/nest/controllers/calendar_events.py
- docker-compose/local.yaml
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: ahmedxgouda
PR: OWASP/Nest#2211
File: backend/apps/nest/controllers/calendar_events.py:0-0
Timestamp: 2025-09-10T15:32:12.665Z
Learning: In the backend/apps/nest/controllers/calendar_events.py file, the scheduled_time parameter in the schedule_reminder function is guaranteed to be timezone-aware, so explicit timezone awareness validation is not needed.
📚 Learning: 2025-09-10T15:32:12.665Z
Learnt from: ahmedxgouda
PR: OWASP/Nest#2211
File: backend/apps/nest/controllers/calendar_events.py:0-0
Timestamp: 2025-09-10T15:32:12.665Z
Learning: In the backend/apps/nest/controllers/calendar_events.py file, the scheduled_time parameter in the schedule_reminder function is guaranteed to be timezone-aware, so explicit timezone awareness validation is not needed.
Applied to files:
backend/tests/apps/nest/controllers/calendar_events_test.pybackend/tests/apps/nest/models/reminder_schedule_test.pybackend/apps/slack/common/handlers/calendar_events.py
📚 Learning: 2025-09-08T07:14:17.047Z
Learnt from: ahmedxgouda
PR: OWASP/Nest#2211
File: backend/apps/slack/common/handlers/calendar_events.py:75-75
Timestamp: 2025-09-08T07:14:17.047Z
Learning: In this Django codebase, ValidationError objects have a .message attribute that can be accessed directly (e.g., e.message) for error handling, as evidenced by consistent usage in backend/apps/core/api/internal/algolia.py and backend/apps/slack/common/handlers/calendar_events.py.
Applied to files:
backend/apps/slack/common/handlers/calendar_events.py
🧬 Code graph analysis (10)
backend/tests/apps/nest/controllers/calendar_events_test.py (5)
backend/apps/nest/controllers/calendar_events.py (2)
schedule_reminder(15-31)set_reminder(34-86)backend/apps/nest/models/google_account_authorization.py (1)
GoogleAccountAuthorization(28-225)backend/apps/nest/models/reminder.py (1)
Reminder(6-33)backend/apps/nest/models/reminder_schedule.py (1)
ReminderSchedule(6-59)backend/apps/slack/models/member.py (1)
Member(10-78)
backend/tests/apps/nest/utils/calendar_events_test.py (1)
backend/apps/nest/utils/calendar_events.py (1)
parse_reminder_args(7-45)
backend/tests/apps/nest/schedulers/calendar_events/slack_test.py (1)
backend/apps/nest/schedulers/calendar_events/slack.py (1)
SlackScheduler(7-17)
backend/tests/apps/nest/models/reminder_schedule_test.py (4)
backend/apps/slack/models/member.py (1)
Member(10-78)backend/apps/nest/models/reminder.py (1)
Reminder(6-33)backend/apps/nest/schedulers/calendar_events/base.py (1)
schedule(16-37)backend/apps/nest/models/reminder_schedule.py (3)
ReminderSchedule(6-59)Recurrence(20-24)cron_expression(41-55)
backend/tests/apps/slack/common/handlers/calendar_events_test.py (6)
backend/apps/nest/models/google_account_authorization.py (1)
GoogleAccountAuthorization(28-225)backend/apps/nest/models/reminder.py (1)
Reminder(6-33)backend/apps/nest/models/reminder_schedule.py (1)
ReminderSchedule(6-59)backend/apps/slack/common/handlers/calendar_events.py (1)
get_reminder_blocks(63-98)backend/apps/slack/common/presentation.py (1)
EntityPresentation(7-25)backend/apps/slack/models/member.py (1)
Member(10-78)
backend/apps/nest/utils/calendar_events.py (1)
backend/apps/slack/models/message.py (1)
text(83-85)
backend/tests/apps/nest/schedulers/calendar_events/base_test.py (2)
backend/apps/nest/schedulers/calendar_events/base.py (2)
BaseScheduler(8-43)schedule(16-37)backend/apps/nest/models/reminder_schedule.py (1)
cron_expression(41-55)
backend/tests/apps/slack/commands/set_reminder_test.py (2)
backend/apps/slack/blocks.py (1)
markdown(21-34)backend/apps/slack/commands/set_reminder.py (1)
SetReminder(9-24)
backend/tests/apps/nest/clients/google_calendar_test.py (2)
backend/apps/nest/clients/google_calendar.py (3)
GoogleCalendarClient(9-42)get_events(19-36)get_event(38-42)backend/apps/nest/models/google_account_authorization.py (1)
GoogleAccountAuthorization(28-225)
backend/apps/slack/common/handlers/calendar_events.py (7)
backend/apps/slack/blocks.py (2)
get_pagination_buttons(113-149)markdown(21-34)backend/apps/nest/clients/google_calendar.py (2)
GoogleCalendarClient(9-42)get_events(19-36)backend/apps/nest/models/google_account_authorization.py (2)
GoogleAccountAuthorization(28-225)authorize(55-93)backend/apps/owasp/models/event.py (2)
Event(25-375)parse_google_calendar_events(173-183)backend/apps/nest/controllers/calendar_events.py (1)
set_reminder(34-86)backend/apps/nest/schedulers/calendar_events/slack.py (2)
SlackScheduler(7-17)send_message(11-17)backend/apps/nest/schedulers/calendar_events/base.py (2)
schedule(16-37)send_message(40-43)
🔇 Additional comments (6)
backend/tests/apps/nest/controllers/calendar_events_test.py (1)
1-17: Good coverage and mocking strategy.Solid happy/sad-path coverage for both controllers with correct patch targets (patching in the module under test). Timezone-aware datetimes are used consistently, aligning with our earlier learning.
backend/tests/apps/nest/models/reminder_schedule_test.py (1)
3-3: Good: timezone-aware setup aligns with contract.Using timezone.make_aware is correct and matches the retrieved learning that scheduled_time is guaranteed to be aware in controllers. Thanks for keeping tests consistent with that.
(Used retrieved learnings to validate this.)backend/tests/apps/nest/utils/calendar_events_test.py (1)
9-21: Happy path coverage looks good.backend/tests/apps/nest/schedulers/calendar_events/base_test.py (1)
72-76: LGTM: explicit NotImplementedError message is validated.backend/tests/apps/slack/commands/set_reminder_test.py (2)
1-36: LGTM on core happy-path and error-path coverage.The tests exercise the main branches of SetReminder.render_blocks and use targeted patching to isolate behavior.
5-6: Confirm import base is correct in your test runner.If tests execute from repo root, ensure PYTHONPATH includes backend so imports like apps.slack.* resolve; otherwise switch to backend.apps.slack.*.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
backend/apps/nest/models/reminder_schedule.py (2)
10-15: Revisit uniqueness scope and add uniqueness forjob_id(when set)
- Consider including
recurrencein the unique key; current constraint blocks creating different recurrence modes at the same timestamp for the same reminder.- Also ensure
job_idis unique when present to avoid ambiguous job references.Suggested change:
class ReminderSchedule(models.Model): class Meta: constraints = [ - models.UniqueConstraint( - fields=["reminder", "scheduled_time"], - name="unique_reminder_schedule", - ) + models.UniqueConstraint( + fields=["reminder", "scheduled_time", "recurrence"], + name="unique_reminder_schedule", + ), + models.UniqueConstraint( + fields=["job_id"], + condition=models.Q(job_id__isnull=False), + name="unique_job_id_when_set", + ), ]Note: Changing the existing unique fields will require a new migration and potential data backfill if duplicates exist.
47-62: Confirm cron format and timezone semantics
- Validate your scheduler expects 5-field crons (min hour dom mon dow). Some libs allow/expect seconds.
scheduled_timetz: If stored in UTC and the scheduler interprets crons in local/server TZ, DST shifts may fire at unintended wall-times. Document the assumption or normalize to the scheduler’s TZ before deriving the cron.If needed, clarify via docstring:
- def cron_expression(self) -> str | None: - """Get cron expression for the scheduled time.""" + def cron_expression(self) -> str | None: + """Get a 5-field cron expression (minute hour day-of-month month day-of-week). + Assumes scheduled_time is in the scheduler’s effective timezone."""
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/apps/nest/migrations/0012_reminderschedule_job_id.py(1 hunks)backend/apps/nest/models/reminder_schedule.py(2 hunks)backend/apps/nest/schedulers/calendar_events/base.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/nest/schedulers/calendar_events/base.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 backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
arkid15r
changed the base branch from
feature/nestbot-calendar-events-draft
to
feature/nestbot-calendar-events
ahmedxgouda
force-pushed
the
nestbot-calendar/schedule-event
branch
from
ee50d9c to
9899144
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/apps/slack/common/handlers/calendar_events.py (1)
18-21: Offer a “Sign in with Google” button instead of a plain link.Improves UX with a clickable action block.
Example:
- if not isinstance(auth, GoogleAccountAuthorization): - return [markdown(f"*Please sign in with Google first through this <{auth[0]}|link>*")] + if not isinstance(auth, GoogleAccountAuthorization): + authorization_url, _state = auth + return [ + markdown("*Please sign in with Google first.*"), + {"type": "actions", "elements": [ + {"type": "button", "text": {"type": "plain_text", "text": "Sign in with Google"}, "url": authorization_url} + ]}, + ]
🧹 Nitpick comments (5)
backend/apps/nest/models/google_account_authorization.py (1)
181-185: Guard against None expires_at to avoid AttributeError.If
expires_atis ever missing (e.g., odd auth responses),timezone.make_naive(None)will raise. ReturnNonewhen unset;Credentialsacceptsexpiry=None.Apply:
@property def naive_expires_at(self): """Get the naive datetime of token expiry.""" - return timezone.make_naive(self.expires_at) + return timezone.make_naive(self.expires_at) if self.expires_at else Nonebackend/tests/apps/slack/commands/set_reminder_test.py (1)
23-36: Invalid‑args path is covered.Good assertion on the user‑facing error. Consider a follow‑up test for connectivity errors (ServerNotFoundError) to lock UI copy.
backend/apps/slack/common/handlers/calendar_events.py (3)
29-31: Fix empty-results condition; check parsed events only.If Google returns items but all are filtered out, header renders with no items.
Apply:
- if not (events or parsed_events): + if not parsed_events: return [markdown("*No upcoming calendar events found*.")]
33-38: Show correct timezone label for both bounds.Include %Z for min_time and ensure localtime formatting is consistent.
Apply:
- f"*Your upcoming calendar events from {min_time.strftime('%Y-%m-%d %H:%M')}" - f" to {max_time.strftime('%Y-%m-%d %H:%M %Z')}*" + f"*Your upcoming calendar events from " + f"{timezone.localtime(min_time).strftime('%Y-%m-%d %H:%M %Z')}" + f" to {timezone.localtime(max_time).strftime('%Y-%m-%d %H:%M %Z')}*"
47-49: Guard when end_date is missing.Some events may not have an end; calling
strftimeonNonewill crash.Apply:
- f"{NL}- Ends at: {event.end_date.strftime('%Y-%m-%d %H:%M %Z')}" + f"{NL}- Ends at: " + f"{timezone.localtime(event.end_date).strftime('%Y-%m-%d %H:%M %Z') if event.end_date else 'N/A'}"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
backend/poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (41)
backend/apps/common/utils.py(2 hunks)backend/apps/nest/clients/google_calendar.py(1 hunks)backend/apps/nest/controllers/calendar_events.py(1 hunks)backend/apps/nest/migrations/0009_reminder_event.py(1 hunks)backend/apps/nest/migrations/0010_reminderschedule_unique_reminder_schedule.py(1 hunks)backend/apps/nest/migrations/0011_merge_20250908_0548.py(1 hunks)backend/apps/nest/migrations/0012_reminderschedule_job_id.py(1 hunks)backend/apps/nest/models/google_account_authorization.py(1 hunks)backend/apps/nest/models/reminder.py(1 hunks)backend/apps/nest/models/reminder_schedule.py(2 hunks)backend/apps/nest/schedulers/calendar_events/base.py(1 hunks)backend/apps/nest/schedulers/calendar_events/slack.py(1 hunks)backend/apps/nest/utils/calendar_events.py(1 hunks)backend/apps/owasp/migrations/0053_alter_event_end_date_alter_event_start_date.py(1 hunks)backend/apps/owasp/migrations/0054_merge_20250908_0549.py(1 hunks)backend/apps/owasp/models/event.py(4 hunks)backend/apps/slack/MANIFEST.yaml(1 hunks)backend/apps/slack/actions/home.py(3 hunks)backend/apps/slack/blocks.py(1 hunks)backend/apps/slack/commands/__init__.py(1 hunks)backend/apps/slack/commands/set_reminder.py(1 hunks)backend/apps/slack/common/handlers/calendar_events.py(1 hunks)backend/apps/slack/constants.py(1 hunks)backend/apps/slack/utils.py(1 hunks)backend/docker/Dockerfile(0 hunks)backend/pyproject.toml(1 hunks)backend/settings/base.py(2 hunks)backend/tests/apps/common/utils_test.py(3 hunks)backend/tests/apps/nest/clients/google_calendar_test.py(1 hunks)backend/tests/apps/nest/controllers/calendar_events_test.py(1 hunks)backend/tests/apps/nest/models/reminder_schedule_test.py(2 hunks)backend/tests/apps/nest/schedulers/calendar_events/base_test.py(1 hunks)backend/tests/apps/nest/schedulers/calendar_events/slack_test.py(1 hunks)backend/tests/apps/nest/utils/calendar_events_test.py(1 hunks)backend/tests/apps/owasp/models/event_test.py(2 hunks)backend/tests/apps/slack/commands/set_reminder_test.py(1 hunks)backend/tests/apps/slack/common/handlers/calendar_events_test.py(1 hunks)cspell/custom-dict.txt(2 hunks)docker-compose/local.yaml(3 hunks)docker-compose/production.yaml(1 hunks)docker-compose/staging.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- backend/docker/Dockerfile
✅ Files skipped from review due to trivial changes (2)
- backend/apps/nest/migrations/0011_merge_20250908_0548.py
- backend/apps/nest/migrations/0012_reminderschedule_job_id.py
🚧 Files skipped from review as they are similar to previous changes (34)
- backend/apps/slack/commands/init.py
- backend/apps/nest/migrations/0010_reminderschedule_unique_reminder_schedule.py
- backend/apps/owasp/migrations/0054_merge_20250908_0549.py
- backend/tests/apps/nest/schedulers/calendar_events/base_test.py
- backend/tests/apps/slack/common/handlers/calendar_events_test.py
- backend/apps/nest/migrations/0009_reminder_event.py
- backend/apps/slack/constants.py
- backend/apps/slack/utils.py
- backend/apps/nest/utils/calendar_events.py
- backend/apps/nest/clients/google_calendar.py
- backend/tests/apps/nest/schedulers/calendar_events/slack_test.py
- cspell/custom-dict.txt
- backend/apps/nest/schedulers/calendar_events/base.py
- backend/tests/apps/common/utils_test.py
- backend/tests/apps/nest/models/reminder_schedule_test.py
- backend/apps/slack/actions/home.py
- backend/apps/nest/models/reminder_schedule.py
- backend/tests/apps/nest/clients/google_calendar_test.py
- backend/pyproject.toml
- backend/apps/slack/blocks.py
- backend/tests/apps/nest/controllers/calendar_events_test.py
- backend/apps/nest/models/reminder.py
- backend/apps/owasp/migrations/0053_alter_event_end_date_alter_event_start_date.py
- backend/apps/slack/MANIFEST.yaml
- backend/tests/apps/nest/utils/calendar_events_test.py
- docker-compose/production.yaml
- backend/apps/slack/commands/set_reminder.py
- backend/tests/apps/owasp/models/event_test.py
- docker-compose/local.yaml
- backend/apps/nest/controllers/calendar_events.py
- docker-compose/staging.yaml
- backend/settings/base.py
- backend/apps/nest/schedulers/calendar_events/slack.py
- backend/apps/common/utils.py
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-10T15:32:12.688Z
Learnt from: ahmedxgouda
PR: OWASP/Nest#2211
File: backend/apps/nest/controllers/calendar_events.py:0-0
Timestamp: 2025-09-10T15:32:12.688Z
Learning: In the backend/apps/nest/controllers/calendar_events.py file, the scheduled_time parameter in the schedule_reminder function is guaranteed to be timezone-aware, so explicit timezone awareness validation is not needed.
Applied to files:
backend/apps/slack/common/handlers/calendar_events.pybackend/apps/owasp/models/event.py
📚 Learning: 2025-09-08T07:14:17.047Z
Learnt from: ahmedxgouda
PR: OWASP/Nest#2211
File: backend/apps/slack/common/handlers/calendar_events.py:75-75
Timestamp: 2025-09-08T07:14:17.047Z
Learning: In this Django codebase, ValidationError objects have a .message attribute that can be accessed directly (e.g., e.message) for error handling, as evidenced by consistent usage in backend/apps/core/api/internal/algolia.py and backend/apps/slack/common/handlers/calendar_events.py.
Applied to files:
backend/apps/slack/common/handlers/calendar_events.py
📚 Learning: 2025-09-08T07:11:00.508Z
Learnt from: ahmedxgouda
PR: OWASP/Nest#2211
File: backend/apps/owasp/models/event.py:60-61
Timestamp: 2025-09-08T07:11:00.508Z
Learning: Django DateTimeField accepts date objects and converts them properly during database operations, even though it doesn't perform immediate type conversion at the attribute level.
Applied to files:
backend/apps/owasp/models/event.py
🧬 Code graph analysis (3)
backend/apps/slack/common/handlers/calendar_events.py (6)
backend/apps/slack/blocks.py (2)
get_pagination_buttons(113-149)markdown(21-34)backend/apps/nest/clients/google_calendar.py (2)
GoogleCalendarClient(9-42)get_events(19-36)backend/apps/nest/models/google_account_authorization.py (2)
GoogleAccountAuthorization(28-225)authorize(55-93)backend/apps/owasp/models/event.py (2)
Event(25-375)parse_google_calendar_events(173-183)backend/apps/nest/controllers/calendar_events.py (1)
set_reminder(34-86)backend/apps/nest/schedulers/calendar_events/slack.py (2)
SlackScheduler(7-17)send_message(11-17)
backend/apps/owasp/models/event.py (2)
backend/apps/common/utils.py (1)
parse_date_and_convert_to_local(38-58)backend/apps/owasp/models/chapter.py (2)
generate_suggested_location(128-149)generate_geo_location(116-126)
backend/tests/apps/slack/commands/set_reminder_test.py (2)
backend/apps/slack/blocks.py (1)
markdown(21-34)backend/apps/slack/commands/set_reminder.py (1)
SetReminder(9-24)
🔇 Additional comments (7)
backend/apps/nest/models/google_account_authorization.py (1)
158-172: Credentials property looks good; relies on naive expiry correctly.Minor nit: it assumes
naive_expires_atnever errors; see follow‑up on handlingNone.backend/apps/owasp/models/event.py (4)
21-21: Import additions are appropriate.Bringing in
parse_date_and_convert_to_localaligns with the new parsing helpers.
60-61: DateTimeField migration acknowledged; confirm upstream setters.
from_dictstill assignsdateobjects. Per prior learning, this is acceptable in your codebase, but please confirm tests cover both naive and tz‑aware paths to avoid implicit tz shifts on save.
172-184: Aggregator is clean and efficient.List‑comprehension with generator keeps memory overhead low and filters Nones correctly.
368-374: Conditional enrich on save is sensible.Skipping OpenAI/geocoding for COMMUNITY avoids unnecessary I/O on save.
backend/tests/apps/slack/commands/set_reminder_test.py (1)
10-22: Happy path test reads well and isolates dependencies.Mocks
get_reminder_blockscorrectly and validates the returned blocks.backend/apps/slack/common/handlers/calendar_events.py (1)
41-42: Global numbering avoids pagination collisions.Offset scheme is simple and effective. Nice.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/tests/apps/nest/models/google_account_authorization_test.py (1)
50-56: Reverse O2O access on unsaved instances will error without DB; either persist or drop the reverse assertion.
Accessingself.member.google_account_authorizationtriggers a DB query. Here neitherMembernorGoogleAccountAuthorizationis saved, and the test isn’t marked for DB use, so this will raise instead of testing the relationship.Choose one:
Option A — enable DB and persist:
@@ - def test_one_to_one_relationship(self): + @pytest.mark.django_db + def test_one_to_one_relationship(self): @@ - auth = GoogleAccountAuthorization(member=self.member, access_token=self.valid_token) + self.member.save() + auth = GoogleAccountAuthorization(member=self.member, access_token=self.valid_token) + auth.save() @@ assert self.member.google_account_authorization == auth assert auth.member == self.memberOption B — avoid reverse access (no DB needed):
@@ - assert self.member.google_account_authorization == auth assert auth.member == self.member
🧹 Nitpick comments (3)
backend/tests/apps/nest/models/google_account_authorization_test.py (3)
96-98: Use the correct parameter type and make the KMS error regex tolerant to punctuation.
authorizeexpectsslack_user_id: str; passing aMemberinstance hides type issues.- Error text in code may end with a period; make the regex accept both.
@@ - with pytest.raises(ValueError, match=r"AWS KMS is not enabled"): - GoogleAccountAuthorization.authorize(self.member) + with pytest.raises(ValueError, match=r"AWS KMS is not enabled\.?"): + GoogleAccountAuthorization.authorize(self.member.slack_user_id) @@ - with pytest.raises(ValueError, match=r"AWS KMS is not enabled."): + with pytest.raises(ValueError, match=r"AWS KMS is not enabled\.?"): GoogleAccountAuthorization.refresh_access_token(auth)Also applies to: 239-241
392-405: Patch where used; also set urlparse.query for realism in the callback tests.
- Best practice: patch the symbol where it’s looked up (in
apps.nest.models.google_account_authorization), not the original module.- Setting
querymakes the test intent clearer.@@ - @patch("apps.slack.models.member.Member.objects.get") + @patch("apps.nest.models.google_account_authorization.Member.objects.get") @@ - mock_urlparse.return_value = Mock() + parsed = Mock() + parsed.query = "state=test_state" + mock_urlparse.return_value = parsed - mock_parse_qs.return_value.get.return_value = ["test_state"] + mock_parse_qs.return_value.get.return_value = ["test_state"]
84-86: Import and assert against AUTH_ERROR_MESSAGE (escaped) instead of matching a brittle literal
AUTH_ERROR_MESSAGE is defined in backend/apps/nest/models/google_account_authorization.py; in backend/tests/apps/nest/models/google_account_authorization_test.py import AUTH_ERROR_MESSAGE and re, then replace match=r"Google OAuth client ID" with match=re.escape(AUTH_ERROR_MESSAGE) for the occurrences around lines 84–86, 90–92, 227–229 and 318–320.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/tests/apps/common/model_fileds_test.py(5 hunks)backend/tests/apps/nest/models/google_account_authorization_test.py(8 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/tests/apps/common/model_fileds_test.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-12T17:38:56.165Z
Learnt from: ahmedxgouda
PR: OWASP/Nest#2021
File: backend/apps/slack/models/google_auth.py:39-52
Timestamp: 2025-08-12T17:38:56.165Z
Learning: In the OWASP Nest project, IS_GOOGLE_AUTH_ENABLED in backend/settings/base.py already validates that all three Google OAuth settings (CLIENT_ID, CLIENT_SECRET, REDIRECT_URI) are present and not None/"None"/"", so additional validation in GoogleAuth.get_flow() is redundant.
Applied to files:
backend/tests/apps/nest/models/google_account_authorization_test.py
🧬 Code graph analysis (1)
backend/tests/apps/nest/models/google_account_authorization_test.py (1)
backend/apps/nest/models/google_account_authorization.py (4)
GoogleAccountAuthorization(28-225)get_flow(152-156)authorize(55-93)authorize_callback(96-149)
⏰ 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 (3)
backend/tests/apps/nest/models/google_account_authorization_test.py (3)
29-36: LGTM: creation test reads clearly and asserts core fields.
112-114: LGTM: interpolated rf-string makes the assertion precise without escaping pitfalls.
293-305: LGTM: clear negative path when refresh token is missing.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
backend/apps/slack/common/handlers/calendar_events.py (1)
18-21: Optional: return a clickable “Sign in with Google” button.The markdown link works; a button improves UX. This was suggested previously.
Example actions block to add alongside the message:
[ markdown("*Please sign in with Google first.*"), { "type": "actions", "elements": [ { "type": "button", "text": {"type": "plain_text", "text": "Sign in with Google"}, "url": auth[0], } ], }, ]backend/apps/nest/schedulers/calendar_events/base.py (1)
20-25: Fix enqueue_at: kwargs passed incorrectly; normalize run_at to UTC
enqueue_atexpects target-args viaargs/kwargs. Passingmessage=/channel_id=at this level will raise a TypeError at runtime. Also convert the scheduled time to UTC to avoid DST/drift issues.- self.reminder_schedule.job_id = self.scheduler.enqueue_at( - self.reminder_schedule.scheduled_time, - self.__class__.send_message, - message=self.reminder_schedule.reminder.message, - channel_id=self.reminder_schedule.reminder.channel_id, - ).get_id() + run_at = self.reminder_schedule.scheduled_time.astimezone(timezone.utc) + self.reminder_schedule.job_id = self.scheduler.enqueue_at( + run_at, + self.__class__.send_message, + kwargs={ + "message": self.reminder_schedule.reminder.message, + "channel_id": self.reminder_schedule.reminder.channel_id, + }, + ).get_id()
🧹 Nitpick comments (3)
backend/tests/apps/nest/utils/calendar_events_test.py (1)
53-81: Recurrence date roll-forward tests look solid (daily/weekly/monthly).As a small tidy-up, you could parametrize these three cases to reduce duplication, but not required.
backend/apps/slack/common/handlers/calendar_events.py (2)
28-31: Fix “no parsable events” case.If Google returns items but all are filtered out, the UI still shows a header. Check parsed list only.
- if not (events or parsed_events): + if not parsed_events: return [markdown("*No upcoming calendar events found.*")]
33-38: Include timezone on both range endpoints for consistency.- f"*Your upcoming calendar events from {min_time.strftime('%Y-%m-%d %H:%M')}" + f"*Your upcoming calendar events from {min_time.strftime('%Y-%m-%d %H:%M %Z')}" f" to {max_time.strftime('%Y-%m-%d %H:%M %Z')}*"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
backend/apps/nest/schedulers/calendar_events/base.py(1 hunks)backend/apps/nest/schedulers/calendar_events/slack.py(1 hunks)backend/apps/nest/utils/calendar_events.py(1 hunks)backend/apps/slack/actions/home.py(4 hunks)backend/apps/slack/blocks.py(1 hunks)backend/apps/slack/commands/set_reminder.py(1 hunks)backend/apps/slack/common/handlers/calendar_events.py(1 hunks)backend/apps/slack/constants.py(1 hunks)backend/tests/apps/nest/schedulers/calendar_events/base_test.py(1 hunks)backend/tests/apps/nest/schedulers/calendar_events/slack_test.py(1 hunks)backend/tests/apps/nest/utils/calendar_events_test.py(1 hunks)backend/tests/apps/slack/commands/set_reminder_test.py(1 hunks)backend/tests/apps/slack/common/handlers/calendar_events_test.py(1 hunks)cspell/custom-dict.txt(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- backend/tests/apps/nest/schedulers/calendar_events/base_test.py
- backend/tests/apps/nest/schedulers/calendar_events/slack_test.py
- backend/apps/slack/actions/home.py
- cspell/custom-dict.txt
- backend/apps/slack/commands/set_reminder.py
- backend/apps/nest/schedulers/calendar_events/slack.py
- backend/tests/apps/slack/commands/set_reminder_test.py
- backend/apps/slack/constants.py
- backend/apps/nest/utils/calendar_events.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-10T15:32:12.688Z
Learnt from: ahmedxgouda
PR: OWASP/Nest#2211
File: backend/apps/nest/controllers/calendar_events.py:0-0
Timestamp: 2025-09-10T15:32:12.688Z
Learning: In the backend/apps/nest/controllers/calendar_events.py file, the scheduled_time parameter in the schedule_reminder function is guaranteed to be timezone-aware, so explicit timezone awareness validation is not needed.
Applied to files:
backend/apps/slack/common/handlers/calendar_events.pybackend/apps/nest/schedulers/calendar_events/base.pybackend/tests/apps/nest/utils/calendar_events_test.py
📚 Learning: 2025-09-08T07:14:17.047Z
Learnt from: ahmedxgouda
PR: OWASP/Nest#2211
File: backend/apps/slack/common/handlers/calendar_events.py:75-75
Timestamp: 2025-09-08T07:14:17.047Z
Learning: In this Django codebase, ValidationError objects have a .message attribute that can be accessed directly (e.g., e.message) for error handling, as evidenced by consistent usage in backend/apps/core/api/internal/algolia.py and backend/apps/slack/common/handlers/calendar_events.py.
Applied to files:
backend/apps/slack/common/handlers/calendar_events.py
🧬 Code graph analysis (4)
backend/apps/slack/common/handlers/calendar_events.py (7)
backend/apps/slack/blocks.py (2)
get_pagination_buttons(123-159)markdown(21-34)backend/apps/nest/clients/google_calendar.py (2)
GoogleCalendarClient(9-42)get_events(19-36)backend/apps/nest/models/google_account_authorization.py (2)
GoogleAccountAuthorization(28-225)authorize(55-93)backend/apps/owasp/models/event.py (2)
Event(25-375)parse_google_calendar_events(173-183)backend/apps/nest/models/reminder_schedule.py (1)
ReminderSchedule(6-66)backend/apps/nest/controllers/calendar_events.py (1)
set_reminder(34-86)backend/apps/nest/schedulers/calendar_events/slack.py (2)
SlackScheduler(8-24)send_message(12-18)
backend/tests/apps/slack/common/handlers/calendar_events_test.py (7)
backend/apps/nest/models/google_account_authorization.py (1)
GoogleAccountAuthorization(28-225)backend/apps/nest/models/reminder.py (1)
Reminder(6-33)backend/apps/nest/models/reminder_schedule.py (1)
ReminderSchedule(6-66)backend/apps/slack/common/handlers/calendar_events.py (3)
get_events_blocks(12-60)get_reminders_blocks(63-84)get_setting_reminder_blocks(87-122)backend/apps/slack/common/presentation.py (1)
EntityPresentation(7-25)backend/apps/slack/models/member.py (1)
Member(10-78)backend/apps/nest/schedulers/calendar_events/base.py (2)
schedule(17-46)send_message(55-58)
backend/apps/nest/schedulers/calendar_events/base.py (2)
backend/apps/nest/models/reminder_schedule.py (3)
ReminderSchedule(6-66)Recurrence(20-24)cron_expression(48-62)backend/apps/nest/schedulers/calendar_events/slack.py (2)
send_message(12-18)send_message_and_update(21-24)
backend/tests/apps/nest/utils/calendar_events_test.py (2)
backend/apps/nest/models/reminder_schedule.py (1)
ReminderSchedule(6-66)backend/apps/nest/utils/calendar_events.py (2)
parse_reminder_args(13-51)update_reminder_schedule_date(54-77)
⏰ 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 (9)
backend/tests/apps/nest/utils/calendar_events_test.py (1)
15-27: Good coverage of arg parsing (required + optional).backend/tests/apps/slack/common/handlers/calendar_events_test.py (2)
26-82: Events flow tests are thorough (auth, success, pagination, client calls).
179-216: Reminder setup happy-path assertions (schedule + send) are on point.backend/apps/slack/common/handlers/calendar_events.py (2)
40-43: Event-numbering scheme avoids page collisions. LGTM.Confirm the 1-hour cache TTL is sufficient for typical user flows; consider increasing if feedback shows frequent expirations.
108-111: ValidationError handling aligns with repo convention (.message).backend/apps/slack/blocks.py (1)
98-117: Block Kit limit exceeded — split the "actions" block into two (max 5 buttons per block)File: backend/apps/slack/blocks.py (lines 98–117). The snippet shows 7 buttons in a single "actions" block; Slack only allows 5 elements per actions block — split the two new buttons into a second actions block as shown.
Apply this diff to remove the two buttons from the first actions block and insert a second actions block:
- { - "type": "button", - "text": { - "type": "plain_text", - "text": "Calendar Events", - "emoji": True, - }, - "value": "view_calendar_events", - "action_id": "view_calendar_events_action", - }, - { - "type": "button", - "text": { - "type": "plain_text", - "text": "Reminders", - "emoji": True, - }, - "value": "view_reminders", - "action_id": "view_reminders_action", - },Insert this block immediately after the first actions block:
{ "type": "actions", "elements": [ { "type": "button", "text": {"type": "plain_text", "text": "Calendar Events", "emoji": True}, "value": "view_calendar_events", "action_id": "view_calendar_events_action", }, { "type": "button", "text": {"type": "plain_text", "text": "Reminders", "emoji": True}, "value": "view_reminders", "action_id": "view_reminders_action", }, ], },Sandbox lacked apps/slack so verification couldn't run here. Run locally to confirm handlers are wired and locate usages:
rg -n "view_calendar_events_action|view_reminders_action" -C2backend/apps/nest/schedulers/calendar_events/base.py (3)
33-44: Cron timezone alignment checkYou use
use_local_timezone=Truebutcron_expressionderives hour/minute directly fromscheduled_time. Ensurescheduled_timeis first converted to the scheduler’s local tz before building the expression (e.g.,timezone.localtime(scheduled_time)inReminderSchedule.cron_expression) to avoid hour drift on non-local inputs.
9-16: LGTM on scheduler wiringInitialization and persistence of
job_idlook good.
48-53: Cancel should not delete the entire Reminder; clear job_id (optionally delete only this schedule)Deleting the parent Reminder here cascades to its schedules and may remove unrelated UI references—clear the schedule's job_id and save, or delete the schedule itself if intended.
def cancel(self): """Cancel the scheduled reminder.""" if self.reminder_schedule.job_id: self.scheduler.cancel(self.reminder_schedule.job_id) - self.reminder_schedule.reminder.delete() + self.reminder_schedule.job_id = "" + self.reminder_schedule.save(update_fields=["job_id"]) + # If business logic expects removing this specific schedule: + # self.reminder_schedule.delete()Confirm intended semantics: cancel only this schedule, or delete the whole reminder entity?
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
backend/apps/nest/schedulers/calendar_events/base.py (2)
20-25: enqueue_at kwargs are wrong; pass target kwargs via kwargs= and normalize to UTC.
This will mis-call Scheduler.enqueue_at; also avoid TZ drift by converting to UTC.- self.reminder_schedule.job_id = self.scheduler.enqueue_at( - self.reminder_schedule.scheduled_time, - self.__class__.send_message, - message=self.reminder_schedule.reminder.message, - channel_id=self.reminder_schedule.reminder.channel_id, - ).get_id() + run_at = self.reminder_schedule.scheduled_time.astimezone(timezone.utc) + self.reminder_schedule.job_id = self.scheduler.enqueue_at( + run_at, + self.__class__.send_message, + kwargs={ + "message": self.reminder_schedule.reminder.message, + "channel_id": self.reminder_schedule.reminder.channel_id, + }, + ).get_id()
27-31: timezone.timedelta does not exist; and avoid enqueuing a bound model method.
Use datetime.timedelta and a module-level helper that takes an ID.+from datetime import timedelta @@ - self.scheduler.enqueue_at( - self.reminder_schedule.scheduled_time + timezone.timedelta(minutes=1), - self.reminder_schedule.reminder.delete, - ) + self.scheduler.enqueue_at( + run_at + timedelta(minutes=1), + delete_reminder_by_id, + kwargs={"reminder_id": self.reminder_schedule.reminder_id}, + )Add this helper in the module (outside the class):
from apps.nest.models.reminder import Reminder def delete_reminder_by_id(reminder_id: int) -> None: Reminder.objects.filter(pk=reminder_id).delete()backend/apps/nest/schedulers/calendar_events/slack.py (1)
11-19: Handle Slack API errors and missing app to avoid job failures/silent drops.
Wrap chat_postMessage in try/except and log; warn when SlackConfig.app is None.+import logging +from slack_sdk.errors import SlackApiError + +logger = logging.getLogger(__name__) @@ - def send_message(message: str, channel_id: str): + def send_message(message: str, channel_id: str): """Send message to the specified Slack channel.""" - if app := SlackConfig.app: - app.client.chat_postMessage( - channel=channel_id, - text=message, - ) + if app := SlackConfig.app: + try: + app.client.chat_postMessage(channel=channel_id, text=message) + except SlackApiError as e: + logger.exception("Slack API error posting to %s: %s", channel_id, getattr(e.response, "data", e)) + else: + logger.warning("Slack app not configured; dropping message to %s", channel_id)backend/apps/slack/common/handlers/calendar_events.py (2)
49-50: Fix empty-after-filtering caseIf Google returns items but all are filtered out, nothing is shown. Check
parsed_eventsonly.- if not (events or parsed_events): + if not parsed_events: return [markdown("*No upcoming calendar events found.*")]
38-43: Handle auth exceptions and return a clickable Google sign‑in buttonCatching
ValidationError/ValueErrorprevents 500s; button improves UX over a plain link.- auth = GoogleAccountAuthorization.authorize(slack_user_id) - if not isinstance(auth, GoogleAccountAuthorization): - return [markdown(f"*Please sign in with Google first through this <{auth[0]}|link>*")] - try: - client = GoogleCalendarClient(auth) + try: + auth_result = GoogleAccountAuthorization.authorize(slack_user_id) + except ValidationError as e: + return [markdown(f"*{e.message}*")] + except ValueError as e: + return [markdown(f"*{str(e)}*")] + if not isinstance(auth_result, GoogleAccountAuthorization): + authorization_url, _state = auth_result + return [ + markdown("*Please sign in with Google first.*"), + { + "type": "actions", + "elements": [ + { + "type": "button", + "text": {"type": "plain_text", "text": "Sign in with Google"}, + "url": authorization_url, + } + ], + }, + ] + auth = auth_result + try: + client = GoogleCalendarClient(auth)
🧹 Nitpick comments (9)
backend/tests/apps/slack/commands/cancel_reminder_test.py (1)
16-29: Use a lightweight args object instead of MagicMock.
Prefer types.SimpleNamespace or argparse.Namespace to model parsed args; tighter type-safety and clearer intent.- mock_args = MagicMock() - mock_args.number = 4 + from types import SimpleNamespace + mock_args = SimpleNamespace(number=4)backend/apps/slack/commands/cancel_reminder.py (1)
20-27: Drop redundant int() cast (argparse already returns int).
Keeps call signature clean and eases testing.- return get_cancel_reminder_blocks(int(args.number), self.get_user_id(command)) + return get_cancel_reminder_blocks(args.number, self.get_user_id(command))backend/apps/nest/utils/calendar_events.py (2)
22-50: Validate positive integers for event_number and minutes_before; keep message default as list.
Prevents negative/zero values and parsing surprises.-from argparse import ArgumentParser +from argparse import ArgumentParser, ArgumentTypeError @@ - parser = ArgumentParser(prog="/set-reminder", description="Set a reminder for a Slack event.") + parser = ArgumentParser(prog="/set-reminder", description="Set a reminder for a Slack event.") + + def positive_int(v: str) -> int: + try: + iv = int(v) + except ValueError as e: + raise ArgumentTypeError("must be an integer") from e + if iv <= 0: + raise ArgumentTypeError("must be > 0") + return iv @@ - "--event_number", type=int, help="The event number to set the reminder for.", required=True + "--event_number", type=positive_int, help="The event number to set the reminder for.", required=True @@ - type=int, + type=positive_int,
63-71: Also enforce positive reminder number for cancel command.- parser.add_argument( + from argparse import ArgumentTypeError # if not already imported + + def positive_int(v: str) -> int: + try: + iv = int(v) + except ValueError as e: + raise ArgumentTypeError("must be an integer") from e + if iv <= 0: + raise ArgumentTypeError("must be > 0") + return iv + + parser.add_argument( "--number", - type=int, + type=positive_int, help="The reminder number to cancel.", required=True, )backend/apps/nest/schedulers/calendar_events/base.py (1)
33-44: Verify cron_expression is not None before scheduling.
Guard against accidental None to avoid runtime errors.- else: - self.reminder_schedule.job_id = self.scheduler.cron( + else: + assert self.reminder_schedule.cron_expression, "cron_expression must be set for recurring schedules" + self.reminder_schedule.job_id = self.scheduler.cron( self.reminder_schedule.cron_expression,backend/apps/slack/common/handlers/calendar_events.py (4)
34-37: Also handle Google API HttpError on events fetchAvoids surfacing tracebacks when Google API responds with errors.
from apps.nest.clients.google_calendar import GoogleCalendarClient from apps.nest.models.google_account_authorization import GoogleAccountAuthorization from apps.owasp.models.event import Event + from googleapiclient.errors import HttpError @@ - except ServerNotFoundError: - return [markdown("*Please check your internet connection.*")] + except (ServerNotFoundError, HttpError): + return [markdown("*Please check your internet connection or try again later.*")]Also applies to: 46-47
53-55: Standardize timezone display; include zone on all timestampsUse
timezone.localtime(...)and include%Zconsistently.- f"*Your upcoming calendar events from {min_time.strftime('%Y-%m-%d %H:%M')}" - f" to {max_time.strftime('%Y-%m-%d %H:%M %Z')}*" + f"*Your upcoming calendar events from {timezone.localtime(min_time).strftime('%Y-%m-%d %H:%M %Z')}" + f" to {timezone.localtime(max_time).strftime('%Y-%m-%d %H:%M %Z')}*" @@ - f" at {reminder_schedule.scheduled_time.strftime('%Y-%m-%d %H:%M %Z')}", + f" at {timezone.localtime(reminder_schedule.scheduled_time).strftime('%Y-%m-%d %H:%M %Z')}", @@ - f"{reminder_schedule.scheduled_time.strftime('%Y-%m-%d %H:%M %Z')}" + f"{timezone.localtime(reminder_schedule.scheduled_time).strftime('%Y-%m-%d %H:%M %Z')}"Also applies to: 125-125, 138-138
61-63: Make event-number cache TTL last until the event (with floor), and note shared cache requirementPrevents mappings from expiring before users act; ensure a shared cache backend across workers.
- cache.set(f"{slack_user_id}_{event_number}", event.google_calendar_id, timeout=3600) + now = timezone.now() + ttl_seconds = max(3600, int((event.start_date - now).total_seconds())) + cache.set( + f"{slack_user_id}_{event_number}", + event.google_calendar_id, + timeout=ttl_seconds, + )Operational note: if using locmem cache, this mapping may be missing on the worker handling
/set-reminder. Prefer Redis/Memcached.
26-28: Format channels as mentionsImproves readability in Slack by rendering channel names.
- f"*Canceled the reminder for event '{reminder_schedule.reminder.event.name}'*" - f" in {reminder_schedule.reminder.channel_id}" + f"*Canceled the reminder for event '{reminder_schedule.reminder.event.name}'*" + f" in <#{reminder_schedule.reminder.channel_id}>"- f"{NL}- Channel: {reminder_schedule.reminder.channel_id}" + f"{NL}- Channel: <#{reminder_schedule.reminder.channel_id}>"- f" '{reminder_schedule.reminder.event.name}'*" - f" in {args.channel}" + f" '{reminder_schedule.reminder.event.name}'*" + f" in <#{args.channel}>"Also applies to: 96-97, 136-136
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
backend/apps/nest/schedulers/calendar_events/base.py(1 hunks)backend/apps/nest/schedulers/calendar_events/slack.py(1 hunks)backend/apps/nest/utils/calendar_events.py(1 hunks)backend/apps/slack/MANIFEST.yaml(2 hunks)backend/apps/slack/commands/__init__.py(2 hunks)backend/apps/slack/commands/cancel_reminder.py(1 hunks)backend/apps/slack/common/handlers/calendar_events.py(1 hunks)backend/tests/apps/nest/schedulers/calendar_events/base_test.py(1 hunks)backend/tests/apps/nest/utils/calendar_events_test.py(1 hunks)backend/tests/apps/slack/commands/cancel_reminder_test.py(1 hunks)cspell/custom-dict.txt(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- cspell/custom-dict.txt
🚧 Files skipped from review as they are similar to previous changes (4)
- backend/tests/apps/nest/schedulers/calendar_events/base_test.py
- backend/apps/slack/MANIFEST.yaml
- backend/apps/slack/commands/init.py
- backend/tests/apps/nest/utils/calendar_events_test.py
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-10T15:32:12.688Z
Learnt from: ahmedxgouda
PR: OWASP/Nest#2211
File: backend/apps/nest/controllers/calendar_events.py:0-0
Timestamp: 2025-09-10T15:32:12.688Z
Learning: In the backend/apps/nest/controllers/calendar_events.py file, the scheduled_time parameter in the schedule_reminder function is guaranteed to be timezone-aware, so explicit timezone awareness validation is not needed.
Applied to files:
backend/apps/nest/schedulers/calendar_events/slack.pybackend/apps/slack/common/handlers/calendar_events.pybackend/apps/nest/utils/calendar_events.pybackend/apps/nest/schedulers/calendar_events/base.py
📚 Learning: 2025-09-08T07:14:17.047Z
Learnt from: ahmedxgouda
PR: OWASP/Nest#2211
File: backend/apps/slack/common/handlers/calendar_events.py:75-75
Timestamp: 2025-09-08T07:14:17.047Z
Learning: In this Django codebase, ValidationError objects have a .message attribute that can be accessed directly (e.g., e.message) for error handling, as evidenced by consistent usage in backend/apps/core/api/internal/algolia.py and backend/apps/slack/common/handlers/calendar_events.py.
Applied to files:
backend/apps/slack/common/handlers/calendar_events.py
📚 Learning: 2025-09-17T10:40:27.899Z
Learnt from: ahmedxgouda
PR: OWASP/Nest#2211
File: backend/apps/nest/schedulers/calendar_events/base.py:27-31
Timestamp: 2025-09-17T10:40:27.899Z
Learning: In the OWASP/Nest project, the user ahmedxgouda has confirmed that enqueuing bound model methods (like self.reminder_schedule.reminder.delete) works in their RQ setup in backend/apps/nest/schedulers/calendar_events/base.py, despite potential serialization concerns.
Applied to files:
backend/apps/nest/schedulers/calendar_events/base.py
🧬 Code graph analysis (6)
backend/tests/apps/slack/commands/cancel_reminder_test.py (2)
backend/apps/slack/blocks.py (1)
markdown(21-34)backend/apps/slack/commands/cancel_reminder.py (1)
CancelReminder(7-27)
backend/apps/nest/schedulers/calendar_events/slack.py (3)
backend/apps/nest/schedulers/calendar_events/base.py (3)
BaseScheduler(9-64)send_message(55-58)send_message_and_update(61-64)backend/apps/nest/utils/calendar_events.py (1)
update_reminder_schedule_date(74-103)backend/apps/slack/apps.py (1)
SlackConfig(13-26)
backend/apps/slack/common/handlers/calendar_events.py (8)
backend/apps/slack/blocks.py (2)
get_pagination_buttons(123-159)markdown(21-34)backend/apps/nest/models/reminder_schedule.py (1)
ReminderSchedule(6-66)backend/apps/nest/schedulers/calendar_events/slack.py (2)
SlackScheduler(8-24)send_message(12-18)backend/apps/nest/schedulers/calendar_events/base.py (3)
cancel(48-52)schedule(17-46)send_message(55-58)backend/apps/nest/clients/google_calendar.py (2)
GoogleCalendarClient(9-42)get_events(19-36)backend/apps/nest/models/google_account_authorization.py (2)
GoogleAccountAuthorization(28-225)authorize(55-93)backend/apps/owasp/models/event.py (2)
Event(25-375)parse_google_calendar_events(173-183)backend/apps/nest/controllers/calendar_events.py (1)
set_reminder(34-86)
backend/apps/nest/utils/calendar_events.py (1)
backend/apps/nest/models/reminder_schedule.py (2)
ReminderSchedule(6-66)Recurrence(20-24)
backend/apps/slack/commands/cancel_reminder.py (4)
backend/apps/slack/blocks.py (1)
markdown(21-34)backend/apps/slack/commands/command.py (1)
CommandBase(20-157)backend/apps/nest/utils/calendar_events.py (1)
parse_cancel_reminder_args(53-71)backend/apps/slack/common/handlers/calendar_events.py (1)
get_cancel_reminder_blocks(12-29)
backend/apps/nest/schedulers/calendar_events/base.py (2)
backend/apps/nest/models/reminder_schedule.py (3)
ReminderSchedule(6-66)Recurrence(20-24)cron_expression(48-62)backend/apps/nest/schedulers/calendar_events/slack.py (2)
send_message(12-18)send_message_and_update(21-24)
⏰ 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). (4)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (6)
backend/tests/apps/slack/commands/cancel_reminder_test.py (2)
12-15: LGTM: command name test is correct and stable.
31-40: LGTM: invalid-args path matches production behavior.backend/apps/slack/commands/cancel_reminder.py (1)
11-14: LGTM: explicit command_name avoids default derivation pitfalls.backend/apps/nest/schedulers/calendar_events/slack.py (1)
20-24: LGTM: update hook composes send and schedule update correctly.backend/apps/slack/common/handlers/calendar_events.py (2)
61-61: Event numbering offset (×1000 per page) avoids collisionsNice solution to keep
/set-reminder --event_numberstable across pages.
128-129: Use ofe.messageis consistent with this codebaseAcknowledging prior repo convention; no change needed here.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
backend/apps/slack/common/handlers/calendar_events.py (2)
38-45: Handle auth errors and return a clickable “Sign in with Google” button.Catches
ValidationError/ValueErrorfromauthorizeand improves UX with a Slack button instead of a raw link.Apply:
- auth = GoogleAccountAuthorization.authorize(slack_user_id) - if not isinstance(auth, GoogleAccountAuthorization): - return [markdown(f"*Please sign in with Google first through this <{auth[0]}|link>*")] - try: - client = GoogleCalendarClient(auth) + try: + auth_result = GoogleAccountAuthorization.authorize(slack_user_id) + except ValidationError as e: + return [markdown(f"*{e.message}*")] + except ValueError as e: + return [markdown(f"*{e!s}*")] + if not isinstance(auth_result, GoogleAccountAuthorization): + authorization_url, _state = auth_result + return [ + markdown("*Please sign in with Google first.*"), + { + "type": "actions", + "elements": [ + { + "type": "button", + "text": {"type": "plain_text", "text": "Sign in with Google"}, + "url": authorization_url, + } + ], + }, + ] + auth = auth_result + try: + client = GoogleCalendarClient(auth)
48-50: Fix empty-after-filtering case.If Google returns items but all are filtered out, nothing renders except the header.
Apply:
- if not (events or parsed_events): + if not parsed_events: return [markdown("*No upcoming calendar events found.*")]
🧹 Nitpick comments (7)
backend/apps/slack/common/handlers/calendar_events.py (7)
53-55: Include timezone on both bounds and use localtime.Consistency and correct tz label.
Apply:
- markdown( - f"*Your upcoming calendar events from {min_time.strftime('%Y-%m-%d %H:%M')}" - f" to {max_time.strftime('%Y-%m-%d %H:%M %Z')}*" + markdown( + f"*Your upcoming calendar events from " + f"{timezone.localtime(min_time).strftime('%Y-%m-%d %H:%M %Z')}" + f" to {timezone.localtime(max_time).strftime('%Y-%m-%d %H:%M %Z')}*"
60-63: Event-number namespacing looks good; consider longer cache TTL.The 1000-based offset solves collisions. Consider a longer TTL so users have time to run
/set-reminderafter viewing events.Apply (example 6h):
- cache.set(f"{slack_user_id}_{event_number}", event.google_calendar_id, timeout=3600) + cache.set(f"{slack_user_id}_{event_number}", event.google_calendar_id, timeout=21600)
32-33: Defensive guard for page input.Clamp to 1 to avoid negative/zero pages from malformed payloads.
Apply:
def get_events_blocks(slack_user_id: str, presentation, page: int = 1) -> list[dict]: """Get Google Calendar events blocks for Slack home view.""" + page = max(1, int(page))
24-28: Make channel clickable in Slack.Format channel ID as a Slack mention.
Apply:
- f"*Canceled the reminder for event '{reminder_schedule.reminder.event.name}'*" - f" in {reminder_schedule.reminder.channel_id}" + f"*Canceled the reminder for event '{reminder_schedule.reminder.event.name}'*" + f" in <#{reminder_schedule.reminder.channel_id}>"
97-99: Show reminder time in local tz.Consistency with events display.
Apply:
- f"{NL}- Scheduled Time: " - f"{reminder_schedule.scheduled_time.strftime('%Y-%m-%d, %H:%M %Z')}" + f"{NL}- Scheduled Time: " + f"{timezone.localtime(reminder_schedule.scheduled_time).strftime('%Y-%m-%d, %H:%M %Z')}"
123-127: Localize scheduled times in messages.Aligns with other tz-aware outputs.
Apply:
- f" at {reminder_schedule.scheduled_time.strftime('%Y-%m-%d %H:%M %Z')}", + f" at {timezone.localtime(reminder_schedule.scheduled_time).strftime('%Y-%m-%d %H:%M %Z')}",- f"{reminder_schedule.scheduled_time.strftime('%Y-%m-%d %H:%M %Z')}" + f"{timezone.localtime(reminder_schedule.scheduled_time).strftime('%Y-%m-%d %H:%M %Z')}"Also applies to: 140-140
46-47: Optionally handle Google API HttpError.Network isn’t the only failure mode; HttpError is common.
Apply:
- except ServerNotFoundError: + except ServerNotFoundError: return [markdown("*Please check your internet connection.*")] + except Exception as e: + # Optionally: from googleapiclient.errors import HttpError and branch on it. + return [markdown("*Failed to fetch calendar events. Please try again later.*")]
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/apps/nest/utils/calendar_events.py(1 hunks)backend/apps/slack/common/handlers/calendar_events.py(1 hunks)backend/tests/apps/nest/utils/calendar_events_test.py(1 hunks)backend/tests/apps/slack/common/handlers/calendar_events_test.py(1 hunks)cspell/custom-dict.txt(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- backend/tests/apps/nest/utils/calendar_events_test.py
- backend/tests/apps/slack/common/handlers/calendar_events_test.py
- cspell/custom-dict.txt
- backend/apps/nest/utils/calendar_events.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-10T15:32:12.688Z
Learnt from: ahmedxgouda
PR: OWASP/Nest#2211
File: backend/apps/nest/controllers/calendar_events.py:0-0
Timestamp: 2025-09-10T15:32:12.688Z
Learning: In the backend/apps/nest/controllers/calendar_events.py file, the scheduled_time parameter in the schedule_reminder function is guaranteed to be timezone-aware, so explicit timezone awareness validation is not needed.
Applied to files:
backend/apps/slack/common/handlers/calendar_events.py
📚 Learning: 2025-09-08T07:14:17.047Z
Learnt from: ahmedxgouda
PR: OWASP/Nest#2211
File: backend/apps/slack/common/handlers/calendar_events.py:75-75
Timestamp: 2025-09-08T07:14:17.047Z
Learning: In this Django codebase, ValidationError objects have a .message attribute that can be accessed directly (e.g., e.message) for error handling, as evidenced by consistent usage in backend/apps/core/api/internal/algolia.py and backend/apps/slack/common/handlers/calendar_events.py.
Applied to files:
backend/apps/slack/common/handlers/calendar_events.py
🧬 Code graph analysis (1)
backend/apps/slack/common/handlers/calendar_events.py (8)
backend/apps/slack/blocks.py (2)
get_pagination_buttons(123-159)markdown(21-34)backend/apps/nest/models/reminder_schedule.py (1)
ReminderSchedule(6-66)backend/apps/nest/schedulers/calendar_events/slack.py (2)
SlackScheduler(8-24)send_message(12-18)backend/apps/nest/schedulers/calendar_events/base.py (3)
cancel(48-52)schedule(17-46)send_message(55-58)backend/apps/nest/clients/google_calendar.py (2)
GoogleCalendarClient(9-42)get_events(19-36)backend/apps/nest/models/google_account_authorization.py (2)
GoogleAccountAuthorization(28-225)authorize(55-93)backend/apps/owasp/models/event.py (2)
Event(25-375)parse_google_calendar_events(173-183)backend/apps/nest/controllers/calendar_events.py (1)
set_reminder(34-86)
⏰ 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 (1)
backend/apps/slack/common/handlers/calendar_events.py (1)
71-79: Pagination wiring LGTM.Passing
total_pages=page+1intentionally keeps “Next” available; “Previous” gates onpage > 1.
… is once and update the object to the next scheduled date if recurrence is otherwise and add list_reminders button
ahmedxgouda
force-pushed
the
nestbot-calendar/schedule-event
branch
from
381bf94 to
1764fa9
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (11)
backend/apps/nest/models/reminder.py (1)
14-14: Indexchannel_idfor faster lookups.Likely queried frequently; add an index.
Apply this diff:
- channel_id = models.CharField(verbose_name="Channel ID", max_length=15, default="") + channel_id = models.CharField(verbose_name="Channel ID", max_length=15, default="", db_index=True)docker-compose/production.yaml (6)
22-25: Use exec-form command for better signal handling (PID 1).Avoid sh -c to ensure graceful shutdowns and correct exit codes.
Apply:
- command: > - sh -c ' - python manage.py rqworker - ' + command: ["python", "manage.py", "rqworker"]
32-35: Trim networks to least privilege.Worker likely doesn’t need nest-app-network. Reduces blast radius.
networks: - - nest-app-network - nest-cache-network - nest-db-network
31-35: Graceful shutdown for workers.Add stop_grace_period so long-running jobs get time to finish on deploy/stop.
restart: unless-stopped + stop_grace_period: 60s
37-44: Use exec-form for rqscheduler too.Same rationale as worker.
- command: > - sh -c ' - python manage.py rqscheduler - ' + command: ["python", "manage.py", "rqscheduler"]
50-54: Graceful shutdown for scheduler.Align stop behavior with worker.
restart: unless-stopped + stop_grace_period: 60s
52-55: Limit scheduler networks as well.Drop nest-app-network unless needed.
networks: - - nest-app-network - nest-cache-network - nest-db-networkbackend/apps/nest/schedulers/calendar_events/base.py (2)
33-44: Prefer kwargs over args in cron job for signature safety and readabilityKeeps parity with the ONCE path and avoids positional ordering mistakes.
- self.reminder_schedule.job_id = self.scheduler.cron( - self.reminder_schedule.cron_expression, - func=self.__class__.send_and_update, - args=( - self.reminder_schedule.reminder.message, - self.reminder_schedule.reminder.channel_id, - self.reminder_schedule.pk, - ), - queue_name="default", - use_local_timezone=True, - result_ttl=500, - ).get_id() + self.reminder_schedule.job_id = self.scheduler.cron( + self.reminder_schedule.cron_expression, + func=self.__class__.send_and_update, + kwargs={ + "message": self.reminder_schedule.reminder.message, + "channel_id": self.reminder_schedule.reminder.channel_id, + "reminder_schedule_id": self.reminder_schedule.pk, + }, + queue_name="default", + use_local_timezone=True, + result_ttl=500, + ).get_id()
50-53: Clear job_id after cancel to keep model state consistentif self.reminder_schedule.job_id: self.scheduler.cancel(self.reminder_schedule.job_id) self.reminder_schedule.reminder.delete() + self.reminder_schedule.job_id = "" + self.reminder_schedule.save(update_fields=["job_id"])backend/tests/apps/slack/common/handlers/calendar_events_test.py (1)
56-71: Stabilize time-dependent test data to reduce flakiness.Capture a single now = timezone.now() per test or freeze time to avoid DST/second-boundary drift.
- mock_parse_events.return_value = [ - Event( - name="Test Event 1", - start_date=timezone.now() + timezone.timedelta(hours=1), - end_date=timezone.now() + timezone.timedelta(hours=2), + now = timezone.now() + mock_parse_events.return_value = [ + Event( + name="Test Event 1", + start_date=now + timezone.timedelta(hours=1), + end_date=now + timezone.timedelta(hours=2), google_calendar_id="event1", key="event1", ), Event( name="Test Event 2", - start_date=timezone.now() + timezone.timedelta(hours=3), - end_date=timezone.now() + timezone.timedelta(hours=4), + start_date=now + timezone.timedelta(hours=3), + end_date=now + timezone.timedelta(hours=4), google_calendar_id="event2", key="event2", ), ]backend/apps/slack/actions/home.py (1)
94-103: Calendar events case wired correctly; clamp page >= 1 to avoid “yesterday” on 0/negativesCurrent parsing can yield page=0 for "0" values. Consider clamping to 1 when deriving page (e.g., try/except with max(1, int(value))) near the page parsing.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
backend/poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (43)
backend/apps/common/utils.py(2 hunks)backend/apps/nest/clients/google_calendar.py(1 hunks)backend/apps/nest/handlers/calendar_events.py(1 hunks)backend/apps/nest/migrations/0009_reminder_event.py(1 hunks)backend/apps/nest/migrations/0010_reminderschedule_unique_reminder_schedule.py(1 hunks)backend/apps/nest/migrations/0011_merge_20250908_0548.py(1 hunks)backend/apps/nest/migrations/0012_reminderschedule_job_id.py(1 hunks)backend/apps/nest/migrations/0013_merge_0006_delete_apikey_0012_reminderschedule_job_id.py(1 hunks)backend/apps/nest/models/google_account_authorization.py(1 hunks)backend/apps/nest/models/reminder.py(1 hunks)backend/apps/nest/models/reminder_schedule.py(2 hunks)backend/apps/nest/schedulers/calendar_events/base.py(1 hunks)backend/apps/nest/schedulers/calendar_events/slack.py(1 hunks)backend/apps/nest/utils/calendar_events.py(1 hunks)backend/apps/owasp/migrations/0053_merge_20250918_1659.py(1 hunks)backend/apps/owasp/models/event.py(3 hunks)backend/apps/slack/MANIFEST.yaml(1 hunks)backend/apps/slack/actions/home.py(4 hunks)backend/apps/slack/blocks.py(1 hunks)backend/apps/slack/commands/__init__.py(1 hunks)backend/apps/slack/commands/nestbot.py(1 hunks)backend/apps/slack/common/handlers/calendar_events.py(1 hunks)backend/apps/slack/constants.py(1 hunks)backend/apps/slack/utils.py(1 hunks)backend/docker/Dockerfile(0 hunks)backend/pyproject.toml(2 hunks)backend/settings/base.py(2 hunks)backend/tests/apps/common/model_fileds_test.py(5 hunks)backend/tests/apps/common/utils_test.py(2 hunks)backend/tests/apps/nest/clients/google_calendar_test.py(1 hunks)backend/tests/apps/nest/handlers/calendar_events_test.py(1 hunks)backend/tests/apps/nest/models/google_account_authorization_test.py(8 hunks)backend/tests/apps/nest/models/reminder_schedule_test.py(2 hunks)backend/tests/apps/nest/schedulers/calendar_events/base_test.py(1 hunks)backend/tests/apps/nest/schedulers/calendar_events/slack_test.py(1 hunks)backend/tests/apps/nest/utils/calendar_events_test.py(1 hunks)backend/tests/apps/owasp/models/event_test.py(2 hunks)backend/tests/apps/slack/commands/nestbot_test.py(1 hunks)backend/tests/apps/slack/common/handlers/calendar_events_test.py(1 hunks)cspell/custom-dict.txt(2 hunks)docker-compose/local.yaml(3 hunks)docker-compose/production.yaml(1 hunks)docker-compose/staging.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- backend/docker/Dockerfile
🚧 Files skipped from review as they are similar to previous changes (34)
- backend/apps/nest/migrations/0010_reminderschedule_unique_reminder_schedule.py
- backend/apps/slack/commands/init.py
- docker-compose/staging.yaml
- backend/tests/apps/owasp/models/event_test.py
- backend/apps/owasp/migrations/0053_merge_20250918_1659.py
- backend/apps/nest/migrations/0013_merge_0006_delete_apikey_0012_reminderschedule_job_id.py
- backend/pyproject.toml
- cspell/custom-dict.txt
- backend/apps/nest/migrations/0012_reminderschedule_job_id.py
- backend/tests/apps/nest/models/google_account_authorization_test.py
- backend/tests/apps/nest/handlers/calendar_events_test.py
- backend/tests/apps/slack/commands/nestbot_test.py
- backend/apps/nest/migrations/0009_reminder_event.py
- backend/tests/apps/nest/utils/calendar_events_test.py
- backend/apps/nest/handlers/calendar_events.py
- backend/apps/nest/migrations/0011_merge_20250908_0548.py
- backend/apps/common/utils.py
- backend/apps/owasp/models/event.py
- backend/apps/slack/constants.py
- backend/tests/apps/nest/clients/google_calendar_test.py
- backend/apps/nest/utils/calendar_events.py
- backend/apps/slack/commands/nestbot.py
- backend/apps/slack/common/handlers/calendar_events.py
- backend/apps/slack/MANIFEST.yaml
- backend/tests/apps/nest/schedulers/calendar_events/base_test.py
- backend/tests/apps/nest/models/reminder_schedule_test.py
- backend/apps/nest/models/reminder_schedule.py
- backend/apps/slack/blocks.py
- backend/tests/apps/common/utils_test.py
- backend/tests/apps/nest/schedulers/calendar_events/slack_test.py
- backend/settings/base.py
- backend/tests/apps/common/model_fileds_test.py
- backend/apps/nest/schedulers/calendar_events/slack.py
- backend/apps/slack/utils.py
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-09-10T15:32:12.688Z
Learnt from: ahmedxgouda
PR: OWASP/Nest#2211
File: backend/apps/nest/controllers/calendar_events.py:0-0
Timestamp: 2025-09-10T15:32:12.688Z
Learning: In the backend/apps/nest/controllers/calendar_events.py file, the scheduled_time parameter in the schedule_reminder function is guaranteed to be timezone-aware, so explicit timezone awareness validation is not needed.
Applied to files:
backend/apps/nest/schedulers/calendar_events/base.py
📚 Learning: 2025-09-17T10:40:27.924Z
Learnt from: ahmedxgouda
PR: OWASP/Nest#2211
File: backend/apps/nest/schedulers/calendar_events/base.py:27-31
Timestamp: 2025-09-17T10:40:27.924Z
Learning: In the OWASP/Nest project, the user ahmedxgouda has confirmed that enqueuing bound model methods (like self.reminder_schedule.reminder.delete) works in their RQ setup in backend/apps/nest/schedulers/calendar_events/base.py, despite potential serialization concerns.
Applied to files:
backend/apps/nest/schedulers/calendar_events/base.py
📚 Learning: 2025-09-17T20:16:51.298Z
Learnt from: ahmedxgouda
PR: OWASP/Nest#2211
File: backend/tests/apps/nest/schedulers/calendar_events/base_test.py:42-47
Timestamp: 2025-09-17T20:16:51.298Z
Learning: In the OWASP/Nest project, the RQ scheduler's enqueue_at method accepts job function arguments as direct keyword arguments (e.g., enqueue_at(time, func, message="...", channel_id="...")), not wrapped in a kwargs dictionary.
Applied to files:
backend/apps/nest/schedulers/calendar_events/base.py
📚 Learning: 2025-09-10T16:04:28.502Z
Learnt from: ahmedxgouda
PR: OWASP/Nest#2211
File: docker-compose/production.yaml:21-0
Timestamp: 2025-09-10T16:04:28.502Z
Learning: The RQ worker service (backend-worker) in production requires access to various secrets including the GitHub PEM file for potential GitHub API operations, even though primary tasks like Slack messaging use different credentials (SLACK_BOT_TOKEN, SLACK_SIGNING_SECRET). The worker might handle diverse background tasks beyond calendar/Slack operations that could require GitHub authentication.
Applied to files:
docker-compose/production.yaml
📚 Learning: 2025-09-12T05:01:51.608Z
Learnt from: ahmedxgouda
PR: OWASP/Nest#2211
File: backend/apps/nest/migrations/0012_reminderschedule_job_id.py:12-21
Timestamp: 2025-09-12T05:01:51.608Z
Learning: The reminderschedule table in the OWASP Nest project is currently empty, so migrations adding non-nullable fields don't pose safety risks.
Applied to files:
backend/apps/nest/models/reminder.py
🧬 Code graph analysis (4)
backend/apps/nest/clients/google_calendar.py (1)
backend/apps/nest/models/google_account_authorization.py (2)
GoogleAccountAuthorization(28-225)credentials(159-172)
backend/apps/nest/schedulers/calendar_events/base.py (2)
backend/apps/nest/models/reminder_schedule.py (3)
ReminderSchedule(6-66)Recurrence(20-24)cron_expression(48-62)backend/apps/nest/schedulers/calendar_events/slack.py (2)
send_message(12-18)send_and_update(21-24)
backend/apps/slack/actions/home.py (4)
backend/apps/slack/common/presentation.py (1)
EntityPresentation(7-25)backend/apps/slack/common/handlers/calendar_events.py (2)
get_events_blocks(32-80)get_reminders_blocks(83-104)backend/apps/slack/common/handlers/google_sign_in.py (1)
get_blocks(6-26)backend/apps/slack/blocks.py (1)
markdown(21-34)
backend/tests/apps/slack/common/handlers/calendar_events_test.py (9)
backend/apps/nest/models/google_account_authorization.py (1)
GoogleAccountAuthorization(28-225)backend/apps/nest/models/reminder.py (1)
Reminder(6-33)backend/apps/nest/models/reminder_schedule.py (1)
ReminderSchedule(6-66)backend/apps/owasp/models/event.py (1)
Event(25-377)backend/apps/slack/common/handlers/calendar_events.py (3)
get_events_blocks(32-80)get_reminders_blocks(83-104)get_setting_reminder_blocks(107-144)backend/apps/slack/common/presentation.py (1)
EntityPresentation(7-25)backend/apps/slack/models/member.py (1)
Member(10-78)backend/apps/nest/schedulers/calendar_events/base.py (2)
schedule(17-46)send_message(55-58)backend/apps/nest/schedulers/calendar_events/slack.py (1)
send_message(12-18)
🔇 Additional comments (17)
backend/apps/nest/models/reminder.py (1)
15-21: Optional FK should be optional in forms/admin: addblank=True.Without
blank=True, ModelForms/admin will require an Event even though the DB allows NULL.Apply this diff:
event = models.ForeignKey( "owasp.Event", verbose_name="Event", on_delete=models.SET_NULL, related_name="reminders", null=True, + blank=True, )docker-compose/production.yaml (4)
6-6: Verify entrypoint presence and exec perms.Ensure /home/owasp/entrypoint.sh exists in the backend image and is executable (chmod +x). Also confirm the Dockerfile copies it to that exact path.
7-16: Confirm Compose supports depends_on: condition: service_healthy.This requires a recent Compose implementation. Validate your deployment environment supports it; otherwise containers may start without waiting for health.
26-31: RQ connectivity: verify Redis URL/password are in .env.backend.rqworker must reach Redis with auth. Confirm .env.backend sets something like REDIS_URL=redis://:{$REDIS_PASSWORD}@production-nest-cache:6379/0 (or equivalent settings your app reads).
18-35: Secrets mount removal acknowledged; double-check PEM not needed by worker.Given earlier context that some worker tasks might need GitHub PEM, confirm none of the scheduled/queue tasks in production require it. If needed later, prefer Docker secrets or a scoped credential rather than a host bind mount.
backend/apps/nest/schedulers/calendar_events/base.py (2)
20-25: Time zone check: confirm scheduler expects the same tz as scheduled_time
scheduled_timeis timezone-aware (per controllers). Ensure the scheduler node runs with the same tz for enqueue_at, or normalize to UTC:run_at = self.reminder_schedule.scheduled_time.astimezone(timezone.utc)Then pass
run_attoenqueue_at. Please confirm desired behavior in your deployment.
54-64: Verify subclass hook signatures match exactlyEnsure concrete schedulers (e.g., Slack) implement:
- send_message(message: str, channel_id: str)
- send_and_update(message: str, channel_id: str, reminder_schedule_id: int)
- send_once_and_delete(message: str, channel_id: str, reminder_id: int) [new from above]
Mismatches will surface at runtime when RQ imports the target.
backend/apps/nest/models/google_account_authorization.py (2)
158-172: Credentials property will break if naive_expires_at returns None without proper handling.This is addressed by fixing naive_expires_at above; ensure that change lands with this PR.
181-185: Fix None handling and normalize to UTC before dropping tzinfo (prevents crashes).timezone.make_naive(None) raises; also avoid TZ drift by normalizing to UTC first.
@property def naive_expires_at(self): """Get the naive datetime of token expiry.""" - return timezone.make_naive(self.expires_at) + if not self.expires_at: + return None + dt = self.expires_at + if timezone.is_aware(dt): + return timezone.make_naive(dt, timezone.utc) + return dtbackend/apps/nest/clients/google_calendar.py (2)
19-36: Handle API errors, add light retries, validate time bounds, and persist refreshed tokens.Without this, HttpError will bubble and refreshed tokens won’t be saved.
def get_events(self, min_time=None, max_time=None) -> list[dict]: """Retrieve events from Google Calendar.""" if not min_time: min_time = timezone.now() if not max_time: max_time = min_time + timezone.timedelta(days=1) - events_result = ( - self.service.events() - .list( - calendarId="primary", - timeMin=min_time.isoformat(), - timeMax=max_time.isoformat(), - singleEvents=True, - orderBy="startTime", - ) - .execute() - ) - return events_result.get("items", []) + if min_time and max_time and min_time > max_time: + raise ValueError("min_time must be <= max_time") + try: + events_result = ( + self.service.events() + .list( + calendarId="primary", + timeMin=min_time.isoformat(), + timeMax=max_time.isoformat(), + singleEvents=True, + orderBy="startTime", + ) + .execute(num_retries=2) + ) + # Persist refreshed tokens if google-auth refreshed in-flight + if ( + getattr(self, "_creds", None) + and getattr(self._creds, "token", None) + and self._creds.token != self.google_account_authorization.access_token + ): + expires_at = getattr(self._creds, "expiry", None) + if expires_at and timezone.is_naive(expires_at): + expires_at = timezone.make_aware(expires_at) + self.google_account_authorization.access_token = self._creds.token + if getattr(self._creds, "refresh_token", None): + self.google_account_authorization.refresh_token = self._creds.refresh_token + if expires_at: + self.google_account_authorization.expires_at = expires_at + self.google_account_authorization.save( + update_fields=["access_token", "refresh_token", "expires_at"] + ) + return events_result.get("items", []) + except (HttpError, Exception) as e: + logging.getLogger(__name__).warning("Google Calendar events fetch failed: %s", e) + return []
38-42: Add error handling and retries to get_event; persist refreshed tokens.Prevents unhandled HttpError and keeps tokens in sync.
- return ( - self.service.events().get(calendarId="primary", eventId=google_calendar_id).execute() - ) + try: + event = ( + self.service.events() + .get(calendarId="primary", eventId=google_calendar_id) + .execute(num_retries=2) + ) + if ( + getattr(self, "_creds", None) + and getattr(self._creds, "token", None) + and self._creds.token != self.google_account_authorization.access_token + ): + expires_at = getattr(self._creds, "expiry", None) + if expires_at and timezone.is_naive(expires_at): + expires_at = timezone.make_aware(expires_at) + self.google_account_authorization.access_token = self._creds.token + if getattr(self._creds, "refresh_token", None): + self.google_account_authorization.refresh_token = self._creds.refresh_token + if expires_at: + self.google_account_authorization.expires_at = expires_at + self.google_account_authorization.save( + update_fields=["access_token", "refresh_token", "expires_at"] + ) + return event + except (HttpError, Exception) as e: + logging.getLogger(__name__).warning("Google Calendar get event failed: %s", e) + return {}docker-compose/local.yaml (2)
9-9: LGTM: Single tagged image for all Python services.Using build+image to tag as
nest-local-backendworks well for backend/worker/scheduler reuse.
26-26: Fixed: Redis password env uses correct var.Good correction from HOST→PASSWORD.
backend/apps/slack/actions/home.py (4)
11-18: Import of calendar_events handler — OKMatches the new handlers usage below.
111-112: Reminders case — OKHandler call and signature match.
143-146: Action registry updated — OKNew actions correctly registered.
Also applies to: 149-149
22-24: Approve — constants defined & uniqueDefined in backend/apps/slack/constants.py (lines 29–31, 37); no duplicate action string values found; no existing usages across the repo.
| self.reminder_schedule.job_id = self.scheduler.enqueue_at( | ||
| self.reminder_schedule.scheduled_time, | ||
| self.__class__.send_message, | ||
| message=self.reminder_schedule.reminder.message, | ||
| channel_id=self.reminder_schedule.reminder.channel_id, | ||
| ).get_id() | ||
|
|
||
| # Schedule deletion of the reminder after sending the message | ||
| self.scheduler.enqueue_at( | ||
| self.reminder_schedule.scheduled_time + timezone.timedelta(minutes=1), | ||
| self.reminder_schedule.reminder.delete, | ||
| ) |
There was a problem hiding this comment.
Blocker: Don’t pre-schedule deletion; tie cleanup to the send job (also removes invalid timezone.timedelta)
- If the ONCE job is canceled or fails, the separately scheduled deletion will still run and delete the reminder.
timezone.timedeltadoes not exist; this would crash if reached.
Replace the ONCE path with a single job that sends then deletes:
- self.reminder_schedule.job_id = self.scheduler.enqueue_at(
- self.reminder_schedule.scheduled_time,
- self.__class__.send_message,
- message=self.reminder_schedule.reminder.message,
- channel_id=self.reminder_schedule.reminder.channel_id,
- ).get_id()
-
- # Schedule deletion of the reminder after sending the message
- self.scheduler.enqueue_at(
- self.reminder_schedule.scheduled_time + timezone.timedelta(minutes=1),
- self.reminder_schedule.reminder.delete,
- )
+ self.reminder_schedule.job_id = self.scheduler.enqueue_at(
+ self.reminder_schedule.scheduled_time,
+ self.__class__.send_once_and_delete,
+ message=self.reminder_schedule.reminder.message,
+ channel_id=self.reminder_schedule.reminder.channel_id,
+ reminder_id=self.reminder_schedule.reminder_id,
+ ).get_id()Add a static hook to be implemented by subclasses:
@staticmethod
def send_once_and_delete(message: str, channel_id: str, reminder_id: int):
"""Send the message, then delete the reminder (one-shot). Implement in subclass."""
raise NotImplementedError("Subclasses must implement this method.")🤖 Prompt for AI Agents
In backend/apps/nest/schedulers/calendar_events/base.py around lines 20 to 31,
the code pre-schedules a separate deletion job (and uses the non-existent
timezone.timedelta) which will delete reminders even if the send job is canceled
or fails; instead remove the separate enqueue_at for deletion and replace the
ONCE scheduling with a single job that calls a static
send_once_and_delete(message, channel_id, reminder_id) method on the class so
the job both sends and deletes atomically; add the static method signature to
the base class as an abstract/NotImplementedError placeholder so subclasses
implement the actual send-and-delete behavior, pass reminder id to the job, and
set self.reminder_schedule.job_id from that single enqueue_at call (and remove
any timezone.timedelta usage).
| return "".join(segments) | ||
|
|
||
|
|
||
| def parse_date(date_string: str | None) -> datetime | None: |
There was a problem hiding this comment.
This one should be on its place in this file
| def schedule_reminder( | ||
| reminder: Reminder, | ||
| scheduled_time: timezone.datetime, | ||
| recurrence=ReminderSchedule.Recurrence.ONCE, | ||
| ) -> ReminderSchedule: | ||
| """Schedule a reminder.""" | ||
| if scheduled_time < timezone.now(): | ||
| message = "Scheduled time must be in the future." | ||
| raise ValidationError(message) | ||
| if recurrence not in ReminderSchedule.Recurrence.values: | ||
| message = "Invalid recurrence value." | ||
| raise ValidationError(message) | ||
| return ReminderSchedule.objects.create( | ||
| reminder=reminder, | ||
| scheduled_time=scheduled_time, | ||
| recurrence=recurrence, | ||
| ) |
There was a problem hiding this comment.
Your single piece of text functions are still hard to read.
There was a problem hiding this comment.
Maybe adding comments will improve this?
| if minutes_before <= 0: | ||
| message = "Minutes before must be a positive integer." | ||
| raise ValidationError(message) | ||
| auth = GoogleAccountAuthorization.authorize(user_id) | ||
| if not isinstance(auth, GoogleAccountAuthorization): | ||
| message = "User is not authorized with Google. Please sign in first." | ||
| raise ValidationError(message) | ||
| google_calendar_id = cache.get(f"{user_id}_{event_number}") | ||
| if not google_calendar_id: | ||
| message = ( | ||
| "Invalid or expired event number. Please get a new event number from the events list." | ||
| ) | ||
| raise ValidationError(message) |
There was a problem hiding this comment.
This one has some splitting but still could use better partitioning.
| python manage.py migrate && | ||
| python manage.py runserver 0.0.0.0:8000 | ||
| ' | ||
| image: nest-local-backend |
| sh -c ' | ||
| python manage.py rqworker | ||
| ' | ||
| image: nest-local-backend |
There was a problem hiding this comment.
Locally we mount code as a volume, why do you add image: ?
There was a problem hiding this comment.
To not build the same image 3 times. Backend, worker, and scheduler have the same build context. The only difference between them is the entrypoint, and a container depends on an image by default, so either we refer an image that the container use or a build context, and this is the same image for all three services. This results: building the image one time and using it in each service.
|
|
||
| USER owasp | ||
|
|
||
| CMD ["/home/owasp/entrypoint.sh"] |
There was a problem hiding this comment.
This is technically a CMD even though named weirdly. I think it'd be better to fix the name to something like run_backend.py and create similar scripts for worker/scheduler. We have some mix up of cmd/entrypoint attributes here.
arkid15r
merged commit 8931916
into
OWASP:feature/nestbot-calendar-events
* Add Google Calendar Client * Parse events * Add event to reminder * Add tests for client and parsing events * Add viewing events button * Apply check spelling * Add reminder blocks and setting a reminder * Refactor * Apply sonar * Add command to manifest and fix bugs * Add constraint and remove null possibility from event * Add RQ * Refactor * Merge migrations * Refactor * Move parse_args to nest app * Update calendar events blocks * Use user_id as an argument name instead of slack_user_id * Add rq to settings * Update event tests * Add rq scheduler and add base scheduler * Fix scheduling * Add worker and scheduler to docker compose * Refactor * Apply check spelling * Apply rabbit's suggestions * Add tests * Apply sonar suggestions * Apply check spelling * Update event number * Optimize docker compose * Add job id to reminder schedule model and cancel method to the base scheduler * Update poetry lock * Update tests * Merge migrations * Update tests * Clean up reminder schedule after completing the job if the recurrence is once and update the object to the next scheduled date if recurrence is otherwise and add list_reminders button * Update tests * Apply check-spelling * Add cancel reminder command and update update_reminder_schedule_date function * Fix schedule month update * Fix tests * Add tests * Apply rabbit's suggestions * Apply suggestions * Update tests * Clean up migrations * Add atomic transaction to set_reminder handler * Update poetry.lock
* Add Google Calendar Client * Parse events * Add event to reminder * Add tests for client and parsing events * Add viewing events button * Apply check spelling * Add reminder blocks and setting a reminder * Refactor * Apply sonar * Add command to manifest and fix bugs * Add constraint and remove null possibility from event * Add RQ * Refactor * Merge migrations * Refactor * Move parse_args to nest app * Update calendar events blocks * Use user_id as an argument name instead of slack_user_id * Add rq to settings * Update event tests * Add rq scheduler and add base scheduler * Fix scheduling * Add worker and scheduler to docker compose * Refactor * Apply check spelling * Apply rabbit's suggestions * Add tests * Apply sonar suggestions * Apply check spelling * Update event number * Optimize docker compose * Add job id to reminder schedule model and cancel method to the base scheduler * Update poetry lock * Update tests * Merge migrations * Update tests * Clean up reminder schedule after completing the job if the recurrence is once and update the object to the next scheduled date if recurrence is otherwise and add list_reminders button * Update tests * Apply check-spelling * Add cancel reminder command and update update_reminder_schedule_date function * Fix schedule month update * Fix tests * Add tests * Apply rabbit's suggestions * Apply suggestions * Update tests * Clean up migrations * Add atomic transaction to set_reminder handler * Update poetry.lock
2 participants
Proposed change
Resolves #2175
Add the PR description here.
Checklist
make check-testlocally; all checks and tests passed.