Pushes view is the most complex view that has been designed incrementally as we learned what we want. The result of that is that it's UX is not userfiendly and the client-side code is a mess. We should think of a new design for that view and implement it.

It'd be nice if the new view dealt good with long check-in comments like the one for http://hg.mozilla.org/l10n-central/lt/rev/6c325200c0c9, <quote>Suite sync with en-US, covers bugs 563261, 570970, 577514: Bug 563261: Make lightweight themes / personas work with SeaMonkey trunk in browser windows. Bug 570970: Move the Link Behaviour preferences from the tabs pane to a separate pane. Bug 577514: Remove redundant pref-offline UI which was commented out since 2001.</quote> Generally, I guess hg shortens to the first line in cases like this?

AFAIK, hg shortens on the first line break first, and then possible also by some maximum character length. That's off the top of my head, I haven't looked into code, actually.

Comment on attachment 470411 [details] Mock up made by RQ the mock is compatible with what jesper suggested in the m.d.l10n thread. I like it!

I'm the one among us hackers that gets hurt most, so I'll take a stab at this while elmo is on the road.

Copying stas and my notes on http://people.mozilla.com/~chowse/drop/l10n/wireframes/v1/Combined_Overview.html and http://people.mozilla.com/~chowse/drop/l10n/wireframes/v1/Combined_Sign-Offs.html from etherpad: 5.1 Recent Sign-offs in reverse-polish-notation, going from the bottom up: * Last row is currently accepted sign-off with snapshot data * Then last test results for accepted sign-off * rejected and pending sign-offs, in the order of their pushes, with snapshot data * then most recent test data (grouped by push) * most recent push with test data for this project, (with the ability to sign off if logged in) * The commit data in the center column should show commits grouped by push, where sign-off is only availabe for the tip of that push. The commit messages are truncated to the first line each, similar to what hg does by default. (re http://hg.mozilla.org/l10n-central/lt/rev/6c325200c0c9 and http://hg.mozilla.org/l10n-central/lt/shortlog/501) Right now, test data is compare-locales runs, but that may change and increase. Possible additional data would be result of tinderbox dep and nightly builds, l10n-src-verifciation, mozmill autoruns (these may be results for different revisions, even) * make "diff" work like on https://l10n-stage-sj.mozilla.org/pushes/ * include: time of the last action (signoff, accept, reject etc), author of the last action * the action buttons (for localizers: (sign off, cancel signoff) and for l10n-drivers: (accept, reject, revoke)) show when you hover over a push (on the left, next to the tick). the button should say "sign off" and the other actions should be accessible via a gears icon menu. * the author of the last action gets credit in a column next to the tick * if the signoff has been acted upon by a l10n-driver (accepted, rejected), you can click on the tick (if accepted) or the cross (if rejected) to make the line expand and show all history plus the rejection comment Likely gonna do a "combined signoff", if we want to merge that with combined_overview, we can do that later.

I'm trying to wrap my head around how to land this, as I'm getting closer. I'm drastically changing the interactions, and I don't think that it's reasonable to review the code to get a feel for that. So I'm wondering how to get you guys a live playground to look at. Right now, the code is strictly additive, so I could just make the shipping dashboard link to both the old and the new view, and land in that deployment. Or we could make the test install on https://l10n-stage-sj.mozilla.org/test/ carry the new code, either pointing to a back up or the live db (I'm in favor of the latter) and have people test and play with that. What do you think? I'm for /test/ on the live db personally, I think.

Is our code prepared for a deployment in a subfolder, and not the domain's root directory? In either case, I think I'm in favor of the former solution (link to both). I believe our URLs are already messy as they are and adding anything that has 'test' in it to a WIP project that's already on a 'l10n-*stage*-sj' will likely only add to the confusion.

Yes, as you can see the /test/ setup exists, just follow the link :-) The /test/ was set up exactly to be able to point to in-development versions. I don't see a reason why we should bring url structure into the discussion about a test-bed, really. Not that I don't feel sorry about how we didn't grab the l10n.m.o domain yet.

(In reply to comment #9) > Yes, as you can see the /test/ setup exists, just follow the link :-) The > /test/ was set up exactly to be able to point to in-development versions. Oh, nice :) I assumed it was an example of a proposed URL and didn't even try clicking :) I would be for /test/ if this was only intended for us to see and comment on, but if you want wider testing and feedback from localizers, I'd say linking from the current dashboard would help at discoverability and would increase testing coverage ('hmm, I wonder what this link does [click]' vs. 'Do I need to read this message in the newsgroups about some testing instance of the dashboard? nah...').

gandalf, looking at the existing code, I can't find a permissions check for someone to sign-off. I only find "is not anonymous and is not staff" [1], aka, anyone with an ldap account. And that's for the UI, that's not for doing manual push requests. Do you find something restricting to localizers? [1] http://hg.mozilla.org/l10n/django-site/file/d4a71dcef068/shipping/views/__init__.py#l201

No, we did not have Localizers group in our django instance at the time of writing this app so we only recognized is_staff, not logged in and logged in. This code has been written in summer 2009. We added group checking to authentication in december 2009...

This is the new UI, ready for review. Right now, it's hooked on to the shipping dashboard, and leaves an "old UI" link in the column for folks that are confused. I suggest to rip out the old ui in a second step. I wish I had an angle at waffle, I'd so use that. Probably a hefty patch to review as such, should I grab the /test/ setup with a recent clone of the production db to look at things?

Woohoo, I'm happy to see this patch here! :) That said, I might not be able to review it until later this week. I'll try to get to it asap. A preview on /test/ would be helpful for sure, although I'll probably set things up locally anyways to test queries and such. So I wouldn't make this priority no. 1 :)

Patch is now up on /test/ with a somewhat recent database dump from /. Two things I noticed: The chunk to settings.py is bad mq, ignore that. Also, there's a disfunctional button to get more pushes. That's not implemented at this point, and I guess doing that will end up moving some code around again. Not sure if we should block on that, though.

stas, sorry to bug, can you get to this? I'd like to pre-load a significant part of data2, that is, change the mapping of tree to repos to be time-dependent to fix the branch bustage (showing only pushes on one repo) in (both) pushes views. I'd rather only fix that once, soooo, the plan would be get pushes 2 in remove pushes 1 move the link between tree and l10n forest to be a time-dependent many2many, juggling data pretty hard, and fix pushes 2 to go over all repos in the forests.

Having noticed this patch wasn't folded into hg trunk of "django-site" and I have now moved it into github and started refactor it as per the Playdoh base, is this patch ready to be merged in? If so, I'll patch it manually into the git repository.

I'll review it today/tomorrow. Sorry for the wait.

Hi Peter, for most parts of mozilla, we track code reviews inside bugzilla. When you see a "review?" on an attachment, that is still waiting for review here. "l10n: review? (stas)" means that I requested review from stas here. Additional reviews are welcome.

