Send ical Notifications (Version 2) #623

paulandm · 2024-10-01T23:36:03Z

A refactored / cleaned up version of the original PR for the Send ical notifications task.

The notifications are now based on the zoom event record (instead of the zoom record).
Was able to incorporate the 'zoom_helper_icalendar_event' function in the creation of the ical event.
The buffer of 10 minutes before sending out invites remain in place. The idea is still that an ical invite should only be sent once per Zoom event. Since we don't have access to the participants' calendars, we can't actually update already sent ical invites. So if we change the code to send ical invites for a Zoom activity edit action (after the 10 minute buffer period), it will send an additional ical invite instead of actually changing the previous one sent. This might be appropriate behavior for some, but it is not applicable in our use case. @jrchamp I'm happy to discuss some ideas around this 'update' scenario further if required.
The buffer period of 10 minutes could also potentially move out to a setting if preferred?

Add initial send ical notification changes.

Corrected Moodle styling errors.

Corrected function documentation.

Remove registration validation feature from this branch

Revert file to original state

Code cleanup of send_ical_notifications task

Base ical notifications task on zoom event ids

Move the ical invite buffer back to 10 minutes

Fix Moodle Code and PHPDoc Checker errors

Fix Moodle Code Checker warning

Add missing messageprovider string

Moodle 3.9 compatibility: Removed 'MESSAGE_DEFAULT_ENABLED' constant

Moodle 3.9 compatibility fixes.

This reverts commit c99249a.

db/upgrade.php

Move ical notifications table to new savepoint block

Update version number to match upgrade save point Fix descriptions of zoom_ical_notifications table in the install file.

Updated from main, adjusted version number

jrchamp

Hi @paulandm! Thank you for all your work on this. I had a few ideas that took a while to collect. I may have missed some things, so apologies if I have more comments later on items that were already present in this version of the code.

lang/en/zoom.php

db/upgrade.php

db/tasks.php

db/install.xml

classes/task/send_ical_notifications.php

jrchamp · 2024-10-30T21:35:30Z

classes/task/send_ical_notifications.php

+        if ($zoom->registration == ZOOM_REGISTRATION_OFF) {
+            $icalevent->add_property('location', $zoom->join_url);
+        } else {
+            $registrantjoinurl = zoom_get_registrant_join_url($user->email, $zoom->meeting_id, $zoom->webinar);
+            if ($registrantjoinurl) {
+                $icalevent->add_property('location', $registrantjoinurl);
+            } else {
+                $icalevent->add_property('location', $zoom->join_url);
+            }
+        }


Instead of providing the stored join_url, it's probably better to send the user to the Moodle site's join URL. That way they won't circumvent the authentication/logging mechanisms or be left with an outdated/cached version of the URL.

@jrchamp Would it be a good idea if we maybe add a feature to the mod/zoom/view.php page to include a optional parameter like 'autojoin', so that the authenticated user is automatically forwarded to the zoom url?

Implement PR feedback

paulandm · 2024-11-11T18:32:26Z

Implemented the PR feedback.

Implemented PR feedback

jrchamp

Thanks @paulandm! We'll take a closer look at this updated version during our next meeting. There are a few comments from last time, like the MySQL-specific SQL that will need to be addressed before it can be merged, but I'm excited to see the progress.

db/upgrade.php

jrchamp · 2024-11-11T22:53:18Z

db/install.xml

@@ -184,5 +184,17 @@
        <KEY NAME="fk_breakoutroomid" TYPE="foreign" FIELDS="breakoutroomid" REFTABLE="zoom_meeting_breakout_rooms" REFFIELDS="id"/>
      </KEYS>
    </TABLE>
+    <TABLE NAME="zoom_ical_notifications" COMMENT="Identifies the zoom event for which ical notifications have been emailed.">


Note to self: Generate this using XMLDB and export it using XMLDB. Check for differences.

Implemented PR Feedback related to DB

paulandm · 2024-11-12T08:54:28Z

Thanks @jrchamp I've implemented some improvements related to DB compatibility and have simplified the keys of the table.

jrchamp · 2024-11-14T16:22:58Z

classes/task/send_ical_notifications.php

+            $messagedata->subject = $zoomevent->name;
+            $messagedata->fullmessage = $zoomevent->description;
+            $messagedata->fullmessageformat = FORMAT_HTML;
+            $messagedata->fullmessagehtml = $zoomevent->description;
+            $messagedata->smallmessage = $zoomevent->name . ' - ' . $zoomevent->description;


Note to tester: Do we need to apply filters like in #615 so the messages reflect the displayed values for the activities? Do the event names and descriptions already have the post-processed value or do they need to have the filter applied? There is a helper function for the activity name, but testing would be needed to determine if the description gets automatically passed through filters or not or if we need to supply the context.

jrchamp · 2024-11-14T16:24:33Z

classes/task/send_ical_notifications.php

+            $messagedata = new \core\message\message();
+            $messagedata->component = 'mod_zoom';


Note to reviewer: Compare to core Moodle to see if this is the recommended pattern for initializing the object.

jrchamp · 2024-11-14T16:26:37Z

classes/task/send_ical_notifications.php

+                'contextid' => \context_user::instance($user->id)->id,
+                'component' => 'user',
+                'filearea' => 'draft',
+                'itemid' => file_get_unused_draft_itemid(),
+                'filepath' => '/',
+                'filename' => clean_filename('icalexport.ics'),


Reviewer: Determine if these is a concern about unique filenames. Do/should these files get cleaned up after use?

jrchamp · 2024-11-14T16:28:38Z

classes/task/send_ical_notifications.php

+            // Check if user has "Disable notifications" set.
+            if ($user->emailstop) {
+                continue;
+            }


Reviewer: Is emailstop intended to stop all messages or only emails? Are we supposed to be doing this check, or is this supposed to be handled by the messaging system itself?

jrchamp · 2024-11-14T16:31:48Z

classes/task/send_ical_notifications.php

+     * @param string $zoomeventid The zoom event id.
+     */
+    private function set_notification_executiontime(string $zoomeventid) {


Reviewer: I would fully expect this to be an integer. Maybe it depends on the database and PHP versions, but that would mean this will break on some systems. Probably best to cast as (int) before passing to this function and update the parameter types.

jrchamp · 2024-11-14T16:32:17Z

classes/task/send_ical_notifications.php

+     * @param string $zoomeventid The zoom event id.
+     * @return string The timestamp of the last execution.
+     */
+    private function get_notification_executiontime(string $zoomeventid) {


Reviewer: Same concern about parameter types (should probably cast to `(int)``)

paulandm added 11 commits September 30, 2024 21:19

Add initial send ical notification changes.

7d91031

Add initial send ical notification changes.

Corrected Moodle styling errors.

c5a07e3

Corrected Moodle styling errors.

Corrected function documentation.

f6a8f7f

Corrected function documentation.

Remove registration validation feature from this branch

3dca4b7

Remove registration validation feature from this branch

Revert file to original state

3b80e0e

Revert file to original state

Code cleanup of send_ical_notifications task

c918384

Code cleanup of send_ical_notifications task

Base ical notifications task on zoom event ids

708b379

Base ical notifications task on zoom event ids

Move the ical invite buffer back to 10 minutes

3d39c40

Move the ical invite buffer back to 10 minutes

Fix Moodle Code and PHPDoc Checker errors

079efb4

Fix Moodle Code and PHPDoc Checker errors

Fix Moodle Code Checker warning

7fd8e0d

Fix Moodle Code Checker warning

Add missing messageprovider string

e7d6de2

Add missing messageprovider string

paulandm mentioned this pull request Oct 2, 2024

Send iCal notifications task #452

Closed

paulandm added 3 commits October 2, 2024 02:27

Moodle 3.9 compatibility: Removed 'MESSAGE_DEFAULT_ENABLED' constant

29cfee1

Moodle 3.9 compatibility: Removed 'MESSAGE_DEFAULT_ENABLED' constant

Moodle 3.9 compatibility fixes.

c99249a

Moodle 3.9 compatibility fixes.

Revert "Moodle 3.9 compatibility fixes."

1c70ce7

This reverts commit c99249a.

paulandm commented Oct 2, 2024

View reviewed changes

db/upgrade.php Outdated Show resolved Hide resolved

smbader requested a review from jrchamp October 3, 2024 15:39

paulandm added 2 commits October 3, 2024 19:12

Move ical notifications table to new savepoint block

46ce080

Move ical notifications table to new savepoint block

Update version number & fix install table descriptions

acc7ce8

Update version number to match upgrade save point Fix descriptions of zoom_ical_notifications table in the install file.

jrchamp added the enhancement Adds new functionality label Oct 10, 2024

paulandm added 2 commits October 17, 2024 18:15

Merge branch 'main' into feature/ical_notifications

bac9230

Updated from main, adjusted version number

4ac56e6

Updated from main, adjusted version number

jrchamp reviewed Oct 30, 2024

View reviewed changes

Implement PR feedback

31ae11c

Implement PR feedback

Implemented PR feedback

4713350

Implemented PR feedback

jrchamp reviewed Nov 11, 2024

View reviewed changes

Implemented PR Feedback related to DB

8a098fd

Implemented PR Feedback related to DB

jrchamp reviewed Nov 14, 2024

View reviewed changes

jrchamp assigned sgrandh3 and armccoy Nov 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Send ical Notifications (Version 2) #623

Send ical Notifications (Version 2) #623

paulandm commented Oct 1, 2024 •

edited

Loading

jrchamp left a comment

jrchamp Oct 30, 2024

paulandm Nov 13, 2024

paulandm commented Nov 11, 2024

jrchamp left a comment

jrchamp Nov 11, 2024

paulandm commented Nov 12, 2024

jrchamp Nov 14, 2024

jrchamp Nov 14, 2024

jrchamp Nov 14, 2024

jrchamp Nov 14, 2024

jrchamp Nov 14, 2024

jrchamp Nov 14, 2024

		$messagedata = new \core\message\message();
		$messagedata->component = 'mod_zoom';

Send ical Notifications (Version 2) #623

Are you sure you want to change the base?

Send ical Notifications (Version 2) #623

Conversation

paulandm commented Oct 1, 2024 • edited Loading

jrchamp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulandm commented Nov 11, 2024

jrchamp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulandm commented Nov 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulandm commented Oct 1, 2024 •

edited

Loading