Page MenuHomePhabricator

Maniphest batch editor can affect far too many tasks
Closed, ResolvedPublic

Description

There is a serious UX issue in the batch editor which can cause far too many tasks to be affected. To reproduce:

  • Try to edit a collection of tasks which are all not editable (you must not have permission to edit any of the tasks you select).
  • The edit will incorrectly apply to every task you do have permission to edit.

This is prevented by D13388 and the root issue is fixed by D13390.

I think the additional steps below remain worthwhile.


Previously

A user showed up in IRC with a batch edit misfire that is not reproducible or technically distinguishable from user error (edit: now reproducible and highly distinguishable), while running an old version of Phabricator with third-party plugins. Since this is hugely disruptive and I can't prove it isn't our fault (edit: 100% our fault) I don't want to brush it off, but it would be helpful to have better tools to log and recover from these misfires.

Particularly:

  • Log batch edits and give them PHIDs, so there's a record of who batch edited what and when. We need this for T5166 anyway.
  • Save the batch edit PHID on the transaction record: this can make recovery easier, at least, by letting us identify the changes with certainty.

And maybe try harder to prevent user error:

  • Require extra-confirm if editing more than ~100 things?

Hypothetically, we could make transactions reversible so you could undo edits, but I'm not eager to pursue this given the tremendous amount of work it entails.

Event Timeline

epriestley raised the priority of this task from to Normal.
epriestley updated the task description. (Show Details)
epriestley added a project: Maniphest.
epriestley added a subscriber: epriestley.

This appears to work to undo a status change batch edit:

  • Drop this into phabricator/ as undo_batch_edit.php or similar.
  • Run phabricator/ $ php -f undo_batch_edit.php.
IMPORTANT: You need to configure the variables in the header for this to work.
<?php

// IMPORTANT: This script ONLY works for "status" batch edits, not for other
// types of batch edits.

// Put the PHID of the user who performed the bad edit here. You can get this
// by running `user.query` from the Conduit web API.
$actor_phid = 'PHID-USER-cvfydnwadpdj7vdon36z';


// Specify the start and end time of the bad edit here. We'll ignore changes
// outside this time range. By default, this is looking at changes in the last
// 24 hours.
$min_date = time() - (24 * 60 * 60);
$max_date = time();


// Specify the bad status that was set with the edit here. This is a key from
// the `maniphest.statuses` config, like "closed" or "invalid".
$bad_status = 'spite';


// Change this to "true" to actually apply changes. With this flag set to
// false, the script will run and show debugging output, but not do anything.
$apply_changes = false;


// --- End of Configuration ------------------------------------------------- //

require_once 'scripts/__init_script__.php';

$table = new ManiphestTask();
$conn_w = $table->establishConnection('w');
foreach (new LiskMigrationIterator($table) as $task) {
  $id = $task->getID();
  echo "Processing task T{$id}.\n";

  $status = $task->getStatus();
  if ($status != $bad_status) {
    echo "This task has status '{$status}', not the bad status; skipping.\n";
    continue;
  }

  $xactions = id(new ManiphestTransactionQuery())
    ->setViewer(PhabricatorUser::getOmnipotentUser())
    ->withObjectPHIDs(array($task->getPHID()))
    ->execute();
  msort($xactions, 'getID');

  // Find the bad edit so we know if and when this task was affected.

  $bad_edit = null;
  foreach ($xactions as $xaction) {
    if ($xaction->getTransactionType() != ManiphestTransaction::TYPE_STATUS) {
      continue;
    }

    if ($xaction->getAuthorPHID() != $actor_phid) {
      continue;
    }

    if ($xaction->getDateCreated() < $min_date) {
      continue;
    }

    if ($xaction->getDateCreated() > $max_date) {
      continue;
    }

    if ($xaction->getNewValue() != $bad_status) {
      continue;
    }

    if ($bad_edit) {
      if ($xaction->getID() > $bad_edit->getID()) {
        continue;
      }
    }

    $bad_edit = $xaction;
  }

  if (!$bad_edit) {
    echo "No matching edit to this task; skipping.\n";
    continue;
  }

  // Find the prior status change so we can figure out what status to revert
  // to.

  $last_good_status = ManiphestTaskStatus::getDefaultStatus();
  foreach ($xactions as $xaction) {
    if ($xaction->getID() == $bad_edit->getID()) {
      break;
    }

    if ($xaction->getTransactionType() != ManiphestTransaction::TYPE_STATUS) {
      continue;
    }

    $last_good_status = $xaction->getNewValue();
  }

  if (!$apply_changes) {
    echo "Would change status to '{$last_good_status}' if apply changes ".
         "flag was set.\n";
    continue;
  }

  echo "Updating task to status '{$last_good_status}'.\n";
  queryfx(
    $conn_w,
    'UPDATE %T SET status = %s WHERE id = %d',
    $table->getTableName(),
    $last_good_status,
    $task->getID());
}
epriestley renamed this task from Create a log for Maniphest batch edits to Maniphest batch editor can affect far too many tasks.Jun 22 2015, 1:20 PM
epriestley updated the task description. (Show Details)

After mulling this over for a bit while dealing with other stuff, I'm pretty convinced that the general purpose infrastructure in T5166 is the way forward here, so my plan is to move forward with that.

epriestley raised the priority of this task from Normal to High.Jun 22 2015, 3:21 PM

I have this working functionally, but the UI is still an atrocious mess. Probably have something ready tomorrow.