Summary: This allows users to add a revision's author as reviewer according Differential configuration using the 'Leap Into Action' form.
Test Plan: Tested on local install.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1885
Differential Revision: https://secure.phabricator.com/D3682
Summary: Checks if the revision author is in reviewers only if differential.allow-self-accept is false.
Test Plan:
Tested locally pushing revisions with "arc diff" to a Phabricator
server with differential.allow-self-accept at true or false with
myself or not as reviewer.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1879
Differential Revision: https://secure.phabricator.com/D3673
Summary: `actorPHID` no longer gets set or exists.
Test Plan: Updated a revision without fataling.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3684
Summary: We need to be able to query for all properties matching a given pattern, as mentioned in D3676
Test Plan: Loaded Phabricator, ensured diff loaded, ensured field using all properties was able to enumerate them.
Reviewers: vrana, epriestley
Reviewed By: vrana
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3679
Summary:
We need to load all properties with some prefix in one field.
We can't merge them in one property because there will be a race condition for update (we don't have API for load+update+save).
Instead of providing API for this and complicating the code even more, just load everything unconditionally.
It shouldn't waste much bandwith or memory because we use most of the properties anyway.
It also looked overengineered to me.
Test Plan: Displayed revision with fields using diff properties.
Reviewers: epriestley, royw
Reviewed By: royw
CC: aran, royw, Korvin
Differential Revision: https://secure.phabricator.com/D3676
Summary: When we receive an email, figure out if any of the other tos and ccs are users. If they are, pass their phids through the stach as "exclude phids" and exclude them from getting the email.
Test Plan: used the various applications (audit, differential, maniphest) and noted emails were sent as expected.
Reviewers: epriestley, vrana
Reviewed By: vrana
CC: aran, Korvin, vrana
Maniphest Tasks: T1676
Differential Revision: https://secure.phabricator.com/D3645
Summary:
sometimes we show moved files as unchanged, sometimes deleted,
and sometimes inconsistent with what actually happened at the
destination of the move. Rather than make a false claim,
omit the 'not changed' notice if this is the source of a move.
Background: one of our users was confused by the source of a move
claiming to be unmodified when the destination of the move had
extensive modifications.
There may be a question about the quality of the data we keep/compute
about the changes, so maybe we could do a better or more consistent
job there (may just be nuances of the SCM in use); this approach
just smoothes that out and doesn't require fixing up the status
in pre-existing diffs.
Test Plan:
In the facebook phabricator, D594195#739ace1a and D590020#b97cfcfd
For others, find a diff that has moved files.
For the source of a move, we should now only show the "X moved to Y"
header and not the "unchanged" shield.
Reviewers: nh, vrana, epriestley
Reviewed By: vrana
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3669
Summary:
This is killing us since D3615.
Maybe we should also change `differential_inlinecomment` index <commentID> to <commentID, authorPHID>.
Test Plan: Checked queries at **/differential/** with comment drafts.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3667
Summary: Make these always work. Notably, this makes them work in Maniphest. Previously this was at odds with stuff fixed in D3651.
Test Plan: Dragged and dropped files into Remarkup in Maniphest.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3652
Summary:
D03646 works, I don't want it to work.
Theoretically, it can cause us some troubles if we use this string in JS number context where 030 is 24.
Test Plan: D03646, D3646
Reviewers: epriestley, edward
Reviewed By: edward
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3646
Summary: Show First 20 Lines doesn't display gap context and emits error.
Test Plan: Showed first 20 lines, last 20 lines, middle 20 lines - saw context and no error.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3616
Summary: Previously, only inline comment drafts were included.
Test Plan:
**/differential/** - verified that there are drafts where they weren't before.
Checked executed queries.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3615
Summary: DifferentialAffectedPath has no id or phid key so delete() won't work and we have to do things this other way.
Test Plan: deleted a few diffs on my test reproduction. aside from warnings about missing keys (epriestley is on it as I write this) no errors found and diff observed as deleted
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1846
Differential Revision: https://secure.phabricator.com/D3610
Summary: So they're maybe a little easier to deal with? I'm going to take this formally to "plz @chad plz help" land.
Test Plan: {F20329}
Reviewers: btrahan, vrana, chad
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3609
Summary: We have lots of empty drafts in DB.
Test Plan: Wrote revision comment, deleted it, checked db.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3591
Test Plan: Created diff, opened the file from Differential, opened the file in Diffusion.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3538
Summary:
It happens to me quite often that I leave the window with revision (by closing it or by visiting a link from it).
When I return then the comment draft is there so I clowncopterize it but forget that I wanted to take some other action than Comment.
Test Plan: Selected "Add Reviewers", added some reviewers, closed the window, opened it - the action and reviewers were still there.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3530
Summary: ...also makes Maniphest Task Edit Controller use this when its not appropriate to upsell email.
Test Plan: played around with each tool and verified the Remarkup reference was present
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1756
Differential Revision: https://secure.phabricator.com/D3468
Summary: This takes the place of D2721 which I am going to abandon.
Test Plan:
* Make a commit with "Closes T###" in the summary field. See that the mentioned task gets closed.
* Make a commit with "refs T###" in the summary. See that it gets added as a related commit via edges.
Reviewers: 20after4
Reviewed By: epriestley
CC: aran, Korvin, champo
Maniphest Tasks: T945
Differential Revision: https://secure.phabricator.com/D3466
Summary:
This is the last Paste UI element that doesn't work properly on tablets/phones. Make it flexible.
Also add empty states to Paste.
Test Plan: Viewed various errors, and `/uiexample/errors/`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3429
Summary:
This is mostly ripped from D2721, but doesn't implement the T945 part.
After we parse a commit message, give DifferentialFieldSpecifications an opportunity to react to the message as well (e.g., by updating related objects).
Test Plan: Impelmented a var_dump() body, ran `reparse.php` on a commit.
Reviewers: vrana, 20after4, btrahan, edward
Reviewed By: 20after4
CC: aran
Maniphest Tasks: T945, T1544
Differential Revision: https://secure.phabricator.com/D3444
Summary:
We have lots of info about unit tests.
This allows linking them from Unit field.
Test Plan: Monkey patched `$test['link']`, clicked on it.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3434
Summary:
I've replaced all `id(new PhabricatorObjectHandleData(...))->loadHandles()` by `$this->loadViewerHandles(...)`.
Lint caught one usage in a static method.
Test Plan: Displayed revision with sporadic author.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3432
Summary: It would be best to add this everywhere at once but I'm too lazy for it.
Test Plan: Displayed package list and detail and revision comment with away users.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3419
Summary:
Some fields require displayed diff (e.g. Lines), some require last manual diff (Lint, Unit).
Lint requires both - it needs to find if the line with the error is displayed or not.
Test Plan: Displayed committed diff with lint errors, clicked on them, got last manual diff at the correct line.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3331
Summary:
Ball is more obvious and visible than I thought.
Delete the status word and display until date in title.
Also display the until date in revision list.
Also display near future dates with DoW instead of year.
Test Plan: Displayed revision and revision list with away reviewers.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3407
Summary:
We have some complaints on this feature:
- It's not clear that the displayed code is context from gap.
- It would be better if the context would be displayed with its real indentation.
- It's not clear how far the context is from the displayed code.
- Links revealing gap aren't on consistent place.
This solves all these problems and introduces new one:
It now seems that the reveal links works only with the left side.
Anyway, I think that this is better overall.
I don't want to put the context on a separate line to not waste space.
Test Plan: Displayed various contexts, revealed context.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, bh, jwatzman
Differential Revision: https://secure.phabricator.com/D3404
Summary:
I have a chain of revisions sometimes.
I want to see also revisions depending on me.
Test Plan: Displayed revision in the middle of chain.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3399
Summary: this is useful for certain workflows, typically where the reviewer is a gatekeeper of sorts who does the acutal commit. Special thanks to D2900 which made this relatively brain-dead to code up.
Test Plan: set to "true" in my local development environment and verified test user "xerxes" could close my stuff
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1732
Differential Revision: https://secure.phabricator.com/D3398
Summary: said differently, if the user included another to address or one or more cc's, don't send the error message email.
Test Plan: played around in the metamta test console and verified that blank replies generated the error handler.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1643
Differential Revision: https://secure.phabricator.com/D3345
Summary:
We have `max_allowed_packet` 1 GiB but our replication dies if the query is longer than unknown value (it dies with 293 MB long query).
Anyway, there's no reason why we should not save the cache if you have small `max_allowed_packet`.
Test Plan: Lowered `$size` to 100, deleted cache from DB, displayed changeset, verified issued queries in DarkConsole, verified DB.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3390
Summary:
See T1602#15.
I don't intend to land this right away, because I'm not sure I won't
need another change or two to the rendering code. Keeping it here so I
won't forget about it.
Test Plan: iiam
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1602
Differential Revision: https://secure.phabricator.com/D3240
Summary: See comment inline. We should fix this properly but it goes faily deep so I just stopped the bleeding for now. It's OK if we end up with a silly-looking file tree view for bizarre edge cases.
Test Plan: Created a problem diff as described, verified it no longer threw.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran, Avish
Maniphest Tasks: T1702
Differential Revision: https://secure.phabricator.com/D3373
Summary:
I quite often wonder what's inside these gaps displayed in changeset. In which method I am? Is it a while loop or a foreach? Maybe I'm in a class but the project doesn't have a sctrict policy of one class per file so what's the name of the class?
I've experimented with bunch of rules:
- Always display 0 indentation: useless for one class per file.
- Always display 1 indentation: weird inside global functions.
- Display closest lower indentation: works best.
I'm not sure about highlighting the context. I like highlighting but maybe in this case subtler monochrome text will work better.
Test Plan: Browsed bunch of diffs, loved it.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3371
Summary:
Adds a flexible navigation menu to diffs that shows you your current position in the diff.
Anticipating some "this is the best thing ever" and some "this is the wosrt thing ever" on this, but let's see how much pushback we get? It seems pretty good to me.
Test Plan: Will attach screenshots.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1633, T1591
Differential Revision: https://secure.phabricator.com/D3355
Summary:
We have /differential/filter/drafts/ but nobody knows about it.
This diff displays the draft only if there is no flag to not waste space.
Test Plan: /differential/filter/revisions/
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, alanh
Differential Revision: https://secure.phabricator.com/D3324
Summary:
Rendering method shouldn't load data.
The view probably shouldn't load data either because it is a job for component (object that both loads data and displays them) but we don't have that concept in Phabricator.
This at least improves the architecture a little bit.
Test Plan: /differential/
Reviewers: epriestley
Reviewed By: epriestley
CC: alanh, aran, Korvin
Differential Revision: https://secure.phabricator.com/D3325
Summary:
This is another experiment for reducing reviewers response time.
I stole the idea (and colors) from [[ http://www.reviewboard.org/media/screenshots/2009/02/02/dashboard.png | ReviewBoard ]].
I actually quite like it (except when everything is red) and I can image that people will review just to have better color balance.
The code is not production ready for these reasons:
- We load holidays again and again for each revision. I couldn't cache it to static variable because it could persist multiple requests, right?
- I don't know how to expand height to the whole cell (I'm really bad in CSS).
- CSS rules are probably in wrong file.
- We probably want to use different colors.
This is how it looks:
{F16406}
Test Plan: Displayed revision list.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3190
Summary: See T1677. I think wanting bots to be able to post comments without sending email is a pretty reasonable use case. Eventually we should probably support this more broadly and maybe protect it with permissions (normal users maybe shouldn't be able to do this?) but we can wait for use cases.
Test Plan: Made comments with and without "silent". Verified that the non-silent comment sent email, and the silent comment did not.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1677
Differential Revision: https://secure.phabricator.com/D3341
Summary:
The logic here was swapped - new file should be on the right side.
Plus we had a fatal for VS = -1 where new file should be on left.
Test Plan:
Downloaded raw diff of:
- base VS change
- change VS change
- change VS change with unmodified file
Reviewers: epriestley, btrahan
Reviewed By: btrahan
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3333
