Add feature: For cancelled event won't ping for chanel #82

zangjiucheng · 2025-02-15T13:22:39Z

This pull request introduces several enhancements and new features to the Minerva project, including updates to documentation, additions to the CalendarEvent class, and improvements to event reminders. The most important changes include adding a cancelled status to calendar events, updating the README file with usage and development guides, and modifying event reminders to account for cancelled events.

Documentation Updates:

README.md: Added links to usage guides and development onboarding and reference guides for Minerva.

CalendarEvent Class Enhancements:

src/classes/CalendarEvent.ts: Added a cancelled property to the CalendarEvent class to indicate if an event is cancelled. [1] [2] [3]

Event Reminder Improvements:

src/utils/eventReminders.ts: Modified the generateEventReminderChannelText function to exclude reminders for cancelled events.

Test Updates:

tests/fixtures/googleCalendarEvent.json: Updated test fixture to include a cancelled event.
tests/fixtures/googleCalendarEvents.json: Updated test fixture to include a cancelled event.
tests/unit/classes/CalendarEvent.test.ts: Added tests to verify the handling of cancelled events in the CalendarEvent class. [1] [2]
tests/unit/utils/googleCalendar.test.ts: Added tests to verify the parsing of cancelled events.

This change is

QuantumManiac · 2025-02-15T16:44:59Z

Please refer to the repo's CONTRIBUTING.md for details on contributing to minerva, e.g. making feature branches and testing your functionality in the development Slack/GCal. You should ideally not be committing to the development branch directly as you'll be triggering a broken deploy every time you commit to your feature until it's complete.

README.md: Added links to usage guides and development onboarding and reference guides for Minerva.

What did you change? I don't see any changes to it in this PR and they already exist in there anyway.

src/classes/CalendarEvent.ts

QuantumManiac · 2025-02-15T16:34:35Z

src/classes/CalendarEvent.ts

      throw new Error("Event summary is undefined");
    }
    const title = event.summary;
+    const cancelled = title.includes("CANCELLED");


Nit:

This should probably be case-insensitive? If not, be sure to explicitly mention it in the documentation

Hi! This should be case-sensitive by behavior requested :) (But nice checking)

Maybe I should switch to case-insensitve which seems make more sense. Thanks for reminding this!

QuantumManiac · 2025-02-15T16:34:37Z

src/classes/CalendarEvent.ts


    parsedEvent.location = event.location ?? undefined;
    parsedEvent.url = event.htmlLink ?? undefined;
+    parsedEvent.cancelled = cancelled;


Use the constructor to pass the cancelled status instead (see Line 69)

This might be good to do for location and url as well, unless there isn't a good reason to do so

This is a good idea! I switch to use constructor with is_cancelled.

I didn't change location and url, not sure if other behavior on it.

QuantumManiac · 2025-02-15T16:37:51Z

tests/fixtures/googleCalendarEvents.json

      "updated": "2023-12-07T15:33:49.949Z",
      "location": "The Bay",
-      "summary": "Minerva Testing",
+      "summary": "[CANCELLED] Minerva Testing",


See comment re: unittesting

QuantumManiac · 2025-02-15T16:38:00Z

