jazzband / django-newsletter

An email newsletter application for the Django web application framework, including an extended admin interface, web (un)subscription, dynamic e-mail templates, an archive and HTML email support.
GNU Affero General Public License v3.0
846 stars 204 forks source link

Add Django 2.2 and 3.0 Support #300

Closed studybuffalo closed 4 years ago

studybuffalo commented 4 years ago

Major changes to allow Django 3.0 support

Other minor changes

Relevant Issues: #291 #295 #296 #299

studybuffalo commented 4 years ago

Question to be resolved: is it acceptable to continue with sorl-thumbnail pinned to the specific commit, or do we remove it as dependency as per issue #297

coveralls commented 4 years ago

Coverage Status

Coverage remained the same at 88.635% when pulling 4c52bb2e4471202f7111b64ebb938fe5fb240d25 on studybuffalo:support-django-22-and-30 into 29f8208c55d4d4c181f67a7bb3ed449a35a3fcb1 on dokterbob:master.

coveralls commented 4 years ago

Coverage Status

Coverage remained the same at 88.635% when pulling 5bafbdb7fd08582adb43773970e6a2a22d8da390 on studybuffalo:support-django-22-and-30 into 29f8208c55d4d4c181f67a7bb3ed449a35a3fcb1 on dokterbob:master.

dokterbob commented 4 years ago

Great work! I'll try and review it ASAP.

Question to be resolved: is it acceptable to continue with sorl-thumbnail pinned to the specific commit, or do we remove it as dependency as per issue #297 I think pinning the version is good enough for now. There might be a future release of sorl-thumbnail - but removing the dependency is potentially a lot of work and until it's done this will allow users to stay using django-newsletter. :)

Thanks again!

dokterbob commented 4 years ago

Wow! Such a clean contribution! I've already reviewed it, there's only one comment, really:

Please add comments in the Travis file as well as in the requirements.txt about the pinned version of sorl-thumbnail, so that we're reminded to remove it once a new release comes out. I was also wondering why the dependency needs to be explicitly installed with Travis, while it's already in the requirements. That suggests it might create problems for users and/or require additional install instructions. Could you please verify whether it is necessary (to add this install step in Travis)?

Lastly, once the patch has been reviewed, I would appreciate if you could do a pipenv install -r requirements.txt to update the Pipenv file as well as the lockfile.

studybuffalo commented 4 years ago

Thanks for catching that - I learned that requirements.txt editable dependencies don't work the same way for setuptools. I put the Travis file changes in to get things moving forward for development and then forgot about it.

I think I have got a simple enough fix working now that resolves those noted issues and we can easily pull it out once a new sorl-thumbnail version is released. I also made sure to flag it all with TODO comments.

With these changes passing CI, I have also gone and updated the lockfile as requested.