fix: Dashboard time grain in Pivot Table

[michael-s-molina]

SUMMARY

Fixes #24460.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before: Check the original issue.

After: The time grain is correctly applied

Screen.Recording.2023-07-11.at.11.20.33.mov

TESTING INSTRUCTIONS

Check the original issue for instructions.

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

[hughhhh]

can we write a small unit test to for buildQuery to verify the grains are being properly set?

I ran this thru chat gpt, but it would be good to have this to make sure that queries are being built properly

import buildQuery from './path-to-your-file';

describe('buildQuery', () => {
  it('should build query with expected columns', () => {
    const formData = {
      groupbyColumns: ['column1', 'column2'],
      groupbyRows: ['row1', 'row2'],
      extra_form_data: {
        time_grain_sqla: 'monthly'
      },
      time_grain_sqla: 'weekly',
      temporal_columns_lookup: { column1: true },
      granularity_sqla: 'column1'
    };
    const result = buildQuery(formData as any);
    // Define your expected output here.
    const expected = [
      // Fill in with expected transformed columns
    ];

    expect(result).toEqual(expected);
  });
  it('should prefer extra_form_data.time_grain_sqla over formData.time_grain_sqla', () => {
    const formData = {
      groupbyColumns: ['column1'],
      groupbyRows: [],
      extra_form_data: {
        time_grain_sqla: 'monthly'
      },
      time_grain_sqla: 'weekly',
      temporal_columns_lookup: { column1: true },
      granularity_sqla: 'column1'
    };

    const result = buildQuery(formData as any);

    // Assuming that the time_grain_sqla is reflected in the 'timeGrain' property of the result
    const timeGrain = result[0]?.timeGrain;
    expect(timeGrain).toEqual('monthly');
  });

  it('should fallback to formData.time_grain_sqla if extra_form_data.time_grain_sqla is not set', () => {
    const formData = {
      groupbyColumns: ['column1'],
      groupbyRows: [],
      extra_form_data: {},
      time_grain_sqla: 'weekly',
      temporal_columns_lookup: { column1: true },
      granularity_sqla: 'column1'
    };

    const result = buildQuery(formData as any);

    // Assuming that the time_grain_sqla is reflected in the 'timeGrain' property of the result
    const timeGrain = result[0]?.timeGrain;
    expect(timeGrain).toEqual('weekly');
  });
});

[michael-s-molina]

Thank you for the references @hughhhh. I added the tests with modifications.

One tip: you can ask Chat GPT to avoid using describe and it blocks to generate tests that conform to our best practices 😉

[codecov]

Codecov Report

Merging #24665 (4861b5e) into master (c53b249) will increase coverage by 0.07%.
The diff coverage is 36.70%.

❗ Current head 4861b5e differs from pull request most recent head 1a06d5a. Consider uploading reports for the commit 1a06d5a to get more accurate results

@@            Coverage Diff             @@
##           master   #24665      +/-   ##
==========================================
+ Coverage   68.97%   69.05%   +0.07%     
==========================================
  Files        1907     1908       +1     
  Lines       74153    74174      +21     
  Branches     8182     8179       -3     
==========================================
+ Hits        51148    51220      +72     
+ Misses      20882    20833      -49     
+ Partials     2123     2121       -2     

Flag	Coverage Δ
javascript	55.75% <37.77%> (-0.01%)	⬇️

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

Impacted Files	Coverage Δ
...hart-echarts/src/MixedTimeseries/transformProps.ts	0.78% <ø> (ø)
...frontend/src/SqlLab/components/SqlEditor/index.jsx	59.88% <ø> (ø)
superset-frontend/src/components/Icons/Icon.tsx	0.00% <ø> (ø)
...Filters/FilterBar/FilterControls/FilterControl.tsx	27.69% <0.00%> (-0.44%)	⬇️
...veFilters/FilterBar/FilterControls/FilterValue.tsx	5.76% <ø> (+0.10%)	⬆️
...tersConfigModal/FiltersConfigForm/DefaultValue.tsx	9.09% <0.00%> (ø)
...mponents/nativeFilters/FiltersConfigModal/utils.ts	46.96% <0.00%> (ø)
...nd/src/dashboard/components/nativeFilters/state.ts	64.28% <ø> (ø)
.../src/explore/components/ControlPanelsContainer.tsx	69.76% <0.00%> (ø)
...features/annotationLayers/AnnotationLayerModal.tsx	58.97% <ø> (ø)
... and 31 more

... and 9 files with indirect coverage changes

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

[kgabryje]

/testenv up

[github-actions]

@kgabryje Ephemeral environment spinning up at http://34.217.82.78:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

[kgabryje]

LGTM

[github-actions]

Ephemeral environment shutdown and build artifacts deleted.

[yousoph]

Thanks for the fix @michael-s-molina ! I didn't get a chance to check on the ephemeral before the merge, but I believe this was broken for tables as well and it looks like this fix was only for pivot tables, is that right?

[michael-s-molina]

Thanks for the fix @michael-s-molina ! I didn't get a chance to check on the ephemeral before the merge, but I believe this was broken for tables as well and it looks like this fix was only for pivot tables, is that right?

Yes, it was only for pivot tables. I'll check the table too and submit a fix if it's broken.

[unnyns-307]

Hi @michael-s-molina I also found the same behavior occurs on table too.
Just FYI, I tried setting GENERIC_CHART_AXIS = FALSE and the problem is gone. Hope this might help 👍
I also hope we could fix it to use along with this feature flag too.

[michael-s-molina]

Hi @michael-s-molina I also found the same behavior occurs on table too. Just FYI, I tried setting GENERIC_CHART_AXIS = FALSE and the problem is gone. Hope this might help 👍 I also hope we could fix it to use along with this feature flag too.

Thank you for the details @unnyns-307. I'll fix it.