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

[7782] django upgrade #910

Merged
merged 7 commits into from
Nov 2, 2023
Merged

[7782] django upgrade #910

merged 7 commits into from
Nov 2, 2023

Conversation

m4ra
Copy link
Contributor

@m4ra m4ra commented Oct 31, 2023 •
edited
Loading

Some issues:

django 4.0 set_language doesn't set the user language in request.session (key _language)
django-cps package has not been updated since 2yrs ago and gives deprecation warning:

csp/utils.py:105: DeprecationWarning: child-src is deprecated in CSP v3. Use frame-src and worker-src

django-multiselectfield also appears unsupported/dead and gives error with django > 4.0. Fixed by adding max_length in respected models that use MultiSelectField

@m4ra m4ra changed the title WIP django upgrade [7786] WIP django upgrade Oct 31, 2023
@m4ra m4ra changed the title [7786] WIP django upgrade [7782] WIP django upgrade Oct 31, 2023
@m4ra m4ra force-pushed the mk-2023-10-django-upgrade branch from a1da0bb to fab08a8 Compare October 31, 2023 13:00
@goapunk
Copy link
Contributor

goapunk commented Oct 31, 2023

sounds like we maybe should look into getting rid of django-multiselectfield :/

@m4ra m4ra force-pushed the mk-2023-10-django-upgrade branch from fab08a8 to 7976808 Compare October 31, 2023 14:17
@m4ra
Copy link
Contributor Author

m4ra commented Oct 31, 2023

sounds like we maybe should look into getting rid of django-multiselectfield :/

@goapunk it's fixed by adding max_length.
Bcz if we get rid of this one, we shall also get rid of django-cps.. also not changed since 2yrs ago

@m4ra m4ra changed the title [7782] WIP django upgrade [7782] django upgrade Oct 31, 2023
@m4ra m4ra requested review from goapunk, philli-m and hom3mad3 October 31, 2023 14:32
goapunk
goapunk reviewed Oct 31, 2023
View reviewed changes
changelog/7782.md Outdated Show resolved Hide resolved
goapunk
goapunk reviewed Oct 31, 2023
View reviewed changes
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, just some questions

@goapunk
Copy link
Contributor

goapunk commented Oct 31, 2023 •
edited
Loading

some notes:

@m4ra m4ra requested a review from goapunk November 1, 2023 11:06
philli-m
philli-m approved these changes Nov 1, 2023
View reviewed changes
Copy link
Contributor

@philli-m philli-m left a comment

Choose a reason for hiding this comment

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

Nice! quick click around and multi select field adding is all working smooth, couple questions but I think it's good to go, as we wait anyway for @goapunk PR will leave merge to him

digitalstrategie/templates/includes/upper_footer.html Outdated Show resolved Hide resolved
apps/measures/models.py
blank=True,
max_choices=3,
choices=INCLUSIVE_SHAPING_OF_URBAN_LIFE,
)

facilitative_administration = MultiSelectField(
max_length=120,
Copy link
Contributor

Choose a reason for hiding this comment

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

nice that we can keep it for a little longer :)

@m4ra m4ra force-pushed the mk-2023-10-django-upgrade branch 3 times, most recently from f370b4a to 4388a5c Compare November 1, 2023 17:27
@m4ra m4ra force-pushed the mk-2023-10-django-upgrade branch from 4388a5c to eb22b2b Compare November 2, 2023 12:59
m4ra
m4ra commented Nov 2, 2023
View reviewed changes
changelog/7782.md Outdated Show resolved Hide resolved
@m4ra m4ra requested a review from philli-m November 2, 2023 13:02
@m4ra
Copy link
Contributor Author

m4ra commented Nov 2, 2023 •
edited
Loading

some notes:
allowed_hosts needs update in admin once merged, see https://docs.djangoproject.com/en/4.2/releases/4.0/#csrf-trusted-origins-changes

@goapunk so we cannot merge yet bcz admin repo salt update messed it up?

@goapunk
Copy link
Contributor

goapunk commented Nov 2, 2023

some notes:
allowed_hosts needs update in admin once merged, see https://docs.djangoproject.com/en/4.2/releases/4.0/#csrf-trusted-origins-changes

@goapunk so we cannot merge yet bcz admin repo salt update messed it up?

no, should be ok to merge, these kind of changes still work

@m4ra m4ra force-pushed the mk-2023-10-django-upgrade branch from eb22b2b to a5876d3 Compare November 2, 2023 13:19
goapunk
goapunk requested changes Nov 2, 2023
View reviewed changes
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 mostly good to me, just some questions and small notes

requirements/dev.txt Outdated
@@ -5,5 +5,5 @@ isort==5.12.0
pytest-cov==4.1.0
pytest-django==4.6.0
pytest==7.4.3
psycopg2-binary==2.9.9
psycopg-binary==3.1.12
Copy link
Contributor

Choose a reason for hiding this comment

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

need psycopg[binary]== here

requirements/prod.txt Outdated
@@ -1,2 +1,2 @@
-r base.txt
psycopg2==2.9.9
psycopg==3.1.12
Copy link
Contributor

Choose a reason for hiding this comment

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

should now be psycopg[c]==

Copy link
Contributor Author

Choose a reason for hiding this comment

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

[c]?

Copy link
Contributor

Choose a reason for hiding this comment

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

see https://www.psycopg.org/psycopg3/docs/basic/install.html#local-installation

requirements/base.txt Outdated Show resolved Hide resolved
requirements/base.txt Outdated Show resolved Hide resolved
@goapunk goapunk self-requested a review November 2, 2023 14:07
goapunk
goapunk approved these changes Nov 2, 2023
View reviewed changes
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

@goapunk goapunk merged commit 5e79d9f into main Nov 2, 2023
2 checks passed
@goapunk goapunk deleted the mk-2023-10-django-upgrade branch November 2, 2023 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@goapunk goapunk goapunk approved these changes

@hom3mad3 hom3mad3 Awaiting requested review from hom3mad3

@philli-m philli-m Awaiting requested review from philli-m

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

Successfully merging this pull request may close these issues.
3 participants
@m4ra @goapunk @philli-m