Summary:
Ref T4398. Ref T4842. I want to let users review their own account activity, partly as a general security measure and partly to make some of the multi-factor stuff easier to build and debug.
To support this, implement modern policies and application search.
I also removed the "old" and "new" columns from this output, since they had limited utility and revealed email addresses to administrators for some actions. We don't let administrators access email addresses from other UIs, and the value of doing so here seems very small.
Test Plan: Used interface to issue a bunch of queries against user logs, got reasonable/expected results.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: keir, epriestley
Maniphest Tasks: T4842, T4398
Differential Revision: https://secure.phabricator.com/D8856
Summary:
Ref T4398. This is roughly a "sudo" mode, like GitHub has for accessing SSH keys, or Facebook has for managing credit cards. GitHub actually calls theirs "sudo" mode, but I think that's too technical for big parts of our audience. I've gone with "high security mode".
This doesn't actually get exposed in the UI yet (and we don't have any meaningful auth factors to prompt the user for) but the workflow works overall. I'll go through it in a comment, since I need to arrange some screenshots.
Test Plan: See guided walkthrough.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4398
Differential Revision: https://secure.phabricator.com/D8851
Summary: Removes many tables and uses PropertyLists and ObjectItemList when possible. Adds cleaner CSS, makes mobile editing more possible.
Test Plan: Test new UI on desktop and mobile. Verify all functionality still exists.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4272
Differential Revision: https://secure.phabricator.com/D8860
Summary: ...also kills off "PhabricatorAuditCommitQuery" and "PhabricatorAuditQuery", by moving the work to "DiffusionCommitQuery". Generally cleans up some code around the joint on this too. Also provides policies for audit requests, which is basically the policy for the underlying commit. Fixes T4715. (For the TODO I added about files, I just grabbed T4713.)
Test Plan:
Audit: verified the three default views all showed the correct things, including highligthing. did some custom queries and got the correct results.
Diffusion: verified "blame view" still worked. verified paths were highlighted for packages i owned.
Home: verified audit boxes showed up with proper commits w/ audits
bin/audit: played around with it via --dry-run and got the right audits back
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: chad, epriestley, Korvin
Maniphest Tasks: T4715
Differential Revision: https://secure.phabricator.com/D8805
Summary:
When showing contents of a file with the blame mode enabled, tooltips pops out
when the mouse hovers over previous commit linkes on left side. The last part of the
tooltips is the author's name. If an author is unregistered, the name becomes
<span>name</span>.
{F147724}
This doesn't happen if the author is registered.
Test Plan:
Check tooltips after making the change.
{F147725}
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8869
Summary:
This algorithm is tricky, and uses `phutil_safe_html()` directly, which makes it potentially unsafe.
In particular, D8859 fixes a bug with it which caused it to produce non-utf8 output. This doesn't guarantee it's a security problem, but does make it suspicious.
I don't actually see a way to break it, but rewrite it so that it's absolutely bulletproof and does not need to call `phutil_safe_html()`.
Test Plan:
{F147487}
@rugabarbo, if you have a chance, can you check if this still works for you?
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, rugabarbo
Differential Revision: https://secure.phabricator.com/D8862
Summary: Fixes T4899. Action strengths got lost somewhere along the way; actions like "Accepted" should be stronger than "Changed Subscribers".
Test Plan: Verified things sort as expected now, with major actions at the top.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4899
Differential Revision: https://secure.phabricator.com/D8857
Summary: Fixes T4903. At some point maybe-soonish we should maybe go make `"device" => true` the default, and put `"device" => "hella-busted"` on the remaining bad pages.
Test Plan: L@@K @ W/ iOS Simulator
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley, k
Maniphest Tasks: T4903
Differential Revision: https://secure.phabricator.com/D8863
Summary:
I created this review to get an answer...
It should not be taken as a real fix.
I noticed that phabricator return corrupted search results for some russian queries (without this patch).
See screenshot:
{F147443}
But I can't reproduce this bug on https://secure.phabricator.com/
This search query causes problems only for my phabricator instance.
More than that, I didn't find any php.ini-settings that can resolve this problem.
It's look like your phabricator instance use /u-modifier by default.
But how is it possible?
Test Plan: NONE
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8859
Summary: Fixes T4878
Test Plan:
1. Go to paste
2. Add comment
3. Check that paste subsribers got paste link by email
See T4878 for more details.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T4878
Differential Revision: https://secure.phabricator.com/D8861
Summary: Some actions (notably, `!accept`) require more information than we currently load.
Test Plan: Piped in some `!accept` mail using `bin/mail receive-test`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8844
Summary: Fixes T3566 List of poll actions should include ability to close an open poll or reopen a closed poll.
Test Plan: Poll author should be able to close/reopen poll. Non-author should get policy screen when attempting to close/reopen poll.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T3566
Differential Revision: https://secure.phabricator.com/D8846
Summary: See IRC. We construct this a little bit wrong if there are multiple "open" statuses. Use a more modern construction.
Test Plan: Hit `?statuses=wontfix,invalid`, etc. Clicked "view all" from projects.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8854
Summary: Fixes T4606. Also shortens two unusual type names which are currently inconsistent.
Test Plan: Expanded advanced search.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4606
Differential Revision: https://secure.phabricator.com/D8853
Summary:
In some applications, using `{V2}` syntax to embed a vote throws. The chain of causality looks like this:
- We try to render a `phabricator_form()`.
- This requires a CSRF token.
- We look for a CSRF token on the user.
- It's an omnipotent user with no token, so everything fails.
To resolve this, make sure we always pass the real user in.
Test Plan:
- Lots of `grep`.
- Made a Differential comment with `{V2}`.
- Made a Diffusion comment with `{V2}`.
- Made a Maniphest comment with `{V2}`.
- Replied to a Conpherence thread with `{V2}`.
- Created a Conpherence thread with `{V2}`.
- Used Conduit to update a Conpherence thread with `{V2}`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, lkassianik
Differential Revision: https://secure.phabricator.com/D8849
Summary:
If you create a diff with no hunks (e.g., it adds a single empty file), we never attachHunks() so we throw on getHunks().
Instead, make sure changesets get hunks attached if they expect it.
Test Plan: Created a new diff with a single empty file in it.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: zeeg, epriestley
Differential Revision: https://secure.phabricator.com/D8842
Summary: These are a little easier on the eyes.
Test Plan:
Reject an epriestley diff.
{F146851}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8841
Summary:
Ref T4866. I did a fancy version of this but it looks pretty bad/confusing so here's a simple version.
Fancy-but-whack version:
{F146847}
Test Plan: This version is like that, but just always uses `fa-user`.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4866
Differential Revision: https://secure.phabricator.com/D8840
Summary: center aligns the icons in the fill area, removes some of the positioning jank. Also set new icons for maniphest custom.
Test Plan: test desktop and mobile layouts, tested thin pins for proper centering.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4866
Differential Revision: https://secure.phabricator.com/D8839
Summary: Throwing this up for testing, swapped out all icons in timeline for their font equivelants. Used better icons where I could as well. We should feel free to use more / be fun with the icons when possible since there is no penalty anymore.
Test Plan: I browsed many, not all, timelines in my sandbox and in IE8. Some of these were just swagged, but I'm expecting we'll do more SB testing before landing.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8827
Summary: For the time being, no need to have these in the repository.
Test Plan: Reload UIExamples, only see FontAwesome
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8835
Summary: The token transactions can publish empty transaction feed stories.
Stop them from doing that, and make notifications fail more quietly.
Auditors: btrahan
Summary:
- Support file attachments in Mailgun, after D8831.
- Fix `bin/mail send-test --attach ...` flag.
- Make `bin/mail send-test` route mail through the daemons.
- Remove the `workerTaskID` on MetaMTAMail, which is only used (needlessly) by `bin/mail resend` and creates a huge mess elsewhere.
- Currently, when mail fails, the daemon exits with a very generic and useless message. Instead, make `sendNow()` throw when it fails, so the real reason is surfaced. This is OK now because mail is always sent via the daemons.
- Now that Mailgun supports attachments, document it.
- Update a bunch of mail docs.
Test Plan:
- Sent mail.
- Sent mail with attachments.
- Read documentation.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8832
Summary: Fixes T4833. I wish there was an elegant way to catch this exception but I think the stack is written such that we really should just do this one-off query here...
Test Plan: from the "create project" link under "edit task" I received a more detailed exception than the report in T4833 post patch. I also tested editing an existing project - yay - and editing an existing project to some other existing project's name - got a nice error dialogue.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4833
Differential Revision: https://secure.phabricator.com/D8834
Summary:
Ref T4830. A few methods, like `conduit.ping`, are callable without authentication, so this even has some use cases. Also:
- Make some Differential stuff a little more consistent.
- Use slightly more modern rendering.
- Deprecate the status-oriented `user` calls; these will be replaced by Calendar methods.
Test Plan: Browsed console as logged out / logged in users.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4830
Differential Revision: https://secure.phabricator.com/D8826
Summary:
Ref T4830.
- If the application policy is public, allow logged-out users to browse examples.
- Use standard elements instead of custom ones.
Test Plan: Browsed UIExamples.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4830
Differential Revision: https://secure.phabricator.com/D8825
Summary:
Ref T3551. Currently, there are many layers of indirection between pull requests and revisions. After D8822, revisions and other types of requested objects are recorded directly on the request. This allows us to simplify data access and querying.
A lot of stuff here is doing `instanceof` checks to keep APIs stable, but most of those can go away in the long run.
Test Plan:
- Browsed requests.
- Verified revision-dependent fields (like "Revision", "Size", "Churn") still render correctly.
- Called `releeph.queryrequests`.
- Called `releephwork.nextrequest`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3551
Differential Revision: https://secure.phabricator.com/D8824
Summary:
Ref T3662. Releeph blocks users from requsting unparsed commits, but there's no real technical reason for this.
The `releephwork.getorigcommitmessage` method assumes data exists, but should be replaced with `diffusion.querycommits` anyway.
Test Plan: Ran `diffusion.querycommits`. Requested a commit.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3662
Differential Revision: https://secure.phabricator.com/D8823
Summary:
Ref T3551. Currently, ReleephRequests don't have a direct concept of the //object// being requested. You can request `D123`, but that is just a convenient way to write `rXyyyy`.
When the UI wants to display information about a revision, it deduces it by examining the commit.
This is primarily an attack on T3551, so we don't need to load <commit -> edge -> revision> (in an ad-hoc way) to get revisions. Instead, when you request a revision we keep track of it and can load it directly later.
Later, this will let us do more things: for example, if you request a branch, we can automatically update the commits (as GitHub does), etc. (Repository branches will need PHIDs first, of course.)
This adds and populates the column but doesn't use it yet. The second part of the migration could safely be run while Phabricator is up, although even for Facebook this table is probably quite small.
Test Plan:
- Ran migration.
- Verified existing requests associated sensibly.
- Created a new commit request.
- Created a new revision request.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3551
Differential Revision: https://secure.phabricator.com/D8822
Summary: Ref T3551. Repository is guaranteed if a product is loaded with modern mechanisms.
Test Plan:
- Edited a request.
- Called `releephwork.getbranchcommitmessage`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3551
Differential Revision: https://secure.phabricator.com/D8821
Summary: Ref T3551. Ref T3549. Mostly unnecessary with modern calls.
Test Plan:
- Called `releeph.queryrequests`.
- Called `releeph.request`.
- Called `releephwork.getbranchcommitmessage`.
- Called `releephwork.getcommitmessage`.
- Called `releephwork.nextrequest`.
- Viewed and edited branches and requests.
- Made a comment on a request.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3549, T3551
Differential Revision: https://secure.phabricator.com/D8820
Summary:
Ref T3551. Releeph does a bunch of old-school on-object data loading; start cleaning that up.
This doesn't change anything, just makes the code more modern/consistent.
Test Plan: Edited a request; called `releephwork.nextrequest`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3551
Differential Revision: https://secure.phabricator.com/D8819
Summary:
Ref T3662. Ref T3549. These methods are pretty conservative for now, but get the structure in place.
Also do a bunch more project -> product stuff.
Test Plan: Made calls to both methods, browsed around the UI a fair amount.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3549, T3662
Differential Revision: https://secure.phabricator.com/D8816
Summary:
This adds FontAwesome and attempts to make use as icons as consistent as possible. May require additional tweaks once we start using, but in practice this is pretty finished.
- Adds FontAwesome
- Adds additional transforms (rotates, spins)
- Adds additional colors
- Better scopes halflings and fontawesome
- Shares CSS between fonts for consistency
Test Plan:
Tested various browsers back to IE8, mobile.
{F146146}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8818
Summary: Ref T3718. Move from unbatched / ad-hoc loading to standard stuff for handles.
Test Plan: Looked at some requests and saw no changes.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3718
Differential Revision: https://secure.phabricator.com/D8810
Summary: This `%d` should be a `%s`, since the `PhutilNumber` value may get formatted according to locale settings.
Test Plan: will make @zeeg
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, zeeg
Differential Revision: https://secure.phabricator.com/D8814
Summary:
Ref T1049. When Harbormaster tests pass, don't bother sending an email about it.
(I tried to implement this earlier but didn't test it entirely properly, and we needed a little more code.)
Test Plan: Used `bin/harbormaster build` to build some junk, got no email about passes.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D8813
Summary: A small but appreciable number of users find flavor on buttons confusing. Remove this flavor. This retains flavor in headers, error messages, etc., which doesn't cause confusion.
Test Plan: Looked at a revision, task, paste, macro, etc.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8812
Summary:
Ref T3718. Ref T3644. Ref T3092. Switches from the Releeph UI elements to standard ones. I'll attach some screenshots.
Also fixes CSRF against the request action endpoint.
Test Plan:
- Viewed request details.
- Took actions on a request from detail page.
- Viewed request list.
- Took actions on a request from list page.
- Used keyboard shortcuts to navigate list.
- Used keyboard shortcuts to take actions.
- Simulated errors.
- Viewed on devices.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: grp, FacebookPOC, mattlqx, tala, beng, LegNeato, epriestley
Maniphest Tasks: T3718, T3092, T3644
Differential Revision: https://secure.phabricator.com/D8771
Summary: This adds in the Glyphicons Halflings Font/Iconset as an option for PHUIIconView along with a standard set of 10 colors. This will be a replacement for the standard action icon set in upcoming diffs, as well as obviously give us more flexibility, less KB, and less design resource time managing images.
Test Plan: UIExamples, Diviner
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8798
Summary:
Fixes T4810. When a buildable completes, make an effort to update the corresponding object with a success or failure message. Commits don't support this yet, but revisions do.
{F144614}
Test Plan:
- Used `bin/harbormaster build` and `bin/harbormaster update` to run a pile of builds.
- Tried good/bad builds.
- Sent some normal mail to make sure the mail reentrancy change didn't break stuff.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4810
Differential Revision: https://secure.phabricator.com/D8803
Summary:
Ref T4810. Ultimate goal is to let Harbormaster post a "build passed/failed" transaction. To prepare for that, implement `PhabricatorApplicationTransactionInterface` in Differential.
To allow Harbormaster to take action on //diffs// but have the transactions apply to //revisions//, I added a new method so that objects can redirect transactions to some other object.
Test Plan:
- Subscribed/unsubscribed/attached/detached from Differential, saw transactions appear properly.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4810
Differential Revision: https://secure.phabricator.com/D8802
Summary: Ref T4809. This saves us a few round trips to find a Buildable, and generally makes the notion of "active" more explicit (i.e., not just the diff with the largest ID). In the future, we may let you revert to previous diffs, which would make the "largest number" rule not always correct.
Test Plan: Ran `differential.query`, got sensible results.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4809
Differential Revision: https://secure.phabricator.com/D8800
Summary:
Ref T4809. Currently, buildables have a status field but nothing populates it. Populate it:
- When builds change state, update the Buildable state.
- Use the new Buildable state on the web UI.
- Return the new Buildable state from Conduit.
To make it easier to debug/test this:
- Provide `bin/harbormaster update Bxxx ...` to force foreground update of a Buildable.
Test Plan:
- Used `bin/harbormaster update Bxxx --force --trace` to update buildables.
- Looked at buidlable list, saw statuses reported properly.
- Used Conduit to read statuses.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4809
Differential Revision: https://secure.phabricator.com/D8799
Summary:
Ref T4809. Buildables currently have buildStatus and buildableStatus. Neither are used, and no one knows why we have two.
I'm going to use buildableStatus shortly, but buildStatus is meaningless; burn it.
Test Plan: `grep`, examined similar get/set calls, created a new buildable, ran storage upgrade.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4809
Differential Revision: https://secure.phabricator.com/D8796
Summary:
Ref T4809. This one is more straightforward. A couple of tweaks:
- Remove the WAITING status, since nothing ever sets it and I suspect nothing ever will with the modern way artifacts work (maybe). At a minimum, it's confusing with the new Target status that's also called "WAITING" but means something different.
- Consolidate 17 copies of these status names into one method.
Test Plan: Ran some queries via Conduit, got reasonable looking results.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4809
Differential Revision: https://secure.phabricator.com/D8795
Summary: Ref T4809. I need to sort out some of the "status" stuff we're doing before this is actually useful (there's no sensible "status" value to expose right now) but once that happens `arc` can query this to figure out whether it needs to warn the user about pending/failed builds.
Test Plan: Ran query with various different parameters.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4809
Differential Revision: https://secure.phabricator.com/D8794
Summary: See IRC. Some users are having difficulty figuring out why Herald is taking some actions. Make it easier to get to the transcript.
Test Plan: {F144622}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: dctrwatson, epriestley
Differential Revision: https://secure.phabricator.com/D8804
Summary:
Couple of minor cleanup things here:
- Pass handles to ApplicationTransactions when rendering their stories; this happened implicitly before but doesn't now.
- Add `?text=1` to do ad-hoc rendering of a story in text mode.
- Make Conduit skip unrenderable stories.
- Fix/modernize some text in the Commit story.
Test Plan: Rendered text versions of stories via Conduit and `?text=1`.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: zeeg, spicyj, epriestley
Differential Revision: https://secure.phabricator.com/D8793
Summary:
For Harbormaster tasks which want to poll or wait, this lets them say "try again a little later" without having to sleep and hold a queue slot.
This is basically the same as failing, except that we don't increment the failure counter. Instead, we just set the current lease to the correct length and then exit. The task will be retried after the lease expires.
Test Plan: Using both `bin/harbormaster` and `phd debug taskmaster`, ran a lot of waiting tasks through the queue, faking them to either yield or not yield in a controlled manner. The queue responded as expected, yielding tasks appropraitely and retrying them later.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8792
Summary:
Without this, build steps that have no options (like "wait for previous commits") don't actually save, since the transaction array is empty.
This also generally nice and consistent.
Test Plan: Created a new "wait" step, viewed transaction log.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8791
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
