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.

Sign up for GitHub

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

Jump to bottom

[#939] Upgrade to Django 4.2 #1024

Merged
merged 24 commits into from
Mar 1, 2024
Merged

[#939] Upgrade to Django 4.2 #1024

merged 24 commits into from
Mar 1, 2024

Conversation

pi-sigma
Copy link
Contributor

@pi-sigma pi-sigma commented Feb 13, 2024 •
edited
Loading

Closes #939

TODO:

@alextreme alextreme mentioned this pull request Feb 13, 2024
Closed
@pi-sigma pi-sigma mentioned this pull request Feb 14, 2024
Closed
@pi-sigma pi-sigma force-pushed the upgrade/django-4.2 branch 4 times, most recently from 393c89a to 6dcd451 Compare February 14, 2024 15:51
@pi-sigma
Copy link
Contributor Author

pi-sigma commented Feb 14, 2024 •
edited
Loading

  • I changed the logging in connection with digid user creation/change because the attempt to create a TimelineLog as part of the on_bsn_change function gave a ValueError: the related object (i.e. the user) is not saved. Perhaps we can revert to database logging by making the signal post_save instead of pre_save? Needs some thought/discussion.

Bartvaderkin
Bartvaderkin reviewed Feb 15, 2024
View reviewed changes
Copy link
Contributor

@Bartvaderkin Bartvaderkin left a comment

Choose a reason for hiding this comment

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

I added some notes.

Also I find the many migrations suspicious, I suppose this is a side-effect of the strings and newer Django code?

@codecov-commenter
Copy link

codecov-commenter commented Feb 16, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.95%. Comparing base (d11a962) to head (4dc32fd).

❗ Current head 4dc32fd differs from pull request most recent head 43c6395. Consider uploading reports for the commit 43c6395 to get more accurate results

Additional details and impacted files 
@@           Coverage Diff            @@
##           develop    #1024   +/-   ##
========================================
  Coverage    94.94%   94.95%           
========================================
  Files          883      889    +6     
  Lines        30950    30970   +20     
========================================
+ Hits         29386    29407   +21     
+ Misses        1564     1563    -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jiromaykin
jiromaykin reviewed Feb 19, 2024
View reviewed changes
requirements/dev.txt Outdated Show resolved Hide resolved
@pi-sigma pi-sigma force-pushed the upgrade/django-4.2 branch 6 times, most recently from 271d8af to b8a7ae3 Compare February 22, 2024 08:39
@pi-sigma
Copy link
Contributor Author

@Bartvaderkin

Also I find the many migrations suspicious, I suppose this is a side-effect of the strings and newer Django code?

A few of the migrations (auto-generated of cource) are cms-related changes to the related_name field. Not sure myself what that's about.

@pi-sigma pi-sigma marked this pull request as ready for review February 22, 2024 09:23
Bartvaderkin
Bartvaderkin suggested changes Feb 22, 2024
View reviewed changes
Copy link
Contributor

@Bartvaderkin Bartvaderkin left a comment

Choose a reason for hiding this comment

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

The Django update stuff looks good but there is the signals logging, I put some input to clarify.

src/open_inwoner/haalcentraal/signals.py Outdated Show resolved Hide resolved
stevenbal
stevenbal requested changes Feb 22, 2024
View reviewed changes
Copy link
Contributor

@stevenbal stevenbal left a comment

Choose a reason for hiding this comment

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

Just some minor remarks/questions

src/open_inwoner/questionnaire/tests/test_views.py Outdated
@@ -20,6 +22,15 @@
from .factories import QuestionnaireStepFactory, QuestionnaireStepFileFactory


class SessionMiddleware(DefaultSessionMiddleWare):
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is also defined in open_inwoner.cms.tests.cms_tools, we can move this to open_inwoner.utils.test maybe?

src/open_inwoner/accounts/migrations/0073_alter_user_user_contacts.py Show resolved Hide resolved
src/open_inwoner/accounts/views/inbox.py Show resolved Hide resolved
pi-sigma and others added 24 commits February 29, 2024 16:15
@pi-sigma
[#2093] Remove bottom border from menu
67e50cc
@pi-sigma
[#2076] Upgrade dependencies and fix tests
0078e1a 
    * upgrade mayin-2fa to 1.0.0
    * upgrade django-otp to 1.3.0
    * upgrade django-two-factor-auth to 1.16.0
@pi-sigma
[#939] Upgrade to Django 4.2
08f513a
@pi-sigma
[#939] Refactor function names
b0e489a
@pi-sigma
[#939] Upgrade django-elasticsearch
3a6b767
@pi-sigma
[#939] Upgrade django-view-breadcrumbs
46302b7
@pi-sigma
[#939] Remove ddt_api_calls from dev config
0397d0d
@pi-sigma
[#939] Upgrade django-solo to 2.2.0
63e6a02
@pi-sigma
[#939] Upgrade django-yubin to 2.0.3
7184368
@pi-sigma
[#939] Upgrade aldryn-apphooks-config to 0.7.0
5e18561
@pi-sigma
[#939] Upgrade django-appdata to 0.4.0
2d03930
@pi-sigma
[#939] Upgrade django-sniplates to 0.7.2
5badd00
@pi-sigma
[#939] Upgrade djangorestframework-camel-case to 1.4.2
5cbd800
@pi-sigma
[939] Upgrade django-timeline-logger to 3.0.0
db127b8
@pi-sigma
[#939] Fix tests
5017492 
    * refactor logging, move logic from DocumentDeleteView.delete() to
      form_valid()
    * normalize data structure and correct type annotation in `InboxView`
      (confusion between queryset and list)
    * refactor logging in the context of DigiD user change/creation
      since db logging caused `ValueError: save() prohibited to prevent
      data loss due to unsaved related object 'content_object'`
@pi-sigma
[#939] Fix migration conflict
e92044d
@pi-sigma
[#939] Update postgres/postgis in CI to 15-3.4
b50eac4
@pi-sigma
[#939] Upgrade djangocms_file + djangocms_picture
27e7194
@pi-sigma
[#939] Upgrade django-webtest
092eb26
@alextreme @pi-sigma
Pinning mail-editor to release
4b967de
@pi-sigma
[#939] Update mail-editor
7894ad6
@pi-sigma
[#939] Rebase and resolve conflicts
c4ee1dc
@pi-sigma
[#939] Process PR feedback
b4d4725
@pi-sigma
Fixed user save-signal issue with Haalcentraal BRP retrieval logging
4a9a460
@pi-sigma pi-sigma marked this pull request as ready for review February 29, 2024 15:51
Bartvaderkin
Bartvaderkin approved these changes Mar 1, 2024
View reviewed changes
Copy link
Contributor

@Bartvaderkin Bartvaderkin left a comment

Choose a reason for hiding this comment

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

👍 lets go! 🚀

@alextreme alextreme merged commit 53a9201 into develop Mar 1, 2024
17 checks passed
@alextreme alextreme deleted the upgrade/django-4.2 branch March 1, 2024 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Bartvaderkin Bartvaderkin Bartvaderkin approved these changes

@stevenbal stevenbal stevenbal approved these changes

@jiromaykin jiromaykin Awaiting requested review from jiromaykin

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Django 4.2 upgrade issues
6 participants
@pi-sigma @codecov-commenter @Bartvaderkin @stevenbal @jiromaykin @alextreme