Maniphest T12084

`differential.updaterevision` conduit method no longer updates custom fields
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	jcox
	Jan 9 2017, 4:22 PM

Description

We have a class that extends DifferentialCoreCustomField (class body included below). Prior to D17067, calling differential.updaterevision with fields: {"devTestPHIDs" : ["PHID-TASK-ooohnicetaskphidjosh"] } worked as expected, adding the devTestPHIDs field to the revision object. After D17067, the call to updaterevision seems to succeed (no errors are returned), but it does not add the value to the revision.

While digging into it I found that the field map which gets created in DifferentialConduitAPIMethod no longer includes our custom field (as of https://secure.phabricator.com/rP64509dc#202368f8).

The old way of constructing the $field_map is:

$field_list = PhabricatorCustomField::getObjectFields(
    $revision,
    DifferentialCustomField::ROLE_COMMITMESSAGEEDIT);

$field_list
    ->setViewer($viewer)
    ->readFieldsFromStorage($revision);
$field_map = mpull($field_list->getFields(), null, 'getFieldKeyForConduit');

This includes our CIDevelopmentTestsDifferentialCustomField. However the new way of constructing the $field_map is:

$field_map = DifferentialCommitMessageField::newEnabledFields($viewer);

This does not include our CIDevelopmentTestsDifferentialCustomField.

I'm wondering whether the differential overhaul changed how custom fields should be added or if there's a legitimate bug with differential.updaterevision since the update.

Repro Steps

Add a custom differential field class by extending DifferentialCoreCustomField
Attempt to update your custom field by calling the differential.updaterevision conduit endpoint.
Note that the field does not get updated.

Version Info

phabricator 9d10727f651fd4b359c4893f1886b7e436faad91 (Fri, Dec 30)
arcanist e8b580d2e5e740071b25d087a0aca33f0b0dd691 (Fri, Dec 30)
phutil 0ae0cc00acb1413c22bfe3384fd6086ade4cc206 (Fri, Dec 9)

CIDevelopmentTestsDifferentialCustomField.php:

<?php

final class CIDevelopmentTestsDifferentialCustomField
  extends DifferentialCoreCustomField {

  public function getFieldKey() {
    return 'integrator:dev-tests';
  }

  public function getFieldKeyForConduit() {
    return 'devTestPHIDs';
  }

  public function canDisableField() {
    return false;
  }

  public function shouldAppearInEditView() {
    return false;
  }

  public function getFieldName() {
    return pht('Dev Tests');
  }

  public function getFieldDescription() {
    return pht(
      'Tasks corresponding to extended testing performed on this revision, '.
      'such as track testing on a vehicle, or staging area deployments.');
  }

  public function shouldAppearInPropertyView() {
    return true;
  }

  public function renderPropertyViewLabel() {
    return $this->getFieldName();
  }

  protected function readValueFromRevision(DifferentialRevision $revision) {
    if (!$revision->getPHID()) {
      return array();
    }

    return PhabricatorEdgeQuery::loadDestinationPHIDs(
      $revision->getPHID(),
      CIRevisionHasDevelopmentTestEdgeType::EDGECONST);
  }

  public function getApplicationTransactionType() {
    return PhabricatorTransactions::TYPE_EDGE;
  }

  public function getApplicationTransactionMetadata() {
    return array(
      'edge:type' => CIRevisionHasDevelopmentTestEdgeType::EDGECONST,
    );
  }

  public function getNewValueForApplicationTransactions() {
    $edges = array();
    foreach ($this->getValue() as $phid) {
      $edges[$phid] = $phid;
    }

    return array('=' => $edges);
  }

  public function getRequiredHandlePHIDsForPropertyView() {
    return $this->getValue();
  }

  public function renderPropertyViewValue(array $handles) {
    return $this->renderHandleList($handles);
  }

  public function shouldAppearInCommitMessage() {
    return true;
  }

  public function shouldAllowEditInCommitMessage() {
    return true;
  }

  public function getCommitMessageLabels() {
    return array(
      'Dev Test',
      'Dev Tests',
    );
  }

  public function parseValueFromCommitMessage($value) {
    return $this->parseObjectList(
      $value,
      array(
        ManiphestTaskPHIDType::TYPECONST,
      ));
  }

  public function getRequiredHandlePHIDsForCommitMessage() {
    return $this->getRequiredHandlePHIDsForPropertyView();
  }

  public function renderCommitMessageValue(array $handles) {
    return $this->renderObjectList($handles);
  }

}

Related Objects

Mentioned Here: rP62cf4e6b95ab: Remove some remnants of the old ways commit mesage fields worked from…
D17207: Remove some remnants of the old ways commit mesage fields worked from Differential
T12085: Upgrading: Changes to Differential Custom Fields in Commit Messages
rPHU0ae0cc00acb1: (stable) Promote 2016 Week 50
rP9d10727f651f: (stable) Promote 2016 Week 53
rARCe8b580d2e5e7: (stable) Promote 2016 Week 53
D17067: Drive CLI-based revision edits through "differential.revision.edit" API + EditEngine

Event Timeline

jcox created this task.Jan 9 2017, 4:22 PM

I think this is mostly guidance I neglected to issue, sorry about that.

The ...From/ForCommitMessage stuff should move to a new subclass of DifferentialCommitMessageField.

For example, DifferentialTestPlanField now holds the web/storage parts of the Test Plan, but DifferentialTestPlanCommitMessageField handles the message parsing.

So:

Define CIDevelopmentTestsDifferentialCommitMessageField.
Tie it together by preserving the constants:

const FIELDKEY = 'devTestPHIDs';

public function getCustomFieldKey() {
  return 'integrator:dev-tests';
}

Update these methods:

shouldAppearInCommitMessage() - remove
shouldAllowEditInCommitMessage() - remove
getCommitMessageLabels() - rename to getFieldAliases() on the MessageField
parseValueFromCommitMessage() - rename to parseFieldValue() on the MessageField
getRequiredHandlePHIDsForCommitMessage() - remove
renderCommitMessageValue() - rename to renderFieldValue()

You can look at DifferentialTasksCommitMessageField (another object-valued field) and DifferentialRevertPlanCommitMessageField (another custom field) as guides, I think.

I've collected some guidance more formally in T12085.

Thanks for the help Evan. I've gone through and converted everything as you suggested. The conduit method then started telling me

Method getFieldTransactions in class CIDevelopmentTestsDifferentialCommitMessageField is not implemented!

I then added that method (following the example in DifferentialTasksCommitMessageField) and was given this error:

EXCEPTION (Exception): Transaction with key "1" has invalid type "devtests". This type is not recognized. Valid types are: update, comment, title, summary, testPlan, reviewers.add, reviewers.remove, reviewers.set, repositoryPHID, tasks.add, tasks.remove, tasks.set, view, edit, projects.add, projects.remove, projects.set, subscribers.add, subscribers.remove, subscribers.set, phabricator:auditors.

So I tried to just use edit as the transaction type, which got rid of that error. However, the conduit method still doesn't actually add the field to the revision when I call it. I suspect I may have to make a new transaction type for my field.

Oh -- try subclassing DifferentialCommitMessageCustomField instead of DifferentialCommitMessageField directly?

Doing that without defining getFieldTransactions() in my class yields this exception:

Transaction with key "1" has invalid type "devTestPHIDs". This type is not recognized. Valid types are: update, comment, title, summary, testPlan, reviewers.add, reviewers.remove, reviewers.set, repositoryPHID, tasks.add, tasks.remove, tasks.set, view, edit, projects.add, projects.remove, projects.set, subscribers.add, subscribers.remove, subscribers.set, phabricator:auditors.

If I define getFieldTransactions() like this, it adds the object as a "task" on the revision and not in my custom field.

public function getFieldTransactions($value) {
    return array(
      array(
        'type' => 'tasks.add',
        'value' => $value,
      ),
    );
  }

Ah, sorry, I overlooked that you're using edges.

Subclass PhabricatorEditEngineExtension, following PhabricatorProjectsEditEngineExtension to define a new edge transaction for revisions. Without changes elsewhere, this should appear in the Conduit console for differential.revision.edit alongside subscribers.add.

That should also populate it into the "Valid types: ..." list, and then you can define getFieldTransactions() to apply a devtests.set transaction or similar (not sure if "set" or "add" is more appropriate in this context).

You likely do not actually need to subclass DifferentialCommitMessageCustomField; DifferentialCommitMessageField is probably fine.

Thanks that did it! Still working through an issue with one of our more *ahem* exotic applications, but I suspect that'll be a more involved refactor on our end.

Awesome! Sorry for the churn here -- I think we have a brighter future on the other side, but have to break a few eggs to get there.

Checking in for guidance in case this is the wrong way to go. I have a custom field which extends DifferentialStoredCustomField and I'm trying to convert it in the same way as the above. Thus far my field isn't being saved when calling differential.createrevision. I've included the DifferentialCommitMessageCustomField and the DifferentialStoredCustomField below.

I suspect that I need to define a new DifferentialRevisionTransactionType (perhaps following DifferentialRevisionTestPlanTransaction) and move the *ApplicationTransaction* methods from below into my new type.

CIReleaseNotesDifferentialCommitMessageCustomField.php

<?php

final class CIReleaseNotesDifferentialCommitMessageCustomField
  extends DifferentialCommitMessageCustomField {

  const FIELDKEY = 'releaseNotes';

  public function getFieldName() {
    return pht('Release Notes');
  }

  public function getFieldAliases() {
    return array();
  }

  public function getCustomFieldKey() {
    return self::FIELDKEY;
  }

  public function isTemplateField() {
    return true;
  }

  public function renderFieldValue($value) {
    return $value;
  }

}

CIReleaseNotesDifferentialCustomField.php

<?php

final class CIReleaseNotesDifferentialCustomField
  extends DifferentialStoredCustomField {

  const KEY_RELEASE_NOTES = 'integrator:release-notes';

  public function getFieldKey() {
    return self::KEY_RELEASE_NOTES;
  }

  public function shouldAppearInListView() {
    return true;
  }

  public function shouldAppearInConduitDictionary() {
    return true;
  }

  public function getFieldName() {
    return pht('Release Notes');
  }

  public function getFieldDescription() {
    return pht(
      'Summarizes information about this change suitable for release notes.');
  }

  public function getWarningsForRevisionHeader(array $handles) {
    $warnings = [];
    $revision = $this->getObject();
    $release_products = PhabricatorCustomField::getObjectField(
      $revision,
      PhabricatorCustomField::ROLE_VIEW,
      CIReleaseProductsDifferentialCustomField::KEY_RELEASE_PRODUCTS);
    $phids = $release_products->getValue();
    if ($this->getValue() && !$phids) {
      $warnings[] = pht(
        'Release notes are specified for this revision but there are no '.
        'accompanying release products.');
    }
    return $warnings;
  }

  public function shouldDisableByDefault() {
    return false;
  }

  public function renderPropertyViewLabel() {
    return $this->getFieldName();
  }

  public function shouldAppearInPropertyView() {
    return true;
  }

  public function getStyleForPropertyView() {
    return 'block';
  }

  public function getIconForPropertyView() {
    return 'fa-sticky-note-o';
  }

  public function renderPropertyViewValue(array $handles) {
    if (!strlen($this->getValue())) {
      return null;
    }

    return new PHUIRemarkupView($this->getViewer(), $this->getValue());
  }

  public function shouldAppearInEditView() {
    return true;
  }

  public function shouldAppearInApplicationTransactions() {
    return false;
  }

  public function getOldValueForApplicationTransactions() {
    return $this->getValue();
  }

  public function getNewValueForApplicationTransactions() {
    return $this->getValue();
  }

  public function readValueFromRequest(AphrontRequest $request) {
    $this->setValue($request->getStr($this->getFieldKey()));
  }

  public function renderEditControl(array $handles) {
    return id(new PhabricatorRemarkupControl())
      ->setUser($this->getViewer())
      ->setName($this->getFieldKey())
      ->setValue($this->getValue())
      ->setLabel($this->getFieldName());
  }

  public function getApplicationTransactionTitle(
    PhabricatorApplicationTransaction $xaction) {
    $author_phid = $xaction->getAuthorPHID();
    $old = $xaction->getOldValue();
    $new = $xaction->getNewValue();

    return pht(
      '%s updated the release notes for this revision.',
      $xaction->renderHandleLink($author_phid));
  }

  public function getApplicationTransactionTitleForFeed(
    PhabricatorApplicationTransaction $xaction) {

    $object_phid = $xaction->getObjectPHID();
    $author_phid = $xaction->getAuthorPHID();
    $old = $xaction->getOldValue();
    $new = $xaction->getNewValue();

    return pht(
      '%s updated the release notes for %s.',
      $xaction->renderHandleLink($author_phid),
      $xaction->renderHandleLink($object_phid));
  }

  public function getApplicationTransactionHasChangeDetails(
    PhabricatorApplicationTransaction $xaction) {
    return true;
  }

  public function getApplicationTransactionChangeDetails(
    PhabricatorApplicationTransaction $xaction,
    PhabricatorUser $viewer) {
    return $xaction->renderTextCorpusChangeDetails(
      $viewer,
      $xaction->getOldValue(),
      $xaction->getNewValue());
  }

  public function shouldHideInApplicationTransactions(
    PhabricatorApplicationTransaction $xaction) {
    return ($xaction->getOldValue() === null);
  }

  public function getApplicationTransactionRemarkupBlocks(
    PhabricatorApplicationTransaction $xaction) {
    return array($xaction->getNewValue());
  }

  public function readValueFromCommitMessage($value) {
    $this->setValue($value);
    return $this;
  }

  public function shouldOverwriteWhenCommitMessageIsEdited() {
    return true;
  }

}

@jcox, do you have 62cf4e6b95abbebc8ae2b1591039967651188c6e (D17207) yet? That might be the fix. Nothing jumps out at me as wrong, but I can take a closer look if that change doesn't help.

I pulled in that change and I'm still seeing the same issue. Editing the field from /differential/revision/edit/... seems to work normally, but adding it via the differential.createrevision endpoint does not.

Alright, I'll take a more detailed look.

I changed CIReleaseNotesDifferentialCustomField->shouldAppearInApplicationTransactions() to return true to make the web edit to work correctly. (Did you not want this field to be editable from the web?)
I changed CIReleaseNotesDifferentialCommitMessageCustomField::FIELDKEY to integrator:release-notes (the same value as CIReleaseNotesDifferentialCustomField::KEY_RELEASE_NOTES). Although this is not strictly required, it needs to be the same in some cases to bind the CommitMessage field to the Custom field, and making them the same makes several behaviors work correctly without additional changes.
I implemented CIReleaseNotesDifferentialCustomField->shouldAppearInConduitTransactions() to return true;.
I implemented CIReleaseNotesDifferentialCustomField->newConduitEditParameterType() to return new ConduitStringParameterType();.

After these changes, things seem to work. Here's: cli, detail view, web edit, conduit for me locally:

Screen Shot 2017-01-16 at 8.43.51 AM.png (1×1 px, 103 KB)

Screen Shot 2017-01-16 at 8.44.02 AM.png (1×1 px, 213 KB)

Screen Shot 2017-01-16 at 8.44.07 AM.png (1×1 px, 184 KB)

Screen Shot 2017-01-16 at 8.44.24 AM.png (1×1 px, 203 KB)

Does that seem to work on your end?

ah yep that seemed to work. I was still specifying releaseNotes for the field which I'm guessing is why it wasn't getting saved. Thanks again for all the help! If you're ever in Pittsburgh I think I owe you about 50 beers at this point.

	F2396440: Screen Shot 2017-01-16 at 8.44.02 AM.png
	Jan 16 2017, 4:50 PM

	F2396439: Screen Shot 2017-01-16 at 8.44.07 AM.png
	Jan 16 2017, 4:50 PM

	F2396437: Screen Shot 2017-01-16 at 8.44.24 AM.png
	Jan 16 2017, 4:50 PM

	F2396438: Screen Shot 2017-01-16 at 8.43.51 AM.png
	Jan 16 2017, 4:50 PM

`differential.updaterevision` conduit method no longer updates custom fieldsClosed, ResolvedPublicActions