Skip to content

Conversation

@svenseeberg
Copy link
Member

@svenseeberg svenseeberg commented Aug 12, 2025

Short description

In general it seems to work quite well. Some problems occur.

Proposed changes

  • Upgrade to Django 5.2
  • Add Python 3.13 support
  • We have to switch to a HTTP POST method for logging out.

Side effects

  • Creating new pages breaks (ValueError: Model instances passed to related filters must be saved., see conversation
  • Logout breaks: https://chat.tuerantuer.org/digitalfabrik/pl/sniyhe9ei7y47p83a96tt8u5fc
  • ValueError in terminal because Page does not exist (happens for example when creating a new page)
  • Creating or updating pages button isn't there anymore for roles: CMS/Service team, management or editors -> Should be fixed with rebase!

Faithfulness to issue description and design

There are no intended deviations from the issue and design.

Resolved issues

Fixes: #
Blocks: #3826


Pull Request Review Guidelines

@PeterNerlich PeterNerlich added the dependencies Pull requests that update a dependency file label Aug 12, 2025
@svenseeberg svenseeberg force-pushed the feature/python3.13-django-5.2 branch 6 times, most recently from 1abb293 to 7b35ed6 Compare August 13, 2025 12:33
@JoeyStk JoeyStk added this to the Next-up milestone Aug 18, 2025
@hannaseithe hannaseithe added the blocker This issue blocks another issue label Sep 8, 2025
@JoeyStk JoeyStk requested review from JoeyStk and removed request for hannaseithe November 16, 2025 14:01
@JoeyStk JoeyStk assigned JoeyStk and unassigned hannaseithe Nov 16, 2025
@JoeyStk
Copy link
Contributor

JoeyStk commented Nov 16, 2025

@svenseeberg Could you please rebase this PR to a more recent version? That would be great! 😊

@JoeyStk
Copy link
Contributor

JoeyStk commented Nov 16, 2025

Creating new pages breaks: https://chat.tuerantuer.org/digitalfabrik/pl/1wqgnpwonpdhzbywei4dw1r4xe

I'm working on a fix for it 👍
I noticed one more thing: I logged in as root and when I wanted to update a page, I didn't had the option to update the page anymore, but instead only the option to "submit for review" - which is a function available to authors. I assume this could be related to the mentioned issue for new pages, but it also might be a different issue

@JoeyStk
Copy link
Contributor

JoeyStk commented Nov 16, 2025

@MizukiTemma Could you please test this branch in the next week and just check if this PR breaks any functionality. I already did it myself, but I think we need another set of eyes here :) Since I mostly tested as root, it would be great if you could test other user roles :)

EDIT: Could you also please do some more exhaustive testing for mirroring regions and pages? That would be awesome!

EDIT2: You can do a full review now :)

@PeterNerlich PeterNerlich force-pushed the feature/python3.13-django-5.2 branch from 7190826 to b9f2fd8 Compare November 16, 2025 15:43
@JoeyStk
Copy link
Contributor

JoeyStk commented Nov 16, 2025

ValueError in terminal because Page does not exist (happens for example when creating a new page)

Traceback (most recent call last):
  File "/home/johannes/integreat-cms/integreat_cms/cms/models/abstract_base_model.py", line 50, in __repr__
    return self.get_repr()
           ^^^^^^^^^^^^^^^
  File "/home/johannes/integreat-cms/integreat_cms/cms/models/pages/page.py", line 401, in get_repr
    f", slug: {self.best_translation.slug}" if self.best_translation else ""
                                               ^^^^^^^^^^^^^^^^^^^^^
  File "/home/johannes/integreat-cms/.venv/lib/python3.12/site-packages/django/utils/functional.py", line 47, in __get__
    res = instance.__dict__[self.name] = self.func(instance)
                                         ^^^^^^^^^^^^^^^^^^^
  File "/home/johannes/integreat-cms/integreat_cms/cms/models/abstract_content_model.py", line 490, in best_translation
    or self.translations.first()
       ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/johannes/integreat-cms/.venv/lib/python3.12/site-packages/django/db/models/manager.py", line 87, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
                   ^^^^^^^^^^^^^^^^^^^
  File "/home/johannes/integreat-cms/.venv/lib/python3.12/site-packages/django/db/models/fields/related_descriptors.py", line 757, in get_queryset
    raise ValueError(
ValueError: 'Page' instance needs to have a primary key value before this relationship can be used.

@svenseeberg svenseeberg marked this pull request as ready for review November 16, 2025 16:07
@JoeyStk JoeyStk requested a review from MizukiTemma November 16, 2025 16:08
@JoeyStk
Copy link
Contributor

JoeyStk commented Nov 16, 2025

TODOS

  • Creating new POI fails
    DetailsCreate a new POI, after clicking Save -> ValueError at /augsburg/pois/de/new/ Model instances passed to related filters must be saved.
  • Creating new event fails
  • Creating new page fails
  • Fix failing tests (seems to be related to the ValueError)
  • Update Treebeard

@MizukiTemma
Copy link
Member

TODOS

  • Creating new POI fails Create a new POI, after clicking Save -> ValueError at /augsburg/pois/de/new/ Model instances passed to related filters must be saved.

Same with a new page and event 😿

@JoeyStk
Copy link
Contributor

JoeyStk commented Nov 17, 2025

Thank you for the review

@JoeyStk
Copy link
Contributor

JoeyStk commented Dec 12, 2025

We have somewhat of a deadline for this: support for 4.2LTE ends in April of 2026 :/

@JoeyStk JoeyStk force-pushed the feature/python3.13-django-5.2 branch 2 times, most recently from ba31969 to eae92da Compare December 15, 2025 10:28
@JoeyStk
Copy link
Contributor

JoeyStk commented Dec 15, 2025

We need to update treebeard as well, because the version that we use (4.7.1) doesn't support Django5.2.

@JoeyStk
Copy link
Contributor

JoeyStk commented Dec 15, 2025

Intermediate result: We're down to 32 failing tests (which is another -30) 🎉
We are also aware of one downside of the new implementation: We're now required to save the content_form before the content_translation_form can be validated. This means that when the content_translation_form is not valid, we also need to find a way to roll back the save of content_form. The entire post method is wrapped inside a transaction.atomic block via Tree Mutex, but this does not roll back, because post doesn't raise an exception. Instead it renders the form to the user again with any errors.

Next TODOS:

  • Fix remaining 32 failing tests
  • Fix roll back for page/poi and event form

Remaining 32 failing tests:

  • Fix event_form has to be saved before RecurrenceRule (-8)
  • Fix test_create_event_location_check
  • Fix filter_unused
  • Fix amount of queries

@JoeyStk JoeyStk removed the help wanted Extra attention is needed label Dec 15, 2025
@JoeyStk JoeyStk force-pushed the feature/python3.13-django-5.2 branch from 714c132 to b5aa921 Compare December 15, 2025 21:50
@JoeyStk JoeyStk force-pushed the feature/python3.13-django-5.2 branch from b5aa921 to 5852aa3 Compare December 15, 2025 22:09
@JoeyStk
Copy link
Contributor

JoeyStk commented Dec 21, 2025

Current state of this: waiting for feedback from @MizukiTemma and @svenseeberg

@JoeyStk JoeyStk mentioned this pull request Dec 21, 2025
Copy link
Member Author

@svenseeberg svenseeberg left a comment

Choose a reason for hiding this comment

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

Thanks for your work @PeterNerlich and @JoeyStk 🚀. This seems to work well 🥳. I did not test all existing CMS functionality but clicked through most of the forms, saved objects, activated new users, etc and everything worked. I think this change is ready to be tested on the test system.

On the test system we should make sure that especially test filled out page/poi/event forms but missing translation data to see what happens. AFAICT the JS catches missing translations and in practice this should never become an issue.

Last but not least we would not have needed all those if not self.pk: checks as we now create the related objects first. But they do not harm either so I would keep that for sanity reasons.

Consider this an approve ✔️

*edit: the commits should be squashed though ;)

Copy link
Contributor

@JoeyStk JoeyStk left a comment

Choose a reason for hiding this comment

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

(Approve by Sven)

Copy link
Member

@MizukiTemma MizukiTemma left a comment

Choose a reason for hiding this comment

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

Works as beofre so far as I tested 😸

Thank you so much 👍

@JoeyStk JoeyStk removed the request for review from PeterNerlich December 22, 2025 16:26
@JoeyStk JoeyStk removed their assignment Jan 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

blocker This issue blocks another issue dependencies Pull requests that update a dependency file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants