CLI: collections - Upload data from google sheets #909
Conversation
|
@josephjclark Hi, this is an initial setup to outline how things might look. Currently, we can retrieve and parse data from the sheet. There are still some changes and validations I need to implement. I’ve created the PR so you can track the progress as well. Regarding the @openfn/language-googlesheets package, I encountered some unresolved errors, so I implemented a similar function within the utils as a workaround for now. I will try to use that if there is a similar implementation available. |
|
Hi @SatyamMattoo - great work! I'm actually on leave this week so I'll be a bit less active. But I'll take a little look now. Re the googlesheets adaptor, the idea is to use it as a reference rather than to use it directly. But so long as you have what you need, no problem |
| } | ||
|
|
||
| if (options.sheet && options.items) { | ||
| throwAbortableError( |
| `Upserting mapping from a Google Sheet to collection "${options.collectionName}"` | ||
| ); | ||
|
|
||
| let mapping; |
There was a problem hiding this comment.
May as well push mapping into the try/catch and make it const
| default: 1, | ||
| }, | ||
| }, | ||
| // We can have this later if we want |
There was a problem hiding this comment.
Yep - I fear this will be quite hard. I'll ask around and see if anyone has any insight.
| options.lightning || | ||
| process.env.OPENFN_ENDPOINT || | ||
| 'https://app.openfn.org'; | ||
| 'https://app.staging.openfn.org'; |
There was a problem hiding this comment.
Please make sure this is reverted!
To test against staging locally, you should set your own env var rather than commiting changes to code
| * | ||
| * @param sheetId - The Google Sheet ID or URL. | ||
| * @param options - Options for parsing: | ||
| * - range: The range to fetch (default: 'A1:Z'). (Ignored: we fetch the entire sheet.) |
There was a problem hiding this comment.
I'd generally advise against writing extensive documentation like this for internal functions - especially in an early stage PR. The only people who need it are developers working on the CLI, and they should be able to read options from the source code (especially if it's well written). Writing this documentation (which is really high quality!) takes a lot of time, and sooner or later will drift out of date.
| options: Partial<SetOptions> | ||
| ): Promise<Record<string, any>> { | ||
| // Fetch sheet data using our custom function. | ||
| const sheetData = await getPublicSheetValues(sheetId, options.range || 'A1:Z', options.worksheetName || 'Sheet1'); |
There was a problem hiding this comment.
I'd love to be able to just use the first sheet, if there's only one in the document, regardless of its name. Maybe this is the sort of thing the sheets client can help us with.
| * | ||
| * @returns A promise that resolves to the mapping object. | ||
| */ | ||
| export async function parseGoogleSheet( |
There was a problem hiding this comment.
I'd love to see some unit tests around this - even at this early state - because this is the meat of the implementation.
If I was writing this feature, I'd rename the function to parseValues(), pass the values directly as an array (ie, do the sheet loading stuff separately and divorce the "download data from sheets" and "process sheet data steps" entirely). Then I'd write a bunch of unit tests against parseValues with different options and datasets, to really make sure (to myself, my reviewer, and to future developers) that the parsing functionality was working properly.
|
Hi @SatyamMattoo - I think the next step here is to get the sheets client working instead of using rest. Do you need help with that? What was the error you were getting? |
|
Hi @josephjclark Sorry for the delayed response. I was caught up with the work, could not give time to this. I will update you this weekend regarding this (will try to make things working along with the sheets client). I was trying to use the adaptor itself, that might be causing the error. I'll update the function with the OAuth thing, will be easier to debug and will hopefully work as well. |
|
@SatyamMattoo yep, don't use the adaptor directly. Copy code out of its useful. Documentation on the sheets client is dreadful, but usage in the adaptor should have everything you need really |
2 participants
Short Description
Allow users to upload data to collections from public google sheets.
Fixes #902
Implementation Details
QA Notes
Will add tests once this is complete.
AI Usage
Please disclose how you've used AI in this work (it's cool, we just want to know!):