Skip to content

Conversation

@syldb
Copy link

@syldb syldb commented Feb 13, 2018

Review

  • Tests
  • Docs/comments
    • IN CASE YOU'VE CHANGED THE formidable.yml file: run tox -e swagger-statics to rebuild the swagger static files and commit the diff.
  • CHANGELOG.rst Updated
  • Delete your branch

@syldb syldb requested a review from alexdashkov February 13, 2018 10:42
@syldb
Copy link
Author

syldb commented Feb 13, 2018

Done in pair with @brunobord

Copy link
Contributor

@alexdashkov alexdashkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thank you, guys. I have a few little comments but they are not critical.
Also, you should squash your fixup commit (799536d).

/forms/{id}/validate/:
get:
summary: Validate a form
summary: Validate a form (GET method). GET and POST are equivalent.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add some comment that GET method will be removed soon?
Validate a form. GET and POST are equivalent, but GET is deprecated

return self.form_invalid(form)

post = get

Copy link
Contributor

@alexdashkov alexdashkov Feb 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to inverse the methods (as we will remove the GET method soon).

def post(self, request, **kwargs): 
     # all the code goes here

get = post

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, also added a deprecation warning as @brunobord suggested!

GET method is deprecated in favor of POST
"""
warnings.simplefilter('always', category=DeprecationWarning)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why adding a simple filter here? let the user's default, it should be fine.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was done this way on PeopleAsk, I removed it

Copy link
Contributor

@brunobord brunobord left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small change in the Changelog to be added.

CHANGELOG.rst Outdated
master (unreleased)
===================

- Allow POST method for form validation endpoint.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, there's a deprecations.rst document in the docs/ directory. I think we should do the following:

  1. Add a deprecation note here, in the Changelog, to inform the reader that this version (probably 1.3.0) will be the last to support the GET verb for the validation endpoint.
  2. When we'll do the release, we'll add this point to the deprecations.rst document with the correct version number.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changelog has been updated :)

CHANGELOG.rst Outdated
===================

- Allow POST method for form validation endpoint.
- Allow POST method for form validation endpoint. GET method is now deprecated, it's support will be dropped in the next version.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer active voice:

Deprecation Warning: This version is the last to support the GET verb for the validation endpoint. We'll drop it in the next version.

@brunobord
Copy link
Contributor

don't forget to squash these!

@syldb syldb force-pushed the allow-post-method-validation-endpoint branch from 1c1f6b0 to 6da054b Compare February 13, 2018 15:37
@syldb syldb force-pushed the allow-post-method-validation-endpoint branch from 6da054b to 42dc9dc Compare February 13, 2018 15:39
@syldb syldb merged commit 77ad92c into master Feb 13, 2018
@syldb syldb deleted the allow-post-method-validation-endpoint branch February 13, 2018 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants