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: Introduce component ids #1849

Merged
merged 73 commits into from
Nov 20, 2024
Merged

Conversation

bprusinowski
Copy link
Collaborator

@bprusinowski bprusinowski commented Nov 12, 2024

Closes #1696

This PR introduces a new, component id concept to Visualize, which replaces the current component iri convention.

Component id is made by combining unversioned cube iri with unversioned component iri, which is a unique combination even in case we merged two cubes with the same component iris. This means that we can handle these cases correctly, while leaving our business logic mostly intact (and in some cases, even simpler, as we don't need to filter by cube iri and component iri anymore, but instead simply use component id).

This combination of iris is done on the server side and sent to the client, which doesn't need to know about this fact, but simply keep the current logic (chart config migrations are the only exception, as we need to introduce unversioned cube iris to relevant parts of the config dynamically to make sure old charts work).

By proxy, it was necessary to make the migrations async, so we can fetch unversioned cube iri when needed – as immer doesn't support / recommend asynchronous operations, I removed it.

ℹ️ Dimension id docs

🔎 Visual regression testing docs

How to test

🚨 Preferably, the implementation should be tested holistically, as essentially every part of Visualize was affected in one or another way.

  1. Go to this link.
  2. Click on Add dataset.
  3. Add Market figures milk and dairy products - Monthly producer prices dataset.
  4. Open Vertical axis.
  5. ✅ See that it's possible to toggle between two Price dimensions that have the same iris and names, but come from different cubes.
  6. Change chart type to Table.
  7. ✅ See that there are duplicated dimensions visible in the Table (that have the same iris and names, but come from different cubes).

How to reproduce

Repeat the above steps on PROD environment and see that ✅ points are broken ❌.


  • Add docs to feature pages in Notion (both for componentId and visual regression testing)
  • Add tests
  • Check E2E tests locally
  • Check if all old charts work (checked local /_charts page and cross-compared 500 PROD charts on main vs this branch)

Copy link

vercel bot commented Nov 12, 2024

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

Name Status Preview Comments Updated (UTC)
visualization-tool ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 20, 2024 2:50pm

@bprusinowski bprusinowski changed the title feat: Make dimension iris **unique** (merging of cubes improvements) feat: Introduce component ids Nov 14, 2024
@bprusinowski bprusinowski force-pushed the feat/finalize-merging-of-cubes-2 branch from 09328ab to f2359a7 Compare November 14, 2024 14:10
@bprusinowski bprusinowski force-pushed the feat/finalize-merging-of-cubes-2 branch from 2ae25b4 to 7c55a27 Compare November 14, 2024 19:14
@bprusinowski bprusinowski force-pushed the feat/finalize-merging-of-cubes-2 branch from 610fc50 to bcf7644 Compare November 15, 2024 14:19
@bprusinowski bprusinowski marked this pull request as ready for review November 18, 2024 12:23
@bprusinowski bprusinowski requested review from ptbrowne and removed request for noahonyejese November 18, 2024 12:23
@bprusinowski bprusinowski force-pushed the feat/finalize-merging-of-cubes-2 branch from 1a8a6c1 to edfd961 Compare November 18, 2024 14:44
Copy link
Collaborator

@ptbrowne ptbrowne left a comment

Choose a reason for hiding this comment

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

Thanks a lot Bartosz for this massive effort 💪

I see no blockers here, I have mostly put nits and comments.

  • The use of branded types would make sure we do not pass a dimension iri where we expect a dimension id.
  • I think it would be a good idea to perform screenshot testing against production charts to make sure they are ok (load the production charts into local database, screenshot the chart on main, switch to this branch rescreenshot, check if they are the same)

app/graphql/resolvers/rdf.ts Outdated Show resolved Hide resolved
app/rdf/queries.ts Outdated Show resolved Hide resolved
app/rdf/queries.ts Show resolved Hide resolved
app/utils/chart-config/versioning.ts Show resolved Hide resolved
app/utils/chart-config/versioning.ts Show resolved Hide resolved
app/utils/chart-config/versioning.ts Show resolved Hide resolved
app/charts/shared/use-sync-interactive-filters.spec.tsx Outdated Show resolved Hide resolved
playwright.config.ts Show resolved Hide resolved
@bprusinowski bprusinowski force-pushed the feat/finalize-merging-of-cubes-2 branch from e008c29 to 37e6b72 Compare November 20, 2024 10:21
@bprusinowski bprusinowski force-pushed the feat/finalize-merging-of-cubes-2 branch from 4cd6622 to d042fde Compare November 20, 2024 10:28
@bprusinowski bprusinowski merged commit 9081202 into main Nov 20, 2024
6 of 10 checks passed
@bprusinowski bprusinowski deleted the feat/finalize-merging-of-cubes-2 branch November 20, 2024 15:09
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.

✓ Technical clean up of merging of cubes feature
3 participants