fix: Slug uniqueness not checked when moving page #8185

amandasavluchinske · 2025-03-23T16:58:09Z

Description

Closes #6840

Implements slug checking when a page is moved so that there are never two pages at the same level with the same slug.

Checklist

I have opened this pull request against main
I have added or modified the tests when changing logic
I have followed the conventional commits guidelines to add meaningful information into the changelog
I have read the contribution guidelines and I have joined the channel #pr-reviews on our Discord Server to find a “pr review buddy” who is going to review my pull request.

Summary by Sourcery

Bug Fixes:

Ensure slug uniqueness when moving pages to prevent duplicate slugs at the same level.

sourcery-ai · 2025-03-23T16:58:13Z

Reviewer's Guide by Sourcery

This pull request fixes a bug where slug uniqueness was not checked when moving pages in the Django CMS admin interface. It implements slug validation in the MovePageForm to prevent creating pages with duplicate slugs at the same level. The pull request also includes refactoring of the MovePageForm and new tests to ensure the validation works correctly.

No diagrams generated as the changes look simple and do not need a visual representation.

File-Level Changes

Change	Details	Files
Implemented slug uniqueness validation when moving pages in the admin interface.	Added validation in the `MovePageForm` to prevent creating pages with duplicate slugs at the same level. The validation checks for slug collisions when moving a page to a new parent or as a sibling. The form now raises a validation error if moving a page would result in a duplicate slug. Added tests for the `MovePageForm` to verify the slug uniqueness validation and other form behaviors.	`cms/admin/pageadmin.py` `cms/tests/test_forms.py` `cms/admin/forms.py`
Refactored the `MovePageForm` to improve readability and maintainability.	Extracted the logic for determining the target page and position into a separate `get_tree_options` method. Improved the error message when moving the homepage inside another page. Added tests for different position options in `MovePageForm`.	`cms/tests/test_forms.py` `cms/admin/forms.py`

Possibly linked issues

Slug uniqueness not checked when moving page #6840: The PR fixes the issue by implementing slug checking when moving a page.

Tips and commands

Interacting with Sourcery

Trigger a new review: Comment @sourcery-ai review on the pull request.
Continue discussions: Reply directly to Sourcery's review comments.
Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it. You can also reply to a
review comment with @sourcery-ai issue to create an issue from it.
Generate a pull request title: Write @sourcery-ai anywhere in the pull
request title to generate a title at any time. You can also comment
@sourcery-ai title on the pull request to (re-)generate the title at any time.
Generate a pull request summary: Write @sourcery-ai summary anywhere in
the pull request body to generate a PR summary at any time exactly where you
want it. You can also comment @sourcery-ai summary on the pull request to
(re-)generate the summary at any time.
Generate reviewer's guide: Comment @sourcery-ai guide on the pull
request to (re-)generate the reviewer's guide at any time.
Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
pull request to resolve all Sourcery comments. Useful if you've already
addressed all the comments and don't want to see them anymore.
Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
request to dismiss all existing Sourcery reviews. Especially useful if you
want to start fresh with a new review - don't forget to comment
@sourcery-ai review to trigger a new review!
Generate a plan of action for an issue: Comment @sourcery-ai plan on
an issue to generate a plan of action for it.

Customizing Your Experience

Access your dashboard to:

Enable or disable review features such as the Sourcery-generated pull request
summary, the reviewer's guide, and others.
Change the review language.
Add, remove or edit custom review instructions.
Adjust other review settings.

Getting Help

Contact our support team for questions or feedback.
Visit our documentation for detailed guides and information.
Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

cms/admin/pageadmin.py

sourcery-ai

Hey @amandasavluchinske - I've reviewed your changes - here's some feedback:

Overall Comments:

Consider adding a test case that specifically checks the scenario where moving a page would result in a slug collision.
The MovePageForm's clean method could be simplified by extracting the slug uniqueness check into a separate, reusable function.

Here's what I looked at during the review

🟢 General issues: all looks good
🟢 Security: all looks good
🟡 Testing: 1 issue found
🟡 Complexity: 1 issue found
🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

cms/tests/test_forms.py

cms/admin/forms.py

cms/tests/test_forms.py

