Merge lp://staging/~jcsackett/loggerhead/add-privacy-ribbon into lp://staging/loggerhead

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.

1) You got it.

2) We actually spin up loggerhead code in the launchpad code base, using lib/launchpad_loggerhead/app.py, which calls the BranchWSGIApp and passes in private. You can look at lp:~jcsackett/launchpad/private-loggerhead-notifications to see this.

3) I'll add the public_private test; I too am bummed by the lack of JS tests, but I don't have the bandwidth to add js test infrastructure on loggerhead as part of this work. :-/

Thanks for the changes and explanation. I wasn't expecting any js tests, just making an observation on the existing lack of them :-/

NB - the loggerhead/templates/macros.pt template still has the old public_or_private() method (according to the diff). Don't forget to change this prior to landing.

My CSS and Javascript isn't particularly strong, however:
  display_privacy_notification()
calls
  setup_privacy_notification()

however this code:
189 +Y.on('domready', function() {
190 + var body = Y.get('body');
191 + if (body.hasClass('private')) {
192 + setup_privacy_notification();
193 + display_privacy_notification();
194 + }

Calls both directly, seems a bit odd.

Further, I did this change:
=== modified file 'loggerhead/apps/transport.py'
--- loggerhead/apps/transport.py 2011-03-16 14:43:36 +0000
+++ loggerhead/apps/transport.py 2011-11-03 13:22:01 +0000
@@ -63,7 +63,7 @@
             self.root.graph_cache,
             is_root=is_root,
             use_cdn=self._config.get_option('use_cdn'),
- )
+ private=True)
         return branch_app.app

     def app_for_non_branch(self, environ):

And tested locally with "bzr serve --http". I can see that the private attribute is set to True, but the pages themselves are *not* rendering a privacy bar.

Am I missing how you're supposed to be poking a branch to be considered "private"?

> My CSS and Javascript isn't particularly strong, however:
> display_privacy_notification()
> calls
> setup_privacy_notification()
>
> however this code:
> 189 +Y.on('domready', function() {
> 190 + var body = Y.get('body');
> 191 + if (body.hasClass('private')) {
> 192 + setup_privacy_notification();
> 193 + display_privacy_notification();
> 194 + }
>
> Calls both directly, seems a bit odd.

It's an artifact of how this code is/was used in Launchpad. I can remove the redundancy before landing. Good catch.
>
> Further, I did this change:
> === modified file 'loggerhead/apps/transport.py'
> --- loggerhead/apps/transport.py 2011-03-16 14:43:36 +0000
> +++ loggerhead/apps/transport.py 2011-11-03 13:22:01 +0000
> @@ -63,7 +63,7 @@
> self.root.graph_cache,
> is_root=is_root,
> use_cdn=self._config.get_option('use_cdn'),
> - )
> + private=True)
> return branch_app.app
>
> def app_for_non_branch(self, environ):
>
>
> And tested locally with "bzr serve --http". I can see that the private
> attribute is set to True, but the pages themselves are *not* rendering a
> privacy bar.

This was because, as Ian noted, I failed to update the macros.pt template when I renamed the method, so it wasn't setting. This worked with the change I just pushed on my machine. Sorry for missing this, and thanks for taking the time to look this over.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

...

>>
>>
>> And tested locally with "bzr serve --http". I can see that the
>> private attribute is set to True, but the pages themselves are
>> *not* rendering a privacy bar.
>
> This was because, as Ian noted, I failed to update the macros.pt
> template when I renamed the method, so it wasn't setting. This
> worked with the change I just pushed on my machine. Sorry for
> missing this, and thanks for taking the time to look this over.

I can confirm that your current code seems to do the expected thing,
and show the privacy bar.

 merge: approve

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk60R5MACgkQJdeBCYSNAANHkQCg2EeJArOugGqCDohtsLdn3UH/
2ZsAn0+OodrskiPML4BYdkZT4a9jgBvE
=hVrs
-----END PGP SIGNATURE-----