Page MenuHomePhabricator

Unbeta Phame
Closed, ResolvedPublic

Assigned To
Authored By
chad
Sep 8 2015, 2:23 PM
Referenced Files
F1702897: walking-derp-gif_o_705850.gif
Jun 27 2016, 4:41 PM
F1702880: Screen Shot 2016-06-27 at 9.02.06 AM.png
Jun 27 2016, 4:08 PM
F1702849: Screen Shot 2016-06-27 at 8.46.17 AM.png
Jun 27 2016, 4:08 PM
F1702871: Screen Shot 2016-06-27 at 8.58.18 AM.png
Jun 27 2016, 4:08 PM
F1702843: Screen Shot 2016-06-27 at 8.44.12 AM.png
Jun 27 2016, 4:08 PM
F1702873: Screen Shot 2016-06-27 at 9.00.51 AM.png
Jun 27 2016, 4:08 PM
F1702841: Screen Shot 2016-06-27 at 8.43.58 AM.png
Jun 27 2016, 4:08 PM
F1702876: Screen Shot 2016-06-27 at 9.01.21 AM.png
Jun 27 2016, 4:08 PM
Tokens
"Love" token, awarded by johnny-bit.

Description

Container task for kicking Phame out of the nest.

  • Phame Live URLs with fake post_ids cause fatal instead of 404
  • View Live useless on archived blogs
  • Edit Blog Image treatment like profiles
  • Pager next/prev should keep you on whatever view you're on
  • Unset user titles aren't falling back properly
  • Add captions to edit fields for better clarification
  • Blog header edits are not transactional
  • Mechanic to get from Blog -> ApplicationSearch?
  • Make visibility search checkboxes and not a select.
  • HTTP Parameters dropdown on New Post goes to 404
  • Implement EditEngine Comments?
  • Add a Monogram / Remarkup for Posts
  • Force serving https if http is called, etc

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Phame should be "mostly reasonable" to test as an internal blog platform. There is more polish to come, but feel free to send feedback along here.

Not opening another bug report since Phame is not out of beta yet, but I thought I should still let you know that I stumbled upon an exception yesterday after updating from before the comments revamp:

  • When viewing a Blog page (not a post itself): (Unable to resolve method 'isArchived'.)
  • When submitting a New Blog form: >>> UNRECOVERABLE FATAL ERROR <<< Undefined class constant &#039;TYPE_STATUS&#039;

Maybe just a storage upgrade thing. I could try reinstalling my instance and see if the databases match up, holding off for now because maybe you've got some better insight.

The other thing I noticed was that a Phame Title is required for new blog posts. That's not necessary or useful at least for my use case (internal, non-SEO-optimized posts) and likely confusing to users without further explanation. I'm hoping it can be made optional and/or automatic.

Apart from that, it now looks fabulous.

master as of yesterday (Tue) approx. 2 pm PST. When I get home I'll check if the logs match up, but I took care to pull all three repos and ran a storage upgrade.

These errors point to being behind HEAD and/or not restarting Apache/PHP-FPM. rP2a063a for example resolved the constant issue.

I did find D14574 in testing this, so there was at least 1 bug.

Yep, it was the Apache restart. Sorry for poor testing, thanks for fixing the incidentally discovered bug, and hopefully the other part of my comment was less useless :)

Some thoughts from banging on this, not sure how much you want to deal with:

Did you want to rename it to "Salty Sea Captain's Nautical Logbook"? Gets harder after we unbeta.

It's weird to me that /phame/'s home page is recent posts. I'd expect a list of blogs instead. At least personally, I visit Phame directly to write posts, not to read them, and I'd expect this to be somewhat common (i.e., the primary way to find new posts should usually be through feed / profiles / dashboards / sharing, not Phame itself?). If I want to write a post, there's nowhere obvious to do that from -- I have to find "Find Blogs", then find the proper blog. The "all posts you can see" view isn't bad, it just seems sort of weird as the default?

Skin name on the blog list seems pretty random / debugging-ish?

Star icons on the same list are sort of gratuitous?

"This blog has no visible posts." in red on an empty blog seems out of place as an error, should be more grey/muted?

Screen Shot 2015-12-03 at 12.45.33 PM.png (404×908 px, 30 KB)

"Domain" field on blog detail view just renders nothing (just a blank space) if no domain is set. Maybe a note instead, or omit?

Screen Shot 2015-12-03 at 12.46.27 PM.png (145×534 px, 19 KB)

Date rendering issue on blog main view ("Written by chad on .", instead of date)?

Screen Shot 2015-12-03 at 12.47.17 PM.png (194×842 px, 14 KB)

On post detail view, maybe:

  • Put byline at top for consistency, remove "Blogger"?
  • Remove "Blog"? (Redundant with crumbs)
  • "Published" is funky with empty rendering?

Screen Shot 2015-12-03 at 12.48.26 PM.png (225×330 px, 14 KB)

No way to show full history of blog posts, and no record of where they moved, as far as I can tell?

Oblivious skin is terrible and we should just get rid of it. The internal skin is a million times better.

Crumbs on post details are a little weird -- I'd expect "Phame > Blogs > (Blog Name) > (Post Name)" but the "Blogs" crumb vanishes.

Choosing a blog picture takes you back to the blog main view page, instead of to the manage page (where you just were). I'd expect to go to the manage page instead. Likewise, "Cancel" from blog edit page takes you back to blog view page, instead of blog manage page.

"Write Post" and "View Live" from blog manage page are maybe redundant? "Write Post" cancels to the wrong place and removing it seems simpler than fixing it, maybe.

"Formatting is enforced." caption note under "Phame Title" is redundant/not informative?

Per earlier, but we can probably just drop "Published" here in the properties? I think the huge header conveys it pretty effectively.

Screen Shot 2015-12-03 at 12.58.21 PM.png (314×869 px, 32 KB)

In the above screenshot, there's no "Publish or Preview" action that I can find -- probably out-of-date instructions? Maybe:

  • Say "Publish" or "Preview"? Or don't reference Preview at all?
  • Give publish (or preview) a different icon?

If you click "View Live" (disabled action) you get a similar misleading error referring to an action which doesn't exist ("Preview/Publish"):

Screen Shot 2015-12-03 at 1.00.51 PM.png (176×555 px, 16 KB)

"Move Post" could probably be workflow + dialog?

(I thought there were still some some "Custom Domain" issues that I'd like to fix before unbeta'ing but I need to dig those up.)

We should remove phame.createpost, phame.query and phame.queryposts, rather than unbeta'ing with unstable methods (or wait for EditEngine/etc stuff).

We should remove phame.skins before unbeta'ing and just hard-code it for now, in the general interest of reducing the amount of config we have.

Maybe worthwhile to wait for EditEngine? I want to do Maniphest next, but could do Phame after that.

Do you see any point to doing "Custom Domain" stuff? That is, with EditEngine and Conduit, should that use case just be on the end user?

I don't think it costs us very much to keep it around, and it does make some stuff nicer/easier. For example, we'd have no way to link to the live blog or live post if we don't know what domain it's on, and I like checking the live post after publishing something.

We could maybe get rid of skins. One issue is that blog.phacility.com uses them. I guess some other options there are:

  • Just get rid of blog.phacility.com? It's not useful today, but maybe we'd like it to be useful in the future. I'm fine with getting rid of it, although I could imagine us regretting that, at least a bit, in a couple years.
  • Keep blog.phacility.com but move it to Conduit. "View Live Blog/Post" will stop working. I mostly don't really want to do the work for this -- it's kind of a lot of work. This is more work for us than keeping "Domain" in the upstream, I think.
  • Keep blog.phacility.com and de-skin it. Also seems fine?

Particularly, if you use Conduit to build a blog, you have to pull in stuff like remarkup.css somehow and there's pretty much no way to do that that's reasonable, I think.

Oblivious skin is terrible and we should just get rid of it. The internal skin is a million times better.

Not sure what to do with this, mostly.

Yeah, let me dig into that a bit and come up with some options. I have to go look at what the skin code is doing.

I'd ideally like the (by default, maybe only) "live" view to just be the internal skin in a read-only mode with no header or a slightly custom header (so you can breadcrumb www.yourproject.com maybe?) but I'm not sure how practical that is. Are you OK with the Phacility blog getting de-skinned like that if I just rip skins out completely?

DocumentPro is way better than Oblivious

In T9360#146722, @chad wrote:

DocumentPro is way better than Oblivious

Fully agree :)

In T9360#146711, @chad wrote:

Do you see any point to doing "Custom Domain" stuff? That is, with EditEngine and Conduit, should that use case just be on the end user?

Custom domains are nice. Especially for stuff like small development blogs not fully endorsed by company but not compaining either. Plus development compan blogs like blog.phacility.com do look nice :)

We could maybe get rid of skins. One issue is that blog.phacility.com uses them. I guess some other options there are:

  • Just get rid of blog.phacility.com? It's not useful today, but maybe we'd like it to be useful in the future. I'm fine with getting rid of it, although I could imagine us regretting that, at least a bit, in a couple years.

From experience: getting rid of good stuff always comes with a bit of regret and "ok, how do we bring it back and/or make something like that only better this time"

  • Keep blog.phacility.com but move it to Conduit. "View Live Blog/Post" will stop working. I mostly don't really want to do the work for this -- it's kind of a lot of work. This is more work for us than keeping "Domain" in the upstream, I think.
  • Keep blog.phacility.com and de-skin it. Also seems fine?

Particularly, if you use Conduit to build a blog, you have to pull in stuff like remarkup.css somehow and there's pretty much no way to do that that's reasonable, I think.

From my POV, if skins on custom domains won't be too much of a hassle to maintain, it would be better to just have some documentation on making custom skin. Probably would be easier for many than pull phame through Conduit 😉

One other thing is that custom domains won't work on Phacility by default, and I don't think we can easily make them work (we can't just have users CNAME their DNS because we won't have the right SSL certificate). So maybe that's an argument for removing the feature or adding an option (ugh) to disable it.

Alternatively, we could dip our toes in the water of selling instances domains as an add-on. It would work like this:

  • You say you want to serve "blog.mycompany.com" from Phacility and hand us a valid SSL certificate for that domain.
  • We spin up an ELB with the same configuration as the primary ELB, put the certificate on it, and give you the host to CNAME the domain to in DNS.
  • Things now work.

The downsides to this are:

  • I don't know if anyone wants it.
  • It costs us $20/month for each domain in direct costs for the ELB, so we have to charge at least that much.
  • It costs us $20/month immediately, so I don't want to let instances who have never paid us spin up a bunch of domains and then forget about the invoices. So we likely need to pre-charge for this -- or, at least, charge a "setup fee" or something -- which requires some billing changes. We could do stuff like "if you've already paid an invoice and have at least X users, there's some wiggle room", maybe.
  • It would be better if we could use SNI instead (which would let us host a lot of SSL certificates on a single host) but I'm not sure if SNI is really ready for production use yet. (SNI is like virtual hosts for SSL certificates, and not universally available in clients -- for clients without SNI support, each SSL certificate you want to host must generally have a unique IP address.) Another approach is becoming a Certificate Authority and signing our own certificates like Cloudflare does, but this is probably very far out of our reach at our current scale (we must convince a root CA that we are unfailingly trustworthy, because we can sign a valid www.google.com certificate if we can sign a valid blog.mycompany.com certificate).
  • Managing the ELBs is going to be a pain when we need to make changes to the production hardware pool, since we can't just change one configuration in one place (each ELB needs to be updated separately).
  • I'd need to build a fair amount of tooling which we don't currently have for this. Probably at least 3-4 days of work unless we come up with something very clever for the billing (like "email us and we'll manually approve you if you seem trustworthy").

An upside to this is:

  • We need to do 95% of this work anyway if we're going to support phabricator.mycompany.com whitelabeling and/or "Enterprise Clusters" eventually.

It's also possible that we can roll our own loadbalancers by assigning a lot of elastic IPs to a single host. This currently allows us to put ~240 domains on a host, at a cost of ~$4 per domain per month (for the elastic IP) instead of ~$20 per domain per month, but is more work. In contrast, SNI would let us put thousands of domains on a host with no cost per domain.

The story on SNI actually looks pretty reasonable (we're mostly just losing IE on XP and very old mobile devices), and should be more reasonable by the time we're thinking about whitelabeling main instances.

I believe ELBs do not currently support SNI, although if just a hundred more people make "+1" comments on this thread I am sure that will cause SNI support to manifest from on high:

https://forums.aws.amazon.com/thread.jspa?threadID=96534&start=0&tstart=0

It would be nice to use ELBs, but we can roll our own with HAProxy instead (and already run HAProxy elsewhere since we can't ELB SSH traffic either).

We use SNI and nobody says it's bad. Hell, people actually WANT SNI, since when hositng fo example small shop, you basically need SSL (even cheap one), but IP address (especially IPv4) is pricey per month. Most people when faced with "additional $$$/month with only benefit being customers using ancient software" or "0/month additional charges, only lose handfull of ancient software-using customers" chose 0/month 😉

Also usually setup costs with regards to domains and ssl certs are expected -- nobody actually gives trials of domain registration or trial SSL cert from public CA (there are exceptions like free certs and/or cross sell).

chad triaged this task as High priority.Dec 10 2015, 9:24 PM

I think this is happening because both the "Title" and "Body" transactions are emitting text:

Screen Shot 2015-12-11 at 6.41.24 PM.png (219×627 px, 38 KB)

This is a group of transactions like "set title to X, set body to Y", and then we go through them and pull out all the feed remarkup bodies, and get the same text twice (once from title, once from body).

The simple fix would be to remove the parts of getRemarkupBodyForFeed() which emit text for TITLE or VISIBILITY. That seems reasonable to me? TITLE definitely seems fine, and given that "Published" is default now it seems fairly OK to drop the body text on a publish since they'll be rarer now?

If we want to retain the published events it gets a lot more complicated, so even if we want to keep them around maybe we should drop them for now and file a followup.

The behavior of TITLE and VISIBILITY are also wrong: they emit the current body text, not the body text at the time the change was made.

Few more things I caught:

  • When you archive a blog, the "View Live" button remains active, but takes you to a broken page:

Screen Shot 2016-06-27 at 8.43.58 AM.png (158×985 px, 20 KB)

Screen Shot 2016-06-27 at 8.44.12 AM.png (243×998 px, 31 KB)

I'd expect it to grey out and show you an explanation, ideally.

  • Editing the blog header is not transactional, but should be (the edit doesn't go through the Editor, and no transaction log is produced).
  • In practice, these fields seem maybe a bit confusing to me. I'm not sure I'd understand what they mean if I didn't know. Maybe just something we document:

Screen Shot 2016-06-27 at 8.46.17 AM.png (156×184 px, 8 KB)

But maybe "Parent Site" could become "Parent Site Name", and "Parent Domain" could become "Parent Site URI" to start with?

  • It's weird to me that there's no way to get from a blog (or from a post) to the Search UI for posts in that blog. There's also no way to search/filter posts by blog. You have to go all the way up to the root and then click "View All", I think?
  • The edit link on the post list is a little weird to me, since Maniphest is the only other application which has it (I think?) and the behavior is different there. In particular, you don't lose your place in the results in Maniphest, but you do in Phame. (I think this was originally added to Maniphest as a stopgap for T1960? But it doesn't address that use case in Phame, and that use case likely does not exist.)
  • The "Visibility" selector in Phame posts is a <select /> dropdown, which prevents you from selecting, e.g., archived + draft posts. It should be checkboxes instead so that all states are selectable.
  • Maybe the blog profile image should get the same "Edit" treatment that Project profile images do (that is, "edit" cue on hover + edit by clicking)?

Screen Shot 2016-06-27 at 8.58.18 AM.png (212×419 px, 20 KB)

  • When viewing a post for a blog in the internal / non-live view, the crumbs link forward and backward to the live version of the post. Instead, they should keep you in the internal view. That is, on this page:

Screen Shot 2016-06-27 at 9.00.51 AM.png (535×1 px, 62 KB)

...clicking "Previous Post" takes me to the live view of the previous post:

Screen Shot 2016-06-27 at 9.01.21 AM.png (1×1 px, 104 KB)

It should keep me in the internal chrome instead.

  • If you remove your user title, posts credit the author in a confusing way (note no title, instead of "Pocket Monster" as in earlier screenshots):

Screen Shot 2016-06-27 at 9.02.06 AM.png (88×342 px, 13 KB)

Something is probably calling getTitle() or similar instead of getDisplayTitle(). The icons have fallback titles, this should show:

Animal

...by default.

  • Blog post comments don't use EditEngine, but should.
  • The whole application should probably be converted to modular transactions (T9789). This isn't strictly necessary since it has no user-visible effect, but it can only gets harder after unprototyping.
  • T11219 appears to describe a legitimate issue but I haven't dug into it yet, might just be an upstream data thing.

Oh, and we select the correct protocol for custom domains now, but don't enforce it when serving content. That is, these both work, and we always link to "https", but "http" should redirect you directly to "https" instead of serving content:

  • PhameBlog calls delete() on posts directly, but should use the destruction engine to destroy them instead. Calling delete() directly means that edges, comments, etc., on the posts won't be cleaned up when a blog is deleted.

The edit link on the post list is a little weird to me, since Maniphest is the only other application which has it (I think?) and the behavior is different there. In particular, you don't lose your place in the results in Maniphest, but you do in Phame. (I think this was originally added to Maniphest as a stopgap for T1960? But it doesn't address that use case in Phame, and that use case likely does not exist.)

This was a user request, person keeps a list of drafts on their dashboard and wanted easy editing access.

Blog post comments don't use EditEngine, but should.

Not sure I understand what this means. Is there an example somewhere?

chad updated the task description. (Show Details)

PhabricatorPasteViewController is the best example for the EditEngine comment thing. Basically, make the comment form generate like Paste does (Maniphest too, although it's more complex). Then you can remove PhamePostCommentController completely.

It's possible that it's tricky to get the styles right.

Oh, and we select the correct protocol for custom domains now, but don't enforce it when serving content. That is, these both work, and we always link to "https", but "http" should redirect you directly to "https" instead of serving content:

Is this simple? Not sure where to start? Nothing is jumping out at me here.

In T9360#183602, @chad wrote:

Is [HTTPS] simple? Not sure where to start? Nothing is jumping out at me here.

I think you can just implement PhameBlogSite->shouldRequireHTTPS() to do something like this:

public function shouldRequireHTTPS() {
  $full_uri = $this->getBlog()->getDomainFullURI();
  $full_uri = new PhutilURI($full_uri);

  return ($full_uri == 'https');
}

That will only do HTTP -> HTTPS, not HTTPS -> HTTP, but I think that's perfectly fine, since the major goal is just to let blogs be forced to HTTPS.

You should be able to test by setting a blog to https://local.blog.phacility.com/, then hitting http://.../ in your browser and seeing if it redirects you. It won't actually work unless you go through a ton of effort to make HTTPS work locally, but that's okay. If we miss something and it doesn't quite actually work in production it's easy to set the blog back to "http" and fix stuff later.

Anything left to fix before unbeta?

Just the T6299 stuff, I think.

We could just leave that as "known issues", it only affects custom domains which I'd guess won't see tons of use outside of this install (particularly given that I pushed T4936 out)? Maybe just make a note in the docs if you're eager to unbeta it.

I think I started the unbeta task in December. So... Merry Christmas!

I've been pretty excited about this since the unbetaing started.

Oh, now that we gave posts the J monogram, we should probably also find a way to put that on the blog page somewhere (at least the internal one, and maybe the ApplicationSearch results page). Right now you have to know that you can fish the ID out of the URI. Let me see if I can poke at that a bit with my doc stuff.