reordering models has caused major problems in the database #42

Closed
kkellydesign opened this Issue Mar 12, 2015 · 14 comments

Projects

None yet

5 participants

@kkellydesign

Hello,
I have a strange problem. I was reordering the navigation on a client's site and the main list suddenly completely rearranged itself. Clearing the cache and refreshing hrefs isn't helping, and I tried to do a cache.clear from the shell, and nothing seems to be working. You can see the problem here:

screen shot 2015-03-12 at 4 29 11 pm

When you click on "skin type", you can see that the parent is actually "tan safely", not "main-nav"

screen shot 2015-03-12 at 4 38 54 pm

This is the correct ordering, but that's not what is showing up in the Menu Items list or the navigation of the site. This is the second time this has happened to me on different sites in the past two weeks, and it seems like a pretty major issue, so I'm wondering if I'm doing something wrong in my site configuration. Any ideas?

Thanks,

Kasey

@jMyles
Contributor
jMyles commented Aug 28, 2015

This ticket seems to describe two issues:

  • Issue A: MenuItems are listed in the admin in a way that does not correspond to their Parent (ie, 'skin type' is listed as if its parent were 'main-nav', when it is in fact 'tan-safety')
  • Issue B: MenuItems are listed in the admin and displayed in the site navigation in a way that does not order them by the "order" attribute.

In an example (provided to me by @kkellydesign), I am able to easily reproduce Issue B, but not Issue A.

A Menu, populated with MenuItems, appears this way in the admin:

selection_002

Notice that "test" appears above "Cristal" even though their order attributes are opposite. The same is true of "Photo_Gallery" which appears above "Regalia" and "Majestica" despite having a higher order.

However, the same menu appears in the site nav in a completely different, but also incorrect order:

selection_003

As you can see, the order here is also incorrect, but with no apparent pattern compared to the admin (for the record, it's "1, 2, 3, 5, 8, 4, 6, 7, 9").

This may be explained in part by the fact that django-treenav has a Meta class for the MenuItem model which uses "('lft', 'tree_id')" as the ordering directive, but ('order',) as the "order_insertion_by" directive. I've 'never seen "order_insertion_by" before, but a google search yields, among other less interesting leads, this question on StackOverflow:

http://stackoverflow.com/questions/27874094/bug-in-django-mpttmeta-order-insertion-by-causes-nodes-to-report-incorrect-desce

However, the app in question is using a version of django-mptt with the fix (noted in an SO answer above) already applied.

As expected, the value of self._meta.ordering for a MenuItem instance is ('lft', 'tree_id').

@jMyles
Contributor
jMyles commented Aug 28, 2015

It is not immediately obvious to me where the 'lft' attribute is defined.

@jMyles
Contributor
jMyles commented Aug 28, 2015

I'm not sure if this insight is useful or not, but if I simply add "(lft: {{item.node.lft}})" to the template in order to get it to show the value of 'lft' for each MenuItem, I get this:

selection_004

...but it's still not clear to me where that value is set or, for that matter, why.

@mlavin
Member
mlavin commented Aug 28, 2015

lft, rght, tree_id, and level are the fields added by and managed MPTT for the tree structure.

@jMyles
Contributor
jMyles commented Aug 28, 2015

Hey Mark. Good to see you man. :-)

Yeah, I realize that now, and, in the comment above, I indicated the lines where the fields are added. I don't, however, see where the value is assigned.

I don't find this segment particularly cognizable or pythonic; it seems to be largely an exercise in adding obscurity. I realize that its purpose is to "treeify" existing models; perhaps it can be turned into model-swapping logic with the new swappable models API.

In any case, my immediate desire is to understand why this ordering issue persists and how to apply at least a near-term fix that will be forward-compatible.

@mlavin
Member
mlavin commented Aug 29, 2015

There have been a number of issues related to ordering #2/#7 which were related to bug in MPTT and reproduced via 684c070 and #17 which was never reproducible. This project in general needs a bit of love and attention but I don't expect any of these ordering problems can be addressed without some failing test cases.

@jMyles jMyles added a commit to jMyles/django-treenav that referenced this issue Aug 29, 2015
@jMyles jMyles Adds a "Rebuild Tree" button for MenuItem admin.
Provides a workable end-around for #42.
b9d95d7
@vkurup vkurup added a commit that referenced this issue Sep 2, 2015
@jMyles @vkurup jMyles + vkurup Adds a "Rebuild Tree" button for MenuItem admin.
Provides a workable end-around for #42.
d4f4975
@vkurup
Member
vkurup commented Sep 2, 2015

