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

Stop monkey patching models in `pydata/apps.py` #1075

Open
chrismedrela opened this Issue Nov 8, 2016 · 8 comments

Comments

Projects
None yet
2 participants
@chrismedrela
Collaborator

chrismedrela commented Nov 8, 2016

I've raised this issue in #1073:

(...) However, I think we need to address a much more fundamental issue - the way we monkey patch everything in pydata/apps.py scares me, especially that we monkey patch models. It's not going to scale at all.

I think what we need to do is to reuse the same Sponsorship model for both standard and pydata deployment. This can be done by converting it from an enum to a ForeignKey to a new model SponsorshipOption, so that the choices can be dynamically changed by altering the records of SponsorshipOption table.

If for some reason we need different models for standard and pydata deployment, a model in pydata app would inherit from a model in workshops app.

An enhancement for this issue is to get rid of monkey patching of templates, forms and views, but it's not that much important right now.

@aditnryn

This comment has been minimized.

Member

aditnryn commented Nov 9, 2016

Django Oscar and many other Django frameworks employ a similar tactic to deal with customizations. See django-oscar docs.

We can follow it and make AMY modular in the long run.

@chrismedrela

This comment has been minimized.

Collaborator

chrismedrela commented Nov 10, 2016

I've realized that converting Sponsorship.amount to a ForeignKey won't work for standard deployment.

The way Django Oscar customizes models is (for me) quite tricky and I don't really understand it.

I propose to create settings.ENABLE_PYDATA flag and use if ENABLE_PYDATA: conditionals in similar way to compilation conditionals in C. Example of model:

class Sponsorship(models.Model):
    ...

    if settings.ENABLE_PYDATA:
        LEVELS = (
            (0, 'Founding'),
            (15000, 'Diamond'),
            (8000, 'Platinum'),
            (5000, 'Gold'),
            (3000, 'Silver'),
            (1500, 'Supporting'),
            (1, 'Community'),
        )
        amount = models.DecimalField(
            max_digits=8, decimal_places=2,
            blank=True, null=True,
            validators=[MinValueValidator(0)],
            verbose_name='Sponsorship amount',
            help_text='e.g. 1992.33',
            choices=LEVELS,
        )

    else:
        amount = models.DecimalField(
            max_digits=8, decimal_places=2,
            blank=True, null=True,
            validators=[MinValueValidator(0)],
            verbose_name='Sponsorship amount',
            help_text='e.g. 1992.33'
        )

    class Meta:
        unique_together = ('organization', 'event', 'amount')

    if settings.ENABLE_PYDATA:
        def __str__(self):
            return '{}: {}'.format(self.organization, self.amount)

    else:
        def __str__(self):
            return '{}: {}'.format(self.organization, self.get_amount_display())

Example of forms:

class SponsorshipForm(forms.ModelForm):
    ...

    if settings.ENABLE_PYDATA:
        class Media:
            js = ('import_sponsor.js', )

        amount = forms.ChoiceField(choices=Sponsorship.LEVELS)

This is not the most beautiful solution, but IMO better than the current one. The biggest drawback is that you need to be very careful when generating migrations to make sure they work for both standard and pydata deployment.

@narayanaditya95 @pbanaszkiewicz any opinions?

@aditnryn

This comment has been minimized.

Member

aditnryn commented Nov 10, 2016

-1. @pbanaszkiewicz and I wanted a complete separation of code.

My suggestion would be to inherit the Sponsorship model in the pydata/models.py from 'workshops/models.py`.

from workshops.models import Sponsorship as SponsorshipBase

class Sponsorship(SponsorshipBase):
        LEVELS = (
            (0, 'Founding'),
            (15000, 'Diamond'),
            (8000, 'Platinum'),
            (5000, 'Gold'),
            (3000, 'Silver'),
            (1500, 'Supporting'),
            (1, 'Community'),
        )

        amount = models.DecimalField(
            max_digits=8, decimal_places=2,
            blank=True, null=True,
            validators=[MinValueValidator(0)],
            verbose_name='Sponsorship amount',
            help_text='e.g. 1992.33',
            choices=LEVELS,
        )

We would then define its corresponding ModelForm and assign it to SPONSORSHIP_FORM variable in settings.py.

from pydata.models import Sponsorship


class SponsorshipForm(forms.ModelForm):

        class Meta:
                model = Sponsorship

        class Media:
                js = ('import_sponsor.js')

In workshops/views.py, we would utilize the SPONSORSHIP_FORM variable to fetch the right ModelForm for Create and Update views.

from django.conf import settings

SponsorshipForm = settings.SPONSORSHIP_FORM
@chrismedrela

This comment has been minimized.

Collaborator

chrismedrela commented Nov 10, 2016

My suggestion would be to inherit the Sponsorship model in the pydata/models.py from 'workshops/models.py`.

And SponsorshipBase defines amount without choices, is that right? I don't think it'll work. You cannot override parent's field, StackOverflow says.

-1. @pbanaszkiewicz and I wanted a complete separation of code.

I'm a bit lost now. @pbanaszkiewicz @narayanaditya95 could you write a short summary what is your preferred solution to this issue and standard/pydata deployment? In my case it's:

  • introducing ENABLE_PYDATA envvar and using if ENABLE_PYDATA conditionals everywhere in settings.py, models, templates etc.
  • only one branch/fork (develop, no pydata-deploy)
  • only one settings.py file
  • varying between deployments with envvars (like ENABLE_PYDATA or DB_FILENAME).
@aditnryn

This comment has been minimized.

Member

aditnryn commented Nov 10, 2016

And SponsorshipBase defines amount without choices, is that right? I don't think it'll work. You cannot override parent's field, StackOverflow says.

We would then have to divide Sponsorship into an abstract BaseSponsorship model without the amount field. Both workshops app and pydata app would then inherit it and add their respective amount field.

@chrismedrela

This comment has been minimized.

Collaborator

chrismedrela commented Nov 10, 2016

And SponsorshipBase defines amount without choices, is that right? I don't think it'll work. You cannot override parent's field, StackOverflow says.

We would then have to divide Sponsorship into an abstract BaseSponsorship model without the amount field. Both workshops app and pydata app would then inherit it and add their respective amount field.

OK, so in the case of pydata deployment, we have two non-abstract models workshops.Sponsorship and pydata.Sponsorship that results in two tables. Only the latter is used. In the case of standard deployment, we'll have only one table, because pydata app is disabled. Do I understand correctly?

I think this solution is much better than monkey-patching and I'm 👍 (even if it results in an unnecessary empty table).

Your solution to forms monkey-patching (that is, settings.SPONSORSHIP_FORM) is also much better than monkey-patching. However, I'm worrying if it scales up. What happens if we need more forms that differ between standard and pydata deployment? Do we add i.e. settings.MEMBERSHIP_FORM and so on?

@aditnryn

This comment has been minimized.

Member

aditnryn commented Nov 10, 2016

OK, so in the case of pydata deployment, we have two non-abstract models workshops.Sponsorship and pydata.Sponsorship that results in two tables. Only the latter is used. In the case of standard deployment, we'll have only one table, because pydata app is disabled. Do I understand correctly?

Yes. We can avoid having a redundant table by creating a swc-dc app that would house models, forms and templates exclusive to AMY deployed for SWC/DC. However, this is a far-fetched thought and is not necessary as of now.

Your solution to forms monkey-patching (that is, settings.SPONSORSHIP_FORM) is also much better than monkey-patching. However, I'm worrying if it scales up. What happens if we need more forms that differ between standard and pydata deployment? Do we add i.e. settings.MEMBERSHIP_FORM and so on?

Yes.

@chrismedrela

This comment has been minimized.

Collaborator

chrismedrela commented Nov 10, 2016

Your solution to forms monkey-patching (that is, settings.SPONSORSHIP_FORM) is also much better than monkey-patching. However, I'm worrying if it scales up. What happens if we need more forms that differ between standard and pydata deployment? Do we add i.e. settings.MEMBERSHIP_FORM and so on?

Yes.

OK, this is something I can live with.

It seems that we reached consensus. :-) @pbanaszkiewicz any opinions on the proposed design?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment