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

[7777] django upgrade from 3.2.x to 4.2 #5309

Merged
merged 7 commits into from
Dec 5, 2023
Merged

Conversation

m4ra
Copy link
Contributor

@m4ra m4ra commented Nov 9, 2023

Tasks

  • PR name contains story or task reference
  • Documentation (docs and inline)
  • Tests (including n+1 and django_assert_num_queries where applicable)
  • Changelog

@m4ra m4ra changed the title django upgrade from 3.2.x to 4.0 [7777] django upgrade from 3.2.x to 4.0 Nov 9, 2023
@m4ra m4ra changed the title [7777] django upgrade from 3.2.x to 4.0 WIP [7777] django upgrade from 3.2.x to 4.0 Nov 9, 2023
@m4ra m4ra requested review from goapunk, philli-m and hom3mad3 November 9, 2023 15:59
@m4ra m4ra force-pushed the mk-2023-11-django-upgrade-4.2 branch from 8e03616 to 647edfd Compare November 9, 2023 16:16
Copy link
Contributor

@goapunk goapunk left a comment

Choose a reason for hiding this comment

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

LGTM so far, just a small note about the form_valid change

@m4ra m4ra force-pushed the mk-2023-11-django-upgrade-4.2 branch from 647edfd to ccbb047 Compare November 20, 2023 16:26
@m4ra m4ra force-pushed the mk-2023-11-django-upgrade-4.2 branch 3 times, most recently from 20a17a1 to c37da86 Compare November 20, 2023 17:22
@m4ra m4ra changed the title WIP [7777] django upgrade from 3.2.x to 4.0 [7777] django upgrade from 3.2.x to 4.2 Nov 20, 2023
Copy link
Contributor

@goapunk goapunk left a comment

Choose a reason for hiding this comment

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

small note about psycopg

also: do we need to fix the save() method of the models as described here: https://docs.djangoproject.com/en/4.2/releases/4.2/#setting-update-fields-in-model-save-may-now-be-required ?

changelog/7777.md Show resolved Hide resolved
@m4ra m4ra force-pushed the mk-2023-11-django-upgrade-4.2 branch from c37da86 to 641e87f Compare November 21, 2023 12:41
@goapunk
Copy link
Contributor

goapunk commented Nov 21, 2023

some notes:

  • the save methods in the vote models haven't been updated, is that on purpose?
  • the tests for the servicekonto are now failing. I assume because of the allauth update. We'll have to ask @CarolingerSeilchenspringer about whether we need the servicekonto or nor. Fixing it might be annoying

@m4ra
Copy link
Contributor Author

m4ra commented Nov 21, 2023

some notes:

    the save methods in the vote models haven't been updated, is that on purpose?
    the tests for the servicekonto are now failing. I assume because of the allauth update. We'll have to ask @CarolingerSeilchenspringer about whether we need the servicekonto or nor. Fixing it might be annoying

The votes save() are not called for updating, it's to generate tokens. So they don't need the update_fields.
The servicekonto, took me a while but it's fixed now. I will push soon my changes.
@goapunk

@m4ra
Copy link
Contributor Author

m4ra commented Nov 21, 2023

@goapunk now there is an error with duplicate email constrains, from psycopg for the test that checks signup with existing email. I could inherit from allauth SignUp view/form and handle the exception. But I also tried to bump allauth to latest version, and servicekonto gave new errors. So yes good to ask Caroline, and if the servicekonto is still in use, we need to make it work with latest allauth.

@CarolingerSeilchenspringer
Copy link
Contributor

@goapunk @m4ra talked to client and the service konto in its current form is not used. It may come back but maybe in a whole different technical setting so we can take it out for now.

@m4ra m4ra force-pushed the mk-2023-11-django-upgrade-4.2 branch 3 times, most recently from 9cc4cac to 0e46030 Compare November 23, 2023 12:19
@goapunk
Copy link
Contributor

goapunk commented Nov 27, 2023

@m4ra there is one instance of index_together which should be replaced with indexes as described here: https://docs.djangoproject.com/en/4.2/releases/4.2/#index-together-option-is-deprecated-in-favor-of-indexes

@m4ra m4ra force-pushed the mk-2023-11-django-upgrade-4.2 branch 4 times, most recently from 0528c84 to 4f4f55b Compare November 27, 2023 14:54
@m4ra m4ra requested a review from goapunk November 27, 2023 14:55
@m4ra m4ra force-pushed the mk-2023-11-django-upgrade-4.2 branch from 876ff28 to 13e2d79 Compare November 29, 2023 18:19
@@ -62,6 +63,7 @@ class ProjectSerializer(serializers.ModelSerializer, CommonFields):
title = serializers.SerializerMethodField()
type = serializers.SerializerMethodField()
url = serializers.SerializerMethodField()
topics = serializers.SerializerMethodField()
Copy link
Contributor

Choose a reason for hiding this comment

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

This was all just hacked together to test, I think having aTopicsSerializer which we add here instead of the function would probably be nice ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true

meinberlin/apps/plans/models.py Outdated Show resolved Hide resolved
@m4ra m4ra force-pushed the mk-2023-11-django-upgrade-4.2 branch 2 times, most recently from 34467c5 to e27d5b7 Compare December 4, 2023 12:41
@m4ra m4ra requested a review from goapunk December 4, 2023 12:42
@@ -58,6 +82,7 @@ def save(self, commit=True):
plan.group = group
if commit:
plan.save()
self._save_m2m()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@goapunk I am changing self to plan for better readability

@m4ra m4ra force-pushed the mk-2023-11-django-upgrade-4.2 branch from e27d5b7 to b5d8284 Compare December 5, 2023 12:25
@m4ra m4ra force-pushed the mk-2023-11-django-upgrade-4.2 branch from b5d8284 to 344ca63 Compare December 5, 2023 13:15
Copy link
Contributor

@goapunk goapunk left a comment

Choose a reason for hiding this comment

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

looks good to me!

@goapunk goapunk enabled auto-merge (rebase) December 5, 2023 13:17
@goapunk goapunk merged commit 7920f19 into main Dec 5, 2023
2 checks passed
@goapunk goapunk deleted the mk-2023-11-django-upgrade-4.2 branch December 5, 2023 13:23
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.

3 participants