Skip to content
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

Send invite Reminders #4824

Merged
merged 12 commits into from
Dec 5, 2024
Merged

Send invite Reminders #4824

merged 12 commits into from
Dec 5, 2024

Conversation

hardillb
Copy link
Contributor

@hardillb hardillb commented Nov 22, 2024

part of #4380

Description

Sends reminder emails to invitees if not actioned in 2 days.

Also includes a house keeping job to remove expired invites from the database.

Still need to also send email to inviter to say not accepted yet.

Also needs tests, but will need to work out how to inject invites into the database with a custom createdAt entry in the database.

Related Issue(s)

Checklist

  • I have read the contribution guidelines
  • Suitable unit/system level tests have been added and they pass
  • Documentation has been updated
    • Upgrade instructions
    • Configuration details
    • Concepts
  • Changes flowforge.yml?
    • Issue/PR raised on FlowFuse/helm to update ConfigMap Template
    • Issue/PR raised on FlowFuse/CloudProject to update values for Staging/Production

Labels

  • Includes a DB migration? -> add the area:migration label

Copy link

codecov bot commented Nov 22, 2024

Codecov Report

Attention: Patch coverage is 90.69767% with 4 lines in your changes missing coverage. Please review.

Project coverage is 78.74%. Comparing base (4ab0fb2) to head (530f4a8).
Report is 100 commits behind head on main.

Files with missing lines Patch % Lines
forge/housekeeper/tasks/inviteReminder.js 88.46% 3 Missing ⚠️
forge/db/migrations/20241129-01-invite-reminder.js 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4824      +/-   ##
==========================================
+ Coverage   78.67%   78.74%   +0.06%     
==========================================
  Files         314      323       +9     
  Lines       15125    15221      +96     
  Branches     3483     3496      +13     
==========================================
+ Hits        11900    11986      +86     
- Misses       3225     3235      +10     
Flag Coverage Δ
backend 78.74% <90.69%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hardillb
Copy link
Contributor Author

Need to work out how to write tests for this (need time travel or backdated database entries)

Also need to ensure this only runs if email enabled and do we want to make this behind a flag in the admin settings?

Also check the email content
@hardillb hardillb marked this pull request as ready for review November 26, 2024 12:11
@knolleary
Copy link
Member

Looks okay from a first pass through. Want to sanity check the right emails are sent to the right parties - I note the tests just verify the right number of emails are sent, not that the right content was sent to the right recipient.

We also need to consider the scaling issue. Is the easy option just to add a 'reminderSent' column to the table?

@hardillb
Copy link
Contributor Author

We could add a column to the Invitations table, but we'd have to have it run multiple times a day to reduce the chance of restarts meaning that we never sent the reminders.

I'll add some more to the tests to check the email contents

@hardillb
Copy link
Contributor Author

OK, I have a version that uses a db column to record when the invite was sent.

I'm running the job with the following cron job

    // runs every 15mins with a random offset from the start of the hour
    schedule: `${randomInt(0, 15)}/15 * * * *`,

This will run at 15 min intervals, with a random offset between 0 & 15mins. This should mean that different instances run at different times, but that it not skip a day if the app is restarted. It may be a bit too many times, but it should only send reminders once, so should be a null op most of the time.

This is to allow horizontal scaling
Co-authored-by: Nick O'Leary <nick.oleary@gmail.com>
@knolleary
Copy link
Member

Testing locally, I see the emails get sent, but I consistently get this error in the logs:

[2024-12-05T11:39:00.066Z] TRACE: Completed task 'inviteReminder'
[2024-12-05T11:39:00.503Z] WARN: Failed to send email: Error: Message failed: 450 SQLITE_ERROR: cannot start a transaction within a transaction

The Invitation's reminderSentAt column has been updated, so not sure what DB interaction is triggering this.

@knolleary
Copy link
Member

WARN: Failed to send email: Error: Message failed: 450 SQLITE_ERROR: cannot start a transaction within a transaction

I think this is a complete red herring. The error is the response being returned by my local test SMTP server. From our logs, two mails were sent. Looking at the inbox, only one arrived.

@knolleary
Copy link
Member

Yup - upstream issue with my choice of SMTP tools - nodemailer/nodemailer-app#3

@knolleary knolleary merged commit e13231e into main Dec 5, 2024
13 checks passed
@knolleary knolleary deleted the invite-reminder branch December 5, 2024 13:42
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