Skip to content

Adjusting post_save User model import#7

Open
mauricioabreu wants to merge 1 commit intomfogel:developfrom
mauricioabreu:fix-user-model-import
Open

Adjusting post_save User model import#7
mauricioabreu wants to merge 1 commit intomfogel:developfrom
mauricioabreu:fix-user-model-import

Conversation

@mauricioabreu
Copy link
Copy Markdown
Contributor

Moved post_save signal to init.
Added noqa flake8 to ignore unused imports.

Moved post_save signal to __init__.
Added noqa flake8 to ignore unused imports.
@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.33%) when pulling b8ec639 on mauricioabreu:fix-user-model-import into 551f172 on mfogel:develop.

@mfogel
Copy link
Copy Markdown
Owner

mfogel commented Sep 24, 2014

Thanks for the pull request. I see two parts to it.

The first the NOQA markings for flake8 - I think I've addressed that since this PR was made on develop by adding an __all__ to the module. Please verify.

The second - django 1.7 best practices with the signal registration - it looks like the best practice is explained here: https://docs.djangoproject.com/en/dev/topics/signals/#django.dispatch.receiver Looking at that, I'm thinking something like

if getattr(settings, 'SIMPLE_EMAIL_CONFIRMATION_AUTO_ADD', True):
    @reciever(post_save, sender=get_user_model())
    def auto_add(sender, **kwargs):
        # same as before

in simple_email_confirmation/signals.py

Then, per https://docs.djangoproject.com/en/dev/ref/applications/#configuring-applications we should define a AppConfig subclass that imports signals.py in it's ready() method. Also, default_app_config should be configured to point to that AppConfig subclass.

Final, I think the signal definitions currently in simple_email_confirmation/signals.py should probably be moved to models.py to ensure they're always imported, even in django < 1.7.

Thoughts?

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.

3 participants