544366 - [shipping] localizer should have a way to cancel a signoff

Status: RESOLVED FIXED

(Webtools Graveyard :: Elmo, defect, P2)

Description

[Axel Hecht [:Pike]]

Right now, there's no way to actually cancel an erroneous signoff.

Should have one, though.

P2, as it cause real-world confusion already.

Comment 1

[Cédric Corazza]

Axel, well, signing with a previous or a new patchset should fix this, right?

Comment 2

[Axel Hecht [:Pike]]

For one, this is what wladow just did, and the only way that this shows is that the drivers are asked to accept a sign-off without a diff. Really cumbersome in the current layout.

The other piece is, if there's no good sign-off to get back to, you're just lost in the woods.

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

one thought. except of obvious need to be able to back out signoff, maybe we should use some logic to detect that signoff is requested against last accepted one and just bring it back?

If we accepted something, and the localizer is trying another, and then he clicks back on the accepted one, this should get him back to the entry point. And that could be yet another way to cancel a signoff request.

Comment 4

[Axel Hecht [:Pike]]

Technically, I think we should stick to the ID to be a measure of time.

Personally, I think that going from an accepted sign-off to a previously accepted sign-off should require approval, the previous one doesn't need to be good anymore. You could miss bug fixes or productization changes etc.

So, Cancel should probably really only be an option for pending sign-offs.

Comment 5

[Peter Bengtsson [:peterbe]]

Attachment: UI for cancelling a pending signoff

Be kind. This is my first time around this code. 

So, first it looks like this: http://cl.ly/JLYb
Then like this: http://cl.ly/JMjm
Then of course, back to this: http://cl.ly/JLzN

Help me, should it perhaps instead of deleting the whole Signoff, just delete the last action that isn't a pending one?

Comment 6

[Axel Hecht [:Pike]]

Comment on attachment 659897 [details] [diff] [review]
UI for cancelling a pending signoff

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

Hi there, got a couple of comments.

Primarily, the folks that are offered the cancel button should be localizers, which changes the permission to key this off to add_signoff.

Also, canceling a sign-off is actually designed to be an additional Action, not actually removing the data. Thus way, there's a track record of what's happening left in the db.

I know that I started with the triple data stuff, but I wonder if we need to redo this for this code, that is, use all of signoff_id, locale code and appversion code. I'm not sure I see the value of the three params at this point. I know I did the same thing for review_signoff, but looking at it, it feels funny.

Also, because this happens all the time, please add a check that the latest Action isn't a CANCELED action, too.

::: apps/shipping/templates/shipping/signoff-rows.html
@@ +51,5 @@
>                type="button" value="review…"
>                {% if not perms.shipping.review_signoff %}disabled{% endif %}>
> +              <input class="cancel_signoff" data-signoff="{{so.signoff.id}}"
> +              data-state="{{flag}}"
> +              type="button" value="cancel sign-off"

I'd make the value just "Cancel&hellip;".

@@ +52,5 @@
>                {% if not perms.shipping.review_signoff %}disabled{% endif %}>
> +              <input class="cancel_signoff" data-signoff="{{so.signoff.id}}"
> +              data-state="{{flag}}"
> +              type="button" value="cancel sign-off"
> +              {% if not perms.shipping.review_signoff %}disabled{% endif %}>

The right permission is add_signoff.

Also, I think we should just hide this button if the signoff status isn't PENDING.

::: apps/shipping/templates/shipping/signoffs.html
@@ +57,5 @@
> +<div id="cancel_signoff" style="display:none">
> +  <form id="cancel_signoff_form" method="post"
> +  action="{% url shipping.views.signoff.cancel_signoff language.code appver.code %}">{% csrf_token %}
> +  <input type="hidden" name="signoff_id" value="">
> +  <p>Pending sign-offs can be cancelled.</p>

This string should be a plain action, like

'Cancel sign-off'

::: apps/shipping/views/signoff.py
@@ +243,5 @@
>      return _redirect
> +
> +
> +@require_POST
> +def cancel_signoff(request, locale_code, app_code):

This is an appver code, not an app code here, rename or drop?

@@ +250,5 @@
> +    """
> +    _redirect = redirect('shipping.views.signoff.signoff',
> +                         locale_code, app_code)
> +    if not request.user.has_perm("shipping.review_signoff"):
> +        return _redirect

The permission should be the same as for localizers, thus use 'shipping.add_signoff'

@@ +255,5 @@
> +
> +    lang = get_object_or_404(Locale, code=locale_code)
> +    try:
> +        appver = (AppVersion.objects
> +                .select_related('tree').get(code=app_code))

why the select_related? Also, that smells like pre-data-1.1

@@ +263,5 @@
> +    # permissions are cool, let's check the data
> +
> +    try:
> +        signoff_id = int(request.POST['signoff_id'])
> +        so = appver.signoffs.get(locale=lang, id=signoff_id)

I'd tune all the params into one db query, as in

Signoff.objects.get(id=signoff_id, locale__code=locale_code, appver__code=app_code)

@@ +275,5 @@
> +    if so.status != Action.PENDING:
> +        return HttpResponseBadRequest("Signoff not pending (%s)" % so.flag)
> +
> +    Action.objects.filter(signoff=so).delete()
> +    so.delete()

This should really just add a new Action with Action.CANCELED.

Comment 7

[Peter Bengtsson [:peterbe]]

I've implemented all those changes. Thank you. However, I think there's something I don't really understand about the UI logic. 

First, what I did was make it so that cancel_signoff doesn't remove the actions and the sign-off. Instead it just adds another action on top of type CANCELED. 

However, looking at signoff-rows.html it appears that you only get the "sign off..." button if the push does not have any sign-offs. Independent of the state the sign-off. 
See https://github.com/mozilla/elmo/blob/develop/apps/shipping/templates/shipping/signoff-rows.html#L21

So, it means that once canceled the you can't sign off on it again. Right?

Either there's something intricate I haven't understood or...
Am I just supposed to improve the logic so that it does something like:

  if not push.signoffs or (push.signoffs[0].status in (PENDING, CANCELED))

instead of just:

  if not push.signoffs

(here I'm referring to that same line in the code as above)

Comment 8

[Axel Hecht [:Pike]]

Good catch. I guess we should allow to re-PENDING a CANCELED signoff. I bet the code path for that would look different than 'signoff', though, as it shouldn't create a sign-off, but just add another PENDING action. Maybe toggle the 'cancel' button for that? "Re-request" might be a descent title, as bad English as that is.

Comment 9

[Peter Bengtsson [:peterbe]]

(In reply to Axel Hecht [:Pike] from comment #8)
> Good catch. I guess we should allow to re-PENDING a CANCELED signoff. I bet
> the code path for that would look different than 'signoff', though, as it
> shouldn't create a sign-off, but just add another PENDING action. Maybe
> toggle the 'cancel' button for that? "Re-request" might be a descent title,
> as bad English as that is.

I'm not entirely sure I follow. Even if I do this:

  def cancel_signoff():
      signoff = Signoff.objects.get(...
      Action.objects.create(signoff=signoff,
          status=Action.CANCELED,
          author=request.user)
      Action.objects.create(signoff=signoff,
          status=Action.PENDING,
          author=request.user)

The status of the sign-off will then be PENDING but the logic in the UI still applies. The only thing that enables the signoff... button is whether there are no sign-offs at all.

Comment 10

[Axel Hecht [:Pike]]

I thought the question was if a localizer should be able to sign-off on a push after a sign-off on that push has been cancelled.

I'd say yes, but not by opening a new Signoff entry, but by adding yet another Action.PENDING to the queue of the existing one.

The cancel_signoff() path should be un-affected.

My comment about switching the meaning of the 'cancel' button wasn't really helpful, probably.

Comment 11

[Peter Bengtsson [:peterbe]]

Attachment: Cancel and re-open

http://cl.ly/JX9v

I ran a check and did some clean up which I'm not sure are related.

Comment 12

[Axel Hecht [:Pike]]

Comment on attachment 661874 [details] [diff] [review]
Cancel and re-open

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

This does the right thing, I found two nits about upper-casing the http method, and then I think that for wrong params for lang and appver, we should probably also return a 400 instead of a 404?

Not an r+, but an f+.

::: apps/shipping/templates/shipping/signoffs.html
@@ +85,5 @@
> +</div>
> +{% endif %}
> +{% if perms.shipping.add_signoff %}
> +<div id="cancel_signoff" style="display:none">
> +  <form id="cancel_signoff_form" method="post"

uppercase the POST?

@@ +94,5 @@
> +  </form>
> +</div>
> +
> +<div id="reopen_signoff" style="display:none">
> +  <form id="reopen_signoff_form" method="post"

also, POST.

::: apps/shipping/tests/test_signoff.py
@@ +227,5 @@
> +        )
> +
> +        # not a recognized appversion code
> +        junk_url = cancel_url.replace(appver.code, 'xxx')
> +        eq_(self.client.post(junk_url).status_code, 404)

If we'd go for 400's, we'd need to fix the tests here and in the reopen test, too.

::: apps/shipping/views/signoff.py
@@ +253,5 @@
> +    if not request.user.has_perm("shipping.add_signoff"):
> +        return _redirect
> +
> +    lang = get_object_or_404(Locale, code=locale_code)
> +    appver = get_object_or_404(AppVersion, code=appver_code)

Should we fold these into the HttpResponseBadRequest("Signoff not found")?

@@ +293,5 @@
> +    if not request.user.has_perm("shipping.add_signoff"):
> +        return _redirect
> +
> +    lang = get_object_or_404(Locale, code=locale_code)
> +    appver = get_object_or_404(AppVersion, code=appver_code)

Same here.

Comment 13

[Peter Bengtsson [:peterbe]]

Comment on attachment 661874 [details] [diff] [review]
Cancel and re-open

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

I hope I have used splinter right to respond to the f?

::: apps/shipping/templates/shipping/signoffs.html
@@ +85,5 @@
> +</div>
> +{% endif %}
> +{% if perms.shipping.add_signoff %}
> +<div id="cancel_signoff" style="display:none">
> +  <form id="cancel_signoff_form" method="post"

Not needed. It always gets converted to POST in django's `request.method`.

In fact, a lowercase value is more correct I remember reading somewhere.

::: apps/shipping/tests/test_signoff.py
@@ +228,5 @@
> +
> +        # not a recognized appversion code
> +        junk_url = cancel_url.replace(appver.code, 'xxx')
> +        eq_(self.client.post(junk_url).status_code, 404)
> +

My thinking there was that it's part of the URL. Not part of the query string.

::: apps/shipping/views/signoff.py
@@ +253,5 @@
> +    if not request.user.has_perm("shipping.add_signoff"):
> +        return _redirect
> +
> +    lang = get_object_or_404(Locale, code=locale_code)
> +    appver = get_object_or_404(AppVersion, code=appver_code)

Again, I think since it's part of the URL and not the query string I think a 404 is in order. 

Django's URL regexes protects us and yields 404 if you mess with it. In this case, we snuck in by bypassing the regex but still provided bad values so I think it's still part of getting to an invalid URL.

Comment 14

[Axel Hecht [:Pike]]

Comment on attachment 661874 [details] [diff] [review]
Cancel and re-open

Talked about the 400 vs 404 on the phone, r+ now.

Comment 15

[[github robot]]

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

https://github.com/mozilla/elmo/commit/d5f135d05feda2d1adea1b036bf0a02112ce432d
bug544366 - ability to cancel spending signoffs, r=Pike