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

Upgrade to Django 4.2 #3049

Closed
43 tasks done
sergei-maertens opened this issue May 3, 2023 · 29 comments · Fixed by #3746 or #3844
Closed
43 tasks done

Upgrade to Django 4.2 #3049

sergei-maertens opened this issue May 3, 2023 · 29 comments · Fixed by #3746 or #3844
Assignees
Milestone

Comments

@sergei-maertens
Copy link
Member

sergei-maertens commented May 3, 2023

Checklist to prepare the Django 4.2 LTS upgrade

3rd party packages to verify/make compatible

Open Forms itself

  • dark theme for tinymce (d9731f6)
  • Check deprecation warnings
  • Remove ugettext* variants (the u prefix can be dropped)
  • Bump the version of Django's intersphinx version to 4.2
  • CSRF_TRUSTED_ORIGINS needs to have the protocol
  • Update dark mode handling for TinyMCE? See here and here
  • Migrate from pytz to zoneinfo
@sergei-maertens sergei-maertens self-assigned this May 3, 2023
sergei-maertens added a commit that referenced this issue Jun 11, 2023
This version is Django 4.2 compatible
@ErhanCitil
Copy link
Contributor

Ik heb het samen nagelopen met @sergei-maertens en we zijn tot de conclusie gekomen dat de versie van django-autoslug compatible is met Django 4.2 en Python 3.10. Dus het is geschikt voor Open Forms.

sergei-maertens added a commit that referenced this issue Jun 30, 2023
This version supports Django 4.2. Thanks to @ErhanCitil for doing the
research.
@ErhanCitil
Copy link
Contributor

Django Axes: Deze package wordt gebruikt in Open Forms met de versie 5.31.0, ik heb gekeken naar de originele bron code en het ondersteunt Python 3.10 en Django 3.2 in de nieuwere versie. Deze versies worden ook gebruikt bij Open Forms.

Het enige wat er gedaan moet worden is het updaten van de package naar een nieuwere versie in de requirements, de laatste versie op dit moment is: 6.0.5.

sergei-maertens added a commit that referenced this issue Jul 4, 2023
django-yubin now uses celery for queuing out of the box, so we no longer
need to simulate cron to actually send out the e-mails.

See https://django-yubin.readthedocs.io/en/latest/install.html\#upgrading-from-previous-versions
which documents the upgrade steps.
sergei-maertens added a commit that referenced this issue Jul 5, 2023
django-yubin now uses celery for queuing out of the box, so we no longer
need to simulate cron to actually send out the e-mails.

See https://django-yubin.readthedocs.io/en/latest/install.html\#upgrading-from-previous-versions
which documents the upgrade steps.
sergei-maertens added a commit that referenced this issue Jul 5, 2023
django-yubin now uses celery for queuing out of the box, so we no longer
need to simulate cron to actually send out the e-mails.

See https://django-yubin.readthedocs.io/en/latest/install.html\#upgrading-from-previous-versions
which documents the upgrade steps.
@ErhanCitil
Copy link
Contributor

ErhanCitil commented Jul 5, 2023

Django-Camunda: Dit is een package van Maykin zelf, op het eerste gezicht zie ik dat de code wel werkt met Django 4.2. Alleen er moeten kleine aanpassingen gedaan worden en ik ga de test verbeteren zodat we zeker zijn dat het werkt met versie 4.2 van Django.

@ErhanCitil
Copy link
Contributor

Django Colorfield: De nieuwste versie van deze package dat is op dit moment: 0.9.0. Deze nieuwe versie heeft support voor DJango versie 3.2/4.2 en Python versie 3.10 wat gebruikt wordt in Open Forms. Dus het is een kwestie van updaten van de package naar versie 0.9.0.

Ik heb de tox bestand bekeken en de GitHub actions en de tests die slagen.

@ErhanCitil
Copy link
Contributor

Django-digid-eherkenning: Is al geupdate door Sergei en Chris. De laatste versie wordt gebruikt in Open Forms. In de tox test zie ik dat het werkt met Django 4.2 en in de GitHub Actions staat hij op groen.

Deze package kan goedgekeurd worden op de bovenstaande lijst.

@ErhanCitil
Copy link
Contributor

Django Hijack: In Open Forms wordt deze package gebruikt met de versie 3.1.6, de nieuwste versie dat is 3.4.1 deze ondersteunt wel Django versie 4.2. Dit kan je zien aan de releases en in de GitHub Actions zie je dat het vinkje groen is met de test voor Django versie 4.2. Het is ook compatible met Python versie 3.10.

Alleen de Django Hijack package moet geupdate worden naar 3.4.1 in de requirements en voor de rest niks anders.

@ErhanCitil
Copy link
Contributor

Django Redis: Zoals Sergei heeft vermeldt werkt de versie 5.3.0 met Django versie 4.2 en Python versie 3.10. Dit kunnen we terug zien in de CHANGELOG, en zien we ook aan de tests in de GitHub Actions.

Deze package moet geupdate worden in de requirements naar de versie 5.3.0 in Open Forms.

@ErhanCitil
Copy link
Contributor

Django Solo: In Open Forms wordt de versie 2.0.0 gebruikt, er is een nieuwere versie waarbij er support is voor Django 4.2 en Python 3.10 & 3.11. Dit kunnen we zien in CHANGELOG, en er is een tox test en in de GitHub actions wordt de tox test getest dus het bewijst dat Django versie 4.2 compatible is met het project.

Dus de package moet in de requirements geupdate worden van 2.0.0 naar 2.1.0.

@ErhanCitil
Copy link
Contributor

Django Treebeard: De versie 4.7 van deze package support Django versie 4.2, dit zien we terug in de CHANGE. In de readme wordt het ook beschreven dat er support is voor Django versie 4.2. Daarnaast zien we in de Tox test dat er getest wordt de versie 4.2 en in de GitHub Workflow zien we dat ook terug komen met de groene vinkje.

Dus de package django-treebeard moet in de requirements geupdate worden van 4.5.1 naar 4.7.

@ErhanCitil
Copy link
Contributor

Django Filter: Deze package heeft de versie 2.4.0 in Open Forms, de nieuwste versie dat compatible is met Django versie 4.2 is django-filter 23.2. Dit staat beschreven in de CHANGES, en in de Tox Tests.

Deze package moet dus geupdate worden van versie 2.4.0 naar 23.2 in de requirements, en het kan afgevinkt worden in de bovenstaande lijst.

@sergei-maertens
Copy link
Member Author

@ErhanCitil je mag gerust pull requests maken naar open forms om deze dependencies te updaten! Ik zorg even dat er een fork is onder maykinmedia

@ErhanCitil
Copy link
Contributor

@ErhanCitil je mag gerust pull requests maken naar open forms om deze dependencies te updaten! Ik zorg even dat er een fork is onder maykinmedia

Isgoed zal ik doen!

sergei-maertens added a commit that referenced this issue Feb 21, 2024
Logging out is now done through a POST request with a form, so the
link styles are no longer applied automatically.
sergei-maertens added a commit that referenced this issue Feb 21, 2024
The logout link is now a logout form, so response.form in webtest
crashes because multiple forms are on the page.
sergei-maertens added a commit that referenced this issue Feb 21, 2024
Due to auto/explicit modes now being a thing, the detection purely through
a media query is not sufficient. Instead, if an explicit theme is set,
that maps to the skin/content_css properties.

If the theme mode is set to 'auto', then the media query applies to
figure out whether dark or light theme needs to be applied.

The code is refactored to remove duplication and instead have a more
declarative approach for possible future themes.
sergei-maertens added a commit that referenced this issue Feb 21, 2024
… changes

tinyMCE does not handle this itself, you need to explicitly force it
to destroy and re-initialize itself.

Django has two mechanisms w/r to the theme:
* store the selected theme in localStorage (if unset, it's assumed to
  be 'auto')
* set the data-theme attribute on the root html node, which triggers
  the relevant CSS rules to change the appearance

So, whenever the theme changes, the localStorage is updated and the
data-attribute on the HTML node changes.

We can observe the html node changes, and use that to synchronize our
(derived) global state. We don't care about dark/light/auto, only
about the resulting theme of a particular choice - this result is
stored in the global state. Then, our pages with tinymce editors need
to subscribe to the global state changes so that they can re-initialize:

* react component, easy, just make sure the 'key' prop changes and
  react will unmount and remount the node, triggering the re-init in
  the process from @tinymce/react
* the django-tinymce code is trickier, since there we need to get each
  editor instance ourselves and subscribe to the global state to
  ensure we reset the editors. This further customizes the overridden
  code from the upstream package.
sergei-maertens added a commit that referenced this issue Feb 21, 2024
* Fixed missing (new) class name in react components to apply
  inline labels + fields
* Override django admin styling to not vertically stretch input fields
  for labels longer than 160px that wrap unto the next line
* Override checkbox styling to vertically center checkbox + label
  instead of the weird offsets we otherwise have.

It's probably wise to re-do the admin overrides implementation from
vanilla admin CSS again to see what is actually still needed, but
ideally we'd have visual regression testing set up for all involved
components.
sergei-maertens added a commit that referenced this issue Feb 21, 2024
The Django markup changed substantially so the existing CSS was no
longer usable. However, using the :has pseudo selector we can
re-instate the same appearance with some clever CSS grid/flexbox
usage.

