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

feat: save semantic viewer charts and add in the dashboards #11485

Merged
merged 18 commits into from
Sep 16, 2024
Merged

feat: save semantic viewer charts and add in the dashboards #11485

merged 18 commits into from
Sep 16, 2024

Conversation

IrakliJani
Copy link
Contributor

@IrakliJani IrakliJani commented Sep 10, 2024 •
edited
Loading

Closes: #11366
Closes: #11368

Description:

Reviewer actions

  • I have manually tested the changes in the preview environment
  • I have reviewed the code
  • I understand that "request changes" will block this PR from merging

@owlas owlas requested a deployment to feat/semantic-viewer-save-charts - jaffle_db_pg_13 PR #11485 September 10, 2024 02:59 — with Render Abandoned
@owlas owlas deployed to feat/semantic-viewer-save-charts - headless-browser PR #11485 September 10, 2024 02:59 — with Render Active
@netlify Netlify
Copy link

netlify bot commented Sep 10, 2024 •
edited
Loading

✅ Deploy Preview for peaceful-bassi-cbf284 canceled.

Name Link
🔨 Latest commit 890e20a
🔍 Latest deploy log https://app.netlify.com/sites/peaceful-bassi-cbf284/deploys/66e81118d923f10008550e62

@owlas owlas temporarily deployed to feat/semantic-viewer-save-charts - lightdash PR #11485 September 10, 2024 03:00 — with Render Destroyed
@owlas owlas deployed to feat/semantic-viewer-save-charts - headless-browser PR #11485 September 10, 2024 03:16 — with Render Active
@IrakliJani IrakliJani changed the title feat: semantic viewer save charts and show in the dashboard feat: save semantic viewer charts and add in the dashboards Sep 10, 2024
@IrakliJani IrakliJani force-pushed the feat/semantic-viewer-save-charts branch from 0803e2d to afd2937 Compare September 10, 2024 09:26
@owlas owlas deployed to feat/semantic-viewer-save-charts - headless-browser PR #11485 September 10, 2024 09:26 — with Render Active
IrakliJani
IrakliJani commented Sep 10, 2024
View reviewed changes
packages/backend/src/analytics/LightdashAnalytics.ts
organizationId: string;
chartKind: ChartKind;
semanticLayerQuery: SemanticLayerQuery;
barChart?: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this does not have table config. do we also need to keep all configuration blobs as the analytics event or the active one @TuringLovesDeathMetal

Copy link
Contributor

@TuringLovesDeathMetal TuringLovesDeathMetal Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

What do you mean, sorry irakli?

I'd expect each saved chart created new saved chart version to have the table config if we're sharing an analytics event for each save chart.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TuringLovesDeathMetalsSorry for providing less than a zero context 😂

I noticed that with SQL Runner, when you save a chart, we fire an analytics event. The analytics event includes all of the configurations for all charts, not just the active chart that's being used. Additionally, we are not including the table visualization configuration at all. I copied this "blindly" from SQL Runner but left a comment not to forget to update both places if needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

The analytics event includes all of the configurations for all charts, not just the active chart that's being used.

wut - that sounds wild 😂

Ya, can we just include the active chart configuration. Also, if there's a table visualization, then include it's configuration

packages/backend/src/services/SavedSemanticLayerService/SavedSemanticLayerService.ts Outdated
user.ability.cannot(
'manage',
// TODO: add it's own ability
subject('CustomSql', { organizationUuid, projectUuid }),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

add ability

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can be done as a separate ticket

packages/frontend/src/components/DashboardTiles/DashboardSemanticLayerChartTile.tsx Outdated
fields,
query: data.chart.semanticLayerQuery,
rows: chartData.results,
columns: [], // TODO: sqlRunnerChartData.columns,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

forgot column conversion here

packages/frontend/src/components/common/Page/Sidebar.tsx
@@ -107,20 101,21 @@ const Sidebar: FC<React.PropsWithChildren<Props>> = ({
>
{(style) => (
<>
<Card
Copy link
Contributor Author

Choose a reason for hiding this comment

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

card is too opinionated to be a sidebar component

@owlas owlas deployed to feat/semantic-viewer-save-charts - headless-browser PR #11485 September 10, 2024 15:10 — with Render Active
@IrakliJani IrakliJani force-pushed the feat/semantic-viewer-save-charts branch from 28890f8 to 6a28b4c Compare September 10, 2024 15:24
@owlas owlas temporarily deployed to feat/semantic-viewer-save-charts - headless-browser PR #11485 September 10, 2024 15:24 — with Render Destroyed
@owlas owlas temporarily deployed to feat/semantic-viewer-save-charts - lightdash PR #11485 September 13, 2024 14:52 — with Render Destroyed
@IrakliJani IrakliJani closed this Sep 13, 2024
@IrakliJani IrakliJani reopened this Sep 13, 2024
@owlas owlas requested a deployment to feat/semantic-viewer-save-charts - jaffle_db_pg_13 PR #11485 September 13, 2024 15:00 — with Render Abandoned
@owlas owlas deployed to feat/semantic-viewer-save-charts - headless-browser PR #11485 September 13, 2024 15:00 — with Render Active
@owlas owlas temporarily deployed to feat/semantic-viewer-save-charts - lightdash PR #11485 September 13, 2024 15:01 — with Render Destroyed
@owlas owlas deployed to feat/semantic-viewer-save-charts - headless-browser PR #11485 September 13, 2024 15:18 — with Render Active
@owlas owlas deployed to feat/semantic-viewer-save-charts - headless-browser PR #11485 September 16, 2024 08:47 — with Render Active
@IrakliJani IrakliJani mentioned this pull request Sep 16, 2024
Open
@owlas owlas temporarily deployed to feat/semantic-viewer-save-charts - lightdash PR #11485 September 16, 2024 09:41 — with Render Destroyed
@IrakliJani IrakliJani force-pushed the feat/semantic-viewer-save-charts branch from 2010f22 to 5145cb4 Compare September 16, 2024 09:54
@owlas owlas deployed to feat/semantic-viewer-save-charts - headless-browser PR #11485 September 16, 2024 09:54 — with Render Active
@IrakliJani IrakliJani force-pushed the feat/semantic-viewer-save-charts branch from 5145cb4 to 890e20a Compare September 16, 2024 11:05
@owlas owlas temporarily deployed to feat/semantic-viewer-save-charts - lightdash PR #11485 September 16, 2024 11:05 — with Render Destroyed
@owlas owlas temporarily deployed to feat/semantic-viewer-save-charts - headless-browser PR #11485 September 16, 2024 11:06 — with Render Destroyed
@IrakliJani IrakliJani merged commit e02ff01 into main Sep 16, 2024
35 of 38 checks passed
@IrakliJani IrakliJani deleted the feat/semantic-viewer-save-charts branch September 16, 2024 11:18
lightdash-bot pushed a commit that referenced this pull request Sep 16, 2024
@semantic-release-bot
chore(release): 0.1256.0
e3911f6 
# [0.1256.0](0.1255.6...0.1256.0) (2024-09-16)

### Features

* save semantic viewer charts and add in the dashboards ([#11485](#11485)) ([e02ff01](e02ff01))
@lightdash-bot
Copy link
Collaborator

🎉 This PR is included in version 0.1256.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TuringLovesDeathMetal TuringLovesDeathMetal TuringLovesDeathMetal left review comments

@almeidabbm almeidabbm almeidabbm approved these changes

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

I'm able to add semantic viewer charts to a dashboard I'm able to save charts from the Semantic Viewer
5 participants
@IrakliJani @lightdash-bot @almeidabbm @TuringLovesDeathMetal @owlas