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

Add support for Wagtail 2.3 #1

Closed
wants to merge 10 commits into from
Closed

Add support for Wagtail 2.3 #1

wants to merge 10 commits into from

Conversation

cwdavies
Copy link

@cwdavies cwdavies commented Feb 19, 2020

Update to use latest version 1.0.2 of regdown and Python 3.8
Fix ValueError: No item named "strong_em" exists when running tox

Additions

  • Support Wagtail 2.3 and 2.8 in tox.ini and travis.yml and README.md
  • Add wagtail-copyablemodeladmin to regulations_example/requirements.txt

Removals

  • Wagtail 2.2 and 2.7 from tox.ini and travis.yml
  • Python 3.7 from tox.ini and travis.yml
  • six from known_future_library
  • six.moves from pages.py

Changes

  • Use Python 3.8 in Dockerfile and tox.ini and travis.yml
  • Use Django 2.2 in tox.ini and travis.yml and setup.py and README.md
  • Update regdown to 1.0.2 in regulations_example/requirements.txt
  • Update wagtail-treemodeladmin to 1.1.1 in regulations_example/requirements.txt

Testing

  1. tox
...
py38-dj22-wag28 run-test: commands[0] | coverage erase
py38-dj22-wag28 run-test: commands[1] | coverage run --source=wagtailregulations /Users/DaviesC/Projects/CFPB/wagtail-regulations/.tox/py38-dj22-wag28/bin/django-admin.py test
Creating test database for alias 'default'...
System check identified no issues (0 silenced).
........................................................................
----------------------------------------------------------------------
Ran 72 tests in 5.481s

OK
Destroying test database for alias 'default'...
py38-dj22-wag28 run-test: commands[2] | coverage report -m
Name                                                 Stmts   Miss  Cover   Missing
----------------------------------------------------------------------------------
wagtailregulations/__init__.py                           0      0   100%
wagtailregulations/blocks.py                            35      3    91%   51-55
wagtailregulations/migrations/0001_initial.py            7      0   100%
wagtailregulations/migrations/__init__.py                0      0   100%
wagtailregulations/models/__init__.py                    2      0   100%
wagtailregulations/models/django.py                    127      0   100%
wagtailregulations/models/pages.py                     161      2    99%   315, 331
wagtailregulations/parser/__init__.py                    0      0   100%
wagtailregulations/parser/integer_conversion.py         52     38    27%   14-30, 35-48, 57-68
wagtailregulations/parser/paragraphs.py                 34     25    26%   14-17, 24-30, 35, 47-49, 53-57, 61-64, 72-79
wagtailregulations/parser/patterns.py                  213    182    15%   62, 65, 69, 72, 75-78, 81-84, 87-90, 107-112, 115-118, 122-134, 138-159, 163-187, 191-260, 268-274, 278-287, 296-300, 313-336, 341-356
wagtailregulations/parser/payload.py                    54     28    48%   46-58, 61-72, 75-86, 89-100
wagtailregulations/parser/regtable.py                   43     36    16%   18-23, 26, 33-40, 45-82
wagtailregulations/resolver.py                          35      0   100%
wagtailregulations/scripts/__init__.py                   0      0   100%
wagtailregulations/scripts/ecfr_importer.py            341    300    12%   66-113, 118-134, 142-179, 187-198, 202-207, 212-226, 230-237, 241-253, 257-261, 272-302, 313-326, 333-354, 381-397, 402-418, 424-430, 441-446, 473-533, 555-589, 593-620
wagtailregulations/scripts/insert_section_links.py      58      0   100%
wagtailregulations/serializers.py                       30      5    83%   21-29
wagtailregulations/tests/__init__.py                     0      0   100%
wagtailregulations/tests/settings.py                    15      0   100%
wagtailregulations/tests/test_blocks.py                 27      0   100%
wagtailregulations/tests/test_hooks.py                  33      0   100%
wagtailregulations/tests/test_models_django.py          60      0   100%
wagtailregulations/tests/test_models_pages.py          152      0   100%
wagtailregulations/tests/test_resolver.py               41      0   100%
wagtailregulations/tests/test_scripts.py                59      0   100%
wagtailregulations/tests/test_serializers.py             0      0   100%
wagtailregulations/tests/urls.py                         4      0   100%
wagtailregulations/tests/utils.py                       25      0   100%
wagtailregulations/wagtail_hooks.py                     67     18    73%   99-124
----------------------------------------------------------------------------------
TOTAL                                                 1675    637    62%
_______________________________________________ summary ________________________________________________
  lint: commands succeeded
  py36-dj20-wag23: commands succeeded
  py36-dj22-wag28: commands succeeded
  py38-dj22-wag28: commands succeeded
  congratulations :)

Todos

  • Fix AttributeError: 'RelatedManager' object has no attribute 'url'

Checklist

  • PR has an informative and human-readable title
  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows the standards laid out in the development playbook
  • Passes all existing automated tests
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future todos are captured in comments
  • Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)
  • Reviewers requested with the Reviewers tool ➡️

@willbarton
Copy link
Member

Because wagtail-regulations is still very much a work in progress, I'm not inclined to go piece-meal to Wagtail 2.3. We do not have to worry about it right now — it's not complete and we do not use it in cfgov-refresh.

@cwdavies cwdavies changed the title Support Wagtail 2.3 Add support for Wagtail 2.3 Feb 24, 2020
@cwdavies
Copy link
Author

As wagtail-regulations is work in progress, changes have now been made to go directly to Wagtail 2.3 and remove the support for Wagtail 1.13 in this PR. As the SerializersTestCase was not working, this class has been commented out so that tox passes locally.

@willbarton
Copy link
Member

@cwdavies I'd rather leave code in and have the tests fail until we can fix them than pursue passing tests no matter what.

Once we get to Wagtail 2.8 I intend to go through and update this package from cfgov/regulations3k in cfgov-refresh anyway, so we should spend our time working on any failures or issues that come up there.

@cwdavies
Copy link
Author

cwdavies commented Feb 27, 2020

@willbarton I will uncomment code for failing tests in test_serializers.py and commit to the support-wag23 branch. When I ran the following command I was seeing an error:

$ python manage.py makemigrations
...
mod = importlib.import_module(self.SETTINGS_MODULE)
...
ModuleNotFoundError: No module named 'regulations_example.settings.dev'; 'regulations_example.settings' is not a package

$ grep -r SETTINGS_MODULE .
./Dockerfile:ENV DJANGO_SETTINGS_MODULE=regulations_example.settings
./tox.ini:    DJANGO_SETTINGS_MODULE=wagtailregulations.tests.settings
./manage.py:        "DJANGO_SETTINGS_MODULE",

The output from running the first serializers test is:

py38-dj22-wag28 run-test: commands[3] | coverage run --source=wagtailregulations /Users/DaviesC/Projects/CFPB/wagtail-regulations/.tox/py38-dj22-wag28/bin/django-admin.py test
Creating test database for alias 'default'...
System check identified no issues (0 silenced).
....................................................................E....
======================================================================
ERROR: test_section_serialier (wagtailregulations.tests.test_serializers.SerializersTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/DaviesC/Projects/CFPB/wagtail-regulations/wagtailregulations/tests/test_serializers.py", line 14, in test_section_serialier
    data = SectionSerializer(self.section_num4).data
  File "/Users/DaviesC/Projects/CFPB/wagtail-regulations/.tox/py38-dj22-wag28/lib/python3.8/site-packages/rest_framework/serializers.py", line 562, in data
    ret = super().data
  File "/Users/DaviesC/Projects/CFPB/wagtail-regulations/.tox/py38-dj22-wag28/lib/python3.8/site-packages/rest_framework/serializers.py", line 260, in data
    self._data = self.to_representation(self.instance)
  File "/Users/DaviesC/Projects/CFPB/wagtail-regulations/.tox/py38-dj22-wag28/lib/python3.8/site-packages/rest_framework/serializers.py", line 529, in to_representation
    ret[field.field_name] = field.to_representation(attribute)
  File "/Users/DaviesC/Projects/CFPB/wagtail-regulations/.tox/py38-dj22-wag28/lib/python3.8/site-packages/rest_framework/fields.py", line 1905, in to_representation
    return method(value)
  File "/Users/DaviesC/Projects/CFPB/wagtail-regulations/wagtailregulations/serializers.py", line 24, in get_html_contents
    html_contents = regdown(
  File "/Users/DaviesC/Projects/CFPB/regdown/regdown/__init__.py", line 261, in regdown
    return markdown(
  File "/Users/DaviesC/Projects/CFPB/wagtail-regulations/.tox/py38-dj22-wag28/lib/python3.8/site-packages/markdown/core.py", line 388, in markdown
    return md.convert(text)
  File "/Users/DaviesC/Projects/CFPB/wagtail-regulations/.tox/py38-dj22-wag28/lib/python3.8/site-packages/markdown/core.py", line 265, in convert
    root = self.parser.parseDocument(self.lines).getroot()
  File "/Users/DaviesC/Projects/CFPB/wagtail-regulations/.tox/py38-dj22-wag28/lib/python3.8/site-packages/markdown/blockparser.py", line 90, in parseDocument
    self.parseChunk(self.root, '\n'.join(lines))
  File "/Users/DaviesC/Projects/CFPB/wagtail-regulations/.tox/py38-dj22-wag28/lib/python3.8/site-packages/markdown/blockparser.py", line 105, in parseChunk
    self.parseBlocks(parent, text.split('\n\n'))
  File "/Users/DaviesC/Projects/CFPB/wagtail-regulations/.tox/py38-dj22-wag28/lib/python3.8/site-packages/markdown/blockparser.py", line 123, in parseBlocks
    if processor.run(parent, blocks) is not False:
  File "/Users/DaviesC/Projects/CFPB/regdown/regdown/__init__.py", line 238, in run
    url = self.url_resolver(label)
  File "/Users/DaviesC/Projects/CFPB/wagtail-regulations/wagtailregulations/resolver.py", line 79, in url_resolver
    page_url=page.url,
AttributeError: 'RelatedManager' object has no attribute 'url'

As there are always multiple sections per regulations page I was looking at the code in serializers.py

$ grep Serializer wagtailregulations/serializers.py
class SectionSerializer(serializers.ModelSerializer):
    html_contents = serializers.SerializerMethodField()
class SubpartSerializer(serializers.ModelSerializer):
    sections = SectionSerializer(many=True)
class EffectiveVersionSerializer(serializers.ModelSerializer):
    subparts = SubpartSerializer(many=True)
class PartSerializer(serializers.ModelSerializer):
    versions = EffectiveVersionSerializer(many=True)

I learned that if you have nested serializers and you're expecting more than one, you should add the many=True otherwise DRF will treat the manager as the object.

DRF is Django Rest Framework and I do see many=True in serializers.py

@willbarton willbarton force-pushed the master branch 4 times, most recently from e39f527 to 87d1485 Compare March 23, 2020 18:39
@willbarton willbarton closed this Mar 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants