Princeton-CDH / mep-django

Shakespeare and Company Project - Python/Django web application
https://shakespeareandco.princeton.edu
Apache License 2.0
5 stars 1 forks source link

Maintenance and upgrades #789

Closed quadrismegistus closed 7 months ago

codecov[bot] commented 9 months ago

Codecov Report

Merging #789 (bccdb29) into develop (4526351) will increase coverage by 0.02%. The diff coverage is 100.00%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #789 +/- ## =========================================== + Coverage 97.49% 97.51% +0.02% =========================================== Files 228 229 +1 Lines 12732 12859 +127 Branches 81 81 =========================================== + Hits 12413 12540 +127 Misses 316 316 Partials 3 3 ```
quadrismegistus commented 8 months ago

Does it pass locally for you?

I'm getting the same error locally -- it's a mismatch in a testing fixture (in mep/pages/fixtures/wagtail_pages.json). It was:

{
        "fields": {
            "approved_go_live_at": null,
            "content_json": "{\"pk\": 14, \"path\": \"0001000100020001\", \"depth\": 4, \"numchild\": 0, \"title\": \"Test Analysis\", \"draft_title\": \"Test Analysis\", \"slug\": \"test-analysis\", \"content_type\": 60, \"live\": true, \"has_unpublished_changes\": false, \"url_path\": \"/home/analysis/2019/11/test-analysis/\", \"owner\": 2, \"seo_title\": \"\", \"show_in_menus\": false, \"search_description\": \"\", \"go_live_at\": null, \"expire_at\": null, \"expired\": false, \"locked\": false, \"first_published_at\": null, \"last_published_at\": null, \"latest_revision_created_at\": null, \"live_revision\": null, \"description\": \"<p>here&#x27;s my description!</p>\", \"body\": \"[{\\\"type\\\": \\\"paragraph\\\", \\\"value\\\": \\\"<p>here&#x27;s the body.</p>\\\", \\\"id\\\": \\\"52af9af6-48ba-4aef-a2b1-8a3562e6b281\\\"}]\", \"authors\": \"[]\", \"editors\": \"[]\", \"featured_image\": null}",
            "created_at": "2019-11-15T15:40:39.303Z",
            "page": 14,
            "submitted_for_moderation": false,
            "user": 2
        },
        "model": "wagtailcore.pagerevision",
        "pk": 1
    },

I'm updating it to, according to here and here:

{
        "fields": {
            "approved_go_live_at": null,
            "content": {"pk": 14, "path": "0001000100020001", "depth": 4, "numchild": 0, "title": "Test Analysis", "draft_title": "Test Analysis", "slug": "test-analysis", "content_type": 60, "live": true, "has_unpublished_changes": false, "url_path": "/home/analysis/2019/11/test-analysis/", "owner": 2, "seo_title": "", "show_in_menus": false, "search_description": "", "go_live_at": null, "expire_at": null, "expired": false, "locked": false, "first_published_at": null, "last_published_at": null, "latest_revision_created_at": null, "live_revision": null, "description": "<p>here&#x27;s my description!</p>", "body": "[{\"type\": \"paragraph\", \"value\": \"<p>here&#x27;s the body.</p>\", \"id\": \"52af9af6-48ba-4aef-a2b1-8a3562e6b281\"}]", "authors": "[]", "editors": "[]", "featured_image": null},
            "created_at": "2019-11-15T15:40:39.303Z",
            "page": 14,
            "submitted_for_moderation": false,
            "user": 2
        },
        "model": "wagtailcore.revision",
        "pk": 1
},

But I'm still getting this error:

Problem installing fixture '/Users/ryanheuser/github/mep-django/mep/pages/fixtures/wagtail_pages.json': 'GenericRel' object has no attribute 'to_python': (wagtailcore.revision:pk=1) field_value was '14'

From trace:

_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