fsbraun · 2025-03-25T13:32:34Z

@amandasavluchinske What do you think about the edge case moving a page next to itself? Can (or have) we cover this?

Also, what do you think about sourcery's idea of factoring out the uniqueness check?

amandasavluchinske · 2025-03-26T09:47:03Z

Thanks for the suggestions @fsbraun ! The changes have been pushed :)

jrief · 2025-03-27T16:07:08Z

In our private version of django-CMS we actually use these constraints to prevent inconsistencies while moving pages, etc.:

https://github.com/jrief/django-cms/blob/a54ec3d02d80064b64e6d22da45512ad13211f5d/cms/models/pagemodel.py#L1092

…l.py's PageUrl model

…hat unique constraints are respected

…sts so that unique constraints are respected" This reverts commit 3c5ac5d.

…pagemodel.py's PageUrl model" This reverts commit f1ead07.

…l.py's PageUrl model

…ent sites

fsbraun

@amandasavluchinske I have not been able to find out what makes the tests fail on mysql. I'm still on it. In the meantime I have two questions.

cms/models/pagemodel.py

amandasavluchinske · 2025-04-15T14:20:01Z

I have not been able to find out what makes the tests fail on mysql.

Thanks @fsbraun , I'll reply to your comments as soon as I can, but in the meantime: I looked the error up, and it seems like it's related to the condition= in line 1117 of pagemodel.py, which is not supported by MySQL.

fsbraun · 2025-04-15T14:21:43Z

That seems reasonable. But I still try to wrap my head around why we need it.

amandasavluchinske · 2025-04-15T14:22:45Z

That seems reasonable. But I still try to wrap my head around why we need it.

Yes, I'll review everything once I get some time.

.github/workflows/test.yml

amandasavluchinske · 2025-04-16T09:52:36Z

Hi @fsbraun ! I performed the requested changes. The condition wasn't needed after all. I tested manually and everything seems to be working as expected, but it would be good to have another pair of eyes on it as well. Thanks!

fsbraun

@amandasavluchinske I feel this is really getting somewhere! Sorry for the bickering, but I have two additional comments (see below) which could save us from having to duplicate the site field and might improve DRY. What do you think?

fsbraun · 2025-04-17T16:12:07Z

cms/api.py

+        # 3. The matching path is not the child of our page
+        if existing_url_collision.exists():
+            # For parent pages, the collision is not a problem if it's their child
+            if page.parent_id is not None:


I am wondering if we cen reduce the amount of repeated code if we use the validator here?

fsbraun · 2025-04-17T16:14:30Z

cms/models/pagemodel.py

