-
Notifications
You must be signed in to change notification settings - Fork 610
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 API token expiry notification emails #8290
Send API token expiry notification emails #8290
Conversation
811cc48
to
8638983
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8290 +/- ##
==========================================
+ Coverage 88.45% 88.53% +0.07%
==========================================
Files 275 275
Lines 27255 27101 -154
==========================================
- Hits 24108 23993 -115
+ Misses 3147 3108 -39 ☔ View full report in Codecov by Sentry. |
b9bdf9d
to
80d1907
Compare
80d1907
to
af6b1b3
Compare
fd03865
to
8f2f2e5
Compare
f4b1858
to
0a1214c
Compare
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.
🔢 Self-check (PR reviewed by myself and ready for feedback.)
Tested locally:
crates.io on rustin-patch-expiry_notification_job [$!] via 🐳 orbstack is 📦 v0.0.0 via v20.12.0 via 🦀 v1.77.0
❯ docker ps
CONTAINER ID IMAGE COMMAND CREATED STATUS PORTS NAMES
ea2f555e8f9f cratesio-frontend "pnpm start" 40 minutes ago Up 15 minutes 0.0.0.0:4200->4200/tcp, :::4200->4200/tcp cratesio-frontend-1
a3f3d61c8730 c944ce643ba1 "cargo run --bin bac…" 40 minutes ago Up 2 minutes cratesio-worker-1
d0dc3fabda26 cratesio-backend "/app/docker_entrypo…" 40 minutes ago Up 14 minutes 0.0.0.0:8888->8888/tcp, :::8888->8888/tcp cratesio-backend-1
13a8ba48f41e postgres:13 "docker-entrypoint.s…" 40 minutes ago Up 40 minutes 127.0.0.1:5432->5432/tcp cratesio-postgres-1
Message-ID: <[email protected]>
To: [email protected]
From: [email protected]
Subject: Your token is about to expire
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Date: Fri, 05 Apr 2024 08:26:31 +0000
Hi @hi-rustin,
We noticed your token "test_token" will expire in about 3 days.
If this token is still needed, visit https://crates.io/settings/tokens/new =
to generate a new one.
Thanks,
The crates.io team
|
2922fbc
to
97411c9
Compare
A couple of comments, but overall this looks good to me! 👍 |
I also noticed that since we don't have an index on |
d0efb44
to
c2ca897
Compare
Signed-off-by: hi-rustin <[email protected]>
Signed-off-by: hi-rustin <[email protected]>
Signed-off-by: hi-rustin <[email protected]>
Signed-off-by: hi-rustin <[email protected]>
Signed-off-by: hi-rustin <[email protected]>
Signed-off-by: hi-rustin <[email protected]>
Signed-off-by: hi-rustin <[email protected]>
Signed-off-by: hi-rustin <[email protected]>
Using multiple `filter()` calls is equivalent to using `.and()`, but reduces the indentation level and makes the code slightly more readable :)
We can calculate the cut-off datetime on the application side instead of relying on `.num_days().day()`, which was slightly error-prone for intervals that are not full days.
394f1af
to
cb19848
Compare
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.
Looks great! I've rebased the branch and added a couple of small commits on top. Thanks for working on this!
ref #8154
Description
This PR adds a new background job to send the expiry notification for tokens that will expire within 3 days.
As #6664 (comment) states, such emails are seen as a requirement before we can change the default expiration setting on the API token creation page.
Currently, GitHub only sends notifications twice - one after 6 days and another after 12 hours, and after it has expired.
Perhaps we should consider adopting a similar approach, although it may be too aggressive.
So I just picked 3 days as the default value. It should be easy to extend the current design to send multiple emails by reusing 'expiry_notification_at' multiple times.
Explanation of Changes
expiry_notification_at
to theapi_tokens
table.SendTokenExpiryNotifications
that will scan all the expiry tokens within 3 days and send an email to the users.