Pull requests

#6 Declined
Repository
oblalex oblalex
Branch
default
Repository
bkroeze bkroeze
Branch
default

related_name for site provided

Author
  1. Alexander Oblovatniy
Reviewers
Description

Added related_name property for Setting and LongSetting to fix next issue:

CommandError: One or more models did not validate:
livesettings.setting: Accessor for field 'site' clashes with related field 'Site.setting_set'. Add a related_name argument to the definition for 'site'.

Comments (5)

  1. Hynek Cernoch

    I did not understand your problem first. I will try to guess it and explain it:

    You created a project with another application that also use a model Setting with a ForeignKey to Site and also has not defined a related_name for a ForeignKey field. Is not it? (That can cause the same error.)

    I will fix it only by this replace:

        -    site = models.ForeignKey(Site, verbose_name=_('Site'))
        +    site = models.ForeignKey(Site, verbose_name=_('Site'), related_name="livesettings_set")
    
    • Lovercase are better than camelCase for field names.
    • It is not probable that the name "longsetting" causes a conflict for anybody.
    • In my fork is prepared a new version of livesettings with an applied patch from #24. That compoetely removes the table livesettings_longsetting. (with a migration for South for doin it automatically etc.) Livesettings will have only one table and a model name is not important more.
  2. Alexander Oblovatniy author
    1. Seems you are right about the reason of the problem. Test application works fine. I suppose, the problem was caused by Grappelli, because my project has no own models yet and [other used apps].(https://github.com/IL2HorusTeam/commander/blob/8670d48cfcfcbca21fba00ad67bc18452942468a/requirements.txt) seem to be OK. So, it's better to use project-specific prefixes for table names.
    2. Yes, this better to use uppercase chars only. My defect for using camel case.
    3. There are so many forks over the network. Seems like lots of people are fixing same bugs separately :)
    4. I can see you have left only 1 model which uses only TextField. Personally I think that storing everything in TextField is not good approach. This is an overkill for boolean and numeric data.
  3. Hynek Cernoch

    Ad 4) I can think about long settings again. The necessary condition for retaining longsetting is that the apropriate db table must be known by declared value type before any access to db. Otherwise the current implementation is less effective than future saving everything into TextField. Expect that the application is general enough for everybody and an average user needs less than half of declared settings. Other settings remain on default values. For them is nothing saved to the database. The current implementation is first to query livesettings_setting, then to query livesettings_longsetting and if it is also not found then the default value is used.

    Postgres 9 declares that there is no performance difference between fixed char, var char and text field. http://www.postgresql.org/docs/9.0/static/datatype-character.html

  4. Alexander Oblovatniy author

    Yes, for Postgres CharField and TextField are the same and using only TextField looks pretty good. But it can be another DB system. Even if every setting will be changed by user, caching still will be helping to decrease payload. So, p4 from my side was not about performance, just a logical issue :)

  5. Hynek Cernoch

    Removing longsetting allowed to fix many subtle issues that earlier was not possible. E.g. exception handling. It is possible that at loading different models.py, the tables livesetting_* are present, but the command "test" is running and later will be the connection to the normal database closed and replaced to a connection to an new created empty database. Or the initial "syncdb" is running on postgres db with enabled transactions. All access to livesettings must be skipped otherwise all following commands will fail or some structure updates will be hurt by rollback. Everything would be twice more complicated and you must buy twice bigger monitor -:)