1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-19 12:00:55 +01:00
Commit graph

167 commits

Author SHA1 Message Date
vrana
ec068ff453 Avoid fatal error if there's no Arcanist project or repository
Test Plan: Display diff without repository

Reviewers: nh, epriestley

Reviewed By: epriestley

CC: aran, Koolvin

Differential Revision: https://secure.phabricator.com/D2373
2012-05-02 17:39:52 -07:00
Nick Harper
6039ca6fb5 Create option for differential custom fields to display warning on accept
Summary:
This adds support to differential fields to display warnings before a revision
gets accepted. Since lint and unit are differential fields, the code for their
warnings was moved into their respective field specification classes, so there
is only one code path for warnings (lint, unit, or custom).

Test Plan:
Select 'Accept' on a revision with lint/unit warnings and see messages appear
like they used to. Change it back to 'Comment' and they go away. Repeat with
a revision without lint/unit warnings and see no warnings appear. Checked
darkconsole to see no errors due to this.

Reviewers: jungejason, epriestley, vrana

Reviewed By: epriestley

CC: aran, Koolvin

Differential Revision: https://secure.phabricator.com/D2363
2012-05-02 14:30:21 -07:00
Nick Harper
56529f88e0 Show verb of closed diff based on backing vcs, not local vcs
Summary:
When choosing a verb to show with a closed differential revision, choose the
verb based on the upstream vcs, not the vcs used to create the diff, since these
are not the same thing. I also updated the documentation for the next step for
an accepted diff for the case where the local vcs and backing vcs aren't the
same (since arc land doesn't work for those).

Test Plan:
Loaded a committed diff and an accepted diff from fbcode and www to check that
they show the correct thing.

Reviewers: jungejason, epriestley

Reviewed By: epriestley

CC: aran, Koolvin

Maniphest Tasks: T1118

Differential Revision: https://secure.phabricator.com/D2360
2012-05-01 11:30:02 -07:00
epriestley
f04d8ab1a7 Further improve unit/lint rendering
Summary:
I think this improves things, let me know if you have feedback.

Also addresses T840.

Test Plan: See screenshots...

Reviewers: vrana, btrahan, jungejason

Reviewed By: vrana

CC: aran, zeeg

Maniphest Tasks: T840

Differential Revision: https://secure.phabricator.com/D2357
2012-05-01 10:15:56 -07:00
vrana
4cc61687aa Hide lint and unit error details
Summary:
Before: {F10754}

After: {F10753}

Test Plan:
View revision with lint warnings and unit errors.
Click on Details.
Click on Details.
Click on Details.
Click on Details.

Reviewers: asukhachev, epriestley

Reviewed By: epriestley

CC: aran, Koolvin

Differential Revision: https://secure.phabricator.com/D2351
2012-04-30 16:50:36 -07:00
epriestley
7e9a2dc4d2 Clarify "Closed" revision status with VCS-specific language
Summary: Instead of rendering "Revision Status: Closed (Sat, Apr 28, 10:28 AM)", render "Revision Status: Closed (Pushed Sat, Apr 28, 10:28 AM)", or "Committed", as appropriate.

Test Plan: Looked at a committed revision.

Reviewers: btrahan, vrana, jungejason

Reviewed By: jungejason

CC: jeffmo, rdbayer, aran

Maniphest Tasks: T909

Differential Revision: https://secure.phabricator.com/D2334
2012-04-28 14:10:39 -07:00
vrana
1f2cf78c1b Display committed date in Revision Status field
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
2012-04-19 15:05:09 -07:00
epriestley
2360504462 Fix null test plan database error
Summary:
Some Differential fields are not nullable; when Test Plan is switched to non-required mode we can end up trying to save a null value to a non-nullable column (see D2193).

(I should probably just alter the schema to make these fields nullable, but that might have farther-reaching effects.)

Test Plan: Reproduced error, applied patch, no more error.

Reviewers: btrahan, vrana, jungejason

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D2200
2012-04-10 16:54:05 -07:00
epriestley
01907bcccc Allow "Test Plan" to be disabled in config
Summary:
This is a somewhat common request, and far more difficult than necessary currently.

I think the field is useful enough to leave it default-enabled, but there's wide diversity in testing philosophy.

Test Plan: Verified "test plan" field appeared. Disabled config. Verified "test plan" field vanished.

Reviewers: btrahan, vrana, jungejason

Reviewed By: vrana

CC: aran, asouza

Differential Revision: https://secure.phabricator.com/D2193
2012-04-10 13:36:05 -07:00
epriestley
34ca4a9ba7 Update arcanist documentation to reflect "land", a sane relative commit, and "--auto"
Summary: See D2080. The introduction of `arc land`, defaulting to `origin/master`, and --auto enormously simplifies the documentation.

Test Plan: Read documentation.

Reviewers: btrahan

Reviewed By: btrahan

CC: 20after4, aran

Maniphest Tasks: T894

Differential Revision: https://secure.phabricator.com/D2082
2012-04-07 10:39:51 -07:00
vrana
582fc847f2 Use assert_instances_of() in Differential
Summary: NOTE: This is not produced by a script so there might be errors. Please review carefully.

Test Plan: Browse around Differential.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran

Differential Revision: https://secure.phabricator.com/D2103
2012-04-04 15:11:30 -07:00
vrana
84398fc581 Allow system agents in commit message object lists
Summary:
When system agent adds a comment then he is added to CC.
When I amend and update then I get message "Commit message references nonexistent ..."

Test Plan: Update revision with system agent in CC.

Reviewers: epriestley

Reviewed By: epriestley

CC: michalburger1, aran

Differential Revision: https://secure.phabricator.com/D2100
2012-04-04 10:46:00 -07:00
vrana
4fba549a99 Use PhabricatorEnv::newObjectFromConfig() wherever possible
Test Plan:
/mail/send/
scripts/aphront/aphrontpath.php /

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1983
2012-03-21 14:57:52 -07:00
epriestley
3d3e63e4fc Fix an issue with empty 'Summary' fields in commit messages
Summary:
If the 'Summary' is not present and not inferred to be empty from a newline after the title with no explicit 'Summary' field, we'll copy all the field values from the revision (including NULL), not overwrite the 'Summary' value from the message (since it's not present) and then write the NULL back to the revision.

