On 01/12/2012 03:46 PM, Matt Wagner wrote:
On Thu, Jan 12, 2012 at 02:35:05PM -0500, Tzu-Mainn Chen wrote:
> The patch does two things:
>
> a) change all html error messages to flash errors
> b) changes the user index page to redirect to accounts if the user doesn't have
the appropriate permissions
ACK, with a few notes:
- I fear I may have steered you wrong earlier. I don't think you need
to update cs.yml. (At least, I haven't been doing so.)
- You commented in the BZ that it looks rather odd to have the error as
a flash message, and the rest of the layout blank. I agree, but I think
it looks nominally less-ugly than it did before, which was just
unstyled error text in the layout. So I think it's fine. (IMHO)
- This is not the fault of your patch by any stretch, but while I'm
logged in as a non-admin user, I notice that we have a *ton* of links
that give permission denied errors. I thought our general approach was
to not show links the viewer didn't have permission to see, which
means that we have work to do in that case.
On this last point, we should probably track that as a separate bug, but
we do have helpers to hide links (and any other content) that the
logged-in user doesn't have permissions on. There's examples in the
permissions views themselves (i.e. privilege level to see the list page
(controller check) is different than the level needed to edit the list
(conditional check in view for the action links and/or form elements)
Scott
Of course, don't worry about the last issue -- it's not
caused by your
patch, it's just an incidental observation.
-- Matt