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

feat: add png rendering action to the Metabase piece #93

Merged

Conversation

valentin-mourtialon
Copy link

@valentin-mourtialon valentin-mourtialon commented Feb 3, 2025

What does this PR do?

Add the getQuestionPngPreview action to the Metabase piece.

if (response.error) {
throw new Error(response.error);
}
return response;

Choose a reason for hiding this comment

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

What's the output? a URL, a base64 image, or something else?

Copy link
Author

Choose a reason for hiding this comment

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

It's an entire HTML file (but when I open it in the browser I can see the PNG)

Copy link
Author

Choose a reason for hiding this comment

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

Is that expected?

Copy link
Author

Choose a reason for hiding this comment

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

When I test "Get Question" locally, I also get an HTML file so I thought that was expected. However, when I execute "Get Question" in staging it returns a JSON...

Choose a reason for hiding this comment

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

You're supposed to get a JSON indeed - the html is possibly an error page?
For the PNG: can you try calling the API manually with the token (e.g. with curl) to see the exact expected response? I'm surprised it's HTML - we can work with that but it's more difficult than needed

Copy link
Author

@valentin-mourtialon valentin-mourtialon Feb 3, 2025

Choose a reason for hiding this comment

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

the html is possibly an error page?

No actually, that's what's super weird! 😅 When I open the HTML page in the browser I do see the expected result which is the PNG of the question, whether it's a viz or a table....

Choose a reason for hiding this comment

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

can we isolate the actual PNG? Is it a data: blob or an actual file hosted on the metabase server?

@valentin-mourtialon valentin-mourtialon force-pushed the valentinmourtialon/feat/metabase-png-rendering branch from 50155b5 to 68c9df2 Compare February 4, 2025 11:27
const questionId = propsValue.questionId.split('-')[0];

const response = await fetch(
`${auth.baseUrl}/api/pulse/preview_card_png/${questionId}`,
Copy link
Author

Choose a reason for hiding this comment

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

I did not use the helper you made because I didn't want to change both the common.ts and the get-png-rendering.ts in the same PR

Choose a reason for hiding this comment

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

Why would you need to modify the helper?
And you can definitely update both in the same PR if it's required for your change!

@valentin-mourtialon valentin-mourtialon force-pushed the valentinmourtialon/feat/metabase-png-rendering branch from 68c9df2 to e4770b8 Compare February 4, 2025 15:36

return {
fileName: `metabase_question_${questionId}.png`,
file: fileUrl,
Copy link
Author

Choose a reason for hiding this comment

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

file is more user friendly than url, less misleading

@valentin-mourtialon
Copy link
Author

Tested in a flow where I sent myself an email with the PNG as an attachment, and it worked.

Copy link

@AdamSelene AdamSelene left a comment

Choose a reason for hiding this comment

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

Just one question, LGTM either way!

const questionId = propsValue.questionId.split('-')[0];

const response = await fetch(
`${auth.baseUrl}/api/pulse/preview_card_png/${questionId}`,

Choose a reason for hiding this comment

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

Why would you need to modify the helper?
And you can definitely update both in the same PR if it's required for your change!

@valentin-mourtialon valentin-mourtialon force-pushed the valentinmourtialon/feat/metabase-png-rendering branch from e4770b8 to 1ffcdcb Compare February 5, 2025 11:12
@valentin-mourtialon valentin-mourtialon merged commit 3e412cf into main Feb 5, 2025
4 of 6 checks passed
@valentin-mourtialon valentin-mourtialon deleted the valentinmourtialon/feat/metabase-png-rendering branch February 5, 2025 12:34
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.

2 participants