karanlyons / django-save-the-change

Your DB Got It the First Time.
http://django-save-the-change.readthedocs.io
Other
114 stars 29 forks source link

Bug fix: Creating a new object instance causes a DoesNotExist exception #3

Closed hellsgate1001 closed 11 years ago

hellsgate1001 commented 11 years ago

This prevents a DoesNotExists exception when creating a new instance rather than editing an existing one

coveralls commented 11 years ago

Coverage Status

Coverage remained the same when pulling 48a5b0ff298e25ed89587aa7d9ff8323310a93a9 on hellsgate1001:master into 47a3e24c3fa095648a1dc447d778a13f847daa76 on karanlyons:master.

karanlyons commented 11 years ago

Hey @hellsgate1001, I'm not sure I fully understand the bug this solves. Could you add a test showing the behavior it fixes? Also, self.id should be self.pk to support models using custom primary keys. Thanks!

coveralls commented 11 years ago

Coverage Status

Coverage remained the same when pulling 252b6f7a3db06d73d1309ae0cb4100560958fe20 on hellsgate1001:master into 47a3e24c3fa095648a1dc447d778a13f847daa76 on karanlyons:master.

hellsgate1001 commented 11 years ago

I've made that change. Would you still like a test to be added? I have to be honest and say that I'm not sure how I'd go about adding one so if you have any hints on how I do that I'd be grateful.

karanlyons commented 11 years ago

Test-wise, if you can perhaps just paste some code into this issue that shows what you were doing that broke STC, I can write a proper test for it. I'm still not a huge fan of the self.pk check for previously stated reasons, so if I can get a proper test to see if there's a better way to handle this case, it'd be really awesome. Thanks!

hellsgate1001 commented 11 years ago

It seems to be models which have a required foreign key which are tripping the error. It happens in admin with no custom forms. In the following example the error would be seen when attempting to create a new Tag:

class TagCategory(SaveTheChange, models.Model):
    name = models.CharField(max_length=100)

    def __unicode__(self):
        return self.name

    def get_id_and_name(self):
        return [self.id, self.name]

    class Meta:
        verbose_name_plural = 'Tag Categories'
        ordering = ['name']

class Tag(SaveTheChange, models.Model):
    name = models.CharField(max_length=100)
    category = models.ForeignKey(TagCategory, related_name='tag_category_name')

    def __unicode__(self):
        return self.name

    def get_id_and_name(self):
        return [self.id, self.name]

    class Meta:
        ordering = ['category__name','name']

I've tested this with the foreign key being optional and it works correctly in that case.

karanlyons commented 11 years ago

Awesome, thanks for the example Brian! I'll take a closer look at this over the weekend, and roll some fix into the next release.

bkonkle commented 11 years ago

Here's a test that triggers the bug: https://github.com/lincolnloop/django-save-the-change/commit/3add04c135aec4cfc31a6be26b5f7b48d4b6d0e7

I'm playing around with it to see if I can find a more ideal fix.

karanlyons commented 11 years ago

Superceded by #4, thanks again @hellsgate1001 & @bkonkle.