Instead, string cast the read from the Revision so we write back empty string in the not-provided case.

Test Plan: Ran "arc diff --create --use-commit-message HEAD" with P336 in HEAD, didn't fail (previously, it failed).

Reviewers: btrahan, 20after4

Reviewed By: 20after4

CC: aran, epriestley

Maniphest Tasks: T1019

Differential Revision: https://secure.phabricator.com/D1956
2012-03-19 16:06:45 -07:00
Nick Harper
345340005b Don't put bad mailing list names in the CC list of a commit message
Summary:
If we can't look up the name for a mailing list, don't put 'Unknown Mailing
List' in the commit message - it will confuse things later down the road.

Surely there is a better way of doing this than checking the name of the
handle for the mailing list.

Test Plan:
called differential.getcommitmessage on a revision that had an invalid mailing
list phid.

Reviewers: epriestley, schrockn, jungejason

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1910
2012-03-15 10:21:40 -07:00
epriestley
11cccb98c2 Add "final" to more classes
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
2012-03-13 11:18:11 -07:00
epriestley
492d047a49 Improve tokenizer sorting rules
Summary:
Currently, we sort all results alphabetically. This isn't ideal. Instead, sort them like this:

  - If the viewing user appears in the list, always sort them first. This is common in a lot of contexts and some "Ben Evans" guy is sorting first on secure.phabricator.com and causing me no end of aggravation.
  - If the tokens match a "priority" component (e.g., username), sort that before results which do not have a "priority" match.
  - Within a group (self, priority, everything else) sort tokens alphabetically.

NOTE: I need to go add setUser() to all the tokenizers to make the "self" rule work, but that's trivial so I figured I'd get this out first.