+
+            for lang in languages_to_check:
+                # Check if the new path exists for this language on another page on the same site
+                existing_url_collision = (


Again I am wondering if the check can be done in the validator to improve DRY?

cms/models/pagemodel.py

fsbraun

Great work! Thank you, @amandasavluchinske !

fix: Slug uniqueness not checked when moving page

06ec243

amandasavluchinske commented Mar 23, 2025

View reviewed changes

cms/admin/pageadmin.py Show resolved Hide resolved

sourcery-ai bot reviewed Mar 23, 2025

View reviewed changes

cms/tests/test_forms.py Show resolved Hide resolved

cms/admin/forms.py Show resolved Hide resolved

amandasavluchinske commented Mar 23, 2025

View reviewed changes

cms/tests/test_forms.py Show resolved Hide resolved

Merge branch 'develop-4' into fix-slug-uniqueness-when-moving-pages

eebd73f

fsbraun and others added 4 commits March 25, 2025 16:39

Merge branch 'develop-4' into fix-slug-uniqueness-when-moving-pages

1b2a6d4

Merge branch 'develop-4' into fix-slug-uniqueness-when-moving-pages

186fe49

test: adds test for moving pages next to themselves

0526e41

refactor: simplifies MovePageForm clean function

b2e7cc3

amandasavluchinske force-pushed the fix-slug-uniqueness-when-moving-pages branch from 7198e2d to b2e7cc3 Compare March 26, 2025 09:46

Merge branch 'develop-4' into fix-slug-uniqueness-when-moving-pages

3ce4ad4

amandasavluchinske and others added 10 commits March 28, 2025 11:55

Merge branch 'develop-4' into fix-slug-uniqueness-when-moving-pages

f63ad62

Merge branch 'develop-4' into fix-slug-uniqueness-when-moving-pages

d92559f

Merge branch 'develop-4' into fix-slug-uniqueness-when-moving-pages

19a35b9

refactor: use UniqueConstraint instead of unique_together in pagemode…

f1ead07

…l.py's PageUrl model

refactor: modifies BaseCMSTestCase to cleanup data between tests so t…

3c5ac5d

…hat unique constraints are respected

Revert "refactor: modifies BaseCMSTestCase to cleanup data between te…

3734e65

…sts so that unique constraints are respected" This reverts commit 3c5ac5d.

Revert "refactor: use UniqueConstraint instead of unique_together in …

66cee72

…pagemodel.py's PageUrl model" This reverts commit f1ead07.

refactor: use UniqueConstraint instead of unique_together in pagemode…

018be90

…l.py's PageUrl model

refactor: allows identical paths + languages if they belong to differ…

bb03038

…ent sites

refactor: supports pages in draft state and prioritizes published

16824f6

amandasavluchinske force-pushed the fix-slug-uniqueness-when-moving-pages branch from ed4fa02 to 16824f6 Compare April 13, 2025 10:56

fsbraun added 3 commits April 15, 2025 15:28

Update test.yml

c17711f

Use MySql 8.4 LTS

46a0e38

Update test.yml

fe8e6a4

fsbraun reviewed Apr 15, 2025

View reviewed changes

cms/models/pagemodel.py Outdated Show resolved Hide resolved

cms/models/pagemodel.py Outdated Show resolved Hide resolved

marksweb reviewed Apr 15, 2025

View reviewed changes

.github/workflows/test.yml Outdated Show resolved Hide resolved

amandasavluchinske added 2 commits April 16, 2025 10:41

refactor: removes unnecessary condition from pagemodel

efb5bb7

cleanup: removes comment about supported python versions on test.yml

1c78841

amandasavluchinske requested a review from fsbraun April 16, 2025 15:25

Merge branch 'develop-4' into fix-slug-uniqueness-when-moving-pages

adb70f1

fsbraun reviewed Apr 17, 2025

View reviewed changes

fsbraun and others added 4 commits April 26, 2025 13:43

Remove the site field for PageUrl and add validator to cms.api

2353f29

Merge branch 'develop-4' into fix-slug-uniqueness-when-moving-pages

0327d17

fix: Update migration (no need for deduplication)

c542cab

Merge branch 'develop-4' into fix-slug-uniqueness-when-moving-pages

efc890c

fsbraun approved these changes Apr 28, 2025

View reviewed changes

fsbraun merged commit 8d50660 into django-cms:develop-4 Apr 28, 2025
63 checks passed

Uh oh!

fix: Slug uniqueness not checked when moving page #8185

fix: Slug uniqueness not checked when moving page #8185

Conversation

amandasavluchinske commented Mar 23, 2025 • edited by sourcery-ai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Summary by Sourcery

Uh oh!

sourcery-ai bot commented Mar 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer's Guide by Sourcery

File-Level Changes

Possibly linked issues

Interacting with Sourcery

Customizing Your Experience

Getting Help

Uh oh!

Uh oh!

sourcery-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fsbraun commented Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amandasavluchinske commented Mar 26, 2025

Uh oh!

jrief commented Mar 27, 2025

Uh oh!

fsbraun left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

amandasavluchinske commented Apr 15, 2025

Uh oh!

fsbraun commented Apr 15, 2025

Uh oh!

amandasavluchinske commented Apr 15, 2025

Uh oh!

Uh oh!

amandasavluchinske commented Apr 16, 2025

Uh oh!

fsbraun left a comment

Choose a reason for hiding this comment

Uh oh!

fsbraun Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

fsbraun Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fsbraun left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

amandasavluchinske commented Mar 23, 2025 •

edited by sourcery-ai bot

Loading

sourcery-ai bot commented Mar 23, 2025 •

edited

Loading

fsbraun commented Mar 25, 2025 •

edited

Loading