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

📱Kobotoolbox API and Trigger nodes #2510

Closed
wants to merge 24 commits into from
Closed

📱Kobotoolbox API and Trigger nodes #2510

wants to merge 24 commits into from

Conversation

Yann-J
Copy link
Contributor

@Yann-J Yann-J commented Dec 1, 2021

KoboToolbox is a popular open-source field survey / data collection tool largely used by humanitarian organizations.

This PR has support for most API calls (no trigger...): fetching form definitions, submissions, and webhooks.

All tested, code linted, all should be pretty polished...

@CLAassistant
Copy link

CLAassistant commented Dec 1, 2021

CLA assistant check
All committers have signed the CLA.

@Yann-J Yann-J changed the title Kobotoolbox node 📱Kobotoolbox API and Trigger nodes Dec 3, 2021
@Yann-J
Copy link
Contributor Author

Yann-J commented Dec 3, 2021

Hello there!

Sorry for the multiple commits, but I kept thinking of more features to add, including light reformatting of the form submission data, and a trigger node to receive submission notifications.

The PR is now finished as far as I can tell - tested, linted and snyk check finally passed!

@Yann-J
Copy link
Contributor Author

Yann-J commented Dec 14, 2021

Hey @janober wondering if there's anything I can do to help with the review? I've been happily adding some polish over the last few days but I think it's basically finished...

Let me know if it looks good!

@Yann-J
Copy link
Contributor Author

Yann-J commented Dec 15, 2021

X-ref the corresponding docs PR: n8n-io/n8n-docs#678

@ivov ivov added node/new Creation of an entirely new node community Authored by a community member labels Dec 15, 2021
@Yann-J
Copy link
Contributor Author

Yann-J commented Jan 12, 2022

Hello there @ivov @janober ... Let me know if there's anything I can do for a speedy review... we are eagerly waiting for this node to be available in our setup...

@csuermann
Copy link
Contributor

Hi @Yann-J, sorry for the delay. We have a growing number of nodes to review but this one should be coming up quite soon.

Copy link
Contributor

@RicardoE105 RicardoE105 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Yann-J got reviewed. Let me know if you have questions about the comments.

@Yann-J
Copy link
Contributor Author

Yann-J commented Jan 18, 2022

Thanks a lot @RicardoE105 for the thorough review!

I've implemented most of the suggested changes!