tests/unit/classes/CalendarEvent.test.ts

      const calendarEvent = CalendarEvent.fromGoogleCalendarEvent(googleCalendarEvent, slackChannels);
      expect(calendarEvent).toEqual({
        title: googleCalendarEvent.summary,
+        cancelled: true,


See comment re: unittesting

QuantumManiac · 2025-02-15T16:38:05Z

tests/unit/classes/CalendarEvent.test.ts

      const calendarEvent = CalendarEvent.fromGoogleCalendarEvent(calendarEventJson, slackChannels);
      expect(calendarEvent).toEqual({
        title: googleCalendarEvent.summary,
+        cancelled: true,


See comment re: unittesting

QuantumManiac · 2025-02-15T16:38:13Z

tests/unit/utils/googleCalendar.test.ts

      expect(parseEvents(googleCalendarEvents, slackChannels)).toEqual([
        {
          title: googleCalendarEvent.summary,
+          cancelled: true,


See comment re: unittesting

QuantumManiac

One of my review comments magically disappeared. See below.

QuantumManiac · 2025-02-15T16:49:33Z

tests/fixtures/googleCalendarEvent.json

  "updated": "2023-12-07T15:33:49.949Z",
  "location": "The Bay",
-  "summary": "Minerva Testing",
+  "summary": "[CANCELLED] Minerva Testing",


You shoud add more unittests to test the cancelled functionality instead of modifying existing ones to test it. Additionally, refrain from modifying the test fixture like this and change the summary to your liking from within the unittest (e.g. calendarEventJson.summary = "[CANCELLED] Minerva Testing" in whatever unittest tests the cancel functionality.

Sounds good! Thanks! I am not familiar with this project. Defiantly I will make more test on this project 👍

QuantumManiac · 2025-02-15T17:11:34Z

Please also update the Documentation for this feature as well.

…d logic and tests

Enhance from dev branch PR for Cancelled Function

zangjiucheng · 2025-02-16T17:26:35Z

Thanks for your detailed review :) I did fix everything, please double check before we merge this to production environment

QuantumManiac

LGTM so far. Just one more thing that might be nice to have.

src/utils/eventReminders.ts

Add Cancelled Text

QuantumManiac

Some fixing to the wording to the reminder message and you should be good to go.

EDIT: It seems that my code suggestion decided not to be included in the review. See below instead.

The wording still seems a bit awkward when you just append "has been cancelled" to generateEventReminderChannelText. How about:

export function generateEventReminderChannelText(event: CalendarEvent, reminderType: EventReminderType): string {
  let message = `${
    !event.is_cancelled &&
    (reminderType == EventReminderType.FIVE_MINUTES || reminderType == EventReminderType.MANUAL_PING)
      ? "<!channel>\n"
      : ""
  }Reminder: *${event.title}*`;

  let timeUntilEventMessage: string;

  if (reminderType === EventReminderType.FIVE_MINUTES) {
    const timeUntilEvent = event.start.getTime() - new Date().getTime();
    timeUntilEventMessage = ` in *${Math.ceil(timeUntilEvent / 1000 / 60)} minutes*`;
  } else {
    timeUntilEventMessage = ` at *${moment(event.start).tz("America/Toronto").format("MMMM Do, YYYY [at] h:mm A")}*`;
  }

  if (event.is_cancelled) {
    message += ` that was supposed to occur ${timeUntilEventMessage} *has been cancelled*`;
  } else {
    message += ` is occurring ${timeUntilEventMessage}`;
  }
...

refactor: simplify event reminder message construction

QuantumManiac

LGTM

zangjiucheng · 2025-02-20T02:02:27Z

Thanks so much for your review :)

shirleyfyx and others added 5 commits April 16, 2024 20:08

New branch for readme update

ffdd4df

Update README.md

8b83597

Update README.md

8b2f9f3

Add cancelled status to CalendarEvent and update reminder logic

6467484

Mark events as cancelled in test fixtures and update related tests

345d658

zangjiucheng requested review from a team, 0foxes, PCModeActivate, Panizghi, QuantumManiac, billyao021031, jeffrey-huang-yz, shirleyfyx, wxp02 and yxyyeah as code owners February 15, 2025 13:22

Merge branch 'main' into development

adcdc3e

zangjiucheng temporarily deployed to development February 15, 2025 13:23 — with GitHub Actions Inactive

QuantumManiac requested changes Feb 15, 2025

View reviewed changes

QuantumManiac reviewed Feb 15, 2025

View reviewed changes

zangjiucheng added 2 commits February 16, 2025 12:18

Refactor CalendarEvent to use is_cancelled property and update relate…

56c2ec7

…d logic and tests

Merge pull request #84 from waterloo-rocketry/j7zang/81-cancel-handle

f38a386

Enhance from dev branch PR for Cancelled Function

zangjiucheng temporarily deployed to development February 16, 2025 17:21 — with GitHub Actions Inactive

Merge branch 'main' into development

9cf6991

zangjiucheng temporarily deployed to development February 16, 2025 17:25 — with GitHub Actions Inactive

zangjiucheng requested a review from QuantumManiac February 16, 2025 17:26

QuantumManiac requested changes Feb 16, 2025

View reviewed changes

src/utils/eventReminders.ts Outdated Show resolved Hide resolved

Add cancellation message to event reminder text

a615962

Merge pull request #85 from waterloo-rocketry/j7zang/enhance-message

5d82c50

Add Cancelled Text

zangjiucheng temporarily deployed to development February 16, 2025 18:27 — with GitHub Actions Inactive

zangjiucheng requested review from QuantumManiac and removed request for 0foxes, PCModeActivate, Panizghi, billyao021031, jeffrey-huang-yz, shirleyfyx, wxp02 and yxyyeah February 16, 2025 18:27

QuantumManiac requested changes Feb 17, 2025

View reviewed changes

zangjiucheng added 2 commits February 17, 2025 19:29

refactor: simplify event reminder message construction

0f0f531

Merge pull request #86 from waterloo-rocketry/j7zang/enhance-message

7567a00

refactor: simplify event reminder message construction

zangjiucheng deployed to development February 18, 2025 00:34 — with GitHub Actions Active

zangjiucheng requested a review from QuantumManiac February 18, 2025 03:19

QuantumManiac approved these changes Feb 20, 2025

View reviewed changes

zangjiucheng merged commit 9dccc5a into main Feb 20, 2025
1 check passed

Add feature: For cancelled event won't ping for chanel #82

Add feature: For cancelled event won't ping for chanel #82

Uh oh!

Conversation

zangjiucheng commented Feb 15, 2025 • edited by QuantumManiac Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Documentation Updates:

CalendarEvent Class Enhancements:

Event Reminder Improvements:

Test Updates:

Uh oh!

QuantumManiac commented Feb 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

QuantumManiac Feb 15, 2025

Choose a reason for hiding this comment

Uh oh!

zangjiucheng Feb 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zangjiucheng Feb 16, 2025

Choose a reason for hiding this comment

Uh oh!

QuantumManiac Feb 15, 2025

Choose a reason for hiding this comment

Uh oh!

zangjiucheng Feb 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QuantumManiac Feb 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QuantumManiac Feb 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QuantumManiac Feb 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QuantumManiac Feb 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QuantumManiac left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QuantumManiac Feb 15, 2025

Choose a reason for hiding this comment

Uh oh!

zangjiucheng Feb 15, 2025

Choose a reason for hiding this comment

Uh oh!

zangjiucheng Feb 16, 2025

Choose a reason for hiding this comment

Uh oh!

QuantumManiac commented Feb 15, 2025

Uh oh!

zangjiucheng commented Feb 16, 2025

Uh oh!

QuantumManiac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

QuantumManiac left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QuantumManiac left a comment

Choose a reason for hiding this comment

Uh oh!

zangjiucheng commented Feb 20, 2025

Uh oh!

Uh oh!

Reviewers

zangjiucheng commented Feb 15, 2025 •

edited by QuantumManiac

Loading

QuantumManiac commented Feb 15, 2025 •

edited

Loading

zangjiucheng Feb 16, 2025 •

edited

Loading

zangjiucheng Feb 16, 2025 •

edited

Loading

QuantumManiac Feb 15, 2025 •

edited

Loading

QuantumManiac Feb 15, 2025 •

edited

Loading

QuantumManiac Feb 15, 2025 •

edited

Loading

QuantumManiac Feb 15, 2025 •

edited

Loading

QuantumManiac left a comment •

edited

Loading

QuantumManiac left a comment •

edited

Loading