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

fix(filters): Stop breaking if translateToSql returns an object #23715

Merged

Conversation

Antonio-RiveroMartnez
Copy link
Member

SUMMARY

There might be times where after changing the dataset, the translateToSql is not just a string but an object and thus you get an error when trying to perform the substring operation. In that case we need to get the label from the column_name if present if not just return an empty string instead.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:
error

After:
test

TESTING INSTRUCTIONS

  1. Load a BigNumber chart
  2. Swap the dataset to be something that has a time filter like Vehicle Sales in the examples DB
  3. Change the chart type to be area chart
  4. You should not get an error, instead, the filters should be populated using the new dataset info

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@Antonio-RiveroMartnez Antonio-RiveroMartnez assigned geido and unassigned geido Apr 17, 2023
@Antonio-RiveroMartnez Antonio-RiveroMartnez changed the title fix(filters): Stop breaking translateToSql returns an object fix(filters): Stop breaking if translateToSql returns an object Apr 17, 2023
@@ -135,6 +135,10 @@ export default class AdhocFilter {

getDefaultLabel() {
const label = this.translateToSql();
// If returned value is an object after changing dataset
if (typeof label === 'object') {
return label.column_name ?? '';
Copy link
Member

Choose a reason for hiding this comment

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

Do I understand correctly that the bug occurs when adhocFIlter.subject is an object and not a string? If so, maybe we could move the logic of extracting column_name to getSimpleSQLExpression, line 298 - let expression = subject ?? '';?

- Consider cases where translateToSql doesn't return a string when getting the default label of a filter
- Add tests for changes
- Moving logic to extract colum_name to getSimpleSQLExpression instead
Copy link
Member

@kgabryje kgabryje left a comment

Choose a reason for hiding this comment

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

Lgtm

- Optional chaining when accessing column_name
@codecov
Copy link

codecov bot commented May 8, 2023

Codecov Report

Merging #23715 (280894c) into master (9681d85) will increase coverage by 0.00%.
The diff coverage is 52.00%.

❗ Current head 280894c differs from pull request most recent head 41c4a07. Consider uploading reports for the commit 41c4a07 to get more accurate results

@@           Coverage Diff           @@
##           master   #23715   +/-   ##
=======================================
  Coverage   68.18%   68.19%           
=======================================
  Files        1941     1941           
  Lines       75241    75242    +1     
  Branches     8158     8157    -1     
=======================================
+ Hits        51306    51308    +2     
+ Misses      21852    21851    -1     
  Partials     2083     2083           
Flag Coverage Δ
javascript 54.49% <52.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...d/src/SqlLab/components/TabbedSqlEditors/index.jsx 50.00% <50.00%> (+0.52%) ⬆️
...uperset-frontend/src/explore/exploreUtils/index.js 78.76% <100.00%> (ø)

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Antonio-RiveroMartnez Antonio-RiveroMartnez merged commit 724fd82 into apache:master May 8, 2023
25 checks passed
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.0.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/M 🚢 3.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants