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
Global filters #26
Global filters #26
Conversation
… FIL-448-create-prototype
…ocal variable unused)
…d on content type
setup.py
Outdated
@@ -7,6 +7,7 @@ | |||
'Django>=1.11,<2.0', | |||
'django-cms>=3.5.0', | |||
'django-haystack>=2.7.0', | |||
'django-filer', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't be a requirement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Due to Filer integration code in the codebase and If I remove django-filer
, it's complaining about easy_thumbnails. I tried other combination defining in tests.requirements.txt
and INSTALLED_APP
in HELPER_SETTINGS
but no luck.
File "setup.py", line 32, in <module>
test_suite='tests.settings.run',
File "/var/www/djangocms-4.0/cms_project/djangocms-internalsearch/.tox/py36-dj20-sqlite-cms40/lib/python3.6/site-packages/setuptools/__init__.py", line 140, in setup
return distutils.core.setup(**attrs)
File "/home/riz/miniconda3/lib/python3.6/distutils/core.py", line 148, in setup
dist.run_commands()
File "/home/riz/miniconda3/lib/python3.6/distutils/dist.py", line 955, in run_commands
self.run_command(cmd)
File "/home/riz/miniconda3/lib/python3.6/distutils/dist.py", line 974, in run_command
cmd_obj.run()
File "/var/www/djangocms-4.0/cms_project/djangocms-internalsearch/.tox/py36-dj20-sqlite-cms40/lib/python3.6/site-packages/setuptools/command/test.py", line 228, in run
self.run_tests()
File "/var/www/djangocms-4.0/cms_project/djangocms-internalsearch/.tox/py36-dj20-sqlite-cms40/lib/python3.6/site-packages/setuptools/command/test.py", line 250, in run_tests
exit=False,
File "/home/riz/miniconda3/lib/python3.6/unittest/main.py", line 94, in __init__
self.parseArgs(argv)
File "/home/riz/miniconda3/lib/python3.6/unittest/main.py", line 141, in parseArgs
self.createTests()
File "/home/riz/miniconda3/lib/python3.6/unittest/main.py", line 148, in createTests
self.module)
File "/home/riz/miniconda3/lib/python3.6/unittest/loader.py", line 219, in loadTestsFromNames
suites = [self.loadTestsFromName(name, module) for name in names]
File "/home/riz/miniconda3/lib/python3.6/unittest/loader.py", line 219, in <listcomp>
suites = [self.loadTestsFromName(name, module) for name in names]
File "/home/riz/miniconda3/lib/python3.6/unittest/loader.py", line 204, in loadTestsFromName
test = obj()
File "/var/www/djangocms-4.0/cms_project/djangocms-internalsearch/tests/settings.py", line 20, in run
runner.cms('djangocms_internalsearch', extra_args=[])
File "/var/www/djangocms-4.0/cms_project/djangocms-internalsearch/.tox/py36-dj20-sqlite-cms40/lib/python3.6/site-packages/djangocms_helper/runner.py", line 52, in cms
return runner(argv)
File "/var/www/djangocms-4.0/cms_project/djangocms-internalsearch/.tox/py36-dj20-sqlite-cms40/lib/python3.6/site-packages/djangocms_helper/runner.py", line 86, in runner
return main(argv)
File "/var/www/djangocms-4.0/cms_project/djangocms-internalsearch/.tox/py36-dj20-sqlite-cms40/lib/python3.6/site-packages/djangocms_helper/main.py", line 383, in main
return core(args=args, application=application)
File "/var/www/djangocms-4.0/cms_project/djangocms-internalsearch/.tox/py36-dj20-sqlite-cms40/lib/python3.6/site-packages/djangocms_helper/main.py", line 320, in core
_make_settings(args, application, settings, STATIC_ROOT, MEDIA_ROOT)
File "/var/www/djangocms-4.0/cms_project/djangocms-internalsearch/.tox/py36-dj20-sqlite-cms40/lib/python3.6/site-packages/djangocms_helper/utils.py", line 319, in _make_settings
django.setup()
File "/var/www/djangocms-4.0/cms_project/djangocms-internalsearch/.eggs/Django-1.11.15-py3.6.egg/django/__init__.py", line 27, in setup
apps.populate(settings.INSTALLED_APPS)
File "/var/www/djangocms-4.0/cms_project/djangocms-internalsearch/.eggs/Django-1.11.15-py3.6.egg/django/apps/registry.py", line 85, in populate
app_config = AppConfig.create(entry)
File "/var/www/djangocms-4.0/cms_project/djangocms-internalsearch/.eggs/Django-1.11.15-py3.6.egg/django/apps/config.py", line 94, in create
module = import_module(entry)
File "/var/www/djangocms-4.0/cms_project/djangocms-internalsearch/.tox/py36-dj20-sqlite-cms40/lib/python3.6/importlib/__init__.py", line 126, in import_module
return _bootstrap._gcd_import(name[level:], package, level)
File "<frozen importlib._bootstrap>", line 994, in _gcd_import
File "<frozen importlib._bootstrap>", line 971, in _find_and_load
File "<frozen importlib._bootstrap>", line 953, in _find_and_load_unlocked
ModuleNotFoundError: No module named 'easy_thumbnails'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add filer and easy-thumbnails to test requirements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@czpython, I need your help on this. I tried that but still no luck. while running tox it fail for py36-dj20
ModuleNotFoundError: No module named 'easy_thumbnails'
ERROR: InvocationError for command '/var/www/djangocms-4.0/cms_project/djangocms-internalsearch/.tox/py36-dj20-sqlite-cms40/bin/coverage run setup.py test' (exited with code 1)
summary
flake8: commands succeeded
isort: commands succeeded
SKIPPED: py34-dj111-sqlite-cms40: InterpreterNotFound: python3.4
py35-dj111-sqlite-cms40: commands succeeded
py36-dj111-sqlite-cms40: commands succeeded
SKIPPED: py34-dj20-sqlite-cms40: InterpreterNotFound: python3.4
py35-dj20-sqlite-cms40: commands succeeded
ERROR: py36-dj20-sqlite-cms40: commands failed
Also, noticed that easy_thumbnails is in beta version for django2.0 https://github.com/SmileyChris/easy-thumbnails/blob/master/CHANGES.rst
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe don't test 2.0 for now
djangocms_internalsearch/helpers.py
Outdated
return apps_config | ||
|
||
|
||
def get_model_class(model_meta): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't call this function if there's no model_meta
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok but now remove the if model_meta check :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, these are equivalent:
In [1]: apps.get_model('cms.Page')
Out[1]: cms.models.pagemodel.Page
In [2]: apps.get_model('cms', 'Page')
Out[2]: cms.models.pagemodel.Page
I'd just use that instead of having a separate function
@@ -4,7 +4,7 @@ | |||
|
|||
from cms.app_base import CMSAppConfig, CMSAppExtension | |||
|
|||
from djangocms_internalsearch.internal_search import ( | |||
from djangocms_internalsearch.contrib.cms.internal_search import ( | |||
FilerFileConfig, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
filer config should be in contrib.filer
in the right sidebar. | ||
""" | ||
languages = ( | ||
Title.objects |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.objects on next line
in the right sidebar. | ||
""" | ||
sites = ( | ||
Site.objects |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.objects on next line
in the right sidebar. | ||
""" | ||
authors = ( | ||
Page.objects |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.objects on next line
djangocms_internalsearch/filters.py
Outdated
# to decide how to filter the queryset. | ||
# qs = super(InternalSearchAdmin, self).changelist(request, queryset) | ||
if self.value(): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove newline
from djangocms_internalsearch.filters import ContentTypeFilter | ||
|
||
|
||
class InternalSearchAdminSetting: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify:
The default admin should live in internal_search.py
The cms admin in contrib.cms.internal_search.py
The filer admin in contrib.filer.internal_search.py
setup.py
Outdated
@@ -7,6 +7,7 @@ | |||
'Django>=1.11,<2.0', | |||
'django-cms>=3.5.0', | |||
'django-haystack>=2.7.0', | |||
'django-filer', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add filer and easy-thumbnails to test requirements
djangocms_internalsearch/helpers.py
Outdated
return apps_config | ||
|
||
|
||
def get_model_class(model_meta): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok but now remove the if model_meta check :)
return obj.result.version_status | ||
|
||
|
||
def get_request(language=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move this to djangocms_internalsearch/helpers.py
djangocms_internalsearch/filters.py
Outdated
# Compare the requested value (either '80s' or '90s') | ||
# to decide how to filter the queryset. | ||
# qs = super(InternalSearchAdmin, self).changelist(request, queryset) | ||
if self.value(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if not self.value():
return
model = get_model_class(self.value())
if model:
return queryset.models(model)
djangocms_internalsearch/admin.py
Outdated
list_display = app_config.list_display | ||
for item in list_display: | ||
if callable(getattr(app_config, item)): | ||
setattr(InternalSearchAdmin, item, getattr(app_config, item)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is dangerous. why is it needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This required when configuration have list_display attributes with the custom column name and custom getter method. The above code will apply all config methods to model admin.
list_display = list(InternalSearchAdminSetting.list_display) | ||
|
||
model_meta = request.GET.get('type') | ||
if model_meta: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the indentation makes this difficult to read.
maybe handle validation outside of if and then just do a single if
djangocms_internalsearch/admin.py
Outdated
qs = qs.order_by(*ordering) | ||
if model_meta: | ||
model_class = apps.get_model(model_meta) | ||
if model_class: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a case where this is false? get_model returns a model or raises an exception. Since you check if model_meta is there, you can just skip this check.
djangocms_internalsearch/admin.py
Outdated
|
||
# re applying method attributes to class in case any model has same column name | ||
for item in InternalSearchAdminSetting.list_display: | ||
setattr(InternalSearchAdmin, item, getattr(InternalSearchAdminSetting, item)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed? list_display can contain callables. This could be refactored into get_list_display method where you return a list of callables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those callable methods needs to apply to admin class (InternalSearchAdmin
) hence setting them here
djangocms_internalsearch/admin.py
Outdated
model_class = apps.get_model(model_meta) | ||
if model_class: | ||
app_config = get_internalsearch_model_config(model_class) | ||
qs = InternalSearchQuerySet(self.haystack_connection).models(app_config.model).all() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to use get_internalsearch_model_config since you already have model_class
available
|
||
|
||
def get_modified_date(obj): | ||
return obj.result.creation_date |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation
ordering = ('-id',) | ||
list_per_page = 50 | ||
|
||
get_slug.short_description = 'slug' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this not set above?
No description provided.