Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Picture plugin: small improvements #1468

Merged
merged 3 commits into from

3 participants

Richard Barran Benjamin Wohlwend Patrick Lauber
Richard Barran

Made a couple of small changes to the Picture plugin: tried to improve the help_text, added some input validation.

Question: the url field is used to store URLs (as the name suggests!) and is a CharField - I would have expected a URLField,
Is there a reason to use a CharField - I went through the history of this file and could not find any clues.
I would be happy to change the field type, but only if it's likely to get merged :-)

Benjamin Wohlwend
Collaborator

My guess is that it is a CharField to allow values that fail validation of the URLField, e.g. relative links, or internal links without protocol and domain.

btw, the travis build failure seems to have been caused by a travis problem, not your code. I hate that they advertise it as "failed build" when it is actually the service that failed...

Patrick Lauber
Collaborator

I don't remember either why it is a Charfield. I would merge a field type change. But only if it get's in after #1343

Patrick Lauber digi604 merged commit 764702a into from
Benjamin Wohlwend
Collaborator

@digi604 a switch to URLField would make the above use cases impossible, and I think they are relevant enough to keep around

Richard Barran

@piquadrat Storing relative links instead of a full URL - yes, I can see how that could be useful. It then leads on to the question "Should Django's URLField also handle these edge cases?", but that's a project for another day...

@digi604 Thanks for merging my code.

Benjamin Wohlwend
Collaborator

@richardbarran FYI, there's a ticket on Django's trac, but it's closed as wontfix: https://code.djangoproject.com/ticket/10896

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 110 additions and 71 deletions.
  1. +28 −13 cms/plugins/picture/models.py
  2. +82 −58 cms/tests/plugins.py