Test Plan:
https://secure.phabricator.com/file/data/4s2a72l5hhyyqqkq4bnd/PHID-FILE-x2r6ubk7s7dz54kxmtwx/Screen_Shot_2012-03-07_at_9.18.03_AM.png

Previously, "aaaaaepriestley" (first alphabetic match) would sort before "epriestley" (the viewing user). Now, "epriestley" sorts first because that is the viewer.

https://secure.phabricator.com/file/data/rmnxgnafz42f23fsjwui/PHID-FILE-yrnn55jl3ysbntldq3af/Screen_Shot_2012-03-07_at_9.18.09_AM.png

Previously, "aaaagopher" (first alphabetic match) would sort before "banana" (the "priority" match). Now, "banana" sorts first because it priority matches on username.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, epriestley

Maniphest Tasks: T946

Differential Revision: https://secure.phabricator.com/D1807
2012-03-07 13:17:44 -08:00
epriestley
1706212e24 Minor, ensure auditors is an array. 2012-03-06 19:46:26 -08:00
epriestley
a95c9873aa Add an "Auditors" field to commit messages which pushes audit requests when present
Summary:
Adds an optional "Auditors" field (like "Reviewers") to commit messages which gives installs a zero-config method for making audit requests.

This field does not appear on templates unless set, and is mostly ignored (but validated and preserved) by Differential.

It is then parsed by the daemons if present, and audit requests are pushed to valid users.

Test Plan: Made an "Auditors" commit and verified it was retained with "arc amend --show". Pushed it and verified the audit was triggered.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, epriestley

Maniphest Tasks: T904, T880

Differential Revision: https://secure.phabricator.com/D1793
2012-03-06 15:10:35 -08:00
vrana
f5f7987013 Revert rP87c60abbd02d, apply D1772
Test Plan:
Apply SQL patch.
Visit /differential/.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1781
2012-03-05 11:04:55 -08:00
vrana
87c60abbd0 Display Reviewed By instead of first Reviewer in revisions overview
Summary:
Displaying reviewer who was by coincidence listed first is quite confusing, especially for committed revisions.
This displays the one who really reviewed the revision if available.

This implementation is pretty bad from performance perspective - O(N) queries to retrieve all comments.
The page load still feels quite fast.

Test Plan: /differential/filter/revisions/

Reviewers: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1772
2012-03-05 10:30:36 -08:00
Natthu Bharambe
a22827865f Merge branch 'master' of github.com:facebook/phabricator 2012-02-24 15:29:04 -08:00
Natthu Bharambe
ed1928eee2 Show lint/unit failure explanation in Phabricator
Summary:
Tweaked lint/unit field specifications to introduce the failure
explanation read from arc:[lint|unit]-excuse.

Task ID: #

Blame Rev:

Test Plan:
Create dumb diffs with errors - run modified 'arc' and change
conduit_url to http://phabricator.dev1020.facebook.com/api/ - verified
that explanation shows up with proper formatting.

Revert Plan:

Tags:

Reviewers: epriestley, nh

Reviewed By: epriestley

CC: Girish, akramer, blair, aran, epriestley, andreygoder

Differential Revision: https://secure.phabricator.com/D1689
2012-02-24 15:28:06 -08:00
epriestley
92f3ffd811 Drive differential revision list with custom fields
Summary:
Build the revision list table out of custom fields instead of hard-coding it, so
installs can add all sorts of zany things to it.

NOTE: You may need to implement sortFieldsForRevisionList() if you have a custom
DifferentialFieldSelector, or some fields might show up out of order.

This implementation will preserve the expected behavior:

  public function sortFieldsForRevisionList(array $fields) {
    $default = new DifferentialDefaultFieldSelector();
    return $default->sortFieldsForRevisionList($fields);
  }

Test Plan:
  - Loaded differential revision list, identical to old list.
  - Profiled page to verify the cost increase isn't significant (it's quite
small).

Reviewers: jungejason, btrahan

