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

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

mistercrunch 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_r279513362
 
 

 ##########
 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 repeated some of the logic (like applying defaults), but skipped important part that should be consistent like `mapStateToProps`. I'm guessing this was mostly fine because in the end the logic here cares only about the `value` part of the `controlState`. 
   
   Let me see if I can throw in some useful unit tests 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