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

Error when loading in wagtailtrans own hooks #153

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 0 additions & 1 deletion .coveragerc
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,3 @@ omit =
src/wagtailtrans/__init__.py
src/wagtailtrans/admin.py
src/wagtailtrans/config.py
src/wagtailtrans/wagtail_hooks.py
5 changes: 2 additions & 3 deletions src/wagtailtrans/wagtail_hooks.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,13 +41,12 @@ def global_admin_js():
path=static('wagtailtrans/js/site_languages_editor.js'))
)


if not get_wagtailtrans_setting('SYNC_TREE'):
"""Only load hooks when WAGTAILTRANS_SYNC_TREE is disabled"""

@hooks.register('register_page_listing_buttons')
def page_translations_menu(page, page_perms, is_parent=False):
if not hasattr(page, 'language'):
if not issubclass(page.__class__, TranslatablePage):
return

if hasattr(page, 'canonical_page') and page.canonical_page:
Expand Down Expand Up @@ -116,7 +115,7 @@ def edit_in_language_button(page, page_perms, is_parent=False):
clear interface to work in.

"""
if not hasattr(page, 'language'):
if not issubclass(page.__class__, TranslatablePage):
Copy link
Contributor

@kaedroho kaedroho Mar 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry, my last comment was misleading. I meant isinstance, as you had changed it just before, not sure why I said issubclass!

return

yield widgets.ButtonWithDropdownFromHook(
Expand Down
29 changes: 29 additions & 0 deletions tests/_sandbox/pages/migrations/0003_article.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
# Generated by Django 2.1.5 on 2019-02-12 08:23

from django.db import migrations, models
import django.db.models.deletion
import wagtail.core.fields


class Migration(migrations.Migration):

dependencies = [
('wagtailcore', '0040_page_draft_title'),
('pages', '0002_auto_20161214_0842'),
]

operations = [
migrations.CreateModel(
name='Article',
fields=[
('page_ptr', models.OneToOneField(auto_created=True, on_delete=django.db.models.deletion.CASCADE, parent_link=True, primary_key=True, serialize=False, to='wagtailcore.Page')),
('date', models.DateField()),
('intro_text', wagtail.core.fields.RichTextField()),
('language', models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, to='wagtailtrans.Language')),
],
options={
'abstract': False,
},
bases=('wagtailcore.page',),
),
]
13 changes: 13 additions & 0 deletions tests/_sandbox/pages/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,16 @@ class HomePage(TranslatablePage):
]

subpage_types = ['HomePage']


class Article(Page):
date = models.DateField()
language = models.ForeignKey('wagtailtrans.Language', on_delete=models.SET_NULL, null=True)
intro_text = RichTextField()

content_panels = [
FieldPanel('title', 'full title'),
FieldPanel('date'),
FieldPanel('intro_text'),
FieldPanel('language'),
]
37 changes: 36 additions & 1 deletion tests/factories/pages.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
import datetime

import factory
from factory.fuzzy import FuzzyDate
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

factory.fuzzy is deprecated according to the docs: https://factoryboy.readthedocs.io/en/latest/fuzzy.html

from wagtail.core.models import Page
from wagtail.images.tests.utils import (
get_image_model, get_test_image_file)

from wagtailtrans import models

from tests._sandbox.pages.models import HomePage
from tests._sandbox.pages.models import HomePage, Article
from tests.factories import language


Expand Down Expand Up @@ -74,3 +77,35 @@ class WagtailPageFactory(factory.DjangoModelFactory):

class Meta:
model = Page

@classmethod
def _create(cls, *args, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think we need this since we're mostly using a pytest fixture when creating a site tree. Or is there any particular reason why you implemented this?

model = args[0]
parent = kwargs.pop(
'parent',
Page.objects.get(path='0001')
)

try:
return model.objects.get(title=kwargs['title'])
except model.DoesNotExist:
kwargs['depth'] = parent.depth + 1

if kwargs['depth'] == 0:
return model.add_root(**kwargs)
else:
return parent.add_child(
instance=model(
**kwargs
)
)


class ArticleFactory(WagtailPageFactory):
title = 'Article 1'
intro_text = 'This is an article'
date = FuzzyDate(datetime.date(2019, 1, 1))
language = factory.SubFactory(language.LanguageFactory)

class Meta:
model = Article
50 changes: 50 additions & 0 deletions tests/unit/test_wagtail_hooks.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
import pytest

from wagtail.admin import widgets
from wagtail.core import hooks

from tests.factories.pages import ArticleFactory
from wagtailtrans.models import TranslatablePage
from wagtailtrans.wagtail_hooks import edit_in_language_button


@pytest.mark.django_db
class TestWagtailHooks:
def setup(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need the 2 pages created in the setup for each test, so lets just create the page we need in the test itself

self.article = ArticleFactory()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The purpose of this ArticlePage is to have a page with a relation to language which doesn't extend from TranslatablePage right? so lets call it: NormalPageWithLanguage or something like that so we have a more clear view for what purpose this page is created

self.transpage = TranslatablePage()

@hooks.register('register_page_listing_buttons')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you defining this here? I think we can just import it right, just like the edit_in_language_button, now you're testing code which is defined in the test, seems a bit weird..

def page_translations_menu(self, page, page_perms, is_parent=False):
# To test this hook i need to add it here, if we don't do this
# we can't import the hook like the edit_in_language_button
if not issubclass(page.__class__, TranslatablePage):
return

if hasattr(page, 'canonical_page') and page.canonical_page:
return

yield widgets.ButtonWithDropdownFromHook(
'Translate into',
hook_name='wagtailtrans_dropdown_hook',
page=page,
page_perms=page_perms,
is_parent=is_parent,
priority=10
)

def test_edit_in_language_wagtail_hook_with_regular_page(self):
assert list(edit_in_language_button(self.article, page_perms=[])) == []

def test_edit_in_language_wagtail_hook_translateable_page(self):
result = list(edit_in_language_button(self.transpage, page_perms=[]))
assert len(result) == 1
assert result[0].label == 'Edit in'

def test_page_translations_menu_with_regular_page(self):
assert list(self.page_translations_menu(self.article, page_perms=[])) == []

def test_page_translations_menu_translateable_page(self):
result = list(self.page_translations_menu(self.transpage, page_perms=[]))
assert len(result) == 1
assert result[0].label == 'Translate into'