Reviewed By: btrahan

CC: aran, btrahan, davidreuss, epriestley

Maniphest Tasks: T773, T729

Differential Revision: https://secure.phabricator.com/D1388
2012-02-20 05:38:21 -08:00
vrana
8da4f981fb Always display Branch in revision
Summary:
I, as an author, sometimes forget branch associated with a revision.
Plus setting ##differential.show-host-field## makes a false sense of security
that branch will stay hidden so that I can name it
//finally_solve_this_crap_which_makes_no_sense//. But it is published in
Accepted and Request Changes e-mails anyway.

Test Plan: Display revision with disabled ##differential.show-host-field##.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1602
2012-02-13 11:02:46 -08:00
vrana
18ba5fa0ad Separate field for branch in revision
Summary:
The main purpose of this change is to allow selecting the branch by
triple-click.
Plus it is not perfectly clear that the text in brackets means branch.

Test Plan: Display revision.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1585
2012-02-06 17:28:46 -08:00
epriestley
36e72639de Reduce visibility of "Host" and "Path" Differential fields by default
Summary:
See discussion in T838. These fields expose information which it isn't necessary
or useful to expose in the general case.

  - Disable fields by default, allow them to be enabled in config (these fields
were useful for me at Facebook when I had access to all the machines).
  - Remove 'sourcePath' from Conduit methods other than differential.query.
  - Condition 'sourcePath' field in Conduit on the caller being the revision
author. This is a bit hacky but not so awful.

Test Plan:
  - Verified fields are gone by default and restored by configuration.
  - Verified Conduit no longer returns these fields other than
differential.query.
  - Verified field presence/absence according to authorship in
differential.query.
  - Grepped around in arcanist to make sure we aren't relying on sourcePath.
There's a workflow in "arc merge" that technically might hit it, but I think
it's unreachable, definitely irrelvant (we never use source path as a
distinguisher under git/hg, and can't 'arc merge' in SVN) and it's going away
Real Soon Now anyway.

Reviewers: btrahan, arice

Reviewed By: arice

CC: aran, epriestley

Maniphest Tasks: T838

Differential Revision: https://secure.phabricator.com/D1582
2012-02-06 12:14:07 -08:00
vrana
4ee714d404 Use hsprintf() in lint message
Test Plan: Display revision with lint problems

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1578
2012-02-06 10:10:13 -08:00
epriestley
bbf69309c0 Remove the "arc export" field from Differential revisions by default
Summary: This is kind of confusing (you need to specify an export format) and
not very useful now that "arc patch" has gotten pretty good. I'm leaving the
field itself in case installs want to add it back or otherwise depend on it.

Test Plan: Looked at a revision, wasn't told to export it.

Reviewers: nh, btrahan, jungejason

Reviewed By: nh

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1507
2012-01-27 13:02:11 -08:00
vrana
79218b6e47 Factor out DifferentialDiffTableOfContentsView::renderChangesetLink()
Summary:
Links from lint errors for large diffs don't work.
This diff adds TODO for it because I am not sure how to do it.
Move of changeset links rendering to a separate method would be still useful.

Test Plan:
Display ToC of large diff, verify link.
Repeat for small diff.

Reviewers: tuomaspelkonen, epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1476
2012-01-24 10:42:01 -08:00
epriestley
51bc18e93f Improve "next steps" text in Differential
Summary:
  - Even for immutable-history Git workflows, we suggest "arc amend". Instead,
suggest "arc amend" or "arc merge" (ideally we'd know which, but we can't
currently get that information).
  - We suggest "arc amend --revision X", but this is less safe and less simple
than "arc amend", especially after D1480.
  - For Mercurial, suggest "arc merge".

Test Plan: Looked at some "Accepted" revisions.

Reviewers: btrahan, jungejason

CC: aran, epriestley

Maniphest Tasks: T662

