Conversation
dburkhart07
left a comment
There was a problem hiding this comment.
looks really good so far! this function looked like it was kinda evil to do 😅
|
|
||
| const uploadedPhotoUrls = | ||
| photos && photos.length > 0 ? await this.awsS3Service.upload(photos) : []; | ||
| console.log( |
There was a problem hiding this comment.
i will miss seeing these every time i run yarn test :/
| occurrences -= 1; | ||
| occurrencesUpdated = true; | ||
|
|
||
| if (occurrences > 0 && donation.recurrence !== RecurrenceEnum.NONE) { |
There was a problem hiding this comment.
I think this recurrence check should be at the very start. If we don't have a recurrence, we should immediately skip.
| ); | ||
|
|
||
| // cascading recalculation of next dates when replacement dates are also expired | ||
| while (nextDate.getTime() <= today.getTime() && occurrences > 0) { |
There was a problem hiding this comment.
I have some thoughts on this.
Let's say im an fm that said i could donate food on MTW every week. To me, receiving several email reminders all at once doesn't make sense to me, and is somewhat bad UX in my opinion. Ultimately, I wish we could make it so that the cron job could be unique to the donations to run accordingly to how often they said they could donate, but that's unrealistic I think.
Given that we need one cron job for all donations, what makes the most sense is to run this every single day, in which we should not face this cascading issue. I'm overall just trying to envision why we would want to set our cron job to such a time that we would encounter cascading issues, but as of right now I cant see one. The idea of FMs getting an email every single month saying "you need to do 4 weekly donations" just is very strange.
Interested to get thoughts on this @amywng @sam-schu @Yurika-Kan
There was a problem hiding this comment.
the intended case is for cron job to run daily, that way as you said, we should not by normally be sending multiple emails. however, to account for the case that cron job may be down just in case, we have this logic to send 1 email for every 1 missed recurring donation. the logic here is that then the fm can see 3 emails about 3 missed rounds of a donation, make it easy to visualize.
i do agree after revisiting the content of the automated email that it would be beneficial to add the date and the specific donation id of in the body of the email (currently an identical script for every recurring donation reminder email).
yes so it should & will be set to run daily but fallback behavior in case of a possible error is to catch up on past recurrences - so fms should not be receiving several emails at once as a default.
lmk if this is clear to your question
| ); | ||
| if (!alreadyExists) { | ||
| dates.push(nextDate); | ||
| dates.sort((a, b) => a.getTime() - b.getTime()); |
There was a problem hiding this comment.
If we are going through the sorted dates array originally, shouldn't this already be sorted? If not, maybe we should just sort them at the very end before calling this.repo.update.
ℹ️ Issue
Closes SSF-146
📝 Description
Implemented logic for processing recurring donations - every day, the cron job calls the service to check whether any scheduled donation dates have passed, send an email to the FM (placeholder log for now), and reschedule the next occurrence.
handleRecurringDonations():nextDonationDatesoccurrencesRemainingand removes the date fromnextDonationDatesrecurrenceandrecurrenceFreq(in a helper calledcalculateNextDate()) and adds it tonextDonationDatesif there are remaining occurrencesoccurrencesRemaininguntil a future date is reached or occurrences run outAdded service tests to cover all cases
insertDonation()helper to easily insert a new donation with custom recurrence info✔️ Verification
Tests pass