Updates Passphrase for modular transactions.
Details
Details
- Reviewers
epriestley - Commits
- rP06eae5578bff: Update Passphrase for modular transactions
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
Diff Detail
- Repository
- rP Phabricator
- Branch
- passphrase-xaction (branched from master)
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 16798 Build 22419: Run Core Tests Build 22418: arc lint + arc unit
Event Timeline
Comment Actions
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). | |
| 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. | |
Comment Actions
Oh, no, it won't work for transactions which have been modularized. You can implement shouldHide() on the LookedAtTransaction and other modular types instead.
Comment Actions
Still stuck on the destroy transaction, which doesn't seem to call shouldHide at all when I drop in debugging.
Comment Actions
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"?