Summary: make the change, kill the function. be sure to get a good $user or
$viewer variable
Test Plan:
for each controller or view, look at it in the ui. change timezone, refresh ui
and note change. i did not test the OAuthSettingsPanelController; not sure how
to get to that badboy and i got a bit lazy
Maniphest Tasks: T222
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, btrahan, epriestley
Maniphest Tasks: T222
Differential Revision: 1166
Summary:
kill the tabs and make it a create button instead. pertinent notes:
* added a "Filter diffs" button to the form. optional, but i thought it
necessary with the new green button
* linked to Arcanist user guide on the create diff page. somewhat unrelated but
i think create diff will get more traffic now so linking to help seemed like a
reasonable add on here.
Test Plan:
viewed differential homepage
* clicked left hand filter elements. noted "Create Diff" button on filters
within user revisions and no button on filters within all revisions.
* entered another user into Select User UI and viewed their diffs via button and
pressing enter
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, btrahan, epriestley
Differential Revision: 1157
Test Plan:
Login as admin, look at an open revision you don't own, you should be able to
choose '(Admin) Abandon Revision', the option should be on the bottom and should
abandon the revision after sending the comment.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1060
Test Plan:
Login as an admin, go to a revision that you don't own - you should be able to
abandon this revision.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1048
Summary:
Add possibility for not logged in users to browse and see Differential
revisions.
Test Plan:
Set 'differential.anonymous-access' config option to true, log out, you should
be able to browse Differential without logging back in.
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, epriestley, mareksapota
Differential Revision: 1044
Summary: Girish wants to be able to do this.
Test Plan: Checked that I had the option in my sandbox on an accepted diff.
Reviewers: epriestley, jungejason
Reviewed By: jungejason
CC: aran, jungejason, tuomaspelkonen, epriestley
Differential Revision: 1020
Test Plan:
Go to /differential/diff/create and upload a diff file - result should be the
same as pasting the diff into the textarea.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1019
Summary:
This makes symbol cross-references work in Differential. You need to do a little
legwork but I'll document that once the change has baked for a little while.
Basically:
- Projects are annotated with indexed languages, and "shared library" projects
(for example, symbols in Phabricator should be searched for in Arcanist and
libphutil).
- When we render a changeset, we check if its language is an indexed one. If
it is, we invoke the decorator Javascript.
- The Javascript takes you to a lookup page, which either gives you a list of
matching symbols (if several match) or redirects you instantly to the
definition.
Test Plan: Clicked class and function symbols in a diff, got jumped into
sensible sorts of places in Diffusion.
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 980
Summary: I goofed this, $phids was already being populated and I changed the
meaning. This causes a fatal if you filter the list by a user who is not an
author or first reviewer for any of the revisions (e.g., no open revisions).
Test Plan: Looked at the list of a user with no revisions.
Reviewers: codeblock, jungejason
Reviewed By: codeblock
CC: aran, codeblock, jungejason
Differential Revision: 989
Summary: I want to throw this in Diffusion as part of T262, but it's embedded in
the controller right now. Split it out.
Test Plan: Looked at various revision list views, no changes.
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 977
Summary:
After D857, we try to attach local commit information to revisions. If this
information is available, display it on the revision.
Design on this is a little rough, I might try to combine this into the revision
update view or something like that since we're starting to take up a lot of real
estate for metadata.
Test Plan: Local diffed this and got some commit info.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, epriestley, jungejason
Differential Revision: 872
Summary:
When an object is updated, record the content source for the update. This mostly
isn't terribly useful but one concrete thing I want to do with it is let admins
audit via-email replies more easily since there are a bunch of options which let
you do hyjinx if you intentionally configure them insecurely. I think having a
little more auditability around this feature is generally good. At some point
I'm going to turn this into a link admins can click to see details.
It also allows us to see how frequently different mechanisms are used, and lets
you see if someone is at their desk or on a mobile or whatever, at least
indirectly.
The "tablet" and "mobile" sources are currently unused but I figured I'd throw
them in anyway. SMS support should definitely happen at some point.
Not 100% sure about the design for this, I might change it to plain text at some
point.
Test Plan: Updated objects and saw update sources rendered.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, epriestley, jungejason
Differential Revision: 844
Summary:
deprecate generateProperties() from class
DifferentialRevisionDetailRenderer. Custom fields now provides a much
more powerful version of generateProperties().
Depends on D814.
Test Plan:
implemented facebook task field with custom field and
verified it worked.
Reviewers: epriestley, tuomaspelkonen
Reviewed By: epriestley
CC: aran, jungejason, epriestley
Differential Revision: 826
Summary:
Move all the rest of the fields into the custom field schema, for revision
views.
I left a couple of stubs in here (willWriteRevision, didWriteRevision) since I'd
planned to do edits here too, but this diff is sort of big-ish already. I'll do
all the edit fields in the next revision.
Depends on D808.
Test Plan: Viewed, edited and conduit'ed some revisions.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason, epriestley
Differential Revision: 809
Summary:
Move additional fields (which rely on loading handles) to the extensible field
classes and out of hardcoding in the controller.
Depends on D807.
Test Plan: Viewed, edited, and hit conduit for revisions.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason, epriestley
Differential Revision: 808
Summary:
Differential has a bunch of display-only fields, implement them all as field
specifications instead of hard-coded fields.
Also add some more documentation and fix redundant string constants in blame
rev/revert plan fields.
Test Plan: Viewed, edited, and hit conduit for revisions.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason, epriestley
Differential Revision: 807
Summary: Depends on D798. Extends custom fields and makes the vaguely useful:
they can appear on the edit and view interfaces. This does not integrate them
with commit messages yet; that's more complicated but I plan to do it shortly.
Test Plan: Implemented a custom field per P123, it correctly appears on the edit
interface, persists, validates, and shows up when viewing the revision.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, epriestley, jungejason
Differential Revision: 800
Summary:
This allows you to edit dependencies. It is a better patch than it used to be.
It depends on D725.
- If you create a cycle, it just throws an exception and aborts the workflow.
It should not do this.
- Tasks which depend on the current task aren't shown in the UI. Need to add a
new table for this.
- Transaction text says "attached Task" but should probably say "added a
dependency on task".
Test Plan: Created valid and invalid dependencies between tasks. Created valid
and invalid dependencies between revisions.
Reviewed By: tuomaspelkonen
Reviewers: davidreuss, jungejason, tuomaspelkonen, aran
Commenters: codeblock
CC: aran, codeblock, tuomaspelkonen, epriestley
Differential Revision: 595
Summary:
This is really rough and needs work (particularly, there's some diff code I
really need to refactor since I sort-of-copy-pasted it) but basically
functional.
Show text changes between diffs and allow users to revert to earlier versions.
Differential's line-oriented diff style isn't ideal for large blocks of text but
I'm betting this is probably good enough in most cases. We can see how bad it is
in practice and then fix it if needbe.
I added a bunch of support for "description" but didn't add the feature in this
diff, I'll either follow up or task it out since it should be a pretty
straightforward change.
Test Plan: Looked at history for several Phriction documents, clicked "previous
change" / "next change", clicked revert buttons.
Reviewed By: hsb
Reviewers: hsb, codeblock, jungejason, tuomaspelkonen
CC: aran, hsb, epriestley
Differential Revision: 687
Summary:
Show line count, arcanist project and base revision.
This adds a little clutter but I think we're still okay and I can play around
with it later.
Test Plan: Looked at a couple of revisions. I'm actually not 100% sure about the
SVN logic but maybe I will test that before committing.
Reviewed By: tomo
Reviewers: tomo, jungejason, tuomaspelkonen, aran
CC: aran, tomo
Differential Revision: 685
Summary:
This thing services every app but it lives inside Differential right now. Pull
it out, and separate the factory interfaces per-application.
This will let us accommodate changes we need to make for Phriction to support
wiki linking.
Test Plan: Tested remarkup in differential, diffusion, maniphest, people,
slowvote.
Reviewed By: hsb
Reviewers: hsb, codeblock, jungejason, tuomaspelkonen, aran
CC: aran, hsb
Differential Revision: 646
Summary:
See T303. Enable comment panel haunting.
I hid the preview for the sticky panel, which I think is reasonable?
Test Plan:
https://secure.phabricator.com/file/view/PHID-FILE-64713fa8a7c2a22e5b93/
Reviewed By: broofa
Reviewers: broofa, jungejason, aran, tomo, tuomaspelkonen
CC: aran, broofa
Differential Revision: 615
Summary: The field hints on this interface don't behave correctly. Particularly, when you add yourself as a reviewer you aren't pointed at the issue.
Test Plan: Edited a revision and tried to save invalid changes, including self-reviewership.
Reviewers: moskov, jungejason, tuomaspelkonen, aran
CC:
Differential Revision: 565
Summary:
We currently have only an "Add reviewers" action, add "Add CCs". This can also
be accomplished less-discoverably with mentions.
Test Plan:
Added reviewers and CCs to revisions. Toggled display between reviewers and CCs.
Reviewed By: jungejason
Reviewers: tomo, mroch, jsp, jungejason, aran, tuomaspelkonen
CC: aran, jungejason
Differential Revision: 521
Summary:
Replace some more date() calls with locale-aware calls.
Also, at least on my system, the DateTimeZone / DateTime stuff didn't actually
work and always rendered in UTC. Fixed that.
Test Plan:
Viewed daemon console, differential revisions, files, and maniphest timestamps
in multiple timezones.
Reviewed By: toulouse
Reviewers: toulouse, fratrik, jungejason, aran, tuomaspelkonen
CC: aran, toulouse
Differential Revision: 530
Differential
Summary:
Make some display stuff more consistent.
Test Plan:
Looked at a task and a revision.
Reviewed By: jungejason
Reviewers: aran, jungejason, tuomaspelkonen
CC: aran, jungejason
Differential Revision: 462
Summary:
- Make wrap width settable in PHP.
- Dynamically generate max-width based on configurable maximum width.
- Constrain non-diff elements to standard width.
- Provide a configuration setting.
Test Plan:
Set various things to 100 / 120, as far as I could tell everything seemed to
render sensibly? This should have no effect on 80-col changes.
Reviewed By: jdperlow
Reviewers: jdperlow, tuomaspelkonen, jungejason, aran
CC: aran, jdperlow
Differential Revision: 413
Summary:
They currently have "Next", "Previous" and "Reply" links which don't work. Don't
render these links.
Test Plan:
Looked at inline previews, didn't see any silly/nonfunctional links.
Reviewed By: tuomaspelkonen
Reviewers: aran, jungejason, tuomaspelkonen
CC: aran, epriestley, tuomaspelkonen
Differential Revision: 419
Summary:
It was not possible before to update arc unit results for a postponed
test. This change makes it possible. Also the number of postponed tests
are shown in differential.
Let me know if this looks too Facebook specific.
Test Plan:
Tested the conduit call manually from Conduit Console and updated test
results for a diff that had 20 postponed tests.
Reviewed By: jungejason
Reviewers: epriestley, jungejason
Commenters: epriestley
CC: slawekbiel, aran, tuomaspelkonen, jungejason, epriestley
Differential Revision: 416
reviewers as metadata
Summary:
The "Add reviewer" implementation is super lazy right now since I didn't want to
do a schema change. Man up and add a column. I also plan to store "via"
information here (e.g., via email or via mobile).
NOTE: This schema change may take a while since the comment table is pretty big
in Facebook's install.
This needs a little CSS work but I think it's reasonable for now.
Test Plan:
Made comments on revisions and tasks. Added reviewers to a revision, got linked
names instead of a blob of text.
Reviewed By: aran
Reviewers: tomo, jungejason, tuomaspelkonen, aran
Commenters: tomo
CC: aran, tomo, epriestley
Differential Revision: 394
Summary:
When a user hits 'cancel' on a 'new', 'edit', or 'reply' operation, add a little
"Changes discarded. __Undo__" insert so they can get their change back. No undo
for delete since there's an explicit prompt. Once this lands we can make
'escape' work again to close dialogs.
This change started feeling really good when I was merging all the duplicate
code and making things more consistent, but by the time I started writing client
rendering it felt gross. I'm not really thrilled with it but I guess it's a step
forward? The feature seems pretty OK in practice. Let me know how much barfing
this causes and I can try to remedy the most acute concerns.
This also fixes a bug where replies always (?) appear on the 'new' side of the
diff (I think?).
Test Plan:
Applied 'new', 'edit', 'delete' and 'reply' operations, pressed 'cancel' and
'okay' in each case, with and without changing text where relevant. All
behaviors seem to conform with expectations, except that canceling out of 'edit'
without changing the text gives you an option to undo when it shouldn't really.
There's no super easy way to get at the original text right now.
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen
CC: simpkins, aran, epriestley
Differential Revision: 406
Summary:
Historically, we had a bug at some point which caused inline comments to get
associated with changeset 0. Prevent that explicitly. See T108.
Test Plan:
Set "$changeset = 0" in the endpoint and got an exception.
Reviewed By: aran
Reviewers: aran
CC: aran
Differential Revision: 374
Summary:
See task. Allows users to unsubscribe via email.
Test Plan:
Used mail receiver to unsubscribe from a revision. Tested subscribe/unsubscribe
buttons. Verified "!unsubscribe" appears as an avilable action in email.
Reviewed By: ola
Reviewers: ola, mroch, jungejason, tuomaspelkonen, aran
CC: aran, epriestley, ola
Differential Revision: 385
Summary:
You can currently attach tasks to revisions from Differential, but not revisions
to tasks from Maniphest. Allow editing from either side.
This logic is kind of tricky but the alternative was massive code duplication.
Test Plan:
Added and removed revisions from maniphest. Added and removed tasks from
differential.
This should have no impact on the Facebook install since none of this is used
there.
Reviewed By: aran
Reviewers: tomo, tuomaspelkonen, jungejason, aran
CC: aran, epriestley
Differential Revision: 288
Summary:
Separates changeset IDs from rendering. Now each changeset has a "rendering
reference" which is basically a description of what the ajax endpoint should
render. For Differential, it's in the form "id/vs". For Diffusion,
"branch/path;commit".
I believe this fixes pretty much all of the bugs related to "show more" breaking
in various obscure ways, although I never got a great repro for T153.
Test Plan:
Clicked "show more" in diffusion change and commit views and differential diff,
diff-of-diff, standalone-diff, standalone-diff-of-diff views. Verified refs and
'whitespace' were always sent correctly.
Made inline comments on diffs and diffs-of-diffs. Used "Reply".
Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen, jungejason, aran
CC: aran, tuomaspelkonen, epriestley
Differential Revision: 274
Summary:
Browsing comments was a bit difficult without the possibllity to jump
between comments. These links will make the browsing easier.
Test Plan:
Tested on multiple diffs that the links were working correctly.
Reviewed By: epriestley
Reviewers: epriestley
CC: jungejason, aran, tuomaspelkonen, epriestley
Differential Revision: 266
Summary:
Expanding lines duplicated some lines occasionally, because whitespace
option was different for the original request and the following request.
Test Plan:
Tested that the broken changeset was correct now.
Reviewed By: epriestley
Reviewers: epriestley
CC: jungejason, aran, epriestley
Differential Revision: 263
Summary:
Fixes the issue caused by rPa0af5b66437719dba6136579c051982ab275e6a0. Prior to
that patch, isCommentInNewFile() returned $comment->getIsNewFile(). While this
was often the wrong value, it came from the database and was the integer 1 if
true.
After the patch, the function returns 'true' as a boolean, which is passed to JS
and then back to PHP, interpreted as an integer, and evaluates to 0.
To avoid this issue in general, provide an isBool() method on AphrontRequest
which interprets this correctly.
I will also revert the revert of rPa0af5b66437719dba6136579c051982ab275e6a0 when
I land this.
Test Plan:
Clicked "reply" on the right hand side of a diff, got a right-hand-side inline
comment.
Reviewed By: rm
Reviewers: tuomaspelkonen, jungejason, aran, rm
CC: simpkins, aran, epriestley, rm
Differential Revision: 250
Summary:
This is a followup to D228. Basically, we use "changeset" (and, implicitly,
changesetID) for way too much stuff now.
One thing it can't possibly capture is the complete, arbitrary mapping between
the left and right sides of the displayed diff and the places we want to store
the left and right side comments. This causes a bunch of bugs; basically adding
inline comments is completely broken in diff-of-diff views prior to this patch.
Make this mapping explicit.
Note that the renderer already passes this mapping to
DifferentialChangesetParser which is why there are no changes outside this file,
I just didn't finish the implementation during the port.
This has the nice side-effect of fixing T132 and several other bugs.
Test Plan:
Made new-file and old-file comments on a normal diff; reloaded page, verified
comments didn't do anything crazy.
Expanded text on a normal diff, made new-file and old-file comments; reloaded
page, verified comments.
Repeated these steps for a previous diff in the same revision; verified
comments.
Loaded diff-of-diffs and verified expected comments appeared. Made new left and
right hand side comments, which almost work, see below.
NOTE: There is still a bug where comments made in the left-display-side of a
diff-of-diffs will incorrectly be written to the right-storage-side of the
right-display-side diff. However, this is an issue with the JS (the PHP is
correct) so I want to pull it out of scope for this patch since I think I need
to fix some other JS stuff too and this improves the overall state of the world
even if not everything is fixed.
Reviewed By: tuomaspelkonen
Reviewers: jungejason, tuomaspelkonen, aran, ola
CC: aran, tuomaspelkonen, epriestley
Differential Revision: 237
Summary:
Point users toward 'arc amend', 'arc commit', 'arc patch' and 'arc export' since
no one is going to read 'arc help'.
There's kind of a tradeoff here where we're wasting a fair amount of UI space
for expert users with the patch/export hints but I think it's probably okay
since there's really no other way to figure out that these features exist.
Note that the "export" command given isn't complete (it needs --git or
--unified), but it will give you a useful error message when you run it, telling
you to specify --git or --unified. If it turns out users get confused by this,
let me know.
Test Plan:
Loaded a revision and looked at it. Faked it into 'accepted' status.
Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen, aran, jungejason
CC: aran, tuomaspelkonen, epriestley
Differential Revision: 242
Summary:
Add links to the 'standalone view' to grab the raw source text. I think this
operation is rare enough that it's okay to hide it like this. I changed
'Standalone View' to 'View Standalone / Raw' to improve discoverability.
This also fixes the broken Standalone View links in Diffusion by no longer
rendering them.
Test Plan:
viewed old and new sources for a changeset
Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen, jungejason, aran
CC: aran, tuomaspelkonen
Differential Revision: 243
Summary:
DifferentialChangesetParser currently takes the Changeset object to mean a bunch
of different and mutually conflicting things implicitly:
- Changeset ID is used to access the render cache.
- Changeset ID is also used to tell the ajax endpoint what to render when
clicking "show more".
- Changeset object has the actual changes.
- Changeset ID and "oldChangesetID" are used to choose where to show inline
comments and how to attach new ones.
This indirectly causes a bunch of problems, like T141 and T132. Move toward
making all these separate things explicit. I want to have the changeset object
only mean the actual changes to display.
Test Plan:
Looked at changesets and verified the render cache was accessed correctly (and
not accessed in other cases).
Reviewed By: tuomaspelkonen
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, epriestley, tuomaspelkonen
Differential Revision: 228
Summary:
This mode was fixed in D174 by grglr and is the best mode for almost all changes
once nonbroken, so make it the default.
This is also the mode which takes advantage of the render cache.
Test Plan:
Loaded a revision, got "Ignore All" as the default mode, and revision rendering
wasn't silly/broken.
Reviewed By: tuomaspelkonen
Reviewers: grglr, jungejason, tuomaspelkonen, aran
CC: aran, tuomaspelkonen
Differential Revision: 229
