Skip to content

Conversation

@one-new-message
Copy link
Contributor

@one-new-message one-new-message commented Apr 20, 2024

Obviously I can't work on implementing this in the official client, but I think this is all that is necessary to add the feature to the API. Currently drafting this PR to enable discussion, but maybe the issue tracker is a better place for this??

On Discord I shared this mockup:
screenshot

Hopefully this type of design is versatile enough (I believe it is). Though discussion of the frontend should probably be held somewhere else entirely :)

@Cellaryllis
Copy link
Collaborator

Hey @one-new-message, only saw this now.

In general this seems fine, however a few things to keep in mind:

  • Fields are not a given to exist, so checking !data.enabled will return true on everyone who has not set that property to true (which means everyone as soon as the PR would be merged). For every property check you need to explicitly check against the default value or the inverse of the default value.

For example enabled would be true by default so we would check against the inverse, this ensures that people who don't have the property don't get their reminders turned off by default.
date.enabled === false

Secondly, this would still send a reminder and not delete any scheduled reminders yet-to-be-sent. You would need to delete all queued events that are scheduled for the reminders you disable.

@one-new-message one-new-message deleted the add-reminder-toggle branch May 12, 2024 19:58
@one-new-message one-new-message restored the add-reminder-toggle branch May 12, 2024 20:01
@one-new-message
Copy link
Contributor Author

Thanks for the feedback! To start, I've removed the requirement to set the enabled property when creating reminders to retain compatibility with the existing client.

Changing the check to data.enabled === false is correct; I did not think through how !data.enabled would behave.

There is no need for further changes for repeated reminders--when a PATCH request is received at the v1/timer/repeated/:id endpoint, it calls repeatedTimer.update(). That async function calls repeatRemindersEvent() in src/modules/events/repeatReminders.ts, which already includes its own step of removing and rescheduling already scheduled reminders updated by the request. This rescheduling is performed by scheduleReminder(), which still begins with the enabled check I implemented: if (data.enabled === false) return;

I don't believe this affects automated reminders either. These are not queued until they are triggered, and the check I implemented prevents them from being scheduled if disabled, which means they are never added to the queue. (Unless I'm missing something with the way the routes and events are configured/controlled.)

Side note: I've managed to break my commit history in a way that may break merging again, so I'll likely need to move my changes into a new clean topic branch. If these changes are still potentially wanted, I'll draft a new PR and close this one.

@Cellaryllis
Copy link
Collaborator

Hey @one-new-message thanks for the update (although by now the commits have merge conflicts 🙈 We did a lot of refactoring so I suspect that's what broke it. Once 1.11 is out we'll reach back out to get this PR merged and then we can update the app.

@Cellaryllis
Copy link
Collaborator

Hey @one-new-message the API is now 'ready' in a sense that nothing major will change for a while so if you want to do a PR feel free.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants