Summary: 'cuz who cares unless you need review?
Test Plan: noted the UI showed up appropriately to my new business logix
Reviewers: epriestley, chad
Reviewed By: chad
CC: aran, Korvin
Maniphest Tasks: T2010
Differential Revision: https://secure.phabricator.com/D3958
Summary: 'cuz we need it in arcanist for T479 to commit as author
Test Plan: verified the return value was correct in conduit
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T479
Differential Revision: https://secure.phabricator.com/D3917
Summary: ...and do so for a few fields -- summary, test plan, and revert plan.
Test Plan: added NATASHA and BULLWINKLE to summary and test plan of existing diff. Diff showed up in search!
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T654
Differential Revision: https://secure.phabricator.com/D3915
Summary:
This commit doesn't change license of any file. It just makes the license implicit (inherited from LICENSE file in the root directory).
We are removing the headers for these reasons:
- It wastes space in editors, less code is visible in editor upon opening a file.
- It brings noise to diff of the first change of any file every year.
- It confuses Git file copy detection when creating small files.
- We don't have an explicit license header in other files (JS, CSS, images, documentation).
- Using license header in every file is not obligatory: http://www.apache.org/dev/apply-license.html#new.
This change is approved by Alma Chao (Lead Open Source and IP Counsel at Facebook).
Test Plan: Verified that the license survived only in LICENSE file and that it didn't modify externals.
Reviewers: epriestley, davidrecordon
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2035
Differential Revision: https://secure.phabricator.com/D3886
Summary:
a few things
- make the parent mailhandler class not send "blank body" error if you have attachments
- make both differential and maniphest append a list of attachments to the body if any exist
- BONUS - made the cc stuff work in Maniphest
Test Plan: I haven't actually tested this yet. :( i need to figure out how to send a mail with an attachment from the command-line and figured I'd serve this up first.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2012
Differential Revision: https://secure.phabricator.com/D3868
Summary:
all sorts of stuff
- made comment form width flexible
- made margins element specific rather than part of differential-primary-pane
- made box elements all veritically align left and right until code stuff
- re-factored width calculaton stuff a bunch so only the code section has to suffer from max-width calculations; everything else can flex
- made colspan 3 for rightmost table header element. this is so the "View Options" UI element ends up lining up correctly with the "Show All Lines" element just below
Test Plan: looked at revision view and changeset view and it all looked hot. note I did not test what things looked like with different word wrap values; that should still work given the re-factoring and not re-design here. also toggled haunted panel mode and it looked good.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2006
Differential Revision: https://secure.phabricator.com/D3866
Summary: this makes notifications work better for folks who choose to handle things in Phabricator and not over email
Test Plan: had my test account and "real" account battle each other on a few tasks and divs. Noted that I received emails appropos to the respective settings.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1977
Differential Revision: https://secure.phabricator.com/D3856
Summary: we do this by passing the "seenOnBranches" commit data detail through the stack
Test Plan: browse in diffusion link worked for non-master checkins under git
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1949
Differential Revision: https://secure.phabricator.com/D3853
Test Plan: Looked at diff with several different lint errors, saw correct messages in their inline comments.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3827
Summary: This check is currently wrong -- the actor is only //coincidentally// the owner (and only most of the time). It also raises at parse time, preventing any user from parsing a message with their own name in the "Reviewers" field. Instead, check against the right owner PHID and raise it only if a revision is available. See https://github.com/facebook/arcanist/issues/54 and next diff.
Test Plan: Tried to add myself as a reviewer to revisions I own via web and Conduit, got rejected. Parsed a message with myself in the "Reviewers:" field, it worked correctly.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3820
Summary:
See T1963 for discussion of the Facebook-specific hack.
Differential currently uses a one-stage cache (render -> postprocess -> save in cache) rather than the two-stage cache (render -> save in cache -> postprocess) offered by `PhabricatorMarkupInteface`. This breaks Differential comments coming out of cache for the lightbox, and makes various other things suboptimal (status of handles like @mentions and embeds are not displayed accurately).
Instead, use the modern stuff.
Test Plan:
- Created preview comments and inlines in Differential.
- Edited a Differential inline.
- Submitted main and inline Differential comments.
- Viewed and edited Differential summary and test plan.
- Created preview comments and inlines in Diffusion.
- Submitted comments and inlines in Diffusion.
- Verified Differential now loads and saves to the generalized markup cache (Diffusion is close, but main comments still hold a single-stage cache).
- Verified old Differential comments work correctly with the lightbox.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1963
Differential Revision: https://secure.phabricator.com/D3804
Summary: See D3789. Same thing for Differential.
Test Plan: Created a new revision and made a comment. Verified reviewer got popup notifications but the in-app notifications were delivered already marked as read.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1403
Differential Revision: https://secure.phabricator.com/D3790
Summary: This should all go away at some point when we move to fluid layout, but don't be more annoying than necessary in the meantime.
Test Plan: Meta.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3788
Summary:
Same as D3759 with a fix:
Previously, we would insert `null` to indicate a line that doesn't exist on one side of a diff, and then implode on "\n", so we'd get "\n" as a result.
In D3759, we'd insert `null` but implode on empty string, and get nothing as a result.
When a placeholder null is present, explicitly insert a newline.
Test Plan: D3759 plus examined a diff with removed lines on the left side prior to new lines on the right side.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3765
This reverts commit f6cb51562e.
This has some bugs in normal diffs that I haven't immediately been able to figure out. I'll reapply it once I sort them out.
Auditors: vrana
Summary:
- We currently treat "\r" as a newline, but should not because VCSes do not.
- We get an extra empty line at the end of diffs created after D3442 because we now retain newlines.
- Historically we've converted tab pre-cache, but do it post-cache instead so we can add prefs about it, as we should handle it better than we do (e.g., let the user set it to a different width, infer width from comments in the file, expand it to actual tab stops, or show it visually in some way).
Test Plan:
- Verified diffs no longer have an empty line at the end.
- Created a diff of a "\r" file and verified it displayed somewhat reasonably. All browsers treat "\r" as a real newline so it's not necessarily perfect, but we can clean that up later. Hopefully these files are exceedingly rare.
- Created a file with tabs and verified it came out reasonably.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1857
Differential Revision: https://secure.phabricator.com/D3759
Summary: If you `git show` and copy/paste it into Differential, we die trying to save the DifferentialChangeset corresponding to the commit message (error: column "filename" can not be null). Instead, drop the message change for raw diffs.
Test Plan: Copy/pasted `git show` into Differential.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3740
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