Colors for the tooltip are now specified as CSS variables too, so
this patch should be easily backported to default-project. The
relevant styles are put in a separate SASS module to easier keep
track of which aspect of the django admin they touch.
sergei-maertens added a commit that referenced this issue Feb 21, 2024
Set to auto so that the browser preferences and storybooks default
dark/light mode tool (hopefully) work out of the box without requiring
an explicit theme switcher.
sergei-maertens pushed a commit that referenced this issue Feb 21, 2024
The admin markup has changed, so hijack needs to be pointed to the
correct HTML element to properly display.
sergei-maertens added a commit that referenced this issue Feb 21, 2024
In 4.2 it is no longer possible to query relations on instances
not in the DB https://github.com/django/django/blob/4.2/django/db/models/fields/related_descriptors.py#L718

In some places this points out a genuine defect in the test where
helpers run DB queries that could lead to a different outcome,
in other places (like audit logging) the audit logging is not
what is being tested and we can just disable it so we can benefit
from the SimpleTestCase and .build() speed advantages.

Co-authored-by: @sergei-maertens <sergei@maykinmedia.nl>
sergei-maertens added a commit that referenced this issue Feb 21, 2024
Django supports stdlib zoneinfo since 4.0.
sergei-maertens added a commit that referenced this issue Feb 21, 2024
The pytz -> zoneinfo update results in different behaviour how DST seems to be
treated. The new variant captures correctly the beginning and end DST/UTC offset,
and it keeps the hour for the end-user consistent, so in my mind it makes sense?
sergei-maertens added a commit that referenced this issue Feb 21, 2024
Added in Django 4 in the base templates, which we need to account for
in our overrides.
sergei-maertens added a commit that referenced this issue Feb 21, 2024
The theme toggle sets a theme attribute on the html node, so that the
user can configure this instead of configuring their entire browser
appearance.

In auto mode, this requires some special care to not accidentally
apply dark theme styles in some places (like form fields).
sergei-maertens added a commit that referenced this issue Feb 21, 2024
Logging out is now done through a POST request with a form, so the
link styles are no longer applied automatically.
sergei-maertens added a commit that referenced this issue Feb 21, 2024
The logout link is now a logout form, so response.form in webtest
crashes because multiple forms are on the page.
sergei-maertens added a commit that referenced this issue Feb 21, 2024
Due to auto/explicit modes now being a thing, the detection purely through
a media query is not sufficient. Instead, if an explicit theme is set,
that maps to the skin/content_css properties.

If the theme mode is set to 'auto', then the media query applies to
figure out whether dark or light theme needs to be applied.

The code is refactored to remove duplication and instead have a more
declarative approach for possible future themes.
sergei-maertens added a commit that referenced this issue Feb 21, 2024
… changes

tinyMCE does not handle this itself, you need to explicitly force it
to destroy and re-initialize itself.

Django has two mechanisms w/r to the theme:
* store the selected theme in localStorage (if unset, it's assumed to
  be 'auto')
* set the data-theme attribute on the root html node, which triggers
  the relevant CSS rules to change the appearance

So, whenever the theme changes, the localStorage is updated and the
data-attribute on the HTML node changes.

We can observe the html node changes, and use that to synchronize our
(derived) global state. We don't care about dark/light/auto, only
about the resulting theme of a particular choice - this result is
stored in the global state. Then, our pages with tinymce editors need
to subscribe to the global state changes so that they can re-initialize:

* react component, easy, just make sure the 'key' prop changes and
  react will unmount and remount the node, triggering the re-init in
  the process from @tinymce/react
* the django-tinymce code is trickier, since there we need to get each
  editor instance ourselves and subscribe to the global state to
  ensure we reset the editors. This further customizes the overridden
  code from the upstream package.
sergei-maertens added a commit that referenced this issue Feb 21, 2024
* Fixed missing (new) class name in react components to apply
  inline labels + fields
* Override django admin styling to not vertically stretch input fields
  for labels longer than 160px that wrap unto the next line
* Override checkbox styling to vertically center checkbox + label
  instead of the weird offsets we otherwise have.

It's probably wise to re-do the admin overrides implementation from
vanilla admin CSS again to see what is actually still needed, but
ideally we'd have visual regression testing set up for all involved
components.
sergei-maertens added a commit that referenced this issue Feb 21, 2024
The Django markup changed substantially so the existing CSS was no
longer usable. However, using the :has pseudo selector we can
re-instate the same appearance with some clever CSS grid/flexbox
usage.

Colors for the tooltip are now specified as CSS variables too, so
this patch should be easily backported to default-project. The
relevant styles are put in a separate SASS module to easier keep
track of which aspect of the django admin they touch.
sergei-maertens added a commit that referenced this issue Feb 21, 2024
@github-project-automation github-project-automation bot moved this from In Progress to Done in Development Feb 21, 2024
sergei-maertens added a commit that referenced this issue Feb 21, 2024
👽 [#3049] Ensure the data-theme attribute is set
@sergei-maertens sergei-maertens added this to the Release 2.6.0 milestone Feb 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
4 participants