PipedreamHQ / pipedream

Connect APIs, remarkably fast. Free for developers.
https://pipedream.com
Other
8.32k stars 5.27k forks source link

Fix sendgrid ASM config #5673

Closed RtpChris closed 1 year ago

RtpChris commented 1 year ago

Currently the ASM config doesn't work with the sendgrid actions. This is because the Sendgrid API requires the group_id and other params to be integers, not strings.

This code parses the ASM object as integers if set. It also adds the two ASM subfields as their own parameters so that the Sendgrid API can be queried for the groups (like the templateId param)

Sendgrid API reference: https://docs.sendgrid.com/api-reference/mail-send/mail-send

vercel[bot] commented 1 year ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
pipedream-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 3, 2023 5:52pm
pipedream-docs-redirect-do-not-edit ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 3, 2023 5:52pm
pipedream-component-development commented 1 year ago

Thank you so much for submitting this! We've added it to our backlog to review, and our team has been notified.

pipedream-component-development commented 1 year ago

Thanks for submitting this PR! When we review PRs, we follow the Pipedream component guidelines. If you're not familiar, here's a quick checklist:

RtpChris commented 1 year ago

Here's the error that we get currently when trying to use this:

Unexpected error (status code: 400): { "headers": { "server": "nginx", "date": "Fri, 10 Mar 2023 00:51:02 GMT", "content-type": "application/json", "content-length": "300", "connection": "close", "access-control-allow-origin": "https://sendgrid.api-docs.io", "access-control-allow-methods": "POST", "access-control-allow-headers": "Authorization, Content-Type, On-behalf-of, x-sg-elas-acl", "access-control-max-age": "600", "x-no-cors-reason": "https://sendgrid.com/docs/Classroom/Basics/API/cors.html", "strict-transport-security": "max-age=600; includeSubDomains" }, "body": { "errors": [ { "message": "Invalid type. Expected: object, given: string.", "field": "personalizations.0.bcc.0", "help": "http://sendgrid.com/docs/API_Reference/Web_API_v3/Mail/errors.html#message.personalizations.bcc" }, { "message": "Invalid type. Expected: object, given: string.", "field": "asm", "help": null } ] } }

Also posted here https://pipedream.com/community/t/fixing-sendgrid-component-asm-fields/5047

vunguyenhung commented 1 year ago

Hello everyone, I have tested this PR and there're some test cases failed or needed improvement.

Please check the test report below for more information SendGrid_5673_2151.pdf

RtpChris commented 1 year ago

I've added a new validator for the asmGroupsToShow, set asmGroupsToShow as optional, and have the fetch return undefined if the user doesn't have any ASMs configured

RtpChris commented 1 year ago

@michelle0927 - Thanks! I think I've gotten the conflict resolved; sorry for the mess!

vunguyenhung commented 1 year ago

Hello @RtpChris, I'm still facing error while fetching for ASM Group ID and ASM Groups to Display

13 41 31 - 30 - 03 - 2023

image

Would you mind checking again and sharing the steps taken to test this?

RtpChris commented 1 year ago

@vunguyenhung - I tested it locally as well as I could, but I'm probably missing something. I think this time I screwed up by returning undefined instead of an int

Does the sendgrid account you're using have any suppression groups defined? I'm still getting familiar with the pipe dream code and I'm not entirely sure how to return a custom error message for that (I based the options code on the work that was already there)

michelle0927 commented 1 year ago

@vunguyenhung - I tested it locally as well as I could, but I'm probably missing something. I think this time I screwed up by returning undefined instead of an int

Does the sendgrid account you're using have any suppression groups defined? I'm still getting familiar with the pipe dream code and I'm not entirely sure how to return a custom error message for that (I based the options code on the work that was already there)

@RtpChris Sorry I overlooked this during review. The prop options are expecting an array. Try returning [] instead of undefined.

     async options() {
        const asmGroups = await this.listAsmGroups();
        if (asmGroups) {
          return asmGroups.map((group) => ({
            label: group.name,
            value: group.id,
          }));
        }
        return [];
      },
RtpChris commented 1 year ago

@vunguyenhung - I tested it locally as well as I could, but I'm probably missing something. I think this time I screwed up by returning undefined instead of an int Does the sendgrid account you're using have any suppression groups defined? I'm still getting familiar with the pipe dream code and I'm not entirely sure how to return a custom error message for that (I based the options code on the work that was already there)

@RtpChris Sorry I overlooked this during review. The prop options are expecting an array. Try returning [] instead of undefined.

     async options() {
        const asmGroups = await this.listAsmGroups();
        if (asmGroups) {
          return asmGroups.map((group) => ({
            label: group.name,
            value: group.id,
          }));
        }
        return [];
      },

Ahh ok thanks! I've made those changes

dannyroosevelt commented 1 year ago

@michelle0927 @vellames what's left here to get this merged?

vellames commented 1 year ago

It was just a comment left in the code, not a big deal, we can move forward with this if you want

binamov commented 1 year ago

can this be merged?

dannyroosevelt commented 1 year ago

@binamov looks like it's waiting to be QA'd -- have you gotten a chance to test the changes end to end?

binamov commented 1 year ago

@binamov looks like it's waiting to be QA'd -- have you gotten a chance to test the changes end to end?

nope. I'm blocked with this integration as well, i'm trusting the numerous positive responses before mine.

dannyroosevelt commented 1 year ago

@vunguyenhung @michelle0927 what's the latest on this one?

vercel[bot] commented 1 year ago

@michelle0927 is attempting to deploy a commit to the Pipedreamers Team on Vercel.

A member of the Team first needs to authorize it.

dannyroosevelt commented 1 year ago

@michelle0927 https://github.com/PipedreamHQ/pipedream/actions/runs/4874592849/jobs/8695733981?pr=5673

michelle0927 commented 1 year ago

Testing passed. Ready for release!

Test report Sendgrid_5673.pdf