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:
This simplifies it a lot and prevents it from spazzing out when some control is
foucsed.
Test Plan:
Hit "?", "`".
Reviewed By: aran
Reviewers: jungejason, aran, tuomaspelkonen
CC: aran
Differential Revision: 410
Summary:
This already pretty much works, document it explicitly.
Test Plan:
Moved my MySQL server over to port 3307.
Reviewed By: aran
Reviewers: jungejason, aran
CC: aran
Differential Revision: 411
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:
If you name an attached file something like
"derpderpderpderpderdperdperpereprederpderplolololololo.png", the display
overflows. Crop and shorten it so that it looks decent. Show the full name as
the image title.
Test Plan:
Named a file "derpderpderpderpderdperdperpereprederpderplolololololo.png" and
got a sensible display out of it.
Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen, jungejason, aran
CC: aran, tuomaspelkonen
Differential Revision: 389
Summary: Normally this gives you a prompt about taking down services, provide a noninteractive mode for scripting the upgrade process.
Also drop a generally bad/confusing/irrelevant piece of advice from the documentation and replace it with information about -f.
Test Plan: Ran with and without -f. Ran with -h.
Reviewers: moskov, tuomaspelkonen, jungejason, aran
CC:
Differential Revision: 387
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:
Simple script to install dependencies on Ubuntu.
There's probably lots of room for improvement here.
Test Plan:
Imaged a clean Ubuntu box in EC2 and ran this script, it appeared to work?
Reviewed By: tuomaspelkonen
Reviewers: kevinwallace, jungejason, tuomaspelkonen, aran
Commenters: aran
CC: moskov, aran, tuomaspelkonen, epriestley
Differential Revision: 384
Summary:
- Allow user to specify "myconf" (recommended) or "myconf.conf.php" (less
surprising).
- Make sure syntax errors and other problems are surfaced.
- If the configuration value isn't valid, give them a list of all valid
values.
Test Plan:
- Added a syntax error, got a useful error.
- Set PHABRICATOR_ENV to a silly value, got a list of valid values.
- Set PHABRICATOR_ENV to have .conf.php suffix, site still worked.
Reviewed By: kevinwallace
Reviewers: kevinwallace, codeblock, aran, jungejason, tuomaspelkonen
CC: aran, epriestley, kevinwallace
Differential Revision: 381
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:
In RHEL6 at the least, pcntl installs from distro package management to the CLI
but not to Apache. Since we don't need it in apache and it's a pain to build
manually, just verify it exists on the CLI.
Test Plan:
Simulated script failures and verified setup output.
Reviewed By: codeblock
Reviewers: codeblock, aran, jungejason, tuomaspelkonen
CC: aran, epriestley, kevinwallace, codeblock
Differential Revision: 380
Summary:
the daemon is generating a lot of warning about timezone not
being set. Add it.
Test Plan:
php -l. Launched the daemon and it didn't crash. It's
difficult to make it parse a commit on my sandbox though. I will see if
it fixes the problem once it is live.
Reviewed By: epriestley
Reviewers: epriestley, tuomaspelkonen
CC: aran, epriestley, jungejason
Differential Revision: 356
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:
Removes calling `which` and instead checks the file path that PHPMailerLite uses directly (/usr/bin/sendmail).
This fixes $PATH export issues which occur on certain platforms causing un-expected errors during setup.
Test Plan:
* Run setup on server without sendmail install & error should be presented.
* Install sendmail then re-run setup & no error should be presented.
Reviewers: epriestley
Differential Revision: 375
Summary:
- Provide a red version of the logo for the admin view.
- Make selected tabs in the admin view look correct.
- Fix a Chrome styling issue where the username and "settings" would have
weird offsets.
Test Plan:
Loaded Phabricator in chrome and clicked around tabs of an admin interface.
Hovered over logo.
Reviewed By: jungejason
Reviewers: cadamo, jungejason, tuomaspelkonen, aran
CC: aran, epriestley, jungejason
Differential Revision: 371
Summary:
I ran a 99designs contest and this gear-eye thing was actually pretty okay. All
this stuff needs tweaks but at least it won't render with a big square on
windows anymore.
Test Plan:
Looked at menu, it seemed slightly more legitimate and designey than before?
Reviewed By: jungejason
Reviewers: tuomaspelkonen, jungejason, aran
CC: aran, epriestley, jungejason
Differential Revision: 363
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
