Show an "approval queue" item on the home page for admins, and sort out menu item visibility
ClosedPublic
Actions

Authored by epriestley on Nov 13 2013, 2:52 AM.

Details

Reviewers

Commits

Restricted Diffusion Commit
rP0fa411083f72: Show an "approval queue" item on the home page for admins, and sort out menu…

Summary

If you're an administrator and there are users waiting for approval, show a count on the home page.
Sort out the isUserActivated() access check.
Hide all the menu widgets except "Logout" for disabled and unapproved users.
Add a "Log In" item.
Add a bunch of unit tests.

Test Plan

Ran unit tests, clicked around as unapproved/approved/logged-in/logged-out users.

Diff Detail

Branch

appqueue2

Lint

Lint Passed

Severity	Location	Code	Message
Advice	src/applications/auth/application/PhabricatorApplicationAuth.php:52	XHP16	TODO Comment

Unit

Tests Passed

Event Timeline

I thought "maybe we should have userIsActive(), which would imply logged in and activated", but I figure since that's a made up term we're better off with the && action in this diff.

src/applications/auth/application/PhabricatorApplicationAuth.php
52	is this a @chad todo?

yes @chad plz fix thx u :3

The double checks are technically redundant, since logged out users won't be isApproved, but they weren't redundant when I was writing this since isApproved = 1 was still in the code. We could probably simplify it, but I figured I'd leave it explicit for now.

Particularly, isUserActivated() should maybe give different answers for logged-out users depending on context. When viewing public content, we want them to count as "activated" (i.e., "not disabled"). But in cases like the menu stuff, we want them to count as not-activated (i.e., don't show them settings).

We don't have that many checks, all of them are explicit, all the isUserActivated is greppable, and the important ones have test coverage, so I figure we can run with this for a bit and then adjust later in whichever direction we need to go.

(That is, I don't think isUserActivated() should actually give context-sensitive results, but that it should maybe be split into two methods some day or something.)