loggerhead

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

  1. add-privacy-ribbon
  2. Merge into trunk-rich
Proposed by j.c.sackett
Status: Merged
Approved by: John A Meinel
Approved revision: 465
Merged at revision: 459
Proposed branch: lp:~jcsackett/loggerhead/add-privacy-ribbon
Merge into: lp:loggerhead
Diff against target: 236 lines (+165/-5)
5 files modified
loggerhead/apps/branch.py (+8/-1)
loggerhead/static/css/global.css (+61/-0)
loggerhead/static/javascript/custom.js (+87/-1)
loggerhead/templates/macros.pt (+1/-1)
loggerhead/tests/test_simple.py (+8/-2)
To merge this branch: bzr merge lp:~jcsackett/loggerhead/add-privacy-ribbon
Reviewer Review Type Date Requested Status
John A Meinel Approve
Ian Booth (community) code Approve
Review via email: mp+80477@code.launchpad.net

Commit message

Adds a privacy ribbon display to private branches in loggerhead.

Description of the change

Summary
=======
This branch adds an option privacy ribbon to be displayed when a branch's contents are private. It does so by adding javascript, css, and template changes to emulate the launchpad privacy ribbon.

Preimp
======
Spoke with Curtis Hovey

Implementation
==============
* Privacy functions from launchpad were added to custom.js; they have been modified to work with the YUI modules already existing in loggerhead.
* The global notification CSS has been added to the global.css for loggerhead.
* BranchWSGIApp has been updated to accept a private param, defaulting to False. It also has a method to provide the public or private class for the body element on the base layout.
* The base template for loggerhead sets the body class to "private" or "public" based on the above parameter.

QA
==
* Open a private branch on loggerhead; you should see the privacy ribbon.
* Open a public branch on loggerhead; you should not see the ribbon.

To post a comment you must log in.
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
Revision history for this message
j.c.sackett (jcsackett) wrote :

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. :-/

lp:~jcsackett/loggerhead/add-privacy-ribbon updated
462. By j.c.sackett

public_or_private renamed public_private_css and tests added.

463. By j.c.sackett

Fixed test.

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

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.

review: Approve (code)
Revision history for this message
John A Meinel (jameinel) wrote :

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"?

review: Needs Fixing
lp:~jcsackett/loggerhead/add-privacy-ribbon updated
464. By j.c.sackett

Fixed template to use the updated public/private thing.

465. By j.c.sackett

Updated from trunk.

Revision history for this message
j.c.sackett (jcsackett) wrote :

> 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.

Revision history for this message
John A Meinel (jameinel) wrote :

-----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-----

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'loggerhead/apps/branch.py'
2--- loggerhead/apps/branch.py 2011-11-02 01:23:00 +0000
3+++ loggerhead/apps/branch.py 2011-11-04 18:21:25 +0000
4@@ -50,7 +50,7 @@
5
6 def __init__(self, branch, friendly_name=None, config={},
7 graph_cache=None, branch_link=None, is_root=False,
8- served_url=_DEFAULT, use_cdn=False):
9+ served_url=_DEFAULT, use_cdn=False, private=False):
10 self.branch = branch
11 self._config = config
12 self.friendly_name = friendly_name
13@@ -62,6 +62,13 @@
14 self.is_root = is_root
15 self.served_url = served_url
16 self.use_cdn = use_cdn
17+ self.private = private
18+
19+ def public_private_css(self):
20+ if self.private:
21+ return "private"
22+ else:
23+ return "public"
24
25 def get_history(self):
26 revinfo_disk_cache = None
27
28=== modified file 'loggerhead/static/css/global.css'
29--- loggerhead/static/css/global.css 2011-04-22 17:22:49 +0000
30+++ loggerhead/static/css/global.css 2011-11-04 18:21:25 +0000
31@@ -343,3 +343,64 @@
32 #footer {
33 font-size: 77%;
34 }
35+
36+
37+/* ====================
38+ Global notifications
39+*/
40+/* Move the content down so the notification banner doesn't hide any content. */
41+body.global-notification-visible {
42+ padding-top: 40px;
43+ }
44+body.global-notification-visible .login-logout {
45+ top: 45px;
46+ }
47+.global-notification {
48+ position: fixed;
49+ z-index: 10;
50+ top: 0;
51+ left: 0;
52+ right: 0;
53+ padding: 8px 20px;
54+ /* Define colour for browsers that don't support transparency */
55+ background-color: #8d1f1f;
56+ /* Set transparent background for browsers that support it */
57+ background-color: rgba(125,0,0,0.9);
58+ color: #fff;
59+ text-shadow: 0 -1px 0 #631616;
60+ font-size: 14px;
61+ line-height: 21px;
62+ font-weight: bold;
63+ -moz-box-shadow: 0 0 5px #333;
64+ -webkit-box-shadow: 0 0 5px #333;
65+ box-shadow: 0 0 5px #333;
66+ }
67+.global-notification .sprite.notification-private {
68+ float: left;
69+ display: inline-block;
70+ height: 21px;
71+ width: 20px;
72+ margin-right: 10px;
73+ padding: 0;
74+ }
75+.global-notification-close, .global-notification-close:active,
76+.global-notification-close:visited {
77+ color: #e47a7a;
78+ }
79+.global-notification-close {
80+ display: block;
81+ position: absolute;
82+ top: 11px;
83+ right: 20px;
84+ font-size: 12px;
85+ font-weight: normal;
86+ line-height: 14px;
87+ }
88+.global-notification-close .sprite.notification-close {
89+ float: right;
90+ display: block;
91+ height: 9px;
92+ width: 8px;
93+ margin: 3px 0 0 7px;
94+ padding: 0;
95+ }
96
97=== modified file 'loggerhead/static/javascript/custom.js'
98--- loggerhead/static/javascript/custom.js 2009-04-01 15:28:21 +0000
99+++ loggerhead/static/javascript/custom.js 2011-11-04 18:21:25 +0000
100@@ -1,4 +1,4 @@
101-Y = YUI().use("node", "io-base", "anim");
102+Y = YUI().use("base", "node", "io-base", "anim");
103
104 var global_timeout_id = null;
105 var global_search_request = null;
106@@ -280,3 +280,89 @@
107 }
108 };
109
110+var notification_node = null;
111+/*
112+ * Display privacy notifications
113+ *
114+ * This should be called after the page has loaded e.g. on 'domready'.
115+ */
116+function setup_privacy_notification(config) {
117+ if (notification_node !== null) {
118+ return;
119+ }
120+ var notification_text = 'The information on this page is private';
121+ var hidden = true;
122+ var target_id = "loggerheadCont";
123+ if (config !== undefined) {
124+ if (config.notification_text !== undefined) {
125+ notification_text = config.notification_text;
126+ }
127+ if (config.hidden !== undefined) {
128+ hidden = config.hidden;
129+ }
130+ if (config.target_id !== undefined) {
131+ target_id = config.target_id;
132+ }
133+ }
134+ var id_selector = "#" + target_id;
135+ var main = Y.get(id_selector);
136+ notification_node = Y.Node.create('<div></div>')
137+ .addClass('global-notification');
138+ if (hidden) {
139+ notification_node.addClass('hidden');
140+ }
141+ var notification_span = Y.Node.create('<span></span>')
142+ .addClass('sprite')
143+ .addClass('notification-private');
144+ notification_node.set('innerHTML', notification_text);
145+ main.appendChild(notification_node);
146+ notification_node.appendChild(notification_span);
147+};
148+
149+function display_privacy_notification() {
150+ /* Set a temporary class on the body for the feature flag,
151+ this is because we have no way to use feature flags in
152+ css directly. This should be removed if the feature
153+ is accepted. */
154+ var body = Y.get('body');
155+ body.addClass('feature-flag-bugs-private-notification-enabled');
156+ // Set the visible flag so that the content moves down.
157+ body.addClass('global-notification-visible');
158+
159+ setup_privacy_notification();
160+ var global_notification = Y.get('.global-notification');
161+ if (global_notification.hasClass('hidden')) {
162+ global_notification.addClass('transparent');
163+ global_notification.removeClass('hidden');
164+
165+ var fade_in = new Y.Anim({
166+ node: global_notification,
167+ to: {opacity: 1},
168+ duration: 0.3
169+ });
170+ var body_space = new Y.Anim({
171+ node: 'body',
172+ to: {'paddingTop': '40px'},
173+ duration: 0.2,
174+ easing: Y.Easing.easeOut
175+ });
176+ var black_link_space = new Y.Anim({
177+ node: '.black-link',
178+ to: {'top': '45px'},
179+ duration: 0.2,
180+ easing: Y.Easing.easeOut
181+ });
182+
183+ fade_in.run();
184+ body_space.run();
185+ black_link_space.run();
186+ }
187+};
188+
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+ }
195+});
196
197=== modified file 'loggerhead/templates/macros.pt'
198--- loggerhead/templates/macros.pt 2011-03-03 10:58:57 +0000
199+++ loggerhead/templates/macros.pt 2011-11-04 18:21:25 +0000
200@@ -43,7 +43,7 @@
201 <metal:block metal:define-slot="header_extras" />
202 </head>
203
204-<body>
205+ <body tal:attributes="class python:branch.public_private_css()">
206 <!-- Loggerhead Content Area -->
207
208 <div metal:define-slot="backlink" />
209
210=== modified file 'loggerhead/tests/test_simple.py'
211--- loggerhead/tests/test_simple.py 2011-09-08 00:33:28 +0000
212+++ loggerhead/tests/test_simple.py 2011-11-04 18:21:25 +0000
213@@ -56,8 +56,8 @@
214 self.assertEqual(None, start[2])
215 simplejson.loads(content)
216
217- def make_branch_app(self, branch):
218- branch_app = BranchWSGIApp(branch, friendly_name='friendly-name')
219+ def make_branch_app(self, branch, **kw):
220+ branch_app = BranchWSGIApp(branch, friendly_name='friendly-name', **kw)
221 branch_app._environ = {
222 'wsgi.url_scheme':'',
223 'SERVER_NAME':'',
224@@ -84,6 +84,12 @@
225 self.msg = 'a very exciting commit message <'
226 self.revid = self.tree.commit(message=self.msg)
227
228+ def test_public_private(self):
229+ app = self.make_branch_app(self.tree.branch, private=True)
230+ self.assertEqual(app.public_private_css(), 'private')
231+ app = self.make_branch_app(self.tree.branch)
232+ self.assertEqual(app.public_private_css(), 'public')
233+
234 def test_changes(self):
235 app = self.setUpLoggerhead()
236 res = app.get('/changes')

Subscribers

People subscribed via source and target branches