-
Notifications
You must be signed in to change notification settings - Fork 30
Notify teams of AB tests close to expiry #15060
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
Conversation
09333a5 to
b78385a
Compare
90f2421 to
c50a223
Compare
338e176 to
3284aa3
Compare
|
Hello 👋! When you're ready to run Chromatic, please apply the You will need to reapply the label each time you want to run Chromatic. |
3284aa3 to
0f44f6d
Compare
deedeeh
left a comment
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.
Awesome work @cemms1 👏🏼👏🏼 You've covered everything from the main functionality to testing which I particularly love about this PR ✨
It took me a bit of time to understand few things but I just had to look up for and I am glad I did!
I left just a comment just to understand your thoughts on the implementation.
…l address as verified source and add file extensions to imports
…se.all to handle async email sending for all owners
…clear on html email
0f44f6d to
77c4e5f
Compare
|
This is great! Should we also notify someone else if the email failed to send to the test owner? Say if someone mistyped an email address? |
I did wonder that! I have set up an SNS topic for lambda failures but I haven't set up a subscription for anyone to be notified yet... Who do you think would be best to add to that? |
| ), | ||
| ); | ||
| } catch (error) { | ||
| console.log(`Error sending email to ${recipient}`, 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.
Will this catch prevent the SNS topic being notified of an 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.
Aha, maybe not... I'll fix this in a follow up PR and test failures properly!
|
I was more thinking of a fallback email of the expired tests, perhaps to But an SNS topic alert also makes sense! |
What does this change?
Adds a scheduled lambda for the new AB testing framework to notify teams of upcoming AB test expiry dates or to let them know that tests have already expired.
PRODbut not forCODE, though in both environments it can be triggered manuallyExample email format
Tested in CODE
Why?
We want to make sure expired tests are turned off and/or removed from the codebase if no longer active. Notifying teams of this helps to keep them aware of this.
How to test
Running locally
/ab-testing/config/abTests.tsthat either has an expiry less than two days in the future or an expiry in the past. Ensure your email is marked as an owner of that testnode src/run.tsfrom thenotification-lambdadirectory)Running in CODE
/ab-testing/config/abTests.tsthat either has an expiry less than two days in the future or an expiry in the past. Ensure your email is marked as an owner of that test