Page MenuHomePhabricator

Write separate comments for every action in Audit
ClosedPublic

Authored by epriestley on Jul 25 2014, 2:00 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Sep 6, 12:26 PM
Unknown Object (File)
Thu, Sep 5, 9:42 AM
Unknown Object (File)
Thu, Sep 5, 9:12 AM
Unknown Object (File)
Wed, Sep 4, 6:49 AM
Unknown Object (File)
Sun, Aug 25, 9:02 AM
Unknown Object (File)
Thu, Aug 22, 11:24 PM
Unknown Object (File)
Thu, Aug 22, 11:24 PM
Unknown Object (File)
Thu, Aug 22, 11:24 PM
Subscribers

Details

Summary

Ref T4896. Depends on D10023. Prepares the code for the final migration.

The transaction table stores one row per distinct effect (e.g., add CCs) rather than one row per user action (e.g., "add CCs + comment"). We can double-read that table as long as the code doesn't expect transactions/comments to have multiple different effects, and doesn't try to write any such rows.

Everywhere that we were writing a big "X + Y" comment, write two separate "X" and "Y" comments instead. Like D10023, this disrupts the UI a little (you get more boxes), but that will be resolved once the rendering code swaps over. Otherwise, this retains the existing behavior.

Test Plan
  • Used diffusion.createcomment to add comments, raise concern, and accept.
  • Previewed commenting, adding auditors/ccs, accepting, raising concern.
  • Actually performed commenting, adding auditors/ccs, accepting, raising concern.
  • Added a user with mentions.
  • Added an explicit CC and a mention user.

Diff Detail

Repository
rP Phabricator
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

epriestley retitled this revision from to Write separate comments for every action in Audit.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added reviewers: joshuaspence, btrahan.
joshuaspence edited edge metadata.
joshuaspence added inline comments.
src/applications/audit/controller/PhabricatorAuditAddCommentController.php
37

Maybe change this to PhabricatorAuditActionConstants::ADD_CCS while you're at it.

src/applications/audit/controller/PhabricatorAuditPreviewController.php
30

Because this is handled at line 56 right?

src/applications/audit/editor/PhabricatorAuditCommentEditor.php
41–45

You could write this more succinctly as $content_blocks = array_keys(mpull($comments, 'getContent'))

233–234

I'm not quite sure what these two lines are for

This revision is now accepted and ready to land.Jul 27 2014, 10:15 PM
src/applications/audit/controller/PhabricatorAuditAddCommentController.php
37

Oh, haha.

src/applications/audit/controller/PhabricatorAuditPreviewController.php
30

Yep, exactly.

src/applications/audit/editor/PhabricatorAuditCommentEditor.php
233–234

They remove previously-existing CCs/auditors from the list, so we don't try to write redundant rows.

This should all get reworked soon, I'm mostly just trying to disturb it as little as possible.

src/applications/audit/editor/PhabricatorAuditCommentEditor.php
233–234

Gotcha

src/applications/audit/editor/PhabricatorAuditCommentEditor.php
233–234

There's also a bad bug here: no unique key on the table, which causes T1768. I think we can fix that a bit further down this line.

epriestley edited edge metadata.
  • Use constant.
epriestley updated this revision to Diff 24203.

Closed by commit rP608e1d20b48a (authored by @epriestley).