Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Send ical Notifications (Version 2) #623
base: main
Are you sure you want to change the base?
Send ical Notifications (Version 2) #623
Changes from 16 commits
7d91031
c5a07e3
f6a8f7f
3dca4b7
3b80e0e
c918384
708b379
3d39c40
079efb4
7fd8e0d
e7d6de2
29cfee1
c99249a
1c70ce7
46ce080
acc7ce8
bac9230
4ac56e6
31ae11c
4713350
8a098fd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After the require_once() calls, collect all of the used classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a lot of log output here, even for items that were already processed. I don't love the use of the word "execution", it reminds me of trauma. Plus, "notification time" is accurate and clear without unnecessary words.
The "[Zoom ical Notifications] " prefix is not necessary for every line. At most, the start and end task messages are already providing that context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewer: Same concern about parameter types (should probably cast to `(int)``)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewer: Determine if these is a concern about unique filenames. Do/should these files get cleaned up after use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to reviewer: Compare to core Moodle to see if this is the recommended pattern for initializing the object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self: Generate this using XMLDB and export it using XMLDB. Check for differences.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use Moodle strict parameter formatting.