Summary:
Git accepts either "git@x:/path" or "ssh://git@x.com/path" URIs to mean the
exact same thing, which is causing some false positives and confusion,
particularly because we sometimes mutate URIs.
Since this is just a sanity check, we don't really care about the username,
domain or credentials -- matching the paths is good enough. We're just trying to
make it hard to shoot yourself in the foot by copy-pasting the same local path
into two repositories and forgetting to change one, like I did. :P
Relax the check to only verify the paths are the same.
Test Plan:
- Ran unit tests, which should fully cover things.
- Ran commit discovery daemon in debug mode on incorrectly and correctly
configured repositories.
Reviewers: ajtrichards, jungejason, btrahan
Reviewed By: jungejason
CC: aran, jungejason
Maniphest Tasks: T710
Differential Revision: https://secure.phabricator.com/D1279
layout
Summary:
- Use new less-horrible layout.
- Organize information more completely and sensibly.
Test Plan: Looked at some profiles.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: https://secure.phabricator.com/D1281
Summary:
It used to be more useful for daemons to spew random debugging information, but
features like "phd debug" and some fixes to error reporting like D1101 provide
better ways to debug, test, develop and diagnose daemons.
- Stop writing "." every time MetaMTA sends a message.
- Stop spewing the entire IRC protocol from the IRC bot unless in debug mode.
- Stop writing GC daemon log entries about collecting daemon logs (DURRR)
unless in debug mode.
Test Plan: Ran daemons in debug and non-debug modes, got expected level of
noisiness.
Reviewers: jungejason, nh, btrahan
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: https://secure.phabricator.com/D1268
Summary: There can be Dxxx, rXXXxxx or even full URL in //Blame Revision// field
so just highlighting it as normal text would work probably best
Test Plan:
Go to https://secure.phabricator.com/D277
You should see a link from //Blame Revision// (if it would be displayed)
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1274
Summary:
when a path is '/' in defining a package, D1251 is generating
an extra '//'.
Test Plan: veryfied adding path '/', '/src' and '/src/' all worked.
Reviewers: nh, epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1266
Summary: I think we only hit this because I mucked around with the database to
recover from the runaway parse of the Diviner repository (now prevented by
D1253), but be more robust against missing data in this interface.
Test Plan: After applying this patch, no longer received a fatal on the commit
history page for users linked to nonexistant/bogus commits.
Reviewers: jack, btrahan, jungejason, aran
Reviewed By: aran
CC: aran
Maniphest Tasks: T701
Differential Revision: https://secure.phabricator.com/D1264
- Use the computed remote URI (which may have an explicit 'ssh://' under Git in some cases).
- Use '$id' correctly rather than casting the URI to an int in the message parser.
Summary: I want to add a command like "where is ArcanistUnitTestEngine" to
phabot. I also want to add a symbol typeahead to Diffusion and generally finish
up that feature since it's useful but only half-implemented. Consolidate the
query logic and expose the data over Conduit.
Test Plan: Used /symbol/ and Conduit to lookup symbols.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, jungejason
Maniphest Tasks: T315
Differential Revision: https://secure.phabricator.com/D1260
Summary: See D1257. Also make the error message more friendly, and remove a very
very old Facebook-specific error.
Test Plan:
- Tried to diff with an older arc.
- Tried to diff with a newer arc.
- Diffed with the right arc.
Reviewers: btrahan, jungejason, aran
Reviewed By: aran
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1258
Summary:
- Previously, used IDs like "33" to match a commit to a Differential revision.
This has a namespacing problem because we now have an arbitrarily large number
of Phabricator installs in the world, and they may want to track commits from
other installs.
- In Differential, parse raw IDs or full URIs. Emit only full URIs.
- In Repositories, parse only full URIs.
- This might cause a few commits to not be picked up in rare circumstances.
Users can fix them with "arc mark-committed". This should be exceedingly rare
because of hash matching.
- There are some caveats for reparsing older repositories, see comments
inline. I don't think there's much broad impact here.
Test Plan:
- Created a new revision, got a full URI.
- Updated revision, worked correctly.
- Ran unit tests.
- Monkeyed with "Differential Revision" field.
- Reviewers: btrahan, jungejason
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, epriestley, jungejason
Maniphest Tasks: T54, T692
Differential Revision: 1250
Summary:
Although I couldn't repro the issue in T692, I did manage to point the "Diviner"
repository at the "Phabricator" working copy and screw some stuff up on
secure.phabricator.com.
Before discovering commits in a repository, ensure the 'origin' remote points at
the configured URI. This prevents issues where the working copy gets configured
to point at an existing (but incorrect) checkout.
Test Plan:
- Ran gitcommitdiscovery daemon normally under "phd debug", saw it execute the
"remote show -n" command and then start working.
- Intentionally botched the config, got an exception:
(Exception) Working copy '/INSECURE/repos/phabricator' has origin URL
'ssh://git@github.com/facebook/phabricator.git', but the configured URL
'git://github.com/facebook/diviner.git' is expected. Refusing to proceed.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, jungejason
Maniphest Tasks: T692
Differential Revision: 1253
Summary:
Allow entry of "CC: alincoln" to match user "ALincoln".
Put both variations in the map and try the exact case version first since we'll
also match email addresses and mailables, and theoretically some mailable might
have the same name as a user, as we're effectively abandoning restriction of
which characters can appear in usernames.
Test Plan: Created a local revision with a reviewer in CrAzY CaPs.
Reviewers: jungejason, btrahan
Reviewed By: jungejason
CC: aran, jungejason
Maniphest Tasks: T697
Differential Revision: 1255
Summary:
Paths in owners packages when referring to a directory should always end with
a trailing slash. (Otherwise, some things break, like loading the owning
packages for a path.) With this change, PhabricatorOwnersPackage now requires
that the path provided for a package is valid, and if the path is for a
directory, it adds a trailing slash if one was not provided.
Test Plan:
Edited a path in a package and left off the trailing slash. Saw that the slash
was added. Tried again with the trailing slash, and checked that another slash
was not added. Did this with a path in both a git and svn repository.
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1251
Summary:
- Old page was useless and dumb.
- New page looks a little less bad, functions a little less poorly.
- Still lots of work to be done.
Test Plan:
- Viewed a project.
- Clicked all the links on the left nav.
- Here is a screenshot:
https://secure.phabricator.com/file/view/PHID-FILE-4buzquotb3fo4dhlicrw/
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, jungejason
Maniphest Tasks: T681
Differential Revision: 1246
Summary: Share more code; reduce the number of ad-hoc versions of this rendering
loop.
Test Plan: Clicked all the filters.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 1247
Summary:
Provide tighter integration between Projects and Phriction. Partly, I have most
of a rewrite for the Projects homepage ready but it's not currently possible to
publish feed stories about a project so all the feeds are empty/boring. This
partly makes them more useful and partly just provides a tool integration point.
- When you create a project, all the wiki pages in projects/<project_name>/*
are associated with it.
- Publish updates to those pages as being related to the project so they'll
show up in project feeds.
- Show a project link on those pages.
This is very "convention over configuration" but I think it's the right
approach. We could provide some sort of, like, "@project=derp" tag to let you
associated arbitrary pages to projects later, but just letting you move pages is
probably far better.
Test Plan:
- Ran upgrade scripts against stupidly named projects ("der", " der", " der
", "der (2)", " der (2) (2)", etc). Ended up with uniquely named projects.
- Ran unit tests.
- Created /projects/ wiki documents and made sure they displayed correctly.
- Verified feed stories publish as project-related.
- Edited projects, including perfomring a name-colliding edit.
- Created projects, including performing a name-colliding create.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley, btrahan
Maniphest Tasks: T681
Differential Revision: 1231
Summary:
add basic auditing functionalities. For the related commits for a
package, we detect the following conditions which might be suspicious to the
owners of the package:
* no revision specified
* revision not found
* author not match
* reviewedby not match
* owners not involved
* commit author not recognized
The owners of the package can change the status of the audit entries by
accepting it or specify concern.
The owner can turn on/off the auditing for a package.
Test Plan:
* verified that non-owner cannot see the details of the audit and cannot modify
it
* verified that all the audit reasons can be detected
* tested dropdown filtering and package search
* verified really normal change not detected
* verified accept/concern a commit
* tested enable/disable a package for auditing
* verified one audit applies to all <commit, packages> to the packages the
auditor owns
* verified that re-parsing a commit won't have effect if there exists a
relationship for <commit, package> already
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: aran, benmathews, btrahan, mpodobnik, prithvi, TomL, epriestley
Differential Revision: 1242
Summary: This was well-intentioned but has not actually proven to be useful.
Test Plan:
- No list tab shows up anymore.
- Looked up a PHID.
Reviewers: btrahan, jungejason, Girish
Reviewed By: Girish
CC: aran, jungejason, edward, emiraga, Girish, nh, tuomaspelkonen, epriestley
Maniphest Tasks: T631
Differential Revision: 1234
Summary: I didn't get around to this earlier; add Feed/Maniphest integration.
This is partly motivated by wanting Projects to not be terrible. Pretty
straightforward.
Test Plan:
- Created, updated, reassigned and closed a task.
- Verified feed stories render reasonably.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T681
Differential Revision: 1232
Summary: These are "local" commands, but need remote credentials. If the daemon
runs as a user who does not have credentials, the initial clone will work but
subsequent updates will fail.
Test Plan:
- Nuked a local copy of a Git repo.
- Ran "phd debug fetch <phid>" as root (or any other user with no natural SSH
keys). Verified initial clone worked (since it passes credentials to the command
correctly).
- Killed daemon, re-ran, verified "fetch" failed (no credentials passed).
- Applied this patch.
- Re-ran "phd debug fetch <phid>", verified it passed credentials and
succeeded.
- Did all these steps for a Mercurial repo.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T686
Differential Revision: 1236
Summary:
I pretty much copy/pasted this code; rather than do that again now that I want
to add feeds to projects, share the code.
This "Builder" is a little weird -- I don't want to call it a "View" because it
does data access. "Builder" seemed okay. We don't really have much code that
does this sort of thing right now, elsewhere.
Test Plan:
- Viewed public feed.
- Viewed private feed.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T681
Differential Revision: 1233
Summary:
- For context, see T547. This is the last (maybe?) in a series of diffs that
moves us off raw sha1() calls in order to make it easier to audit the codebase
for correct use of hash functions.
- This breaks CSRF tokens. Any open forms will generate an error when
submitted, so maybe upgrade off-peak.
- We now generate HMAC mail keys but accept MAC or HMAC. In a few months, we
can remove the MAC version.
- The only remaining callsite is Conduit. We can't use HMAC since Arcanist
would need to know the key. {T550} provides a better solution to this, anyway.
Test Plan:
- Verified CSRF tokens generate properly.
- Manually changed CSRF to an incorrect value and got an error.
- Verified mail generates with a new mail hash.
- Verified Phabricator accepts both old and new mail hashes.
- Verified Phabricator rejects bad mail hashes.
- Checked user log, things look OK.
Reviewers: btrahan, jungejason, benmathews
Reviewed By: btrahan
CC: aran, epriestley, btrahan
Maniphest Tasks: T547
Differential Revision: 1237
Summary:
Outlook wraps a message in 5 dashes on each side when doing replies.
This strips english and danish versions.
Test Plan:
Tried parsing emails with different messages and saw the
expected behaviour with patch applied. Ran arc unit, and saw test
passed.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1239
Summary:
When changing attachments, the removed part is squished together
with the added parts, making it hard to read. This changes the output
so it looks like other changes, seperating each action by a semicolon.
Test Plan:
Viewed a task where i had attached and deleted revisions, and
saw the output look as other changes of same type.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1238
Summary:
- Add a "delete" operation. Delete is just a special edit which removes the
page from indexes and shows a notice that the document has been deleted.
- When a user deletes all the content on a page, treat it as a delete.
- When a conduit call deletes all the content on a page, treat it as a delete.
- Add page status to Conduit.
- Add change type field to history.
- Added a couple of constants to support a future 'move' change, which would
move content from one document to another.
Test Plan:
- Verified deleted pages vanish from the document index (and restoring them
puts them back).
- Verified deleted pages show "This page has been deleted...".
- Created, edited and deleted a document via Conduit.
- Deleted pages via "delete" button.
- Deleted pages via editing content to nothing.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: skrul, aran, btrahan, epriestley
Maniphest Tasks: T680
Differential Revision: 1230
Summary: Remove a bunch of relatively useless stuff from the Project list
interfaces.
Test Plan: Looked at project lists, less random busy junk.
Reviewers: btrahan, jungejason, zeeg
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T681
Differential Revision: 1229
Summary: Add 'addLines' and 'delLines' properties to differential.getdiff return
dictionary. These properties are aggregated from the changesets.
Test Plan: Issue a differential.getdiff query via conduit and verify that
'addLines' and 'removeLines' properties are included and accurate
Reviewers: epriestley
Reviewed By: epriestley
CC: epriestley, aran, jonathanhester
Differential Revision: 1209
Summary:
This is a needlessly confusing/complex feature that I originally wrote sort of
speculativley. I think we can better serve what little need may exist here with
project feeds.
I'm probably going to get rid of or deemphasize "role" too and just add "Join
Project" and "Leave Project" buttons.
Test Plan: Viewed project list, project profile. Edited project profile and
affiliation.
Reviewers: btrahan, jungejason, zeeg
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T681
Differential Revision: 1228
Summary:
- Allow more than the 100 most recent projects to be viewed.
- Provide some useful filters.
- Default the view to your projects, not all projects.
- Put query logic in a query object.
- Put filter view logic in a view object. We can port more stuff to it later.
Test Plan: Looked at active/owned/all projects. Set page size to 5 and paged
through projects.
Reviewers: btrahan, jungejason, zeeg
Reviewed By: btrahan
CC: aran, btrahan
Differential Revision: 1227
Summary:
We have this code in two places; split it into an editor class so we can share
it.
This also fixes some probems with this field not //detaching// tasks properly.
Test Plan:
- Created a revision with no attached tasks.
- Attached it to a task.
- Updated it.
- Detached it.
- Used web UI to attach/detach tasks/revisions.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan, epriestley
Differential Revision: 1225
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:
...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
Summary: Oops, I left this in from an earlier version and missed it since I was
mostly looking at Maniphest for testing. We already render this information in
the header, don't additionally render it under the comments.
Test Plan: derp derp, loaded any revision with sourced comments
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 871
Summary: Execute searches like the primary Maniphest task list. Let me know what
else you guys need from this API.
Test Plan: Executed "maniphest.info" and "maniphest.find"
Reviewers: jungejason, tuomaspelkonen, aran, nh
Reviewed By: nh
CC: blair, skrul, aran, jungejason, epriestley, nh, tuomaspelkonen
Differential Revision: 867
Summary:
When an object is updated, record the content source for the update. This mostly
isn't terribly useful but one concrete thing I want to do with it is let admins
audit via-email replies more easily since there are a bunch of options which let
you do hyjinx if you intentionally configure them insecurely. I think having a
little more auditability around this feature is generally good. At some point
I'm going to turn this into a link admins can click to see details.
It also allows us to see how frequently different mechanisms are used, and lets
you see if someone is at their desk or on a mobile or whatever, at least
indirectly.
The "tablet" and "mobile" sources are currently unused but I figured I'd throw
them in anyway. SMS support should definitely happen at some point.
Not 100% sure about the design for this, I might change it to plain text at some
point.
Test Plan: Updated objects and saw update sources rendered.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, epriestley, jungejason
Differential Revision: 844
Summary: When a user stores the empty string in an auxiliary field, simply don't
store it, and delete it if it already exists.
Test Plan: Edited a revision with an empty "Quack" field, got an empty row in
the DB. Applied patch, edited empty again, row went away. Edited empty again,
still no row. Edited and put something in the field, got a row.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 865
Summary:
It is possible to view a comment that has no cache; when viewing such a comment
the request doesn't have a csrf token and there is no need for one, so we turn
off the write guard.
Test Plan:
loaded an old diff that had no cache, and the page loaded instead of throwing
an AphrontCSRFException.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 858
Summary: Quora wants to handle some moderation tasks with Phabricator, but want
to lower the barrier to entry for the install and let moderators adopt it
gradually. One request is to allow auth rules to be relaxed so we can auth based
on Reply-To to make things easier. This is insecure if configured but not really
a big deal and the patch isn't big or complicated.
Test Plan: Sent a test email with bogus "From" but valid "Reply-To". It was
rejected with this setting off, and allowed with this setting on.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 842
Summary: Delete one line which has no effect.
Test Plan: Open revision page to make sure it still works.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 852
Summary: Instead of just saying a task is "Closed", say "Resolved", "Wontfix",
etc.
Test Plan: Looked at task list view, saw "Resolved", "Wontfix", etc.
Reviewers: skrul, hunterbridges, jungejason, tuomaspelkonen, aran
Reviewed By: skrul
CC: aran, skrul, jungejason, epriestley
Differential Revision: 851
Summary: If a user partially discovers a repository and then deletes it, the
timeline will have events from the old repository which this daemon won't be
able to parse.
Test Plan: @ajtrichards, can you apply this locally and restart your daemons
(##phd stop##, then relaunch them) and let me know if it fixes the issue?
Reviewers: ajtrichards, jungejason, tuomaspelkonen, aran
Reviewed By: ajtrichards
CC: aran, epriestley, ajtrichards
Differential Revision: 845
Summary:
When selecting children of a directory, it is possible that none of its
children exist anymore even though the directory still exists. After fetching
the children but before returning them, we should check whether there are any,
and if there are no children, set the reason as empty directory.
Test Plan:
In sandbox, browsed in diffusion to a directory that exists but has no
files and saw that it has a useful message instead of a vague exception.
Reviewers: epriestley, tuomaspelkonen, jungejason
Reviewed By: tuomaspelkonen
CC: aran, tuomaspelkonen
Differential Revision: 846
Summary:
This allows you to configure a single mailbox for all mail sent by phabricator,
so you
can keep a mailaddress like bugs@example.com and don't need a catchall on your
domain/subdomain.
Test Plan:
Enabled and disabled suffix. Saw mails generated have to correct prefix. Also
piped raw mails
into the scripts/mail/mail_handler.php and ensured comments went into
phabricator for both maniphest
and differential.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 815
Summary: This could be a lot fancier but let's see what else we need. Also fixed
some bugs with maniphest.info.
Test Plan: Used the Conduit web console to create some tasks with different
values.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason, epriestley
Differential Revision: 824
Summary: Open AphrontWriteGuard for user login.
Test Plan: verified that the user can log in.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 840
Summary: When a user has bad cookies, try to clear everything and tell them they
might need to manually clear things.
Test Plan: Added "&& false" to the valid branch and got the exception message.
Reviewers: jungejason
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 839
Summary:
After D814 and D829, you should be able to implement this logic in the
didWriteRevision() method of the field.
Note that the attacher is still referenced in
ConduitAPI_differential_updatetaskrevisionassoc_Method. This method should
probably be moved to facebook/ since it's pretty Facebook-specific.
No rush on any of this, it's not hurting anything.
Test Plan:
- Hit differential.getcommitmessage
- Ran 'arc diff'
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 830
Summary:
In D758, I tightened the scope for which we issue cookies. Instead of setting
them on the whole domain we set them only on the subdomain, and we set them as
HTTPS only if the install is HTTPS.
However, this can leave the user with a stale HTTP cookie which the browser
sends and which never gets cleared. Handle this situation by:
- Clear all four <domain, https> pairs when clearing cookies ("nuke it from
orbit").
- Clear 'phsid' cookies when they're invalid.
Test Plan: Applied a hackier version of this patch to secure.phabricator.com and
was able to login with a stale HTTP cookie.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 838
Summary:
This commit lets users who are filing Maniphest tasks to attache files to them
right off the bat.
Test Plan:
{F3545}
and
{F3546}
Reviewers: epriestley, fmoo, aran
CC:
Differential Revision: 837
Summary:
Remove the blame revision, revert plan and lines fields from the default field
loadout. (After D829 this doesn't cause issues where we have bogus dictionary
entries.)
You should add these back to the Facebook configuration since Facebook wants
these fields. However, I want to keep the default stack very light and I never
saw a huge amount of value in these fields at Facebook so I don't think they
make the cut. Sorry, tomo. ;_;
Test Plan: Ran "arc diff" locally.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: aran
CC: aran, tomo, epriestley
Differential Revision: 831
Differential comments
Summary: If you @mention several users, at least one of which is already CC'd,
we unset all the CCs and don't attach the "Added CCs: ..." block to the comment.
Test Plan: @mentioned two users, one of whom was already CC'd.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 827
Summary:
I think this is the last major step -- use the fields to parse commit messages,
not a hard-coded list of stuff. This adds two primary methods to fields, one to
get all the labels they'll parse (so we can do "CC" and "CCs" and treat them as
the same field) and one to parse the string into a canonical representation
(e.g., lookup reviewers and such).
You'll need to impelement the one block of task-specific stuff I removed in
Facebook's task field:
list($pre_comment) = split(' -- ', $data);
$data = array_filter(preg_split('/[^\d]+/', $pre_comment));
foreach ($data as $k => $v) {
$data[$k] = (int)$v;
}
$data = array_unique($data);
break;
Otherwise I think this is clean.
Test Plan:
- Called the conduit method with various commit messages, parsed fields/errors
seemed correct.
- "arc diff"'d this diff onto localhost, then updated it.
- "arc amend"'d this diff.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason, epriestley
Differential Revision: 829
Summary: While I thought this was complicated, there was nothing subtle or
tricky here -- I just misnamed a variable.
Test Plan: Created a revision with default CCs, got CCs instead of nothing.
Reviewers: aran, jungejason, tuomaspelkonen
Reviewed By: aran
CC: aran
Differential Revision: 834
Summary:
deprecate generateProperties() from class
DifferentialRevisionDetailRenderer. Custom fields now provides a much
more powerful version of generateProperties().
Depends on D814.
Test Plan:
implemented facebook task field with custom field and
verified it worked.
Reviewers: epriestley, tuomaspelkonen
Reviewed By: epriestley
CC: aran, jungejason, epriestley
Differential Revision: 826
Summary:
When rendering commit messages, drive all the logic through field specification
classes instead of the hard-coded DifferentialCommitMessageData class. This
removes DifferentialCommitMessageData and support classes.
Note that this effectively reverts D546, and will cause a minor break for
Facebook (Task IDs will no longer render in commit messages generated by "arc
amend", and will not be editable via "arc diff --edit"). This can be resolved by
implementing the feature as a custom field. While I've been able to preserve the
task ID functionality elsewhere, I felt this implementation was too complex to
reasonably leave hooks for, and the break is pretty minor.
Test Plan:
- Made numerous calls to differential.getcommitmessage across many diffs in
various states, with and without 'edit' and with and without various field
overrides.
- General behavior seems correct (messages look accurate, and have the
expected information). Special fields like "Reviewed By" and "git-svn-id" seem
to work correctly.
- Edit behavior seems correct (edit mode shows all editable fields, hides
fields like "Reviewed By").
- Field overwrite behavior seems correct (overwritable fields show the correct
values when overwritten, ignore provided values otherwise).
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason
Differential Revision: 814
Summary:
See T354. List every rule which has ever been applied in X-Herald-Rules, not
just the ones which most recently triggered.
Also some random fixes while I was debugging this:
- When conduit methods throw non-conduit exceptions, make sure they get
logged.
- Trigger the Facebook "tasks" backcompat block only if we were going to fail
(this should reduce the shakniess of the transition).
- Fix some log spew from the new field stuff.
Test Plan:
- Created a rule (ID #3) "No Zebras" which triggers for revisions without
"zebra" in the title.
- Created a revision without "zebra" in the title, got X-Herald-Rules: <2>,
<3>
- Updated revision to have "zebra" in the title, verified rule did not trigger
in Herald transcript.
- Verified X-Herald-Rules is still: <2>, <3>
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen
CC: aran, epriestley
Differential Revision: 817
Summary:
Provide a catchall mechanism to find unprotected writes.
- Depends on D758.
- Similar to WriteOnHTTPGet stuff from Facebook's stack.
- Since we have a small number of storage mechanisms and highly structured
read/write pathways, we can explicitly answer the question "is this page
performing a write?".
- Never allow writes without CSRF checks.
- This will probably break some things. That's fine: they're CSRF
vulnerabilities or weird edge cases that we can fix. But don't push to Facebook
for a few days unless you're prepared to deal with this.
- **>>> MEGADERP: All Conduit write APIs are currently vulnerable to CSRF!
<<<**
Test Plan:
- Ran some scripts that perform writes (scripts/search indexers), no issues.
- Performed normal CSRF submits.
- Added writes to an un-CSRF'd page, got an exception.
- Executed conduit methods.
- Did login/logout (this works because the logged-out user validates the
logged-out csrf "token").
- Did OAuth login.
- Did OAuth registration.
Reviewers: pedram, andrewjcg, erling, jungejason, tuomaspelkonen, aran,
codeblock
Commenters: pedram
CC: aran, epriestley, pedram
Differential Revision: 777
domain
Summary:
See D758, D759.
- Provide a strongly recommended setting which permits configuration of an
alternate domain.
- Lock cookies down better: set them on the exact domain, and use SSL-only if
the configuration is HTTPS.
- Prevent Phabriator from setting cookies on other domains.
This assumes D759 will land, it is not effective without that change.
Test Plan:
- Attempted to login from a different domain and was rejected.
- Logged out, logged back in normally.
- Put install in setup mode and verified it revealed a warning.
- Configured an alterate domain.
- Tried to view an image with an old URI, got a 400.
- Went to /files/ and verified links rendered to the alternate domain.
- Viewed an alternate domain file.
- Tried to view an alternate domain file without the secret key, got a 404.
Reviewers: andrewjcg, erling, aran, tuomaspelkonen, jungejason, codeblock
CC: aran
Differential Revision: 760
Summary: This prevents <applet /> attacks unless the attacker can upload an
applet which has a viewable MIME type as detected by `file`. I'm not sure if
this is possible or not. It should, at least, narrow the attack window. There
are no real tradeoffs here, this is probably a strictly better application
behavior regardless of the security issues.
Test Plan:
- Tried to download a file via GET, got redirected to info.
- Downloaded a file via POST + CSRF from the info page.
Reviewers: andrewjcg, erling, aran, jungejason, tuomaspelkonen
CC: aran
Differential Revision: 759
Summary:
We don't currently validate CSRF tokens on this workflow. This allows an
attacker to upload arbitrary files on the user's behalf. Although I believe the
tight list of servable mime-types means that's more or less the end of the
attack, this is still a vulnerability.
In the long term, the right solution is probably to pass CSRF tokens on all Ajax
requests in an HTTP header (or just a GET param) or something like that.
However, this endpoint is unique and this is the quickest and most direct way to
close the hole.
Test Plan:
- Drop-uploaded files to Files, Maniphest, Phriction and Differential.
- Modified CSRF vaidator to use __csrf__.'x' and verified uploads and form
submissions don't work.
Reviewers: andrewjcg, aran, jungejason, tuomaspelkonen, erling
Commenters: andrewjcg, pedram
CC: aran, epriestley, andrewjcg, pedram
Differential Revision: 758
Summary: See T429. When you hit certain errors, you get less-than-helpful
messages like "upload error 3". Instead, produce human-readable errors.
Test Plan: Simulated errors, verified user receives decent error messages.
Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran, startupguy
CC: aran
Differential Revision: 816
Summary:
When we create or update a revision, we use a parsed commit message dictionary
to edit its fields. Drive consumption of the dictionary through custom fields
instead of hardcoding.
This requires adding some fields which don't really do anything right now to
cover fields which appear only in the commit message.
Test Plan: "arc diff"'d this revision against localhost, "arc diff"'d again to
update.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason
Differential Revision: 811
Summary:
Move all the rest of the fields into the custom field schema, for revision
views.
I left a couple of stubs in here (willWriteRevision, didWriteRevision) since I'd
planned to do edits here too, but this diff is sort of big-ish already. I'll do
all the edit fields in the next revision.
Depends on D808.
Test Plan: Viewed, edited and conduit'ed some revisions.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason, epriestley
Differential Revision: 809
Summary:
Move additional fields (which rely on loading handles) to the extensible field
classes and out of hardcoding in the controller.
Depends on D807.
Test Plan: Viewed, edited, and hit conduit for revisions.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason, epriestley
Differential Revision: 808
Summary:
Differential has a bunch of display-only fields, implement them all as field
specifications instead of hard-coded fields.
Also add some more documentation and fix redundant string constants in blame
rev/revert plan fields.
Test Plan: Viewed, edited, and hit conduit for revisions.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason, epriestley
Differential Revision: 807
Summary:
- Fix a bug where 'caption' didn't do anything.
- Provide an abstract base implementation for extensions.
- Add some documentation.
- Expose aux fields via conduit.
Test Plan: Added some fields like "Dinosaur", "Kilograms" and "derp" on my local
install. Read documentation.
Reviewed By: jungejason
Reviewers: hunterbridges, jungejason, tuomaspelkonen, aran
CC: aran, philc, jungejason
Differential Revision: 785
Summary: Similar to D785 for Maniphest, expose auxiliary field values via
Conduit.
Test Plan: Ran revision.getinfo on a revision with aux fields, got them in the
response.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason
Differential Revision: 802
Summary:
This is just to ease transitions for any installs which use these fields (e.g.,
Facebook). I'll write some docs and a migration script once this stuff is a
little more solid, too.
Depends on D800.
Technically these are "better" than the current fields since they show up other
places than the edit screen (derp derp).
Test Plan: Created a field selector which provides these; verified they work by
typing stuff into them and saving the revision.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, epriestley, jungejason
Differential Revision: 801
Summary: Depends on D798. Extends custom fields and makes the vaguely useful:
they can appear on the edit and view interfaces. This does not integrate them
with commit messages yet; that's more complicated but I plan to do it shortly.
Test Plan: Implemented a custom field per P123, it correctly appears on the edit
interface, persists, validates, and shows up when viewing the revision.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, epriestley, jungejason
Differential Revision: 800
Summary:
Precursor to building this out to solve T343. This is similar to the Maniphest
fields we landed recently, although I think they're dissimilar enough that it
isn't worth going crazy trying to make them share code, at least for now.
This doesn't really do anything yet, just adds a storage object and a couple of
selector/field indirection classes.
Test Plan: Ran SQL upgrade script, created an aux field.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason
Differential Revision: 798
Summary: Simplify the division of a parent task into several subtasks by looping
the "create subtask" workflow. This replaces "Create Another Task" with "Create
Another Subtask" when you arrive via subtasking.
Test Plan:
- Created a task, looped task create flow.
- Created a subtask, looped subtask create flow.
Reviewed By: codeblock
Reviewers: hunterbridges, codeblock, jungejason, tuomaspelkonen, aran
CC: aran, codeblock, epriestley
Differential Revision: 779
Summary:
we need to know the revision id in order to generate
differential links. It would be amazing if there existed some library
for object <-> network call mapping, and we could get all the
information about an object given some unique ID rather than having to
create a dictionary manually. One can dream...
Test Plan:
1. setup on test phabricator instance
2. called differential.getdiff from client code with a valid diffid
3. verified that the correct revisionID was included
Reviewed By: epriestley
Reviewers: epriestley
CC: dpepper, aran, epriestley, mgummelt
Differential Revision: 795
Summary:
In preparation for adding another search engine (see T355):
- Rename "executor" to "engine".
- Move all engine-specific operations into the engine. Specifically, this
means that indexing moves out of the document store and into the engine (it was
sort of silly where it was before).
- Split choice of an engine into an overridable "selector" class, a base API,
and a concrete MySQL implementation (just like storage engine selection).
- Make all callers go through the indirection layer.
The default selector just unconditionally selects the MySQL engine, but now
(with D786) I can build an Elastic Search engine and you guys can build a
multi-target engine if you want and I don't get there fast enough.
Test Plan:
- Created a new document (task).
- Searched for and found it.
- Viewed index reconstruction.
Reviewed By: jungejason
Reviewers: jungejason, amckinley, tuomaspelkonen, aran
CC: aran, jungejason, epriestley
Differential Revision: 788
Summary:
This is a very small step toward building a Status and possibly an Oncall tool.
Build a calendar view which renders months.
Much of my hesitance to bang these tools out is that dealing with
dates/calendaring is basically horrible, so I'm trying to ease into it.
This calendar is locale-aware and all that jazz.
Test Plan:
- See:
https://secure.phabricator.com/file/view/PHID-FILE-c07a9c663a7d040d2529/
- Verified that months have the right number of days, today is the right day
of the week, months begin on the day after previous months end on, etc.
Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran
Commenters: cwbeck, jungejason
CC: blair, aran, epriestley, cwbeck, jungejason
Differential Revision: 791
Summary:
Provide a {T123} syntax which pulls in the entire name of an object, not just a
link to it. A major use for this is organizing projects using wiki pages. Since
handle links show object status now, this lets you organize stuff in an ad-hoc
way and get a reasonable overview of it. We can make handles richer in the
future, too.
The performance on this isn't perfect (it adds some potential single gets) but I
think it's okay for now and I don't want to make remarkup engine even more
complex until the preprocess/postprocess stuff has had a chance to settle and
I'm more confident it works.
In Differential and Maniphest we'll also incorrectly cache the object
state/name, but that'll fix itself once I move the cache code to use
preprocess/postprocess correctly.
Test Plan:
- See https://secure.phabricator.com/file/view/PHID-FILE-5f9ca32407bec20899b9/
for an example.
- Generated and looked over the documentation.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran, hunterbridges
CC: skrul, aran, jungejason, epriestley
Differential Revision: 784
Summary:
This accomplishes two goals:
- Data fetching is now grouped across blocks.
- Demonstrates that D737 actually works.
Test Plan: Used @mentions in Phriction preview, they rendered properly. Verified
only one service call was being made across blocks.
Reviewed By: jungejason
Reviewers: hunterbridges, jungejason, tuomaspelkonen, aran
CC: aran, jungejason, epriestley
Differential Revision: 738
Summary:
Right now, the "SimpleEmailService" class uses trigger_error() to communicate
error messages. This means they get lost in the error logs and aren't visible in
the MetaMTA interface.
Provide a flag to strengthen them into exceptions, instead.
(I've attempted to emulate the prevailing style so I can offer this upstream.)
Test Plan: Faked an error condition and got a detailed stack trace in MetaMTA
instead of an empty "Message" field.
Reviewed By: jungejason
Reviewers: hunterbridges, codeblock, jungejason, tuomaspelkonen, aran
CC: aran, jungejason
Differential Revision: 783
Summary:
Add a new "Create Subtask" action that allows you to quickly split a task into
dependent subtasks, using the parent task as a template.
Followups:
- Cause "workflow=<parent>" to change "Create Another Task" into "Create
Another Subtask" (after D736).
- Show parent tasks (requires some schema stuff).
Test Plan:
- Created a new task.
- Created a new subtask.
Reviewed By: codeblock
Reviewers: hunterbridges, codeblock, jungejason, tuomaspelkonen, aran
CC: aran, codeblock
Differential Revision: 774
Summary:
We use ObjectHandles as proxy objects which can refer to any other object in the
system. Add the concept of the underlying object's "status" (e.g., open, closed
or busy).
This allows us to render completed tasks and revisions with strikethrough. In
the future, if we implement OOO or something, we could render users with a
"busy" status if they're on vacation, etc.
Test Plan: Viewed a task with closed revisions and dependencies:
https://secure.phabricator.com/file/view/PHID-FILE-6183e81286fa3288d33d/
Reviewed By: codeblock
Reviewers: codeblock, hunterbridges, jungejason, tuomaspelkonen, aran
CC: aran, codeblock
Differential Revision: 772
Summary:
Not totally sure I'm in love with this but I think it's somewhat non-terrible,
despite the lack of lens flare.
Also made "Cancel" take you back to the task if you got to "Create" from "Create
Another Task".
Test Plan:
- Style:
https://secure.phabricator.com/file/view/PHID-FILE-ad37d3c1f3b2c7a7a7d1/
- Hit "Cancel" from "Create Another", got sent back to task.
- Hit "Cancel" from normal create, got sent back to list.
- Tried to save an invalid task after making changes to CC/Projects, changes
were preserved.
Reviewed By: codeblock
Reviewers: hunterbridges, jungejason, tuomaspelkonen, aran, codeblock
CC: aran, epriestley, codeblock
Differential Revision: 736
Summary: Implements an S3 storage engine option for Phabricator.
Test Plan:
- Uploaded files to S3.
- Looked at them.
- Verified they appeared in S3 using the S3 file browser.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason
Differential Revision: 752
Summary:
This allows you to edit dependencies. It is a better patch than it used to be.
It depends on D725.
- If you create a cycle, it just throws an exception and aborts the workflow.
It should not do this.
- Tasks which depend on the current task aren't shown in the UI. Need to add a
new table for this.
- Transaction text says "attached Task" but should probably say "added a
dependency on task".
Test Plan: Created valid and invalid dependencies between tasks. Created valid
and invalid dependencies between revisions.
Reviewed By: tuomaspelkonen
Reviewers: davidreuss, jungejason, tuomaspelkonen, aran
Commenters: codeblock
CC: aran, codeblock, tuomaspelkonen, epriestley
Differential Revision: 595
Test Plan: Add fields to config based on specification on T335. View on Task
Edit and Task Detail. Supported types are string, int and select
Reviewed By: epriestley
Reviewers: epriestley
CC: aran, epriestley, hunterbridges
Differential Revision: 753
Summary: See T382. I think bill also asked about this (T308); there's no way to
figure it out right now.
Test Plan:
- Hit "?", verified the dialog now reveals that escape closes it.
- Hit escape, dialog vanished.
Reviewed By: jungejason
Reviewers: fzamore, bill, codeblock, jungejason, tuomaspelkonen, aran
CC: aran, jungejason
Differential Revision: 763
Summary:
@skrul reports receiving multiple copies of notification emails since
@hunterbridges configured some bizarre dystopian email replication factory on
their outbound route. Two fixes:
- Ensure "To" and "Cc" are unique. Email shouldn't be replicated for "To:
x@y.com, x@y.com" but it's silly that we do this.
- Remove "To" addresses from "Cc". Email shouldn't be replicated here either,
but we don't really lose anything by accommodating this.
Test Plan:
Sent a mail to the same to/cc, verified I was to'd only and not cc'd when the
mail was delivered.
@hunterbridges, can you apply this patch locally and verify it fixes the issue?
You can test by going to MetaMTA -> Send New Message and sending a message to
yourself as both To and CC.
Reviewed By: skrul
Reviewers: skrul, hunterbridges, jungejason, tuomaspelkonen, aran
CC: aran, skrul, epriestley
Differential Revision: 751
Summary: See T389. If you're looking at tasks in project X, default new tasks
into project X.
Test Plan:
- Created a new task without associated projects.
- Created a new task from a filter view with 1 and 2 projects; defaults were
filled in.
- Submitted task with invalid title, projects were correctly preserved.
Reviewed By: skrul
Reviewers: skrul, hunterbridges, jungejason, tuomaspelkonen, aran
CC: aran, skrul, epriestley
Differential Revision: 757
Summary: Try to break this apart a little better in preparation for D595. No
functional changes, just refactored the relatively large processRequest()
method.
Test Plan:
- Attached and detached revisions from tasks.
- Attached and detached tasks from revisions.
- Merged tasks.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason
Differential Revision: 725
Summary:
This gets all the major pieces working. Allows you to drag-and-drop files in
Differential and Phriction, and embed files in remarkup with {Fxxx} references.
See also task.
I'm explicitly not documenting this yet since it's still pretty rough.
Test Plan: Dragged and dropped stuff into Differential and Phriction.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran, tomo
Commenters: tomo
CC: aran, tomo, jungejason
Differential Revision: 674
Summary:
See T372. Always render commit messages on one display line, so the table
doesn't jump around as they AJAX in on browse views.
The goal here is to have the cell choose a size naturally and for its content to
render with "overflow: hidden" if the natural size isn't large enough to contain
the content. "white-space: pre" or "white-space: nowrap" would prevent wrapping
but potentially make the table exceed the display width when a better behavior
is to hide some of the commit message.
Also use utf8-aware shortening, now that we have a function for it.
Casting a wide net in case anyone has a better way to do the CSS here. It's kind
of nasty that we have to use so many DOM nodes.
Test Plan:
- Resized window while viewing browse and history views in Safari, Chrome and
Firefox. Table exhibited described behavior.
- Verified summaries render sensibly and are properly truncated to 100
characters.
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen, tomo, mroch, cpojer
CC: aran, epriestley
Differential Revision: 750
Summary:
- Allow the console to handle abstract classes correctly.
- Move paste dictionary generation to an abstract base class.
- Add paste.create.
- Add 'objectName', 'parentPHID', and 'content' to Paste info dictionaries
(you can use filePHID with file.download to get the content but I think just
always sending it back is reasonable).
Test Plan:
- Use paste.create to create new pastes.
- Used paste.info to get existing pastes.
- Checked console UI to make sure "paste." didn't show up or anything
silly/dumb like that.
- Tried to call the method "paste" and got the right exception.
Reviewed By: codeblock
Reviewers: codeblock, jungejason, tuomaspelkonen, aran
CC: aran, codeblock
Differential Revision: 747
Summary:
- There's no way you can figure out the ID of a file right now. Expose that
more prominently.
- Put the drag-and-drop uploader on the main page so you don't have to click
through.
- Restore the basic uploader so IE users can theoretically use the suite I
guess? Added author info to basic uploader.
- Show author information in the table.
- Show date information in the table.
- Link file names.
- Rename table for filter views.
- When you upload one file, just jump to it. When you upload multiple files,
jump to your uploads and highlight them.
- Add an "arc download" hint.
Test Plan: Uploaded single files, groups of files, and files via simple
uploader.
Reviewers: codeblock, jungejason, tuomaspelkonen, aran
Commenters: codeblock
CC: aran, codeblock, epriestley
Differential Revision: 746
Summary: See T368. The current rendering result can cause some confusion for the
first/last chunks, make their behavior more explicit.
Test Plan:
- Clicked various "show more" links on a bunch of top/bottom/middle omitted
context blocks in a variety of diffs.
- Located a @generated shielded file and verified the initial render is
correct when the entire file is default-hidden.
Reviewed By: avitaloliver
Reviewers: avitaloliver, jungejason, tuomaspelkonen, aran
CC: aran, avitaloliver
Differential Revision: 744
Summary:
Need this for 'arc upload' and 'arc download'.
Given a file PHID or ID, provides information about it.
Test Plan:
- Implemented 'arc upload' and 'arc download' based on 'file.info'.
- Used Conduit API console to test method.
Reviewed By: codeblock
Reviewers: codeblock, jungejason, tuomaspelkonen, aran
CC: aran, codeblock, epriestley
Differential Revision: 741
Summary:
We just weren't handling these at all reasonably, must have dropped the logic
when they got ported.
This still isn't perfect: we have some display glitches around file names, so
the 'away' part renders as "This file was moved to .". I'll see if I can follow
up and fix that, but this resolves the more immediate issue of the interface
just not working at all.
Test Plan: Moved and copied files in my test repository, verified they rendered
somewhat correctly.
Reviewed By: jungejason
Reviewers: tuomaspelkonen, jungejason, aran
CC: aran, jungejason, epriestley
Differential Revision: 699
Summary: create the page by getting data from the search result.
Test Plan:
load page with url /author/, /author/valid_username, and
/uathor/invalid_username, and verified that it works as expected.
Reviewed By: epriestley
Reviewers: epriestley, tuomaspelkonen
Commenters: tuomaspelkonen
CC: hwang, aran, tuomaspelkonen, epriestley, jungejason
Differential Revision: 723
Test Plan: Look at a task detail. Some dummy attributes are automatically added in ManiphestTaskDetailController.
Reviewed By: epriestley
Differential Revision: 730
Summary:
Python people don't seem to like the 'ignore-all' as default. Provide a way
to configure which file types should not use 'ignore-all'.
Test Plan:
Tested that it worked with bunch of Python of files and non-python
files. Cache was disabled during the test.
Reviewed By: jungejason
Reviewers: epriestley, jungejason
Commenters: epriestley
CC: aran, jungejason, epriestley
Differential Revision: 713
Summary:
I want clean this up enough that I can land D595 without making a complete mess,
here's a small simplification.
Move object load logic into PhabricatorObjectHandleData.
Test Plan: Attached tasks and revisions, merged tasks.
Reviewed By: aran
Reviewers: tuomaspelkonen, jungejason, aran
CC: aran
Differential Revision: 724
Summary: See T266. Combine these interfaces into one and move it to settings.
Test Plan: Edited my profile and account.
Reviewers: codeblock, tcook, jungejason, tuomaspelkonen, aran
CC:
Differential Revision: 722
Summary: We omit a 'status' dropdown when creating a new task since it's silly
to create a non-open task and would just clutter the interface, but there's no
reason not to allow status to be edited on the "Edit" interface.
Test Plan: Created a new task (no status dropdown). Edited a task, including
changing the task status.
Reviewed By: jungejason
Reviewers: toulouse, jungejason, tuomaspelkonen, aran
CC: aran, jungejason
Differential Revision: 720
interface
Summary:
See T58. When I originally wrote this interface you couldn't make a form that
looked like a form but wasn't really a form, so I made it a real form. That
meant you could hit "return" in the text input and submit the form, which didn't
do anything but is vaguely silly.
After D422 there's a layout-only form element, so switch to that.
Test Plan: Loaded the tab, hit return in the textarea, it didn't do a form
submit.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason, epriestley
Differential Revision: 719
Summary:
It makes more sense to just make this a settings panel rather than a standalone
app, particularly since setting panels are relatively well separated now.
Also default-disabled the SSH Keys interface since it won't currently be useful
for most installs.
Test Plan: Edited preferences.
Reviewed By: jungejason
Reviewers: tuomaspelkonen, jungejason, aran
CC: aran, jungejason
Differential Revision: 716
Summary:
Add a new column to PhrictionContent called 'comment' or 'description' or
something
Add an optional field to the Phriction document editing interface that
allows you to add a comment
Test Plan:
Run the sql patch to modify phriction_content table
Create a new wiki page in Phriction
Type in words in description field and save the page
Visit this page and click "Edit Page" button
The content in the desciption field is saved
Reviewed By: epriestley
Reviewers: epriestley, hsb, codeblock
Commenters: codeblock
CC: aran, codeblock, hwang, epriestley
Differential Revision: 709
Summary:
With the sshd-vcs thing I hacked together, this will enable Phabricator to host
repositories without requiring users to have SSH accounts.
I also fixed "subporjects" and added an explicit ENGINE to it.
Test Plan: Created, edited and deleted public keys. Attempted to add the same
public key twice. Attempted to add invalid and unnamed public keys.
Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran, cadamo, codeblock
CC: aran, epriestley
Differential Revision: 711
Summary:
Currently, you can only edit your own affiliation to projects. Enable users to
be managed in a more reasonable batched way.
I'll lock this down to admins/owners and add a transaction log at some point.
Test Plan: Edited project affiliations. Verified Herald still works.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason
Differential Revision: 677
Summary:
See T344. Currently, there's a hard-coded 12MB filesize limit and some awkward
interactions with MySQL's max_allowed_packet. Make this system generally more
robust:
- Move the upload limit to configuration.
- Add setup steps which reconcile max_allowed_packet vs MySQL file storage
limits.
- Add a layer of indirection between uploading files and storage engines.
- Allow the definition of new storage engines.
- Define a local disk storage engine.
- Add a "storage engine selector" class which manages choosing which storage
engines to put files in.
- Document storage engines.
- Document file storage classes.
Test Plan:
Setup mode:
- Disabled MySQL storage engine, misconfigured it, configured it correctly.
- Disabled file storage engine, set it to something invalid, set it to
something valid.
- Verified max_allowed_packet is read correctly.
Application mode:
- Configured local file storage.
- Uploaded large and small files.
- Verified larger files were written to local storage.
- Verified smaller files were written to MySQL blob storage.
Documentation:
- Read documentation.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, epriestley, jungejason
Differential Revision: 695
Summary:
I want to do two things here:
- Add SSH Keys
- Move "Preferences" into this panel
But this controller was pretty gigantic and messy. Split it apart and use
delegation instead.
There are no functional changes. I changed some of the conduit certificate text
to simplify it since no one should need to go through that workflow anymore,
given the existence of "arc install-certificate".
Test Plan:
- Edited realname, including attempting to remove it.
- Edited profile picture.
- Edited timezone.
- Edited email, including attempting to remove it.
- Regenerated condiut certificate.
- Linked and unlinked an OAuth account.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason
Differential Revision: 688
Summary:
You can only call setHeader() on a Panel once. Otherwise the last sticks. Move the "forks of this paste" stuff to its own panel (only shown if there are, indeed, forks), and make the columns look nicer.
Test Plan:
Viewed previously forked pastes, forked a paste and looked at the original, and looked at a non-forked paste. All looked sane.
Reviewers:
epriestley
CC:
Differential Revision: 700
Summary:
Share code with the new PhabricatorDifferenceEngine, which handles diffs with no
changes correctly.
(This isn't the same issue as file moves, but I ran into it while generating a
repro case.)
Test Plan: Previously, changes which didn't change file content (e.g., property
changes) would throw. Now they work.
Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen, jungejason, aran
CC: aran, epriestley, tuomaspelkonen
Differential Revision: 698
Summary:
See T325. While this is a touch hacky it ends up being fairly clean, and we can
now do initial imports much more quickly and this actually cleaned up some of
the code. I also made the repository edit interface a little less foreboding.
@tuomaspelkonen, did you get anywhere with that bug you were chasing down a
couple days ago? We can hold this if it throws a wrench into stuff you're
working on.
Test Plan:
- Imported a subdirectory of a midsized SVN project (jQuery UI).
- Commit discovery for ~3500/4500 commits took just a few seconds.
- Commit discovery correctly ignored commits which didn't affect this
directory.
- Commit discovery correctly stopped at commit 13.
- Browse interface shows an incomplete listing, but that's fine, and
everything is otherwise functionally correct. We can add a note or something
later ("this is a view of commits affecting a subdirectory, some paths aren't
available"), but this behavior probably won't be too startling to users.
- Edited Git and SVN repositories to test form logic.
Reviewed By: jungejason
Reviewers: tuomaspelkonen, jungejason, aran, Girish
Commenters: tuomaspelkonen
CC: jcleveley, aran, jungejason, tuomaspelkonen
Differential Revision: 696
Summary:
Quite basic subproject attachment.
Test Plan:
Go to some proj
ect and add another as a subproject.
Play around the ui.
Reviewers: epr
iestley
CC:
Summary:
This is really rough and needs work (particularly, there's some diff code I
really need to refactor since I sort-of-copy-pasted it) but basically
functional.
Show text changes between diffs and allow users to revert to earlier versions.
Differential's line-oriented diff style isn't ideal for large blocks of text but
I'm betting this is probably good enough in most cases. We can see how bad it is
in practice and then fix it if needbe.
I added a bunch of support for "description" but didn't add the feature in this
diff, I'll either follow up or task it out since it should be a pretty
straightforward change.
Test Plan: Looked at history for several Phriction documents, clicked "previous
change" / "next change", clicked revert buttons.
Reviewed By: hsb
Reviewers: hsb, codeblock, jungejason, tuomaspelkonen
CC: aran, hsb, epriestley
Differential Revision: 687
Summary:
Show line count, arcanist project and base revision.
This adds a little clutter but I think we're still okay and I can play around
with it later.
Test Plan: Looked at a couple of revisions. I'm actually not 100% sure about the
SVN logic but maybe I will test that before committing.
Reviewed By: tomo
Reviewers: tomo, jungejason, tuomaspelkonen, aran
CC: aran, tomo
Differential Revision: 685
Summary: Preview Phriction documents as they are edited, similar to how
Differential/Maniphest work.
Test Plan: Mashed my keyboard while editing a Phriction document.
Reviewed By: hsb
Reviewers: hsb, codeblock, jungejason, tuomaspelkonen, aran
CC: aran, hsb
Differential Revision: 684
Summary: Pull the next couple levels of hierarchy and render them at the bottom
of the page. This might need some tweaking but it seems OK at first glance.
Test Plan:
https://secure.phabricator.com/file/info/PHID-FILE-ef0af5d4dc6dceaeb2e3/
Also reduced limit to 1 and verified the "more" behavior worked properly.
Reviewed By: hsb
Reviewers: hsb, codeblock, jungejason, tuomaspelkonen, aran
CC: aran, hsb
Differential Revision: 676
Summary:
Added a 'parent' field which stores a PHID of another paste. If it is not NULL show a list of children pastes on view.
Also did some misc. refactoring to clean up the code a bit, specifically in the Create controller.
Test Plan:
- Checked old pastes, they were not affected.
- Added a paste, successfully.
- Forked it, successfully.
- Went to the original paste, saw the child paste listed.
- Forked it again, saw the new one added to the list.
Reviewers:
epriestley
CC:
Differential Revision: 672
Summary: So you don't have to edit if you don't want to.
Test Plan: Hit "cancel" on an edit of an existing page and an edit of a new
page.
Reviewed By: codeblock
Reviewers: hsb, codeblock, jungejason, tuomaspelkonen, aran
CC: aran, codeblock
Differential Revision: 675
Summary: This fixes a bug where doing an "Add CC" on a task you were not CC'd on
would remove all the CCs except yourself. It also simplifies the CC handling
code a lot.
Test Plan:
- Added myself and another user to a task neither of us were CC'd on (old
behavior: added both then removed them; new behavior: added both)
- Added a user to CC with @mentions.
- Made a comment on a task I wasn't CC'd on (I was CC'd).
- Closed a task I wasn't assigned or CC'd on (I was not CC'd, but was
assigned).
- Made an "Add CC" with new CCs and comment text (ccs added, text appeared).
- Made an "Add CC" with no CCs and comment text (text appeared, transaction
correctly downgraded to "comment").
- Made an "Add CC" with exsiting CCs and comment text (text appeared,
transaction correctly downgraded to "comment").
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason
Differential Revision: 668
