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

Fix: Bad Title.path in Multilanguage sites if parent slug is created or modified #6968

Merged
merged 12 commits into from
Feb 6, 2021

Conversation

fp4code
Copy link

@fp4code fp4code commented Jan 22, 2021

Description

Fix #6622 Bad Title.path in Multilanguage sites if parent slug is created or modified
Just a reopening of PR #6965

Related resources

First bug:

In 'en','fr' site

Create /en/en-1/ page
Create from this page a subpage with slug "en-1-1"
Create "fr" translation with slug "fr-1-1"
Create "fr" translation of parent /en/en-1/ with slug "fr-1"

As a result, the path of subpage "fr-1-1" is not updated. Here the paths:

/en/en-1/
/en/en-1/en-1-1/
/fr/fr-1/
/fr/en-1/fr-1-1/ <-- Bad Title.path "en-1/fr-1-1" instead of "fr-1/fr-1-1"
Second bug:

Change "en" slug of /en/en-1/ page to "en-1b",
As a result the path of subpage "en-1-1" is changed to "fr-1/en-1-1" instead of "en-1b/en-1-1". Here the paths:

/en/en-1b/
/en/fr-1/en-1-1/ <-- Bad Title.path "fr-1/en-1-1" instead of "en-1b/fr-1-1"
/fr/fr-1/
/fr/en-1/fr-1-1/ <-- Bad Title.path "en-1/fr-1-1" instead of "fr-1/fr-1-1"

Checklist

  • I have opened this pull request against develop
  • I have updated the CHANGELOG.rst
  • I have added or modified the tests when changing logic

@Aiky30
Copy link
Contributor

Aiky30 commented Jan 26, 2021

@fp4code I think that this PR has all of the changes in it so the following PR can be closed: #6967

If you agree with the above then this PR (6968) becomes the one with all of the changes ready to review.

Once you confirm that is correct I can start to review this PR.

@Aiky30 Aiky30 changed the title Pull request #6965 with tests added Fix #6622 Bad Title.path in Multilanguage sites if parent slug is created or modified Jan 26, 2021
@Aiky30 Aiky30 changed the title Fix #6622 Bad Title.path in Multilanguage sites if parent slug is created or modified Fix: Bad Title.path in Multilanguage sites if parent slug is created or modified Jan 26, 2021
@fp4code
Copy link
Author

fp4code commented Jan 26, 2021

@Aiky30 yes, please start to review this PR. I have just added in Related resources the two bug descriptions. These bugs are also tested now in cms/tests/test_titlepath.py file.

Copy link
Contributor

@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.

Thanks for your effort on this issue.

I hope that you agree with the feedback and if you need any help please reach out and we can discuss any of your concerns in depth.

CHANGELOG.rst Outdated Show resolved Hide resolved
CHANGELOG.rst Outdated Show resolved Hide resolved
cms/tests/test_titlepath.py Outdated Show resolved Hide resolved
cms/tests/test_titlepath.py Outdated Show resolved Hide resolved
cms/tests/test_titlepath.py Outdated Show resolved Hide resolved

class TitlePathTestCase(CMSTestCase):

def test0001_translated_subpage_title_path(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Pleae name the tests consistently with the other tests in the project. Maybe: "test_translated_subpage_slug_generation_for_pages_with_different_language_translations"

Copy link
Author

@fp4code fp4code Jan 27, 2021

Choose a reason for hiding this comment

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

New name is:
"test_translated_subpage_title_path_regeneration"

cms/tests/test_titlepath.py Outdated Show resolved Hide resolved
cms/tests/test_titlepath.py Outdated Show resolved Hide resolved
@Aiky30
Copy link
Contributor

Aiky30 commented Jan 26, 2021

@victor-yunenko This will need your review as this is directly related to an issue that you have raised. :-)

@fp4code
Copy link
Author

fp4code commented Jan 27, 2021

I would like to write tests similar to "test_translated_subpage_title_path_regeneration" and "test_subpage_title_path_regeneration_after_parent_slug_change", but at form level. Precisely to exercise cms.forms.ChangePageForm.save. Any advice ?

Copy link
Contributor

@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.

My apologies for what may look like a lot of minor changes. I have only added them because the PEP 008 validation will fail due to 2 lines between the test functions. We may as well do a little more clean up at the same time.

You can accept or reject each suggestion using Github and will require no "manual" code changes. This should be a very easy task in the Github user interface. :-)

cms/tests/test_page.py Show resolved Hide resolved
cms/tests/test_page.py Outdated Show resolved Hide resolved
cms/tests/test_page.py Show resolved Hide resolved
cms/tests/test_page.py Outdated Show resolved Hide resolved
cms/tests/test_page.py Show resolved Hide resolved
cms/tests/test_page.py Outdated Show resolved Hide resolved
fp4code and others added 6 commits January 27, 2021 17:21
Required by PEP008 validation

Co-authored-by: Aiky30 <Aiky30@users.noreply.github.com>
Remove empty line break

Co-authored-by: Aiky30 <Aiky30@users.noreply.github.com>
Comment enhancement

Co-authored-by: Aiky30 <Aiky30@users.noreply.github.com>
Comment enhancement

Co-authored-by: Aiky30 <Aiky30@users.noreply.github.com>
Remove empty line as required by PEP 008

Co-authored-by: Aiky30 <Aiky30@users.noreply.github.com>
Remove an empty line

Co-authored-by: Aiky30 <Aiky30@users.noreply.github.com>
@fp4code
Copy link
Author

fp4code commented Jan 27, 2021

Thanks @Aiky30 for code polishing. It is simple to accept your suggestions. Maybe I should have used the "batch..." button. Unfortunately this button is not clear for me. I hope there will be a rebase before merging.

@Aiky30
Copy link
Contributor

Aiky30 commented Jan 27, 2021

Thanks @Aiky30 for code polishing. It is simple to accept your suggestions. Maybe I should have used the "batch..." button. Unfortunately this button is not clear for me. I hope there will be a rebase before merging.

Glad that it made it easy. We are obviously super grateful for your contribution and want to make it as easy as possible by also improving the codebase with tests. :-)

We just need a final sign off from @victor-yunenko and then we can look to merge it and then maybe backport it into a version if this is blocking people.

@Aiky30
Copy link
Contributor

Aiky30 commented Jan 27, 2021

@fp4code to bring the branch back up to date are yiu able to pull upstream master onto you branch?

You may have to set upstream for that to work.

@fp4code
Copy link
Author

fp4code commented Jan 27, 2021

@fp4code to bring the branch back up to date are yiu able to pull upstream master onto you branch?

You may have to set upstream for that to work.

Hi @Aiky30, I can do what you want but I'm not sure to understand exactly what is required. For example, I can

  1. merge my branch c2n-dju:fp-fix6622 with current django-cms:develop
  2. do a fast-forward of c2n-dju:master to c2n-dju:fp-fix6622
  3. do a fast-forward of c2n-dju:master to django-cms:master (not possible, there is no "django-cms:master" branch)

Eventually, I have done 1), created c2n-dju:master and done 2). Is it what you want? Why "master"?

Maybe, you want a branch "fp-fix6622-on-3.8.0"? Please, let me know!

@Aiky30
Copy link
Contributor

Aiky30 commented Jan 27, 2021

Hi @fp4code, I'm not sure if there's a quicker way mut o do the following to be sure that I have the latest. If I'm on a current working branch then I first move back out to my forked master:

'''
git checkout master
Git fetch upstream
Git pull upstream master
Git checkout Your Feature branch
Git merge master
'''

It might be possible to pull the latest changes straight in by git pull upstream master. I'll need to test this out in future.

@Aiky30
Copy link
Contributor

Aiky30 commented Jan 27, 2021

The pr no longer states that it's out of date so you must have done the update successfully. :-)

@fp4code
Copy link
Author

fp4code commented Jan 27, 2021

Git pull upstream master

Hi @fp4code, I'm not sure if there's a quicker way mut o do the following to be sure that I have the latest. If I'm on a current working branch then I first move back out to my forked master:

'''
git checkout master
Git fetch upstream
Git pull upstream master
Git checkout Your Feature branch
Git merge master
'''

It might be possible to pull the latest changes straight in by git pull upstream master. I'll need to test this out in future.

I have done the same things, but with "develop" instead of "master". There is no "master" branch in django-cms I think.

@fp4code
Copy link
Author

fp4code commented Jan 27, 2021

The pr no longer states that it's out of date so you must have done the update successfully. :-)

Thanks @Aiky30

@fp4code fp4code closed this Jan 27, 2021
@fp4code fp4code reopened this Jan 27, 2021
@fp4code
Copy link
Author

fp4code commented Jan 28, 2021

Sorry I made a confusion about closing button (my idea was to "close" a small discussion thread, not the PR of course!)

Copy link
Contributor

@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.

I'm happy with this change, thanks for your persistence.

Copy link

@viktor-yunenko viktor-yunenko left a comment

Choose a reason for hiding this comment

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

Makes sense to me 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants