-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CON Reminder Emails #910
CON Reminder Emails #910
Conversation
WalkthroughThe recent update to the NestJS API introduces a new Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant Client
participant ReminderEmailsController
participant ReminderEmailsService
participant SfApiEmailTemplatesService
participant AWS SES
Client->>+ReminderEmailsController: Request to send reminder email
ReminderEmailsController->>+ReminderEmailsService: Process request
ReminderEmailsService->>+SfApiEmailTemplatesService: Retrieve email template
SfApiEmailTemplatesService-->>-ReminderEmailsService: Email template
ReminderEmailsService->>+AWS SES: Send email
AWS SES-->>-ReminderEmailsService: Confirmation
ReminderEmailsService-->>-ReminderEmailsController: Email sent
ReminderEmailsController-->>-Client: Response
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Ready for review? |
@ericbolikowski Yes. Please go ahead and review 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apps/nestjs-api/src/reminder-emails/reminder-emails.controller.ts
Outdated
Show resolved
Hide resolved
apps/nestjs-api/src/reminder-emails/reminder-emails.controller.ts
Outdated
Show resolved
Hide resolved
throw new Error(`Email template not found: ${sfEmailTemplateName}`) | ||
} | ||
|
||
const sanitizedHtml = template.HtmlValue.replace( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could even do some kind of test to check for existence of {{{Recipient.FirstName}}}
here? And throw if not present. That is something that I see relatively easily happening because of user error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will actually work on these a bit more when I know the script and automation is working correctly. Can we skip this for now?
}, | ||
Subject: { Data: template.Subject }, | ||
}, | ||
Source: 'career@redi-school.org', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will we get into trouble with Malmö?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, I can do the fine tuning once I see everything is working correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@helloanil, great job! 👏
…ct-reminder-emails
…ions by days and location
…cation method signature
@@ -59,6 +59,7 @@ export class ReminderEmailsService { | |||
Profile_First_Approved_At__c: { | |||
$eq: jsforce.SfDate.toDateLiteral(approvedDate), | |||
}, | |||
'Contact__r.ReDI_Active_Mentorship_Matches_Mentee__c': null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think 0
is better than null
here. AFAIK this is a formula field in Salesforce, so it shouldn't take on other types than Number
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0 actually didn’t work. That was also my first attempt, I was quite surprised why it was not working and while debugging it I sent myself 650 emails and lost some valuable time on the Salesforce Dev Console.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this field has a different purpose. I just checked my CON profile on production. I have an ongoing mentorship with Eric, and this field displays 1.
So, it correctly shows the Active_Mentorship_Matches
, not the number of applications.
And this is from Eric's mentor CON profile:
Maybe creating a custom field on the CON profile to check the mentee applications would be better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@katamatata But I tested with using ReDI_Active_Mentorship_Matches_Mentee__c
and when I didn't have any applications vs when I had an application (wiithout accepted), it worked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, so Kate and I dug a bit into this.
To summarize: none of the four fields...
ReDl_Active_Mentorship_Matches_Mentee
, ReDI_Total_Mentorship_Matches_Mentee_c
, Total_Mentorship_Matches_Mentor_c
, Active_Mentorship_Matches_Mentor_c
... are referenced in the codebase. It's unclear how three out of four of them are actually updated in Salesforce (since they're just a Number
field). Our working theory is that a Salesforce "plugin" called Rollup Helper was doing this job, but checking its config, we can't find configuration that ensures such updates. Also, I remember discussing with Manu that the triggers to update the contents of these fields was brittle to begin with.
Recommendation: only use Active_Mentorship_Matches_Mentor_c
in our code. Don't use it to count number of applications a mentee has sent. If using that (or other fields) for some reason works, we should either:
- figure out how it works, then use it if its reliability satisfies us
- use/configure another field and use that
@helloanil, thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @ericbolikowski, yeah it's a mess that we have all those fields we don't even know how they work 😄. I'm not sure if I can follow your recommendation though: "Only use in our code, not to count number of applications a mentee has sent." You say "use it in code" but then "don't use it".
If we don't feel safe using this field, I'm happy to follow number 2 and configure another field for this.
…g approved mentees with no mentor applications
Hey @ericbolikowski and @katamatata, here are the latest news on this task and I need your help: I decided to move script file to gitignore for two reasons. I'll create this file manually on the server, alongside the background service that will take care of the automation. I will send you the script code on Slack so you can review it there.
I will start testing this task with two reminders only, which is the top priority ones. (Mentors and Mentees to complete their profile). There are few more reminders ready but I haven't tested them properly yet. They are commented out (see the last commit). Please let's focus on only the first two reminders for now. |
Just added another reminder:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@helloanil, can you please comment all the functions in reminder-emails.service.spec.ts
that are not used by the reminders implemented in this PR (getApprovedMenteesWithNoMentorApplicationsByDays
, getUnmatchedMenteesWithApprovedProfiles
, getConProfilesWithMentorshipMatchesWithoutMentoringSessions
, sendApplyToMentorFirstReminder
, sendApplyToMentorSecondReminder
, sendMenteesPlatformAndNewMentorsReminder
)? They made it harder to review the current implementation, and some of these functions require changes, as we discussed in the previous comments.
I left one comment regarding your email. Otherwise, it looks good to me.
const params = { | ||
Destination: { | ||
ToAddresses: isProductionOrDemonstration() | ||
? ['anilakarsu93@gmail.com'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this email be changed before deploying?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (10)
- .gitignore (1 hunks)
- apps/nestjs-api/src/app/app.module.ts (2 hunks)
- apps/nestjs-api/src/auth/cronjob-auth.guard.ts (1 hunks)
- apps/nestjs-api/src/reminder-emails/reminder-emails.controller.spec.ts (1 hunks)
- apps/nestjs-api/src/reminder-emails/reminder-emails.controller.ts (1 hunks)
- apps/nestjs-api/src/reminder-emails/reminder-emails.module.ts (1 hunks)
- apps/nestjs-api/src/reminder-emails/reminder-emails.service.spec.ts (1 hunks)
- apps/nestjs-api/src/reminder-emails/reminder-emails.service.ts (1 hunks)
- apps/nestjs-api/src/salesforce-api/sf-api-email-templates.service.ts (1 hunks)
- apps/nestjs-api/src/salesforce-api/sf-api.module.ts (3 hunks)
Files skipped from review due to trivial changes (3)
- .gitignore
- apps/nestjs-api/src/reminder-emails/reminder-emails.controller.spec.ts
- apps/nestjs-api/src/reminder-emails/reminder-emails.service.spec.ts
Additional comments not posted (13)
apps/nestjs-api/src/auth/cronjob-auth.guard.ts (1)
6-17
: Implementation ofCronjobAuthGuard
follows NestJS best practices for guards and securely checks authorization tokens.apps/nestjs-api/src/reminder-emails/reminder-emails.module.ts (1)
9-18
: Configuration ofReminderEmailsModule
correctly sets up dependencies and controllers needed for the reminder emails functionality.apps/nestjs-api/src/salesforce-api/sf-api.module.ts (1)
60-60
: The comment about a hack in the exports section should be addressed. If it's no longer necessary, consider removing it.- //! TODO: this was hack - remote this export - SfApiRepository,apps/nestjs-api/src/app/app.module.ts (1)
12-12
: Integration ofReminderEmailsModule
intoAppModule
looks good and aligns with the PR's objectives.Also applies to: 61-61
apps/nestjs-api/src/reminder-emails/reminder-emails.controller.ts (4)
1-4
: Setup ofReminderEmailsController
with necessary imports and guard looks correct.Also applies to: 6-9
11-32
: The methodsendMentorCompleteProfileReminders
is well-implemented with proper asynchronous handling and conditional logic based on the fetched data.
34-55
: The methodsendMenteeCompleteProfileReminders
mirrors the structure of the mentor reminder method, which maintains consistency in the controller's design.
105-135
: Implementation ofsendMentorshipFollowUpReminders
correctly handles the complex logic of sending emails to both mentors and mentees based on the age of the mentorship match.apps/nestjs-api/src/reminder-emails/reminder-emails.service.ts (5)
18-33
: Initialization of AWS SES in the constructor ofReminderEmailsService
is correctly configured with environment-specific parameters.
35-46
: The methodgetDraftingConProfiles
uses a clear and concise query to fetch profiles in drafting status, which is crucial for the reminder functionality.
67-130
: The methodgetThreeMonthsOldMentorshipMatches
and its transformation logic are well-implemented, ensuring that the necessary data is formatted correctly for the follow-up reminders.
171-223
: ThesendCompleteProfileReminder
method is robust, handling potential errors in fetching the email template and sending emails through SES with appropriate error logging.
318-367
: ThesendMentorshipFollowUpReminder
method is implemented with detailed attention to replacing placeholders in the email template, which is essential for personalized emails.
What Github issue does this PR relate to? Insert link.
#712
What should the reviewer know?
This PR is focusing only on the top 3 priority emails from the reminders list:
The automation part will be done in parallel, by writing a service script either directly in the server, or using a service like val.town
Summary by CodeRabbit
New Features
ReminderEmailsModule
to manage reminder emails for mentorship activities.CronjobAuthGuard
to secure routes based on authorization tokens.Tests
ReminderEmailsController
andReminderEmailsService
.Chores
.gitignore
to exclude specific infrastructure and script files.