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

feat: Issue reminder job #19

Merged
merged 9 commits into from
Apr 1, 2024

Conversation

Chigala
Copy link
Contributor

@Chigala Chigala commented Mar 2, 2024

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)
Screenshot 2024-03-02 at 09 57 53

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

  • Filled out the "How to test" section in this PR
  • Read How we Code at oss.gg
  • Self-reviewed my own code
  • Commented on my code in hard-to-understand bits
  • Ran pnpm build
  • Checked for warnings, there are none
  • Removed all console.logs
  • Merged the latest changes from main onto my branch with git pull origin main
  • My changes don't cause any responsiveness issues

Appreciated

  • If a UI change was made: Added a screen recording or screenshots to this PR
  • Updated the Formbricks Docs if changes were necessary

Copy link

vercel bot commented Mar 2, 2024

@Chigala is attempting to deploy a commit to the formbricks Team on Vercel.

A member of the Team first needs to authorize it.

Copy link

github-actions bot commented Mar 2, 2024

Thank you for following the naming conventions for pull request titles! 🙏

@Chigala Chigala changed the title Feat/issue reminder job Feat: issue reminder job Mar 2, 2024
@Chigala Chigala changed the title Feat: issue reminder job feat: Issue reminder job Mar 2, 2024
Copy link
Contributor

@ShubhamPalriwala ShubhamPalriwala left a 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:
    image 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",
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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
Comment on lines 5 to 6
apiKey: process.env.TRIGGER_API_KEY,
apiUrl: process.env.TRIGGER_API_URL,
Copy link
Contributor

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?

env.mjs Outdated Show resolved Hide resolved
Comment on lines +17 to +24
//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",
});

Copy link
Contributor

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?

Copy link
Contributor Author

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.

trigger.ts Outdated Show resolved Hide resolved
jobs/issueReminder.ts Outdated Show resolved Hide resolved
.env.example Outdated Show resolved Hide resolved
@Chigala
Copy link
Contributor Author

Chigala commented Mar 19, 2024

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

@ShubhamPalriwala
Copy link
Contributor

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 💪🏼

Copy link
Contributor

@ShubhamPalriwala ShubhamPalriwala left a 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:

@jobenjada jobenjada added this pull request to the merge queue Apr 1, 2024
Merged via the queue into formbricks:main with commit 85133cf Apr 1, 2024
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants