Sohail: Fix bugs preventing 4am cron job from sending follow-up emails#2051
Sohail: Fix bugs preventing 4am cron job from sending follow-up emails#2051sohailuddinsyed wants to merge 5 commits intodevelopmentfrom
Conversation
Hi Anthony, |
There was a problem hiding this comment.
Hi Sohail,
I have tested your PR locally and in backend it gave me some error but i have received email and even though only my email is given and NODE_ENV is set to production in the first try and then removed in the second try. But still the email is being sent to multiple people. Was not able to stop in first try as i was trying to figure out the error, but stopped the code from running in the second try. Please advice on how to proceed as i assumed this was similar to test as i had tested PR #1788 which was isolated testing
Note: I do understand test 1 is already working hence i choose it to begin testing, do let me know if that is what caused this widespread email trigger
Intial error stack trace



The values replaced

Multiple mails being sent

ENV variables for first try

ENV variables for second try and the terminal showing that multiple emails are being sent again

the corn job

I think I can share some info after investigating it myself on seeing you encountered errors with it, the reason why it's hitting other emails, is because the function assignBlueSquareForTimeNotMet, was changed back. To explain, in the original PR, we had it where it was: const assignBlueSquareForTimeNotMet = async (emailConfig = {}) => {, but at somepoint in other merges, it was reverted back to const assignBlueSquareForTimeNotMet = async () => {, same with the rest of the function's code, which is not setup to work with preset values anymore. As for the error stack, I am unsure about, potentially related but might not be, good thinking on testing it, as I knew the function works already, but hadn't thought to test to double check if any merges since had affected it. So I only checked the other three just to see if they'd launch and show the logs for attempts. |
AnthonyWeathers
left a comment
There was a problem hiding this comment.
I tested the email functions, and used added time entries on the dev site for the different time spent conditions. Only had to modify the value used for the todayBlueSquare and the amount of infringements checked due my testing account having more than 4. Otherwise, the functions fired using the testEmailJobs.js and created emails.
So just the assignBlueSquareForTimeNotMet function needs to be readjusted like done in the original PR, as it doesn't have the opts value either anymore so it wouldn't be apart of the same email chain as the others.
After you can fix that function back up, I think it'd be good to go for another live test.
|
I've restored the emailConfig logic in assignBlueSquareForTimeNotMet to ensure it correctly supports the targetUserId and email overrides required for safe testing. This resolves a regression where the function was missing the parameterization needed to interact with the testEmailJobs.js script. I also audited the other three blue square functions and confirmed they already implement this setup as intended. Ready for re-review! @Anusha-Gali @AnthonyWeathers |
|
AnthonyWeathers
left a comment
There was a problem hiding this comment.
Spending quite a number of hours, checking out everything and retesting just to confirm I was sure, here's my results.
The functions work as expected, I can call them and they'll function.
Issue: Emails weren't being sent from any of the 4, had to look into it, and I commented out these lines of code in emailSender.js (just below). This was stopping the test emails, they began working after I commented it out. This isn't against the PR, it's code that was merged in, so this is more just to inform anyone attempting to test this PR.

2nd issue: Or well, perhaps an issue, I noticed the emails weren't being grouped, I am unsure if they are required to be, or not. Just to clarify, it's the email that'll be sent ideally at midnight that is not grouped with the other automated emails.
This is with me having added the optional data to the email sent from time not met, but not changing the subject.

This was with testing the email sent from time not met, twice, once for unmodified subject and no optional, then the second email was for a modified subject to match the later automated ones but not including the optional email.

Again, unsure if we're wanting the emails to be grouped together, if so then the subject in assignBlueSquareForTimeNotMet would need to be updated to match, then add the optional data to match. If grouping them all is not the goal, then it'll be fine as is, and I'll change my review from needs changes to approval.
Anusha-Gali
left a comment
There was a problem hiding this comment.
Hi @AnthonyWeathers ,
Thank you for looking into the PR and for the suggestion.
@sohailuddinsyed , i was able to test the assignBlueSquareForTimeNotMet with no issues. Find below my changes in code as per documentation and for the above scenario test.
inCompleteHoursEmailFunction
For 0 hours completed - no mail received as per requirement.

