Remove captcha field on WagtailCaptcha and uses AbstractEmailForm to … #2
Conversation
…process form - Added constant with captcha field name on WagtailCaptchaFormBuilder - Using constant from WagtailCaptchaFormBuilder on Model to check if it is necessary to remove the field - Fix #1
| def __init__(self, fields): | ||
| super(WagtailCaptchaFormBuilder, self).__init__(fields) | ||
| # Add wagtailcaptcha to FIELD_TYPES declaration | ||
| self.FIELD_TYPES.update({'wagtailcaptcha': self.create_wagtailcaptcha_field}) | ||
| self.FIELD_TYPES.update({self.CAPTCHA_FIELD_NAME: self.create_wagtailcaptcha_field}) |
There was a problem hiding this comment.
Also need to do the same at line 21:
fields['wagtailcaptcha'] = ReCaptchaField(label='')
to:
fields[self.CAPTCHA_FIELD_NAME] = ReCaptchaField(label='')
There was a problem hiding this comment.
Nice catch. I will fix it when I get to my computer.
| if WagtailCaptchaFormBuilder.CAPTCHA_FIELD_NAME in form.fields.keys(): | ||
| form.fields.pop(WagtailCaptchaFormBuilder.CAPTCHA_FIELD_NAME) | ||
|
|
||
| return super(WagtailCaptchaEmailForm, self).process_form_submission(form) |
There was a problem hiding this comment.
So that the captcha doesn't propagate to database, please also add this exact same function to WagtailCaptchaForm.
There was a problem hiding this comment.
I was wrong about this.
def process_form_submission(self, form):
FormSubmission.objects.create(
form_data=json.dumps(form.cleaned_data, cls=DjangoJSONEncoder),
page=self,
)
Parent's class uses form.cleaned_data, so removing the field from form.fields will not remove the wagtailcaptcha (captcha field & value) from database. It will only remove from the e-mail sent. Based on this, we don't need to propagate it to WagtailCaptchaForm.
Do you agree @Adrian-Turjak and @thibaudcolas ?
There was a problem hiding this comment.
Looking at the django-recaptcha code, looks like it will still propagate the captcha challenge. It doesn't matter too much if that data is in the db, but if we want it not stored we can always instead just do:
def process_form_submission(self, form):
if WagtailCaptchaFormBuilder.CAPTCHA_FIELD_NAME in form.cleaned_data.keys():
form.cleaned_data.pop(WagtailCaptchaFormBuilder.CAPTCHA_FIELD_NAME)
return super(WagtailCaptchaEmailForm, self).process_form_submission(form)
At least that way the code isn't entirely duplicated. Although now I'm curious why AbstractEmailForm isn't using cleaned_data...
There was a problem hiding this comment.
I don't think we really care about the captcha being saved or not. I created the discussion about saving/not saving the captcha code on the database because I thought the refactoring would change the current behaviour but it doesn't. Therefore the to_address not being correctly handle is solved and the behaviour is preserved, I think we are good to go, but I would like your opinion as well.
In my opinion the discussion about saving/not saving the captcha on the database should be done in a different Issue/PR.
PS: Yes, I'm also curious about why wagtail used fields data directly instead of using cleaned_data.
|
Adrian's suggestions look very sensible. I do not know this codebase at all so un-assigning myself for now. There doesn't seem to be any tests in this repo so I guess we could use https://github.com/springload/madewithwagtail as our test case before releasing the new version. |
|
Based on discussion, patch looks good to merge. Can test later today against my own project to confirm, but it's a simple and small enough change. @thibaudcolas probably want to test against madewithwagtail to confirm it doesn't break anything just in case. |
|
I'm a noob with python dependencies, would one of you mind explaining me how to install this from GitHub / from a local checkout in an existing virtualenv? Or pointing to a good resource about that topic. |
|
Not on my machine but installing it on your machine with virtualenv enabled pip install git+git://github.com/springload/wagtail-django-recaptcha.git@bug/send-email http://stackoverflow.com/questions/8247605/configuring-so-that-pip-install-can-work-from-github |
|
I'm expecting to take care of this sometime this week, will merge and then release an update on pypi once it's tested. |
|
Tested on 🎉 ! Thank you! |
* Update README for latest Wagtail versions (#25) * Updated tests and documentations * Fixed Wagtail version until 4.1 for this PR, updated django url to re_path * Ran updatemodulepaths, isort * Linting * Silence warnings * Added migration file from upgrade * Fix failing tests * Linting * Silence more warnings * Add github actions * Fixed .travis.yml to reflect updated tox.ini * Remove SessionAuthenticationMiddleware https://docs.djangoproject.com/en/2.0/releases/2.0/#miscellaneous --------- Co-authored-by: benoitvogel <benoit.vogel@gmail.com> Co-authored-by: Katherine Domingo <katherine.domingo@torchbox.com> Co-authored-by: Nick Moreton <nick.moreton@torchbox.com>
…process form