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.

Sign up for GitHub

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

Jump to bottom

[EPIC] Google Ads Source to Beta #10786

Closed
8 tasks done
edgao opened this issue Mar 2, 2022 · 7 comments
Closed
8 tasks done

[EPIC] Google Ads Source to Beta #10786

edgao opened this issue Mar 2, 2022 · 7 comments
Assignees
@edgao
Labels
area/connectors Connector related issues certification connectors/source/google-ads connectors/sources-api
Milestone
ConnCore March 9,...

Comments

@edgao
Copy link
Contributor

edgao commented Mar 2, 2022 •
edited
Loading

@edgao edgao added the area/connectors Connector related issues label Mar 2, 2022
@edgao edgao added this to the ConnCore March 9, 2022 milestone Mar 2, 2022
This was referenced Mar 8, 2022
Closed
Closed
Closed
Closed
@edgao
Copy link
Contributor Author

edgao commented Mar 16, 2022

@misteryeo this is how errors from the "test connection" button get displayed, is this something that needs to be fixed before beta? (note the \\\ and \n)

Screen Shot 2022-03-16 at 11 49 55 AM

@misteryeo
Copy link
Contributor

misteryeo commented Mar 19, 2022 •
edited
Loading

Ooh, thanks for surfacing this @edgao. We should definitely fix this - do you know if this is just how we're parsing the error returned from Google Ads API directly?

Or would this parsing issue go across all connectors?

@sherifnada feels like we should add this to the checklist when certifying. WDYT?

@sherifnada
Copy link
Contributor

If I had to guess, the problem is that because this is rendered as HTML, newlines expressed as \n are not rendered correctly because they need to be expressed using <br />. This seems to me like something the UI should address? Sort of like a "string to HTML text" conversion?

WDYT @timroes

@edgao
Copy link
Contributor Author

edgao commented Mar 21, 2022

I was more thinking that the connector should extract the error message rather than just converting the raw api response to a string and dumping that into the UI. Haven't looked super closely at this, but it seems doable (and much more in-scope than a UI change)

@sherifnada
Copy link
Contributor

@edgao ah got it -- seems simple enough. Works with me

@timroes
Copy link
Collaborator

timroes commented Mar 21, 2022

@sherifnada As you mentioned \n isn't working because even if we render it in HTML it's not causing a line break. While <br /> should be working I'd in general highly discourage trying to use it explicitly (like in connector specs), since it usually means you make assumption about layout, which you should not try to do on the backend side.

In this specific case where we're rendering JSON that's now not correctly formatted, I'd go with @edgao: We should try not to have JSON rendered in a string error message. No matter whether we'll get newlines to work or not, this will look horrible in the UI. We should rather shorten it into a human readable error, and if we want to have the full benefit of the JSON still available in the UI, we should have this as a separate field and actually render it dedicated in the UI in some modal/tooltip/etc as advanced information.

@edgao edgao mentioned this issue Mar 23, 2022
Merged
16 tasks
@edgao
Copy link
Contributor Author

edgao commented Mar 30, 2022

new issues for the streams that fivetran supports that we currently do not:

@edgao edgao mentioned this issue Mar 30, 2022
Closed
@edgao edgao closed this as completed Mar 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@edgao edgao

Labels
area/connectors Connector related issues certification connectors/source/google-ads connectors/sources-api
Projects
None yet
Milestone
ConnCore March 9, 2022
Development

No branches or pull requests
5 participants
@bleonard @timroes @edgao @sherifnada @misteryeo