Create 1.4 compatible HttpResponseRedirect object. #1623
Conversation
|
Refer to this link for build results (access rights to CI server needed): |
|
I expected the branch name to be '149' but instead it is 'task149'. All tasks, issues, stories, etc in Redmine share the same numbering sequence so '149' as the branch name specifies some clearly. This does not need to be adjusted for this PR, just something I observed. |
|
Typically the redmine issue is linked to in the PR description. |
| Test HttpResponseRedirect. | ||
| """ | ||
| test_content = {'foo': 'bar'} | ||
| response = util.generate_json_response_with_pulp_encoder(test_content) |
There was a problem hiding this comment.
Can this call to utils.generate_json_response_with_pulp_encoder build the object manually? A simple HttpResponse(content=test_content) should suffice. This way the unit test won't be affected by breakages in utils.generate_json_response_with_pulp_encoder.
There was a problem hiding this comment.
nice catch, thank you :)
|
flake8 and tests look good. I left some comments. Please make some edits and ping me for re-review. |
|
Refer to this link for build results (access rights to CI server needed): |
| redirect_response = util.generate_redirect_response(response, href) | ||
| self.assertEqual(redirect_response.status_code, 201) | ||
| self.assertEqual(redirect_response.reason_phrase, 'CREATED') | ||
| mock_iri_to_uri.assert_called_once_with(href) |
There was a problem hiding this comment.
Try adding an assert statement to verify that the Location header is set to the return value of mock_iri_to_uri. From my own debugging I think the header is stored at redirect_response._headers['location']. I recommend letting mock auto-create the return_value which you can access with mock_iri_to_uri.return_value.
|
@ipanova if you can add that assert statement and the tests pass on all platforms then LGTM. |
|
Refer to this link for build results (access rights to CI server needed): |
|
On 02/10/2015 08:04 AM, bmbouter wrote:
I don't think we need to have a policy about what branch names are used. As long as we format our commit messages well, I think the developer can |
|
@rbarlow I agree with you, but the docs document that we do assert a branch name convention. Should we adjust the docs? |
|
@ipanova LGTM, great job in resolving this. |
|
On 02/10/2015 10:13 AM, bmbouter wrote:
Yes, I also commented on the pull request that created that doc saying I |
https://pulp.plan.io/issues/149