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

Added a factory for Redirect model #83

Open
wants to merge 2 commits into
base: main
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions src/wagtail_factories/factories.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from factory import errors, utils
from factory.declarations import ParameteredAttribute
from factory.django import DjangoModelFactory
from wagtail.contrib.redirects.models import Redirect
from wagtail.documents import get_document_model
from wagtail.images import get_image_model
from wagtail.models import Collection, Page, Site
Expand All @@ -15,6 +16,7 @@
"PageFactory",
"SiteFactory",
"DocumentFactory",
"RedirectFactory",
]
logger = logging.getLogger(__file__)

Expand Down Expand Up @@ -144,3 +146,26 @@ class Meta:

title = "A document"
file = factory.django.FileField()


class RedirectFactory(DjangoModelFactory):
jams2 marked this conversation as resolved.
Show resolved Hide resolved
old_path = factory.Faker("slug")
site = factory.SubFactory(SiteFactory)
redirect_page = factory.SubFactory(PageFactory)
redirect_link = factory.Faker("url")
is_permanent = factory.Faker("boolean")

class Meta:
model = Redirect

@classmethod
def _create(cls, model_class, *args, **kwargs):
"""
Override _create() to ensure that Redirect.clean() is run
in order to normalise `old_path`.
@see https://github.com/wagtail/wagtail/blob/main/wagtail/contrib/redirects/models.py#L191
"""
obj = model_class(*args, **kwargs)
jams2 marked this conversation as resolved.
Show resolved Hide resolved
obj.clean()
Copy link
Contributor

Choose a reason for hiding this comment

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

Issue: I don't think we should be calling clean here, as it may clobber the values provided by a user. There are scenarios in which you may want to provide invalid data to a model instance — for example, if you want to test the clean method itself. I think our approach should be to make a best effort to generate example data that is both valid, and matches the typical usage of the object — but give the user the ability to create objects with data however they want, if they need to.

I would love to see a custom faker provider developed for old_path. We could use uri_path as a starting point (just using slugs is a little deficient). old_path also accepts query parameters, for which we could use pydict as a source. An API using traits to enable flags like has_query_params would be the icing on the cake.

Copy link
Contributor

Choose a reason for hiding this comment

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

A bit more context on whether or not we should call clean:

obj.save()
return obj
1 change: 1 addition & 0 deletions tests/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
"wagtail.contrib.settings",
"wagtail.contrib.table_block",
"wagtail.contrib.forms",
"wagtail.contrib.redirects",
"wagtail.search",
"wagtail.embeds",
"wagtail.images",
Expand Down
25 changes: 25 additions & 0 deletions tests/test_factories.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import wagtail_factories
from wagtail import blocks
from wagtail.models import Page, Site
from wagtail.contrib.redirects.models import Redirect

from tests.testapp.factories import MyTestPageFactory, MyTestPageGetOrCreateFactory

Expand Down Expand Up @@ -171,3 +172,27 @@ def test_document_add_to_collection():
collection__parent=root_collection, collection__name="new"
)
assert document.collection.name == "new"


@pytest.mark.django_db()
def test_redirect_factory():
"""
Test that a `RedirectFactory` generates a `Redirect` object.
"""
redirect = wagtail_factories.RedirectFactory()

assert type(redirect) is Redirect
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: isinstance is a more conventional way to do this.



@pytest.mark.django_db()
def test_redirect_factory_old_path_normalisation():
"""
Test that `RedirectFactory` uses `Redirect.clean()` to normalise the
`old_path` value.
"""

redirect = wagtail_factories.RedirectFactory(old_path="test-path/")

# Verify that `old_path` has been modified according to
# @https://github.com/wagtail/wagtail/blob/a09bba67cd58f519f3ae5bff32575e7ce9244031/wagtail/contrib/redirects/models.py#L141
assert redirect.old_path == "/test-path"