Differential Revision: https://secure.phabricator.com/D1481
2012-01-24 09:14:04 -08:00
vrana
6472dbe168 Change fileName to filename
Summary: There are lots of callsites to $changeset->getFilename() so it seemed
easier to rename getFileName() to getFilename() even if it includes database
change. Plus I think that getFilename() is better.

Test Plan:
Alter database.
Open revision.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran

Differential Revision: https://secure.phabricator.com/D1437
2012-01-17 10:50:14 -08:00
epriestley
e2c75d5dc2 Improve Differential handling of disabled users
Summary:
We currently allow you to assign code review to disabled users, but
should not.

Test Plan:
  - Created revisions with no reviewers and only disabled reviewers, was
appropriately warned.
  - Looked at a disabled user handle link, was clearly informed.
  - Tried to create a new revision with a disabled reviewer, was rebuffed.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D1429
2012-01-17 09:27:19 -08:00
vrana
8bed2f4387 Utilize phutil_render_tag()
Test Plan: Display diff with lint error.

Reviewers: pad, epriestley

Reviewed By: epriestley

CC: aran

Differential Revision: https://secure.phabricator.com/D1428
2012-01-16 17:08:21 -08:00
vrana
c6febdfc52 Add link from lint error to code
Test Plan:
Display diff with lint errors
Click on a line number in lint errors overview

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1400
2012-01-14 11:11:17 -08:00
epriestley
d84c0a5be5 Validate all fields in differential.parsecommitmessage
Summary:
  - We currently run ##parseValueFromCommitMessage()## on all fields present in
the message, but not ##validateField()##.
  - This detects value errors (e.g., an invalid reviewer) but not higher-level
errors (e.g., a missing field).
  - This can break the stacked-commits Git mutable history workflow by
