fix: time series table

[khtruong]

CATEGORY

Choose one

• Bug Fix
• Enhancement (new features, refinement)
• Refactor
• Add tests
• Build / Development Environment
• Documentation

SUMMARY

There are various issues with the time series table:

1. Cannot create a time series table
2. Cannot save a time series table

The problem is that we pass and save all props as state properties in the TimeSeriesColumnControl. When we make a backend request, we pass all the state properties as form data. Some of these properties do not make sense (i.e. a function to render the tooltip) and thus the request fails.

The fix is to be explicit about the state properties so that we only pass back relevant and comprehensible properties to the backend.

@mistercrunch This addresses some of the issues you were trying to fix in #6959. I think we should be explicit in our state properties versus adding the prop passthroughProps.

TEST PLAN

Tested manually

ADDITIONAL INFORMATION

• Has associated issue:
• Changes UI
• Requires DB Migration.
• Confirm DB Migration upgrade and downgrade tested.
• Introduces new feature or API
• Removes existing feature or API

REVIEWERS

@mistercrunch @xtinec @betodealmeida @DiggidyDave @datability-io

[codecov-io]

Codecov Report

Merging #7302 into lyftga will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           lyftga    #7302   +/-   ##
=======================================
  Coverage   64.58%   64.58%           
=======================================
  Files         425      425           
  Lines       20884    20884           
  Branches     2297     2297           
=======================================
  Hits        13488    13488           
  Misses       7270     7270           
  Partials      126      126

Impacted Files	Coverage Δ
...re/components/controls/TimeSeriesColumnControl.jsx	65.9% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0f1702b...071b242. Read the comment docs.

[khtruong]

Adding @michellethomas to this PR

[michellethomas]

Previously state was set to what's in props const state = { ...props }, should these be set to the values in props instead of nulls and empty strings? It would also be good to explicitly state what's in props and list them as either required or list them in defaultProps.

[khtruong]

None of the properties in props correspond to state properties except label.

The line I removed is definitely confusing to read. It leads the reader to believe that props has values like "colType", "width", etc. when in actuality it has none of these properties.

Listing out the state properties should improve readability.

I've also added a default value for label in props.

[khtruong]

You can also see in the screenshot all the random properties we add to state that leads to a bad request to the backend.

[DiggidyDave]

Instead of adding null values and empty strings, can we just omit them? The consumer of that object should be the thing that decides a) if it wants those values to be there and b) what to do if they are missing. Inserting them here seems like it is coupling things in a way that is going to be fragile

[DiggidyDave]

Nvm, @khtruong I think that works just as a declaration of "here are the fields and initial values that this type cares about". I was thinking it was being used differently.

[michellethomas]

Are you printing out the this.props from CollectionControl? When I look at props from within the constructor of TimeSeriesColumnControl when loading a time series table chart I see the other props like colType d3format set that correspond to state properties. You mentioned in chat that o passed to the Control is undefined, on master it looks like it's getting set correctly. Are there any changes in lyftga that could have affected this?

[khtruong]

Ahh I see what is going on. I was looking into this.props and you were referring to this.props.value. Yes, I can set the state to what is stored in this.props.values.

Also ignore my comment about o being undefined. I was within the map function's scope when I tried to look up this.props which was undefined within that scope.

[khtruong]

Talked to Michelle offline. Turns out she was looking at the props with Max's changes and I was looking at the props without Max's changes. Hehe.

Now I have set state to the prop values if they exist. Otherwise, they are sent to default values.