For 13 hours completed in current week - no mail received as per requirement.

For 12 hours completed in last week - mail received as per requirement.



completeHoursAndMissedSummary
For 20 hours met current week with no summary and last week as mentioned above only 12 hours met - no mail received as per requirement


For 20 hours met last week with no summary - mail received as per requirement



weeklyBlueSquareReminderFunction

Query : for the above weeklyBlueSquareReminderFunction, no email was received, as you can see below i have many blue squares for whole of last week as well as the main condition that i should have a blue square today which i do. But haven't received an email. Also checked by reducing my hours to <65% and have not received email. Is my understanding correct?
|
Sorry for getting back to you late @Anusha-Gali but for the weeklyBlueSquareReminderFunction, yes, the basics are you need blue squares, and gained one today, or the same day as when that function fires. But, the function also relies on the number of infringements/blue squares you possess, mainly 1 - 4, with additional conditions affecting certain cases. The most important one is of course being 1 - 4 blue squares, or infringements. From the image you provided, you have at least 7 blue squares total, so therefore you're above the maximum check of infringements. This is likely why you were unable to receive an email despite having a blue square assigned the same day, and otherwise met the other conditions for one of the emails. I'd suggest removing blue squares so you're at 4 or less, but I will need to inform you that I saw in the screenshots, Today's Blue Square showed a 0, so although you triggered assignBlueSquareForTimeNotMet to get a blue square, this creates a blue square based on UTC, while the other functions worked on PST, so time of day when triggering the function to assign a blue square for the day will affect your ability to properly test weeklyBlueSquareReminderFunction except for when testing later to ensure they're both hitting the same day, or just adjusting the date in the code manually to guarantee it. |



Description
Follow-up fix to PR #1788 - Blue Square Email Automation. After the original PR was merged into production, the 12 AM blue square assignment emails worked correctly, but the 4 AM follow-up emails were not being sent.
Related PRs
Main Changes Explained
1. Fixed Critical Time Range Bug in
completeHoursAndMissedSummaryIssue: Function was checking the CURRENT week's hours instead of LAST week's hours.
Impact: When running at 4 AM on Sunday (start of new week), users had only logged 0-4 hours in the current week, causing
timeNotMetto always betrue. This prevented emails from being sent to users who completed their hours but forgot their summary.Fix: Changed from
pdtStartOfCurrentWeek/pdtEndOfCurrentWeektopdtStartOfLastWeek/pdtEndOfLastWeek.Additional Fix: Corrected array bounds check from
length >= 1tolength > 1to prevent accessingweeklySummaries[1]when array only has 1 element.2. Fixed Critical Time Range Bug in
inCompleteHoursEmailFunctionIssue: Function was checking the CURRENT week's hours instead of LAST week's hours.
Impact: Same as above - users appeared to have completed 0% of hours when checked at 4 AM Sunday, causing incorrect email logic evaluation.
Fix: Changed from
pdtStartOfCurrentWeek/pdtEndOfCurrentWeektopdtStartOfLastWeek/pdtEndOfLastWeek.3. Verified
weeklyBlueSquareReminderFunction(No Bugs Found)Status: This function was already correctly checking last week's hours.
Enhancement: Added comprehensive logging to help debug why emails might not be sent (function is intentionally selective based on user tenure and infringement history).
4. Added Comprehensive Logging
Added detailed console logs to all three functions for production debugging:
Root Cause Analysis
The 12 AM job (
assignBlueSquareForTimeNotMet) was already correctly checking last week's hours, which is why those emails worked. However, the 4 AM follow-up functions were inadvertently checking the current week (which just started 4 hours ago), causing them to fail their conditions and skip sending emails.Expected Behavior After Fix
At 12:00 AM PST Sunday
assignBlueSquareForTimeNotMetruns:At 4:00 AM PST Sunday (4 hours later)
completeHoursAndMissedSummary:inCompleteHoursEmailFunction:weeklyBlueSquareReminderFunction:How to Test
Use Test Script (Recommended)
Important
Follow instructions in the test script documentation from PR #1788.
What to Verify
Important Notes