Settings page for analytics ID

Milo: Settings page for analytics ID

Provide a way to add an /admin/settings page to configure a Google
Analytics ID to add to an app.

BUG=632212
Committed: https://github.com/luci/luci-go/commit/6a138f5bafcba6793847787e20a2afe92ccf1a4f

[Ryan Tseng, 2016-08-15 21:49:23]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-15 21:49:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-15 21:54:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-15 21:54:36]

Dry run: Try jobs failed on following builders:
  Luci-go Linux Trusty 64 Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/30a8fff2df8e4610)

[Ryan Tseng, 2016-08-16 23:43:10]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 23:43:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-16 23:53:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-16 23:53:23]

Dry run: This issue passed the CQ dry run.

[Ryan Tseng, 2016-08-17 00:05:18]

Description was changed from

==========
Settings page for analytics ID

BUG=
==========

to

==========
Settings page for analytics ID

BUG=632212
==========

[Ryan Tseng, 2016-08-23 18:26:00]

Rebase

[Ryan Tseng, 2016-08-23 18:26:01]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-23 18:26:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-23 18:36:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-23 18:36:32]

Dry run: This issue passed the CQ dry run.

[estaab, 2016-08-24 16:42:08]

estaab@chromium.org changed reviewers:
+ estaab@chromium.org

[estaab, 2016-08-24 16:42:09]

Not sure if this is ready for review yet, but you may want to include vadimsh
for comments on organization around app settings.

https://codereview.chromium.org/2248893002/diff/40001/server/analytics/doc.go
File server/analytics/doc.go (right):

https://codereview.chromium.org/2248893002/diff/40001/server/analytics/doc.go...
server/analytics/doc.go:5: // Package gaeanalytics provides a standard way to
store the Google Analytics
name here doesn't match actual package name

https://codereview.chromium.org/2248893002/diff/40001/server/analytics/doc.go...
server/analytics/doc.go:8: // Usage Example (TODO): hinoka
Is anything blocking you from doing this now? Seems better just to fix

[Ryan Tseng, 2016-08-26 21:19:29]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-26 21:19:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hinoka, 2016-08-26 21:20:13]

Description was changed from

==========
Settings page for analytics ID

BUG=632212
==========

to

==========
Milo: Settings page for analytics ID

Provide a way to add an /admin/settings page to configure a Google
Analytics ID to add to an app.

BUG=632212
==========

[hinoka, 2016-08-26 21:20:13]

hinoka@chromium.org changed reviewers:
+ vadimsh@chromium.org

[hinoka, 2016-08-26 21:20:27]

hinoka@chromium.org changed reviewers:
+ hinoka@chromium.org

[hinoka, 2016-08-26 21:20:28]

ptal, I think this is the approach we're going with.

[commit-bot: I haz the power, 2016-08-26 21:29:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-26 21:29:09]

Dry run: This issue passed the CQ dry run.

[Vadim Sh., 2016-08-26 21:55:06]

https://codereview.chromium.org/2248893002/diff/60001/appengine/gaemiddleware...
File appengine/gaemiddleware/settings.go (right):

https://codereview.chromium.org/2248893002/diff/60001/appengine/gaemiddleware...
appengine/gaemiddleware/settings.go:38: AnalyticsID string `json:"analytics_id"`
I'm confused, why do you have it in two places? Let's keep only
server/analytics, since it's not related to GAE at all.

https://codereview.chromium.org/2248893002/diff/60001/server/analytics/doc.go
File server/analytics/doc.go (right):

https://codereview.chromium.org/2248893002/diff/60001/server/analytics/doc.go...
server/analytics/doc.go:23: //		   "AnalyticsID": analytics.ID(c),
add also

func Snippet(c context.Context) template.HTML {
  id := ID(c)
  if id == "" {
     return ""
  }
  sanitized := <check id against regexp or properly escape it>
  return template.HTML(fmt.Sprintf(`(function(......`, sanitized))
}

To avoid copy-pasting stuff in HTML templates.

(template.HTML is an alias for string, but it is not auto-escaped when rendered,
see https://golang.org/pkg/html/template/#HTML)

https://codereview.chromium.org/2248893002/diff/60001/server/analytics/settin...
File server/analytics/settings.go (right):

https://codereview.chromium.org/2248893002/diff/60001/server/analytics/settin...
server/analytics/settings.go:83: Title: "Google Analytics Trakcing ID",
Tracking

[Ryan Tseng, 2016-08-26 22:37:50]

Review

[Ryan Tseng, 2016-08-26 22:37:51]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-26 22:38:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-26 22:42:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-26 22:42:32]

Dry run: Try jobs failed on following builders:
  Luci-go Linux Precise 32 Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/30e1d2664a1bc910)
  Luci-go Linux Trusty 64 Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/30e1d2683260cf10)
  Luci-go Mac Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/30e1d264a3c81210)

[Ryan Tseng, 2016-08-26 23:05:09]

Fix tests

[Ryan Tseng, 2016-08-26 23:05:10]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-26 23:05:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ryan Tseng, 2016-08-26 23:09:29]

Fix tests

[Ryan Tseng, 2016-08-26 23:09:30]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-26 23:09:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hinoka, 2016-08-26 23:11:36]

https://codereview.chromium.org/2248893002/diff/60001/appengine/gaemiddleware...
File appengine/gaemiddleware/settings.go (right):

https://codereview.chromium.org/2248893002/diff/60001/appengine/gaemiddleware...
appengine/gaemiddleware/settings.go:38: AnalyticsID string `json:"analytics_id"`
On 2016/08/26 21:55:05, Vadim Sh. wrote:
> I'm confused, why do you have it in two places? Let's keep only
> server/analytics, since it's not related to GAE at all.

This was a mistake (well, vestige), deleted.

https://codereview.chromium.org/2248893002/diff/60001/server/analytics/doc.go
File server/analytics/doc.go (right):

https://codereview.chromium.org/2248893002/diff/60001/server/analytics/doc.go...
server/analytics/doc.go:23: //		   "AnalyticsID": analytics.ID(c),
On 2016/08/26 21:55:05, Vadim Sh. wrote:
> add also
> 
> func Snippet(c context.Context) template.HTML {
>   id := ID(c)
>   if id == "" {
>      return ""
>   }
>   sanitized := <check id against regexp or properly escape it>
>   return template.HTML(fmt.Sprintf(`(function(......`, sanitized))
> }
> 
> To avoid copy-pasting stuff in HTML templates.
> 
> (template.HTML is an alias for string, but it is not auto-escaped when
rendered,
> see https://golang.org/pkg/html/template/#HTML)

Done.

https://codereview.chromium.org/2248893002/diff/60001/server/analytics/settin...
File server/analytics/settings.go (right):

https://codereview.chromium.org/2248893002/diff/60001/server/analytics/settin...
server/analytics/settings.go:83: Title: "Google Analytics Trakcing ID",
On 2016/08/26 21:55:05, Vadim Sh. wrote:
> Tracking

Done.

[Vadim Sh., 2016-08-26 23:12:00]

https://codereview.chromium.org/2248893002/diff/100001/server/analytics/setti...
File server/analytics/settings.go (right):

https://codereview.chromium.org/2248893002/diff/100001/server/analytics/setti...
server/analytics/settings.go:61: func Snippet(c context.Context) template.HTML {
add documenting comment

https://codereview.chromium.org/2248893002/diff/100001/server/analytics/setti...
server/analytics/settings.go:130: modified.AnalyticsID = values["AnalyticsID"]
might as well do validation against regexp here too

[commit-bot: I haz the power, 2016-08-26 23:21:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-26 23:21:28]

Dry run: This issue passed the CQ dry run.

[Ryan Tseng, 2016-08-29 19:16:06]

Review

[Ryan Tseng, 2016-08-29 19:16:07]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-29 19:16:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ryan Tseng, 2016-08-29 19:21:40]

https://codereview.chromium.org/2248893002/diff/100001/server/analytics/setti...
File server/analytics/settings.go (right):

https://codereview.chromium.org/2248893002/diff/100001/server/analytics/setti...
server/analytics/settings.go:61: func Snippet(c context.Context) template.HTML {
On 2016/08/26 23:12:00, Vadim Sh. wrote:
> add documenting comment

Done.

https://codereview.chromium.org/2248893002/diff/100001/server/analytics/setti...
server/analytics/settings.go:130: modified.AnalyticsID = values["AnalyticsID"]
On 2016/08/26 23:12:00, Vadim Sh. wrote:
> might as well do validation against regexp here too

Done.

[commit-bot: I haz the power, 2016-08-29 19:25:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-29 19:25:44]

Dry run: This issue passed the CQ dry run.

[Vadim Sh., 2016-08-29 19:26:32]

lgtm with one final nit

https://codereview.chromium.org/2248893002/diff/140001/server/analytics/setti...
File server/analytics/settings.go (right):

https://codereview.chromium.org/2248893002/diff/140001/server/analytics/setti...
server/analytics/settings.go:54: // ID returns the Google Analytics ID if it's
set, and "" otherwise.
nit: move ID() and Snippet() into analytics/analytics.go, so that settings.go
implements settings only.

[Ryan Tseng, 2016-08-29 19:34:19]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-29 19:34:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ryan Tseng, 2016-08-29 19:36:01]

https://codereview.chromium.org/2248893002/diff/140001/server/analytics/setti...
File server/analytics/settings.go (right):

https://codereview.chromium.org/2248893002/diff/140001/server/analytics/setti...
server/analytics/settings.go:54: // ID returns the Google Analytics ID if it's
set, and "" otherwise.
On 2016/08/29 19:26:32, Vadim Sh. wrote:
> nit: move ID() and Snippet() into analytics/analytics.go, so that settings.go
> implements settings only.

Done.

[Ryan Tseng, 2016-08-29 19:36:08]

The CQ bit was unchecked by hinoka@google.com

[Ryan Tseng, 2016-08-29 19:36:53]

The CQ bit was checked by hinoka@google.com

[Ryan Tseng, 2016-08-29 19:36:54]

The patchset sent to the CQ was uploaded after l-g-t-m from vadimsh@chromium.org
Link to the patchset: https://codereview.chromium.org/2248893002/#ps160001
(title: "y")

[commit-bot: I haz the power, 2016-08-29 19:37:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-29 19:47:34]

Description was changed from

==========
Milo: Settings page for analytics ID

Provide a way to add an /admin/settings page to configure a Google
Analytics ID to add to an app.

BUG=632212
==========

to

==========
Milo: Settings page for analytics ID

Provide a way to add an /admin/settings page to configure a Google
Analytics ID to add to an app.

BUG=632212

Committed:
https://github.com/luci/luci-go/commit/6a138f5bafcba6793847787e20a2afe92ccf1a4f
==========

[commit-bot: I haz the power, 2016-08-29 19:47:35]

Committed patchset #9 (id:160001) as
https://github.com/luci/luci-go/commit/6a138f5bafcba6793847787e20a2afe92ccf1a4f