Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set proper status code in AdminRenderer for the redirection after POST/DELETE requests. #4106

Merged
merged 1 commit into from
May 16, 2016

Conversation

sasha0
Copy link
Contributor

@sasha0 sasha0 commented May 8, 2016

refs #4032

@xordoquy
Copy link
Collaborator

xordoquy commented May 8, 2016

Seems legit. Will need to properly test to ensure it fixes the original issue.

@xordoquy xordoquy added this to the 3.4.0 Release milestone May 8, 2016
@sasha0
Copy link
Contributor Author

sasha0 commented May 8, 2016

Should I add a test case for it?

@xordoquy
Copy link
Collaborator

xordoquy commented May 8, 2016

I don't think it's necessary. The tests I was referring to are the one with "real" browsers just to make sure it works(tm) since 303 vs 302 difference is browser specific.

@tomchristie
Copy link
Member

I think this thread could do with being a little more specific.
Does this change the browser behavior? If so, how?

@xordoquy
Copy link
Collaborator

xordoquy commented May 9, 2016

@tomchristie Everything is explained in the definition of the 303 code. However, I think there should be a location header which I'm not sure is part of the response.

@tomchristie
Copy link
Member

Fair enough, although still worth confirming if there are any behavioral changes.

@sasha0
Copy link
Contributor Author

sasha0 commented May 10, 2016

@xordoquy don't we set Location header for both types of responses here and here?

@tomchristie
Copy link
Member

I'm happy with this change, once someone's confirmed if it makes any behavioral difference to the browser (ie. actually try it out and report on before and after) - if they're the same then great, let's have this as a valid semantic change. If there's differences in the behavior before and after then we can review those on their merits.

@jpadilla
Copy link
Member

Just confirmed this does fix original issue.

drf-delete

Although behavior still seems off. Shouldn't we be redirecting back to /users/ after deleting?

@tomchristie tomchristie merged commit 1328982 into encode:master May 16, 2016
@tomchristie
Copy link
Member

Shouldn't we be redirecting back to /users/ after deleting?

I think that'd be a valid improvement on DELETE, sure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants