Page MenuHomePhabricator

Move Audit to ApplicationTransactions
Closed, ResolvedPublic

Description

This is the last major application which needs to switch to ApplicationTransaction infrastructure.

Revisions and Commits

rP Phabricator
D10705
D10254
D10152
D10138
D10137
D10128
D10127
D10126
D10125
D10124
D10123
D10114
D10112
D10110
D10109
D10103
D10057
D10056
D10055
D10052
D10023
D10022
D10020
D10019
D10018
D10017
D10016

Related Objects

StatusAssignedTask
Resolvedepriestley
Resolvedepriestley
Resolvedchad
Resolvedepriestley
Resolvedepriestley
Resolvedepriestley
Resolvedepriestley
Resolvedepriestley
Resolved20after4
Resolvedepriestley
Resolvedchad
OpenNone
Duplicateepriestley
Wontfixepriestley
Resolvedepriestley
Resolvedepriestley
Resolvedepriestley
Resolvedbtrahan
Resolvedepriestley
ResolvedNone
DuplicateNone
Resolvedbtrahan
Resolvedbtrahan

Event Timeline

epriestley raised the priority of this task from to Normal.
epriestley updated the task description. (Show Details)
epriestley added projects: Transactions, Audit.
epriestley added a subscriber: epriestley.
epriestley edited this Maniphest Task.Apr 24 2014, 12:37 PM
epriestley edited this Maniphest Task.May 5 2014, 8:35 PM
timor added a subscriber: timor.May 15 2014, 3:15 PM
epriestley edited this Maniphest Task.Jun 24 2014, 4:39 PM
epriestley edited this Maniphest Task.Jul 7 2014, 5:05 PM
joshma added a subscriber: joshma.Jul 10 2014, 7:21 PM

Some discussion on possible approach here:

https://secure.phabricator.com/T4589#14

epriestley edited this Maniphest Task.Jul 13 2014, 4:13 PM
epriestley added a revision: Restricted Differential Revision.

I'm going to land D10016 through D10022 (which don't change the UI, unless I broke something) but hold D10023 (which does, by splitting inlines into separate transactions) until the followups which get the UI back in shape are ready. If I messed anything up that should let us identify it before the big migration lands, too.

The major migrations are in HEAD now, yell if you notice anything off. There are a couple of UI strings that are a bit funky, but that will get cleaned up in future diffs. The expectation is that no data should be missing, and we're in good shape as long as that's the case.

(I checked a handful of audits on this install and they all look correct.)

chad added a subscriber: chad.Jul 30 2014, 2:24 AM
epriestley reassigned this task from epriestley to btrahan.Sep 14 2014, 5:19 PM

Essentially all that remains here is fixing PhabricatorRepositoryCommitHeraldWorker. The problems are:

  • This adds auditors and CCs manually. It should add them with transactions.
  • This builds and sends an email manually. It should send it via PhabricatorAuditEditor, like all of the rest of the emails in the thread.
  • It invokes Herald directly, but should invoke it via transactions.
  • Generally, there's a lot of code duplication with stuff now handled natively by transactions.

Overall flow should look more like:

  • At top level, apply a transaction like "<author> pushed this commit."
  • This should invoke a Herald engine, like Maniphest and Differential do (methods shouldApplyHeraldRules(), buildHeraldAdapter(), etc.)
  • On the editor, didApplyHeraldRules() should handle transaction construction (see Maniphest/Differential).
  • Returning the transactions will make the original editor apply them properly.
  • There's some special stuff going on with the email too, which should also be on the Editor.
  • You might have to fiddle with getIsNewObject() to get some of this to behave correctly, since the Commit will already have a PHID by the time we act on it.

You could also try doing Pholio first if you want to run through a less complicated Herald-In-Transactions implementation (T4484) before this one. That one should be by the book, I think.

One possible issue is that HeraldCommitAdapter may take a long time to build some of the fields, and the parent TransactionEditor may be holding a (database) transaction open during this time.

I think we can ignore this for the first pass since I'm not sure it will cause problems. If it does, the best fix I can see would be:

  • Create the adapter ahead of time.
  • Preload all the data it needs based on the rules it is going to run. The slow thing is the raw diff.
  • Pass it to the Editor, which starts the (database) transactions.
  • The editor returns the pre-built adapter (which can execute quickly) instead of creating a fresh one.

There may be a similar issue with loading the raw patch text to generate an inline diff, where preloading it before applying transactions may be prudent. This one shouldn't hold database transactions open, but could leave a big gap between a commit processing in the UI and the email getting sent.

Support Impact Technical debt and inconsistency.