Skip to content
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

Regression: SortableTabularInline re-ordering not persisting to db in v0.2.4 #152

Closed
adamJLev opened this issue Sep 19, 2013 · 25 comments
Closed

Comments

@adamJLev
Copy link
Contributor

This works in v0.2.3, and I'm guessing this commit may be the issue:

435afaa

Full compare between versions:
8f98158...22fbc80

Cheers 🍻

@darklow
Copy link
Owner

darklow commented Sep 20, 2013

Can you give some more info, version of Django and class definitions.

Because Demo site is using v0.2.4 and SortableTabularInline works:
http://djangosuit.com/admin/examples/continent/9/

@adamJLev
Copy link
Contributor Author

Weird... I've been doing some more testing and tinkering and SortableTabularInline works on most of my ModelAdmins, but not on this specific one. I don't see anything special bout it though (and all of them work properly with suit v.0.2.3)

https://gist.github.com/adamJLev/6637178

django 1.5.4

@darklow
Copy link
Owner

darklow commented Sep 20, 2013

Check if there is no JS errors or missing files in Developer Console.
Any other JS errors on page may stop further JS execution.

@adamJLev
Copy link
Contributor Author

No errors in the console. Using Chrome latest.
Did some more digging and this is the change that causes this bug: https://github.com/darklow/django-suit/blob/22fbc800dcc069aade735d69d6ef164d31b35693/suit/static/suit/js/sortables.js

Not sure why it's only happening to some of the ModelAdmins though.

@darklow
Copy link
Owner

darklow commented Sep 23, 2013

You referenced whole JS file not the diff. Could you please post link to diff?

@adamJLev
Copy link
Contributor Author

Yeah my bad, this is it 435afaa

@darklow
Copy link
Owner

darklow commented Sep 25, 2013

Can you please try latest develop branch and see if your bug is still there or not.
I just want to make sure that this was not related to bug #146 before i make any other changes.

pip uninstall django-suit
pip install https://github.com/darklow/django-suit/tarball/develop

@darklow
Copy link
Owner

darklow commented Sep 26, 2013

@adamJLev Can you please check it with latest develop? I would really like to release new version with previous bug fixes and only this issue is holding me from doing that. Thanks

@adamJLev
Copy link
Contributor Author

Just tested, issue still happening with latest develop branch.

darklow added a commit that referenced this issue Sep 27, 2013
default=0 new and empty extra inlines were not handled properly -
order value was set which means admin tried to save empty inline
@darklow
Copy link
Owner

darklow commented Sep 27, 2013

Please test with latest develop version. Since i can't reproduce i can't be 100% sure this fixed the problem. Thanks.

@adamJLev
Copy link
Contributor Author

Still no dice :(
I know it's hard to fix a bug you can't replicate, so when I have a bit of free time (very busy with work right now) I will try to put together a failing unit test or something like that.
Thanks for digging

@dreipol
Copy link

dreipol commented Sep 30, 2013

I'm having the exactly the same issue. I now tried the most recent developer-version, with no success. All my sortable-fields have the value 0 filled in.

@darklow
Copy link
Owner

darklow commented Sep 30, 2013

@dreipol can you please try to spot the difference why sometimes it is working and sometimes not (check html differences on inline dynamic rows, etc)?
And have you also tried latest develop version?
I tried fixing this thing, but with no luck, since I can't replicate this bug.

@dreipol
Copy link

dreipol commented Sep 30, 2013

I don't know if it has anything to do with this problem, but if I remove the add_permission from the InlineModel, the rows are shown wrong.
InlineSortable

@darklow
Copy link
Owner

darklow commented Sep 30, 2013

@dreipol nope, than can't be related, however apparently it is a separate bug, but it just disables wrong arrows and doesn't affect values.

@dreipol
Copy link

dreipol commented Sep 30, 2013

Ok. I'm just trying to figure out where the fault happens. Should the value of the sortable be set new, when the reordering happens or does it happen when one click save? And where is the new value assigned?

@darklow
Copy link
Owner

darklow commented Sep 30, 2013

Values for hidden order fields are assigned on form submit:
https://github.com/darklow/django-suit/blob/develop/suit/static/suit/js/sortables.js#L102

The script updates order field (puts order values in hidden fields) in two cases:

  1. Inline row is existing
  2. Any of inline fields (other than order field itself) is changed.

@darklow
Copy link
Owner

darklow commented Sep 30, 2013

@dreipol your inline screenshot and my latest comment gave me an idea, i think it is because there is just a checkbox in inline. I think i forgot to handled checkboxes and radios.

@darklow
Copy link
Owner

darklow commented Sep 30, 2013

Fix is on it's way :)

@dreipol
Copy link

dreipol commented Sep 30, 2013

Great. Actually the Checkbox You see there is the normal delete checkbox.

darklow added a commit that referenced this issue Sep 30, 2013
database when all inlines fields were marked as readonly.
Add better existing inline detection.
@darklow
Copy link
Owner

darklow commented Sep 30, 2013

Please check latest develop branch:

pip uninstall django-suit
pip install https://github.com/darklow/django-suit/tarball/develop

@dreipol
Copy link

dreipol commented Sep 30, 2013

Works like a charm! Thank you very much.

@dreipol
Copy link

dreipol commented Sep 30, 2013

Do you have any idea when the next version is released?

@darklow
Copy link
Owner

darklow commented Sep 30, 2013

In few minutes :)

@darklow
Copy link
Owner

darklow commented Sep 30, 2013

v0.2.5 is released and uploaded to pypi

@darklow darklow closed this as completed Sep 30, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants