Bug 1764136: improve styling of uplift request form

Author: cgsheeh

moz-extensions/src/applications/transactions/commentaction/PhabricatorUpdateUpliftCommentAction.php

@@ -9,12 +9,15 @@ public function getPHUIXControlType() {
 
   public function getPHUIXControlSpecification() {
     $value = $this->getValue();
+    $initial = false;
 
     if (empty($value) || $value == null) {
       $value = $this->getInitialValue();
+      $initial = true;
     }
 
     return array(
+        'initial' => $initial,
         'questions' => $value,
     );
 

moz-extensions/src/differential/customfield/DifferentialUpliftRequestCustomField.php

@@ -278,7 +278,7 @@ private function getRemarkup(): string {
     public function newCommentAction() {
         // Returning `null` causes no comment action to render, effectively
         // "disabling" the field.
-        if (!$this->isUpliftTagSet()) {
+        if (!$this->isFieldActive()) {
             return null;
         }
 
@@ -299,22 +299,49 @@ public function validateUpliftForm(array $form): array {
             return $validation_errors;
         }
 
+        $valid_questions = array_keys(self::BETA_UPLIFT_FIELDS);
+
+        $validated_question = array();
         foreach($form as $question => $answer) {
-            // `empty(false)` is `true` so handle `bool` separately.
-            if (is_bool($answer)) {
+            # Assert the question is valid.
+            if (!in_array($question, $valid_questions)) {
+                $validation_errors[] = "Invalid question: '$question'";
                 continue;
             }
 
-            if (empty($answer)) {
+            $default_type = gettype(self::BETA_UPLIFT_FIELDS[$question]);
+
+            # Assert the value is not empty.
+            $empty_string = $default_type == "string" && empty($answer);
+            $null_bool = $default_type == "boolean" && is_null($answer);
+            if ($empty_string || $null_bool) {
                 $validation_errors[] = "Need to answer '$question'";
+                continue;
+            }
+
+            # Assert the type from the response matches the type of the default.
+            $answer_type = gettype($answer);
+            if ($default_type != $answer_type) {
+                $validation_errors[] = "Parsing error: type '$answer_type' for '$question' doesn't match expected '$default_type'!";
+                continue;
+            }
+
+            $validated_question[] = $question;
+        }
+
+        # Make sure we have all the required fields present in the response.
+        $missing = array_diff($valid_questions, $validated_question);
+        if (empty($validation_errors) && $missing) {
+            foreach($missing as $missing_question) {
+                $validation_errors[] = "Missing response for $missing_question";
             }
         }
 
         return $validation_errors;
     }
 
     public function qeRequired() {
-        return $this->getValue()['Needs manual QE test'];
+        return $this->getValue()['Needs manual QE test'] === true;
     }
 
     public function validateApplicationTransactions(

webroot/rsrc/css/phui/phui-form-view.css

@@ -557,6 +557,32 @@ properly, and submit values. */
   margin-left: 4px;
 }
 
+.phuix-form-form-row {
+  margin: 2px;
+  display: table-row;
+}
+
+.phuix-form-form-label {
+  margin: 2px;
+  display: table-cell;
+}
+
+.phuix-form-form-textarea {
+  width: auto;
+  margin-left: 4px;
+  vertical-align: middle;
+  display: table-cell;
+}
+
+.phuix-form-form-select {
+  margin: 2px;
+  display: table-cell;
+}
+
+.phuix-form-form-table {
+  display: table;
+}
+
 .phui-form-timer-icon {
   width: 28px;
   height: 28px;

webroot/rsrc/js/phuix/PHUIXFormControl.js

@@ -347,12 +347,24 @@ JX.install('PHUIXFormControl', {
       };
     },
 
+    // Convert "true" and "false" strings to bool.
+    _boolValue: function(string_value) {
+        switch (string_value) {
+            case 'true':
+                return true;
+            case 'false':
+                return false;
+        }
+
+        return null;
+    },
+
     // Render a sub-form within the form control space.
     // Takes a spec of fields and their answer formats and returns
     // a mapping of fields to values.
     _newForm: function(spec) {
-        //var questions_parsed = JX.JSON.parse(spec.questions);
         var questions_parsed = spec.questions;
+        var initial = spec.initial;
 
         var questions = [];
         var question_list = [];
@@ -362,42 +374,62 @@ JX.install('PHUIXFormControl', {
 
             // Determine input format, either a bool or a text value.
             var value_type = typeof (value);
-            var input_type;
+
+            var question_obj;
             if (value_type == "string") {
-                input_type = "textarea";
+              question_obj = JX.$N(
+                'input',
+                {
+                  className: 'aphront-form-control phuix-form-form-textarea',
+                  type: 'textarea',
+                  value: value,
+                  id: question_id,
+                  question: question,
+              });
             } else if (value_type == "boolean") {
-                input_type = "checkbox";
+                // Set "Select One" as the default.
+                if (initial) {
+                    value = null;
+                }
+                question_obj = JX.Prefab.renderSelect(
+                    {
+                        null: 'Select One',
+                        true: 'Yes',
+                        false: 'No',
+                    },
+                    value,
+                    {
+                        className: 'phuix-form-form-select',
+                        id: question_id,
+                        question: question,
+                        value: value,
+                    },
+                    [null, true, false]
+                );
             }
 
-            var question_obj = JX.$N(
-              'input',
-              {
-                type: input_type,
-                value: value,
-                id: question_id,
-                question: question,
-              });
-
             questions.push(question_obj);
 
             var label = JX.$N(
               'label',
               {
-                className: 'phuix-form-checkbox-label',
+                className: 'phuix-form-form-label',
                 htmlFor: question_id,
               },
               JX.$H(question || ''));
 
             var display = JX.$N(
               'div', {
-                className: 'phuix-form-checkbox-item'
+                className: 'phuix-form-form-row',
               },
-              [question_obj, label]);
+              [label, question_obj]);
 
             question_list.push(display);
         }
 
-        var node = JX.$N('div', {}, question_list);
+        var node = JX.$N('div', {
+            className: 'phui-form-view phui-form-form-table'
+        }, question_list);
 
         return {
             node: node,
@@ -406,8 +438,10 @@ JX.install('PHUIXFormControl', {
                 for (var ii = 0; ii < questions.length; ii++) {
                     var question_obj = questions[ii];
 
-                    if (question_obj.type == "checkbox") {
-                        map[question_obj.question] = question_obj.checked;
+                    if (question_obj.type == "select-one") {
+                        // Convert "true" and "false" string values to boolean.
+                        var bool_val = this._boolValue(question_obj.value);
+                        map[question_obj.question] = bool_val;
                     } else {
                         map[question_obj.question] = question_obj.value;
                     }