Page MenuHomePhabricator

Differential - make DifferentialDiffEditor into a real transaction editor.
ClosedPublic

Authored by btrahan on Nov 18 2014, 8:19 PM.
Tags
None
Referenced Files
F14033892: D10869.diff
Sat, Nov 9, 8:36 PM
F14019001: D10869.diff
Tue, Nov 5, 8:53 PM
F13994643: D10869.id26105.diff
Wed, Oct 23, 7:30 AM
F13961926: D10869.id.diff
Tue, Oct 15, 7:04 AM
Unknown Object (File)
Fri, Oct 11, 7:47 AM
Unknown Object (File)
Sep 15 2024, 5:05 PM
Unknown Object (File)
Sep 12 2024, 9:08 AM
Unknown Object (File)
Sep 2 2024, 10:24 AM
Subscribers

Details

Summary

Ref T6237. This sets us up for some future work like T6152, T6200 and generally cleaning up this workflow a bit. Tried to do as little as possible so not exposing transaction view yet. (Though that timeline is going to be a little funky in the common case of just the lone create transaction.)

Test Plan

made a diff from web ui and it worked. made a herald rule to block certain diffs then tried to make such a diff and saw UI letting me know i was blocked

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

btrahan retitled this revision from to Differential - make DifferentialDiffEditor into a real transaction editor..
btrahan updated this object.
btrahan edited the test plan for this revision. (Show Details)
btrahan added a reviewer: epriestley.

le herald rule

Screen_Shot_2014-11-18_at_12.02.40_PM.png (1×2 px, 489 KB)

le blocked ui

Screen_Shot_2014-11-18_at_12.16.23_PM.png (1×2 px, 468 KB)

epriestley edited edge metadata.

This looks like a good step forward to me. I think we should introduce DifferentialDiffTransaction (see third inline); the other stuff is fluff/future.

src/applications/differential/editor/DifferentialDiffEditor.php
79–93

This might be too eager on the web workflow in the future. Specifically, if we add an explicit "Repository:" field, and the user declines to fill it out, we probably should not guess for them (even though we guess via the API). I don't think this impacts anything for now.

202–203

Does this end up running the rules twice? Are you just future-proofing? I'd expect them to run once in validateTransaction(), and then again here as part of the normal Herald workflow.

Running them here only is probably slightly cleaner, but it may be hard to raise a good error if we do that: I don't think we have any other transaction rules which block the transaction. If it's a mess, maybe run them only in validation?

src/applications/differential/storage/DifferentialTransaction.php
27–29 ↗(On Diff #26103)

We should really be using a new DifferentialDiffTransaction, not reusing DifferentialTransaction (which is really DifferentialRevisionTransaction). This class has stuff (like this constant) which isn't correct for diffs.

This revision now requires changes to proceed.Nov 18 2014, 8:37 PM
src/applications/differential/editor/DifferentialDiffEditor.php
202–203

The rules do end getting run twice. This function is used to build the herald adapter, which gets used in validateTansaction and some lower level ApplicationTransactionEditor codepath way.

validateTransaction itself does some tricky business since that happens so much earlier as compared to the lower level ApplicationTransactionEditor stuff. that's what the updateDiffFromDict helper function is about more or less.

btrahan edited edge metadata.

Introduce and use differentialdifftransaction.

In a future diff, I can get these rules to run once. Per the previous code and comments here, its important to not save a Herald transaction log for these blocking rules - they could save some data we wanted to block - and the normal herald code path runs *after* save, so this has to live in validateTransaction. validateTransaction will thus stop doing the clone thing and use the actual $object, and I'll rip apart the lower level codepath to optionally skip some work if a herald engine is around. Not sure if that makes sense as written but you can pick it apart in the diff.

epriestley edited edge metadata.
epriestley added inline comments.
resources/sql/autopatches/20141118.diffxaction.sql
2

There's a marginally more modern version of this table available here:

https://secure.phabricator.com/diffusion/SAAS/browse/master/resources/sql/20141114.inst.2.xaction.sql

This version is basically fine, but storage adjust will need to fix a bunch of stuff afterward. Using the more modern version will create the table directly with the correct column types, etc.

src/applications/differential/editor/DifferentialDiffEditor.php
195

Maybe we just make this return false; to stop the second run, and keep everything else the same as it is now? The unusual nature of how this rule works makes special casing it seem pretty reasonable to me, but there's no reason we need to do the second run (at least, today) -- the second run never has any meaningful effect.

This revision is now accepted and ready to land.Nov 18 2014, 10:52 PM
btrahan edited edge metadata.
  • use the more modern table definition - THANKS!
  • return false and add a comment to see validateTransaction method
This revision was automatically updated to reflect the committed changes.