Differential D20659 Diff 49281 src/applications/auth/xaction/PhabricatorAuthConfigTrustEmailsTransaction.php

Changeset View

Standalone View

src/applications/auth/xaction/PhabricatorAuthConfigTrustEmailsTransaction.php

This file was added.

				<?php

				final class PhabricatorAuthConfigTrustEmailsTransaction
				extends PhabricatorAuthConfigTransactionType {

				const TRANSACTIONTYPE = 'config:trustEmails';

				public function generateOldValue($object) {
				return (int)$object->getShouldTrustEmails();
				}

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

				public function getTitle() {
				if ($this->getNewValue()) {
				return pht('%s enabled email trust.',
				$this->renderAuthor());
				} else {
				return pht('%s disabled email trust.',
				$this->renderAuthor());
				}
				}

				public function validateTransactions($object, array $xactions) {
				$errors = array();
				$provider = $object->getProvider();

				foreach ($xactions as $xaction) {
				if (!$provider->shouldAllowEmailTrustConfiguration()) {
				$errors[] = $this->newInvalidError(
				epriestleyUnsubmitted Not Done Inline Actions Ideally, we would raise this error only if (a) the transaction actually attempts to change the value and (b) probably, the new value is "enabled". See D20311 for a somewhat-recent somewhat-relevant example. This is mostly for API callers, but the ideas here are: It's okay to write a client that shows the user a form, then submits all the transactions that "Save" implies. We don't require the client to know whether each field is editable or valid: it's okay for them to send back the full current state of the object and let us validate it. A specific subcase of this is when a task has an invalid value for some `<select />` field, like priority set to "zebra". It's okay to apply a transaction which changes the priority from "zebra" to "zebra", even though "zebra" is not valid, because if we don't allow this it's super tough to write a sensible form on the client that doesn't get in the user's way in the face of questionable data. `validateTransactions()` is doing (at least) two types of checks: one type of checks is structural ("is this value in a plausible format?") and one is more permissions-ey ("is the value itself a legal value which the user has permission to select?"). We could structure the code like this instead: validateTransactionStructure($xactions); $xactions = filterNoOpTransactions($xactions); validateTransactionValues($xactions); ...but I think most of the time these checks are pretty part-and-parcel and this would lead to a lot of duplicate code, even though it might technically be more clear. An adjacent issue is that you can submit transactions (for an object with title "B") like "set title to A" and also "set title to B" in the same transaction list. These merge+filter into a no-op (since we change the title, but then change it back), but in this case we do want to raise an error. Basically, there are a million weird cases here and I think we're structurally in reasonable territory where badness is at least relatively low, if not actually minimized, but one piece of badness is that you unintuitively should allow not-exactly-valid-but-no-op transactions to proceed through `validateTransactions()`. epriestley: Ideally, we would raise this error only if (a) the transaction actually attempts to change the…
				pht(
				'Authentication provider "%s" does not support trusting emails.',
				get_class($provider)),
				$xaction);
				continue;
				}
				}

				return $errors;
				}

				}