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
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
Summary:
When you type "5 PM", it should mean "5 PM in your local time", not "5 PM in the
server's time". Use locale-aware functions to read and write times.
Also a couple of usability tweaks.
Test Plan:
Swapped my time between America/Los_Angeles and America/New_York and created a
bunch of countdowns.
Reviewed By: jungejason
Reviewers: hsb, toulouse, jungejason, tuomaspelkonen, aran
CC: aran, jungejason
Differential Revision: 531
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
Summary:
create the indexer for commit
Test Plan:
run the reindex_one_commit.php against one existing commit
and it is returned in search result on the webpage; run parse_one_commit
against another commit; modified reindex_everything.php to let it only
parse one commit after loading all commits.
Reviewed By: epriestley
Reviewers: epriestley, aran
CC: aran, jungejason, epriestley, debow
Differential Revision: 490
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
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
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
Summary:
Add users to the search results. I need to follow this up with a patch to make
the search results stop being terrible. I'll do that.
Test Plan:
Searched for users, ran "reindex_all_users.php"
Reviewed By: jungejason
Reviewers: tomo, jungejason, aran
CC: aran, jungejason
Differential Revision: 508
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
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
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
Summary:
Email was not being sent with the right headers/encoding for UTF-8.
Test Plan:
Sent UTF-8 mail using SES, default and SendGrid adapters. SendGrid already
worked; SES and default share the same code so this fixes both.
Reviewed By: slawekbiel
Reviewers: slawekbiel, aran, jungejason, tuomaspelkonen
CC: aran, slawekbiel
Differential Revision: 401
Summary:
Well, since I couldn't regenerate my arcanist cert I figured out that this wass because "workflows" are unavailable there now. I really can not figure out why but it was.
I added in the setup script, the ability to check if is present the protocol of the host and if it has a trailing slash a the end of the line, since both are needed to generate the cert.
Users now only be able to upload valid image files with mimetype of jpg, jpeg,
png and gif.
Test Plan:
FIRST: DO NOT apply those changes! then
1- go to settings->arcanist certificate and the click on regenerate ... humm
2- On your config file, delete the trailing slash at the end and the protocol on "phabricator.base-uri", then go to setting->arcanist certificate. Here you
will see something like this "phabricator.example.comapi\/" instead of
"http:\/\/phabricator.example.com\/api\/".
SECOND: Now apply this changes:
1- Go to settings->arcanist certificate and the click on regenerate.
2- On your config file, delete the trailing slash at the end and the protocol
on "phabricator.base-uri", and setup "phabricator.setup" to true.
3- Then go to setting->arcanist certificate and you could see that this was successfully generated.
THIRD:
Go to settings->account and try to upload an invalid image file, and do the same on "youruserna"->edit profile.
Reviewed By: epriestley
Reviewers: epriestley jungejason
CC: epriestley jugesason cadamo aran
Differential Revision: 391
Summary:
Some of the improvements from T175: link macro thumbnails to the full image, and
pull an exact count out of the database.
Test Plan:
Clicked a thumb, looked at pager.
Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen, tomo
CC: aran, tuomaspelkonen
Differential Revision: 397
identifier
Summary:
Move the parser to a separate class so it can be easily unit tested, add some
tests. Properly parse emails with linebreaks in the quote line.
Test Plan:
Ran unit tests, used mail receiver to reply to an object.
Reviewed By: cadamo
Reviewers: aran, jungejason, tuomaspelkonen, cadamo
CC: aran, cadamo, epriestley
Differential Revision: 392
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
Summary:
Although these filters work pretty well, you still end up doing a double take
sometimes. Make the behavior simpler and more consistent by adding an "All"
button to "Open / Closed" so all three rows behave the same way (before, the top
row was toggleable but the other rows were select-only-one).
I played around with the styles a little bit too to try to make the selected
state more obvious.
sandra/anjali, let me know if this is good enough once it lands or if I should
go further in playing around with the styles and making it more clear.
Test Plan:
Filtered tasks with the various filter buttons, verified the task list
accurately represented the filters.
Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen, jungejason, aran
CC: anjali, sandra, aran, epriestley, tuomaspelkonen
Differential Revision: 364
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
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
Summary:
- When an administrator creates a user, provide an option to send a welcome
email. Right now this workflow kind of dead-ends.
- Prevent administrators from changing the "System Agent" flag. If they can
change it, they can grab another user's certificate and then act as them. This
is a vaguely weaker security policy than is exhibited elsewhere in the
application. Instead, make user accounts immutably normal users or system agents
at creation time.
- Prevent administrators from changing email addresses after account creation.
Same deal as conduit certs. The 'bin/accountadmin' script can still do this if a
user has a real problem.
- Prevent administrators from resetting passwords. There's no need for this
anymore with welcome emails plus email login and it raises the same issues.
Test Plan:
- Created a new account, selected "send welcome email", got a welcome email,
logged in with the link inside it.
- Created a new system agent.
- Reset an account's password.
Reviewed By: aran
Reviewers: tuomaspelkonen, jungejason, aran
CC: anjali, aran, epriestley
Differential Revision: 379
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
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
Summary:
Sendmail is seriously difficult to configure; SendGrid is extremely easy. It's
also pretty expensive ($80/mo) but there are a bunch of startups that already
have plans so it's effectively free for them.
Test Plan:
Configured SendGrid and sent reply email through it.
Reviewed By: aran
Reviewers: tuomaspelkonen, jungejason, aran
CC: aran, epriestley
Differential Revision: 376
Summary:
Keep him from getting killed every 24 hours by the overseer, add basic commit
support.
Test Plan:
Ran irc bot, fed him a commit, fed him "http://blah/D1".
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen, codeblock, mroch
CC: aran, epriestley
Differential Revision: 377
Summary:
While my client and some others send email replies with an address like
##T1+x+y@example.com##, some other clients have sent either
##<T1+x+y@example.com>## or ##"T1+x+y@example.com" <T1+x+y@example.com>##.
Properly parse all the formats we've seen in the wild.
Test Plan:
Ran the regexp against all the formats observed in the wild (see
https://secure.phabricator.com/mail/received/) and verified it parses them
correctly.
Reviewed By: jungejason
Reviewers: tuomaspelkonen, jungejason, aran
CC: anjali, aran, jungejason
Differential Revision: 370
Summary:
Allow files to be attached to a task by attaching them to an email reply to the
task.
Test Plan:
Applied this patch live since I haven't managed to get inbound email configured
locally, then attached files to a task via email.
Reviewed By: jungejason
Reviewers: tuomaspelkonen, jungejason, aran
CC: anjali, aran, jungejason
Differential Revision: 369
Summary:
I think I wrote this before I laid in workflow, so they always redirect to
another page even if you have JS. Use workflow if it's available.
Test Plan:
Deleted a directory item and a directory category via dialog workflow instead of
full page reloads.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: moskov, aran, jungejason
Differential Revision: 359
Summary:
Currently, we echo the password as the user types it. This turns out to be a bit
of an issue in over-the-shoulder installs. Instead, disable tty echo while the
user is typing their password so nothing is shown (like how 'sudo' works).
Also show a better error message if the user chooses a duplicate email; without
testing for this we just throw a duplicate key exception when saving, which
isn't easy to understand. The other duplicate key exception is duplicate
username, which is impossible (the script updates rather than creating in this
case).
There's currently a bug where creating a user and setting their password at the
same time doesn't work. This is because we hash the PHID into the password hash,
but it's empty if the user hasn't been persisted yet. Make sure the user is
persisted before setting their password.
Finally, fix an issue where $original would have the new username set, creating
a somewhat confusing summary at the end.
I'm also going to improve the password behavior/explanation here once I add
welcome emails ("Hi Joe, epriestley created an account for you on Phabricator,
click here to login...").
Test Plan:
- Typed a password and didn't have it echoed. I also tested this on Ubuntu
without encountering problems.
- Chose a duplicate email, got a useful error message instead of the exception
I'd encountered earlier.
- Created a new user with a password in one pass and logged in as that user,
this worked properly.
- Verified summary table does not contain username for new users.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: moskov, jr, aran, jungejason
Differential Revision: 358
Summary:
Despite the form's claims that you can login with username or email, it actually
accepted only username.
Test Plan:
Logged in using my email.
Reviewed By: jungejason
Reviewers: tuomaspelkonen, jungejason, aran
CC: jr, anjali, aran, jungejason
Differential Revision: 354
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
Summary:
Show large thumbnails of attached files in Maniphest.
Test Plan:
Looked at large thumbnails in Maniphest.
Reviewed By: jungejason
Reviewers: tomo, aran, jungejason, tuomaspelkonen
CC: anjali, aran, epriestley, jungejason
Differential Revision: 335
Summary:
Make it discoverable, show uploading progress, show file thumbnails, allow you
to remove files, make it a generic form component.
Test Plan:
Uploaded ducks
Reviewed By: tomo
Reviewers: aran, tomo, jungejason, tuomaspelkonen
CC: anjali, aran, epriestley, tomo
Differential Revision: 334
Summary:
This is still very rough but provides basic support for generating image
thumbnails. I need to separate stuff out a bit but I'm going to integrate into
Maniphest before I hit the profile stuff so this seems like a reasonable
starting point.
Test Plan:
Generated some image thumbnails in various sizes.
Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran
Differential Revision: 333
Summary: SendGrid is a popular mail delivery platform, similar to Amazon SES. Provide support for delivering email via their REST API.
Test Plan: Created a SendGrid account, configured my local install to use it, sent some mail, received mail.
Reviewers: tuomaspelkonen, jungejason, aran
CC: ccheever
Differential Revision: 347
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
Summary:
Clicking on a line number will remove the current 'view' the user is in.
This patch retains the current view.
Test Plan:
Open a file in diffusion, and change the view to "blame", clicking on the line
number should retain the same view.
Reviewed By: epriestley
Reviewers: epriestley, jungejason
CC: aran, epriestley, aizatto
Differential Revision: 344
Summary: the user can't let the realname and/or e-mail address be empty
Test Plan: enter on 'settings/account' and change your name to '' and the same
for the e-mail 'settings/email'
Reviewers: epriestley
CC: epriestley
Summary:
This needs a bunch of UI polish (critically, it's totally undiscoverable) but it
basically works correctly. I'll clean it up in some followups.
Test Plan:
Uploaded some files via drag-and-drop, made comments, etc.
Reviewed By: aran
Reviewers: tomo, aran, jungejason, tuomaspelkonen
CC: anjali, aran
Differential Revision: 332
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
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
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
Summary:
This is required to make "arc patch" and "arc export" support binary changes.
Test Plan:
Called from web console and "arc".
Reviewed By: aran
Reviewers: tuomaspelkonen, jungejason, aran
CC: aran, epriestley
Differential Revision: 326
Summary:
When a task description is updated, there's currently no way to see the change.
Build an "expanded summary" mode for transactions that shows description change
details. Also include changes in the email.
Test Plan:
Changed task descriptions, clicked "show details", read email.
Reviewed By: aran
Reviewers: tuomaspelkonen, jungejason, aran
CC: anjali, aran, epriestley
Differential Revision: 320
Summary: This isn't complete, but I figured I'd ship it for review while it's still smallish.
Provide an activity log for high-level system actions (logins, admin actions). This basically allows two things to happen:
- The log itself is useful if there are shenanigans.
- Password login can check it and start CAPTCHA'ing users after a few failed attempts.
I'm going to change how the admin stuff works a little bit too, since right now you can make someone an agent, grab their certificate, revert them back to a normal user, and then act on their behalf over Conduit. This is a little silly, I'm going to move "agent" to the create workflow instead. I'll also add a confirm/email step to the administrative password reset flow.
Test Plan: Took various administrative and non-administrative actions, they appeared in the logs. Filtered the logs in a bunch of different ways.
Reviewers: jungejason, tuomaspelkonen, aran
CC:
Differential Revision: 302
Summary:
A lot of history views were empty. This fixes that problem.
Test Plan:
Played with sandbox.
Reviewed By: epriestley
Reviewers: epriestley, jungejason
Commenters: aran
CC: aran, epriestley, tuomaspelkonen
Differential Revision: 316
