1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-11 08:06:13 +01:00
Commit graph

1087 commits

Author SHA1 Message Date
vrana
6929ac539e Print Maniphest tasks in Differential e-mail
Test Plan: Send diff with attached task, read e-mail.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Koolvin

Maniphest Tasks: T1128

Differential Revision: https://secure.phabricator.com/D2256
2012-04-17 11:43:15 -07:00
vrana
dd7087f4db Don't send empty testplan in e-mail
Summary: Allowed after D2193.

Test Plan: Disable `differential.require-test-plan-field`, create diff without test plan.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran

Differential Revision: https://secure.phabricator.com/D2252
2012-04-17 10:54:36 -07:00
vrana
4af3bb9f4b Allow View Standalone in Diff preview
Test Plan: Click View Standalone.

Reviewers: epriestley, jungejason

Reviewed By: jungejason

CC: aran

Differential Revision: https://secure.phabricator.com/D2225
2012-04-12 22:35:13 -07:00
vrana
50e3114896 Link to TOC in very large diff link
Test Plan: Visit the link.

Reviewers: epriestley, jungejason

Reviewed By: jungejason

CC: aran

Differential Revision: https://secure.phabricator.com/D2224
2012-04-12 22:04:13 -07:00
epriestley
c458768415 Fix various threading issues, particularly in Gmail
Summary:
  - Add an explicit multiplexing option, and enable it by default. This is necessary for Mail.app to coexist with other clients ("Re:" breaks outlook at the very least, and generally sucks in the common case), and allows users with flexible clients to enable subject variance.
  - Add an option for subject line variance. Default to not varying the subject, so mail no longer says [Committed], [Closed], etc. This is so the defaults thread correctly in Gmail (not entirely sure this actually works).
  - Add a preference to enable subject line variance.
  - Unless all mail is multiplexed, don't enable or respect the "Re" or "vary subject" preferences. These are currently shown and respected in non-multiplex cases, which creates inconsistent results.

NOTE: @jungejason @nh @vrana This changes the default behavior (from non-multiplexing to multiplexing), and might break Facebook's integration. You should be able to keep the same behavior by setting the options appropriately, although if you can get the new defaults working they're probably better.

Test Plan:
Send mail from Maniphest, Differential and Audit. Updated preferences. Enabled/disabled multiplexing. Things seem OK?

NOTE: I haven't actually been able to repro the Gmail threading issue so I'm not totally sure what's going on there, maybe it started respecting "Re:" (or always has), but @cpiro and @20after4 both reported it independently. This fixes a bunch of bugs in any case and gives us more conservative set of defaults.

I'll see if I can buff out the Gmail story a bit but every client is basically a giant black box of mystery. :/

Reviewers: btrahan, vrana, jungejason, nh

Reviewed By: btrahan

CC: cpiro, 20after4, aran

Maniphest Tasks: T1097, T847

Differential Revision: https://secure.phabricator.com/D2206
2012-04-12 09:31:03 -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
vrana
01bd844926 Display cursor hand on line number in revision but not in standalone view
Summary: Partially broken by D2166.

Test Plan:
Hover line number in revision.
Hover line number in standalone view.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran

Differential Revision: https://secure.phabricator.com/D2196
2012-04-10 15:05:54 -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
5f615c1e6e Fix a warning when viewing a revision not attached to a repository
Summary: We'll get a typehint warning on the repository if there's no repository. Check outside the method instead.

Test Plan: Loaded page, no warning.

Reviewers: btrahan, vrana, jungejason

Reviewed By: vrana

CC: aran

Differential Revision: https://secure.phabricator.com/D2194
2012-04-10 13:34:31 -07:00
epriestley
fe9ba6bc67 Improve DifferentialRevisionQuery and add the ability to query by arcanist project
Summary:
  - We currently post-filter by branches, but should do this in SQL. See T799.
  - We currently identify branch-name-matches as being in the working copy even if they belong to a different project (e.g., two different projects with commits on the branch "master"). See T1100.
  - Denormalize branch and project information into DifferentialRevision.
  - Expose project information in the API.

Test Plan: Ran conduit API queries with branches and arc project IDs, got reasonable results.

Reviewers: btrahan, vrana, jungejason

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1100, T799

Differential Revision: https://secure.phabricator.com/D2190
2012-04-10 12:51:34 -07:00
vrana
347bc357fd Display Browse in Diffusion and Open in Editor links in commit detail
Test Plan:
/rX1
Browse in Diffusion
Open in Editor

Reviewers: epriestley

Reviewed By: epriestley

CC: aran

Differential Revision: https://secure.phabricator.com/D2180
2012-04-10 09:54:38 -07:00
epriestley
32f12d1f8f Don't fatal when generating patch emails for diffs with binaries
Summary:
When Phabricator is configured to generate patch email, we'll fatal if the patch contains binaries and is generating to Git because ArcanistBundle can't load the binary data. Provide a callback to load the data. See D2174.

(This may cause us to generate absolutely enormous emails, but you get what you asked for...)

Test Plan: Created a diff with an image under "send git patches" email configuration.

Reviewers: Makinde, btrahan, vrana, jungejason

Reviewed By: Makinde

CC: aran

Differential Revision: https://secure.phabricator.com/D2175
2012-04-09 17:35:01 -07:00
vrana
974b576df0 Fix whitespace 2012-04-09 16:57:17 -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
epriestley
3fdd8c497c Possible fix for T1076, pushing to verify.
I think the issue is that we don't set the left-side changesetID correctly. This seems to work correctly locally, but I'm not sure I got a good repro. Pushing to verify the production test cases provided in T1076.

Auditors: vrana, btrahan
2012-04-07 10:01:28 -07:00
vrana
36ee5dba51 Jump to TOC after Show Diff
Summary: I've found it useful mainly on smaller screen or with lots of comments.

Test Plan: Show Diff

Reviewers: epriestley

Reviewed By: epriestley

CC: aran

Differential Revision: https://secure.phabricator.com/D2122
2012-04-06 09:56:28 -07:00
vrana
1f2028adf0 Render valid HTML
Summary: Also delete some dead code.

Test Plan: /D1

Reviewers: tuomaspelkonen, epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D2120
2012-04-06 09:56:14 -07:00
vrana
d1b7059a2d Open editor from stack trace
Summary:
I've considered that user may have set editor but not checked out Phabricator repositories.
But stack trace is useful mainly for developers.

Test Plan:
Click on path in Unhandled Exception.
Repeat with disabled editor.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran

Differential Revision: https://secure.phabricator.com/D2107
2012-04-04 18:19:14 -07:00
vrana
bc61f36beb Replace elseif by else if
Summary:
Mostly written by me.
Omit external libraries.

Test Plan: http://phabricator.com/docs/phabricator/article/PHP_Coding_Standards.html

Reviewers: epriestley

Reviewed By: epriestley

CC: aran

Differential Revision: https://secure.phabricator.com/D2104
2012-04-04 15:24:47 -07:00
vrana
b8cb52a9da Return $this from shortcuts
Test Plan:
Search for `>setLineWidthFromChangesets(`.
Search for `>loadAndAttachAuxiliaryAttributes(`.

Reviewers: epriestley, jungejason

Reviewed By: jungejason

CC: aran

Differential Revision: https://secure.phabricator.com/D2102
2012-04-04 15:12:20 -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
6a0b25c188 Fix wrong assert_instances_of() added by D2091
Test Plan: Display revision.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran

Differential Revision: https://secure.phabricator.com/D2095
2012-04-03 19:26:05 -07:00
vrana
8813c7be0e Use assert_instances_of() everywhere but Differential and Diffusion
Summary: NOTE: This is not produced by a script so there might be errors. Please review carefully.

Test Plan: Browse around.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran

Differential Revision: https://secure.phabricator.com/D2091
2012-04-03 14:53:20 -07:00
vrana
67e10e60f2 Return $this from setters
Summary:
Most setters returns `$this` but some don't.
I guess it's not by purpose.

Test Plan:
  arc lint

Reviewers: epriestley

Reviewed By: epriestley

CC: aran

Differential Revision: https://secure.phabricator.com/D2085
2012-04-02 18:48:37 -07:00
epriestley
eeec726ded Remove all current Authors / Reviewers from "CCWelcome" mail
Summary:
We'll incorrectly send CCWelcome mail to users who would be added as CCs but are blocked by the new "$dont_add" stuff, for
example when a revision is updated and the user has a Herald rule which triggers them getting CC'd. See D2057.

Potentially a better fix for this would be to have "addCCs" return a list of the CCs it actually added, rather than duplicating the
logic of removing CCs in two places. However, that's not trivial since it's just a wrapper around alterRelationships() which is nasty
and would need a more complicated return type. I think this whole thing will get a refactoring pass at some point -- I want to build a
more generic "associations"-like datastore and replace some of the ad-hoc associations with it. So maybe I can clean it up when that
happens. For now, this should fix the immediate problem.

Test Plan: Updated a revision, didn't get CC welcomed.

Reviewers: vrana, btrahan

Reviewed By: vrana

CC: aran

Differential Revision: https://secure.phabricator.com/D2072
2012-04-02 10:41:03 -07:00
epriestley
5945546440 Unify Differential/Maniphest/Diffusion styles and allow commits to be flagged explicitly
Summary:
  - Differential, Maniphest and Diffusion use slightly different styles for the object detail panels.
  - Instead, use the same styles and CSS.
  - Add object actions to Diffusion, including "Flag".

Test Plan: Looked at revisions, tasks and commit. Flagged and unflagged commits.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, epriestley

Maniphest Tasks: T1041

Differential Revision: https://secure.phabricator.com/D2062
2012-03-30 14:12:10 -07:00
vrana
62053a39e3 Don't add author and reviewers to CCs in Herald
Summary:
Herald rules are adding CC also for Author and Reviewer.
See also D1397.
I was considering also just don't displaying the extra CC but this is probably better.

There are still cases where there could be reviewer in CC (e.g. by making reviewer from CC or by direct edit) but I think it's not a big problem.

Beeing both Reviewer and CC can be actually useful (e.g. if you resign than you still are in CC) but it's not that useful to justify this:

  Author: vrana
  Reviewers: epriestley
  CCs: vrana, epriestley

Test Plan: Comment on revision where I am author.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D2057
2012-03-30 10:15:43 -07:00
epriestley
b38047006b Show other open revisions affecting the same files in Differential
Summary:
I think this feature is probably good, but Differential is also really starting to get a lot of stuff which is not the diff in it. Not sure how best to deal with that.

The mixed table styles are also pretty ugly.

So I guess this is more feedback / proof-of-concept, I think I want to try to improve it somehow before I land it.

Test Plan: Looked at some diffs, some had an awkward, ugly list of diffs affecting the same files.

Reviewers: bill, aran, btrahan

Reviewed By: aran

CC: aran, epriestley

Maniphest Tasks: T829

Differential Revision: https://secure.phabricator.com/D2027
2012-03-30 10:13:08 -07:00
epriestley
ec8b445727 If there are no flagged revisions, don't show anything on "Active Revisions"
Test Plan: Looked at /differential/ with and without flags.

Reviewers: nh, btrahan

Reviewed By: nh

CC: aran, epriestley

Maniphest Tasks: T1055

Differential Revision: https://secure.phabricator.com/D2044
2012-03-28 20:03:16 -07:00
epriestley
29d8fc04e5 Improve inline comment previews
Summary:
  - When an inline comment preview corresponds to an inline comment on the page, link to it. Just punt in the tough case where the inline is on some other page.
  - In "haunted" mode, "z" now toggles through three modes: normal, comment area only, and comment + previews.

Test Plan:
  - Viewed visible and not-visible inline comment previews, clicked "View" links.
  - Tapped "z" a bunch to toggle haunt modes.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, epriestley

Maniphest Tasks: T517, T214

Differential Revision: https://secure.phabricator.com/D2041
2012-03-28 10:11:41 -07:00
epriestley
7ad68e63e4 Add "Flags" to allow users to collect the things they love
Summary:
Flags are a personal collection of things you want to take a look at later. You can use several different colors and add notes.

Not really sure if this is actually a good idea or not but it was easy to build.

Planned features:

  - Allow Herald rules to add flags.
  - In the "edit flag" dialog, have a "[x] Subscribe Me" checkbox that CCs you.
  - Support Diffusion.
  - Support Phriction.
  - Always show flags on an object if you have them (in every view)?
  - Edit dialog feels a little heavy?
  - More filtering in /flag/ tool.
  - Add a top-level links somewhere?

Test Plan: Added, edited and removed flags from things. Viewed flags in flag view.

Reviewers: aran, btrahan

Reviewed By: btrahan

CC: aran, epriestley, Koolvin

Maniphest Tasks: T1041

Differential Revision: https://secure.phabricator.com/D2024
2012-03-27 16:22:40 -07:00
epriestley
43f0076615 Put inline comment anchors above the comments
Summary: See T955. We jump to an awkard place right now; jump above the comment instead.

Test Plan: Clicked inline comment anchor links.

Reviewers: Makinde, btrahan

Reviewed By: Makinde

CC: aran, epriestley

Maniphest Tasks: T955

Differential Revision: https://secure.phabricator.com/D2029
2012-03-26 21:57:51 -07:00
epriestley
89c66cbbd4 Add an "Explicit-CCs" header to Differential
Summary: This header allows recipients to distinguish between CCs generated by Herald and CCs generated by humans.

Test Plan: Created a Herald rule to add a bunch of CC's to every revision. Created a revision. Added some CCs manually. Verified that only manual CCs appeared in the "Explicit" header.

Reviewers: btrahan, vrana

Reviewed By: btrahan

CC: aran, epriestley

Maniphest Tasks: T808

Differential Revision: https://secure.phabricator.com/D2018
2012-03-26 09:55:38 -07:00
epriestley
b440c95e9b Make "subscribed" filter in Differential accept any mailable
Summary:
In the Differential revision list views:

  - Allow you to filter by mailables (notably, mailing lists).
  - Allow you to filter by user (including disabled users).

Test Plan: Filtered by a mailing list.

Reviewers: btrahan, nh

CC: aran, epriestley

Maniphest Tasks: T1031

Differential Revision: https://secure.phabricator.com/D1994
2012-03-23 08:45:48 -07:00
epriestley
315870d56a Fix various newline problems in the difference engines
Summary: I'll mark this one up inline since it's all separate bugs.

Test Plan:
  - Created a diff with eight changes: (newline absent -> newline present, newline present -> newline absent, newline present -> newline present, newline absent -> newline absent) x (short file with change near end, long file with change near middle).
  - Viewed diff in Ignore All, Ignore Most, Ignore Trailing and Show All whitespace modes.
  - All 32 results seemed sensible.
  - Really wish this stuff was better factored and testable. Need to fix it. :(

Reviewers: vrana, btrahan

Reviewed By: btrahan

CC: aran, epriestley

Maniphest Tasks: T1030

Differential Revision: https://secure.phabricator.com/D1992
2012-03-22 14:13:48 -07:00
epriestley
85f19e16dc Fix the "Browse in Diffusion" URI in Differential
Summary: Apparently I spent like a good month copy/pasting slightly different versions of this logic all over the codebase.

Test Plan: Selected "View Options -> Browse in Diffusion" for a chagneset, got a URI with a branch name in it under Git.

Reviewers: vrana, btrahan

Reviewed By: btrahan

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1993
2012-03-22 13:55:35 -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
vrana
07b60b2016 Require valid class for certain config settings
Summary:
It is now possible to set config setting requiring class of certain implementation to something completely else.
The consequence is that your Phabricator may stop working after update because you didn't implement some new method.

This diff validates the class upon usage.
It throws exception which is better than fatal thrown currently after calling undefined method.

Better solution would be to validate classes when setting the config but it would be too expensive - respective class definitions would have to be loaded and checked by reflection.

I was also thinking about some check script but nobody would run it after changing config.

The same behavior should be implemented for these settings:

- metamta.mail-adapter
- metamta.maniphest.reply-handler
- metamta.differential.reply-handler
- metamta.diffusion.reply-handler
- storage.engine-selector
- search.engine-selector
- differential.field-selector
- maniphest.custom-task-extensions-class
- aphront.default-application-configuration-class
- controller.oauth-registration

Test Plan:
Send comment, verify that it pass.
Change `metamta.differential.reply-handler` to incompatible class, verify that sending comment shows nice red exception.
Set `metamta.differential.reply-handler` to empty string, verify that it throws.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1919
2012-03-21 14:14:01 -07:00
nileema
238b403509 Allow the users to view their unsubmitted inline comments in one view
Summary:
1. Created a filter for user comments that are not submitted yet
2. surface this information to the user by providing a "Draft revisions" link on the differential home page.

Test Plan: tested on one of the diffs

Reviewers: nh, epriestley

Reviewed By: epriestley

CC: aran, epriestley, jungejason

Differential Revision: https://secure.phabricator.com/D1927
2012-03-20 16:16:13 -07:00
epriestley
1227c8d5da Add "View Options" dropdown to Differential / Audit
Summary:
Depends on D1921. Depends on D1899.

Add the "View Options" dropdown menu to Diffusion, with options like "show standalone", "show raw file", "show all", etc.

Test Plan: Viewed commits in Differential and Diffusion.

Reviewers: davidreuss, nh, btrahan

Reviewed By: btrahan

CC: aran, epriestley

Maniphest Tasks: T904

Differential Revision: https://secure.phabricator.com/D1932
2012-03-19 19:57:41 -07:00
epriestley
6a13b3ea7e Separate the inline comment summary element into a separate view
Summary:
  - Affects the "Inline Comments" summary table which appears in comments that have attached inlines in the discussion threads in Differential.
  - Prepares for inclusion in Diffusion.
  - No application changes (minor CSS), just factors code better.
  - Simplify/separate CSS.

Test Plan: Looked at on-diff and off-diff comment summaries in Differential, display looked correct.

Reviewers: davidreuss, nh, btrahan

Reviewed By: davidreuss

CC: aran, epriestley

Maniphest Tasks: T904

Differential Revision: https://secure.phabricator.com/D1928
2012-03-19 19:45:16 -07:00
epriestley
3db02a584c Fix some warnings about markup engines in Audit/Diffusion inline comments
Summary:
Split from D1921.

  - DifferentialChangesetParser doesn't have this property declared.
  - We weren't providing a markup engine, which caused some warnings.
  - This would cause failures if comment caches weren't present. Currently, they always will be though unless someone has wiped them explicitly in the DB.

Test Plan: Viewed a diff with inline comments, didn't get any warnings in the log.

Reviewers: nh, vrana, btrahan

Reviewed By: btrahan

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1925
2012-03-19 19:17:59 -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
epriestley
57fd4bc68b Kind-of-terrible (?) oncopy handler
Summary: Works in Safari, Firefox, Chrome.

Test Plan: Copied some text, threw up a little in my mouth.

Reviewers: aran, tuomaspelkonen, tomo, rstout, btrahan

Reviewed By: aran

CC: aran, epriestley, ddfisher

Maniphest Tasks: T145, T995

Differential Revision: https://secure.phabricator.com/D244
2012-03-15 19:04:59 -07:00
epriestley
cc3e52b8d9 Unfinal "DifferentialReplyHandler"
Summary: Facebook extends this, just unfinal it for now and we can figure out the right long-term fix at our leisure.

Test Plan: Inspection.

Reviewers: vrana, btrahan

Reviewed By: vrana

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1917
2012-03-15 14:16:32 -07:00
vrana
4fa9798c4a Allow selecting text when creating inline comment
Summary:
It is currently not possible to select source code covered by reticle when creating comment.
This diff hides reticle on mouseout from reply area.

Test Plan:
Hover inline comment, verify that reticle is displayed.
Reply, verify that reticle is displayed when mouseover reply, hidden otherwise.
Repeat for create.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1851
2012-03-15 11:02:25 -07:00
vrana
a70466a7fd Don't display "Loading..." for coverage which will never load
Summary: In very large diffs.

Test Plan:
Display very large diff.
Display normal diff.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1918
2012-03-15 10:59:00 -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
vrana
fa687e1773 Allow selecting author by double click in inline comment
Summary: Otherwise line number gets selected too.

Test Plan: Double click author in inline comment.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1912
2012-03-15 07:13:26 -07:00
vrana
d8bb59bd9a Don't display old lines on right side in diff
Summary: I've done the same stupid copy/paste mistake twice in one day :-(.

Test Plan: Display diff with no newline at end of file in new file.

Reviewers: davidreuss, epriestley

Reviewed By: davidreuss

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1911
2012-03-15 05:45:00 -07:00
vrana
0c20d7900e Highlight files with no newline at end of file
Summary: Text "No newline at end of file" was sent to highlighter causing error with most languages.

Test Plan: Display diff containing file with no newline at end of file.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1903
2012-03-14 18:31:35 -07:00
vrana
1464c0f2eb Unhighlight " No newline at end of file" in Differential
Summary:
It looks like there is really this text written e.g. at https://secure.phabricator.com/D1896#0a6a1957

I am not sure that it is the only place which needs to be fixed.

Test Plan: Display diff with no newline at end of file in Differential.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1900
2012-03-14 17:58:14 -07:00
epriestley
900190b2fe Add inline comments to Diffusion/Audit
Summary:
  - Add inline comments to Audits, like Differential.
  - Creates new storage for the comments in the Audits database.
  - Creates a new PhabricatorAuditInlineComment class, similar to DifferentialInlineComment.
  - Defines an Interface which Differential and Audit comments conform to.
  - Makes consumers of DifferentialInlineComments consume objects which implement that interface instead.
  - Adds save

NOTE: Some features are still missing! Wanted to cut this off before it got crazy:

  - Inline comments aren't shown in the main comment list.
  - Inline comments aren't shown in the emails.
  - Inline comments aren't previewed.

I'll followup with those but this was getting pretty big.

@vrana, does the SQL change look correct?

Test Plan:
  - Created, edited, deleted, replied to, reloaded and saved inline comments in Diffusion, on the left and right side of diffs.
  - Created, edited, deleted, replied to, reloaded and saved inline comments in Differentila, on the left and right side of primary and diff-versus-diff diffs.

Reviewers: btrahan, vrana

Reviewed By: btrahan

CC: aran, epriestley

Maniphest Tasks: T904

Differential Revision: https://secure.phabricator.com/D1898
2012-03-14 12:56:01 -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
f158b32a54 Minor, formalize changeset response class. 2012-03-12 21:39:05 -07:00
epriestley
b537d1ec01 Show a tooltip about code coverage
Summary:
  - Show a tip in the margin about what coverage colors mean.
  - Highlight the line when mousing over coverage.
  - Randomly change the colors to different colors.
  - Fix a bug with "show more" that I introduced with the other coverage diff (oops!)

Test Plan: Moused over coverage things.

Reviewers: tuomaspelkonen, btrahan

Reviewed By: tuomaspelkonen

CC: aran, epriestley

Maniphest Tasks: T965

Differential Revision: https://secure.phabricator.com/D1874
2012-03-12 20:04:12 -07:00
epriestley
392d12f1fc When marking a line changed because of internal whitespace in "Ignore Most", intraline diff it
Summary:
Currently, we mark some lines that otherwise count as "unchanged" to be "changed" when they have internal whitespace changes.

However, we've already excluded them from the intra-line diff algorithm. Unset the flag so they can get intra-line diffed.

Test Plan: Viewed a change with internal whitespace changes, internal whitespace changes were intraline diffed.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, epriestley

Maniphest Tasks: T970

Differential Revision: https://secure.phabricator.com/D1871
2012-03-12 18:20:51 -07:00
epriestley
f6fda5ec01 Minor, address feedback frmo D1864. 2012-03-12 17:10:05 -07:00
epriestley
85bdcbdd43 Show coverage percentages in table of contents
Summary:
Rough cut -- this needs style / color / tooltips, etc. Show raw coverage and "modified coverage" (coverage on lines you touched) in the table of contents.

https://secure.phabricator.com/file/data/id3apce5p5gevkee6tg2/PHID-FILE-kxcxlbsej454t4xiht2o/Screen_Shot_2012-03-12_at_3.30.30_PM.png

Test Plan: See screenshot above.

Reviewers: tuomaspelkonen, btrahan, zeeg

Reviewed By: tuomaspelkonen

CC: aran, epriestley

Maniphest Tasks: T965

Differential Revision: https://secure.phabricator.com/D1864
2012-03-12 17:06:55 -07:00
epriestley
09c1bd34f1 Add an "ignore all" whitespace mode
Summary:
We used to have "ignore all", but it became "ignore most". It does not ignore in-line whitespace changes or whitespace changes in language where whitespace is marked semantic.

Add an explicit "ignore all" option.

Test Plan: Used "ignore all" to view a change with heuristically-detected semantic whitespace, the change was ignored.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, epriestley

Maniphest Tasks: T970

Differential Revision: https://secure.phabricator.com/D1865
2012-03-12 17:06:36 -07:00
epriestley
43bd76336c Use Javelin placeholders and new sorting rules broadly; consolidate tokenizer construction code
Summary:
  - We have three nearly-identical blocks of Tokenizer construction code; consolidate them into Prefab.
  - Add placeholder support.
  - Augment server-side stuff to specify placeholder text.

Test Plan: Verified behavior of Differential edit tokenizers, Differential comment tokenizers, Maniphest edit tokenizers, Maniphest comment tokenizers, Maniphest filter tokenizers, Differential filter tokenizers, Owners filter tokenizers, Owners edit tokenizers, Herald edit tokenizers, Audit filter tokenizers.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, epriestley

Maniphest Tasks: T772, T946

Differential Revision: https://secure.phabricator.com/D1844
2012-03-09 15:46:39 -08:00
epriestley
b2890eeb0e Add "final" to all Phabricator "Controller" classes
Summary:
These are all unambiguously unextensible. Issues I hit:

  - Maniphest Change/Diff controllers, just consolidated them.
  - Some search controllers incorrectly extend from "Search" but should extend from "SearchBase". This has no runtime effects.
  - D1836 introduced a closure, which we don't handle correctly (somewhat on purpose; we target PHP 5.2). See T962.

Test Plan: Ran "testEverythingImplemented" unit test to identify classes extending from `final` classes. Resolved issues.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, epriestley

Maniphest Tasks: T795

Differential Revision: https://secure.phabricator.com/D1843
2012-03-09 15:46:25 -08:00
vrana
8b304870c4 Use only the first line of update comment in revision history description
Summary:
More complicated updates deserves title and explanation.
This diff uses only the first line from comment in diff's description.
It also removes the truncating at 80 chars which looks as an error.

Test Plan:
  var_dump(preg_replace('/\n.*/s', '', "abc"));
  var_dump(preg_replace('/\n.*/s', '', "abc\ndef"));
  var_dump(preg_replace('/\n.*/s', '', "abc\ndef"));
  var_dump(preg_replace('/\n.*/s', '', "abc\ndef\nghi"));
  var_dump(preg_replace('/\n.*/s', '', "\nabc")); // empty string

Display revision with 255 chars description in update history - it looks OK.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1812
2012-03-07 14:40:59 -08:00
epriestley
8dfe8e84f0 Improve Diffusion parsing of submoudule changes
Summary: We currently parse these as directory changes and discard them. Instead, parse them as a new "SUBMODULE" type of change.

Test Plan:
  - Reparsed a commit which changes submodules and verified it parses correctly.
  - Reparsed a commit which adds submodules and verified it parses correctly.

Reviewers: btrahan, kdeggelman

Reviewed By: btrahan

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1815
2012-03-07 14:24:43 -08: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
76fd9a2d28 Reduce laziness for "Mark Committed"
Summary:
  - Enforce proper workflow rules.
  - Fix a derp-bug with patches.

Test Plan:
  - Tried to mark a revision I didn't own.
  - Tried to mark a revision already marked committed.
  - Tried to mark a revision otherwise not accepted.
  - Verified daemon can override workflow rules and mark from arbitrary states.

Reviewers: btrahan, Makinde

Reviewed By: Makinde

CC: aran, epriestley

Maniphest Tasks: T948

Differential Revision: https://secure.phabricator.com/D1809
2012-03-07 10:20:17 -08:00
epriestley
62f18914db Add "mark committed" action to the web UI
Summary: When a revision is accepted, allow users to manually mark it committed if there's a daemon/tracking problem. This shouldn't happen in most installs but we have less-robust support for Mercurial and some installs may not be fully configured.

Test Plan: Marked a revision committed.

Reviewers: btrahan, Makinde

Reviewed By: Makinde

CC: aran, epriestley

Maniphest Tasks: T948

Differential Revision: https://secure.phabricator.com/D1808
2012-03-07 09:47:31 -08:00
epriestley
1706212e24 Minor, ensure auditors is an array. 2012-03-06 19:46:26 -08:00
vrana
21d5953093 Add filter for abandoned revisions
Test Plan: /differential/filter/reviews/?status=abandoned

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Maniphest Tasks: T767

Differential Revision: https://secure.phabricator.com/D1799
2012-03-06 16:54:03 -08:00
vrana
ad58491c6c Support /differential/filter/<filter>/<username>/
Summary: NOTE: I didn't add BC for ?phid=.

Test Plan:
/differential/
/differential/filter/active/
/differential/filter/active/epriestley/
/differential/filter/active/x/ - 404
/differential/filter/revisions/?status=open - search for epriestley
/differential/filter/revisions/epriestley/?status=open
/p/jakubv/

Reviewers: epriestley, nh

Reviewed By: epriestley

CC: aran, epriestley

Maniphest Tasks: T900

Differential Revision: https://secure.phabricator.com/D1797
2012-03-06 15:21:59 -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
48f2730110 Change inline edit comment confirm button caption to Ready
Summary:
Caption Submit is quite confusing because it doesn't actually make the data visible to other users.
Especially when comment confirm button is also called Submit in serious mode.

Test Plan: Add inline comment.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1796
2012-03-06 11:58:41 -08:00
epriestley
831133e880 Minor, fix some obvious derp from the attachment patch. 2012-03-05 16:44:24 -08:00
vrana
aa00e2b2e4 Respect Status and Order in Filter Revisions
Summary:
Filter Revisions button currently resets Status and Order fields.
I've rewritten it to GET form because it doesn't perform any action.
It fixed the problem along the way.

Test Plan:
/differential/filter/revisions/
Status: Open.
Filter Revisions.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1771
2012-03-05 13:32:59 -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
vrana
34efbc8eb4 Unify terminology in Differential overview
Summary:
Deja vu: D1472

Blame Rev: rP92f3ff

Test Plan: /differential/filter/revisions/

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1770
2012-03-04 03:17:22 -08:00
epriestley
0962980fef Add an option to inline diffs up to a certain size in emails
Summary:
We already generate patches, but currently attach them. Allow them to be inlined instead (optionally, up to a certain size).

Also allow selection between unified and git patches.

Test Plan: Set these options in my local config, sent out a diff.

Reviewers: btrahan, Makinde

Reviewed By: Makinde

CC: aran, epriestley

Maniphest Tasks: T874

Differential Revision: https://secure.phabricator.com/D1759
2012-03-03 11:05:19 -08:00
vrana
1febd32a76 Don't hide columns with stats from previous age, hide columns with no data
Summary:
This does two things:

- don't fill '-' for columns where there are data from previous week
- completele hide columns if there are now new data so that noobs don't have a
55 years of history

Test Plan:
View my commits stats.
View my requested changes stats.
View epriestley FB stats.

Reviewers: vii, epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1750
2012-03-01 22:09:24 -08:00
vrana
03a108e121 Don't throw exception in diff versus file removed from revision
Test Plan: Display a revision where a file was removed compared to previous
diffs, like https://secure.phabricator.com/D1724?vs=3008&id=3009.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1754
2012-03-01 18:01:47 -08:00
epriestley
b18fa48c89 Move documentation about X-Herald-Rules to an article
Summary:
  - We have a lot of headers now; document them.
  - Remove the one random protip from like 3 years ago from all Differential
mail.

Test Plan: generated; read documentation

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, epriestley

Maniphest Tasks: T931

Differential Revision: https://secure.phabricator.com/D1748
2012-03-01 17:23:18 -08:00
vrana
c0c5b9bb64 Add Edit All link to Differential revision
Summary:
Some text editors support opening multiple files at once.
I've used space as paths separator which may be compatible with some other
editors (I didn't tried any other though).
Note: This approach is incompatible with spaces in paths.
I am fine with changing it to anything else to support such paths or more
editors.
Probably the cleanest solution (yet still incompatible with most editors) would
be to use something like ##editor://open/?file=A&line=1&file=B&line=2## but it
would require also changing the way how it's configured and I think it's not
worth it.
BTW, I've used a hacky bookmarklet for this feature before.

Deleted or added paths may not exist in users filesystem but we don't know which
so the button tries to open everything.

Test Plan:
Click Edit All.
Delete Editor Link in settings, verify that the button is missing.
View diff without revision, verify that the button is missing.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1741
2012-03-01 10:09:32 -08:00
Nick Harper
5042667b96 Improved warning message when accepting diff with skipped lint or unit
Summary:
Some people find the current message stating "This diff has Lint/Unit Test
Problems" confusing if the unit tests or lint was skipped. This revision
clarifies those messages.

Test Plan:
Started to accept a revision with skipped lint and unit tests, and saw the new
message.

Reviewers: epriestley, btrahan, jungejason

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1738
2012-02-29 22:39:37 -08:00
vrana
f6d4cc4896 Send custom headers for author, reviewer and cc in Differential e-mails
Summary:
I want to flag messages which require an immediate action from me in e-mail
client.
It is currently not possible because Author and Reviewers fields are both in
To:.
So the filtering rule cannot recognize if I am the person who should take the
action.

This diff adds these headers:

- X-Differential-Author
- X-Differential-Reviewers
- X-Differential-CCs

Test Plan:
Send comment to the diff.
Verify X-Differential-* headers.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Maniphest Tasks: T808

Differential Revision: https://secure.phabricator.com/D1724
2012-02-29 14:33:18 -08:00
epriestley
21f0aba701 Use an inline dialog element for inline comments in Differential
Summary:
The current approach of using a modal overlay dialog to create/edit inline
comments is pretty silly. Use an inline textarea instead.

This element isn't perfect and we have some mild modalness issues, but I think
it's better than the silly thing we've got going on right now. We can keep
poking it as people break it.

Test Plan:
  - Created comments; submitted and undid them in empty and nonempty states.
Used undo for nonempty states + cancel.
  - Edited comments; saved and canceled them. Used undo for changed state.
  - Replied to comments; yada yada as above.
  - Deleted comments.
  - Did various modal trickery where I clicked "Reply" on something else with a
dialog already up, this very mildly glitches but I think it's not a big issue.

Reviewers: vrana, btrahan, Makinde, nh

Reviewed By: vrana

CC: aran, epriestley

Maniphest Tasks: T431

Differential Revision: https://secure.phabricator.com/D1716
2012-02-29 14:28:48 -08:00
epriestley
3289059452 Unify "toggle buttons" controls
Summary: This control is a very thin shell right now with Maniphest/Differential
code duplication; unify the implemenations better for use in Audit.

Test Plan: Clicked toggle buttons in Differential and Maniphest.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1700
2012-02-27 12:59:05 -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
282d6e5ffa Unify Maniphest + Differential comment styles
Summary:
I want to add comments to commits, and they should obviously share code with the
nearly-identical comments in Maniphest and Differential. Unify code/style as
much as possible.

This program made possible by a generous grant from D1513.

Test Plan:
  - Looked at a bunch of different Differential and Maniphest comments; they
appeared to render identically to how they looked before.
  - Tested some edge cases like anchors and "show details" on description edits
in Maniphest.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran, epriestley

Maniphest Tasks: T904

Differential Revision: https://secure.phabricator.com/D1686
2012-02-24 13:02:35 -08:00
epriestley
bf3dd8663c Add "buoyant" headers to Differential
Summary:
As you scroll through a diff, add a fixed-position header to the top of the
document to provide context. This is particularly useful with keyboard
navigation.

The technical implementation is that we seed the document with invisible
markers. When the user scrolls past one, we show a header with that text until
they scroll past another.

Test Plan:
Scrolled through a revision, was presented with context.

https://secure.phabricator.com/file/data/5xhh2jmoon6ukr5qjkh3/PHID-FILE-463ituscyhyw7utnox7m/Screen_Shot_2012-02-22_at_2.48.19_PM.png

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, epriestley

Maniphest Tasks: T696

Differential Revision: https://secure.phabricator.com/D1673
2012-02-23 12:26:14 -08:00
epriestley
17965cc8be Improve display of "Added Reviewers" and "Added CCs" in Differential, link diffs
Summary:
When a comments add reviewers or CCs, we just dump that sort of nastily into the
body. Put it in the header like Maniphest instead.

Also, record the diff associated with "update" actions and link to it (T871).

Test Plan: {F8546} {F8547}

Reviewers: btrahan, davidreuss

Reviewed By: davidreuss

CC: aran, epriestley

Maniphest Tasks: T871

Differential Revision: https://secure.phabricator.com/D1659
2012-02-22 08:03:48 -08:00
John Fremlin VII
583cca0d7c Various statistics about revisions at /differential/stats/revisions/
Summary:
Show some statistics, like number of revisions, number of
revisions per week, lines per revision, etc. for phrivolous amusement.

Test Plan:
 - Went to /differential/stats/revisions/
Numbers seem right
 - Clicked 'Accepted'
Again
 - Changed to another user with long history
Load time was not too long though delay noticeable
 - Clicked 'Requested changes to'
User was preserved, looks good

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1643
2012-02-21 12:13:18 -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
epriestley
9748520b0e Add "overflow: auto" to all comment boxes
Summary: Set these all to "overflow: auto".

Test Plan:
Made comments like "MMMMMMM..." in:

  - Differential comment preview.
  - Differential comment (saved).
  - Maniphest comment preview.
  - Maniphest comment (saved).
  - Differential inline comment draft.
  - Differential inline comment preview.
  - Differential inline comment (saved).

Also tested code blocks.

Reviewers: nh, btrahan, jungejason

Reviewed By: jungejason

CC: aran, epriestley

Maniphest Tasks: T883

Differential Revision: https://secure.phabricator.com/D1641
2012-02-19 09:07:35 -08:00
epriestley
bfea830d09 Add email preferences to receive fewer less-important notifications
Summary:
A few similar requests have come in across several tools and use cases that I
think this does a reasonable job of resolving.

We currently send one email for each update an object receives, but these aren't
always appreciated:

  - Asana does post-commit review via Differential, so the "committed" mails are
useless.
  - Quora wants to make project category edits to bugs without spamming people
attached to them.
  - Some users in general are very sensitive to email volumes, and this gives us
a good way to reduce the volumes without incurring the complexity of
delayed-send-batching.

The technical mechanism is basically:

  - Mail may optionally have "mail tags", which indicate content in the mail
(e.g., "maniphest-priority, maniphest-cc, maniphest-comment" for a mail which
contains a priority change, a CC change, and a comment).
  - If a mail has tags, remove any recipients who have opted out of all the
tags.
  - Some tags can't be opted out of via the UI, so this ensures that important
email is still delivered (e.g., cc + assign + comment is always delivered
because you can't opt out of "assign" or "comment").

Test Plan:
  - Disabled all mail tags in the web UI.
  - Used test console to send myself mail with an opt-outable tag, it was
immediately dropped.
  - Used test console to send myself mail with an opt-outable tag and a custom
tag, it was delivered.
  - Made Differential updates affecting CCs with and without comments, got
appropriate delivery.
  - Made Maniphest updates affecting project, priority and CCs with and without
comments, got appropriate delivery.
  - Verified mail headers in all cases.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, epriestley, moskov

Maniphest Tasks: T616, T855

Differential Revision: https://secure.phabricator.com/D1635
2012-02-17 22:57:07 -08:00
epriestley
da1d57b60a When viewing raw file content in Differential, cache it into the File tool
before displaying it

Summary:
@alok reported a vulnerability where Flash will run carefully-crafted plain text
files.

When the user requests a raw file, cache it into Files if it isn't already
there. Then redirect them to Files. This solves the problem by executing the
SWF/TXT with CDN-domain permissions, not content-domain permissions, provided
the install is correctly configured. (Followup diff coming to make this more
universally true.)

NOTE: We'll still show raw data in Diffusion. The barrier to XSS here is much
higher (you need commit access) but I'll do something similar there. We aren't
vulnerable in Paste, since we already use Files.

Test Plan: Clicked "View Old File", "View New File" in an alt-domain
configuration, got redirected to a cookie-free domain before being delivered the
response.

Reviewers: btrahan, alok

Reviewed By: btrahan

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1607
2012-02-14 17:00:20 -08:00
epriestley
cd651001b6 Add a contextual "scope" dropdown for searches
Summary: Add a "Search for ... in (document group)" thing that picks the current
scope based on the current application.

Test Plan: Conducted searches in several browsers.

Reviewers: btrahan, skrul

Reviewed By: btrahan

CC: aran, epriestley

Maniphest Tasks: T858

Differential Revision: https://secure.phabricator.com/D1610
2012-02-14 17:00:12 -08:00
vrana
6a17de65df Ability to add reviewers while requesting review
Summary: It makes perfect sense to add more reviewers while requesting review.

Test Plan:
Request review. Verify that Add Reviewers field shows and works.
Add some reviewer. Verify that comment preview works.
Submit. Verify that reviewers are saved and displayed.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1473
2012-02-14 15:14:12 -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
95feb31fbf Whitespace parameter on Show Raw File is useless
Test Plan: Show Raw File

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1600
2012-02-11 09:13:19 -08:00
vrana
cd22d837cb Revive added reviewers and ccs
Test Plan: Display revision with added reviewers.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1598
2012-02-10 11:48:57 -08:00
epriestley
3f46d30e8f Replace home directory list with a dashboard
Summary:
Rough cut that still needs a lot of polish, but replace the directory list with
more of a dashboard type thing:

  - Show "Unbreak Now", triage-in-your-projects, and other stuff that you're
supposed to deal with, then feed.
  - Move tools a click a way behind nav -- this also lets us put more stuff
there and subtools, etc., later.
  - Remove tabs.
  - Merge the category/item editing views.
  - I also added a light blue wash to the side nav, not sure if I like that or
not.

Test Plan:
  - Viewed all elements in empty and nonempty states.
  - Viewed applications, edited items/categories.

Reviewers: btrahan, aran

Reviewed By: btrahan

CC: aran, epriestley, davidreuss

Maniphest Tasks: T21, T631

Differential Revision: https://secure.phabricator.com/D1574
2012-02-07 16:04:48 -08:00
vrana
3e3f73235c Break all old differential comment anchors
Test Plan: Display revision, verify text, click on links.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Maniphest Tasks: T845

Differential Revision: https://secure.phabricator.com/D1591
2012-02-07 15:19:39 -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
15f6216634 Fix displaying of raw files in Differential
Summary:
See D1533#5.
Also deduplicates logic of what is stored to blob in ArcanistDiffWorkflow.

Blame Rev: D1533

Test Plan:
Display raw version of text file.
Display raw version of image.

Reviewers: epriestley, nh

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1583
2012-02-06 12:05:37 -08:00
vrana
d65f62d055 Use constants in DifferentialRevisionUpdateHistoryView
Test Plan: Display diff with whitespace changes.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1581
2012-02-06 11:24:06 -08:00
vrana
1ab2a88605 Reorganize escaping in DifferentialRevisionUpdateHistoryView
Summary:
Escaped $id is compared with non-escaped $max_id.
Escaped $id is escaped again in phutil_render_tag().

Note: $id is numeric :-).

Test Plan: Display diff.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1580
2012-02-06 11:22:43 -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
vrana
6cb5db60d5 Display compatible inline comments (usually synthetic) on the same row
Summary:
This just looks silly:
{F8088, size=full}

It runs in O(N*N) but it's not a big deal because there are usually only few
comments per line.
I didn't implement it for images.

Test Plan:
View revision with compatible inline comments in two diffs.
View revision with different inline comments on same line in two diffs.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1570
2012-02-04 10:20:37 -08:00
epriestley
de7aa2186c Resolve great internal confusion about left vs right inline comments
Summary:
This code was just all kinds of wrong, but got all the common cases anyone cares
about correct.

  - In edit-inline-comments.js, if isOnRight() is true, use data.right, not
data.left (derp).
  - Set data.left correctly, not to the same value as data.right (derp derp).
  - Set "isNewFile" based on $is_new, not $on_right (derp derp derp).

Test Plan:
 - Added JS debugging code to print "OLD" vs "NEW" and "LEFT" vs "RIGHT".
Clicked the left and right sides of diff-vs-base and diff-vs-diff diffs,
verified output was accurate in all cases.
 - Added comments to the left-display-side of a diff-of-diffs, saved them, they
showed up where I put them.

Reviewers: btrahan, vrana

Reviewed By: btrahan

CC: aran, epriestley

Maniphest Tasks: T543

Differential Revision: https://secure.phabricator.com/D1567
2012-02-03 15:26:47 -08:00
epriestley
c86dfd84d9 Wrap basic diff/revision association in a transaction
Summary:
This doesn't cover every case exhaustively (see comments) but should cover like
98% of the practical cases.

This makes one workflow modification: willWriteRevision() was previously
guaranteed to have a revisionID / revisionPHID and no longer is. I verified that
no field implementations depend on this behavior. Fields which depend on IDs
should be using didWriteRevision() instead.

Test Plan: Inserted a "throw" into the middle of the transactions and created
revisions; they didn't orphan. Created revisions normally, they worked
correctly.

Reviewers: btrahan, nh

Reviewed By: btrahan

CC: aran, epriestley

Maniphest Tasks: T605

Differential Revision: https://secure.phabricator.com/D1541
2012-02-02 16:03:44 -08:00
epriestley
29337a9b3a Minor, add "cov" class to inline comment <td /> so new files with lint in them do not go crazy. 2012-01-31 18:30:22 -08:00
Nick Harper
28c9607fd0 Provide better UX when trying to view binary file from differential
Summary:
When a user selects "show raw file (right)" from the dropdown of a binary file
in differential, they should get more than a blank page.

Test Plan: Loaded a raw binary file from differential

Reviewers: tuomaspelkonen, epriestley, jungejason

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1533
2012-01-31 17:24:21 -08:00
epriestley
1e9492f8c2 Show coverage information in Differential
Summary:
Render coverage information in the right gutter, if available.

We could render some kind of summary report deal too but this seems like a good
start.

Test Plan:
  - Looked at diffs with coverage.
  - Looked at diffs without coverage.
  - Used inline comments, diff-of-diff, "show more", "show entire file", "show
generated file", "undo". Nothing seemed disrupted by the addition of a 5th
column.

Reviewers: btrahan, tuomaspelkonen, jungejason

Reviewed By: btrahan

CC: zeeg, aran, epriestley

Maniphest Tasks: T140

Differential Revision: https://secure.phabricator.com/D1527
2012-01-31 12:07:47 -08:00
epriestley
3d7a1d936c Fix issue where all rows in "Revision Update History" render the same base
Summary: We were not correctly updating $diff as we iterated through the loop.

Test Plan: Viewed a revision several diffs that had differing base revision.

Reviewers: fratrik, btrahan

Reviewed By: fratrik

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1523
2012-01-31 12:07:41 -08:00
vrana
deed4ffed0 Don't output empty summary in e-mails
Summary:
Reviews with empty summary are rendered like this:

  Reviewers: ...

  TEST PLAN

Test Plan:
Use empty summary.
Use non-empty summary.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1528
2012-01-31 10:41:31 -08:00
Bob Trahan
5755830faa Make Differential comments be styled similarly to Maniphest comments
Summary:
This diff restructures the DOM and alters some CSS within differential.
Original goal was to unify these codepaths more fully into a base class or
classes, but they have quite a bit of custom code such that didn't feel too
compelling in practice.   It also felt related to feed stories as I thought
about the more general version(s) of this code...

Also deleted some CSS from maniphest that wasn't doing anything.

Test Plan:
looked at a differential diff and liked what I saw.   spent a bunch
of time trying out different types of comments and etc.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Maniphest Tasks: T803

Differential Revision: https://secure.phabricator.com/D1513
2012-01-29 16:18:10 -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
epriestley
dbd91d6818 Allow differential.query to find accepted and committed revisions; fix a fatal
Summary:
  - Expose existing 'committed' filter.
  - Add an 'accepted' filter.
  - Fix a fatal where $repository may not be defined (for diffs not linked to a
repository).

Test Plan: Ran accepted / committed queries. Viewed a previously fataling diff.

Reviewers: btrahan, vrana, Makinde

Reviewed By: Makinde

CC: Koolvin, aran, epriestley

Differential Revision: https://secure.phabricator.com/D1490
2012-01-25 14:50:34 -08:00
vrana
067c7f8a74 Display links to editor in Differential and Diffusion
Summary:
It is possible to open a file in editor by registering a custom URI scheme
(pseudo-protocol). Some editors register it by default.
Having links to open the file in external editor is productivity booster
although it is a little bit harder to set up.
There are several other tools using file_link_format configuration directive
(XDebug, Symfony) to bind to this protocol.
I've added the example with editor: protocol which can be used as a proxy to
actual editor (used by Nette Framework:
http://wiki.nette.org/en/howto-editor-link).

Test Plan:
Configure Editor Link in User Preferences.
Register URI scheme in OS.
Open a file in Diffusion. Click on the Edit button.
Open a revision in Differential. Click on the Edit button.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1422
2012-01-24 10:42:33 -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
vrana
28a5f9f44d Display comment title as title instead of body with inline comments
Test Plan: Display revision containing comments with no content but with inline
comments.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1474
2012-01-24 10:41:47 -08:00
vrana
32fc92bdb9 Display title for lint and unit stars in Revision Update History
Test Plan:
Display revision with different lint and unit results.
Hover over the stars.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1475
2012-01-24 10:41:31 -08:00
epriestley
97820b2ff7 Add branch queries to differential.query
Summary: This enables some improvements in D1478. Allow revisons to be queried
by the branch which they appear on.

Test Plan: Queried revisions by branch. Ran "arc which" branch queries in SVN
and Mercurial.

Reviewers: btrahan, cpiro, jungejason

Reviewed By: btrahan

CC: aran, epriestley

Maniphest Tasks: T787

Differential Revision: https://secure.phabricator.com/D1479
2012-01-24 09:44:59 -08:00
epriestley
3c8bb8a608 Fix comment anchor jump highlighting
Summary: I accidentally broke the feature where we highlight comments which are
jumped to via anchor in D1327. We now test that the jump was sucessful by
looking for an item with the anchor ID, but we were only setting 'name'.
Instead, set 'id' as well so the highlighting code detects that the jump was
successful and adds the highlight class.

Test Plan: Clicked "Comment D1234#7" or whatever, got a nice yellow background.

Reviewers: btrahan, nh, jungejason

Reviewed By: btrahan

CC: aran, epriestley

Maniphest Tasks: T796

Differential Revision: https://secure.phabricator.com/D1471
2012-01-24 09:44:20 -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
58f4dc0369 Unify terminology in Differential overview
Summary: You can order by Modified but the table has Updated column.

Test Plan: /differential/filter/reviews/

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1472
2012-01-23 17:39:49 -08:00
Dave Ingram
3edf60627d Add support for marking files as "generated" by regexp against path
Summary:
Not all auto-generated files can include the magical
"generated" annotation for one reason or another, but they may follow
path rules. This patch allows files to be marked as automatically
generated by matching the path with a regular expression.

Test Plan:
Alter 'differential.generated-paths' setting in config.
Create a new diff that affects a file matching one of those regular
expressions. Verify that Differential marks it as automatically
generated and therefore probably not worth reviewing (in the same way as
the magical "generated" annotation.

Reviewers: epriestley

CC: aran

Differential Revision: https://secure.phabricator.com/D1455
2012-01-19 18:30:19 +00: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
ef768f9694 Use phutil_utf8_hard_wrap() in Phabricator
Summary: See D1433.

Test Plan: Created a new diff with a line >80chars, observed it wrapping
correctly.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D1438
2012-01-17 09:48:58 -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
epriestley
05ee317555 Remove "parsedHunk" property
Summary: This is never read anywhere and clearly has no effect.

Test Plan: grep

Reviewers: vrana, btrahan, jungejason

Reviewed By: vrana

CC: aran

Differential Revision: https://secure.phabricator.com/D1434
2012-01-17 08:09:56 -08:00
epriestley
8f1c7dc663 Remove some unused nonsense
Summary: These blocks do nothing. end() produces a side effect on the internal
array pointer, but the code does not depend on it.

Test Plan: Reasoned about the code? Also viewed some diffs.

Reviewers: vrana, btrahan, jungejason

Reviewed By: vrana

CC: aran

Differential Revision: https://secure.phabricator.com/D1432
2012-01-17 08:09:45 -08:00
epriestley
2d8b35db93 Remove getDisplayLine()
Summary: No callsites anywhere. Unclear what this method is even supposed to do.

Test Plan: grep

Reviewers: vrana, btrahan, jungejason

Reviewed By: vrana

CC: aran

Differential Revision: https://secure.phabricator.com/D1435
2012-01-17 08:09:38 -08:00
vrana
f94c05a5bb Declare property
Test Plan: Display revision

Reviewers: epriestley

Reviewed By: epriestley

CC: aran

Differential Revision: https://secure.phabricator.com/D1430
2012-01-16 17:36:11 -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
f109342a7a Display link in Revision ToC for copied or moved files
Summary:
They are present in the document so there is not reason to omit the links to
them.
They sometimes contains changed lines so the link could be actualy useful.

Test Plan: Display ToC of revision with moved and copied files.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley, nh

Differential Revision: https://secure.phabricator.com/D1412
2012-01-16 09:10:17 -08:00
vrana
c7997e0a7c Display Show Raw File links in Differential only if the file exists
Test Plan:
Open menu for added file
Open menu for deleted file
Open menu for changed file

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1410
2012-01-16 09:08:40 -08:00
vrana
49a59bd885 Fix XSS in Differential
Test Plan: Display a revision with file copied to ##<b>hack</b>##.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1411
2012-01-15 22:36:14 -08:00
epriestley
d8bbf55959 Improve behavior when user submits a no-op action in Differential
Summary:
See T730 and the slightly-less-pretty version of this in D1398.

When a user takes an action in Differential that has no effect (for instance,
accepting an already-accepted revision), prompt them:

  Action Has No Effect

  You can not accept this revision because it has already been accepted.

  Do you want to post the feedback anyway, as a normal comment?

                        [Cancel] [Post as Comment]

If they have no comment text, the dialog only says "Cancel".

I think this is probably the best way to balance all the concerns here -- it
might occasionally be a little annoying, but that should be rare, and it should
never be confusing (the current workflow is extremely confusing).

This also fixes the issue where you can add all sorts of CCs who are already
part of the revision, either explicitly or via mentions.

Test Plan:
Posted some has-effect and has-no-effect comments, made different
choices in the dialog, everything seems to work OK?

Reviewers: vrana, btrahan, jungejason

Reviewed By: vrana

CC: aran, vrana

Maniphest Tasks: T730

Differential Revision: https://secure.phabricator.com/D1403
2012-01-15 03:44:09 -08:00
vrana
4cff02dcc0 Add BRANCH to Accepted and Needs Revision e-mails
Summary:
I always forget a branch which I used for the diff so that I must open
my browser which takes some time. This diff adds the name of the branch to the
sent e-mails. But only if the diff is in the state Accepted or Needs Revision to
not pollute other e-mails.

Test Plan:
Comment
Request changes
Accept
Look at the e-mails

Reviewers: epriestley

Reviewed By: epriestley

CC: olivier, aran, epriestley, vrana

Differential Revision: https://secure.phabricator.com/D1396
2012-01-14 11:12:28 -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
vrana
5cf6d788ce Don't add author and reviewers to CCs
Summary:
Commenting on a diff causes adding the writer to the CCs. It doesn't make much
sense if the writer is author or reviewer who get all the copies anyway.

I've also moved the decision to DifferentialCommentEditor.

Test Plan:
Comment on a diff where I am author
Comment on a diff where I am reviewer
Comment on a diff where I am neither
Explicitely Add CCs where I am author

Reviewers: epriestley

Reviewed By: epriestley

CC: jungejason, aran, epriestley

Differential Revision: https://secure.phabricator.com/D1397
2012-01-14 11:10:40 -08:00
epriestley
04eb1f98e2 Always mark revisions as "updated" when users comment on them
Summary: See T773 and the explanatory inline comment.

Test Plan: Made no-action comments and comments that did something (reject, plan
changes) to revisions. Saw them always jump to the top of the action list.

Reviewers: jungejason, simpkins, btrahan

Reviewed By: jungejason

CC: aran, jungejason

Maniphest Tasks: T773

Differential Revision: https://secure.phabricator.com/D1386
2012-01-12 20:08:26 -08:00
Jason Ge
cf35a640ec Enable filtering for committed revisions
Summary:
engineers requested to supporting filtering by 'committed'
revisions, and I think it makes sense.

Test Plan: verified that all the three options worked

Reviewers: epriestley, btrahan, nh

Reviewed By: nh

CC: nh, wolffiex, aran

Differential Revision: https://secure.phabricator.com/D1383
2012-01-12 17:02:20 -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
84ea5c53e4 Kill PhabricatorFileURI
Summary:
we used to need this function for security purposes, but no longer need
it.   remove it so that some call sites can be optimized via smarter data
fetching, and so the whole codebase can have one less thing in it.

Test Plan:
verified the images displayed properly for each of the following
- viewed a diff with added images.
- viewed a user feed
- viewed a user profile
- viewed all image macros
- viewed a paste and clicked through "raw link"
weakness in testing around proxy files and transformed files.  not sure what
these are.  changes here are very programmatic however.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, btrahan, epriestley

Maniphest Tasks: T672

Differential Revision: https://secure.phabricator.com/D1354
2012-01-10 15:21:39 -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
a4ceba9101 Add more information to differential.getdiff
Summary: We need some additional fields to heuristically match revisions to the
working copy in arc.

Test Plan: Executed conduit method, got correct values in fields

Reviewers: btrahan, jungejason

Reviewed By: jungejason

CC: aran, jungejason

Differential Revision: https://secure.phabricator.com/D1347
2012-01-10 10:40:57 -08:00
Jason Ge
79d6d252b7 Display base correctly for git-svn in update history
Summary:
for a git repo which is using git-svn, we should show the
revisioni number. For example, it is show "svn+ssh" if the git url
starts with "svn+ssh". Show the svn revision number instead.

Before fix:
https://secure.phabricator.com/file/view/PHID-FILE-j5rogyqjkgifoxtpwzcu/

After fix:
https://secure.phabricator.com/file/view/PHID-FILE-4vsnptcjzuzuyj4zo25x/

Test Plan:
- verified a diff with git-svn worked
- verified a pure git diff still worked

Reviewers: epriestley, nh

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1344
2012-01-08 12:18:22 -08:00
Nick Harper
13d930b232 Fix table layout bug for inline comments
Summary:
The filename header for inline comments used to span 2 columns - the line number
and the comment. With the addition of a column for the diff (to link to inline
comments on previous diffs), the filename header should now span 3 columns
instead of just the line number and diff, leaving the comment squished to the
right.

Test Plan:
Opened a differential revision with an inline comment from a previous diff, and
saw that the filename header continued across the comment. Also checked an
inline comment on a current diff, and saw that it looks fine.

Reviewers: epriestley, btrahan, jungejason

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1340
2012-01-06 14:55:18 -08:00
epriestley
3c9551e96d Add "Reveal Entire File" option to Diffusion
Summary: Clicks all the "Show All" links for you at the touch of a button.

Test Plan:
  - Used "reveal entire file" on revealable files.
  - Opened on already-visible files, got "entire file shown".
  - Used other menu options.
  - Used normal "show more" links.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran, epriestley, btrahan

Maniphest Tasks: T497

Differential Revision: https://secure.phabricator.com/D1331
2012-01-06 11:51:10 -08:00
epriestley
e5c0bfc8e3 Link to undisplayed inline comments
Summary:
We currently don't link to comments which aren't visible. Link to the
appropriate diff in a new window, indicating where the comment lives.

Test Plan: Clicked visible, not-so-visible comments.

Reviewers: btrahan, jungejason, davidreuss

Reviewed By: btrahan

CC: aran, btrahan, epriestley

Maniphest Tasks: T555, T449

Differential Revision: https://secure.phabricator.com/D1333
2012-01-06 11:50:59 -08:00
epriestley
1903bb80bb Add a link from Differential to Diffusion
Summary:
Provide an easy way to jump to Diffusion from Differential if we have
the data we need to connect them.

Test Plan: Tested menu in linked and unlinked diffs. Used menu item.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran, epriestley, btrahan

Maniphest Tasks: T309

Differential Revision: https://secure.phabricator.com/D1326
2012-01-05 18:03:08 -08:00
epriestley
da3a972400 Retry failed page anchor navigation
Summary:
When the user loads a page with an anchor on it like #thing, or clicks a link to
#thing, and #thing doesn't exist, keep trying to navigate to #thing for a few
seconds.

This allows anchors to work when the target is in content which is later ajaxed
in. In particular, this affects inline comments in Differential.

Test Plan: Opened inline comment links in a new tab, was in the right place when
I switched tabs.

Reviewers: nh, btrahan, jungejason

Reviewed By: btrahan

CC: aran, btrahan, epriestley

Maniphest Tasks: T492

Differential Revision: https://secure.phabricator.com/D1327
2012-01-05 18:02:02 -08:00
Bob Trahan
e478c0074c Add support for querying by commit hashes to DifferentialRevisionQuery class and
corresponding ConduitAPI

Summary: reasonable title...  also made this new functionality used by the
repository worker for parsing diffs

Test Plan:
- looked at the conduit console and queried for various types of hashes,
including hashes with no match.  got correct results.
- identified a reasonable diff from a local git repo.  set the revision status
to 2 (ACCEPTED) in the database.  augmented the worker parser code to var_dump
and die after finding revision id.   ran scripts/repository/reparse.php
--message rX and verified my var_dumps.  removed var_dumps and die and ran
reparse.php again with same paramters.  verified revision looked good in
diffusion and there were no errors.
- repeated the above reparse.php jonx for a mercurial repo.  note svn isn't in
this hash game so that test was particularly exciting no-op'dness i did not
bother with

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, btrahan, epriestley

Differential Revision: https://secure.phabricator.com/D1315
2012-01-05 13:51:51 -08:00
epriestley
7abdf3afe0 Add a dropdown menu for Differential view options
Summary:
There are several open Differential tasks that are basically blocked on not
having reasonable places in the UI to put things. Replace the "View Standalone /
Raw" button with a "View Options" dropdown menu so we can shove things like
"Expand All", "Fold / Unfold File", and "View in Diffusion" in there.

This doesn't change any behavior, just puts the existing options in a menu.

Test Plan:
  - Toggled menu open by clicking button.
  - Clicked menu items.
  - Toggled menu closed by clicking button.
  - Toggled menu closed by clicking document.
  - Toggled menu closed by opening another menu.
  - Toggled menu closed by selecting an item.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran, btrahan

Maniphest Tasks: T497, T309

Differential Revision: https://secure.phabricator.com/D1316
2012-01-05 12:58:38 -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
1cb81936e7 Minor, fix array_merge() issue. 2012-01-05 09:11:49 -08:00
vrana
460efc4489 Include added reviewers and ccs in preview
Summary: Preview of Add Reviewers looks silly without actually showing them

Test Plan:
Go to any diff
Leap into action: Add Reviewers
Add some reviewers
Write some comment
Preview including Added reviewers should be displayed
Change action to Comment
Added reviewers should disappear
Repeat with Add CCs

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley, vrana

Differential Revision: https://secure.phabricator.com/D1276
2012-01-04 17:08:13 -08:00
epriestley
4077ef2555 Show lint issues inline in Differential
Summary:
Take lint information and use it to render synthetic inline comments in
Differential.

Test Plan:
https://secure.phabricator.com/file/view/PHID-FILE-2uapwmf5tqhgscbuty3b/

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran, btrahan, epriestley, pad

Maniphest Tasks: T24

Differential Revision: https://secure.phabricator.com/D1309
2012-01-04 14:17:33 -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
vrana
7a9be605ae Avoid double escaping in render()
Test Plan: View any comment

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1296
2012-01-03 16:46:05 -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
Nick Harper
369079dd45 Change default status to 'all' in DifferentialRevisionList
Summary: Makes it easier to discover the list of all revisions for a user.

Test Plan:
Opened up /differential/filter/revisions/, and saw that it defaulted
to status of all. Clicked between tabs, and it stayed on all. Selected
open, it only displayed open revisions, including as I switched between
tabs.

Reviewers: epriestley, jungejason

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1278
2012-01-03 14:49:30 -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
a810469929 Improve performance of differential.query
Summary:
  - Instead of running (1 + N * 4) queries, run 5 queries.
  - Add 'dateModified' field.

Test Plan:
  - Ran various queries against various revisions, data seemed unchanged.
  - Checked query log.

Reviewers: nh, jonathanhester, jungejason, btrahan

Reviewed By: jonathanhester

CC: aran, epriestley, jonathanhester, jungejason

Differential Revision: 1252
2011-12-20 19:52:18 -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
jhester
1270b0b5bd Add properties to differential.getdiff
Summary: Add 'addLines' and 'delLines' properties to differential.getdiff return
dictionary.  These properties are aggregated from the changesets.

Test Plan: Issue a differential.getdiff query via conduit and verify that
'addLines' and 'removeLines' properties are included and accurate

Reviewers: epriestley

Reviewed By: epriestley

CC: epriestley, aran, jonathanhester

Differential Revision: 1209
2011-12-16 18:02:35 -08:00
Evan Priestley
770beb5cd5 Merge pull request #85 from dcramer/hide-empty-fields-in-maniphest
Hide auxiliary fields that have no value set
2011-12-16 16:51:58 -08:00
David Cramer
2a80bdd448 Hide auxiliary fields that have no value set
Reviewers: epriestly
2011-12-16 16:49:45 -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
43fc7820e9 August comes AFTER July. 2011-12-16 13:34:36 -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
epriestley
3cd92ab075 Minor fix to D1186 to enforce ordering. 2011-12-16 13:32:16 -08:00
epriestley
4dd87f1ad3 Drive Differential landing page with DifferentialRevisionQuery, simplify UI
Summary:
  - Use DifferentialRevisionQuery, not DifferentialRevisionListData, to select
revisions.
  - Make UI simpler (I hope?) and more flexible, similar to Maniphest. It now
shows "Active", "Revisions", "Reviews" and "Subscribed" instead of a hodge-podge
of miscellaneous stuff. All now really has all revisions, not just open
revisions.
  - Allow views to be filtered and sorted more flexibly.
  - Allow anonymous users to use the per-user views, just don't default them
there.

NOTE: This might have performance implications! I need some help evaluating
them.

@nh / @jungejason / @aran, can one of you run some queries agianst FB's corpus?

The "active revisions" view is built much differently now. Before, we issued two
queries:

  - SELECT (open revisions you authored that need revision) UNION ALL (open
revisions you are reviewing that need review)
  - SELECT (open revisions you authored that need review) UNION ALL (open
revisions you are reviewing that need revision)

These two queries generate the "Action Required" and "Waiting on Others" views,
and are available in P247.

Now, we issue only one query:

  - SELECT (open revisions you authored or are reviewing)

Then we divide them into the two tables in PHP. That query is available in P246.

On the secure.phabricator.com data, this new approach seems to be much better
(like, 10x better). But the secure.phabricator.com data isn't very large. Can
someone run it against Facebook's data (using a few heavy-hitting PHIDs, like
ola or something) to make sure it won't cause a regression?

In particular:

  - Run the queries and make sure the new version doesn't take too long.
  - Run the queries with EXPLAIN and give me the output maybe?

Test Plan:
  - Looked at different filters.
  - Changed "View User" PHID.
  - Changed open/all.
  - Changed sort order.
  - Ran EXPLAIN / select against secure.phabricator.com corpus.

Reviewers: btrahan, nh, jungejason

Reviewed By: btrahan

CC: cpiro, aran, btrahan, epriestley, jungejason, nh

Maniphest Tasks: T586

Differential Revision: 1186
2011-12-16 13:21:54 -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
Marek Sapota
2d232674df Allow customized patterns for marking generated files.
Test Plan:
Created a listener that adds some patterns to $matches array, reloaded
Differential, some changesets were not shown as generated.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley, mareksapota

Differential Revision: 1200
2011-12-13 17:14:25 -08:00
epriestley
682e0aa468 Add "responsible users" and "subscribers" to DifferentialRevisionQuery, plus a
couple bug fixes

Summary:
  - Add the ability to query for "responsible users" (author or reviewer).
  - Add the ability to query for "subscribers" (reviewer or CC).
  - Fix an issue where CC and Reviewer used the same join table alias and were
incompatible.
  - Remove support for 'paths' for the moment, since each path needs a
repository ID. (There are no clients for this.)
  - Remove single withX() methods that have no callsites -- withPath() is
singular because it accepts two arguments and I didn't want to have an ad-hoc
type format, but I think we can get away without these for other conditions.
  - Include GROUP BY in more cases where may need it. This doesn't actually
change program behavior since we uniquify in loadFromArray(), it just means less
data over the wire.

These new query classes are to support rewriting the Differential list view on
top of DifferentialRevisionQuery.

Test Plan:
  - Issued queries via conduit for "responsible users".
  - Issued queries via conduit for "subscribers".
  - Issued queries via conduit for "cc" with "reviewer" at the same time.
  - Issued queries via conduit for "cc", "reviewer", "responsible users" and
"subscribers" at the same time.
  - Issued a "subscribers" and "reviewers" query which returned duplicates;
verified GROUP BY took effect.

Reviewers: nh, btrahan, jungejason

Reviewed By: nh

CC: aran, nh

Differential Revision: 1182
2011-12-08 09:38:52 -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
d13906ff96 Add "tabindex" to Remarkup reference lists
Summary:
Prevent keyboard focus of these links so we don't disrupt tab order from
comments to "Submit".

Arguably I should make a "function" for this or something but there's nowhere to
really put it that makes any sense right now.

Test Plan: Verified Firefox skips these links in tab order.

Reviewers: fratrik, btrahan, jungejason

Reviewed By: fratrik

CC: aran, fratrik

Maniphest Tasks: T661

Differential Revision: 1180
2011-12-07 08:53:12 -08:00
epriestley
4fd81150be Remove "Updated" view from Differential
Summary:
This landed during my review drama embargo and is a generally good idea but had
some implementation issues.

@elynde reports it has been broken for some time, although it still works on
secure.phabricator.com so I'm guessing it's just taking a zillion years to run
at Facebook. It's up to more than a second for me on secure.phabricator.com:

https://secure.phabricator.com/file/view/PHID-FILE-v4ql4c66u3xnkarmrpm4/

The basic problem is that some of the data architecture around this
implementation is hard to scale. I want to pursue a similar feature eventually,
but drive it off notifications that we'll ship through real-time infrastructure
too.

I'm also trying to get rid of DifferentialRevisionListData and this simplifies
that somewhat.

Test Plan:
  - Grepped for table name, table constant, query constant, and class name; no
hits.
  - Applied SQL patch.
  - Verified that Differential no longer shows "Updated".

Reviewers: elynde, btrahan, jungejason

Reviewed By: elynde

CC: aran, elynde

Differential Revision: 1178
2011-12-07 06:55:03 -08:00
Nick Harper
74f710a437 Add sanity to DifferentialRevisionQuery
Summary:
Changed cc/reviewer search to be a union/or instead of intersection/and within
each list. Also added support to search for multiple authors (same behavior as
cc/reviewer), and updated conduit call to match. (See discussion on D1158.)

Test Plan:
Used the conduit call to search for revisions with one of 2 people on the cc
list, and checked the results to see that it wasn't constraining to requiring
both be on the cc list.

Reviewers: epriestley, jungejason, btrahan

Reviewed By: epriestley

CC: aran, nh, epriestley

Differential Revision: 1179
2011-12-06 14:47:12 -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
588b959c03 phabricator_format_timestamp => phabricator_datetime
Summary: make the change, kill the function.   be sure to get a good $user or
$viewer variable

Test Plan:
for each controller or view, look at it in the ui.   change timezone, refresh ui
and note change.   i did not test the OAuthSettingsPanelController; not sure how
to get to that badboy and i got a bit lazy

Maniphest Tasks: T222

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, btrahan, epriestley

Maniphest Tasks: T222

Differential Revision: 1166
2011-12-04 08:51:34 -08:00
Jakub Vrana
9c1697383c Add a link to Remarkup Reference below comment
Summary:
To reduce blindness, all textareas with some kind of special syntax should have
an information about this syntax and a link to its documentation. Preview
function is a nice complement but it doesn't replace this information.

I've added this information and the link below the comment field.

Please note that <a target> is a valid attribute in HTML5.

Test Plan:
Go to https://secure.phabricator.com/D1164#comment-content
There should be a link to Remarkup Reference
This link should open Remarkup Reference in a new window (to not discard the
comment)

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley, vrana

Differential Revision: 1164
2011-12-03 12:28:21 -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
10cc5f2660 Set user on auxiliary fields before validating them on template workflow
Summary: Some fields need this data in some circumstances in order to validate
-- see D1153.

Test Plan: Ran "arc diff" against local, no longer got an exception for access
of this field from the 'Reviewers' validator.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, btrahan

Differential Revision: 1160
2011-12-02 15:14:39 -08:00
Nick Harper
8f5e28bf59 Add differential.query conduit method
Summary:
Created a differential.query conduit method that is built on top of
DifferentialRevisionQuery. I also added support for querying by author, ccs, and
reviewers to DifferentialRevisionQuery, so feature parity can be brought up to
match differential.find and its backing class DifferentialRevisionListData.

Test Plan:
Tried a few calls to the conduit call using the web interface, and got back
reasonable looking data.

Reviewers: epriestley, jungejason, btrahan

Reviewed By: epriestley

CC: aran, nh, epriestley

Differential Revision: 1158
2011-12-02 13:06:43 -08:00
Bob Trahan
519a443eba kill differential tabs in favor of a create diff button
Summary:
kill the tabs and make it a create button instead.  pertinent notes:
* added a "Filter diffs" button to the form.  optional, but i thought it
necessary with the new green button
* linked to Arcanist user guide on the create diff page.  somewhat unrelated but
i think create diff will get more traffic now so linking to help seemed like a
reasonable add on here.

Test Plan:
viewed differential homepage
* clicked left hand filter elements.  noted "Create Diff" button on filters
within user revisions and no button on filters within all revisions.
* entered another user into Select User UI and viewed their diffs via button and
pressing enter

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, btrahan, epriestley

Differential Revision: 1157
2011-12-02 10:39:36 -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
Marek Sapota
e9693f25f8 Move event framework from Phabricator to libphutil
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
2011-11-16 16:34:45 -08:00
Marek Sapota
b71a55900a Allow tweaking of Differential mail by using events
Summary: Allow tweaking Differential mail before sending.

Test Plan:
Wrote a listener renaming Differential attachments and it worked without
problems.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley, mareksapota, davidreuss

Differential Revision: 1091
2011-11-09 10:13:53 -08:00
epriestley
fbfb263cd9 Provide a configuration flag to disable silliness in the UI
Summary: See comments. A few installs have remarked that their organizations
would prefer buttons labled "Submit" to buttons labeled "Clowncopterize".

Test Plan:
  - In "serious" mode, verified Differential and Maniphest have serious strings,
tasks can not be closed out of spite, and reset/welcome emails are extremely
serious.
  - In unserious mode, verified Differential and Maniphest have normal strings,
tasks can be closed out of spite, and reset/welcome emails are silly.
  - This does not disable the "fax these changes" message in Arcanist (no
reasonable way for it to read the config value) or the rainbow syntax
highlighter (already removable though configuration).

Reviewers: moskov, jungejason, nh, tuomaspelkonen, aran

Reviewed By: moskov

CC: aran, moskov

Differential Revision: 1081
2011-11-04 15:24:54 -07:00
Marek Sapota
f1de90d7ef Mark person that actually runs arc commit as committer instead of the author.
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
2011-11-01 15:26:56 -07:00
Marek Sapota
74d1983c68 Move Abandon Revision action to the bottom if it's an admin action.
Test Plan:
Login as admin, look at an open revision you don't own, you should be able to
choose '(Admin) Abandon Revision', the option should be on the bottom and should
abandon the revision after sending the comment.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: 1060
2011-10-28 09:20:18 -07:00
Marek Sapota
9536e5606c Allow admins to abandon Differential revisions.
Test Plan:
Login as an admin, go to a revision that you don't own - you should be able to
abandon this revision.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: 1048
2011-10-25 14:32:50 -07:00
Evan Priestley
9d4793b27f Merge pull request #76 from mareksapota-fb/master
Pull request for differential revision D1044
2011-10-25 10:43:42 -07:00
Marek Sapota
789dc6cb5e Allow anonymus access to Differential.
Summary:
Add possibility for not logged in users to browse and see Differential
revisions.

Test Plan:
Set 'differential.anonymous-access' config option to true, log out, you should
be able to browse Differential without logging back in.

Reviewers: epriestley, jungejason

Reviewed By: epriestley

CC: aran, epriestley, mareksapota

Differential Revision: 1044
2011-10-25 10:23:08 -07:00
epriestley
88be49fd5f Allow DifferentialDiff to construct proper DifferentialChangeset objects from
diffs which add empty files

Summary:
See T507 and some others. We now parse empty git diffs correctly, but the logic
to build DifferentialDiffs out of them leaves the objects with 'null' for
$changesets, when it should be array().

Further layers later throw, believing we have not loaded the changesets, when we
actually have, there just aren't any.

Test Plan: Viewed rJX05d493e17fbbb29f29e4880be6834d1d7415374e in Diffusion,
which adds an empty README file. No exception thrown.

Reviewers: jungejason, nh, tuomaspelkonen, aran

Reviewed By: nh

CC: aran, nh

Differential Revision: 1038
2011-10-24 23:39:59 -07:00
epriestley
4156cf6bd9 Add an optional configuration option to set 'Precedence: bulk' headers on
transactional mail

Summary: See T571. SES refuses to deliver mail with this header and there are
various reports of other issues on the internet so I'm defaulting it to off.

Test Plan: Set config to true, tried to send mail, SES rejected it because of
"Precedence: bulk" header.

Reviewers: bmaurer, ola, jungejason, nh, aran

Reviewed By: aran

CC: aran, epriestley, bmaurer

Differential Revision: 1032
2011-10-23 14:25:13 -07:00
epriestley
661f077bf7 Replace callsites to sha1() that use it to asciify entropy with
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
2011-10-21 11:55:28 -07:00
Emir Habul
f447e5d709 Allow custom hyperlinks; Pass differential.diff-id into remarkup engine config
Summary: This allows extensions to have more options for generating custom
hyperlinks.

Test Plan:
custom-inline rules are moved before default rules. Test existing products which
implement custom rules.
Make sure you use "$this->getEngine()->storeText()" in rules.

Reviewers: epriestley, jungejason

Reviewed By: jungejason

CC: aran, epriestley, emiraga, jungejason

Differential Revision: 1024
2011-10-20 14:39:18 -07:00
Evan Priestley
0cb9f3dcf5 Merge pull request #74 from mareksapota-fb/master
Pull request for differential revision D1019
2011-10-19 15:31:22 -07: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
Marek Sapota
a11053d0fa Add possibility to upload a diff file instead of using copy-paste.
Test Plan:
Go to /differential/diff/create and upload a diff file - result should be the
same as pasting the diff into the textarea.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: 1019
2011-10-19 15:25:25 -07:00
Marek Sapota
5d377e246a Send patch attachments instead of diff attachments.
Test Plan:
Turn on sending patches, create a new revision - you should get a .patch file in
your mail instead of a .diff file.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: 1016
2011-10-18 12:20:24 -07:00
Marek Sapota
87a2987ad6 Differential mail
Test Plan: EMPTY

Reviewers: aran, epriestley

Reviewed By: epriestley

CC: aran, epriestley, mareksapota

Differential Revision: 1004
2011-10-14 12:12:41 -07:00
epriestley
254f606e89 Tie all the pieces for symbol cross-references together
Summary:
This makes symbol cross-references work in Differential. You need to do a little
legwork but I'll document that once the change has baked for a little while.

Basically:

  - Projects are annotated with indexed languages, and "shared library" projects
(for example, symbols in Phabricator should be searched for in Arcanist and
libphutil).
  - When we render a changeset, we check if its language is an indexed one. If
it is, we invoke the decorator Javascript.
  - The Javascript takes you to a lookup page, which either gives you a list of
matching symbols (if several match) or redirects you instantly to the
definition.

Test Plan: Clicked class and function symbols in a diff, got jumped into
sensible sorts of places in Diffusion.

Reviewers: jungejason, nh, tuomaspelkonen, aran

Reviewed By: jungejason

CC: aran, jungejason

Differential Revision: 980
2011-10-09 17:58:17 -07:00
epriestley
0580772805 Add a JS component for crossreferences
Summary: When the user clicks a crossreference, jump them to symbol lookup

Test Plan: Clicked some crossref symbols

Reviewers: jungejason, nh, tuomaspelkonen, aran

Reviewed By: nh

CC: aran, nh, epriestley

Differential Revision: 904
2011-10-09 17:58:01 -07:00
Jason Ge
1e3c10379a Enable typeahead's ondemand on details view page
Summary:
the details pages are using preload instead of ondemand for
typeahead, but the most common actions on the pages are commenting which
would not need the preloaded info. To improve the performance of the
pages, turn on ondemand according to the setting in the config file.

Test Plan: verify it is working with both modes, for both pages.

Reviewers: epriestley, nh

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: 995
2011-10-09 12:33:08 -07:00
epriestley
c29982acb9 Fix phid accumulation for handles
Summary: I goofed this, $phids was already being populated and I changed the
meaning. This causes a fatal if you filter the list by a user who is not an
author or first reviewer for any of the revisions (e.g., no open revisions).

Test Plan: Looked at the list of a user with no revisions.

Reviewers: codeblock, jungejason

Reviewed By: codeblock

CC: aran, codeblock, jungejason

Differential Revision: 989
2011-10-07 12:58:16 -07:00
epriestley
8ce5dd31f6 Show open Differential revisions in Diffusion browse views
Summary:
Still some rough edges, but this adds a table of open revisions to Diffusion.
See T262.

I'll make this a little better (e.g., "see all.." instead of arbitrary 10 cap,
or maybe move to top-level nav?) but I think I have to refactor some other stuff
first. This should let us root out any major issues, at least.

NOTE: You must associate Arcanist Projects with Repositories (in Repositories ->
Arcanist Projects -> Edit) for this to work!

Also made paths include all parent paths so that browse views of directories
will work.

Test Plan: Uploaded a diff which affected "/blah", it appeared when browsing "/"
and "/blah".

Reviewers: jungejason, nh, tuomaspelkonen, aran

Reviewed By: jungejason

CC: aran, jungejason

Differential Revision: 979
2011-10-06 10:27:54 -07:00
epriestley
91bf3e96c9 Provide a Differential Revision query class for affected paths
Summary:
For T262, we need to query for revisions by affected path.

We currently have a class called "DifferentialRevisionListData" but it's sort of
nasty and it would have been really cumbersome to add this query to it.

Instead, this provides a query object more in line with ManiphestTaskQuery,
which I'm pretty happy with. I'd eventually like to get rid of
DifferentialRevisionListData but it's used in a couple of places right now.

Test Plan: Used phpsh to execute queries, got back apparently-sensible result
sets.

Reviewers: jungejason, nh, tuomaspelkonen, aran

Reviewed By: jungejason

CC: aran, jungejason, epriestley

Differential Revision: 978
2011-10-06 10:27:17 -07:00
epriestley
bea4795575 Separate revision list rendering logic into a RevisionListView
Summary: I want to throw this in Diffusion as part of T262, but it's embedded in
the controller right now. Split it out.

Test Plan: Looked at various revision list views, no changes.

Reviewers: jungejason, nh, tuomaspelkonen, aran

Reviewed By: jungejason

CC: aran, jungejason

Differential Revision: 977
2011-10-06 10:26:47 -07:00
Hua Wang
d41fd4a0fa T494 Image displaye issue
Summary: The display of images pairs is not corresponding to the selected two
image diffs. The fix is to use reference to get the phid for each image.

Test Plan: Create a revision with two diffs of images.
           Test the display between base and diff1/diff2.
           Test the rendering of images between diff1 and diff2.
           Test the inline comments also.

Reviewers: epriestley, jungejason

CC:

Differential Revision: 955
2011-09-30 00:25:33 -07:00
tuomaspelkonen
7b8b469da3 Changed the postponed unit tests warning message
Summary:
Postponed unit tests are not unit tests with problems. The results
just haven't arrived yet.

Test Plan: Tested accepting a diff with unit status 1, 3, 5 (ok, errors,
postponed)

Reviewers: jungejason, epriestley

Reviewed By: epriestley

CC: aran, epriestley, tuomaspelkonen

Differential Revision: 969
2011-09-27 13:00:36 -07:00
epriestley
016b060aea Add a relation table for Revisions to local commit hashes
Summary:
This allows us to performantly query for diffs related to a given local hash.
Immediate applications are:

  - Commit detection in Mercurial and Git-Immutable workflows.
  - Some async unit test stuff @mgummelt was doing.

Test Plan:
Diffed locally under SVN/Git/hg, checked the table, got sensible output.

  mysql> select * from differential_revisionhash;
  +------------+------+------------------------------------------+
  | revisionID | type | hash                                     |
  +------------+------+------------------------------------------+
  |         40 | gtcm | 8c6fb2f95598a50f7aac64a5f4cc6c12b5db42f5 |
  |         40 | gttr | 54710e361a465f4ff39565a93b2a221b6e7dd07c |
  |         41 | hgcm | c29cb69aec14                             |
  |         41 | hgcm | e7309be4eabb                             |
  |         41 | hgcm | 4e885caeff60                             |
  |         41 | hgcm | 213ee1cd30ea                             |
  |         41 | hgcm | b4050fb3490f                             |
  |         41 | hgcm | 72a76bd7ffa2                             |
  |         41 | hgcm | 06c2687e63fb                             |
  |         41 | hgcm | 2b464bde6b48                             |
  +------------+------+------------------------------------------+
  10 rows in set (0.00 sec)

NOTE: Mercurial hashes are short-form but I'll shoot out a separate Arcanist
diff to fix this.

Reviewers: Makinde, fratrik, mgummelt, jungejason, nh, tuomaspelkonen, aran

Reviewed By: Makinde

CC: aran, Makinde

Differential Revision: 961
2011-09-26 15:02:37 -07:00
epriestley
43a3f4d234 Build an "affected path" index when attaching diffs to revisions
Summary: See T262. This creates the index on the Differential side which we need in order to execute this query efficiently on the Diffusion side.

Also renames "DiffusionGitPathIDQuery" to "DiffusionPathIDQuery", this query object has nothing to do with git.

Test Plan: Attached top-level and sub-level diffs to revisions and verified they populated the table with sensible data.

Reviewers: bmaurer, aravindn, fmoo, jungejason, nh, tuomaspelkonen, aran

CC:

Differential Revision: 931
2011-09-15 07:45:14 -07:00
epriestley
a42f116749 Allow "!accept" to be enabled through configuration
Summary: For reasons explained in the config I've omitted this from the default
action set, but it's trivial to support it. See D916.

Test Plan: Commented on a revision, was informed I could "!accept" in the email.
Used "!accept" to accept the revision.

Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran

Reviewed By: Makinde

CC: aran, Makinde

Differential Revision: 928
2011-09-14 09:52:13 -07:00
Hua Wang
cd6eb836f6 Enable comments for image
Summary: Added line number 1 for each image and added code to display the
comments for each image.

Test Plan: Adding an image in my local directory and create a revision for it.
Click line number 1, and the comment window prompts.  Adding and save the
comment. The comment shows in the differential comment list and in the inline
comment.  Submit the comment.  Create more comments for the image and the
"Previous" and "Next" buttons all work well.

Reviewers: epriestley, jungejason

CC:

Differential Revision: 901
2011-09-06 18:11:41 -07:00
epriestley
e875c81f6d Remove blameRevision and revertPlan from the DifferentialRevision schema
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
2011-09-04 16:19:12 -07:00
epriestley
76ac8b4196 Display local commit information in Differential
Summary:
After D857, we try to attach local commit information to revisions. If this
information is available, display it on the revision.

Design on this is a little rough, I might try to combine this into the revision
update view or something like that since we're starting to take up a lot of real
estate for metadata.

Test Plan: Local diffed this and got some commit info.

Reviewers: jungejason, tuomaspelkonen, aran

Reviewed By: jungejason

CC: aran, epriestley, jungejason

Differential Revision: 872
2011-08-31 13:49:50 -07:00
epriestley
f7e136ecd2 Remove accidental double-rendering of content sources from Differential
Summary: Oops, I left this in from an earlier version and missed it since I was
mostly looking at Maniphest for testing. We already render this information in
the header, don't additionally render it under the comments.

Test Plan: derp derp, loaded any revision with sourced comments

Reviewers: jungejason, tuomaspelkonen, aran

Reviewed By: jungejason

CC: aran, jungejason

Differential Revision: 871
2011-08-30 12:16:43 -07:00
epriestley
69445222f7 Track content sources (email, web, conduit, mobile) for replies
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
2011-08-30 11:08:27 -07:00
epriestley
0334a92621 Save empty fields as no row, not an empty row
Summary: When a user stores the empty string in an auxiliary field, simply don't
store it, and delete it if it already exists.

Test Plan: Edited a revision with an empty "Quack" field, got an empty row in
the DB. Applied patch, edited empty again, row went away. Edited empty again,
still no row. Edited and put something in the field, got a row.

Reviewers: jungejason, tuomaspelkonen, aran

Reviewed By: jungejason

CC: aran, jungejason

Differential Revision: 865
2011-08-26 16:26:02 -07:00
Nicholas Harper
8c0e5e1c58 Turn off write guard when saving differential comment cache
Summary:
It is possible to view a comment that has no cache; when viewing such a comment
the request doesn't have a csrf token and there is no need for one, so we turn
off the write guard.

Test Plan:
loaded an old diff that had no cache, and the page loaded instead of throwing
an AphrontCSRFException.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: 858
2011-08-25 15:45:16 -07:00
Hua Wang
e903b82fff Delete one line
Summary: Delete one line which has no effect.

Test Plan: Open revision page to make sure it still works.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: 852
2011-08-23 00:45:43 -07:00
epriestley
67de714a2f Remove (most) support for 'differential.attach-task-class'
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
2011-08-19 14:10:30 -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
ebdd6d3d11 Fix a variable typo which prevents metadata from attaching to @mention
Differential comments

Summary: If you @mention several users, at least one of which is already CC'd,
we unset all the CCs and don't attach the "Added CCs: ..." block to the comment.

Test Plan: @mentioned two users, one of whom was already CC'd.

Reviewers: jungejason, tuomaspelkonen, aran

Reviewed By: jungejason

CC: aran, jungejason

Differential Revision: 827
2011-08-18 19:49:52 -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
Jason Ge
4693ffa82b Deprecate generateProperties
Summary:
deprecate generateProperties() from class
DifferentialRevisionDetailRenderer. Custom fields now provides a much
more powerful version of generateProperties().

Depends on D814.

Test Plan:
implemented facebook task field with custom field and
verified it worked.

Reviewers: epriestley, tuomaspelkonen

Reviewed By: epriestley

CC: aran, jungejason, epriestley

Differential Revision: 826
2011-08-18 11:33:10 -07:00
epriestley
6dc193d3d9 Fully update library map. 2011-08-18 09:52:36 -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
88eb6410b3 This is also an unguarded but CSRF-safe cache write. 2011-08-16 14:44:13 -07:00
epriestley
2d22226ff0 Unguard the Differential update time write on GET. 2011-08-16 13:50:47 -07:00
epriestley
39b4d20ce5 Create AphrontWriteGuard, a backup mechanism for CSRF validation
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
2011-08-16 13:29:57 -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
epriestley
f49e35deaf Basic task dependencies for Maniphest
Summary:
This allows you to edit dependencies. It is a better patch than it used to be.
It depends on D725.

  - If you create a cycle, it just throws an exception and aborts the workflow.
It should not do this.
  - Tasks which depend on the current task aren't shown in the UI. Need to add a
new table for this.
  - Transaction text says "attached Task" but should probably say "added a
dependency on task".

Test Plan: Created valid and invalid dependencies between tasks. Created valid
and invalid dependencies between revisions.
Reviewed By: tuomaspelkonen
Reviewers: davidreuss, jungejason, tuomaspelkonen, aran
Commenters: codeblock
CC: aran, codeblock, tuomaspelkonen, epriestley
Differential Revision: 595
2011-08-02 11:16:31 -07:00
Edward Speyer
9e2231f6d6 Revert "Generated code> make it harder to mark code as generated"
Summary: This reverts commit e15da75687.
Test Plan: None
Reviewed By: epriestley
Reviewers: epriestley
CC: aran, epriestley
Differential Revision: 762
2011-08-02 14:47:10 +01:00
Edward Speyer
e15da75687 Generated code> make it harder to mark code as generated
Summary: It's now harder to accidentally mark code as generated.
Test Plan:
Tested with this diff in my sandbox; only file "D" was shown, the others
were marked as generated:

  diff --git a/A b/A
  index e69de29..780b46c 100644
  --- a/A
  +++ b/A
  @@ -0,0 +1,2 @@
  +@generated
  +Tue Jun  7 16:41:17 PDT 2011
  diff --git a/B b/B
  index e69de29..b55fe21 100644
  --- a/B
  +++ b/B
  @@ -0,0 +1,3 @@
  +/**
  + * @generated by ed
  + */
  diff --git a/C b/C
  index e69de29..e0f808a 100644
  --- a/C
  +++ b/C
  @@ -0,0 +1,3 @@
  +/**
  + * {@generated <<jonx>lol>}
  + */
  diff --git a/D b/D
  index e69de29..89e8829 100644
  --- a/D
  +++ b/D
  @@ -0,0 +1,2 @@
  +string = STDIN.readlines
  +string.include?('@generated')

Reviewed By: jungejason
Reviewers: jungejason
CC: aran, edward, jungejason
Differential Revision: 408
2011-08-02 12:50:37 +01:00
epriestley
9d3f33a7a6 Rough implementation of drag-and-drop file uploads
Summary:
This gets all the major pieces working. Allows you to drag-and-drop files in
Differential and Phriction, and embed files in remarkup with {Fxxx} references.
See also task.

I'm explicitly not documenting this yet since it's still pretty rough.

Test Plan: Dragged and dropped stuff into Differential and Phriction.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran, tomo
Commenters: tomo
CC: aran, tomo, jungejason
Differential Revision: 674
2011-08-01 15:27:13 -07:00
epriestley
b70b9bb6d7 Be more explicit in rendering context links in Differential
Summary: See T368. The current rendering result can cause some confusion for the
first/last chunks, make their behavior more explicit.
Test Plan:
  - Clicked various "show more" links on a bunch of top/bottom/middle omitted
context blocks in a variety of diffs.
  - Located a @generated shielded file and verified the initial render is
correct when the entire file is default-hidden.

Reviewed By: avitaloliver
Reviewers: avitaloliver, jungejason, tuomaspelkonen, aran
CC: aran, avitaloliver
Differential Revision: 744
2011-07-29 17:23:36 -07:00
tuomaspelkonen
e00fae8436 Files can be set not to use 'ignore-all' by default.
Summary:
Python people don't seem to like the 'ignore-all' as default. Provide a way
to configure which file types should not use 'ignore-all'.

Test Plan:
Tested that it worked with bunch of Python of files and non-python
files. Cache was disabled during the test.

Reviewed By: jungejason
Reviewers: epriestley, jungejason
Commenters: epriestley
CC: aran, jungejason, epriestley
Differential Revision: 713
2011-07-25 10:46:40 -07:00
epriestley
c0ae2f6289 Show change diffs in Phriction
Summary:
This is really rough and needs work (particularly, there's some diff code I
really need to refactor since I sort-of-copy-pasted it) but basically
functional.

Show text changes between diffs and allow users to revert to earlier versions.

Differential's line-oriented diff style isn't ideal for large blocks of text but
I'm betting this is probably good enough in most cases. We can see how bad it is
in practice and then fix it if needbe.

I added a bunch of support for "description" but didn't add the feature in this
diff, I'll either follow up or task it out since it should be a pretty
straightforward change.

Test Plan: Looked at history for several Phriction documents, clicked "previous
change" / "next change", clicked revert buttons.
Reviewed By: hsb
Reviewers: hsb, codeblock, jungejason, tuomaspelkonen
CC: aran, hsb, epriestley
Differential Revision: 687
2011-07-18 08:46:45 -07:00
epriestley
5e00d00cf7 Show more information on revision views
Summary:
Show line count, arcanist project and base revision.

This adds a little clutter but I think we're still okay and I can play around
with it later.

Test Plan: Looked at a couple of revisions. I'm actually not 100% sure about the
SVN logic but maybe I will test that before committing.
Reviewed By: tomo
Reviewers: tomo, jungejason, tuomaspelkonen, aran
CC: aran, tomo
Differential Revision: 685
2011-07-16 18:54:13 -07:00
epriestley
f761ee91d3 Publish Phriction edits to feed
Summary: Basic integration between Phriction and feed.
Test Plan: Created and edited some documents, they published to feed.
Reviewed By: hsb
Reviewers: hsb, codeblock, jungejason, tuomaspelkonen, aran
CC: aran, hsb
Differential Revision: 653
2011-07-12 16:36:17 -07:00
epriestley
a49138defd Generalize the markup engine factory
Summary:
This thing services every app but it lives inside Differential right now. Pull
it out, and separate the factory interfaces per-application.

This will let us accommodate changes we need to make for Phriction to support
wiki linking.

Test Plan: Tested remarkup in differential, diffusion, maniphest, people,
slowvote.
Reviewed By: hsb
Reviewers: hsb, codeblock, jungejason, tuomaspelkonen, aran
CC: aran, hsb
Differential Revision: 646
2011-07-11 16:36:30 -07:00
mgummelt
36e00db564 bug due to using $fields instead of $this->fields
Summary: see title

Test Plan: ran "arc amend" to ensure that task ids where being included in the commit message

Reviewers: epriestley

CC: dpepper

Differential Revision: 637
2011-07-10 20:26:56 -07:00
epriestley
f55c082e65 Publish Differential stories into feed
Summary: Basic hookup for Differential -> Feed. Also introduces "one-line"
stories for less-important stuff.
Test Plan: Interacted with some revisions, got feed stories out of it.
Reviewed By: jungejason
Reviewers: jungejason, aran, tuomaspelkonen, codeblock
CC: aran, jungejason
Differential Revision: 632
2011-07-09 17:39:17 -07:00
epriestley
51c2726a34 Add Differential parse cache to the GC daemon
Summary:
Add the differential parse cache to the GC. This is the largest object in the
system by a wide margin, I think.

This table is potentially gigantic which is why the script truncates it before
doing a schema change.

Test Plan: Ran the GC daemon, it cleaned up some parse caches.
Reviewed By: jungejason
Reviewers: tuomaspelkonen, jungejason, aran
Commenters: tuomaspelkonen
CC: aran, jungejason, tuomaspelkonen, epriestley
Differential Revision: 620
2011-07-08 17:31:25 -07:00
epriestley
c9acc5b8e9 Allow comment panel to be stuck/unstuck to the bottom of the display
Summary:
See T303. Enable comment panel haunting.

I hid the preview for the sticky panel, which I think is reasonable?

Test Plan:
https://secure.phabricator.com/file/view/PHID-FILE-64713fa8a7c2a22e5b93/
Reviewed By: broofa
Reviewers: broofa, jungejason, aran, tomo, tuomaspelkonen
CC: aran, broofa
Differential Revision: 615
2011-07-08 13:24:20 -07:00
mgummelt
81e3ec5998 Merge branch 'master' of github.com:facebook/phabricator into new 2011-07-07 15:30:20 -07:00
mgummelt
00f4c37ca2 Fixed bug resulting in duplicate field names in commit messages
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
2011-07-07 15:26:55 -07:00
epriestley
85b34c23f9 Clean up Phabricator interface to syntax highlighting
Summary: Reduce the amount of code duplication here and allow for an override
configuration on the filename.map stuff.
Test Plan: Checked paste, diffusion and differential syntax highlighting and
everything appeared reasonable.
Reviewed By: codeblock
Reviewers: tuomaspelkonen, codeblock, jungejason, aran
CC: aran, codeblock, epriestley
Differential Revision: 601
2011-07-06 12:35:36 -07:00
epriestley
70cd8b1b34 Update Phabricator for split syntax highlighting API
Summary: I'll clean some of this stuff up in a followup too, but update the
callers to use the new explicit filename-based API.
Test Plan: Looked at paste, Diffusion and Differential.
Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen, codeblock, jungejason, aran
CC: aran, tuomaspelkonen
Differential Revision: 600
2011-07-06 11:56:37 -07:00
epriestley
6eed2ad2bb Deal more gracefully with unusual display changesets
Summary: These came up in dealing with the diff produced by T271. When a file is
unmodified, don't try to use the "ignore all whitespace" algorithm on it. Also,
detect "changed only by adding or removing trailing whitespace" vs "this file
was not modified" correctly.
Test Plan: Viewed the diff that came out of running 'arc diff' on my T271 mess.
Reviewed By: tuomaspelkonen
Reviewers: alex, jungejason, tuomaspelkonen, aran
CC: aran, tuomaspelkonen
Differential Revision: 548
2011-07-05 09:55:35 -07:00
Ricky Elrod
9454060c29 Add a syntax highlight dropdown, if pygments is enabled.
Summary:
- Add a default list of supported languages to default.conf.php
  and make the initial/default value customizable.
- Store a '' in the database to infer the language from the filename/title.

Test Plan:
Tested in my sandbox with pygments enabled and disabled and various
combinations of filename/extension/dropdown selection.

Reviewers:
epriestley

CC:

Differential Revision: 587
2011-07-04 12:23:43 -04:00
epriestley
a15f07cc33 Allow Phabricator to be configured to use a public Reply-To address
Summary:
We already support this (and Facebook uses it) but it is difficult to configure
and you have to write a bunch of code. Instead, provide a simple flag.

See the documentation changes for details, but when this flag is enabled we send
one email with a reply-to like "D2+public+23hf91fh19fh@phabricator.example.com".
Anyone can reply to this, and we figure out who they are based on their "From"
address instead of a unique hash. This is less secure, but a reasonable tradeoff
in many cases.

This also has the advantage over a naive implementation of at least doing object
hash validation.

@jungejason: I don't think this affects Facebook's implementation but this is an
area where we've had problems in the past, so watch out for it when you deploy.
Also note that you must set "metamta.public-replies" to true since Maniphest now
looks for that key specifically before going into public reply mode; it no
longer just tests for a public reply address being generateable (since it can
always generate one now).

Test Plan:
Swapped my local install in and out of public reply mode and commented on
objects. Got expected email behavior. Replied to public and private email
addresses.

Attacked public addresses by using them when the install was configured to
disallow them and by altering the hash and the from address. All this stuff was
rejected.

Reviewed By: jungejason
Reviewers: moskov, jungejason, tuomaspelkonen, aran
CC: aran, epriestley, moskov, jungejason
Differential Revision: 563
2011-07-03 12:31:00 -07:00
epriestley
af107cff65 Explicitly list reviewers on reviewrequest email
Summary: This used to be in the subject but there was a bunch of churn and now
it's nowhere.
Test Plan: Created, updated, and added CCs to a diff.
Reviewed By: moskov
Reviewers: moskov, avitaloliver, jungejason, tuomaspelkonen, aran
CC: aran, moskov
Differential Revision: 567
2011-06-30 18:56:02 -07:00
epriestley
2b8116d7ae Fix error cues on "Edit Revision" interface
Summary: The field hints on this interface don't behave correctly. Particularly, when you add yourself as a reviewer you aren't pointed at the issue.

Test Plan: Edited a revision and tried to save invalid changes, including self-reviewership.

Reviewers: moskov, jungejason, tuomaspelkonen, aran

CC:

Differential Revision: 565
2011-06-30 13:53:22 -07:00
epriestley
553ef6f587 When sending Differential comment emails, include added CCs and reviewers
explicitly

Summary: You currently have to click through to figure out who got added.
Test Plan:
  - Made a comment which added CCs.
  - Made a comment which added reviewers.
  - Made a comment which added nothing.
  - Made a comment which added CCs and reviewers.

Reviewed By: tomo
Reviewers: tomo, jungejason, tuomaspelkonen, aran
CC: aran, tomo
Differential Revision: 562
2011-06-30 13:04:15 -07:00
epriestley
a46ae11030 Restore heavy arrow to signify linebreak. 2011-06-30 12:14:47 -07:00
mgummelt
3c785cdb5a include task ids in the commit messages returned by "arc amend"
Summary:
when "arc diff" generates a revision, it attaches a task id
if one is included.  However, "arc amend" did not return a task id,
effectively stripping it from the commit message.  This diff fixes
that.

NOTE: This is dependent on revision 549 https://secure.phabricator.com/D549

Test Plan:
0. created a custom class to append Facebook task IDs to commit messages and
attached it to the differential.append-commit-message-class config variable
1. created a new diff in the www repot
2. included Task ID: 609350 in the git commit message
3. "arc diff" to generate the revision
4. "arc amend"
5. ensure that the "Task ID:" field remained in the git commit message

Reviewed By: epriestley
Reviewers: dpepper, jungejason, epriestley
CC: aran, epriestley, mgummelt
Differential Revision: 546
2011-06-29 16:28:21 -07:00
mgummelt
ae78ea38e6 added commit message field class
Summary:
commit message fields were previously stored as name/value
pairs in an associative array.  this resulted in ad hoc code to modify
the structure/rendering of these fields in commit messages.  this diff
introduces a new DifferentialCommitMessageField class.

Test Plan:
ran "arc amend" to ensure the commit message still looked good

Reviewed By: epriestley
Reviewers: epriestley
CC: aran, dpepper, epriestley
Differential Revision: 554
2011-06-29 14:25:24 -07:00
Jun Ge
a54bc391db Merge pull request #26 from hwang36/master
D539 Add P### link to paste
2011-06-28 07:41:40 -07:00
epriestley
07b64dc01f Make @mentions add CCs as a side effect
Summary:
When a user gets @mentioned in Differential, add them as a CC.

No Maniphest hookup yet since I want to make that one a little more formal.

Depends on D518.

Test Plan:
@mentioned a user and they were added as a CC.

Reviewed By: jungejason
Reviewers: tomo, mroch, jsp, jungejason, aran, tuomaspelkonen
CC: aran, jungejason
Differential Revision: 519
2011-06-28 07:00:20 -07:00
epriestley
bb4cf7d6b3 Add an "Add CCs" action to Differential
Summary:
We currently have only an "Add reviewers" action, add "Add CCs". This can also
be accomplished less-discoverably with mentions.

Test Plan:
Added reviewers and CCs to revisions. Toggled display between reviewers and CCs.

Reviewed By: jungejason
Reviewers: tomo, mroch, jsp, jungejason, aran, tuomaspelkonen
CC: aran, jungejason
Differential Revision: 521
2011-06-28 06:41:38 -07:00
hwang
c2d0664c5e Add P### link to paste
Summary:Make a new directory, src/infrastructure/markup/remarkup/markuprule/paste/
	Make a new class called PhabricatorRemarkupRulePaste in that directory.
	Add the rule to DifferentialMarkupEngineFactory.

Test Plan: Created a task in maniphest. Put P1 and P2 in the content.
Created P1 and P2 in Paste. Verified P1 and P2 were highlighted and
linked correctly.

Reviewers:epriestley, codeblock

CC:jungejason

Differential Revision: 539
2011-06-27 22:53:55 -07:00
epriestley
451b0e07dc Improve langauge for "Unsaved Draft"
Summary:
Although I think the recent changes here improved things, the "Unsaved Draft"
language is continuing to confuse new users. Try to find some less-confusing
langauge. Open to suggestions here, too.

Test Plan:
Viewed unsubmitted inline comments.

Reviewed By: jungejason
Reviewers: aran, jungejason, gregprice
CC: aran, epriestley, jungejason
Differential Revision: 501
2011-06-26 12:01:41 -07:00
epriestley
e480233eb8 Remove UTF-8 kludges from Differential
Summary:
This depends on D513 and D514. Those diffs make the display algorithms safe with
respect to mutating utf8, so we no longer need to repair potentially invalid
utf8 sequences with this hack.

Test Plan:
grepped for calls to this method

Reviewers: jungejason, aran, tuomaspelkonen
Commenters: jungejason
CC: aran, epriestley, jungejason
Differential Revision: 515
2011-06-26 11:58:34 -07:00
epriestley
5cfc14cb43 Make Differential linewrap utf-8 aware
Summary:
Differential uses a byte-oriented linewrap algorithm. Instead, use a
character-oriented one which will handle utf-8 properly.

This implies a very slightly performance hit but we only run this code for lines
which need to wrap, and the results get cached. It took about ~2.5ms for the
test file on my machine. I'll keep an eye on it but I think it's currently a
manageable cost.

Test Plan:
Diffed this file: https://secure.phabricator.com/P43
...and got it to render like this:
https://secure.phabricator.com/file/info/PHID-FILE-331ac241bede705b193b/

To do so, I had to disable the un-utf8 block which we can't actually do yet
because of intraline diff, but it shows that once we can get rid of that it
works completely correctly. It will "sort of" work in the meantime (nothing
terrible happens).

Reviewers: jungejason, aran, tuomaspelkonen
CC: aran, epriestley
Differential Revision: 513
2011-06-26 11:58:27 -07:00
epriestley
74d57b0a42 Use phabricator_ time functions in more places
Summary:
Replace some more date() calls with locale-aware calls.

Also, at least on my system, the DateTimeZone / DateTime stuff didn't actually
work and always rendered in UTC. Fixed that.

Test Plan:
Viewed daemon console, differential revisions, files, and maniphest timestamps
in multiple timezones.

Reviewed By: toulouse
Reviewers: toulouse, fratrik, jungejason, aran, tuomaspelkonen
CC: aran, toulouse
Differential Revision: 530
2011-06-26 10:38:25 -07:00
epriestley
405b05a490 Basic @mentions support
Summary:
Provides basic Remarkup support for @mentions. No application integration yet so
these aren't terribly useful until that happens.

Test Plan:
https://secure.phabricator.com/file/view/PHID-FILE-83d68e7af6085ae928df/

Reviewers: tomo, mroch, jsp
Commenters: tomo
CC: aran, tomo, epriestley
Differential Revision: 517
2011-06-24 11:55:15 -07:00
epriestley
b664d67a22 Revert "Make Differential linewrap utf-8 aware"
This shouldn't have landed yet.

This reverts commit e5a036e8c9.
2011-06-24 10:48:37 -07:00
epriestley
3fc817d088 Revert "Remove UTF-8 kludges from Differential"
This shouldn't have landed.

This reverts commit fe04d8bf70.
2011-06-24 10:46:30 -07:00
epriestley
fe04d8bf70 Remove UTF-8 kludges from Differential
Summary: This depends on D513 and D514. Those diffs make the display algorithms safe with respect to mutating utf8, so we no longer need to repair potentially invalidate utf8 sequences with this hack.

Test Plan: grepped for calls to this method

Reviewers: jungejason, aran, tuomaspelkonen

CC:

Differential Revision: 515
2011-06-24 10:18:07 -07:00
epriestley
e5a036e8c9 Make Differential linewrap utf-8 aware
Summary: Differential uses a byte-oriented linewrap algorithm. Instead, use a character-oriented one which will handle utf-8 properly.

This implies a very slightly performance hit but we only run this code for lines which need to wrap, and the results get cached. It took about ~2.5ms for the test file on my machine. I'll keep an eye on it but I think it's currently a manageable cost.

Test Plan: Diffed this file: https://secure.phabricator.com/P43
...and got it to render like this: https://secure.phabricator.com/file/info/PHID-FILE-331ac241bede705b193b/

To do so, I had to disable the un-utf8 block which we can't actually do yet because of intraline diff, but it shows that once we can get rid of that it works completely correctly. It will "sort of" work in the meantime (nothing terrible happens).

Reviewers: jungejason, aran, tuomaspelkonen

CC:

Differential Revision: 513
2011-06-24 09:41:45 -07:00
epriestley
9844bbb4f9 Don't use the "ignore all whitespace" algorithm on multi-hunk diffs
Summary:
Diffs with missing context don't render properly in the "ignore all whitespace"
algorith, so don't try to use it. These diffs can occur if someone creates a
diff via the web interface, for example, or if they muck around in their copy of
'arc'.

See D473, T246 (a problem with D473), rPe5bb756b5191720 (revert of D473) and
T231.

Test Plan:
Viewed a diff with missing context from the web interface. Verified normal diffs
still rendered with all whitespace ignored.

Reviewed By: fratrik
Reviewers: jungejason, aran, tuomaspelkonen, fratrik
Commenters: jungejason
CC: aran, epriestley, fratrik, jungejason
Differential Revision: 500
2011-06-23 16:55:04 -07:00
epriestley
d031d3ae32 Slightly improve UTF-8 handling in Differential
Summary:
See comments. I think this will fix the issue, where we end up handling off
garbage to htmlspecialchars() after highlighting a file we've stuck full of \0
bytes.

The right fix for this is to make wordwrap and intraline-diff utf8 aware and
throw this whole thing away. I'll work on that but I think this fixes the
immediate issue.

Test Plan:
diffed the file with a UTF-8 quote in it and got a reasonable render in
Differential

Reviewed By: jungejason
Reviewers: jungejason, aran, tuomaspelkonen
CC: aran, jungejason
Differential Revision: 504
2011-06-23 12:28:16 -07:00
epriestley
e5bb756b51 Revert "Fixed 'Create Diff'"
See T246.

This reverts commit 6b7db27524.
2011-06-22 15:49:57 -07:00
epriestley
d6bfdf6ce7 Carry "Message-ID" across email replies to prevent Gmail conversation splitting
Summary:
See T251. In Gmail, conversations split if you reply to them and the next email
does not "In-Reply-To" your message ID. When an action is triggered by an email,
carry its Message-ID through the stack and use it for "In-Reply-To" and
"References" on the subsequent message.

Test Plan:
Live-patched phabricator.com and replied to a Maniphest thread in Gmail without
disrupting the thread. Locally replied to Maniphest and Differential threads and
verified Message-ID was carried across the reply boundary.

Reviewed By: rm
Reviewers: tcook, jungejason, aran, tuomaspelkonen, rm
CC: aran, epriestley, rm
Differential Revision: 498
2011-06-22 14:59:40 -07:00
epriestley
4a55af7857 Don't repeat the diff property table when clicking "show more"
Summary:
If you "chmod +x" a file and then generate a diff and click "show 20 lines" on
that diff, you get another janky copy of the property table. Render the property
table only for top-level rendering requests.

This started happening after D409, which fixed the far-more-obvious bug of these
things never showing up. We must have changed the logic at some point since this
side effect was surprising to me. :P

Test Plan:
Created a diff with changes and +x, clicked "show 20 lines".

  - Original diff had file property header table thing, showing the +x.
  - New context brought in by "show 20 lines" didn't have it anymore.

Reviewed By: jungejason
Reviewers: aran, jungejason, tuomaspelkonen
CC: aran, epriestley, jungejason
Differential Revision: 482
2011-06-20 15:54:20 -07:00
Jason Ge
3272008aef Create timeline event for revision creation
Summary:
create the event

Test Plan:
checked the timeline event was consumed successfully by
facebook daemon when the revision is created from the webUI or arc
command line.

Reviewed By: epriestley
Reviewers: epriestley, aran
CC: aran, epriestley
Differential Revision: 478
2011-06-20 11:27:14 -07:00
epriestley
3ec76f5f4a Index "owner" relationship in search correctly for Revisions
Summary:
The owners of a revision are only really the reviewers when the revision is in
NEEDS_REVIEW.

Also build a raw indexed document viewer so you can look at the index of a
document from the web interface.

Finally, reindex revisions when comments are added, not just when the revision
itself is edited.

Test Plan:
Toggled abandon/reclaim on a revision and verified the relationships indexed
properly.

Reviewed By: jungejason
Reviewers: aran, jungejason, tuomaspelkonen
CC: aran, epriestley, jungejason
Differential Revision: 470
2011-06-20 05:16:27 -07:00
tuomaspelkonen
6b7db27524 Fixed 'Create Diff'
Summary:
'Create Diff' with whitespace mode 'ignore-all' is borken, because the
line numbers get mixed up when creating a second diff for the
whitespace changes.

This should be fixed correctly at some point, but currently the
whitespace 'ignore-all' says 'Huge mess' in the comments and I didn't
want to make the mess any bigger.

Test Plan:
Tested that 'Create Diff' showed the diff correctly and a previously
created diff looked correct once the cache was disabled.

Reviewed By: epriestley
Reviewers: epriestley, jungejason
CC: aran, epriestley
Differential Revision: 473
2011-06-18 15:33:22 -07:00
epriestley
4e75080b66 Make naming, titles and layout more consistent between Maniphest and
Differential

Summary:
Make some display stuff more consistent.

Test Plan:
Looked at a task and a revision.

Reviewed By: jungejason
Reviewers: aran, jungejason, tuomaspelkonen
CC: aran, jungejason
Differential Revision: 462
2011-06-14 20:45:43 -07:00
epriestley
0a749ad51b Give unposted comments a distinct visual style
Summary:
See attached tasks. See D459 for the ability to merge tasks.

Test Plan:
Looked at posted and unposted inline comments.

Reviewed By: aran
Reviewers: edward, viyer, aran, jungejason, tuomaspelkonen
CC: aran, epriestley
Differential Revision: 461
2011-06-14 20:42:59 -07:00
tuomaspelkonen
501c001520 Added a big warning if reviewer is about to accept a diff with lint or unit
errors.

Summary:
Make sure reviewers know what they are doing.

Test Plan:
Tested with different diffs that had lint and unit problems.

Reviewed By: epriestley
Reviewers: epriestley, jungejason
CC: grglr, aran, epriestley, tuomaspelkonen
Differential Revision: 432
2011-06-13 11:49:31 -07:00
epriestley
17306b7a92 Provide basic keyboard navigation support for Differential.
Summary:
ReviewBoard has a fancier version of this feature that's more granular -- the
keyboard can focus on individual changes. I think that's good and intend to
implement something similar, but this gets us a step closer and gets rid of some
of the bookkeeping stuff like making shortcuts discoverable.

(I have another brnach with Maniphest merging which also uses fatcow icons,
which is why the README seems a little out of context.)

Test Plan:
Used "j" and "k" to jump between changesets. Pressed "?" and got a list of
available shortcuts.

Reviewed By: tuomaspelkonen
Reviewers: aran, jungejason, tuomaspelkonen
CC: moskov, aran, epriestley, tuomaspelkonen
Differential Revision: 412
2011-06-09 14:55:44 -07:00
epriestley
4ec31ef75c Provide basic capabilities to make Differential column width flexible
Summary:
- Make wrap width settable in PHP.
  - Dynamically generate max-width based on configurable maximum width.
  - Constrain non-diff elements to standard width.
  - Provide a configuration setting.

Test Plan:
Set various things to 100 / 120, as far as I could tell everything seemed to
render sensibly? This should have no effect on 80-col changes.

Reviewed By: jdperlow
Reviewers: jdperlow, tuomaspelkonen, jungejason, aran
CC: aran, jdperlow
Differential Revision: 413
2011-06-09 12:01:11 -07:00
epriestley
4d3ef5ee0b Remove nonfunctional links from Differential inline comment previews
Summary:
They currently have "Next", "Previous" and "Reply" links which don't work. Don't
render these links.

Test Plan:
Looked at inline previews, didn't see any silly/nonfunctional links.

Reviewed By: tuomaspelkonen
Reviewers: aran, jungejason, tuomaspelkonen
CC: aran, epriestley, tuomaspelkonen
Differential Revision: 419
2011-06-09 11:22:07 -07:00
tuomaspelkonen
2521621074 Added a conduit call to update arc unit results for a postponed test.
Summary:
It was not possible before to update arc unit results for a postponed
test.  This change makes it possible. Also the number of postponed tests
are shown in differential.

Let me know if this looks too Facebook specific.

Test Plan:
Tested the conduit call manually from Conduit Console and updated test
results for a diff that had 20 postponed tests.

Reviewed By: jungejason
Reviewers: epriestley, jungejason
Commenters: epriestley
CC: slawekbiel, aran, tuomaspelkonen, jungejason, epriestley
Differential Revision: 416
2011-06-09 10:45:58 -07:00
epriestley
2e8990e9e0 Store metadata with Differential and Maniphest comments, and store added
reviewers as metadata

Summary:
The "Add reviewer" implementation is super lazy right now since I didn't want to
do a schema change. Man up and add a column. I also plan to store "via"
information here (e.g., via email or via mobile).

NOTE: This schema change may take a while since the comment table is pretty big
in Facebook's install.

This needs a little CSS work but I think it's reasonable for now.

Test Plan:
Made comments on revisions and tasks. Added reviewers to a revision, got linked
names instead of a blob of text.

Reviewed By: aran
Reviewers: tomo, jungejason, tuomaspelkonen, aran
Commenters: tomo
CC: aran, tomo, epriestley
Differential Revision: 394
2011-06-09 10:43:25 -07:00
gc3
22c1b38655 herald: add the ability to execute a rule the first time only
Summary:
- added a new config class for representing the kind of repetition a rule has
(once, every time, first time only)

- added an email action to herald rules for differential to allow someone to get
an email but only the first one

- changed the herald rule ui to allow a user to pick the amount of repetition

Test Plan:
created a test rule and ran it over and over

Reviewed By: epriestley
Reviewers: epriestley, tuomaspelkonen
CC: aran, epriestley, gc3
Revert Plan:
Tags:

- begin *PUBLIC* platform impact section -
Bugzilla: #
- end platform impact -

Differential Revision: 357
2011-06-09 10:35:37 -07:00
epriestley
94d0adb140 Add "Undo" for editing Differential inline comments
Summary:
When a user hits 'cancel' on a 'new', 'edit', or 'reply' operation, add a little
"Changes discarded. __Undo__" insert so they can get their change back. No undo
for delete since there's an explicit prompt. Once this lands we can make
'escape' work again to close dialogs.

This change started feeling really good when I was merging all the duplicate
code and making things more consistent, but by the time I started writing client
rendering it felt gross. I'm not really thrilled with it but I guess it's a step
forward? The feature seems pretty OK in practice. Let me know how much barfing
this causes and I can try to remedy the most acute concerns.

This also fixes a bug where replies always (?) appear on the 'new' side of the
diff (I think?).

Test Plan:
Applied 'new', 'edit', 'delete' and 'reply' operations, pressed 'cancel' and
'okay' in each case, with and without changing text where relevant. All
behaviors seem to conform with expectations, except that canceling out of 'edit'
without changing the text gives you an option to undo when it shouldn't really.
There's no super easy way to get at the original text right now.

Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen
CC: simpkins, aran, epriestley
Differential Revision: 406
2011-06-08 10:44:01 -07:00
epriestley
112346ee61 Validate inline comment changeset IDs and revision IDs
Summary:
Historically, we had a bug at some point which caused inline comments to get
associated with changeset 0. Prevent that explicitly. See T108.

Test Plan:
Set "$changeset = 0" in the endpoint and got an exception.

Reviewed By: aran
Reviewers: aran
CC: aran
Differential Revision: 374
2011-06-07 19:58:45 -07:00
epriestley
9284c85876 Display property changes on files
Summary:
Never converted this TODO over from XHP.

NOTE: This looks terrible since the CSS didn't make it over, can one of you grab
the rules for .differential-property-table and .property-table-header? If they
aren't still in trunk, try history for html/intern/css/tools/differential/

Test Plan:
Made a property change, looked at it in Differentila.

Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran
CC: codeblock, aran
Differential Revision: 409
2011-06-07 19:58:15 -07:00
epriestley
404c3283cb Don't ignore internal whitespace changes in "Whitespace: Ignore All"
Summary:
When whitespace changes between two non-whitespace characters (e.g., in a
string), always treat it as a change.

Test Plan:
Disabled render cache, made internal and external whitespace changes, rendered a
diff, got internal change always marked and external change marked correctly
depending on mode.

Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen
CC: aran, tuomaspelkonen, epriestley
Differential Revision: 403
2011-06-07 11:14:16 -07:00
tuomaspelkonen
3c984eec56 Fix for escaping character "'".
Summary:
The position of the custom rule was incorrect.

Test Plan:
Tested that Facebook task remarkup, Differential remarkup, image macros, quotes,
and random characters were working correctly in comment preview

Reviewed By: jungejason
Reviewers: jungejason
CC: aran, jungejason
Differential Revision: 396
2011-06-02 12:00:29 -07:00
epriestley
df2cbf1d29 Don't render "Comment T1#1" links on previews
Summary:
I somehow missed this, we render silly nonsense in the comment previews right
now. Don't render these links if we're rendering a preview.

Test Plan:
Looked at comment previews, less nonsense.

Reviewed By: tuomaspelkonen
Reviewers: aran, jungejason, tuomaspelkonen
CC: aran, tuomaspelkonen
Differential Revision: 388
2011-05-31 19:24:03 -07:00
epriestley
74a2953ffd Support "!unsubscribe" in Differential reply handler
Summary:
See task. Allows users to unsubscribe via email.

Test Plan:
Used mail receiver to unsubscribe from a revision. Tested subscribe/unsubscribe
buttons. Verified "!unsubscribe" appears as an avilable action in email.

Reviewed By: ola
Reviewers: ola, mroch, jungejason, tuomaspelkonen, aran
CC: aran, epriestley, ola
Differential Revision: 385
2011-05-31 15:19:55 -07:00
epriestley
729d2f9c93 Remove .sql3 hacks from Differential
Summary:
See T178. D372 is the correct fix for this problem, hardcoding .sql3 is not.

Test Plan:
This code should be unreachable after T178 since these files will always be
marked as binary.

Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen
CC: elgenie, aran, tuomaspelkonen
Differential Revision: 373
2011-05-31 12:14:58 -07:00
epriestley
d96d515cc2 Add comment linking to Maniphest and Differential
Summary:
Allows you to link to comments with "D123#3" or "T123#3", then adds a pile of JS
to try to make it not terrible. :/

The thing I'm trying to avoid here is when someone says "look at this!
http://blog.com/#comment-239291" and you click and your browser jumps somewhere
random and you have no idea which comment they meant. Since I really hate this,
I've tried to avoid it by making sure the comment is always highlighted.

Test Plan:
Put T1#1 and D1#1 in remarkup and verified they linked properly.

Clicked anchors on individual comments.

Faked all comments hidden in Differential and verified they expanded on anchor
or anchor change.

Reviewed By: aran
Reviewers: aran, tomo, mroch, jungejason, tuomaspelkonen
CC: aran, epriestley
Differential Revision: 383
2011-05-31 11:11:19 -07:00
tuomaspelkonen
f076956f32 Added custom remarkup.
Summary:
Vendor specific markups are now possible.

Test Plan:
Tested with the Facebook specific tasks markup.

Reviewed By: jungejason
Reviewers: epriestley, jungejason
CC: aran, jungejason
Differential Revision: 349
2011-05-27 13:53:06 -07:00
tuomaspelkonen
19e10b2b5d Embedded youtube videos.
Summary:
Markup support for embedding Youtube videos.

Test Plan:
https://www.youtube.com/watch?v=Vw4KVoEVcr0 was embedded

Reviewed By: epriestley
Reviewers: epriestley
CC: aran, epriestley
Differential Revision: 353
2011-05-27 13:50:58 -07:00
jungejason
686ffafa21 Improve logging for syntax highlighting parsing
Summary:
add logging when syntax highlighting parsing throws exception.

Test Plan:
test when exception is thrown with non-php code. I couldn't
create a file to trigger an exception in running pygmentiza, so I
manually threw an exception to test it.

Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen, epriestley
CC: aran, tuomaspelkonen, jungejason
Differential Revision: 350
2011-05-26 17:51:22 -07:00
epriestley
3f11c8a602 Tweak Maniphest CSS, fix remarkup in description change views
Summary:
Various CSS tweaks and fixes:

  - Add remarkup styling to description change views, missed this before.
  - Fix CSS so that transactions with only one item (e.g., changed priority)
don't have weird floater underneath them.
  - Add more space between transaction items.
  - Make default background color lighter and less heavy.
  - Use beigey color for comment form in Maniphest.
  - Share more CSS between Maniphest and Differential (previews, feedback).
  - Move "Leap Into Action" call to Differential, replace Maniphest with
thematically-consistent "Weigh In" (obviously, Maniphest has a nautical theme).

Test Plan:
Browsed Maniphest and Differential in a couple browsers, styling all seems
correct.

Reviewed By: tomo
Reviewers: tomo, aran, jungejason, tuomaspelkonen
CC: anjali, aran, tomo
Differential Revision: 328
2011-05-22 13:26:55 -07:00
epriestley
b77e827bec Fix metadata rendering for files moves, etc.
Summary:
Some changeset metadata was not being correctly passed between the top-level
parser and the subparser, so it would be lost or incorrect when rendering
headers like "This file was moved from x to y." or rendering certain content
shields, like "the contents of this file were not modified".

Test Plan:
Created a new diff with a file move in it, rendered it, saw "This file was moved
from README to READYOU" correctly.

Reviewed By: aran
Reviewers: tuomaspelkonen, jungejason, grglr, aran
CC: aran, epriestley
Differential Revision: 321
2011-05-22 07:23:48 -07:00
epriestley
d202e71ef1 Use parallel syntax highlighting API in differential
Summary:
Use the new API from D322 to highlight text in parallel in Differential.

Test Plan:
Verified that pygemntize calls started within 20ms of one another in DarkConsole
(also: added a feature to let me do this) instead of running serially.

Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran
Differential Revision: 323
2011-05-22 07:21:10 -07:00
epriestley
7e675b6687 Allow email subject prefixes to be configured
Summary:
This is just fluff to let me mailfilter my local sandbox. Would also allow the
Facebook install to return to "[diff]" if eletuchy is still unhappy about this
change.

Test Plan:
Triggered maniphest/differential emails, had normal prefixes. Overrode prefixes
in my custom config, got sandbox-unique prefixes.

Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran
CC: elgenie, aran
Differential Revision: 291
2011-05-16 17:10:41 -07:00
epriestley
a69f217f98 Make reply-to fully work in Maniphest and Differential for open source
Phabricator

Summary:
Hook up the last pieces. This shouldn't impact the Facebook install, EXCEPT that
I removed "!accept" and added "!rethink" (plan changes). If you want to continue
supporting !accept, you should override the method in your subclass if you don't
already.

Test Plan:
Used the Mail Receiver test console to send mail to tasks and revisions.

Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran
Differential Revision: 289
2011-05-16 15:34:11 -07:00
epriestley
4b92b2cead Allow revisions to be edited from Maniphest
Summary:
You can currently attach tasks to revisions from Differential, but not revisions
to tasks from Maniphest. Allow editing from either side.

This logic is kind of tricky but the alternative was massive code duplication.

Test Plan:
Added and removed revisions from maniphest. Added and removed tasks from
differential.

This should have no impact on the Facebook install since none of this is used
there.

Reviewed By: aran
Reviewers: tomo, tuomaspelkonen, jungejason, aran
CC: aran, epriestley
Differential Revision: 288
2011-05-16 15:31:46 -07:00
tuomaspelkonen
ea47f33632 Fixed the table of contents standalone view link.
Summary:
Exception was thrown because there is no getRenderingReference function
for changeset.

Test Plan:
Sandbox loaded and links were working.

Reviewed By: grglr
Reviewers: grglr, epriestley
CC: aran, grglr
Differential Revision: 281
2011-05-13 13:28:21 -07:00
tuomaspelkonen
70807fb4e2 Fixed for /differential/diff
Summary:
It was broken

Test Plan:
Sandbox

Reviewed By: grglr
Reviewers: grglr, epriestley
CC: aran, grglr
Differential Revision: 280
2011-05-13 12:29:47 -07:00
epriestley
54154e4f48 Move "Rendering References" to the DifferentialChangesetParser level
Summary:
Separates changeset IDs from rendering. Now each changeset has a "rendering
reference" which is basically a description of what the ajax endpoint should
render. For Differential, it's in the form "id/vs". For Diffusion,
"branch/path;commit".

I believe this fixes pretty much all of the bugs related to "show more" breaking
in various obscure ways, although I never got a great repro for T153.

Test Plan:
Clicked "show more" in diffusion change and commit views and differential diff,
diff-of-diff, standalone-diff, standalone-diff-of-diff views. Verified refs and
'whitespace' were always sent correctly.

Made inline comments on diffs and diffs-of-diffs. Used "Reply".

Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen, jungejason, aran
CC: aran, tuomaspelkonen, epriestley
Differential Revision: 274
2011-05-13 06:10:57 -07:00
jungejason
f85e693b66 Fix method name after D254
Summary:
D254 removed DifferentialReplyHandler::getRevision(), but
is still using it in two places. Correct them.

Test Plan:
send email email handler and verified it works.

Reviewed By: epriestley
Reviewers: tuomaspelkonen, epriestley
CC: aran, epriestley
Blame Revision:
D254

Differential Revision: 277
2011-05-12 09:17:28 -07:00
epriestley
71efb46ba7 Support email multiplexing for private Reply-To addresses
Summary:
Provide a base PhabricatorMailReplyHandler class which handles the plumbing for
multiplexing email if necessary and supporting public and private reply handler
addressses. DifferentialReplyHandler now extends it, and a new
ManiphestReplyHandler also does.

The general approach here is that we have three supported cases:

  - no reply handler, default config, same as what we're doing now
  - public reply handler, requires overriding classes but just sets "reply-to"
to some address the install generates and still sends only one email
  - private reply handler, provides a default generation mechanism or you can
override it and splits mail apart so we send one to each recipient

Test Plan:
Sent email from Maniphest and Differential with and without
reply-handler-domains set.

Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, epriestley
Differential Revision: 254
2011-05-11 20:21:57 -07:00
tuomaspelkonen
43f6cc75f6 Added 'Next' and 'Previous' links to differential
Summary:
Browsing comments was a bit difficult without the possibllity to jump
between comments. These links will make the browsing easier.

Test Plan:
Tested on multiple diffs that the links were working correctly.

Reviewed By: epriestley
Reviewers: epriestley
CC: jungejason, aran, tuomaspelkonen, epriestley
Differential Revision: 266
2011-05-11 14:46:00 -07:00
tuomaspelkonen
e4f42dcd7d Correct whitespace option is passed to 'Show All Lines' request.
Summary:
Expanding lines duplicated some lines occasionally, because whitespace
option was different for the original request and the following request.

Test Plan:
Tested that the broken changeset was correct now.

Reviewed By: epriestley
Reviewers: epriestley
CC: jungejason, aran, epriestley
Differential Revision: 263
2011-05-10 18:05:13 -07:00
tuomaspelkonen
d5dfa5faf5 Never hide file contents if they have comments.
Summary:
Links to comments were not working because file was hidden after it was deleted.

Test Plan:
Tested that comment anchors were working correctly for deleted files.
Tested that generated files were still hidden.

Reviewed By: epriestley
Reviewers: epriestley
CC: jungejason, aran, epriestley
Differential Revision: 257
2011-05-09 18:01:22 -07:00
epriestley
e313b1d64a Add square brackets around new revision review requests
Summary:
These didn't get covered when we added square brackets to the rest of the emails
(so gmail can thread them properly).

Test Plan:
Created a local diff, got an email with brackets.

Reviewed By: tuomaspelkonen
Reviewers: rm, tuomaspelkonen, jungejason, aran
CC: aran, epriestley, tuomaspelkonen
Differential Revision: 256
2011-05-09 17:09:51 -07:00
epriestley
3ab334af93 Fix "reply" link in Differential
Summary:
Fixes the issue caused by rPa0af5b66437719dba6136579c051982ab275e6a0. Prior to
that patch, isCommentInNewFile() returned $comment->getIsNewFile(). While this
was often the wrong value, it came from the database and was the integer 1 if
true.

After the patch, the function returns 'true' as a boolean, which is passed to JS
and then back to PHP, interpreted as an integer, and evaluates to 0.

To avoid this issue in general, provide an isBool() method on AphrontRequest
which interprets this correctly.

I will also revert the revert of rPa0af5b66437719dba6136579c051982ab275e6a0 when
I land this.

Test Plan:
Clicked "reply" on the right hand side of a diff, got a right-hand-side inline
comment.

Reviewed By: rm
Reviewers: tuomaspelkonen, jungejason, aran, rm
CC: simpkins, aran, epriestley, rm
Differential Revision: 250
2011-05-08 21:23:15 -07:00
epriestley
3ee7b5380e Revert "Revert "Make DifferentialChangesetParser explicitly map display to storage for comments""
This reverts commit 1c2222f26f.
2011-05-08 21:22:25 -07:00
jungejason
162f34b8c8 Add reply handler for differential revision
Summary:
add email reply handler so that the user can reply to a
differential email to act on the revision. It generates the reply-to
email address, creates email body text with supported commands list, and
handle the action request on the differential revision.

Right now the reply-to handing is disabled in the config file. But a
site using Phabricator can enable it and implement a class
inheriting from DifferentialReplyHandler to enable customized email
handing.

Later we will need to add code to DifferentialMail.php to support
sending separate email to each email recipient to achieve better
security (see D226). The reply-to will be something like
D<revision_id>+<user_id>+<hash>@domain.com. We will create separate task
for it.

Test Plan:
tried comment on a revision from web UI and the email was
sent out as before without any change. When a subclass of
DifferentialReplyHandler is implemented and enabled, email's reply-to is
set and email text is added. Reply to the email with valid command did
create action to the revision.

Reviewed By: epriestley
Reviewers: tuomaspelkonen, epriestley, slawekbiel, dpepper
CC: aran, epriestley, jungejason
Differential Revision: 224
2011-05-08 17:08:47 -07:00
Ryan McElroy
846d625ed0 [differential] gmail-compatible emails
Summary:
Gmail ignores text inside of [square brackets] when deciding what to group
together. This diff does two things to create the right behavior for gmail:

  1. put the verb text inside of [square brackets] so different verbs don't
  break gmail threading.
  2. Add the Diff ID to the email thread, so different diffs with the same name
  don't group together.

Furthermore, to aid in distinguishing who is doing what when the from field
can't be spoofed, this diff adds the usename just before the verb. This works
quite well in the english language. For example:

  [Differential] [rm requested a review of] D1: [admin] Create arcconfig for
code reviews
  [Differential] [rm commented on] D1: [admin] Create arcconfig for code reviews

It's almost like a complete sentence. All it's missing is a period.

Test Plan:
Did it live on my test setup. Received emails with subjects that looked right.
Verified that gmail grouped the emails despite the different actions taking
place (tested: comments, planned changes, request review).

Reviewed By: epriestley
Reviewers: epriestley, jungejason
CC: aran, epriestley, rm
Differential Revision: 251
2011-05-08 02:04:16 -07:00
tuomaspelkonen
1c2222f26f Revert "Make DifferentialChangesetParser explicitly map display to storage for comments"
This reverts commit a0af5b6643.
2011-05-06 18:32:28 -07:00
epriestley
a0af5b6643 Make DifferentialChangesetParser explicitly map display to storage for comments
Summary:
This is a followup to D228. Basically, we use "changeset" (and, implicitly,
changesetID) for way too much stuff now.

One thing it can't possibly capture is the complete, arbitrary mapping between
the left and right sides of the displayed diff and the places we want to store
the left and right side comments. This causes a bunch of bugs; basically adding
inline comments is completely broken in diff-of-diff views prior to this patch.
Make this mapping explicit.

Note that the renderer already passes this mapping to
DifferentialChangesetParser which is why there are no changes outside this file,
I just didn't finish the implementation during the port.

This has the nice side-effect of fixing T132 and several other bugs.

Test Plan:
Made new-file and old-file comments on a normal diff; reloaded page, verified
comments didn't do anything crazy.

Expanded text on a normal diff, made new-file and old-file comments; reloaded
page, verified comments.

Repeated these steps for a previous diff in the same revision; verified
comments.

Loaded diff-of-diffs and verified expected comments appeared. Made new left and
right hand side comments, which almost work, see below.

NOTE: There is still a bug where comments made in the left-display-side of a
diff-of-diffs will incorrectly be written to the right-storage-side of the
right-display-side diff. However, this is an issue with the JS (the PHP is
correct) so I want to pull it out of scope for this patch since I think I need
to fix some other JS stuff too and this improves the overall state of the world
even if not everything is fixed.

Reviewed By: tuomaspelkonen
Reviewers: jungejason, tuomaspelkonen, aran, ola
CC: aran, tuomaspelkonen, epriestley
Differential Revision: 237
2011-05-06 15:01:52 -07:00
epriestley
dd8232b766 Hint 'arc' commands in Differential UI
Summary:
Point users toward 'arc amend', 'arc commit', 'arc patch' and 'arc export' since
no one is going to read 'arc help'.

There's kind of a tradeoff here where we're wasting a fair amount of UI space
for expert users with the patch/export hints but I think it's probably okay
since there's really no other way to figure out that these features exist.

Note that the "export" command given isn't complete (it needs --git or
--unified), but it will give you a useful error message when you run it, telling
you to specify --git or --unified. If it turns out users get confused by this,
let me know.

Test Plan:
Loaded a revision and looked at it. Faked it into 'accepted' status.

Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen, aran, jungejason
CC: aran, tuomaspelkonen, epriestley
Differential Revision: 242
2011-05-06 13:59:45 -07:00
epriestley
7ebd0d1efe Provide a web interface to view raw source text in Differential
Summary:
Add links to the 'standalone view' to grab the raw source text. I think this
operation is rare enough that it's okay to hide it like this. I changed
'Standalone View' to 'View Standalone / Raw' to improve discoverability.

This also fixes the broken Standalone View links in Diffusion by no longer
rendering them.

Test Plan:
viewed old and new sources for a changeset

Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen, jungejason, aran
CC: aran, tuomaspelkonen
Differential Revision: 243
2011-05-06 13:36:59 -07:00
epriestley
25dee6ecd2 Support email replies in Phabricator
Summary:
Provides support for per-user x per-object unique reply-to email addresses, plus
SMTP integration.

This does not actually make Phabricator use these in outbound email.

Test Plan:
Used test console to validate in-Phabricator routing and handling.

Piped emails into the "mail_handler.php" script to validate mail parsing.

Configured sendmail and sent mail to Phabricator.

Technically I haven't conducted all parts of this test on the same machine since
I lost the will to configure more SMTP servers after configuring phabricator.com

Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, epriestley, jungejason
Differential Revision: 226
2011-05-05 14:58:57 -07:00
epriestley
af06bfb1cc Make Changeset ID for render cache explicit
Summary:
DifferentialChangesetParser currently takes the Changeset object to mean a bunch
of different and mutually conflicting things implicitly:

  - Changeset ID is used to access the render cache.
  - Changeset ID is also used to tell the ajax endpoint what to render when
clicking "show more".
  - Changeset object has the actual changes.
  - Changeset ID and "oldChangesetID" are used to choose where to show inline
comments and how to attach new ones.

This indirectly causes a bunch of problems, like T141 and T132. Move toward
making all these separate things explicit. I want to have the changeset object
only mean the actual changes to display.

Test Plan:
Looked at changesets and verified the render cache was accessed correctly (and
not accessed in other cases).

Reviewed By: tuomaspelkonen
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, epriestley, tuomaspelkonen
Differential Revision: 228
2011-05-05 11:12:50 -07:00
epriestley
85b09c5ccb Make "Ignore All" the default whitespace mode
Summary:
This mode was fixed in D174 by grglr and is the best mode for almost all changes
once nonbroken, so make it the default.

This is also the mode which takes advantage of the render cache.

Test Plan:
Loaded a revision, got "Ignore All" as the default mode, and revision rendering
wasn't silly/broken.

Reviewed By: tuomaspelkonen
Reviewers: grglr, jungejason, tuomaspelkonen, aran
CC: aran, tuomaspelkonen
Differential Revision: 229
2011-05-05 11:12:09 -07:00
epriestley
03ebbccbc9 Restore image proxying to Remarkup
Summary:
Previously, Remarkup allowed you to paste in an image URI and get an inline
image. However, it did this by hotlinking the image which isn't so hot in an
open source product.

Restore this feature, but use image proxying instead. The existing image macro
code does most of the work.

There is a mild security risk depending on the network setup so I've left this
default-disabled and made a note about it. It should be safe to enable for
Facebook.

Test Plan:
Pasted in image and non-image links, got reasonable behavior. Verified proxying
appears to work. Verified that file:// shenanigans produce 400.

Reviewed By: tuomaspelkonen
Reviewers: aran, jungejason, tuomaspelkonen
Commenters: cpiro
CC: aran, cpiro, tuomaspelkonen
Differential Revision: 214
2011-05-03 18:49:06 -07:00
elynde
72dec7cd25 Faster Query for Differential Updates
Summary:
The old query was effectively

SELECT DISTINCT revision.* FROM `differential_revision` revision
        JOIN `differential_relationship` relationship ON
(relationship.revisionID = revision.id
            AND relationship.objectPHID in
('PHID-USER-a113b9ae4ee9524d0a20'))
          OR revision.authorPHID = 'PHID-USER-a113b9ae4ee9524d0a20'
        LEFT JOIN `differential_viewtime` viewtime ON
viewtime.viewerPHID in ('PHID-USER-a113b9ae4ee9524d0a20')
          AND viewtime.objectPHID = revision.phid
          AND GREATEST(1304022277, IFNULL(viewtime.viewTime, 0)) <
revision.dateModified
        ORDER BY dateModified DESC;

I'm not a db performance expert but it looks like the problem is that we
have to scan all revisions

  mysql> EXPLAIN SELECT DISTINCT revision.* FROM `differential_revision`
revision         JOIN `differential_relationship` relationship ON
(relationship.revisionID = revision.id             AND relationship.objectPHID
in ('PHID-USER-a113b9ae4ee9524d0a20'))           OR revision.authorPHID =
'PHID-USER-a113b9ae4ee9524d0a20'         LEFT JOIN `differential_viewtime`
viewtime ON viewtime.viewerPHID in ('PHID-USER-a113b9ae4ee9524d0a20')
AND viewtime.objectPHID = revision.phid           AND GREATEST(1304022277,
IFNULL(viewtime.viewTime, 0)) < revision.dateModified         ORDER BY
dateModified DESC;

+----+-------------+--------------+-------+--------------------+------------+---------+-------+--------+------------------------------------+
  | id | select_type | table        | type  | possible_keys      | key        |
key_len | ref   | rows   | Extra                              |

+----+-------------+--------------+-------+--------------------+------------+---------+-------+--------+------------------------------------+
  |  1 | SIMPLE      | revision     | ALL   | PRIMARY,authorPHID | NULL       |
NULL    | NULL  | 254127 | Using temporary; Using filesort    |
  |  1 | SIMPLE      | viewtime     | ref   | PRIMARY            | PRIMARY    |
66      | const |     17 | Distinct                           |
  |  1 | SIMPLE      | relationship | index | PRIMARY,objectPHID | objectPHID |
72      | NULL  | 966900 | Using where; Using index; Distinct |

+----+-------------+--------------+-------+--------------------+------------+---------+-------+--------+------------------------------------+

The new query is a lot faster

  mysql> EXPLAIN SELECT revs.* FROM ( (SELECT revision.* FROM
`differential_revision` revision WHERE revision.authorPHID in
('PHID-USER-a113b9ae4ee9524d0a20'))       UNION       (SELECT revision.* FROM
`differential_revision` revision JOIN differential_relationship rel WHERE
rel.revisionId = revision.Id AND rel.objectPHID =
'PHID-USER-a113b9ae4ee9524d0a20')) as revs     LEFT JOIN `differential_viewtime`
viewtime ON viewtime.viewerPHID = 'PHID-USER-a113b9ae4ee9524d0a20' AND
viewtime.objectPHID = revs.phid    WHERE GREATEST(1304022277,
IFNULL(viewtime.viewTime, 0)) < revs.dateModified   ORDER BY revs.dateModified;

+----+--------------+------------+--------+--------------------+------------+---------+-----------------------------------------+------+--------------------------+
  | id | select_type  | table      | type   | possible_keys      | key        |
key_len | ref                                     | rows | Extra
   |

+----+--------------+------------+--------+--------------------+------------+---------+-----------------------------------------+------+--------------------------+
  |  1 | PRIMARY      | <derived2> | ALL    | NULL               | NULL       |
NULL    | NULL                                    | 3021 | Using filesort
   |
  |  1 | PRIMARY      | viewtime   | ref    | PRIMARY            | PRIMARY    |
66      | const                                   |   17 | Using where
   |
  |  2 | DERIVED      | revision   | ref    | authorPHID         | authorPHID |
67      |                                         | 1040 | Using where
   |
  |  3 | UNION        | rel        | ref    | PRIMARY,objectPHID | objectPHID |
66      |                                         | 3822 | Using where; Using
index |
  |  3 | UNION        | revision   | eq_ref | PRIMARY            | PRIMARY    |
4       | phabricator_differential.rel.revisionID |    1 |
   |
  | NULL | UNION RESULT | <union2,3> | ALL    | NULL               | NULL
| NULL    | NULL                                    | NULL |
     |

+----+--------------+------------+--------+--------------------+------------+---------+-----------------------------------------+------+--------------------------+

Test Plan:
Loaded differential updates with new query, made sure page loaded quickly. Ran
the query from the command-line, it took about .4 seconds.

Reviewed By: Girish
Reviewers: tuomaspelkonen, jungejason, Girish
Commenters: btrahan
CC: aran, btrahan, elynde, Girish
Differential Revision: 181
2011-05-03 15:00:46 -07:00
elynde
2e96565f67 Faster 'All Revisions and Reviews' Query
Summary:
The 'All Revisions and Reviews' Query takes about 2 seconds when I run
it from the mysql command-line:

SELECT revision.*
FROM `differential_revision` revision LEFT JOIN
`differential_relationship` relationship
ON revision.id = relationship.revisionID
AND relationship.relation = 'revw'
WHERE revision.authorPHID in ('PHID-USER-a113b9ae4ee9524d0a20') OR
relationship.objectPHID in ('PHID-USER-a113b9ae4ee9524d0a20')
GROUP BY revision.id ORDER BY dateModified DESC

2419 rows in set (2.05 sec)

This takes about 0.1-0.2 seconds. Just dug into this because I guess
phabricator is haven't a bunch of mysql timeouts.

I don't know what the hell I'm doing; this is just faster

Test Plan:
Loaded 'All Revisions and Reviews' in sandbox

  http://phabricator.dev1577.snc6.facebook.com/differential/filter/related/

Made sure it had same results as the version in prod

  https://phabricator.fb.com/differential/filter/related/

Still slow to generate all that html

Reviewed By: epriestley
Reviewers: epriestley, aran, tuomaspelkonen, jungejason
CC: aran, epriestley
Differential Revision: 182
2011-05-03 11:51:03 -07:00
epriestley
8370f93048 Make X-Herald-Rules header sticky
Summary:
This isn't terribly elegant but it solves the problem without loss of
generality. We can pursue a more finessed solution later if it seems prudent.

Test Plan:
Created a revision matched by a blanket herald rule, and then commented on it.
Comment email had X-Herald-Rules header in it.

Reviewed By: aran
Reviewers: aran, tuomaspelkonen, jungejason
CC: aran
Differential Revision: 218
2011-05-03 06:06:57 -07:00
tuomaspelkonen
8c031db32b Added a link from a diff view to diff's revision if there is one.
Summary:
The diff view page should point to the revision, if the diff has
already been attached to one. The form to select the revision was also
removed in this case.

Let's me know if it should be possible to reattach a diff to a different
revision.

Test Plan:
Tested that a new diff created with '--preview' option was not attached to
any revision. After attaching the diff manually, made sure that the diff view
page
showed the link to the revision correctly.

Reviewed By: epriestley
Reviewers: jungejason, epriestley
CC: aran, epriestley
Differential Revision: 216
2011-05-02 18:34:33 -07:00
epriestley
fc2a2a8d09 Enable Pygments in actual source list renders
Summary:
I only actually enabled it in Remarkup previously.

Test Plan:
Created a python diff, got syntax highlighted.

Reviewed By: aran
Reviewers: aran, tuomaspelkonen, jungejason
CC: aran, epriestley
Differential Revision: 202
2011-05-02 14:26:24 -07:00
epriestley
6bec3d2e4f Simplify and demuddle MetaMTA send pathways
Summary:
I pretty shortsightedly made sending a side effect of save() in the case that a
server is configured for immediate sending. Move this out, make it explicit, and
get rid of all the tangles surrounding it.

The web tool now ignores the server setting and only repsects the checkbox,
which makes far more sense.

Test Plan:
Sent mails from Maniphest, Differential, and the web console. Also ran all the
unit tests. Verified headers from Maniphest.

Reviewed By: rm
Reviewers: aran, rm
CC: tuomaspelkonen, rm, jungejason, aran
Differential Revision: 200
2011-05-02 03:07:30 -07:00
epriestley
72e33c9e5a Fix a threading issue with Amazon SES
Summary:
Amazon SES does not allow us to set a Message-ID header, which means
that threads are incorrect in Mail.app (and presumably other applications
which respect In-Reply-To and References) because the initial email does not
have anything which attaches it to the rest of the thread. To fix this, never
rely on Message-ID if the mailer doesn't support Message-ID.

(In the Amazon SES case, Amazon generates its own Message-ID which we can't
know ahead of time).

I additionally used all the Lisk isolation from the other tests to make this
testable and wrote tests for it.

I also moved the idea of a thread ID lower in the stack and out of
DifferentialMail, which should not be responsible for implementation details.

NOTE: If you push this, it will cause a one-time break of threading for
everyone using Outlook since I've changed the seed for generating Thread-Index.
I feel like this is okay to avoid introducing more complexity here.

Test Plan:
Created and then updated a revision, messages delivered over Amazon
SES threaded correctly in Mail.app. Verified headers. Unit tests.

Reviewed By: rm
Reviewers: aran, tuomaspelkonen, jungejason, rm
Commenters: aran
CC: aran, rm, epriestley
Differential Revision: 195
2011-04-30 22:26:07 -07:00
epriestley
0e06cd85b7 Pygments support for Phabricator
Summary:
Thread a config option through, see D197.

Test Plan:
Source code gets highlighted.

Reviewed By: aran
Reviewers: aran, tomo, jungejason, tuomaspelkonen
CC: aran
Differential Revision: 198
2011-04-30 22:01:02 -07:00
epriestley
b3397030e6 Fix Differential "unsubscribe" so it properly blocks resubscription
Summary:
DifferentialRevision stores this field as a dictionary but the
Editor incorrectly passed it to Herald as a raw value array. Ideally the
property should be called unsubscribedDict or something but I'm increasingly
thinking we're going to centralize subscriptions for Adjutant or some similar
system so I'm disinclined to pursue the schema change just yet. I provided an
explicit raw-value-oriented API, at least.

Test Plan:
With two accounts, A and B: created universal Herald CC rule with
user B, created a revision with user A, had user B unsubscribe, had user A
update the revision. User B was not resubscribed.

Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran
CC: ola, aran, epriestley
Differential Revision: 188
2011-04-29 22:31:51 -07:00
epriestley
864e0d8a2f Fix XSS hole in inline comment editing
Summary:
Thanks to erling for the report. This was XSSable, although you could
only get yourself.

Test Plan:
Made a comment like "</textarea><h1>" and edited it before and after
the patch. Proper behavior with this patch.

Reviewed By: aran
Reviewers: erling, jungejason, tuomaspelkonen, aran
CC: aran
Differential Revision: 187
2011-04-29 20:27:25 -07:00
epriestley
78d33b1771 Silence an undeclared variable warning
Summary:
If you don't have a custom renderer, this variable does not get set
and emits a warning when you try to read it.

Test Plan:
Loaded page before and after change, warnings went away (lines
145 and 154 in old file).

Reviewed By: aran
Reviewers: tuomaspelkonen, aran
CC: jungejason, aran, epriestley
Differential Revision: 186
2011-04-29 20:27:02 -07:00
grglr
3f3304fd61 Differential whitespace mode IGNORE ALL now shows correct indentation
Summary:
Fixed buggy and incomplete logic for handling IGNORE ALL mode properly.
A subparser is used to parse the non-ws-ignoring changeset while a
ws-ignoring changeset is handed off to the original parser.  At a later
step, the original parser queries the subparser for its lines of text
(which are formatted properly due to being in non-ws-ignoring mode) and
uses them to replace the text in the ws-ignoring diff.

Task ID: 549940

Test Plan:
-turn off caching temporarily (the cached view is still indented
improperly)
-visit http://phabricator.dev1943.facebook.com/D242591
-note aligned, but not completely highlighted, indentation right above the
comments complaining about
indentation issues

Reviewed By: aran
Reviewers: tuomaspelkonen, jungejason, aran
Commenters: epriestley
CC: aran, epriestley, grglr
Differential Revision: 174
2011-04-28 16:10:59 -07:00
elynde
6fb24ba4a4 Merge branch 'master' of github.com:facebook/phabricator 2011-04-28 14:45:30 -07:00
elynde
bd0a4c0d04 Differential Updates View
Summary:
This adds a new view to differential called Updates.

The high-level goal of Updates is to enabled differential to be
effectively used without email notifications. I've tried doing things
like automatically deleting differential emails where I'm in the 'to'
line since they show up on the main diffential page but then there's
always the chance an important diff flies by without me seeing it. Also,
sometimes someone comments on a diff post-commit but differential
doesn't surface those diffs.

I re-created a test db on my devserver using mysqldump to get data on
revs > 230000 so I would have some test data. We need to add a simple
viewtime table but I didn't want to do that in production. Here's the
table:

  CREATE TABLE differential_viewtime (
    viewerPHID varchar(64) not null,
    objectPHID varchar(64) not null,
    viewTime int unsigned not null,
    PRIMARY KEY (viewerPHID, objectPHID)
  );

Issues:
  -Once we turn this on, all diffs will be 'unviewed'. What do you think
about a 'Clear All' button or something?
  -Maybe we should add a pager

This feature would be insanely useful, let me know what you think.

Test Plan:
Loaded Updates in my sandbox

  http://phabricator.dev1577.snc6.facebook.com/differential/filter/updates/

Clicked a diff, then went back, made sure diff disappeared from Updates
list

Reviewed By: tuomaspelkonen
Reviewers: epriestley, jungejason, tuomaspelkonen
Commenters: epriestley
CC: epriestley, elynde, tuomaspelkonen
Differential Revision: 169
2011-04-28 14:40:41 -07:00
adonohue
f910c379ce Modify whitespace default implementation
Summary:
Take Tuomas's first diff. Sorry, my bad on diff feedback.

Test Plan:
Load D240353 without any options and make sure 'ignore-trailing'
selected.

Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen
CC: epriestley, aran, tuomaspelkonen
Differential Revision: 172
2011-04-28 09:22:02 -07:00
tuomaspelkonen
2b0b39c4e4 Differential defaults to 'ignore-trailing' until indentation issues are solved.
Summary:
'ignore-all' is very confusing, especially for Python, because the indentation
is incorrect.

Test Plan:
Tested loading D240353 without any options and made sure 'ignore-trailing' was
selected and the diff looked correct.

Reviewed By: aran
Reviewers: jungejason, simpkins, aran, dpepper
Commenters: dpepper
CC: grglr, aran, dpepper, tuomaspelkonen
Differential Revision: 171
2011-04-27 18:55:46 -07:00
jungejason
a9e2e51b98 Enable updating task->revision assoc
Summary: add a conduit method to enable querying revisions' phid from
their revision_IDs, and another one to update the task->revision assoc.

Test Plan: for querying revision_phid method, tested empty, one, and two
revisions in the query. For the one to update the task->revision assoc,
I have another diff in facebook which verified it add and remove assoc
correctly.

Reviewers: tuomaspelkonen, epriestley

CC:

Differential Revision: 165
2011-04-26 19:14:47 -07:00
tuomaspelkonen
9367c41a28 Tasks can be defined in the commit message (Phabricator part).
Summary:
Phabricator did not support giving the task ids in the commit message.
Currently there is no default implementation for this, but there is a
Facebook specific implementation. At some point default implementation
for Maniphest tasks to revisions will be added.

Test Plan:
Tested with the Facebook specific implementation that task ids are
recognized in the commit message and tasks are automatically attached
to revisions.

The task attached to this revision was added from the commit message.

Reviewed By: jungejason
Reviewers: jungejason, epriestley
CC: dpepper, edward, gpatangay, tuomaspelkonen, jungejason
Differential Revision: 240996
2011-04-22 18:40:16 -07:00
bhiller
15e71e9bf4 [differential] Prevent un-closeable overlay
Summary:
Currently if I go to leave an inline comment and then decide
against it and hit <tab>-<enter>, the inline comment edit dialog closes
but the overlay doesn't disappear due to a JS error around trying to
access the markup property of null. This fixes that by setting the
payload to an array with empty markup so that the JS works and the
overlay disappears.

Test Plan:
clicked on the line number column, then pressed ok and
confirmed the dialog closed, the overlay disappeared and there were no
JS errors. also tested that for clicking the reply link and when editing
an existing inline comment.

Reviewed By: aran
Reviewers: epriestley, aran
CC: aran, bh
Revert Plan:
OK

Differential Revision: 154
2011-04-20 18:50:57 -07:00
tuomaspelkonen
a6c770217d Fixed whitespace options in Differential.
Summary:
Whitespace options could not be selected and the 'ignore_all' was not working.

Test Plan:
Made sure that whitespace is ignored correctly and selected option is handled
correctly.

Reviewed By: jungejason
Reviewers: jungejason
CC: epriestley, jungejason, tuomaspelkonen
Differential Revision: 149
2011-04-19 23:07:44 -07:00
tuomaspelkonen
1c3e21dda1 Fixed replies to go on the correct side of the diff in Differential.
Summary:
Replies to comments went always to the left side of the diff. There
was a confusion between 'is_new' and 'on_right'. Database wants the
information if the reply is on the left or on the right. The database
does not care if the comment is a reply or an original comment.

The code looks a bit confusing, because the database field is called
'isNewFile' and that is used to determine, which side the comment
applies to.

Test Plan:
Tested that every of combination of new comment/reply to the left/right side
worked when editing and after submitting the comment.

Reviewed By: jungejason
Reviewers: jungejason
CC: epriestley, jungejason
Differential Revision: 148
2011-04-18 17:15:00 -07:00
tuomaspelkonen
ecc32e4d08 Only table of contents are shown for large diffs by default.
Summary:
Differential used to only show diffs for the first 100 files. Now all
the files are shown in the table of contents and there is a link to
the standalone view for every file. The inline diffs can still be
seen, if user clicks "Show All Files Inline".

Inline comments can also be added in the standalone view, but there is
no form to submit them. The revision page must be reloaded to able to
submit the inline comment.

Test Plan:
Changed the limit to three for testing purposes and checked that a diff of
mine with 5 files had the links to the standalone views. Made sure that
adding a comment in a standalone view worked and that after reloading the
revision page the comment was visible. Changed the limit back to 100 and
made sure that my diff had all the files inline and that the anchor links
were working.

Reviewed By: jungejason
Reviewers: jungejason
CC: epriestley, simpkins, jungejason, tuomaspelkonen
Differential Revision: 147
2011-04-18 16:26:08 -07:00
tuomaspelkonen
ebdcedc87f Better error messages for nonexistent CCs, reviewers, etc. in arc
Summary:
The existing message is confusing and might cause people to think
that there is something wrong in the field name, e.g., 'CC' instead
of the value.

Test Plan:
Tested by putting random crap in CC and Reviewers and made sure the
error message was using the new format.

Reviewed By: jungejason
Reviewers: epriestley, jungejason
CC: mdevine, jungejason
Differential Revision: 145
2011-04-15 15:23:59 -07:00
jungejason
825748586b Create timeline events for comments
Summary: to enable publishing to internfeed, we create timeline events
for the comments.

Test Plan: create a test revision, comments on it and check if it
creates a timeline event with correct data at /daemon/timeline.

Reviewers: epriestley,tuomaspelkonen

CC:

Differential Revision: 144
2011-04-14 22:18:01 -07:00
tuomaspelkonen
40554e2d8a Removed broken inline comment anchors.
Summary:
Inline comment anchros were present for all different diffs inside a
revision. They were only working for the current diff. Removed the
links that were for different diffs.

I couldn't get the automatic linking to other diffs working, probably
because the anchors didn't work when the page was reloaded. This is also
a bit confusing if the diff changes when clicking on anchor.

We might want to carry all the comments along in the future, but I
don't think it's needed at the moment.

Test Plan:
Tested on a revision, which had inline comments for differerent diffs
that only the comments for the latest diff had anchors when the page
was loaded. Changed the diff manually on the page and made sure the
anchors for that diff were working correctly and the anchors for the
latest diff were not available.

Reviewed By: epriestley
Reviewers: epriestley
CC: jungejason, epriestley
Differential Revision: 142
2011-04-14 19:03:41 -07:00
adonohue
acd1cc8d22 "Reply" for inline comments
Summary:
"Reply" for inline comments

Test Plan:
Add consecutive and overlapping new inline comments and replies.

Reviewed By: epriestley
Reviewers: epriestley
CC: aran, epriestley
Differential Revision: 143
2011-04-14 18:31:21 -07:00
tuomaspelkonen
28155a5115 Fixed Differential quotes rendering. (phabricator)
Summary:
When quoting someone's text in differential, the text was not properly
rendered. Now the quoted text is rendered nicely and it's easy to locate.

Test Plan:
Disabled the inline cache, Checked that quotes in D211086 are rendered
correctly. Started writing a new comment starting with '>' and made
sure the new comment was rendered correctly while writing it.

Reviewed By: epriestley
Reviewers: epriestley
CC: jungejason, tuomaspelkonen, epriestley
Differential Revision: 138
2011-04-14 16:20:46 -07:00
adonohue
9757edb268 Fix "Resign as Reviewer" to appear only when actually a Reviewer
Summary: Is this an appropriate place to say "herp derp?"

Test Plan: Check for Resign as Reviewer for the following table
                          waiting for review | accepted | committed
Am actually reviewer
Am not reviewer but CC'ed
Unrelated

Reviewers: epriestley

CC:

Differential Revision: 140
2011-04-14 15:14:44 -07:00
tuomaspelkonen
f7fe75f756 Image macros for Phabricator!
Summary:
Added long waited image macro support for differential and others.

Test Plan:
Tried a couple of different macros and made sure they appear nicely
in the comment preview. Made sure that the normal comments are shown
correctly.

Reviewed By: epriestley
Reviewers: epriestley
CC: jungejason, tuomaspelkonen, epriestley
Differential Revision: 129
2011-04-13 20:08:13 -07:00
tuomaspelkonen
2bd51fd125 Implemented "Plan Changes" action for differential.
Summary:
Old differential allowed users to plan changes for their own revisions.
This feature is now available in Phabricator version of differential.

Test Plan:
Tested by selecting "Plan Changes" for one of my own aceepted revisions.

Reviewed By: epriestley
Reviewers: epriestley
CC: jungejason, epriestley
Differential Revision: 130
2011-04-13 16:58:22 -07:00
epriestley
d7c27dafd2 Add config flags for Differential action link stuff. 2011-04-13 12:12:02 -07:00
adonohue
6d20a57ce0 Instant subscribe/unsubscribe
Summary:
No workflow dialog when subscribing or unsubscribing from a Differential
Revision

Test Plan:
Repeatedly subscribe and unsubscribe to a revision. Enter refractory period due
to
speed of page gen.

Reviewed By: epriestley
Reviewers: epriestley
CC: epriestley
Differential Revision: 128
2011-04-12 18:25:46 -07:00
tuomaspelkonen
79b32afeec Added possibility to define custom action links and properties in differential.
Summary:
There was a need to add old facebook specific action links and properties
back to differential.

Test Plan:
Tested that all the facebook specific links work for multiple
different revisions.

Reviewed By: epriestley
Reviewers: epriestley
CC: jungejason, tuomaspelkonen, epriestley
Differential Revision: 127
2011-04-12 16:23:57 -07:00
epriestley
1bba2c9913 Fix XSS in path names of inline comment list. 2011-04-11 20:28:30 -07:00
epriestley
461f608b5c Fix diff-of-diffs to respect the right-hand-side diff. 2011-04-11 12:32:29 -07:00
adonohue
f9a59c9f49 Enable "Resign as Reviewer" from Differential Revision View UI
Summary:
Enable "Resign as Reviewer" from Differential Revision View UI

Test Plan:
Look at revision that I am a reviewer on and that I am not.

Reviewed By: epriestley
Reviewers: epriestley
CC: epriestley
Differential Revision: 120
2011-04-11 10:53:01 -07:00
epriestley
a2a6509dc8 Restore linking of Diffusion commits. 2011-04-11 03:02:19 -07:00
epriestley
675ce22bf4 Allow Maniphest and Differential object lists to be filtered by user. 2011-04-11 02:06:13 -07:00
epriestley
fb020ae27d Improve unit test detail panel in Differential. 2011-04-10 17:47:47 -07:00
epriestley
75b11d6d7d Lint and unit star support.
Test plan: quack

Differential Revision: 35
2011-04-10 17:19:01 -07:00
epriestley
689eb11ff3 Don't send a blank "COMMITS" field for mark-committed revisions. 2011-04-10 14:36:04 -07:00
epriestley
24d01d39c6 Improve linebreak / paragraph behavior. 2011-04-10 14:09:45 -07:00
epriestley
c0629f5ff1 Declown todo.com/herald. 2011-04-10 12:39:05 -07:00
epriestley
e6c5d6c8ae Properly support mailing lists, with actual testing! 2011-04-10 10:16:14 -07:00
epriestley
69bfd8e598 Allow mailing lists to be referenced by name in commit messages. 2011-04-10 09:41:10 -07:00
epriestley
9d4d258a0b Fix up newline formatting in Differential emails a bit. 2011-04-10 09:08:11 -07:00
epriestley
fa38b70ba6 Fix message IDs and Herald URIs. 2011-04-10 08:46:39 -07:00
epriestley
17ea3cfab5 Link to commits when sending Differential Commit notifications.
Wholly untested. Unlikely to work.
2011-04-10 08:31:18 -07:00
epriestley
2c1ed9fd17 Render the cache correctly in DifferentialRevisionCommentView. 2011-04-09 22:34:41 -07:00
epriestley
dd1d593786 Don't send the action cue, use production URIs. 2011-04-09 22:33:31 -07:00
jungejason
cca849c762 Fix 'Transcript' links in Differential
Summary:
add filtering for MetaMTA transcripts, add Herald
transcripts, also fixed PhabricatorObjectHandleData to support commits.
Note that paging in the transcripts pages will be in a different diff.

Test Plan:
test the transcripts for both MetaMTA and Herald.

Reviewed By: epriestley
Reviewers: epriestley, tuomaspelkonen
CC: jungejason, epriestley
Differential Revision: 114
2011-04-08 01:10:05 -07:00
epriestley
22297b71a0 Close the loop on Diffusion commits posting back to Differential. 2011-04-07 21:59:42 -07:00
epriestley
68687a059a Reintegrate Herald with Differential. 2011-04-07 20:59:51 -07:00
epriestley
361ec78b03 Add missing includes from XHPAST parse bug. 2011-04-06 23:14:58 -07:00
epriestley
11ea93260a Sync up UUIDs and create project configs. 2011-04-05 21:55:04 -07:00
epriestley
8f5d01d451 Get rid of +x on a bunch of nonexecutable files because I failed to set
"create mask" on SMB. :/
2011-04-02 16:47:20 -07:00
tuomaspelkonen
28fe9f4eca User preferences ported from tools
Summary:

Internal tools, e.g., differential and diffusion  have user defined
preferences for monospaced font and the option for showing either the
name of the tool or the glyph of the tool in the title.

These preferences were ported to phabricator. These preferences can be
modified in /preferences/ and they both affect diffusion and differential
at the moment.

Test Plan:

* Created an empty database
* Loaded /preferences/ and modified the monospaced font and clicked save
* Confirmed that the same page was loaded with the message that preferences
  have been saved and that the example text used the user defined font

* in /preferences/ changed the option to show tool names as plain text and
  clicked save
* Confirmed that the same page was loaded with '[Preferences]' in the title
  instead of a glyph

* These same tests were also executed for differential and diffusion

Reviewers: epriestley
CC: jungejason

Differential Revision: 91
2011-03-31 13:44:20 -07:00
epriestley
60918c9a9e Improved change view support. 2011-03-30 19:22:11 -07:00
epriestley
3d5f03607b Rough cut of Diffusion change view. 2011-03-30 17:36:32 -07:00
epriestley
f249ffc882 Fix Remarkup styling in Maniphest and Differential
Summary:
This makes some of the line spacing, paragraph spacing and layout
less terrible. In particular, fixes code blocks inside Differential inline
comments.

Test Plan:
Looked at Maniphest Tasks, Differential Revisions and Differential
inline comments with various flavors of remarkup in them.

Reviewed By: jungejason
Reviewers: tuomaspelkonen, jungejason
CC: jungejason
Differential Revision: 89
2011-03-29 22:02:02 -07:00
tuomaspelkonen
31dda42903 Changed 'Comment' to 'Clowncopterize' in Differential
Summary:
Back to the authentic roots.

Test Plan:
Verified that the button had the correct text in the UI.

Reviewed By: epriestley
Reviewers: epriestley
CC: epriestley
Differential Revision: 86
2011-03-29 16:09:21 -07:00
epriestley
370ba966db Rough cut of herald transcripts and Differential adapter. 2011-03-24 21:32:26 -07:00
Slawek Biel
4c9850278c [Differential: removing reviewed/reviewers check from the parser]
Summary:
As we've discussed the check is not needed.

Test Plan:
- php -l

Reviewed By: epriestley
Reviewers: jungejason, epriestley
CC: epriestley
Revert Plan:
sure

Other Notes:

Differential Revision: 81
2011-03-22 12:34:29 -07:00
epriestley
ad7a389106 Fix a bug where too many changesets make it to display.
Summary:

Test Plan:

Reviewers:

CC:
2011-03-07 22:02:46 -08:00
epriestley
e98d1ae9be Enhance Phabricator capabilities for differential.getcommitmessage (2)
Summary: Enables --edit workflow to behave properly [edit]

Test Plan: meta [what]

Reviewers:

CC: epriestley

Differential Revision: 26
2011-03-04 17:14:48 -08:00
jungejason
9bc04fe03d Change hard-coded PHID types to constants.
Summary:
add a constants module
src/applications/phid/constants/PhabricatorPHIDConstants.

Test Plan:
Execute applications which were using the hard-coded string.

Differential Revision: 44
Reviewed By: epriestley
Reviewers: epriestley
CC: epriestley
2011-03-03 12:00:53 -08:00
epriestley
eccc76dae6 Fix some issues caught by HipHop, and work around some issues
caused by HipHop.
2011-02-26 21:01:42 -08:00
epriestley
3d796f0b59 Default to 'active'.
Summary:

Test Plan:

Reviewers:

CC:
2011-02-22 23:39:09 -08:00
epriestley
27dc5eabd7 Make differential revision list less ambiguous.
Summary:

Test Plan:

Reviewers:

CC:
2011-02-22 23:37:43 -08:00
epriestley
c6c6a2cc4c Unify headsup action lists.
Summary:

Test Plan:

Reviewers:

CC:
2011-02-20 13:02:40 -08:00
epriestley
8fe3c80b6c Make header alignment/layout less horrible.
Summary: <table />s repruhsent

Test Plan:

Reviewers:

CC:
2011-02-20 10:00:29 -08:00
epriestley
c5c31d7eb9 Make auto-ccs work, delete some dead code, make inlines show up in email,
and resolve a display caching issue.

Summary:

Test Plan:

Reviewers:

CC:
2011-02-19 15:06:22 -08:00
epriestley
a04a88a843 Make subscribe/unsubscribe work properly on Revisions.
Summary:

Test Plan:

Reviewers:

CC:
2011-02-19 14:36:13 -08:00
epriestley
eec3e8e3aa Move object-selector closable to being usable.
Summary:

Test Plan:

Reviewers:

CC:
2011-02-18 15:12:15 -08:00
epriestley
147d2e2e3d Rought cut of search.
Summary: Botched this pretty badly in git so we'll see how much I broke. :/

Test Plan:

Reviewers:

CC:
2011-02-14 15:34:20 -08:00
epriestley
fcc467fe5e Fix some derp with Differential revision formatting.
Summary:
  - Prevent long comments from expanding the inline box.
  - Make anchor links to inlines work properly.
  - Get rid of "pre" white-space formatting in inline comments.

Test Plan: Viewed a revision with crazy comments in it.

Reviewers:

CC:
2011-02-13 16:00:21 -08:00
epriestley
8784b5bd3b Link "DNNNN" and "TNNNN" in Phabricator remarkup.
Summary: Autolink Differential and Maniphest objects.

Test Plan: Typed "D12345" and "T12345" into the Differential comment preview,
got links. Typed "http://www.elsewhere.com/D12345" and got a single link to
that URI, not a mess where the D12345 part linked incorrectly.

Reviewers: aran

CC:

Differential Revision: 35
2011-02-11 18:07:45 -08:00
epriestley
70fd45b139 Use the correct actor handle when sending DifferentialNewDiffMail.
Summary:

Test Plan:

Reviewers:

CC:
2011-02-09 22:46:18 -08:00
epriestley
56880cc92e Discard no-op transactions from transaction groups.
Move assignees to CC as a side effect of reassignment.

Summary:

Test Plan:

Reviewers:

CC:
2011-02-09 12:57:38 -08:00
epriestley
9eccb3e23d Don't try to render RevisionRefs since they haven't been brought over yet.
Summary:

Test Plan:

Reviewers:

CC:
2011-02-09 12:47:41 -08:00
epriestley
616c22eae2 I think this improves email a good deal, although it's hard to really test it
since Exchange has been down for like 30 minutes now. Push & Pray!

Summary:

Test Plan:

Reviewers:

CC:
2011-02-09 11:11:24 -08:00
epriestley
e3a91902b7 This might make emails work better.
Summary:

Test Plan:

Reviewers:

CC:
2011-02-09 09:58:48 -08:00
epriestley
1369bd3dd4 Conduit: differential.getcommitmessage
Summary:

Test Plan:

Reviewers:

CC:
2011-02-09 09:41:26 -08:00
epriestley
bb8bc4ff54 Make commit message parsing at least sort of work.
Summary:

Test Plan:

Reviewers:

CC:
2011-02-09 09:17:21 -08:00
epriestley
7c209590c1 Use "(?P<" for named capturing subpatterns.
Summary:

Test Plan:

Reviewers:

CC:
2011-02-09 09:13:07 -08:00
epriestley
5817840894 differential.createrevision
Summary: Conduit method.

Test Plan: Meta

Reviewers:

CC:

Differential Revision: 23
2011-02-06 13:56:11 -08:00
epriestley
787eb778b1 differential.parsecommitmessage
Test plan: meta
2011-02-06 13:46:39 -08:00
epriestley
1c586db9be Revision comment drafts. 2011-02-05 16:57:21 -08:00
epriestley
50bfcd0a30 100-changeset cutoff. 2011-02-05 16:43:28 -08:00
epriestley
f0934e655f Fix some typos, restore outline styles. 2011-02-05 12:33:53 -08:00
epriestley
de2a9c634c Differential lazyweb diff create workflow. 2011-02-05 12:20:18 -08:00
epriestley
53f2a433f7 Collapse comments in long threads. 2011-02-05 11:06:56 -08:00
epriestley
18c0515440 Add reviewers workflow fixes. 2011-02-04 22:45:42 -08:00
epriestley
38e53df53c Make revision CCs work properly. 2011-02-04 18:16:02 -08:00
epriestley
905870d793 Various fixes, particularly on the revision update workflow. 2011-02-04 17:53:14 -08:00
epriestley
8b7755b834 Changeset anchor links. 2011-02-04 16:18:08 -08:00
epriestley
12df78ed6a Rough cut of diff-of-diffs. 2011-02-03 15:41:58 -08:00
epriestley
5fd28d35d9 Diff-of-diffs radio button logic. 2011-02-03 13:26:52 -08:00
epriestley
c93dd9c090 Flesh out some DarkConsole stuff. 2011-02-02 22:38:42 -08:00
epriestley
5eb9bce6d4 Remove some commented-out versions of some stuff that works now. 2011-02-02 19:44:12 -08:00
epriestley
4aa72aa5ff Inline comment-related fixes. 2011-02-02 19:38:43 -08:00
epriestley
223ac18287 Straighten out some reviewer-realtd horribleness. 2011-02-02 17:38:03 -08:00
epriestley
ba26f67687 Add some comment caching stuff. 2011-02-02 16:36:53 -08:00
epriestley
c5ce156e71 Edit/Delete for inline comments 2011-02-02 13:51:45 -08:00
epriestley
759eec3a77 Very rough cut of DarkConsole + XHProf 2011-02-02 13:48:52 -08:00
epriestley
246cba2bf0 InlineComments 2011-02-01 21:09:28 -08:00
epriestley
9dac0ed9f1 Bring in JX.Workflow and the inline commenting behavior, plus sync Javelin. 2011-02-01 15:52:04 -08:00
epriestley
233953bc4a Straighten out the "show more context" stuff. 2011-01-31 20:38:13 -08:00
epriestley
6a94f054d2 Fix array/string type bug in comment saving. 2011-01-31 20:03:43 -08:00
epriestley
4736b320ff Differential comment previews. 2011-01-31 18:05:20 -08:00
epriestley
400524abf8 Get rid of some local.aphront references. 2011-01-31 16:59:01 -08:00
epriestley
e28c2e8899 Profile image stuff 2011-01-31 16:00:42 -08:00
epriestley
03fec6e911 PhabricatorEnv
'infratructure' -> 'infrastructure' (rofl)
Recaptcha
Email Login / Forgot Password
Password Reset
2011-01-31 11:55:26 -08:00
epriestley
29f7219a49 CSRF / Logout 2011-01-30 18:52:29 -08:00
epriestley
58d1506499 Clean up the revision list view a bit. 2011-01-30 18:24:57 -08:00
epriestley
a34f946b8e Remarkup for Differential comments. 2011-01-30 13:20:56 -08:00
epriestley
9c3d00de03 Write pathway for Differential Comments 2011-01-30 12:08:58 -08:00
epriestley
5f32fb3283 AddCommentView 2011-01-30 11:02:22 -08:00
epriestley
c55b1ed9bb Basic Differential revision feedback view. 2011-01-30 10:37:58 -08:00
epriestley
7b9c4c5f61 DifferentialRevisionView 2011-01-29 15:37:41 -08:00
epriestley
9f1659b4c4 DifferentialRevisionList 2011-01-27 13:12:07 -08:00
epriestley
63c842105c Differential Revision relationships 2011-01-27 10:07:59 -08:00
epriestley
de1fb8ac7d DifferentialRevisionEditor 2011-01-26 17:17:49 -08:00
epriestley
ccf7df6093 Authentication 2011-01-26 15:34:20 -08:00
epriestley
2b7b03ad0c ChangesetParseCache 2011-01-25 16:10:48 -08:00
epriestley
a997e77693 RevisionList 2011-01-25 16:02:36 -08:00
epriestley
14ed5482ab Typeaheads 2011-01-25 14:41:32 -08:00
epriestley
14163aaeef RevisionEdit 2011-01-25 13:40:57 -08:00
epriestley
16ad2386d8 Javelin integration. 2011-01-25 12:41:55 -08:00
epriestley
7bb0db1365 Celerity, a Haste-style static resource management system. 2011-01-25 10:18:44 -08:00
epriestley
52126c5479 Add proper syntax highlighting with Phutil. 2011-01-24 17:39:14 -08:00
epriestley
a3df19976f DifferentialChangesetView 2011-01-24 17:24:40 -08:00
epriestley
e85ecd03de DifferentialDiffView 2011-01-24 14:44:58 -08:00
epriestley
7779c67668 Conduit: differential.setdiffproperty 2011-01-24 12:14:32 -08:00
epriestley
2bea542920 Conduit: user.find 2011-01-24 12:05:49 -08:00
epriestley
dec8bac3a3 Conduit: differential.creatediff 2011-01-24 11:28:12 -08:00
epriestley
fb9b0db726 Move 'review' -> 'differential'. 2011-01-23 18:10:20 -08:00