Page MenuHomePhabricator

Migrate audit comments to transactions
ClosedPublic

Authored by epriestley on Jul 25 2014, 7:34 PM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 23 2024, 8:44 AM
Unknown Object (File)
Feb 1 2024, 6:05 PM
Unknown Object (File)
Jan 27 2024, 12:24 AM
Unknown Object (File)
Jan 26 2024, 8:50 AM
Unknown Object (File)
Jan 26 2024, 8:15 AM
Unknown Object (File)
Jan 26 2024, 8:02 AM
Unknown Object (File)
Jan 24 2024, 10:14 PM
Unknown Object (File)
Jan 24 2024, 7:27 PM
Subscribers
Tokens
"Pterodactyl" token, awarded by joshuaspence.

Details

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 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.
joshuaspence edited edge metadata.

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
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?

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.

epriestley edited edge metadata.
  • Remove unnecessary casts.
epriestley updated this revision to Diff 24204.

Closed by commit rPf965126dc454 (authored by @epriestley).