The only exceptions are:

  • I'm reluctant to rename the submission query operation to "Get All", because I expect it would be quite rare to really actually get all records - this will often be a lot. I suspect most users will use a query filter, so query feels more appropriate here...
  • I have not implemented support to dynamically load the form ids from the UI... I understand it would be convenient, but I think it can be done later as I lack time to do this properly (there's a little bit of work since the formList API is actually served by a different domain from the main one, and is at risk of deprecation), and I'm a little concerned that this can be a lot for heavy users (many organizations would share all forms with all users...).

Somehow, credentials testing always seems to work for me, on all the servers I've tested this on... I couldn't figure out anything problematic...

@Yann-J
Copy link
Contributor Author

Yann-J commented Jan 21, 2022

OK @RicardoE105 so I was curious about this possibility of looking up the form ID, and gave it a shot - it does make the experience smoother for sure!

Added lookup in both API and Trigger nodes...

Thanks for the tip!

@RicardoE105
Copy link
Contributor

Great that it was helpful.

I will review it again when I have time.

@RicardoE105
Copy link
Contributor

@Yann-J ok gave it a try and found a couple more issues. Can you please address those? Then I will do more testing, fix whatever is left (it should be none or really small stuff), and move internally in the node pipeline. I tried to address them myself since it should be very quick but did not find the Rest API docs. Let me know if you have any questions about my comments.

@RicardoE105
Copy link
Contributor

Also, if you know where the docs about the API are, let me know. Thanks.

@RicardoE105
Copy link
Contributor

Also, I just noticed that the name of the tool is KoBoToolbox, but you used it in the node KoboToolbox. This needs to be updated in the node and in the credentials.

@Yann-J
Copy link
Contributor Author

Yann-J commented Jan 25, 2022

Indeed @RicardoE105 , I believe the new spelling is due to a new branding... I've updated it everywhere.

Regarding the API docs, AFAIK they're not publicly available, but you can browse the live API endpoints if you are logged in to a running instance, e.g. at https://kf.kobotoolbox.org/api/v2/assets/ - the HTML views include the docs.

Thanks for everything!

@RicardoE105
Copy link
Contributor

@Yann-J any update on this?

@Yann-J
Copy link
Contributor Author

Yann-J commented Feb 2, 2022

Hi @RicardoE105 yes, I believe my latest commit has addressed the branding change.

Is there anything else I owe you?

@RicardoE105
Copy link
Contributor

RicardoE105 commented Feb 2, 2022

@Yann-J Ah looks like I never submitted the changes. My bad.

name: 'assetUid',
type: 'options',
typeOptions: {
loadOptionsMethod: 'loadSurveys',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not load the forms for the operation form:get.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is that?

name: 'start',
type: 'number',
displayOptions: {
show: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the operation submission:query the pagination is missing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The query collection should be rename filters, we need to remove the sort and fields parameter. Those two parameters should be moved under options collection.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Get rid of the parameter start since the pagination should be done for the user.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename collections' button name. By default it says "choose option to add" but it should be "Add Option". You can this name by setting the property placeholder to the collection.

image

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I must say I'm not really convinced about this split menu structure, keeping filters, sort, and fields under the same Query Options collection make more sense to me, as they're the 3 typical options you'd expect from a query operation.

Besides they all have the same displayOptions (when I try to move some under the generic options collection, I'm getting a Could not resolve parameter depenencies. Max iterations reached! error...)...

Also I've kept the start parameter because I'm certain that some workflows will want to implement their own pagination (somehow... it might be tough... but some forms will have millions of submissions, we can't load them all in memory...).

@Yann-J
Copy link
Contributor Author

Yann-J commented Feb 3, 2022

OK yes you're right @RicardoE105 , I should be handling the pagination, I will work on it... However I would still think keeping the start parameter is useful, because I think there would be cases where the user might want to handle pagination themselves - e.g. to avoid fetching 500k records (which is the case on some of our forms), and process them by batch.
It won't be the case for forms and hooks presumably, but it could be for submissions and hook logs...

@Yann-J
Copy link
Contributor Author

Yann-J commented Feb 3, 2022

Hmm, on the other hand, the node will only return the result list anyway, and not the pagination links, so it might be hard to implement pagination outside the node anyway...

@Yann-J
Copy link
Contributor Author

Yann-J commented Feb 3, 2022

OK @RicardoE105 , I believe this last commit has addressed most comments... I don't fully agree with some of the menu structure changes, so I've kept them as is, but pagination is alive and well on all list-based operations.

RicardoE105 added a commit that referenced this pull request Feb 7, 2022
@RicardoE105
Copy link
Contributor

@Yann-J could not test the hook resource. When I create a hook on a form and then try to retrieve all the hooks in that form using hook:getAll I always get empty. Any idea what I might be doing wrong?

@Yann-J
Copy link
Contributor Author

Yann-J commented Feb 8, 2022

Hmm... it seems to be working for me. Could it be a permission issue, e.g. if you created the hook with a different user than the one used by n8n?

image

I've pushed a commit that fixed a linting error... maybe that was causing a build issue or something.

@Yann-J
Copy link
Contributor Author

Yann-J commented Feb 9, 2022

FYI, just committed one last change to lookup hook IDs

@RicardoE105
Copy link
Contributor

@Yann-J quick update. I'm waiting for a PR extending the method httpRequest to be merged so move forward with this PR. That functionality would allow following the redirects using the httpRequest method instead of having import axios to do it.

@Yann-J
Copy link
Contributor Author

Yann-J commented Feb 16, 2022

Ok thanks Ricardo... Hope this can happen soon since we're really eager to use this one...

@Yann-J
Copy link
Contributor Author

Yann-J commented Mar 3, 2022

Hey there @RicardoE105 wondering if there's any update here... we've been really waiting for this node...

@RicardoE105
Copy link
Contributor

@Yann-J sorry for the late response. Somehow missed the notification. The PR I was waiting for got merged. So I'm gonna move it forward this week. Hopefully will be available in the next release.

janober added a commit that referenced this pull request Mar 20, 2022
* First version

* Added hooks

* Added Credentials test

* Add support for downloading attachments

* Slight restructure of downloaded binaries

* Added Trigger node

* Some linting

* Reverting package-lock changes

* Minor GeoJSON parsing fixes

* KoboToolbox: improve GeoJSON format

* Kobo: Support for get/set validation status

* Remove some logs

* [kobo] Fix default attachment options

* Proper debug logging

* Support for hook log status filter

* Kobo: Review fixes

* [kobo]: Add Get All Forms + lookup Form ID

* [kobo] Lookup Form ID in Trigger node

* [kobo] Update branded spelling

* [kobo] Support pagination

* ⚡ fix linting issue

* ⚡ Improvements to #2510

* ⚡ Download files using n8n helper

* ⚡ Improvements

* ⚡ Improvements

* 🐛 Fix filenames

* ⚡ Fix some issues

Co-authored-by: Yann Jouanique <[email protected]>
Co-authored-by: Jan Oberhauser <[email protected]>
@janober
Copy link
Member

janober commented Mar 20, 2022

Got merged with #2765 and released with [email protected]

Wowasshi pushed a commit to IT-Global-SVC/n8n that referenced this pull request Mar 30, 2022
* ✨ Make it possible to download binary data

* ⚡ Fix lint issues and add support for filesystem mode

* ⚡ Design adjustment

📜 Change to the Sustainable Use License n8n-io#2932

🚨 Temporarily skip some regularly failing tests (n8n-io#3002)

feat: Add support for reading ids from file with executeBatch command (n8n-io#3008)

feat(Mattermost Node): Add support for Channel Search (n8n-io#2687)

* Squashed commit of the following:

commit 9f76bdca9b4af4fd3ee429d1c381c3ed5529434c
Author: Matt Walther <[email protected]>
Date:   Sun Jan 16 16:40:34 2022 -0600

    Mattermost Channel Search

* Add more boilerplate so Search action renders

* Changed order of search to make the operations alphabetical

* :zap: Add pagination

bug(EmailReadImap Node): Improve error handling (n8n-io#2991)

* Fix: EmailReadImap unhandled promise rejection

Related to n8n-io#2091 (but only partially)

See n8n-io#2091 (comment)

* Send errors from email read imap to logger

feat(HTTP Request Node): Allow Delete requests with body (n8n-io#2900)

delete request with body parameters

feat(KoBoToolbox Node): Add KoBoToolbox Regular and Trigger Node (n8n-io#2765)

* First version

* Added hooks

* Added Credentials test

* Add support for downloading attachments

* Slight restructure of downloaded binaries

* Added Trigger node

* Some linting

* Reverting package-lock changes

* Minor GeoJSON parsing fixes

* KoboToolbox: improve GeoJSON format

* Kobo: Support for get/set validation status

* Remove some logs

* [kobo] Fix default attachment options

* Proper debug logging

* Support for hook log status filter

* Kobo: Review fixes

* [kobo]: Add Get All Forms + lookup Form ID

* [kobo] Lookup Form ID in Trigger node

* [kobo] Update branded spelling

* [kobo] Support pagination

* ⚡ fix linting issue

* ⚡ Improvements to n8n-io#2510

* ⚡ Download files using n8n helper

* ⚡ Improvements

* ⚡ Improvements

* 🐛 Fix filenames

* ⚡ Fix some issues

feat(Linear Node): Add Linear Node (n8n-io#2971)

* ✨ Linear node

* ⚡ Improvements

fix(GitHub Node): Fix credential tests and  File > List operation (n8n-io#2999)

* Fixed credential test failing

* Fixed File list operation not working

fix(Telegram Node): Fix sending binary data when disable notification is set (n8n-io#2990)

feat(Mailjet Node): Add credential tests and support for sandbox, JSON parameters & variables (n8n-io#2987)

* Add Variables JSON to Mailjet Batch send

* ⚡ Improvements

* ⚡ Add credential verification

* ⚡ Small improvement

⬆️ Update package-lock.json file

🔖 Release [email protected]

⬆️ Set [email protected] on n8n-core

🔖 Release [email protected]

⬆️ Set [email protected] and [email protected] on n8n-node-dev

🔖 Release [email protected]

⬆️ Set [email protected] and [email protected] on n8n-nodes-base

🔖 Release [email protected]

🔖 Release [email protected]

⬆️ Set [email protected] and [email protected] on n8n-editor-ui

🔖 Release [email protected]

⬆️ Set [email protected], [email protected], [email protected] and [email protected] on n8n

🔖 Release [email protected]

📚 Update CHANGELOG.md

📚 Fix CHANGELOG.md file

⚡ Add Odoo and RedisTrigger node codex (n8n-io#3005)

* .168.2fixed: Auto stash before rebase of "refs/heads/codex/0.168.2fixed"

Odoo and Redis Trigger codex files update

* Update RedisTrigger.node.json

:zap: Add KoBoToolbox and Linear codex files (n8n-io#3040)

KoBoToolbox
KoBoToolbox Trigger
Linear

:books: Add missing full stop to license text

* (fix): Added missing full stop to license

GitHub does not render the single line breaks in the *Limitations* section. The added full stop makes it easier to read our license.

* :books: Add also to other files

fix(AWS Lambda Node): Fix "Invocation Type" > "Continue Workflow" (n8n-io#3010)

* 🔨 fix for running in continue workflow

* ⚡ Minor simplification

📚 Add one more missing full stop to license text

fix(core): Add logs and error catches for possible failures in queue mode (n8n-io#3032)

fix(Supabase Node): Fix Row > Get operation (n8n-io#3045)

fix(Supabase Node): Send token also via Authorization Bearer (n8n-io#2814)

Send Authorization Bearer in headers
Fix typo in validateCredentials function

fix(Wise Node): Fix issue when executing a transfer (n8n-io#3039)

:zap: Fix credentials import success message (n8n-io#3038)

:books: Add missing full stop to license text (n8n-io#3028)

Adding "." L15.

In addition, the markdown display don't show line break as in the editor.

:books: Add note to changelog linking to historic log (n8n-io#3031)

feat(HTTP Request Node): Add support for OPTIONS method (n8n-io#3030)

fix(Xero Node): Fix some operations and add support for setting address and phone number (n8n-io#3048)

* 🐛 Fix issue when sending Organization ID - Xero node

* 👕 Fix linting issue

feat(Crypto Node): Add Generate operation to generate random values (n8n-io#2541)

* ✨ Add generate action to crypto node

* ⚡ small fixes, nodelinter issues fixes

* ⚡ Improvements

* ⚡ Fix order

feat(Reddit Node): Add possibility to query saved posts (n8n-io#3034)

* chore: add nvmrc with required node version

* feat: added saved posts to reddit node with credentials on User resource

* Changed Details order

* Fixed lint issue

* Moved saved posts to profile as it only works for the logged in user, This avoids the breaking change

* Removed .nvmrc

* ⚡ Improvements

feat(Jira Node): Add Simplify Output option to Issue > Get (n8n-io#2408)

* ✨ Add option to use Jira field display names

* 🚸 Make mapped fields more deterministic

* ♻️ Refactor Jira user loadOptions

* Moved and renamed the option as well as only returning the fields to

* Tweaked Friendly Fields to make it "Simplify Output" following similar patterns to other nodes

* ⚡ Improvements

feat(Zendesk Node): Add ticket status "On-hold"

🔖 Release [email protected]

⬆️ Set [email protected] on n8n-core

🔖 Release [email protected]

⬆️ Set [email protected] and [email protected] on n8n-node-dev

🔖 Release [email protected]

⬆️ Set [email protected] and [email protected] on n8n-nodes-base

🔖 Release [email protected]

🔖 Release [email protected]

⬆️ Set [email protected] and [email protected] on n8n-editor-ui

🔖 Release [email protected]

⬆️ Set [email protected], [email protected], [email protected] and [email protected] on n8n

🔖 Release [email protected]

⬆️ Update package-lock.json file

📚 Update CHANGELOG.md with version 0.170.0

Co-Authored-By: Jonathan Bennetts <[email protected]>
Co-Authored-By: ricardo <[email protected]>
Co-Authored-By: Manuel [tennox] <[email protected]>
Co-Authored-By: Justin Halter <[email protected]>
Co-Authored-By: Yann Jouanique <[email protected]>
Co-Authored-By: Jan Oberhauser <[email protected]>
Co-Authored-By: Marcin Koziej <[email protected]>
Co-Authored-By: Niv <[email protected]>
Co-Authored-By: michael-radency <[email protected]>
Co-Authored-By: Yassine Fathi <[email protected]>
@nivbe06 nivbe06 closed this Mar 31, 2022
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.

7 participants