Fix chart labels

[denisov-vlad]

What type of PR is this?

• Refactor
• Feature
• Bug Fix
• New Query Runner (Data Source)
• New Alert Destination
• Other

Description

Fixed labels for charts. Before that, all labels have values = 0.

The error was that Object in JS has keys as strings, so Object.keys(labelsValuesDict); returned wrong array.

I don't have any experience with JS, so please review it carefully.

How is this tested?

• Unit tests (pytest, jest)
• E2E Tests (Cypress)
• Manually
• N/A

Related Tickets & Documents

#6996

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

[eradman]

This change has 17 commits, it should only be 1. (Error trying to rebase?)

[eradman]

@lvitti if you have time please test this change as well. We want to keep the functionality you implemented in #6908 if possible.

[denisov-vlad]

This change has 17 commits, it should only be 1. (Error trying to rebase?)

They'll be combined into one during the merge (with squash), so I think it's not so important to make a rebase.

[lvitti]

Hi @denisov-vlad, @eradman sorry for that bug, yes look like this fix is working also for Pie aggregation
I'm not an expert in javascript either, here is another implementation of the fix using Map()
https://github.com/getredash/redash/compare/master...lvitti:redash:lvitti/fix_6996_chart_doesnt_show_values?expand=1

[denisov-vlad]

@eradman Are there any blockers to merge it?

[eradman]

Yes, this is a very "hot" code path and testing for adverse side-effects has proven to require more care and the addition of unit tests.

I proposed that we revert the change that caused this issue in #7018

[lvitti]

Yes, this is a very "hot" code path and testing for adverse side-effects has proven to require more care and the addition of unit tests.

I proposed that we revert the change that caused this issue in #7018

And what is the plan to solve the other bug? Because it's as bad as this.

Why not just add a test to this case and consider the 2 fixes ?

[eradman]

And what is the plan to solve the other bug? Because it's as bad as this.

Why not just add a test to this case and consider the 2 fixes ?

The bug with the pie charts should be fixed as well, but this is a regression.

[ezraodio1]

I've been playing around with this feature and have noticed a few things.

1. If you recreate @denisov-vlad's bar chart with the issue and click Edit Visualization -> X Axis -> Scale -> Category, the labels are correct.
2. This issue with the labels is not limited to bar charts - I recreated it with the pie chart. As @denisov-vlad mentioned, the error is because the dictionary forces keys to be strings. Therefore, if your choice for X Axis isn't a column of strings, then the labels will be incorrect.
3. Using @lvitti's implementation with Map() doesn't seem to break anything, which logically makes sense since it isn't forcing a data type on its keys. All of the charts I made with the Map() implementation were correct, and I also didn't need to manually set the X Axis scale to "Category."

Screen.Recording.2024-06-17.at.2.50.52.PM.mov

[eradman]

We merged an alternate fix in #7022