Differential D19877 Diff 47468 src/applications/people/xaction/PhabricatorUserApproveTransaction.php

Changeset View

Standalone View

src/applications/people/xaction/PhabricatorUserApproveTransaction.php

This file was added.

				<?php

				final class PhabricatorUserApproveTransaction
				extends PhabricatorUserTransactionType {

				const TRANSACTIONTYPE = 'user.approve';

				public function generateOldValue($object) {
				return (bool)$object->getIsApproved();
				}

				public function generateNewValue($object, $value) {
				return (bool)$value;
				}

				public function applyInternalEffects($object, $value) {
				$object->setIsApproved((int)$value);
				}

				public function applyExternalEffects($object, $value) {
				$user = $object;
				$this->newUserLog(PhabricatorUserLog::ACTION_APPROVE)
				->setOldValue((bool)$user->getIsApproved())
				->setNewValue((bool)$value)
				->save();

				amckinleyAuthorUnsubmitted Done Inline Actions In `PhabricatorUserDisableTransaction`, this code was inside `applyInternalEffects`, but it feels more like an external effect to me. Or does putting it inside `applyInternalEffects` also wrap the whole thing in a transaction, which we still want? amckinley: In `PhabricatorUserDisableTransaction`, this code was inside `applyInternalEffects`, but it…
				epriestleyUnsubmitted Not Done Inline Actions Yeah, I think it's "internal" since it's on the same database, uh, maybe. The names aren't great nowadays (and perhaps never were?), more like "apply inside-transaction changes" and "apply outside-transaction changes", except the transaction is held through both anyway I'm pretty sure? I'm not sure there's actually any distinction at all anymore. I don't think it matters in any realistic scenario. We should possibly just merge them. Let me see if I can find any reason not to, offhand. epriestley: Yeah, I think it's "internal" since it's on the same database, uh, maybe. The names aren't…
				epriestleyUnsubmitted Not Done Inline Actions Okay, so sequence is: Open transaction. Internal effects. save() External effects. Final effects. Save transaction. didCommitTransactions() Normally, `save()` generates a PHID, so I think the long-ago rationale was: Update the object itself. Do a single save() with all the changes. Now stuff that needs a PHID (edges, etc) can do its thing. But this is muddy because we can generate a PHID at any time, and sometimes do. Another case is that `save()` may occasionally fail with a duplicate key collision that we couldn't (or didn't) detect beforehand. It makes sense to be sure we actually saved the PHID before we start writing rows into other databases. So I guess this distinction is probably worth preserving, but all the rules are very muddy. This is probably an "external" effect from the point of view of "it relies on the user PHID" so I think your implementation is more correct. epriestley: Okay, so sequence is: - Open transaction. - Internal effects. - save() - External…
				$actor = $this->getActor();
				$title = pht(
				'Phabricator Account "%s" Approved',
				$user->getUsername());

				$body = sprintf(
				"%s\n\n %s\n\n",
				pht(
				'Your Phabricator account (%s) has been approved by %s. You can '.
				'login here:',
				$user->getUsername(),
				$actor->getUsername()),
				PhabricatorEnv::getProductionURI('/'));

				$mail = id(new PhabricatorMetaMTAMail())
				->addTos(array($user->getPHID()))
				->addCCs(array($actor->getPHID()))
				->setSubject('[Phabricator] '.$title)
				->setForceDelivery(true)
				->setBody($body)
				->saveAndSend();
				}

				public function getTitle() {
				$new = $this->getNewValue();
				if ($new) {
				return pht(
				'%s approved this user.',
				$this->renderAuthor());
				} else {
				return pht(
				'%s rejected this user.',
				$this->renderAuthor());
				}
				}

				public function shouldHideForFeed() {
				return true;
				}

				public function validateTransactions($object, array $xactions) {
				$actor = $this->getActor();
				$errors = array();

				foreach ($xactions as $xaction) {
				$is_approved = (bool)$object->getIsApproved();

				if ((bool)$xaction->getNewValue() === $is_approved) {
				continue;
				}

				if (!$actor->getIsAdmin()) {
				$errors[] = $this->newInvalidError(
				pht('You must be an administrator to approve users.'));
				}
				}

				return $errors;
				}

				public function getRequiredCapabilities(
				epriestleyUnsubmitted Not Done Inline Actions You should be able to implement `getRequiredCapabilities()` (like `PhabricatorUserDisableTransaction`) to get rid of the need for CAN_EDIT. Since `validateTransactions()` checks admin, it's okay to remove the required capabilities check. That will let you get rid of the "obviously wrong" change above. epriestley: You should be able to implement `getRequiredCapabilities()` (like…
				$object,
				PhabricatorApplicationTransaction $xaction) {

				// Unlike normal user edits, approvals require admin permissions, which
				// is enforced by validateTransactions().

				return null;
				}
				}