Summary: See discussion in T1544. This has been obsoleted by simpler/better mechanisms.
Test Plan: Edited a repository; ran a parse task.
Reviewers: edward
Reviewed By: edward
CC: aran
Maniphest Tasks: T1544
Differential Revision: https://secure.phabricator.com/D3977
Summary: `addDetail()` takes HTML because we have links there fairly often. :/ This design is iffy.
Test Plan: Reloaded `/calendar/status/`, verified no XSS.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T139
Differential Revision: https://secure.phabricator.com/D4074
Summary: I didn't catch this issue at D3986 because we don't have `DifferentialManiphestTasksFieldSpecification` in field selector.
Test Plan:
Added `DifferentialManiphestTasksFieldSpecification` too field selector.
Wrote `Refs T4` and not filled `T4` to Maniphest Tasks field.
Reviewers: epriestley, 20after4
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T945
Differential Revision: https://secure.phabricator.com/D4073
Summary:
I want to add search per owner, this is a prerequisity for it.
There's no link to this page yet, I didn't find a good place for it.
Test Plan: Displayed it, clicked around.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, scottmac
Maniphest Tasks: T2038
Differential Revision: https://secure.phabricator.com/D4067
Summary: make sure we only print the fancy tool tip thing if we have all the data we need. (fixes T2136). additionally, default $branches to array() to prevent errors from reset on null.
Test Plan: made a checkin for a mercurial repo with user "foo@bar" with no branch. verified name showed up in all views. also noted on commit detail view there was no more error about reset() on null for $branches.
Reviewers: epriestley
Reviewed By: epriestley
CC: chad, aran, Korvin
Maniphest Tasks: T2136
Differential Revision: https://secure.phabricator.com/D4065
Summary: The construct `count(x == 0)` should be `count(x) == 0`. This causes a concrete problem because `count(0)` is 1.
Test Plan: eyeballed it~
Reviewers: btrahan, vrana, klimek
Reviewed By: klimek
CC: aran
Maniphest Tasks: T1643
Differential Revision: https://secure.phabricator.com/D4060
Summary:
Users are used to this feature from comments.
Provide it also in title, summary and test plan.
It adds the users to CC only on creating the revision to avoid cases like:
"I mentioned this user but now I want to remove him from CC" or he unsubscribes.
Test Plan: Wrote `@epriestley` to summary.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4050
Summary:
Some users like monospaced textareas and others don't.
This introduces an option to set this as a user preference.
Test Plan: Enabled and saw monospaced textareas, disabled and saw non-monospaced textareas.
Reviewers: epriestley
CC: aran, Korvin
Maniphest Tasks: T2114
Differential Revision: https://secure.phabricator.com/D4037
Summary: This is missing a lot of features, but technically allows working copy allocation.
Test Plan: Ran `drydock lease --type working-copy --attributes repositoryID=12`, got a working copy of Phabricator allocated on disk.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D3999
Summary: This does nothing fancy, just closes the resource and releases/breaks leases. They'll get cleaned up in some to-be-written GC process.
Test Plan: Closed resources from web UI and CLI.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D3998
Summary:
Builds out most of the non-hover-stuff from `overview-hovercards.png`. Things I didn't build:
- Tokens (I like them a lot but don't want to scope creep)
- Functions (backend mess / future work)
- Icons for tags.
- Tags with pointy ends and holes in them (an earlier mock had this I think but they're gone on final)
- The cyaney color for "Sporadic" since I just noticed it while typing this up.
Test Plan: Looked at UIExample page.
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2089
Differential Revision: https://secure.phabricator.com/D4029
Summary:
- The filesystem is now the authority for which sprites are available. If you add new icons, the generation process will pick them up.
- I broke out icon generation and added retina support. App icon generation still uses the old method.
- Update ActionList and RemarkupControl to use the new sheet.
- Use white icons on hover.
- Also fixed a couple of minor issues with some stuff in Firefox/Chrome.
Test Plan:
{F25750}
{F25751}
{F25752}
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2013
Differential Revision: https://secure.phabricator.com/D4027
Summary:
- Show subscribers.
- When a user is mentioned in the description or a comment, subscribe them explicitly.
Test Plan: {F22370}
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2097
Differential Revision: https://secure.phabricator.com/D3838
Summary:
- Use transactions to apply edits.
- Use Editor to apply transactions.
- Some special casing for tricky stuff I don't want to deal with yet (mock images).
Test Plan: {F22368}
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2097
Differential Revision: https://secure.phabricator.com/D3837
Summary: Basic support for adding comments. Missing a lot of frills. Uses new comment/transaction UI.
Test Plan:
Added some comments. Tried to add an empty comment.
Some comments:
{F22361}
No text provided:
{F22362}
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2097
Differential Revision: https://secure.phabricator.com/D3834
Summary:
This is still rough and not completely accurate to the mocks (and the mobile view is quite crude and mostly just "hey this technically works"), but I want to build Pholio on top of it rather than building it on something else and then swapping it out later and the API is reasonable enough.
This should probably be called `PhabricatorTransactionView` but we already have one of those. I might juggle the names in a future diff.
Test Plan:
Desktop
{F22352}
Mobile
{F22351}
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2097
Differential Revision: https://secure.phabricator.com/D3833
Summary:
Just laying more groundwork out of ready-made UI.
{F22349}
Test Plan: Looked at a mock.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2097
Differential Revision: https://secure.phabricator.com/D3832
Summary:
Nothing app-specific yet, just stitching groundwork together from readymade components.
{F22347}
Test Plan: Created some mocks via web UI.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2097
Differential Revision: https://secure.phabricator.com/D3830
Summary:
I'm not going to land this until it's a bit more fleshed out since it would just confuse users, but this is probably more reviewable as a few diffs adding a couple features than one ULTRA-diff adding everything. Implement application basics for Pholio. This does more or less nothing, but adds storage, subscribe, flag, markup, indexing, query basics, PHIDs, handle loads, a couple of realy really basic controllers, etc.
Basic hierarchy is:
- **Moleskine**: Top-level object like a Differential Revision, like "Ponder Feed Ideas".
- **Image**: Each Moleskine has one or more images, like the unexpanded / expanded / mobile / empty states of feed.
- **Transaction**: Comment or edit, like Maniphest. I generally want to move most apps to a transaction model so we can log edits.
- **PixelComment**: Equivalent of an inline comment.
Test Plan: Created a fake object and viewed it.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran, davidreuss
Maniphest Tasks: T2097
Differential Revision: https://secure.phabricator.com/D3817
Summary: D3533 changed the path for files at root from, e.g., "README" to "/README", which fatals when we try to `git cat-file` it.
Test Plan:
This no longer happens:
{F24874}
Viewed a directory in browse without a trailing slash in the URL.
Reviewers: vrana, nh
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4016
Summary:
See T2102 and inline for discussion. This seems like the least-bad approach until we have something better.
The utility of next_uri seems much greater than the minor exposure of routable URIs.
Note that attackers can //not// detect if routable URIs are //valid// (e.g., "/D999" will always hit the login page whether it exists or not), just that they're routable. So you can only really tell if apps are installed or not.
Test Plan: Hit `/alsdknlkasnbla` while logged out, got 404 instead of login.
Reviewers: vrana, codeblock, btrahan
Reviewed By: codeblock
CC: aran
Maniphest Tasks: T2102
Differential Revision: https://secure.phabricator.com/D4012
Summary:
If you try to load a directory in diffusion in a git repo that has a readme
file and you don't include a trailing slash in the path, you get an error
because we don't assemble the full paths of files correctly. This fixes that.
Test Plan: load such a directory in diffusion and see no fatal
Reviewers: epriestley, vrana
Reviewed By: vrana
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3533
Summary: Also disable this feature without 'maniphest.enabled'.
Test Plan:
Wrote "fixes T..." in `arc diff`, verified that the task is attached.
Add another "fixes T..." in edit revision on web, verified that it was added.
Deleted "fixes T..." in edit revision, verified that the attached task wasn't deleted.
Reviewers: 20after4, epriestley
Reviewed By: 20after4
CC: aran, Korvin
Maniphest Tasks: T945
Differential Revision: https://secure.phabricator.com/D3986
Summary: It is used by 'Pending Differential Revisions'.
Test Plan: Created a new file, `arc diff`, looked at this path in Diffusion, saw Pending Differential Revisions.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3991
Summary:
- For Drydock, I want to add section headers to separate user-defined attributes from global attributes.
- Some day for Differential, I want to add "Summary" and "Test Plan" section headers.
- Clean up some stuff a bit; drop the multiple APIs for setting text content. Explicitly disallow appendChild().
- Build out the UIExample a bit.
Test Plan:
{F24821}
{F24822}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D4000
Summary:
we're tracking down a fatal caused by the mandatory
attachments array parameter change in the reply handler and
yo dawg, heard you like to fatal in your fatal.
Test Plan:
internal test scenario with xmail -> phabricator
triggered manually.
Reviewers: nh, vrana, epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D4005
Summary: we need to make sure we should publish to the auditor in a given audit request. write some custom logic for this as it is subtly different than other things like CC.
Test Plan: repro from T2087 produced expected results. further, former auditors who resigned did not get feed stories published.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2087
Differential Revision: https://secure.phabricator.com/D3987
Summary: I am an idiot. :( D3962 added the full set of properties and I derp'ed this part of the diff up
Test Plan: ask lesliepc16 to take a look
Reviewers: epriestley, vrana
Reviewed By: vrana
CC: lesliepc16, aran, Korvin
Maniphest Tasks: T2091
Differential Revision: https://secure.phabricator.com/D3984
Test Plan: Set `lintCommit` to 'x' and browsed a file.
Reviewers: nh, epriestley
Reviewed By: nh
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3976
Summary:
When searching for a user before logging in use the DN from the retrived user.
This allows you to use a less fine grained DN when searching for a user. For example dc=domain,dc=domain instead of ou=unit,dc=domain,dc=com.
Test Plan: Tested on local install with ldap.search-first disabled and enabled.
Reviewers: epriestley, yunake
Reviewed By: epriestley
CC: auduny, briancline, aran, Korvin, vsuba
Differential Revision: https://secure.phabricator.com/D3549
Summary: Also remove some columns.
Test Plan: Looked at SVN dir in Diffusion.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, vsuba
Maniphest Tasks: T2038
Differential Revision: https://secure.phabricator.com/D3949
Summary:
see title. Note that fields that use customs storage don't work because I didn't think it made sense to load a revision object to get that data. Further, we don't have a revision id at some points, so its not clear what does / does not work...?
Also added a link to upsell this diff view as I had trouble finding it.
Test Plan: viewed some diffs
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: chad, aran, Korvin
Maniphest Tasks: T2026
Differential Revision: https://secure.phabricator.com/D3962
Summary:
Allows to use file storage in different Amazon S3 regions as well as it
should support different file storage services with S3 compliant API
(eg.: Bashos' Riak CS).
Test Plan:
1. Create S3 bucket in non-default AWS Region (e.g. EU/Ireland).
2. Set appropriate bucket policy to allow upload & download objects for the specified access credentials.
3. Set `amazon-s3.endpoint` in your configuration file to an appropriate value (e.g. `s3-eu-west-1.amazonaws.com` for EU region).
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3965
Summary:
If a user is asking for a list of documents stored in Phriction, it's pretty
safe to assume that they mean documents that actually exist.
Test Plan:
Made a document, saw it listed in /phriction/list/all. Deleted it, and no longer
saw it.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3951
Summary:
This doesn't actually contain any logic to prevent a user from deleting a
document twice, but it makes it significantly harder to do so.
Test Plan:
Went to edit a document, saw the delete button. Clicked it, then went back to
the edit view, and no longer saw the delete button.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3952
Summary:
I want to attach the task also to revision, not only to commit by mentioning it in summary.
This is a preparation for it, useful by itself:
* One query instead of N.
* Lower CRAP score, yay!
Refs T945.
Test Plan:
My Test Plan is really //plan// this time.
I want to update Phabricator in the minute when I commit this.
Reviewers: 20after4, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T945
Differential Revision: https://secure.phabricator.com/D3873
Summary: wishlist has priority value of 0 which was messing things up. also fix search text so we can search for "0".
Test Plan: searched for stuff, got results
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1878
Differential Revision: https://secure.phabricator.com/D3948
Summary: 'cuz who cares unless you need review?
Test Plan: noted the UI showed up appropriately to my new business logix
Reviewers: epriestley, chad
Reviewed By: chad
CC: aran, Korvin
Maniphest Tasks: T2010
Differential Revision: https://secure.phabricator.com/D3958
Summary: make it so a given user can click to edit status from calendar view. also fix bug so when editing status existing "type" is selected
Test Plan: edited status from calendar view and observed "sporadic" status sticky
Reviewers: epriestley, chad
Reviewed By: chad
CC: aran, Korvin
Maniphest Tasks: T2060, T2061
Differential Revision: https://secure.phabricator.com/D3957
Summary: gives user a dialogue and they can fill out what comes after /w/
Test Plan: made some new wiki docs. played with garbage data and edit scenarios as well as vanilla create -- good ish
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1360
Differential Revision: https://secure.phabricator.com/D3946
Summary: 'cuz we need it in arcanist for T479 to commit as author
Test Plan: verified the return value was correct in conduit
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T479
Differential Revision: https://secure.phabricator.com/D3917
Summary:
This adds a green/blue/grey gradient and new button CSS.
Updated.
Test Plan: Clicked through various pages in my install, but would like more feedback.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
CC: vrana, aran, Korvin
Differential Revision: https://secure.phabricator.com/D3919
Test Plan:
/diffusion/ARC/lint/master/src/, clicked on count link.
/diffusion/ARC/browse/master/src/difference/?lint=XHP9, clicked on file name.
/diffusion/ARC/browse/master/src/difference/ArcanistDiffUtils.php?lint=, verified that all messages are displayed.
/diffusion/ARC/browse/master/src/difference/ArcanistDiffUtils.php?lint=XHP9.
/diffusion/ARC/browse/master/src/difference/ArcanistDiffUtils.php?lint=TXT3, verified that 0 messages are displayed.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3929
Summary: This saves lint errors to the path change of current commit. It requires pushed revision. It doesn't save difference from previous commit mentioned in T2038#comment-4 - I don't plan doing it after all, everything would be much more complicated and the amount of data saved with this approach isn't that bad.
Test Plan: Applied patch, ran script, verified DB.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2038
Differential Revision: https://secure.phabricator.com/D3899
Summary: array_filter to the rescue
Test Plan: mostly lots of reasoning (as opposed to making a fake user with a blank email address to reproduce)
Reviewers: epriestley, chad
Reviewed By: chad
CC: aran, Korvin
Maniphest Tasks: T2052
Differential Revision: https://secure.phabricator.com/D3927
Summary:
See https://github.com/facebook/phabricator/issues/230.
If you searched for a project with the "Any Projects" field, we didn't explicitly include it in the list of handles to fetch. Usually this works fine because something else fetches the handle, but if you, e.g., search for a project that has no tasks, you get a fatal.
Test Plan:
Reproduced fatal described in report by performing a custom query for "Any Projects" using a project with no tasks; applied patch; query worked correctly.
Verified `$xproject_phids` and `$project_phids` are already queried.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3923
Summary: don't need it now that uploading files is so easy. Plus it made for some buggy jonx if / when there were bad image links coupled with caching. In theory this is a lot less pretty though if folks linked to a bunch of files served elsewhere using images.
Test Plan: http://does-not-exist.com/imaginary.jpg rendered as a link!
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2000
Differential Revision: https://secure.phabricator.com/D3908
Summary: ...and do so for a few fields -- summary, test plan, and revert plan.
Test Plan: added NATASHA and BULLWINKLE to summary and test plan of existing diff. Diff showed up in search!
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T654
Differential Revision: https://secure.phabricator.com/D3915
Summary: was poking at T654 and noticed subscribers weren't exposed in search UI so I did so. Also make ponder a little less silly on the double handles load. Finally, stopped showing the "Examine Index" link to non admins since they can't click it. Note this introduces a UI oddity in that you Users and Phriction Documents don't currently have the subscribe functionality.
Test Plan: searched for subscribers in all applications - it worked!
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3907
Summary:
- Remove EC2, RemoteHost, Application, etc., blueprints for now. They're very proof-of-concept and Blueprints are getting API changes I don't want to bother propagating for now. Leave the abstract base class and the LocalHost blueprint. I'll restore the more complicated ones once better foundations are in place.
- Remove the Allocate controller from the web UI. The original vision here was that you'd manually allocate resources in some cases, but it no longer makes sense to do so as all allocations come from leases now. This simplifies allocations and makes the rule for when we can clean up resources clear-cut (if a resource has no more active leases, it can be cleaned up). Instead, we'll build resources like the localhost and remote hosts lazily, when leases come in for them.
- Add some configuration to manage the localhost blueprint.
- Refactor `canAllocateResources()` into `isEnabled()` (for config checks) and `canAllocateMoreResources()` (for quota checks, e.g. too many resources are allocated already).
- Juggle some signatures to align better with a world where blueprints generally do allocate.
- Add some more logging and error handling.
- Fix an issue with log ordering.
Test Plan: Allocated some localhost leases.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D3902
Summary: do this by making sure to filter out those who've "resigned" from the email CC list
Test Plan: resigned from an audit and no longer got emails on updates
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2033
Differential Revision: https://secure.phabricator.com/D3890
Summary:
This commit doesn't change license of any file. It just makes the license implicit (inherited from LICENSE file in the root directory).
We are removing the headers for these reasons:
- It wastes space in editors, less code is visible in editor upon opening a file.
- It brings noise to diff of the first change of any file every year.
- It confuses Git file copy detection when creating small files.
- We don't have an explicit license header in other files (JS, CSS, images, documentation).
- Using license header in every file is not obligatory: http://www.apache.org/dev/apply-license.html#new.
This change is approved by Alma Chao (Lead Open Source and IP Counsel at Facebook).
Test Plan: Verified that the license survived only in LICENSE file and that it didn't modify externals.
Reviewers: epriestley, davidrecordon
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2035
Differential Revision: https://secure.phabricator.com/D3886
Summary: 'TASK DETAIL' links point to the non-production uri. Our daemons run in an environment that uses different baseUrl because we can't use https locally (https is provided by our load balancers)
Test Plan: check emails generated with a non-production environment. See that the TASK DETAIL link points to production url.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3877
Summary: so folks can write applications and whatnot.
Test Plan: set feed.http-hooks to local dev instance (200) and localhost (500) in my conf. Verified succcess and retrying respectively.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T305
Differential Revision: https://secure.phabricator.com/D3874
Summary:
Tightens up a bunch of stuff:
- In `drydock lease`, pull and print logs so the user can see what's happening.
- Remove `DrydockAllocator`, which was a dumb class that did nothing. Move the tiny amount of logic it held directly to `DrydockLease`.
- Move `resourceType` from worker task metadata directly to `DrydockLease`. Other things (like the web UI) can be more informative with this information available.
- Pass leases to `allocateResource()`. We always allocate in response to a lease activation request, and the lease often has vital information. This also allows us to associate logs with leases correctly.
Test Plan: Ran `drydock lease --type host` and saw it perform a host allocation in EC2.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D3870
Prior to D3859, getRequiredLeaseTime() was called before doWork(), which had the critical but subtle side effect of populating `$this->message`. Instead, make the population explicit. This restores email functionality.
Test Plan: ran `phd debug taskmaster` and verified email was delivered
Auditors: btrahan
Summary: Add a bin/drydock symlink and break it into workflows. Nothing too special here.
Test Plan: Ran `bin/drydock wait-for-lease`, `bin/drydock lease`, `bin/drydock help`, etc.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D3867
Summary:
a few things
- make the parent mailhandler class not send "blank body" error if you have attachments
- make both differential and maniphest append a list of attachments to the body if any exist
- BONUS - made the cc stuff work in Maniphest
Test Plan: I haven't actually tested this yet. :( i need to figure out how to send a mail with an attachment from the command-line and figured I'd serve this up first.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2012
Differential Revision: https://secure.phabricator.com/D3868
Summary:
all sorts of stuff
- made comment form width flexible
- made margins element specific rather than part of differential-primary-pane
- made box elements all veritically align left and right until code stuff
- re-factored width calculaton stuff a bunch so only the code section has to suffer from max-width calculations; everything else can flex
- made colspan 3 for rightmost table header element. this is so the "View Options" UI element ends up lining up correctly with the "Show All Lines" element just below
Test Plan: looked at revision view and changeset view and it all looked hot. note I did not test what things looked like with different word wrap values; that should still work given the re-factoring and not re-design here. also toggled haunted panel mode and it looked good.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2006
Differential Revision: https://secure.phabricator.com/D3866
Summary:
This was the major goal of D3859/D3855, and to a lesser degree D3854/D3852.
As Drydock is allocating a resource, it may need to allocate other resources first. For example, if it's allocating a working copy, it may need to allocate a host first.
Currently, we have the process basically queue up the allocation (insert a task into the queue) and sleep() until it finishes. This is problematic for a bunch of reasons, but the major one is that if allocation takes more resources (host, port, machine, DNS) than you have daemons, they could all end up sleeping and waiting for some other daemon to do their work. This is really stupid. Even if you only take up some of them, you're spending slots sleeping when you could be doing useful work.
To partially get around this and make the CLI experience less dumb, there's this goofy `synchronous` flag that gets passed around everywhere and pushes the workflow through a pile of special cases. Basically the `synchronous` flag causes us to do everything in-process. But this is dumb too because we'd rather do things in parallel if we can, and we have to have a lot of special case code to make it work at all.
Get rid of all of this. Instead of sleep()ing, try to work on the tasks that need to be worked on. If another daemon grabbed them already that's fine, but in the worst case we just gracefully degrade and do everything in process. So we get the best of both worlds: if we have parallelizable tasks and free daemons, things will execute in parallel. If we have nonparallelizable tasks or no free daemons, things will execute in process.
Test Plan: Ran `drydock_control.php --trace` and saw it perform cascading allocations without sleeping or special casing.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D3861
Summary:
- Clean up a TODO about permanent failures.
- Clean up a TODO about failing tasks after too many retries.
- Clean up a TODO about testing for bad leases.
- Make the lease/retry implementation more flexible and natural.
- Make completely bogus tasks fail permanently.
- Make PhabricatorMetaMTAWorker use new `getWaitBeforeRetry()` (as intended), not hackily implement logic in `getRequiredLeaseTime()`.
- Document worker hooks for failures and retries.
- Provide coverage on everything.
Test Plan: Ran unit tests. Ran `bin/phd debug taskmaster`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D3859
Summary: this makes notifications work better for folks who choose to handle things in Phabricator and not over email
Test Plan: had my test account and "real" account battle each other on a few tasks and divs. Noted that I received emails appropos to the respective settings.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1977
Differential Revision: https://secure.phabricator.com/D3856
Summary: Make mobile-friendly and provide UI to cancel/retry tasks. Remove display of task data to arbitrary users, as it may be sensitive.
Test Plan:
{F22502}
{F22503}
{F22504}
{F22505}
{F22506}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D3854
Summary:
Currently, when taskmasters complete a task it is immediately deleted. This prevents us from doing some general things, like:
- Supporting the idea of permanent failure (e.g., after N failures just stop trying).
- Showing the user how fast taskmasters are completing tasks.
- Showing the user how long tasks took to complete.
Having better visibility into this is important to Drydock, which builds on the task system. Also, generally buff debug output for task execution.
Test Plan: Ran `bin/phd debug taskmaster`. Ran `bin/phd debug garbage`. Queued some tasks via various systems.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D3852
Summary: we do this by passing the "seenOnBranches" commit data detail through the stack
Test Plan: browse in diffusion link worked for non-master checkins under git
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1949
Differential Revision: https://secure.phabricator.com/D3853
Summary: D3499 introduced some new hotness but broke these less common cases. add slightly updated ui for this. Note we need a 'download' icon for this UI to not be horrendous.
Test Plan: viewed a binary file and an image in diffusion browse view
Reviewers: epriestley
Reviewed By: epriestley
CC: nh, aran, Korvin
Maniphest Tasks: T1997
Differential Revision: https://secure.phabricator.com/D3849
Summary: Add an Application class for Drydock and move routing rules there.
Test Plan: Looked at /applications/, clicked around drydock.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3847
Summary: This doesn't do anything useful yet but Pholio needs to access Files and I wanted to get the groundwork in place for eventual policy-awareness.
Test Plan: Will use in Pholio.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3829
Summary: Sending these as the user doesn't make a ton of sense, and LLVM reports some issues with these emails getting caught in spam filters. Users expect these emails, so just send them from "noreply@example.com" or whatever is configured.
Test Plan: Sent myself a verification email, verified it came from a noreply@ address.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: klimek, aran
Maniphest Tasks: T1994
Differential Revision: https://secure.phabricator.com/D3843
Test Plan: Looked at diff with several different lint errors, saw correct messages in their inline comments.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3827
Summary: This check is currently wrong -- the actor is only //coincidentally// the owner (and only most of the time). It also raises at parse time, preventing any user from parsing a message with their own name in the "Reviewers" field. Instead, check against the right owner PHID and raise it only if a revision is available. See https://github.com/facebook/arcanist/issues/54 and next diff.
Test Plan: Tried to add myself as a reviewer to revisions I own via web and Conduit, got rejected. Parsed a message with myself in the "Reviewers:" field, it worked correctly.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3820
Summary: fancy title. really just make the delete() method aware of related objects and build a quick workflow which calls delete(). also make commit delete savvy about audit requests.
Test Plan: deleted a repository per the instructions given to me in the web UI
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1416, T1958, T1372
Differential Revision: https://secure.phabricator.com/D3822
Summary: Quora requested this (moving to S3) but it's also clearly a good idea.
Test Plan:
Ran with various valid/invalid options to test options. Error/sanity checking seemed OK.
Migrated individual local files.
Migrated all my local files back and forth between engines several times.
Uploaded some new files.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1950
Differential Revision: https://secure.phabricator.com/D3808
Summary: you can now add, edit, and delete status events. also added a "description" to status events and surface it in the big calendar view on mouse hover. some refactoring changes as well to make validation logic centralized within the storage class.
Test Plan: added, edited, deleted. yay.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T407
Differential Revision: https://secure.phabricator.com/D3810
Summary: Followup to D3804. Makes Diffusion main comments (not just inlines) render properly with the modern markup pipeline.
Test Plan: Created previews and inline previews. Edited inlines. Saved comment, viewed comment. Verified caches were read and written using "Services" tab.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3805
Summary:
Minor tweaks to lightboxes.
- With "position: fixed;", we don't need to do any of the scroll/resize stuff. Just remove it.
- Make the lightbox go over the menu bar -- was it intentional that it wasn't?
- Make 'jx-mask' use "position: fixed;" too.
- Add a loading indicator.
- In Differential/Maniphest/etc, a preview may bring in an image but won't bring in the CSS we need. The "real" fix is to ship CSS/JS with ajax, but that's really hard -- fake it by pulling in the right CSS any time we render a remarkup area.
I'm going to do a couple of other tweaks here but need to update JX.Mask.
Test Plan: Verified behavior is reasonable in Safari, Firefox, Chrome with multiple images / scroll / previews / resize.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1896
Differential Revision: https://secure.phabricator.com/D3795
Summary:
See T1963 for discussion of the Facebook-specific hack.
Differential currently uses a one-stage cache (render -> postprocess -> save in cache) rather than the two-stage cache (render -> save in cache -> postprocess) offered by `PhabricatorMarkupInteface`. This breaks Differential comments coming out of cache for the lightbox, and makes various other things suboptimal (status of handles like @mentions and embeds are not displayed accurately).
Instead, use the modern stuff.
Test Plan:
- Created preview comments and inlines in Differential.
- Edited a Differential inline.
- Submitted main and inline Differential comments.
- Viewed and edited Differential summary and test plan.
- Created preview comments and inlines in Diffusion.
- Submitted comments and inlines in Diffusion.
- Verified Differential now loads and saves to the generalized markup cache (Diffusion is close, but main comments still hold a single-stage cache).
- Verified old Differential comments work correctly with the lightbox.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1963
Differential Revision: https://secure.phabricator.com/D3804
Summary: See D3789. Same thing for Differential.
Test Plan: Created a new revision and made a comment. Verified reviewer got popup notifications but the in-app notifications were delivered already marked as read.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1403
Differential Revision: https://secure.phabricator.com/D3790
Summary:
See D3784, T1403. When we send a user an email and a notification from Maniphest, mark the notification as read.
(It would be nice to do the thing with `multiplexMail()` a little less hackily, but it gets very complicated to do correctly because we require handles but sometimes do not have an actor/user so I'm punting for now.)
Test Plan: Acted on a task, verified notification was marked read because I received an email.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1403
Differential Revision: https://secure.phabricator.com/D3789
Summary:
- When a feed story's primary object is a Policy object, use its visibility policy to control story visibility. Leave an exception for
- Augment PhabricatorPolicyAwareQuery so queries may do pre-policy filtering without the need to handle their own buffering/cursor code. (We could slightly improve this: if a query returns less than a page of pre-filtered results we could keep getting pre-filtered results until we had at least a page's worth and then filter them all at once.)
- Load and attach "required objects" to feed stories. We need this for policies anyway, and it will let us simplify story implementations by sourcing data directly from the object when we don't have some need to denormalize it (e.g., "title was changed from X to Y" needs to save the values of X and Y from when we published the story, but "user asked question X" can reflect the current version of the question).
Test Plan: Loaded main feed, project feed, notification menu / dropdown, notificaiton list, paginated things.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3783
Summary: See discussion in T1403. Possibly we'll add a preference for this or something?
Test Plan: Not yet in use. See future diff.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1403
Differential Revision: https://secure.phabricator.com/D3784
Summary:
Provides a simple way for policy-aware queries to pre-filter results without needing to maintain separate cursors, and fixes a bunch of filter-related edge cases.
- For reverse-paged cursor queries, we previously reversed each individual set of results. If the final result set is built out of multiple pages, it's in the wrong order overall, with each page in the correct order in sequence. Instead, reverse everything at the end. This also simplifies construction of queries.
- `AphrontCursorPagerView` would always render a "<< First" link when paging backward, even if we were on the first page of results.
- Add a filtering hook to let queries perform in-application pre-policy filtering as simply as possible (i.e., without maintaing their own cursors over the result sets).
Test Plan: Made feed randomly prefilter half the results, and paged forward and backward. Observed correct result ordering, pagination, and next/previous links.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3787
Summary:
Make the example page a little more useful by showing available icons.
Also replace the "new" image, it had a little arrow which I thought was a "+". Use the one with a "+".
Test Plan: {F21966}
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3794
Summary:
See discussion in T404. Basically, the problem with date-only controls is that they may behave unpredictably in the presence of timezones. When you say "This needs to be done by Oct 23", you probably mean "Oct 23 5PM PST" or something like that, but someone in China may see the "Oct 24" and hit the deadline in good faith but be 10 hours too late. T404 has more discussion and examples. There are ways to fake this, but they get more complicated if the guy in China needs to move the date forward 24 hours.
I think the best solution to this is to not have date-only controls, and always display the time. This makes it absolutley unambiguous what something means, because the guy in the US will set "Oct 23 5PM" and the guy in China will see that accurately in local time.
The downside is that it's slightly more visual clutter and work for the user to specify things precisely, but I added some hints (start/end of day, start/end of business) that will hopefully let us pick the right default in most cases.
Test Plan:
Set some dates.
{F21956}
This has a couple of edge case issues on resize and some not-so-edge-case issues on mobile, but should be good to build T407 on without API changes.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T404, T407
Differential Revision: https://secure.phabricator.com/D3793
Summary: I broke this in D3778. We modify `$parameters` and then ignore it in favor of `$params` for the rest of the method. Unit tests work great since they're one level below this.
Test Plan: Verified "Send email about my own actions" behaved correctly.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3796
Summary:
images attached to maniphest tasks and mentioned in remarkup anywhere now invoke a lightbox control that lets the user page through all the images.
lightbox includes a download button, next / prev buttons, and if we're not at the tippy toppy of hte page an "X" or close button.
we also respond to left, right, and esc for navigating.
next time we should get non-images working in here...!
Test Plan:
played with maniphest - looks good
made comments with images. looks good.
made sure multiple image comments worked.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, chad
Maniphest Tasks: T1896
Differential Revision: https://secure.phabricator.com/D3705
Summary: See D3789. Moving away from constants means less safety; provide a runtime check at least.
Test Plan: Took some actions which caused feed stories to publish, verified they showed up.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3792
Summary: Provide a public interface to get all the filtered recipients of an email. The intent is to pass this along to Notifications so it can mark notifications as read if the user is also receiving an email, possibly based on some preference (see T1403). This also simplifies the enormous sendNow() method a little bit.
Test Plan: Added unit tests, and sent a few mails that should cover most/all of these cases. They appeared to produce the correct recipients.
Reviewers: btrahan, vrana, nh
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1403
Differential Revision: https://secure.phabricator.com/D3778
Summary: The property is called 'actor', not 'user'. Extend from Phobject to catch this class of error automatically. Upgrade a couple of getActor() to requireActor().
Test Plan: Created new users.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3776
Summary: This should all go away at some point when we move to fluid layout, but don't be more annoying than necessary in the meantime.
Test Plan: Meta.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3788
Summary: This got copy/pasted at some point long in the past, it should clearly read "Task".
Test Plan: Looked at the rest of the strings.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3774
Summary: This information may be quite useful.
Test Plan: Uploaded file.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3763
Summary:
Same as D3759 with a fix:
Previously, we would insert `null` to indicate a line that doesn't exist on one side of a diff, and then implode on "\n", so we'd get "\n" as a result.
In D3759, we'd insert `null` but implode on empty string, and get nothing as a result.
When a placeholder null is present, explicitly insert a newline.
Test Plan: D3759 plus examined a diff with removed lines on the left side prior to new lines on the right side.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3765
This reverts commit f6cb51562e.
This has some bugs in normal diffs that I haven't immediately been able to figure out. I'll reapply it once I sort them out.
Auditors: vrana
Summary:
- We currently treat "\r" as a newline, but should not because VCSes do not.
- We get an extra empty line at the end of diffs created after D3442 because we now retain newlines.
- Historically we've converted tab pre-cache, but do it post-cache instead so we can add prefs about it, as we should handle it better than we do (e.g., let the user set it to a different width, infer width from comments in the file, expand it to actual tab stops, or show it visually in some way).
Test Plan:
- Verified diffs no longer have an empty line at the end.
- Created a diff of a "\r" file and verified it displayed somewhat reasonably. All browsers treat "\r" as a real newline so it's not necessarily perfect, but we can clean that up later. Hopefully these files are exceedingly rare.
- Created a file with tabs and verified it came out reasonably.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1857
Differential Revision: https://secure.phabricator.com/D3759
Summary: If you `git show` and copy/paste it into Differential, we die trying to save the DifferentialChangeset corresponding to the commit message (error: column "filename" can not be null). Instead, drop the message change for raw diffs.
Test Plan: Copy/pasted `git show` into Differential.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3740
Summary:
When you delete the content of a document in Phriction, we treat it as an attempt to delete the document.
In the case you're creating the document, we hit an exception trying to delete a document which doesn't exist yet.
Detect this case and raise a better error.
Test Plan: Tried to create an empty document, got a good error. Created a nonempty document. Edited a document to empty to delete it.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1920
Differential Revision: https://secure.phabricator.com/D3728
Summary: Eventually we'll have a real "uninstall" sort of thing, but until we do we should keep respecting this flag.
Test Plan: Disabled the flag, saw Phriction vanish from the application list.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3724
Summary:
Allow skins to serve arbitrary resources without needing to be mapped, so we can have a vibrant community of amateur skinners.
For "basic" skins, just put all the "css/" on the page always.
Includes an image to prove that works.
@vrana, pretty sure this has no impact outside of Phame but it does change Celerity so it might be to blame if there's any weirdness with static resources.
Test Plan:
{F21341}
{F21340}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3719
Summary:
Lower the barrier to entry for installing and creating skins, so we can kill Wordpress. You can now install skins by dropping them into a directory, and build either "advanced" (full phutil library) skins or "basic" (simple PHP templates) skins.
Next up is getting static resources working in an easy way for skins.
I put these in `externals/` for now so they don't get hit by lint.
Test Plan: Viewed the Pokeblog with the Oblivious skin.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3717
Summary: Restore summarization. Use the remarkup cache, and try to do it somewhat-intelligently (pick the first paragraph that looks like it's text).
Test Plan:
{F21323}
{F21324}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3715
Summary:
Two high-level things happening here:
- We no longer ever need to put meta-UI (content creation, editing, notices, etc.) on live blog views, since this is all in Phame now. I pulled this out.
- On the other hand, I pushed more routing/control logic into Skins and made the root skin a Controller instead of a View. This simplifies some of the code above skins, and the theory behind this is that it gives us greater flexibility to, e.g., put a glue layer between Phame and Wordpress templates or whatever else, and allows skins to handle routing and thus add pages like "About" or "Bio".
- I added a basic skin below the root skin which is more like the old root skin and has standard rendering hooks.
- "Ten Eleven" is a play on the popular (default?) Wordpress themes called "Twenty Ten", "Twenty Eleven" and "Twenty Twelve".
Test Plan: Viewed live blog and live posts. They aren't pretty, but they don't have extraneous resources.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3714
Summary:
For Phame, we really want more than a chromeless page -- it shouldn't have Javelin or Phabricator styles on it.
Pull the bare infrastructure out of StandardPageView and make it available as BarePageView.
Test Plan: Viewed bare page in UIExamples, various non-bare pages.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3711
Summary: These haven't had any effect since the redesign, and we're moving to a more granular policy model so it probably doesn't make sense to ever restore them.
Test Plan: Grepped for "admin" and removed all relevant code.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3710
Summary:
- Better icons and action order.
- "Move Post" action.
- (Bugfix) Allow multiple blogs to be set to not having custom domains.
- Make "Write Post" skip the "select a blog" step when coming from a blog view.
- Sort blog list on "Write Post".
- Show messages when a post is a draft or not on a blog.
Test Plan: Created posts, blogs, moved posts, preview/live'd posts, etc.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3708
Summary:
- Clean up the menu selection states.
- Nuke some unused code.
- Show some more contextual error messages.
- Improve/pht() some strings.
Test Plan: Looked at post/blog list, empty state of "new post".
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3706
Summary: Primarily, this makes flagging work on them.
Test Plan: Flagged / Unflagged a post.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3702
Summary: Currently the exception escapes to top level. Instead, intercept it and complain.
Test Plan: Tried to set two blogs to the same domain.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3701
Summary: Currently the new detail pages don't show this information. Show it, and use the remarkup cache for BLAZING OODLES OF PERFOARMSNECES!!~~~
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3700
Summary:
Some objects (like PhamePost and ManiphestTask) have a block of text/remarkup which serves as a description or core piece of content for the object.
Accommodate this in PhabricatorPropertyListView.
(This is primarily to let me do a reasonable first pass on this in Phame.)
Test Plan: Made example, will attach screenshot.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3699
Summary:
Still some big chunks left but this moves us a bit closer to getting everything device-ready.
Stuff not addressed here but which I'm planning to do soon:
- Posts don't have a live URI yet.
- Post detail pages don't actually show the post content. I'm going to tweak PhabricatorObjectPropertyListView for this since we need it some other places.
- Some of the hinting about use/states is gone (e.g., "This post is a draft, publish it to make it live to the world."); I'm planning to restore it.
- Left nav is still a bit of a mess with states/highlighting.
Major changes are:
- If you click "New Post" you get a screen asking you to pick a blog to post to.
- "Publish/Preview" and Unpublish are now separate actions from the post detail screen.
- "Publish/Preview" renders a preview of the post in an iframe and gives you a "Publish" button.
Test Plan: Will attach screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3697
Summary:
Use device UI for all post lists.
Left menu is a bit wonky but I'll clean that up shortly.
Test Plan: Will add screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3696
Summary:
Cleans up some of the mess I made in D3694. Basically:
- All blogs have an "internal" view with posts that uses mobile-friendly UIs, etc., so we don't have to do as much work with skins -- they just have to look pretty.
- Blogs now have a separate "live" view that we use to handle domains / skins.
- Simplified some views and use IDs in some URLs for consistency.
- Delete a bunch of edge/blogger/multi-blog code that's now obsolete.
Test Plan: Will attach screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3695
Summary:
This leaves the UI in a pretty rough state, but implements blog policy controls and queries, and 1:1 relationships between posts and blogs. Needs a bunch more cleanup but seemed like an okayish breaking point in terms of cohesiveness.
Posts have these rules:
- Drafts are visible only to the author.
- Published posts are visible to anyone who can see the blog they appear on.
- Posts are only editable by the author.
...so we don't need any special policy UI or state to accommodate these rules.
Posts may have no blog if they're grandfathered in or you write a post to a blog and then lose the ability to see the blog. This is the messiest edge case -- specifically:
- You write a post to blog A.
- You publish the post.
- I edit the "Visible To:" for blog A and set it to exclude you.
What we do in this case is let you see the post in "My Posts", but you can no longer see the blog and you'll see the post as not being part of a blog. We can maybe give you some UI to let you move it later or something.
Test Plan: Hit all (I think?) of the interfaces without issues. Definitely some UI problems still right now.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3694
Summary:
Adds "can view" and "can edit" policies to blogs. Replaces "bloggers" with "can join".
This doesn't fully remove "bloggers" because I didn't want this to get too crazy/huge.
Test Plan: Created, edited, deleted blogs.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3693
Summary: I set one of my blogs to "phacility.com" based on `arc patch` and it now fatals since that's not a valid class anymore. :P Recover from these cases.
Test Plan: Viewed blog, no missing symbole exception.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3692
Summary:
introduce an abstract "PhameBlogSkin" class and instantiate two versions -- PhabricatorBlogSkin (Default) and PhacilityBlogSkin.
Most notable hack is including the directory /rsrc/images/phacility - this lets things "work" without messing around with the phacility.com CSS and instead just cutting and pasting most of the file.
Test Plan: played around with Phame a bunch. In particular, created a blog with a custom domain and the phacility skin. Verified it looked good and individual posts looked okay.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3687
Summary: This allows users to add a revision's author as reviewer according Differential configuration using the 'Leap Into Action' form.
Test Plan: Tested on local install.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1885
Differential Revision: https://secure.phabricator.com/D3682
Summary: Checks if the revision author is in reviewers only if differential.allow-self-accept is false.
Test Plan:
Tested locally pushing revisions with "arc diff" to a Phabricator
server with differential.allow-self-accept at true or false with
myself or not as reviewer.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1879
Differential Revision: https://secure.phabricator.com/D3673
Summary: Fix an implicit property.
Test Plan: No more `[11-Oct-2012 14:44:26] WARNING: [pool www] child 72785 said into stderr: "NOTICE: PHP message: [2012-10-11 14:44:26] PHLOG: Wrote to undeclared property PhabricatorTypeaheadCommonDatasourceController::$type. at [/INSECURE/devtools/phabricator/src/aphront/AphrontController.php:65]"`
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3685
Summary: `actorPHID` no longer gets set or exists.
Test Plan: Updated a revision without fataling.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3684
Summary: We need to be able to query for all properties matching a given pattern, as mentioned in D3676
Test Plan: Loaded Phabricator, ensured diff loaded, ensured field using all properties was able to enumerate them.
Reviewers: vrana, epriestley
Reviewed By: vrana
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3679
Summary:
We need to load all properties with some prefix in one field.
We can't merge them in one property because there will be a race condition for update (we don't have API for load+update+save).
Instead of providing API for this and complicating the code even more, just load everything unconditionally.
It shouldn't waste much bandwith or memory because we use most of the properties anyway.
It also looked overengineered to me.
Test Plan: Displayed revision with fields using diff properties.
Reviewers: epriestley, royw
Reviewed By: royw
CC: aran, royw, Korvin
Differential Revision: https://secure.phabricator.com/D3676
Summary: When we receive an email, figure out if any of the other tos and ccs are users. If they are, pass their phids through the stach as "exclude phids" and exclude them from getting the email.
Test Plan: used the various applications (audit, differential, maniphest) and noted emails were sent as expected.
Reviewers: epriestley, vrana
Reviewed By: vrana
CC: aran, Korvin, vrana
Maniphest Tasks: T1676
Differential Revision: https://secure.phabricator.com/D3645
Summary:
sometimes we show moved files as unchanged, sometimes deleted,
and sometimes inconsistent with what actually happened at the
destination of the move. Rather than make a false claim,
omit the 'not changed' notice if this is the source of a move.
Background: one of our users was confused by the source of a move
claiming to be unmodified when the destination of the move had
extensive modifications.
There may be a question about the quality of the data we keep/compute
about the changes, so maybe we could do a better or more consistent
job there (may just be nuances of the SCM in use); this approach
just smoothes that out and doesn't require fixing up the status
in pre-existing diffs.
Test Plan:
In the facebook phabricator, D594195#739ace1a and D590020#b97cfcfd
For others, find a diff that has moved files.
For the source of a move, we should now only show the "X moved to Y"
header and not the "unchanged" shield.
Reviewers: nh, vrana, epriestley
Reviewed By: vrana
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3669
Summary:
This is killing us since D3615.
Maybe we should also change `differential_inlinecomment` index <commentID> to <commentID, authorPHID>.
Test Plan: Checked queries at **/differential/** with comment drafts.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3667
Summary:
We need to go slightly farther to stub reply handler functionality for Ponder in at least some configurations, where we rely on the presence of a unique random key to generate per-object or per-object+user reply addresses.
This should probably be formalized in an interface since it's currently pretty ad-hoc.
Test Plan:
- Made comments in Ponder under a per-user email configuration.
- Ran migration, verified mail keys were generated.
- Ran migration again (with --apply), verified existing questions were skipped.
- Created a new question, verified mail key generation.
Reviewers: pieter
Reviewed By: pieter
CC: aran
Maniphest Tasks: T1873
Differential Revision: https://secure.phabricator.com/D3665
Summary:
When directories are added (e.g., on the `hg` initial commit, "/" is added) we don't render any diff for them but try to link to it in the table of contents.
Possibly we should render a diff saying "this directory was added", but stop it from complaining for now.
Test Plan: Looked at several hg and git commits which add directories to verify this gives us generally sensible behvior.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3663
Summary: no parents - no problem - just diff that ish against "null"
Test Plan: initial commits were viewable in my test repos
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1689
Differential Revision: https://secure.phabricator.com/D3660
Summary: For immutable objects, just use the ID as a cursor.
Test Plan:
- Analyzed commits from an empty cursor.
- Checked that cursor was good.
- Pulled some more commits.
- Analyzed commits again, verified it only hit the new ones.
- Verified the graph of "Count of CMIT" looked reasonable.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1866
Differential Revision: https://secure.phabricator.com/D3656
Summary: Make these always work. Notably, this makes them work in Maniphest. Previously this was at odds with stuff fixed in D3651.
Test Plan: Dragged and dropped files into Remarkup in Maniphest.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3652
Summary:
Currently, in Maniphest, if you drag-and-drop a file it always attaches. Instead, I want you to have two options:
- Drag and drop to the attachment area to attach; or
- drag and drop to the Remarkup panel to upload + inline.
For the first step, make the input have a clear drop target instead of it being the entire panel.
Test Plan: Attached files in Create Task, task view, meta mta send test.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3651
Summary:
Alternate proposal for D3635.
- Works better with small images.
- Produces a predictable thumbnail size.
- Somewhat reasonable output on 3000x10 images.
- Increase the size of Macro thumbnails to 240px.
Test Plan: {F20497}
Reviewers: vrana, chad
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3638
Summary:
* Moved the remaining `->save()` calls into editors
* for the `PonderQuestion`, separate `attachRelated` (needed for
indexing and viewing) from `attachVotes` (needed for viewing
but not indexing)
* Update the indexer to include comment and answer content
in the search index.
Test Plan:
Stuff works as before. Also: add a comment or answer with some
easily-identifiable text and search for it; observe that
it gets indexed appropriately.
Reviewers: epriestley, nh, vrana
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1869
Differential Revision: https://secure.phabricator.com/D3654
Summary: @chad, can you do the icon sheets based on 1.6? We're using a few icons not present in 1.5. I put the 1.6 "pro" source on Dropbox.
Test Plan:
Nav hover and selected states:
{F20598}
Launch hover state:
{F20596}
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1856
Differential Revision: https://secure.phabricator.com/D3649
Summary:
D03646 works, I don't want it to work.
Theoretically, it can cause us some troubles if we use this string in JS number context where 030 is 24.
Test Plan: D03646, D3646
Reviewers: epriestley, edward
Reviewed By: edward
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3646
Summary: Attempt to edit macro said that there is a name conflict.
Test Plan: Edited macro.
Reviewers: wez, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3644
Summary:
Basic infrastructure for generalizing subscriptions/CCs for T1808, T1514 and T1663.
- Implement `PhabricatorSubscribableInterface` and you'll get a subscribe/unsubscribe button for free.
- If there are any auto-subscribed users (like the question author) you can specify them; this makes more sense for Tasks and Revisions than Ponder probably, but maybe the author should be auto-subscribed.
- Subscriptions are either "explicit" (the user clicked 'subscribe') or "implicit" (the user did something which causes them to become subscribed naturally). If a user unsubscribes, they'll no longer be added by implicit subscriptions. This may or may not be relevant to Ponder but is an existing Herald feature in Differential.
- Helper method on PhabricatorSubscribersQuery to load subscribers.
- This doesn't handle actually sending email, etc. I think that's all so application-specific that it doesn't belong here.
- Now seems to work.
Test Plan:
{F20552}
{F20553}
Reviewers: pieter, btrahan
Reviewed By: pieter
CC: aran
Maniphest Tasks: T1663, T1514, T1808
Differential Revision: https://secure.phabricator.com/D3637
Summary:
Not sure how this triggers, but we have a macro that has no file
associated with it, and this caused a fatal on the /macro endpoint.
Test Plan: https://phabricator.fb.com/macro/?page=100
Reviewers: vrana, nh, epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D3642
Summary: This is sort of silly as-is, but automatically exposes flagging and will give subscribe/unsubscribe and "Subscribers" a place to plug into shortly. For context, see D3637 and T1808.
Test Plan: {F20550}
Reviewers: pieter, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1808
Differential Revision: https://secure.phabricator.com/D3641
Summary: this lets users specify what "name" to use in email addresses
Test Plan: changed the conf setting in my local instance and used phabricator. observed name format changes
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1862
Differential Revision: https://secure.phabricator.com/D3639
Summary:
Users are complaining that they don't see how the image macro looks until they use it.
Click leads to edit form.
Display it there.
Test Plan:
Edited macro.
Attempted to create macro with duplicate name.
Reviewers: epriestley
Reviewed By: epriestley
CC: chad, aran, Korvin
Differential Revision: https://secure.phabricator.com/D3636
Summary: This could be empty probably for old logs
Test Plan: Ran the controller.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3633
Summary:
Add an "Any Projects" field to the custom search UI.
This is starting to get ugly but we'll do a design pass on it before toooo long.
Test Plan: {F20423}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1610
Differential Revision: https://secure.phabricator.com/D3632
Summary: After D3630, make the API more clear: withAllProjects() vs withAnyProjects()
Test Plan: Loaded project page, maniphest task query, reports, filtered by project and "noproject". Grep.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1610
Differential Revision: https://secure.phabricator.com/D3631
Summary:
Currently, we have a single `projectPHIDs` field, and a separate flag which makes it act like AND or OR.
This is silly. Make two separate methods for setting `AND` vs `OR` projects. This also simplifies the implmentation.
This doesn't change the UI or any behavior (yet), it just makes the API more usable.
Test Plan: Loaded homepage, "All Projects" task view, verified queries made sense and returned correct results. Grepped for changed method name.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1610
Differential Revision: https://secure.phabricator.com/D3630
Summary: instance-wide this setting be
Test Plan: made a new task and noted the default priority honored what was in btrahan.conf
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1842
Differential Revision: https://secure.phabricator.com/D3626
Summary: tricky part is purging symbols at that time too. override "delete" method to get there, use transactions, etc.
Test Plan: deleted an arcanist project - it worked!
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, mbishopim3
Maniphest Tasks: T1738
Differential Revision: https://secure.phabricator.com/D3613
Summary:
This got refactored at some point and lost access to $method. Also make the error a little more helpful.
See https://groups.google.com/forum/?fromgroups=#!topic/phabricator-dev/05voYIPV7uU
Test Plan:
$ arc list --conduit-uri=http://local.aphront.com:8080/
Exception
ERR-CONDUIT-CORE: Invalid parameter information was passed to method 'conduit.connect', could not decode JSON serialization. Data: xxx{"client":"arc","clientVersion":5,"clientDescription":"orbital:\/INSECURE\/devtools\/arcanist\/bin\/..\/scripts\/arcanist.php list --conduit-uri=http:\/\/local.aphront.com:8080\/","user":"epriestley","host":"http:\/\/local.aphront.com:8080\/api\/","authToken":1349367823,"authSignature":"54bc136589c076ea06f8e5fb77c76ea7d57aec5b"}
(Run with --trace for a full exception trace.)
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3622
Summary: Show First 20 Lines doesn't display gap context and emits error.
Test Plan: Showed first 20 lines, last 20 lines, middle 20 lines - saw context and no error.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3616
Summary: Previously, only inline comment drafts were included.
Test Plan:
**/differential/** - verified that there are drafts where they weren't before.
Checked executed queries.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3615
Summary: also did a wee bit o' formatting stuff while I was in there.
Test Plan: it looks... well, it looks like its using the new UI component and all the information is there!
Reviewers: epriestley, pieter
CC: aran, Korvin
Maniphest Tasks: T1845
Differential Revision: https://secure.phabricator.com/D3611
Summary: DifferentialAffectedPath has no id or phid key so delete() won't work and we have to do things this other way.
Test Plan: deleted a few diffs on my test reproduction. aside from warnings about missing keys (epriestley is on it as I write this) no errors found and diff observed as deleted
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1846
Differential Revision: https://secure.phabricator.com/D3610
Summary:
D3555 stopped multiple commenting but it still attaches multiple diffs.
Save earlier to stop doing unnecessary work.
Test Plan: Reparsed the same commit twice at the same time.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3598
Summary: So they're maybe a little easier to deal with? I'm going to take this formally to "plz @chad plz help" land.
Test Plan: {F20329}
Reviewers: btrahan, vrana, chad
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3609
Summary: D3581 removed some flavor text. Allow applications to provide flavor text instead of status information if they so desire.
Test Plan: {F20325}
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran, chad
Differential Revision: https://secure.phabricator.com/D3608
Summary: A bunch of recently-created applications have help available; link to it.
Test Plan: Clicked each app, clicked help link in menu bar, ended up in relevant documentation.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3602
Summary:
It always bothered me that adding/removing blame view takes two clicks.
Showing it like this saves a click for almost all transformations, and I think it feels nicer too.
I'm open to adding a user-setting for this (This form or the drop-down), but figured I'd ask first.
Test Plan:
Use buttons to switch modes - make sure text on button matches action.
Repeate with lines highlighted.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: jungejason, Two9A, aran, Korvin
Differential Revision: https://secure.phabricator.com/D3499
Summary: In some cases, we want an action item (like "Subscribe") to effect a write that needs a CSRF check. Allow such items to render as forms so they gracefully degrade if JS is FUBAR'd. D3499 has a specific example.
Test Plan: Loaded new UI example page, clicked all the actions.
Reviewers: btrahan, vrana, avivey
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3596
Summary: since you can't edit text the correct move is to fork. Make that abundantly clear.
Test Plan: it was clear to me
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1826
Differential Revision: https://secure.phabricator.com/D3593
Summary: I didn't notice that D3494 is revert of D3453.
Test Plan: Checked both line and Blame previous links.
Reviewers: epriestley, fdoemges
Reviewed By: fdoemges
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3566
Summary:
We already have this information for all VCSes, there's no point in parsing it again.
We currently support it only for Git and I remember there were some bugs in it.
It should also be faster.
Test Plan: Blamed previous revision of a file which was moved in SVN.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3564
Summary: We have lots of empty drafts in DB.
Test Plan: Wrote revision comment, deleted it, checked db.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3591
Summary: this plugs this at the controller level. the editor could also be more aware of the "action" and the fix could be there.
Test Plan: set some ccs, changed it to comment, made teh comment, noted no ccs.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1838
Differential Revision: https://secure.phabricator.com/D3590
Summary:
"blog style" for now is just "true" to make this UI render better for the blog
LATER it will be a string which will choose the larger template. this will also have to do some messing around with links; when viewing on a phabricator instance links need to be a bit dirtier to carry around the blog whereas when viewing offsite we can tell what blog it is based on the host domain. anyhoo, this is future diff work
Test Plan: looked at blog - less ugly. resized blog to smaller sizes - became a "single list" of goodness for quality reading quite quickly.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3587
Summary: Moves toward unblocking D3547. Use a pinboard/album view to show image macros. Modernize and make (mostly) responsive.
Test Plan: Will attach screenshots.
Reviewers: btrahan, vrana, chad, teisenbe
Reviewed By: vrana
CC: aran
Maniphest Tasks: T175
Differential Revision: https://secure.phabricator.com/D3576
Summary: D3575, D3576, D3577, D3578, D3579, D3580 put all the /apps/ links on /applications/, so we can get rid of /apps/ without loss of functionality.
Test Plan: Clicked "More Stuff" on the homepage, got /applications/ instead of /apps/.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3581
Summary: Basic step toward modernizing Files, makes it appear on /applications/ and in typeahead.
Test Plan: Looked at /applications/.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3575
Summary:
This is mostly to unblock D3547.
- Move "Macros" to a first-class application called "Macros".
- After D3547, this application will also house "Memes" (macros with text on them).
- This will also make them easier to find; the top navigational query I field is "where are image macros?" nowadays, since it's not intuitive they're part of files.
- This makes some of the UI mobile-aware but doesn't set the `device` flag yet, since there are still some missing pieces.
- I'll separate storage out and continue modernizing the UI as we unblock and integrate D3547.
Test Plan: Created, edited and deleted macros. Viewed files.
Reviewers: btrahan, vrana, teisenbe
Reviewed By: vrana
CC: aran
Maniphest Tasks: T175
Differential Revision: https://secure.phabricator.com/D3572
Summary:
See D3572.
- Make "Diviner" show up on `/applications/`.
- Give it a landing page with links to documentation.
I have a parital port of Diviner proper into Phabricator in a branch, but it's a big chunk of work away from being landable.
Test Plan: Viewed `/applications/`, saw Diviner, clicked it, got documentation links.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3580
Summary:
Use sigils to simplify the vote implementation and move most rendering to the server.
Use unicode glyphs in place of graphics.
Test Plan: {F19539}
Reviewers: pieter, starruler
Reviewed By: pieter
CC: aran
Maniphest Tasks: T1644
Differential Revision: https://secure.phabricator.com/D3518
Summary: No use sites left after D3513.
Test Plan: `grep`
Reviewers: pieter, starruler
Reviewed By: pieter
CC: aran
Maniphest Tasks: T1644
Differential Revision: https://secure.phabricator.com/D3515
Summary: Restore the pager, using `executeWithOffsetPager()` to handle slicing, etc. Simplify and generalize `PonderQuestionQuery`.
Test Plan:
Set page size to 1, used pager to page.
{F19531}
Reviewers: pieter, starruler
Reviewed By: pieter
CC: aran
Maniphest Tasks: T1644
Differential Revision: https://secure.phabricator.com/D3513
Summary: The aoff/qoff thing is pretty awkward and putting these both on the same page is probably only at all useful when looking at someone else's questions/answers -- we should just pursue main profile integration for that.
Test Plan: {F19529}
Reviewers: pieter, starruler
Reviewed By: pieter
CC: aran
Maniphest Tasks: T1644
Differential Revision: https://secure.phabricator.com/D3512
Summary: Use flexible form and application side nav.
Test Plan: {F19525}
Reviewers: pieter, starruler
Reviewed By: pieter
CC: aran
Maniphest Tasks: T1644
Differential Revision: https://secure.phabricator.com/D3511
Summary: good title
Test Plan: set a paste visibility to administrator then forked it. noted new paste had administrators selected. saved it and verified administrators was the value.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1833
Differential Revision: https://secure.phabricator.com/D3570
Summary:
Use the new `PhabricatorObjectItemListView` in Ponder so it works with the new UI. It will also get some features like flags "for free" in the future.
This removes the pager; I'll restore it in the next diff.
Test Plan: Looked at feed.
Reviewers: pieter, starruler
Reviewed By: pieter
CC: aran, chad
Maniphest Tasks: T1644
Differential Revision: https://secure.phabricator.com/D3507
Summary:
- Introduce `shouldAllowPublic()`, indicating that logged-out users are OK in a controller if the install is configured to permit public policies.
- Make Paste views and lists allow public users.
- Make UI do sensible things with respect to disabling links, etc.
- Improve behavior of "you need to login" with respect to policy exceptions and Ajax requests.
Test Plan: Looked at "public" paste, saw all unavailable UI disabled, clicked it, got appropraite prompts.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D3502
Summary: this then enables people to create blog.theircompany.com. And for us, blog.phacility.com...!
Test Plan:
- created custom URIs of various goodness and verified the error messages were sensical.
- verified if "false" in configuration then custom uri stuff disappears
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3542
Summary: Avoid a BadMethodCallException for some pastes
Test Plan: Call up a paste from a day or so ago (in the FB environment)
Reviewers: nh, vrana
Reviewed By: nh
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D3560
Summary: We want to allow a broader access to our installation but we need to check the request in that case.
Test Plan:
Created a simple `PhabricatorRequestChecker` returning a custom controller.
Verified that this controller is used when accessing any page.
Returned `null` from this checker and verified that all 209 Phabricator pages are accessible.
Reviewers: epriestley
Reviewed By: epriestley
CC: scottmac, aran, Korvin, btrahan
Differential Revision: https://secure.phabricator.com/D2488
Summary:
Replace executing 'ps aux' with usage of available api to
control daemons PhabricatorDaemonControl.
This fixes isPullDaemonRunningOnThisMachine returning wrong status
under Fedora & lighttpd & SELinux because SELinux with default
settings blocked getting all processes in 'ps aux'.
Test Plan: start stop daemon and check repository app
Reviewers: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3557
Summary:
If attaching a commit or checking if there are any changes takes nonzero time then the revision may be closed by someone else.
Cleaner solution would be to do it inside a transaction and mark the SELECT as FOR UPDATE but it would be blocking.
Test Plan: Patched `$should_close` to be true, reparsed an already closed commit.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3555
Test Plan: Tested with various unit test states and noted that the
worst unit test result was always the state used for the entire diff.
Reviewers: nh, epriestley
Reviewed By: nh
Differential Revision https://secure.phabricator.com/D3465
Test Plan: Created diff, opened the file from Differential, opened the file in Diffusion.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3538
Summary: default policy to most liberal policy available for the installation
Test Plan: echo "sup man?" | arc paste Was able to view resultant paste with no errors!
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1815
Differential Revision: https://secure.phabricator.com/D3548
Summary:
It happens to me quite often that I leave the window with revision (by closing it or by visiting a link from it).
When I return then the comment draft is there so I clowncopterize it but forget that I wanted to take some other action than Comment.
Test Plan: Selected "Add Reviewers", added some reviewers, closed the window, opened it - the action and reviewers were still there.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3530
Summary: Similar to MySQL search.
Test Plan: Displayed Edit Dependencies dialog on revision.
Reviewers: epriestley, btrahan
Reviewed By: btrahan
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3519
Summary: ...also makes Maniphest Task Edit Controller use this when its not appropriate to upsell email.
Test Plan: played around with each tool and verified the Remarkup reference was present
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1756
Differential Revision: https://secure.phabricator.com/D3468
Summary:
Any answer without non whitespace characters results in an
error.
Test Plan:
- Submitted an empty answer, was told off
- Submitted an answer full of whitespace, didn't like that either
- Submitted a real answer, accepted
Reviewers: pieter, epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin, davidreuss
Maniphest Tasks: T1644
Differential Revision: https://secure.phabricator.com/D3492
Summary:
We have two troubles with this script:
# Our revisions and commits don't fit in the memory. (Our tasks do :-).)
# Reindexing revisions is slow.
Test Plan: Ran it.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3483
Summary:
People have occasionally complained about phabricator being slow. We have
the access log to look at to see when slowness happens, but it doesn't tell
us much about why it happened. Since it's usually a sporadic issue that's
reported, it's hard to reproduce and then profile. This change will allow us
to collect sampled profiles so we can look at them when slowness occurs.
Test Plan:
checking that sampling works correctly:
- set rate to 0; do several page loads; check no new entries in table
- set rate to 1; check that there's a new row in the table for each page load
- set rate to 10; check that some requests write to table and some don't
check new ui for samples:
- load /xhprof/list/all/, see a list with a lot of samples
- load /xhprof/list/sampled/, see only sampled runs
- load /xhprof/list/manual/, see only non-sampled runs
- load /xhprof/list/my-runs/, se only my manual runs
Reviewers: vrana, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3458
Summary: In the long term I think we can probably just explain this feature better, but for now I want to make sure no one makes a mistake with "Public". This seems like a reasonable way to do it.
Test Plan: Looked at the policy selector dropdown.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3486
Test Plan:
# Set default view to non-blame.
# Visit URL with '?view=blame'.
# Click //Skip Past This Commit//.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3494
Summary: Question titles were not escaped; now they are.
Test Plan: Observe the escaping.
Reviewers: epriestley
Reviewed By: epriestley
CC: nh, aran, Korvin
Differential Revision: https://secure.phabricator.com/D3490
Summary: Does what it says on the tin
Test Plan: Viewed ponder question, expanded link, added comment
Reviewers: pieter, epriestley
Reviewed By: pieter
CC: vrana, aran, Korvin
Maniphest Tasks: T1775
Differential Revision: https://secure.phabricator.com/D3485
Summary:
- Renames `PhabricatorPolicyQuery` to `PhabricatorPolicyAwareQuery` (a query which respects policy settings).
- Introduces `PhabricatorPolicyQuery`, which loads available policies (e.g., "member of project X").
- Introduces `PhabricatorPolicy`, which describes a policy.
- Allows projects to be set as policies.
- Allows Paste policies to be edited.
- Covers crazy cases where you make projects depend on themselves or each other because you are a dastardly villan.
Test Plan: Set paste and project policies, including crazy policies like A -> B -> A, A -> A, etc.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D3476
Summary: Add storage to Pastes for view policies.
Test Plan: Set policies on pastes, see next diff.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D3474
Summary: This is pretty spartan, but it does the job.
Test Plan:
Patch, update storage, add some comment
to your favorite question or answer.
Reviewers: nh, vrana, epriestley
Reviewed By: epriestley
CC: aran, Korvin, starruler, syrneus, me.here, victorzarate7
Maniphest Tasks: T1645
Differential Revision: https://secure.phabricator.com/D3471
Summary: This takes the place of D2721 which I am going to abandon.
Test Plan:
* Make a commit with "Closes T###" in the summary field. See that the mentioned task gets closed.
* Make a commit with "refs T###" in the summary. See that it gets added as a related commit via edges.
Reviewers: 20after4
Reviewed By: epriestley
CC: aran, Korvin, champo
Maniphest Tasks: T945
Differential Revision: https://secure.phabricator.com/D3466
Summary:
- Get rid of an AphrontSideNavView callsite.
- Modernize and simplify the application implementation.
- Doesn't work perfectly on tablet/phone but that's because not all the UI examples work there yet.
Test Plan: Looked at /applications/ and /uiexample/.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3431
Summary:
This is the last Paste UI element that doesn't work properly on tablets/phones. Make it flexible.
Also add empty states to Paste.
Test Plan: Viewed various errors, and `/uiexample/errors/`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3429
Summary:
If the actual commit message has a duplicate field and we shouldAutoclose it then the commit message parser fails.
Put the error in `$errors` instead.
Test Plan:
Reparsed commit with duplicate field in message.
Tried to `arc diff` message with duplicate field.
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3470
Summary:
Including view in permalink seemed like the correct solution to me.
Instead, I delete it all the time when copying somewhere else.
Test Plan:
Clicked on the link.
Changed view on permalink.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3453
Summary:
Owners packages might include repositories that are no longer tracked
(or checked out locally), and there's no reason why we need a working copy
to generate a diffusion link. Instead of displaying a red error box, this
diff allows the owners tool to render correctly.
Test Plan:
viewed /owners/view/all/ and /owners/package/395/ and verified no error box
for not having a checkout of the repository.
Reviewers: epriestley, vrana, wez
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3452
Summary:
In the commit message parser worker, it tries to get relationships for a
revision without first loading them. (D3444 introduced the change.) This
is where the get happens, so I'm adding the load here - maybe it should be
added in the CommitMessageParserWorker instead?
Test Plan: Made this change, and my taskmaster daemons stopped failing.
Reviewers: epriestley, vrana
Reviewed By: vrana
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3448
Summary:
This is mostly ripped from D2721, but doesn't implement the T945 part.
After we parse a commit message, give DifferentialFieldSpecifications an opportunity to react to the message as well (e.g., by updating related objects).
Test Plan: Impelmented a var_dump() body, ran `reparse.php` on a commit.
Reviewers: vrana, 20after4, btrahan, edward
Reviewed By: 20after4
CC: aran
Maniphest Tasks: T945, T1544
Differential Revision: https://secure.phabricator.com/D3444
Summary:
Without this, the https redirect doesn't work if you're not logged in, because
the login check in willBeginExecution happens before the redirect controller
can redirect. This also has the unpleasant effect of the login page on http
(when it should have redirected to https) not having any css or js.
Test Plan:
With the https redirect enabled, loaded phabricator without being logged in,
and saw that I got redirected to the https login page instead of seeing a
http login page.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3438
Summary: AphrontSideNavView is oldschool and I want to kill it.
Test Plan: Viewed Ponder, clicked both nav options.
Reviewers: pieter, vrana, btrahan
Reviewed By: pieter
CC: aran
Differential Revision: https://secure.phabricator.com/D3430
Summary:
I want to implement a `{P123}` rule to embed pastes, but we need viewers everywhere before it will work with privacy.
This is not exhaustive; many Remarkup callsites haven't been converted to `PhabricatorMarkupInterface` yet.
Test Plan: Looked at Maniphest, Differential, Diffusion, Phriction; added markup, made edits and hit previews.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3428
Summary:
We have lots of info about unit tests.
This allows linking them from Unit field.
Test Plan: Monkey patched `$test['link']`, clicked on it.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3434
Summary:
I've replaced all `id(new PhabricatorObjectHandleData(...))->loadHandles()` by `$this->loadViewerHandles(...)`.
Lint caught one usage in a static method.
Test Plan: Displayed revision with sporadic author.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3432
Summary: I've done D3432 in the hope that it will fix also this...
Test Plan: Blamed sporadic author.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3433
Summary:
See discussion in D3340. Some configurations set only a search attribute because their records are indexed by username (this is probably not quite the correct LDAP terminology). Other configurations use one attribute to search and a different attribute to select usernames.
After D3340, installs which set only a search attribute broke. Instead, fall back to the search attribute if no username attribute is present.
Test Plan: Successfully logged in on my test slapd.
Reviewers: yunake, voldern, briancline
Reviewed By: voldern
CC: aran
Differential Revision: https://secure.phabricator.com/D3406
Summary: It would be best to add this everywhere at once but I'm too lazy for it.
Test Plan: Displayed package list and detail and revision comment with away users.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3419
Summary:
Currently, if some page /x/ has a grandchild at /x/y/z/ but /x/y/ does not exist, we insert a ghost entry for it in the "Document Hierarchy". However, we don't sort the hierarchy properly after inserting ghosts, so they always appear at the bottom and in an unuseful order.
Instead, sort children again after any ghost insertions.
Test Plan: Created /x/a/, /y/a/, /z/a/, verified ghosts sorted incorrectly before this patch and correctly afterward.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1750
Differential Revision: https://secure.phabricator.com/D3418
Summary:
There is basically no reason for anyone to ever use the uncollapsed mode for more than the first 2 minutes of using the tool.
Delete all code related to collapse/expand.
(I'm going to add tooltips next.)
Also move the drag bar a few pixels to the right, so it does not overlap with the scrollbar on the "local" nav if there is one.
Test Plan: Viewed in desktop/tablet/phone modes.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3413
Summary:
Some fields require displayed diff (e.g. Lines), some require last manual diff (Lint, Unit).
Lint requires both - it needs to find if the line with the error is displayed or not.
Test Plan: Displayed committed diff with lint errors, clicked on them, got last manual diff at the correct line.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3331
Summary:
Ball is more obvious and visible than I thought.
Delete the status word and display until date in title.
Also display the until date in revision list.
Also display near future dates with DoW instead of year.
Test Plan: Displayed revision and revision list with away reviewers.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3407
Summary:
We have some complaints on this feature:
- It's not clear that the displayed code is context from gap.
- It would be better if the context would be displayed with its real indentation.
- It's not clear how far the context is from the displayed code.
- Links revealing gap aren't on consistent place.
This solves all these problems and introduces new one:
It now seems that the reveal links works only with the left side.
Anyway, I think that this is better overall.
I don't want to put the context on a separate line to not waste space.
Test Plan: Displayed various contexts, revealed context.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, bh, jwatzman
Differential Revision: https://secure.phabricator.com/D3404
Summary: the former is self explanatory. the latter is necessary for installations that require email verification. since many system agents are given bogus email address there can become a problem where these accounts can't be verified
Test Plan: created system agent account from scratch. edited user and toggled system agent accountness. created system agent with unverified email address and verified it.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1656
Differential Revision: https://secure.phabricator.com/D3401
Summary:
I have a chain of revisions sometimes.
I want to see also revisions depending on me.
Test Plan: Displayed revision in the middle of chain.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3399
Summary: this is useful for certain workflows, typically where the reviewer is a gatekeeper of sorts who does the acutal commit. Special thanks to D2900 which made this relatively brain-dead to code up.
Test Plan: set to "true" in my local development environment and verified test user "xerxes" could close my stuff
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1732
Differential Revision: https://secure.phabricator.com/D3398
Summary: said differently, if the user included another to address or one or more cc's, don't send the error message email.
Test Plan: played around in the metamta test console and verified that blank replies generated the error handler.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1643
Differential Revision: https://secure.phabricator.com/D3345
Summary:
We have `max_allowed_packet` 1 GiB but our replication dies if the query is longer than unknown value (it dies with 293 MB long query).
Anyway, there's no reason why we should not save the cache if you have small `max_allowed_packet`.
Test Plan: Lowered `$size` to 100, deleted cache from DB, displayed changeset, verified issued queries in DarkConsole, verified DB.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3390
Summary:
I like systems that just work. It is possible to store files larger than max_allowed_packet in MySQL and we shouldn't demand it.
It also fixes a problem when file was smaller than `storage.mysql-engine.max-size` but its escaped version was larger than `max_allowed_packet`.
Test Plan: Reduced the size to 5e4, uploaded 90 kB file, checked the queries in DarkConsole, downloaded the file.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3392
Summary: We recently opted for 'security.alternate-file-domain' and we have some hotlinks to the original domain.
Test Plan: Enabled 'security.alternate-file-domain', observed redirect.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3380
