Summary:
This adds a link to [Closed] e-mail if it detects some changes.
It compares added and removed lines with 3 lines context.
The subtle form of informing is permissive to false negatives and positives.
I have an e-mail filter for [Closed] e-mails so I wouldn't personally notice this change - we should probably promote this feature a little bit.
Test Plan:
Reparse a diff with a change after last update.
Reparse a diff without a change after last update.
Reviewers: jungejason, epriestley
Reviewed By: jungejason
CC: aran, Koolvin, btrahan
Maniphest Tasks: T201
Differential Revision: https://secure.phabricator.com/D2540
Summary:
I thought about it a little bit and this makes the most sense for me:
# Original author usually writes at least something and commander only updates it.
# There's a creation date of revision (= first diff) by these comments. I don't want to change this date because I use this information. Author should correspond to this date.
# It solves all our repros.
Test Plan: Display commandeered revision.
Reviewers: nh, epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2577
Summary:
- We currently have some bugs in account creation due to nontransactional user/email editing.
- We save $user, then try to save $email. This may fail for various reasons, commonly because the email isn't unique.
- This leaves us with a $user with no email.
- Also, logging of edits is somewhat inconsistent across various edit mechanisms.
- Move all editing to a `PhabricatorUserEditor` class.
- Handle some broken-data cases more gracefully.
Test Plan:
- Created and edited a user with `accountadmin`.
- Created a user with `add_user.php`
- Created and edited a user with People editor.
- Created a user with OAuth.
- Edited user information via Settings.
- Tried to create an OAuth user with a duplicate email address, got a proper error.
- Tried to create a user via People with a duplicate email address, got a proper error.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: tberman, aran
Maniphest Tasks: T1184
Differential Revision: https://secure.phabricator.com/D2569
Summary:
- Add "role" information, so clients can identify disabled users.
- Formally deprecate `user.info`
Test Plan: Ran "user.query" and "user.whoami", inspected output. Verified "user.info" appears as deprecated in method list and console.
Reviewers: csilvers, btrahan
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2565
Summary:
Instead of assuming the test plan and summary are written by the author
of the differential revision, let's assume they are written by the author
of the latest differential diff.
Test Plan: viewed a drev that had been commandeered but not updated to check authors
Reviewers: epriestley, jungejason, vrana
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1235
Differential Revision: https://secure.phabricator.com/D2550
Summary:
- If you have an unverified primary email, we show a disabled "Primary" button right now in the "Status" column. Instead we should show an enabled "Verify" button, to allow you to re-send the verification email.
- Sort addresses in a predictable way.
Test Plan:
- Added, verified and removed a secondary email address.
- Resent verification email for primary address.
- Changed primary address.
Reviewers: btrahan, csilvers
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1184
Differential Revision: https://secure.phabricator.com/D2548
Summary: also makes the UI more general for this username + password business.
Test Plan:
- configure a phabricator repository from the svn server @asherwin provided which is configured for svn protocol with SASL
- observed phabricator failing without my patch
- upgraded my SVN client to support SASL (protip for mac users - http://www.wandisco.com/subversion/download#osx)
- applied patch to phabricator
- restarted daemons
- noted daemon success - diffusion populating nicely
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1260
Differential Revision: https://secure.phabricator.com/D2549
Summary:
Git and hg (supposedly..) differentiate between an author (who wrote the patch)
and a committer (who applied the patch).
This patch allows Phabricator to note when a patch is committed
by someone other than the Author.
Test Plan:
Created 2 accounts,
- U (Account with a PHID)
- U' (Account without a PHID)
and had them create and commit commits
testing if their username/real name would be displayed correctly in Diffusion,
- BrowserTable
- HistoryTable
- Code revision
Teztz,
A(uthor)/C(ommitter)
If it's A/A then Author committed
UL = User link (<a href="/p/username">username</a>)
UN = User name ("Firstname Lastname")
Tezt | Expected in table | Got
-------------------------------------------
A/A | UL/UL | UL/UL
A'/C | UN/UL | UN/UL
A/C' | UL/UN | UL/UN
A'/C' | UN/UN | UN/UN
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T688
Differential Revision: https://secure.phabricator.com/D2541
Summary:
If someone typos one name in a cc or reviewer list, it would be nice if we
display all of the valid names in the field when running arc diff (in addition
to the error message).
Test Plan:
used the conduit console to check that calling differential.parsecommitmessage
with a list of some valid and some invalid ccs returns a result with both an
error and a list of some ccs. Also ran arc diff with that list of ccs to check
for the correct user experience.
Reviewers: epriestley, jungejason, vrana
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2542
Summary:
This attaches commit diff to its associated revision as any other diff.
The consequence is that the revision page now shows the actual commit instead of the last diff. It may be disturbing but it is desired.
Another consequence is that lint and unit results are displayed as skipped after the revision was committed. I want to fix it somehow.
My next plan is to automatically diff against the last normal diff and include the link to this diff in commit e-mail if non-empty.
Test Plan:
reparse.php
Diff against last normal diff.
Reviewers: epriestley, jungejason
Reviewed By: jungejason
CC: aran, Koolvin, jungejason
Maniphest Tasks: T201
Differential Revision: https://secure.phabricator.com/D2530
Summary:
Allow callers to find all packages/paths owned by a given
user/project.
Test Plan:
Used conduit api page with --
user owner: PHID-USER-6ce1c976b86e5f3c34f6 (tried both loadPackagesFromProjects
true/false)
proj owner: PHID-PROJ-r5wnmmaawqsn4tvjmqm4
repo/path: E, /tfb/trunk/www/flib/privacy. Checked both path.getowners
and owners.query
(all of these are defined internally at fb)
Reviewers: epriestley, btrahan, nh
Reviewed By: btrahan
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2482
Summary:
Mercurial renamed "--only-branch" to "--branch" about two years ago. "-b" exists in both versions.
http://www.selenic.com/pipermail/mercurial-devel/2010-April/020469.html
We have a few other cases where we use features that exist only in recent Mercurial (notably, 'ancestors' in log) but we can work around this one easily.
Test Plan: Looked at a Mercurial repo in Diffusion, verified that "log -b" commands issued and that the output was correct.
Reviewers: btrahan, Makinde, ipalaus
Reviewed By: ipalaus
CC: aran
Maniphest Tasks: T1264
Differential Revision: https://secure.phabricator.com/D2533
Summary: Add support for "tests", "testplan" and "tested" as alias of "Test Plan".
Test Plan: Created a diff with test plan specified in "tests".
Reviewers: csilvers, btrahan
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2531
Summary:
It's currently possible to configure Phabricator to send mail to some address it recognizes as relating to an object.
When we receive mail from Phabricator, drop it unconditionally.
Test Plan: Wrote two emails, one with the header and one without. Piped them to `mail_handler.php`, one was dropped immediately.
Reviewers: btrahan, nh, mikaaay, jungejason
Reviewed By: jungejason
CC: aran
Differential Revision: https://secure.phabricator.com/D2529
Summary:
Since user emails aren't in the user table, we had to do extra data fetching
for handles, and the emails are only used in MetaMTA, so we move the email
code into MetaMTA and remove it from handles.
Test Plan: send test emails
Reviewers: jungejason, vrana, epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2494
Summary:
We introduced a "user.query" call recently which is only about two weeks old. Bump versions so users get a forced upgrade.
Also, we raise a fairly confusing message when the user calls a nonexistent method. This is not the intent; `class_exists()` throws. Tailor this exception more carefully.
Test Plan:
- Ran `echo {} | arc call-conduit derp.derp`, got a better exception.
- Bumped version, ran `arc list`, got told to upgrade.
Reviewers: indiefan, nh, vrana, btrahan, jungejason, Makinde
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2527
Summary: as title
Test Plan: no
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2526
Summary:
The current state is very confusing:
{F11734, size=full}
Test Plan: Display calendar for user with two different events in the same week.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2522
Summary:
Allow installs to require users to verify email addresses before they can use Phabricator. If a user logs in without a verified email address, they're given instructions to verify their address.
This isn't too useful on its own since we don't actually have arbitrary email registration, but the next step is to allow installs to restrict email to only some domains (e.g., @mycompany.com).
Test Plan:
- Verification
- Set verification requirement to `true`.
- Tried to use Phabricator with an unverified account, was told to verify.
- Tried to use Conduit, was given a verification error.
- Verified account, used Phabricator.
- Unverified account, reset password, verified implicit verification, used Phabricator.
- People Admin Interface
- Viewed as admin. Clicked "Administrate User".
- Viewed as non-admin
- Sanity Checks
- Used Conduit normally from web/CLI with a verified account.
- Logged in/out.
- Sent password reset email.
- Created a new user.
- Logged in with an unverified user but with the configuration set to off.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran, csilvers
Maniphest Tasks: T1184
Differential Revision: https://secure.phabricator.com/D2520
Summary: Ideally there should be a "send epriestley this profile" button but this is a reasonable step forward. Add a "download .xhprof profile" button to profiles, since walking through these things remotely is pretty awkward. Also expand "Excl" and "Incl" acronyms.
Test Plan: Clicked download button.
Reviewers: csilvers, btrahan, vrana
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2523
Summary:
This is a rough cut, but gets some of the basics at least. Here's what it looks like:
{F11690}
Some things that would be nice for future diffs:
- Different colors for different event types (tasks? MEETINGS?!)
- When events span across multiple days, keep them in the same row.
- Switch which month you're looking at.
- Show specific users instead of all.
- etc etc etc
Test Plan: See screenshot.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2514
Summary:
we were parsing the git log output slightly incorrectly and over-exploding on spaces. we also needed to escape the path %20 stuff`.
Not sure if there's something fancy to do given folks should reparse their repos if they are impacted by this issue.
Test Plan:
made a directory with spaces and some dummy revisions. observed diffs wouldn't load and links broken.
with patch, ran scripts/reparse.php for pertinent revisions and diffs loaded and links weren't broken.
Reviewers: floatinglomas, epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1252
Differential Revision: https://secure.phabricator.com/D2510
Summary: See T1254, until this gets paginated properly we can at least show more results. 25 is pretty anemic.
Test Plan: Tweaked limit, verified result count was affected.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1254
Differential Revision: https://secure.phabricator.com/D2513
Summary: If you have an empty value saved in the "default branch" field, we default to empty string (or null, or whatever) instead of the correct default.
Test Plan:
- Looked at a Git repo with an empty default branch, got a default to "master" instead of an error.
- Looked at a Mercurial repo with an empty default branch, got a default to "default" instead of an error.
Reviewers: btrahan, csilvers
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1237
Differential Revision: https://secure.phabricator.com/D2512
Summary:
- We currently write every PHID we generate to a table. This was motivated by two concerns:
- **Understanding Data**: At Facebook, the data was sometimes kind of a mess. You could look at a random user in the ID tool and see 9000 assocs with random binary data attached to them, pointing at a zillion other objects with no idea how any of it got there. I originally created this table to have a canonical source of truth about PHID basics, at least. In practice, our data model has been really tidy and consistent, and we don't use any of the auxiliary data in this table (or even write it). The handle abstraction is powerful and covers essentially all of the useful data in the app, and we have human-readable types in the keys. So I don't think we have a real need here, and this table isn't serving it if we do.
- **Uniqueness**: With a unique key, we can be sure they're unique, even if we get astronomically unlucky and get a collision. But every table we use them in has a unique key anyway. So we actually get pretty much nothing here, except maybe some vague guarantee that we won't reallocate a key later if the original object is deleted. But it's hard to imagine any install will ever have a collision, given that the key space is 36^20 per object type.
- We also currently use PHIDs and Users in tests sometimes. This is silly and can break (see D2461).
- Drop the PHID database.
- Introduce a "Harbormaster" database (the eventual CI tool, after Drydock).
- Add a scratch table to the Harbormaster database for doing unit test meta-tests.
- Now, PHID generation does no writes, and unit tests are isolated from the application.
- @csilvers: This should slightly improve the performance of the large query-bound tail in D2457.
Test Plan: Ran unit tests. Ran storage upgrade.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: csilvers, aran, nh, edward
Differential Revision: https://secure.phabricator.com/D2466
Summary: These patterns are hard-coded, allow them to match case-insenstiviely.
Test Plan: Typed "d3" and "D3", got the right object in the attach dialog.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1253
Differential Revision: https://secure.phabricator.com/D2511
Summary: ...they were broken...
Test Plan: clicked links for both SVN and Git repos and got working results
Reviewers: vrana, floatinglomas, 20after4
Reviewed By: floatinglomas
CC: aran, epriestley
Maniphest Tasks: T1250
Differential Revision: https://secure.phabricator.com/D2505
Summary:
only show the blank, "create new" wiki page for the project if the project actually exists; only allow edit if the project actually exists.
Small wrinkle here is not checking if the project actually exists if the page already exists.
Test Plan:
- viewed a project wiki page
- viewed a prokect wiki page for a fake project and got a 404
- edited a project wiki page
- edited a project wiki page for a fake project and got a 404
Reviewers: epriestley, jacktrades
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1248
Differential Revision: https://secure.phabricator.com/D2506
Summary: we weren't actually removing any edges. now we do.
Test Plan: had a commit associated with task x; removed association. had a commit associated with task x; removed association while adding a different one.
Reviewers: floatinglomas, epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1256
Differential Revision: https://secure.phabricator.com/D2509
Summary: we need a user (the viewer in this case) for the status to render correctly with respect to timezone
Test Plan: my profile no longer fatals with an away status
Reviewers: davidreuss, vrana
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2504
Summary: see T1241, T1242, T1244 for some examples of crud getting saved
Test Plan: threw some crud in my conduit console and got reasonable errors back
Reviewers: mikaaay, epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1241, T1242, T1244
Differential Revision: https://secure.phabricator.com/D2487
Summary: D2492#6
Test Plan:
`user.query` of user which is away.
`user.query` of user which is not away.
`user.whoami` - no information there.
Reviewers: epriestley
Reviewed By: epriestley
CC: btrahan, aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2502
Summary: there were 6 values, but we only need 4. delete the two empty ones as the others are all stylistically valid.
Test Plan: ...looks good!
Reviewers: ric03uec, vrana, epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T1247
Differential Revision: https://secure.phabricator.com/D2498
Summary: There's no method for enabling users somewhat intentionally.
Test Plan: Disable myself (oops, this is probably my last diff ever).
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2497
Test Plan: Display revision list both with last reviewer and without.
Reviewers: epriestley, btrahan
Reviewed By: btrahan
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2495
Summary:
This is not so general as `getRequiredHandlePHIDs()`.
It allows bulk loading of user statuses only in revision list.
It also loads data in `render()`. I'm not sure if it's OK.
Maybe we can use the colorful point here.
Or maybe some unicode symbol?
Test Plan: {F11451, size=full}
Reviewers: btrahan, epriestley
Reviewed By: btrahan
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2484
Summary:
I want to use this to warn user if he specifies reviewers that are away.
We can also implement a general query method but I think that this usage is the
most useful not only for me but also in general case.
Test Plan:
Call the method for user which is away and which is not away.
Add user status through Conduit.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2492
Summary:
I've tried colors, italics, strike-through and this is a compromise between me and @leebyron.
NOTE: I don't want to disturb @epriestley from playing Diablo 3.
Test Plan: {F11439, size=full}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley, leebyron
Differential Revision: https://secure.phabricator.com/D2481
Summary:
Occasionally a bot will get subscribed to a differential revision; when
this happens we shouldn't send them an email, because otherwise the
author of the diff will get a bounce email if the bot has an invalid
emal address.
Test Plan:
re-sent a message that had a bot on the cc list, saw that it was no longer
included when it got sent.
Reviewers: epriestley, jungejason, vrana
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2479
Summary: the existing, more custom text doesn't make sense when viewing someone else's revisions AND in someplaces its the less interesting "no data found".
Test Plan: clicked around the various http://phabricator.dev/differential/filter/* pages with various users.
Reviewers: epriestley, ry
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1222
Differential Revision: https://secure.phabricator.com/D2475
Summary:
D2457 and D2459 made some changes here. This little guy just needed to be touched so it could be arc lint'd to have the proper updated include paths
Sorry to spam reviewers; Evan is in Diablo 3 somewhere. :D
Test Plan: no more fatal on test everything implemented (arc unit src/infrastructure/__tests__/)
Reviewers: epriestley, jungejason, nh, csilvers
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2474
Summary:
For package creation and deletion, send email to all the owners For
package modification, detect important fields such as owners and paths, and then
send out emails to all owners (including deleted owners and current owners)
Also start using transaction for package creation/deletion/modification.
Test Plan:
- tested mail creation and deletion
- tested modification to auditing enabled, primary owners, owners, paths
Reviewers: epriestley, nh, vrana
Reviewed By: epriestley
CC: prithvi, aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2470
Summary: We had a real issue where revision was marked as accepted but the comment wasn't saved.
Test Plan: Reclaim abandoned revision.
Reviewers: epriestley
Reviewed By: epriestley
CC: jungejason, aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2463
Summary: Prelminary, I want to test this a bit more when I'm better rested. Provide a "bulk" import mode for Mercurial so we can do initial discovery more quickly.
Test Plan: Imported the Mercurial repository in 2m45s, blocked on MySQL I/O rather than Mercurial I/O.
Reviewers: csilvers, btrahan
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2457
Summary: For git workflows where developers push personal feature branches to the origin, this is a quick workaround until T1210 is implemented properly. I'll also mention this in D2446.
Test Plan:
- Committed a revision in an "autoclose" repository; it was autoclosed. Committed a revision in a no-autoclose repository, it was not autoclosed.
- Edited repositories, saving the "autoclose" setting as enabled/disabled.
Reviewers: aurelijus, btrahan
Reviewed By: aurelijus
CC: aran
Maniphest Tasks: T1210
Differential Revision: https://secure.phabricator.com/D2448
Summary:
- Support offset/limit as added by D2442, for Mercurial.
- We just list everything and slice, no clear way to do better and this isn't a major perf issue.
- No clear way to easily get by-update sorting, we can implement this by looking up revs if someone asks.
- Bury some of the Git implementation details inside the Git query.
Test Plan:
- Looked at a Git repo, clicked "View All Branches".
- Looked at a Hg repo, clicked "View All Branches".
- Limited page size to 1, made sure it limited properly.
Reviewers: aurelijus, btrahan, vrana
Reviewed By: aurelijus
CC: aran
Differential Revision: https://secure.phabricator.com/D2453
Summary:
Sorting by last commit date
Branch view limit to 25 branches
All branches table page with pagination on Git
Test Plan:
* Check repository view for expected behavior on branch table view
* Check all branches page & test pagination
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1200
Differential Revision: https://secure.phabricator.com/D2442
Summary: Revisions don't necessarily have a backing repository, as in "--raw".
Test Plan: Looked at a "--raw" revision in accepted and closed states.
Reviewers: nh, btrahan
Reviewed By: nh
CC: aran
Differential Revision: https://secure.phabricator.com/D2444
Summary:
The highlighting is distracting according to Nick Shrock and others.
Real designer, Lee Byron, helped me with this.
It also gives us unagressive target for jumping to the source line in future.
Another feature I will probably implement is highlighting also the source of copies/moves.
I will use the right side of the left column for it.
Test Plan:
Hover copied notifier.
Hover coverage notifier.
I've also checked that this doesn't break our super-flaky old/new code JavaScript detector.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin, leebyron, schrockn
Differential Revision: https://secure.phabricator.com/D2443
Summary:
- When you have an un-cloned repository, we currently throw random-looking Git/Hg exception. Instead, throw a useful error.
- When you have a cloned but undiscovered repository, we show no commits. This is crazy confusing. Instead, show commits as "importing...".
- Fix some warnings and errors for empty path table cases, etc.
Test Plan:
- Wiped database.
- Added Mercurial repo without running daemons. Viewed in Diffusion, got a good exception.
- Pulled Mercurial repo without discovering it. Got "Importing...".
- Discovered Mercurial repo without parsing it. Got "Importing..." plus date information.
- Parsed Mercurial repo, got everything working properly.
- Added Git repo without running daemons, did all the stuff above, same results.
- This doesn't improve SVN much but that's a trickier case since we don't actually make SVN calls and rely only on the parse state.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T776
Differential Revision: https://secure.phabricator.com/D2439
Summary:
- Merge CommitTask daemon into PullLocal daemon. This is another artifact of past instability (and order-dependent parsers). We still publish to the timeline, although this was the last consumer. Long term we'll probably delete timeline and move to webhooks, since everyone who has asked about this stuff has been eager to trade away the durability and ordering of the timeline for the ease of use of webhooks. There's also no reason to timeline this anymore since parsing is no longer order-dependent.
- Add `phd start` to start all the daemons you need. Add `phd restart` to restart all the daemons you need. So cool~
- Simplify and improve phd and Diffusion daemon documentation.
Test Plan:
- Ran `phd start`.
- Ran `phd restart`.
- Generated/read documentation.
- Imported some stuff, got clean parses.
Reviewers: btrahan, csilvers
Reviewed By: csilvers
CC: aran, jungejason, nh
Differential Revision: https://secure.phabricator.com/D2433
Summary: When the server version is ahead of the client version, send a more exciting error!!!
Test Plan: omg~~~
Reviewers: nh, Makinde, btrahan, jungejason
Reviewed By: nh
CC: aran
Differential Revision: https://secure.phabricator.com/D2436
Summary: Add history column & history link per branch on branch table view, also later add some more features like last commit date, etc.
Test Plan: Checked if History column is in browser table view & history links are properly linked.
Reviewers: epriestley
Reviewed By: epriestley
CC: davidreuss, aran, Koolvin
Maniphest Tasks: T1201, T1202, T1200
Differential Revision: https://secure.phabricator.com/D2432
Summary:
See D2418. This merges the commit discovery daemon into the same single daemon, and applies all the same rules to it.
There are relatively few implementation changes, but a few things did change:
- I simplified/improved Mercurial importing, by finding full branch tip hashes with "--debug branches" and using "parents --template {node}" so we don't need to do separate "--debug id" calls.
- Added a new "--not" flag to exclude repositories, since I switched to real arg parsing anyway.
- I removed a web UI notification that you need to restart the daemons, this is no longer true.
- I added a web UI notification that no pull daemon is running on the machine.
NOTE: @makinde, this doesn't change anything from your perspective, but it something breaks this is the likely cause.
This implicitly resolves T792, because discovery no longer runs before pulling.
Test Plan:
- Swapped databases to a fresh install.
- Ran "pulllocal" in debug mode. Verified it correctly does nothing (fixed a minor issue with min() on empty array).
- Added an SVN repository. Verified it cloned and discovered correctly.
- Added a Mercurial repository. Verified it cloned and discovered correctly.
- Added a Git repository. Verified it cloned and discovered correctly.
- Ran with arguments to verify behaviors: "--not MTEST --not STEST", "P --no-discovery", "P".
Reviewers: btrahan, csilvers, Makinde
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T792
Differential Revision: https://secure.phabricator.com/D2430
Summary:
also fix some bugs where we weren't properly capturing the expiry value or scope of access tokens.
This code isn't the cleanest as some providers don't confirm what scope you've been granted. In that case, assume the access token is of the minimum scope Phabricator requires. This seems more useful to me as only Phabricator at the moment really easily / consistently lets the user increase / decrease the granted scope so its basically always the correct assumption at the time we make it.
Test Plan: linked and unlinked Phabricator, Github, Disqus and Facebook accounts from Phabricator instaneces
Reviewers: epriestley
Reviewed By: epriestley
CC: zeeg, aran, Koolvin
Maniphest Tasks: T1110
Differential Revision: https://secure.phabricator.com/D2431
Summary:
For most actions (like "accept"), we write a row only if you aren't acting on behalf of anything else. This avoids cases like every accept causing two relationships:
Some Project | Accept
Some User | Accept
For "Resign", we must always write the row. Break the logic out and handle it separately.
Test Plan: Poked it locally, but let me know if this fixes things?
Reviewers: 20after4, btrahan
Reviewed By: 20after4
CC: aran
Differential Revision: https://secure.phabricator.com/D2423
Summary:
Allow the pull daemon to take a list of repositories. By default, pull all repositories.
Make some effort to respect pull frequencies, although we'll necessarily suffer a bit if running with only one process.
NOTE: We still launch one discovery daemon per working copy, so this only cuts the daemon count in half.
Test Plan:
- Ran `phd debug pulllocal`, verified behavior.
- Ran `pull.php P MTEST SVNTEST --trace`, verified it pulled the repos and ran the right commands.
- Ran `phd repository-launch-master`, verified the right daemons launched, checked daemon console.
- Ran `phd repository-launch-readonly`, verified the right daemon launched, checked daemon console.
Reviewers: btrahan, csilvers, davidreuss
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2418
Summary: specify user table to make things not ambiguous
Test Plan: conduit console still works, including ID query...!
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1075
Differential Revision: https://secure.phabricator.com/D2419
Summary:
When you create a new task, the UI gives you the option to create another similar task. We copy some fields, but not others.
Currently, the field list is hard-coded and excludes auxiliary fields. Instead, allow auxiliary fields to elect to be copied.
Test Plan:
- Created a new task, verified appropriate field defuaults.
- Created a new "similar" task, verified 'copy' fields copied in.
- Edited an existing task, verified appropriate values.
- Edited-with-errors, verified new values didn't get reverted in the form.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1193
Differential Revision: https://secure.phabricator.com/D2410
Summary: The PHID list is a list, not a map -- I must have broken this in refactoring or something, since everything else works fine. See D2013.
Test Plan: Viewed a resignable revision, saw "Resign" (new after this commit), resignd.
Reviewers: 20after4, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D2417
Summary:
- Move email to a separate table.
- Migrate existing email to new storage.
- Allow users to add and remove email addresses.
- Allow users to verify email addresses.
- Allow users to change their primary email address.
- Convert all the registration/reset/login code to understand these changes.
- There are a few security considerations here but I think I've addressed them. Principally, it is important to never let a user acquire a verified email address they don't actually own. We ensure this by tightening the scoping of token generation rules to be (user, email) specific.
- This should have essentially zero impact on Facebook, but may require some minor changes in the registration code -- I don't exactly remember how it is set up.
Not included here (next steps):
- Allow configuration to restrict email to certain domains.
- Allow configuration to require validated email.
Test Plan:
This is a fairly extensive, difficult-to-test change.
- From "Email Addresses" interface:
- Added new email (verified email verifications sent).
- Changed primary email (verified old/new notificactions sent).
- Resent verification emails (verified they sent).
- Removed email.
- Tried to add already-owned email.
- Created new users with "accountadmin". Edited existing users with "accountadmin".
- Created new users with "add_user.php".
- Created new users with web interface.
- Clicked welcome email link, verified it verified email.
- Reset password.
- Linked/unlinked oauth accounts.
- Logged in with oauth account.
- Logged in with email.
- Registered with Oauth account.
- Tried to register with OAuth account with duplicate email.
- Verified errors for email verification with bad tokens, etc.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1184
Differential Revision: https://secure.phabricator.com/D2393
Summary: The various interfaces here are in conflict about what a role is and isn't. Make them all consistent.
Test Plan: Edited some users into various roles, verified they reported correctly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1190
Differential Revision: https://secure.phabricator.com/D2415
Summary:
- In practice, 'edit' has two modes, 'create' and 'edit'. These seem like they should map to "create a revision" and "update a revision", but they are completely different.
- We use the "create" mode:
- When creating a message from the working copy.
- When creating a message from a file.
- When creating a message from a commit.
- When creating a message from a user template.
- When creating a message from an "--edit"!
- We use the "edit" mode:
- ONLY when updating a revision with `arc diff --verbatim`.
- The only difference is in which fields may be overwritten. Under "create", all fields may be overwritten. Under "edit", only safe fields may be overwritten.
- The "Differential Revision" field currently does not render in either edit mode. This is wrong. Even though it can not be updated in the "edit" mode, it should still render in both modes. This is the only material change this revision makes.
- Without this change, when we "create" a new message from a working copy and the working copy has a "Differential Revision" field, we incorrectly discard it.
- The only field which does not render on edit modes now is "Reviewed by" (not "Reviewers"), which is correct, since we do not read the value.
Test Plan: Ran "arc diff" to create/update revisions. Ran "arc diff --verbatim" to create/update revisions with implicit edits (with D2411). Ran "arc diff --edit" to update revisions with explicit edits.
Reviewers: jungejason, btrahan
Reviewed by: jungejason
CC: vrana, aran
Differential Revision: https://secure.phabricator.com/D2412
Summary:
- When a user uploads an oversized file, throw an exception.
- When an uncaught exception occurs during a Conduit request, return a Conduit response.
- When an uncaught exception occurs during a non-workflow Ajax request, return an Ajax response.
Test Plan:
- Uploaded overlarge files.
- Hit an exception page with ?__ajax__=1 and ?__conduit__=1
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T875, T788
Differential Revision: https://secure.phabricator.com/D2385
Summary:
The highlighting is distracting according to Nick Shrock and others.
Real designer, Lee Byron, helped me with this.
It also gives us unagressive target for jumping to the source line in future.
Another feature I will probably implement is highlighting also the source of copies/moves.
I will use the right side of the left column for it.
Test Plan:
Hover copied notifier.
Hover coverage notifier.
I've also checked that this doesn't break our super-flaky old/new code JavaScript detector.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin, leebyron, schrockn
Differential Revision: https://secure.phabricator.com/D2403
Summary: We currently try to do "app login" for all OAuth providers, but not all of them support it in a meaningful way. Particularly, it always fails for Google.
Test Plan: Ran google diagnostics on a working config, no longer got a diagnostic failure.
Reviewers: btrahan, vrana, csilvers
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2377
Summary: Primarily for @csilvers who has 92 million repositories or something. This is a touch hacky, but movitated by pragmatism.
Test Plan:
- Ran "repository.create" to create repositories, "repository.query" to list them.
- Tested most or maybe all of the error conditions, probably.
Reviewers: btrahan, vrana, csilvers
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2396
Summary:
It saves some time on non-highlighting generated and other not interesting code.
The code is quite complex (300 lines methods) so I'm not sure if everything is moved correctly.
P.S. I hope that moved code detector will work...
Test Plan:
Display generated file with all whitespace, verify that it is not highlighted.
Display normal file.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1134
Differential Revision: https://secure.phabricator.com/D2358
Summary:
I wanted to point someone on a file uploaded to Phabricator and the normal link is just too long.
I guess that this also improves security. Because pointing someone to the file directly reveals the secret key used in /data/ and it can be served without auth?
We already use `{F123}` so there will be no conflicts in future because we wouldn't want to reuse it for something else.
I promote the link on /file/ - it adds one redirect but I think it's worth it. I also considered making the link from the File ID column but there are already too many links (with some duplicity).
Test Plan:
/file/
/F123 (redirect)
/F9999999999 (404)
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2380
Summary:
I will use it for highlighting users which are not currently available.
Maybe I will also use it in the nagging tool.
I don't plan creating a UI for it as API is currently enough for us.
Maybe I will visualize it at /calendar/ later.
I plan creating `user.deletestatus` method when this one will be done.
Test Plan:
`storage upgrade`
Call Conduit `user.addstatus`.
Verify DB.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2382
Summary: I have a patch which makes uploads all fancy and adds progress bars, but document the landscape first since it's quite complicated.
Test Plan: Generated, read docs. Configured `storage.upload-size-limit` to various values.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T875
Differential Revision: https://secure.phabricator.com/D2381
Summary: We currently make a ludicrously gigantic permission request to do Google auth (read/write access to the entire address book), since I couldn't figure out how to do a more narrowly tailored request when I implemented it. @csilvers pointed me at some much more sensible APIs; we can now just ask for user ID, name, and email address.
Test Plan: Created a new account via Google Oauth. Linked/unlinked an existing account. Verified diagnostics page still works correctly. Logged in with a pre-existing Google account created with the old API (to verify user IDs are the same through both methods).
Reviewers: btrahan, vrana, csilvers, Makinde
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2378
Summary: I will need it for nagging tool.
Test Plan:
None yet.
Please suggest me how to create a testing database (I need to insert some data in the table). I guess that it is now possible?
There is also probably some bug in `arc unit` - `setEnvConfig()` is not called before `getEnvConfig()` resulting in fatal error.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2376
Summary:
We will need it for two purposes:
- Status tool.
- Nagging tool - @aran suggested using "3 business days" and I don't want it to fall on New Year's Eve or such.
I don't plan working on any interface for editing this as this kind of data should be always imported.
Test Plan:
`bin/storage upgrade`
`scripts/calendar/import_us_holidays.php`
/calendar/
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2375
Summary: This allows writing inline comments and reduces different behavior between normal and very large diffs.
Test Plan:
Verify that normal diff works.
Verify that very large diff works.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2361
Summary:
This adds support to differential fields to display warnings before a revision
gets accepted. Since lint and unit are differential fields, the code for their
warnings was moved into their respective field specification classes, so there
is only one code path for warnings (lint, unit, or custom).
Test Plan:
Select 'Accept' on a revision with lint/unit warnings and see messages appear
like they used to. Change it back to 'Comment' and they go away. Repeat with
a revision without lint/unit warnings and see no warnings appear. Checked
darkconsole to see no errors due to this.
Reviewers: jungejason, epriestley, vrana
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2363
Summary: NOTE: `renderViewOptionsDropdown()` adds unnecessary parameters to URL but the link just redirects anyway.
Test Plan:
Show Raw File (Left and Right) in SVN and Git.
Verify also Added and Deleted files.
Reviewers: epriestley, aran
Reviewed By: epriestley
CC: Koolvin
Differential Revision: https://secure.phabricator.com/D2370
Summary:
- This is only slightly useful for updating Differential, since DiffQuery (vs RawDiffQuery) already gets you most of what you need. The only thing is that DiffQuery returns the diff for one path only right now(and the SVN version is very "special"). Should be easy to fix in the Git/HG cases at least, though (or maybe just use RawDiffQuery to avoid the SVN mess).
- Added a "download raw diff" link.
Test Plan: Viewed Diffusion and raw commits for SVN, Mercurial and Git repositories.
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2350
Summary:
- Unit tests can request storage fixtures.
- We build one fixture across all tests in the process, which can quickstart (takes roughly 1s to build, 200ms to destroy for me). This is a one-time cost for running an arbitrary number of fixture-based tests.
- We isolate all the connections inside transactions for each test, so individual tests don't affect one another.
Test Plan: Ran unit tests, which cover the important properties of fixtures.
Reviewers: btrahan, vrana, jungejason, edward
Reviewed By: btrahan
CC: aran, davidreuss
Maniphest Tasks: T140
Differential Revision: https://secure.phabricator.com/D2345
Summary:
When choosing a verb to show with a closed differential revision, choose the
verb based on the upstream vcs, not the vcs used to create the diff, since these
are not the same thing. I also updated the documentation for the next step for
an accepted diff for the case where the local vcs and backing vcs aren't the
same (since arc land doesn't work for those).
Test Plan:
Loaded a committed diff and an accepted diff from fbcode and www to check that
they show the correct thing.
Reviewers: jungejason, epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1118
Differential Revision: https://secure.phabricator.com/D2360
Summary: Only inlines were indexed (contrary to what comment claims).
Test Plan: Index one revision, check database.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2359
Summary:
I think this improves things, let me know if you have feedback.
Also addresses T840.
Test Plan: See screenshots...
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran, zeeg
Maniphest Tasks: T840
Differential Revision: https://secure.phabricator.com/D2357
Summary: basically by validating we have good user data when we set the user data.
Test Plan: simulated a failure from a phabricator on phabricator oauth scenario. viewed ui that correctly told me there was an error with the provider and to try again.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1077
Differential Revision: https://secure.phabricator.com/D2337
Summary: 'cuz it looks dumb to use a URI slug
Test Plan: viewed a post liked the title
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2338
Summary:
Before: {F10754}
After: {F10753}
Test Plan:
View revision with lint warnings and unit errors.
Click on Details.
Click on Details.
Click on Details.
Click on Details.
Reviewers: asukhachev, epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2351
Summary:
- Currently, connections are responsible for connection caching. However, I want unit tests to be able to say "throw away the entire connection cache" with storage fixtures, and this is difficult/impossible when connections are responsible for the cache.
- The only behavioral change is that previously we would use the same connection for read-mode and write-mode queries. We'll now establish two connections. No installs actually differentiate between the modes so it isn't particularly relevant what we do here. In the long term, we should probably check the "w" cache before building a new "r" connection, so transactional code which involves reads and writes works (we don't have any such code right now).
Test Plan: Loaded pages, verified only one connection was established per database. Ran unit tests.
Reviewers: btrahan, vrana, jungejason, edward
Reviewed By: vrana
CC: aran
Maniphest Tasks: T140
Differential Revision: https://secure.phabricator.com/D2342
Summary: Allow the default namespace to be set in configuration, so you can juggle multiple copies of sandbox test data or whatever.
Test Plan: Changed default namespace, verified web UI and "storage" script respect it.
Reviewers: btrahan, vrana, jungejason
Reviewed By: vrana
CC: aran
Maniphest Tasks: T345
Differential Revision: https://secure.phabricator.com/D2341
Summary:
Also reduce the memory usage a little bit (before increasing it again).
I use the same CSS class as for the copied code.
Test Plan: Parsed 100 diffs and checked about 10 of them - looks good.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2339
Summary:
This addresses three issues with the current patch management system:
# Two people developing at the same time often pick the same SQL patch number, and then have to go rename it. The system catches this, but it's silly.
# Second/third-party developers can't use the same system to manage auxiliary storage they may want to add.
# There's no way to build mock databases for unit tests that need to do reads.
To resolve these things, you can now name your patches whatever you want and conflicts are just merge conflicts, which are less of a pain to fix than filename conflicts.
Dependencies are now a DAG, with implicit dependencies created on the prior patch if no dependencies are specified. Developers can add new concrete subclasses of `PhabricatorSQLPatchList` to add storage management, and define the dependency branchpoint of their patches so they apply in the correct order (although, generally, they should not depend on the mainline patches, presumably).
The commands `storage upgrade --namespace test1234` and `storage destroy --namespace test1234` will allow unit tests to build and destroy MySQL storage.
A "quickstart" mode allows an upgrade from scratch in ~1200ms. Destruction takes about 200ms. These seem like fairily reasonable costs to actually use in tests. Building from scratch patch-by-patch takes about 6000ms.
Test Plan:
- Created new databases from scratch with and without quickstart in a separate test namespace. Pointed the webapp at the test namespaces, browsed around, everything looked good.
- Compared quickstart and no-quickstart dump states, they're identical except for mysqldump timestamps and a few similar things.
- Upgraded a legacy database to the new storage format.
- Destroyed / dumped storage.
Reviewers: edward, vrana, btrahan, jungejason
Reviewed By: btrahan
CC: aran, nh
Maniphest Tasks: T140, T345
Differential Revision: https://secure.phabricator.com/D2323
Summary:
- Show README on the repository screen.
- Move README to the bottom of the page for both repository and browse screens.
- Support "README.rainbow".
Test Plan: Looked at repository, browse screens. Made a "README.rainbow".
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1104
Differential Revision: https://secure.phabricator.com/D2336
Summary: The color used for this feature is pretty important and I am bad with colors.
Test Plan:
View diff created by D2320 with some copied lines and one line changed:
{F10604, size=full}
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2321
Summary:
Required for D2321.
Deprecates D2320.
Uses algorithm described at D2320#16.
Complexity of this algorithm would be `O(N)` (`N` stands for number of lines) in most cases.
The worst case is `O(A*F)` (`A` stands for number of added lines, `F` for number of colliding lines) but it should be pretty rare. Real-world example is 100 modified files with moved license block (15 lines) in each. This will require 1500*100 comparisons because the algorithm will be trying to find the longest block in each file.
Test Plan:
`arc diff --only` on commit with copied code.
More tests on standalone algorithm.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2333
Summary:
We added the ability to delete files a while ago, but this interface isn't happy about it.
I still render the macro so you can see/delete it, e.g.
Test Plan: Viewed a deleted macro page, got a page instead of an error. Also verified that the actual remarkup part doesn't have issues.
Reviewers: btrahan, vrana, jungejason
Reviewed By: jungejason
CC: aran
Differential Revision: https://secure.phabricator.com/D2328
Summary:
This is mostly intended to simplify D2323.
We currently allow users to edit and customize the links on the homepage, but as far as I know no one actually does this (no one complained when we redid the homepage earlier this year) and it creates a lot of mess in the database patches and quickstart dump. After D2331, this is the only data we load in the patch files. The patch files are also a mess with respect to this data and have various different versions of it.
Also the current UI is just kind of bad, it stretches stuff across too many screens and is generally ungood. Nuking this lets us nuke a lot of code in general.
(In the long term, I think we'll move toward an "application" model anyway, and this stuff will go away sooner or later.)
I'll add a drop-database patch some time later, just in case anyone does actually use this, so they can get their data out of MySQL.
Test Plan: Looked at home page, clicked "More Stuff", got a single list of other apps/things.
Reviewers: btrahan, vrana, edward, jungejason
Reviewed By: jungejason
CC: aran
Maniphest Tasks: T345
Differential Revision: https://secure.phabricator.com/D2332
Summary:
This is mostly in an effort to simplify D2323. Currently, we load one image into the database by default. This is a weird special case that makes things more complicated than necessary.
Instead, use a disk-based default avatar.
Test Plan: Verified that a user without an image appears with the default avatar as a handle, in profile settings, and on their person page.
Reviewers: btrahan, vrana, edward, jungejason
Reviewed By: vrana
CC: aran
Maniphest Tasks: T345
Differential Revision: https://secure.phabricator.com/D2331
Summary:
Also couple of small changes:
- Add method name to title.
- 404 for /conduit/method/x/.
- Remove utilities from side panel.
- Remove side panel from log.
Test Plan:
/conduit/
/conduit/method/x/
/conduit/method/user.whoami/
/conduit/log/
Reviewers: btrahan, epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2326
Summary: We now allow symbolic commits, so let them through the pipeline.
Test Plan: {F10571}
Reviewers: davidreuss, btrahan, vrana
Reviewed By: davidreuss
CC: aran
Maniphest Tasks: T1130
Differential Revision: https://secure.phabricator.com/D2315
Summary:
This is somewhat controversial but push date is usually more useful than commit date (which can be for example a month before other people can see the commit).
We can also store both dates.
Test Plan:
git log --pretty="%ct %at"
Reviewers: epriestley
Reviewed By: epriestley
CC: nh, aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2319
Summary: Inspired by D2242.
Test Plan:
Select text in left pane.
Select text in right pane.
Select all.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2249
Summary: I thought that this will be fun but the elasticsearch API is horrible and the documentation is poor.
Test Plan:
Search for:
- string
- author
- author, owner
- string, author
- open
- string, open, author
- string, exclude
- several authors, several owners
- nothing
- probably all other combinations
Normally, such an exhaustive test plan wouldn't be required but each combination requires a completely different query.
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, Koolvin, btrahan
Differential Revision: https://secure.phabricator.com/D2298
Summary:
- When viewing a commit, show its tags.
- For commits with many tags, show a list of all tags on the tag list interface.
- Improve some handling of symbolic references.
- When tags contain content, show it on the browse view reached by clicking the tag name.
Test Plan: Looked at commits with and without tags, clicked "More tags...", clicked tag names.
Reviewers: btrahan, vrana, davidreuss, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1130
Differential Revision: https://secure.phabricator.com/D2290
Summary:
- Track + message through file moves.
- Stop + message on file create.
- Stop + message on first commit.
Test Plan:
- Tested blaming through a move, through a create, and through the first commit.
- Verified this doesn't break anything in SVN / Mercurial.
Reviewers: vrana, btrahan, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1091
Differential Revision: https://secure.phabricator.com/D2295
Summary: "Committed" is SVN-specific language, and confusing in Git and Mercurial. Use neutral language instead.
Test Plan: Inspection.
Reviewers: btrahan, Makinde, vrana, jungejason
Reviewed By: vrana
CC: aran
Maniphest Tasks: T909
Differential Revision: https://secure.phabricator.com/D2087
Test Plan:
Added CC's/Auditors, clicked the form elements, and saw correct
behaviour. Verified that metadata was present in the detail table.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, 20after4, Koolvin
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D2002
Summary: we were using the "path" as the next_uri and that drops some delicious get parameters
Test Plan: see T1140; basically re-ran the steps listed there and they passed!
Reviewers: epriestley, njhartwell
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1140, T1009
Differential Revision: https://secure.phabricator.com/D2299
Summary:
I have no idea what I'm doing, but here's part of an elasticsearch engine. These things work:
- Indexing stuff (??)
- Searching for text/type?
- Reconstructing things??
All the complicated stuff doesn't work. I'm having a hard time figuring out the best way to model things because elasticsearch's documentation is not exactly the most complete or illuminating.
@amckinley, does this look sane-ish so far? Particularly, the /phabricator/<type>/<phid>/ URI scheme and how I've set up the relationships and fields in the documents?
How should I model the relationship and field queries? I want, like, an "equal" query but it seems like I've got "text" or "term" to work with and neither are exact match? And "term" doesn't consider PHIDs to be terms since they have hyphens in them?
I'll keep kind of slogging my way forward here but if you have valuable wisdom to share it would probably get me to a better end state much faster. The whole query construction phase is pretty much black magic to me.
Test Plan: nyancat
Reviewers: amckinley, vrana
Reviewed By: vrana
CC: jungejason, tuomaspelkonen, aran, 20after4, vrana
Differential Revision: https://secure.phabricator.com/D790
Summary:
We will need it for intl.
I've put it to User instead of UserProfile to be easier accessible.
Test Plan:
Apply SQL patch.
Change sex to Male.
Change sex to Unknown.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1139
Differential Revision: https://secure.phabricator.com/D2287
Summary:
This is slightly more complicated for this reason:
- We don't set `dateCommitted` for normal commits, only for markcommitted.
-- We need to add this date to old revisions now.
Test Plan:
Reparse a revision - commit date was set.
Conduit `markcommitted` - commit date was set.
Run SQL script.
Display closed revision.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2282
Summary:
PHP has this crazy [[ http://php.net/arg_separator.output | arg_separator.output ]] INI setting which allows setting different string for URL parameters separator instead of `&` (e.g. in `?a=1&b=2`).
Don't use it for external URLs.
Test Plan: Log in through OAuth.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2284
Summary:
These are explicit copies of implicitly-generated Lisk methods.
See brief discussion in rPdec8bac3a3af6065166d485db80fffa70dc2abe3.
Test Plan: Looked at a diff in Differential.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2283
Summary:
This is better than writing "(UNSTABLE!!!)" in front of the text description.
I'll add a wiki to keep track of API changes, too.
See also D2087, which motivates this.
Test Plan: Browsed console, saw "deprecated" and "unstable" on appropriate methods.
Reviewers: btrahan, vrana, jungejason
Reviewed By: vrana
CC: aran
Maniphest Tasks: T909
Differential Revision: https://secure.phabricator.com/D2271
Summary:
move up the panels which generally has short length.
Test Plan:
view the page.
Reviewers: blair, vrana
Reviewed By: vrana
CC: epriestley, aran
Differential Revision: https://secure.phabricator.com/D2228
Summary: This is not very nice.
Test Plan: /P1
Reviewers: codeblock
Reviewed By: codeblock
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2267
Summary: NOTE: This is starting to be too hacky.
Test Plan:
View revision with inline diffs, verify that Reply is there.
View standalone - no Reply.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2263
Summary: Lists the 25 most recent tags on the "Repository" page.
Test Plan: Looked at a git repository with a tag, saw it. Looked at HG/SVN repos, they didn't break.
Reviewers: davidreuss, 20after4, btrahan, vrana, jungejason
Reviewed By: davidreuss
CC: aran
Maniphest Tasks: T1130
Differential Revision: https://secure.phabricator.com/D2255
Summary: Also link to `D1?id=` instead of `?id=` because some IE versions linked to root in this case.
Test Plan: Click on old diff's inline comment link on large revision.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2260
Summary:
- Adds "Commandeer Revision", to allow you to plunder revisions from those lost to sea (e.g., interns who have left or co-workers who are dealing with a family emergency).
- Removes admin-abandon to simplify things, since you can just Commandeer + Abandon now.
- There are other workarounds available but this is the natural/expected workflow (and the one everyone always asks for) and there's no real reason not to allow it.
Test Plan: Swashbuckled.
Reviewers: cpiro, btrahan, vrana, jungejason
Reviewed By: cpiro
CC: aran, zeeg
Differential Revision: https://secure.phabricator.com/D2257
Summary:
If I have Pygments enabled in config but `pygmentize` doesn't work then unhighlighted source is stored to cache.
If I later make `pygmentize` work then the unhighlighted source is still loaded from the cache.
Test Plan:
Break `pygmentize`.
View a diff with JS files.
Fix `pygmenize`.
View the diff again.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, gatos99, Koolvin
Differential Revision: https://secure.phabricator.com/D2227
Summary:
Couple of small improvements:
- Delete `randomon` macro.
- Make name unique (deleting current conflicts randomly).
- Image macro must be alone on the line.
- Filter by name.
Test Plan:
Run SQL.
/file/macro/
/file/macro/?name=imagemacro
Try to create conflicting name.
Write this comment:
Test imagemacro.
imagemacro
Reviewers: aran, epriestley
Reviewed By: epriestley
CC: epriestley, Koolvin
Differential Revision: https://secure.phabricator.com/D2230
Summary:
This event is fired after a task is created and assigned with an id.
Use case is sending an email notification to everyone in a project when a new task is
submitted to said project.
Test Plan:
Implement the event listener, submit a new task to a project, see if the project members
receive an email notification. I will submit the event handler in a separate diff once it's a bit
prettier and tested more thoroughly.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, jungejason
Differential Revision: https://secure.phabricator.com/D2159
Summary:
- Add an "Administrators" policy.
- Allow "Public" to be completely disabled in configuration.
- Simplify unit tests, and cover the new policies.
Test Plan: Ran unit tests.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D2238
Summary: This appears to sometimes be effective (for MS clients), and we've seen it in the wild on inbound mail.
Test Plan: Sent myself some mail, verified it had the right header.
Reviewers: btrahan, vrana, jungejason
Reviewed By: vrana
CC: aran
Maniphest Tasks: T571
Differential Revision: https://secure.phabricator.com/D2241
Summary:
- For line numbers, use "user-select: none" to make them unselectable. This provides a stronger visual cue that copy/paste is enchanted.
- In Paste, make it look sensible again after the blame-on-blame refactor in Diffusion. See also TODO to share this code formally.
- In Diffusion, use the "phabricator-oncopy" behavior.
NOTE: I left blame/commit columns selectable in Diffusion, since you might reasonably want to copy/paste them?
NOTE: In Differential, the left side of the diff still highlights, even though it will be copied only if you select part of a line on the left and nothing else. But this seemed like a reasonable behavior, so I left it.
Test Plan:
- Looked at Paste. Saw a nice line number column. Selected text, got the expected selection. Copied text, got the expected copy.
- Looked at Diffusion. Saw a nice line number column, still. Selected text, got expected selection. Copied text, got expected copy.
- Looked at Differential. Highlighted stuff, got expected results. Copied stuff, got expected results.
Reviewers: btrahan, vrana, jungejason
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1123
Differential Revision: https://secure.phabricator.com/D2242
Summary: See rP23f25edd97f052ff4c1c5d8c4be962b4da149bca.
Test Plan: RAN LINT AND UNIT TESTS. VERIFIED THERE ARE NO SYNTAX ERRORS.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2240
Summary:
With invalid session (which happens for me when I change production and dev db but can of course happen in other cases), Phabricator displays an ugly unhandled exception dialog suggesting to logging in again.
But there's no login dialog on that page.
This also changes how users with invalid session are treated on pages not requiring logging.
Previously, an exception was thrown on them. Now they are treated as unlogged users.
Test Plan: Corrupt session, go to /, login.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2236
Summary:
This continues work started at D2215.
Files moved from deleted directory were marked as Copied Here instead of Moved Here.
Test Plan: Reparsed two commits which was previously wrong, now correct.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Maniphest Tasks: T1114
Differential Revision: https://secure.phabricator.com/D2229
Summary:
Provides a basic start for access policies. Objects expose various capabilities, like CAN_VIEW, CAN_EDIT, etc., and set a policy for each capability. We currently implement three policies, PUBLIC (anyone, including logged-out), USERS (any logged-in) and NOONE (nobody). There's also a way to provide automatic capability grants (e.g., the owner of an object can always see it, even if some capability is set to "NOONE"), but I'm not sure how great the implementation feels and it might change.
Most of the code here is providing a primitive for efficient policy-aware list queries. The problem with doing queries naively is that you have to do crazy amounts of filtering, e.g. to show the user page 6, you need to filter at least 600 objects (and likely more) before you can figure out which ones are 500-600 for them. You can't just do "LIMIT 500, 100" because that might have only 50 results, or no results. Instead, the query looks like "WHERE id > last_visible_id", and then we fetch additional pages as necessary to satisfy the request.
The general idea is that we move all data access to Query classes and have them do object filtering. The ID paging primitive allows efficient paging in most cases, and the executeOne() method provides a concise way to do policy checks for edit/view screens.
We'll probably end up with mostly broader policy UIs or configuration-based policies, but there are at least a few cases for per-object privacy (e.g., marking tasks as "Security", and restricting things to the members of projects) so I figured we'd start with a flexible primitive and the simplify it in the UI where we can.
Test Plan: Unit tests, played around in the UI with various policy settings.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D2210
Summary:
...pretty sure the JS is too hack-tastic but it works...! :D
also fixed a small error from assert_instances_of change where a null value is all errors and what have you
Test Plan: played around with tasks in firefox and safari. made cc, owner, and project changes, as well as priority, etc.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Maniphest Tasks: T1074
Differential Revision: https://secure.phabricator.com/D2234
Summary: This is not perfect. Moved files are reported as deleted but I'm happy with it.
Test Plan: Reparsed two commits which was previously wrong, now semi-correct.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Maniphest Tasks: T1114
Differential Revision: https://secure.phabricator.com/D2215
Summary:
'cuz we need to be phamous!
V1 feature set
- posts
-- standard thing you'd expect - a title and a remarkup-powered body and...
-- "phame" title - a short string that can be used to reference the story. this gets auto-updated when you mess with the title.
-- configuration - for now, do you want Facebook, Disqus or no comments? this is a per-post thing but feeds from an instance-wide configuration
Please do toss out any must have features or changes.
Test Plan: played around with this bad boy like whoa
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, vrana
Maniphest Tasks: T1111
Differential Revision: https://secure.phabricator.com/D2202
Summary:
- Add an explicit multiplexing option, and enable it by default. This is necessary for Mail.app to coexist with other clients ("Re:" breaks outlook at the very least, and generally sucks in the common case), and allows users with flexible clients to enable subject variance.
- Add an option for subject line variance. Default to not varying the subject, so mail no longer says [Committed], [Closed], etc. This is so the defaults thread correctly in Gmail (not entirely sure this actually works).
- Add a preference to enable subject line variance.
- Unless all mail is multiplexed, don't enable or respect the "Re" or "vary subject" preferences. These are currently shown and respected in non-multiplex cases, which creates inconsistent results.
NOTE: @jungejason @nh @vrana This changes the default behavior (from non-multiplexing to multiplexing), and might break Facebook's integration. You should be able to keep the same behavior by setting the options appropriately, although if you can get the new defaults working they're probably better.
Test Plan:
Send mail from Maniphest, Differential and Audit. Updated preferences. Enabled/disabled multiplexing. Things seem OK?
NOTE: I haven't actually been able to repro the Gmail threading issue so I'm not totally sure what's going on there, maybe it started respecting "Re:" (or always has), but @cpiro and @20after4 both reported it independently. This fixes a bunch of bugs in any case and gives us more conservative set of defaults.
I'll see if I can buff out the Gmail story a bit but every client is basically a giant black box of mystery. :/
Reviewers: btrahan, vrana, jungejason, nh
Reviewed By: btrahan
CC: cpiro, 20after4, aran
Maniphest Tasks: T1097, T847
Differential Revision: https://secure.phabricator.com/D2206
Summary: Sometimes we get a lowercase "Meddelelse" in Danish outlook. Relax the patterns since the risk of hitting false positives here is essentially nonexistant.
Test Plan: Unit tests.
Reviewers: davidreuss, btrahan, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2205
Summary:
Owners field is filled by Primary Owner which is required.
So that it is not neccessary to require filling Owners explicitly.
Test Plan: Don't fill Owners and successfully save the form //before// this change.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2201
Summary:
Some Differential fields are not nullable; when Test Plan is switched to non-required mode we can end up trying to save a null value to a non-nullable column (see D2193).
(I should probably just alter the schema to make these fields nullable, but that might have farther-reaching effects.)
Test Plan: Reproduced error, applied patch, no more error.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2200
Summary: Partially broken by D2166.
Test Plan:
Hover line number in revision.
Hover line number in standalone view.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2196
Summary:
This is a somewhat common request, and far more difficult than necessary currently.
I think the field is useful enough to leave it default-enabled, but there's wide diversity in testing philosophy.
Test Plan: Verified "test plan" field appeared. Disabled config. Verified "test plan" field vanished.
Reviewers: btrahan, vrana, jungejason
Reviewed By: vrana
CC: aran, asouza
Differential Revision: https://secure.phabricator.com/D2193
Summary: We'll get a typehint warning on the repository if there's no repository. Check outside the method instead.
Test Plan: Loaded page, no warning.
Reviewers: btrahan, vrana, jungejason
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2194
Summary:
- We currently post-filter by branches, but should do this in SQL. See T799.
- We currently identify branch-name-matches as being in the working copy even if they belong to a different project (e.g., two different projects with commits on the branch "master"). See T1100.
- Denormalize branch and project information into DifferentialRevision.
- Expose project information in the API.
Test Plan: Ran conduit API queries with branches and arc project IDs, got reasonable results.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1100, T799
Differential Revision: https://secure.phabricator.com/D2190
Summary:
Otherwise useless query is executed:
lang=sql
SELECT c.*
FROM `repository_commit` c
ORDER BY c.epoch DESC
Test Plan: /diffusion/X/browse/x
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2186
Test Plan:
/rX1
Browse in Diffusion
Open in Editor
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2180
Summary:
There have been a couple of requests for this since bookmarks are "out this year like woah" and "totally uncool dude".
Allow users to save named custom queries and make them the /maniphest/ default if they so desire.
A little messy. :/
Test Plan: Saved, edited, deleted custom queries. Made custom query default; made 'no default' default. Verified default behavior. Issued a modified search from a custom query.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley, 20after4
Maniphest Tasks: T923, T1034
Differential Revision: https://secure.phabricator.com/D1964
Summary:
When Phabricator is configured to generate patch email, we'll fatal if the patch contains binaries and is generating to Git because ArcanistBundle can't load the binary data. Provide a callback to load the data. See D2174.
(This may cause us to generate absolutely enormous emails, but you get what you asked for...)
Test Plan: Created a diff with an image under "send git patches" email configuration.
Reviewers: Makinde, btrahan, vrana, jungejason
Reviewed By: Makinde
CC: aran
Differential Revision: https://secure.phabricator.com/D2175
Summary:
PHP arrays have an internal "current position" marker. (I think because foreach() wasn't introduced until PHP 4 and there was no way to get rid of it by then?)
A few functions affect the position of the marker, like reset(), end(), each(), next(), and prev(). A few functions read the position of the marker, like each(), next(), prev(), current() and key().
For the most part, no one uses any of this because foreach() is vastly easier and more natural. However, we sometimes want to select the first or last key from an array. Since key() returns the key //at the current position//, and you can't guarantee that no one will introduce some next() calls somewhere, the right way to do this is reset() + key(). This is cumbesome, so we introduced head_key() and last_key() (like head() and last()) in D2161.
Switch all the reset()/end() + key() (or omitted reset() since I was feeling like taking risks + key()) calls to head_key() or last_key().
Test Plan: Verified most of these by visiting the affected pages.
Reviewers: btrahan, vrana, jungejason, Koolvin
Reviewed By: jungejason
CC: aran
Differential Revision: https://secure.phabricator.com/D2169
Summary:
There was a typo:
`PHID-!!!!-NO_PROJECT` instead of
`PHID-!!!!-NO-PROJECT`
Also use `<em>` to differentiate from project named "(No Project)".
Test Plan:
/maniphest/report/project/
Click on (No Project).
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2167
Test Plan: I didn't repro it probably because of custom rules.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Maniphest Tasks: T1090
Differential Revision: https://secure.phabricator.com/D2150
Summary: See T1021. Raise configuration or implementation exceptions immediately. When all engines fail, raise an aggregate exception with details.
Test Plan: Forced all engines to fail, received an aggregate exception. Forced an engine to fail with a config exception, recevied it immediately.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1021
Differential Revision: https://secure.phabricator.com/D2157
Summary: See discussion in T789. Covered the obvious cases, at least. We can refine this as we get a larger sample size.
Test Plan: Unit test coverage.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T789
Differential Revision: https://secure.phabricator.com/D2154
Summary:
We don't use versioned URIs for images, so when they change users may get old versions.
This was a particular issue with the recent logo change, which several users reported cache-related issues from.
Instead, use Celerity to manage image URI versions in addition to CSS/JS.
This is complicated, because we need to rewrite image URIs inside of CSS, which means the hash of a CSS file has to be derived from the current image data. Otherwise, when we updated an image the CSS wouldn't update, so we wouldn't be any better off.
So basically we:
- Find all the "raw" files, and put them into the map.
- Find all the CSS/JS, perform content-altering transformations on it (i.e., not minification) based on the partial map, and then put it into the map based on transformed hashes.
(If we wanted, we could now do CSS variables or whatever for "free", more or less.)
Test Plan:
- Regenerated celerity map, browsed site, verified images generated with versioned URIs.
- Moved "blue" flag image over "green" flag image, regenerated map, verified "green" flag image and the associated CSS changed hashes.
- Added transformation unit tests; ran unit tests.
Reviewers: btrahan, vrana, jungejason
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1073
Differential Revision: https://secure.phabricator.com/D2146
Summary: These elements look heavy and out of place right now.
Test Plan: Looked at error views in uiexample page.
Reviewers: btrahan, vrana, jungejason
Reviewed By: jungejason
CC: aran
Differential Revision: https://secure.phabricator.com/D2144
Summary: Currently, we show them everything. Instead, show them an explicit notice.
Test Plan: Looked at "My Projects" feed with no projects.
Reviewers: btrahan, vrana, jungejason
Reviewed By: jungejason
CC: aran
Maniphest Tasks: T1015
Differential Revision: https://secure.phabricator.com/D2143
Summary:
- Make some effort to simplify the code.
- Make "Skip Past This Commit" work in Git and Mercurial.
- Make blame work in Mercurial.
- Add tooltip hover state to show more information about commits.
Test Plan: Viewed blame views in SVN, Git, Hg. Clicked line numbers, hovered/clicked commits, hovered/clicked "blame past..."
Reviewers: btrahan, vrana, jungejason
Reviewed By: vrana
CC: aran
Maniphest Tasks: T378
Differential Revision: https://secure.phabricator.com/D2142
Summary: See D2080. The introduction of `arc land`, defaulting to `origin/master`, and --auto enormously simplifies the documentation.
Test Plan: Read documentation.
Reviewers: btrahan
Reviewed By: btrahan
CC: 20after4, aran
Maniphest Tasks: T894
Differential Revision: https://secure.phabricator.com/D2082
I think the issue is that we don't set the left-side changesetID correctly. This seems to work correctly locally, but I'm not sure I got a good repro. Pushing to verify the production test cases provided in T1076.
Auditors: vrana, btrahan
Summary:
The audit tools has many false positive about Author Not
Matching with Revision. The fix is to set the authorPHID which was
missing in the existing code
Test Plan:
run reparse.php and it doesn't generate false positive result
anymore.
Reviewers: nh, epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2119
Summary: renderMiniPanel() renders the entire <p>.
Test Plan: Looked at page source for homepage, verified there was no double </p>.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1079
Differential Revision: https://secure.phabricator.com/D2128
Summary: This separates common MySQL stuff (identifiers and comments escaping, error codes, connection retries) from PHP extension specific stuff (connect, query, fetch, errors, escape string).
Test Plan:
/
Use `AphrontMySQLiDatabaseConnection` in `PhabricatorLiskDAO`, load homepage, edit task, save task.
Reviewers: epriestley
Reviewed By: epriestley
CC: nh, aran
Differential Revision: https://secure.phabricator.com/D2113
Summary: I've found it useful mainly on smaller screen or with lots of comments.
Test Plan: Show Diff
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2122
Summary:
This introduces some boundary checking for
PhabricatorOwnersOwner::loadAffiliatedUserPHIDs() if it gets passed an empty
array, which happened when I ran arc diff and it called
differential.createrevision.
Test Plan: ran arc diff
Reviewers: epriestley, meitros, jungejason
CC: aran
Differential Revision: https://secure.phabricator.com/D2112
Summary:
I've considered that user may have set editor but not checked out Phabricator repositories.
But stack trace is useful mainly for developers.
Test Plan:
Click on path in Unhandled Exception.
Repeat with disabled editor.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2107
Summary: ...also make the pager usage in ChatLog use the nice formatWhereClause functionality
Test Plan: set $page_size = 2 and paged around the data a bit
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Maniphest Tasks: T905
Differential Revision: https://secure.phabricator.com/D2106
Summary: Use Edges to attach Commits and Tasks. Note, no "edit attached commits" interface from tasks yet since the search backend needs a little work to list commits in a sensible way.
Test Plan: Attached commits to tasks. Looked at commits, looked at tasks.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D2105
Summary:
various stripe stuff, including
- external stripe library
- payment form
- test controller to play with payment form, sample business logic
My main questions / discussion topics are...
- is the stripe PHP library too big? (ie should I write something more simple just for phabricator?)
-- if its cool, what is the best way to include the client? (ie should I make it a submodule rather than the flat copy here?)
- is the JS I wrote (too) ridiculous?
-- particularly unhappy with the error message stuff being in JS *but* it seemed the best choice given the most juicy error messages come from the stripe JS such that the overall code complexity is lowest this way.
- how should the stripe JS be included?
-- flat copy like I did here?
-- some sort of external?
-- can we just load it off stripe servers at request time? (I like that from the "if stripe is down, stripe is down" perspective)
- wasn't sure if the date control was too silly and should just be baked into the form?
-- for some reason I feel like its good to be prepared to walk away from Stripe / switch providers here, though I think this is on the wrong side of pragmatic
Test Plan: - played around with sample client form
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2096
Summary:
We have a lot of cases where we store object relationships, but it's all kind of messy and custom. Some particular problems:
- We go to great lengths to enforce order stability in Differential revisions, but the implementation is complex and inelegant.
- Some relationships are stored on-object, so we can't pull the inverses easily. For example, Maniphest shows child tasks but not parent tasks.
- I want to add more of these and don't want to continue building custom stuff.
- UIs like the "attach stuff to other stuff" UI need custom branches for each object type.
- Stuff like "allow commits to close tasks" is notrivial because of nonstandard metadata storage.
Provide an association-like "edge" framework to fix these problems. This is nearly identical to associations, with a few differences:
- I put edge metadata in a separate table and don't load it by default, to keep edge rows small and allow large metadata if necessary. The on-edge metadata seemed to get abused a lot at Facebook.
- I put a 'seq' column on the edges to ensure they have an explicit, stable ordering within a source and type.
This isn't actually used anywhere yet, but my first target is attaching commits to tasks for T904.
Test Plan: Made a mock page that used Editor and Query. Verified adding and removing edges, overwriting edges, writing and loading edge data, sequence number generation.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, 20after4
Differential Revision: https://secure.phabricator.com/D2088
Summary: NOTE: This is not produced by a script so there might be errors. Please review carefully.
Test Plan: Browse around Differential.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2103
Summary: I looooove JS! It makes me giddy with glee!
Test Plan: Picked dates. See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2086
Summary:
When system agent adds a comment then he is added to CC.
When I amend and update then I get message "Commit message references nonexistent ..."
Test Plan: Update revision with system agent in CC.
Reviewers: epriestley
Reviewed By: epriestley
CC: michalburger1, aran
Differential Revision: https://secure.phabricator.com/D2100
Test Plan:
Jump to head.
Go to doctor.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2097
Summary: NOTE: This is not produced by a script so there might be errors. Please review carefully.
Test Plan: Browse around Diffusion.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2094
Summary:
- The UI is pretty straightforward, since Handle just works (tm)
- Added two methods to the owners object to handle the new layer of
indirection. Then ran git grep PhabricatorOwnersOwner and changed
callsites as appropriate.
Sending this to get a round of feedback before I test the non-trivial
changes in this diff.
Test Plan:
- owners tool: edit, view, list for basic functionality.
- phlog for the two new methods I added
Reviewers: epriestley, blair, jungejason
CC: aran
Differential Revision: https://secure.phabricator.com/D2079
Summary: NOTE: This is not produced by a script so there might be errors. Please review carefully.
Test Plan: Browse around.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2091
Summary: Delete some dead code in Feed along the way.
Test Plan:
/feed/
/search/
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2092
Summary:
Most setters returns `$this` but some don't.
I guess it's not by purpose.
Test Plan:
arc lint
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2085
Summary: As title
Test Plan: hit diffcamp, owners to test HandleData
Reviewers: epriestley, btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2063
Summary:
I wanted to search for D1234 in texts of other documents.
But search tool always redirects me.
I've left the redirect behavior for simple search forms (header and home) and removed it from full search form.
I don't consider this complete because the first result in search for D1234 should be of course D1234 which is not the case currently.
I am not sure how to solve it:
- We can display a special result in this case.
- We can index the documents so that they will be searchable also for short strings.
I tend to use the first solution because revisions can be truncated at arbitrary length (rX1f1f1f should display revision rX1f1f1f1f1f1f1f).
Test Plan: Search for D1234, rX123, T4.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, ddfisher
Differential Revision: https://secure.phabricator.com/D1905
Summary: Added a regex to remove the text
Test Plan: Tested a few messages, from mail application them gmail, both seemed fine, will add unit tests
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2078
Summary:
Show application names, then a human-readable description of what they're for.
Eventually we'll have better help / tutorial / onboarding / etc systems too.
Test Plan: See screenshot.
Reviewers: btrahan, mgummelt
Reviewed By: btrahan
CC: aran, davidreuss
Differential Revision: https://secure.phabricator.com/D2075
Summary:
Like the title says, similar to Facebook Tasks.
Not sure how I really feel about this, but I guess it's kind of OK? I never used
this feature in Facebook Tasks but I think some people like it.
The drag-and-drop to repri across priorities feels okayish.
Because subpriority is a double and we just split the difference when
reprioritizing, you lose ~a bit of precision every time you repri two tasks
against each other and so you can break it by swapping the priorities of two
tasks ~50 times. This case is pretty silly and pathological. We can add some
code to deal with this at some point if necessary.
I think this also fixes the whacky task layout widths once and for all.
(There are a couple of minor UI glitches like headers not vanishing and header
counts not updating that I'm not fixing because I am lazy.)
Test Plan: Dragged and dropped tasks around.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley, mgummelt
Maniphest Tasks: T859
Differential Revision: https://secure.phabricator.com/D1731
Summary:
We'll incorrectly send CCWelcome mail to users who would be added as CCs but are blocked by the new "$dont_add" stuff, for
example when a revision is updated and the user has a Herald rule which triggers them getting CC'd. See D2057.
Potentially a better fix for this would be to have "addCCs" return a list of the CCs it actually added, rather than duplicating the
logic of removing CCs in two places. However, that's not trivial since it's just a wrapper around alterRelationships() which is nasty
and would need a more complicated return type. I think this whole thing will get a refactoring pass at some point -- I want to build a
more generic "associations"-like datastore and replace some of the ad-hoc associations with it. So maybe I can clean it up when that
happens. For now, this should fix the immediate problem.
Test Plan: Updated a revision, didn't get CC welcomed.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2072
Summary:
- Remove the "Priority" column, since this is indicated by the color swatch, to save space.
- Reduce the "Updated" column from datetime to date only, since time isn't incredibly useful, to save space.
- Show the first two projects a task is associated with, and "..." if there are more.
- Show "None" (for "no owner") in a lighter color.
Test Plan: Looked at tasks on homepage and in Maniphest.
Reviewers: btrahan, 20after4
Reviewed By: btrahan
CC: aran, edward
Maniphest Tasks: T967
Differential Revision: https://secure.phabricator.com/D2065
Summary:
Format a date as 'today', 'yesterday', or 'Mar 27 2012'. Optionally,
the final example can be rendered 'on Mar 27 2012' for things like:
$excuse =
'I fell out of a window '.
phabricator_on_rel_date($time, $me);
Test Plan: Tested in my sandbox!!!!
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2035
Summary:
Boolean search supports operators, such as phrase search.
It can be further improved by setting [[http://dev.mysql.com/doc/mysql/en/server-system-variables.html#sysvar_ft_boolean_syntax | ft_boolean_syntax]] to `' |-><()~*:""&^'` (note the leading space):
Default value uses no operator for "optional word" and `+` for "mandatory word".
This value uses no operator for "mandatory word" and `|` for "optional word".
Test Plan: Search for "Enter the name" (with quotes).
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2064
Summary:
- Differential, Maniphest and Diffusion use slightly different styles for the object detail panels.
- Instead, use the same styles and CSS.
- Add object actions to Diffusion, including "Flag".
Test Plan: Looked at revisions, tasks and commit. Flagged and unflagged commits.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T1041
Differential Revision: https://secure.phabricator.com/D2062
Summary:
**Who can delete global rules?**: I discussed this with @jungejason. The current behavior is that the rule author or any administrator can delete a global rule, but this
isn't consistent with who can edit a rule (anyone) and doesn't really make much sense (it's an artifact of the global/personal split). I proposed that anyone can delete a
rule but we don't actually delete them, and log the deletion. However, when it came time to actually write the code for this I backed off a bit and continued actually
deleting the rules -- I think this does a reasonable job of balancing accountability with complexity. So the new impelmentation is:
- Personal rules can be deleted only by their owners.
- Global rules can be deleted by any user.
- All deletes are logged.
- Logs are more detailed.
- All logged actions can be viewed in aggregate.
**Minor Cleanup**
- Merged `HomeController` and `AllController`.
- Moved most queries to Query classes.
- Use AphrontFormSelectControl::renderSelectTag() where appropriate (this is a fairly recent addition).
- Use an AphrontErrorView to render the dry run notice (this didn't exist when I ported).
- Reenable some transaction code (this works again now).
- Removed the ability for admins to change rule authors (this was a little buggy, messy, and doesn't make tons of sense after the personal/global rule split).
- Rules which depend on other rules now display the right options (all global rules, all your personal rules for personal rules).
- Fix a bug in AphrontTableView where the "no data" cell would be rendered too wide if some columns are not visible.
- Allow selectFilter() in AphrontNavFilterView to be called without a 'default' argument.
Test Plan:
- Browsed, created, edited, deleted personal and gules.
- Verified generated logs.
- Did some dry runs.
- Verified transcript list and transcript details.
- Created/edited all/any rules; created/edited once/every time rules.
- Filtered admin views by users.
Reviewers: jungejason, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2040
Summary:
Herald rules are adding CC also for Author and Reviewer.
See also D1397.
I was considering also just don't displaying the extra CC but this is probably better.
There are still cases where there could be reviewer in CC (e.g. by making reviewer from CC or by direct edit) but I think it's not a big problem.
Beeing both Reviewer and CC can be actually useful (e.g. if you resign than you still are in CC) but it's not that useful to justify this:
Author: vrana
Reviewers: epriestley
CCs: vrana, epriestley
Test Plan: Comment on revision where I am author.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2057
Summary:
I think this feature is probably good, but Differential is also really starting to get a lot of stuff which is not the diff in it. Not sure how best to deal with that.
The mixed table styles are also pretty ugly.
So I guess this is more feedback / proof-of-concept, I think I want to try to improve it somehow before I land it.
Test Plan: Looked at some diffs, some had an awkward, ugly list of diffs affecting the same files.
Reviewers: bill, aran, btrahan
Reviewed By: aran
CC: aran, epriestley
Maniphest Tasks: T829
Differential Revision: https://secure.phabricator.com/D2027
Summary:
These are the issues identified by the linter in D2052. I don't think any cause bugs, but they are all reasonable errors to raise and the linter correctly
detected that they are suspicious.
Test Plan: Mostly inspection.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2053
Summary: We may overwrite $comment as a side effect of iteration.
Test Plan: Made some audit comments as different users.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2050
Summary:
When there is a single line Test Plan (or anything else) then `arc amend` puts it on the same line as label.
It is a problem with indented line (as in this diff) because next run of `arc diff` will trim the leading spaces.
Test Plan: arc amend
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2054
Summary:
When there is a single line Test Plan (or anything else) then `arc amend` puts it on the same line as label.
It is a problem with indented line (as in this diff) because next run of `arc diff` will trim the leading spaces.
Test Plan: arc amend
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2054
Summary:
The full path field of the DiffusionRepositoryPath object is used by the
DiffusionBrowseController when viewing a directory with a readme file, so
we should set this field.
Test Plan: loaded a directory containing a readme in a svn repo
Reviewers: epriestley, jungejason, emiraga
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2045
Test Plan: Looked at /differential/ with and without flags.
Reviewers: nh, btrahan
Reviewed By: nh
CC: aran, epriestley
Maniphest Tasks: T1055
Differential Revision: https://secure.phabricator.com/D2044
Summary:
- When an inline comment preview corresponds to an inline comment on the page, link to it. Just punt in the tough case where the inline is on some other page.
- In "haunted" mode, "z" now toggles through three modes: normal, comment area only, and comment + previews.
Test Plan:
- Viewed visible and not-visible inline comment previews, clicked "View" links.
- Tapped "z" a bunch to toggle haunt modes.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T517, T214
Differential Revision: https://secure.phabricator.com/D2041
Summary: 'cuz I miss out on chat room goodness and can't paginate around in the current version
Test Plan: setup a phabot and spammed it in phabot-test. with new test data, set $page_limit = 1 and paged about -- looks good!
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T990
Differential Revision: https://secure.phabricator.com/D2032
Summary:
Flags are a personal collection of things you want to take a look at later. You can use several different colors and add notes.
Not really sure if this is actually a good idea or not but it was easy to build.
Planned features:
- Allow Herald rules to add flags.
- In the "edit flag" dialog, have a "[x] Subscribe Me" checkbox that CCs you.
- Support Diffusion.
- Support Phriction.
- Always show flags on an object if you have them (in every view)?
- Edit dialog feels a little heavy?
- More filtering in /flag/ tool.
- Add a top-level links somewhere?
Test Plan: Added, edited and removed flags from things. Viewed flags in flag view.
Reviewers: aran, btrahan
Reviewed By: btrahan
CC: aran, epriestley, Koolvin
Maniphest Tasks: T1041
Differential Revision: https://secure.phabricator.com/D2024
Summary:
If a file was last modified at revision 50 and you look at revision 55, we currently 404. Instead, always identify the last modification.
Also simplify some of the query objects.
Test Plan: Viewed after-modification revisions for several files in SVN.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran, epriestley
Maniphest Tasks: T851
Differential Revision: https://secure.phabricator.com/D2028
Summary: See T955. We jump to an awkard place right now; jump above the comment instead.
Test Plan: Clicked inline comment anchor links.
Reviewers: Makinde, btrahan
Reviewed By: Makinde
CC: aran, epriestley
Maniphest Tasks: T955
Differential Revision: https://secure.phabricator.com/D2029
Summary:
- Still really really rough.
- Adds a full synchronous mode for debugging.
- Adds some logging.
- It can now allocate EC2 machines and put webroots on them in a hacky, terrible way.
- Adds a base query class.
Test Plan: oh hey look a test page? http://ec2-50-18-65-151.us-west-1.compute.amazonaws.com:2011/
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D2026
Summary: Show parent commit information to make it easier to understand merges.
Test Plan: Looked at commits in SVN, hg, git.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T961
Differential Revision: https://secure.phabricator.com/D2021
Summary: COMPLETELY ORIGINAL IDEA
Test Plan: Browsed around Phabricator, got helpful readmes in some cases.
Reviewers: davidreuss, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2022
Summary: This header allows recipients to distinguish between CCs generated by Herald and CCs generated by humans.
Test Plan: Created a Herald rule to add a bunch of CC's to every revision. Created a revision. Added some CCs manually. Verified that only manual CCs appeared in the "Explicit" header.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T808
Differential Revision: https://secure.phabricator.com/D2018
Summary:
See some discussion in D2002. Add two new actions:
- Resign: (auditor only) closes your open request (user request ONLY) by putting it in a "resigned" state.
- Close: (author only) closes all open requests by putting them in a "closed" state.
@davidreuss, this is probably conflict-city with D2002 -- I'll wait for you to land first and then handle the merge on my end.
Test Plan: Resigned from and closed audits.
Reviewers: 20after4, davidreuss, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D2013
Summary: Adds an "allactive" filter in addition to the all projects filter.
Test Plan:
visit All Active view (/project/filter/allactive/) and
see that it lists all projects except those which have been archived.
Visit other filter views to be sure nothing else got broken.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2010
Summary:
$stdout from the previous run would be reused if an exception
occurred
Test Plan: that's a negative, ghostrider.
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2008
Summary:
When a commit is a merge, show what it merged.
Also fix some bugs:
- Mercurial queries may contain ":", but mercurial rev ranges may also contain ":". A rev range with a branch that has a ":" in it is ambigiuous, e.g. branch "a:b" might appear in a rev range like "a🅱️0", which can not be parsed. Use stable commit names instead.
- Mercurial stable commit name implementation was broken, fix it.
- Extend DiffusionHistoryQuery from DiffusionQuery to share code.
- Fix a bug where Mercurial's main browse list would not show the most recent commit if it was a merge commit.
Test Plan: Generated a bunch of mercurial/git merge commits and looked at them, they seemed to accurately represent the repository state.
Reviewers: btrahan, Makinde
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T961
Differential Revision: https://secure.phabricator.com/D2005
Summary:
- Show the canonical (i.e., shorter) commit identifier in the subject.
- For commits without a revision, put the commit summary in the subject.
Test Plan: Ran "scripts/repository/reparse.php <commit> --herald" for a number of different commits (with revision, without revision); got more useful email subjects.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T1028
Differential Revision: https://secure.phabricator.com/D2004
Summary: $link gets reused later in the function, use a different variable name to avoid broken nonsense.
Test Plan: Clicked users/projects links.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T1038
Differential Revision: https://secure.phabricator.com/D2003
Summary:
In the Differential revision list views:
- Allow you to filter by mailables (notably, mailing lists).
- Allow you to filter by user (including disabled users).
Test Plan: Filtered by a mailing list.
Reviewers: btrahan, nh
CC: aran, epriestley
Maniphest Tasks: T1031
Differential Revision: https://secure.phabricator.com/D1994
Summary: I'll mark this one up inline since it's all separate bugs.
Test Plan:
- Created a diff with eight changes: (newline absent -> newline present, newline present -> newline absent, newline present -> newline present, newline absent -> newline absent) x (short file with change near end, long file with change near middle).
- Viewed diff in Ignore All, Ignore Most, Ignore Trailing and Show All whitespace modes.
- All 32 results seemed sensible.
- Really wish this stuff was better factored and testable. Need to fix it. :(
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T1030
Differential Revision: https://secure.phabricator.com/D1992
Summary: Apparently I spent like a good month copy/pasting slightly different versions of this logic all over the codebase.
Test Plan: Selected "View Options -> Browse in Diffusion" for a chagneset, got a URI with a branch name in it under Git.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1993
Summary:
- We need to sort Projects explicitly because we go through task-by-task which ruins the ordering. My test case was just small enough not to notice.
- Push "No Project" to the bottom explicitly.
- Simplify/fix the pull of "Unassigned" to the top.
- Fix a return type.
Test Plan: Tested the various sorting cases against a larger test data set.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1988
Summary:
turns out both github and Phabricator fall back to if the user already has a login session when accessing the pertinent profile picture data. Facebook on the other hand is a stingy bastard about have an actual access token. Ergo, in production (once I could test Facebook) this button failed.
The patch sets the access token properly such that the provider can use it properly when retrieving the profile image.
Test Plan: re-did my meta-Phabricator test and it still passed. setup my phabricator dev instance for Facebook OAuth (created a test app and everything... :/ ) and it worked end to end.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T870
Differential Revision: https://secure.phabricator.com/D1986
Summary:
Currently, we use "git log" to detect the change list for all commits, but this produces no output for merge commits.
Instead, parse them as changes against the first parent (the merge destination). This produces generally sensible/expected behavior, and is consistent with what GitHub does.
We need to special-case the first commit because it doesn't have parents.
NOTE: This is a parser change so you need to run `./scripts/repository/reparse.php --all <callsign> --change` to reparse merge commits in already-imported repositories after updating.
Test Plan: Reparsed a merge commit, a non-merge commit, and the first commit in the Phabricator repository.
Reviewers: btrahan, gschmidt
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T961
Differential Revision: https://secure.phabricator.com/D1985
Summary: We need to build a request in order to pick up an appropriate default branch name, instead of using the raw static generator.
Test Plan: Clicked a symbol link, got /master/path/blahblah
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1982
Summary:
It is now possible to set config setting requiring class of certain implementation to something completely else.
The consequence is that your Phabricator may stop working after update because you didn't implement some new method.
This diff validates the class upon usage.
It throws exception which is better than fatal thrown currently after calling undefined method.
Better solution would be to validate classes when setting the config but it would be too expensive - respective class definitions would have to be loaded and checked by reflection.
I was also thinking about some check script but nobody would run it after changing config.
The same behavior should be implemented for these settings:
- metamta.mail-adapter
- metamta.maniphest.reply-handler
- metamta.differential.reply-handler
- metamta.diffusion.reply-handler
- storage.engine-selector
- search.engine-selector
- differential.field-selector
- maniphest.custom-task-extensions-class
- aphront.default-application-configuration-class
- controller.oauth-registration
Test Plan:
Send comment, verify that it pass.
Change `metamta.differential.reply-handler` to incompatible class, verify that sending comment shows nice red exception.
Set `metamta.differential.reply-handler` to empty string, verify that it throws.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1919
Summary:
- Feature request from Airtime that I missed in the feedback notes, came up yesterday.
- Identify git submodules as "FILE_SUBMODULE", not "FILE_NORMAL".
- Link git submodules to an external resolver endpoint, which tries to find commits in tracked repositories.
- Identify git symlinks as "FILE_SYMLINK", not "FILE_NORMAL".
- Add folder, file, symlink and externals icons.
Test Plan:
- externals/javelin is now identified as a submoudule and links to Javelin, not identified as a file and links to error.
- bin/phd is now identified as a symlink.
- Interfaces have pretty icons.
Reviewers: btrahan, cpiro, ddfisher, keebuhm, allenjohnashton
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1975
Summary: Add @20after4's new rule to the Phabricator engine.
Test Plan: Wrote some ~~deleted~~ text.
Reviewers: 20after4, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1972
Summary: I think these are all the actions which make any sense.
Test Plan:
- Performed and verified each action through the batch editor.
- Performed a large batch edit which applied each action type multiple times and verified the aggregate behavior was correct.
Reviewers: btrahan, cpiro
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T441
Differential Revision: https://secure.phabricator.com/D1971
Test Plan:
Search for some symbol. Click on the result. Verify that there is not // in URL.
Click on the link from generated exception.
View history in Diffusion, click on Browse.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1979
Test Plan: Clicked on `Exception` in `throw new Exception`.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1978
Summary:
A bug was introduced in
rP30ae22bfcff71fa3f14e38fd3cd597448df501c3 which caused commits to start
being parsed as if the repository callsign was empty. This caused errors
and problems and much unhappiness. Example error:
EXCEPTION: (Exception) No such repository ''. at [.../phabricator/src/applications/diffusion/request/base/DiffusionRequest.php:130]
Test Plan: Saw that commit parsing was breaking. Applied fix. Saw that commits parsed again.
Reviewers: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1974
Summary:
Aggregate multiple add/remove transactions so we don't restore removed projects for a (remove + add) batch edit.
(Possibly we should do this in the TransactionEditor as well / instead, but it's fairly easy here and this is the only possible case currently.)
Test Plan: Performed a remove + add batch edit without issues.
Reviewers: btrahan
Reviewed By: btrahan
CC: cpiro, aran, epriestley
Maniphest Tasks: T985
Differential Revision: https://secure.phabricator.com/D1967
Summary:
1. Created a filter for user comments that are not submitted yet
2. surface this information to the user by providing a "Draft revisions" link on the differential home page.
Test Plan: tested on one of the diffs
Reviewers: nh, epriestley
Reviewed By: epriestley
CC: aran, epriestley, jungejason
Differential Revision: https://secure.phabricator.com/D1927
Summary:
Depends on D1921. Depends on D1899.
Add the "View Options" dropdown menu to Diffusion, with options like "show standalone", "show raw file", "show all", etc.
Test Plan: Viewed commits in Differential and Diffusion.
Reviewers: davidreuss, nh, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1932
Summary:
Depends on D1929. In emails, notify recipients that inlines are attached.
Vaguely copy/pastey from Differential but they only share like six lines and this seems like a random piece of code to pull out.
Test Plan: Added inline comments, got email mentioning them
Reviewers: davidreuss, nh, btrahan
Reviewed By: davidreuss
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1930
Summary: Depends on D1928. Uses the new UI element to display inlines in Diffusion.
Test Plan: Looked at a commit with inline comments, saw them in the summaries.
Reviewers: davidreuss, nh, btrahan
Reviewed By: davidreuss
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1929
Summary:
This is in preparation for getting the "View Options" dropdown working on audits.
- Use Files to serve raw data so we get all the security benefits of the alternate file domain. Although the difficulty of exploiting this is high (you need commit access to the repo) there's no reason to leave it dangling.
- Add a "contentHash" to Files so we can lookup files by content rather than adding some weird linker table. We can do other things with this later, potentially.
- Don't use 'data' URIs since they're crazy and we can just link to the file URI.
- When showing a binary file or an image, don't give options like "show highlighted text with blame" or "edit in external editor" since they don't make any sense.
- Use the existing infrastructure to figure out if things are images or binaries instead of an ad-hoc thing in this class.
Test Plan: Looked at text, image and binary files in Diffusion. Verified we reuse existing files if we've already generated them.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1899
Summary:
Diffusion request/uri handling is currently a big, hastily ported mess. In particular, it has:
- Tons and tons of duplicated code.
- Bugs with handling unusual branch and file names.
- An excessively large (and yet insufficiently expressive) API on DiffusionRequest, including a nonsensical concrete base class.
- Other tools were doing hacky things like passing ":" branch names.
This diff attempts to fix these issues.
- Make the base class abstract (it was concrete ONLY for "/diffusion/").
- Move all URI generation to DiffusionRequest. Make the core static. Add unit tests.
- Delete the 300 copies of URI generation code throughout Diffusion.
- Move all URI parsing to DiffusionRequest. Make the core static. Add unit tests.
- Add an appropriate static initializer for other callers.
- Convert all code calling `newFromAphrontRequestDictionary` outside of Diffusion to the new `newFromDictionary` API.
- Refactor static initializers to be sensibly-sized.
- Refactor derived DiffusionRequest classes to remove duplicated code.
- Properly encode branch names (fixes branches with "/", see <https://github.com/facebook/phabricator/issues/100>).
- Properly encode path names (fixes issues in D1742).
- Properly escape delimiter characters ";" and "$" in path names so files like "$100" are not interpreted as "line 100".
- Fix a couple warnings.
- Fix a couple lint issues.
- Fix a bug where we would not parse filenames with spaces in them correctly in the Git browse query.
- Fix a bug where Git change queries would fail unnecessarily.
- Provide or improve some documentation.
This thing is pretty gigantic but also kind of hard to split up. If it's unreasonably difficult to review, let me know and I can take a stab at it though.
This supplants D1742.
Test Plan:
- Used home, repository, branch, browse, change, history, diff (ajax), lastmodified (ajax) views of Diffusion.
- Used Owners typeaheads and search.
- Used diffusion.getrecentcommitsbypath method.
- Pushed a change to an absurdly-named file on an absurdly-named branch, everything worked properly.
{F9185}
Reviewers: nh, vrana, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1921
Summary: Allow AphrontTableView to render with sort indicators and links in its columns.
Test Plan: Looked at UI example.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley, sandra
Maniphest Tasks: T994
Differential Revision: https://secure.phabricator.com/D1946
Summary:
Allow tasks to be grouped by project. Since this is many-to-many and we're a little deficient on indexes for doing this on the database, we pull all matching tasks and group them in PHP. This shouldn't be a huge issue for any existing installs, though, and we can add keys when we run into one.
- When a task is in multiple projects, it appears under multiple headers.
- When a query has a task filter, those projects are omitted from the grouping (they'd always show everything, which isn't useful). Notably, if you search for "Differential", you can now see "Bugs", "Feature Requests", etc.
Test Plan: Selected "Group by: Project".
Reviewers: btrahan, Josereyes
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T923
Differential Revision: https://secure.phabricator.com/D1953
Summary: Part of a user feature request, see T994.
Test Plan: Looked at data in columns, seemed to line up with reality.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley, sandra
Maniphest Tasks: T994
Differential Revision: https://secure.phabricator.com/D1944
Summary:
- Affects the "Inline Comments" summary table which appears in comments that have attached inlines in the discussion threads in Differential.
- Prepares for inclusion in Diffusion.
- No application changes (minor CSS), just factors code better.
- Simplify/separate CSS.
Test Plan: Looked at on-diff and off-diff comment summaries in Differential, display looked correct.
Reviewers: davidreuss, nh, btrahan
Reviewed By: davidreuss
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1928
Summary:
See <https://github.com/facebook/phabricator/issues/102>. Commit data may not be available for unpared commits, but we'll raise a warning about $commit_data in that case (the UI correctly handles missing $commit_data).
Also some minor cleanup / UI fixes.
Test Plan: Browsed around hg / git repos, including unparsed commits.
Reviewers: btrahan, killermonk
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1961
Summary:
See <https://github.com/facebook/phabricator/issues/102>. Between Feb 1 and Mar 1, the hg released changed the exit code behavior of "hg pull". This broke us mildly (and a bunch of other applications more severely, which is why it was reverted).
Detect the common case of this (english) and don't fail.
Test Plan: @killermonk, can you try applying this? I'll try to do an upgrade to 2.1 and see if I can also do a proper test.
Reviewers: Makinde, btrahan, killermonk
Reviewed By: btrahan
CC: killermonk, aran, epriestley
Differential Revision: https://secure.phabricator.com/D1948
Summary:
- We incorrectly count resolution changes and other noise as opens / closes.
- Show one graph: open bugs over time (red line minus green line). This and its derivative are the values you actually care about. It is difficult to see the derivative with both lines, but easy with one line.
Test Plan: Looked at burnup chart. Saw charty things. Verified resolution changes no longer make the line move.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T923, T982
Differential Revision: https://secure.phabricator.com/D1945
Summary:
Split from D1921.
- DifferentialChangesetParser doesn't have this property declared.
- We weren't providing a markup engine, which caused some warnings.
- This would cause failures if comment caches weren't present. Currently, they always will be though unless someone has wiped them explicitly in the DB.
Test Plan: Viewed a diff with inline comments, didn't get any warnings in the log.
Reviewers: nh, vrana, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1925
Summary:
Split from D1921. We'll explode each line into too many parts currently, if the filename contains spaces.
Also use -z to get \0 newlines.
Test Plan: Browsed a directory containing files with spaces in their names, links etc were correct.
Reviewers: nh, vrana, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1924
Summary:
In D1926 this got converted to use the new InsetView, but we lost the 'id="resources"' on the hidden input which is required by the JS.
Just use phutil_render_tag() to make sure the `id` shows up.
Test Plan: Edited a project without bumping into an exception.
Reviewers: hsb, btrahan
Reviewed By: hsb
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1957
Summary:
If the 'Summary' is not present and not inferred to be empty from a newline after the title with no explicit 'Summary' field, we'll copy all the field values from the revision (including NULL), not overwrite the 'Summary' value from the message (since it's not present) and then write the NULL back to the revision.
Instead, string cast the read from the Revision so we write back empty string in the not-provided case.
Test Plan: Ran "arc diff --create --use-commit-message HEAD" with P336 in HEAD, didn't fail (previously, it failed).
Reviewers: btrahan, 20after4
Reviewed By: 20after4
CC: aran, epriestley
Maniphest Tasks: T1019
Differential Revision: https://secure.phabricator.com/D1956
Summary: This diff is really complicated, only a master programmer like myself could have accomplished it.
Test Plan: derrrrrp
Reviewers: davidreuss, nh, btrahan, jungejason
Reviewed By: jungejason
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1936
Summary: Works in Safari, Firefox, Chrome.
Test Plan: Copied some text, threw up a little in my mouth.
Reviewers: aran, tuomaspelkonen, tomo, rstout, btrahan
Reviewed By: aran
CC: aran, epriestley, ddfisher
Maniphest Tasks: T145, T995
Differential Revision: https://secure.phabricator.com/D244
Summary:
T937 suggests 'inset' could have its own view controller.
It has the following methods:
- setTitle for title
- setRightbutton if you have to place something (preferably a button)
on the right side of the form
- setDescription if you want to describe what it does
- setContent for the main content
- addDivAttributes REALLY not sure about this one but it had to be included
because of a single controller (see owners/controller/edit/PhabricatorOwnersEditController.php:238)
- appendChild works as usual if your form is complex but you still want to remove
->appendChild('<div class..') ->appendChild('</div>');
It might be an overkill so maybe some could be dropped:
- addDivAttributes() and just rewrite how PhabricatorOwnersEditController.php works
- setContent() and use appendChild for the main content?
Test Plan:
- Looked at the controllers in phabricator
- Changed the controller
- Opened the page in another tab
- If something didnd't look the same I fixed it.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1926
Test Plan:
type /a<enter> - should jump to audits
type /f<enter> - should jump to feed
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1823
Summary: Facebook extends this, just unfinal it for now and we can figure out the right long-term fix at our leisure.
Test Plan: Inspection.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1917
Summary:
It is currently not possible to select source code covered by reticle when creating comment.
This diff hides reticle on mouseout from reply area.
Test Plan:
Hover inline comment, verify that reticle is displayed.
Reply, verify that reticle is displayed when mouseover reply, hidden otherwise.
Repeat for create.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1851
Summary: In very large diffs.
Test Plan:
Display very large diff.
Display normal diff.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1918
Summary:
It should be possible to write `D1234` and others without making links from them.
Depends on D1913.
Test Plan:
`D1234`
`<a href="">`
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1915
Summary:
If we can't look up the name for a mailing list, don't put 'Unknown Mailing
List' in the commit message - it will confuse things later down the road.
Surely there is a better way of doing this than checking the name of the
handle for the mailing list.
Test Plan:
called differential.getcommitmessage on a revision that had an invalid mailing
list phid.
Reviewers: epriestley, schrockn, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1910
Summary: I've done the same stupid copy/paste mistake twice in one day :-(.
Test Plan: Display diff with no newline at end of file in new file.
Reviewers: davidreuss, epriestley
Reviewed By: davidreuss
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1911
Summary:
Resolves T989
- users can now disable the '/' keyboard shortcut which focuses the
search box
- users can now disable the jump nav functionality of the search box
Test Plan:
- verified that the '/' keyboard shortcut works with preference enabled
or unset
- verified that '/' no longer has any effect and disappears from
keyboard shortcuts help overlay with preference disabled
- verified that search boxes have jump nav capabilities with jump nav
functionality preference unset or enabled
- verified that search boxes do not jump with jump nav preference
disabled
- verified that the jump nav still works as a jump nav with jump nav
preference disabled
Reviewers: epriestley
Reviewed By: epriestley
CC: simpkins, aran, epriestley, vrana
Maniphest Tasks: T989
Differential Revision: https://secure.phabricator.com/D1902
Summary: Text "No newline at end of file" was sent to highlighter causing error with most languages.
Test Plan: Display diff containing file with no newline at end of file.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1903
Summary:
- Adds "User Projects" filter to Maniphest.
- The filter expands the user(s) specified into a set of projects and issues a query on that project set.
- "View All Triage" button in Tactical Command's Needs Triage panel now points to User Projects-Need Triage filter.
Test Plan: - User Projects filter correctly displays only the tasks in projects the specified users are involved in.
Reviewers: epriestley
Reviewed By: epriestley
CC: keebuhm, ddfisher, allenjohnashton, epriestley, aran
Differential Revision: https://secure.phabricator.com/D1901
Summary:
It looks like there is really this text written e.g. at https://secure.phabricator.com/D1896#0a6a1957
I am not sure that it is the only place which needs to be fixed.
Test Plan: Display diff with no newline at end of file in Differential.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1900
Summary:
- Add inline comments to Audits, like Differential.
- Creates new storage for the comments in the Audits database.
- Creates a new PhabricatorAuditInlineComment class, similar to DifferentialInlineComment.
- Defines an Interface which Differential and Audit comments conform to.
- Makes consumers of DifferentialInlineComments consume objects which implement that interface instead.
- Adds save
NOTE: Some features are still missing! Wanted to cut this off before it got crazy:
- Inline comments aren't shown in the main comment list.
- Inline comments aren't shown in the emails.
- Inline comments aren't previewed.
I'll followup with those but this was getting pretty big.
@vrana, does the SQL change look correct?
Test Plan:
- Created, edited, deleted, replied to, reloaded and saved inline comments in Diffusion, on the left and right side of diffs.
- Created, edited, deleted, replied to, reloaded and saved inline comments in Differentila, on the left and right side of primary and diff-versus-diff diffs.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1898
Summary:
We give you a pretty bad error right now if your server doesn't have, say, png support, saying "only png is supportd loololloo".
Instead, show you which formats are supported in the error messsage, and tell you upfront.
Test Plan: Tried to upload supported and unsupported images, got appropriate errors and supported format text.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T981
Differential Revision: https://secure.phabricator.com/D1894
Summary:
It is misplaced from the beginning but it got worse after some CSS tweaks.
I was also thinking about using the same DropdownMenu as in Differential but I don't feel strongly about it to do it myself.
Test Plan:
Display file in Diffusion.
Repeat with disabled Editor.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1892
Summary: I missed this URI when grepping for old links.
Test Plan: Clicked the link, didn't 404.
Reviewers: davidreuss, btrahan
Reviewed By: davidreuss
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1895
Summary: Last of the big final patches. Left a few debatable classes (12 out of about 400) that I'll deal with individually eventually.
Test Plan: Ran testEverythingImplemented.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T795
Differential Revision: https://secure.phabricator.com/D1881
Summary: svn cat returns a non-zero exit status when trying to cat a directory.
Test Plan: Browsed diffusion in my sandbox
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1882
Summary: No big surprises here, delted the unused "DarkConsole" class.
Test Plan: Ran 'testEverythingImplemented' to verify I wasn't finalizing anything we extend.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T795
Differential Revision: https://secure.phabricator.com/D1876
Summary:
- Order audits by id desc so they tend to be in descending time order, like other content.
- In audit tables without commit context (audit tool, home page) show commit descriptions.
- Correctly hide pagers on "active" audit filter.
- Make pagers work correctly on commit / audit views.
Test Plan: Looked at homepage, audit, owners, differential. Paginated relevant interfaces.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1875
Summary: Remove these from typeaheads, since "archive" basically means "delete".
Test Plan: Tried to typeahead an archived project.
Reviewers: btrahan, skrul
Reviewed By: skrul
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1872
Summary:
- Show a tip in the margin about what coverage colors mean.
- Highlight the line when mousing over coverage.
- Randomly change the colors to different colors.
- Fix a bug with "show more" that I introduced with the other coverage diff (oops!)
Test Plan: Moused over coverage things.
Reviewers: tuomaspelkonen, btrahan
Reviewed By: tuomaspelkonen
CC: aran, epriestley
Maniphest Tasks: T965
Differential Revision: https://secure.phabricator.com/D1874
Summary: There are a few things we can improve with tooltips.
Test Plan: Moused over all the stuff on the test page.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T965
Differential Revision: https://secure.phabricator.com/D1870
Summary:
Currently, we mark some lines that otherwise count as "unchanged" to be "changed" when they have internal whitespace changes.
However, we've already excluded them from the intra-line diff algorithm. Unset the flag so they can get intra-line diffed.
Test Plan: Viewed a change with internal whitespace changes, internal whitespace changes were intraline diffed.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T970
Differential Revision: https://secure.phabricator.com/D1871
Summary:
We used to have "ignore all", but it became "ignore most". It does not ignore in-line whitespace changes or whitespace changes in language where whitespace is marked semantic.
Add an explicit "ignore all" option.
Test Plan: Used "ignore all" to view a change with heuristically-detected semantic whitespace, the change was ignored.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T970
Differential Revision: https://secure.phabricator.com/D1865
Summary:
svn 1.7 changed their xml format slightly, they now have a
##<?xml version="1.0" encoding="UTF-8"?>## tag instead of
##<?xml version="1.0"?>##. This relaxes matching this tag.
Test Plan: ./scripts/repository/reparse.php rE521979 --change
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1866
Summary: We've hit a couple of these in the wild, raise better error messages when the local repo is toast / broken / nonsense.
Test Plan: Broke my local repo in all of the different ways we test for, verified I got an error message in each case.
Reviewers: btrahan, abirchall
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T964, T924
Differential Revision: https://secure.phabricator.com/D1855
Summary:
These are all unambiguously unextensible. Issues I hit:
- Maniphest Change/Diff controllers, just consolidated them.
- Some search controllers incorrectly extend from "Search" but should extend from "SearchBase". This has no runtime effects.
- D1836 introduced a closure, which we don't handle correctly (somewhat on purpose; we target PHP 5.2). See T962.
Test Plan: Ran "testEverythingImplemented" unit test to identify classes extending from `final` classes. Resolved issues.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T795
Differential Revision: https://secure.phabricator.com/D1843
Summary: We now support an optional coverage parameter.
Test Plan: ??? iiam
Reviewers: tuomaspelkonen, btrahan
Reviewed By: tuomaspelkonen
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1850
Summary:
Adds a macro handler that spams your channel with macros. Config is:
- macro.size: scale macros to this size before rasterizing
- macro.sleep: sleep this many seconds between lines (evade flood protection)
Test Plan: derpderp
Reviewers: kdeggelman, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1838
Summary:
I'm on a host where I have the PhabricatorRepositoryPullLocalDaemons
tracking a remote repo. In my case, these end up as local git repos in
/var/repo/$name.
I'm working on another daemon that is going to automatically make
changes and commit them back upstream. I figured it would be best to do
this in a new local repo. I'll put these in /var/repo-clones/$name.
It's nice to use the exec*() functions in PhabricatorRepository, so the
approach I thought of was to load the PhabricatorRepository object from
the database, then change its localPath to point at the
/var/repo-clones/$name directory instead.
I didn't really want to change the local-path detail with setDetail(),
as that risks committing the change upstream. It's nice to use the
repo's execLocalCommand() methods though, hence wanting to change the
local path.
Test Plan: None yet.
Reviewers: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1821
Summary: We'll keep deleted branches around right now because Git's behavior is not to remove them without --prune.
Test Plan: Ran "git fetch --all --prune" to make sure it at least ostensibly works.
Reviewers: btrahan, 20after4
Reviewed By: 20after4
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1833
Summary: New implicit fallthrough linter detected a few issues; none of these have behavioral impacts but they can clearly be tightened up. See D1824.
Test Plan: Lint; inspection.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1825
Summary:
More complicated updates deserves title and explanation.
This diff uses only the first line from comment in diff's description.
It also removes the truncating at 80 chars which looks as an error.
Test Plan:
var_dump(preg_replace('/\n.*/s', '', "abc"));
var_dump(preg_replace('/\n.*/s', '', "abc\ndef"));
var_dump(preg_replace('/\n.*/s', '', "abc\ndef"));
var_dump(preg_replace('/\n.*/s', '', "abc\ndef\nghi"));
var_dump(preg_replace('/\n.*/s', '', "\nabc")); // empty string
Display revision with 255 chars description in update history - it looks OK.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1812
Summary: We currently parse these as directory changes and discard them. Instead, parse them as a new "SUBMODULE" type of change.
Test Plan:
- Reparsed a commit which changes submodules and verified it parses correctly.
- Reparsed a commit which adds submodules and verified it parses correctly.
Reviewers: btrahan, kdeggelman
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1815
Summary:
They end up in "CCs:" fields where they can't be parsed.
Not bothering to migrate since I think only Dropbox has hit this.
Also improved another error condition's handling.
Test Plan: Tried to save a mailing list with spaces and commas in the name.
Reviewers: btrahan, Makinde
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T947
Differential Revision: https://secure.phabricator.com/D1813
Summary:
Currently, we sort all results alphabetically. This isn't ideal. Instead, sort them like this:
- If the viewing user appears in the list, always sort them first. This is common in a lot of contexts and some "Ben Evans" guy is sorting first on secure.phabricator.com and causing me no end of aggravation.
- If the tokens match a "priority" component (e.g., username), sort that before results which do not have a "priority" match.
- Within a group (self, priority, everything else) sort tokens alphabetically.
NOTE: I need to go add setUser() to all the tokenizers to make the "self" rule work, but that's trivial so I figured I'd get this out first.
Test Plan:
https://secure.phabricator.com/file/data/4s2a72l5hhyyqqkq4bnd/PHID-FILE-x2r6ubk7s7dz54kxmtwx/Screen_Shot_2012-03-07_at_9.18.03_AM.png
Previously, "aaaaaepriestley" (first alphabetic match) would sort before "epriestley" (the viewing user). Now, "epriestley" sorts first because that is the viewer.
https://secure.phabricator.com/file/data/rmnxgnafz42f23fsjwui/PHID-FILE-yrnn55jl3ysbntldq3af/Screen_Shot_2012-03-07_at_9.18.09_AM.png
Previously, "aaaagopher" (first alphabetic match) would sort before "banana" (the "priority" match). Now, "banana" sorts first because it priority matches on username.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T946
Differential Revision: https://secure.phabricator.com/D1807
Summary:
- Enforce proper workflow rules.
- Fix a derp-bug with patches.
Test Plan:
- Tried to mark a revision I didn't own.
- Tried to mark a revision already marked committed.
- Tried to mark a revision otherwise not accepted.
- Verified daemon can override workflow rules and mark from arbitrary states.
Reviewers: btrahan, Makinde
Reviewed By: Makinde
CC: aran, epriestley
Maniphest Tasks: T948
Differential Revision: https://secure.phabricator.com/D1809
Summary: When a revision is accepted, allow users to manually mark it committed if there's a daemon/tracking problem. This shouldn't happen in most installs but we have less-robust support for Mercurial and some installs may not be fully configured.
Test Plan: Marked a revision committed.
Reviewers: btrahan, Makinde
Reviewed By: Makinde
CC: aran, epriestley
Maniphest Tasks: T948
Differential Revision: https://secure.phabricator.com/D1808
Summary: Provide a reasonable JS API for the Aphlict client. Provide an example behavior to invoke it.
Test Plan:
Ran "aphlict_server.js" with:
$ sudo node aphlict_server.js
Loaded /aphlict/. Opened console. Got "hello" from the server every second.
Got reasonable errors with the server not present ("Security exception", but this is because it can't connect to port 843 to access the policy server).
Reviewers: ddfisher, keebuhm, allenjohnashton, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T944
Differential Revision: https://secure.phabricator.com/D1800
Summary:
- Use a join to effect this query.
- Fixes a bug where packages with no commits would raise an exception because of the awkward query construction.
- Fixes a bug on audit views.
Test Plan:
- Viewed a package with no commits.
- Altered audit filters.
Reviewers: btrahan, 20after4
Reviewed By: 20after4
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1801
Summary: Conduit access for open audits.
Test Plan: Used test console to run some queries.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1792
Summary:
Adds an optional "Auditors" field (like "Reviewers") to commit messages which gives installs a zero-config method for making audit requests.
This field does not appear on templates unless set, and is mostly ignored (but validated and preserved) by Differential.
It is then parsed by the daemons if present, and audit requests are pushed to valid users.
Test Plan: Made an "Auditors" commit and verified it was retained with "arc amend --show". Pushed it and verified the audit was triggered.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904, T880
Differential Revision: https://secure.phabricator.com/D1793
Summary: We render these in a realtively unreadable way right now; allow customization and provide reasonable defaults.
Test Plan: Looked at some tasks with custom fields on them.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T930
Differential Revision: https://secure.phabricator.com/D1790
Summary:
Caption Submit is quite confusing because it doesn't actually make the data visible to other users.
Especially when comment confirm button is also called Submit in serious mode.
Test Plan: Add inline comment.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1796
Summary:
- all search boxes are now jump navs (old functionality retained if none
of the jump nav patterns match)
- added global keyboard shortcut '/' to focus the search box in the upper
right
Test Plan:
- pressed '/' and noticed the search box gains keyboard focus
- triggered jump nav functionality from search box and saw it worked
- did a search which did not match a jump nav pattern and saw it worked
(and searched in the selected context)
NOTE: The search box on the /search/ page is also changed to have jump
nav functionality. Old functionality is not impared. Still, this may not
be desirable.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1794
Summary:
- Remove "0.5%" padding which makes Safari flip out and render every row differently sometimes.
- Remove list padding from ManiphestTaskListView, put it in the controller composition instead.
Test Plan: Viewed all places where task lists appear.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1788
Summary:
Filter Revisions button currently resets Status and Order fields.
I've rewritten it to GET form because it doesn't perform any action.
It fixed the problem along the way.
Test Plan:
/differential/filter/revisions/
Status: Open.
Filter Revisions.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1771
Summary:
- Move table to Repository, since we have no Owners joins in the application anymore but would like to do a Repository join.
- Rename "packagePHID" to "auditorPHID", since this column may contain package, project, or user PHIDs.
Test Plan:
- Browsed Owners, Audit, and Differential interfaces to the Audit tool.
- Made comments and state changes.
- Ran "reparse.php --herald --owners" on several commits.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley, nh, vrana
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1787
Summary:
Show the retry count in the meta mta view (in addition to the list of
messages) - I find this info useful when I'm trying to debug what's going on
with mail failures.
Task ID: #
Blame Rev:
Test Plan:
loaded /mail/view/NNNNN/ and saw the retry count
Revert Plan:
Tags:
Reviewers: epriestley, btrahan, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1782
Summary:
Added a query option of status for the MetaMTA list controller. There currently
isn't a ui for accessing this.
Task ID: #
Blame Rev:
Test Plan:
loaded /mail/, /mail/?status=queued, /mail?phid=PHID...&status=...
each request returned a sane list of data
Revert Plan:
Tags:
Reviewers: epriestley, btrahan, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1784
Summary:
Displaying reviewer who was by coincidence listed first is quite confusing, especially for committed revisions.
This displays the one who really reviewed the revision if available.
This implementation is pretty bad from performance perspective - O(N) queries to retrieve all comments.
The page load still feels quite fast.
Test Plan: /differential/filter/revisions/
Reviewers: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1772
Summary:
- Owners has "by user" commit views, but these are supplanted by the Audit views. Just nuke them.
- Owners has "by package" commit views; consolidate these onto the package detail pages and link into Audit for full details.
Test Plan: Browsed all the Owners interfaces, clicked "View All ... Commits" buttons.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1764
Summary:
- Users may elect to receive an initial notification about a commit; allow it to be replied to in order to interact with the object.
- Share thread headers between emails.
- Add the "REPLY HANDLER ACTIONS" section to both emails.
Test Plan:
- Used "reparse.php --herald" to trigger herald emails, verified reply-to and email body.
- Made audit comments, verified body.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1762
Summary:
We already generate patches, but currently attach them. Allow them to be inlined instead (optionally, up to a certain size).
Also allow selection between unified and git patches.
Test Plan: Set these options in my local config, sent out a diff.
Reviewers: btrahan, Makinde
Reviewed By: Makinde
CC: aran, epriestley
Maniphest Tasks: T874
Differential Revision: https://secure.phabricator.com/D1759
Summary:
I use a smart editor which wraps words by itself so that I don't need them to be
wrapped by actual newlines.
Curent state disallows me adding or removing words later without uglying the
formatting.
Also the wordwrapped message looks ugly in Phabricator.
I am not sure how the commit message would look like on other places (such as
GitHub) but all reasonable tools should be able to wrap the text by itself.
Test Plan: arc amend --show # on a diff with long lines
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1755
Summary:
This does two things:
- don't fill '-' for columns where there are data from previous week
- completele hide columns if there are now new data so that noobs don't have a
55 years of history
Test Plan:
View my commits stats.
View my requested changes stats.
View epriestley FB stats.
Reviewers: vii, epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1750
Summary:
This will allow sending mail to be done by task workers. See T750.
Task ID: #
Blame Rev:
Test Plan:
- started taskmaster daemon in test env
- used "send new test message" feature in MetMTA (with send now unchecked)
- confirmed receipt of 1 email
- repeated 2 & 3 with send now checked
Revert Plan:
Tags:
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T388, T750
Differential Revision: https://secure.phabricator.com/D1723
Summary: add a big ole HELP tab and make "scope" link to the specific
sub-section about scope
Test Plan:
read my doc a few times, it basically english
verified links looked correct and should work right once this is all in
production
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T910
Differential Revision: https://secure.phabricator.com/D1752
Summary: Sandra had trouble opening the Spreadsheet_Excel_Writer ones so use
PHPExcel, which is way better, just a bit more complicated.
Test Plan:
- Generated modern Excel 2007 .xslx sheets.
- Opened them in Excel in Office Mac 2011.
- Opened them in Apple Numbers from the app store.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, epriestley
Maniphest Tasks: T911
Differential Revision: https://secure.phabricator.com/D1744
Summary:
- We have a lot of headers now; document them.
- Remove the one random protip from like 3 years ago from all Differential
mail.
Test Plan: generated; read documentation
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T931
Differential Revision: https://secure.phabricator.com/D1748
Summary:
- Update the Javelin submodule to pick up recent fixes (like D1749).
- Update the package definitions do do a slightly better job of packaging
resources.
Test Plan:
Up and down work in tokenizers now. Pages load slightly fewer
resources.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T927
Differential Revision: https://secure.phabricator.com/D1751
Summary:
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
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: 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
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
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:
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:
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:
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 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:
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:
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: 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
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: 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:
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: 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:
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:
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:
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:
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:
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: 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:
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: 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: 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:
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: ..."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:
- 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:
- 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:
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
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:
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: 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: 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
