-
Notifications
You must be signed in to change notification settings - Fork 46
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
feat: Issue reminder job #19
Conversation
@Chigala is attempting to deploy a commit to the formbricks Team on Vercel. A member of the Team first needs to authorize it. |
Thank you for following the naming conventions for pull request titles! 🙏 |
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 @Chigala, apologies for the delay in the review!
I left few minor comments specific to the files and have addressed my testing feedback here:
-
While testing, I saw 2 events being generated on 1 issue assignment:
Is this expected? As per my understanding, we only create the 12hr event after the 36 hour event has expired right? -
I also did not see a comment on the issue (PS: for testing I set the times to seconds instead of hours). Am I doing something wrong?
In terms of config, I got all 3 Dev env vars from the Trigger.dev cloud console! Thanks again for the PR! Looking forward to your changes & suggestions on why it did not run for me so that I can fix it. 🚀
Let me know if anything is unclear of if I can help in any way!
import { TriggerClient } from "@trigger.dev/sdk"; | ||
|
||
export const client = new TriggerClient({ | ||
id: "testing-bHhV", |
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 also be an env variable?
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.
Nope, it doesn't have to be an env variable.
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.
So if we create a new Trigger.dev account for production? This stays the same?
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.
The endpoint id is just there to group all your endpoint url(staging, dev and prod) in the trigger.dev dashboard. You can change it to any name you want.
trigger.ts
Outdated
apiKey: process.env.TRIGGER_API_KEY, | ||
apiUrl: process.env.TRIGGER_API_URL, |
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.
We try to import as many of our env variables through our constants.ts
file so could you please change this as well?
//Converting PKCS#1 to PKCS#8 | ||
//For it to work trigger it has to be in PKCS#8 format | ||
|
||
const privateKeyPkcs8 = crypto.createPrivateKey(GITHUB_APP_PRIVATE_KEY).export({ | ||
type: "pkcs8", | ||
format: "pem", | ||
}); | ||
|
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'm not sure if we should include this in this PR as things work just fine for now. Can you revert this change please?
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.
If I revert this, it'll throw an error when the trigger job runs. I think that was why you weren't able to see the comment, I had the same issue while I was testing.
Hello @ShubhamPalriwala sorry I totally forgot about this. I think the 2 events were created because of a bug from smee.io payload delivery, sometimes it appears to call our api endpoint more than once, this in turn creates more than one trigger event. I noticed this while testing too |
Hey @Chigala, let me know once the PR is ready from your end then I'll test & review it again. Currently I see that you have added comments where you plan to improve the codebase. Looking forward to them 💪🏼 |
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.
Thanks for the changes! Just tested this & it works nicely with trigger.dev! 💪🏼 Solid work! The PR is gtg!
PS: I just made a small commit to revert back the change in the constants file for ossgg label!
We just need to add the below env vars to cloud before merging:
TRIGGER_API_KEY:
TRIGGER_API_URL:
…/issue-reminder-job
What does this PR do?
This PR creates a remainder job which basically sends a remainder to the user after 36hrs if the user hasn't opened a PR
Fixes # (issue)
How should this be tested?
You'll need trigger API Key and the API URL to test the issue reminder job. The API url is
https://api.trigger.dev
(if you are using trigger cloud). To learn more about this visit (https://trigger.dev/docs/documentation/introduction)Checklist
Required
pnpm build
console.logs
git pull origin main
Appreciated