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

Don't depend on SITE_ID of project settings #266

Open
woodz- opened this issue Jan 26, 2019 · 10 comments
Open

Don't depend on SITE_ID of project settings #266

woodz- opened this issue Jan 26, 2019 · 10 comments

Comments

@woodz-
Copy link

woodz- commented Jan 26, 2019

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.

@dokterbob
Copy link
Collaborator

Makes total sense to me. Happy to merge a patch regarding this.

@woodz-
Copy link
Author

woodz- commented Feb 22, 2019

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

@dokterbob
Copy link
Collaborator

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.

@zelenij
Copy link

zelenij commented Jan 7, 2021

@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?

@woodz-
Copy link
Author

woodz- commented Jan 8, 2021

@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

@zelenij
Copy link

zelenij commented Jan 10, 2021

@woodz- did you push these changes to github? Can't see anything obvious in your fork...

@woodz-
Copy link
Author

woodz- commented Feb 3, 2021

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.

@zelenij
Copy link

zelenij commented Feb 3, 2021

Don't worry, I did my own change already, there is a PR open for it

@woodz-
Copy link
Author

woodz- commented Feb 3, 2021

Is it #357? Does it do?

@zelenij
Copy link

zelenij commented Feb 3, 2021

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.

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

No branches or pull requests

3 participants