Add a check for changes in study session message before reposting #48
Conversation
| if (isMessageOlderThan5HoursAgo(mostRecentStudySessionMessage)) { | ||
| mostRecentStudySessionMessage.delete().then(() => { | ||
| makeStudySessionMessage().then((studyMessage) => { | ||
| studySessionChannel.send(studyMessage).catch((error) => { | ||
| makeStudySessionMessage().then((studyMessage) => { | ||
| if (upcomingSessionsAreDifferent(mostRecentStudySessionMessage, studyMessage)) { | ||
| mostRecentStudySessionMessage.delete().then(() => { | ||
| studySessionChannel.send(studyMessage).catch((error) => { | ||
| console.log(error); | ||
| }); | ||
| }).catch((error) => { | ||
| console.log(error); | ||
| }); | ||
| }); | ||
| } | ||
| }).catch((error) => { |
There was a problem hiding this comment.
With our current logic, it seems like we might be making a query to the database every minute after the 5 hours have passed until there is a change in session messages.
What do you think is the best way to handle this? I thought about changing the length of the interval, but that seems like it would get complicated.
There was a problem hiding this comment.
Ooh, yeah, you're right, I didn't notice that! I think there are two simple fixes:
- Have an additional
cron.schedulein scheduler.js that runs every five hours, and put this task in that instead of in the minute one - Instead of checking if the post is 5 hours or more ago, check if the time between the post and now is exactly a multiple of 5 hours
Are either of these ok? Which one would you prefer?
Edit: I've created an additional commit with the cron job change
There was a problem hiding this comment.
Smart! I like it a lot.
If people want to have notifications on for the events channel it might get annoying having the ping go off even if there are no changes
Fox-Islam
force-pushed
the
enhancement/study-session-difference-check
branch
from
e5ee53e to
4209d12
Compare
Explicitly running the reminder task every 5 hours instead of running it every minute and trying to handle the timing from within the task logic
Fox-Islam
force-pushed
the
enhancement/study-session-difference-check
branch
from
4209d12 to
4d4dff6
Compare
src/tasks/index.js
Outdated
| const tasks = { | ||
| "minute": [], | ||
| "hour": [notifySubscribers], | ||
| "five-hour": [sendChannelReminder] | ||
| }; |
There was a problem hiding this comment.
I love the separation here! It definitely gives us a lot more control. What do you think about having this sendChannelReminder function get called when new study sessions/events are made?
Hopefully that would also remove any frustration of users scheduling a session and having to wait a number of hours before it shows up in the events channel.
There was a problem hiding this comment.
That makes a lot of sense!
In fact, since that would handle all cases where a study session is added to the list (there's no other way to create a study session), the only thing that would be handled by the scheduler is the case when removing a study session from the list (either through cancellation or the date passing), which I think we can get away with being less urgent with an update for - could maybe drop the scheduled task time to 12-hourly? Daily?
Also, I haven't checked but I think once a study session passes it remains in the database forever, do you think those are useful to keep around? If not, I think it would be a "good first issue" to create a weekly/fortnightly/monthly scheduled task that deletes all study sessions that have passed
If people want to have notifications on for the events channel it might get annoying having the ping go off even if there are no changes