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

SIGNL4 Node: Attachment Support #1356

Merged
merged 6 commits into from
Jan 31, 2021
Merged

SIGNL4 Node: Attachment Support #1356

merged 6 commits into from
Jan 31, 2021

Conversation

rons4
Copy link
Contributor

@rons4 rons4 commented Jan 19, 2021

Attachment support added for the SIGNL4 node.

Attachment support added to SIGNL4.
Attachment support added for SIGNL4.
@CLAassistant
Copy link

CLAassistant commented Jan 19, 2021

CLA assistant check
All committers have signed the CLA.

@rons4
Copy link
Contributor Author

rons4 commented Jan 19, 2021

I signed the CLA and it said something like "thank you for signing" but it seems it does not recognize it here.

@lublak
Copy link
Contributor

lublak commented Jan 20, 2021

@rons4 jear. You need to force push to trigger it after signing.
That's how it worked for me.

@rons4
Copy link
Contributor Author

rons4 commented Jan 20, 2021

Thanks @lublak, I tried to change something and then did:
git push --force
It seems the CLA still shows as pending.
The CLA page shows: You have agreed to the CLA for n8n-io/n8n

@rons4
Copy link
Contributor Author

rons4 commented Jan 20, 2021

Hello, just in case someone has the same issue, I had some issues with signing the CLA. I just changes my GitHub email address for this request and now it seems to work.

@RicardoE105
Copy link
Contributor

@rons4 thanks for the contribution. However, can you please explain to me how is this different from what the node currently does? It seems like the only difference is that instead of sending the data using JSON, now it's sending the data using multipart/form-data. As far as I know, this node was working fine so why change it to send the data as multipart/form-data now?

Also now the operation alert:send returns a string instead of a JSON. This will break existing workflows that expect the data to be returned as JSON.

@rons4
Copy link
Contributor Author

rons4 commented Jan 22, 2021

Hi @RicardoE105, thanks and the new functionality is the ability to send an attachment now. Therefore, the whole encoding was changed. The attachment parameter was present before but unfortunately attachments did not work. Now you can send an alert with an image or audio attachment.

You are right, the node should stay compatible with previous versions and there was an issue with the return text. I changed this and it should return a JSON object again.

By the way, is there any way to use "https" instead of the "request" module?

@RicardoE105
Copy link
Contributor

@rons4 Gotcha. It's working because of a change in the API? Because I have in my mind, this was working just fine. Either way will check it out.

All HTTP request has to use the helper method this.helpers.request!(options) (Which uses the request library under the hood). I wonder why do you want to use the HTTP module instead. To send multipart-form/data using the request library check here.

Let me know if you have any questions.

@rons4
Copy link
Contributor Author

rons4 commented Jan 25, 2021

Hi @RicardoE105, thanks and indeed attachments never seemed to work. To be honest I am not sure at what point the attachments parameter came in here. Anyway, now it is used and sends the attachment through to SIGNL4. There was no API change of SIGNL4 in this regards.

I used the manual approach for multipart-form/data because I had this code available. My question to use the "https" library instead of "request" came up because I saw a discussion on the n8n community mentioning that "request" was depreciated and that it might get replaced by another method.

I hope this makes sense.

@RicardoE105
Copy link
Contributor

Hi @RicardoE105, thanks and indeed attachments never seemed to work. To be honest I am not sure at what point the attachments parameter came in here. Anyway, now it is used and sends the attachment through to SIGNL4. There was no API change of SIGNL4 in this regards.

I just checked, and I created the node with the attachment feature. Again, as far I remember, I tested it. You can check the commit here. Either way, if it does not, it does not work.

I used the manual approach for multipart-form/data because I had this code available. My question to use the "https" library instead of "request" came up because I saw a discussion on the n8n community mentioning that "request" was depreciated and that it might get replaced by another method.

Yes, exactly for that reason. We will soon be working on replacing the request library. If everybody uses the same method this.helpers.request!(options), we can change this function to change it for most of the nodes. I encourage you to use the built-in method.

As soon as that is added, I can review it again. Let me know if you have any questions. Thanks.

@rons4
Copy link
Contributor Author

rons4 commented Jan 27, 2021

OK, great and thanks. I already use this.helpers.request!(options) to send the request. So, I guess all should be fine here now.

RicardoE105 added a commit that referenced this pull request Jan 29, 2021
RicardoE105 added a commit that referenced this pull request Jan 29, 2021
janober added a commit that referenced this pull request Jan 31, 2021
* SIGNL4: Attachment Support

Attachment support added to SIGNL4.

* SI'GNL4: Attachment Support

Attachment support added for SIGNL4.

* Update GenericFunctions.ts

* Update GenericFunctions.ts

* Update GenericFunctions.ts

* Update GenericFunctions.ts

* ⚡ Improvements to #1356

Co-authored-by: rons4 <ron@signl4.com>
@janober janober merged commit 6ccc104 into n8n-io:master Jan 31, 2021
@janober
Copy link
Member

janober commented Feb 1, 2021

Got released with n8n@0.105.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants