1002440 - [homepage][shipping] Add progress preview to team pages and exhibit table

Status: RESOLVED FIXED

(Webtools Graveyard :: Elmo, defect, P2)

Description

[Axel Hecht [:Pike]]

Attachment: team page snippet [screenshot]

I hacked something up to show progress on product l10n on the team pages and the exhibit dashboards.

Basics:

I really don't want to compute stuff on the fly, so I'm generating a static image in a command (and cron), and use that as background for a div. To reduce network requests, I'm putting all of them in one image, and use background-position.

There are some ride-alongs for the "dashboard" page:
Stop linking up the webdashboard, that's a thing of the past.
Unbreak linking to multiple trees or locales.
Drop the "H/C" column, nobody ever got that. The status ruler links to the latest comparison, the history preview links to the tree progress. Same timeframe as the preview, too.

Open item:

Not sure if I need to put the image outside of static/ so that it updates OK when the underlying file changes.

Things we could tweak:

The amount of time shown. I picked 50 days, mostly because I wanted something that makes "sense" for a picture 100px wide. I chose the height to be 20px.
There's some random scaling involved, to zoom in on the area of progress. Basically, I try to make the minimum value map to 25 or lower, them maximum to 75 or higher.
I show 'completion', mostly because it's going up for better.
Color choice is the same green that we use in the status slider. Fill is 20% alpha.
Still doing piecewise constant graphs. They're uncommon, but the truth. I've played with piecewise linear, but they're technically harder (going into "experimental" APIs in pillow), and they're actually confusing at times, too.
I'm not picking up the last value before the timeperiod. The graphs start at the first activity in the last 50 days. That's a tad weird, but getting the last value is also hard. Might have to add that.
I'm not checking for locales that got removed from the builds, that's OK.

Comment 1

[Axel Hecht [:Pike]]

Attachment: 'dashboard' page [screenshot]

PS: further ride-alongs: I'm using "none" instead of "no sign-offs", as that fits into one line, and looks better. Also, locale code now links to the team page, and I dropped the background color for "tree", as that red really doesn't serve any purpose today

Comment 2

[Francesco Lodolo [:flod]]

Dashboard page: if I understand it correctly, the current "Stats" column is replaced by History.

Any way to get to the last compare-locales result from this screen (i.e. the current "C" in Stats)? Personally I have an App page set on the dashboard page filtered for my locale, because I can fit all products in a single window without scrolling.

Comment 3

[Axel Hecht [:Pike]]

The "Status" column bar chart links to the latest comparison. So the Stats column is moved half into the Status column, and half in the History column.

Comment 4

[Francesco Lodolo [:flod]]

Thanks, makes definitely sense. I also like the idea of having a small graph without going into a different page.

A couple more thoughts on the same view:
* Should Errors and Warnings be near each other instead of separated by Missing? 
* I'd prefer to always have either 0 or an empty cell for value=0 (Errors+Obsolete display an empty cell, others display a 0).
* What is the column "Reported" used for? I don't think I ever saw a value different than 0 in there

Comment 5

[Axel Hecht [:Pike]]

I'd prefer to focus this bug on the team pages and the graphs, and not on the status quo of the exhibit table.

Comment 6

[Axel Hecht [:Pike]]

Note to self, s/.locale.signoff/.signoff/ as sort expression to fix bug 566324

Comment 7

[Axel Hecht [:Pike]]

Attachment: bug-1002440-progress-image.patch

This is basically one command to generate one big background image, and then picking up that image as background of a progress div on the team pages, and the dashboard tables.

I've packed up a few ride-alongs in the dashboard table part.

Design decisions that have their pros and cons:
I've put the constants around size and duration of the progress image into settings/base.py. That does come with CSS etc in the page and not as an external stylesheet, but it also comes with better factorization if we were to ever change the size of the image, for example.
I've went for a single big image. It's not tooooo big, 50k. I played with the idea to have vertical and horizontal slices, but that felt like complicating the code further, with little gain.
I'd expect the progress images to be generated through a cron job, probably hourly or so. The job itself is pretty short-lived, so it's not too critical to run it at low-traffic times.

Comment 8

[Peter Bengtsson [:peterbe]]

Comment on attachment 8422398 [details] [diff] [review]
bug-1002440-progress-image.patch

Review of attachment 8422398 [details] [diff] [review]:
-----------------------------------------------------------------

I was not able to test this. First of all, before having run the command line program I get this:
https://gist.github.com/peterbe/dbe951a952b93bc0664a

I think it's because you're doing `{% if run.prog_pos %}` and `run` is a `RunElement` instance which effectively changes an AttributeError to a KeyError and the django template does not like that. 

Secondly, I can't run the progress command because of the python 2.7 issue:
https://gist.github.com/peterbe/433c66b0239132f04b6c

::: apps/homepage/templates/homepage/locale-team.html
@@ +27,5 @@
> +<style>
> +.treeprogress-graph {
> +    width: {{ PROGRESS_IMG_SIZE.x }}px;
> +    height: {{ PROGRESS_IMG_SIZE.y }}px;
> +    background-image: url("{% static PROGRESS_IMG_NAME %}");

Why not just `background-image: url("{% static "l10nstats/progress.png" %}");`?
Why does it need to be a settings variable?

::: apps/l10nstats/management/commands/progress.py
@@ +5,5 @@
> +'''Create background images for progress previews
> +'''
> +
> +from collections import defaultdict
> +from datetime import timedelta

You know where I stand on an import like this :)

@@ +30,5 @@
> +    line_fill = (0, 128, 0)
> +    area_fill = (0, 128, 0, 20)
> +
> +    def handle(self, *args, **options):
> +        from PIL import Image, ImageDraw

Why is this here? If there's a good reason, perhaps an explanation comment is in order.

@@ +31,5 @@
> +    area_fill = (0, 128, 0, 20)
> +
> +    def handle(self, *args, **options):
> +        from PIL import Image, ImageDraw
> +        runs = Run.objects.exclude(srctime=None)

IMHO a cleaner use is `.exclude(srctime__isnull=True)`

@@ +34,5 @@
> +        from PIL import Image, ImageDraw
> +        runs = Run.objects.exclude(srctime=None)
> +        enddate = runs.aggregate(Max('srctime'))['srctime__max']
> +        startdate = enddate - timedelta(days=self.days)
> +        scale = 1. * (self.width - 1) / (enddate - startdate).total_seconds()

Someone's been using python 2.7 locally :)

This will break in 2.6

