762551 - [shipping] data 1.1 regressed signing off

Status: RESOLVED FIXED

(Webtools Graveyard :: Elmo, defect, P1)

Description

[Axel Hecht [:Pike]]

pff, details, sign-offs don't work no more.

Got a fix in hand, though.

Comment 1

[Axel Hecht [:Pike]]

Attachment: fix signoffs, revision -> push

I've had to redo the logic. The previous logic was keyed off of revision hashes, and a single repo for appversion. Which works, because you can push a changeset only once.

Now, in the rapid world, a changeset can be the tip of the push to aurora, and then the tip of the push to beta.

Which means that the runs need to be more tricky, and that I really need to know which push we're signing off on.

Let's do that then.

Comment 2

[Peter Bengtsson [:peterbe]]

Comment on attachment 631017 [details] [diff] [review]
fix signoffs, revision -> push

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

Some nits on input handling and the jquery lookups.

::: apps/shipping/static/shipping/js/signoffs.js
@@ +39,4 @@
>  
>    function hoverSO(showOrHide) {
>      return function() {
> +      var q = $('input[data-push=' + this.getAttribute('data-push') + ']')

What would make this much faster is:

  $('input[data-push=' + this.getAttribute('data-push') + ']', '#signoff_form'))

it reduces the number of elements to traverse before completing the whole DOM element comparison thingy.

@@ +113,4 @@
>  function doSignoff(event) {
>    event.stopPropagation();
>    var t = $(event.target);
> +  var push = t.attr('data-push');

Shouldn't that be `t.data('push')`?
Maybe that's only possible with jQuery 1.7.

::: apps/shipping/views/signoff.py
@@ +126,1 @@
>      except:

This should be `except (KeyError, ValueError):`

@@ +147,5 @@
> +        if avt.start:
> +            _q['srctime__gte'] = avt.start
> +        if avt.end:
> +            _q['srctime__lte'] = avt.end
> +        q = q is None and Q(**_q) or q | Q(**_q)

Thank you! I've never grasped a better way to do that.

@@ +207,4 @@
>                  # we're not accepting signoffs, someone's hitting urls manually
>                  raise ValueError("not accepting signoffs for %s" %
>                                   app_code)
> +            push_id = int(request.POST['push'])  # ValueError -> 500

try:
    push_id = int(request.POST['push'])
except (KeyError, ValueError), msg:
    return http.HttpResponseBadRequest(str(msg))

Comment 3

[[github robot]]

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

https://github.com/mozilla/elmo/commit/50072237c76ad396fef6d09b63b3bca6136b8456
bug 762551, fix signing off after data1.1, r=peterbe

Comment 4

[Axel Hecht [:Pike]]

I'd rather not use t.data(), it's not equivalent, and it doesn't match up when changing the attribute later, see the tailing end of http://api.jquery.com/data/ (that's consistent since 1.4.3, though).

Also, the data-push queries are for the sign-off buttons on the push rows, so we can't restrict them much. I guess we could make the query more complicated, but at least in gecko, I'd expect that to slow us down, actually.

I found a few more occasions of push.id and so_rev, fixed those, too, and pushed.

Comment 5

[Axel Hecht [:Pike]]

FIXED, and deployed on dev.