Summary: New implicit fallthrough linter detected a few issues; none of these have behavioral impacts but they can clearly be tightened up. See D1824.
Test Plan: Lint; inspection.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1825
Summary:
This adds a new configuration setting:
"notification.actions" : [
"commit",
"abandon",
"actions"
]
if not set, displays all actions, if is set, display only what is set to display
Test Plan: add the notification.actions settings and set accordingly
Reviewers: epriestley, zeeg
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1820
Summary:
More complicated updates deserves title and explanation.
This diff uses only the first line from comment in diff's description.
It also removes the truncating at 80 chars which looks as an error.
Test Plan:
var_dump(preg_replace('/\n.*/s', '', "abc"));
var_dump(preg_replace('/\n.*/s', '', "abc\ndef"));
var_dump(preg_replace('/\n.*/s', '', "abc\ndef"));
var_dump(preg_replace('/\n.*/s', '', "abc\ndef\nghi"));
var_dump(preg_replace('/\n.*/s', '', "\nabc")); // empty string
Display revision with 255 chars description in update history - it looks OK.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1812
Summary: We currently parse these as directory changes and discard them. Instead, parse them as a new "SUBMODULE" type of change.
Test Plan:
- Reparsed a commit which changes submodules and verified it parses correctly.
- Reparsed a commit which adds submodules and verified it parses correctly.
Reviewers: btrahan, kdeggelman
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1815
Summary:
They end up in "CCs:" fields where they can't be parsed.
Not bothering to migrate since I think only Dropbox has hit this.
Also improved another error condition's handling.
Test Plan: Tried to save a mailing list with spaces and commas in the name.
Reviewers: btrahan, Makinde
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T947
Differential Revision: https://secure.phabricator.com/D1813
Summary:
Currently, we sort all results alphabetically. This isn't ideal. Instead, sort them like this:
- If the viewing user appears in the list, always sort them first. This is common in a lot of contexts and some "Ben Evans" guy is sorting first on secure.phabricator.com and causing me no end of aggravation.
- If the tokens match a "priority" component (e.g., username), sort that before results which do not have a "priority" match.
- Within a group (self, priority, everything else) sort tokens alphabetically.
NOTE: I need to go add setUser() to all the tokenizers to make the "self" rule work, but that's trivial so I figured I'd get this out first.
Test Plan:
https://secure.phabricator.com/file/data/4s2a72l5hhyyqqkq4bnd/PHID-FILE-x2r6ubk7s7dz54kxmtwx/Screen_Shot_2012-03-07_at_9.18.03_AM.png
Previously, "aaaaaepriestley" (first alphabetic match) would sort before "epriestley" (the viewing user). Now, "epriestley" sorts first because that is the viewer.
https://secure.phabricator.com/file/data/rmnxgnafz42f23fsjwui/PHID-FILE-yrnn55jl3ysbntldq3af/Screen_Shot_2012-03-07_at_9.18.09_AM.png
Previously, "aaaagopher" (first alphabetic match) would sort before "banana" (the "priority" match). Now, "banana" sorts first because it priority matches on username.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T946
Differential Revision: https://secure.phabricator.com/D1807
Summary:
- Enforce proper workflow rules.
- Fix a derp-bug with patches.
Test Plan:
- Tried to mark a revision I didn't own.
- Tried to mark a revision already marked committed.
- Tried to mark a revision otherwise not accepted.
- Verified daemon can override workflow rules and mark from arbitrary states.
Reviewers: btrahan, Makinde
Reviewed By: Makinde
CC: aran, epriestley
Maniphest Tasks: T948
Differential Revision: https://secure.phabricator.com/D1809
Summary: When a revision is accepted, allow users to manually mark it committed if there's a daemon/tracking problem. This shouldn't happen in most installs but we have less-robust support for Mercurial and some installs may not be fully configured.
Test Plan: Marked a revision committed.
Reviewers: btrahan, Makinde
Reviewed By: Makinde
CC: aran, epriestley
Maniphest Tasks: T948
Differential Revision: https://secure.phabricator.com/D1808
Summary:
A user reported that their install (on an unusual piece of hardware) was hitting this timeout. We don't need to be quite so stingy; just use the default 30s timeout.
Also remove some kind of sentence fragment since I no longer remember what it meant and it doesn't make sense.
Test Plan: This change resolved the issue.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1805
Summary: Provide a reasonable JS API for the Aphlict client. Provide an example behavior to invoke it.
Test Plan:
Ran "aphlict_server.js" with:
$ sudo node aphlict_server.js
Loaded /aphlict/. Opened console. Got "hello" from the server every second.
Got reasonable errors with the server not present ("Security exception", but this is because it can't connect to port 843 to access the policy server).
Reviewers: ddfisher, keebuhm, allenjohnashton, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T944
Differential Revision: https://secure.phabricator.com/D1800
Summary:
- Use a join to effect this query.
- Fixes a bug where packages with no commits would raise an exception because of the awkward query construction.
- Fixes a bug on audit views.
Test Plan:
- Viewed a package with no commits.
- Altered audit filters.
Reviewers: btrahan, 20after4
Reviewed By: 20after4
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1801
Summary:
I typed (tilde) (space) (space) (space) (space) (space) (tilde) earlier but it just vanishes, make it do what I intended.
Also fix a missing space that was breaking a CSS rule (in h1), set it to the intended value, and remove redundant margins (h1..h6).
Test Plan: Put some spaces in tildes, made some headers.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1803
Summary: Conduit access for open audits.
Test Plan: Used test console to run some queries.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1792
Summary:
Adds an optional "Auditors" field (like "Reviewers") to commit messages which gives installs a zero-config method for making audit requests.
This field does not appear on templates unless set, and is mostly ignored (but validated and preserved) by Differential.
It is then parsed by the daemons if present, and audit requests are pushed to valid users.
Test Plan: Made an "Auditors" commit and verified it was retained with "arc amend --show". Pushed it and verified the audit was triggered.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904, T880
Differential Revision: https://secure.phabricator.com/D1793
Summary: We render these in a realtively unreadable way right now; allow customization and provide reasonable defaults.
Test Plan: Looked at some tasks with custom fields on them.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T930
Differential Revision: https://secure.phabricator.com/D1790
Summary:
Caption Submit is quite confusing because it doesn't actually make the data visible to other users.
Especially when comment confirm button is also called Submit in serious mode.
Test Plan: Add inline comment.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1796
Summary:
- all search boxes are now jump navs (old functionality retained if none
of the jump nav patterns match)
- added global keyboard shortcut '/' to focus the search box in the upper
right
Test Plan:
- pressed '/' and noticed the search box gains keyboard focus
- triggered jump nav functionality from search box and saw it worked
- did a search which did not match a jump nav pattern and saw it worked
(and searched in the selected context)
NOTE: The search box on the /search/ page is also changed to have jump
nav functionality. Old functionality is not impared. Still, this may not
be desirable.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1794
Summary:
- Remove "0.5%" padding which makes Safari flip out and render every row differently sometimes.
- Remove list padding from ManiphestTaskListView, put it in the controller composition instead.
Test Plan: Viewed all places where task lists appear.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1788
Summary:
Filter Revisions button currently resets Status and Order fields.
I've rewritten it to GET form because it doesn't perform any action.
It fixed the problem along the way.
Test Plan:
/differential/filter/revisions/
Status: Open.
Filter Revisions.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1771
Summary:
- Move table to Repository, since we have no Owners joins in the application anymore but would like to do a Repository join.
- Rename "packagePHID" to "auditorPHID", since this column may contain package, project, or user PHIDs.
Test Plan:
- Browsed Owners, Audit, and Differential interfaces to the Audit tool.
- Made comments and state changes.
- Ran "reparse.php --herald --owners" on several commits.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley, nh, vrana
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1787
Summary: These are because they forgot to upgrade_schema.php like 99% of the time.
Test Plan: Hit such an error, got a better error message than before.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1786
Summary:
Show the retry count in the meta mta view (in addition to the list of
messages) - I find this info useful when I'm trying to debug what's going on
with mail failures.
Task ID: #
Blame Rev:
Test Plan:
loaded /mail/view/NNNNN/ and saw the retry count
Revert Plan:
Tags:
Reviewers: epriestley, btrahan, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1782
Summary:
Added a query option of status for the MetaMTA list controller. There currently
isn't a ui for accessing this.
Task ID: #
Blame Rev:
Test Plan:
loaded /mail/, /mail/?status=queued, /mail?phid=PHID...&status=...
each request returned a sane list of data
Revert Plan:
Tags:
Reviewers: epriestley, btrahan, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1784
Summary:
Displaying reviewer who was by coincidence listed first is quite confusing, especially for committed revisions.
This displays the one who really reviewed the revision if available.
This implementation is pretty bad from performance perspective - O(N) queries to retrieve all comments.
The page load still feels quite fast.
Test Plan: /differential/filter/revisions/
Reviewers: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1772
Summary:
- Owners has "by user" commit views, but these are supplanted by the Audit views. Just nuke them.
- Owners has "by package" commit views; consolidate these onto the package detail pages and link into Audit for full details.
Test Plan: Browsed all the Owners interfaces, clicked "View All ... Commits" buttons.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1764
Summary: The docs say "http://www.domain.com/" but if you don't put "/api/" it fails. GOTCHA!
Test Plan: Removed "/api/", launched bot, it worked.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T935
Differential Revision: https://secure.phabricator.com/D1763
Summary:
- Users may elect to receive an initial notification about a commit; allow it to be replied to in order to interact with the object.
- Share thread headers between emails.
- Add the "REPLY HANDLER ACTIONS" section to both emails.
Test Plan:
- Used "reparse.php --herald" to trigger herald emails, verified reply-to and email body.
- Made audit comments, verified body.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1762
Summary:
We already generate patches, but currently attach them. Allow them to be inlined instead (optionally, up to a certain size).
Also allow selection between unified and git patches.
Test Plan: Set these options in my local config, sent out a diff.
Reviewers: btrahan, Makinde
Reviewed By: Makinde
CC: aran, epriestley
Maniphest Tasks: T874
Differential Revision: https://secure.phabricator.com/D1759
Summary:
I use a smart editor which wraps words by itself so that I don't need them to be
wrapped by actual newlines.
Curent state disallows me adding or removing words later without uglying the
formatting.
Also the wordwrapped message looks ugly in Phabricator.
I am not sure how the commit message would look like on other places (such as
GitHub) but all reasonable tools should be able to wrap the text by itself.
Test Plan: arc amend --show # on a diff with long lines
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1755
Summary:
This does two things:
- don't fill '-' for columns where there are data from previous week
- completele hide columns if there are now new data so that noobs don't have a
55 years of history
Test Plan:
View my commits stats.
View my requested changes stats.
View epriestley FB stats.
Reviewers: vii, epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1750
Summary:
This will allow sending mail to be done by task workers. See T750.
Task ID: #
Blame Rev:
Test Plan:
- started taskmaster daemon in test env
- used "send new test message" feature in MetMTA (with send now unchecked)
- confirmed receipt of 1 email
- repeated 2 & 3 with send now checked
Revert Plan:
Tags:
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T388, T750
Differential Revision: https://secure.phabricator.com/D1723
Summary: add a big ole HELP tab and make "scope" link to the specific
sub-section about scope
Test Plan:
read my doc a few times, it basically english
verified links looked correct and should work right once this is all in
production
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T910
Differential Revision: https://secure.phabricator.com/D1752
Summary: Sandra had trouble opening the Spreadsheet_Excel_Writer ones so use
PHPExcel, which is way better, just a bit more complicated.
Test Plan:
- Generated modern Excel 2007 .xslx sheets.
- Opened them in Excel in Office Mac 2011.
- Opened them in Apple Numbers from the app store.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, epriestley
Maniphest Tasks: T911
Differential Revision: https://secure.phabricator.com/D1744
Summary:
- We have a lot of headers now; document them.
- Remove the one random protip from like 3 years ago from all Differential
mail.
Test Plan: generated; read documentation
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T931
Differential Revision: https://secure.phabricator.com/D1748
Summary:
- Update the Javelin submodule to pick up recent fixes (like D1749).
- Update the package definitions do do a slightly better job of packaging
resources.
Test Plan:
Up and down work in tokenizers now. Pages load slightly fewer
resources.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T927
Differential Revision: https://secure.phabricator.com/D1751
Summary:
I am not sure if it is by purpose but Phabricator now process paths like
https://secure.phabricator.com/D1681-so-freaking-cool.
The reason is that there are bunch of rules with missing '$' at the end.
This mistake is so common and easy to create that I've rather removed all '$'
and changed the way how the key is processed.
I am not absolutelly sure if the '$' was missing in some rules by purpose but if
it is the case then we should rather add explicit '.*'.
This change is backwards compatible with custom maps ending with '$'. It is not
compatible with paths not ending with '$' by purpose.
Test Plan:
Visit /, /differential/, /differential/stats/revisions/, /D1681.
Run before and after:
./aphrontpath.php D123
./aphrontpath.php D123-cool
./aphrontpath.php /
./aphrontpath.php differential
./aphrontpath.php differential/
./aphrontpath.php differential/stats/revisions/
./aphrontpath.php /file/data/x/PHID-FILE-y/z
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1745
Summary:
This diff makes the OAuthServer more compliant with the spec by
- making it return well-formatted error codes with error types from the spec.
- making it respect the "state" variable, which is a transparent variable the
client passes and the server passes back
- making it be super, duper compliant with respect to redirect uris
-- if specified in authorization step, check if its valid relative to the client
registered URI and if so save it
-- if specified in authorization step, check if its been specified in the access
step and error if it doesn't match or doesn't exist
-- note we don't make any use of it in the access step which seems strange but
hey, that's what the spec says!
This diff makes the OAuthServer suck less by
- making the "cancel" button do something in the user authorization flow
- making the client list view and client edit view be a bit more usable around
client secrets
- fixing a few bugs I managed to introduce along the way
Test Plan:
- create a test phabricator client, updated my conf, and then linked and
unlinked phabricator to itself
- wrote some tests for PhabricatorOAuthServer -- they pass!
-- these validate the various validate URI checks
- tried a few important authorization calls
--
http://phabricator.dev/oauthserver/auth/?client_id=X&state=test&redirect_uri=http://www.evil.com
--- verified error'd from mismatching redirect uri's
--- verified state parameter in response
--- verified did not redirect to client redirect uri
-- http://phabricator.dev/oauthserver/auth/?client_id=X w/ existing
authorization
--- got redirected to proper client url with error that response_type not
specified
-- http://phabricator.dev/oauthserver/auth/?client_id=X&response_type=code w/
existing authorization
--- got redirected to proper client url with pertinent code!
- tried a few important access calls
-- verified appropriate errors if missing any required parameters
-- verified good access code with appropriate other variables resulted in an
access token
- verified that if redirect_uri set correctly in authorization required for
access and errors if differs at all / only succeeds if exactly the same
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, ajtrichards
Maniphest Tasks: T889, T906, T897
Differential Revision: https://secure.phabricator.com/D1727
Summary:
- These are still slow, awkward and hideous -- but slightly better than
before.
- Allow "open" reports to be sorted.
- Add a "burn" chart/table for assessing project volatility.
- Add navigation.
Test Plan: Looked at reports.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T923
Differential Revision: https://secure.phabricator.com/D1737
Summary:
Some text editors support opening multiple files at once.
I've used space as paths separator which may be compatible with some other
editors (I didn't tried any other though).
Note: This approach is incompatible with spaces in paths.
I am fine with changing it to anything else to support such paths or more
editors.
Probably the cleanest solution (yet still incompatible with most editors) would
be to use something like ##editor://open/?file=A&line=1&file=B&line=2## but it
would require also changing the way how it's configured and I think it's not
worth it.
BTW, I've used a hacky bookmarklet for this feature before.
Deleted or added paths may not exist in users filesystem but we don't know which
so the button tries to open everything.
Test Plan:
Click Edit All.
Delete Editor Link in settings, verify that the button is missing.
View diff without revision, verify that the button is missing.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1741
Test Plan:
Edit inline comment.
Clear its text.
Hit Cancel.
Try to find Undo link on the usual place with no success.
Repeat for same text.
Repeat for different text, this time with success.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1739
Summary:
Some people find the current message stating "This diff has Lint/Unit Test
Problems" confusing if the unit tests or lint was skipped. This revision
clarifies those messages.
Test Plan:
Started to accept a revision with skipped lint and unit tests, and saw the new
message.
Reviewers: epriestley, btrahan, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1738
Summary:
See T926. If you want to write a mail rule that, e.g., captures Differential
mail but ignores people replying to it, it's kind of tricky right now. You can
use the 'X-Mail-Transport-Agent' header but that's not obvious and it's not
necessarily stable.
Add a nice, obvious "X-Phabricator-Sent-This-Message" header.
Test Plan: Sent myself some mail, verified the header appeared.
Reviewers: vrana, btrahan, fugalh, jungejason
Reviewed By: jungejason
CC: aran, epriestley
Maniphest Tasks: T926
Differential Revision: https://secure.phabricator.com/D1732
Summary:
I want to flag messages which require an immediate action from me in e-mail
client.
It is currently not possible because Author and Reviewers fields are both in
To:.
So the filtering rule cannot recognize if I am the person who should take the
action.
This diff adds these headers:
- X-Differential-Author
- X-Differential-Reviewers
- X-Differential-CCs
Test Plan:
Send comment to the diff.
Verify X-Differential-* headers.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T808
Differential Revision: https://secure.phabricator.com/D1724
Summary:
The current approach of using a modal overlay dialog to create/edit inline
comments is pretty silly. Use an inline textarea instead.
This element isn't perfect and we have some mild modalness issues, but I think
it's better than the silly thing we've got going on right now. We can keep
poking it as people break it.
Test Plan:
- Created comments; submitted and undid them in empty and nonempty states.
Used undo for nonempty states + cancel.
- Edited comments; saved and canceled them. Used undo for changed state.
- Replied to comments; yada yada as above.
- Deleted comments.
- Did various modal trickery where I clicked "Reply" on something else with a
dialog already up, this very mildly glitches but I think it's not a big issue.
Reviewers: vrana, btrahan, Makinde, nh
Reviewed By: vrana
CC: aran, epriestley
Maniphest Tasks: T431
Differential Revision: https://secure.phabricator.com/D1716
Summary: Added support for audit comment, concern, accept
Test Plan: Comment / Concern / Accept audit, and say "What's new?" in IRC
Reviewers: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1730
Summary:
Added phabot irc command to directly message a user rather than outputting in a
channel.
Syntax:
ex:
````Korvin, D1717```
results in phabot private messaging me the info on D1717
Test Plan: ##nick##, [DTPVF]n
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1717
Summary: Since we embed comments/audits into Diffusion now, we don't need the
old edit interface.
Test Plan: Grepped for links to old interface.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1714
Summary:
We can drive this query better from the Audit tool now; get rid of the Diffusion
version.
Preserve usernames in URIs as per T900.
Test Plan: Clicked "Commits" from profile. Browsed audit commit filters.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1713
Summary:
The general idea here is to build a Differential-like dashboard which shows all
the things you need to audit and all the things that other people have raised
issues with, so you have a one-stop "what do I need to deal with?" interface.
- Add problem commits to the "active" view of /audit/.
- Add problem commits to homepage.
- Add commit browsing interfaces to /audit/.
- Add an "Audit" app button.
Test Plan: Looked at homepage, commit filters. Audited commits, verified state
changes reflected properly.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1712
Summary:
Improve the custom query interface:
- Allow search for tasks not in projects.
- Allow search for tasks with no projects.
- Allow custom search to include author/owner constraints.
Test Plan: Searched for various sorts of tasks.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T911
Differential Revision: https://secure.phabricator.com/D1722
Summary:
Allow Maniphest result sets to be exported to Excel.
Spreadsheet_Excel_Writer is awful but comparatively easy to get working. There's
also a "PHPExcel" package but it has some autoload conflicts right now and this
seems good-enough.
Test Plan: Exported a bunch of tasks to Excel.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T923
Differential Revision: https://secure.phabricator.com/D1721
Summary:
- Add a proper mailKey field to make these things mailable. Backfill all
existing objects.
- Denormalize authorPHID to the commit object so we can query by it
efficiently in a future diff. We currently use the search engine to drive
"commits by author" but that's not so good for audit, which needs more
constraints.
- Add an overall audit status field so we can efficiently query "commits that
needs your attention".
- Add enough code to convince myself that these fields are basically
reasonable and work correctly.
Test Plan:
- Ran schema upgrades. Checked database state afterward.
- Ran "reparse.php --owners --herald" to verify worker changes.
- Looked at a commit, altered aggregate status via audits / reparse.php,
verified it responded correctly.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, epriestley, nh
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1706
Summary:
The feed time is stored as the upper 32 bits of
PhabricatorFeedStoryData::chronologicalKey. These bits were previously accessed
by right shifting, which does not work properly on 32 bit machines (the result
is PHP_INT_MAX). We now attempt to use the bc extension (if available) and fall
back on mysql math otherwise. (See T500, D912).
Test Plan:
The calculation is unchanged for 64 bit machines. I checked both
paths on a 32 bit machine with bc extension available by setting the appropriate
if-condition to false and true.
Reviewers: epriestley
Reviewed By: epriestley
CC: ddfisher, aran, epriestley
Differential Revision: https://secure.phabricator.com/D1726
Summary:
Adds softer parse modes with less validation for doing partial parses
during the "arc diff --create" flow.
Test Plan:
Ran "arc diff --create" and got sensible results for inputs like bad
reviewers but a good title/summary.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T614
Differential Revision: https://secure.phabricator.com/D1720
Test Plan:
Type ##@makinde## to comment, verify that it is converted to ##@Makinde##.
Verify that ##@NonExistent## stays ##@NonExistent##.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1725
Summary: Add more filters/options to the /audit/ interface (By User, By Package,
By Project...)
Test Plan: Looked at audits via /audit/.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1705
Summary: See D1707 -- just in case the Phabricator server is configured
suspiciously.
Test Plan: Cursory inspection.
Reviewers: Makinde, btrahan
Reviewed By: Makinde
CC: aran, epriestley
Maniphest Tasks: T922
Differential Revision: https://secure.phabricator.com/D1708
Summary:
- Move the buttons in the jump nav to iOS-style "application" buttons in the
header. These are sort of ugly right now, but I think serviceable enough. Some
day we will hire a designer whose entire job is to pick up after me.
- This gives us more room (allowing us to restore "Maniphest" and
"Differential").
- This also disassociates the app buttons from the jump nav, which was a
point of confusion (user expectation that the text input is related to the
buttons).
- Allow "Active Revisions" and "Assigned Tasks" to collapse completely. They
didn't completely collapse before because the top-level "Active Tasks" / "Active
Revisions" was sort of overloaded as quick nav to apps. Now we have app buttons.
- Reduce overall size of jump nav.
Test Plan: Looked at homepage in various states of need-for-attention.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1694
Summary: Add comment previews and saved drafts to audits, like Maniphest /
Differential.
Test Plan: Typed stuff into the box. Got a preview. Reloaded page. Stuff was
still there. Submitted comment. Stuff is gone.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1699
Summary: This control is a very thin shell right now with Maniphest/Differential
code duplication; unify the implemenations better for use in Audit.
Test Plan: Clicked toggle buttons in Differential and Maniphest.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1700
Summary:
When users submit an audit, send email to relevant parties informing them.
Allow email to be replied to. Just basic support so far; no "!raise" stuff and
no threading with the Herald commit notification.
Test Plan: Made comments, got email. Replied to email, got comments.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1698
Summary:
If a user comments on a commit but they don't currently have any audits they're
authoritative on, create a new one.
This makes it easier to handle other things more consistently, like figuring out
the overall audit status of a commit and who should get emails.
Test Plan: Made comments on commits I had authority on and did not have
authority on.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1697
Summary: Add audit information to the commit search index.
Test Plan: Updated a commit, searched for terms in its comments, got hits.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1696
Summary: When a user posts an action in the audit tool, publish it to feed.
Test Plan: Made some comments, saw them show up in feed.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1695
Summary: When a user has pending audits, show them on the homepage.
Test Plan: Looked at my homepage with and without pending audits.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1692
Summary:
Allows you to write a commit rule that triggers an audit by a user (personal
rules) or a project (global rules).
Mostly this is trying to make auditing more lightweight and accessible in
environments where setting up Owners packages doesn't make sense.
For instance, Disqus wants a rule like "trigger an audit for everything that
didn't have a Differential revision". While not necessarily scalable, this is a
perfectly reasonable rule for a small company, but a lot of work to implement
with Owners (and you'll get a lot of collateral damage if you don't make every
committer a project owner).
Instead, they can create a project called 'Unreviewed Commits' and write a rule
like:
- When: Differential revision does not exist
- Action: Trigger an Audit for project: "Unreviewed Commits"
Then whoever cares can join that project and they'll see those audits in their
queue, and when they approve/raise on commits their actions will affect the
project audit.
Similarly, if I want to look at all commits that match some other rule (say,
XSS) but only want to do it like once a month, I can just set up an audit rule
and go through the queue when I feel like it.
NOTE: This abuses the 'packagePHID' field to also store user and project PHIDs.
Through the magic of handles, this (apparently) works fine for now; I'll do a
big schema patch soon but have several other edits I want to make at the same
time.
Also:
- Adds an "active" fiew for /audit/, eventually this will be like the
Differential "active" view (stuff that is relevant to you right now).
- On commits, highlight triggered audits you are responsible for.
Test Plan: Added personal and global audit triggers to Herald, reparsed some
commits with --herald, got audits. Browsed all audit interfaces to make sure
nothing exploded. Viewed a commit where I was responsible for only some audits.
Performed audits and made sure the triggers I am supposed to be responsible for
updated properly.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1690
Summary: just changed the regex to only look at the beginning of the string
Test Plan: works with: s PhabricatorDAO, rP, r,
rPda892bde7c6e9c8f08572fde2d55c934f26dbb86
Reviewers: epriestley
Reviewed By: epriestley
CC: ddfisher, aran, epriestley
Differential Revision: https://secure.phabricator.com/D1703
Test Plan:
Go to revision with lots of comments in Firefox.
Click on one of the last three comments permalink.
Repeat in Chrome.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1702
Summary:
Current approach has several problems:
- if there is no link in the cell then it still shows a link cursor
- if there is a link then it is clickable only on the text
Test Plan:
Display file in Differential, hover over cell with link.
Repeat for Paste.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1701
Summary:
I added multiline highlighting with the syntax:
http://site/path/to/file$from-to
NOTE: you can reverse the from and to
Test Plan: Open a file in diffusion and attempt to highlight multiple lines
Reviewers: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1693
Summary:
D1631 updated the url for related commits, but missed the link here. This
rev updates the link in the owners tool list.
Task ID: #
Blame Rev:
Test Plan:
clicked the link, and it worked
Revert Plan:
Tags:
Reviewers: epriestley, jungejason
Reviewed By: jungejason
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1691
Summary:
This is intended to supplant the existing "audit edit" interface. I've changed
them to both drive down the same write pathway, but the UIs are still different.
I'll fully merge them in a future diff.
Add a comment box (like Maniphest and Differential) to Diffusion. When users
make comments, their comments appear on the commit. Any audits triggers they are
responsible for are updated to reflect actions they take, as well.
Currently, audits can only be triggered by packages, but I intend to allow them
to be triggered by users and projects (via herald rules) in an upcoming diff.
Thus some of the language like "projects, users or packages" when the code is
clearly dealing only with "packagePHID".
Test Plan: Made audit updates via commit interface and via existing edit
interface. Verified both interfaces updated correctly, and that audit
responsibility rules were applied properly.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1688
Summary:
beyond the title, this diff tweaks the test console to have a bit more
functionality. also makes a small change to CSS for AphrontFormControlMarkup,
which IMO fixes a display issue on
https://secure.phabricator.com/settings/page/profile/ where the Profile URI is
all up in the air and whatnot
I think this is missing pagination. I am getting tired of the size though and
will add later. See T905.
Test Plan:
viewed, updated and deleted client authorizations. viewed, created,
updated and deleted clients
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T849, T850, T848
Differential Revision: https://secure.phabricator.com/D1683
Summary: We already allow you to create comments, but we don't show them on the
commit page. After style / view unification this is easy; show comments on the
commit page.
Test Plan: Made comments on a commit using the audit too, saw them show up in
Diffusion.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1687
Summary:
I want to add comments to commits, and they should obviously share code with the
nearly-identical comments in Maniphest and Differential. Unify code/style as
much as possible.
This program made possible by a generous grant from D1513.
Test Plan:
- Looked at a bunch of different Differential and Maniphest comments; they
appeared to render identically to how they looked before.
- Tested some edge cases like anchors and "show details" on description edits
in Maniphest.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1686
Summary:
Currently, audits are only accessible through the Owners tool. Start moving them
to their own first-class tool in preparation for broader audit integration.
- Lay some infrastructure groundwork (e.g. AuditQuery).
- Build a basic /audit/ view.
- Show audits on the commit page in Diffusion.
This has some code duplication with stuff we've already got, but I'll merge
everything together as we move forward on this.
Test Plan: Looked at /audit/ and a commit.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1685
Summary:
First stab at a batch editor for Maniphest. Basically, you can select a group of
tasks and then import them into the "batch" interface, where you can edit all of
them at once.
High level goal is to make it easier for users in PM/filer/support/QA roles to
deal with large numbers of tasks quickly.
This implementation has a few major limitations:
- The only available actions are "add projects" and "remove projects".
- There is no review / undo / log stuff.
- All the changes are applied in-process, which may not scale terribly well.
However, the immediate need is just around projects and this seemed like a
reasonable place to draw the line for a minimal useful version of the tool.
Test Plan: Used batch editor to add and remove projects from groups of tasks.
Reviewers: btrahan, yairlivne
Reviewed By: btrahan
CC: aran, epriestley, sandra
Maniphest Tasks: T441
Differential Revision: https://secure.phabricator.com/D1680
Summary:
Added what's new flood protection and fixed array_push issues.
Also added rhetoric for "Commit"
Test Plan: say "What's new?" twice within one minute
Reviewers: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1684
Summary:
Added "What's new?" to the ircbot
====Matches
```What is new?
What's new?
Whats new```
Test Plan:
<`Korvin> what is new?
<korvinbot-local> Derpen created D1: Herped the derp - http://phabricator.net/D1
It shows five.
Reviewers: epriestley
Reviewed By: epriestley
CC: epriestley, aran
Differential Revision: https://secure.phabricator.com/D1666
Summary:
This is so freaking cool that I will try to implement it also on Facebook.
Idea is from
http://strd6.com/2011/09/html5-javascript-pasting-image-data-in-chrome/.
I don't know how to properly detect support but lying about it is not a big
deal.
Test Plan:
Go to revision comment textarea.
Paste some text data - works as usual.
Paste some image data in Chrome - file is uploaded and a link to it is inserted.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1681
Summary:
The mailing list page in MetaMTA only showed the first 100
sorted by ID, so it made it seem like lists were missing. Changed it to
do paging and short by name, so it has some user-understandable order.
Test Plan:
- Go to /mail/lists/
- Step through pager, confirm ordering.
Reviewers: nh, epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1670
Summary: Document "--" list sytle and improve explicitness of list documentation
in general.
Test Plan: Generated, read documentation.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1661
Summary: D1595 split encodeJSONForHTTPResponse() into two methods, but left a
straggling $use_javelin_shield parameter which is no longer used.
Test Plan: Caught errors in error log.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1663
Summary:
When a comments add reviewers or CCs, we just dump that sort of nastily into the
body. Put it in the header like Maniphest instead.
Also, record the diff associated with "update" actions and link to it (T871).
Test Plan: {F8546} {F8547}
Reviewers: btrahan, davidreuss
Reviewed By: davidreuss
CC: aran, epriestley
Maniphest Tasks: T871
Differential Revision: https://secure.phabricator.com/D1659
Summary: The /public/ rule needs to come before the more general subfilter rule.
Test Plan: Hit "all", "my projects" and "public" feeds, they all work.
Reviewers: davidreuss
Reviewed By: davidreuss
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1667
Summary:
DifferentialRevisionListView requires setFields to be called before
calling getRequiredHandlePHIDs; this adds that call for DiffusionController
Test Plan:
loaded diffusion and saw the "Pending Differential Revisions" section
populated, and no errors in the darkconsole
Reviewers: epriestley, btrahan, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1665
for scope
Summary:
this patch makes the access token response "complete" relative to spec by
returning when it expires AND that the token_type is in fact 'Bearer'.
This patch also lays the groundwork for scope by fixing the underlying data
model and adding the first scope checks for "offline_access" relative to expires
and the "whoami" method. Further, conduit is augmented to open up individual
methods for access via OAuth generally to enable "whoami" access. There's also
a tidy little scope class to keep track of all the various scopes we plan to
have as well as strings for display (T849 - work undone)
Somewhat of a hack but Conduit methods by default have SCOPE_NOT_ACCESSIBLE. We
then don't even bother with the OAuth stuff within conduit if we're not supposed
to be accessing the method via Conduit. Felt relatively clean to me in terms
of additional code complexity, etc.
Next up ends up being T848 (scope in OAuth) and T849 (let user's authorize
clients for specific scopes which kinds of needs T850). There's also a bunch of
work that needs to be done to return the appropriate, well-formatted error
codes. All in due time...!
Test Plan:
verified that an access_token with no scope doesn't let me see
anything anymore. :( verified that access_tokens made awhile ago expire. :(
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T888, T848
Differential Revision: https://secure.phabricator.com/D1657
Summary:
I typed up like 30 pages here and then got my soul crushed by T895, but here's
the summary:
I looked at several charting libraries. There aren't very many that seem to be
any good and have an open-source license.
I also want the charts to be scriptable in JS so we can add good interactivity
where appropriate.
Raphael is an SVG drawing library which seems very solid. gRaphael is a charting
library on top of Raphael that is a lot less solid, but seems kind of OK.
Overall, I think this selection gives us a lot of flexibility, although we'll
have to pay some costs up front. I'd rather do that then get limited later,
though.
That said, I'm open to other suggestions here if anyone has experience or wants
to take a different stab at researching things.
This is largely for @vii and D1643.
Test Plan: Created a basic, fairly OK chart (see next revision).
Reviewers: btrahan, vii
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1654
Summary:
I haven't actually been using this as much as I thought, and am more interested
in the full view than the per-project view.
Let's try moving it off /home/ and then maybe adding some filtering options at
some point.
Test Plan: Looked at "all" and "my projects" in feed. Looked at home page.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1658
Summary:
Show some statistics, like number of revisions, number of
revisions per week, lines per revision, etc. for phrivolous amusement.
Test Plan:
- Went to /differential/stats/revisions/
Numbers seem right
- Clicked 'Accepted'
Again
- Changed to another user with long history
Load time was not too long though delay noticeable
- Clicked 'Requested changes to'
User was preserved, looks good
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1643
create new tasks
Summary: see title
Test Plan: Tested jump nav and found the correct urls were being loaded. Old
functionality was not effected.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: ddfisher, allenjohnashton, kpark517, aran, epriestley
Differential Revision: https://secure.phabricator.com/D1642
Summary: This makes the oauth server a bunch more useful.
Test Plan:
- used /oauth/phabricator/diagnose/ and it actually passed!
- played around with conduit via hacking URL to include access_token on a logged
out browser
- linked my account to itself by going to /settings/page/phabricator/, clicking
"link" account, then cutting and pasting the pertinent ?code=X into
/oauth/phabricator/login/.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T852
Differential Revision: https://secure.phabricator.com/D1644
Summary:
Build the revision list table out of custom fields instead of hard-coding it, so
installs can add all sorts of zany things to it.
NOTE: You may need to implement sortFieldsForRevisionList() if you have a custom
DifferentialFieldSelector, or some fields might show up out of order.
This implementation will preserve the expected behavior:
public function sortFieldsForRevisionList(array $fields) {
$default = new DifferentialDefaultFieldSelector();
return $default->sortFieldsForRevisionList($fields);
}
Test Plan:
- Loaded differential revision list, identical to old list.
- Profiled page to verify the cost increase isn't significant (it's quite
small).
Reviewers: jungejason, btrahan
Reviewed By: btrahan
CC: aran, btrahan, davidreuss, epriestley
Maniphest Tasks: T773, T729
Differential Revision: https://secure.phabricator.com/D1388
Summary:
adds a Phabricator OAuth server, which has three big commands:
- auth - allows $user to authorize a given client or application. if $user has already authorized, it hands an authoization code back to $redirect_uri
- token - given a valid authorization code, this command returns an authorization token
- whoami - Conduit.whoami, all nice and purdy relative to the oauth server.
Also has a "test" handler, which I used to create some test data. T850 will
delete this as it adds the ability to create this data in the Phabricator
product.
This diff also adds the corresponding client in Phabricator for the Phabricator
OAuth Server. (Note that clients are known as "providers" in the Phabricator
codebase but client makes more sense relative to the server nomenclature)
Also, related to make this work well
- clean up the diagnostics page by variabilizing the provider-specific
information and extending the provider classes as appropriate.
- augment Conduit.whoami for more full-featured OAuth support, at least where
the Phabricator client is concerned
What's missing here... See T844, T848, T849, T850, and T852.
Test Plan:
- created a dummy client via the test handler. setup development.conf to have
have proper variables for this dummy client. went through authorization and
de-authorization flows
- viewed the diagnostics page for all known oauth providers and saw
provider-specific debugging information
Reviewers: epriestley
CC: aran, epriestley
Maniphest Tasks: T44, T797
Differential Revision: https://secure.phabricator.com/D1595
Summary: Update Phabricator for Remarkup changes in D1638.
Test Plan: Looked at various sorts of nested, enumerated, and phantom-item
lists.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, epriestley
Maniphest Tasks: T885
Differential Revision: https://secure.phabricator.com/D1639
Summary:
A few similar requests have come in across several tools and use cases that I
think this does a reasonable job of resolving.
We currently send one email for each update an object receives, but these aren't
always appreciated:
- Asana does post-commit review via Differential, so the "committed" mails are
useless.
- Quora wants to make project category edits to bugs without spamming people
attached to them.
- Some users in general are very sensitive to email volumes, and this gives us
a good way to reduce the volumes without incurring the complexity of
delayed-send-batching.
The technical mechanism is basically:
- Mail may optionally have "mail tags", which indicate content in the mail
(e.g., "maniphest-priority, maniphest-cc, maniphest-comment" for a mail which
contains a priority change, a CC change, and a comment).
- If a mail has tags, remove any recipients who have opted out of all the
tags.
- Some tags can't be opted out of via the UI, so this ensures that important
email is still delivered (e.g., cc + assign + comment is always delivered
because you can't opt out of "assign" or "comment").
Test Plan:
- Disabled all mail tags in the web UI.
- Used test console to send myself mail with an opt-outable tag, it was
immediately dropped.
- Used test console to send myself mail with an opt-outable tag and a custom
tag, it was delivered.
- Made Differential updates affecting CCs with and without comments, got
appropriate delivery.
- Made Maniphest updates affecting project, priority and CCs with and without
comments, got appropriate delivery.
- Verified mail headers in all cases.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley, moskov
Maniphest Tasks: T616, T855
Differential Revision: https://secure.phabricator.com/D1635
Summary:
Some user feedback:
- Named link information not present in quick reference.
- Named link information buried in Phriction docs.
- Optional omission of trailing "=" in headers not documented.
Fix these things.
Test Plan: generated, read documentation
Reviewers: btrahan, paularmstrong, Josereyes
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T884
Differential Revision: https://secure.phabricator.com/D1637
Summary:
We don't use maniphest or phriction in our install, so the links/references to
them in tactical command and jump nav can be confusing for users. This hides
these elements if they aren't enabled.
Test Plan: loaded the front page of phabricator in my sandbox, saw they went
away
Reviewers: epriestley, btrahan, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1636
Summary:
This gets added in D1595 (which hasn't landed yet), but was referred to in
D1632 (already committed). This unbreaks master for me.
Test Plan: I no longer get an error trying to load
PhabricatorOAuthProviderPhabricator
Reviewers: btrahan, epriestley
Reviewed By: epriestley
CC: jungejason, aran, epriestley
Differential Revision: https://secure.phabricator.com/D1633
Summary:
This is pretty simple and unpolished, but it's getting pretty big and it seems
like a reasonable starting point.
- Log chat in various "channels".
- Conduit record and query methods.
- IRCBot integration for IRC logging
Major TODO:
- Web UI is really unpolished and has no search, paging, anchor-linking, etc.
Basically all presentation stuff, though.
- I think the bot should have a map of channels to log with channel aliases?
- The "channels" should probably be in a separate table.
- The "authors" should probably be correlated to Phabricator accounts somehow,
where possible.
Test Plan: Used phabotlocal to log #phabricator.
Reviewers: kdeggelman, btrahan, Koolvin
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T837
Differential Revision: https://secure.phabricator.com/D1625
Summary:
add support for searching by package owner for Related Commits
and commits that Need Attention.
Test Plan:
verified that
- searching by package still works when there is or there is no commits
found
- searching by package owner works when there is or there is no commits
found
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: aran, epriestley, prithvi, dihde14, Girish
Differential Revision: https://secure.phabricator.com/D1631
Summary:
Getting ready to support searching for the related commits by
package owner (D1631):
- Add 'relative' option to the Nav Filter
- Refactor Owners page
Test Plan: - owners page still renders with the filter displayed correctly.
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1630
Summary:
- Restore quick methods for getting to common features (upload file, create
task, etc.)
- Provide a flexible cli-like navigation element similar to stuff used at
Facebook (bunny1 / lolbunny).
Test Plan: Used jump nav and nav buttons.
Reviewers: btrahan, fratrik
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1619
Summary:
Pretty straightforward; see title. Kind of gross but I have a bunch
more iterations in mind here (like filtering). Paging this is a little tricky
since we can't easily use AphrontPagerView, as it relies on OFFSET, and I think
that's sort of sketchy to use here for UX reasons (query performance and view
consistency as feed updates).
Test Plan: Looked at feed, paged through feed.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1616
Summary:
We sometimes call PhabricatorEnv::getProductionURI($file->getBestURI()) or
similar, but this may currently cause us to construct a URI like this:
http://domain.com/http://cdn-domain.com/file/data/xxx/yyy/name.jpg
Instead, if the provided URI has a domain already, leave it unmodified.
Test Plan: Attached a file to a task; got an email with a valid URI instead of
an invalid URI.
Reviewers: btrahan
Reviewed By: btrahan
CC: Makinde, aran, epriestley
Differential Revision: https://secure.phabricator.com/D1622
Summary: The effect of this is just to order tasks by (priority, modified)
instead of (modified), i.e. in the same default order as Maniphest, so the top
10 tasks here are the top 10 tasks in your assigned list.
Test Plan: Looked at "Assigned Tasks" on the homepage.
Reviewers: fratrik, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1621
triage tasks
Summary: The "with projects ... " query boils down to "all triage tasks" when
you don't belong to any projects. Just render the "no needs triage in projects
you are a member of" element unconditionally in this case.
Test Plan: Looked at homepage as a user with no project memberships but some
triage-requiring tasks before and after this change. Prior to this change, all
triage tasks show; afterwards, none.
Reviewers: fratrik, btrahan
Reviewed By: fratrik
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1620
Summary:
Provide a phid.query method that returns the same information as phid.info,
but allows querying for multiple phids at once.
Test Plan: Called the method from the web conduit console.
Reviewers: btrahan, epriestley, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1617
before displaying it
Summary:
@alok reported a vulnerability where Flash will run carefully-crafted plain text
files.
When the user requests a raw file, cache it into Files if it isn't already
there. Then redirect them to Files. This solves the problem by executing the
SWF/TXT with CDN-domain permissions, not content-domain permissions, provided
the install is correctly configured. (Followup diff coming to make this more
universally true.)
NOTE: We'll still show raw data in Diffusion. The barrier to XSS here is much
higher (you need commit access) but I'll do something similar there. We aren't
vulnerable in Paste, since we already use Files.
Test Plan: Clicked "View Old File", "View New File" in an alt-domain
configuration, got redirected to a cookie-free domain before being delivered the
response.
Reviewers: btrahan, alok
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1607
Summary: Add a "Search for ... in (document group)" thing that picks the current
scope based on the current application.
Test Plan: Conducted searches in several browsers.
Reviewers: btrahan, skrul
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T858
Differential Revision: https://secure.phabricator.com/D1610
Summary: The `file` binary doesn't exist everywhere, use the more flexible
wrapper introduce in D1609.
Test Plan: Uploaded a file via drag-and-drop, it got MIME'd correctly.
Reviewers: btrahan, davidreuss, Koolvin
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T869
Differential Revision: https://secure.phabricator.com/D1615
Summary:
I accidentally added two "104" patches. This actually works OK for the most part
but is fundamentally bad and wrong.
Merge the patches (installs applied both as "104", so we can't move one to
"105") and add a safeguard.
Test Plan: Ran upgrade_schema.php with two "104" patches, got error'd. Ran
without, got successs.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1614
required
Summary: Make these things like 1/4th the size if they aren't actionable.
Test Plan: Loaded home page with actionable, unactionable panels.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1613
Summary:
'this._request' was never set so 'waiting' was always false.
Result was that several requests were sent at once which wastes resources and
leads to weird bugs when responses don't arrive in sending order.
Blame Rev: D258
Test Plan:
Write a comment extremely fast, watch for requests sent.
Add sleep(5) for some inputs to DifferentialCommentPreviewController, verify
correct order.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1612
Summary: It makes perfect sense to add more reviewers while requesting review.
Test Plan:
Request review. Verify that Add Reviewers field shows and works.
Add some reviewer. Verify that comment preview works.
Submit. Verify that reviewers are saved and displayed.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1473
"Content-Disposition: attachment"
Summary:
We currently serve some files off the primary domain (with "Content-Disposition:
attachment" + a CSRF check) and some files off the alternate domain (without
either).
This is not sufficient, because some UAs (like the iPad) ignore
"Content-Disposition: attachment". So there's an attack that goes like this:
- Alice uploads xss.html
- Alice says to Bob "hey download this file on your iPad"
- Bob clicks "Download" on Phabricator on his iPad, gets XSS'd.
NOTE: This removes the CSRF check for downloading files. The check is nice to
have but only raises the barrier to entry slightly. Between iPad / sniffing /
flash bytecode attacks, single-domain installs are simply insecure. We could
restore the check at some point in conjunction with a derived authentication
cookie (i.e., a mini-session-token which is only useful for downloading files),
but that's a lot of complexity to drop all at once.
(Because files are now authenticated only by knowing the PHID and secret key,
this also fixes the "no profile pictures in public feed while logged out"
issue.)
Test Plan: Viewed, info'd, and downloaded files
Reviewers: btrahan, arice, alok
Reviewed By: arice
CC: aran, epriestley
Maniphest Tasks: T843
Differential Revision: https://secure.phabricator.com/D1608
Summary:
Some browsers will still sniff content types even with "Content-Type" and
"X-Content-Type-Options: nosniff". Encode "<" and ">" to prevent them from
sniffing the content as HTML.
See T865.
Also unified some of the code on this pathway.
Test Plan: Verified Opera no longer sniffs the Conduit response into HTML for
the test case in T865. Unit tests pass.
Reviewers: cbg, btrahan
Reviewed By: cbg
CC: aran, epriestley
Maniphest Tasks: T139, T865
Differential Revision: https://secure.phabricator.com/D1606
Summary:
I, as an author, sometimes forget branch associated with a revision.
Plus setting ##differential.show-host-field## makes a false sense of security
that branch will stay hidden so that I can name it
//finally_solve_this_crap_which_makes_no_sense//. But it is published in
Accepted and Request Changes e-mails anyway.
Test Plan: Display revision with disabled ##differential.show-host-field##.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1602
Summary:
As title.
Please help me to improve the wording!
Test Plan:
generate the documentation from the diviner file; read it; spell
check
Reviewers: epriestley, nh
Reviewed By: nh
CC: aran, dihde14, mpodobnik, prithvi, TomL, epriestley
Differential Revision: https://secure.phabricator.com/D1395
Summary:
@vrana patched an important external-CSRF-leaking hole recently (D1558), but
since we are sloppy in building this form it got caught in the crossfire.
We set action to something like "http://this.server.com/oauth/derp/", but that
triggers CSRF protection by removing CSRF tokens from the form. This makes OAuth
login not work.
Instead, use the local path only so we generate a CSRF token.
Test Plan: Registered locally via oauth.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran, epriestley, demo
Maniphest Tasks: T853
Differential Revision: https://secure.phabricator.com/D1597
Summary: Rough cut for Quora, we want this too eventually but it's super basic
right now so I'm not linking it anywhere. Once we get a couple more iterations
I'll put it in the UI.
Test Plan: Looked at stats for test data.
Reviewers: btrahan
Reviewed By: btrahan
CC: anjali, aran, epriestley
Differential Revision: https://secure.phabricator.com/D1594
Summary: Looping on this interface is pretty useful but you don't always want to
keep the projects/owners.
Test Plan: Clicked both buttons.
Reviewers: btrahan
Reviewed By: btrahan
CC: anjali, aran, epriestley
Differential Revision: https://secure.phabricator.com/D1593
Summary:
Rough cut that still needs a lot of polish, but replace the directory list with
more of a dashboard type thing:
- Show "Unbreak Now", triage-in-your-projects, and other stuff that you're
supposed to deal with, then feed.
- Move tools a click a way behind nav -- this also lets us put more stuff
there and subtools, etc., later.
- Remove tabs.
- Merge the category/item editing views.
- I also added a light blue wash to the side nav, not sure if I like that or
not.
Test Plan:
- Viewed all elements in empty and nonempty states.
- Viewed applications, edited items/categories.
Reviewers: btrahan, aran
Reviewed By: btrahan
CC: aran, epriestley, davidreuss
Maniphest Tasks: T21, T631
Differential Revision: https://secure.phabricator.com/D1574
Summary:
This was a sort of speculative feature added by a contributor some time ago and
just serves as a label; for now, simplify it into "active" and "archived" and
remove "archived" projects from the "active" list.
- Fix a bug where we'd publish a "renamed from X to X" transaction that had no
effect.
- Publish stories about status changes.
- Remove the "edit affiliation" controller, which has no links in the UI
(effectively replaced by join/leave links).
- Add query/conduit support.
Test Plan: Edited the status of several projects.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T681
Differential Revision: https://secure.phabricator.com/D1573
Summary:
We save search information and then redirect to a "/search/<query_id>/" URI in
order to make search URIs short and bookmarkable, and save query data for
analysis/improvement of search results.
Currently, there's a vague object enumeration security issue with using
sequential IDs to identify searches, where non-admins can see searches other
users have performed. This isn't really too concerning but we lose nothing by
using random keys from a large ID space instead.
- Drop 'authorPHID', which was unused anyway, so searches can not be
personally identified, even by admins.
- Identify searches by random hash keys, not sequential IDs.
- Map old queries' keys to their IDs so we don't break any existing bookmarked
URIs.
Test Plan: Ran several searches, got redirected to URIs with random hashes from
a large ID space rather than sequential integers.
Reviewers: arice, btrahan
Reviewed By: arice
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1587
Summary:
In D1515, I introduced some excessively-complicated semantics for detecting
connections that are lost while transactional. These semantics cause us to
reenter establishConnection() and establish twice as many connections as we need
in the common case.
We don't need a hook there at all -- it's sufficient to throw the exception
rather than retrying the query when we encounter it. This doesn't have
reentrancy problems.
Test Plan:
- Added some encapsulation-violating hooks and a unit test for them
- Verified we no longer double-connect.
Reviewers: btrahan, nh
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T835
Differential Revision: https://secure.phabricator.com/D1576
Summary:
This got caught in the crossfire when we admin-only'd the whole MetaMTA tool. It
should not be admin only.
(Generally, we should probably separate this out better at some point.)
Test Plan: Hit /mail/sendgrid/ as a logged-out, non-admin user (like SendGrid
does).
Reviewers: s, btrahan
Reviewed By: s
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1588
Summary:
The main purpose of this change is to allow selecting the branch by
triple-click.
Plus it is not perfectly clear that the text in brackets means branch.
Test Plan: Display revision.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1585
Summary:
See discussion in T838. These fields expose information which it isn't necessary
or useful to expose in the general case.
- Disable fields by default, allow them to be enabled in config (these fields
were useful for me at Facebook when I had access to all the machines).
- Remove 'sourcePath' from Conduit methods other than differential.query.
- Condition 'sourcePath' field in Conduit on the caller being the revision
author. This is a bit hacky but not so awful.
Test Plan:
- Verified fields are gone by default and restored by configuration.
- Verified Conduit no longer returns these fields other than
differential.query.
- Verified field presence/absence according to authorship in
differential.query.
- Grepped around in arcanist to make sure we aren't relying on sourcePath.
There's a workflow in "arc merge" that technically might hit it, but I think
it's unreachable, definitely irrelvant (we never use source path as a
distinguisher under git/hg, and can't 'arc merge' in SVN) and it's going away
Real Soon Now anyway.
Reviewers: btrahan, arice
Reviewed By: arice
CC: aran, epriestley
Maniphest Tasks: T838
Differential Revision: https://secure.phabricator.com/D1582
Summary:
See D1533#5.
Also deduplicates logic of what is stored to blob in ArcanistDiffWorkflow.
Blame Rev: D1533
Test Plan:
Display raw version of text file.
Display raw version of image.
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1583
Summary:
Escaped $id is compared with non-escaped $max_id.
Escaped $id is escaped again in phutil_render_tag().
Note: $id is numeric :-).
Test Plan: Display diff.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1580
Summary:
This addresses a few things:
- Provide a software HTTP response spliting guard as an extra layer of
security, see http://news.php.net/php.internals/57655 and who knows what HPHP/i
does.
- Cleans up webroot/index.php a little bit, I want to get that file under
control eventually.
- Eventually I want to collect bytes in/out metrics and this allows us to do
that easily.
- We may eventually want to write to a socket or do something else like that,
ala Litespawn.
Test Plan:
- Ran unit tests.
- Browsed around, checked headers and HTTP status codes.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1564
Summary: I've chosen passing callsign even if it is more complicated because it
is nicer than PHID and can be written by hand.
Test Plan:
Search without repository.
Search with repository.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1571
Summary:
This just looks silly:
{F8088, size=full}
It runs in O(N*N) but it's not a big deal because there are usually only few
comments per line.
I didn't implement it for images.
Test Plan:
View revision with compatible inline comments in two diffs.
View revision with different inline comments on same line in two diffs.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1570
Summary: Remarkup object names require #1 for linking to comments which is not
very intuitive.
Test Plan:
D1558#4e01328c
D1558#1
D1558#comment-1
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1565
Summary:
This code was just all kinds of wrong, but got all the common cases anyone cares
about correct.
- In edit-inline-comments.js, if isOnRight() is true, use data.right, not
data.left (derp).
- Set data.left correctly, not to the same value as data.right (derp derp).
- Set "isNewFile" based on $is_new, not $on_right (derp derp derp).
Test Plan:
- Added JS debugging code to print "OLD" vs "NEW" and "LEFT" vs "RIGHT".
Clicked the left and right sides of diff-vs-base and diff-vs-diff diffs,
verified output was accurate in all cases.
- Added comments to the left-display-side of a diff-of-diffs, saved them, they
showed up where I put them.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T543
Differential Revision: https://secure.phabricator.com/D1567
Summary:
add a static variable to the method and use it so we don't init more
than once!
Test Plan:
add a "phlog" and noted only init'd one time. verified "show more"
links worked correctly.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T666
Differential Revision: https://secure.phabricator.com/D1553
Summary:
Sending CSRF token in GET forms is dangerous because if there are external links
on the target page then the token could leak through Referer header.
The token is not required for anything because GET forms are used only to
display data, not to perform operations.
Sending CSRF tokens to external URLs leaks the token immediately.
Please note that <form action> defaults to GET.
PhabricatorUserOAuthSettingsPanelController suffered from this problem for both
reasons.
Test Plan: Save my settings (POST form).
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1558
Summary: Phabricator sends information about encoding in Content-Type header but
when I save the HTML page then this information is lost.
Test Plan: /
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1561
Summary:
The form doesn't perform any action, only displays data.
URL wouldn't exceed its maximum length.
Bookmarking results can be useful.
Linking from other pages can be even more useful.
Also browsing through history is more fluent.
Test Plan:
Search by path.
Search by owner.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1559
Summary:
Search tool currently allows only searching by substring which is useful.
But searching by the full path (for packages in which the path is contained) is
probably even more useful.
NOTE: I used a trick to perform the search by superstring.
Packages containing path '/' are found always which could seem strange but it is
correct after all.
Test Plan:
Search for /src/x/a.php. Found package with path /src/x/.
Search for /src/. Found package with path /src/x/.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1560
Summary: Also changes rCALL in package detail to bold CALL.
Test Plan:
/owners/view/all/, click on link
/owners/package/1/
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1550
Summary: This is not totally done yet, and i'm submitting for feedback.
Test Plan: Played with various settings in local conduit console.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T790
Differential Revision: https://secure.phabricator.com/D1555
Summary: This exposes a few remarkup engines over conduit.
Test Plan:
Local conduit console, and playing with
'cat example.json | arc call-conduit remarkup.process'
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1551
Summary:
@rguerin ran into an issue in his install where Phabricator appears to have
discovered commits which no longer exist, and thus is failing to proceed with
its repository import.
It's not clear how we got into this state. Previously, it was possible by, e.g.,
parsing a different repository's working copy and then switching them back, but
there are now safeguards against that.
I'm taking a three-pronged approach to try to sort this out:
- Provide a script to get out of this state (this script) and reconcile
Phabricator's view of a repository with an authoritative copy of it. This
basically "un-discovers" any discovered commits which don't actually exist (any
queued tasks to parse them will fail permanently when they fail to load the
commit object).
- Add more logging to the discovery daemon so we can figure out where commits
came from.
- Improve Diffusion's UI when stuff is partially discovered (T776).
(This script should also clean up some nonsense on secure.phabricator.com from a
botched Diviner import.)
Test Plan: Ran "reconcile.php" with bogus commits and bogus differential/commit
links, had them expunged. Will work with @rguerin to see if this resolves
things.
Reviewers: btrahan, rguerin
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1552
Summary:
This doesn't cover every case exhaustively (see comments) but should cover like
98% of the practical cases.
This makes one workflow modification: willWriteRevision() was previously
guaranteed to have a revisionID / revisionPHID and no longer is. I verified that
no field implementations depend on this behavior. Fields which depend on IDs
should be using didWriteRevision() instead.
Test Plan: Inserted a "throw" into the middle of the transactions and created
revisions; they didn't orphan. Created revisions normally, they worked
correctly.
Reviewers: btrahan, nh
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T605
Differential Revision: https://secure.phabricator.com/D1541
Summary: posix may not be loaded on the web/cgi SAPI but we call posix functions
on this pathway, which we hit on /daemon/. Fall back to exec if we don't have
posix.
Test Plan: Added "&& false" and verified the page executed a bunch of "ps"
tests.
Reviewers: Koolvin, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T821
Differential Revision: https://secure.phabricator.com/D1540
