Add storage for new audit transactions and comments
ClosedPublic
Actions

Authored by epriestley on Jul 22 2014, 5:58 PM.

Details

Reviewers

joshuaspence
btrahan

Maniphest Tasks

T4896: Move Audit to ApplicationTransactions

Commits

Restricted Diffusion Commit
rP416f3d9ede5a: Add storage for new audit transactions and comments

Summary

Ref T4896. This adds the new storage, without any code changes.

This storage is substantially identical to the Differential storage, except that changesetID has been replaced by pathID.

I've retained the properties intended to be used to implement T1460. They might not be quite right, but at least we'll be able to make any fixes consistently to both applications. For now, these fields are empty and ignored.

Test Plan

Ran ./bin/storage upgrade. Nothing calls this code yet.

Diff Detail

Repository

rP Phabricator

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

epriestley updated this revision to Diff 24075.Jul 22 2014, 5:58 PM

epriestley retitled this revision from to Add storage for new audit transactions and comments.

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 22 2014, 5:58 PM

Harbormaster completed remote builds in B1838: Diff 24075.Jul 22 2014, 5:59 PM

Oh, let me add the boilerplate Query classes in this one too.

Add query boilerplate too.

Harbormaster completed remote builds in B1839: Diff 24076.Jul 22 2014, 7:15 PM

joshuaspence added a parent revision: D10016: Hide direct accesses to Audit inline comment table behind API.Jul 22 2014, 10:36 PM

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

joshuaspence accepted this revision.Jul 22 2014, 10:47 PM

joshuaspence edited edge metadata.

joshuaspence added inline comments.

src/applications/audit/storage/PhabricatorAuditTransaction.php
12	Yeah, I'll land this sometime today
src/applications/audit/storage/PhabricatorAuditTransactionComment.php
9	Should this be `false`? I'm guessing this is something to do with MySQL represent `BOOL` as `TINYINT(1)`?
10–11	Are these sane default values?

This revision is now accepted and ready to land.Jul 22 2014, 10:47 PM

Rebase to catch new PHIDType class name.
The 0 (vs false) stuff is for MySQL int types, yeah.
The 0 defaults for line number/length could be null instead and maybe be slightly more correct, but we'd have to swap the Differential table too for consistency. I don't think this is worth fixing. The notable case here is that this table stores both inline comments (which have a meaningful line number) and normal comments (which do not). In practice it would be fairly clear from the UI if we ended up with a bug in this stuff somehow.