recognizing too many commit messages as valid ("multiple valid commit messages,
this is ambiguous").
  - This also gives you some errors ("Missing test plan") too late in "arc diff
--create" (after the diff has been built).

Test Plan:
  - Grepped for validateField() calls, removed a couple of calls that had the
same implementation as the base class.
  - Grepped for other calls to this to make sure I'm not stumbling into
unintended side effects, but it only runs from the diff workflow.
  - Ran "arc diff --create" with an invalid test plan, got a good error early in
the process.
  - Ran "arc diff master" with stacked local commits, got a correct selection of
the intended message.

Reviewers: cpiro, btrahan, jungejason

Reviewed By: cpiro

CC: aran, cpiro

Differential Revision: https://secure.phabricator.com/D1373
2012-01-12 15:20:11 -08:00
Bob Trahan
2a29a51080 Deploy new ArcanistManyWordsAboutDifferentialConstants class from D1328 into
Phabricator

Summary: ...this breaks without D1328.   Used good ole "codemod" to do this
work, with lots of manual edits around 80 chars.

Test Plan: clicked around phabricator tool suite, particular differential, a
bunch

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1351
2012-01-10 11:49:20 -08:00
epriestley
275472d70f Fix appearance of "Differential Revision" on edit interfaces
Summary: The recent change to the field causes us to render "http://junk.com/D"
in some cases, just null the field if there's no data.

Test Plan: Ran "arc diff --create".

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran, btrahan

Differential Revision: https://secure.phabricator.com/D1321
2012-01-05 12:57:40 -08:00
epriestley
28b606394b Fix undefined variable warning in unit test field
Summary:
See D1295. $unit_messages may be undefined.

I'll see if I can improve the visibility of warnings, the red dot in DarkConsole
is easy to miss right now. See T734.

Test Plan: Loaded a revision with no unit failures, didn't receive a warning.

Reviewers: nh, btrahan, jungejason

Reviewed By: btrahan

CC: aran, btrahan

Differential Revision: https://secure.phabricator.com/D1306
2012-01-04 10:20:53 -08:00
Andrew Gallagher
cd0386bf8b Remarkup unittest result messages
Summary: This diffs adds support for marking up unittest result messages.

Test Plan: Verified that links in unittest results were markup'd.

Reviewers: epriestley, jungejason

Reviewed By: epriestley

CC: aran, epriestley, zeeg

Differential Revision: https://secure.phabricator.com/D1298
2012-01-03 18:23:34 -08:00
Nick Harper
4a77906141 Don't show detailed unittest result box if all tests passed
Summary:
When all unit tests pass, a box appears between the unit test results and lint
status (for test failures to go in). This checks if there's anything to put
in that div/ul before putting it on the page.

Test Plan:
Loaded a revision with unit tests OK and saw no box. Loaded a revision with
failing unittests, and saw the same box from before.

Reviewers: tuomaspelkonen, epriestley

Reviewed By: epriestley

CC: jungejason, aran, nh, epriestley

Differential Revision: https://secure.phabricator.com/D1295
2012-01-03 14:58:22 -08:00
vrana
cce212dac7 Link from Blame Revision
Summary: There can be Dxxx, rXXXxxx or even full URL in //Blame Revision// field
so just highlighting it as normal text would work probably best

Test Plan:
Go to https://secure.phabricator.com/D277
You should see a link from //Blame Revision// (if it would be displayed)

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1274
2011-12-22 15:57:53 -08:00
epriestley
9d8b5481ae Emit full URIs to identify Differential revisions
Summary:
  - Previously, used IDs like "33" to match a commit to a Differential revision.
This has a namespacing problem because we now have an arbitrarily large number
of Phabricator installs in the world, and they may want to track commits from
other installs.
  - In Differential, parse raw IDs or full URIs. Emit only full URIs.
  - In Repositories, parse only full URIs.
  - This might cause a few commits to not be picked up in rare circumstances.
Users can fix them with "arc mark-committed". This should be exceedingly rare
because of hash matching.
  - There are some caveats for reparsing older repositories, see comments
inline. I don't think there's much broad impact here.

Test Plan:
  - Created a new revision, got a full URI.
  - Updated revision, worked correctly.
  - Ran unit tests.
  - Monkeyed with "Differential Revision" field.
  - Reviewers: btrahan, jungejason

Reviewers: btrahan, jungejason

Reviewed By: jungejason

CC: aran, epriestley, jungejason

Maniphest Tasks: T54, T692

Differential Revision: 1250
2011-12-20 19:58:22 -08:00
epriestley
bd3c2355e8 Match usernames in any case in commit message object lists (CC, Reviewers, etc.)
Summary:
Allow entry of "CC: alincoln" to match user "ALincoln".

Put both variations in the map and try the exact case version first since we'll
also match email addresses and mailables, and theoretically some mailable might
have the same name as a user, as we're effectively abandoning restriction of
which characters can appear in usernames.

Test Plan: Created a local revision with a reviewer in CrAzY CaPs.

Reviewers: jungejason, btrahan

Reviewed By: jungejason

CC: aran, jungejason

Maniphest Tasks: T697

Differential Revision: 1255
2011-12-20 19:48:56 -08:00
epriestley
c97fcd12bc Share Revision/Task attaching code
Summary:
We have this code in two places; split it into an editor class so we can share
it.

This also fixes some probems with this field not //detaching// tasks properly.

Test Plan:
  - Created a revision with no attached tasks.
  - Attached it to a task.
  - Updated it.
  - Detached it.
  - Used web UI to attach/detach tasks/revisions.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran, btrahan, epriestley

Differential Revision: 1225
2011-12-16 16:13:00 -08:00
epriestley
dfc3506240 Prevent "Maniphest Tasks" field from creating empty comment when revisions are
updated

Summary:
  - If you update a revision with a nonempty "Maniphest Tasks" field, an empty
comment is posted (see T586).
  - The transaction email currently says "Attached revision 'Unknown
Differential Revision'", move attaching to "didWriteRevision()" to make sure the
object has been written.

Test Plan:   - Attached; updated a revision.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran, btrahan

Maniphest Tasks: T685

Differential Revision: 1223
2011-12-16 13:32:52 -08:00
David Cramer
2677217244 Revised unit display to resemble lint output
Reviewers: epriestley

Test Plan: View a differential with unit results and compare display.

Differential Revision: 1216
2011-12-14 16:11:55 -08:00
epriestley
bd12a2b839 Fix a final (?) task field issue which slipped through the cracks
Summary:
Derped this one up; while my testing was successful in preventing runaway
attaching I missed the bit where it doesn't actually work.

This resolves the "Unknown Object" link seen on T661.

Test Plan:
  - Created two new revisions, each attached to a local task.
  - Verified that they attached additively, Maniphest and Differential were
linked to the right places, and nothign else bad happened.

Reviewers: btrahan, fratrik

Reviewed By: fratrik

CC: aran, fratrik, btrahan

Differential Revision: 1181
2011-12-07 08:53:19 -08:00
epriestley
2797903776 Fix bad query edge condition when updating a revision with no attached tasks. 2011-12-04 13:25:32 -08:00
Bob Trahan
83efa6e1c5 make arc diff link maniphest tasks with revisions
Summary:
add "Maniphest Task:" or "Maniphest Tasks:" followed by text that has TX in it.
foreach TX the task will be attached to the revision and the revision will be
attached to the task.  parsing is pretty... ummm, robust such that it will pick
up any TX substring and parse that as a Maniphest Task just fine.   it errors
out if there is not an actual task for TX and otherwise churns along pretty
nicely.

Also, make sure the PhabricatorObjectHandle loads the task ID as the alternateID
since we need that here and it should be that way anyhoo.

Test Plan:
made a diff and in the commit message added Maniphest Task(s): TX combination.
Tried various combinations of TX -- single, multiple with commas, multiple many
lines, single bad, multiple bad, multiple mix of bad and good. verified that the
good tasks were attached to the diff and diff was attached to the good tasks.

Maniphest Tasks: T137

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, btrahan, epriestley

Differential Revision: 1165
2011-12-03 11:34:55 -08:00
epriestley
19f2110e74 Allow "differential.getcommitmessage" to be called without a revision ID in
order to generate a template

Summary: See T614. This allows us to generate an empty template by calling
Conduit, so we can build command-line editing workflows for SVN, Mercurial, and
conservative-Git.

Test Plan: Used web console to invoke Conduit method; got a reasonable empty
template out of it.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran, epriestley, btrahan

Differential Revision: 1156
2011-12-02 07:28:55 -08:00
epriestley
40221feed9 Validate commit message fields on the server side
Summary:
See T643. We have some hard-coded checks in Arcanist for the existence of
'testPlan' and 'title', and don't properly validate those fields on the server.
Add a validation pass in the Conduit-based edit pathway.

In particular, this means that if you disable the "Test Plan" field, Arcanist
won't block you anymore.

Test Plan: Disabled Arcanist checks and ran "arc diff"; got blocked on the
server side.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran, btrahan

Differential Revision: 1153
2011-12-02 07:28:43 -08:00
tuomaspelkonen
a102c9a0fe Allow to resign from an accepted revision when you didn't accept the diff.
Summary: Girish wants to be able to do this.

Test Plan: Checked that I had the option in my sandbox on an accepted diff.

Reviewers: epriestley, jungejason

Reviewed By: jungejason

CC: aran, jungejason, tuomaspelkonen, epriestley

Differential Revision: 1020
2011-10-19 15:27:36 -07:00
epriestley
dc39571d63 Simplify default field loadout for Differential
Summary:
Remove the blame revision, revert plan and lines fields from the default field
loadout. (After D829 this doesn't cause issues where we have bogus dictionary
entries.)

You should add these back to the Facebook configuration since Facebook wants
these fields. However, I want to keep the default stack very light and I never
saw a huge amount of value in these fields at Facebook so I don't think they
make the cut. Sorry, tomo. ;_;

Test Plan: Ran "arc diff" locally.

Reviewers: jungejason, tuomaspelkonen, aran

Reviewed By: aran

CC: aran, tomo, epriestley

Differential Revision: 831
2011-08-18 19:51:32 -07:00
epriestley
0be3db03ee Drive Differential commit message parsing through extensible fields
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
2011-08-18 19:49:39 -07:00
epriestley
735120b842 Fix bug where CC value is ignored on intial parse from commit message
Summary: While I thought this was complicated, there was nothing subtle or
tricky here -- I just misnamed a variable.

Test Plan: Created a revision with default CCs, got CCs instead of nothing.

Reviewers: aran, jungejason, tuomaspelkonen

Reviewed By: aran

CC: aran

Differential Revision: 834
2011-08-18 13:08:28 -07:00
epriestley
ae7488f710 Drive commit message rendering from field specifications
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
2011-08-18 07:20:20 -07:00
epriestley
cd3a3bf759 Make Herald Rules sticky in X-Herald-Rules
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
2011-08-17 10:38:29 -07:00
epriestley
ec0d91a3ff Drive revision update from Conduit via custom fields
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
2011-08-15 10:25:54 -07:00
epriestley
a869dbf45b Implement all field edit interfaces on the custom field schema
Summary:
Moves the revision edit controller to be completely schema-driven.

Depends on D810.

Test Plan: Edited revisions. Entered intentionally invalid values to trigger
error conditions.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason, epriestley
Differential Revision: 810
2011-08-15 10:21:00 -07:00
epriestley
442d1dbeaa Move Differential's remaining field views to extensble field schema
Summary:
Move all the rest of the fields into the custom field schema, for revision
views.

I left a couple of stubs in here (willWriteRevision, didWriteRevision) since I'd
planned to do edits here too, but this diff is sort of big-ish already. I'll do
all the edit fields in the next revision.

Depends on D808.

Test Plan: Viewed, edited and conduit'ed some revisions.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason, epriestley
Differential Revision: 809
2011-08-15 10:20:46 -07:00
epriestley
5038b26018 Move Differential's read-only fields to the extensible field schema
Summary:
Move additional fields (which rely on loading handles) to the extensible field
classes and out of hardcoding in the controller.

Depends on D807.

Test Plan: Viewed, edited, and hit conduit for revisions.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason, epriestley
Differential Revision: 808
2011-08-15 08:39:58 -07:00
epriestley
52ec6c02ee Move Differential's simple fields to the extensible field schema
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
2011-08-15 08:39:48 -07:00
epriestley
7aa1eff383 Expose Differential auxiliary fields in Conduit
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
2011-08-14 10:43:38 -07:00
epriestley
e196bf5b43 Provide builtin definitions for "Blame Revision" and "Revert Plan" fields
Summary:
This is just to ease transitions for any installs which use these fields (e.g.,
Facebook). I'll write some docs and a migration script once this stuff is a
little more solid, too.

Depends on D800.

Technically these are "better" than the current fields since they show up other
places than the edit screen (derp derp).

Test Plan: Created a field selector which provides these; verified they work by
typing stuff into them and saving the revision.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, epriestley, jungejason
Differential Revision: 801
2011-08-14 10:04:50 -07:00
epriestley
9b3370368d Allow Differential custom fields to appear on edit and view interfaces
Summary: Depends on D798. Extends custom fields and makes the vaguely useful:
they can appear on the edit and view interfaces. This does not integrate them
with commit messages yet; that's more complicated but I plan to do it shortly.
Test Plan: Implemented a custom field per P123, it correctly appears on the edit
interface, persists, validates, and shows up when viewing the revision.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, epriestley, jungejason
Differential Revision: 800
2011-08-14 10:04:37 -07:00
epriestley
dd74903cae Add basic auxiliary field storage for Differential
Summary:
Precursor to building this out to solve T343. This is similar to the Maniphest
fields we landed recently, although I think they're dissimilar enough that it
isn't worth going crazy trying to make them share code, at least for now.

This doesn't really do anything yet, just adds a storage object and a couple of
selector/field indirection classes.

Test Plan: Ran SQL upgrade script, created an aux field.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason
Differential Revision: 798
2011-08-14 10:04:21 -07:00