[#59436] Set reminders for work packages to automatically generate date alert notifications (backend services) #17229
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
akabiru
force-pushed
the
implementation/59433-define-database-model-for-reminders
branch
from
2e0428f to
b466fba
Compare
akabiru
changed the title
akabiru
force-pushed
the
implementation/59433-define-database-model-for-reminders
branch
from
745d3f8 to
78e2a7a
Compare
akabiru
force-pushed
the
implementation/59433-define-database-model-for-reminders
branch
from
bd3c339 to
6137d1b
Compare
Once a reminder is scheduled, track the job id- this way in case of rescheduling (snoozing reminder) there is
a record we can rely on to have GoodJob reschedule directly. Further, we use this as a control to ensure unique job schedules.
TODO: Once a notification is created from a reminder, create a "reminder_notification" - join table that tracks notifications created from reminders
for further management
…date `update_column` is faster and updates the DB directly, skipping validations; which are not needed in this case. It is more important that we guarantee the job id is written into the database irrespective of any side effects from model validations
akabiru
force-pushed
the
implementation/59433-define-database-model-for-reminders
branch
from
753f8a0 to
3e6ab0d
Compare
Arises from: 73aa20c Once a reminder is created we need to keep track of: (1) The scheduled job: this is done via Reminder#job_id which corresponds to a GoodJob::Job#id (2) Any Notification(s) resulting from the reminder This is important in order to easily handle any modifications to the reminder such as snoozing, editing & deleting reminders
const_defined? looks up ancestors OR descendants by default. Which resulted in a NameError
```ruby
Failure/Error: "API::V3::Notifications::PropertyFactory::#{property_key.camelcase}".constantize
NameError:
uninitialized constant API::V3::Notifications::PropertyFactory::Reminder
```
To enforce descendants lookup, provide `false` as the second arg
akabiru
force-pushed
the
implementation/59433-define-database-model-for-reminders
branch
from
3e6ab0d to
b471590
Compare
As reminders are generally scheduled in the future, we need to keet track of (1) scheduled status - make it easy to track down the (good) job for edit/reschedule actions (2) notified at timestamp - prevents duplicate notifications, skip schedule when already notified
A notification cannot be linked to more than one reminder, every reminder should generate it's own unique notification.
Semantically on the recepient is relevant, as the actor is a reminder
akabiru
force-pushed
the
implementation/59433-define-database-model-for-reminders
branch
3 times, most recently
from
f30c92a to
eefbc1b
Compare
…tion Introduce simple status enums to track various reminder states
akabiru
force-pushed
the
implementation/59433-define-database-model-for-reminders
branch
from
eefbc1b to
ae3f77d
Compare
…notification" This reverts commit ae3f77d. Maintain a persistent record of reminder notifications as notifications are always retrievable by the user, e.g. via email, viewing "all" notifications. A reminder notification is only created IF there are no active unread notifications, hence when rescheduling, snoozing or editing a reminder then ensure all previous notifications are marked as read- as we should not have duplicate notifications for a given reminder.
akabiru
force-pushed
the
implementation/59433-define-database-model-for-reminders
branch
from
117fd17 to
b636799
Compare
When a reminder schedule is updated, any existing unread notifications are marked as read; and new notification is scheduled
In CI env the change is not seen for some odd reason- passes locally ```ruby expected `Reminder#job_id` to have changed from "1" to "2", but did not change ```
When using AR #update_all, a direct SQL update is performed which means updated_at is not updated (lol)
exists? can be chained to where without having adverse effects See: https://docs.rubocop.org/rubocop-rails/cops_rails.html#railswhereexists
akabiru
force-pushed
the
implementation/59433-define-database-model-for-reminders
branch
from
7aa3f2b to
8053cb6
Compare
Failing in CI - perhaps when ran in global context
akabiru
force-pushed
the
implementation/59433-define-database-model-for-reminders
branch
2 times, most recently
from
49b3ad2 to
e9f5c93
Compare
akabiru
force-pushed
the
implementation/59433-define-database-model-for-reminders
branch
from
e9f5c93 to
2bebbd5
Compare
In case of wonky time comparison issues, we'd rather have the reminder rescheduled with minimal tolerance than not having the reminder set at all
8 tasks
jjabari-op
approved these changes
Dec 3, 2024
There was a problem hiding this comment.
Great work @akabiru!
The only thing I've found was the max length of the notes, which I think is too short. Should be discussed with Wieland probably.
As everything else looks good, I approve this PR!
|
|
||
| module Reminders | ||
| class BaseContract < ::ModelContract | ||
| MAX_NOTE_CHARS_LENGTH = 80 |
There was a problem hiding this comment.
Good eye! We discussed this actually and 80 chars was the arrived spec. We can revisit and easily change if too short.
akabiru
deleted the
implementation/59433-define-database-model-for-reminders
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Ticket
https://community.openproject.org/work_packages/59436
What are you trying to accomplish?
remindersremind_atmanage_own_reminderspermissionScreenshots
Note
Reminder note covered in #17323
What approach did you choose and why?
Reminders are set as a polymorphic association to allow for any future extension to other models such as comments, activities or wiki pages (which are out of scope for this first version)
At present a user can only create their own reminders; if there is a need to create for other users, an explicit
recipientcan be added to the reminders table. Right now, we take a minimal approach.Merge checklist