Page MenuHomePhabricator
Differential D9833

Make Maniphest project edits more transaction-oriented
ClosedPublic
Actions

Authored by epriestley on Jul 5 2014, 8:09 PM.
Tags
None
Referenced Files
F13099107: D9833.diff
Fri, Apr 26, 1:10 PM
Unknown Object (File)
Wed, Apr 24, 10:30 PM
Unknown Object (File)
Sat, Apr 20, 7:13 PM
Unknown Object (File)
Thu, Apr 18, 3:48 AM
Unknown Object (File)
Thu, Apr 11, 9:07 AM
Unknown Object (File)
Sun, Mar 31, 9:55 AM
Unknown Object (File)
Fri, Mar 29, 12:26 AM
Unknown Object (File)
Thu, Mar 28, 8:05 AM
View All Files
Subscribers
epriestley

Details

Reviewers
joshuaspence
btrahan
chad
Maniphest Tasks
T5245: Migrate Maniphest Projects to use edge infrastructure
Commits
Restricted Diffusion Commit
rP7deec8208ff9: Make Maniphest project edits more transaction-oriented
Summary

Ref T5245. Currently, task/project links rely on side effects in save(). Make them more transaction-oriented, with the goal of moving fully to edges a few diffs down the line.

Test Plan
  • Added and removed projects using "Edit Task", "Associate Projects" comment action, and Herald.
  • Verified database ended up in the expected state.

Diff Detail

Repository
rP Phabricator
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

epriestley updated this revision to Diff 23595.Jul 5 2014, 8:09 PM
epriestley retitled this revision from to Make Maniphest project edits more transaction-oriented.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added reviewers: joshuaspence, chad, btrahan.
epriestley added a task: T5245: Migrate Maniphest Projects to use edge infrastructure.
Herald added a subscriber: epriestley. · View Herald TranscriptJul 5 2014, 8:09 PM
Harbormaster completed remote builds in B1534: Diff 23595.Jul 5 2014, 8:10 PM
joshuaspence accepted this revision.Jul 5 2014, 10:25 PM
joshuaspence edited edge metadata.

I'm not very familiar with this code, but from what I can tell this diff is definitely an improvement.

This revision is now accepted and ready to land.Jul 5 2014, 10:25 PM
epriestley added a comment.Jul 5 2014, 10:31 PM

(This sequence of diffs ended up going deep into the dark corners of the edge stuff, so feel free to bail out if things get too murky -- none of this is urgent, and we can wait for @btrahan to get back from paternity leave.)

joshuaspence added a comment.Jul 5 2014, 10:38 PM

Yeah I noticed. The edge stuff isnt so bad and I think I have a reasonable understanding. I'm not confident with the old code (pre-edges) I'll take a closer look tomorrow.

joshuaspence added a comment.Jul 6 2014, 4:06 PM

At the same time, based on the calendar it appears that @btrahan is due back shortly... so if you don't trust my reviewing capabilities (no offence taken), feel free to wait a little longer. I'm taking a look through the diffs now.

joshuaspence added a comment.Jul 6 2014, 4:09 PM

This may be addressed in another diff, but is the intention to eventually remove/deprecate the ManiphestTaskProject class? It seems that this class does what will be done with edges.

joshuaspence added a comment.Jul 6 2014, 4:14 PM

Just looking over this again as a sanity check... still looks good to me. One minor inline.

src/applications/maniphest/editor/ManiphestTransactionEditor.php
419–421

It might be cleaner to just move this into the above if statement now.

epriestley added a comment.Jul 6 2014, 4:29 PM

Yeah, the class is removed in D9852 and there will be a diff at the end of this to drop the table.

joshuaspence added a comment.Jul 6 2014, 4:30 PM

Gotcha.

epriestley updated this revision to Diff 23624.Jul 7 2014, 12:13 AM
epriestley edited edge metadata.
  • Clean up the $save_again logic.
Harbormaster completed remote builds in B1556: Diff 23624.Jul 7 2014, 12:14 AM
joshuaspence added a comment.Jul 17 2014, 12:18 PM

Just curious, is this stuff going to land anytime soon?

epriestley added a comment.Jul 17 2014, 2:13 PM

Waiting on D9839.

epriestley closed this revision.Jul 17 2014, 10:40 PM
epriestley updated this revision to Diff 23924.

Closed by commit rP7deec8208ff9 (authored by @epriestley).

Revision Contents
Changeset List

PathSize
src/
applications/
maniphest/
editor/
30 lines
storage/
9 lines

Diff 23924

View Options

src/applications/maniphest/editor/ManiphestTransactionEditor.php

Loading...
View Options

src/applications/maniphest/storage/ManiphestTask.php

Loading...
Phabricator · Built by Phacility