fusionbox / django-widgy

A CMS framework for Django built on a heterogenous tree editor.
http://wid.gy
Other
332 stars 52 forks source link

Remove dependency for ReviewedVersionTracker in migrations #315

Closed jxcl closed 9 years ago

jxcl commented 9 years ago

The base widgy migrations had references to ReviewedVersionTracker, which is not part of the base widgy install. This commit changes the dependency to VersionTracker instead, which is part of the base widgy install.

rockymeza commented 9 years ago

I love new migrations.

LGTM

gavinwahl commented 9 years ago
  • ('root_node', widgy.db.fields.VersionedWidgyField(to='widgy.VersionTracker')),

What will that do when review queue is enabled?

acatton commented 9 years ago

@gavinwahl a ReviewedVersionTracker is a subclass of a VersionTracker. This works. But we don't have any data integrity if the review queue is installed, this means that there's no way to guarantee that root_node is a foreignkey to a ReviewedVersionTracker.

gavinwahl commented 9 years ago

It's a subclass but it has its own table. Doesn't the foreign key need to point to it?

acatton commented 9 years ago

@gavinwahl from a Django ORM point of view, no; from an integrity point of view, yes.

rockymeza commented 9 years ago

I think still no. It's guaranteed to point to a version tracker, but a reviewedversiontracker is not guaranteed to exist for that version tracker

Rocky Meza, Programmer fusionbox o: 303.952.7490

www.fusionbox.com http://www.fusionbox.com/?utm_source=fb%2Bemail&utm_medium=email&utm_campaign=email%2Btracking

This information is confidential and intended solely for the use of the individual or entity to whom it is addressed. If you have received this email in error please notify the sender.

On Wed, May 6, 2015 at 1:56 PM, Antoine Catton notifications@github.com wrote:

@gavin https://github.com/gavin from a Django ORM point of view, no; from an integrity point of view, yes.

— Reply to this email directly or view it on GitHub https://github.com/fusionbox/django-widgy/pull/315#issuecomment-99586678 .

acatton commented 9 years ago

@rockymeza @gavinwahl This is how south does it

It's guaranteed to point to a version tracker, but a reviewedversiontracker is not guaranteed to exist for that version tracker

@rockymeza That's what I said. It doesn't change anything for the ORM (since it doesn't check if there is a foreign key.) But, yes, it does not guarantee integrity.

gavinwahl commented 9 years ago

The foreign key is pointing to the wrong table. How is that OK?

acatton commented 9 years ago

@gavinwahl you can think of it as being a generic foreign key. This is what it looks like:

 +-----------+                             
 | WidgyPage |                             
 +-----------+                             
 | root_node +--+                          
 +-----------+  |                          
                |                          
                |                          
                |   +----------------+     
                |   | VersionTracker |     
                |   +----------------+     
                +---> id             <----+
                    +----------------+    |
                                          |
                                          |
+------------------------+                |
| ReviewedVersionTracker |                |
+------------------------+                |
| id                     +----------------+
+------------------------+                 
gavinwahl commented 9 years ago

Which is wrong, right? It should be

   +-----------+                             
   | WidgyPage |                             
   +-----------+                             
 +-> root_node +                             
 | +-----------+                             
 |                                           
 |                                           
 |                    +----------------+     
 |                    | VersionTracker |     
 |                    +----------------+     
 |                    | id             <----+
 |                    +----------------+    |
 |                                          |
 |                                          |
 |+------------------------+                |
 || ReviewedVersionTracker |                |
 |+------------------------+                |
 +> id                     +----------------+
  +------------------------+    

I understand that it happens to work in this case, but is the really no way to do it right?

rockymeza commented 9 years ago

I agree with @gavinwahl. If WidgyPage doesn't point to ReviewedVersionTracker, there is no guarantee that a ReviewedVersionTracker exists.

Rocky Meza, Programmer fusionbox o: 303.952.7490

www.fusionbox.com http://www.fusionbox.com/?utm_source=fb%2Bemail&utm_medium=email&utm_campaign=email%2Btracking

This information is confidential and intended solely for the use of the individual or entity to whom it is addressed. If you have received this email in error please notify the sender.

On Wed, May 6, 2015 at 2:21 PM, Gavin Wahl notifications@github.com wrote:

Which is wrong, right? It should be

+-----------+ WidgyPage +-----------+ +-> root_node + +-----------+
+----------------+
VersionTracker
+----------------+
id <----+
+----------------+
+------------------------+
ReviewedVersionTracker
+------------------------+

+> id +----------------+ +------------------------+

I understand that it happens to work in this case, but is the really no way to do it right?

— Reply to this email directly or view it on GitHub https://github.com/fusionbox/django-widgy/pull/315#issuecomment-99594403 .