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
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
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: 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:
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
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
Summary: some ground work for T479
Test Plan:
called up a diff via the conduit api console
it had the right project name and did not error
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, btrahan, epriestley
Differential Revision: 1139
Summary: the tab is a bit silly right next to DIRECTORY
Test Plan:
viewed phabricator with an admin account
* looks good on load
* clicked Categories and Items; looked good
viewed phabricator with a non-admin account
* looks good on load
* nothing else to click in the header
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1131
Summary: See T547. One of these I just missed in D1000; the comment change just
makes it easier to audit use of hash functions by cleaning up "grep" output.
Test Plan: Ran isolation unit test.
Reviewers: btrahan, jungejason, nh, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 1124
Summary:
Added a new method differential.createcomment
Task ID: #752014
Test Plan:
I created a test diff and called this method via the conduit
from a client PHP script to add comments. I confirmed that
1) the comment appeared on the revision, 2) URLs within the
comment were turned into hyperlinks, and 3) Phabricator
sent a notification email to the people watching the test
diff.
Reviewers: nh, jungejason, epriestley
Reviewed By: nh
CC: aran, nh
Differential Revision: 1128
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: See T625. Facebook's REST-based MTA layer had a check for this so I
overlooked it in porting it out. We should not attempt to deliver email to
disabled users.
Test Plan:
Used MetaMTA console to send email to:
- No users: received "no To" exception.
- A disabled user: received "all To disabled" exception.
- A valid user: received email.
- A valid user and a disabled user: received email to valid user only.
(Note that you can't easily send to disabled users directly since they don't
appear in the typeahead, but you can prefill it and then disable the user by
hitting "Send".)
Reviewers: btrahan, jungejason, nh, tuomaspelkonen, aran
Reviewed By: aran
CC: skrul, aran, epriestley
Differential Revision: 1120
Summary: See T626. Use array_values() to discard keys, for consistency and so
this will always encode as a list (JSON array) over the wire.
Test Plan: Added and removed CCs from a task while calling maniphest.info on it;
CCs worked and I always received a list.
Reviewers: btrahan, jungejason, nh, tuomaspelkonen, aran
Reviewed By: aran
CC: skrul, aran, btrahan, epriestley
Differential Revision: 1118
Summary:
enable admin to delete user's herald rules. This is useful for
managing non-active users' rules. For example, ex-employees' rules. The
code change includes:
- Added a 'All' tab which is only accessible to admin.
- Refactor out a HeraldRuleListView which is used by both the home
controller and the all rule controller
Test Plan:
delete an ex-employee rule as an admin; disable myself as
admin and verified that I don't have access to view other user's rules
and I'am not be able to delete them; also verified that as a non-admin,
I can still view, create and delete my own rules.
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: aran, epriestley, jungejason
Differential Revision: 1064
Summary:
0d5b0f21ad added string conversion but MBString always needs an argument for endcoding.
It looks like we can get away with doing this in git instead, with the --encoding='UTF-8' flag. Then we should be safe to remove the test for output type, and stay UTF-8 safe.
Test Plan:
Run updaters with change. Verify commits are updated.
Reviewers: epriestley
CC:
Differential Revision: 1108
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:
- Add attachment support for SendGrid.
- Add attachment support to the MetaMTA test console.
Test Plan:
- Sent myself a file with Amazon SES via test console.
- Sent myself a file with SendGrid via test console.
Reviewers: mareksapota, jungejason, nh, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 1089
Summary: createrevision creates the revision as the user which certificate is
used. Add a meta parameter to API calls named actAsUser so one user can create
revisions for someone else. Right now there is no authentication.
Test Plan: Called createrevision with one users cert and set actAsUser to
someone else. The revision was created as the actAsUser user.
Reviewers: epriestley, nh, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1087
Summary: Provide a dirt-simple working example of client-side templating and
reactive programming.
Test Plan: Load the examples
Reviewers: epriestley, mroch, tomo
Reviewed By: epriestley
CC: ide, schrockn, aran, rzadorozny, epriestley
Differential Revision: 908
Summary:
I was reading herald code for a task and realized that the method was
really long. So I refactor it to shorter methods.
Test Plan:
was still able to create a differential rule and commit rule; and
verified that dry-run still worked.
Reviewers: epriestley, tuomaspelkonen
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1077
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:
the code tries to access 'encoding' property even when the
repository is empty. The fix is to set it to null in that case.
Test Plan: run the conduit method on my sandbox and it works now.
Reviewers: grglr, epriestley, nh
Reviewed By: grglr
CC: aran, grglr
Differential Revision: 1075
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
Summary:
Allow anonymous access to conduit getdiff method, which is needed for anonymous
`arc patch`.
Test Plan: Running getdiff with an unauthenticated conduit should work.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1068
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
Summary: .. IN (%Ls) with no file phids fails miserably.
Test Plan:
Went to /file/macro/ with and without patch. An exception is thrown
without it.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1063
Test Plan:
used the reparse.php script for reparsing commit messages and saw the
correct author name (and mapped correctly as a phabricator user) in diffusion.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1059
Summary: This should hopefully kill off the last of these :P
Test Plan: Should be self explanatory
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1043
Summary:
This adds an encoding detail to the repository, so we can attempt to
convert hunks previously detected as binary.
We also add the encoding information to the arcanist projectinfo
API so we can pull the information if we have it when uploading changes
via arc.
Test Plan:
Changed encoding through the edit UI, and saw "This is binary file", and
changed it back and saw the correct output from the diff.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1009
Test Plan:
Used the scripts/mail_handler.php with and without patch and saw
the maniphest task being created with patch applied.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, davidreuss
Differential Revision: 1041
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:
Discover commits then return; useful when initializing new repositories
in unit tests.
By which I mean "when initializing a new repository in my unit test that
I'm working on".
Test Plan: Using this in a PhabricatorTestCase.
Reviewers: epriestley, aran
Reviewed By: epriestley
CC: aran, edward, epriestley
Differential Revision: 948
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
Summary:
See T549. Under configurations where files are served from an alternate domain
which does not have cookie credentials, we use random keys to prevent browsing,
similar to how Facebook relies on pseudorandom information in image URIs (we
could some day go farther than this and generate file sessions on the alternate
domain or something, I guess).
Currently, we generate these random keys in a roundabout manner. Instead, use a
real entropy source and store the key on the object. This reduces the number of
sha1() calls in the codebase as per T547.
Test Plan: Ran upgrade scripts, verified database was populated correctly.
Configured alternate file domain, uploaded file, verified secret generated and
worked properly. Changed secret, was given 404.
Reviewers: jungejason, benmathews, nh, tuomaspelkonen, aran
Reviewed By: aran
CC: aran, epriestley
Differential Revision: 1036
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
without quotes around it (and similar)
Summary: See D1010. The API uniformly requires JSON, which is good for
strictness and predictablity but can be bad for UEX, especially considering that
we silently continue after failing to decode things. Toss the user a lifeline
when they make this common mistake.
Test Plan: Ran API calls with invalid and valid inputs. Invalid inputs gave me a
reasonable error message.
Reviewers: davidreuss, jungejason, nh, tuomaspelkonen, aran
Reviewed By: nh
CC: aran, nh
Differential Revision: 1012
Summary:
make sure all symboles can be loaded to avoid issues like missing
methods in descendants of abstract base class.
Test Plan:
ran it and verified it passes; remove a method in a descendant class
and verified that the test failed.
Reviewers: epriestley, nh
Reviewed By: nh
CC: aran, nh, jungejason
Differential Revision: 1023
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: Allow configuration of a default author for bugs@ emails which don't
correspond to a known system user.
Test Plan: Configured a default author, sent some mails from nonsense addresses,
tasks were created.
Reviewers: davidreuss, jungejason, nh, tuomaspelkonen, aran
Reviewed By: aran
CC: aran, epriestley, ide
Differential Revision: 1013
Summary: It looked stupid.
Test Plan: It looks better now and other options still work.
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, epriestley, tuomaspelkonen
Differential Revision: 1017
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:
Set 'metamta.send-immediately' to true. Start up several MTA daemons, without
the patch you'll probably get multiple emails, with the patch you should get
only one.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, mareksapota, epriestley
Differential Revision: 1021
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:
@danielraffel is reporting an issue with file attachments which I can't
reproduce, but I did find a couple of minor things.
- Elsewhere, we store array() as the value of these PHID dictionaries (the
idea being that we might store metadata there some day). While we may or may not
do this, we should at least be consistent.
- When you edit a task, there's a file upload control but it doesn't actually
do anything. Just don't show it, there's no real reason to have it there.
Test Plan: Created a new task with attached files, verified they encoded as "[]"
instead of "true" in the database. Edited a task and didn't get a file control.
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: aran
CC: danielraffel, aran
Differential Revision: 1003
function.
Summary: Fix PhabricatorMailImplementationPHPMailerLiteAdapter to actually use
given parameter.
Test Plan: Use setIsHTML with false as parameter, sent mail should be in plain
text.
Reviewers: jungejason
Reviewed By: jungejason
CC: aran, jungejason, epriestley
Differential Revision: 1001
Summary:
Clarified the instructions when editing a repository for the remote uri to
mention that a local path for the remote uri must be specified as
file:///local/path/to/repo instead of /local/path/to/repo. (The latter used to
work, but stopped functioning for new repositories as of D888.)
Test Plan:
loaded /repository/edit/NN/tracking (where NN is a repo number), and saw the
updated instruction message.
Reviewers: jungejason, epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1005
Summary: See T548 and D996. Makes Phabricator configure the remarkup engine so
http:// and https:// get linked. Also make the "named link" syntax respect the
whitelist.
Test Plan:
- Whitelisted URIs (they get linked).
- Other URIs (not linked).
- Whitelisted, named URIs (linked).
- Other, named URIs (treated as phriction links).
- Actual phriction links (work correctly).
Reviewers: jungejason, nh, tuomaspelkonen, aran, benmathews
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 997
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:
Change the differential typeahead to only load columns that it needs. To do
this, I also enabled partial objects for PhabricatorUser (and made necessary
changes to support this). I also changed the functionality of Lisk's loadColumns
to either accept columns as multiple string arguments or a single array of
strings.
Test Plan:
With tokenizer.ondemand set to false, checked that the typeahead loaded and I
can type multiple people's names. Set tokenizer.ondemand to true and tried
again. In both cases, the typeahead worked.
Reviewers: epriestley
Reviewed By: epriestley
CC: jungejason, aran, epriestley, nh
Differential Revision: 990
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:
this is a request from Facebook:
> someone has added macros for common words like 'push', 'lgtm'.
> We want to let he image macro page at least attribute to an owner so
> that we can publicly shame whoever added the 800px 'clowntown' macro.
Test Plan:
test macros with/without author (all macro should have
author. This is just to be safe).
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: hwang, aran, arice, jungejason, epriestley
Differential Revision: 984
Summary:
This is an attempt to satisfy a lot of the one-off requests a little more
generally, by providing a relatively generic piece of event architecture.
Allow the registation of event listeners which can react to various application
events (currently, task editing).
I'll doc this a bit better but I wanted to see if anyone had massive objections
to doing this or the broad approach. The specific problem I want to address is
that one client wants to do a bunch of routing for tasks via email, so it's
either build a hook, or have them override most of ManiphestReplyHandler, or
something slightly more general like this.
Test Plan: Wrote a silly listener that adds "Quack!" to a task every time it is
edited and edited some tasks. I was justly rewarded.
Reviewers: nh, jungejason, tuomaspelkonen, aran
Reviewed By: aran
CC: aran, epriestley
Differential Revision: 881
Summary: See T507. Since you can't do "xxxxxxxx^" where "xxxxxxxx" is the first
commit in a repository, fall back to diffing against the empty tree if we fail
to diff against the parent commit.
Test Plan: Looked at the first commit in libphutil on my local.
Reviewers: edward, jungejason, nh, tuomaspelkonen, aran
Reviewed By: nh
CC: aran, edward, epriestley, nh
Differential Revision: 953
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:
We need to issue all commands as $repository->junk() so we can pick up
credentials. Some of this stuff predates that change landing.
(I removed the "https" vs "svn+ssh" fallback code since it's specific to
Facebook, affected a tiny number of commits, is basically an SVN bug with UTF-8
handling and HTTP support, and doesn't make sense in the general case. The user
has the tools they need to force it via "reparse.php" if it's really an issue.)
Test Plan: Created new authenticated-remote mercurial and git repositories and
pulled/discovered them with credentials.
Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran
Reviewed By: Makinde
CC: aran, Makinde
Differential Revision: 970
Summary: Change import script plus almost all the view stuff. Still some rough
edges but this seems to mostly work. Blame is currently unsupported but I think
everything else works properly.
Test Plan:
Imported the hg repository itself. It doesn't immediately seem completely
broken. Here are some screens:
https://secure.phabricator.com/file/view/PHID-FILE-1438b71cc7c4a2eb4569/https://secure.phabricator.com/file/view/PHID-FILE-3cec4f72f39e7de2d041/https://secure.phabricator.com/file/view/PHID-FILE-2ea4883f160e8e5098f9/https://secure.phabricator.com/file/view/PHID-FILE-35f751a36ebf65399ade/
All the parsers were able to churn through it without errors.
Ran the new "reparse.php" script in various one-commit and repository modes.
Browsed/imported some git repos for good measure.
NOTE: The hg repository is only 15,000 commits and around 1,000 files.
Performance is okay but hg doesn't provide performant, native APIs to get some
data efficiently so we have to do some dumb stuff. If some of these interfaces
are cripplingly slow or whatever, let me know and we can start bundling some
Mercurial extensions with Arcanist.
Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran
Reviewed By: Makinde
CC: aran, Makinde, epriestley
Differential Revision: 960
Summary:
Splitting up D960 a bit, see that for context.
We currently have two scripts, "parse_one_commit.php" and
"reparse_all_commit_messages.php", but they're sort of silly and you can't do
certain things with them. Replace them with one script which is more flexible
and can do specific reparse steps on individual commits or entire repos.
I left the old scripts as stubs since I think there are some FB wiki docs and
stuff that mention them. I'll delete them in a month or whenever I remember or
something.
Test Plan: Ran "reparse.php" with various arguments, including vs-one-commit,
vs-repository, with --trace, and against different types of repos.
Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 964
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:
Just breaking D960 into some smaller parts, this is a standalone method used in
Mercurial parsing.
(There's a bad version of this function in the SVN stuff but I'll get rid of it
the next time I'm in there.)
Test Plan: See D960.
Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran
Reviewed By: Makinde
CC: aran, Makinde
Differential Revision: 965
Summary:
When we discover a new commit and it has a known local commit or tree hash, mark
it committed.
This supports Mercurial and Git-Immutable workflows, and improves
hybrid-Git-Mutable workflows and covers some cases where poeple just make
mistakes or whatever.
Test Plan: Parsed Mercurial, Git and SVN commits.
Reviewers: Makinde
Reviewed By: Makinde
CC: aran, Makinde
Differential Revision: 963
Summary: See T502. Under some VCS setups, we get full email addresses instead of
usernames or real names. Try harder to find matches, by falling back to email
address parsing if we don't get hits on the straight-up token parsing.
Test Plan:
This is difficult to test because it depends on the account state and repository
state, and hard to pull out so it's more testable without better mocking
facilities. I just dumped this into the parser to verify the behavior:
foreach (array(
'epriestley',
'Evan Priestley',
'epriestley@epriestley.com',
'derp <epriestley@epriestley.com>',
'"Evan Priestley" <derpderpderp@derpderpderp.com>',
'quackderp <derpderpderp@derpderpderp.com>',
) as $email) {
echo "{$email} = ".$this->resolveUserPHID($email)."\n";
}
die();
Running PhabricatorRepositoryGitCommitMessageParserWorker...
epriestley = PHID-USER-79f25616ea2635089a31
Evan Priestley = PHID-USER-79f25616ea2635089a31
epriestley@epriestley.com = PHID-USER-1bec59b91be6223f07fd
derp <epriestley@epriestley.com> = PHID-USER-1bec59b91be6223f07fd
"Evan Priestley" <derpderpderp@derpderpderp.com> =
PHID-USER-79f25616ea2635089a31
quackderp <derpderpderp@derpderpderp.com> =
This is expected (all variations of my identity parsed correctly, and the bogus
one failed). There are two different user PHIDs in the result set because I have
like 30 different similar accounts on my local, including one called "derp" and
another one with address "derp@derp.com", which prevented an earlier version of
this test case from working correctly.
Reviewers: zachallia, aran, Makinde, jungejason, nh, tuomaspelkonen
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 968
datasources
Summary:
The open source Phabricator has like 3,500 user accounts now and it takes a
while to pull/render them. Add an option to switch to ondemand for large
installs.
I'll follow up with a patch at some point to address a couple of name things:
- Denormalize last names into a keyed column (although this evidences some
bias toward the western world).
- Force all usernames to lowercase (sorry Girish, Makinde).
Also this patch is so clean it's crazy.
Didn't bother with other object types for now, I'm planning to dedicate a few
days to Projects at some point and I'll flesh out some auxiliary features like
this when I do that.
Test Plan: Switched to ondemand, verified data was queried dynamically. Switched
back, verified data was preloaded.
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: nh
CC: aran, epriestley, nh
Differential Revision: 923
Summary:
We currently rely on "remote_hooks_enabled" in .arcconfig to determine whether
commands like "arc amend" and "arc merge" should imply "arc mark-committed".
However, this is a historical artifact that is now bad for a bunch of reasons:
- The option name is confusing, it really means 'repository is tracked'.
- The option is hard to discover and generally sucks.
- We can empirically determine the right answer since we now know if a project
is in a tracked repository.
Add a call which arcanist can make on these workflows to figure out if it is
interacting with a project in a tracked repository or not.
Also added an "isTracked()" convenience method to reduce the number of magic
strings all over the place.
Test Plan: Ran "arcanist.projectinfo" for nonexistent, untracked and tracked
projects.
Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran
Reviewed By: Makinde
CC: aran, epriestley, Makinde
Differential Revision: 945
Summary:
See D943, this is the second parse stage. This will mark Differential revisions
as "Committed" among other things.
Almost all the logic here is shared between VCSes so the implementation itself
is straightforward.
Test Plan: Parsed all messages for the official Mercurial repository.
Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran
Reviewed By: Makinde
CC: aran, Makinde
Differential Revision: 944
Summary:
Repository import has three major steps:
- Commit discovery (serial)
- Message parsing (parallel, mostly VCS independent)
- Change parsing (parallel, highly VCS dependent)
This implements commit discovery for Mercurial, similar to git's parsing:
- List the heads of all the branches.
- If we haven't already discovered them, follow them back to their roots (or
the first commit we have discovered).
- Import all the newly discovered commits, oldest first.
This is a little complicated but it ensures we discover commits in depth order,
so the discovery process is robust against interruption/failure. If we just
inserted commits as we went, we might read the tip, insert it, and then crash.
When we ran again, we'd think we had already discovered commits older than HEAD.
This also allows later stages to rely on being able to find Phabricator commit
IDs which correspond to parent commits.
NOTE: This importer is fairly slow because "hg" has a large startup time
(compare "hg --version" to "git --version" and "svn --version"; on my machine,
hg has 60ms of overhead for any command) and we need to run many commands (see
the whole "hg id" mess). You can expect something like 10,000 per hour, which
means you may need to run overnight to discover a large repository (IIRC, the
svn/git discovery processes are both about an order of magnitude faster). We
could improve this with batching, but I want to keep it as simple as possible
for now.
Test Plan: Discovered all the commits in the main Mercurial repository,
http://selenic.com/repo/hg.
Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran
Reviewed By: Makinde
CC: aran, Makinde
Differential Revision: 943
Summary:
dirname('x') returns '.', not '/'; this caused some issues for repositories with
files at the root.
There are some cases in the parsers where I should probably swap this out too
but I'll wait until I'm doing some more rigorous testing since that stuff is a
bit fragile and this fixes an immediate issue.
Test Plan: Ran unit tests. Viewed a file at root level in a test repository.
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: nh
CC: aran, nh
Differential Revision: 932
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: This method relies on 64-bit math being available, which isn't a safe
assumption. Use the builtin bc functions instead for arbitrarily large integers.
Test Plan: @skrul, can you apply this locally and let me know if it works?
Reviewers: skrul, hunterbridges, jungejason, nh, tuomaspelkonen, aran
Reviewed By: skrul
CC: aran, skrul, epriestley
Differential Revision: 912
Summary:
This is pretty straightforward, except:
- We need to request read/write access to the address book to get the account
ID (which we MUST have) and real name, email and account name (which we'd like
to have). This is way more access than we should need, but there's apparently no
"get_loggedin_user_basic_information" type of call in the Google API suite (or,
at least, I couldn't find one).
- We can't get the profile picture or profile URI since there's no Plus API
access and Google users don't have meaningful public pages otherwise.
- Google doesn't save the fact that you've authorized the app, so every time
you want to login you need to reaffirm that you want to give us silly amounts of
access. Phabricator sessions are pretty long-duration though so this shouldn't
be a major issue.
Test Plan:
- Registered, logged out, and logged in with Google.
- Registered, logged out, and logged in with Facebook / Github to make sure I
didn't break anything.
- Linked / unlinked Google accounts.
Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran
Reviewed By: aran
CC: aran, epriestley, Makinde
Differential Revision: 916
Summary: No actual parsing/import yet, but now you can define and pull Mercurial
repositories. I merged most of the local pull code so we can share it between
hg/git.
Test Plan:
- Created a new Mercurial repository to track Codeigniter off Bitbucket
- Edited / saved / etc.
- Launched the mercurial pull daemon, it pulled the repo. Killed and
relaunched, it updated the repo.
- Launched the git fetch deamon, it still works correctly.
Reviewers: Makinde, aran, jungejason, tuomaspelkonen
Reviewed By: Makinde
CC: aran, Makinde
Differential Revision: 793
Summary:
Changed the documentation to describe the project-subproject join table
instead of the task-project join table.
Test Plan:
none
Reviewers:
epriestley, cadamo
CC:
Differential Revision: 927
Summary:
The CSRF changes meant that we can't generate a file URI with just its PHID
anymore, and converted a mathematical function into a service call.
Unfortunately, this caused massive perf problems in some parts of the
application, critically handles, where loading N users became N single gets.
Derp derp derp. Remedy this by doing a single multiget. This substantially
improves performance of many interfaces, particularly the Maniphest task list.
I need to go through the rest of the PhabricatorFileURI callsites and get rid of
them, but I think this is the most substantive one.
Test Plan: Profiled Maniphest task list, queries went from >100 to a handful.
Explosion of multiderp. :/ Looked at some views with profile photos to verify
they still render accurately.
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: aran
CC: aran
Differential Revision: 921
Summary: This will get fancier, but here's a basic interface for doing symbol
lookups. Still all pretty tentative.
Test Plan: Looked up various things, got some sensible results.
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: tuomaspelkonen
CC: aran, tuomaspelkonen
Differential Revision: 900
Summary: See T315 for an extensive description of this feature. Adds the
descibed storage table.
Test Plan: Used phpsh to read/write symbol objects.
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: tuomaspelkonen
CC: aran, epriestley, tuomaspelkonen
Differential Revision: 897
Summary: This got caught in the CSRF filter but is a safe write.
Test Plan: Pasted the URI for a picture of a goat into a diff, saw a goat.
Reviewers: aran, jungejason
Reviewed By: aran
CC: aran
Differential Revision: 910
Summary: See D902. As @abdul notes, a password input is probably more
appropraite here.
Test Plan: Mashed stuff into it, got bullets instead of text.
Reviewers: abdul, jungejason, nh, tuomaspelkonen, aran
Reviewed By: nh
CC: aran, nh
Differential Revision: 913
Summary: phriction.edit allows you to omit the content string, meaning "don't
edit content". If you're also creating the page, we currently break in a
terrible, horrible, no-good, very-bad way because 'content' can't be null.
Default to empty string instead so phriction.edit creates an empty page instead
of a broken mess.
Test Plan: Called phriction.edit on a new page with no content.
Reviewers: skrul, jungejason, nh, tuomaspelkonen, aran
Reviewed By: skrul
CC: aran, skrul
Differential Revision: 920
Summary:
When Diffusion encounters an image file, it displays it as an
image, but when it encounters a PDF file, it currently shows only some
gibberish. This fixes that.
Test Plan:
I tried it. Embedding a large PDF in a data URL is a little
bit slow, but it works.
Reviewers: tuomaspelkonen, epriestley, gc3, waltermundt, jungejason, nh
Reviewed By: epriestley
CC: aran, tuomaspelkonen, epriestley, jaapweel
Differential Revision: 915
Summary:
We need to query the unit status in order to determine if
there are postponed unit tests to update after running "arc unit"
Test Plan:
1) set my conduit uri to a server running the new code
2) ensured unitStatus existed when retrieving a diff
Reviewers: epriestley
Reviewed By: epriestley
CC: dpepper, aran, epriestley
Differential Revision: 918
Summary:
@tomo ran into an issue where he had some non-SSL-only cookie or whatever, so
"Logout" had no apparent effect. Make sure "Logout" really works by destroying
the session.
I originally kept the sessions around to be able to debug session stuff, but we
have a fairly good session log now and no reprorted session bugs except for all
the cookie stuff. It's also slightly more secure to actually destroy sessions,
since it means "logout" breaks any cookies that attackers somehow stole (e.g.,
by reading your requests off a public wifi network).
Test Plan: Commented out the cookie clear and logged out. I was logged out and
given a useful error message about clearing my cookies.
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: aran
CC: tomo, aran, epriestley
Differential Revision: 911
- Removed irrelevant csprintf(..)
- Updated code to use $repository->getRemoteURI()
- Updated code to use getRemoteCommandFuture(..) in Diffusion code
- Updated code to use $repository->getRemoteURI()
Summary: I still need to go through all the daemon and Diffusion code and change
the bare execx() calls to $repository->execxXXX() to actually make this work,
but we're getting close.
Test Plan: Configured repositories with various HTTP / SVN setups and ran the
test_connection.php script to verify keys were located and added and
username/password information was supplied.
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: nh
CC: aran, nh, jungejason
Differential Revision: 902
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:
Move toward storing credentials in configuration so it's easier to get the
daemons working. This should eventually solve all the key juggling junk you have
to do right now.
This only gets us part of the way to actually using these credentials in the
daemons since I have to go swap everything for $repository->execBlah().
I tried to write a web "Test Connection" button but it was too much of a mess to
get git to work since git doesn't give you access to its SSH command and SSH has
a bunch of interactive prompts which you can't really do anything about without
it or a bunch of ~/.ssh/config editing. This is what Git recommends:
https://git.wiki.kernel.org/index.php/GitFaq#How_do_I_specify_what_ssh_key_git_should_use.3F
..but it's not a great match for this use case.
Test Plan:
- Only partial.
- Ran "test_connection.php" on a Git repo with and without SSH, and with and
without valid credentials. This part works properly.
- Ran "test_connection.php" on a public SVN repo, but I don't have private or
WEBDAV repos set up at the moment.
- Mercurial doesn't work yet.
- Daemons haven't been converted yet.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, abdul, nmalcolm, epriestley, jungejason
Differential Revision: 888
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
Summary:
Give users better errors and UI:
- For subpath SVN repositories, default the path to the subdirectory, not to
"/". This makes the home screen useful and things generally less confusing.
- For unparsed commits, show a more descriptive error message without the
"blah blah" silliness.
- For paths outside of the subpath parse tree, short circuit into an
appropriate error message.
- For foreign SVN stub commits (see D892), show an explicit message.
Test Plan: Looked at unparsed commits, subpath repositories, foreign stub
commits, and paths outside of the subpath parse tree. Received sensible error
messages.
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 894
Summary:
In password-based auth environments, there is now a user settings
panel to allow them to change their password.
Test Plan:
Click settings, choose password from the left:
* enter current password, new password (twice), log out, and log in with
new password
* enter current password, non-matching passwords, and get error
* enter invalid old password, and get error
* use firebug to change csrf token and verify that it does not save with
and invalid token
Changed config to disable password auth, loaded settings panel and saw
that password was no longer visible. Tried loading the panel anyway and
got redirected.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 890
references
Summary:
See T325. We tentatively support doing partial subdirectory parses in
Phabricator for Subversion, so you can elect to import only "trunk/local/" or
similar. We do this by importing only some of the commits (those commits which
affected that directory).
In Subversion, you can also "svn cp
svn+ssh://example.com/svnroot/trunk/foreign/example.c@13 local.c". This means
that commits which reference "trunk/local/" may themselves reference foreign
commits.
Currently, we break in this case and can't find the commit reference. Instead,
generate a foreign commit stub so we can at least point at some reasonable
object.
Test Plan: Successfully imported trunk/a/ of the test repo in T325 without
errors. Verified commit 3 in that repo is imported as a foreign stub.
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason, epriestley
Differential Revision: 892
repositories
Summary:
This query isn't scoped correctly to the repository ID, so we may identify
commits from other repositories.
This causes a somewhat subtle issue since we only use it to manage file
copies/moves, so you end up with a file "copied from" the same revision in
another repository. I think the UI probably even renders correctly.
Once I finish T325 and better understand what's going on here, I'll see how much
work is involved in writing an SQL patch to fix this.
Test Plan: Parsed the test repo from T325 with the expected error.
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 891
Summary:
oh god everyone hates this
revert revert
https://www.facebook.com/photo.php?fbid=787360256660&set=p.787360256660&type=1&theater
(I left the icons themselves since I have some plans to do other things with
them.)
Test Plan: I am not good at designer
Reviewers: ola, elynde, bh, ashwin, jungejason, kdelong, zrait, tomo, aran
Reviewed By: aran
CC: aran, epriestley, tomo
Differential Revision: 885
Summary:
I didn't realize createDiffDict was a public method when I
modified it, and I broke the API call in getrevision. This moves the
modification inside the method and reverts the method header back to
it's original form.
Test Plan: none
Reviewers: epriestley
Reviewed By: epriestley
CC: edward, aran, epriestley
Differential Revision: 883
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:
Provide three Phriction methods for programmatic access to Phriction:
- phriction.info: get information about a document
- phriction.history: get change history for a document
- phriction.edit: create or update a document
I moved all the editing/creation logic into a new PhrictionDocumentEditor to
share code between the Conduit and Web edit pathways.
Test Plan: Got info and history via conduit. Edited and created new pages via
conduit and web.
Reviewers: hsb, jungejason, tuomaspelkonen, aran, hunterbridges
Reviewed By: hunterbridges
CC: skrul, aran, hunterbridges
Differential Revision: 866
Summary:
We need createlintresults because we are doing extended
static analysis offline, and thus we need to be able to update the
lint results associated with a diff. This is similar to
updateunitresults, but "create" is more accurate than "update" since
we never need to change existing lint results.
getdiffproperty is used by the client to ensure it isn't creating any
duplicates lint results. It's the symmetric operation to
setdiffproperty, which already exists.
Test Plan:
We have a new offline linter that I used to test. This
linter calls getdiffproperty on every run.
1. Tested updating an existing set of lint results by first running
"arc diff" with lint errors caught by the local linter, then later
running offline analysis which catches one other error and updates via
createlintresults. Ensured the differential lint results were as
expected.
2. Tested the creation of an entirely new diff property through
createlintresults. I first ran "arc diff --nolint" to skip all lint
results, then ran offline analysis which caught an error and updated
through createlintresults. Ensured differential lint results were as
expected.
Reviewers: epriestley
Reviewed By: epriestley
CC: dpepper, aran, mgummelt, jungejason, epriestley
Differential Revision: 868
