Skip to content
This repository has been archived by the owner on Jun 10, 2024. It is now read-only.

Management command "usersettings_init" #2

Closed
wants to merge 3 commits into from

Conversation

akolpakov
Copy link

Allow you to initialize db records for your USERSETTINGS_MODEL.
With tests.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.5%) when pulling e6d0ade on akolpakov:usersettings_init into e1f75d6 on mishbahr:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) when pulling 4d9b388 on akolpakov:usersettings_init into e1f75d6 on mishbahr:master.

@mishbahr
Copy link
Owner

Im not sure how this passed all the tests!

UserSettings model used for testing::

USERSETTINGS_MODEL='example.SiteSettings'

SiteSettings has many required attributes (e.g. street_address, address_locality, postal_code ... etc ) which is not provided when creating UserSettings object in the management command::

See https://github.com/akolpakov/django-usersettings2/blob/usersettings_init/usersettings/management/commands/usersettings_init.py#L25

So the test should fail. Please correct me if I'm wrong.

EDIT: Thank you for your pull request.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.53%) when pulling 7fd7d9a on akolpakov:usersettings_init into e1f75d6 on mishbahr:master.

@akolpakov
Copy link
Author

So the test should fail. Please correct me if I'm wrong.

If you look at django function https://github.com/django/django/blob/master/django/db/models/fields/__init__.py#L720 you'll see that mostly return empty string when no default. Thats why model can be created without defining fields.

But I've got your message and made some checks.

@mishbahr
Copy link
Owner

Thank you for a quick response. I did not know about get_default returning an empty string. Thats very interesting .. you learn something everyday!

However, this management command would still fail (to batch add usersettings) if the UserSettings model has any field that does not allow empty_strings as default. For example, a BooleanField or IntegerField ...etc.

While I realise, you have added a try/except block to catch the exception in such scenario. This management command will still be useless for a most people (i.e anyone who is using a field that does not allow empty strings as default). Hence I don't think it would be beneficial to merge this pull into master at this stage. Thank you again for you contribution to this project.

@akolpakov
Copy link
Author

So, it was my suggestion. Because for me it is very useful.
I create issue #3 where describe a problem and real case of usage. If you will find some better solution, please let me know
Thanks!

@mishbahr
Copy link
Owner

I close this for now. I'll take this idea of into consideration for the next release, see if I can figure out a way to batch create usersettings.... may be using some sort of fixtures.

I'll look into issue #3 .. that's a big oversight in my side (Sorry). I should have thought of that. I will push a fix as soon as I can. Hopefully, all I'll have to do is a put try/except block to catch DoesNotExist error and return None for the get_current_usersettings() shortcut method.

@mishbahr mishbahr closed this Sep 25, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants