A cookies warning handler, enabled by analytics.

A cookies warning handler, enabled by analytics.

When "useAnalytics" is set to "true", we use a cookie to record user
agreement to the usage of cookies. :-)

A banner will be displayed asking for cookie usage agreement,
and the cookie will be set on user dismissal of said banner.

https://code.launchpad.net/~teknico/juju-gui/add-cookies-usage-banner/+merge/168495

(do not edit description out of merge proposal)

[teknico, 2013-06-10 17:03:32]

Please take a look.

[jeff.pihach, 2013-06-10 17:24:53]

Thanks for this! However, I feel like this could be done in a more efficient
manner. Let me know if you would like some help with my comments.

https://codereview.appspot.com/9967044/diff/1/app/app.js
File app/app.js (right):

https://codereview.appspot.com/9967044/diff/1/app/app.js#newcode1212
app/app.js:1212: * @param {Object} res ???
res is the router response object.

https://codereview.appspot.com/9967044/diff/1/app/app.js#newcode1216
app/app.js:1216: check_cookies: function(req, res, next) {
camelCase

https://codereview.appspot.com/9967044/diff/1/app/app.js#newcode1219
app/app.js:1219: this.cookieChecker = this.cookieChecker || new
Y.juju.Cookies();
This should be documented as an instance level property on our app class and
should be init'd in the initializer,

/**
  Reference to the juju.Cookies instance

  @property cookieChecker
  @type {juju.Cookies}
  @default null
*/
this.cookieChecker = null;

https://codereview.appspot.com/9967044/diff/1/app/app.js#newcode1266
app/app.js:1266: { path: '*', callbacks: 'check_cookies'},
camelCase

https://codereview.appspot.com/9967044/diff/1/app/config-prod.js
File app/config-prod.js (right):

https://codereview.appspot.com/9967044/diff/1/app/config-prod.js#newcode54
app/config-prod.js:54: useAnalytics: true
Weren't we setting this to false by default?

https://codereview.appspot.com/9967044/diff/1/app/views/cookies.js
File app/views/cookies.js (right):

https://codereview.appspot.com/9967044/diff/1/app/views/cookies.js#newcode31
app/views/cookies.js:31: juju.Cookies = Y.Base.create('Cookies', Y.Base, [], {
This would make more sense as a manually rendered Y.View or as a utility class
as being Y.Base derived doesn't really gain us anything here.

https://codereview.appspot.com/9967044/diff/1/app/views/cookies.js#newcode33
app/views/cookies.js:33: warning: (
This should be moved into a template file

https://codereview.appspot.com/9967044/diff/1/app/views/cookies.js#newcode36
app/views/cookies.js:36: '<a href="javascript:void(0)"
class="link-cta">Close</a>' +
javascript:void not needed when preventingDefault()

https://codereview.appspot.com/9967044/diff/1/app/views/cookies.js#newcode60
app/views/cookies.js:60: Y.one('.cookie-policy .link-cta').on('click',
function(evt) {
This event needs to be detached when the element goes away.

https://codereview.appspot.com/9967044/diff/1/app/views/cookies.js#newcode74
app/views/cookies.js:74: Y.one('.cookie-policy').setStyle('display', 'none');
Why are we keeping this in the DOM after they have dismissed it as there is no
way to make it visible again?

https://codereview.appspot.com/9967044/diff/1/lib/views/cookies.less
File lib/views/cookies.less (right):

https://codereview.appspot.com/9967044/diff/1/lib/views/cookies.less#newcode1
lib/views/cookies.less:1: .cookie-policy {
please convert to LESS

https://codereview.appspot.com/9967044/diff/1/test/test_cookies.js
File test/test_cookies.js (right):

https://codereview.appspot.com/9967044/diff/1/test/test_cookies.js#newcode36
test/test_cookies.js:36: checker = new juju.Cookies();
instance isn't destroyed after each test

https://codereview.appspot.com/9967044/diff/1/test/test_cookies.js#newcode50
test/test_cookies.js:50: Y.one('.cookie-policy').setStyle('visibility',
'hidden');
Is this to avoid the flash in the browser tests? If so the cookie class should
probably be configurable to pass in a container to render into.

[bac, 2013-06-10 20:42:58]

I tried to QA this branch but could not get the banner to show even after
changing useAnalytics to true.  Not sure if it was PEBCAK or a problem with the
branch.

Since I'll be gone I abstain but wanted my comments to be noted.

https://codereview.appspot.com/9967044/diff/1/app/app.js
File app/app.js (right):

https://codereview.appspot.com/9967044/diff/1/app/app.js#newcode1210
app/app.js:1210: * @method check_cookies
camelCase -- Ha!  Missed one Hatch!

https://codereview.appspot.com/9967044/diff/1/app/config-debug.js
File app/config-debug.js (right):

https://codereview.appspot.com/9967044/diff/1/app/config-debug.js#newcode53
app/config-debug.js:53: useAnalytics: true
The default is being set to false in my branch.  Be sure to keep false when you
merge.

https://codereview.appspot.com/9967044/diff/1/app/config-prod.js
File app/config-prod.js (right):

https://codereview.appspot.com/9967044/diff/1/app/config-prod.js#newcode54
app/config-prod.js:54: useAnalytics: true
Yes, prod is false for now.  I think debug should always be false.

https://codereview.appspot.com/9967044/diff/1/lib/views/cookies.less
File lib/views/cookies.less (right):

https://codereview.appspot.com/9967044/diff/1/lib/views/cookies.less#newcode2
lib/views/cookies.less:2: z-index: 100;
I'm -0 on converting to LESS at this point.  This is contributed code (though in
the main part of our tree I admit) so I'd rather see as few changes as possible
in case we want to stay in sync with the styling of ubuntu.com.

https://codereview.appspot.com/9967044/diff/1/test/test_cookies.js
File test/test_cookies.js (right):

https://codereview.appspot.com/9967044/diff/1/test/test_cookies.js#newcode36
test/test_cookies.js:36: checker = new juju.Cookies();
Use of 'checker' is confusing.  'cookies' might be a more readable alternative,
then you'd have 'cookies.check()'

[teknico, 2013-06-11 11:16:49]

Thanks for you comments, Brad and Jeff.

Brad, I resolved all your points except the LESS one, to be discussed.

Jeff, I'll coordinate with you later to fix the few unresolved issues.

https://codereview.appspot.com/9967044/diff/1/app/app.js
File app/app.js (right):

https://codereview.appspot.com/9967044/diff/1/app/app.js#newcode1210
app/app.js:1210: * @method check_cookies
bac wrote:
> camelCase -- Ha!  Missed one Hatch!

I was under the delusion that we were going to use lowercase+underscore and not
camelCase anymore. I will add our preference to the style guide later.

Nonetheless, I managed to use both styles in one branch. Converted everything to
camelCase.

https://codereview.appspot.com/9967044/diff/1/app/app.js#newcode1212
app/app.js:1212: * @param {Object} res ???
jeff.pihach wrote:
> res is the router response object.

Thank you.

https://codereview.appspot.com/9967044/diff/1/app/app.js#newcode1216
app/app.js:1216: check_cookies: function(req, res, next) {
jeff.pihach wrote:
> camelCase

Done.

https://codereview.appspot.com/9967044/diff/1/app/app.js#newcode1219
app/app.js:1219: this.cookieChecker = this.cookieChecker || new
Y.juju.Cookies();
jeff.pihach wrote:
> This should be documented as an instance level property on our app
> class and should be init'd in the initializer,

Right, thank you. Done.

https://codereview.appspot.com/9967044/diff/1/app/app.js#newcode1266
app/app.js:1266: { path: '*', callbacks: 'check_cookies'},
jeff.pihach wrote:
> camelCase

Done.

https://codereview.appspot.com/9967044/diff/1/app/config-debug.js
File app/config-debug.js (right):

https://codereview.appspot.com/9967044/diff/1/app/config-debug.js#newcode53
app/config-debug.js:53: useAnalytics: true
bac wrote:
> The default is being set to false in my branch.  Be sure to keep
> false when you merge.

Indeed. I also kept both your and my comments.

https://codereview.appspot.com/9967044/diff/1/app/config-prod.js
File app/config-prod.js (right):

https://codereview.appspot.com/9967044/diff/1/app/config-prod.js#newcode54
app/config-prod.js:54: useAnalytics: true
bac wrote:
> Yes, prod is false for now.  I think debug should always be false.

Done.

https://codereview.appspot.com/9967044/diff/1/app/views/cookies.js
File app/views/cookies.js (right):

https://codereview.appspot.com/9967044/diff/1/app/views/cookies.js#newcode31
app/views/cookies.js:31: juju.Cookies = Y.Base.create('Cookies', Y.Base, [], {
jeff.pihach wrote:
> This would make more sense as a manually rendered Y.View or as a utility
> class as being Y.Base derived doesn't really gain us anything here.

I had that suspicion myself. I'll ask you about the best way to do this later.

https://codereview.appspot.com/9967044/diff/1/app/views/cookies.js#newcode33
app/views/cookies.js:33: warning: (
jeff.pihach wrote:
> This should be moved into a template file

Same as above. However, note that it's static, no value to interpolate.

https://codereview.appspot.com/9967044/diff/1/app/views/cookies.js#newcode36
app/views/cookies.js:36: '<a href="javascript:void(0)"
class="link-cta">Close</a>' +
jeff.pihach wrote:
> javascript:void not needed when preventingDefault()

Substituted with "#".

https://codereview.appspot.com/9967044/diff/1/app/views/cookies.js#newcode60
app/views/cookies.js:60: Y.one('.cookie-policy .link-cta').on('click',
function(evt) {
jeff.pihach wrote:
> This event needs to be detached when the element goes away.

Indeed.

https://codereview.appspot.com/9967044/diff/1/app/views/cookies.js#newcode74
app/views/cookies.js:74: Y.one('.cookie-policy').setStyle('display', 'none');
jeff.pihach wrote:
> Why are we keeping this in the DOM after they have dismissed it
> as there is no way to make it visible again?

That's how the code we were given does it, but we can improve on it. :-)

https://codereview.appspot.com/9967044/diff/1/lib/views/cookies.less
File lib/views/cookies.less (right):

https://codereview.appspot.com/9967044/diff/1/lib/views/cookies.less#newcode2
lib/views/cookies.less:2: z-index: 100;
bac wrote:
> I'm -0 on converting to LESS at this point.

It looks like this warrants some more discussion.

https://codereview.appspot.com/9967044/diff/1/test/test_cookies.js
File test/test_cookies.js (right):

https://codereview.appspot.com/9967044/diff/1/test/test_cookies.js#newcode36
test/test_cookies.js:36: checker = new juju.Cookies();
jeff.pihach wrote:
> instance isn't destroyed after each test

Indeed, done.

https://codereview.appspot.com/9967044/diff/1/test/test_cookies.js#newcode36
test/test_cookies.js:36: checker = new juju.Cookies();
bac wrote:
> Use of 'checker' is confusing.  'cookies' might be a more readable
> alternative, then you'd have 'cookies.check()'

Right. Calling it "cookieHandler", as in main code.

https://codereview.appspot.com/9967044/diff/1/test/test_cookies.js#newcode50
test/test_cookies.js:50: Y.one('.cookie-policy').setStyle('visibility',
'hidden');
jeff.pihach wrote:
> Is this to avoid the flash in the browser tests?

The idea is to avoid the banner text to be visible. A slight flicker is still
there.

> If so the cookie class should probably be configurable
> to pass in a container to render into.

This also does away with the flicker. Thank you.

[teknico, 2013-06-12 17:05:42]

Please take a look.

[jeff.pihach, 2013-06-12 17:27:32]

LGTM - I have a few very small improvement comments below but overall thanks for
the great work with this changeover!

https://codereview.appspot.com/9967044/diff/12001/app/app.js
File app/app.js (right):

https://codereview.appspot.com/9967044/diff/12001/app/app.js#newcode1225
app/app.js:1225: handleCookies: function(req, res, next) {
A more descriptive name here would probably be "authorizeCookieUse"

https://codereview.appspot.com/9967044/diff/12001/app/assets/javascripts/app-...
File app/assets/javascripts/app-cookies-extension.js (right):

https://codereview.appspot.com/9967044/diff/12001/app/assets/javascripts/app-...
app/assets/javascripts/app-cookies-extension.js:25: 
Add some documentation about the configuration properties being set re the
testing procedure.

https://codereview.appspot.com/9967044/diff/12001/app/assets/javascripts/app-...
app/assets/javascripts/app-cookies-extension.js:46: Y.one('.cookie-policy
.link-cta').once('click', function(evt) {
This should probably be:

this.node.one('.link-cta').once ...

https://codereview.appspot.com/9967044/diff/12001/app/index.html
File app/index.html (right):

https://codereview.appspot.com/9967044/diff/12001/app/index.html#newcode114
app/index.html:114: continued use of this site you accept such use.
I wonder if we should change 'site' to 'application'

https://codereview.appspot.com/9967044/diff/12001/test/test_cookies_app_exten...
File test/test_cookies_app_extension.js (right):

https://codereview.appspot.com/9967044/diff/12001/test/test_cookies_app_exten...
test/test_cookies_app_extension.js:52: assert.equal(display, 'block');
This should check that the display is none pre check() call.

https://codereview.appspot.com/9967044/diff/12001/test/test_cookies_app_exten...
test/test_cookies_app_extension.js:59: assert.equal(cookie, 'true');
assert that cookie is false pre check()

[teknico, 2013-06-12 17:50:27]

All done. Thank you again for your help.

https://codereview.appspot.com/9967044/diff/12001/app/app.js
File app/app.js (right):

https://codereview.appspot.com/9967044/diff/12001/app/app.js#newcode1225
app/app.js:1225: handleCookies: function(req, res, next) {
On 2013/06/12 17:27:32, jeff.pihach wrote:
> A more descriptive name here would probably be "authorizeCookieUse"

Done.

https://codereview.appspot.com/9967044/diff/12001/app/assets/javascripts/app-...
File app/assets/javascripts/app-cookies-extension.js (right):

https://codereview.appspot.com/9967044/diff/12001/app/assets/javascripts/app-...
app/assets/javascripts/app-cookies-extension.js:25: 
On 2013/06/12 17:27:32, jeff.pihach wrote:
> Add some documentation about the configuration properties being set re the
> testing procedure.

Done.

https://codereview.appspot.com/9967044/diff/12001/app/assets/javascripts/app-...
app/assets/javascripts/app-cookies-extension.js:46: Y.one('.cookie-policy
.link-cta').once('click', function(evt) {
On 2013/06/12 17:27:32, jeff.pihach wrote:
> This should probably be:
> 
> this.node.one('.link-cta').once ...

Done.

https://codereview.appspot.com/9967044/diff/12001/app/index.html
File app/index.html (right):

https://codereview.appspot.com/9967044/diff/12001/app/index.html#newcode114
app/index.html:114: continued use of this site you accept such use.
On 2013/06/12 17:27:32, jeff.pihach wrote:
> I wonder if we should change 'site' to 'application'

Done.

https://codereview.appspot.com/9967044/diff/12001/test/test_cookies_app_exten...
File test/test_cookies_app_extension.js (right):

https://codereview.appspot.com/9967044/diff/12001/test/test_cookies_app_exten...
test/test_cookies_app_extension.js:52: assert.equal(display, 'block');
On 2013/06/12 17:27:32, jeff.pihach wrote:
> This should check that the display is none pre check() call.

Done.

https://codereview.appspot.com/9967044/diff/12001/test/test_cookies_app_exten...
test/test_cookies_app_extension.js:59: assert.equal(cookie, 'true');
On 2013/06/12 17:27:32, jeff.pihach wrote:
> assert that cookie is false pre check()

Done.

[teknico, 2013-06-12 18:04:14]

*** Submitted:

A cookies warning handler, enabled by analytics.

When "useAnalytics" is set to "true", we use a cookie to record user
agreement to the usage of cookies. :-)

A banner will be displayed asking for cookie usage agreement,
and the cookie will be set on user dismissal of said banner.

R=jeff.pihach, bac
CC=
https://codereview.appspot.com/9967044