django-newsletter
django-newsletter copied to clipboard
jazzband
Don't depend on SITE_ID of project settings
might be in range of #238
When submitting via $ ./manage.py submit_newsletter, I would suggest not to fetch site id via parameter-less Site.objects.get_current() in module model.py, function send_message.
When not supplying a parameter, the site id gets fetched from the settings.py configured SITE_ID, which most likely will not equal to the id the user has related when creating a newsletter instance via admin panel.
It would be better to self take care of querying the proper site id via the anyway maintained model table newsletter_newsletter_site. This is the place, where the id gets in, when users do a newsletter instance to site relation on admin.
I am having a fork ready - still need to add doc. But having trouble with tests. I am looking for a simple way to run all tests with a default settings file (is it the one in test_project folder?). Then I need to introduce a NEWSLETTER_... setting (like the NEWSLETTER_RICHTEXT_WIDGET, but for all the tests, not only a temporary overridable), and run the whole test suite again. How to do this with less effort? For the ones interested: NEWSLETTER_SWAP_SITE_NAME
is it the one in test_project folder?
Yap.
You can also override settings during the tests, this is well documented in the Django docs.
@woodz- any progress with this patch? I think I need it for my project. My single Django instance actually serves more than one domain, so I really don't want to set SITE_ID to anything - if I do, get_current_site function always resolves to this site, rather than to what the Host: header is set to. If you need help with the patch, I'm happy to assist.
@dokterbob is there any other PR for this issue, or anyone else looking into it by any chance?
@zelenij sure, you can take over my fork, checking out the diff and head over to comprehensive tests. The latter was the blocker for me at the time I was working on. I am not aware of the test system and I do not understand it. So I gave up working on it. Check, if the change is sufficient for you and feel free to extend it if you like. Greetings woodz
@woodz- did you push these changes to github? Can't see anything obvious in your fork...
I need to have a look. Currently I've no clue what the thing was when I was working on. There must be uncommitted changes around.
As I can remember I had something lightweight in my head. The main problem is the SITE_ID in settings.py which by no way correlates to the defined sites (different domains) on the admin page.
I anyhow wanted to smartly keep track and switch the SITE_ID, hence the symbol NEWSLETTER_SWAP_SITE_NAME.
Yes, that's the PR. Seems to be working. I have tests, and I already deploy it in a prod site with multiple domains defined.