Summary:
Added a new method differential.createcomment
Task ID: #752014
Test Plan:
I created a test diff and called this method via the conduit
from a client PHP script to add comments. I confirmed that
1) the comment appeared on the revision, 2) URLs within the
comment were turned into hyperlinks, and 3) Phabricator
sent a notification email to the people watching the test
diff.
Reviewers: nh, jungejason, epriestley
Reviewed By: nh
CC: aran, nh
Differential Revision: 1128
Summary:
Move event framework from Phabricator to libphutil so it can be used in other
phutil projects, such as Arcanist.
Test plan:
Use along with path to libphutil, events should work as expected.
Reviewers: epriestley
Differential Revision: 1098
Summary: createrevision creates the revision as the user which certificate is
used. Add a meta parameter to API calls named actAsUser so one user can create
revisions for someone else. Right now there is no authentication.
Test Plan: Called createrevision with one users cert and set actAsUser to
someone else. The revision was created as the actAsUser user.
Reviewers: epriestley, nh, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1087
Summary:
the code tries to access 'encoding' property even when the
repository is empty. The fix is to set it to null in that case.
Test Plan: run the conduit method on my sandbox and it works now.
Reviewers: grglr, epriestley, nh
Reviewed By: grglr
CC: aran, grglr
Differential Revision: 1075
Summary:
`arc commit` and `arc mark-committed` would only add comments <author> committed
this revision, since now everyone can run this commands it makes more sense to
show the actual committer instead of the author.
Test Plan:
Commit (or mark committed) not your revision, Phabricator should add <you>
committed this revision comment instead of <author> committed this revision.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1067
Summary:
Allow anonymous access to conduit getdiff method, which is needed for anonymous
`arc patch`.
Test Plan: Running getdiff with an unauthenticated conduit should work.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1068
Summary:
This adds an encoding detail to the repository, so we can attempt to
convert hunks previously detected as binary.
We also add the encoding information to the arcanist projectinfo
API so we can pull the information if we have it when uploading changes
via arc.
Test Plan:
Changed encoding through the edit UI, and saw "This is binary file", and
changed it back and saw the correct output from the diff.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1009
Filesystem::readRandomCharacters()
Summary: See T547. To improve auditability of use of crypto-sensitive hash
functions, use Filesystem::readRandomCharacters() in place of
sha1(Filesystem::readRandomBytes()) when we're just generating random ASCII
strings.
Test Plan:
- Generated a new PHID.
- Logged out and logged back in (to test sessions).
- Regenerated Conduit certificate.
- Created a new task, verified mail key generated sensibly.
- Created a new revision, verified mail key generated sensibly.
- Ran "arc list", got blocked, installed new certificate, ran "arc list"
again.
Reviewers: jungejason, nh, tuomaspelkonen, aran, benmathews
Reviewed By: jungejason
CC: aran, epriestley, jungejason
Differential Revision: 1000
without quotes around it (and similar)
Summary: See D1010. The API uniformly requires JSON, which is good for
strictness and predictablity but can be bad for UEX, especially considering that
we silently continue after failing to decode things. Toss the user a lifeline
when they make this common mistake.
Test Plan: Ran API calls with invalid and valid inputs. Invalid inputs gave me a
reasonable error message.
Reviewers: davidreuss, jungejason, nh, tuomaspelkonen, aran
Reviewed By: nh
CC: aran, nh
Differential Revision: 1012
Summary:
This is an attempt to satisfy a lot of the one-off requests a little more
generally, by providing a relatively generic piece of event architecture.
Allow the registation of event listeners which can react to various application
events (currently, task editing).
I'll doc this a bit better but I wanted to see if anyone had massive objections
to doing this or the broad approach. The specific problem I want to address is
that one client wants to do a bunch of routing for tasks via email, so it's
either build a hook, or have them override most of ManiphestReplyHandler, or
something slightly more general like this.
Test Plan: Wrote a silly listener that adds "Quack!" to a task every time it is
edited and edited some tasks. I was justly rewarded.
Reviewers: nh, jungejason, tuomaspelkonen, aran
Reviewed By: aran
CC: aran, epriestley
Differential Revision: 881
Summary:
We currently rely on "remote_hooks_enabled" in .arcconfig to determine whether
commands like "arc amend" and "arc merge" should imply "arc mark-committed".
However, this is a historical artifact that is now bad for a bunch of reasons:
- The option name is confusing, it really means 'repository is tracked'.
- The option is hard to discover and generally sucks.
- We can empirically determine the right answer since we now know if a project
is in a tracked repository.
Add a call which arcanist can make on these workflows to figure out if it is
interacting with a project in a tracked repository or not.
Also added an "isTracked()" convenience method to reduce the number of magic
strings all over the place.
Test Plan: Ran "arcanist.projectinfo" for nonexistent, untracked and tracked
projects.
Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran
Reviewed By: Makinde
CC: aran, epriestley, Makinde
Differential Revision: 945
Summary:
We need to query the unit status in order to determine if
there are postponed unit tests to update after running "arc unit"
Test Plan:
1) set my conduit uri to a server running the new code
2) ensured unitStatus existed when retrieving a diff
Reviewers: epriestley
Reviewed By: epriestley
CC: dpepper, aran, epriestley
Differential Revision: 918
Summary:
These fields use auxiliary storage now. Migrate the data and get rid of the
columns in the main table.
- This might take a little while to run, although there are <500k rows so
probably not too long.
- Maybe grab a backup of the table first, if I screwed something up this will
delete the data in these fields.
Test Plan:
- Ran migration locally.
- Browsed Differential.
- Grepped for "revertPlan" and "blameRevision".
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason, epriestley
Differential Revision: 832
Summary:
I didn't realize createDiffDict was a public method when I
modified it, and I broke the API call in getrevision. This moves the
modification inside the method and reverts the method header back to
it's original form.
Test Plan: none
Reviewers: epriestley
Reviewed By: epriestley
CC: edward, aran, epriestley
Differential Revision: 883
Summary:
Provide three Phriction methods for programmatic access to Phriction:
- phriction.info: get information about a document
- phriction.history: get change history for a document
- phriction.edit: create or update a document
I moved all the editing/creation logic into a new PhrictionDocumentEditor to
share code between the Conduit and Web edit pathways.
Test Plan: Got info and history via conduit. Edited and created new pages via
conduit and web.
Reviewers: hsb, jungejason, tuomaspelkonen, aran, hunterbridges
Reviewed By: hunterbridges
CC: skrul, aran, hunterbridges
Differential Revision: 866
Summary:
We need createlintresults because we are doing extended
static analysis offline, and thus we need to be able to update the
lint results associated with a diff. This is similar to
updateunitresults, but "create" is more accurate than "update" since
we never need to change existing lint results.
getdiffproperty is used by the client to ensure it isn't creating any
duplicates lint results. It's the symmetric operation to
setdiffproperty, which already exists.
Test Plan:
We have a new offline linter that I used to test. This
linter calls getdiffproperty on every run.
1. Tested updating an existing set of lint results by first running
"arc diff" with lint errors caught by the local linter, then later
running offline analysis which catches one other error and updates via
createlintresults. Ensured the differential lint results were as
expected.
2. Tested the creation of an entirely new diff property through
createlintresults. I first ran "arc diff --nolint" to skip all lint
results, then ran offline analysis which caught an error and updated
through createlintresults. Ensured differential lint results were as
expected.
Reviewers: epriestley
Reviewed By: epriestley
CC: dpepper, aran, mgummelt, jungejason, epriestley
Differential Revision: 868
Summary: Execute searches like the primary Maniphest task list. Let me know what
else you guys need from this API.
Test Plan: Executed "maniphest.info" and "maniphest.find"
Reviewers: jungejason, tuomaspelkonen, aran, nh
Reviewed By: nh
CC: blair, skrul, aran, jungejason, epriestley, nh, tuomaspelkonen
Differential Revision: 867
Summary:
When an object is updated, record the content source for the update. This mostly
isn't terribly useful but one concrete thing I want to do with it is let admins
audit via-email replies more easily since there are a bunch of options which let
you do hyjinx if you intentionally configure them insecurely. I think having a
little more auditability around this feature is generally good. At some point
I'm going to turn this into a link admins can click to see details.
It also allows us to see how frequently different mechanisms are used, and lets
you see if someone is at their desk or on a mobile or whatever, at least
indirectly.
The "tablet" and "mobile" sources are currently unused but I figured I'd throw
them in anyway. SMS support should definitely happen at some point.
Not 100% sure about the design for this, I might change it to plain text at some
point.
Test Plan: Updated objects and saw update sources rendered.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, epriestley, jungejason
Differential Revision: 844
Summary: This could be a lot fancier but let's see what else we need. Also fixed
some bugs with maniphest.info.
Test Plan: Used the Conduit web console to create some tasks with different
values.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason, epriestley
Differential Revision: 824
Summary:
After D814 and D829, you should be able to implement this logic in the
didWriteRevision() method of the field.
Note that the attacher is still referenced in
ConduitAPI_differential_updatetaskrevisionassoc_Method. This method should
probably be moved to facebook/ since it's pretty Facebook-specific.
No rush on any of this, it's not hurting anything.
Test Plan:
- Hit differential.getcommitmessage
- Ran 'arc diff'
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 830
Summary:
I think this is the last major step -- use the fields to parse commit messages,
not a hard-coded list of stuff. This adds two primary methods to fields, one to
get all the labels they'll parse (so we can do "CC" and "CCs" and treat them as
the same field) and one to parse the string into a canonical representation
(e.g., lookup reviewers and such).
You'll need to impelement the one block of task-specific stuff I removed in
Facebook's task field:
list($pre_comment) = split(' -- ', $data);
$data = array_filter(preg_split('/[^\d]+/', $pre_comment));
foreach ($data as $k => $v) {
$data[$k] = (int)$v;
}
$data = array_unique($data);
break;
Otherwise I think this is clean.
Test Plan:
- Called the conduit method with various commit messages, parsed fields/errors
seemed correct.
- "arc diff"'d this diff onto localhost, then updated it.
- "arc amend"'d this diff.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason, epriestley
Differential Revision: 829
Summary:
When rendering commit messages, drive all the logic through field specification
classes instead of the hard-coded DifferentialCommitMessageData class. This
removes DifferentialCommitMessageData and support classes.
Note that this effectively reverts D546, and will cause a minor break for
Facebook (Task IDs will no longer render in commit messages generated by "arc
amend", and will not be editable via "arc diff --edit"). This can be resolved by
implementing the feature as a custom field. While I've been able to preserve the
task ID functionality elsewhere, I felt this implementation was too complex to
reasonably leave hooks for, and the break is pretty minor.
Test Plan:
- Made numerous calls to differential.getcommitmessage across many diffs in
various states, with and without 'edit' and with and without various field
overrides.
- General behavior seems correct (messages look accurate, and have the
expected information). Special fields like "Reviewed By" and "git-svn-id" seem
to work correctly.
- Edit behavior seems correct (edit mode shows all editable fields, hides
fields like "Reviewed By").
- Field overwrite behavior seems correct (overwritable fields show the correct
values when overwritten, ignore provided values otherwise).
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason
Differential Revision: 814
Summary:
See T354. List every rule which has ever been applied in X-Herald-Rules, not
just the ones which most recently triggered.
Also some random fixes while I was debugging this:
- When conduit methods throw non-conduit exceptions, make sure they get
logged.
- Trigger the Facebook "tasks" backcompat block only if we were going to fail
(this should reduce the shakniess of the transition).
- Fix some log spew from the new field stuff.
Test Plan:
- Created a rule (ID #3) "No Zebras" which triggers for revisions without
"zebra" in the title.
- Created a revision without "zebra" in the title, got X-Herald-Rules: <2>,
<3>
- Updated revision to have "zebra" in the title, verified rule did not trigger
in Herald transcript.
- Verified X-Herald-Rules is still: <2>, <3>
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen
CC: aran, epriestley
Differential Revision: 817
Summary:
Provide a catchall mechanism to find unprotected writes.
- Depends on D758.
- Similar to WriteOnHTTPGet stuff from Facebook's stack.
- Since we have a small number of storage mechanisms and highly structured
read/write pathways, we can explicitly answer the question "is this page
performing a write?".
- Never allow writes without CSRF checks.
- This will probably break some things. That's fine: they're CSRF
vulnerabilities or weird edge cases that we can fix. But don't push to Facebook
for a few days unless you're prepared to deal with this.
- **>>> MEGADERP: All Conduit write APIs are currently vulnerable to CSRF!
<<<**
Test Plan:
- Ran some scripts that perform writes (scripts/search indexers), no issues.
- Performed normal CSRF submits.
- Added writes to an un-CSRF'd page, got an exception.
- Executed conduit methods.
- Did login/logout (this works because the logged-out user validates the
logged-out csrf "token").
- Did OAuth login.
- Did OAuth registration.
Reviewers: pedram, andrewjcg, erling, jungejason, tuomaspelkonen, aran,
codeblock
Commenters: pedram
CC: aran, epriestley, pedram
Differential Revision: 777
Summary:
When we create or update a revision, we use a parsed commit message dictionary
to edit its fields. Drive consumption of the dictionary through custom fields
instead of hardcoding.
This requires adding some fields which don't really do anything right now to
cover fields which appear only in the commit message.
Test Plan: "arc diff"'d this revision against localhost, "arc diff"'d again to
update.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason
Differential Revision: 811
Summary:
Differential has a bunch of display-only fields, implement them all as field
specifications instead of hard-coded fields.
Also add some more documentation and fix redundant string constants in blame
rev/revert plan fields.
Test Plan: Viewed, edited, and hit conduit for revisions.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason, epriestley
Differential Revision: 807
Summary:
- Fix a bug where 'caption' didn't do anything.
- Provide an abstract base implementation for extensions.
- Add some documentation.
- Expose aux fields via conduit.
Test Plan: Added some fields like "Dinosaur", "Kilograms" and "derp" on my local
install. Read documentation.
Reviewed By: jungejason
Reviewers: hunterbridges, jungejason, tuomaspelkonen, aran
CC: aran, philc, jungejason
Differential Revision: 785
Summary: Similar to D785 for Maniphest, expose auxiliary field values via
Conduit.
Test Plan: Ran revision.getinfo on a revision with aux fields, got them in the
response.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason
Differential Revision: 802
Summary:
we need to know the revision id in order to generate
differential links. It would be amazing if there existed some library
for object <-> network call mapping, and we could get all the
information about an object given some unique ID rather than having to
create a dictionary manually. One can dream...
Test Plan:
1. setup on test phabricator instance
2. called differential.getdiff from client code with a valid diffid
3. verified that the correct revisionID was included
Reviewed By: epriestley
Reviewers: epriestley
CC: dpepper, aran, epriestley, mgummelt
Differential Revision: 795
Summary:
- Allow the console to handle abstract classes correctly.
- Move paste dictionary generation to an abstract base class.
- Add paste.create.
- Add 'objectName', 'parentPHID', and 'content' to Paste info dictionaries
(you can use filePHID with file.download to get the content but I think just
always sending it back is reasonable).
Test Plan:
- Use paste.create to create new pastes.
- Used paste.info to get existing pastes.
- Checked console UI to make sure "paste." didn't show up or anything
silly/dumb like that.
- Tried to call the method "paste" and got the right exception.
Reviewed By: codeblock
Reviewers: codeblock, jungejason, tuomaspelkonen, aran
CC: aran, codeblock
Differential Revision: 747
Summary:
Need this for 'arc upload' and 'arc download'.
Given a file PHID or ID, provides information about it.
Test Plan:
- Implemented 'arc upload' and 'arc download' based on 'file.info'.
- Used Conduit API console to test method.
Reviewed By: codeblock
Reviewers: codeblock, jungejason, tuomaspelkonen, aran
CC: aran, codeblock, epriestley
Differential Revision: 741
Summary:
Provides a slowvote.poll.info method.
Test Plan:
Web console - seemed to work fine.
Reviewers:
epriestley, phuzion
CC:
Differential Revision: 659
Summary:
Now that we store language with pastes, we can include this in Conduit.
Test Plan:
Tried it on a paste with a blank language, and one with a specified language.
16:14:50 <@CodeBlock> P1
16:14:51 <@codeblock-phabot> P1: http://phabricator.local/P1 - test.php
16:15:05 <@CodeBlock> P43
16:15:06 <@codeblock-phabot> P43: http://phabricator.local/P43 - sadoijfoisaf (php)
Reviewers:
epriestley, Ttech
CC:
Differential Revision: 616
Summary:
- have files be uploaded by drag+drop instead of browse.
- Files are named by their uploaded filename, the user isn't given a chance to enter a file name. Is this bad?
- Store author PHID now with files
- Allow an ?author=<username> to limit the /files/ list by author.
- If one file is uploaded, the user is taken to its info page.
- If several are uploaded, they are taken to a list of their files.
Test Plan:
- Quickly tested everything and it still worked, I'd recommend some people try this out before it gets committed though. It's a rather huge revision.
Reviewers:
epriestley, Ttech
CC:
Differential Revision: 612
Summary:
My earlier diff refactored some code without completely
respecting the semantics, sometimes resulting in duplicate field names
returned from differential.getcommitmessage. This fixes that.
Test Plan:
ran "arc diff" with diff causing the bug (commit message
had an empty Revert Plan: field) and verified no duplicate fields
Reviewed By: epriestley
Reviewers: epriestley
CC: aran, dpepper, epriestley
Differential Revision: 610
Summary: This defines an extremely basic version of an activity feed, like
Facebook's news feed. It doesn't do much of interest yet.
Test Plan: Published some feed stories:
https://secure.phabricator.com/file/view/PHID-FILE-5061aa72105bbdc05b21/
Reviewed By: tuomaspelkonen
Reviewers: jungejason, tuomaspelkonen, aran
Commenters: codeblock, jungejason
CC: aran, epriestley, codeblock, tuomaspelkonen, jungejason
Differential Revision: 593
Summary: This allows us to detect a mismatched client and server hostname. See
D591.
Test Plan: See D591.
Reviewed By: tuomaspelkonen
Reviewers: jungejason, llorca, tuomaspelkonen, aran
CC: aran, tuomaspelkonen
Differential Revision: 592
Summary: We added a proper way to get the authoritative user a while ago, this
method just never got switched to use it.
Test Plan: Created a revision locally, was recognized as the revision author.
Reviewed By: jungejason
Reviewers: gc3, jungejason, tuomaspelkonen, aran
CC: aran, jungejason
Differential Revision: 566
Summary:
Might be useful to include a $paste['uri'] in the response. Specifically for Pxxx responses in phabot.
Test Plan:
Local Conduit web console.
Reviewers:
epriestley
CC:
Differential Revision: 552
