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