41 cms/plugins/picture/models.py
View
@@ -1,36 +1,51 @@
from django.db import models
from django.utils.translation import ugettext_lazy as _
+from django.core.exceptions import ValidationError
from cms.models import CMSPlugin, Page
from os.path import basename
class Picture(CMSPlugin):
"""
- A Picture with or without a link
+ A Picture with or without a link.
"""
- CENTER = "center"
LEFT = "left"
RIGHT = "right"
- FLOAT_CHOICES = ((CENTER, _("center")),
- (LEFT, _("left")),
+ CENTER = "center"
+ FLOAT_CHOICES = ((LEFT, _("left")),
(RIGHT, _("right")),
+ (CENTER, _("center")),
)
-
-
+
image = models.ImageField(_("image"), upload_to=CMSPlugin.get_media_path)
- url = models.CharField(_("link"), max_length=255, blank=True, null=True, help_text=_("if present image will be clickable"))
- page_link = models.ForeignKey(Page, verbose_name=_("page"), null=True, blank=True, help_text=_("if present image will be clickable"))
- alt = models.CharField(_("alternate text"), max_length=255, blank=True, null=True, help_text=_("textual description of the image"))
- longdesc = models.CharField(_("long description"), max_length=255, blank=True, null=True, help_text=_("additional description of the image"))
- float = models.CharField(_("side"), max_length=10, blank=True, null=True, choices=FLOAT_CHOICES)
-
+ url = models.CharField(_("link"), max_length=255, blank=True, null=True,
+ help_text=_("If present, clicking on image will take user to link."))
+ page_link = models.ForeignKey(Page, verbose_name=_("page"), null=True,
+ blank=True, help_text=_("If present, clicking on image will take user \
+ to specified page."))
+ alt = models.CharField(_("alternate text"), max_length=255, blank=True,
+ null=True, help_text=_("Specifies an alternate text for an image, if \
+ the image cannot be displayed.<br />Is also used by search engines to \
+ classify the image."))
+ longdesc = models.CharField(_("long description"), max_length=255,
+ blank=True, null=True, help_text=_("When user hovers above picture,\
+ this text will appear in a popup."))
+ float = models.CharField(_("side"), max_length=10, blank=True, null=True,
+ choices=FLOAT_CHOICES, help_text=_("Move image left, right or center."))
+
def __unicode__(self):
if self.alt:
return self.alt[:40]
elif self.image:
- # added if, because it raised attribute error when file wasn't defined
+ # added if, because it raised attribute error when file wasn't
+ # defined.
try:
return u"%s" % basename(self.image.path)
except:
pass
return "<empty>"
+
+ def clean(self):
+ if self.url and self.page_link:
+ raise ValidationError(
+ _("You can enter a Link or a Page, but not both."))
140 cms/tests/plugins.py
View
@@ -11,21 +11,22 @@
from cms.plugins.inherit.models import InheritPagePlaceholder
from cms.plugins.link.forms import LinkForm
from cms.plugins.link.models import Link
+from cms.plugins.picture.models import Picture
from cms.plugins.text.models import Text
-from cms.plugins.text.utils import (plugin_tags_to_id_list,
+from cms.plugins.text.utils import (plugin_tags_to_id_list,
plugin_tags_to_admin_html)
from cms.plugins.twitter.models import TwitterRecentEntries
from cms.test_utils.project.pluginapp.models import Article, Section
from cms.test_utils.project.pluginapp.plugins.manytomany_rel.models import (
ArticlePluginModel)
-from cms.test_utils.testcases import (CMSTestCase, URL_CMS_PAGE,
+from cms.test_utils.testcases import (CMSTestCase, URL_CMS_PAGE,
URL_CMS_PAGE_ADD, URL_CMS_PLUGIN_ADD, URL_CMS_PLUGIN_EDIT, URL_CMS_PAGE_CHANGE, URL_CMS_PLUGIN_REMOVE, URL_CMS_PLUGIN_HISTORY_EDIT)
from cms.sitemaps.cms_sitemap import CMSSitemap
from cms.test_utils.util.context_managers import SettingsOverride
from cms.utils.copy_plugins import copy_plugins_to
from django.conf import settings
from django.contrib.auth.models import User
-from django.core.exceptions import ImproperlyConfigured
+from django.core.exceptions import ImproperlyConfigured, ValidationError
from django.core.files.uploadedfile import SimpleUploadedFile
from django.core.management import call_command
from django.forms.widgets import Media
@@ -46,7 +47,7 @@ def render(self, context, instance, placeholder):
class PluginsTestBaseCase(CMSTestCase):
def setUp(self):
- self.super_user = User(username="test", is_staff = True, is_active = True, is_superuser = True)
+ self.super_user = User(username="test", is_staff=True, is_active=True, is_superuser=True)
self.super_user.set_password("test")
self.super_user.save()
@@ -57,10 +58,10 @@ def setUp(self):
self.FIRST_LANG = settings.LANGUAGES[0][0]
self.SECOND_LANG = settings.LANGUAGES[1][0]
-
+
self._login_context = self.login_user_context(self.super_user)
self._login_context.__enter__()
-
+
def tearDown(self):
self._login_context.__exit__(None, None, None)
@@ -91,7 +92,7 @@ def _create_text_plugin_on_page(self, page):
return created_plugin_id
def _edit_text_plugin(self, plugin_id, text):
- edit_url = "%s%s/" % (URL_CMS_PLUGIN_EDIT, plugin_id)
+ edit_url = "%s%s/" % (URL_CMS_PLUGIN_EDIT, plugin_id)
response = self.client.get(edit_url)
self.assertEquals(response.status_code, 200)
data = {
@@ -156,7 +157,7 @@ def test_plugin_order(self):
Test that plugin position is saved after creation
"""
page_en = create_page("PluginOrderPage", "col_two.html", "en",
- slug="page1",published=True, in_navigation=True)
+ slug="page1", published=True, in_navigation=True)
ph_en = page_en.placeholders.get(slot="col_left")
# We check created objects and objects from the DB to be sure the position value
@@ -167,13 +168,13 @@ def test_plugin_order(self):
db_plugin_2 = CMSPlugin.objects.get(pk=text_plugin_2.pk)
with SettingsOverride(CMS_MODERATOR=False, CMS_PERMISSION=False):
- self.assertEqual(text_plugin_1.position,1)
- self.assertEqual(db_plugin_1.position,1)
- self.assertEqual(text_plugin_2.position,2)
- self.assertEqual(db_plugin_2.position,2)
+ self.assertEqual(text_plugin_1.position, 1)
+ self.assertEqual(db_plugin_1.position, 1)
+ self.assertEqual(text_plugin_2.position, 2)
+ self.assertEqual(db_plugin_2.position, 2)
## Finally we render the placeholder to test the actual content
- rendered_placeholder = ph_en.render(self.get_context(page_en.get_absolute_url()),None)
- self.assertEquals(rendered_placeholder,"I'm the firstI'm the second")
+ rendered_placeholder = ph_en.render(self.get_context(page_en.get_absolute_url()), None)
+ self.assertEquals(rendered_placeholder, "I'm the firstI'm the second")
def test_add_cancel_plugin(self):
"""
@@ -269,54 +270,54 @@ def test_copy_plugins(self):
page_de = create_page("CopyPluginTestPage (DE)", "nav_playground.html", "de")
ph_en = page_en.placeholders.get(slot="body")
ph_de = page_de.placeholders.get(slot="body")
-
+
# add the text plugin
text_plugin_en = add_plugin(ph_en, "TextPlugin", "en", body="Hello World")
self.assertEquals(text_plugin_en.pk, CMSPlugin.objects.all()[0].pk)
-
+
# add a *nested* link plugin
link_plugin_en = add_plugin(ph_en, "LinkPlugin", "en", target=text_plugin_en,
name="A Link", url="https://www.django-cms.org")
-
+
# the call above to add a child makes a plugin reload required here.
text_plugin_en = self.reload(text_plugin_en)
-
+
# check the relations
self.assertEquals(text_plugin_en.get_children().count(), 1)
self.assertEqual(link_plugin_en.parent.pk, text_plugin_en.pk)
-
+
# just sanity check that so far everything went well
self.assertEqual(CMSPlugin.objects.count(), 2)
-
+
# copy the plugins to the german placeholder
copy_plugins_to(ph_en.cmsplugin_set.all(), ph_de, 'de')
-
+
self.assertEqual(ph_de.cmsplugin_set.filter(parent=None).count(), 1)
text_plugin_de = ph_de.cmsplugin_set.get(parent=None).get_plugin_instance()[0]
self.assertEqual(text_plugin_de.get_children().count(), 1)
link_plugin_de = text_plugin_de.get_children().get().get_plugin_instance()[0]
-
-
+
+
# check we have twice as many plugins as before
self.assertEqual(CMSPlugin.objects.count(), 4)
-
+
# check language plugins
self.assertEqual(CMSPlugin.objects.filter(language='de').count(), 2)
self.assertEqual(CMSPlugin.objects.filter(language='en').count(), 2)
-
-
+
+
text_plugin_en = self.reload(text_plugin_en)
link_plugin_en = self.reload(link_plugin_en)
-
+
# check the relations in english didn't change
self.assertEquals(text_plugin_en.get_children().count(), 1)
self.assertEqual(link_plugin_en.parent.pk, text_plugin_en.pk)
-
+
self.assertEqual(link_plugin_de.name, link_plugin_en.name)
self.assertEqual(link_plugin_de.url, link_plugin_en.url)
-
+
self.assertEqual(text_plugin_de.body, text_plugin_en.body)
-
+
def test_remove_plugin_before_published(self):
"""
@@ -452,7 +453,7 @@ def test_unregister_non_existing_plugin_should_raise(self):
# Let's count, to make sure we didn't remove a plugin accidentally.
number_of_plugins_after = len(plugin_pool.get_all_plugins())
self.assertEqual(number_of_plugins_before, number_of_plugins_after)
-
+
def test_inheritplugin_media(self):
"""
Test case for InheritPagePlaceholder
@@ -461,34 +462,34 @@ def test_inheritplugin_media(self):
inheritfrompage = create_page('page to inherit from',
'nav_playground.html',
'en')
-
+
body = inheritfrompage.placeholders.get(slot="body")
-
+
plugin = TwitterRecentEntries(
plugin_type='TwitterRecentEntriesPlugin',
- placeholder=body,
- position=1,
+ placeholder=body,
+ position=1,
language=settings.LANGUAGE_CODE,
twitter_user='djangocms',
)
plugin.insert_at(None, position='last-child', save=True)
-
+
page = create_page('inherit from page',
'nav_playground.html',
'en',
published=True)
-
+
inherited_body = page.placeholders.get(slot="body")
-
+
inherit_plugin = InheritPagePlaceholder(
plugin_type='InheritPagePlaceholderPlugin',
- placeholder=inherited_body,
- position=1,
+ placeholder=inherited_body,
+ position=1,
language=settings.LANGUAGE_CODE,
from_page=inheritfrompage,
from_language=settings.LANGUAGE_CODE)
inherit_plugin.insert_at(None, position='last-child', save=True)
-
+
self.client.logout()
response = self.client.get(page.get_absolute_url())
self.assertTrue('%scms/js/libs/jquery.tweet.js' % settings.STATIC_URL in response.content, response.content)
@@ -531,7 +532,7 @@ def test_copy_textplugin(self):
Test that copying of textplugins replaces references to copied plugins
"""
page = create_page("page", "nav_playground.html", "en")
-
+
placeholder = page.placeholders.get(slot='body')
plugin_base = CMSPlugin(
@@ -639,7 +640,7 @@ class FileSystemPluginTests(PluginsTestBaseCase):
def setUp(self):
super(FileSystemPluginTests, self).setUp()
call_command('collectstatic', interactive=False, verbosity=0, link=True)
-
+
def tearDown(self):
for directory in [settings.STATIC_ROOT, settings.MEDIA_ROOT]:
for root, dirs, files in os.walk(directory, topdown=False):
@@ -652,10 +653,10 @@ def tearDown(self):
# Now all directories we walked...
os.rmdir(os.path.join(root, name))
super(FileSystemPluginTests, self).tearDown()
-
+
def test_fileplugin_icon_uppercase(self):
page = create_page('testpage', 'nav_playground.html', 'en')
- body = page.placeholders.get(slot="body")
+ body = page.placeholders.get(slot="body")
plugin = File(
plugin_type='FilePlugin',
placeholder=body,
@@ -672,22 +673,22 @@ def test_fileplugin_icon_uppercase(self):
class PluginManyToManyTestCase(PluginsTestBaseCase):
def setUp(self):
- self.super_user = User(username="test", is_staff = True, is_active = True, is_superuser = True)
+ self.super_user = User(username="test", is_staff=True, is_active=True, is_superuser=True)
self.super_user.set_password("test")
self.super_user.save()
self.slave = User(username="slave", is_staff=True, is_active=True, is_superuser=False)
self.slave.set_password("slave")
self.slave.save()
-
+
self._login_context = self.login_user_context(self.super_user)
self._login_context.__enter__()
-
+
# create 3 sections
self.sections = []
self.section_pks = []
for i in range(3):
- section = Section.objects.create(name="section %s" %i)
+ section = Section.objects.create(name="section %s" % i)
self.sections.append(section)
self.section_pks.append(section.pk)
self.section_count = len(self.sections)
@@ -700,7 +701,7 @@ def setUp(self):
)
self.FIRST_LANG = settings.LANGUAGES[0][0]
self.SECOND_LANG = settings.LANGUAGES[1][0]
-
+
def test_add_plugin_with_m2m(self):
# add a new text plugin
page_data = self.get_new_page_data()
@@ -779,7 +780,7 @@ def test_add_plugin_with_m2m_and_publisher(self):
def test_copy_plugin_with_m2m(self):
page = create_page("page", "nav_playground.html", "en")
-
+
placeholder = page.placeholders.get(slot='body')
plugin = ArticlePluginModel(
@@ -899,7 +900,7 @@ class Meta:
class SekizaiTests(TestCase):
def test_post_patch_check(self):
post_patch_check()
-
+
def test_fail(self):
with SettingsOverride(CMS_TEMPLATES=[('fail.html', 'fail')]):
self.assertRaises(ImproperlyConfigured, post_patch_check)
@@ -948,30 +949,53 @@ def test_render_meta_is_unique(self):
text = Text()
link = Link()
self.assertNotEqual(id(text._render_meta), id(link._render_meta))
-
+
def test_render_meta_does_not_leak(self):
text = Text()
link = Link()
-
+
text._render_meta.text_enabled = False
link._render_meta.text_enabled = False
-
+
self.assertFalse(text._render_meta.text_enabled)
self.assertFalse(link._render_meta.text_enabled)
-
+
link._render_meta.text_enabled = True
self.assertFalse(text._render_meta.text_enabled)
self.assertTrue(link._render_meta.text_enabled)
-
+
def test_db_table_hack(self):
# TODO: Django tests seem to leak models from test methods, somehow
# we should clear django.db.models.loading.app_cache in tearDown.
plugin_class = PluginModelBase('TestPlugin', (CMSPlugin,), {'__module__': 'cms.tests.plugins'})
self.assertEqual(plugin_class._meta.db_table, 'cmsplugin_testplugin')
-
+
def test_db_table_hack_with_mixin(self):
class LeftMixin: pass
class RightMixin: pass
plugin_class = PluginModelBase('TestPlugin2', (LeftMixin, CMSPlugin, RightMixin), {'__module__': 'cms.tests.plugins'})
self.assertEqual(plugin_class._meta.db_table, 'cmsplugin_testplugin2')
+
+class PicturePluginTests(PluginsTestBaseCase):
+
+ def test_link_or_page(self):
+ """Test a validator: you can enter a url or a page_link, but not both."""
+
+ page_data = self.get_new_page_data()
+ response = self.client.post(URL_CMS_PAGE_ADD, page_data)
+ page = Page.objects.all()[0]
+
+ picture = Picture(url="test")
+ # Note: don't call full_clean as it will check ALL fields - including
+ # the image, which we haven't defined. Call clean() instead which
+ # just validates the url and page_link fields.
+ picture.clean()
+
+ picture.page_link = page
+ picture.url = None
+ picture.clean()
+
+ picture.url = "test"
+ self.assertRaises(ValidationError, picture.clean)
+
Something went wrong with that request. Please try again.