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

Add SeaTable node #1976

Closed
wants to merge 1 commit into from
Closed

Add SeaTable node #1976

wants to merge 1 commit into from

Conversation

ktomk
Copy link
Contributor

@ktomk ktomk commented Jul 8, 2021

Node for SeaTable, initial credentials, trigger- and standard-node.

Contribution-by: SeaTable GmbH https://seatable.io
Signed-off-by: Tom Klingenberg tkl@seatable.io

@CLAassistant
Copy link

CLAassistant commented Jul 8, 2021

CLA assistant check
All committers have signed the CLA.

@ktomk ktomk marked this pull request as draft July 8, 2021 09:59
@ktomk ktomk marked this pull request as ready for review July 14, 2021 12:25
@ktomk ktomk changed the title WIP - Add SeaTable node Add SeaTable node Jul 15, 2021
@janober
Copy link
Member

janober commented Jul 17, 2021

Sorry for the delay. We try to review the PR asap.

@ktomk
Copy link
Contributor Author

ktomk commented Aug 10, 2021

Just refreshed on top of master as the PR got a bit stale @janober

@janober
Copy link
Member

janober commented Aug 10, 2021

Thanks! Very sorry. We will try to review now really asap.

@janober
Copy link
Member

janober commented Aug 10, 2021

Btw. had a very fast look.

It looks like it would have to be changed to work consistently as recent similar nodes like for example Baserow or NocoDB. You can find the code here:
https://github.com/n8n-io/n8n/tree/master/packages/nodes-base/nodes/Baserow
https://github.com/n8n-io/n8n/tree/master/packages/nodes-base/nodes/NocoDB

It allows users to set data in two different ways. One by using the incoming data directly and the other by setting it field by field.

Hope that makes sense.

@janober
Copy link
Member

janober commented Aug 10, 2021

Ah and additionally do we now have a node linter thanks to @ivov :
https://github.com/n8n-io/nodelinter

That already displays a lot of common issues of nodes. If all of that is done we should be able to merge the node very fast as there would not be much to do on our side anymore. Thanks!

@ktomk
Copy link
Contributor Author

ktomk commented Aug 10, 2021

It allows users to set data in two different ways. One by using the incoming data directly and the other by setting it field by field.

Can it be described on high level that a mapping of the input data is possible (on entity level) - instead of having a mapping node in front @janober ?

Ah and additionally do we now have a node linter thanks to @ivov

I'll take a look and then looking forward to integrate it. Much appreciated @janober

@janober
Copy link
Member

janober commented Aug 10, 2021

Yes, that is correct. But important to mention that both are possible and the user can select which way of setting the data he/she prefers. If you run the Baserow or NocoDB node in n8n it will become directly clear.

@ktomk
Copy link
Contributor Author

ktomk commented Aug 16, 2021

Please find this PR updated for nodelinter changes. As I had some problem getting it to run, please take a look this looks good so far @janober

@ktomk ktomk force-pushed the seatable-pr branch 2 times, most recently from 51692ec to 0ba664e Compare August 26, 2021 08:58
@ktomk
Copy link
Contributor Author

ktomk commented Aug 26, 2021

It allows users to set data in two different ways. One by using the incoming data directly and the other by setting it field by field.

Please find this in the updated PR incl. loading table- and column-names from SeaTable server (load-options methods) @janober

That [nodelinter] already displays a lot of common issues of nodes. If all of that is done we should be able to merge the node very fast as there would not be much to do on our side anymore.

I addressed those common issues, some of them look like false-positives to me and then I worked with the annotations.

Additionally I was able to crash node.js by running nodelinter.

@ktomk
Copy link
Contributor Author

ktomk commented Aug 26, 2021

Just updated from beta-3 to beta-4 which prevents crashing nodelinter and also the change to async credentials getting reminded to improve the nodes credentials handling which has been done as well. rtr.

Node for SeaTable, initial credentials, trigger- and standard-node.

Contribution-by: SeaTable GmbH <https://seatable.io>
Signed-off-by: Tom Klingenberg <tkl@seatable.io>
@ktomk
Copy link
Contributor Author

ktomk commented Sep 7, 2021

Refreshed against current head, please review @janober.

@janober
Copy link
Member

janober commented Sep 29, 2021

Thanks a lot. Got merged with #2240

@janober janober closed this Sep 29, 2021
@janober
Copy link
Member

janober commented Sep 30, 2021

Got released with n8n@0.141.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.

3 participants