From 670b18fc2e49fb05483414caf4a73b3563af3c9f Mon Sep 17 00:00:00 2001 From: Connor Sheehan Date: Fri, 5 Sep 2025 12:36:36 -0400 Subject: [PATCH 1/2] Bug 1987196 - remove server side uplift request assessment form validation Remove the server-side validation of the uplift request assessment form. Also remove the form comment action, so the only way to update the form is from Lando. --- .../PhabricatorUpdateUpliftCommentAction.php | 26 --- .../DifferentialUpliftRequestCustomField.php | 171 +----------------- ...entialUpliftRequestCustomFieldTestCase.php | 36 ---- 3 files changed, 1 insertion(+), 232 deletions(-) delete mode 100644 moz-extensions/src/applications/transactions/commentaction/PhabricatorUpdateUpliftCommentAction.php delete mode 100644 moz-extensions/src/differential/customfield/__tests__/DifferentialUpliftRequestCustomFieldTestCase.php diff --git a/moz-extensions/src/applications/transactions/commentaction/PhabricatorUpdateUpliftCommentAction.php b/moz-extensions/src/applications/transactions/commentaction/PhabricatorUpdateUpliftCommentAction.php deleted file mode 100644 index 9466401df4..0000000000 --- a/moz-extensions/src/applications/transactions/commentaction/PhabricatorUpdateUpliftCommentAction.php +++ /dev/null @@ -1,26 +0,0 @@ -getValue(); - $initial = false; - - if (empty($value) || $value == null) { - $value = $this->getInitialValue(); - $initial = true; - } - - return array( - 'initial' => $initial, - 'questions' => $value, - ); - - } - -} diff --git a/moz-extensions/src/differential/customfield/DifferentialUpliftRequestCustomField.php b/moz-extensions/src/differential/customfield/DifferentialUpliftRequestCustomField.php index a2e43eca58..6f01b95c00 100644 --- a/moz-extensions/src/differential/customfield/DifferentialUpliftRequestCustomField.php +++ b/moz-extensions/src/differential/customfield/DifferentialUpliftRequestCustomField.php @@ -9,19 +9,6 @@ final class DifferentialUpliftRequestCustomField extends DifferentialStoredCustomField { - // Questions for beta, with defaults. - const BETA_UPLIFT_FIELDS = array( - "User impact if declined" => "", - "Code covered by automated testing" => false, - "Fix verified in Nightly" => false, - "Needs manual QE test" => false, - "Steps to reproduce for manual QE testing" => "", - "Risk associated with taking this patch" => "", - "Explanation of risk level" => "", - "String changes made/needed" => "", - "Is Android affected?" => false, - ); - // How each field is formatted in ReMarkup: // a bullet point with text in bold. const QUESTION_FORMATTING = "- **%s** %s"; @@ -88,80 +75,11 @@ public function renderPropertyViewValue(array $handles) { return new PHUIRemarkupView($this->getViewer(), $this->getRemarkup()); } - // Returns `true` if the field meets all conditions to be editable. - public function isFieldActive() { - return $this->isUpliftTagSet() && $this->objectHasBugNumber(); - } - - public function objectHasBugNumber(): bool { - // Similar idea to `BugStore::resolveBug`. - $bugzillaField = new DifferentialBugzillaBugIDField(); - $bugzillaField->setObject($this->getObject()); - (new PhabricatorCustomFieldStorageQuery()) - ->addField($bugzillaField) - ->execute(); - $bug = $bugzillaField->getValue(); - - if (!$bug) { - return false; - } - - return true; - } - // How the field can be edited in the "Edit Revision" menu. public function renderEditControl(array $handles) { return null; } - // -- Comment action things - - public function getCommentActionLabel() { - return pht('Request Uplift'); - } - - // Return `true` if the `uplift` tag is set on the repository belonging to - // this revision. - private function isUpliftTagSet() { - $revision = $this->getObject(); - $viewer = $this->getViewer(); - - if ($revision == null || $viewer == null) { - return false; - } - - try { - $repository_projects = PhabricatorEdgeQuery::loadDestinationPHIDs( - $revision->getFieldValuesForConduit()['repositoryPHID'], - PhabricatorProjectObjectHasProjectEdgeType::EDGECONST - ); - } catch (Exception $e) { - return false; - } - - if (!(bool)$repository_projects) { - return false; - } - - $uplift_project = id(new PhabricatorProjectQuery()) - ->setViewer($viewer) - ->withNames(array('uplift')) - ->executeOne(); - - // The `uplift` project isn't created or can't be found. - if (!(bool)$uplift_project) { - return false; - } - - // If the `uplift` project PHID is in the set of all project PHIDs - // attached to the repo, return `true`. - if (in_array($uplift_project->getPHID(), $repository_projects)) { - return true; - } - - return false; - } - // Convert `bool` types to readable text, or return base text. private function valueAsAnswer($value): string { if ($value === true) { @@ -188,94 +106,7 @@ private function getRemarkup(): string { } public function newCommentAction() { - // Returning `null` causes no comment action to render, effectively - // "disabling" the field. - if (!$this->isFieldActive()) { - return null; - } - - $action = id(new PhabricatorUpdateUpliftCommentAction()) - ->setConflictKey('revision.action') - ->setValue($this->getValue()) - ->setInitialValue(self::BETA_UPLIFT_FIELDS) - ->setSubmitButtonText(pht('Request Uplift')); - - return $action; - } - - public function validateUpliftForm(array $form): array { - $validation_errors = array(); - - # Allow clearing the form. - if (empty($form)) { - return $validation_errors; - } - - $valid_questions = array_keys(self::BETA_UPLIFT_FIELDS); - - $validated_question = array(); - foreach($form as $question => $answer) { - # Assert the question is valid. - if (!in_array($question, $valid_questions)) { - $validation_errors[] = "Invalid question: '$question'"; - continue; - } - - $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 validateApplicationTransactions( - PhabricatorApplicationTransactionEditor $editor, - $type, array $xactions) { - - $errors = parent::validateApplicationTransactions($editor, $type, $xactions); - - foreach($xactions as $xaction) { - // Validate that the form is correctly filled out. - // This should always be a string (think if the value came from the remarkup edit) - $validation_errors = $this->validateUpliftForm( - phutil_json_decode($xaction->getNewValue()), - ); - - // Push errors into the revision save stack - foreach($validation_errors as $validation_error) { - $errors[] = new PhabricatorApplicationTransactionValidationError( - $type, - '', - pht($validation_error) - ); - } - } - - return $errors; + return null; } // When storing the value convert the question => answer mapping to a JSON string. diff --git a/moz-extensions/src/differential/customfield/__tests__/DifferentialUpliftRequestCustomFieldTestCase.php b/moz-extensions/src/differential/customfield/__tests__/DifferentialUpliftRequestCustomFieldTestCase.php deleted file mode 100644 index e63cb1320a..0000000000 --- a/moz-extensions/src/differential/customfield/__tests__/DifferentialUpliftRequestCustomFieldTestCase.php +++ /dev/null @@ -1,36 +0,0 @@ -validateUpliftForm(array( - "User impact if declined" => "", - "Steps to reproduce for manual QE testing" => "", - "Risk associated with taking this patch" => "", - "Explanation of risk level" => "", - "String changes made/needed" => "", - )); - - $expected = array(); - foreach(DifferentialUpliftRequestCustomField::BETA_UPLIFT_FIELDS as $question => $answer) { - if (is_bool($answer)) { - continue; - } - $expected[] = "Need to answer '$question'"; - } - $this->assertEqual($expected, $errors); - - // Ensure the field can be set as empty - $errors = $field->validateUpliftForm(array()); - $this->assertEqual( - array(), - $errors, - "The empty form leads to errors - should be allowed."); - } -} From f1f0bebac32972f5e2db05cd58cb3472df18c64c Mon Sep 17 00:00:00 2001 From: Connor Sheehan Date: Tue, 9 Sep 2025 10:46:23 -0400 Subject: [PATCH 2/2] remove from edit view --- .../customfield/DifferentialUpliftRequestCustomField.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/moz-extensions/src/differential/customfield/DifferentialUpliftRequestCustomField.php b/moz-extensions/src/differential/customfield/DifferentialUpliftRequestCustomField.php index 6f01b95c00..ae817bd42a 100644 --- a/moz-extensions/src/differential/customfield/DifferentialUpliftRequestCustomField.php +++ b/moz-extensions/src/differential/customfield/DifferentialUpliftRequestCustomField.php @@ -63,7 +63,7 @@ public function shouldAppearInApplicationTransactions() { public function shouldAppearInEditView() { // Should the field appear in Edit Revision feature - return true; + return false; } // How the uplift text is rendered in the "Details" section.