[Feature:Notifications] Released Gradeable Notification#11897
Conversation
…e notification types
jeffrey-cordero
moved this from Awaiting Maintainer Review
to In Review
in Submitty Development
|
This should remain blocked until SQLAlchemy v2 is fully integrated into the main branch, updating deprecated methods as needed. |
JManion32
left a comment
JManion32
left a comment
There was a problem hiding this comment.
Please note I was unable to test the email notification as it seems my local version doesn't have logs right now (probably from running recreate_sample_course and only creating sample), but they still showed up on the status page when I ran send_notification.py.
Other than my local issue with the mailbox, everything worked as expected. I commented a few stylistic changes that I think would benefit the PR, but feel free to push back.
Additionally, if there is going to be multiple entries on the email status page as described in the PR description, we should create an issue since it could add a lot of bloat if many students have the email for this type of notification toggled.
Overall, great work!
| def construct_notifications(term, course, pending): | ||
| def format_timestamp(timestamp): | ||
| """Format a timestamp into a readable string.""" | ||
| return timestamp.strftime("%B %d, %Y at %I:%M %p") |
There was a problem hiding this comment.
If possible, we should use YYYY-MM-DD formatting for dates to be consistent with the rest of the site.
| notification_content = ( | ||
| f"{email_subject} (Due {format_timestamp(gradeable['submission_due_date'])} - " | ||
| f"{format_late_days(gradeable['remaining_late_days'])} remaining, " | ||
| f"{format_late_days(gradeable['max_late_days'])} allowed)" |
There was a problem hiding this comment.
I think we can make the notification more concise and readable. How about:
Submissions Open: Gradeable | Due: 2025-08-05 @ 11:59 PM | 0 late days remaining (0 allowed)
Submissions Open: Gradeable | Due: 2025-08-05 @ 11:59 PM | 0/0 late days
I'm personally a fan of the latter as it is the most concise while still conveying all relevant information to the user. It may not be immediately clear, but I don't think users would struggle to figure it out. Adding late days to the notification should only be a small portion of it 1) because there is already so much information here 2) it is not the main focus. Additionally, I added vertical bars to separate everything a little clearer.
Additionally, we could make the email more concise with something like:
Late Days: 2 remaining, 5 allowed
(not sure if you already changed this)
bmcutler
left a comment
bmcutler
left a comment
There was a problem hiding this comment.
I haven't tested myself yet, but how does this handle gradeables with a prerequisite?
For example, our crash course in C++ has a set of work-at-your-own-pace gradeables that are available at start of term, but a student can only submit to the first one. Do we alert the student about all of them at the start of term? It might be better to alert them one at a time, only when they have met the prerequisite?
…mums for single and team assignments
There was a problem hiding this comment.
Notifications and Emails look and work well!
One case that I don't think should hold up this PR but is worth thinking about:
Should a gradeable's notifications still go out to staff if they haven't satisfied that gradeable's prerequisite?
A consideration for a future PR (I can do it when I port the notifications page to Vue):
For gradeable notifications, it would be nice to leave space for an icon even if we don't have one so they line up with forum notifications.
github-project-automation
Bot
moved this from In Review
to Awaiting Maintainer Review
in Submitty Development
williamjallen
left a comment
williamjallen
left a comment
There was a problem hiding this comment.
I'll take a closer look later, but here's what I noticed on an initial pass.
| course_db.commit() | ||
| master_db.commit() | ||
| course_db.commit() | ||
| master_db.commit() |
There was a problem hiding this comment.
Was this change intentional? At first glance, it looks like the original indentation was correct.
There was a problem hiding this comment.
It's reverting the change in #11578, but both positions are valid as gradeables should always be non-empty when calling this method.
### Why is this Change Important & Necessary? Students previously had to manually click a notification or visit the notification page to mark gradeable release and grade release notifications when visiting the respective page via a link, which is a poor-quality-of-life experience. ### What is the New Behavior? When a student visits a gradeable page, any unseen grading notifications for that gradeable (submission open or grade release) are automatically marked as seen in a single database update. ### What steps should a reviewer take to reproduce or test the bug or new feature? 1. Run the respective course migration. 2. Trigger a gradeable release or grade release notification via `send_notification.py` (`udo -u "submitty_daemon" python3 sbin/send_notification.py`: #11897 and #10358 have more in-depth explanations). 3. Log in as the notified student (notification bell should show unread). 4. Navigate to the gradeable page. 5. Verify the notification is now marked as seen (bell clears, notification panel shows read). ### Automated Testing & Documentation <!-- Is this feature sufficiently tested by unit tests and end-to-end tests? If this PR does not add/update the necessary automated tests, write a new GitHub issue and link it below. Is this feature sufficiently documented on submitty.org? Link related PRs or new GitHub issue to update documentation. --> Unit tests have been added, but an issue will be made to extend this notification feature to end-to-end testing coverage. ### Other information N/A --------- Co-authored-by: Justin Manion <[email protected]>
…itty#12658) ### Why is this Change Important & Necessary? Students previously had to manually click a notification or visit the notification page to mark gradeable release and grade release notifications when visiting the respective page via a link, which is a poor-quality-of-life experience. ### What is the New Behavior? When a student visits a gradeable page, any unseen grading notifications for that gradeable (submission open or grade release) are automatically marked as seen in a single database update. ### What steps should a reviewer take to reproduce or test the bug or new feature? 1. Run the respective course migration. 2. Trigger a gradeable release or grade release notification via `send_notification.py` (`udo -u "submitty_daemon" python3 sbin/send_notification.py`: Submitty#11897 and Submitty#10358 have more in-depth explanations). 3. Log in as the notified student (notification bell should show unread). 4. Navigate to the gradeable page. 5. Verify the notification is now marked as seen (bell clears, notification panel shows read). ### Automated Testing & Documentation <!-- Is this feature sufficiently tested by unit tests and end-to-end tests? If this PR does not add/update the necessary automated tests, write a new GitHub issue and link it below. Is this feature sufficiently documented on submitty.org? Link related PRs or new GitHub issue to update documentation. --> Unit tests have been added, but an issue will be made to extend this notification feature to end-to-end testing coverage. ### Other information N/A --------- Co-authored-by: Justin Manion <[email protected]>
5 participants
Why is this Change Important & Necessary?
Notifications for grade releases were originally introduced within #10358. Still, students have asked for more types of notifications tied to gradeables, such as receiving one when submissions are open for a given gradeable.
What is the New Behavior?
This extends the existing hourly cron job to check for electronic gradeables with pending submission-related notifications ready for release to all active course users. By default, site notifications are enabled, while email notifications are disabled to help minimize email fatigue. For assignments with prerequisites, such as a crash course, the student will only be notified if the minimum autograding score for the gradeable dependency is met.
This is the site notification, which compacts the due date and late day information into a single line.
This is the email notification, including all relevant information on a separate line for readability.
Additionally, the Email Status page may have multiple entries for a gradeable as the content includes the unique students' remaining late days, along with the applicable late days, and deadline for the assignment.
What steps should a reviewer take to reproduce or test the bug or new feature?
submitty_installsudo -u "submitty_daemon" python3 sbin/send_notification.pyAutomated Testing & Documentation
I will create an issue for us to introduce a new Cypress file,
notifications.spec.js, within theCypress-Featureworkflow to handle all types of notifications, including verification of their release through the cron job script.Other information
Existing variable and function names related to score notifications have been refactored for improved readability and to better support integration of additional notification types.