1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-15 19:32:40 +01:00
Commit graph

106 commits

Author SHA1 Message Date
epriestley
15561a27c3 When a conduit method requires a string constant, call it "string-const" not "enum"
Summary: Ref T5058. The use of "enum" is confusing; we mean "choose one of these specific string constants". Make this more clear.

Test Plan: Viewed each call from the web UI.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5058

Differential Revision: https://secure.phabricator.com/D9127
2014-05-14 21:59:03 -07:00
epriestley
997c8591b2 Add 'repositoryPHID' to 'differential.createrawdiff'
Summary:
See <https://github.com/facebook/phabricator/issues/596>

Broadly, Facebook would like to bring Pull Requests from GitHub into Phabricator.

In the long term we can do this properly via Doorkeeper/Nuance, but that's probably a ways off. This seems like a reasonable low-budget compromise for now.

I'm a little hesitant to add a ton of parameters to this call, but `repositoryPHID` seems pretty reasonable, and is notable because it also controls default policies.

Test Plan:
  - Created a diff with no repositoryPHID.
  - Created a diff with a repositoryPHID.
    - Verified it carried over when the diff was used to create a revision.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: ptarjan, jamesgpearce, epriestley

Differential Revision: https://secure.phabricator.com/D9023
2014-05-09 18:06:41 -07:00
epriestley
27d426e3fe Allow Conduit console to be browsed by logged-out users
Summary:
Ref T4830. A few methods, like `conduit.ping`, are callable without authentication, so this even has some use cases. Also:

  - Make some Differential stuff a little more consistent.
  - Use slightly more modern rendering.
  - Deprecate the status-oriented `user` calls; these will be replaced by Calendar methods.

Test Plan: Browsed console as logged out / logged in users.

Reviewers: chad, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4830

Differential Revision: https://secure.phabricator.com/D8826
2014-04-21 15:32:48 -07:00
epriestley
62973e0f53 Add activeDiffPHID to differential.query
Summary: Ref T4809. This saves us a few round trips to find a Buildable, and generally makes the notion of "active" more explicit (i.e., not just the diff with the largest ID). In the future, we may let you revert to previous diffs, which would make the "largest number" rule not always correct.

Test Plan: Ran `differential.query`, got sensible results.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4809

Differential Revision: https://secure.phabricator.com/D8800
2014-04-17 16:01:27 -07:00
epriestley
03c6bf0d09 Make Herald less ambitious about resolving repositories for revisions
Summary:
Fixes T4636. If a user manually deletes a "repository" setting from a revision, Herald attempts to resolve it. Instead, Herald should now just trust Differential. Generally, the new logic is:

  - When diffs are created, figure out repository information.
  - When revisions are updated, copy info from diffs.
  - Everywhere else, just trust the revision field.

Test Plan:
  - Created revisions.
  - Used Herald to dry-run revisions before and after a manual edit to remove the repository setting.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4636

Differential Revision: https://secure.phabricator.com/D8576
2014-03-21 14:39:56 -07:00
epriestley
e068b04b5c Silence a warning about unknown fields in 'getcommitmessage'
Summary: Similar to D8491, some of my new exceptions are a bit too aggressive. See IRC. This one's hitting an edit workflow with 'revisionID' onboard somehow.

Test Plan: Not entirely sure how to hit this, but it won't throw anymore.

Reviewers: btrahan, dctrwatson, zeeg

Reviewed By: zeeg

Subscribers: zeeg, aran, epriestley

Differential Revision: https://secure.phabricator.com/D8514
2014-03-12 17:24:58 -07:00
epriestley
4173367f87 Add missing needReviewerAuthority() for differential.createcomment
Summary: For some actions (like accept) we need to load reviewer authority so we can figure out if the actor can act on behalf of project reviewers, etc.

Test Plan: Will make @dctrwatson do it.

Reviewers: dctrwatson, btrahan

Reviewed By: btrahan

Subscribers: chad, aran, dctrwatson, epriestley

Differential Revision: https://secure.phabricator.com/D8505
2014-03-12 15:24:54 -07:00
Bob Trahan
46cf263e9d Fix a bug for updating diffs
Summary: Hit this issue in D8485. I think reviewedByPHID changes should appear in application transactions.

Test Plan: Would like to deploy this and try updating D8485 again.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: aran, epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8491
2014-03-11 15:36:03 -07:00
epriestley
592591e715 Clean up various pieces of dead/obsolete Differential code
Summary:
Ref T2222.

  - Removes `DifferentialTasksAttacher`, which has had no callsites for a very long time.
  - Moves `differential.getrevisioncomments` off `DifferentialCommentQuery`.
  - Moves Releeph churn field off `DifferentialCommentQuery`.
  - Removes dead code in `DifferentialRevisionViewController`.
  - Removes `DifferentialException` (no references).
  - Removes `DifferentialRevision->loadComments()` (no callsites).
  - Removes `DifferentialRevision->loadReviewedBy()` (all callsites updated).
  - Removes `DifferentialCommentQuery` (all callsites updated).

Test Plan: Mostly a lot of `grep`.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8476
2014-03-11 13:02:19 -07:00
epriestley
48059265f3 Use CustomFields to power Conduit auxiliary dictionaries
Summary: Ref T2222. Moves this Conduit stuff over.

Test Plan: Made Conduit calls, saw data in results.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8469
2014-03-11 13:02:09 -07:00
epriestley
a5fbe921b7 Use CustomFields in differential.createrevision
Summary:
Ref T2222. Ref T3886. Medium term goal is to remove `DifferentialRevisionEditor`.

This temporarily reduces a couple of pieces of functionality unique to the RevisionEditor, but I'm going to go clean those up in the next couple diffs.

Test Plan: Used `arc diff --create` to create several revisions with different data.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3886, T2222

Differential Revision: https://secure.phabricator.com/D8452
2014-03-11 13:01:59 -07:00
epriestley
d8968755e9 Use CustomField for differential.updaterevision
Summary: Ref T2222. Ref T3794. Medium term goal is to remove `DifferentialRevisionEditor`. This removes one of two callsites.

Test Plan: Used `arc diff --edit` to repeatedly update a revision, making changes to various fields.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3794, T2222

Differential Revision: https://secure.phabricator.com/D8451
2014-03-11 13:01:58 -07:00
epriestley
ae3c1f7819 Perform commit message parsing and construction with new CustomFields
Summary: Ref T2222. Ref T3886. Converts parsing and construction of commit messages to be driven by CustomField.

Test Plan:
This is a huge, messy change. I've made an effort to test it exhasutively, but suspect I probably missed a few behaviors. Roughly:

  - Enumerted all current fields (fields implementing `shouldAppearOnCommitMessage()`) and tried to test them one by one.
  - Used `arc diff --edit` repeatedly to manipulate each field (this workflow hits both the parse and construct steps).
  - Used `arc amend --show` to examine construct output (this does not activate the "edit" mode).

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3886, T2222

Differential Revision: https://secure.phabricator.com/D8449
2014-03-11 13:01:55 -07:00
epriestley
aff34077c5 Move Differential commit message parsing to a separate, tested class
Summary: Ref T2222. We have a hunk of logic that purely does text parsing here; separate it and get coverage on it.

Test Plan:
  - Ran new unit tests.
  - Used `differential.parsecommitmessage`.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8444
2014-03-07 17:44:35 -08:00
epriestley
1c51ed5940 Use TransactionEditor in differential.createcomment
Summary: Ref T2222. Update this callsite; pretty straightforward.

Test Plan: Used Conduit to take actions and saw their effects in Differential.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8442
2014-03-07 17:44:10 -08:00
epriestley
75514cc0f7 Update differential.close to use DifferentialTransactionEditor
Summary: Ref T2222. Straightforward update to new stuff.

Test Plan:
  - Tried to close an uncloseable revision.
  - Closed a closeable revision.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8425
2014-03-07 08:09:59 -08:00
epriestley
3dc9afa28d Remove differential.markcommitted
Summary: Ref T2222. Primary goal is to remove this callsite for `DifferentialCommentEditor`, but rather than updating it I'm just nuking this method since it's been deprecated for more than a year (more than two years?)

Test Plan: Reloaded Conduit method list.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8424
2014-03-07 08:09:33 -08:00
epriestley
0f0673b9e5 Remove "dateCommitted" field from DifferentialRevision
Summary: Ref T2222. This is obsolete and no longer used. We could deduce it from transactions or commits in modern Phabricator if we wanted it. We may implement a more general mechanism for T4434.

Test Plan: `grep`

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8330
2014-02-25 12:36:14 -08:00
epriestley
18938b5310 Migrate Differential comments to ApplicationTransactions
Summary:
Ref T2222. This is the big one.

This migrates each `DifferentialComment` to one or more ApplicationTransactions (action, cc, reviewers, update, comment, inlines), and makes `DifferentialComment` a double-reader for ApplicationTransactions.

The migration is pretty straightforward:

  - If a comment took an action not otherwise covered, it gets an "action" transaction. This is something like "epriestley abandoned this revision.".
  - If a comment updated the diff, it gets an "updated diff" transaction. Very old transactions of this type may not have a diff ID (probably only at Facebook).
  - If a comment added or removed reviewers, it gets a "changed reviewers" transaction.
  - If a comment added CCs, it gets a "subscribers" transaction.
  - If a comment added comment text, it gets a "comment" transaction.
  - For each inline attached to a comment, we generate an "inline" transaction.

Most comments generate a small number of transactions, but a few generate a significant number.

At HEAD, the code is basically already doing this, so comments in the last day or two already obey these rules, roughly, and will all generate only one transaction (except inlines).

Because we've already preallocated PHIDs in the comment text table, we only need to write to the transaction table.

NOTE: This significantly degrades Differential, making inline comments pretty much useless (they each get their own transaction, and don't show line numbers or files). The data is all fine, but the UI is garbage now. This needs to be fixed before we can deploy this to users, but it's easily separable since it's all just display code.

Specifically, they look like this:

{F112270}

Test Plan:
I've migrated locally and put things through their paces, but it's hard to catch sketchy stuff locally because most of my test data is nonsense and bad migrations wouldn't necessarily look out of place.
IMPORTANT: I'm planning to push this to a branch and then shift production over to the branch, and run it for a day or two before bringing it to master.

I generally feel good about this change: it's not that big since we were able to separate a lot of pieces out of it, and it's pretty straightforward. That said, it's still one of the most scary/dangerous changes we've ever made.

Reviewers: btrahan

CC: chad, aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8210
2014-02-12 14:34:48 -08:00
epriestley
3092efdc65 Reduce reliance on getRevisionID() on DifferentialComment
Summary: Ref T2222. A few rendering interfaces rely on fishing the revision ID out of a DifferentialComment, but it will only have the PHID soon. Pass in the revision and use it to determine the ID instead.

Test Plan: Browsed, previewed, examined comments. Clicked anchors.

Reviewers: btrahan, chad

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8209
2014-02-12 14:34:26 -08:00
epriestley
8f9b7f4196 Move Differential to proper subscriptions
Summary:
Ref T2222. Ref T4415. We're still writing Differential subscription stuff into this weird legacy `differential_relationship` table, which is like an edge table but extremely ancient.

Move it into a proper table.

I've removed `withSubscriptions()` from `DifferentialRevisionQuery`. It was weird, doesn't work consistently with other similar filters, and was only used by the API. Now it means "ccs", which is consistent with the ApplicationSearch UI and with Maniphest.

Test Plan:
Without migrating, added and removed subscribers via various workflows. Queried for subscribers. Everything worked as expected.

Ran the migration, verified data survived.

Reviewers: btrahan

Reviewed By: btrahan

CC: FacebookPOC, aran

Maniphest Tasks: T2222, T4415

Differential Revision: https://secure.phabricator.com/D8202
2014-02-12 08:53:40 -08:00
epriestley
8735ec4dbe Wrong method name.
Auditors: btrahan
2014-01-26 15:30:38 -08:00
epriestley
014a873773 Update DifferentialDiff: add repositoryPHID, drop parentRevisionID
Summary:
Moves away from ArcanistProjects:

  - Adds storage for diffs to be directly associated with a repository (instead of indirectly, through arcanist projects). Not really populated yet.
  - Drops `parentRevisionID`, which is obsoleted by the "Depends On" edge. This is not exposed in the UI anywhere and doesn't do anything. Resolves TODO.

Test Plan: Ran storage upgrades, browsed around, lots of `grep`.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D8072
2014-01-26 15:29:22 -08:00
epriestley
1da691113a Normalize the definition of "closed" revision statuses
Summary:
Currently, "Closed" and "Abandoned" are treated as "closed". I want to add a flag which treats "Accepted" as "Closed", too, for Asana and other companies who use an Asana-like workflow.

The background here is that their workflow is a bit weird. They basically do audits, but have a lot of things which Diffusion doesn't do well right now. This one change makes Differential fit their workflow fairly well, even though it's an audit workflow.

To prepare for this, normalize the definition of "closed" better. We have a few callsites which explicitly check for "ABANDONED || CLOSED", and normalizing this is cleaner anyway.

Also delete the very old COMMITTED status, which has been obsolete for over a year.

Test Plan: Browsed around most/all of the affected interfaces.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D7653
2013-11-25 17:39:24 -08:00
epriestley
c818e6e159 Remove differential.anonymous-access
Summary:
Fixes T3034. This is obsoleted by modern policies.

This was written by a Facebook intern and is rarely used -- the Hive install might be the only use in the wild. It has never really worked correctly.

Test Plan: `grep`; browsed Differential.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3034

Differential Revision: https://secure.phabricator.com/D7568
2013-11-11 16:05:19 -08:00
Bob Trahan
819bd7f03b Add arcanist project phid to differential.query
Summary: Fixes T3535. Also, flip flop on that spacing thing and make the spaces purdy

Test Plan: got an arcanist projected phid in the json dict

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T3535

Differential Revision: https://secure.phabricator.com/D7565
2013-11-11 15:01:21 -08:00
epriestley
29c7de621e Split exceptionally huge revision titles into the summary
Summary: Fixes T4034.

Test Plan: See screenshot.

Reviewers: spicyj, btrahan

Reviewed By: spicyj

CC: aran

Maniphest Tasks: T4034

Differential Revision: https://secure.phabricator.com/D7443
2013-10-29 21:19:51 -07:00
epriestley
436a403357 Add a "default view" policy to Differential
Summary:
Ref T603. Allows the Differential view policy to be configured with a default.

I've omitted "edit" because I want to wait and see how comment/comment-action policies work out. I could imagine locking "edit" down to only the owner at some point, and providing a wider "interact" capability, or something like that, which would cover accept/reject/commandeer. Users in this group could still edit indirectly by commandeering first.

Test Plan: Created new revisions from the CLI and conduit.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7269
2013-10-09 13:58:00 -07:00
epriestley
2abbd51868 Don't raise a policy exception if a user can't see the parent revision of a new diff
Summary: Ref T603. We currently bomb out here, but should just continue forward. I'm fairly certain we don't even use this for anything anymore (it has been replaced by "depends on") but need to check that.

Test Plan: Created a new revision with `arc diff`.

Reviewers: ljalonen, btrahan, #blessed_reviewers, chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7255
2013-10-07 12:51:04 -07:00
David Cramer
eb548f5af7 Add differential.getrawdiff to Conduit
Test Plan: Confirm the API returns a single flat result with a unified git diff.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, aran, charles

Differential Revision: https://secure.phabricator.com/D7199
2013-10-02 17:03:53 -07:00
epriestley
fc57995330 Fix a typo in differential.querydiffs
Summary:
  - "revision" is misspelled.
  - Remove an unused variable.

Test Plan: Used API console to call method.

Reviewers: btrahan, chad

Reviewed By: chad

CC: aran

Differential Revision: https://secure.phabricator.com/D7184
2013-09-30 17:49:23 -07:00
epriestley
9b3d7b0dba Make most Differential reads policy-aware
Summary: Ref T603. Makes the majority of reads policy aware (and pretty much all the important ones).

Test Plan:
  - Created a comment with `differential.createcomment`.
  - Created a new revision with `arc diff` in order to exercise `differential.creatediff`.
  - Created an inline comment with `differential.createinline`.
  - Added a comment to a revision.
  - Edited an inline comment.
  - Edited a revision.
  - Wrote "Depends on ..." in a summary, saved, verified link was created.
  - Browsed a file in Diffusion.
  - Got past the code I changed in the Releeph request thing.
  - Edited a Releeph request.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7136
2013-09-26 12:37:19 -07:00
epriestley
c467cc464f Make most repository reads policy-aware
Summary: Ref T603. This swaps almost all queries against the repository table over to be policy aware.

Test Plan:
  - Made an audit comment on a commit.
  - Ran `save_lint.php`.
  - Looked up a commit with `diffusion.getcommits`.
  - Looked up lint messages with `diffusion.getlintmessages`.
  - Clicked an external/submodule in Diffusion.
  - Viewed main lint and repository lint in Diffusion.
  - Completed and validated Owners paths in Owners.
  - Executed dry runs via Herald.
  - Queried for package owners with `owners.query`.
  - Viewed Owners package.
  - Edited Owners package.
  - Viewed Owners package list.
  - Executed `repository.query`.
  - Viewed "Repository" tool repository list.
  - Edited Arcanist project.
  - Hit "Delete" on repository (this just tells you to use the CLI).
  - Created a repository.
  - Edited a repository.
  - Ran `bin/repository list`.
  - Ran `bin/search index rGTESTff45d13dffcfb3ea85b03aac8cc36251cacdf01c`
  - Pushed and parsed a commit.
  - Skipped all the Drydock stuff, as it it's hard to test and isn't normally reachable.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7132
2013-09-25 16:54:48 -07:00
epriestley
806ba1e7a1 Fix a missing setViewer() in differential.getrevision
Summary: DiffQuery now requires this.

Auditors: btrahan
2013-09-23 15:22:57 -07:00
epriestley
a025050e87 Fix an issue with differential.getdiff when providing a revision ID
Summary:
If handed a revision ID, we might get more than one result, which causes `executeOne()` to throw. Instead, translate the revision id into a diff ID before querying for the diff.

Also one small consistency change to parameter casing.

Test Plan: Used console to query for a revision with more than one diff using the revision id.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, mbishopim3

Differential Revision: https://secure.phabricator.com/D7026
2013-09-18 15:31:48 -07:00
Joel Beales
df8474d778 Conduit - Add option for createcomment to attach draft inline comments
Summary:
Conduit has a query to make a draft inline comment, but createcomment doesn't have the ability to attach them.
Added optional parameter to attach any existing draft comments. Default value is false, so existing api users won't be effected by the change.

Test Plan: Tested no draft comments and multiple draft comments, attach_inlines =true, false, and empty.

Reviewers: vrana

Reviewed By: vrana

CC: epriestley, aran

Differential Revision: https://secure.phabricator.com/D7019
2013-09-18 10:03:35 -07:00
Bob Trahan
52e65f3d47 Add a differential.getdiffs method
Summary: I kind of made a mess of the API doing T2784. I figure just adding this is fine but LMK if you'd prefer something like diffquery got cleaned up more to handle this.  Also adds an idx() call as I was getting errors looking at old diffs. Fixes T3823.

Test Plan: used the new api via test console - great success.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, aran

Maniphest Tasks: T3823

Differential Revision: https://secure.phabricator.com/D6966
2013-09-17 13:55:41 -07:00
Bob Trahan
b902005bed Kill PhabricatorObjectDataHandle
Summary: Ref T603. Killing this class is cool because the classes that replace it are policy-aware. Tried to keep my wits about me as I did this and fixed a few random things along the way. (Ones I remember right now are pulling a query outside of a foreach loop in Releeph and fixing the text in UIExample to note that the ace of hearts if "a powerful" card and not the "most powerful" card (Q of spades gets that honor IMO))

Test Plan: tested the first few changes (execute, executeOne X handle, object) then got real mechanical / careful with the other changes.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, aran, FacebookPOC

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D6941
2013-09-11 12:27:28 -07:00
Wez Furlong
7b5471b1ae fix perf regression when using 'owned' query
Summary:
D6335 has some unexpected side effects.  This adds back the
where clause for the owned query.  There may be other problems.

Test Plan:
Ran:

```
echo '{"query":"owned","guids":["myphid"]}' | arc --conduit-uri=https://myhost call-conduit differential.find
```

Reviewers: epriestley, dschleimer

Reviewed By: epriestley

CC: Korvin, aran

Differential Revision: https://secure.phabricator.com/D6832
2013-08-28 13:29:36 -07:00
Juan Pablo Civile
d6ce5a31e3 Use DifferentialRevisionQuery in conduit when reviewers are needed
Summary:
Ref T1279.

Switched all differential conduit methods to use `DifferentialRevisionQuery` where `loadRelations` was being used.

Test Plan: Called all the methods changed and verified they still worked as advertised.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T603, T1279

Differential Revision: https://secure.phabricator.com/D6452
2013-07-15 03:29:13 -07:00
epriestley
90123dd739 Add DifferentialDiffQuery and change most callsites
Summary:
Ref T603. This introduces a policy-aware DifferentialDiffQuery and converts most callsites.

I've left unusual callsites (mostly: hard to get the viewer, unusual query, queries related to active diffs) alone for now, so this isn't exhaustive but hits 60-80% of sites.

Test Plan: Created diff; created revision; viewed diffs and revisions; made additional conduit calls.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D6338
2013-07-01 12:38:42 -07:00
epriestley
328aa383e4 Always provide a viewer when executing DifferentialRevisionQuery
Summary: Ref T603. This query isn't policy-aware yet, but prepare for it to be one day.

Test Plan: Looked at: home page; differential home; differential detail; diffusion browse. Made differential.query conduit call.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D6337
2013-07-01 12:38:27 -07:00
epriestley
ab2ed06c38 Remove DifferentialRevisionListData
Summary: Ref T603. This is a very old, very bad version of `DifferentialRevisionQuery`. I want to modernize only the latter. Express the remaining callsite of the former in terms of `DifferentialRevisionQuery`.

Test Plan: Executed all four modes of `differential.find`.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D6335
2013-07-01 12:38:08 -07:00
epriestley
6a2ae07791 Abstract access to DifferentialInlineComment behind a Query
Summary:
Ref T2222. See D6260.

Push all this junk behind a Query so I can move the storage out from underneath it.

Test Plan: Viewed home page, list view, revision. Made draft, looked at preview, submitted draft, viewed inline, replied to inline.

Reviewers: btrahan

Reviewed By: btrahan

CC: chad, aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D6262
2013-06-21 12:54:56 -07:00
epriestley
6a2e27ba8d Put all DifferentialComment loading behind DifferentialCommentQuery
Summary:
Ref T2222.

I'm thinking about how I want to approach the Asana sync, and I want to try to do T2222 first so that we can build it cleanly on top of ApplicationTransactions. I think we can at least walk down this road a little bit and if it turns out to be scary we can take another approach.

I was generally very happy with how the auth migration turned out (seemingly, it was almost completely clean), and want to pursue a similar strategy here. Basically:

  - Wrap the new objects in the old objects for reads/writes.
  - Migrate all the existing data to the new table.
  - Everything hard is done; move things over a piece at a time at a leisurely pace in lots of smallish, relatively-easy-to-understand changes.

This deletes or abstracts all reads of the DifferentialComment table. In particular, these things are **deleted**:

  - The script `undo_commits.php`, which I haven't pointed anyone at in a very long time.
  - The `differential.getrevisionfeedback` Conduit method, which has been marked deprecated for a year or more.
  - The `/stats/` interface in Differential, which should be rebuilt on Fact and has never been exposed in the UI. It does a ton of joins and such which are prohibitively difficult to migrate.

This leaves a small number of reading interfaces, which I replaced with a new `DifferentialCommentQuery`. Some future change will make this actually load transactions and wrap them with DifferentialComment interfaces.

Test Plan: Viewed a revision; made revision comments

Reviewers: btrahan

Reviewed By: btrahan

CC: edward, chad, aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D6260
2013-06-21 12:51:18 -07:00
Jakub Vrana
2427d317b2 Reference task based on branch name in arc diff
Summary:
This was mentioned in T2928 and nobody objected.
It just references the task instead of fixing it as that would be too aggressive.
It also doesn't check assignee of the task (by purpose).

Test Plan: Created diff from a branch named T2928.

Reviewers: epriestley, edward

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T2928

Differential Revision: https://secure.phabricator.com/D5640
2013-05-30 17:33:17 -07:00
Gareth Evans
94e7878a57 Route internal conduit calls if other hosts available
Summary:
Ref T2785

Looks for hosts in `conduit.servers` config and if any exist route any conduit calls through any one of the hosts.

Test Plan:
Make some curl calls to public methods (`conduit.ping`), watch the access log for two requests. Make some calls from the UI that require authentication, watch the access log a bit more.

Also ran the unit tests.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T2785

Differential Revision: https://secure.phabricator.com/D5970
2013-05-19 04:16:10 -07:00
Afaque Hussain
d43bcdc1ee Adding pro tips to Differential commit message
Summary: Randomly displaying tips about different features of arc.

Test Plan: Will test by running arc diff and seeing if the tips appear in text-based UI of arc

Reviewers: epriestley, AnhNhan

Reviewed By: AnhNhan

CC: aran, Korvin, AnhNhan

Differential Revision: https://secure.phabricator.com/D5351
2013-03-19 09:29:25 -07:00
Afaque Hussain
05c5c43159 Yell at developers if they don't replace the default revision title with thier new revision title.
Summary: Same as the title

Test Plan: Will test locally by running arc diff

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin, AnhNhan, starruler

Maniphest Tasks: T2663

Differential Revision: https://secure.phabricator.com/D5297
2013-03-13 09:34:29 -07:00
epriestley
855e085c6f Uninstall Conduit calls when uninstalling applications
Summary: Fixes T2698. When applications are installed, their Conduit calls should drop out. This will also let us land Releeph without exposing Conduit calls.

Test Plan:
  - Viewed Conduit console; uninstalled some applications and verified their calls dropped out.
  - Tried to make an uninstalled call; got an appropriate error.

Reviewers: edward, btrahan

Reviewed By: edward

CC: aran

Maniphest Tasks: T2698

Differential Revision: https://secure.phabricator.com/D5302
2013-03-13 07:09:05 -07:00
Afaque Hussain
a521790f13 Ported loadAuxiliaryFields method to differential.query conduit method
Summary: Ported the the loadAuxiliaryfields method from differential.getrevision method to modern conduit differential.query method

Test Plan: Created a test diff in differential to see if nothing has been broken

Reviewers: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D5230
2013-03-05 05:58:04 -08:00
epriestley
0a069cb55a Require a viewer to load handles
Summary:
Unmuck almost all of the we-sort-of-have-viewers-some-of-the-time mess.

There are a few notable cases here:

  - I used Omnipotent users when indexing objects for search. I think this is correct; we do policy filtering when showing results.
  - I cheated in a bad way in the Remarkup object rule, but fixing this requires fixing all the PhabricatorRemarkupEngine callsites (there are 85). I'll do that in the next diff.
  - I cheated in a few random places, like when sending mail about package edits. These aren't a big deal.

Test Plan:
  - Grepped for all PhabricatorObjectHandleData references.
  - Gave them viewers.

Reviewers: vrana

Reviewed By: vrana

CC: aran, edward

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D5151
2013-02-28 17:15:09 -08:00
epriestley
a22bea2a74 Apply lint rules to Phabricator
Summary: Mostly applies a new call spacing rule; also a few things that have slipped through via pull requests and such

Test Plan: `find src/ -type f -name '*.php' | xargs -n16 arc lint --output summary --apply-patches`

Reviewers: chad

Reviewed By: chad

CC: aran

Differential Revision: https://secure.phabricator.com/D5002
2013-02-19 13:33:10 -08:00
epriestley
24ced7e7bd Expose commit information via conduit instead of user information
Summary:
After D4825, this information is often available to us in a safe way. Provide it explictly.

This removes or reduces functionality in some cases, but I think we can plug those holes with Conpherence addresses and/or explicit user acknowledgement/config.

Test Plan: Patched a commit with `arc patch` and got the original address out.

Reviewers: btrahan, edward, vrana

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D4828
2013-02-05 20:10:57 -08:00
vrana
6c44e704b5 Delete differential.updatetaskrevisionassoc
Summary: Used only by Facebook.

Test Plan: Moved to Facebook repo and verified it still works.

Reviewers: nh, epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4515
2013-01-18 18:20:53 -08:00
vrana
2cc7f82ece Move Conduit methods inside applications
Test Plan:
/conduit/
/conduit/method/arcanist.projectinfo/
Call method

  $ echo '{}' | arc call-conduit user.whoami

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4268
2012-12-21 12:21:59 -08:00