(In reply to comment #19) > Hi Peter, for most parts of mozilla, we track code reviews inside bugzilla. > When you see a "review?" on an attachment, that is still waiting for review > here. "l10n: review? (stas)" means that I requested review from stas here. > All cool with that. With github, does it mean that some code review can be done on github alone or does all patches need to go through bugzilla? > Additional reviews are welcome. From skimming the diff; there are many things in that that I don't understand. However, I do object to intermingling django template code into the inline Javascript. Playdoh and webdev's policy is to always use external scripts instead of inlining. If Django needs to share something with the Javascript I'd prefer to use constants like: <!-- pseudo code --> <script> var DIFF_VERSION = "{{diff_version}}"; </script> <script src="/js/show_diff.js"></script>

Doh, I got better at that, but apparently not good enough. I'm not going to extract the actual script, I think we should do that once we have a command to extract media to static/whatever (aka playdoh?), but I'll factor the template and the script cleanly. Stas, more comments?

Updated patch to seperate the template tag stuff into a separate script tag. I've updated the /test/ install, too.

This patch has been applied to the elmo/playdoh-reorg branch https://github.com/mozilla/elmo/commit/827e10332c90fa3d05da415563e6437f1df75bb8 Note to self: because the patch was made based on the idea that all apps are in the root directory and not inside `apps/` I had to modify the patch manually so that I could run 'git apply' on the file.

We dropped the patch from playdoh, which is cherry-picked playdoh-reorg. I've added a feature/pushes2 branch on my elmo fork, see https://github.com/Pike/elmo/commits/feature%2Fpushes2. The follow-up landing is moving the 'old UI' link behind an old_signoff=1 query param instead of showing it all times. Somewhat waffle inspired, but not using waffle.

Comment on attachment 527787 [details] [diff] [review] address Peter's comment Gandalf, can you help out stas with a review here? Also, Peter, want to do either a review or feedback pass on this again, too? Now that you know a tad more of our code.

Comment on attachment 527787 [details] [diff] [review] address Peter's comment Review of attachment 527787 [details] [diff] [review]: ----------------------------------------------------------------- ::: l10nstats/models.py @@ +83,5 @@ > warnings = models.IntegerField(default=0) > completion = models.SmallIntegerField(default=0) > > + @property > + def allmissing(self): Would be nice to see a comment about WHY this property is added. After all, it adds "bloat" to an otherwise pure and simple model. Is it because you can't do the + in the templates? ::: l10nstats/templatetags/run_filters.py @@ +59,5 @@ > + return mark_safe("&nbsp;") > + fmt = ('<a %s href="' + > + reverse('l10nstats.views.compare')+ > + '?run=%d">%s</a>') > + missing = run.missing + run.missingInFiles Either this is right or it should be "missing = run.allmissing" ::: shipping/models.py @@ +111,5 @@ > + (ACCEPTED, 'accepted'), > + (REJECTED, 'rejected'), > + (CANCELLED, 'cancelled'), > + (OBSOLETED, 'obsoleted'), > + ) Excellent! ::: shipping/templates/shipping/signoff-rows.html @@ +88,5 @@ > + <td class="diff {{flag}}">{% if so.diffbases %}<div class="diffanchor"></div>{% endif %}</td> > + </tr> > + {% endwith %}{% endfor %} > + {% else %} > + <tr class = "signoffrow hidden" data-push="{{push.id}}"> can that excess whitespace around the equal sign cause problems? ::: shipping/templates/shipping/signoffs.html @@ +278,5 @@ > + </tr> > + <tr> > + <td></td> > + <td> > + <input type="checkbox" name="clear_old" value="yes"disabled> whitespace missing between "yes" and disabled ::: shipping/views/signoff.py @@ +52,5 @@ > +from shipping.views import _signoffs > +from l10nstats.models import Run, Run_Revisions > + > + > +class __RowCollector: Why the double __? @@ +228,5 @@ > + """ > + try: > + # rev query arg is required, it's not a url param for caching, and because it's dynamic > + # in the js code, so the {% url %} tag prefers this > + rev = request.GET['rev'] If it's required, shouldn't it also check that it's non-blank?

I should add, regarding my review, that because I don't fully understand the whole scope I just nitpicked small details. What about tests?

Comment on attachment 527787 [details] [diff] [review] address Peter's comment Review of attachment 527787 [details] [diff] [review]: ----------------------------------------------------------------- My general comment is about the way the signoffs are presented. I find it confusing for a couple of reasons: - they look like another changeset - they are at the bottom of the push - the comparison result looks the same for the push (where it corresponds to the latest comparison for this push) and the same for the signoff (where it is a snapshot of what the localizer saw when they signed off) I think it would be clearer to include all the signoff-related data and actions at the top of the push. Mostly because you can only sign off on the top-most changeset anyways. I can see how doing this right now can be tricky, with rowspans at al. I'm generally not a fan of the flat list of <tr/> that you went for. True, this data is tabular, but it's also highly structured (pushes > changesets). Maybe make it a series of tables, one per each push, instead? I still need to dig into the main signoff view (shipping.views.signoff.signoff) and try to figure out how the diffbases work. I've noticed some problems around them, like the UI suggesting I can diff between a changeset and the signoff made on this very changeset. This looked like a bug. Even if the whole patch looks pretty good and the main functionality is there (and is more usable than the current implementation), I'm going to r- this patch, as I think it needs some more discussion before it lands. Most of my inline comments are nits. I'd like to focus our discussion on the tabularity of the data and its presentation. ::: l10nstats/templatetags/run_filters.py @@ +58,5 @@ > + if not isinstance(run, Run): > + return mark_safe("&nbsp;") > + fmt = ('<a %s href="' + > + reverse('l10nstats.views.compare')+ > + '?run=%d">%s</a>') fmt = '<a %%s href="%s?run=%%d">%%s</a>' % reverse('l10nstats.views.compare') ::: shipping/models.py @@ +109,5 @@ > + FLAG_CHOICES = ( > + (PENDING, 'pending'), > + (ACCEPTED, 'accepted'), > + (REJECTED, 'rejected'), > + (CANCELLED, 'cancelled'), In en-US (which we follow elsewhere) this should be "canceled". But, more to the point, why do we need canceled? Seeing how this works, I'm wondering if Actions can be simplified by removing the canceled flag in favor of using rejected. I'm also happy to see revoked go away. My understanding is that a localizer should just ping a driver and ask to reject a mistakenly submitted signoff, should such situation occur. ::: shipping/templates/shipping/dashboard.html @@ +51,1 @@ > <span ex:content=".signoff"></span> Not really related to this very change, but this file in general. I've noticed that sometimes the dashboard view will show "accepted and pending" while the signoff view only shows pending. An example of this would be /shipping/signoffs/it/sea2.1 on the recent DB dump. I wasn't able to track this down. Another artifact of how we juggle the repos? ::: shipping/templates/shipping/signoff-details.html @@ +39,5 @@ > +{% if good %} > +<div class="good_signoff"> > +Technically, this looks good. You did test > + yourself, right?</div> > +{% else %} Indent everything below this for improved readability? ::: shipping/templates/shipping/signoff-rows.html @@ +43,5 @@ > + <td rowspan="{{push.changerows}}">{{push.run|showrun}}<br> > + <span class="csuser">{{push.run.build.starttime|date}}</span></td> > + {% for cs in push.changes %} > + <td class="rev_td">{% if forloop.first %}{% if not push.signoffs|length %}<div > + class="do_signoff"><input type="button" id="so_{{push.id}}" value="signoff&hellip;" This code is hard to read. Can you put a line break after html tags and template variables and tags? Also, indentation. Nit: it's a verb, so it should read "sign off..." @@ +44,5 @@ > + <span class="csuser">{{push.run.build.starttime|date}}</span></td> > + {% for cs in push.changes %} > + <td class="rev_td">{% if forloop.first %}{% if not push.signoffs|length %}<div > + class="do_signoff"><input type="button" id="so_{{push.id}}" value="signoff&hellip;" > + onclick="return false;" {% if push.run %} data-run="{{push.run.id}}" {% endif %}class="do_signoff {% ifequal suggested_signoff push.id %}suggested{% else %}hidden{% endifequal %}"{%if not perms.shipping.add_signoff %} disabled {%endif%}></div> {% endif %}{% endif %} File a follow-up bug to assign this permission to the Localizer group, please. @@ +46,5 @@ > + <td class="rev_td">{% if forloop.first %}{% if not push.signoffs|length %}<div > + class="do_signoff"><input type="button" id="so_{{push.id}}" value="signoff&hellip;" > + onclick="return false;" {% if push.run %} data-run="{{push.run.id}}" {% endif %}class="do_signoff {% ifequal suggested_signoff push.id %}suggested{% else %}hidden{% endifequal %}"{%if not perms.shipping.add_signoff %} disabled {%endif%}></div> {% endif %}{% endif %} > +<div class="rev">{{cs.description}}<br> > + <span class="csuser">{{cs.user}}</span> Can we go an extra mile here and strip the email address from the value of {{cs.user}}? @@ +59,5 @@ > + {% for so in push.signoffs %}{% with so.action.get_flag_display as flag %} > + <tr class="signoffrow" data-push="{{push.id}}"> > + <td class="run {{flag}}">{{so.run|showrun}}</td> > + <td class="signoff {{flag}}"> > + <input class="review_action" data-signoff="{{so.signoff.id}}" data-state="{{flag}}" type="button" value="review&hellip;"{% if not perms.shipping.review_signoff %} disabled{% endif %}> Hide it entirely if user prefs are not sufficient? ::: shipping/templates/shipping/signoffs.html @@ +301,5 @@ > + </form> > +</div> {% endif %} > +<table cellspacing="0" cellpadding="5" border="1" id="pushtable"> > +{% include "shipping/signoff-rows.html" %} > + <tr><td colspan="4" align="center"><input type="button" value="more pushes" onclick="" disabled></td></tr> Why is this always disabled? ::: shipping/views/signoff.py @@ +296,5 @@ > + """Actual worker to add a sign-off to the database. > + Requires shipping.add_signoff permission. > + """ > + _redirect = redirect('shipping.views.signoff.signoff', locale_code, app_code) > + if request.user.has_perm("shipping.add_signoff"): You can move this into a django.contrib.auth.decorators.permission_required decorator.

I've put the signoff data beneath the push because it's meta data to the push, and the description of the push is not so much the sign-off and its metadata but the check in comment and the push date. The tabular structure is because it makes recognition of data flow easier. I wouldn't know how to align the data of multiple pushes in separate tables. We can probably make the difference between the pushes itself and the signoff data be more apparent by visually separating them a bit, backgrounds might be enough. Also, in the case where the test snapshots are the current tests, we probably should de-emphasize them to some extent.

(In reply to comment #29) > I've put the signoff data beneath the push because it's meta data to the > push, and the description of the push is not so much the sign-off and its > metadata but the check in comment and the push date. My rationale was that signing off is an action that is made on a push, so having it closer to the top-most changeset (if there is more than one) seems more logical. > The tabular structure is because it makes recognition of data flow easier. I > wouldn't know how to align the data of multiple pushes in separate tables. I'm not against the tabular structure, but I was thinking out loud if it would be better to have each push be a separate table, listed one below another. That's because there's a hierarchical structure to our data (pushes contain changesets), and if you want to emphasize that in a flat table, you usually end up with if-else clauses that sometimes print </tr> and sometimes don't. That's soooo PHP-like :) > We can probably make the difference between the pushes itself and the > signoff data be more apparent by visually separating them a bit, backgrounds > might be enough. Also, in the case where the test snapshots are the current > tests, we probably should de-emphasize them to some extent. Good thinking. Maybe even hide them entirely if they're the same, and if they're not, explicitly say "the result of the comparison at the time of the signoff was: ...".

Here's how Islandic on 3.6 would look, if "More pushes" would work. I.e., with the current code, you only see the first push and its metadata. FYI: Magic line to get to this is to tweak shipping.views.signoff.signoff to have if False and current_so is not None: Then it's at /shipping/signoffs/is/fx3.6 Here you see a few things with a bit more detail: Signoffs for some locales are rather frequent meta data. The test runs, i.e., the second column with compare-locales runs is ordered in time (can't tell from the screenshot, of course, but the latest run on the last push is actually newer than the snapshot run). The yellow "pending review" should be grey "not reviewed" if there's a green newer. This isn't a problem as long as "More pushes" doesn't work, though. I'd love to have a visual seperator between pushes, and thus a tighter coupling within a push, but my HTML/CSS can't do it. The diff anchors definitly need love, but my visuals don't have that. The awkward initial state of the diffanchors for a good sign-off as last push is due to a slight shift, I place the diffanchors on the sign-off rows by default and not on the tip of the push the signoff is on. Maybe that's wrong? Can go both ways.

To frame our options here a bit, the underlying idea of comment 16 still holds, I doubt I can do the data1.1 patch for two sign-off views. It's a beast as is. This is about shipping fx, so I'll make some calls here, sorry. data1.1 needs to happen on a timeline, and I need a stable code base in terms of sign-off view to work on. That means, we either commit to adopting the signoff2 design, and get it master-ready and landed next week. Or we'll table this bug, and update the signoff view after data1.1 is in production and stabilized. I think it's a considerable risk to our ability to ship software if we work on both in parallel, and I'm unwilling to take that. There are a few categories to think in when looking at the status quo: - UX design decisions: This is basically a yes/no. - implemenation decisions: Those can be both yes/no, depending on scope. They can also be follow-up bugs, if they don't block a landing on master.

I rebased the branch onto develop and renamed it, it's now on https://github.com/Pike/elmo/commits/feature%2Fsignoff2. Had to do two follow up landings so far, moving additional files, and making the CSS work in strict mode.

PERF: https://github.com/Pike/elmo/blob/0d87870a7218b2c5d992a2d4414bc356b26ab434/apps/shipping/templates/shipping/signoff-rows.html#L51 has a {{cs}}, which calls Changeset.__unicode__, which does a non-cheap query for the repo it was pushed to, each.

(In reply to comment #34) > PERF: > https://github.com/Pike/elmo/blob/0d87870a7218b2c5d992a2d4414bc356b26ab434/ > apps/shipping/templates/shipping/signoff-rows.html#L51 has a {{cs}}, which > calls Changeset.__unicode__, which does a non-cheap query for the repo it > was pushed to, each. https://github.com/Pike/elmo/commit/ad5e7134da1858e31d6e86b9a93fb66060b2b4cd fixes that.

I'll do the needed patch for csrf tomorrow, fwiw.

(In reply to comment #28) Going into the nitty gritty detail: > ::: shipping/models.py > @@ +109,5 @@ > > + FLAG_CHOICES = ( > > + (PENDING, 'pending'), > > + (ACCEPTED, 'accepted'), > > + (REJECTED, 'rejected'), > > + (CANCELLED, 'cancelled'), > > In en-US (which we follow elsewhere) this should be "canceled". > > But, more to the point, why do we need canceled? Seeing how this works, I'm > wondering if Actions can be simplified by removing the canceled flag in > favor of using rejected. Rejected comes with a message, or should. The intended use case for this are those localizers that sign off on every other revision and still have 500+ missing strings. There, I'd go in and reject the sign-off with a single message, and cancel the others. As opposed to "this one is bad, but I should review the previous one still, as you busted search on the latest one". > ::: shipping/templates/shipping/dashboard.html > @@ +51,1 @@ > > <span ex:content=".signoff"></span> > > Not really related to this very change, but this file in general. > > I've noticed that sometimes the dashboard view will show "accepted and > pending" while the signoff view only shows pending. > > An example of this would be /shipping/signoffs/it/sea2.1 on the recent DB > dump. > > I wasn't able to track this down. Another artifact of how we juggle the > repos? The old signoff view doesn't check if there's already a sign-off on a push, and if people hit reload, it totally adds a second. Which is why in the new view, add_signoff actually checks. > @@ +46,5 @@ > > + <td class="rev_td">{% if forloop.first %}{% if not push.signoffs|length %}<div > > + class="do_signoff"><input type="button" id="so_{{push.id}}" value="signoff&hellip;" > > + onclick="return false;" {% if push.run %} data-run="{{push.run.id}}" {% endif %}class="do_signoff {% ifequal suggested_signoff push.id %}suggested{% else %}hidden{% endifequal %}"{%if not perms.shipping.add_signoff %} disabled {%endif%}></div> {% endif %}{% endif %} > > +<div class="rev">{{cs.description}}<br> > > + <span class="csuser">{{cs.user}}</span> > > Can we go an extra mile here and strip the email address from the value of > {{cs.user}}? Follow up bug? I'm not sure that's right, nor easy to get right. Looking at the data, you're all of Stas Malolepszy (stas@mozilla.com> Stas Malolepszy <stas@mozilla.com> Staś Małolepszy :stas <stas@mozilla.com> Staś Małolepszy <stas@mozilla.com> > @@ +59,5 @@ > > + {% for so in push.signoffs %}{% with so.action.get_flag_display as flag %} > > + <tr class="signoffrow" data-push="{{push.id}}"> > > + <td class="run {{flag}}">{{so.run|showrun}}</td> > > + <td class="signoff {{flag}}"> > > + <input class="review_action" data-signoff="{{so.signoff.id}}" data-state="{{flag}}" type="button" value="review&hellip;"{% if not perms.shipping.review_signoff %} disabled{% endif %}> > > Hide it entirely if user prefs are not sufficient? There have been too many questions on hidden UI due to not being logged in. I think it's also informative for the user to see "there's something about a review now, but I can't do that". > ::: shipping/templates/shipping/signoffs.html > @@ +301,5 @@ > > + </form> > > +</div> {% endif %} > > +<table cellspacing="0" cellpadding="5" border="1" id="pushtable"> > > +{% include "shipping/signoff-rows.html" %} > > + <tr><td colspan="4" align="center"><input type="button" value="more pushes" onclick="" disabled></td></tr> > > Why is this always disabled? Because it's not implemented yet. Not sure if that's a good follow up bug or if we need to get that now? > ::: shipping/views/signoff.py > @@ +296,5 @@ > > + """Actual worker to add a sign-off to the database. > > + Requires shipping.add_signoff permission. > > + """ > > + _redirect = redirect('shipping.views.signoff.signoff', locale_code, app_code) > > + if request.user.has_perm("shipping.add_signoff"): > > You can move this into a django.contrib.auth.decorators.permission_required > decorator. I hate the permission_required decorator, with passion. I also don't think it's doing the right thing for us, as it's pretty tightly integrated into the standard UX thinking about logging in. The comments I stripped are stuff that I'll follow up with a changeset.

(In reply to comment #26) > Comment on attachment 527787 [details] [diff] [review] [review] > address Peter's comment > > Review of attachment 527787 [details] [diff] [review] [review]: > ----------------------------------------------------------------- > > ::: l10nstats/models.py > @@ +83,5 @@ > > warnings = models.IntegerField(default=0) > > completion = models.SmallIntegerField(default=0) > > > > + @property > > + def allmissing(self): > > Would be nice to see a comment about WHY this property is added. After all, > it adds "bloat" to an otherwise pure and simple model. > > Is it because you can't do the + in the templates? > > ::: l10nstats/templatetags/run_filters.py > @@ +59,5 @@ > > + return mark_safe("&nbsp;") > > + fmt = ('<a %s href="' + > > + reverse('l10nstats.views.compare')+ > > + '?run=%d">%s</a>') > > + missing = run.missing + run.missingInFiles > > Either this is right or it should be "missing = run.allmissing" > > ::: shipping/models.py > @@ +111,5 @@ > > + (ACCEPTED, 'accepted'), > > + (REJECTED, 'rejected'), > > + (CANCELLED, 'cancelled'), > > + (OBSOLETED, 'obsoleted'), > > + ) > > Excellent! > > ::: shipping/templates/shipping/signoff-rows.html > @@ +88,5 @@ > > + <td class="diff {{flag}}">{% if so.diffbases %}<div class="diffanchor"></div>{% endif %}</td> > > + </tr> > > + {% endwith %}{% endfor %} > > + {% else %} > > + <tr class = "signoffrow hidden" data-push="{{push.id}}"> > > can that excess whitespace around the equal sign cause problems? > > ::: shipping/templates/shipping/signoffs.html > @@ +278,5 @@ > > + </tr> > > + <tr> > > + <td></td> > > + <td> > > + <input type="checkbox" name="clear_old" value="yes"disabled> > > whitespace missing between "yes" and disabled > > ::: shipping/views/signoff.py > @@ +52,5 @@ > > +from shipping.views import _signoffs > > +from l10nstats.models import Run, Run_Revisions > > + > > + > > +class __RowCollector: > > Why the double __? Somewhat private but not totally? I never really thought about it, but the __ pattern helps me hack things on the ldap backend, while I think _ would hide that completely? > @@ +228,5 @@ > > + """ > > + try: > > + # rev query arg is required, it's not a url param for caching, and because it's dynamic > > + # in the js code, so the {% url %} tag prefers this > > + rev = request.GET['rev'] > > If it's required, shouldn't it also check that it's non-blank? I should unify the error handling there, need to think about that a bit. Also, I don't handle the 404s in the signoff ajax so far :-/

(In reply to comment #38) > (In reply to comment #26) > > Comment on attachment 527787 [details] [diff] [review] [review] [review] > > address Peter's comment > > > > Review of attachment 527787 [details] [diff] [review] [review] [review]: > > ----------------------------------------------------------------- > > > > ::: l10nstats/models.py > > @@ +83,5 @@ > > > warnings = models.IntegerField(default=0) > > > completion = models.SmallIntegerField(default=0) > > > > > > + @property > > > + def allmissing(self): > > > > Would be nice to see a comment about WHY this property is added. After all, > > it adds "bloat" to an otherwise pure and simple model. > > > > Is it because you can't do the + in the templates? > > > > ::: l10nstats/templatetags/run_filters.py > > @@ +59,5 @@ > > > + return mark_safe("&nbsp;") > > > + fmt = ('<a %s href="' + > > > + reverse('l10nstats.views.compare')+ > > > + '?run=%d">%s</a>') > > > + missing = run.missing + run.missingInFiles > > > > Either this is right or it should be "missing = run.allmissing" > > > > ::: shipping/models.py > > @@ +111,5 @@ > > > + (ACCEPTED, 'accepted'), > > > + (REJECTED, 'rejected'), > > > + (CANCELLED, 'cancelled'), > > > + (OBSOLETED, 'obsoleted'), > > > + ) > > > > Excellent! > > > > ::: shipping/templates/shipping/signoff-rows.html > > @@ +88,5 @@ > > > + <td class="diff {{flag}}">{% if so.diffbases %}<div class="diffanchor"></div>{% endif %}</td> > > > + </tr> > > > + {% endwith %}{% endfor %} > > > + {% else %} > > > + <tr class = "signoffrow hidden" data-push="{{push.id}}"> > > > > can that excess whitespace around the equal sign cause problems? > > > > ::: shipping/templates/shipping/signoffs.html > > @@ +278,5 @@ > > > + </tr> > > > + <tr> > > > + <td></td> > > > + <td> > > > + <input type="checkbox" name="clear_old" value="yes"disabled> > > > > whitespace missing between "yes" and disabled > > > > ::: shipping/views/signoff.py > > @@ +52,5 @@ > > > +from shipping.views import _signoffs > > > +from l10nstats.models import Run, Run_Revisions > > > + > > > + > > > +class __RowCollector: > > > > Why the double __? > > Somewhat private but not totally? I never really thought about it, but the > __ pattern helps me hack things on the ldap backend, while I think _ would > hide that completely? > The double prefixed __ makes the method unavailable in subclasses so it has a very strong use. The single prefixed _ just indicates that it's a private method but doesn't really have much of an effect. The double prefix should only really be used for rare exceptional cases. To this day I've never needed it for anything real-world. In the backends.py example it actually does make sense because of those `import ldap; ...; ldap.set_option(...)` things which aren't pretty but do make sense. > > @@ +228,5 @@ > > > + """ > > > + try: > > > + # rev query arg is required, it's not a url param for caching, and because it's dynamic > > > + # in the js code, so the {% url %} tag prefers this > > > + rev = request.GET['rev'] > > > > If it's required, shouldn't it also check that it's non-blank? > > I should unify the error handling there, need to think about that a bit. > Also, I don't handle the 404s in the signoff ajax so far :-/

I've fixed a good deal of comments in follow up landings, best seen on the network graph, I guess, https://github.com/mozilla/elmo/network. I intend to rebase them all into one patch for final landing, I just keep them separate now as that shows more clearly which review comments got addressed.

I have a general comment about the variable names, especially in views/signoff.py. I think it doesn't help the readability of the code to use all these 2- or 3-letter-long variable names that doesn't mean anything if take out of context. I'd suggest to rewrite some of the code to be more readable, or we will end up writing signoff3 from scratch, because we won't understand what's going on in signoff2 :) Here's an example to illustrate what I have in mind: current version: ================ > #defined earlier > pcs = Push_Changesets.objects.filter(push__in=_p).order_by('-push__push_date','-changeset__id') > > # get latest runs for our changesets > csl = list(pcs.values_list('changeset__id',flat=True)) > rrs = Run_Revisions.objects.filter(run__tree=appver.tree_id, > run__locale=lang, > changeset__in=csl) > rrs = rrs.order_by('changeset','run') > c2r = dict(rrs.values_list('changeset', 'run')) > r2r = dict((r.id, r) > for r in Run.objects.filter(id__in=c2r.values()).select_related('build')) rewritten version: ================== > # defined earlier > changesets_in_pushes = Push_Changesets.objects \ > .filter(push__in=_p) \ > .order_by('-push__push_date', > '-changeset__id') > > # get latest runs for our changesets > > changesets_ids = changesets_in_pushes.values_list('changeset__id', flat=True) > changesets_ids = list(changeset_ids) > > changesets_in_runs = Run_Revisions.objects \ > .filter(run__tree=appver.tree_id, > run__locale=lang, > changeset__in=changesets_ids) \ > .order_by('changeset','run') \ > .values_list('changeset', 'run') > changesets_in_runs = dict(changesets_in_runs) > > runs = ((r.id, r) for r in Run.objects \ > .filter(id__in=c2r.values()) \ > .select_related('build')) > runs = dict(runs) Also, as per pep8, it would be great if the code respected the 79-character line length limit. It makes using the vertical splits in the editor so much easier! (In reply to comment #37) > Rejected comes with a message, or should. The intended use case for this are > those localizers that sign off on every other revision and still have 500+ > missing strings. There, I'd go in and reject the sign-off with a single > message, and cancel the others. As opposed to "this one is bad, but I should > review the previous one still, as you busted search on the latest one". This sounds good, thanks for providing the rationale. > Follow up bug? I'm not sure that's right, nor easy to get right. Looking at > the data, you're all of > > Stas Malolepszy (stas@mozilla.com> > Stas Malolepszy <stas@mozilla.com> > Staś Małolepszy :stas <stas@mozilla.com> > Staś Małolepszy <stas@mozilla.com> Yeah, I'll file a follow-up. > > Hide it entirely if user prefs are not sufficient? > > There have been too many questions on hidden UI due to not being logged in. > I think it's also informative for the user to see "there's something about a > review now, but I can't do that". If it's disabled, it might look like broken. The recent problems with hidden UI were due to our group perms problems, which I don't expect here at all. I'd suggest hiding. > Because it's not implemented yet. Not sure if that's a good follow up bug or > if we need to get that now? I'd hide it, or disable with the value on the button saying "More pushes (doesn't work yet)". > I hate the permission_required decorator, with passion. I also don't think > it's doing the right thing for us, as it's pretty tightly integrated into > the standard UX thinking about logging in. As per IRC, the permission_required decorator redirects to the login page on failuer, which doesn't play nicely with our dynamically loaded login form. So, agreed.

Landed, releases, deployed on -dev- and -stage-. Marking FIXED :-) https://github.com/mozilla/elmo/commit/104e9d47a27ae4b5c1c368be72efa9840da1c0c2 https://github.com/mozilla/elmo/commit/84e7739a1a52049502c2339b3aa7958ad4c21f32