1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-09-22 10:18:48 +02:00
Commit graph

411 commits

Author SHA1 Message Date
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
epriestley
3c30ea41f1 Enable multiple web sessions
Summary:
Conduit already has multiple-session code, just move it to the main
establishSession() method and set a web session limit larger than 1.

NOTE: This will log everyone out since we no longer look for the "web" session,
only for "web-1", "web-2", ..., etc. Presumably this doesn't matter.

Test Plan:
Applied patch, was logged out. Logged in in Safari. Verified I was issued
"web-1". Logged in in Firefox. Verified I was issued "web-2".

Kept logging in and out until I got issued "web-5", then did it again and was
issued "web-1" with a new key.

Ran conduit methods and verified they work and correctly cycled session keys.

Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen, jungejason, aran
Commenters: jungejason
CC: rm, fzamore, ola, aran, epriestley, jungejason, tuomaspelkonen
Differential Revision: 264
2011-05-12 18:45:19 -07:00
jungejason
a457f85101 Added two fields to be returned from differential.find
Summary:
Add the two fields in the conduit call.

Test Plan:
Called it with two user phids and verified the added fields are
returned.

Reviewed By: aran
Reviewers: tuomaspelkonen, aran
CC: aran
Differential Revision: 267
2011-05-12 15:16:22 -07:00
epriestley
f9f8ef0e6e Admin and disabled flags for users
Summary:
Provide an "isAdmin" flag for users, to designate administrative users.

Restore the account editing interface and allow it to set role flags and reset
passwords.

Provide an "isDisabled" flag for users and shut down all system access for them.

Test Plan:
Created "admin" and "disabled" users. Did administrative things with the admin
user. Tried to do stuff with the disabled user and was rebuffed. Tried to access
administrative interfaces with a normal non-admin user and was denied.

Reviewed By: aran
Reviewers: tuomaspelkonen, jungejason, aran
CC: ccheever, aran
Differential Revision: 278
2011-05-12 11:17:50 -07:00
epriestley
03b56c1035 Explicitly list To and Cc in multiplexed email
Summary:
When we multiplex email, add information to the body with an explicit list of
recipients. Also add some headers if people want to write mail rules.

Test Plan:
Commented on a task and a revision, got reasonable looking emails about them.

Reviewed By: jungejason
Reviewers: aran, jungejason, tuomaspelkonen
CC: aran, jungejason, epriestley
Differential Revision: 272
2011-05-12 11:14:28 -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
adonohue
30db15bc83 Add name to path.getowners
Summary:
Also retrieve package name in path.getowners, useful for 'arc owners'.

Test Plan:
Conduit console

Reviewed By: epriestley
Reviewers: epriestley
CC: aran, epriestley
Differential Revision: 273
2011-05-11 21:27:41 -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
adonohue
9f12ffbaba Clean up path.getowners
Summary:
Change some stuff in path.getowners for clarity

Test Plan:
Conduit test console

Reviewed By: epriestley
Reviewers: epriestley, Leon
CC: aran, epriestley
Differential Revision: 271
2011-05-11 19:59:30 -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
epriestley
47895afbd1 Show transaction types in Maniphest previews
Summary: When rendering a Maniphest comment preview, also render a preview of the transaction.

Test Plan: tested previews for all transaction types, got reasonable renders
2011-05-11 04:17:48 -07:00
epriestley
c51eb2696d Add drafts to Maniphest
Summary:
Use PhabricatorDraft to save text when previewing it so you don't lose stuff if
your browser crashes.

Test Plan:
Typed some text, closed/reopened the page, text was still there. Submitted
comment, text vanished.

Reviewed By: tuomaspelkonen
Reviewers: rm, tuomaspelkonen, jungejason, aran
CC: aran, tuomaspelkonen
Differential Revision: 262
2011-05-10 19:01:11 -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
jungejason
e8744e7eeb Enable pygment highlighting for diffusion
Summary:
set the config for the diffusion highlighter. In D202 we
enabled it for differential already.

Test Plan:
opened a python file in diffusion and verified it is highlighted.

Reviewed By: epriestley
Reviewers: epriestley, tuomaspelkonen
CC: aran, epriestley
Differential Revision: 260
2011-05-10 16:13:20 -07:00
epriestley
a64d5177a7 Spoof usernames with Amazon SES
Summary:
When I tested this earlier I was incorrectly interpreting PHPMailer errors as
SES errors. This works fine as long as you get around the peculiarities of
PHPMailer.

Test Plan:
Sent email to myself, received email from a human-readable address in my mail
client.

Reviewed By: rm
Reviewers: rm, tuomaspelkonen, jungejason, aran
CC: aran, rm, epriestley
Differential Revision: 246
2011-05-10 16:12:28 -07:00
epriestley
90364cafdc Add comment previews to Maniphest
Summary:
Moves shared code from Differential and Maniphest comment previews into
PhabricatorShapedRequest, and then implements Maniphest previews.

This doesn't implement comment drafts, I'll follow up with that but it requires
this and is completely separable.

This also always shows the preview as "commented" rather than previewing the
actual transaction. I'll follow up with that but I think it will require a
little factoring and this is useful even without transaction details.

I need to tweak the styling a bit too.

Test Plan:
Typed text in Maniphest and Differential. Toggled Differential action. Made
comments.

Reviewed By: rm
Reviewers: rm, tuomaspelkonen, jungejason, aran
CC: aran, rm
Differential Revision: 258
2011-05-10 14:35:00 -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
tuomaspelkonen
d63f3d479c Fixed 'Modified' link in Diffusion History View.
Summary:
'Modified' link always pointed to the latest change. Fixed the link.

Test Plan:
Tested that clicking 'Modified' took me to the correct place. Made sure that
Diffusion commit view was still working correctly.

Reviewed By: epriestley
Reviewers: epriestley
CC: jungejason, aran, epriestley
Differential Revision: 255
2011-05-09 16:53:59 -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
jungejason
4a76174118 Adding paging to transcript pages
Summary:
enable paging by adding a AphrontPagerView to the two
transcript page.

Test Plan:
test the paging on both pages. Also test it with a certain
phid is given.

Reviewed By: epriestley
Reviewers: epriestley, tuomaspelkonen
CC: epriestley, jungejason
Differential Revision: 116
2011-05-08 15:59:39 -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
1ed915aef2 Detect edits which don't actually change projects in Maniphest tasks
Summary:
Be smarter about detecting when projects haven't actually changed so we don't
create silly transactions which just reorder them or change (entirely arbitrary)
dictionary keys.

Test Plan:
Edited a task with several projects and swapped their order, didn't get a bogus
project transaction.

Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen, jungejason, aran
CC: anjali, sandra, aran, tuomaspelkonen, epriestley
Differential Revision: 249
2011-05-06 18:10:53 -07:00
epriestley
870f4bfe73 Fix GitHub OAuth Registration for users without a name
Summary:
Github allows you to have an account without a real name. The OAuth controller
actually handles this fine, mostly, except that it calls a bogus method. Also
there is some null vs empty string confusion.

Test Plan:
Deleted my name on Github and then registered for an account on Phabricator.

Reviewed By: tuomaspelkonen
Reviewers: jungejason, tuomaspelkonen, aran
CC: anjali, aran, tuomaspelkonen
Differential Revision: 247
2011-05-06 18:09:44 -07:00
epriestley
4e70c434fd Fix Maniphest fatal when editing a task without changing anything
Summary:
Transaction editor attempted to do things with an empty transaction array, just
skip editing ops if nothing changed.

Test Plan:
Edited a Maniphest task without changing anything.

Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen, jungejason, aran
CC: sandra, anjali, aran, tuomaspelkonen
Differential Revision: 248
2011-05-06 18:01:00 -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
7566f50d8f Enable prefilling of some Maniphest fields in task creation
Summary:
This came up in discussions with both ccheever and fratrik so I prototyped a
"send screenshot to maniphest" feature, which needs this:

https://www.facebook.com/video/video.php?v=892599296749

Test Plan:
Sent screenshot to maniphest.

Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran
CC: ccheever, fratrik, aran, epriestley
Differential Revision: 240
2011-05-06 10:55:30 -07:00
tuomaspelkonen
ebbd57771f Missing optional Conduit arguments do not throw errors anymore.
Summary:
ConduitAPIRequest::getValue call for optional arguments which are
not given added a line to the error log file.

Test Plan:
Tested that Conduit API calls were working from Conduit console.

Reviewed By: epriestley
Reviewers: jungejason, epriestley
Commenters: jungejason
CC: epriestley, aran, jungejason
Differential Revision: 236
2011-05-05 19:09:19 -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
19b23e2dd0 Fix repository deletion
Summary:
I never actually wrote this controller.

Test Plan:
Deleted a repository via web UI.

Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen, jungejason, aran
Commenters: aran
CC: aran, tuomaspelkonen, epriestley
Differential Revision: 231
2011-05-05 13:01:45 -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
7eefbfaa51 Add <tr /> tags to Maniphest task summary tables
Summary:
That's not how tables work!

Test Plan:
Load maniphest, do not receive a zillion console warnings in Safari.

Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen
CC: aran
Differential Revision: 227
2011-05-05 11:05:07 -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
epriestley
b75960c578 Web editor/browser for image macros
Summary:
Restores the old "pokedex" feature and allows easy definition of new macros.

Only good can come of this!

Critical feature!!

Test Plan:
nyancat

Reviewed By: tuomaspelkonen
Reviewers: aran, bh, tomo, jungejason, tuomaspelkonen
CC: aran, tuomaspelkonen
Differential Revision: 220
2011-05-03 15:50:40 -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
6229cdadd8 Improve web tools for viewing daemons
Summary:
- Provides an "all daemons" view to look at more than the first 15 daemons.
  - Provides a "combined log" view with a large page size, to quickly look at
the log across all the daemons, making it easier to find issues when you have a
bunch of the same daemon and only one is having issues.
  - When viewing the web console on the same host as a daemon, show whether it's
running or not.

Test Plan:
Clicked the various daemon log interfaces.

Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran
Differential Revision: 215
2011-05-03 06:38:12 -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
59cd14bc61 Add a pager to "Files"
Summary:
Adds a pager control to the "Files" tool so you can page through files if there
are >100.

Test Plan:
Set page size to a smaller number, paged through files.

Reviewed By: aran
Reviewers: aran, tuomaspelkonen, jungejason
CC: aran
Differential Revision: 211
2011-05-02 13:39:46 -07:00
jungejason
c728e4f7da Open database connection with 'w' instead of 'r' for writing
Summary:
there are several places we open an 'r' connection but use it
for writing. Fix them.

Test Plan:
ran parse_one_commit.php against one revision which executes
the code with problem. It used to throw exception. Now it works fine.

Reviewed By: Girish
Reviewers: tuomaspelkonen, Girish
Commenters: aran
CC: aran, Girish
Differential Revision: 213
2011-05-02 13:31:12 -07:00