Summary:
This hooks up all the pieces of the build pipeline so `harbormaster.sendmessage` actually works. Particularly:
- Candidate build steps (i.e., those which interact with external systems) can now "Wait for Message". This pauses them indefinitely when they complete, until something calls `harbormaster.sendmessage`.
- After processing a target, we check if we should move it to PASSED or WAITING.
- Before updating a build, we move WAITING targets with pending messages to either PASSED or FAILED.
- I added an explicit "Building" state, which doesn't affect workflows but communicates more information to human users.
A big part of this is avoiding races. I believe we get the correct behavior no matter which order events occur in:
- We update builds after targets complete and after we receive messages, so we're guaranteed to update once both these conditions are true. This means messages can't be lost (even if they arrive before a build completes).
- The minor changes to the build engine logic mean that firing additional build updates is always safe, no matter what the current state of the build is.
- The build itself is protected by a lock in the build engine.
- The target is not covered by an explicit lock, but for all states only the engine (waiting) //or// the worker (all other states) can interact with it. All of the interactions also move the target state forward to the same destination and have no other side effects.
- Messages are only consumed inside the engine lock, so they don't need an explicit lock.
Test Plan:
- Made an HTTP request wait after completion, then ran a pile of builds through it using `bin/harbormaster build` and the web UI.
- Passed and failed message-awaiting builds with `harbormaster.sendmessage`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, zeeg
Differential Revision: https://secure.phabricator.com/D8788
Summary: Fixes T4590. Use the credentials custom field to allow Harbormaster HTTP requests to include usernames/passwords.
Test Plan: Ran a build plan with credentials, verified they were sent to the remote server.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4590
Differential Revision: https://secure.phabricator.com/D8786
Summary:
Ref T4605. When figuring out how long to wait to update a repository, factor in when it was last pushed. For rarely updated repositories, wait longer between updates.
(A slightly funky thing about this is that empty repos update every 15 seconds, but that seems OK for the moment.)
Test Plan:
Ran `bin/phd debug pulllocal` and saw sensible calculations and output:
```
...
<VERB> PhabricatorRepositoryPullLocalDaemon Last commit to repository "rPOEMS" was 1,239,608 seconds ago; considering a wait of 6,198 seconds before update.
>>> [79] <query> SELECT * FROM `repository` r ORDER BY r.id DESC
<<< [79] <query> 514 us
>>> [80] <query> SELECT * FROM `repository_statusmessage` WHERE statusType = 'needs-update'
<<< [80] <query> 406 us
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rINIH" is not due for an update for 8,754 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rDUCK" is not due for an update for 14 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rMTESTX" is not due for an update for 21,598 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rQWER" is not due for an update for 14 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rBT" is not due for an update for 13 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rSVNX" is not due for an update for 21,598 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rINIG" is not due for an update for 13 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rHGTEST" is not due for an update for 21,598 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rBTX" is not due for an update for 14 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rGX" is not due for an update for 13 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rMTX" is currently updating.
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rPOEMS" is not due for an update for 6,198 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rPHU" is currently updating.
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rSVN" is not due for an update for 21,598 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rPHY" is currently updating.
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rGTEST" is not due for an update for 21,598 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rINIS" is not due for an update for 6,894 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rARCLINT" is not due for an update for 21,599 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rLPHX" is not due for an update for 1,979 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rARC" is not due for an update for 1,824 second(s).
<VERB> PhabricatorRepositoryPullLocalDaemon Repository "rINIHG" is not due for an update for 21,599 second(s).
...
```
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4605
Differential Revision: https://secure.phabricator.com/D8782
Summary:
Ref T4605. Fixes T3466. The major change here is that we now run up to four simultaneous updates. This should ease cases where, e.g., one very slow repository was blocking other repositories. It also tends to increase load; the next diff will introduce smart backoff for cold repositories to ease this.
The rest of this is just a ton of logging so I can IRC debug these things by having users run them in `phd debug pulllocal` mode.
For T3466:
- You now have to hit four simultaneous hangs to completely block the update process.
- Importing repository updates are killed after 4 hours.
- Imported repository updates are killed after 15 minutes.
Test Plan:
- Ran `phd debug pulllocal` and observed sensible logs and behavior.
- Interrupted daemon from sleeps and processing with `diffusion.looksoon`.
- Ran with various `--not`, `--no-discovery` flags.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3466, T4605
Differential Revision: https://secure.phabricator.com/D8785
Summary:
Ref T4605. Before discovering branches, try to prefill the cache in bulk. For repositories with large numbers of branches, this allows us to issue dramatically fewer queries.
(Before D8780, this cache was usually held across discovery events, so being able to fill it cheaply was not as relevant.)
Test Plan: Ran discovery on Git, Mercurial and SVN repositories. Observed fewer queries for Git/Mercurial.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4605
Differential Revision: https://secure.phabricator.com/D8781
Summary:
Ref T4605. Currently, the PullLocal daemon is responsible for two relatively distinct things:
- scheduling repository updates; and
- actually updating repositories.
Move the "actually updating" part into a new `bin/repository update` command, which basically runs the pull, discover, refs and mirror commands. This will let the parent process focus on scheduling in a more understandable way and update multiple repositories at once. It also makes it easier to debug and understand update behavior since the non-scheduling pipeline can be run separately.
Test Plan:
- Ran `update --trace` on SVN, Mercurial and Git repos.
- Ran PullLocal daemon for a while without issues.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4605
Differential Revision: https://secure.phabricator.com/D8780
Summary: We have too much space on workboards when displayed on mobile devices.
Test Plan: Shrink browser display, note that all workboards align to common gutters.
Reviewers: epriestley, btrahan
Reviewed By: btrahan
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8790
Summary: sets action list to crumbs
Test Plan: shrink browser, see mobile action list, click on it, edit
Reviewers: epriestley, btrahan
Reviewed By: btrahan
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8789
Summary: I recently made this better about accepting project names, but we use it in some cases with PHIDs. Make that work properly again.
Test Plan: Clicked "New Task" from a project page.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8778
Summary:
Fixes T4477. Sort of winging this but it's probably the right fix?
One error in T4477.
One error via email:
```
[2014-04-15 17:44:34] ERROR 8: Undefined index: /some_index/ at [/phab_path/phabricator/src/applications/owners/storage/PhabricatorOwnersPackage.php:213]
#0 PhabricatorOwnersPackage::findLongestPathsPerPackage(Array of size 3 starting with: { 0 => Array of size 3 starting with: { id => 5 } }, Array of size 8 starting with: { / => Array of size 2 starting with: { /some_index/some_file.py => true } }) called at [/phab_path/phabricator/src/applications/owners/storage/PhabricatorOwnersPackage.php:170]
#1 PhabricatorOwnersPackage::loadPackagesForPaths(Object PhabricatorRepository, Array of size 2 starting with: { 0 => /some_index/some_file.py }) called at [/phab_path/phabricator/src/applications/owners/storage/PhabricatorOwnersPackage.php:119]
...
```
Test Plan: Will make @zeeg do it.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, zeeg
Maniphest Tasks: T4477
Differential Revision: https://secure.phabricator.com/D8779
Summary: Fixes T4655. Basically leaves the display code intact for legacy installs but removes the option from the UI and removes "create" code.
Test Plan:
tried to attach file and the action was not in the dropdown!
made a new task and it worked!
commented on an old task and it worked!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4655
Differential Revision: https://secure.phabricator.com/D8777
Summary:
See discussion in D8773. Three small adjustments which should help prevent this kind of issue:
- When queueing followup tasks, hold them on the worker until we finish the task, then queue them only if the work was successful.
- Increase the default lease time from 60 seconds to 2 hours. Although most tasks finish in far fewer than 60 seconds, the daemons are generally stable nowadays and these short leases don't serve much of a purpose. I think they also date from an era where lease expiry and failure were less clearly distinguished.
- Increase the default wait-after-failure from 60 seconds to 5 minutes. This largely dates from the MetaMTA era, where Facebook ran services with high failure rates and it was appropriate to repeatedly hammer them until things went through. In modern infrastructure, such failures are rare.
Test Plan:
- Verified that tasks queued properly after the main task was updated.
- Verified that leases default to 7200 seconds.
- Intentionally failed a task and verified default 300 second wait before retry.
- Removed all default leases shorter than 7200 seconds (there was only one).
- Checked all the wait before retry implementations for anything much shorter than 5 minutes (they all seem reasonable).
Reviewers: btrahan, sowedance
Reviewed By: sowedance
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8774
Summary:
Recently we see issues with huge commits (branch cuts for www) where people received hundreds of emails for the same commit. By checking all the active and archived tasks related to such commits, I saw the following pattern:
- The commit itself is marked as importStatus = 15 which means all the processing was actually done;
- In archived tasks, I see one PhabricatorRepositorySvnCommitMessageParserWorker, one PhabricatorRepositorySvnCommitChangeParserWorker, followed by many PhabricatorRepositoryCommitHeraldWorker, which means that the PhabricatorRepositoryCommitOwnersWorker (who schedule those herald tasks) was never done;
- PhabricatorRepositoryCommitOwnersWorker is always active (for days) with failureCount = 0;
- In daemon log I see a lot of lease expire exception for PhabricatorRepositoryCommitOwnersWorker.
So to me it looks like the following happened:
- Everything is fine until we schedule the PhabricatorRepositoryCommitOwnersWorker
- PhabricatorRepositoryCommitOwnersWorker actually successfully finished but its running time exceed 60s. Before it finishes, it scheduled the PhabricatorRepositoryCommitHeraldWorker task
- When we try to archive it, the lease expiration exception happened. As a result, it stayed active and will be picked up immediately since it is in the head of the queue
- The two steps above repeat forever until we kill it
I am not sure why we want to check lease expiration when we are archiving the task. For now I am giving the worker a little more time since parsing half million affected path needs some time..
Test Plan: Patched in our production and it worked.
Reviewers: lifeihuang, JoelB, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8773
Summary: Ref T3551. Since we now require repositories in order to perform policy checks, things that did loads properly don't need to load this data explicitly.
Test Plan: Edited a product, cut a new branch.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3551
Differential Revision: https://secure.phabricator.com/D8769
Summary:
Ref T3551. Releeph has old-style `loadX()` methods; get rid of one of them.
Differential has a couple of copies of this too, clean them up.
Test Plan:
- Viewed various differential revisions (with and without projects).
- Viewed and edited Releeph products.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3551
Differential Revision: https://secure.phabricator.com/D8768
Summary:
Fixes T3657. We no longer construct ambiguous URIs, so product names are no longer restricted.
Also fix some minor URI construction stuff.
Test Plan: Created a product called "branch".
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3657
Differential Revision: https://secure.phabricator.com/D8767
Summary:
Ref T3657. General changes here:
- Removes `ReleephProjectController`, which is the source of T3657.
- Mostly moves requests from "RQ" as a monogram to "Y" (looks like a merge, mnemonic for "yank"?, we don't have too many characters left). This should be essentially only cosmetic. This reduces ambiguity with "rQ" and "R123", which are current and future repository monograms. This will continue in the next few diffs.
- Makes requests implement policies correctly.
Test Plan: Created, edited, browsed requests.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3657
Differential Revision: https://secure.phabricator.com/D8766
Summary:
Ref T4045. We have a lot of direct queries against the hunk table right now. These are messy, not really policy-aware, and limit our options on T4045.
This query is unusual (it requires changesets, and does not accept IDs). This keeps us from having to load changeset -> diff -> revision in order to do policy checks. We could also fix this with smarter policy checks and caching, but I'd rather not open that can of worms for now. This object is very low level and relatively unusual, and this small deviation from convention seems like the cleanest cut to make to keep this from snowballing.
Test Plan: Used Herald dry runs to verify that the affected rules still output the same data.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4045
Differential Revision: https://secure.phabricator.com/D8765
Summary: Ref T4045. These three methods are fairly copy-pastey. Provide a more formal DifferentialHunk API for querying various types of line ranges.
Test Plan: Used test console to verify that "added content", "removed content", and "changed content" rules still produce the same data.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4045
Differential Revision: https://secure.phabricator.com/D8764
Summary:
Ref T3644. Ref T3657. Ref T3549. Basically:
- Move these controllers to modern query/policy infrastructure.
- Move them to consistent, ID-based URIs.
- Rename "Project" to "Product"; "Pick Request" to "Pull Request".
- Clean up a few UI things here and there.
Test Plan:
- Created and edited branches.
- Opened and closed branches.
- Viewed branch history.
- Searched within a branch.
- Browsed to branches from products.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3644, T3549, T3657
Differential Revision: https://secure.phabricator.com/D8646
Summary: Fixes T4774. With the new code and configuration instructions downplaying the role of arcanist project we weren't writing affected paths at all! I had this issue on my installation - no affected paths were written. We seem to always have the repository now though if we can see it, so not too bad of a fix.
Test Plan: updated a diff and was able to browse in diffusion.
Reviewers: epriestley, bitglue
Reviewed By: epriestley, bitglue
Subscribers: bitglue, epriestley, Korvin
Maniphest Tasks: T4774
Differential Revision: https://secure.phabricator.com/D8757
Summary: Fixes T2328. Note the audit part is fixed now.
Test Plan: Tried to reproduce the audit issue by raising my own commit as a problem; it showed up before code changes! Made a diff with my self as author and reviewer; it showed up as expected
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T2328
Differential Revision: https://secure.phabricator.com/D8755
Summary: Fixes T3576
Test Plan: made a countdown and it looked right on view. edited it and it had the right values pre and post edit.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T3576
Differential Revision: https://secure.phabricator.com/D8754
Summary: We make a silly query for every commit if you copy/paste a diff.
Test Plan: Copy/pasted diffs now render in fewer than 30 seconds.
Reviewers: btrahan, spicyj
Reviewed By: btrahan, spicyj
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8758
Summary: I haven't been able to understand why this isn't set by default in production environments (since it is recommended to do so anyway).
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8743
Summary: ...add a "renderingTarget" to FeedStory and use it as appropos. Overall, not a ton of changes was necessary to make this work. I think this could be made to be even cleaner by going through each and every feed story and re-implementing as necessary with the full toolset available. But this is good enough for now I think, and just something to keep cleaning up when we're in here. Fixes T4630.
Test Plan: made a task. gave it a token. viewed my feed - saw stories. used conduit.feed.query with mode == 'text' and got good readable results.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: spicyj, epriestley, Korvin
Maniphest Tasks: T4630
Differential Revision: https://secure.phabricator.com/D8750
Summary: ...use the prefab stuff as it does fancier things than we were doing. Only trick then really is to pass username and the map of handle phids => icons to the client so prefab can work nicely. Fixes T4775.
Test Plan: made a herald rule with projects and users. Saw nice icons. Reloaded page and still saw nice icons.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4775
Differential Revision: https://secure.phabricator.com/D8749
Summary:
Ref T4786. This doesn't fully fix the issue since there's no way to make channels public yet, but gets some of the infrastructure more up to date.
- Allow public access to the list and log controllers.
- Implement proper policy checks in the Events (this has no practical impact on the only controller that loads this stuff, it's just for general/future purposes).
- Remove a old-style unused method for building page frames.
Test Plan: Viewed log list and log details as logged-in and logged out users.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4786
Differential Revision: https://secure.phabricator.com/D8746
Summary:
When we generate account tokens for CSRF keys and email verification, one of the inputs we use is the user's password hash. Users won't always have a password hash, so this is a weak input to key generation. This also couples CSRF weirdly with auth concerns.
Instead, give users a dedicated secret for use in token generation which is used only for this purpose.
Test Plan:
- Ran upgrade scripts.
- Verified all users got new secrets.
- Created a new user.
- Verified they got a secret.
- Submitted CSRF'd forms, they worked.
- Adjusted the CSRF token and submitted CSRF'd forms, verified they don't work.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8748
Summary:
I added a getTaskPriorityColor function to the ManiphestTaskPriority class which returns the color set in the maniphest config for the given priority.
This is in preparation for a change to arcanist which will allow it to display the priority color (if it is a supported color) upon running `arc tasks`.
Fixed some linting issues
Test Plan:
Invoke the maniphest.info method from conduit and ensure that:
* The priorityColor property is given in the json
* the priorityColor property is set correctly
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8734
Summary: Fixes T4755. This also includes putting in a note that Google might ToS you to use the Google+ API. Lots of code here as there was some repeated stuff between OAuth1 and OAuth2 so I made a base OAuth with less-base OAuth1 and OAuth2 inheriting from it. The JIRA provider remains an independent mess and didn't get the notes field thing.
Test Plan: looked at providers and read pretty instructions.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4755
Differential Revision: https://secure.phabricator.com/D8726
Summary: Fixes T4777. We technically support `?projects=...` already, but parse it in an unusual way and apply old, awkward, excessively strict lookups to it.
Test Plan: Used reasonable, standard, human-readable strings to prefill `?projects=` and got the results I expected.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4777
Differential Revision: https://secure.phabricator.com/D8733
Summary:
Arcanist is currently displaying all tasks as closed when invoking `arc tasks`.
This is because arcanist is setting the display to closed if there is anything in the `status` property. Adding an isClosed property will allow arcanist to properly display open/closed status on tasks by checking against the isClosed property. The isClosed property will be set according to the closed property that is set on each status in maniphest.
Test Plan:
Invoke the conduit maniphest.info method on any task and insure that:
# The isClosed property is included in the properties
# that it is set properly according to the statuses set for maniphest.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: chad, epriestley, Korvin
Maniphest Tasks: T4744
Differential Revision: https://secure.phabricator.com/D8731
Summary:
Fixes T4759.
Turns out Chrome on windows doesn't really like the word joiner character. We'll switch back to zwsp but make it `position: absolute;` so it doesn't turn into a line break.
Test Plan: Looked at diffs in IE9 and Chrome Windows. Made sure copying still works as expected.
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4759
Differential Revision: https://secure.phabricator.com/D8727
Summary: Fixes T4772. We weren't parsing generated public keys properly, and were storing them in the wrong format.
Test Plan:
- Updated a private key.
- Generated a public key.
- Saved the public key.
- Used a generated private key to authenticate.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4772
Differential Revision: https://secure.phabricator.com/D8721
Summary:
Fixes T4773. For config settings of type `list<string>`, `set`, or `list<regex>`, the "defaults" table and "examples" aren't always in the same format you should actually use when changing the setting.
This is pretty confusing. Instead, always show the settings in the desired format. For example, if the user should enter a newline-separated list, show them a newline separated list.
Test Plan:
- Grepped for `list<string>`, `list<regex>`, and `'set'`; verified all the config had the right example format (most already did).
- Viewed config settings of various kinds, including custom settings.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4773
Differential Revision: https://secure.phabricator.com/D8725
Summary: wasn't working due to some type issues. Fixes T4756. I also made it display nicer while I was debugging this.
Test Plan: created a herald rule to block changes that added refs. git tag -a "test" -m "test test"; git push origin test got me blocked!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4756
Differential Revision: https://secure.phabricator.com/D8724
Summary: Fixes T3208. This forces us to bind+search even if there are no anonymous credentials.
Test Plan: Checked the box, saved the form. Unchecked the box, saved the form. LDAP??
Reviewers: Firehed
Reviewed By: Firehed
Subscribers: epriestley
Maniphest Tasks: T3208
Differential Revision: https://secure.phabricator.com/D8723
Summary: Ref T182. This feature rarely/never works and is on the balance enormously confusing to users (see <https://github.com/facebook/phabricator/issues/566>). If installs have somehow made it sort of work, they can comment this line out for now until we have time to make this work more reasonably.
Test Plan: Looked at a revision in Differential.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T182
Differential Revision: https://secure.phabricator.com/D8719
Summary:
Fixes T4736. Currently, we incorrectly skip the `writeImportStatusFlag()` call if publishing is disabled (the `herald-disabled`) check. This means we don't flag the commit as imported, and don't move the pipeline forward correctly.
Instead, we only want to skip the owners stuff, not the pipeline stuff. Move that to a method.
(Also fix a nearby TODO now that we have a permanent failure exception.)
Test Plan:
- Used `scripts/repository/reparse.php --owners ...` to execute this code, fiddled with things to hit both the disabled and enabled branches and verified the flag stuff is still reached.
- Faked the exceptions and made sure they raise correctly.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4736
Differential Revision: https://secure.phabricator.com/D8715
Summary: I also changed PhabricatorApplicationTransactionFeedStory and the TokenGivenFeedStory to include only the title/first line of the feed story, which is more convenient (previously, strip_tags gave a multi-line story without even any linebreaks) and more consistent with the other story types.
Test Plan: Added a requestbin URL to feed.http-hooks, commented on a Differential, and saw storyText equal to "alpert added a comment to D2: c." in the POST data it received.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T4630
Differential Revision: https://secure.phabricator.com/D8710
Summary: From IRC, this is sometimes helpful for debugging if there's a mailing list issue or something like that. For example, it can show "To" and "Cc".
Test Plan: Got some email, saw headers in it.
Reviewers: dctrwatson, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8708
Summary:
This text is overly repetitive and is not super important. Keeps the other states. Also
- Easier to parse reviewers now
- Mobile is less janky
Test Plan:
reload my list of diffs
{F138756}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8707
Summary: Fixes T4687. This was also pretty easy...!
Test Plan: made a package with a test user as owner. added package as owner. looked right on commit page. logged in as test user and verified audit showed up on home page.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: chad, epriestley, Korvin
Maniphest Tasks: T4687
Differential Revision: https://secure.phabricator.com/D8705
Summary: ...the key is to move a layer lower and beam down the updated comment. There is a wee bit of Javascript gymnastics going on here. Fixes T4608.
Test Plan: made a comment + resolve. clicked edit and made changes. noted transaction updated correctly and "history" link worked. edited again to a deletion and noted the "this is deleted" looked right and history link still worked
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin, chad
Maniphest Tasks: T4608
Differential Revision: https://secure.phabricator.com/D8702
Summary: Ref T4687. Trickier part is adding packages; will require some typeahead core changes
Test Plan: add a project as an auditor succuessfully!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4687
Differential Revision: https://secure.phabricator.com/D8704
Summary: Ref T4371. We can reuse more code for this "your stuff is empty" error, now, and benefit from global rate limiting and being able to reply to arbitrary addresses.
Test Plan: Sent valid, empty, and empty-ignored email via `mail_handler.php`, got appropriate actions/errors/states.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4371
Differential Revision: https://secure.phabricator.com/D8701
Summary: This "Reply to comment, etc., etc." section got lost along the way at some point. Restore it for transaction mail.
Test Plan: Received mail from Maniphest with reply instructions.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8700
Summary:
We currently share the same regexp between PHID matching (usually unambiguous) and remarkup matching (often ambiguous).
This means that some project monograms which should work fine don't work properly in some contexts. Improve these behaviors.
For example:
- `#domain.com`
- Previously did not work at all.
- Now works in unambiguous cases, and in remarkup.
- `#1`
- Previously did not work at all.
- Now works in unambiguous cases.
- `#dot.`
- Previously did not work at all.
- Now works in unambiguous cases.
Test Plan:
- Created projects `domain.com`, `1`, etc.
- Used jump nav to match them unambiguously, everything worked.
- Used remarkup preview to match them ambiguously, the reasonable ones worked.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8703
Summary:
Ref T4371. Ref T4699. Fixes T3994.
Currently, we're very conservative about sending errors back to users. A concern I had about this was that mistakes could lead to email loops, massive amounts of email spam, etc. Because of this, I was pretty hesitant about replying to email with more email when I wrote this stuff.
However, this was a long time ago. We now have Message-ID deduplication, "X-Phabricator-Sent-This-Mail", generally better mail infrastructure, and rate limiting. Together, these mechanisms should reasonably prevent anything crazy (primarily, infinite email loops) from happening.
Thus:
- When we hit any processing error after receiving a mail, try to send the author a reply with details about what went wrong. These are limited to 6 per hour per address.
- Rewrite most of the errors to be more detailed and informative.
- Rewrite most of the errors in a user-facing voice ("You sent this mail..." instead of "This mail was sent..").
- Remove the redundant, less sophisticated code which does something similar in Differential.
Test Plan:
- Using `scripts/mail/mail_receiver.php`, artificially received a pile of mail.
- Hit a bunch of different errors.
- Saw reasonable error mail get sent to me.
- Saw other reasonable error mail get rate limited.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3994, T4371, T4699
Differential Revision: https://secure.phabricator.com/D8692
Summary: This ensures that two comments by the same author on the same line are sorted properly.
Test Plan: Before this patch, made two comments that appeared in the wrong order. With this patch, they sort correctly.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8697
Summary: Make the actions appear in crumbs on mobile
Test Plan: Test action list on a mobile diff layout
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4730
Differential Revision: https://secure.phabricator.com/D8691
Summary: I accidentally made these exceptionally ugly recently.
Test Plan: {F137411}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley, chad
Differential Revision: https://secure.phabricator.com/D8684
Summary: The "burnup chart" relies on these to determine when tasks opened and we recently stopped writing them. Keep writing them for now. They're fluff and don't show up in the UI, but draw the right chart.
Test Plan: Saw chart go up when I made tasks.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8682
Summary:
This adds a system which basically keeps a record of recent actions, who took them, and how many "points" they were worth, like:
epriestley email.add 1 1233989813
epriestley email.add 1 1234298239
epriestley email.add 1 1238293981
We can use this to rate-limit actions by examining how many actions the user has taken in the past hour (i.e., their total score) and comparing that to an allowed limit.
One major thing I want to use this for is to limit the amount of error email we'll send to an email address. A big concern I have with sending more error email is that we'll end up in loops. We have some protections against this in headers already, but hard-limiting the system so it won't send more than a few errors to a particular address per hour should provide a reasonable secondary layer of protection.
This use case (where the "actor" needs to be an email address) is why the table uses strings + hashes instead of PHIDs. For external users, it might be appropriate to rate limit by cookies or IPs, too.
To prove it works, I rate limited adding email addresses. This is a very, very low-risk security thing where a user with an account can enumerate addresses (by checking if they get an error) and sort of spam/annoy people (by adding their address over and over again). Limiting them to 6 actions / hour should satisfy all real users while preventing these behaviors.
Test Plan:
This dialog is uggos but I'll fix that in a sec:
{F137406}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8683
Summary: This should prevent long lines from making the code width different between files, which can be annoying. (And of course, it stops long lines from making a giant scrollbar too.)
Test Plan:
Loaded this diff in Chrome, Firefox, IE9, and IE8:
{F137505}
(That's a screenshot from Chrome, but it looks about the same in the other browsers.)
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, chad
Maniphest Tasks: T2004
Differential Revision: https://secure.phabricator.com/D8686
Summary: This sets the name parameter when Drydock uploads a file so that the storage engine picks it up correctly.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8673
Summary:
This does two things
- Modernizes Table of Contents
- Makes Differential reasonable on mobile
I say resonable, as you still have to scroll horizontal to see the entire diff. This is minor as the rest of the page is 100x more useful. A 1-up view would be preferred, but this is still an improvement.
Test Plan: Used iOS simulator for browsing diffs.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin, chad
Differential Revision: https://secure.phabricator.com/D8681
Summary: ...also link to commits we know about in "Local Commits" and "Revision Update History" tables. Fixes T4585.
Test Plan: made a repo. made a diff (foo) and committed it (bar). made a new diff that was comprised of two local commits. noted links to (bar) in various commit hashes as expected
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin, chad
Maniphest Tasks: T4585
Differential Revision: https://secure.phabricator.com/D8679
Summary: Fixes T3047. Update this document and remove some lies ("menu bar is read in admin interfaces"!!!!).
Test Plan:
- Read text.
- Searched for "System Agent" in the UI and replaced it with "bot" or "bot/script" or similar.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3047
Differential Revision: https://secure.phabricator.com/D8675
Summary:
Fixes T4065. This divides user creation into separate "Standard User" and "Script/Bot" workflows which show only relevant fields and provide guidance.
This fixes the verification mess associated with script/bot users by verifying their email addresses automatically.
Test Plan:
- Created a standard user.
- Created a script/bot.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4065
Differential Revision: https://secure.phabricator.com/D8674
Summary: Ref T4065. Moves the last of the weird alternate edit UI to profiles. The old "Edit" controller is now for creation only, and the funky pencil icon is gone.
Test Plan: Created accounts; sent welcome email.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4065
Differential Revision: https://secure.phabricator.com/D8670
Summary: Ref T4065. Give administrators an "Edit Settings" link from profiles, which allows selective edit of settings panels. Enable Conduit, SSH Keys, and VCS Password.
Test Plan:
- Used these panels for a bot.
- Used these panels on my own account.
- Tried to use these panels for a non-bot account, was denied.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4065
Differential Revision: https://secure.phabricator.com/D8668
Summary: Ref T4065. Moves the "disable / enable" and "make / unmake administrator" actions to profiles.
Test Plan: Disabled and enabled users, and made and unmade administrators.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4065
Differential Revision: https://secure.phabricator.com/D8666
Summary:
Ref T4065. Currently, we have this super copy/pasted "edit profile picture" UI for system agents.
Instead, give administrators direct access from profiles, so they can use the same code pages do.
Test Plan: Edited my profile picture and profile details. Edited an agent's. Was unable to edit a non-agent user.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4065
Differential Revision: https://secure.phabricator.com/D8664
Summary: Ref T4065. Make this work in a more standard way which administrators have a reasonable shot at finding and using. See D8662 for discussion.
Test Plan: Changed a user's username.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4065
Differential Revision: https://secure.phabricator.com/D8663
Summary:
Ref T4065. The existence of two separate edit workflows for users is broadly confusing to administrators.
I want to unify user administration and improve administration of system agent accounts. Particularly, I plan to:
- Give administrators limited access to profile editing of system agents (e.g., change profile picture).
- Give administrators limited access to Settings for system agents.
- Broadly, move all the weird old special editing into standard editing.
Test Plan:
- Hit all the errors (delete self, no username, wrong username).
- Deleted a user.
- Visited page as a non-admin, got 403'd.
- Viewed old edit UI.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4065
Differential Revision: https://secure.phabricator.com/D8662
Summary:
Currently, users get an error when making any changes to this field if they don't have a linked JIRA account.
Instead:
- We should only raise an error if they're trying to //add// issues, and only on the new issues. It's always fine to remove issues, and existing issues the author can't see are also fine.
- When we can't add things because there's no account (vs because there's a permissions error or they don't exist), raise a more tailored exception.
Test Plan:
- As JIRA and non-JIRA users, made various edits to this field.
- Got appropriate exceptions, including better tailoring.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: mbishopim3, epriestley
Differential Revision: https://secure.phabricator.com/D8676
Summary: Fixes T4632.
Test Plan: viewed a transcript for rule x which depends on rule y and noted "rule y" printed out rather than "PHID-BLAH-BLAH"
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4632
Differential Revision: https://secure.phabricator.com/D8678
Summary: the quotes are 'cuz "create" is inferred. Previously, we inferred on "status", but since we set that on "initializeNewTask" instead infer off "title" (aka "name") like most other apps do. Only hairy tweak was to elevate TYPE_TITLE to the most important of all maniphest transactions, which doesn't actually seem too unreasonable if not correct even? Fixes T4686.
Test Plan: made a new task, used bin/mail, got the right headers (mail vary prefix == created)
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4686
Differential Revision: https://secure.phabricator.com/D8639
Summary: When trying to render "BRANCH", we need the active diff. Load it
in general since it seems reasonable for custom fields to expect it to
exist during mail rendering.
Summary: Fixes T4697. When pushing moved/copied files, SVN sends an "add-file" protocol frame which has a URI in it that needs translation from external format ("/diffusion/X/") to internal format ("/path/to/svn").
Test Plan:
- Copied/moved files and committed them in SVN.
- Added files (no copy/move) and committed them in SVN.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4697
Differential Revision: https://secure.phabricator.com/D8654
Summary: Ref T418. Fixes T4642. The "changes since last update" and "branch" fields got dropped; restore them in a general, field-driven way.
Test Plan:
- Created a revision, got relevant sections in mail.
- Commented on a revision, got relevant sections in mail.
- Updated a revision, got relevant sections in mail.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: spicyj, epriestley
Maniphest Tasks: T418, T4642
Differential Revision: https://secure.phabricator.com/D8657
Summary: Fixes T4683. This was just a missing method implementation. Also provide a couple of translation things.
Test Plan:
- Created a revision from the command line with a nonempty `JIRA Issues:` line, via `arc diff`.
- Looked at the translation strings.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4683
Differential Revision: https://secure.phabricator.com/D8656
Summary: Previously, you would not receive a mail message for the first comment you make on an audit, but you would for subsequent comments because everyone who's made a comment would be CCed on the email. This mirrors DifferentialTransactionEditor's getMailTo which always adds `$object->getAuthorPHID()`.
Test Plan: With self mail turned on, made the first comment on a commit and received an email for it. With self mail turned off, commented on a different commit and saw in `bin/mail list-outbound` that the message was voided.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8650
Summary: Uses cards, fixes bgcolors.
Test Plan: View edit history on a few documents.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin, chad
Differential Revision: https://secure.phabricator.com/D8648
Summary:
When sending the "Reply-To" header to Mailgun, Phabricator would
previously send two headers for every "Reply-To": "Reply-To[0][email]" and
"Reply-To[0][name]". Instead, explicitly build the header as specified by RFC
2822 and send it to Mailgun pre-baked.
Pretty sure this bug was a cargo-cult from the Sendgrid code, where (apparently)
this actually works.
Test Plan:
Triggered an email from Phabricator, saw that the header was sent
properly.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8645
Summary:
It needs attention! Serious color for serious action.
Also, Flags probably need urgent action!1!
Test Plan: Reload Hompage, see new color
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin, chad
Differential Revision: https://secure.phabricator.com/D8644
Summary: Ref T3092. Ref T3549. Modernize the product creation and edit UIs and make them say "product" instead of "project".
Test Plan:
- Created products.
- Edited products.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3092, T3549
Differential Revision: https://secure.phabricator.com/D8636
Summary: Ref T3549. This table isn't written to yet; rename it and the DAOs and modernize the history controller.
Test Plan: Viewed history page for a product.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3549
Differential Revision: https://secure.phabricator.com/D8633
Summary:
Ref T3549. A few things here:
- Releeph has an object called a "Project". We'd like to call this a "Product" instead. See T3549. Rename easy instances that don't break URIs.
- Releeph has a "ProjectController" which tries to be smart about loading objects. However, it's big and messy and doesn't have the finesse to do policies or `needX(...)` correctly. It also generates URIs which collide with one another. Introduce "ProductController" to start to move away from it.
- Some small modernizations to this controller to take advantage of newer infrastructure (like easier dialog rendering).
Test Plan: Deactivated and reactivated products.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3549
Differential Revision: https://secure.phabricator.com/D8632
Summary:
Fixes T3738. Facebook uses this to provide a couple of integrations (push karma, is user an intern?), but the mechanism is both very complex and not very general.
Instead, these features are better implemented in Hovercards or via CustomField. We'll help Facebook integrate things when the time comes, but per discussion in T3738 none of this is critical or especially complicated.
Test Plan:
- Grepped for all callsites.
- Viewed a request and verified that author/requestor populated and rendered correctly.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T3738
Differential Revision: https://secure.phabricator.com/D8631
Summary:
Fixes T3659. Releeph has some awkward complexity around who ends up as a commit author. Instead, we should always try to use the original author.
Metadata (like the requestor's identity) should be accessed via Conduit or other channels instead.
Test Plan: Saved some projects, grepped for all related symbols.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T3659
Differential Revision: https://secure.phabricator.com/D8630
Summary: Fixes T3658. This field doesn't make much sense and doesn't appear to ever have actually been implemented. Particularly, the `%N` pattern doesn't actually work and I can't find anything which actually calls this stuff or exposes it externally. Facebook doesn't use it (see T3658) and I don't think it's useful in general.
Test Plan: Used `grep` to look for stuff, edited a project.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T3658
Differential Revision: https://secure.phabricator.com/D8628
Summary: move code to uninstallable help app rather than diviner. Fixes T4690.
Test Plan: uninstalled diviner, noted no links, then moved the code and suddenly helpful help links showed up once more.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4690
Differential Revision: https://secure.phabricator.com/D8638
Summary: 0 => imploded string of hotness. Fixes T4689
Test Plan: for each spot i fixed, clicked link and it worked! (I did a grep for "/maniphest" to find these spots; 98% confident I got them all.)
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4689
Differential Revision: https://secure.phabricator.com/D8626
Summary: OMG We Have TOKENS
Test Plan: TOKENS, also UIExamples
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin, chad
Differential Revision: https://secure.phabricator.com/D8624
Summary: Fixes T4641.
Test Plan: Dragged a "normal" task between "high" and "low" tasks and it stayed as "normal". Generally seems correct when playing around.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: mbishopim3, Beltran-rubo, epriestley, Korvin
Maniphest Tasks: T4641
Differential Revision: https://secure.phabricator.com/D8622
