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

SaveTheChange doesn't work on fields added dynamically with add_to_class #26

Open oTree-org opened 7 years ago

oTree-org commented 7 years ago

I don't know if this is really considered a bug, because add_to_class is not even part of Django's public API, it's just internal, so feel free to close if you think this is out of scope. But I thought I would point this out because it used to work in the stable release of save-the-change.

I'm using Django 1.8.8 and the latest master of save-the-change.

models.py:

from django.db import models
from save_the_change.decorators import SaveTheChange

@SaveTheChange
class Knight(models.Model):
    name = models.CharField(max_length=100)
    age = models.PositiveIntegerField()

@SaveTheChange
class KnightA2C(models.Model):
    name = models.CharField(max_length=100)
KnightA2C.add_to_class('age', models.PositiveIntegerField())

tests.py:

from django.test import TestCase
from .models import Knight, KnightA2C

class TestA2C(TestCase):

    def test_a2c(self):
        kn = Knight.objects.create(name='Lancelot', age=20)
        kn.age = 21
        kn.save()

        kn = Knight.objects.get()
        self.assertEqual(kn.age, 21)

        kn_a2c = KnightA2C.objects.create(name='Lancelot', age=20)
        kn_a2c.age = 21
        kn_a2c.save()

        kn_a2c = KnightA2C.objects.get()
        self.assertEqual(kn_a2c.age, 21)

Result:

C:\oTree\tmp_stc> python manage.py test a2c
Creating test database for alias 'default'...
F
======================================================================
FAIL: test_a2c (a2c.tests.TestA2C)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\oTree\tmp_stc\a2c\tests.py", line 19, in test_a2c
    self.assertEqual(kn_a2c.age, 21)
AssertionError: 20 != 21

----------------------------------------------------------------------
Ran 1 test in 0.003s

FAILED (failures=1)
Destroying test database for alias 'default'...
C:\oTree\tmp_stc>
karanlyons commented 7 years ago

Oh man.

This is actually fixable, I can just wrap add_to_class when the class is passed to the decorator.

That’s kinda a weird thing to be doing, though: what’s your use case?

oTree-org commented 7 years ago

I have a framework here where each app has 3 models: Player, Group, Subsession. Usually, 3 models is enough. However, occasionally, users need a variable number of fields on their Player model.

In normal circumstances, one would solve this with a many-to-one relationship via an additional model & a foreign key, but that introduces other complications due to the way my framework is designed (the aim of the framework is to be simple to program, at the expense of some flexibility). So, some users discovered the add_to_class technique and have been using that (adding fields dynamically in a for-loop). I have now discouraged users from relying on add_to_class; however, it would be nice if save-the-change supported this because I don't want to break people's existing programs when they upgrade.

Anyway, thanks for the very useful library! It definitely has enabled some nice functionality in my framework 👍

karanlyons commented 7 years ago

Oh, neat!

Regarding add_to_class, I just need to go through again and work out what the lifecycle for fields in models is. Pretty sure just wrapping the method is good enough, but since STC has exchanged the nightmares of metaclasses for the weirdness/abhorrence of decorators modifying __bases__ I want to make sure I’m covering all the bases for “normal” additions of fields.

Glad my library’s can help you out!

oTree-org commented 7 years ago

Great, thank you so much!