maccesch / cmsplugin-contact

Extendable contact plugin for django-cms with spam protection and i18n
BSD 2-Clause "Simplified" License
71 stars 62 forks source link

RECAPTCHA_*KEY semantics #43

Closed Lacrymology closed 11 years ago

Lacrymology commented 11 years ago

with * == (public|private), of course

The form class uses self.recaptcha_*_key as preferred value, falling back to settings.RECAPTCHA_*_KEY when it's absent.

https://github.com/maccesch/cmsplugin-contact/blob/master/cmsplugin_contact/nospam/forms.py#L66

but the plugin, when it instantiates the form, does it the other way around: it uses the setting if present, and falls back to the instance value if it's not

https://github.com/maccesch/cmsplugin-contact/blob/master/cmsplugin_contact/cms_plugins.py#L84

what is the preferred semantics? Is the setting a default, to be overriden by the instance value, or the other way around?

I'd EXPECT it to be the former: have the instance to take precedence before the global setting. Is this a bug? Should I fix it?

mitar commented 11 years ago

Yes, it should be the other way around in https://github.com/maccesch/cmsplugin-contact/blob/master/cmsplugin_contact/cms_plugins.py#L84 Nice catch. If you can, please make a pull request.

Lacrymology commented 11 years ago

It should be an or instead of a getattr, I guess. something like

recaptcha_public_key = instance.recaptcha_public_key or getattr(settings, 'RECAPTCHA_PUBLIC_KEY', None),
...

Or, to make the default at the form make any sense, maybe remove it from the kwargs altoghether, something like

if instance.recaptcha_public_key:
   kwargs['recaptcha_public_key'] = instance.recaptcha_public_key
...
RecaptchaForm(**kwargs)

but that will quickly get ugly and difficult to read.

I'll send a PR tomorrow

mitar commented 11 years ago

I prefer:

recaptcha_public_key = instance.recaptcha_public_key or getattr(settings, 'RECAPTCHA_PUBLIC_KEY', None),