Page MenuHomePhabricator

Move Audit to ApplicationTransactions
Closed, ResolvedPublic

Description

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

Details

Differential Revisions
Restricted Differential Revision
Commits
D10705 / rP3dd92a78ac2a: Move audit to application transactions
D10254 / rPa5d24609748c: Probably fix bad method call in Diffusion
D10152 / rPe68b6deccbad: Remove PHID_TYPE_ACMT
D10138 / rPe8d272b0dad1: Use standard infrastructure to attach commits to other objects
D10137 / rP725e2fa41055: Write a "resign" audit relationship even if actor has no relationship
D10128 / rPb5750412c75b: Apply normal Audit actions directly with Transaction editor
D10127 / rP25acf5d1304c: Apply Diffusion reply email directly with transaction editor
D10126 / rP508260e4fef1: Apply `diffusion.createcomment` directly with transaction editor in Audit
D10125 / rPa297450aa97b: Apply "accept" and "resign" actions with transactions
D10124 / rP78e164aea6e0: Use transactions to apply "resign" and "close" Audit actions
D10123 / rP688f245a95be: Use transactions to apply "add auditors" action in Audit
D10114 / rP49bd5721c5a7: Use standard infrastructure for Feed in Audit
D10112 / rP64736264a62d: Use standard infrastructure for Audit email generation
D10110 / rPb787d3ef0df7: Use standard infrastructure for Audit search indexing
D10109 / rP5b969fb5b809: Provide a transaction editor to perform Audit row writes
D10103 / rP89b942c183eb: Move Audit to proper Subscriptions
D10057 / rPa6698f2ea53c: Use ApplicationTransactions when indexing commit/audit comments
D10056 / rP2082eda67bfe: Convert Audit comment rendering to standard infrastructure
D10055 / rPf965126dc454: Migrate audit comments to transactions
D10052 / rP608e1d20b48a: Write separate comments for every action in Audit
D10023 / rPbf397480114d: Build separate comments for each inline comment in Audit
D10022 / rP970058927987: Allow audit email to generate from multiple transactions
D10020 / rP3d78c0eff707: Migrate Audit comment text into new storage
D10019 / rPdc5c87f74cda: Hide Audit comment table reads behind an API
D10018 / rPc01aa794c1f2: Migrate Audit inline comments to new storage
D10017 / rP416f3d9ede5a: Add storage for new audit transactions and comments
D10016 / rP8605a1808d01: Hide direct accesses to Audit inline comment table behind API

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.