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

'real' multisite support and other minor changes #14

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

Conversation

dborisov
Copy link
Contributor

Hi

In addition to default Django's site detection by SITE_ID from settings.py I want to add support of dynamic site detection mechanism. It is very usefull when you does not have separate settings.py per-site but want to serve multiple sites on single Django instance. In my case it is Mezzanine CMS which has native multisite without per-site settings support and I want to run django-sitemetrics same way.
This solution is not Mezzanine specific, it is pretty universal and disabled by default.

Changes:

  • Separate settings file to see all config options in one file
  • PEP8 fixes
  • SitemetricsNode class and sitemetrics tag rewrites to have ability to pass the request context and detect current site via HTTP_HOST var

@idlesign
Copy link
Owner

idlesign commented Feb 1, 2016

Thank you. I'll try to review this one in a week.

@idlesign
Copy link
Owner

idlesign commented Feb 9, 2016

Thank you.
There are some comments, need you opinion on them.
We'll also need to extend our test suite if it's decided to merge into master.

@dborisov
Copy link
Contributor Author

I've updated the docs. Will update tests soon.

@dborisov
Copy link
Contributor Author

Are you sure that tests working correctly?
I've added some incorrect symbols to first arg of assertIn and all tests still pass.
My experience with django testing framework is poor. May be my understanding is not correct.
Please look at the tests and correct me if I'm not right

@idlesign
Copy link
Owner

Cannot see anything wrong in Travis logs.
What exactly are those assertIn you've mentioned? How do you run the tests?

@dborisov
Copy link
Contributor Author

cd sitemetrics/ && ./runtests.py

Creating test database for alias 'default'...

.....

Ran 5 tests in 0.016s

OK
Destroying test database for alias 'default'...

@dborisov
Copy link
Contributor Author

grep assertIn sitemetrics/tests.py

self.assertIn('138500', render_string(tpl))
self.assertIn('23;dfw2sdfsdf', '%s' % site_two_code)
self.assertIn('22rjwj3n32n32', render_string(tpl)) # Test active
self.assertIn('asdr4i96y6', render_string(tpl)) # Test cached hit
self.assertIn('2j4irj4ijri', render_string(tpl))

@idlesign
Copy link
Owner

I've added some incorrect symbols to first arg of assertIn and all tests still pass.

Can't reproduce on master, fails as expected.

@dborisov
Copy link
Contributor Author

I've tried on clean venv and it is failed as expected.
Sorry for that headache. I was tired yesterday evening.
Will do some tests now.

@dborisov
Copy link
Contributor Author

I'm sorry but I've failed to make test.
I didn't find a way to test SITE_ID=1 with domain name from SITE_ID=2
Could you please help to do it?

@idlesign
Copy link
Owner

I didn't find a way to test SITE_ID=1 with domain name from SITE_ID=2

Can't quite understand a use case you're to implement. Please reveal the code that didn't budge.

@dborisov
Copy link
Contributor Author

I want to test SITE_BY_REQUEST conf option.
Django must use request object to detect current domain if this option is enabled.
I tryed to use django's test client but got exception because sitemetrics doesn't have urls.py
In short, we need to ensure sitemetrics returns keycode from corresponding site (e.g. some.com) but not site from settings.py (SITE_ID) if SITE_BY_REQUEST is enabled.

@idlesign
Copy link
Owner

Basically you can point ROOT_URLCONF to any module with urlpatterns attribute defined, let it be tests.py. Take a look at https://github.com/idlesign/django-sitemessage/blob/master/sitemessage/runtests.py#L18
Hope this'll help.

@dborisov
Copy link
Contributor Author

Thnx. Will try soon.

@dborisov
Copy link
Contributor Author

I've added empty urlpatterns because sitemetrics doesn't have any views, but django test client throws 404 because there is no any view under "/" pattern.
AFAICS there is no simple solution to test this feature because it is non standard django's multisite
Any ideas?

@idlesign
Copy link
Owner

Certainly. There seems nothing that could prevent you from making any number of test views and wiring those to urlpatterns. The other way is not to use test client at all and test our template node as is with various mock contexts.

@dborisov
Copy link
Contributor Author

Ok, will try the second way

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants