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

Django 5.0 support #383

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Conversation

khasbilegt
Copy link

No description provided.

@khasbilegt
Copy link
Author

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

jrief
jrief previously approved these changes Jan 4, 2024
Copy link
Owner

@jrief jrief left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You then have to change the CLASSIFIERS in setup.py as well.
And please add this modification to the ChangeLog.

@khasbilegt
Copy link
Author

@jrief the workflows should pass now.

jrief
jrief previously approved these changes Jan 4, 2024
@khasbilegt
Copy link
Author

@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
Copy link
Author

khasbilegt commented Jan 8, 2024

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
Copy link
Owner

jrief commented Jan 8, 2024

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
Copy link

@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
Copy link
Author

Yes it will

@ChrisCrossCrash
Copy link

ChrisCrossCrash commented Jan 23, 2024

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
Copy link
Author

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

@jrief
Copy link
Owner

jrief commented Jan 24, 2024

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

@aeyoll
Copy link

aeyoll commented Mar 6, 2024

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

@robertarnorsson
Copy link

@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
Copy link
Owner

jrief commented Mar 9, 2024

Unfortunately, not yet.

@eyalch
Copy link

eyalch commented Mar 29, 2024

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

@mgrdcm
Copy link
Contributor

mgrdcm commented May 16, 2024

Looks like this is handled as of the 2.2 release?

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

Successfully merging this pull request may close these issues.

None yet

8 participants