910381 - Collection creation without region/carrier fails with a 500

Status: VERIFIED FIXED

(Marketplace Graveyard :: API, defect, P2)

Description

[krupa raj[:krupa]]

steps to reproduce:
1. Load https://marketplace-dev.allizom.org/curation/new_collection?type=operator
2. Try to create a collection after filling in all the required fields. Make sure to leave the region/carrier section empty.

expected behavior:
collection creation is successful

observed behavior:
Collection creation fails with a 500

[11:31:57.335] POST https://marketplace-dev.allizom.org/api/v1/rocketfuel/collections/?_user=kraj%40mozilla.com%2C3c51af17bfcb5c4d62d2bd781fda327c58d707147858405179d2a1002a0739cc094e4ede0880e84cb32a323694393d927d77cdce59a45073fbb318936b01dd65%2C2fdc85b495484ddfb9a77b81d1d38f0d&carrier=carrierless&lang=en-US [HTTP/1.1 500 INTERNAL SERVER ERROR 1909ms]

Comment 1

[Christopher Van Wiemeersch [:cvan]]

(1) Add HTML5 `required` attributes to Region and Carrier fields.
(2) Disallow the API from ever 500ing if region and/or carrier are missing.

Comment 2

[Matt Basta [:basta]]

Region and carrier are not required and should not be required.

Comment 3

[Christopher Van Wiemeersch [:cvan]]

(In reply to Matt Basta [:basta] from comment #2)
> Region and carrier are not required and should not be required.

For operator shelf? I thought carrier *was* required for operator shelf?

Comment 4

[Matt Basta [:basta]]

Sorry, region is optional. Carrier for OSCs is not. The API doesn't necessarily need to enforce that, though.

Comment 5

[Mathieu Pillard [:mat]]

It's definitely weird that it's 500ing anyway, it shouldn't do that whatever parameters we pass or omit :)

Comment 6

[Mathieu Pillard [:mat]]

http://sentry.dmz.phx1.mozilla.com/addons/marketplace-dev/group/16674/

Comment 7

[Mathieu Pillard [:mat]]

This is an issue with Django-Rest-Framework https://groups.google.com/forum/#!msg/django-rest-framework/g3Mnp7YY5hU/_LwTqKf8-Z0J

It tries to do some magic with the choices and that doesn't work well for us. I'll find a workaround.

Comment 8

[Mathieu Pillard [:mat]]

I have a workaround but it's too ugly, I submitted a pull request to DRF and I'm waiting to see if it's accepted and if people disagree with it to check if that's the right way to fix it.

https://github.com/tomchristie/django-rest-framework/pull/1074

Depending on how fast things are going we might need to apply my ugly workaround or use a fork of drf to get this fixed.

Comment 9

[Matt Basta [:basta]]

(In reply to Mathieu Pillard [:mat] from comment #8)
> I have a workaround but it's too ugly, I submitted a pull request to DRF and
> I'm waiting to see if it's accepted and if people disagree with it to check
> if that's the right way to fix it.
> 
> https://github.com/tomchristie/django-rest-framework/pull/1074
> 
> Depending on how fast things are going we might need to apply my ugly
> workaround or use a fork of drf to get this fixed.

Well that was fast!

If it doesn't get merged/tagged into a stable version soon, I'm not terribly concerned about leaving this bug hanging. We now enforce `carrier` to be non-null in the front-end, so this shouldn't happen unless someone is manually hitting the API.

Comment 10

[Mathieu Pillard [:mat]]

It can also happen with 'region' though. I think a workaround that could work would be to completely omit it from the POST/PUT/PATCH data when null instead of including it with an empty value. 

In the meantime, I've submitted a pull request on zamboni switch our django-rest-framework requirement to point to the commit that has the fix while we wait for a stable release, on a suggestion from kumar. https://github.com/mozilla/zamboni/pull/1068

Comment 11

[Mathieu Pillard [:mat]]

Fixed in https://github.com/mozilla/zamboni/commit/a436ab7e23eb4cb2cb14239fe915b8571f141d72

Comment 12

[Iulian Timis]

Verified as fixed in https://marketplace-dev.allizom.org/curation/new_collection?type=operator on FF26 (Win 7).
Postfix screencast http://screencast.com/t/jkVmJGOxV
Closing bug.