Skip to content

Bug 1987196 - remove server side uplift request assessment form validation #69

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Sep 10, 2025
Merged

Bug 1987196 - remove server side uplift request assessment form validation #69

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
670b18f
Bug 1987196 - remove server side uplift request assessment form valid…
cgsheeh Sep 5, 2025
f1f0beb
remove from edit view
cgsheeh Sep 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 0 additions & 26 deletions ...ions/src/applications/transactions/commentaction/PhabricatorUpdateUpliftCommentAction.php
View file
Open in desktop

This file was deleted.

173 changes: 2 additions & 171 deletions moz-extensions/src/differential/customfield/DifferentialUpliftRequestCustomField.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -76,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.
Expand All @@ -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) {
Expand All @@ -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.
Expand Down
36 changes: 0 additions & 36 deletions ...s/src/differential/customfield/__tests__/DifferentialUpliftRequestCustomFieldTestCase.php
View file
Open in desktop

This file was deleted.