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
Summary: See task; installs occasionally need to do this themselves, and a
script is much better than me telling them to truncate tables.
Test Plan:
Ran various flavors of this command:
- purge_cache.php
- purge_cache.php derp
- purge_cache.php --help
- purge_cache.php --all
- purge_cache.php --differential
- purge_cache.php --differential --maniphest
Then I verified the actual behavior:
- Visited a Differential revision with comments, observed cache update in
'Services' tab.
- Visited a Maniphest task with comments, observed cache update in 'Services'
tab.
- Reloaded a diff standalone view, got a cache update.
Reviewers: Makinde, btrahan, jungejason
Reviewed By: jungejason
CC: aran, jungejason
Maniphest Tasks: T676
Differential Revision: 1214
Summary:
...except that pesky help tab which remains.
Pertinent bits here...
- move "History" into button "View History" that is grey and next to "Edit Page"
- for history page, add breadcrumb similar to the one on "diff" page. This
unifies the experiencing on history <=> diffs as well as gives the user a link
back to the document, which was a tab on the History page before this diff.
Thoughts for next time...
- I'd like to further unify the breadcrumbs between "View" and "History / Diff".
- The "Document Index" is pretty sweet and feels a bit buried. I wonder if
unifying breadcrumbs is the key here?
Test Plan: clicked around phriction. viewed a document, viewed its history.
verified links in breadcrumbs were correct
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T631
Differential Revision: 1221
Summary:
kill tabs for Files application. Technique is the "filter list" on the left
hand side, with separation for "Files" versus "Image Macros". UI quirks
include:
- the page title does not change for the 3 files filters while it does change
for each of the two image macro filters.
- standalone "file" pages do not have the filter view
- you can visit /file/upload/ standalone and it doesn't have the pretty filter
list on it
Please do give direction on these quirks if you like. :)
This change also neuters the ?author= functionality for files. The code is
written such that it can easily be brought back.
Test Plan: clicked around on the filters, liked what I saw. uploaded files
fancy-like and basic-like and it worked! made image macros and it worked!
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, btrahan, epriestley
Maniphest Tasks: T631
Differential Revision: 1219
Summary:
For each commit, find the affected packages, and provide a way to
search by package.
Test Plan:
create commits that touch and don't touch two packages, and verify
that they display correctly in all the UI pages.
Reviewers: epriestley, blair, nh, tuomaspelkonen
Reviewed By: epriestley
CC: benmathews, aran, epriestley, btrahan, jungejason, mpodobnik, prithvi
Maniphest Tasks: T83
Differential Revision: 1208
Test Plan: ran upgrade_schema.php on my local install. re-examined SQL query
"explain select * from phabricator_pastebin.pastebin_paste where authorPHID =
"PHID-USER-xee4ju2teq7mflitwfcs" ORDER BY id DESC;" on my local install and
noted fewer rows scanned, index used, etc
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1218
test
Summary: @jungejason reported seeing test failures here. I can't reproduce them
and my read of the code doesn't suggest why they might be happening, but add a
little more debug info in hopes of chasing this down.
Test Plan:
- Ran test in a loop for a long time, couldn't get it to fail.
- Changed assertEquals() condition to force test to fail, verified output
message was informative.
Reviewers: jungejason, btrahan
Reviewed By: jungejason
CC: aran, epriestley, jungejason
Differential Revision: 1212
Summary:
after a class is deleted/added, we need to run
arcanist/scripts/phutil_mapper src to update the library map.
Task ID: #
Test Plan:
run test case testEverythingImplemented and it passes.
Revert Plan:
Tags:
Reviewers: btrahan, epriestley
Reviewed By: btrahan
CC: aran, btrahan
Differential Revision: 1211
Summary: grab all the files in one big fetch, rather than serially fetching
them. follow up from D1198.
Test Plan: viewed paste and there were no errors!
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, jungejason, btrahan, epriestley
Differential Revision: 1202
Summary:
See D1195, which fataled this daemon.
https://secure.phabricator.com/daemon/log/2966/
Test Plan: Applied this patch to secure.phabricator.com, restarted daemon, it
picked up D1203.
Reviewers: btrahan, jungejason, mareksapota
Reviewed By: btrahan
CC: aran, btrahan, mareksapota
Differential Revision: 1204
Summary: D1174 caught this issue -- we mean to load all //your// rules, but
actually load //all// rules. Use %s correctly.
Test Plan: Hit /herald/rule/ without an exception.
Reviewers: fmoo, btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley, btrahan
Differential Revision: 1203
Summary:
merge paste create and paste list into a single controller. Add a "filter list"
to the left hand side and have new "create w/ recent", "my" and "all" views. UI
wrinkle -- "create w/ recent" does not paginate the recent pastes and instead
upsells the user to the new "all" view.
Also includes a business logic clean up or two for simplicity of code.
Test Plan:
- created a paste from the UI
- tried to create a paste with title and no body
- tried to create a paste with no title and no body
- viewed the paste list on "create" view
- viewed the paste list on "author" view
- viewed the paste list on "all" view
- viewed page 2 of the paste list for "author" and "all" views
- "forked" a given paste through completion
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, btrahan
Maniphest Tasks: T631
Differential Revision: 1198
Summary:
I broke this a little bit in my overzealous D1174, since this block validates
both '%nd' (nullable integer) and '%d' (non-nullable integer).
Clean up the conditional checks so we catch the bad case ('%d' on a PHID
converting to 0) but let the good case ('%nd' with null) through.
Test Plan: Unit tests failed; applied patch; unit tests pass.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T670
Differential Revision: 1201
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
Summary: Add the author PHID to the differential.getrevisionfeedback conduit api
method
Test Plan: issue differential.getrevisionfeedback query via conduit against a
valid revision and verify author phid is included in results
Reviewers: epriestley
Reviewed By: epriestley
CC: epriestley, aran, jungejason, tuomaspelkonen, jonathanhester
Differential Revision: 1190
Test Plan: Commit as not the author and see what shows up.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, mareksapota, zeeg
Differential Revision: 1195
Summary:
use the handy DifferentialChangesetParser to do most of the heavy lifting inside
the pertinent view object. update the controller to be aware of the "show
more" calls coming from the new ui and update the transactionID appropriately.
also snuck in a small change to AprontRequest to all getting all the request
data. I used it to debug building this.
Test Plan: made a task and entered a bunch of test data. had descriptions of
various lengths, as well as really long descriptions that i did not change to
much. verified the diff looked correct and various "show more" links worked as
expected
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, btrahan, epriestley
Differential Revision: 1187
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: There was some documentation for this but it was kind of buried in a
random, difficult-to-discover file. Separate it into its own file and link to it
from the previous location.
Test Plan: Regenerated documentation and read through it without catching
anything terrible.
Reviewers: btrahan
Reviewed By: btrahan
CC: zeeg, aran, btrahan
Maniphest Tasks: T643
Differential Revision: 1161
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
Summary:
If mails are not sent in UTF-8 we cannot just it verbatim, so we have to
encode it into UTF-8 if it is not the case. Mime headers use different
encodings like "quoted-printable", which we have to handle.
It looks like "Subject: =?iso-8859-1?Q?opr=E6t_s=E5_den_task?=", and can
be decoded by ##iconv_mime_decode##.
Furthermore the body of the email might be in various encodings as well,
which we attempt to pull from the content-type header of the plain text
part of the mail.
Test Plan:
Attempted receiving mails in a variety of flavors. These could be
converted to test-cases once i know if this is a sane solution. Got
expected results from mails sent with Windows-1252 and ISO-8859-1.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, davidreuss
Differential Revision: 1093
Summary:
This limits a maniphest task query to only contain certain ids set
by the tasks query parameter.
Test Plan:
none yet, i wrote this at a computer with no phabricator
install while bored and eating dinner.
Reviewers: skrul, epriestley
Reviewed By: epriestley
CC: aran, davidreuss, epriestley, skrul
Differential Revision: 1137
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: As of D1154, we don't need this anymore. See that change for context.
Test Plan: See D1154.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Differential Revision: 1155
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
Test Plan:
Go to any diff
Click on a line number with right mouse button
No dialog should be opened
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1150
Summary: This is sort of silly but maybe useful? The real problem is that there
are like 500k conduit call logs and the real solution to that is better
filtering options, but this seems sort of okay.
Test Plan: Used "[" and "]" to switch between pages on the conduit call log.
Reviewers: btrahan, jungejason, nh, aran
Reviewed By: nh
CC: aran, nh, epriestley
Differential Revision: 1145
Summary:
The conduit access to Differential kind of sucks and we want to break
back-compat in order to fix it (see D1114).
To make it easier to pull this off, I want to build out the Conduit logging a
bit so administrators can identify which users are making deprecated calls.
We should probably build a little more infrastructure around this too (API
versions?), but this is at least a reasonable step forward which gives us more
insight into the use of Conduit and more tools to smooth the deprecation
process.
This initial commit is super basic but the interface currently says "stuff",
I'll build this out a little more in a bit.
Test Plan: Looked at call logs.
Reviewers: btrahan, nh, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Differential Revision: 1144
Summary:
- Update documentation for changes in D1148.
- Link to Remarkup documentation from Maniphest.
- Support "Note:" syntax in Phabricator (previously, it was only supported in
Diviner, but I've found it pretty good and useful).
Test Plan: Regenerated and perused documentation; made a "NOTE:".
Reviewers: btrahan, broofa, fugalh, jungejason, nh, aran
Reviewed By: btrahan
CC: aran, btrahan
Differential Revision: 1149
