Page MenuHomePhabricator

Update Passphrase for modular transactions
ClosedPublic

Authored by chad on May 4 2017, 12:59 AM.
Tags
None
Referenced Files
F14230913: D17824.diff
Wed, Dec 11, 10:01 PM
Unknown Object (File)
Wed, Dec 4, 7:52 PM
Unknown Object (File)
Wed, Dec 4, 7:48 PM
Unknown Object (File)
Tue, Nov 26, 11:06 PM
Unknown Object (File)
Thu, Nov 21, 8:01 PM
Unknown Object (File)
Mon, Nov 18, 8:43 AM
Unknown Object (File)
Fri, Nov 15, 7:04 AM
Unknown Object (File)
Fri, Nov 15, 5:29 AM
Subscribers

Details

Summary

Updates Passphrase for modular transactions.

Test Plan

Create, edit, lock, view, lots of different types of Passphrases. Enable Conduit, Lock Passphrases, Destroy Secrets from the interface and verify from the DB it was eradicated.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

To swap this to EditEngine, I think we'll have to rewrite how some of the transactions work (for example, the "destroy" + "set secret" would become a single "set secret" transaction, and the new PassphraseSecret() stuff will happen inside that transaction).

The logic around "it doesn't count if the value is just a bunch of bullets" probably needs to move to some new PhabricatorSecretEditField, too.

We don't need to do this here; moving to EditEngine doesn't block T10448.

The rest of this looks right.

src/applications/passphrase/xaction/PassphraseCredentialConduitTransaction.php
18

These are correct, although it's a little weird to switch ($old) and then render the opposite, instead of switch ($new).

This revision is now accepted and ready to land.May 4 2017, 1:52 PM
src/applications/passphrase/storage/PassphraseCredentialTransaction.php
17–18

Does ShouldHide work in Modular Transactions then? Mainly, I'm getting an "set the credential to locked" transaction on any create regardless of if it's locked or not, so it's not a good experence.

Oh, no, it won't work for transactions which have been modularized. You can implement shouldHide() on the LookedAtTransaction and other modular types instead.

  • updates for shouldHide

Still stuck on the destroy transaction, which doesn't seem to call shouldHide at all when I drop in debugging.

image.png (1×996 px, 128 KB)

I think:

  • DestroyTransaction->shouldHide() isn't being called because we don't add a DestroyTransaction during creation.
  • The string "... destroyed the secret for this credential." is actually coming from the PassphraseCredentialSecretIDTransaction, which renders in sort of a weird way.
  • You can hide it like this, by implementing shouldHide() on the SecretID transaction:
diff --git a/src/applications/passphrase/xaction/PassphraseCredentialSecretIDTransaction.php b/src/applications/passphrase/xaction/PassphraseCredentialSecretIDTransaction.php
index 1afacb88d2..09108b66fd 100644
--- a/src/applications/passphrase/xaction/PassphraseCredentialSecretIDTransaction.php
+++ b/src/applications/passphrase/xaction/PassphraseCredentialSecretIDTransaction.php
@@ -17,6 +17,14 @@ final class PassphraseCredentialSecretIDTransaction
     $object->setSecretID($value);
   }
 
+  public function shouldHide() {
+    if (!$this->getOldValue()) {
+      return true;
+    }
+
+    return false;
+  }
+
   public function getTitle() {
     return pht(
       '%s destroyed the secret for this credential.',
  • The strings themselves probably need tweaking too, since I think they're from the wrong place and should actually be "updated the secret for this credential"?
  • better language, icons
chad edited the test plan for this revision. (Show Details)

ok, im like, going to land this.

This revision was automatically updated to reflect the committed changes.