fix(chart): legacyRegularTime and charts are being rendered, and solves issue #26181

[sivasathyaseeelan]

SUMMARY

This PR solves the issue mentioned in #26181. I have solved the logical error in legacyRegularTime and also made some changes in superset-frontend/plugins/legacy-plugin-chart-calendar/src/controlPanel.ts for better app structure.

BEFORE

Screencast.from.10-01-24.12.21.36.AM.IST.webm

AFTER

Screencast.from.10-01-24.12.23.35.AM.IST.webm

TESTING INSTRUCTIONS

Kindly create any chart which renders legacyRegularTime (Eg: Event Flow chart which is mentioned in #26181) and now you can be able to see desired result.

ADDITIONAL INFORMATION

• Has associated issue: Fixes Event flow chart does not display Time section in configuration #26181
• 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

[villebro]

@sivasathyaseeelan I think you've stumbled on a very critical bug here. I believe checking for hasGenericChartAxes should be removed from legacyRegularTime, i.e. those controls should always be present, irrespective of whether GENERIC_CHART_AXES is enabled.

#23865 fixed something similar. Ping @kgabryje , let's sync up on this asap, I'll ping you on Slack. @sivasathyaseeelan are you on Superset Slack? It might be easier to discuss this there..

[villebro]

FYI @michael-s-molina I think this may be important to get fixed in both the 3.0 and 3.1 RCs that are being voted on right now..

[sivasathyaseeelan]

Hello @villebro, yeah I on superset slack!

I think changing legacyRegularTime to

export const legacyRegularTime: ControlPanelSectionConfig = {
  ...baseTimeSection,
  controlSetRows: [['granularity_sqla'], ['time_range']],
};

will solve this issue and it works fine in my local machine.

Can I apply this piece of code to modify this PR?

[michael-s-molina]

@sivasathyaseeelan I think you've stumbled on a very critical bug here. I believe checking for hasGenericChartAxes should be removed from legacyRegularTime, i.e. those controls should always be present, irrespective of whether GENERIC_CHART_AXES is enabled.

I'm not sure about this. The only difference between the legacyRegularTime and genericTime is the time_grain_sqla control. It seems these controls were created for the case where the plugin is able to handle both states of the feature flag. One example is the line chart that displays the legacy time section when the feature flag is off and uses the x-axis control when the feature flag is on. In that case the logic is correct because you don't want to display the legacy time when the feature is off. I'm not sure if legacy plugins have this ability, we would need to check if legacyRegularTime will behave differently than genericTime or if we need to go plugin by plugin and see if it has the ability to handle both feature flag states.

FYI @michael-s-molina I think this may be important to get fixed in both the 3.0 and 3.1 RCs that are being voted on right now..

Considering that this problem already exists in 3.0.2 and that is affects mostly legacy plugins, I believe we could cherry-pick the fix in the next patch.

[kgabryje]

I think this change will break things when GENERIC_CHART_AXES is disabled.
AFAIK the goal of that check was not to display time column and time range fields, as in most charts those 2 were used only for filtering, and we moved the filtering capability to adhoc filters.
The problem is that some charts, like Calendar or Event flow, require a time column to be defined for other purposes than just filtering. I think we should treat those cases individually, like it was done for Calendar chart's control panel.
In the case of Event flow chart, we could replace sections.legacyRegularTime with { ...baseTimeSection, controlSetRows: [['granularity_sqla']] }
to make the time section independent of GENERIC_CHART_AXES feature flag

[sivasathyaseeelan]

@kgabryje and @michael-s-molina I understood, then i will go and replace sections.legacyRegularTime in controlPanel.ts of event-flow.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.16%. Comparing base (ff025b7) to head (ef3c3f3).
Report is 1442 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #26439   +/-   ##
=======================================
  Coverage   69.16%   69.16%           
=======================================
  Files        1948     1948           
  Lines       76054    76054           
  Branches     8493     8493           
=======================================
  Hits        52601    52601           
  Misses      21273    21273           
  Partials     2180     2180           

Flag	Coverage Δ
javascript	56.50% <ø> (ø)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[villebro]

@michael-s-molina @kgabryje I did some more checking, and I misremembered how the generic chart x-axis charts import handle the time section. Sorry for the noisy escalation - It seems the majority of legacy charts work fine with the new time filter (=that essentially replaces the old time section), and no longer need the dedicated time section.

However, I noticed, that at least the t-test chart is also no longer functioning with generic chart axes turned on. To DRY this up, we may want to consider creating a shared control that always has the time section, but if it's only event flow and t-test at this point, then maybe it's not worth the effort.

[rusackas]

Noticed this one has slipped through the cracks for quite some time. I'm not sure how critical it is at this point, but if it were to move forward, it looks like it needs a rebase at the very least. @michael-s-molina and @villebro might know best about how (or if) we want to move this forward from here, but I'll put this in Draft mode until it's rebased at the very least.