PipedreamHQ / pipedream

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

Airtable Create & Update Record actions - Column names as distinct props #2600

Closed js07 closed 1 year ago

js07 commented 2 years ago

To add a column names as props, the actions use the airtable app prop's .table() method in the additionalProps. .table's return value is an Airtable table schema, which includes an array of fields (i.e., columns).

Part of #2536

vercel[bot] commented 2 years ago

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

pipedream-docs-redirect-do-not-edit – ./docs

🔍 Inspect: https://vercel.com/pipedreamers/pipedream-docs-redirect-do-not-edit/7LiyYFWFKf8FhncLG2RGqpjyqhcr
✅ Preview: https://pipedream-docs-redirect-do-not-edit-git-for-8a1b4f-pipedreamers.vercel.app

pipedream-docs – ./docs

🔍 Inspect: https://vercel.com/pipedreamers/pipedream-docs/AiGpCr6MRz5snt2ZwH8MPhSs138t
✅ Preview: https://pipedream-docs-git-fork-js07-airtable-table-6dd2f9-pipedreamers.vercel.app

dylburger commented 2 years 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:

vercel[bot] commented 2 years ago

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

Name Status Preview Updated
pipedream-docs ✅ Ready (Inspect) Visit Preview May 24, 2022 at 4:12PM (UTC)
pipedream-docs-redirect-do-not-edit ✅ Ready (Inspect) Visit Preview May 24, 2022 at 4:12PM (UTC)
dannyroosevelt commented 2 years ago

@js07 What is the value we're shwoing here? Is this the JSON of the table itself? Is there any slightly more user-friendly value we could display here? Not a big deal if not, just looks confusing IMO. image

dannyroosevelt commented 2 years ago

@js07 can you add a $summary message similar to the GSheets Add Single Row action? image

dannyroosevelt commented 2 years ago

@js07 is the create-single-record action the only one that got updated w/ the new $.airtable.table prop? Can we also refactor some of the other actions (especially the create-multiple-records action)?

js07 commented 2 years ago

@js07 What is the value we're shwoing here? Is this the JSON of the table itself? Is there any slightly more user-friendly value we could display here? Not a big deal if not, just looks confusing IMO.

Yes, this is the JSON of the table schema. We could do something like base64 encode it in the API and decode it in the component, so it appears as a base64 string (e.g. YXdwb2VmdWkgYXdlanFpb2pvaWZqYXdmaXEyamYwYW8gc2RqZmFsO2tzZGogZmFqZW93aWpmIGE5O3dqIDRhdyA4OTNqMnE4OWFqIGZhOTgyaiBmOTg0cTJqIGE5OGozZjRqOThhajI5YWogOThqZmE5ODJqNDkyYTkzODI0amE5OGY=)

js07 commented 2 years ago

@js07 is the create-single-record action the only one that got updated w/ the new $.airtable.table prop? Can we also refactor some of the other actions (especially the create-multiple-records action)?

The create-single-record and update-record actions got updated with the new $.airtable.table prop.

Refactor the create-multiple-records action to add column name props in additionalProps?

dannyroosevelt commented 2 years ago

@js07 What is the value we're shwoing here? Is this the JSON of the table itself? Is there any slightly more user-friendly value we could display here? Not a big deal if not, just looks confusing IMO.

Yes, this is the JSON of the table schema. We could do something like base64 encode it in the API and decode it in the component, so it appears as a base64 string (e.g. YXdwb2VmdWkgYXdlanFpb2pvaWZqYXdmaXEyamYwYW8gc2RqZmFsO2tzZGogZmFqZW93aWpmIGE5O3dqIDRhdyA4OTNqMnE4OWFqIGZhOTgyaiBmOTg0cTJqIGE5OGozZjRqOThhajI5YWogOThqZmE5ODJqNDkyYTkzODI0amE5OGY=)

No need, we just typically show an ID or something similar as the value, so the JSON was surprising, but that's fine.

vellames commented 2 years ago

@js07 We have one conflict here

vercel[bot] commented 2 years ago

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

A member of the Team first needs to authorize it.

vunguyenhung commented 1 year ago

Hi everyone, I have tested the changes in this PR and they're all good! The actions having columns as prop fields are:

Test report AirTable_PR_2600.pdf

Though @dannyroosevelt, could you confirm if those 2 actions are enough for closing the related issue? I see there's another action Create Multiple Records that doesn't have columns as prop fields improvement, but I suspect that Pipedream Action UI couldn't support it. What do you think?

dannyroosevelt commented 1 year ago

These are great changes! Nice job @js07!

The only request I'd ask for is that we throw new ConfigurationError when we cannot fetch either a manually referenced Base ID or Table ID (links to docs).

@js07 do you want to implement, or should we hand that off to another dev?

js07 commented 1 year ago

@dannyroosevelt Should we throw new ConfigurationError when we cannot fetch a Base ID or Table ID when configuring and when running the action? I.e., in the additionalProps and run methods, for all of the Airtable actions?

js07 commented 1 year ago

@dannyroosevelt Thanks for the feedback. I made a couple changes and have a few questions.

ConfigurationError in async options

delete-record-options-error

Would we prefer to show an empty list of options here instead of an error here (image above)? In the case where a user references an exported value from a previous step, we also show an error here:

delete-record-export-config-error

ConfigurationError in additionalProps

create-single-record-config-error-2

If we make an Airtable API request in the run method using a Base ID or Table ID that's not found, we currently display the error from Airtable in this format:

list-records-not-found

Would we prefer to show a ConfigurationError with a "not found" message instead?

js07 commented 1 year ago

If the Base ID and Table ID inputs are specified using an expression (e.g., {{steps.code.$return_value.baseId}}, additional props aren't generated (because the action doesn't know what the values will be at runtime).

airtable-create-single-record-config-error

Do we want to support dynamic Base ID and Table ID inputs (i.e., referenced step exports) in the Create Single Record action–like the prod actions do–using an Record object prop?

airtable-create-single-record-old
dannyroosevelt commented 1 year ago

@js07 I just threw some time on our calendars to chat about these open questions tomorrow so we can hopefully nail down answers asap.

js07 commented 1 year ago

@dannyroosevelt I made these changes:

Do the descriptions for Table, View, and Sort: Field need any changes?

dannyroosevelt commented 1 year ago

@dannyroosevelt I made these changes:

  • Updated the descriptions of the Table, View, and Sort: Field props to suggest entering a custom expression if the previous prop input references dynamic data from a previous step (2b037da, 21adadd)
  • Changed Create Single Record and Update Record actions to add a Record field instead of column name fields when a table is not found in additionalProps and the Table prop is set manually (i.e., could be dynamic) (b13ef25)

Do the descriptions for Table, View, and Sort: Field need any changes?

The last suggestion I'd make is to include an example in the description for the record prop. Then let's ship it! Since it's gone through PR review and QA I'm good to publish straightaway if you are.