1127428 - fix commented out test for requiring boolean

Status: RESOLVED FIXED

(Input Graveyard :: Code Quality, defect)

Description

[Will Kahn-Greene [:willkg] ET needinfo? me]

We have a test that I commented out because DRF had a bug where you couldn't specify that a BooleanField was required--the code didn't work right.

https://github.com/mozilla/fjord/blob/625ca6e52645747c7bc21670b0182704d4457961/fjord/feedback/tests/test_api.py#L511-L514

Ricky is updating DRF to a more recent version in bug #934979, but says that that test still fails.

This bug covers looking into that and figuring out why it's still not working.

Comment 1

[Will Kahn-Greene [:willkg] ET needinfo? me]

You'll want to run the tests locally to verify that all the tests pass. After doing that, find the test in question (see bug description) and remove the # which comment the test out. Run the tests again.

If the test passes, then everything is good and you can create a PR with the code changes.

If the test fails, then look into why the test is failing and fix it.

Marking this as a mentored bug. If you're interested in working on it, please read through our "Join this project" guide [1], complete the list of things you need to do before contributing and then add a comment to this bug asking to have it assigned to you.

If you have any questions, please ask in the bug comments, on the mailing list or in the #input channel on irc.mozilla.org.

[1] http://fjord.readthedocs.org/en/latest/welcome.html

Comment 2

[dlucian93]

The test name says that when "happy" is missing from the POSTed data then it should return 400, but in the model declaration the "happy" field has the default value set to True which means that it shouldn't return 400. Then the test needs to be changed to "test_missing_happy_does_not_return_400". Am I right?

Comment 3

[Will Kahn-Greene [:willkg] ET needinfo? me]

The API uses the ResponseSerializer which has the happy field as required and doesn't set a default:

https://github.com/mozilla/fjord/blob/e165f49bc4094790cb94e08ea3160df852770ffd/fjord/feedback/models.py#L627

The problem is that the version of DRF that we were using when I implemented things would ignore the required thing.

So, no... the test shouldn't get changed to what you're suggesting. The correct behavior for the API is that if you don't provide a happy field, then you get back an HTTP 400.

Comment 4

[Will Kahn-Greene [:willkg] ET needinfo? me]

I looked into this again and discovered the issue was a little trickier than I expected.

Nixing this as a mentored bug and grabbing it.

In a PR: https://github.com/mozilla/fjord/pull/539

Comment 5

[Will Kahn-Greene [:willkg] ET needinfo? me]

Landed in https://github.com/mozilla/fjord/commit/47db8d6028fff7a9d45ac283b6517d447ac3cdcc

This only affects tests, so I'm going to mark it FIXED now.