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

Adding Google Chat node #2424

Closed

Conversation

valentina98
Copy link
Contributor

This node contains 5 resources with different operations.

Media
Download

Space
Get
Get All

Member
Get
Get All

Message
Create
Delete
Get
Webhook

Attachment
Get

@luizeof
Copy link
Contributor

luizeof commented Nov 10, 2021

@valentina98 I was doing one, but yours is perfect. a long-awaited node. good work !

@RicardoE105
Copy link
Contributor

Reviewed the node. Can you please address the issues below? Let me know if something it's not clear. I'm happy to help.

media:download

  • Ask for the binary property to add the downloaded data to but it does not use it in the code?
  • resource description should end with a dot
  • resource description should be download instead of downloads

space:getAll

  • Missing limit field when return all is false
  • Page token and page size are not needed there. Delete them.
  • It should return the array of objects. Get rid of the property spaces

member:get

  • fix description

member:getAll

  • Fix description
  • limit parameter missing when returnAll is set to false
  • Remove page size and page token
  • It should return the array of objects. Get rid of the property memberships

message:create

  • Parent name should be renamed to Space ID and Thread Key and Request ID are optional parameters, hence should be under a collection.
  • Looks like it's not possible to build a complex message from the UI without using JSON. I would refer her/him to the Slack node, specifically to the message blocks
  • Update mask should be handled behind the scenes

message:webhook

  • I would rename this else it's kind of confusing. Something like incomingwebhook:create

@valentina98
Copy link
Contributor Author

valentina98 commented Dec 6, 2021

@RicardoE105 thank you for the feedback!

media:download

  • I used the Next Cloud node as a reference for this endpoint. I think binaryPropertyName is used?
    const binaryPropertyName = this.getNodeParameter('binaryPropertyName', i) as string; items[i].binary![binaryPropertyName] = await this.helpers.prepareBinaryData(responseData, endpoint);
  • I didn't see the description not ending with a dot
  • downloads -> download

space:getAll

  • Added limit field when return all is false
  • Page size and page token removed
  • Returning the array of objects; got rid of the property spaces

member:get

  • Description fixed

member:getAll

  • Description fixed
  • Added limit field when return all is false
  • Page size and page token removed
  • Returning the array of objects; got rid of the property memberships

message:create

  • Parent Name renamed to Space Name. Thread Key and Request ID are under collection.
  • Yes, there is only the JSON option for complex messages. I've refered the Slack message blocks in the TODOs for future developmet
  • Update mask is handled behind the scenes

message:webhook

  • Renamed message:webhook -> incomingwebhook:create

@RicardoE105
Copy link
Contributor

@valentina98 thanks. Will review it again as soon as I can.

@RicardoE105
Copy link
Contributor

Looking much better @valentina98. Can you please address the changes below? After that, I will move the node forward internally. Thanks very much.

20201213

  • Run linter to fix some formatting issues

message:create

  • Load spaces from the API
  • Rename Json parameter message to JSON Parameters
  • Rename collection from Query parameters to Additional Fields
  • Add placeholder Add Field to collection

message:delete

  • Return { "success": true } instead of empty

message:update

  • Rename Json parameter message to JSON Parameters

member:getAll

  • Load spaces from the API

member:get

  • Rename Name to Space Name

@ivov ivov added node/new Creation of an entirely new node community Authored by a community member labels Dec 15, 2021
@ivov ivov mentioned this pull request Dec 20, 2021
@RicardoE105
Copy link
Contributor

@valentina98 any update about this?

@valentina98
Copy link
Contributor Author

I made the changes.
I'm sorry for the delay and thank you a lot for your feedback!

@RicardoE105
Copy link
Contributor

@valentina98 have a couple of questions for you.

  1. How did you test the attachment:get and media:get? I do not know what to set for
  2. Did you run the linter as I suggested in the latest review? I just did and there are still some issues that need to be addressed.

image

@valentina98
Copy link
Contributor Author

@RicardoE105

  1. I couldn't obtain an attacment name to test them. I searched a lot about it and I came to conclusion that bots can get only messages that they created
    https://stackoverflow.com/a/61870947
    and they have limted message types so they cannot send an attachment
    https://stackoverflow.com/a/67737236/8966141
    I used just the documentation to implemet tese two endpoints. And the Next Cloud node as a reference for the download endpoint.

  2. It is my mistake, now they are fixed.

image

The error in the picture is because of a dash in the url.

@RicardoE105
Copy link
Contributor

@valentina98 cool, then will remove those two endpoints in the meantime and review the rest. Thanks.

@valentina98
Copy link
Contributor Author

Thank you, the feedback was very useful for me and I'll apply it in future development of nodes. Please, let me know if there is something else.

@janober janober mentioned this pull request Feb 10, 2022
@janober
Copy link
Member

janober commented Feb 20, 2022

Thanks a lot! Got merged with #2795 and released with n8n@0.164.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Authored by a community member node/new Creation of an entirely new node
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants