Summary:
Default of 300 seconds is more than likely too much in most cases.
Provide the option to override.
Test Plan:
Blocked ElasticSearch with iptables
Set timeout to 5 seconds and make sure we error early
Reviewers: epriestley, vrana
Reviewed By: vrana
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2678
Summary:
TransactionType gives us more information than
update, open, close, assign. We can display those in feed/notifications along with and comments on the actions.
Test Plan: did on local machine tested out.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: ddfisher, keebuhm, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2683
Summary: First diff in a series of diffs to add notifications to Phabricator. This is the notification application ONLY. This commit does not include the changes to other applications that makes them add notifications. As such, no notifications will be generated beyond the initial database import.
Test Plan: This is part of the notifications architecture which has been running on http://theoryphabricator.com for the past several months.
Reviewers: epriestley, btrahan, ddfisher
Reviewed By: epriestley
CC: allenjohnashton, keebuhm, aran, Korvin, jungejason, nh
Maniphest Tasks: T974
Differential Revision: https://secure.phabricator.com/D2571
Summary:
I need this information quite often.
I don't know how many people are working on a single project and this information will be useless for them but I guess it won't hurt much?
Test Plan: Commented on accepted revision.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2674
Summary: These columns holds the same value in most cases which irritates me.
Test Plan: Displayed history with same author and committer, emulated different committer.
Reviewers: hsb, epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2671
Summary: As per title: when browsing files in Diffusion, set "Highlighted with blame" as the default view instead of falling back to "Highlighted".
Test Plan: Tested view with a local Diffusion setup, defaults correctly.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1278
Differential Revision: https://secure.phabricator.com/D2669
Summary:
This also changes some stuff:
- Reviewers used to be at top, now they are under comments.
- Primary reviewer is now rendered as first.
Test Plan: Added `renderValueForMail()` to a custom field, created diff, commented on it and verified e-mails.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2664
Summary:
Give them a big essay about how it's dangerous, but allow them to do it formally.
Because the username is part of the password salt, users must change their passwords after a username change.
Make password reset links work for already-logged-in-users since there's no reason not to (if you have a reset link, you can log out and use it) and it's much less confusing if you get this email and are already logged in.
Depends on: D2651
Test Plan: Changed a user's username to all kinds of crazy things. Clicked reset links in email. Tried to make invalid/nonsense name changes.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1303
Differential Revision: https://secure.phabricator.com/D2657
Summary:
See T1303, which presents a reasonable case for inclusion of these characters in valid usernames.
Also, unify username validity handling.
Test Plan: Created a new user with a valid name. Tried to create a new user with an invalid name. Ran unit tests.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1303
Differential Revision: https://secure.phabricator.com/D2651
Summary:
See https://github.com/facebook/phabricator/issues/117
- The $user save can hit a duplicate key exception like the email, but we don't handle it correctly.
- When the $user saves but the $email does not, the $user is left with a (rolled-back, invalid) ID. This makes the UI glitch out a bit. Wipe the ID if we abort the transaction.
- We show the "Required" star marker even if the email is filled in.
The ID issue is sort of a general problem, but I think it's fairly rare: you must be doing inserts on related objects and the caller must catch the transaction failure and attempt to handle it in some way.
I can think of three approaches:
- Manually "roll back" the objects inside the transaction, as here. Seems OK if this really is a rare problem.
- Automatically roll back the 'id' and 'phid' columns (if they exist). Seems reasonable but maybe more complicated than necessary. Won't get every case right. For instance, if we inserted a third object here and that failed, $email would still have the userPHID set.
- Automatically roll back the entire object. We can do this by cloning all the writable fields. Seems like it might be way too magical, but maybe the right solution? Might have weird bugs with nonwritable fields and other random stuff.
We can trigger the rollback by storing objects we updated on the transaction, and either throwing them away or rolling them back on saveTransaction() / killTransaction().
These fancier approaches all seem to have some tradeoffs though, and I don't think we need to pick one yet, since this has only caused problems in one case.
Test Plan: Tried to create a new user (via People -> Create New User) with a duplicate username. Got a proper UI message with no exception and no UI glitchiness.
Reviewers: btrahan, vrana, hgrimberg, hgrimberg01
Reviewed By: hgrimberg01
CC: aran
Differential Revision: https://secure.phabricator.com/D2650
Summary:
We need to generate the URI dynamically.
This code is also generally better.
Test Plan: Created custom search selector passing the custom URI to engine.
Reviewers: epriestley, edward
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2655
Summary: If, e.g., $PATH is broken we may not be able to run "ps". We'll explode pretty hard, currently. Instead, just show a harsher warning.
Test Plan: Changed "ps auxwww" to "psq", which doesn't exist on my system. Loaded page, got warning instead of explosion.
Reviewers: nathanws, vrana, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2624
Summary: These were in an unusual location, but are better back in policy/
Test Plan: implicit arc unit
Reviewers: vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1103
Differential Revision: https://secure.phabricator.com/D2638
Summary:
- `kill_init.php` said "Moving 1000 files" - I hope that this is not some limit in `FileFinder`.
- [src/infrastructure/celerity] `git mv utils.php map.php; git mv api/utils.php api.php`
- Comment `phutil_libraries` in `.arcconfig` and run `arc liberate`.
NOTE: `arc diff` timed out so I'm pushing it without review.
Test Plan:
/D1234
Browsed around, especially in `applications/repository/worker/commitchangeparser` and `applications/` in general.
Auditors: epriestley
Maniphest Tasks: T1103
Summary: They were only displaying seconds. I found a function in viewutils.php that allowed for single-unit precision formatting, but I wanted more, so I wrote another function to allow more detail.
Test Plan: [site]/mail, and watch it work. It's a new function, so it shouldn't break anything else.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1296
Differential Revision: https://secure.phabricator.com/D2616
Summary: Mark these actions with the same markers we use in Differential.
Test Plan: {F12094}
Reviewers: csilvers, jungejason, btrahan
Reviewed By: csilvers
CC: aran
Maniphest Tasks: T1289
Differential Revision: https://secure.phabricator.com/D2601
Summary: D2216 tried to ask the user, this one is explicit.
Test Plan: Click the button
Reviewers: epriestley, lucian
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2600
Summary: This is an example of code simplification with D2557.
Test Plan: Display user list, verify the SQL queries.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2558
Summary:
Some lint errors (e.g. Javelin) don't have a line number.
Put them on the first line.
Putting them above the first line would be even nicer but much more complicated.
Test Plan: Display diff with lint error on line 0 (D2583).
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2599
Summary: Also fix the notice text.
Test Plan:
Display diff with inline comments and lint errors.
Click on inline comment and lint link.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2594
Summary:
Allow allowed email addresses to be restricted to certain domains. This implies email must be verified.
This probably isn't QUITE ready for prime-time without a few other tweaks (better administrative tools, notably) but we're nearly there.
Test Plan:
- With no restrictions:
- Registered with OAuth
- Created an account with accountadmin
- Added an email
- With restrictions:
- Tried to OAuth register with a restricted address, was prompted to provide a valid one.
- Tried to OAuth register with a valid address, worked fine.
- Tried to accountadmin a restricted address, got blocked.
- Tried to accountadmin a valid address, worked fine.
- Tried to add a restricted address, blocked.
- Tried to add a valid address, worked fine.
- Created a user with People with an invalid address, got blocked.
- Created a user with People with a valid address, worked fine.
Reviewers: btrahan, csilvers
Reviewed By: csilvers
CC: aran, joe, csilvers
Maniphest Tasks: T1184
Differential Revision: https://secure.phabricator.com/D2581
Summary:
This adds a link to [Closed] e-mail if it detects some changes.
It compares added and removed lines with 3 lines context.
The subtle form of informing is permissive to false negatives and positives.
I have an e-mail filter for [Closed] e-mails so I wouldn't personally notice this change - we should probably promote this feature a little bit.
Test Plan:
Reparse a diff with a change after last update.
Reparse a diff without a change after last update.
Reviewers: jungejason, epriestley
Reviewed By: jungejason
CC: aran, Koolvin, btrahan
Maniphest Tasks: T201
Differential Revision: https://secure.phabricator.com/D2540
Summary:
I thought about it a little bit and this makes the most sense for me:
# Original author usually writes at least something and commander only updates it.
# There's a creation date of revision (= first diff) by these comments. I don't want to change this date because I use this information. Author should correspond to this date.
# It solves all our repros.
Test Plan: Display commandeered revision.
Reviewers: nh, epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2577
Summary:
- We currently have some bugs in account creation due to nontransactional user/email editing.
- We save $user, then try to save $email. This may fail for various reasons, commonly because the email isn't unique.
- This leaves us with a $user with no email.
- Also, logging of edits is somewhat inconsistent across various edit mechanisms.
- Move all editing to a `PhabricatorUserEditor` class.
- Handle some broken-data cases more gracefully.
Test Plan:
- Created and edited a user with `accountadmin`.
- Created a user with `add_user.php`
- Created and edited a user with People editor.
- Created a user with OAuth.
- Edited user information via Settings.
- Tried to create an OAuth user with a duplicate email address, got a proper error.
- Tried to create a user via People with a duplicate email address, got a proper error.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: tberman, aran
Maniphest Tasks: T1184
Differential Revision: https://secure.phabricator.com/D2569
Summary:
- Add "role" information, so clients can identify disabled users.
- Formally deprecate `user.info`
Test Plan: Ran "user.query" and "user.whoami", inspected output. Verified "user.info" appears as deprecated in method list and console.
Reviewers: csilvers, btrahan
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2565
Summary:
Instead of assuming the test plan and summary are written by the author
of the differential revision, let's assume they are written by the author
of the latest differential diff.
Test Plan: viewed a drev that had been commandeered but not updated to check authors
Reviewers: epriestley, jungejason, vrana
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1235
Differential Revision: https://secure.phabricator.com/D2550
Summary:
- If you have an unverified primary email, we show a disabled "Primary" button right now in the "Status" column. Instead we should show an enabled "Verify" button, to allow you to re-send the verification email.
- Sort addresses in a predictable way.
Test Plan:
- Added, verified and removed a secondary email address.
- Resent verification email for primary address.
- Changed primary address.
Reviewers: btrahan, csilvers
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1184
Differential Revision: https://secure.phabricator.com/D2548
Summary: also makes the UI more general for this username + password business.
Test Plan:
- configure a phabricator repository from the svn server @asherwin provided which is configured for svn protocol with SASL
- observed phabricator failing without my patch
- upgraded my SVN client to support SASL (protip for mac users - http://www.wandisco.com/subversion/download#osx)
- applied patch to phabricator
- restarted daemons
- noted daemon success - diffusion populating nicely
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1260
Differential Revision: https://secure.phabricator.com/D2549
Summary:
Git and hg (supposedly..) differentiate between an author (who wrote the patch)
and a committer (who applied the patch).
This patch allows Phabricator to note when a patch is committed
by someone other than the Author.
Test Plan:
Created 2 accounts,
- U (Account with a PHID)
- U' (Account without a PHID)
and had them create and commit commits
testing if their username/real name would be displayed correctly in Diffusion,
- BrowserTable
- HistoryTable
- Code revision
Teztz,
A(uthor)/C(ommitter)
If it's A/A then Author committed
UL = User link (<a href="/p/username">username</a>)
UN = User name ("Firstname Lastname")
Tezt | Expected in table | Got
-------------------------------------------
A/A | UL/UL | UL/UL
A'/C | UN/UL | UN/UL
A/C' | UL/UN | UL/UN
A'/C' | UN/UN | UN/UN
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T688
Differential Revision: https://secure.phabricator.com/D2541
Summary:
If someone typos one name in a cc or reviewer list, it would be nice if we
display all of the valid names in the field when running arc diff (in addition
to the error message).
Test Plan:
used the conduit console to check that calling differential.parsecommitmessage
with a list of some valid and some invalid ccs returns a result with both an
error and a list of some ccs. Also ran arc diff with that list of ccs to check
for the correct user experience.
Reviewers: epriestley, jungejason, vrana
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2542
Summary:
This attaches commit diff to its associated revision as any other diff.
The consequence is that the revision page now shows the actual commit instead of the last diff. It may be disturbing but it is desired.
Another consequence is that lint and unit results are displayed as skipped after the revision was committed. I want to fix it somehow.
My next plan is to automatically diff against the last normal diff and include the link to this diff in commit e-mail if non-empty.
Test Plan:
reparse.php
Diff against last normal diff.
Reviewers: epriestley, jungejason
Reviewed By: jungejason
CC: aran, Koolvin, jungejason
Maniphest Tasks: T201
Differential Revision: https://secure.phabricator.com/D2530
