updated
Summary:
- If you update a revision with a nonempty "Maniphest Tasks" field, an empty
comment is posted (see T586).
- The transaction email currently says "Attached revision 'Unknown
Differential Revision'", move attaching to "didWriteRevision()" to make sure the
object has been written.
Test Plan: - Attached; updated a revision.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T685
Differential Revision: 1223
Summary:
- Use DifferentialRevisionQuery, not DifferentialRevisionListData, to select
revisions.
- Make UI simpler (I hope?) and more flexible, similar to Maniphest. It now
shows "Active", "Revisions", "Reviews" and "Subscribed" instead of a hodge-podge
of miscellaneous stuff. All now really has all revisions, not just open
revisions.
- Allow views to be filtered and sorted more flexibly.
- Allow anonymous users to use the per-user views, just don't default them
there.
NOTE: This might have performance implications! I need some help evaluating
them.
@nh / @jungejason / @aran, can one of you run some queries agianst FB's corpus?
The "active revisions" view is built much differently now. Before, we issued two
queries:
- SELECT (open revisions you authored that need revision) UNION ALL (open
revisions you are reviewing that need review)
- SELECT (open revisions you authored that need review) UNION ALL (open
revisions you are reviewing that need revision)
These two queries generate the "Action Required" and "Waiting on Others" views,
and are available in P247.
Now, we issue only one query:
- SELECT (open revisions you authored or are reviewing)
Then we divide them into the two tables in PHP. That query is available in P246.
On the secure.phabricator.com data, this new approach seems to be much better
(like, 10x better). But the secure.phabricator.com data isn't very large. Can
someone run it against Facebook's data (using a few heavy-hitting PHIDs, like
ola or something) to make sure it won't cause a regression?
In particular:
- Run the queries and make sure the new version doesn't take too long.
- Run the queries with EXPLAIN and give me the output maybe?
Test Plan:
- Looked at different filters.
- Changed "View User" PHID.
- Changed open/all.
- Changed sort order.
- Ran EXPLAIN / select against secure.phabricator.com corpus.
Reviewers: btrahan, nh, jungejason
Reviewed By: btrahan
CC: cpiro, aran, btrahan, epriestley, jungejason, nh
Maniphest Tasks: T586
Differential Revision: 1186
Test Plan:
Created a listener that adds some patterns to $matches array, reloaded
Differential, some changesets were not shown as generated.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, mareksapota
Differential Revision: 1200
couple bug fixes
Summary:
- Add the ability to query for "responsible users" (author or reviewer).
- Add the ability to query for "subscribers" (reviewer or CC).
- Fix an issue where CC and Reviewer used the same join table alias and were
incompatible.
- Remove support for 'paths' for the moment, since each path needs a
repository ID. (There are no clients for this.)
- Remove single withX() methods that have no callsites -- withPath() is
singular because it accepts two arguments and I didn't want to have an ad-hoc
type format, but I think we can get away without these for other conditions.
- Include GROUP BY in more cases where may need it. This doesn't actually
change program behavior since we uniquify in loadFromArray(), it just means less
data over the wire.
These new query classes are to support rewriting the Differential list view on
top of DifferentialRevisionQuery.
Test Plan:
- Issued queries via conduit for "responsible users".
- Issued queries via conduit for "subscribers".
- Issued queries via conduit for "cc" with "reviewer" at the same time.
- Issued queries via conduit for "cc", "reviewer", "responsible users" and
"subscribers" at the same time.
- Issued a "subscribers" and "reviewers" query which returned duplicates;
verified GROUP BY took effect.
Reviewers: nh, btrahan, jungejason
Reviewed By: nh
CC: aran, nh
Differential Revision: 1182
Summary:
Derped this one up; while my testing was successful in preventing runaway
attaching I missed the bit where it doesn't actually work.
This resolves the "Unknown Object" link seen on T661.
Test Plan:
- Created two new revisions, each attached to a local task.
- Verified that they attached additively, Maniphest and Differential were
linked to the right places, and nothign else bad happened.
Reviewers: btrahan, fratrik
Reviewed By: fratrik
CC: aran, fratrik, btrahan
Differential Revision: 1181
Summary:
Prevent keyboard focus of these links so we don't disrupt tab order from
comments to "Submit".
Arguably I should make a "function" for this or something but there's nowhere to
really put it that makes any sense right now.
Test Plan: Verified Firefox skips these links in tab order.
Reviewers: fratrik, btrahan, jungejason
Reviewed By: fratrik
CC: aran, fratrik
Maniphest Tasks: T661
Differential Revision: 1180
Summary:
This landed during my review drama embargo and is a generally good idea but had
some implementation issues.
@elynde reports it has been broken for some time, although it still works on
secure.phabricator.com so I'm guessing it's just taking a zillion years to run
at Facebook. It's up to more than a second for me on secure.phabricator.com:
https://secure.phabricator.com/file/view/PHID-FILE-v4ql4c66u3xnkarmrpm4/
The basic problem is that some of the data architecture around this
implementation is hard to scale. I want to pursue a similar feature eventually,
but drive it off notifications that we'll ship through real-time infrastructure
too.
I'm also trying to get rid of DifferentialRevisionListData and this simplifies
that somewhat.
Test Plan:
- Grepped for table name, table constant, query constant, and class name; no
hits.
- Applied SQL patch.
- Verified that Differential no longer shows "Updated".
Reviewers: elynde, btrahan, jungejason
Reviewed By: elynde
CC: aran, elynde
Differential Revision: 1178
Summary:
Changed cc/reviewer search to be a union/or instead of intersection/and within
each list. Also added support to search for multiple authors (same behavior as
cc/reviewer), and updated conduit call to match. (See discussion on D1158.)
Test Plan:
Used the conduit call to search for revisions with one of 2 people on the cc
list, and checked the results to see that it wasn't constraining to requiring
both be on the cc list.
Reviewers: epriestley, jungejason, btrahan
Reviewed By: epriestley
CC: aran, nh, epriestley
Differential Revision: 1179
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:
To reduce blindness, all textareas with some kind of special syntax should have
an information about this syntax and a link to its documentation. Preview
function is a nice complement but it doesn't replace this information.
I've added this information and the link below the comment field.
Please note that <a target> is a valid attribute in HTML5.
Test Plan:
Go to https://secure.phabricator.com/D1164#comment-content
There should be a link to Remarkup Reference
This link should open Remarkup Reference in a new window (to not discard the
comment)
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, vrana
Differential Revision: 1164
Summary:
add "Maniphest Task:" or "Maniphest Tasks:" followed by text that has TX in it.
foreach TX the task will be attached to the revision and the revision will be
attached to the task. parsing is pretty... ummm, robust such that it will pick
up any TX substring and parse that as a Maniphest Task just fine. it errors
out if there is not an actual task for TX and otherwise churns along pretty
nicely.
Also, make sure the PhabricatorObjectHandle loads the task ID as the alternateID
since we need that here and it should be that way anyhoo.
Test Plan:
made a diff and in the commit message added Maniphest Task(s): TX combination.
Tried various combinations of TX -- single, multiple with commas, multiple many
lines, single bad, multiple bad, multiple mix of bad and good. verified that the
good tasks were attached to the diff and diff was attached to the good tasks.
Maniphest Tasks: T137
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, btrahan, epriestley
Differential Revision: 1165
Summary: Some fields need this data in some circumstances in order to validate
-- see D1153.
Test Plan: Ran "arc diff" against local, no longer got an exception for access
of this field from the 'Reviewers' validator.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, btrahan
Differential Revision: 1160
Summary:
Created a differential.query conduit method that is built on top of
DifferentialRevisionQuery. I also added support for querying by author, ccs, and
reviewers to DifferentialRevisionQuery, so feature parity can be brought up to
match differential.find and its backing class DifferentialRevisionListData.
Test Plan:
Tried a few calls to the conduit call using the web interface, and got back
reasonable looking data.
Reviewers: epriestley, jungejason, btrahan
Reviewed By: epriestley
CC: aran, nh, epriestley
Differential Revision: 1158
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
order to generate a template
Summary: See T614. This allows us to generate an empty template by calling
Conduit, so we can build command-line editing workflows for SVN, Mercurial, and
conservative-Git.
Test Plan: Used web console to invoke Conduit method; got a reasonable empty
template out of it.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley, btrahan
Differential Revision: 1156
Summary:
See T643. We have some hard-coded checks in Arcanist for the existence of
'testPlan' and 'title', and don't properly validate those fields on the server.
Add a validation pass in the Conduit-based edit pathway.
In particular, this means that if you disable the "Test Plan" field, Arcanist
won't block you anymore.
Test Plan: Disabled Arcanist checks and ran "arc diff"; got blocked on the
server side.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Differential Revision: 1153
Summary:
Move event framework from Phabricator to libphutil so it can be used in other
phutil projects, such as Arcanist.
Test plan:
Use along with path to libphutil, events should work as expected.
Reviewers: epriestley
Differential Revision: 1098
Summary: Allow tweaking Differential mail before sending.
Test Plan:
Wrote a listener renaming Differential attachments and it worked without
problems.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, mareksapota, davidreuss
Differential Revision: 1091
Summary: See comments. A few installs have remarked that their organizations
would prefer buttons labled "Submit" to buttons labeled "Clowncopterize".
Test Plan:
- In "serious" mode, verified Differential and Maniphest have serious strings,
tasks can not be closed out of spite, and reset/welcome emails are extremely
serious.
- In unserious mode, verified Differential and Maniphest have normal strings,
tasks can be closed out of spite, and reset/welcome emails are silly.
- This does not disable the "fax these changes" message in Arcanist (no
reasonable way for it to read the config value) or the rainbow syntax
highlighter (already removable though configuration).
Reviewers: moskov, jungejason, nh, tuomaspelkonen, aran
Reviewed By: moskov
CC: aran, moskov
Differential Revision: 1081
Summary:
`arc commit` and `arc mark-committed` would only add comments <author> committed
this revision, since now everyone can run this commands it makes more sense to
show the actual committer instead of the author.
Test Plan:
Commit (or mark committed) not your revision, Phabricator should add <you>
committed this revision comment instead of <author> committed this revision.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1067
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
diffs which add empty files
Summary:
See T507 and some others. We now parse empty git diffs correctly, but the logic
to build DifferentialDiffs out of them leaves the objects with 'null' for
$changesets, when it should be array().
Further layers later throw, believing we have not loaded the changesets, when we
actually have, there just aren't any.
Test Plan: Viewed rJX05d493e17fbbb29f29e4880be6834d1d7415374e in Diffusion,
which adds an empty README file. No exception thrown.
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: nh
CC: aran, nh
Differential Revision: 1038
transactional mail
Summary: See T571. SES refuses to deliver mail with this header and there are
various reports of other issues on the internet so I'm defaulting it to off.
Test Plan: Set config to true, tried to send mail, SES rejected it because of
"Precedence: bulk" header.
Reviewers: bmaurer, ola, jungejason, nh, aran
Reviewed By: aran
CC: aran, epriestley, bmaurer
Differential Revision: 1032
Filesystem::readRandomCharacters()
Summary: See T547. To improve auditability of use of crypto-sensitive hash
functions, use Filesystem::readRandomCharacters() in place of
sha1(Filesystem::readRandomBytes()) when we're just generating random ASCII
strings.
Test Plan:
- Generated a new PHID.
- Logged out and logged back in (to test sessions).
- Regenerated Conduit certificate.
- Created a new task, verified mail key generated sensibly.
- Created a new revision, verified mail key generated sensibly.
- Ran "arc list", got blocked, installed new certificate, ran "arc list"
again.
Reviewers: jungejason, nh, tuomaspelkonen, aran, benmathews
Reviewed By: jungejason
CC: aran, epriestley, jungejason
Differential Revision: 1000
Summary: This allows extensions to have more options for generating custom
hyperlinks.
Test Plan:
custom-inline rules are moved before default rules. Test existing products which
implement custom rules.
Make sure you use "$this->getEngine()->storeText()" in rules.
Reviewers: epriestley, jungejason
Reviewed By: jungejason
CC: aran, epriestley, emiraga, jungejason
Differential Revision: 1024
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
Test Plan:
Turn on sending patches, create a new revision - you should get a .patch file in
your mail instead of a .diff file.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1016
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: When the user clicks a crossreference, jump them to symbol lookup
Test Plan: Clicked some crossref symbols
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: nh
CC: aran, nh, epriestley
Differential Revision: 904
Summary:
the details pages are using preload instead of ondemand for
typeahead, but the most common actions on the pages are commenting which
would not need the preloaded info. To improve the performance of the
pages, turn on ondemand according to the setting in the config file.
Test Plan: verify it is working with both modes, for both pages.
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 995
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:
Still some rough edges, but this adds a table of open revisions to Diffusion.
See T262.
I'll make this a little better (e.g., "see all.." instead of arbitrary 10 cap,
or maybe move to top-level nav?) but I think I have to refactor some other stuff
first. This should let us root out any major issues, at least.
NOTE: You must associate Arcanist Projects with Repositories (in Repositories ->
Arcanist Projects -> Edit) for this to work!
Also made paths include all parent paths so that browse views of directories
will work.
Test Plan: Uploaded a diff which affected "/blah", it appeared when browsing "/"
and "/blah".
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 979
Summary:
For T262, we need to query for revisions by affected path.
We currently have a class called "DifferentialRevisionListData" but it's sort of
nasty and it would have been really cumbersome to add this query to it.
Instead, this provides a query object more in line with ManiphestTaskQuery,
which I'm pretty happy with. I'd eventually like to get rid of
DifferentialRevisionListData but it's used in a couple of places right now.
Test Plan: Used phpsh to execute queries, got back apparently-sensible result
sets.
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason, epriestley
Differential Revision: 978
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: The display of images pairs is not corresponding to the selected two
image diffs. The fix is to use reference to get the phid for each image.
Test Plan: Create a revision with two diffs of images.
Test the display between base and diff1/diff2.
Test the rendering of images between diff1 and diff2.
Test the inline comments also.
Reviewers: epriestley, jungejason
CC:
Differential Revision: 955
Summary:
Postponed unit tests are not unit tests with problems. The results
just haven't arrived yet.
Test Plan: Tested accepting a diff with unit status 1, 3, 5 (ok, errors,
postponed)
Reviewers: jungejason, epriestley
Reviewed By: epriestley
CC: aran, epriestley, tuomaspelkonen
Differential Revision: 969
Summary: See T262. This creates the index on the Differential side which we need in order to execute this query efficiently on the Diffusion side.
Also renames "DiffusionGitPathIDQuery" to "DiffusionPathIDQuery", this query object has nothing to do with git.
Test Plan: Attached top-level and sub-level diffs to revisions and verified they populated the table with sensible data.
Reviewers: bmaurer, aravindn, fmoo, jungejason, nh, tuomaspelkonen, aran
CC:
Differential Revision: 931
Summary: For reasons explained in the config I've omitted this from the default
action set, but it's trivial to support it. See D916.
Test Plan: Commented on a revision, was informed I could "!accept" in the email.
Used "!accept" to accept the revision.
Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran
Reviewed By: Makinde
CC: aran, Makinde
Differential Revision: 928
Summary: Added line number 1 for each image and added code to display the
comments for each image.
Test Plan: Adding an image in my local directory and create a revision for it.
Click line number 1, and the comment window prompts. Adding and save the
comment. The comment shows in the differential comment list and in the inline
comment. Submit the comment. Create more comments for the image and the
"Previous" and "Next" buttons all work well.
Reviewers: epriestley, jungejason
CC:
Differential Revision: 901
Summary:
These fields use auxiliary storage now. Migrate the data and get rid of the
columns in the main table.
- This might take a little while to run, although there are <500k rows so
probably not too long.
- Maybe grab a backup of the table first, if I screwed something up this will
delete the data in these fields.
Test Plan:
- Ran migration locally.
- Browsed Differential.
- Grepped for "revertPlan" and "blameRevision".
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason, epriestley
Differential Revision: 832
