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

Updates for compatibility with django 4.0+ #12

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

Conversation

rlskoeser
Copy link

@rlskoeser rlskoeser commented Aug 13, 2024

as discussed on #9

requirements.txt Outdated
@@ -1,2 +1,2 @@
django
django==4.1
Copy link
Member

Choose a reason for hiding this comment

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

This would prevent the install of a newer version. I typically would set the minimum version and then go up – for example django>=4.1 or, if we wanted to restrict it to the ones we had in mind while testing, perhaps django>=4.1,<6.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, I will switch to that.

I think I set it as a shortcut for testing with tox. I was trying to figure out how to do a matrix build with tox to test multiple versions of django but I'm not that familiar with tox and I use github actions for matrix builds

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I'd like to pivot to GitHub actions for this as well since it'd be faster but I no longer work on the project where I primarily used this code so it's somewhat far down the to-do list.

Copy link
Author

Choose a reason for hiding this comment

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

if I have energy to make a quick stab at github actions based on my other projects would it be ok to include in this pr?

tox.ini Outdated Show resolved Hide resolved
@rlskoeser
Copy link
Author

rlskoeser commented Aug 15, 2024

@acdha I've simplified the conditional imports, made the changes you flagged, marked the unit tests that are failing for me to be skipped (I can't tell if they are expected to pass), and added a basic github actions unit test with a matrix build. Do you want to look it over and approve the workflow if it looks ok?

update: tests are passing on github actions on my branch https://github.com/rlskoeser/django-tabular-export/actions/runs/10410420372

LMK if you want me to adjust the build matrix and drop the older versions of django before you approve it to run in your org

@@ -159,6 +159,7 @@ def test_export_to_csv_response(self):
'1,2\r\n', '3,4\r\n', 'abc,def\r\n',
'2015-08-28T00:00:00,2015-08-28\r\n'])

@unittest.skip("failing - does not return an HttpResponse")
Copy link
Member

Choose a reason for hiding this comment

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

I was not expecting that…

Copy link
Author

Choose a reason for hiding this comment

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

yeah, I wasn't sure how to troubleshoot this - I skipped it for now so I could make sure the build was working

could it be something I don't have installed? or something that changed with debug behavior?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I'm waiting for the punchline to be that it's returning a redirect or something like that.

Copy link
Author

Choose a reason for hiding this comment

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

there is a redirect but when I added the arg to follow it rendered an admin list template which I didn't think was the expected behavior (based on my memory from the other day)

Copy link
Member

Choose a reason for hiding this comment

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

Ah, running it without the skip:

TypeError: never_cache didn't receive an HttpRequest. If you are decorating a classmethod, be sure to use @method_decorator.```

Copy link
Member

Choose a reason for hiding this comment

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

Okay, so it looks like this was a backwards-incompatible change in Django but there's an easy fix to switch to add_never_cache_headers which is simpler anyway.

@acdha
Copy link
Member

acdha commented Aug 15, 2024

Those fast matrix builds are so nice, thank you for adding them!


@unittest.skip("error (where is custom export configured?)")
Copy link
Member

Choose a reason for hiding this comment

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

The underlying problem here is probably that the form parameters which the Django admin interface expects has probably changed: it's generating a 302 redirect to the requested URL so the test code must be missing something basic.

(Pdb) p response
<HttpResponseRedirect status_code=302, "text/html; charset=utf-8", url="/admin/tests/testmodel/">
(Pdb) p changelist_url
'/admin/tests/testmodel/'

Copy link

Choose a reason for hiding this comment

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

Hi both! @rlskoeser asked me to take a look at this.

The error message Django sends back here is "No action selected." You can check this with:

from django.contrib.messages import get_messages
messages = [m.message for m in list(get_messages(response.wsgi_request))]

I notice that there's no action actually named custom_export_to_csv_action so maybe this is the intended behavior? While that is the same error message you get on form validation errors, I don't see any difference in the form params—the underlying Django code hasn't actually changed (Django 3.2.x, Django 5), and I can't get this test to succeed under Django 3.2 either.

It seems correct to redirect with this error message unless an admin action with that exact name was registered, unless I am missing something.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for looking into this, @blms.

@acdha let us know how you'd like to resolve this.

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.

3 participants