Summary:
I didn't grep carefully enough when renaming this to 'loadProject' in D485. See
task. I am terrible at the programmings.
Test Plan:
Hit the "Edit project" view, grepped more thoroughly. :P
Reviewed By: tomo
Reviewers: tomo, toulouse, cadamo
CC: aran, tomo
Differential Revision: 503
Summary:
See T251, where gregprice correctly argues that we need both:
None of the other people on the thread will have seen that message, so it
seems
like a lot of clients would put the server's message in a new thread. In
general, I think you want the References: header to mention every ancestor
message in the thread that you know about, because that's how MUAs keep a
thread
together in the face of missing some of its messages.
Test Plan:
Sent a reply email locally, got a response with both Message-IDs in
"references".
Reviewed By: rm
Reviewers: gregprice, rm
Commenters: gregprice
CC: aran, gregprice, epriestley, rm
Differential Revision: 499
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
Summary:
D477 added functionality to the project list view but had a couple of
performance issues that I missed in review, because it took the query count for
the page from around 3 to as many as 300, including up to 100 heavyweight search
index queries.
This fixes the two simple N+1 query problems. This general pattern of data
access often occurs:
COUNTEREXAMPLE
$cats = load_cats();
foreach ($cats as $cat) {
$cats_hats = load_hats_for_cat($cat);
// ...
}
But this issues "N+1" queries, i.e. if you load 100 cats you issue 101 queries.
It is faster to group the queries instead:
$cats = load_cats();
$hats = load_all_hats_for_these_cats($cats);
foreach ($cats as $cat) {
$cats_hats = $hats[$cat->getID()];
}
MySQL can execute one query which returns all the results much faster than 100
queries which return one result, especially if the database is not local (i.e.,
over the network).
However, this doesn't save a ton of time. The bigger issue is that I didn't have
the right keys on the relationship tables in the search engine. This adds them,
and reduces the search engine lookup cost from 25-80ms (for
secure.phabricator.com) down to 1-3ms.
I still probably want to get this out of the loop at some point but it's okay
for now and the page loads in a few ms rather than taking more than a second.
Test Plan:
Used "services" tab, "xhprof" and "EXPLAIN" to analyze page performance. I
measured these changes:
- Query count: 1 + (3 * N projects) -> 3 + (N projects) (e.g., 301 -> 103)
- Total time spent querying, ignoring search indexes: 40ms (local.aprhont.com)
-> 20ms (local.aphront.com)
- Cost for search index query: 25-80ms (secure.phabricator.com) -> 1-3ms
Reviewed By: cadamo
Reviewers: cadamo, aran, jungejason, tuomaspelkonen
CC: aran, cadamo, epriestley
Differential Revision: 485
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
Summary:
Without this, user creation throws an exception when trying to insert NULL into
a non-NULL field.
Test Plan:
Created a new user.
Reviewed By: fratrik
Reviewers: fratrik, toulouse, aran, jungejason, tuomaspelkonen
CC: aran, fratrik
Differential Revision: 480
Summary: Added some change on the project's list view, to show information about
active tasks, population, etc. Also modified the "profile view", and added a class "PhabricatorProfileView" to render the profile, both on projects and users.
Test Plan: play around the project directory :)
Reviewers: epriestley ericfrenkiel
CC:
Differential Revision: 477
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
Summary:
Provides a new workflow for making it non-horrible to install certificates.
Basically you run "arc install-certificate" and then copy/paste a short token
off a webpage and it does the ~/.arcrc edits for you.
Test Plan:
Installed certificates, used bad tokens, hit rate limiting.
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen
CC: aran
Differential Revision: 460
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
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
Summary:
Allows user-configurable timezones. Adds a preference panel, and migrates to the
new date rendering in easily-modified areas of the code. ***In progress***.
Test Plan:
Check database to make sure the field is being changed when the settings are
changed; check affected views to see how they render times.
Reviewed By: epriestley
Reviewers: epriestley
CC: aran, epriestley, toulouse
Differential Revision: 475
Summary:
Paste was cutting off lines when they extended past the width of the screen.
Also change a pair of double quotes to single quotes, to follow convention.
Test Plan:
Tried this in Firefox 4 and Chrome.
Reviewers:
epriestley, toulouse
CC:
Differential Revision: 469
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
Summary:
Allow duplicate tasks to be selected and merged in Maniphest.
I didn't create a separate transaction type for this because that implies a
bunch of really complicated rules which I don't want to sort out right now
(e.g., do we need to do cycle detection for merges? If so, what do we do when we
detect a cycle?) since I think it's unnecessary to get right for the initial
implementation (my Tasks merge implementation was similar to this and worked
quite well) and if/when we eventually need the metadata to be available in a
computer-readable form that need should inform the implementation.
Plenty of room for improvement here, of course.
Test Plan:
Merged duplicate tasks, tried to perform invalid merge operations (e.g., merge a
task into itself).
Tested existing attach workflows (task -> revision, revision -> task).
Reviewed By: aran
Reviewers: tuomaspelkonen, jungejason, aran
CC: anjali, aran
Differential Revision: 459
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
Summary:
For some unknown reasons, our test framework fails to update test
results occasionally. Instead of throwing a nasty exception, log
the error message. This will help us to figure out the real reason
behind the error.
Test Plan:
arc lint.
Reviewed By: epriestley
Reviewers: jungejason, epriestley
CC: epriestley, aran
Differential Revision: 463
Summary:
This implements a paste.info conduit method, for retrieving info about a paste. Imagine that.
Test Plan:
[ricky@rhelpad01 method]$ echo '{"paste_id":1}' | arc call-conduit --conduit-uri=http://phabricator.local/api/ paste.info
{"error":null,"errorMessage":null,"response":{"id":"1","phid":"PHID-PSTE-10934f3df8ed33c06555","authorPHID":"PHID-USER-9d03e8fa47516d37dc92","filePHID":"PHID-FILE-e85f6a72c773d24f4981","title":"test.php","dateCreated":"1307731614"}}
Reviewers:
epriestley
CC:
Differential Revision: 458
Summary:
Tweaks to the paste app:
- I realized that unlike all the other apps, it makes more sense for the
default view of this one to be "create paste" instead of "list pastes" since
when you access the application directly you are most often wanting to share
something. Swap list out of the default slot and make edit the default.
- Make the textarea bigger (usability).
- Allow you to copy an existing paste.
- Implement 'raw view'.
- Tweak/adjust list view (usability, formatting).
- Tweak page titles.
Test Plan:
Created, copied, and listed pastes. Viewed raw paste. Created an invalid paste.
Tried to create a copy of a nonexistant paste.
Reviewed By: codeblock
Reviewers: codeblock, jungejason, aran, tuomaspelkonen
CC: aran, epriestley, codeblock
Differential Revision: 456
Summary:
Show how many tasks are in each group. This number is useful when you are
insufficiently depressed.
Test Plan:
Looked at maniphest task list.
Reviewed By: toulouse
Reviewers: aran, jungejason, tuomaspelkonen, toulouse
CC: anjali, aran, toulouse
Differential Revision: 457
Summary:
A few tweaks to hsb's Countdown implementation:
- Allow the page to be rendered "chromeless", suitable for display on one of
the dozens of monitors everyone has laying around.
- Show title of countdown in deletion dialog.
- When creating a new countdown default to time(), not Dec 31, 1969.
- Add extra "/" after editing to avoid needless redirect.
- Tweak some page titles.
- Show countdown author in list view.
- Highlight tab in list view.
- Tweak menu copy.
- Link countdown title in list view, separate buttons into different columns
so they pick up padding.
Test Plan:
Created, edited and deleted a timer. Viewed a timer and toggled chrome mode.
Viewed timer list.
Reviewed By: hsb
Reviewers: hsb, aran, jungejason, tuomaspelkonen
CC: aran, hsb, epriestley
Differential Revision: 454
Summary:
remove accessing the db config info directly. Use
DatabaseConfigurationProvider instead. Also fixed a minor issue where
different number of newlines are output in PhabricatorSetup.php's output.
Test Plan:
executed upgrade_schema.php; executed PhabricatorSetup.php by
setting 'phabricator.setup' to true.
Reviewed By: epriestley
Reviewers: epriestley
CC: aran, jungejason, epriestley
Differential Revision: 443
Summary:
Make 'gd' an explicit optional dependency, test for it in setup, and make the
software behave correctly if it is not available.
When generating file thumnails, provide reasonable defaults and behavior for
non-image files.
Test Plan:
Uploaded text files, pdf files, etc., and got real thumbnails instead of a
broken image.
Simulated setup and gd failures and walked through setup process and image
fallback for thumbnails.
Reviewed By: aran
Reviewers: toulouse, jungejason, tuomaspelkonen, aran
CC: aran, epriestley
Differential Revision: 446
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
Summary:
Provide a quick workflow for adding a new project. This ended up being sort of
complicated because we don't currently put forms in dialogs. I separated the
actual <form /> tag out of the display/layout of AphrontFormView to enable this
(the dialog is itself a form).
Limitations: if you create a new project and then remove it, it won't appear in
the tokenizer until you reload the page. We need to add the ability for the
datasource to drop its cache to enable this, which is super complicated.
Test Plan:
Used "Create new project" to add a new project when creating a task.
Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran
CC: anjali, aran, epriestley
Differential Revision: 422
Summary:
- Make the instructional text generally more useful.
- Show the current configured adapter.
- When the configuration prevents outbound email from being delivered, show a
warning.
- Detect 'curl' extension during setup since it's more-or-less required
- Add curl extension to the install scripts
codeblock: can you verify the rhel-derivs changes are correct?
Test Plan:
Set adapter to test, verified warning; entered setup mode and verified curl. Ran
apt-get on an ubuntu box. Ran yum on an amazon linux box.
Reviewed By: toulouse
Reviewers: toulouse, codeblock
Commenters: codeblock
CC: aran, jungejason, tuomaspelkonen, codeblock, epriestley, toulouse
Differential Revision: 438
Summary:
Addon that allows you to create a live countdown page to
some event.
Here is the ticket that this code is based on
https://secure.phabricator.com/T36
Test Plan:
Tested by manually setting dates in the timer.js file and
checking if they made sense.
I'm not sure if it works across different timezones though.
Reviewers: epriestley
CC:
Differential Revision: 436
Summary:
Wrong header, should say Project instead of People and two buttons had "Project
Project"
Test Plan:
UI fixes, no real test plan needed
Reviewers: tuomaspelkonen, aran
Commenters: tuomaspelkonen, aran
CC: aran, tuomaspelkonen, hsb, epriestley
Revert Plan:
No plan needed, just revert this UI fix revision
Differential Revision: 338
Summary:
PhabricatorFile() was setting the mimetype based on extension, meaning that you couldn't view the plain-text file if you saved a file, for example, as a .php. It would set the mimetype to "text/x-php; charset=us-ascii". In this commit, I force the mimetype to text/plain.
Test Plan:
Tried pasting a new file and was able to both see it via the pastebin viewer and in plain-text via File.
Reviewers:
epriestley
CC:
Differential Revision: 429
Summary:
This is Paste. It needs some work, but epriestley recommended that I just commit something that works, and expand on that later.
Specifically, it lacks the ability to view a raw paste right now, and to turn off line numbers, making it hard to copy/paste from for now. It works for showing other people code, however.
Test Plan:
Pasted stuff, and was able to view it, and see it in the list on /paste/. Put a file extension in the title, and saw that syntax highlighting worked as expected.
Reviewers:
epriestley
CC:
Differential Revision: 424
Summary:
"--date short" was introduced to git somewhere between 1.7.2.2 and 1.7.3.4,
despite 1.7.2.2 saying "--date <format>" in "git help blame". The older version
of git accepts "--date=short", however.
Also, the URI construction means you get "?view=" if you click a line number to
get a deep link, which I found vaguely annoying. Drop 'view' if we don't need
it.
Test Plan:
Looked at blame in my sandbox, although it worked before the date patch since I
have 1.7.3.4. Clicked a line number. Switched viewmodes.
Reviewed By: codeblock
Reviewers: codeblock, jungejason, tuomaspelkonen, aran
CC: aran, codeblock
Differential Revision: 423
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
Summary:
This is pretty subtle and tricky, but some apply transcripts don't have a rule
ID because they're purely informational. We currently get an exception, which
prevnets diff updates.
jason/tuomas: don't update phabricator.fb.com until this lands :P
Test Plan:
Applied this patch live to secure.phabricator.com and was able to update D420.
Reviewed By: gc3
Reviewers: gc3, aran, jungejason, tuomaspelkonen
CC: aran, epriestley, gc3
Differential Revision: 421
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
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
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
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
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
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
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
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
Summary: Implements a simple infrastructure for keyboard shortcuts, see T184, and a "help" shortcut.
There's a lot of room for refinement here but I think it basically works. Each shortcut can also provide a "tooltip" handler which allows it to show help when the alt/option key is held down.
Test Plan: Pressed "?" and got help. Pressed "?" in various contexts where it should not activate (modifier keys, text input focused) and didn't get help.
Reviewers: aran, tuomaspelkonen, jungejason
CC: moskov
Differential Revision: 362
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
Summary:
The current git log parsing wouldn't work correctly when space
appeared in the author name. This diff fixes to regex to work
with multi-word author names and increases the author column
width when viewing blame info to accound for larger author names.
Test Plan:
viewed file with blame info in diffusion where author
name contained multiple words
Reviewed By: jungejason
Reviewers: jungejason, epriestley
CC: aran, rm, jungejason
Differential Revision: 399
