774714 - [shipping] create_milestones should error on re-creating existing milestones

Status: RESOLVED FIXED

(Webtools Graveyard :: Elmo, defect, P1)

Description

[[e-mail] (:bhearsum)]

When I hit https://l10n.mozilla.org/shipping/confirm-ship?ms=fx15_beta_b1 I get an ISE 500. This is blocking the Thunderbird and Firefox 15.0b1 releases.

Comment 1

[Axel Hecht [:Pike]]

Moving over to elmo.

The cause is multiple generation of the same milestone code, the code should error on that.

Comment 2

[[e-mail] (:bhearsum)]

Not sure if you did something, but I was able to ship the milestones for Firefox/Fennec a few minutes ago.

Comment 3

[Peter Bengtsson [:peterbe]]

Attachment: proper constant

This patch doesn't solve the problem.

I think the problem is that I don't understand what the problem is. You can't accidentally POST to ship_mstone twice because the status flag changes and thus prevents it from being changed again since it's already shipped.

Comment 4

[Axel Hecht [:Pike]]

Comment on attachment 648405 [details] [diff] [review]
proper constant

Yeah, this patch doesn't have anything to do with this bug.

The problem is that Milestone.code is supposed to be unique, but isn't in https://github.com/mozilla/elmo/blob/develop/apps/shipping/models.py#L172, and that shipping.views.release.create_milestones in https://github.com/mozilla/elmo/blob/develop/apps/shipping/views/release.py#L176 doesn't enforce that.

Comment 5

[Peter Bengtsson [:peterbe]]

Attachment: prevention of duplicate code on Milestone

Personally I prefer we return a 400 Bad Request code instead of raising a 5xx error. 
Also, I think the same with regards to the `code` or `name` parameters missing.

Comment 6

[Axel Hecht [:Pike]]

Comment on attachment 649412 [details] [diff] [review]
prevention of duplicate code on Milestone

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

r-, the code wants to do the right thing, but the release.py chunk is actually not indented right, or does the wrong query at the indention level.

::: apps/shipping/tests/test_release.py
@@ +60,5 @@
> +        response = self.client.post(url, no_name_data)
> +        eq_(response.status_code, 404)
> +
> +        response = self.client.post(url, new_milestones)
> +        eq_(response.status_code, 302)

Move this part of the test up to follow the appver creation right away? I found that confusing when reading the test.

Adding a few more comments as to why you test which things at which point would be good, I often thought "oh, wrong test" until I figured out which 404 you're hitting just now (yeah, 404 is probably a poor error in most cases, doh me).

::: apps/shipping/views/release.py
@@ +195,5 @@
>              raise Http404
> +
> +    # check that no milestone has been created for this code
> +    if Milestone.objects.filter(code=details['code']).exists():
> +        raise ValueError("Milestone for %r already created" % details['code'])

Indention is wrong here.

I'd also favor something like

code__in=[details['code'] for details in new_miles.itervalues()]

so that we get along with just one query for those.

Comment 7

[Peter Bengtsson [:peterbe]]

Attachment: fix for indentation and more comments on the unit test

Comment 8

[Peter Bengtsson [:peterbe]]

Attachment: 400isms

voila!

Comment 9

[[github robot]]

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

https://github.com/mozilla/elmo/commit/e5a5a930149bfa452931c05d196d323417179b05
bug 774714 - create_milestones should error on re-creating existing milestones, r=Pike