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: Allow for nested grouping in group_label meta property #10119

Merged
merged 21 commits into from
May 30, 2024
Merged

feat: Allow for nested grouping in group_label meta property #10119

merged 21 commits into from
May 30, 2024

Conversation

mmcordoba
Copy link
Contributor

Closes: #3593
Closes: #2686

Description:

SubGroupsHover.mov

NOTE: that there are changes to the field.ts (removed property group_label) Field Type and to TreeGroupNode to deal with the new way to represents groups and subgroups. This change was made so even if we do support still the group_label property on the model yamel definition, i wanted to clean up the UI part of it.

This change requires the DBT models to be refreshed by lightdash?. Not sure if this happens by default when a new version gets deployed.

Acceptance criteria

  • I'm able to have up to 1 sub-groups in my table group labels (more than 1 level of sub-grouping doesn't get rendered in Lightdash)

We have implemented the feature by not bounding the depth of the subgroups, we have done this, because after testing, it feels like it should be up to the user to decide how many subgroups it needs.

  • The sub-groups appear in the sidebar nested in the groups
  • The sub groups are ordered alphabetically within a group
    Documentation
  • update the docs on groups to include information on how to build subgroups

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

@netlify Netlify
Copy link

netlify bot commented May 20, 2024 •
edited

👷 Deploy request for peaceful-bassi-cbf284 pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 72b9305

@ZeRego ZeRego self-requested a review May 20, 2024 13:19
@ZeRego ZeRego self-assigned this May 20, 2024
@ZeRego
Copy link
Contributor

ZeRego commented May 20, 2024

This change requires the DBT models to be refreshed by lightdash?. Not sure if this happens by default when a new version gets deployed.

@mmcordoba This doesn't happen by default so we would "break" current grouping. We would like to reduce the problem by following one of 2 options:

  1. Add backwards compatibility in the UI.

Adjust the frontend code to fallback to "group_label" and "group" when available and "groups" is empty.
Create a ticket to remove tech debt in 1 week once most of the users have re-deployed their project and are stored with the groups values.

  1. Release in 2 phases

First we change the BE to store the groups property alongside the existing properties.
After 1 week we remove the legacy properties and change the remaining of the code.

I'm inclined towards the first approach but you can decide.

ZeRego
ZeRego reviewed May 20, 2024
View reviewed changes
packages/common/src/types/field.ts Show resolved Hide resolved
packages/common/src/types/field.ts Outdated Show resolved Hide resolved
packages/common/src/types/field.ts Outdated Show resolved Hide resolved
@mmcordoba
Copy link
Contributor Author

This change requires the DBT models to be refreshed by lightdash?. Not sure if this happens by default when a new version gets deployed.

@mmcordoba This doesn't happen by default so we would "break" current grouping. We would like to reduce the problem by following one of 2 options:

  1. Add backwards compatibility in the UI.

Adjust the frontend code to fallback to "group_label" and "group" when available and "groups" is empty. Create a ticket to remove tech debt in 1 week once most of the users have re-deployed their project and are stored with the groups values.

  1. Release in 2 phases

First we change the BE to store the groups property alongside the existing properties. After 1 week we remove the legacy properties and change the remaining of the code.

I'm inclined towards the first approach but you can decide.

Hi i have implemented option 1 as suggested.
Regarding the ticket for the tech debt, i think i can only open an issue , what type of issue you need me to create for tech debt? or could you open a ticket for me for the tech debt clean up?
Thanks

@ZeRego ZeRego mentioned this pull request May 29, 2024
Closed
@owlas owlas requested a deployment to duplicate_feat/sub-groups - jaffle_db_pg_13 PR #10236 May 29, 2024 16:34 — with Render Abandoned
@owlas owlas temporarily deployed to duplicate_feat/sub-groups - headless-browser PR #10236 May 29, 2024 16:34 — with Render Destroyed
@owlas owlas temporarily deployed to duplicate_feat/sub-groups - lightdash PR #10236 May 29, 2024 16:35 — with Render Destroyed
@ZeRego
Copy link
Contributor

ZeRego commented May 29, 2024

Regarding the ticket for the tech debt, i think i can only open an issue , what type of issue you need me to create for tech debt? or could you open a ticket for me for the tech debt clean up?

@mmcordoba you can open a feature request issue and share it with me. I can update the labels and milestone ;)

I have pushed 2 changes:

  • Fix mutation issue that caused date/time dimensions to have duplicated groups. 1 subgroup per time interval
Screenshot 2024-05-29 at 15 58 34
  • Implement max group nesting of 2
    If you require more nesting you can change MAX_GROUP_DEPTH in your fork or create a ticket to add an env var to configure this without a fork.

Running e2e test before approving 🚀

@mmcordoba mmcordoba mentioned this pull request May 29, 2024
Open
@ZeRego ZeRego self-requested a review May 30, 2024 09:30
@ZeRego ZeRego merged commit fe5e254 into lightdash:main May 30, 2024
50 of 57 checks passed
lightdash-bot pushed a commit that referenced this pull request May 30, 2024
@semantic-release-bot
chore(release): 0.1112.0
a4d1522 
# [0.1112.0](0.1111.3...0.1112.0) (2024-05-30)

### Features

* Allow for nested grouping in group_label meta property ([#10119](#10119)) ([fe5e254](fe5e254))
@lightdash-bot
Copy link
Collaborator

🎉 This PR is included in version 0.1112.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@Martin-Carlsson
Copy link

Wow! This is really cool 🤩

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

@ZeRego ZeRego ZeRego approved these changes

Assignees

@ZeRego ZeRego

Labels
released
Projects
Selected for Development
Archived in project
Milestone
No milestone
5 participants
@mmcordoba @ZeRego @lightdash-bot @Martin-Carlsson @owlas