jrief / django-admin-sortable2

Generic drag-and-drop ordering for objects in the Django admin interface
https://django-admin-sortable2.readthedocs.io/en/latest/
Other
773 stars 180 forks source link

Django 5.0 support #383

Open khasbilegt opened 10 months ago

khasbilegt commented 10 months ago

@jrief could you please take a look at this and approve the workflows to run?

khasbilegt commented 10 months ago

@jrief the workflows should pass now.

khasbilegt commented 10 months ago

@jrief I don't think I've added a change that might cause a dependency build to fail, haven't I? I think I can updating the setuptools and wheel via python pip, however why not use the pip provided by the setup-python action in the first place? What do you think?

khasbilegt commented 10 months ago

Hello @jrief, I found out the reason why the workflow was failing and there were two issues.

  1. Previously, the setup python action didn't specify the python version explicitly. Hence, the action used the python version that was initially set in the PATH. Basically the python version is determined by the whichever matrix instance that is successfully executed the setup-python first. If the python 3.6 and django 4.0 ran first then rest of the instances will use python 3.6. So I added python-version option in the action. Also I updated the action version to v5.

  2. Python 3.12 support was added in Playwright version 1.39.0. So I had to bump Playwright, Greentlet and asgiref versions.

Even though I was able to fix to those, I couldn't figure out the e2e inline test (test_e2e_inline.py::test_drag_down) that's failing. Could you please help me figure out why is this test failing?

jrief commented 10 months ago

Even though I was able to fix to those, I couldn't figure out the e2e inline test (test_e2e_inline.py::test_drag_down) that's failing. Could you please help me figure out why is this test failing?

Okay, I'll have a look. Thanks for fixing the workflow.

herrbenesch commented 10 months ago

@khasbilegt just to validate: I have failing tests with django 5 where a template file is missing:

django.template.exceptions.TemplateDoesNotExist: adminsortable2/edit_inline/tabular-django-5.0.html

would this be resolved with admin-sortable being compatible with django 5?

khasbilegt commented 10 months ago

Yes it will

ChrisCrossCrash commented 10 months ago

Thank you for working on this. I'm looking forward to the fix! In the meantime, In case anybody else is stuck waiting, here's a workaround:

  1. Create two new templates: adminsortable2/edit_inline/stacked-django-5.0.html and adminsortable2/edit_inline/tabular-django-5.0.html in your templates directory
  2. Copy-paste the contents of the 4.2 versions of those files from your site-packages/adminsortable2/templates/adminsortable2/edit_inline/<stacked or tabular>-django-4.2.html files. This should fix the TemplateDoesNotExist error.

I wonder, would it be possible to cause Django to default to the newest available template if the Django version exceeds the highest template version available?

khasbilegt commented 10 months ago

The only reason this MR isn't being merged is because some E2E tests are failing due to changes in Django 5.0.

jrief commented 10 months ago

@khasbilegt and unfortunately I haven't found the reason yet. I have to dig further into the internals.

aeyoll commented 8 months ago

Hello @jrief, Any possible way to get this fixed please? Thanks a lot!

robertarnorsson commented 8 months ago

@khasbilegt and unfortunately I haven't found the reason yet. I have to dig further into the internals.

Hi, @jrief. Have you found a way to fix the E2E tests?

jrief commented 8 months ago

Unfortunately, not yet.

eyalch commented 8 months ago

@jrief Could the issue be related to upgrading Python dependencies?

mgrdcm commented 6 months ago

Looks like this is handled as of the 2.2 release?

ldeluigi commented 3 months ago

any news?

mgrdcm commented 3 months ago

Looks like 5 is supported as of 2.0, is this PR still needed?

ldeluigi commented 3 months ago

Looks like 5 is supported as of 2.0, is this PR still needed?

Right, atm the only issue is with 5.1, as per #405