Added a factory for Redirect model #83
Conversation
JakubMastalerz
changed the title
JakubMastalerz
force-pushed
the
feature/redirect-factory
branch
from
41c036c
to
28e7b21
Compare
JakubMastalerz
changed the title
There was a problem hiding this comment.
Looking great so far @JakubMastalerz. It would be nice to see documentation too if you have time.
| @see https://github.com/wagtail/wagtail/blob/main/wagtail/contrib/redirects/models.py#L191 | ||
| """ | ||
| obj = model_class(*args, **kwargs) | ||
| obj.clean() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
A bit more context on whether or not we should call clean:
- Django does not call
full_cleaninsaveby default (https://docs.djangoproject.com/en/5.0/ref/models/instances/#django.db.models.Model.full_clean) - Wagtail does for
Pagemodels, but it is optional, and exists to facilitate restoring revisions (https://github.com/wagtail/wagtail/blob/main/wagtail/models/__init__.py#L1495)
| """ | ||
| redirect = wagtail_factories.RedirectFactory() | ||
|
|
||
| assert type(redirect) is Redirect |
There was a problem hiding this comment.
Suggestion: isinstance is a more conventional way to do this.
JakubMastalerz
force-pushed
the
feature/redirect-factory
branch
from
28e7b21
to
5ed7344
Compare
Resolves #82
This PR adds a factory for Wagtail's Redirect model.