config_service: Added revision and timestamp to config-set-cards and config-set pages

config_service: Added revision and timestamp to config-set-cards and config-set pages

I refactored the code to avoid repitition by creating a common-behaviors.html file as
well.

BUG=749540
Review-Url: https://codereview.chromium.org/2991013002
Committed: https://github.com/luci/luci-py/commit/acf054e95ab4e943dadcab133bde9a8e5d5c5a97

[cwpayton, 2017-07-28 19:45:42]

cwpayton@google.com changed reviewers:
+ hinoka@chromium.org, nodir@chromium.org, sergeyberezin@chromium.org

[cwpayton, 2017-07-28 19:45:43]

PTAL

[cwpayton, 2017-07-28 21:15:22]

Link to staging version here:
https://3003-5925b54-tainted-cwpayton-dot-luci-config.appspot.com

[Sergey Berezin, 2017-07-31 20:21:19]

Overall this is good; some polishing comments.

https://codereview.chromium.org/2991013002/diff/40001/appengine/config_servic...
File appengine/config_service/ui/common/common-behaviors.html (right):

https://codereview.chromium.org/2991013002/diff/40001/appengine/config_servic...
appengine/config_service/ui/common/common-behaviors.html:23:
ConfigUIBehaviors.CommonBehavior = {
Could you add unittests for these functions?

https://codereview.chromium.org/2991013002/diff/40001/appengine/config_servic...
appengine/config_service/ui/common/common-behaviors.html:44: " " + hours + ":" +
minutes;
Why not just use date.toString()? In particular, it prints the timezone (local
by default), so the timestamp is not ambiguous.

Also consider formatting it as "x minutes/hours/days/... ago" with a tooltip
showing the full timestamp (see comments in other files).

https://codereview.chromium.org/2991013002/diff/40001/appengine/config_servic...
File appengine/config_service/ui/src/config-ui/config-set-card.html (right):

https://codereview.chromium.org/2991013002/diff/40001/appengine/config_servic...
appengine/config_service/ui/src/config-ui/config-set-card.html:63:
on-tap="_openConfig">
This correctly opens a new window, but also opens the original page on the
config set (probably because it's click on the entire card). I'd expect the
original page to stay unchanged.

Perhaps, we should change the clickable surface inside the card to exclude the
external link.

https://codereview.chromium.org/2991013002/diff/40001/appengine/config_servic...
appengine/config_service/ui/src/config-ui/config-set-card.html:99: <p>Timestamp:
[[timestamp]]</p>
I'd make this more compact:
- Print only the fist 7 digits of the revision (similar to git short hash)
- Place the date on the same line, possibly shortened e.g. to "x
min/hours/days/.. ago" with a bubble showing the actual date, similar to how
Rietveld and Monorail do it.

In fact, if the revision / date fit on the same line as the project title, I'd
try doing that too. It should be possible to let HTML format it dynamically if
the card is wide enough, and otherwise wrap it to the next line.

https://codereview.chromium.org/2991013002/diff/40001/appengine/config_servic...
File appengine/config_service/ui/src/config-ui/config-set.html (right):

https://codereview.chromium.org/2991013002/diff/40001/appengine/config_servic...
appengine/config_service/ui/src/config-ui/config-set.html:144: <p>Revision:
[[_getRevision(revision)]]</p>
I'd try to fit it all in one line, see comment in config-set-card.html. The
timestamp can remain absolute though, since we have more space here.

[cwpayton, 2017-07-31 23:40:53]

PTAL. Staging link is still the same.

https://codereview.chromium.org/2991013002/diff/40001/appengine/config_servic...
File appengine/config_service/ui/common/common-behaviors.html (right):

https://codereview.chromium.org/2991013002/diff/40001/appengine/config_servic...
appengine/config_service/ui/common/common-behaviors.html:23:
ConfigUIBehaviors.CommonBehavior = {
On 2017/07/31 20:21:18, Sergey Berezin wrote:
> Could you add unittests for these functions?

Done.

https://codereview.chromium.org/2991013002/diff/40001/appengine/config_servic...
appengine/config_service/ui/common/common-behaviors.html:44: " " + hours + ":" +
minutes;
On 2017/07/31 20:21:18, Sergey Berezin wrote:
> Why not just use date.toString()? In particular, it prints the timezone (local
> by default), so the timestamp is not ambiguous.
> 
> Also consider formatting it as "x minutes/hours/days/... ago" with a tooltip
> showing the full timestamp (see comments in other files).

Done.

https://codereview.chromium.org/2991013002/diff/40001/appengine/config_servic...
File appengine/config_service/ui/src/config-ui/config-set-card.html (right):

https://codereview.chromium.org/2991013002/diff/40001/appengine/config_servic...
appengine/config_service/ui/src/config-ui/config-set-card.html:63:
on-tap="_openConfig">
On 2017/07/31 20:21:18, Sergey Berezin wrote:
> This correctly opens a new window, but also opens the original page on the
> config set (probably because it's click on the entire card). I'd expect the
> original page to stay unchanged.
> 
> Perhaps, we should change the clickable surface inside the card to exclude the
> external link.

Done.

https://codereview.chromium.org/2991013002/diff/40001/appengine/config_servic...
appengine/config_service/ui/src/config-ui/config-set-card.html:99: <p>Timestamp:
[[timestamp]]</p>
On 2017/07/31 20:21:18, Sergey Berezin wrote:
> I'd make this more compact:
> - Print only the fist 7 digits of the revision (similar to git short hash)
> - Place the date on the same line, possibly shortened e.g. to "x
> min/hours/days/.. ago" with a bubble showing the actual date, similar to how
> Rietveld and Monorail do it.
> 
> In fact, if the revision / date fit on the same line as the project title, I'd
> try doing that too. It should be possible to let HTML format it dynamically if
> the card is wide enough, and otherwise wrap it to the next line.

Done.

https://codereview.chromium.org/2991013002/diff/40001/appengine/config_servic...
File appengine/config_service/ui/src/config-ui/config-set.html (right):

https://codereview.chromium.org/2991013002/diff/40001/appengine/config_servic...
appengine/config_service/ui/src/config-ui/config-set.html:144: <p>Revision:
[[_getRevision(revision)]]</p>
On 2017/07/31 20:21:18, Sergey Berezin wrote:
> I'd try to fit it all in one line, see comment in config-set-card.html. The
> timestamp can remain absolute though, since we have more space here.

Done.

[Sergey Berezin, 2017-08-01 00:28:39]

Missing the test file, otherwise the code looks good!

Make sure to rebase your change on top of the latest changes; in particular, I
believe the "reload" icon should've changed now.

https://codereview.chromium.org/2991013002/diff/60001/appengine/config_servic...
File appengine/config_service/ui/test/index.html (right):

https://codereview.chromium.org/2991013002/diff/60001/appengine/config_servic...
appengine/config_service/ui/test/index.html:27:
'common/common-behaviors_test.html?dom=shadow',
This file is missing from the CL, please add and upload.

[cwpayton, 2017-08-01 16:09:00]

PTAL. Added the test file for common-behaviors

https://codereview.chromium.org/2991013002/diff/60001/appengine/config_servic...
File appengine/config_service/ui/test/index.html (right):

https://codereview.chromium.org/2991013002/diff/60001/appengine/config_servic...
appengine/config_service/ui/test/index.html:27:
'common/common-behaviors_test.html?dom=shadow',
On 2017/08/01 00:28:39, Sergey Berezin wrote:
> This file is missing from the CL, please add and upload.

Done.

[Sergey Berezin, 2017-08-01 18:51:11]

LGTM + a couple of more suggestions. Thanks!

https://codereview.chromium.org/2991013002/diff/80001/appengine/config_servic...
File appengine/config_service/ui/test/common/common-behaviors_test.html (right):

https://codereview.chromium.org/2991013002/diff/80001/appengine/config_servic...
appengine/config_service/ui/test/common/common-behaviors_test.html:31:
suite('unit test each function in common-behaviors.html', function() {
Suggestion: create a separate suite for each method. Then you can name it e.g.

suite('_formatDate', ... {
  test('returns the proper amount of time passed')
...
}
etc.

https://codereview.chromium.org/2991013002/diff/80001/appengine/config_servic...
appengine/config_service/ui/test/common/common-behaviors_test.html:34: common =
fixture('common-behaviorsTestFixture');
Is it needed? The methods under test don't access DOM, I don't think they need a
fixture.

[cwpayton, 2017-08-01 19:18:49]

https://codereview.chromium.org/2991013002/diff/80001/appengine/config_servic...
File appengine/config_service/ui/test/common/common-behaviors_test.html (right):

https://codereview.chromium.org/2991013002/diff/80001/appengine/config_servic...
appengine/config_service/ui/test/common/common-behaviors_test.html:31:
suite('unit test each function in common-behaviors.html', function() {
On 2017/08/01 18:51:11, Sergey Berezin wrote:
> Suggestion: create a separate suite for each method. Then you can name it e.g.
> 
> suite('_formatDate', ... {
>   test('returns the proper amount of time passed')
> ...
> }
> etc.

Done.

https://codereview.chromium.org/2991013002/diff/80001/appengine/config_servic...
appengine/config_service/ui/test/common/common-behaviors_test.html:34: common =
fixture('common-behaviorsTestFixture');
On 2017/08/01 18:51:11, Sergey Berezin wrote:
> Is it needed? The methods under test don't access DOM, I don't think they need
a
> fixture.

This is needed because the CommonBehaviors are imported as behavior in the
creation of Polymer elements, but the test file isn't a polymer element, it's a
testing suite, so it can't access the methods in common-behaviors.html because
ConfigUI is not defined in the window.

[cwpayton, 2017-08-01 19:18:55]

The CQ bit was checked by cwpayton@google.com

[cwpayton, 2017-08-01 19:18:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
sergeyberezin@chromium.org
Link to the patchset: https://codereview.chromium.org/2991013002/#ps100001
(title: "Refactored test to give each function its own suite")

[commit-bot: I haz the power, 2017-08-01 19:19:01]

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, 2017-08-01 19:20:37]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1501615135347950,
"parent_rev": "5d2675b14a7e4901352587f172ac0113033c9235", "commit_rev":
"acf054e95ab4e943dadcab133bde9a8e5d5c5a97"}

[commit-bot: I haz the power, 2017-08-01 19:20:43]

Description was changed from

==========
config_service: Added revision and timestamp to config-set-cards and config-set
pages

I refactored the code to avoid repitition by creating a common-behaviors.html
file as
well.

BUG=749540
==========

to

==========
config_service: Added revision and timestamp to config-set-cards and config-set
pages

I refactored the code to avoid repitition by creating a common-behaviors.html
file as
well.

BUG=749540

Review-Url: https://codereview.chromium.org/2991013002
Committed:
https://github.com/luci/luci-py/commit/acf054e95ab4e943dadcab133bde9a8e5d5c5a97
==========

[commit-bot: I haz the power, 2017-08-01 19:20:44]

Committed patchset #6 (id:100001) as
https://github.com/luci/luci-py/commit/acf054e95ab4e943dadcab133bde9a8e5d5c5a97