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

Register user with form's save method, whenever possible #15

Merged
merged 1 commit into from Mar 12, 2015

Conversation

laginha
Copy link
Contributor

@laginha laginha commented Oct 23, 2014

Hey,

In a nutshell, as I added to the CHANGELOG:

  • Feature: Added settings' options that allows to exlude the default auth urls (INCLUDE_AUTH_URLS) and register url (INCLUDE_REGISTER_URL).
  • Feature: Added support to dynamically import any chosen registration form using the settings option REGISTRATION_FORM.
  • Enhancement: Make RegistrationForm a subclass of Django's UserCreationForm
  • Enhancement: Use registration form save method to create user instance, whenever form is a subclass of Django's ModelForm.

Basically I wanted to use your useful app in a project I'm working on, but with a few twists:

  • I didn't want to use the register/ url, because I have two distinct profiles each one with a different register form and view.
  • I wanted to use a registration form of my own, which is a subclass of ModelForm, with logic in the save method, associated with a custom user with no username field.

I've run the tests and everything is working. I've also updated the docs, version number and changelog.

Hope you approve my work.
If you have any doubt, please contact me and I'll try to explain things better.

DLM

@macropin
Copy link
Owner

Can you please fix this so Travis passes the tests on Django < 1.7. Thanks. I'll then take a closer look.

At a quick glance the REGISTRATION_FORM changes look good. However, I'm not sure about how generally useful the INCLUDE_REGISTER_URL / INCLUDE_AUTH_URLS changes are, for those that need these they could be handled included in a custom url.conf.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.87%) when pulling 824a059 on laginha:master into 623baa7 on macropin:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.87%) when pulling 49bc170 on laginha:master into 623baa7 on macropin:master.

@laginha
Copy link
Contributor Author

laginha commented Oct 30, 2014

Hey,

It seems there is no import_string (nor import_by_path) for Django < 1.5...weird. Would you be ok if I just copied that function to registration/utils.py and use it for any Django < 1.7 ? Not ideal I know, but I just want to hear your thoughts on this issue.

Regarding INCLUDE_REGISTER_URL, as I tried to explain, I didn't want the default /register/ url to exist. Instead I wanted /register/<profile_name>/. That is why I found it interesting. Regarding INCLUDE_AUTH_URLS, I just thought it could be equally useful.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.68%) when pulling 62e115c on laginha:master into 623baa7 on macropin:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.78%) when pulling c170cd4 on laginha:master into 623baa7 on macropin:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.78%) when pulling beadbf5 on laginha:master into 623baa7 on macropin:master.

@pauloxnet
Copy link

I succesfully used this pull request's code in my project for save first_name and last_name fields in registration form.

django 1.5 compatibility issue
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.72%) when pulling 7673925 on laginha:master into 623baa7 on macropin:master.

@macropin
Copy link
Owner

Sorry it's taken me a while to get back to reviewing this. It's a big commit so I'd like to look at it carefully. Are the urlconf defaults aligned to the current behaviour?

@laginha
Copy link
Contributor Author

laginha commented Feb 20, 2015

I did not understand what you mean...

The urlconf defaults behaves as it did before, by default. The new settings I included (INCLUDE_REGISTER_URL and INCLUDE_AUTH_URLS) are optional.
Every other changes I made are backward compatible, I believe.

@macropin macropin merged commit 7673925 into macropin:master Mar 12, 2015
macropin added a commit that referenced this pull request Mar 12, 2015
… into laginha-master

Conflicts:
	registration/backends/simple/views.py
	registration/models.py

Refs #15
@macropin
Copy link
Owner

Can you please also take a look at default/views.py. That is not using form.save(). Thanks.

@laginha
Copy link
Contributor Author

laginha commented Mar 12, 2015

It is if form has a save method. Did I understand you wrong?

@macropin
Copy link
Owner

Nevermind. I think I was lacking caffeine yesterday. :)

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.

None yet

4 participants