A workaround for this has been provided by @jMyles and is in the 0.9.2 Release.

May be worth keeping this issue open, at least to remind us to add documentation to use the new 'rebuild tree' command if ordering issues arise.

@vkurup vkurup added this to the v1.0 milestone Sep 2, 2015
@benred42
Contributor

I did a little looking around and there are a few people who have mentioned that having list editable fields in the admin can lead to tree structure issues:
django-mptt/django-mptt#233
http://stackoverflow.com/questions/20578704/ordering-mptt-tree-using-a-field

Don't know if that's the case here, but the label field for menu items is list editable.

@kkellydesign

hm. I also get an error when I try to delete an item from the list view –
that could be related as well.

On Tue, Sep 22, 2015 at 11:18 AM, Benjamin Phillips <
notifications@github.com> wrote:

I did a little looking around and there are a few people who have
mentioned that having list editable fields in the admin can lead to tree
structure issues:
django-mptt/django-mptt#233
django-mptt/django-mptt#233

http://stackoverflow.com/questions/20578704/ordering-mptt-tree-using-a-field

Don't know if that's the case here, but the label field for menu items is
list editable.


Reply to this email directly or view it on GitHub
#42 (comment)
.

Kasey Kelly
kellycreativetech.com
330.351.9352

@jMyles
Contributor
jMyles commented Sep 22, 2015

Kasey,

What is the error?

On Tue, Sep 22, 2015 at 11:20 AM, Kasey Kelly notifications@github.com
wrote:

hm. I also get an error when I try to delete an item from the list view –
that could be related as well.

On Tue, Sep 22, 2015 at 11:18 AM, Benjamin Phillips <
notifications@github.com> wrote:

I did a little looking around and there are a few people who have
mentioned that having list editable fields in the admin can lead to tree
structure issues:
django-mptt/django-mptt#233
django-mptt/django-mptt#233

http://stackoverflow.com/questions/20578704/ordering-mptt-tree-using-a-field

Don't know if that's the case here, but the label field for menu items is
list editable.


Reply to this email directly or view it on GitHub
<
https://github.com/caktus/django-treenav/issues/42#issuecomment-142321334>
.

Kasey Kelly
kellycreativetech.com
330.351.9352


Reply to this email directly or view it on GitHub
#42 (comment)
.

@kkellydesign

Sorry, I should've probably filed another ticket. I'm pretty sure it's
unrelated to any of your work, Justin. It's easy to recreate though:

  • In the list view (/admin/treenav/menuitem/), hit a checkbox and then
    choose "delete selected menu items", and confirm on the next screen.
  • error message: 'MenuItemManager' object has no attribute
    'delay_mptt_updates'

On Tue, Sep 22, 2015 at 11:26 AM, Justin Holmes notifications@github.com
wrote:

Kasey,

What is the error?

On Tue, Sep 22, 2015 at 11:20 AM, Kasey Kelly notifications@github.com
wrote:

hm. I also get an error when I try to delete an item from the list view –
that could be related as well.

On Tue, Sep 22, 2015 at 11:18 AM, Benjamin Phillips <
notifications@github.com> wrote:

I did a little looking around and there are a few people who have
mentioned that having list editable fields in the admin can lead to
tree
structure issues:
django-mptt/django-mptt#233
django-mptt/django-mptt#233

http://stackoverflow.com/questions/20578704/ordering-mptt-tree-using-a-field

Don't know if that's the case here, but the label field for menu items
is
list editable.


Reply to this email directly or view it on GitHub
<

https://github.com/caktus/django-treenav/issues/42#issuecomment-142321334>

.

Kasey Kelly
kellycreativetech.com
330.351.9352


Reply to this email directly or view it on GitHub
<
https://github.com/caktus/django-treenav/issues/42#issuecomment-142321810>

.


Reply to this email directly or view it on GitHub
#42 (comment)
.

Kasey Kelly
kellycreativetech.com
330.351.9352

@benred42
Contributor

That could be due to the fact that MenuItemManager inherits from Django's models.Manager instead of from the custom MPTT TreeManager. It looks like the MenuItem model uses MenuItemManager as its default manager and the TreeManager for tree-specific functions?

@benred42 benred42 pushed a commit that referenced this issue Sep 25, 2015
bphillips #42 - Added test for reordering menu items in the admin (currently fa…
…ils as expected)
beb1469
@benred42 benred42 was assigned by vkurup Sep 30, 2015
@benred42
Contributor

We've put a fix in place that looks to have dealt with this issue so I'm going to go ahead and close it, but please let us know if you see it reappear at any point.

@benred42 benred42 closed this Sep 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment