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
Summary:
conduit was using getProductionURI instead of getURI for checking that the
request was sent to the correct host, which causes problems in some dev
environments
Test Plan:
echo '{}' | arc call-conduit --conduit-uri=mydevserver
where my dev server is configured with phabricator.production-uri pointing to
prod instead of my devserver
Reviewers: epriestley, btrahan, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1543
Summary: I think that these are the only links that are useful - commit which
deleted the path and last version of the path.
Test Plan: Display deleted path, click on links.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1536
Summary:
When a user selects "show raw file (right)" from the dropdown of a binary file
in differential, they should get more than a blank page.
Test Plan: Loaded a raw binary file from differential
Reviewers: tuomaspelkonen, epriestley, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1533
Summary: Since mailing list rules are now "global", don't run "personal" rules
for disabled/invalid users.
Test Plan: Added a personal rule that matches every revision for a test user.
Created a revision, checked transcript, rule matched. Disabled user, updated
revision, checked transcript, rule got auto-disabled.
Reviewers: btrahan, jungejason, nh, xela
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1517
Summary:
- Default "personal" vs "global" choice to "personal".
- Don't show global rules under "My Rules".
- After editing or creating a global rule, redirect back to global rule list.
- Use radio buttons for "personal" vs "global" and add captions explaining the
difference.
- For "global" rules, don't show the owner/author in the rule detail view --
they effectively have no owner (see also D1387).
- For "global" rules, don't show the owner/author in the rule list view, as
above.
- For admin views, show rule type (global vs personal).
Test Plan:
- Created and edited new global and personal rules.
- Viewed "my", "global" and "admin" views.
Reviewers: btrahan, jungejason, nh, xela
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1518
Summary: Provide explicit guidance in the documentation about liberal use of
"final".
Test Plan: Generated, read documentation.
Reviewers: btrahan, jungejason, aran, nh
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T795
Differential Revision: https://secure.phabricator.com/D1520
Summary:
Render coverage information in the right gutter, if available.
We could render some kind of summary report deal too but this seems like a good
start.
Test Plan:
- Looked at diffs with coverage.
- Looked at diffs without coverage.
- Used inline comments, diff-of-diff, "show more", "show entire file", "show
generated file", "undo". Nothing seemed disrupted by the addition of a 5th
column.
Reviewers: btrahan, tuomaspelkonen, jungejason
Reviewed By: btrahan
CC: zeeg, aran, epriestley
Maniphest Tasks: T140
Differential Revision: https://secure.phabricator.com/D1527
Summary: We were not correctly updating $diff as we iterated through the loop.
Test Plan: Viewed a revision several diffs that had differing base revision.
Reviewers: fratrik, btrahan
Reviewed By: fratrik
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1523
Summary:
Restores a (simplified and improved) version of Lisk transactions.
This doesn't actually use transactions anywhere yet. DifferentialRevisionEditor
is the #1 (and only?) case where we have transaction problems right now, but
sticking save() inside a transaction unconditionally will leave us holding a
transaction open for like a million years while we run Herald rules, etc. I want
to do some refactoring there separately from this diff before making it
transactional.
NOTE: @jungejason / @nh, can one of you verify these unit tests pass on
HPHP/i/vm when you get a chance? I vaguely recall there was some problem with
(int)$resource. We can land this safely without verifying that, but should check
before we start using it anywhere.
Test Plan: Ran unit tests.
Reviewers: btrahan, nh, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T605
Differential Revision: https://secure.phabricator.com/D1515
Summary:
pretty standard MO, but a little tricky in that we dynamically pre-pend
filters for "new", "edit", "search results" and "details" use cases.
Test Plan:
clicked around owners a bunch and verified proper filters showed up
and that when clicked they worked as expected
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T631
Differential Revision: https://secure.phabricator.com/D1516
Summary:
Reviews with empty summary are rendered like this:
Reviewers: ...
TEST PLAN
Test Plan:
Use empty summary.
Use non-empty summary.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1528
Summary: Provide some documentation for this feature since it's not super
obvious how it works.
Test Plan: Generated documentation, read documentation.
Reviewers: btrahan, vrana, jungejason, nh
Reviewed By: vrana
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1521
Summary:
Add a very basic edit history table to herald rules. This table is updated
whenever saving a herald rule. The contents of the save are not examined, and
the edit history contains no information about the rule itself *yet*. Edit
history can be viewed by anyone through /herald/history/<rule id>/.
Task ID: #
Blame Rev:
Test Plan:
Made a test rule, saved some stuff.
Revert Plan:
Tags:
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: zizzy, aran, xela, epriestley
Differential Revision: https://secure.phabricator.com/D1387
Summary:
- Use $this->linkTo($phid) to render all links.
- Simplify code.
Test Plan: Public feed renders with 'target="_top"' links. Nonpublic feed
doesn't. Looked at a bunch of feed stories, none seem broken.
Reviewers: btrahan, aran, nh, jungejason, ide
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T453
Differential Revision: https://secure.phabricator.com/D1514
Summary:
This diff restructures the DOM and alters some CSS within differential.
Original goal was to unify these codepaths more fully into a base class or
classes, but they have quite a bit of custom code such that didn't feel too
compelling in practice. It also felt related to feed stories as I thought
about the more general version(s) of this code...
Also deleted some CSS from maniphest that wasn't doing anything.
Test Plan:
looked at a differential diff and liked what I saw. spent a bunch
of time trying out different types of comments and etc.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T803
Differential Revision: https://secure.phabricator.com/D1513
Summary:
this has a single side nav now. added a Utilites section below the methods
which houses Logs and Token.
On logs I ended up deleting this whole concept of "view" and the existing side
nav -- I think there were plans to add a way to filter down to subset of the
conduit calls. For logs, I envision that being a separate first class tool when
/ if we think we need additional complexity.
On token I made the form FULL so it was like the rest of the views in this page.
Test Plan:
looks good! clicked on a few methods and it worked! clicked on the
logs and they were there! clicked on the pager within the logs and it worked!
checked out the token page and it looked good too.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T631
Differential Revision: https://secure.phabricator.com/D1499
Summary:
While sort of gross, this seems fairly reasonable overall? I guess?
(This patch clearly does more good than harm, although it could just do the good
without the harm.)
Test Plan: Clicked XHProf links from the frame and from the /xhprof/ tool.
Reviewers: btrahan, aran, jungejason, ide
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T453
Differential Revision: https://secure.phabricator.com/D1498
Summary: As per discussion with @johnduhart, improve documentation around
reusing and customizing linters.
Test Plan: Generated and read documentation.
Reviewers: btrahan, jungejason, johnduhart
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T795
Differential Revision: https://secure.phabricator.com/D1501
Summary: This is kind of confusing (you need to specify an export format) and
not very useful now that "arc patch" has gotten pretty good. I'm leaving the
field itself in case installs want to add it back or otherwise depend on it.
Test Plan: Looked at a revision, wasn't told to export it.
Reviewers: nh, btrahan, jungejason
Reviewed By: nh
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1507
Summary:
Fix two issues in PhabricatorRepositoryCommitOwnersWorker:
- if a commit was reviewed by some owner of the package, it should not be
marked as needing audit
- do not run herald worker when it is not needed (for example, when the
worker is executed from reparse.php)
Test Plan:
reparse a commit which is reviewed by the owner of a package
and verify that it is not marked as needing audit, and herald is not
executed.
Reviewers: nh, epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1496
Summary:
- Expose existing 'committed' filter.
- Add an 'accepted' filter.
- Fix a fatal where $repository may not be defined (for diffs not linked to a
repository).
Test Plan: Ran accepted / committed queries. Viewed a previously fataling diff.
Reviewers: btrahan, vrana, Makinde
Reviewed By: Makinde
CC: Koolvin, aran, epriestley
Differential Revision: https://secure.phabricator.com/D1490
Summary:
- Only write the <ruleID, phid> row if the rule is a one-time rule.
- Delete all the rows for rules which aren't one-time.
NOTE: This is probably like several million rows for Facebook and could take a
while.
Test Plan:
Added some one-time and every-time rules, ran them against objects, verified
only relevant rows were inserted.
Ran upgrade script against a database with one-time and every-time "ruleapplied"
rows, got the irrelevant rows removed.
Reviewers: nh, btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1484
Summary: Make it easy to join or leave (well, slightly less easy) a project.
Publish join/leave to feed. Fix a couple of membership editor bugs.
Test Plan: Joined, left a project.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T681
Differential Revision: https://secure.phabricator.com/D1485
Summary: We currently allow you to launch abstract daemons; use
setConcreteOnly() to only list/launch concrete daemons.
Test Plan: Ran "phd list" (no abstract daemons listed), "phd launch
PhabricatorRepositoryCommitDiscoveryDaemon" (reasonable error message).
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T801
Differential Revision: https://secure.phabricator.com/D1487
Summary: also fixes a small bug where the page title was always "Create Task".
switch it to the header name which is much more descriptive / correct IMO.
Test Plan:
created a new task and watched the description preview update.
edited an old task and saw the description preview populate with the correct
existing data.
edited an old task and edited the description and saw the description preview
update
Reviewers: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1489
Summary:
Herald rules may be marked as "one-time". We track this by writing a row with
<ruleID, phid> when we apply a rule.
However, the current test for rule application involves loading every <ruleID,
*> pair. We also always write this row even for rules which are not one-time, so
if there are 100 rules, we'll load 1,000,000 rows after processing 10,000
objects.
Instead, load only the <phid, *> pairs, which are guaranteed to be bounded to at
most the number of rules.
I'll follow up with a diff that causes us to write rows only for one-time rules,
and deletes all historic rows which are not associated with one-time rules.
Test Plan:
Grepped for callsites to loadAllByContentTypeWithFullData(). Ran
rules in test console.
Reviewers: nh, btrahan, jungejason
Reviewed By: nh
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1483
Summary:
It is possible to open a file in editor by registering a custom URI scheme
(pseudo-protocol). Some editors register it by default.
Having links to open the file in external editor is productivity booster
although it is a little bit harder to set up.
There are several other tools using file_link_format configuration directive
(XDebug, Symfony) to bind to this protocol.
I've added the example with editor: protocol which can be used as a proxy to
actual editor (used by Nette Framework:
http://wiki.nette.org/en/howto-editor-link).
Test Plan:
Configure Editor Link in User Preferences.
Register URI scheme in OS.
Open a file in Diffusion. Click on the Edit button.
Open a revision in Differential. Click on the Edit button.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1422
Summary:
Links from lint errors for large diffs don't work.
This diff adds TODO for it because I am not sure how to do it.
Move of changeset links rendering to a separate method would be still useful.
Test Plan:
Display ToC of large diff, verify link.
Repeat for small diff.
Reviewers: tuomaspelkonen, epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1476
Test Plan: Display revision containing comments with no content but with inline
comments.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1474
Test Plan:
Display revision with different lint and unit results.
Hover over the stars.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1475
Summary: This enables some improvements in D1478. Allow revisons to be queried
by the branch which they appear on.
Test Plan: Queried revisions by branch. Ran "arc which" branch queries in SVN
and Mercurial.
Reviewers: btrahan, cpiro, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T787
Differential Revision: https://secure.phabricator.com/D1479
Summary:
- Make some editing operations transaction-oriented, like Maniphest. (This
seems to be a good model, particularly for extensibility.) I'll move the rest of
the editing operations to transactions in future diffs.
- Make transaction-oriented operations publish feed stories.
Test Plan:
- Created a new project.
- Edited an existing project.
- Created a new project via quick create flow from Maniphest.
- Verified feed stories publish correctly.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T681
Differential Revision: https://secure.phabricator.com/D1477
Summary: I accidentally broke the feature where we highlight comments which are
jumped to via anchor in D1327. We now test that the jump was sucessful by
looking for an item with the anchor ID, but we were only setting 'name'.
Instead, set 'id' as well so the highlighting code detects that the jump was
successful and adds the highlight class.
Test Plan: Clicked "Comment D1234#7" or whatever, got a nice yellow background.
Reviewers: btrahan, nh, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T796
Differential Revision: https://secure.phabricator.com/D1471
Summary: Run the actual resource allocation for Drydock out-of-process via the
task queue.
Test Plan: Ran "drydock_control.php", saw it insert a task and wait for task
completion. Ran "phd debug taskmaster" and saw it run the task.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1470
Summary: See T709. I also ran into a case in Drydock where this is useful for
testing/development.
Test Plan: Freed lease of a task; deleted a task.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T709
Differential Revision: https://secure.phabricator.com/D1469
Summary:
- Even for immutable-history Git workflows, we suggest "arc amend". Instead,
suggest "arc amend" or "arc merge" (ideally we'd know which, but we can't
currently get that information).
- We suggest "arc amend --revision X", but this is less safe and less simple
than "arc amend", especially after D1480.
- For Mercurial, suggest "arc merge".
Test Plan: Looked at some "Accepted" revisions.
Reviewers: btrahan, jungejason
CC: aran, epriestley
Maniphest Tasks: T662
Differential Revision: https://secure.phabricator.com/D1481
Summary: You can order by Modified but the table has Updated column.
Test Plan: /differential/filter/reviews/
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1472
Summary:
D1449 removed HeraldActionConfig::getActionMap(), but it was still used in
HeraldTranscriptController. This fixes the controller to use the method that
replaced getActionMap.
Test Plan: loaded a herald transcript
Reviewers: epriestley, xela
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D1466
Summary:
They are present in the document so there is no reason to omit the links to
them.
Similar to D1412.
Test Plan: Display commit.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D1464
Summary:
We retried if a db connection was lost when executing a query, but not when
establishing a connection. I've seen a lot of failures establishing connections
in our install (they go away when retrying), so this diff retries when
establishing connections, and logs when we retry.
Test Plan:
- Loaded phabricator in a sandbox
- Temporarily added a check in the try block to throw if there were still
retries (to test logging, retry logic)
Reviewers: epriestley, blair
Reviewed By: epriestley
CC: aran, btrahan
Differential Revision: https://secure.phabricator.com/D1460
Summary:
Rough cut of Drydock. This is very basic and doesn't do much of use yet (it
//does// allocate EC2 machines as host resources and expose interfaces to them),
but I think the overall structure is more or less reasonable.
== Interfaces
Vision: Applications interact with Drydock resources through DrydockInterfaces,
like **command**, **filesystem** and **httpd** interfaces. Each interface allows
applications to perform some kind of operation on the resource, like executing
commands, reading/writing files, or configuring a web server. Interfaces have a
concrete, specific API:
// Filesystem Interface
$fs = $lease->getInterface('filesystem'); // Constants, some day?
$fs->writeFile('index.html', 'hello world!');
// Command Interface
$cmd = $lease->getInterface('command');
echo $cmd->execx('uptime');
// HTTPD Interface
$httpd = $lease->getInterface('httpd');
$httpd->restart();
Interfaces are mostly just stock, although installs might add new interfaces if
they expose different ways to interact with resources (for instance, a resource
might want to expose a new 'MongoDB' interface or whatever).
Currently: We have like part of a command interface.
== Leases
Vision: Leases keep track of which resources are in use, and what they're being
used for. They allow us to know when we need to allocate more resources (too
many sandcastles on the existing hosts, e.g.) and when we can release resources
(because they are no longer being used). They also give applications something
to hold while resources are being allocated.
// EXAMPLE: How this should work some day.
$allocator = new DrydockAllocator();
$allocator->setResourceType('sandcastle');
$allocator->setAttributes(
array(
'diffID' => $diff->getID(),
));
$lease = $allocator->allocate();
$diff->setSandcastleLeaseID($lease->getID());
// ...
if ($lease->getStatus() == DrydockLeaseStatus::STATUS_ACTIVE) {
$sandcastle_link = $lease->getInterface('httpd')->getURI('/');
} else {
$sandcastle_link = 'Still building your sandcastle...';
}
echo "Sandcastle for this diff: ".$sandcastle_link;
// EXAMPLE: How this actually works now.
$allocator = new DrydockAllocator();
$allocator->setResourceType('host');
// NOTE: Allocation is currently synchronous but will be task-driven soon.
$lease = $allocator->allocate();
Leases are completely stock, installs will not define new lease types.
Currently: Leases exist and work but are very very basic.
== Resources
Vision: Resources represent some actual thing we've put somewhere, whether it's
a host, a block of storage, a webroot, or whatever else. Applications interact
through resources by acquiring leases to them, and then getting interfaces
through these leases. The lease acquisition process has a side effect of
allocating new resources if a lease can't be acquired on existing resources
(e.g., the application wants storage but all storage resources are full) and
things are configured to autoscale.
Resources may themselves acquire leases in order to allocate. For instance, a
storage resource might first acquire a lease to a host resource. A 'test
scaffold' resource might lease a storage resource and a mysql resource.
Not all resources are auto-allocate: the entry-level version of Drydock is that
you manually allocate a couple boxes and configure them through the web console.
Then, e.g., 'storage' / 'webroot' resources allocate on top of them, but the
host pool itself does not autoscale.
Resources are completely stock, they are abstract shells representing any
arbitrary thing.
Currently: Resource exist ('host' only) but are very very basic.
== Blueprints
Vision: Blueprints contain instructions for building interfaces to, (possibly)
allocating, updating, managing, and destroying a specific type of resource in a
specific location. One way to think of them is that they are scripts for
creating and deleting resources. For example, the LocalHost, RemoteHost and
EC2Host blueprints can all manage 'host' resources.
Eventually, we will support more types of resources (storage, webroot,
sandcastle, test scaffold, phacility deployment) and more providers for resource
types, some of which will be in the Phabricator mainline and some of which will
be custom.
Blueprints are very custom and specific to application types, so installs will
define new blueprints if they are making significant use of Drydock.
Currently: They exist but have few capabilities. The stock blueprints do nearly
nothing useful. There is a technically functional blueprint for host allocation
in EC2.
== Allocator
This is just the actual code to execute the lease acquisition process.
Test Plan: Ran "drydock_control.php" script, it allocated a machine in EC2,
acquired a lease on it, interfaced with it, and then released the lease. Ran it
again, got a fresh lease on the existing resource.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D1454
Summary:
We show the contextual branch (always the repository default branch) when
viewing a commit. Instead, show all branches the commit appears on.
Also pull some of the duplicated DiffusionXQuery stuff into a DiffusionQuery
base class, I'll do a followup to reduce more duplication.
Test Plan: Looked at a commit in Git. My HG and SVN setups are a little borked
so I kind of faked tests in them -- I'm fixing them now.
Reviewers: btrahan, jungejason, fratrik
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T768
Differential Revision: https://secure.phabricator.com/D1458
Summary: getBestURI() = best URI
Test Plan:
It says "best" in the name so it must be the best!
Also in Maniphest emails we'll link you to /view/ even for binaries and other
non-viewable content.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: anjali, aran
Differential Revision: https://secure.phabricator.com/D1461
Summary:
A personal rule only has actions targeting the owner. Likewise, only they can
edit the rule. OTOH, a global may affect any target and is editable by anyone.
There are no new action types. Instead, type of the rule modifies the available
targets and the messaging in the ui. This is beneficial because herald rule
adapters don't need to be aware of the difference between emailing the owner of
a personal rule and emailing an arbitrary user.
This diff sets up the logic and ui for creating personal/global rules. All
existing rules have been defaulted to global.
TODO: Filter all existing rules into personal/global
TODO: Create a UI for surfacing (relevant?) global rules.
Test Plan:
1. Created a personal rule to email myself. Created a dumby revision satisfying
the conditions of that rule. Verified that I recieved a herald email.
2. Removed my adminship, change the owner of a personal rule. verified that I
couldn't edit the rule.
3.Changed rule type to global. verified that I could edit the rule.
4. Verified that admins can edit both global and personal rules.
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, zizzy
Differential Revision: https://secure.phabricator.com/D1449
Summary:
Not all auto-generated files can include the magical
"generated" annotation for one reason or another, but they may follow
path rules. This patch allows files to be marked as automatically
generated by matching the path with a regular expression.
Test Plan:
Alter 'differential.generated-paths' setting in config.
Create a new diff that affects a file matching one of those regular
expressions. Verify that Differential marks it as automatically
generated and therefore probably not worth reviewing (in the same way as
the magical "generated" annotation.
Reviewers: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D1455
Summary: It was broken by D!352
Test Plan: Praying that it works.
Reviewers: nh, epriestley, andrewjcg
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D1453
your own actions
Summary:
- Mail.app on Lion has cumbersome threading rules, see T782. Add an option to
stick "Re: " in front of all threaded mail so it behaves. This is horrible, but
apparently the least-horrible option.
- While I was in there, I added an option for T228.
Test Plan:
- Sent a bunch of threaded and unthreaded mail with varous "Re:" settings,
seemed to get "Re:" in the right places.
- Disabled email about my stuff, created a task with just me, got voided mail,
added a CC, got mail to just the CC.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, mkjones
Maniphest Tasks: T228, T782
Differential Revision: https://secure.phabricator.com/D1448
Summary:
Added a typeahead in the edit herald rule page that allows an admin or
owner to change the current owner of a rule. If the typeahead is emptied, the
current owner will remain owner.
Test Plan:
Created a test rule. Changed the owner. Deleted the owner in the
typahead. Verified expected behavior.
Reviewers: jungejason, epriestley
Reviewed By: epriestley
CC: aran, jungejason, epriestley, xela
Differential Revision: https://secure.phabricator.com/D1322
Summary: Add a conduit method to query project information.
Test Plan: Ran method from API test console.
Reviewers: bill, btrahan, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T681
Differential Revision: https://secure.phabricator.com/D1444
Summary: There are lots of callsites to $changeset->getFilename() so it seemed
easier to rename getFileName() to getFilename() even if it includes database
change. Plus I think that getFilename() is better.
Test Plan:
Alter database.
Open revision.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D1437
Summary: The <a href> attribute is useful because user knows where the link goes
before opening it plus he can copy it to the clipboard plus he can add it to the
bookmarks.
Test Plan:
Display revision.
View Options.
Click.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D1436
Summary: See D1433.
Test Plan: Created a new diff with a line >80chars, observed it wrapping
correctly.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D1438
Summary:
We currently allow you to assign code review to disabled users, but
should not.
Test Plan:
- Created revisions with no reviewers and only disabled reviewers, was
appropriately warned.
- Looked at a disabled user handle link, was clearly informed.
- Tried to create a new revision with a disabled reviewer, was rebuffed.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D1429
Summary:
enable herald commit rules to have access to auditing info.
Note that the new herald condition I added contains info for the
packages. I thought about using a simpler herald condition like
"Requires audit is true or false" and let it work together with the
existing "Affected package contains any of the package". It doesn't work
because we need the info about the package to decide if the commit
requires audit, but the herald conditions work separately.
Test Plan:
- A commit requiring auditing was detected by a herald rule that checks
the auditing status
- A commit not requiring auditing was not detected by a herald rule
which checks auditing status, but was detected by a rule which doesn't
check the auditing status
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1399
Summary: This is never read anywhere and clearly has no effect.
Test Plan: grep
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D1434
Summary: These blocks do nothing. end() produces a side effect on the internal
array pointer, but the code does not depend on it.
Test Plan: Reasoned about the code? Also viewed some diffs.
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D1432
Summary: No callsites anywhere. Unclear what this method is even supposed to do.
Test Plan: grep
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D1435
usefully-named file
Summary:
If you Command-L + Option-Return to download stuff off, e.g., Paste,
you get "PHID-FILE-ad98abg9bsd9ashbs.txt" in your download folder. Put the file
name in the URI instead, so you get a reasonably named file.
Test Plan: Downloaded some files, got reasonable results.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D1427
Summary:
A couple of people mentioned that they've had users accidentally upload
sensitive files. Allow files to be deleted.
(At some point it might be nice to keep the file handle around and log who
deleted it, but this addresses the immediate problem without needing too much
work.)
Test Plan: Deleted some files.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T780
Differential Revision: https://secure.phabricator.com/D1423
Summary:
/diffusion/X/history/?copies=0 is same as /diffusion/X/history/
/countdown/1/?chrome=1 is same as /countdown/1/
Test Plan:
Visit /diffusion/X/history/, click on Show/Hide Copies/Branches twice.
Visit /countdown/1/, click on Disable/Enable Chrome twice.
Reviewers: epriestley, tuomaspelkonen
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D1424
Summary: When we try to kill a daemon but discover it isn't running, we should
remove the PID file. We can also simplify the logic here.
Test Plan: Ran "phd stop" a couple of times, subsequent runs did not try to stop
a legion of dead daemons.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T781
Differential Revision: https://secure.phabricator.com/D1421
Summary:
See D1416. Add options to file-embed syntax, and document new code and
embed options.
Test Plan: Used new options in markup blocks.
Reviewers: davidreuss, btrahan, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T336
Differential Revision: https://secure.phabricator.com/D1417
Summary: Revisit of D1254. Don't require lowercase, just standardize the logic.
The current implementation has nonuniform logic -- PeopleEditController forbids
uppercase.
Test Plan: Ran unit tests, see also D1254.
Reviewers: btrahan, jungejason, aran
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D1415
Summary: Make it more explicit that headers are block formatters, see T778.
Test Plan: Read docs.
Reviewers: davidreuss, btrahan, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T778
Differential Revision: https://secure.phabricator.com/D1420
Summary:
By default, PHP-FMP (an alternate PHP FCGI SAPI) cleans the entire environment
for child processes. This means we have no $PATH.
This causes some confusing failures for reasons I don't fully understand. If you
do these things:
exec_manual('env');
exec_manual('export');
...they show no $PATH, as expected. If you do this:
exec_manual('echo $PATH');
...it shows a path. And this works (i.e., it finds the executable):
exec_manual('ls');
...but this fails (it says "no ls in ((null))"):
exec_manual('which ls');
So, basically, the sh -c process itself gets a default PATH somehow, but its
children don't. I don't realllly get why this happens, but clearly an empty
$PATH is a misconfiguration, and can easily be remedied.
See discussion here: https://github.com/facebook/libphutil/issues/7
Test Plan: Applied patch to Centos6 + nginx + PHP-FPM machine, ran setup, the
configuration issue was detected and I was given information on resolving it.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D1413
Summary:
A few people in IRC have been having issues here recently. If you misconfigure
the IRC bot, e.g., you get a 200 response back with a bunch of login HTML in it.
This is unhelpful.
Try to detect that a conduit request is going to the wrong path and raise a
concise, explicit error which is comprehensible from the CLI.
Also created a "PlainText" response and moved the IE nosniff header to the base
response object.
Test Plan: As a logged-out user, hit various nonsense with "?__conduit__=true"
in the URI. Got good error messages. Hit nonsense without it, got login screens.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T775
Differential Revision: https://secure.phabricator.com/D1407
Summary:
They are present in the document so there is not reason to omit the links to
them.
They sometimes contains changed lines so the link could be actualy useful.
Test Plan: Display ToC of revision with moved and copied files.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, nh
Differential Revision: https://secure.phabricator.com/D1412
Test Plan:
Open menu for added file
Open menu for deleted file
Open menu for changed file
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1410
Summary: D1354 added a query for a possibly-empty list -- only show the table if
there are transformations.
Test Plan: Reloaded a previously-fataling page, no fatals. Viewed a file with
transformations, got a list.
Reviewers: davidreuss, btrahan, jungejason
Reviewed By: davidreuss
CC: aran, davidreuss
Differential Revision: https://secure.phabricator.com/D1414
Summary: I've also moved the response generation for 404 from
##AphrontDefaultApplicationConfiguration## to ##buildResponseString()##
Test Plan:
Visit /
Visit /mail/
Visit /x/
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, vrana
Differential Revision: https://secure.phabricator.com/D1406
Summary:
See T730 and the slightly-less-pretty version of this in D1398.
When a user takes an action in Differential that has no effect (for instance,
accepting an already-accepted revision), prompt them:
Action Has No Effect
You can not accept this revision because it has already been accepted.
Do you want to post the feedback anyway, as a normal comment?
[Cancel] [Post as Comment]
If they have no comment text, the dialog only says "Cancel".
I think this is probably the best way to balance all the concerns here -- it
might occasionally be a little annoying, but that should be rare, and it should
never be confusing (the current workflow is extremely confusing).
This also fixes the issue where you can add all sorts of CCs who are already
part of the revision, either explicitly or via mentions.
Test Plan:
Posted some has-effect and has-no-effect comments, made different
choices in the dialog, everything seems to work OK?
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran, vrana
Maniphest Tasks: T730
Differential Revision: https://secure.phabricator.com/D1403
Summary:
I always forget a branch which I used for the diff so that I must open
my browser which takes some time. This diff adds the name of the branch to the
sent e-mails. But only if the diff is in the state Accepted or Needs Revision to
not pollute other e-mails.
Test Plan:
Comment
Request changes
Accept
Look at the e-mails
Reviewers: epriestley
Reviewed By: epriestley
CC: olivier, aran, epriestley, vrana
Differential Revision: https://secure.phabricator.com/D1396
Test Plan:
Display diff with lint errors
Click on a line number in lint errors overview
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1400
Summary:
Commenting on a diff causes adding the writer to the CCs. It doesn't make much
sense if the writer is author or reviewer who get all the copies anyway.
I've also moved the decision to DifferentialCommentEditor.
Test Plan:
Comment on a diff where I am author
Comment on a diff where I am reviewer
Comment on a diff where I am neither
Explicitely Add CCs where I am author
Reviewers: epriestley
Reviewed By: epriestley
CC: jungejason, aran, epriestley
Differential Revision: https://secure.phabricator.com/D1397
Summary:
I locked this down a little bit recently, but make
double-extra-super-sure that we aren't sending the user anywhere suspicious or
open-redirecty. This also locks down protocol-relative URIs (//evil.com/path)
although I don't think any browsers do bad stuff with them in this context, and
header injection URIs (although I don't think any of the modern PHP runtimes are
vulnerable).
Test Plan:
- Ran tests.
- Hit redirect page with valid and invalid next URIs; was punted to / for
invalid ones and to the right place for valid ones.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: arice, aran, epriestley, btrahan
Differential Revision: https://secure.phabricator.com/D1369
Summary:
- PHP uses a SAPI ("server API") to determine how it interacts with the caller
(e.g., how to read the environment, how to read flags, what code to execute).
- There are several different SAPIs: cli, cgi, cgi-fcgi, apache, etc.
- Each SAPI has different behavior -- for instance, the "cgi" SAPI emits some
CGI headers unless told not to, so a script like 'echo "x"' actually echoes some
headers and then 'x' as an HTTP body.
- In some setups, "php" may be php-cgi.
- If you run php-cgi as "php scriptname.php" and your ENV has an existing CGI
request in it, it runs that CGI request instead of the script. This causes an
infinite loop.
- Add checks to verify that "php" is the "cli" SAPI binary, not some other
SAPI.
- In particular, cPanel uses suphp and is affected by this configuration
issue. See this thread:
https://lists.marsching.com/pipermail/suphp/2008-September/002036.html
Test Plan:
- On a cPanel + suphp machine, ran setup and was stopped for having the
"cgi-fcgi" SAPI instead of throw into an infinite loop.
- Applied the suggested remedy, setup now runs fine.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan, epriestley
Differential Revision: https://secure.phabricator.com/D1390
Summary: ..."ssh" is in quotes 'cuz this is step 1 and there's no ssh in sight
at the moment.
Test Plan:
ran api.php PHID-USER-xee4ju2teq7mflitwfcs differential.query a few times...
- tried valid input, it worked!
- tried bad input, it worked in that it failed and told me so!
ran api.php crap_user differential.query a few times...
- verified error message with respect to crap_user
ran api.php PHID-USER-xee4ju2teq7mflitwfcs crap_method a few times...
- verified error message with respect to crap_method
visited http://phabricator.dev/conduit/method/differential.query a few times...
- tried valid input, it worked!
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, btrahan, epriestley
Maniphest Tasks: T550
Differential Revision: https://secure.phabricator.com/D1357
Summary: See T773 and the explanatory inline comment.
Test Plan: Made no-action comments and comments that did something (reject, plan
changes) to revisions. Saw them always jump to the top of the action list.
Reviewers: jungejason, simpkins, btrahan
Reviewed By: jungejason
CC: aran, jungejason
Maniphest Tasks: T773
Differential Revision: https://secure.phabricator.com/D1386
Summary:
- Link to "importing a repository" from Config next steps, since it's not
obvious (and the article isn't obviously named).
- Some minor doc tweaks.
- Remove "Roadmap" document since it's super out of date and not very useful.
Test Plan: Regenerated and read documentation.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, jungejason
Maniphest Tasks: T743
Differential Revision: https://secure.phabricator.com/D1384
Summary:
- When a user is creating a Phriction document, save a draft as
"phriction:<slug>".
- When a user is editing a Phriction document, save a draft as "<document
phid>:<document version>".
- If a user has an available draft, use that instead of the native content.
- If using a draft, tell the user and give them an option to discard it.
- If a page is updated, your draft is lost (we show new page content
unconditionally) but this should be rare and is the simplest way to resolve this
issue in a realtively consistent way.
Test Plan:
- Recovered drafts for new and edited pages.
- Used "nodraft" to discard drafts.
Reviewers: davidreuss, btrahan, jungejason
Reviewed By: davidreuss
CC: aran, davidreuss
Maniphest Tasks: T769
Differential Revision: https://secure.phabricator.com/D1378
Summary:
engineers requested to supporting filtering by 'committed'
revisions, and I think it makes sense.
Test Plan: verified that all the three options worked
Reviewers: epriestley, btrahan, nh
Reviewed By: nh
CC: nh, wolffiex, aran
Differential Revision: https://secure.phabricator.com/D1383
Summary: If a remote address has too many recent login failures, require they
fill out a captcha before they can attempt to login.
Test Plan: Tried to login a bunch of times, then submitted the CAPTHCA form with
various combinations of valid/invalid passwords and valid/invalid captchas.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, epriestley, jungejason
Maniphest Tasks: T765
Differential Revision: https://secure.phabricator.com/D1379
Summary:
- We currently run ##parseValueFromCommitMessage()## on all fields present in
the message, but not ##validateField()##.
- This detects value errors (e.g., an invalid reviewer) but not higher-level
errors (e.g., a missing field).
- This can break the stacked-commits Git mutable history workflow by
recognizing too many commit messages as valid ("multiple valid commit messages,
this is ambiguous").
- This also gives you some errors ("Missing test plan") too late in "arc diff
--create" (after the diff has been built).
Test Plan:
- Grepped for validateField() calls, removed a couple of calls that had the
same implementation as the base class.
- Grepped for other calls to this to make sure I'm not stumbling into
unintended side effects, but it only runs from the diff workflow.
- Ran "arc diff --create" with an invalid test plan, got a good error early in
the process.
- Ran "arc diff master" with stacked local commits, got a correct selection of
the intended message.
Reviewers: cpiro, btrahan, jungejason
Reviewed By: cpiro
CC: aran, cpiro
Differential Revision: https://secure.phabricator.com/D1373
interfaces
Summary:
- We have a hard-coded minimum length of 3 right now (and 1 in the other
interface), which is sort of silly.
- Provide a more reasonable default, and allow it to be configured.
- We have two password reset interfaces, one of which no longer actually
requires you to verify you own the account. This is more than a bit derp.
- Merge the interfaces into one, using either an email token or the account's
current password to let you change the password.
Test Plan:
- Reset password on an account.
- Changed password on an account.
- Created a new account, logged in, set the password.
- Tried to set a too-short password, got an error.
Reviewers: btrahan, jungejason, nh
Reviewed By: jungejason
CC: aran, jungejason
Maniphest Tasks: T766
Differential Revision: https://secure.phabricator.com/D1374
Summary:
Until T605 gets fixed, you might end up with a Project without a Profile if the
Profile insert failed. This fatals the list view; instead, don't fatal if a
profile is missing.
(At some point we should probably just merge this field into the Project object,
I was just mimicking the user/profile separation but we have partial-field
object support now and Projects aren't super heavily used or very big.)
Test Plan:
- Viewed list view including a project with a missing profile.
- Edited the project, creating its profile.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: arice, aran, btrahan
Differential Revision: https://secure.phabricator.com/D1368
Summary:
With T764, http://localhost doesn't work anymore. So add instructions
about how to support it by modifying the hosts file.
Test Plan:
- turned on setup mode and the error message did show up
- turned off the setup mode and the error message also showed up
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T764
Differential Revision: https://secure.phabricator.com/D1370
Summary:
- Add some captions to make it more clear what these fields mean.
- Require "name", since tokenizers use it exclusively.
- Limit URI to allowed protocols, since admins can currently XSS users by
entering a "javascript:" URI and then tricking the user into clicking the
mailing list name. This exploit is dumb, but technically privilege escallation.
Test Plan:
- Created a new mailing list.
- Edited a mailing list.
- Tested URI: valid, invalid, omitted.
- Tested name: valid, omitted.
Reviewers: btrahan, jungejason, davidreuss
Reviewed By: btrahan
CC: aran, btrahan
Differential Revision: https://secure.phabricator.com/D1365
Summary:
Added a Conduit API method to return all transactions for a
given set of task_ids. This will be used to comments and other important
information about the tasks.
Test Plan:
Use Conduit to execute ##maniphest.gettasktransactions## and
visually verify that transaction information is returned.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1361
Summary: @s reported an issue with implicit file:// URIs in Git, see P270.
Recognize and handle URIs in this format. For URIs we don't understand, raise an
exception.
Test Plan:
- Added failing tests.
- Fixed code.
- Tests pass.
Reviewers: btrahan, jungejason, s
Reviewed By: s
CC: aran, epriestley, s
Differential Revision: https://secure.phabricator.com/D1362
Summary:
- There are some recent reports of login issues, see T755 and T754. I'm not
really sure what's going on, but this is an attempt at getting some more
information.
- When we login a user by setting 'phusr' and 'phsid', send them to
/login/validate/ to validate that the cookies actually got set.
- Do email password resets in two steps: first, log the user in. Redirect them
through validate, then give them the option to reset their password.
- Don't CSRF logged-out users. It technically sort of works most of the time
right now, but is silly. If we need logged-out CSRF we should generate it in
some more reliable way.
Test Plan:
- Logged in with username/password.
- Logged in with OAuth.
- Logged in with email password reset.
- Sent bad values to /login/validate/, got appropriate errors.
- Reset password.
- Verified next_uri still works.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan, j3kuntz
Maniphest Tasks: T754, T755
Differential Revision: https://secure.phabricator.com/D1353
Summary:
Chrome/Chromium won't set cookies on these domains, at least under
Ubuntu. See T754. Detect brokenness and explode.
Test Plan:
Logged into phabricator as "http://derps/" (failed) and
"http://derps.com/" (worked) in Chromium. Set config to "http://derps/" (config
exploded) and "http://local.aphront.com/" (config OK).
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T754
Differential Revision: https://secure.phabricator.com/D1355
Summary:
Just talked to @tuomaspelkonen, and turns out there is a case where
postponed tests results use the filepath for both the name and file
parameters. Then, after the tests have completed, the unittest
results are updated with the class name as the test name. To handle
this, this diff matches the stored unittest results name against
either the name or file component of the updated unittest info.
Not sure of great way to generally handle these situations. Perhaps,
long term, we can just use a placeholder unittest result, mark that
as passed (or delete it?) then add a new test result with the correct
name.
Test Plan: updated unittest result with new name (but file was the same).
Reviewers: epriestley, tuomaspelkonen
Reviewed By: epriestley
CC: aran, epriestley, andrewjcg
Differential Revision: https://secure.phabricator.com/D1356
Summary:
When using postponed unittests to make 'arc diff' faster, there
are some situations where it is difficult to know exactly how
many unittests will be run. This is the case for many of our
C++ unittests, which we can't really know until we compile the
tests (which is slow, and probably isn't reasonable to be done
before posting the diff). I suppose we could make sure we
explicitly which tests a C++ unittest will run in some way, but
this would require a lot of change to our backend test infra.
Also, it seems that this is a pretty general issue of not knowing
how many unittests will be run until they actually run.
This diff adds an optional "create" parameter to updateunitresults
which wil create a new unit tests result rather than updating an
existing one. I am not sure if this really fits here or should
be its own method, but there is a lot of code re-use between them
so I consolidated.
Test Plan: updated a diff with a new unit test result
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, epriestley, andrewjcg, tuomaspelkonen
Differential Revision: https://secure.phabricator.com/D1352
Summary:
we used to need this function for security purposes, but no longer need
it. remove it so that some call sites can be optimized via smarter data
fetching, and so the whole codebase can have one less thing in it.
Test Plan:
verified the images displayed properly for each of the following
- viewed a diff with added images.
- viewed a user feed
- viewed a user profile
- viewed all image macros
- viewed a paste and clicked through "raw link"
weakness in testing around proxy files and transformed files. not sure what
these are. changes here are very programmatic however.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, btrahan, epriestley
Maniphest Tasks: T672
Differential Revision: https://secure.phabricator.com/D1354
Summary: When private messaged, the bot responds via private message to the
sender, instead of sending a private message to itself.
Test Plan: Mentioned tasks in public channels and private messages.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T274
Differential Revision: https://secure.phabricator.com/D1350
Phabricator
Summary: ...this breaks without D1328. Used good ole "codemod" to do this
work, with lots of manual edits around 80 chars.
Test Plan: clicked around phabricator tool suite, particular differential, a
bunch
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1351
Summary: We need some additional fields to heuristically match revisions to the
working copy in arc.
Test Plan: Executed conduit method, got correct values in fields
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: https://secure.phabricator.com/D1347
Summary: XHProf install documentation went missing a month or two ago (see T725)
and doesn't work in the widely deployed versions of PEAR/PECL. Provide
build-from-source instructions inline.
Test Plan: Generated, read documentation.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T725
Differential Revision: https://secure.phabricator.com/D1345
Summary: Simple notificaiton handler that reads the difx event timeline and
posts notifications to IRC.
Test Plan: Ran it in #phabricator.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Differential Revision: https://secure.phabricator.com/D1337
Summary:
The filename header for inline comments used to span 2 columns - the line number
and the comment. With the addition of a column for the diff (to link to inline
comments on previous diffs), the filename header should now span 3 columns
instead of just the line number and diff, leaving the comment squished to the
right.
Test Plan:
Opened a differential revision with an inline comment from a previous diff, and
saw that the filename header continued across the comment. Also checked an
inline comment on a current diff, and saw that it looks fine.
Reviewers: epriestley, btrahan, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1340
Summary: Not really thrilled about my fix for T684 in D1224. This makes some
design tweaks to solve it without the awkward horizontal scrollbar in the page
content div.
Test Plan: Looked at diffs overflowing the window. Looked at footer on several
pages.
Reviewers: btrahan, jungejason, Makinde
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T684
Differential Revision: https://secure.phabricator.com/D1332
Summary: Clicks all the "Show All" links for you at the touch of a button.
Test Plan:
- Used "reveal entire file" on revealable files.
- Opened on already-visible files, got "entire file shown".
- Used other menu options.
- Used normal "show more" links.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley, btrahan
Maniphest Tasks: T497
Differential Revision: https://secure.phabricator.com/D1331
Summary:
We currently don't link to comments which aren't visible. Link to the
appropriate diff in a new window, indicating where the comment lives.
Test Plan: Clicked visible, not-so-visible comments.
Reviewers: btrahan, jungejason, davidreuss
Reviewed By: btrahan
CC: aran, btrahan, epriestley
Maniphest Tasks: T555, T449
Differential Revision: https://secure.phabricator.com/D1333
Summary: Make it a little easier to create a bunch of accounts if your company
has more than like 5 employees.
Test Plan: Ran "add_user.php" to create new users. Created new users from the
web console.
Reviewers: btrahan, jungejason, rguerin
Reviewed By: btrahan
CC: aran, btrahan, rguerin
Differential Revision: https://secure.phabricator.com/D1336
Summary:
If you try to establish several sessions quickly (e.g., by running several
copies of "arc" at once, as in "arc x | arc y"), the current logic has a high
chance of making them all pick the same conduit session to refresh (since it's
the oldest one when each process selects the current sessions). This means they
all issue updates against "conduit-3" (or whatever) and one ends up with a bogus
session.
Instead, do an update against the table with the session key we read, so only
one process wins the race. If we don't win the race, try again until we do or
have tried every session slot.
Test Plan:
- Wiped conduit sessions, ran arc commands to verify the fresh session case.
- Ran a bunch of arc piped to itself, e.g. "arc list | arc list | arc list |
...". It succeeds up to the session limit, and above that gets failures as
expected.
- Manually checked the session table to make sure things seemed reasonable
there.
- Generally ran a bunch of arc commands.
- Logged out and logged in on the web interface.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T687
Differential Revision: https://secure.phabricator.com/D1329
Summary:
Provide an easy way to jump to Diffusion from Differential if we have
the data we need to connect them.
Test Plan: Tested menu in linked and unlinked diffs. Used menu item.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley, btrahan
Maniphest Tasks: T309
Differential Revision: https://secure.phabricator.com/D1326
Summary:
When the user loads a page with an anchor on it like #thing, or clicks a link to
#thing, and #thing doesn't exist, keep trying to navigate to #thing for a few
seconds.
This allows anchors to work when the target is in content which is later ajaxed
in. In particular, this affects inline comments in Differential.
Test Plan: Opened inline comment links in a new tab, was in the right place when
I switched tabs.
Reviewers: nh, btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan, epriestley
Maniphest Tasks: T492
Differential Revision: https://secure.phabricator.com/D1327
Summary: These seem to work relatively reasonably and don't have any known
deal-breaking failures.
Test Plan: shrug~
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Differential Revision: https://secure.phabricator.com/D1324
corresponding ConduitAPI
Summary: reasonable title... also made this new functionality used by the
repository worker for parsing diffs
Test Plan:
- looked at the conduit console and queried for various types of hashes,
including hashes with no match. got correct results.
- identified a reasonable diff from a local git repo. set the revision status
to 2 (ACCEPTED) in the database. augmented the worker parser code to var_dump
and die after finding revision id. ran scripts/repository/reparse.php
--message rX and verified my var_dumps. removed var_dumps and die and ran
reparse.php again with same paramters. verified revision looked good in
diffusion and there were no errors.
- repeated the above reparse.php jonx for a mercurial repo. note svn isn't in
this hash game so that test was particularly exciting no-op'dness i did not
bother with
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, btrahan, epriestley
Differential Revision: https://secure.phabricator.com/D1315
Summary:
There are several open Differential tasks that are basically blocked on not
having reasonable places in the UI to put things. Replace the "View Standalone /
Raw" button with a "View Options" dropdown menu so we can shove things like
"Expand All", "Fold / Unfold File", and "View in Diffusion" in there.
This doesn't change any behavior, just puts the existing options in a menu.
Test Plan:
- Toggled menu open by clicking button.
- Clicked menu items.
- Toggled menu closed by clicking button.
- Toggled menu closed by clicking document.
- Toggled menu closed by opening another menu.
- Toggled menu closed by selecting an item.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T497, T309
Differential Revision: https://secure.phabricator.com/D1316
Summary:
- Use n/p to jump between comments.
- Use r to reply to the selected comment.
- Use e to edit the selected comment.
Test Plan: Verified n, p, r, e, j, k, J, K, "click edit" and "click reply"
behavior in as many weird cases as I could come up with.
Reviewers: btrahan, jungejason, nh, cpiro, jl
Reviewed By: btrahan
CC: aran, btrahan, epriestley
Maniphest Tasks: T583
Differential Revision: https://secure.phabricator.com/D1308
Summary: The recent change to the field causes us to render "http://junk.com/D"
in some cases, just null the field if there's no data.
Test Plan: Ran "arc diff --create".
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Differential Revision: https://secure.phabricator.com/D1321
Summary: Preview of Add Reviewers looks silly without actually showing them
Test Plan:
Go to any diff
Leap into action: Add Reviewers
Add some reviewers
Write some comment
Preview including Added reviewers should be displayed
Change action to Comment
Added reviewers should disappear
Repeat with Add CCs
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, vrana
Differential Revision: https://secure.phabricator.com/D1276
Summary: This is a fairly common question but I think it's the right product
behavior, document it so I can reference the docs next time it comes up.
Test Plan: Generated and read documentation.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, jungejason
Maniphest Tasks: T735
Differential Revision: https://secure.phabricator.com/D1310
Summary:
Create a visual hierarchy with the <span>s and <a>s in the aphront-side-nav
so people don't try to click on a span thinking it's a link. This is to
help specifically with the case of the "All Revisions" header on the
differential revision list page - I've had a few people ask about that
broken link.
Test Plan:
Loaded the differential revision list view and the maniphest task list
view to check that their left-hand navs look ok (did this in ff8 on ubuntu
11.10).
Reviewers: epriestley, jungejason, btrahan
Reviewed By: jungejason
CC: aran, jungejason, epriestley
Differential Revision: https://secure.phabricator.com/D1303
Summary:
If a page generates warnings or errors, you only get a little red dot in
DarkConsole which is hard to see. DarkConsole is also fairly big and there are
plenty of reasons not to leave it open all the time.
Instead, unconditionally show a big message to developers if there are errors or
warnings.
We could make this more sophisticated eventually, but the value is just that you
see it.
Test Plan: Browsed pages with and without warnings, got the right banner state.
Reviewers: nh, btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T734
Differential Revision: https://secure.phabricator.com/D1307
Summary:
See D1295. $unit_messages may be undefined.
I'll see if I can improve the visibility of warnings, the red dot in DarkConsole
is easy to miss right now. See T734.
Test Plan: Loaded a revision with no unit failures, didn't receive a warning.
Reviewers: nh, btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Differential Revision: https://secure.phabricator.com/D1306
Summary:
This provides an easier way to get a quick handle on page costs without
installing XHProf, which can be a bit complicated.
- We currently show an "All" line, but it means "All Services".
- Rename "All" to "All Services".
- Add "Entire Page".
Test Plan: Looked at the services tab, saw "All Services" and "Entire Page".
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Differential Revision: https://secure.phabricator.com/D1305
Summary:
Some installs use Git as the backbone of a CI framework or use a Git remote to
share patches. The tracker scripts currently recognize associated revisions as
"Committed" when they appear in any branch, even if that branch is
"alincoln-personal-development_test_hack" or whatever.
To address the broadest need here, allow Git repositories to be configured to
track only certain branches instead of all branches.
This doesn't allow you to import a branch into Diffusion but ignore it in
Differential. Supporting that is somewhat technically complicated because the
parser currently goes like this:
- Look at HEAD of all branches.
- For any commits we haven't seen before, follow them back to something we
have seen (or the root).
- "Discover" everything new.
Since this doesn't track <branch, commit> pairs, we currently don't have enough
information to tell when a commit appears in a branch for the first time, so we
don't have anywhere we can put a test for whether that branch is tracked and do
the Differential hook only if it is.
However, I think this cruder patch satisfies most of the need and is simple and
obvious in its implementation.
See also D1263.
Test Plan:
- Updated a Git repository with various filters: "", "master, remote", "derp",
" ,,, master ,,,,,"
- Edited SVN and Mercurial repositories to verify they didn't get caught in
the crossfire.
- Ran daemon in debug mode on libphutil with filter "derp", got exception
about no tracked branches. Ran with filter "master", got tracking. Ran with no
filter, got tracking.
- Looked at Diffusion with "derp" and "master", saw no branches and "master"
respectively.
- Added unit tests to cover filtering logic.
Reviewers: btrahan, jungejason, nh, fratrik
Reviewed By: fratrik
CC: aran, fratrik, epriestley
Maniphest Tasks: T270
Differential Revision: https://secure.phabricator.com/D1290
Summary:
- We have a few places where we do some kind of ad-hoc comma list tokenizing,
and I'm adding another one in D1290. Add a helper to the request object.
- Add some unit tests.
Test Plan:
- Ran unit tests.
- Used PHID manager, Maniphest custom view, and Repository project editor.
Reviewers: btrahan, fratrik, jungejason
Reviewed By: btrahan
CC: aran, btrahan, epriestley
Differential Revision: https://secure.phabricator.com/D1302
