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

switch pk check to test for None #94

Closed
wants to merge 2 commits into from
Closed

switch pk check to test for None #94

wants to merge 2 commits into from

Conversation

alee
Copy link
Contributor

@alee alee commented Apr 6, 2018

models with a pk of 0 cannot be saved with the existing pk check, see also
https://code.djangoproject.com/ticket/2160

- models with a pk of 0 cannot be saved with the existing pk check, see
https://code.djangoproject.com/ticket/2160
@gasman
Copy link
Contributor

gasman commented Apr 6, 2018

Thanks @alee! Please can you update the tests in https://github.com/wagtail/django-modelcluster/tree/master/tests to cover this case?

@alee
Copy link
Contributor Author

alee commented Apr 6, 2018

Sure, would be happy to - is test_cluster.py the right home for this?

@gasman
Copy link
Contributor

gasman commented Apr 6, 2018

Yep, that's right.

- models with a pk of zero should still be able to commit their members
(e.g., legacy data imports)
- models with None cannot commit their members but `parent.save()` will
still work as Django will save a clone of that object (assuming no
uniqueness or integrity constraints are violated)
@alee
Copy link
Contributor Author

alee commented Apr 24, 2018

Done, though I'm not sure I did a very good job with the tests 😨

The scenario we encountered this in involved a data migration with legacy data that has some instances with a pk of 0. These were converted into a Model that uses django-modelcluster to handle tagging. The instances with a pk of 0 can't be saved from the CLI or wagtail admin because they fail the if not pk check.

https://sentry.commons.asu.edu/share/issue/9300febee94c45819c3e1df8cfa61112/

gasman added a commit that referenced this pull request Aug 7, 2018
@gasman
Copy link
Contributor

gasman commented Aug 7, 2018

Thanks again! Merged in 9168a8f.

@gasman gasman closed this Aug 7, 2018
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

2 participants