Summary:
I want to implement a `{P123}` rule to embed pastes, but we need viewers everywhere before it will work with privacy.
This is not exhaustive; many Remarkup callsites haven't been converted to `PhabricatorMarkupInterface` yet.
Test Plan: Looked at Maniphest, Differential, Diffusion, Phriction; added markup, made edits and hit previews.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3428
Summary:
We have lots of info about unit tests.
This allows linking them from Unit field.
Test Plan: Monkey patched `$test['link']`, clicked on it.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3434
Summary:
This does a few things:
- Allows you to flag pastes. This is straightforward.
- Allows Applications to register event listeners.
- Makes object action lists emit a 'didrenderactions' event, so other applications can add more actions. The Flags application injects its action in this way. This should generally make it much easier to add actions to objects when we add new applications, with less code duplication and better modularity. We have a really hacky version of this in Differential that I want to get rid of in lieu of this more general approach. I'm going to make object lists do the same thing, so any application can jump in and add stuff.
Test Plan: Flagged and unflagged pastes. Viewed home page, differential, flags list.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3377
Summary: This has been deprecated for quite a while and I'm pretty sure there are no callsites in the wild since this tool doesn't get much use outside of Facebook.
Test Plan: grep
Reviewers: vrana, btrahan, meitros
Reviewed By: vrana
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D3195
Summary: See T1677. I think wanting bots to be able to post comments without sending email is a pretty reasonable use case. Eventually we should probably support this more broadly and maybe protect it with permissions (normal users maybe shouldn't be able to do this?) but we can wait for use cases.
Test Plan: Made comments with and without "silent". Verified that the non-silent comment sent email, and the silent comment did not.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1677
Differential Revision: https://secure.phabricator.com/D3341
Summary: See D3291.
Test Plan: Ran `phpast.getast` via API.
Reviewers: alanh
Reviewed By: alanh
CC: aran
Differential Revision: https://secure.phabricator.com/D3292
Summary:
Create `phpast.{version,getast}` methods for calling xhpast
with `--version` and with source code as input, respectively.
Test Plan:
Run `arc call-conduit` a bunch of times; delete xhpast; run
it a bunch more times.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1534
Differential Revision: https://secure.phabricator.com/D3289
Summary:
- Add getHelpURI() to PhabricatorApplication for application user guides.
- Add a new "help" icon menu item and skeletal Diviner application.
- Move help tabs to Applications where they exist, document the other ones that don't exist yet.
- Grep for all tab-related stuff and delete it.
Test Plan: Clicked "help" for some apps. Clicked around randomly in a bunch of other apps.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3267
Summary: 'cuz we don't need it and it's lame complexity for API clients of all kinds. Rip the band-aid off now.
Test Plan: used conduit console and verified no more shield. also did some JS stuff around the suite to verify I didn't kill JS
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T891
Differential Revision: https://secure.phabricator.com/D3265
Summary: We managed to move enough Owners stuff aside to make this reasonable; make projects implement the policy interface and projectquery use cursor-based paging.
Test Plan:
- Grepped for ProjectQuery callsites.
- Created an audit comment.
- Used `project.query` to query projects.
- Loaded homepage.
- Viewed Maniphest task list, grouped by project.
- Viewed project list.
- Created / edited project.
- Browsed Owners.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D3200
Summary:
I want to:
- move the membership storage to edges
- remove the concepts of "roles" (which are decorative text only) and "owners" (which will be replaced with policy-based controls)
This moves us a step closer to that by reducing the use of ProjectAffiliation outside of the class.
Test Plan: Loaded project profile. Called `project.query`. Joined and left a project.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D3182
Summary: This is clearer and more consistent with other Query classes.
Test Plan: Used home page, conduit api, project list, other interfaces.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D3179
Summary:
- Put the code to generate informational dicts about flags into the
base class.
- Update flag.delete to accept an object PHID in order to delete the
flag on that object, since currently the model is that each object
may have at most one flag, and each flag has exactly one object,
although the former is not enforced.
- Add flag.edit, which creates or updates a flag, optionally with the
given color and note.
Test Plan:
Spend endless hours repeatedly running arc call-conduit and
arc flag.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3141
Summary: as title
Test Plan: tested without params. Tested with single known path
Reviewers: epriestley, vrana, nh
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3139
Summary:
To make it easier to monitor daemons, let's store their current state
(running, died, exited, or unknown) to the db. The purpose of this is to
provide more information on the daemon console about the status of daemons,
especially when they are running on multiple machines. This is mostly backend
work, with only a few frontend changes. (It is also dependent on a change
to libphutil.)
These changes will make dead or stuck daemons more obvious, and will allow
more work on the frontend to hide daemons (and logs) that have exited cleanly,
i.e. ones we don't care about any more.
Test Plan:
- run db migration, check in db that all daemons were marked as exited
- start up a daemon, check in db that it is marked as running
- open web interface, check that daemon is listed as running
- after daemon has been running for a little bit, check in db that dateModified
is being updated (indicating daemon is properly sending heartbeat)
- kill -9 daemon (but don't run bin/phd yet), and check that db still shows it
as running
- edit daemon db entry to show it as being on a different host, and backdate
dateModified field by 3 minutes, and check the web ui to show that the status
is unknown.
- change db entry to have proper host, check in web ui that daemon status is
displayed as dead. Check db to see that the status was saved.
- run bin/phd stop, and see that the formerly dead daemon is now exited.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3126
Summary: ...also swapped "status" and "order" so "status" is first, as in my testing it was sub-optimal to specifiy status (more of "what i want") after order ("how I want it")
Test Plan: ran various queries on my test instance via conduit console and the results all seem correct
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1381
Differential Revision: https://secure.phabricator.com/D3028
Summary:
- Share code between `createinline` and `getcomments`
- Make them both extend the base class.
- Sync up the parameters (this is more or less nonbreaking since the call is ~6 hours old).
Test Plan: Created and fetched inlines via the API.
Reviewers: alanh, vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1500
Differential Revision: https://secure.phabricator.com/D2988
Summary: a silly thing because I was bored
Test Plan: and I said "arc call-conduit", and there were comments
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1500
Differential Revision: https://secure.phabricator.com/D2971
Test Plan:
Created diff with a postponed linter and the lint status set to such
via arcanist. Verified lint status showed as postponed in diff view.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1332
Differential Revision: https://secure.phabricator.com/D2932
Summary:
Brazenly copied from differential.close. Ulterior motive
is to be able to automate discarding of useless commits from
arcanist testing.
Test Plan:
Called method in phabricator sandbox. Revisions were
successfully abandoned.
Reviewers: epriestley
Reviewed By: epriestley
CC: nh, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2928
Summary: Simplify FeedQuery by making it extend from PhabricatorIDPagedPolicyQuery
Test Plan: Looked at feed on home, projects, user profile, and called `feed.query`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2905
Summary:
- Allow clients to query for specific closed statuses (invalid, resolved, wontfix, etc), not just "closed" tasks.
- Rename this method to maniphest.query and deprecate maniphest.find as an alias to maniphest.query, for API consistency.
Test Plan: Ran queries for all tasks, "wontfix" tasks, closed tasks.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2887
Summary:
This adds:
1) A new "arc:lint-postponed" diff property which stores a list of
lint names that are postponed and a finishpostponedlinters conduit
method which removes linters from this list. Postponed linters are
shown in the lint details.
2) A updatelintmessages conduit message, which adds additional lint
messages to the "arc:lint" diff property.
In combination, this provides very basic support for running
asynchronous static analysis tools. When the diff is being created,
a list of asynchronous static analysis runs can be added to the
diff's postponed linters list. As these postponed linters finish
up, then can report new lint messages back to the diff then mark
themselves as complete.
The client is currently responsible for filtering the lint messages
by things like affected lines and files.
Test Plan:
Used conduit call API to add lint messages and remove postponed
linters from a test diff.
Reviewers: epriestley, vrana, nh, jungejason
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1332
Differential Revision: https://secure.phabricator.com/D2792
Summary:
These are currently not available via Conduit.
Also fix a bug where bad JSON input triggers an error about undefined `$metadata`.
Test Plan: Ran 'repository.create' with and without a description and with and without autoclose. Verified the created repositories had the requested attributes.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2881
Summary: See T693. To do "arc branch" performantly in immutable history repositories, we need to be able to do a single query to identify revisions related by hash. Return hash information to enable this.
Test Plan: Ran `differential.query`.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T693
Differential Revision: https://secure.phabricator.com/D2859
Summary:
See D2855 for usage.
This has a drawback that inlines without a comment (synthetic comments) are not attached anywhere.
I don't like adding more and more methods so I've chosen this solution.
Plus comments and inline comments are often useful together.
Test Plan: Called the method on a revision with inline comments.
Reviewers: epriestley
Reviewed By: epriestley
CC: vii, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2844
Summary:
"differential.creatediff" requires a mostly-parsed diff, but there's no reason we can't make DifferentialDiffs out of raw diffs.
This mainly serves to lower the adoption barrier if getting "arc" distributed is too much of a hassle.
Test Plan: Made a diff out of a raw block of diff text.
Reviewers: ffx, vrana, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2751
Summary:
Sorry this took so long, had a bunch of stuff going on today.
Separate the actual core part of making conduit calls from the controller, so the application can make conduit calls without needing to invoke HTTP or redo auth. Generally, this lets us build more parts of the application on top of Conduit, as appropriate.
This diff can be simplified, but I wanted to unblock you guys first. I'll followup with a cleanup patch once I have a chance.
Test Plan: Ran unit tests, ran calls from the conduit API console, and ran calls over arc.
Reviewers: nodren, 20after4, btrahan, vrana
Reviewed By: 20after4
CC: aran, svemir
Maniphest Tasks: T945
Differential Revision: https://secure.phabricator.com/D2718
Summary: See discussion in rP2f138d0501887fd0aca0f8536176f092880f662c.
Test Plan: Ran `user.query` on verified and unverified users.
Reviewers: csilvers, vrana
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2768
Test Plan:
Verified that the method retuns the same output.
Verified the number of SQL queries.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2764
Summary: Implemented it how it was suggested in ticket comments
Test Plan: create a revision in a branch, push that branch up, verify it's visible in diffusion and also that revision is not closed, then merge and push to master, verify that revision closed
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, 20after4
Maniphest Tasks: T1210
Differential Revision: https://secure.phabricator.com/D2706
Summary:
- `kill_init.php` said "Moving 1000 files" - I hope that this is not some limit in `FileFinder`.
- [src/infrastructure/celerity] `git mv utils.php map.php; git mv api/utils.php api.php`
- Comment `phutil_libraries` in `.arcconfig` and run `arc liberate`.
NOTE: `arc diff` timed out so I'm pushing it without review.
Test Plan:
/D1234
Browsed around, especially in `applications/repository/worker/commitchangeparser` and `applications/` in general.
Auditors: epriestley
Maniphest Tasks: T1103
Summary:
Allow allowed email addresses to be restricted to certain domains. This implies email must be verified.
This probably isn't QUITE ready for prime-time without a few other tweaks (better administrative tools, notably) but we're nearly there.
Test Plan:
- With no restrictions:
- Registered with OAuth
- Created an account with accountadmin
- Added an email
- With restrictions:
- Tried to OAuth register with a restricted address, was prompted to provide a valid one.
- Tried to OAuth register with a valid address, worked fine.
- Tried to accountadmin a restricted address, got blocked.
- Tried to accountadmin a valid address, worked fine.
- Tried to add a restricted address, blocked.
- Tried to add a valid address, worked fine.
- Created a user with People with an invalid address, got blocked.
- Created a user with People with a valid address, worked fine.
Reviewers: btrahan, csilvers
Reviewed By: csilvers
CC: aran, joe, csilvers
Maniphest Tasks: T1184
Differential Revision: https://secure.phabricator.com/D2581
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:
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:
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:
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:
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: 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'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
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: 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:
- 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:
- 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: 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:
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:
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: "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
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:
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:
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:
- 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:
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: 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:
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:
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:
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: 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: 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:
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: 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:
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:
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
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: 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