@@ +42,5 @@
> +                        .distinct())
> +        trees = sorted(runs.values_list('tree__code', flat=True)
> +                       .distinct())
> +        runs = runs.order_by('srctime')
> +        offloc = dict((l, (i + 1) * self.height)

Extreme nit-picking; `l` (lower case L) is an evil variable name because it looks like a 1 (the number one).

@@ +103,5 @@
> +        pal = im.convert("P", palette="ADAPTIVE")
> +        pal.save(os.path.join(settings.STATIC_ROOT,
> +                              settings.PROGRESS_IMG_NAME))
> +        ProgressPosition.objects.all().delete()
> +        ProgressPosition.objects.bulk_create(backobjs)

I didn't know you can do this! Cool

::: apps/shipping/templates/shipping/dashboard.html
@@ +80,5 @@
> +           </table>
> +         </a>
> +       </td>
> +       <td>
> +         <a ex:href-subcontent="{% url 'l10nstats.views.history_plot' %}?tree={&#123;.tree}}&locale={&#123;.locale}}&starttime={{ progress_start }}">

s/&/&amp;/

::: apps/shipping/templates/shipping/team-snippet.html
@@ +52,5 @@
>                </tr></tbody></table>
>            </td>
> +          <td class="treeprogress">
> +            {% if run.prog_pos %}
> +            <a href="{% url 'l10nstats.views.history_plot' %}?tree={{run.tree.code}}&locale={{run.locale.code}}&starttime={{ progress_start }}">

s/&/&amp;/

::: apps/shipping/views/__init__.py
@@ +6,4 @@
>  '''
>  
>  from collections import defaultdict
> +from datetime import datetime, timedelta

Please use `import datetime` and
`progress_start = datetime.datetime.utcnow() - datetime.timedelta(days=settings.PROGRESS_DAYS)` instead.

The only time it's OK to import specific functions from a module (only) is if the module doesn't act as a "namespace". 

http://mozweb.readthedocs.org/en/latest/coding.html#import-statements

Actually, poking around our code it seems this usage pattern is mixed. About 50-50 :(
It would be good if we can keep to just one standard. Ideally that standard would be to import only the module. :)

Take it or leave it if you disagree.

@@ +122,5 @@
> +    progresses = dict(((pp.tree.code, pp.locale.code), pp)
> +                       for pp in
> +                       (ProgressPosition.objects
> +                        .filter(tree__run__in=runs)
> +                        .select_related('tree', 'locale')))

Major nit but I would recommend this notation instead:

```
progresses = dict(
    ((pp.tree.code, pp.locale.code), pp)
    for pp in (
        ProgressPosition.objects
	.filter(tree__run__in=runs)
	.select_related('tree', 'locale')
    )
)

```
That way you're not at the mercy of the length of the first variable (ie. "progresses")
Meaning if you'd decide to s/progresses/progs/ it wouldn't mean a 5 line diff.

@@ +147,5 @@
>                  if ratio:
>                      ratio -= 1
>                      break
> +        if (run.tree.code, run.locale.code) in progresses:
> +            run.prog_pos = progresses[(run.tree.code, run.locale.code)]

Perhaps because the Django templating engine is (too) forgiving it's ok to do {{ run.prog_pos }} even if it doesn't exist. I think it's a give flaw in the the templating engine. What if you accidentally type {{ run.prog_poss }}. Then it'll accept that as a falsy value.

I'd sleep better at night if you add:
```
else:
    run.prog_pos = None
```

@@ +213,5 @@
>      return render_to_string('shipping/team-snippet.html',
>                              {'locale': loc,
>                               'other_team_locales': other_team_locales,
>                               'applications': applications,
> +                             'progress_start': progress_start.strftime('%Y-%m-%d'),

Can we not worry about the presentation (such as date formatting) in the templates where that kind of stuff belongs?

::: settings/base.py
@@ +222,5 @@
>  }
> +
> +# settings for the compare-locales progress preview images
> +PROGRESS_DAYS = 50
> +PROGRESS_IMG_SIZE = {'x': 100, 'y': 20}

The convention, I think, is usually `(100, 20)` meaning `(width, height)`. That's how PIL works and it's something I've seen in many other places where width and height is defined.

Comment 9

[Axel Hecht [:Pike]]

Done a bunch of fixes for 2.6, and the lack of progress entries. Nothing too intense, as I'm keeping the assumption that we're having progress images when there's progress. Just not getting 500s.

New patch coming up, I'll force update my fork, too (sorry, didn't think about just pushing an update and rebase afterwards early enough)

More inline.

(In reply to Peter Bengtsson [:peterbe] from comment #8)]
> 
> ::: apps/homepage/templates/homepage/locale-team.html
> @@ +27,5 @@
> > +<style>
> > +.treeprogress-graph {
> > +    width: {{ PROGRESS_IMG_SIZE.x }}px;
> > +    height: {{ PROGRESS_IMG_SIZE.y }}px;
> > +    background-image: url("{% static PROGRESS_IMG_NAME %}");
> 
> Why not just `background-image: url("{% static "l10nstats/progress.png"
> %}");`?
> Why does it need to be a settings variable?

Because it's used in places that are rather far apart, it's used in homepage, l10nstats, and shipping.

> ::: apps/l10nstats/management/commands/progress.py
> @@ +5,5 @@
> > +'''Create background images for progress previews
> > +'''
> > +
> > +from collections import defaultdict
> > +from datetime import timedelta
> 
> You know where I stand on an import like this :)
> 
> @@ +30,5 @@
> > +    line_fill = (0, 128, 0)
> > +    area_fill = (0, 128, 0, 20)
> > +
> > +    def handle(self, *args, **options):
> > +        from PIL import Image, ImageDraw
> 
> Why is this here? If there's a good reason, perhaps an explanation comment
> is in order.

Moved up. I initially had it there because then you don't technically have to have pillow installed at all. But that's not that much of a win.

> @@ +31,5 @@
> > +    area_fill = (0, 128, 0, 20)
> > +
> > +    def handle(self, *args, **options):
> > +        from PIL import Image, ImageDraw
> > +        runs = Run.objects.exclude(srctime=None)
> 
> IMHO a cleaner use is `.exclude(srctime__isnull=True)`

fixed.

> @@ +34,5 @@
> > +        from PIL import Image, ImageDraw
> > +        runs = Run.objects.exclude(srctime=None)
> > +        enddate = runs.aggregate(Max('srctime'))['srctime__max']
> > +        startdate = enddate - timedelta(days=self.days)
> > +        scale = 1. * (self.width - 1) / (enddate - startdate).total_seconds()
> 
> Someone's been using python 2.7 locally :)
> 
> This will break in 2.6
> 
> @@ +42,5 @@
> > +                        .distinct())
> > +        trees = sorted(runs.values_list('tree__code', flat=True)
> > +                       .distinct())
> > +        runs = runs.order_by('srctime')
> > +        offloc = dict((l, (i + 1) * self.height)
> 
> Extreme nit-picking; `l` (lower case L) is an evil variable name because it
> looks like a 1 (the number one).

fixed, the 't' below, too.
> 
> @@ +103,5 @@
> > +        pal = im.convert("P", palette="ADAPTIVE")
> > +        pal.save(os.path.join(settings.STATIC_ROOT,
> > +                              settings.PROGRESS_IMG_NAME))
> > +        ProgressPosition.objects.all().delete()
> > +        ProgressPosition.objects.bulk_create(backobjs)
> 
> I didn't know you can do this! Cool
> 
> ::: apps/shipping/templates/shipping/dashboard.html
> @@ +80,5 @@
> > +           </table>
> > +         </a>
> > +       </td>
> > +       <td>
> > +         <a ex:href-subcontent="{% url 'l10nstats.views.history_plot' %}?tree={&#123;.tree}}&locale={&#123;.locale}}&starttime={{ progress_start }}">
> 
> s/&/&amp;/
> 
> ::: apps/shipping/templates/shipping/team-snippet.html
> @@ +52,5 @@
> >                </tr></tbody></table>
> >            </td>
> > +          <td class="treeprogress">
> > +            {% if run.prog_pos %}
> > +            <a href="{% url 'l10nstats.views.history_plot' %}?tree={{run.tree.code}}&locale={{run.locale.code}}&starttime={{ progress_start }}">
> 
> s/&/&amp;/

fixed

> ::: apps/shipping/views/__init__.py
> @@ +6,4 @@
> >  '''
> >  
> >  from collections import defaultdict
> > +from datetime import datetime, timedelta
> 
> Please use `import datetime` and
> `progress_start = datetime.datetime.utcnow() -
> datetime.timedelta(days=settings.PROGRESS_DAYS)` instead.
> 
> The only time it's OK to import specific functions from a module (only) is
> if the module doesn't act as a "namespace". 
> 
> http://mozweb.readthedocs.org/en/latest/coding.html#import-statements
> 
> Actually, poking around our code it seems this usage pattern is mixed. About
> 50-50 :(
> It would be good if we can keep to just one standard. Ideally that standard
> would be to import only the module. :)
> 
> Take it or leave it if you disagree.

The doc you're referencing isn't all that strong, and really only tries to enforce that for methods. For that reason, I actually did switch the PIL import.

But for well-known datastructures like datetime or defaultdict, I really don't see the point.
> 
> @@ +122,5 @@
> > +    progresses = dict(((pp.tree.code, pp.locale.code), pp)
> > +                       for pp in
> > +                       (ProgressPosition.objects
> > +                        .filter(tree__run__in=runs)
> > +                        .select_related('tree', 'locale')))
> 
> Major nit but I would recommend this notation instead:
> 
> ```
> progresses = dict(
>     ((pp.tree.code, pp.locale.code), pp)
>     for pp in (
>         ProgressPosition.objects
> 	.filter(tree__run__in=runs)
> 	.select_related('tree', 'locale')
>     )
> )
> 
> ```
> That way you're not at the mercy of the length of the first variable (ie.
> "progresses")
> Meaning if you'd decide to s/progresses/progs/ it wouldn't mean a 5 line
> diff.

fixed.

> @@ +147,5 @@
> >                  if ratio:
> >                      ratio -= 1
> >                      break
> > +        if (run.tree.code, run.locale.code) in progresses:
> > +            run.prog_pos = progresses[(run.tree.code, run.locale.code)]
> 
> Perhaps because the Django templating engine is (too) forgiving it's ok to
> do {{ run.prog_pos }} even if it doesn't exist. I think it's a give flaw in
> the the templating engine. What if you accidentally type {{ run.prog_poss
> }}. Then it'll accept that as a falsy value.
> 
> I'd sleep better at night if you add:
> ```
> else:
>     run.prog_pos = None
> ```

.get() it is now.

> @@ +213,5 @@
> >      return render_to_string('shipping/team-snippet.html',
> >                              {'locale': loc,
> >                               'other_team_locales': other_team_locales,
> >                               'applications': applications,
> > +                             'progress_start': progress_start.strftime('%Y-%m-%d'),
> 
> Can we not worry about the presentation (such as date formatting) in the
> templates where that kind of stuff belongs?

FIXED, I just failed to get the date formatter working the first time, and chickened out. Reread the docs, that helped.

> ::: settings/base.py
> @@ +222,5 @@
> >  }
> > +
> > +# settings for the compare-locales progress preview images
> > +PROGRESS_DAYS = 50
> > +PROGRESS_IMG_SIZE = {'x': 100, 'y': 20}
> 
> The convention, I think, is usually `(100, 20)` meaning `(width, height)`.
> That's how PIL works and it's something I've seen in many other places where
> width and height is defined.

I went both ways, I made the call because in the templates, I can use .x instead of [0] and [1], which is rather cryptic.

Comment 10

[Axel Hecht [:Pike]]

Attachment: now with python 2.6

Comment 11

[Peter Bengtsson [:peterbe]]

Comment on attachment 8423239 [details] [diff] [review]
now with python 2.6

Review of attachment 8423239 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome work! It's really fast and seems to work really well. 

My only concern is the risk of the image getting cached by the browser. The name recycled even when the image changes. At the moment I don't think we set any far-futures on all things "/static/*" in apache. I'm just a tiny bit worried that one day we'll look at the graphs and say "That's not right. I updated those locales!" and it's just because the browser tried to be smart. Let's not block on this. After all, an advantage with the current solution is that the name can be hardcoded and simple presence on the disk is the state it needs.

::: apps/l10nstats/management/commands/progress.py
@@ +128,5 @@
> +        return td.total_seconds()
> +else:
> +    def total_seconds(td):
> +        return ((td.microseconds + (td.seconds + td.days * 24 * 3600) * 10**6)
> +                / 10**6)

pyflake gets pissy about this because it's a re-definition of the function. 
You could just do:

```
# python 2.6 helper
# XXX remove when we migrate to 2.7
def total_seconds(td):
    return (
        hasattr(td, 'total_seconds') and td.total_seconds() or
        (td.microseconds + (td.seconds + td.days * 24 * 3600) * 10**6) / 10**6
    )
        
```

::: apps/shipping/templates/shipping/team-snippet.html
@@ +52,5 @@
>                </tr></tbody></table>
>            </td>
> +          <td class="treeprogress">
> +            {% if run.prog_pos %}
> +            <a href="{% url 'l10nstats.views.history_plot' %}?tree={{run.tree.code}}&locale={{run.locale.code}}&starttime={{ progress_start|date:"Y-m-d" }}">

Those & still needs to be replaced with &amp; otherwise you get invalid HTML.

@@ +55,5 @@
> +            {% if run.prog_pos %}
> +            <a href="{% url 'l10nstats.views.history_plot' %}?tree={{run.tree.code}}&locale={{run.locale.code}}&starttime={{ progress_start|date:"Y-m-d" }}">
> +              <div class="treeprogress-graph"
> +  style='background-position:{{run.prog_pos.x}}px
> +  {{run.prog_pos.y}}px;'></div>

Any particular reason why this is the only place where ' is used instead of "?

Comment 12

[[github robot]]

Commit pushed to develop at https://github.com/mozilla/elmo

https://github.com/mozilla/elmo/commit/fe09276e8ac41a476d7456860f9a003d5de97512
bug 1002440, add progress graphs to team page and exhibit dashboard, r=peterbe

Comment 13

[Axel Hecht [:Pike]]

Attachment: half a patch

Drats, actually we've set up apache to aggressively cache images.

Workaround by ?last-run-id, Peter, what do you think?

Comment 14

[[github robot]]

Commit pushed to develop at https://github.com/mozilla/elmo

https://github.com/mozilla/elmo/commit/062126b3542cf403dc9b20ce8c21fa7b7d800628
bug 1002440, follow up: cache-bust query for apache image handling, r=peterbe

Comment 15

[Axel Hecht [:Pike]]

We had to do a follow up to work around the apache caching configs, but this works on dev now. Marking FIXED.