Summary:
Thanks to erling for the report. This was XSSable, although you could
only get yourself.
Test Plan:
Made a comment like "</textarea><h1>" and edited it before and after
the patch. Proper behavior with this patch.
Reviewed By: aran
Reviewers: erling, jungejason, tuomaspelkonen, aran
CC: aran
Differential Revision: 187
Summary:
If you don't have a custom renderer, this variable does not get set
and emits a warning when you try to read it.
Test Plan:
Loaded page before and after change, warnings went away (lines
145 and 154 in old file).
Reviewed By: aran
Reviewers: tuomaspelkonen, aran
CC: jungejason, aran, epriestley
Differential Revision: 186
Summary:
Just minor bookkeeping, but the current regexp is too liberal and
will match things which can't possibly be revision hashes.
Test Plan:
Typed things which should and shouldn't be revision links, they
got handled properly.
Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran
Differential Revision: 185
Summary:
This permits individual deployments to better configure their
database configuration, e.g. to allow more dynamic configuration that reacts
to database moves or master/slave replication.
Test Plan:
Browse
Reviewed By: epriestley
Reviewers: Girish, epriestley
CC: aran, epriestley
Differential Revision: 183
Summary:
Fixed buggy and incomplete logic for handling IGNORE ALL mode properly.
A subparser is used to parse the non-ws-ignoring changeset while a
ws-ignoring changeset is handed off to the original parser. At a later
step, the original parser queries the subparser for its lines of text
(which are formatted properly due to being in non-ws-ignoring mode) and
uses them to replace the text in the ws-ignoring diff.
Task ID: 549940
Test Plan:
-turn off caching temporarily (the cached view is still indented
improperly)
-visit http://phabricator.dev1943.facebook.com/D242591
-note aligned, but not completely highlighted, indentation right above the
comments complaining about
indentation issues
Reviewed By: aran
Reviewers: tuomaspelkonen, jungejason, aran
Commenters: epriestley
CC: aran, epriestley, grglr
Differential Revision: 174
Summary:
Add ability to define mysql slaves and then use that connection on 'r'
connection modes. 'w' connections go to the master server.
Test Plan:
- php -l and checkModule
- worked in my devbox
Reviewed By: jungejason
Reviewers: dpepper, tuomaspelkonen, jungejason
CC: jungejason, aran
Revert Plan:
sure
Differential Revision: 175
Summary:
This adds a new view to differential called Updates.
The high-level goal of Updates is to enabled differential to be
effectively used without email notifications. I've tried doing things
like automatically deleting differential emails where I'm in the 'to'
line since they show up on the main diffential page but then there's
always the chance an important diff flies by without me seeing it. Also,
sometimes someone comments on a diff post-commit but differential
doesn't surface those diffs.
I re-created a test db on my devserver using mysqldump to get data on
revs > 230000 so I would have some test data. We need to add a simple
viewtime table but I didn't want to do that in production. Here's the
table:
CREATE TABLE differential_viewtime (
viewerPHID varchar(64) not null,
objectPHID varchar(64) not null,
viewTime int unsigned not null,
PRIMARY KEY (viewerPHID, objectPHID)
);
Issues:
-Once we turn this on, all diffs will be 'unviewed'. What do you think
about a 'Clear All' button or something?
-Maybe we should add a pager
This feature would be insanely useful, let me know what you think.
Test Plan:
Loaded Updates in my sandbox
http://phabricator.dev1577.snc6.facebook.com/differential/filter/updates/
Clicked a diff, then went back, made sure diff disappeared from Updates
list
Reviewed By: tuomaspelkonen
Reviewers: epriestley, jungejason, tuomaspelkonen
Commenters: epriestley
CC: epriestley, elynde, tuomaspelkonen
Differential Revision: 169
Summary:
Take Tuomas's first diff. Sorry, my bad on diff feedback.
Test Plan:
Load D240353 without any options and make sure 'ignore-trailing'
selected.
Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen
CC: epriestley, aran, tuomaspelkonen
Differential Revision: 172
Summary:
'ignore-all' is very confusing, especially for Python, because the indentation
is incorrect.
Test Plan:
Tested loading D240353 without any options and made sure 'ignore-trailing' was
selected and the diff looked correct.
Reviewed By: aran
Reviewers: jungejason, simpkins, aran, dpepper
Commenters: dpepper
CC: grglr, aran, dpepper, tuomaspelkonen
Differential Revision: 171
Summary: add a conduit method to enable querying revisions' phid from
their revision_IDs, and another one to update the task->revision assoc.
Test Plan: for querying revision_phid method, tested empty, one, and two
revisions in the query. For the one to update the task->revision assoc,
I have another diff in facebook which verified it add and remove assoc
correctly.
Reviewers: tuomaspelkonen, epriestley
CC:
Differential Revision: 165
Summary:
differential.getrevision now returns commits for a given revision.
URI is also return in differential.getrevision.
Test Plan:
Tested from Conduit Console UI that the calls were working
correctly.
Reviewed By: jungejason
Reviewers: jungejason
CC: epriestley, simpkins, dpepper, jungejason
Differential Revision: 163
conduit call.
Summary:
The reviewer information was not available for revisions before this.
Test Plan:
Tested with Conduit Console that correct reviewers were returned.
Reviewed By: jungejason
Reviewers: jungejason
Commenters: epriestley
CC: epriestley, tuomaspelkonen, jungejason
Differential Revision: 160
Summary:
Phabricator did not support giving the task ids in the commit message.
Currently there is no default implementation for this, but there is a
Facebook specific implementation. At some point default implementation
for Maniphest tasks to revisions will be added.
Test Plan:
Tested with the Facebook specific implementation that task ids are
recognized in the commit message and tasks are automatically attached
to revisions.
The task attached to this revision was added from the commit message.
Reviewed By: jungejason
Reviewers: jungejason, epriestley
CC: dpepper, edward, gpatangay, tuomaspelkonen, jungejason
Differential Revision: 240996
Summary:
Having the filename in the <title> will make it easier to browse through many
many tabs of diffusion
Test Plan:
Open up a file or directory in diffusion and ensure that it shows only the file
name.
Reviewed By: tuomaspelkonen
Reviewers: epriestley, tuomaspelkonen, jungejason
Commenters: epriestley
CC: epriestley, aizatto, tuomaspelkonen
Differential Revision: 150
Summary:
Add a method to get the information about a revision, including its full
list of diffs.
We could add an option to just return the diff IDs if we wanted. For my
use case, I need the full set of information for each diff, so fewer
round trips is better. This is also how the old json.php page used to
work.
Test Plan:
Tuomas tested it in his sandbox.
Reviewed By: tuomaspelkonen
Reviewers: jungejason, tuomaspelkonen, epriestley
CC: tuomaspelkonen
Differential Revision: 156
Summary:
Currently if I go to leave an inline comment and then decide
against it and hit <tab>-<enter>, the inline comment edit dialog closes
but the overlay doesn't disappear due to a JS error around trying to
access the markup property of null. This fixes that by setting the
payload to an array with empty markup so that the JS works and the
overlay disappears.
Test Plan:
clicked on the line number column, then pressed ok and
confirmed the dialog closed, the overlay disappeared and there were no
JS errors. also tested that for clicking the reply link and when editing
an existing inline comment.
Reviewed By: aran
Reviewers: epriestley, aran
CC: aran, bh
Revert Plan:
OK
Differential Revision: 154
Summary:
Remove reordering code for package array as it's ordered in the first place
Test Plan:
Called API through conduit console, still works
Reviewed By: epriestley
Reviewers: epriestley, tuomaspelkonen
CC: ju, dpepper, epriestley
Blame Revision:
126
Differential Revision: 141
Summary:
Adding method that given a path will go up the folder hierarchy until it finds
the owning package
and return owners for that package.
Task ID: #403724
Test Plan:
Tried the new API call through console on various path combinations
Reviewed By: epriestley
Reviewers: epriestley, dpepper, tuomaspelkonen
CC: epriestley, Leon
Revert Plan:
n/a
Tags: bootcamp, Push Efficiency
- begin *PUBLIC* platform impact section -
Bugzilla: #
- end platform impact -
Differential Revision: 126
Summary:
Fixed the image macro regex not to use '-' as the separator.
Also minor improvement to randomon.
Test Plan:
Tried different image marcors.
Reviewed By: jungejason
Reviewers: jungejason
CC: epriestley, jungejason
Differential Revision: 153
Summary:
Loading all diffs for a differential revision is needed by at least
perflab.
Test Plan:
Created a simple script that queried the conduit and made sure that
it returned correct values.
Reviewed By: jungejason
Reviewers: jungejason
CC: epriestley, jungejason
Differential Revision: 155
Summary:
Whitespace options could not be selected and the 'ignore_all' was not working.
Test Plan:
Made sure that whitespace is ignored correctly and selected option is handled
correctly.
Reviewed By: jungejason
Reviewers: jungejason
CC: epriestley, jungejason, tuomaspelkonen
Differential Revision: 149
Summary:
Replies to comments went always to the left side of the diff. There
was a confusion between 'is_new' and 'on_right'. Database wants the
information if the reply is on the left or on the right. The database
does not care if the comment is a reply or an original comment.
The code looks a bit confusing, because the database field is called
'isNewFile' and that is used to determine, which side the comment
applies to.
Test Plan:
Tested that every of combination of new comment/reply to the left/right side
worked when editing and after submitting the comment.
Reviewed By: jungejason
Reviewers: jungejason
CC: epriestley, jungejason
Differential Revision: 148
Summary:
Differential used to only show diffs for the first 100 files. Now all
the files are shown in the table of contents and there is a link to
the standalone view for every file. The inline diffs can still be
seen, if user clicks "Show All Files Inline".
Inline comments can also be added in the standalone view, but there is
no form to submit them. The revision page must be reloaded to able to
submit the inline comment.
Test Plan:
Changed the limit to three for testing purposes and checked that a diff of
mine with 5 files had the links to the standalone views. Made sure that
adding a comment in a standalone view worked and that after reloading the
revision page the comment was visible. Changed the limit back to 100 and
made sure that my diff had all the files inline and that the anchor links
were working.
Reviewed By: jungejason
Reviewers: jungejason
CC: epriestley, simpkins, jungejason, tuomaspelkonen
Differential Revision: 147
Summary: add --load-phutil-library for libraries other than phabricator
and libphutil.
Test Plan: launch a daemon in facebook such as
PhabricatorDifferentialCommentDaemon.
Reviewers: epriestley, tuomaspelkonen
CC:
Differential Revision: 146
Summary:
The existing message is confusing and might cause people to think
that there is something wrong in the field name, e.g., 'CC' instead
of the value.
Test Plan:
Tested by putting random crap in CC and Reviewers and made sure the
error message was using the new format.
Reviewed By: jungejason
Reviewers: epriestley, jungejason
CC: mdevine, jungejason
Differential Revision: 145
Summary: to enable publishing to internfeed, we create timeline events
for the comments.
Test Plan: create a test revision, comments on it and check if it
creates a timeline event with correct data at /daemon/timeline.
Reviewers: epriestley,tuomaspelkonen
CC:
Differential Revision: 144
Summary:
Inline comment anchros were present for all different diffs inside a
revision. They were only working for the current diff. Removed the
links that were for different diffs.
I couldn't get the automatic linking to other diffs working, probably
because the anchors didn't work when the page was reloaded. This is also
a bit confusing if the diff changes when clicking on anchor.
We might want to carry all the comments along in the future, but I
don't think it's needed at the moment.
Test Plan:
Tested on a revision, which had inline comments for differerent diffs
that only the comments for the latest diff had anchors when the page
was loaded. Changed the diff manually on the page and made sure the
anchors for that diff were working correctly and the anchors for the
latest diff were not available.
Reviewed By: epriestley
Reviewers: epriestley
CC: jungejason, epriestley
Differential Revision: 142
Summary:
When quoting someone's text in differential, the text was not properly
rendered. Now the quoted text is rendered nicely and it's easy to locate.
Test Plan:
Disabled the inline cache, Checked that quotes in D211086 are rendered
correctly. Started writing a new comment starting with '>' and made
sure the new comment was rendered correctly while writing it.
Reviewed By: epriestley
Reviewers: epriestley
CC: jungejason, tuomaspelkonen, epriestley
Differential Revision: 138
Summary:
The transparent mask around the dialogs was too dark. Code was hard to
read. Now opacity has a value of 37, which makes the code easy to read,
but still clearly indicates that the dialog is present.
Test Plan:
I was able to read the code easily through the mask.
Reviewed By: epriestley
Reviewers: epriestley
CC: epriestley, tuomaspelkonen
Differential Revision: 139
Summary: Is this an appropriate place to say "herp derp?"
Test Plan: Check for Resign as Reviewer for the following table
waiting for review | accepted | committed
Am actually reviewer
Am not reviewer but CC'ed
Unrelated
Reviewers: epriestley
CC:
Differential Revision: 140
Summary:
There's an OAuth diagnostics page at /oauth/facebook/diagnose/, which
shows some diagnostic information. Currently, it attempts to establish an
application token session and shows the token if it is successful. An attacker
could use this to do vaguely nefarious things (retreive application statistics,
I think?).
This interface was originally admin-only but then I threw out the very silly
admin mode patch I had at the time and we currently have no admin mode, and
thus this interface is public. This token isn't useful in diagnosis anyway,
so don't reveal it.
Test Plan:
Visited oauth diagnostics page, no token revealed
Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen, jungejason
CC: tuomaspelkonen
Differential Revision: 136
Summary: See D133. Workers can also be subject to the same race, invert the
row relationship in the same way.
Test Plan: Launched repository master daemons and some taskmasters and used
the Daemon console to veify that they were able to process tasks. Manually
checked the database to make sure data got linked correctly and that new data
was inserted correctly.
Reviewers: jungejason
CC: tuomaspelkonen
Differential Revision: 135
Summary: While I should fix the transactional stuff, that patch is going to be
tricky and transactions have some performance implications. This is a simple
fix which prevents the race.
Instead of having the data point at the event ID, have the event point at a
data ID. Insert the data first, then insert the event with the right data
pointer. This is super simple and prevents the race issue.
Test Plan:
- Ran the schema upgrade script, verified that the database was
correctly upgraded. Was also prompted to stop daemons.
- Ran 'repository-launch-master', verified that the discovery daemons were
able to discover new commits and insert events for them. Verified the
committask daemon was consuming events and converting them into tasks.
- Verified new tasks looked correct in the database.
- Browsed web interface.
Reviewers: jungejason
CC: tuomaspelkonen
Differential Revision: 133
Summary:
Added long waited image macro support for differential and others.
Test Plan:
Tried a couple of different macros and made sure they appear nicely
in the comment preview. Made sure that the normal comments are shown
correctly.
Reviewed By: epriestley
Reviewers: epriestley
CC: jungejason, tuomaspelkonen, epriestley
Differential Revision: 129
Summary:
There's no reason to default-reject clients since they can just
pretend to be arc anyway. If they're speaking the right protocol, let them
communicate over Conduit.
Test Plan:
Changed arc to identify as 'arczsdba', ran an arc command.
Reviewed By: simpkins
Reviewers: simpkins
CC: simpkins
Differential Revision: 132
Summary:
Old differential allowed users to plan changes for their own revisions.
This feature is now available in Phabricator version of differential.
Test Plan:
Tested by selecting "Plan Changes" for one of my own aceepted revisions.
Reviewed By: epriestley
Reviewers: epriestley
CC: jungejason, epriestley
Differential Revision: 130
Summary:
No workflow dialog when subscribing or unsubscribing from a Differential
Revision
Test Plan:
Repeatedly subscribe and unsubscribe to a revision. Enter refractory period due
to
speed of page gen.
Reviewed By: epriestley
Reviewers: epriestley
CC: epriestley
Differential Revision: 128
Summary:
There was a need to add old facebook specific action links and properties
back to differential.
Test Plan:
Tested that all the facebook specific links work for multiple
different revisions.
Reviewed By: epriestley
Reviewers: epriestley
CC: jungejason, tuomaspelkonen, epriestley
Differential Revision: 127
Summary:
We were showing all kinds of information about the object in object
transcript, but there was no link to the actual object.
Test Plan:
Checked that links were working correctly for both differential and
commit objects.
Reviewed By: epriestley
Reviewers: epriestley
CC: jungejason, epriestley
Differential Revision: 125
Summary:
Documentation describes how to use the script to upgrade schema.
Test Plan:
Generated the documentation and it looked good.
Reviewed By: epriestley
Reviewers: epriestley
CC: jungejason, epriestley
Differential Revision: 124
Summary:
Like the title says.
Test Plan:
grep for ': ' didn't reveal any other similar problems.
Reviewed By: jungejason
Reviewers: epriestley, jungejason
CC: jungejason
Differential Revision: 121
Summary:
Enable "Resign as Reviewer" from Differential Revision View UI
Test Plan:
Look at revision that I am a reviewer on and that I am not.
Reviewed By: epriestley
Reviewers: epriestley
CC: epriestley
Differential Revision: 120
performance (e.g., for profile images) and you need to know a highly entropic
PHID to access a file in the first place, plus installs should generally be
doing HTTPS.
Summary:
add filtering for MetaMTA transcripts, add Herald
transcripts, also fixed PhabricatorObjectHandleData to support commits.
Note that paging in the transcripts pages will be in a different diff.
Test Plan:
test the transcripts for both MetaMTA and Herald.
Reviewed By: epriestley
Reviewers: epriestley, tuomaspelkonen
CC: jungejason, epriestley
Differential Revision: 114
Summary:
Users were able to accidentally update revisions they didn't own. Now
it is impossible to update a revision that belongs to someone else or
has been marked as committed.
Test Plan:
Tested that normal workflow works as previously, but after running
'arc amend', running 'arc diff' fails.
Manually changed the revision number in the git commit message and tried
to update something that belongs to Jason -> Failed.
Reviewed By: epriestley
Reviewers: epriestley
CC: jungejason, epriestley, tuomaspelkonen
Differential Revision: 112
Summary:
Allow Conduit methods to retrieve the authoritative, logged-in user
identity.
Test Plan:
Ran user.whoami (an authenticated method) and got my info back. Ran
conduit.connect (an unauthenticated method) and the world did not explode.
Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen
CC: tuomaspelkonen, epriestley
Differential Revision: 113
Summary:
If there are more than 100 changed files in a single commit in
Diffusion, only the first 100 changes will be shows. There is a warning
sign about this and a button that will reload the same page with all the
changes visible.
Test Plan:
Tested that everything worked as expected with commits over and under 100
commits.
Reviewed By: epriestley
Reviewers: epriestley
CC: jungejason, epriestley
Differential Revision: 110
Summary:
By default, indirect events in Diffusion SVN history views were visible.
Now they are invisible, but the visiblity can be changed using a button
in the top right corner.
Test Plan:
Tested on multiple different files that the history is shown correctly after
the page is loaded, after the button is clicked once and after the button is
clicked the second time.
Reviewed By: epriestley
Reviewers: epriestley
CC: jungejason, epriestley, tuomaspelkonen
Differential Revision: 106
Summary:
Task selector didn't support searching tasks with their IDs, e.g., 'T17'.
This was confusing, because in the task list the task ID was visible, but you
could not search them.
Test Plan:
* Checked that searching 'T<task_id>' works with all the filters
* Checked that using multiple task IDs in the same query works
* Check that mixing task IDs and free text works
Reviewers: epriestley
CC: jungejason
Differential Revision: 105
Summary:
When function phlog() is called, stacktrace and detailed log information
is shown in DarkConsole.
Test Plan:
Called 'phlog' function from various places in Phabricator and checked that
the debug information was available in DarkConsole.
Reviewed By: epriestley
Reviewers: epriestley
CC: jungejason, epriestley
Differential Revision: 101
Summary:
Editing Maniphest tasks for a Differential Revision required user to hit
'search' every time he changed search parameters. Now select and text input
changes trigger search automatically.
Test Plan:
Tested that changing the select and entering text automatically gave the
correct results.
Reviewed By: epriestley
Reviewers: epriestley, jungejason
Commenters: jungejason
CC: epriestley, jungejason
Differential Revision: 102