object_list = [{'fields': {'content_type': ['pages', 'homepage'], 'depth': 2, 'draft_title': 'Shakespeare & Company Project', 'expir...ne, 'tagline': 'Learn about the Shakespeare and Company Project.'}, 'model': 'pages.contentlandingpage', 'pk': 4}, ...]
using = 'default', ignorenonexistent = False, options = {}, handle_forward_references = True
field_names_cache = {<class 'wagtail.models.Page'>: {'_revisions', '_workflow_states', 'alias_of', 'aliases', 'content_type', 'contentland...models.EssayLandingPage'>: {'_revisions', '_workflow_states', 'alias_of', 'aliases', 'body', 'content_type', ...}, ...}
d = {'fields': {'approved_go_live_at': None, 'content': {'authors': '[]', 'body': '[{"type": "paragraph", "value": "<p>her...'depth': 4, ...}, 'created_at': '2019-11-15T15:40:39.303Z', 'page': 14, ...}, 'model': 'wagtailcore.revision', 'pk': 1}
Model = <class 'wagtail.models.Revision'>
data = {'approved_go_live_at': None, 'content': {'authors': '[]', 'body': '[{"type": "paragraph", "value": "<p>here&#x27;s th..._type': 60, 'depth': 4, ...}, 'created_at': datetime.datetime(2019, 11, 15, 15, 40, 39, 303000, tzinfo=<UTC>), 'id': 1}

    def Deserializer(object_list, *, using=DEFAULT_DB_ALIAS, ignorenonexistent=False, **options):
        """
        Deserialize simple Python objects back into Django ORM instances.

        It's expected that you pass the Python objects themselves (instead of a
        stream or a string) to the constructor
        """
        handle_forward_references = options.pop('handle_forward_references', False)
        field_names_cache = {}  # Model: <list of field_names>

        for d in object_list:
            # Look up the model and starting build a dict of data for it.
            try:
                Model = _get_model(d["model"])
            except base.DeserializationError:
                if ignorenonexistent:
                    continue
                else:
                    raise
            data = {}
            if 'pk' in d:
                try:
                    data[Model._meta.pk.attname] = Model._meta.pk.to_python(d.get('pk'))
                except Exception as e:
                    raise base.DeserializationError.WithData(e, d['model'], d.get('pk'), None)
            m2m_data = {}
            deferred_fields = {}

            if Model not in field_names_cache:
                field_names_cache[Model] = {f.name for f in Model._meta.get_fields()}
            field_names = field_names_cache[Model]

            # Handle each field
            for (field_name, field_value) in d["fields"].items():

                if ignorenonexistent and field_name not in field_names:
                    # skip fields no longer on model
                    continue

                field = Model._meta.get_field(field_name)

                # Handle M2M relations
                if field.remote_field and isinstance(field.remote_field, models.ManyToManyRel):
                    try:
                        values = base.deserialize_m2m_values(field, field_value, using, handle_forward_references)
                    except base.M2MDeserializationError as e:
                        raise base.DeserializationError.WithData(e.original_exc, d['model'], d.get('pk'), e.pk)
                    if values == base.DEFER_FIELD:
                        deferred_fields[field] = field_value
                    else:
                        m2m_data[field.name] = values
                # Handle FK fields
                elif field.remote_field and isinstance(field.remote_field, models.ManyToOneRel):
                    try:
                        value = base.deserialize_fk_value(field, field_value, using, handle_forward_references)
                    except Exception as e:
                        raise base.DeserializationError.WithData(e, d['model'], d.get('pk'), field_value)
                    if value == base.DEFER_FIELD:
                        deferred_fields[field] = field_value
                    else:
                        data[field.attname] = value
                # Handle all other fields
                else:
                    try:
                        data[field.name] = field.to_python(field_value)
                    except Exception as e:
>                       raise base.DeserializationError.WithData(e, d['model'], d.get('pk'), field_value)
E                       django.core.serializers.base.DeserializationError: Problem installing fixture '/Users/ryanheuser/github/mep-django/mep/pages/fixtures/wagtail_pages.json': 'GenericRel' object has no attribute 'to_python': (wagtailcore.revision:pk=1) field_value was '14'

venv/lib/python3.8/site-packages/django/core/serializers/python.py:146: DeserializationError

Google isn't turning up anything explicitly about this error; let me know if you sense any immediate clues here about the fixtures?, otherwise I'll keep looking.

rlskoeser commented 8 months ago

Not sure what's going on exactly; I do remember that the pages fixture is a little brittle, since it's so specific to the version of the database. I suspect that more fields have been changed than what they put in the release notes, since they probably don't expect people to be using the internals of page and revision models much.

It might be easiest if we can get wagtail to migrate the fixture for us. The way to do that would be to create an empty database, reverse data migrations back to before this wagtail upgrade (or maybe easier: create the empty db in a different branch first), load the fixture, run migrations, and then reexport with dumpdata (or django fixture magic if necessary) and update the fixture with the new version.

Another option would be to export some current wagtail pages and revisions and inspect them to see how the structure of our fixture needs to change.

You can test loading the fixture using loaddata:

./manage.py loaddata mep/pages/fixtures/wagtail_pages.json
quadrismegistus commented 8 months ago

How does one export the current wagtail pages? That seems more straightforward approach (copying their structure) worth trying first?

rlskoeser commented 8 months ago

How does one export the current wagtail pages? That seems more straightforward approach (copying their structure) worth trying first?

Look at the dumpdata manage command. You can specify an app or model to limit to a particular set of records - like wagtailcore.revision or wagtailcore.page. You'll get all the records of that type, I recommend using --indent 2 to make it readable and piping to less or your favorite pager.

quadrismegistus commented 7 months ago

Ok, making progress on these tests beyond the issues described here by turning to wagtail-factories, which in turn uses factory boy, a package which tries to replace fixtures and turn them into class factories for a few a different ORMs, including Django.

I was able to get the home page tests working using the following code. I added some tests to make sure the content is correct as well. I can condense them, but I wanted to paste this here for my and our future reference. I'm pushing this change and working on converting other tests now.

class HomePageFactory(wagtail_factories.PageFactory):
    title=HOMEPAGE_TITLE
    slug=HOMEPAGE_SLUG
    body=wagtail_factories.StreamFieldFactory({
        "paragraph":factory.SubFactory(wagtail_factories.CharBlockFactory),
        "footnotes":factory.SubFactory(wagtail_factories.CharBlockFactory)
    })

    class Meta:
        model = HomePage

class TestHomePage(WagtailPageTests):
    fixtures = ["wagtail_pages"]

    def setUp(self):
        self.homepage = HomePageFactory.create(
            body__0__paragraph=RichText(HOMEPAGE_PARA),
            body__1__footnotes=RichText(HOMEPAGE_FOOT),
        )
        self.site = Site.objects.first()
        self.site.root_page = self.homepage
        self.site.save()
        self.site.refresh_from_db()

    def test_can_create(self):
        self.assertIsNotNone(self.homepage.pk)
        assert self.homepage.title == HOMEPAGE_TITLE
        assert self.homepage.slug == HOMEPAGE_SLUG
        body = self.homepage.body

        assert len(body) == 2
        block1,block2 = body
        type1,type2=block1.block_type, block2.block_type
        val1,val2=block1.value.source, block2.value.source

        assert type1 == 'paragraph'
        assert val1 == HOMEPAGE_PARA

        assert type2 == 'footnotes'
        assert val2 == HOMEPAGE_FOOT

    def test_parent_pages(self):
        self.assertAllowedParentPageTypes(HomePage, [Page])

    def test_subpages(self):
        self.assertAllowedSubpageTypes(
            HomePage, [ContentLandingPage, EssayLandingPage, ContentPage]
        )

    def test_template(self):
        url = self.homepage.relative_url(self.site)
        response = self.client.get(url)
        self.assertTemplateUsed(response, "base.html")
        self.assertTemplateUsed(response, "pages/home_page.html")

Here's the commit making this change.

quadrismegistus commented 7 months ago

All but one test passing, which I believe is related to this unanswered stackoverflow question. [Edit: I asked the poster in a comment if he ever figured out the issue; he replied that he did not.]

It's test_template below, because the url it's retrieving /analysis2/2023/11/new-essay/ returns 404. (analysis2 is the slug for EssayLandingPageFactory).

SECONDESSAYPAGE_TITLE="A new analysis esssay"
SECONDESSAYPAGE_SLUG="new-essay"
SECONDESSAYPAGE_PARA="this page lives right under analysis"

class TestEssayPage(PageTester):
    def setUp(self):
        super().setUp()
        self.landingpage = EssayLandingPageFactory.create(
            body__0__paragraph=RichText(ESSAYLANDINGPAGE_PARA),
            parent=self.homepage
        )
        self.page = EssayPageFactory.create(
            title=SECONDESSAYPAGE_TITLE,
            slug=SECONDESSAYPAGE_SLUG,
            body__0__paragraph=RichText(SECONDESSAYPAGE_PARA),
            parent=self.landingpage
        )

    def test_template(self):
        url = self.page.relative_url(self.site)
        print('getting url:', url)
        response = self.client.get(url)
        self.assertTemplateUsed(response, "base.html")
        self.assertTemplateUsed(response, "pages/base_page.html")
        self.assertTemplateUsed(response, "pages/essay_page.html")
quadrismegistus commented 7 months ago

@rlskoeser Thanks for the first_published_at tip: coding that into the factory class fixed it! All tests passing now. Re-tagging you for review. Hoping we can merge this, finish maintenance work and get onto new data exports.