Migrate audit comments to transactions
ClosedPublic
Actions

Authored by epriestley on Jul 25 2014, 7:34 PM.

Details

Reviewers

joshuaspence
btrahan

Maniphest Tasks

T4896: Move Audit to ApplicationTransactions

Commits

Restricted Diffusion Commit
rPf965126dc454: Migrate audit comments to transactions

Summary

Ref T4896. Depends on D10052. This is the major/scary migration, but not really so bad. It is substantially similar to D8210, but less complex because there are fewer actions here.

This moves PhabricatorAuditComment storage to PhabricatorAuditTransaction, then reads PhabricatorAuditComments as a proxy around the new objects.

Test Plan

Before migrating, browsed around. Nothing appeared broken.
Migrated cleanly.
Viewed old transactions (inlines, comments, accept/reject/etc, add auditors, add ccs, implicit CCs).
Added all of those comment types.
Edited a draft.
Deleted a draft.
Spot checked the database for sanity.

Diff Detail

Repository

rP Phabricator

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

epriestley updated this revision to Diff 24178.Jul 25 2014, 7:34 PM

epriestley retitled this revision from to Migrate audit comments to transactions.

epriestley updated this object.

epriestley edited the test plan for this revision. (Show Details)

epriestley added reviewers: joshuaspence, btrahan.

epriestley added a task: T4896: Move Audit to ApplicationTransactions.

Herald added a subscriber: epriestley. · View Herald TranscriptJul 25 2014, 7:34 PM

epriestley updated this object.Jul 25 2014, 7:34 PM

epriestley added a parent revision: D10052: Write separate comments for every action in Audit.

Harbormaster completed remote builds in B1900: Diff 24178.Jul 25 2014, 7:35 PM

joshuaspence awarded a token.Jul 27 2014, 10:15 PM

Close enough

resources/sql/autopatches/20140725.audit.1.migxactions.php
33	Maybe use `phutil_json_decode`? (Although we wouldn't expect the field to contain invalid JSON)
43	I was thinking you should replace `'added-auditors'` with `PhabricatorAuditComment::METADATA_ADDED_AUDITORS`... although I guess you can't do this because `PhabricatorAuditComment` is going away
44–48	Should we do this if `$phids` is an empty array?
55	Is `array_fuse` necessary?
74	I'd probably prefer `$comment` as the iterator variable, but meh.
128	Is the cast necessary?
133	As above
src/applications/audit/storage/PhabricatorAuditComment.php
180	I'm not sure why you `array_fuse` the data on the way in and then `array_keys` on the way out?
184	As above.

This revision is now accepted and ready to land.Jul 28 2014, 12:05 PM

joshuaspence added inline comments.Jul 28 2014, 12:21 PM

src/applications/audit/constants/PhabricatorAuditActionConstants.php
5–14	The (mis)alignment here is distracting
src/applications/audit/storage/PhabricatorAuditComment.php
65	Do you need to do `$this->proxyComment->setAuthorPHID($actor_phid)` as well?

epriestley added inline comments.Jul 28 2014, 1:13 PM

resources/sql/autopatches/20140725.audit.1.migxactions.php
43	Yeah, this is the reasoning -- the constant is getting wiped out soon.
44–48	For this and the `array_fuse()` below, I have to do followups for Auditors (possibly moving it to edges) and CCs (definitely moving it to subscriptions), so these are kind of temporary states for the data until that can happen. In cases like this where it's slightly simpler not to make an adjustment for now, like this, I've tried to keep this migration as simple as possible (e.g., the rest of this migration is maybe easier to reason about if `$xaction` always exists, and debugging migration issues might also be easier). The eventual followup can make adjustments like stripping empty rows. In the `array_fuse()` case below, I think fusing made something a little easier for now. This will definitely move to subscriptions so the data format should be temporary.
128–133	Ah, no, these aren't necessary. The only risky value here is `null`. These casts came from the other migration, where author/actor was nullable for bizarre legacy reasons.
src/applications/audit/storage/PhabricatorAuditComment.php
65	The save() workflow and CommentEditor do this for us, although it's probably slightly more correct/predictable to do it here too.
200	Specifically, this forces the comment's author PHID to the correct value.