-
-
Notifications
You must be signed in to change notification settings - Fork 205
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
Don't depend on SITE_ID of project settings #266
Comments
Makes total sense to me. Happy to merge a patch regarding this. |
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? |
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. |
@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. |
Don't worry, I did my own change already, there is a PR open for it |
Is it #357? Does it do? |
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. |
might be in range of #238
When submitting via
$ ./manage.py submit_newsletter
, I would suggest not to fetch site id via parameter-lessSite.objects.get_current()
in module model.py, functionsend_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.The text was updated successfully, but these errors were encountered: