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

feat: Django 4.2 support #262

Merged
merged 29 commits into from
Mar 16, 2024
Merged

feat: Django 4.2 support #262

merged 29 commits into from
Mar 16, 2024

Conversation

fsbraun
Copy link
Member

@fsbraun fsbraun commented Mar 12, 2024

Description

This PR updates djangocms-moderation to support Django 4.2 and 5.0. It is based on the ideas and work of #256 by @Aiky30 .

  • If fixes tests so they can run with django CMS 4.0 and django CMS 4.1

    • CMS 4.0: Tests run with djangocms-version-locking
    • CMS 4.1: Tests run with DJANGOCMS_VERSIONING_LOCK_VERSIONS setting set to True.
  • Since aldryn-forms does not support Django 4.2, the optional djangocms-moderation.contrib.moderation_forms app has been discontinued.

  • Currently, tests against django CMS 4.0 fail (against django CMS 4.1 everything is fine).
    @joshyu: This is not an effect of this PR. Tests failed before already. Do you know why? I would love to get them in line.

Related resources

Checklist

@fsbraun fsbraun requested a review from Aiky30 March 12, 2024 14:34
@Aiky30 Aiky30 mentioned this pull request Mar 12, 2024
Copy link
Collaborator

@Aiky30 Aiky30 left a comment

Choose a reason for hiding this comment

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

Changes look OK here, although the changes on djangocms-versioning regarding the button looks to be regression that may need to be fixed.

setup.py Show resolved Hide resolved
tests/requirements/requirements_base.txt Outdated Show resolved Hide resolved
self.assertNotIn(archive_url, archive_link)
if versioning_version < "2":
# Edit link is inactive as `mock_is_obj_review_locked` is True
self.assertIn("inactive", archive_link)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks to be a regression of functionality in djangocms-versioning 2 where the edit link is disabled vs what appears to have been removed.

@joshyu joshyu mentioned this pull request Mar 15, 2024
3 tasks
@joshyu
Copy link
Contributor

joshyu commented Mar 15, 2024

@fsbraun , I create a PR #263
it fixes all check errors and failed tests.

@joshyu
Copy link
Contributor

joshyu commented Mar 15, 2024

@fsbraun , about the failed pre-commit-config issue, It is related to the version of isort and other pre-commit plugins.

Freeman and I ever met this errors while fixing another repo.

refer to this:
https://stackoverflow.com/questions/75269700/pre-commit-fails-to-install-isort-5-11-4-with-error-runtimeerror-the-poetry-co

@joshyu
Copy link
Contributor

joshyu commented Mar 15, 2024

@fsbraun ,

about the failed tests, it is because that djangocms-version-locking addon create a monkeypatch to override djangocms-moderation.models.ModerationCollection._add_nested_children method, it add a logic to check whether this version is locked. If locked, current children plugins will not be added to the current collection.

I didn't find when versionlock field in version instance is set, so I hard-code the version check and give the different return value.

@fsbraun
Copy link
Member Author

fsbraun commented Mar 15, 2024

@joshyu Thanks for the explanation. To me, this is very helpful: So that actually is a desired behavior. I also understand FidelityInternational/djangocms-version-locking#80 now (an issue @marksweb raised a while ago)... Let me look into this, maybe we can identical results for CMS 4.0 and CMS 4.1 😃

@joshyu
Copy link
Contributor

joshyu commented Mar 15, 2024

@fsbraun,
need your help to fix the left pre-commit error about 0001_squashed_0017_auto_20220831_0727.py file

@fsbraun
Copy link
Member Author

fsbraun commented Mar 15, 2024

@joshyu Pre-commit now passes. I'd like to look into the version locking effect on the collections with djangocms-versioning 2. Probably need today. Sufficient for you?

@fsbraun
Copy link
Member Author

fsbraun commented Mar 15, 2024

@joshyu I've updated the tests to test the same outcomes for djangocms-versioning 1 and 2. For most tests that means, removing the version lock before the test. I've also added a test that ensures that locked versions are not added to collections. If you're ok, I can merge.

@joshyu
Copy link
Contributor

joshyu commented Mar 16, 2024

@fsbraun ,
LGTM. Thank you for giving such a perfect solution, this is really better than my update on test_view.py.
please help to merge it.

Thanks.

@fsbraun fsbraun merged commit fcd5a0a into master Mar 16, 2024
29 checks passed
@marksweb marksweb deleted the feat/django-4-2-supprt branch March 16, 2024 15:30
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.

4 participants