Code review comment for lp://staging/~jcsackett/loggerhead/add-privacy-ribbon

Revision history for this message
Ian Booth (wallyworld) wrote :

Hi Jon

This looks nice (the answer to my question below notwithstanding). I have a small request and a question.

1. The Request

The method BranchWSGIApp.public_or_private() should be called public_private_css() to (i) be consistent with how Launchpad does it, and (ii) better reflect the semantics of what the method is used for.

2. The Question

I assume you needed to run up loggerhead locally to test? I'm confused how it works since I can't see how 'private' is ever set. The new 'private' parameter was added to BranchWSGIApp, but I can't see in the loggerhead code base anywhere this parameter is passed in. There are 2 places I can see where a BranchWSGIApp instance is instantiated with the bzr branch, but none of those call sites pass in the lp branch privacy parameter. So how does 'private' ever get set to anything other than the default = False.

3. Tests

There aren't any. Ah, I see there are no js tests in the loggerhead code base :-/
Should there be a test for the public_private_css() method. Seems trivial I know, so your call whether to add one.

Thanks.

review: Needs Fixing

« Back to merge proposal