[GitHub] [incubator-superset] michellethomas commented on a change in pull request #7350: Refactor out controlUtils.js module + unit tests

[GitBox <gi...@apache.org>]

michellethomas commented on a change in pull request #7350: Refactor out controlUtils.js module + unit tests
URL: https://github.com/apache/incubator-superset/pull/7350#discussion_r278775518
 
 

 ##########
 File path: superset/assets/src/explore/store.js
 ##########
 @@ -66,104 +35,54 @@ function handleDeprecatedControls(formData) {
   }
 }
 
-export function getControlsState(state, form_data) {
+export function getControlsState(state, inputFormData) {
   /*
   * Gets a new controls object to put in the state. The controls object
   * is similar to the configuration control with only the controls
   * related to the current viz_type, materializes mapStateToProps functions,
-  * adds value keys coming from form_data passed here. This can't be an action creator
+  * adds value keys coming from inputFormData passed here. This can't be an action creator
   * just yet because it's used in both the explore and dashboard views.
   * */
 
   // Getting a list of active control names for the current viz
-  const formData = Object.assign({}, form_data);
+  const formData = Object.assign({}, inputFormData);
   const vizType = formData.viz_type || 'table';
 
   handleDeprecatedControls(formData);
 
-  const controlNames = getControlNames(vizType, state.datasource.type);
+  const controlNames = getControlKeys(vizType, state.datasource.type);
 
   const viz = controlPanelConfigs[vizType] || {};
-  const controlOverrides = viz.controlOverrides || {};
   const controlsState = {};
-  controlNames.forEach((k) => {
-    if (React.isValidElement(k)) {
-      // no state
-      return;
-    }
-    const control = Object.assign({}, controls[k], controlOverrides[k]);
-    if (control.mapStateToProps) {
-      Object.assign(control, control.mapStateToProps(state, control));
-      delete control.mapStateToProps;
-    }
-
-    formData[k] = (control.multi && formData[k] && !Array.isArray(formData[k])) ? [formData[k]]
-      : formData[k];
-
-    // If the value is not valid anymore based on choices, clear it
-    if (
-      control.type === 'SelectControl' &&
-      !control.freeForm &&
-      control.choices &&
-      k !== 'datasource' &&
-      formData[k]
-    ) {
-      const choiceValues = control.choices.map(c => c[0]);
-      if (control.multi && formData[k].length > 0) {
-        formData[k] = formData[k].filter(el => choiceValues.indexOf(el) > -1);
-      } else if (!control.multi && choiceValues.indexOf(formData[k]) < 0) {
-        delete formData[k];
-      }
-    }
 
-    if (typeof control.default === 'function') {
-      control.default = control.default(control);
-    }
-    control.validationErrors = [];
-    control.value = control.default;
-    // formData[k]'s type should match control value type
-    if (formData[k] !== undefined &&
-      (Array.isArray(formData[k]) && control.multi || !control.multi)
-    ) {
-      control.value = formData[k];
-    }
-    controlsState[k] = validateControl(control);
+  controlNames.forEach((k) => {
+    const control = getControlState(k, vizType, state, formData[k]);
+    controlsState[k] = control;
+    formData[k] = control.value;
   });
+
   if (viz.onInit) {
     return viz.onInit(controlsState);
   }
   return controlsState;
 }
 
-export function applyDefaultFormData(form_data) {
-  const datasourceType = form_data.datasource.split('__')[1];
-  const vizType = form_data.viz_type || 'table';
-  const viz = controlPanelConfigs[vizType] || {};
-  const controlNames = getControlNames(vizType, datasourceType);
-  const controlOverrides = viz.controlOverrides || {};
+export function applyDefaultFormData(inputFormData) {
+  const datasourceType = inputFormData.datasource.split('__')[1];
+  const vizType = inputFormData.viz_type;
+  const controlNames = getControlKeys(vizType, datasourceType);
   const formData = {};
   controlNames.forEach((k) => {
-    const control = Object.assign({}, controls[k]);
-    if (controlOverrides[k]) {
-      Object.assign(control, controlOverrides[k]);
-    }
-    if (form_data[k] === undefined) {
-      if (typeof control.default === 'function') {
-        formData[k] = control.default(controls[k]);
-      } else {
-        formData[k] = control.default;
-      }
+    const controlState = getControlState(k, vizType, null, inputFormData[k]);
 
 Review comment:
   It looks like getControlState does more than what was previously going on in this function. It seems like it wouldn't be a problem to have extra functionality added like applyMapStateToPropsToControl, validateControl, and handleMissingChoice but I'm not super familiar with where this is used. Do you think they'll be any changes around adding this additional logic (or maybe it just wasn't needed but is not harmful)? From codecov it looks like this is one of the sections of the diff that doesn't have any added tests, is there anything that would be useful to test here?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org