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

Support Django 2.2 #5692

Merged
merged 5 commits into from
May 4, 2020
Merged

Support Django 2.2 #5692

merged 5 commits into from
May 4, 2020

Conversation

willbarton
Copy link
Member

@willbarton willbarton commented Apr 30, 2020

This PR makes a few modifications to support Django 2.2:

  • The undocumented request._post_parse_error, used in our logging alert handler, was removed in Django 2.1. This PR simply removes the check for it, as we have a generic Exception handler for formatting post data.
  • None is no longer encoded in post data by RequestFactory, and values that would be None should. instead be omitted. This PR makes a modification to the feedback tests where referrer and is_helpful were being passed as None if no other value was given.
  • schema_json must have a JSON-parsable value. In the jobmanager tests we perform a post to the Wagtail admin and schema_json was not defined. This PR defines it with a blank string.
  • Django 2.1+ enforces the use of positional arguments when calling management commands with call_command. In the feedback admin export, we use call_command to call the export_feedback management command, and were previously providing pages as a keyword argument. This PR explodes pages as positional arguments instead.

There is one final category of test failures we're currently seeing, with is_safe_url. @cwdavies has addressed these in #5680. unittest-future will fail here until #5680 is merged and this PR is rebased.

Testing

1.tox -e unittest-future

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 CFPB development guidelines
  • 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
  • Visually tested in supported browsers and devices (see checklist below 👇)
  • Project documentation has been updated
  • Reviewers requested with the Reviewers tool ➡️

Copy link
Contributor

@cwdavies cwdavies left a comment

Choose a reason for hiding this comment

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

All these changes look really good to me.
The changes you made were necessary to upgrade to Django 2.2.

This change sets Django 2.2 as our "future" version of Django for unit testing purposes.
This attribute was removed in django/django#10025. It also appears to be superfluous here given the `Exception` catcher.
The Django test RequestFactory will no longer encode `None` in post data. It should instead be omitted.
When POSTing page data, we need to provide a JSON-parsable value for CFGOVPage's schema_json.
Django 2.1+ handles positional arguments differently than Django 2.0 in `call_command`. In our feedback export in the admin it actually calls the `export_feedback` management command. This fixes the way it gets called to be compatible.
@willbarton willbarton merged commit fbf4913 into master May 4, 2020
@willbarton willbarton deleted the django22 branch May 4, 2020 14:05
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.

None yet

2 participants