853068 - [shipping] add counts to the locale-changes page to indicate current, recent, and old locales

Status: RESOLVED FIXED

(Webtools Graveyard :: Elmo, defect, P2)

Description

[Axel Hecht [:Pike]]

We'd like to understand better how many locales are good and how many are lagging.

To make that easier, we'd rather not count ourselves but have elmo do that.

First bucket is current, second number is on the previous release, third bucket is "older than that".

Mockup could be like:

20 | ab-CD    | 1

And then a row group after that for the old bucket and the current bucket (which might span multiple beta milestones)

Giving this a P2, as it'd be nice to have that soon to drill down our goals for next quarter.

Comment 1

[Peter Bengtsson [:peterbe]]

Pardon my goldfish-like memory, but what's the "locale-changes page"?

I wouldn't mind working on this but I think I'd need a refresher about what it is and how it would work.

Comment 2

[Axel Hecht [:Pike]]

https://l10n.mozilla.org/shipping/app/locale-changes/fx25 is an example URL, it's in apps/shipping/views/app.py

Comment 3

[Peter Bengtsson [:peterbe]]

Wow! I started reading the code and my head exploded. In a bad way :)
I think we better step through this bug in the next Elmo call. Then you can explain how you want this to work.

Comment 4

[Peter Bengtsson [:peterbe]]

Attachment: Screen Shot 2013-11-25 at 3.24.05 PM.png

The code I had to write to get to this is pretty horrible so I won't share yet but looking at the output (and pausing on aesthetics for the moment too) is this what you'd expect?

I reversed the list of rows, and put things in buckets if the code name was the same (this puts fx23_beta_1 together with fx23)

Then as I loop through all the rows, just before I dump a lump into a bucket I calculate the rowspan (and count the number of unique locales) and put that into the "last" row so it becomes easy to render in the template. 

What stumped me big time was that you said you wanted 4 groups and the third group should be fx20 and fx21. That I have not been able to achieve. I don't think I understand the data well enough to be able to make those groups.

Comment 5

[Peter Bengtsson [:peterbe]]

Attachment: Screenshot 2013-12-03 16.43.45.png

This way, the last of the 4 groups' rowspan is top vertical aligned. 

Now I just need to write a unit test to back this all up.

Comment 6

[chris hofmann]

screenshot looks like what we need.

Comment 7

[Peter Bengtsson [:peterbe]]

Attachment: bug853068-2.diff

I tried to start writing a test to prove this but there's nothing to build on for a test. I got started but quickly realised I would have to make fixture for every model under the sun. It just got messier and messier so in favor of progress some time this year I chickened out on a working test. 

The code is still a bit of a blur to me how it works with all the optimized dicts and lists of IDs. Perhaps it could be a follow-up bug to rewrite this into something more structured and modular.

Comment 8

[Axel Hecht [:Pike]]

Comment on attachment 8342475 [details] [diff] [review]
bug853068-2.diff

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

I don't think this actually does what we talked about.

The groups were:

This cycle
Last cycle
Prior two cycles
Old cycles

I find the code hard to grok as well, it'd be great if there were more comments on what that code actually does.

Lastly, I'd prefer to go for classes for the rowspan rather than inline style, where possible.

I'm OK with chickening out of a test for this, though.

Comment 9

[Axel Hecht [:Pike]]

Comment on attachment 8338095 [details]
Screen Shot 2013-11-25 at 3.24.05 PM.png

This one showed the right numbers, as there's no Firefox 20.

The only request I had for this was to align the last bucket top.

Comment 10

[Peter Bengtsson [:peterbe]]

The code under review lumps (aka. buckets) like this (from the bottom):

1. Latest release
2. Second latest release
3. Third latest release
4. All other older releases

I believe you said you wanted it as:

1. Latest release
2. Second latest release
3. Third AND forth latest release
4. All other older releases

This was something I could NOT make happen with the code. If you can please have a go :)

Comment 11

[Axel Hecht [:Pike]]

I think if you hook up the group logic inside the loop in line 58

    while av_ and locs4av:

it should be OK to get the same offset in releases grouped together. Probably keeping a counter there, storing the offset in rows, and inc'ing by len(accepted4loc), and on the next group bump, add the extra data to the row you kept. You can also ignore dealing with the milestone rows.

There's a bit of paperwork, but I don't think it'll be worse than the code you're having.

Comment 12

[Axel Hecht [:Pike]]

I've taken this branch and rewrote the logic to support variable-sized buckets.

The result is in https://github.com/Pike/elmo/compare/mozilla:develop...feature%2Fbug-853068-group-locale-changes?expand=1.

There's one caveat, Peter, can you look at that? I can't figure out why the rowspan="2" doesn't open up the last column, there's still a full-width border between the two rows. The larger first and last buckets work fine and block out the border from the last column.

Comment 13

[Peter Bengtsson [:peterbe]]

I tested it locally. It seems to work like a charm! You're good at this kind of stuff. 

How do you want to proceed? That you make a patch for final review or that I take your fork and make a patch for final review? After all, now it's almost entirely your code.

Comment 14

[Axel Hecht [:Pike]]

I'd appreciated if you could figure out the remaining html table problem here.

Comment 15

[Axel Hecht [:Pike]]

Attachment: show the table color, red square content bothers me

Here's a screenshot to show where the table border is that bothers me, I squared it in red. The 8 actually counts 'nso te' and 'af bn-BD fa kk mk vi', aka, 18 and 19. But there's a border that looks like it's only counting 19.

Comment 16

[Peter Bengtsson [:peterbe]]

Attachment: bug853068-3.diff

Your work, my css hack :)

Comment 17

[[github robot]]

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

https://github.com/mozilla/elmo/commit/90de3e64708a7058e958bd0ef4924381e5c20c56
bug 853068 - add counts to the locale-changes page to indicate current, recent, and old locales, r=Pike