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 djangocms-versioning draft deletion bug (#214) #215

Merged
merged 2 commits into from
Mar 10, 2024

Conversation

fscherf
Copy link
Contributor

@fscherf fscherf commented Jan 29, 2024

Previously, djangocms_alias.models.AliasContent.delete deleted all plugins related to AliasContent.alias, with the same language. Drafts and previously published versions included.
This meant that the deletion of a draft resulted in plugins disappearing from published pages.

This patch removes the mechanism entirely and instead adds a warning, notifying the user that the last version or translation of a language got deleted.

Copy link
Member

@fsbraun fsbraun left a comment

Choose a reason for hiding this comment

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

@fscherf Thanks for the pull request!

I've added two comments. Can I ask you to create a test for the behaviour, including the scenario without a message: Delete a draft when a published version is there?


def delete_model(self, request: HttpRequest, obj: AliasContent):
if obj.alias.contents.filter(language=obj.language).count() == 1:
message = _("Draft deleted. A new empty draft will be created automatically if needed.")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
message = _("Draft deleted. A new empty draft will be created automatically if needed.")
message = _("Alias content for language {} deleted. A new empty alias content will be created if needed.").format(obj.language)

Not all use cases of djangocms-alias will be in association with versioning. The reference to a draft version does not apply universally. (Sorry, I know I suggested the text in the first place. )

@@ -173,3 +173,13 @@ def has_module_permission(self, request: HttpRequest) -> bool:
"""Hides admin class in admin site overview"""

return False

def delete_model(self, request: HttpRequest, obj: AliasContent):
if obj.alias.contents.filter(language=obj.language).count() == 1:
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if that condition is correct. If verisoning is stalled, alias.contents will give published results only.

Suggested change
if obj.alias.contents.filter(language=obj.language).count() == 1:
if obj.alias._default_manager.filter(language=obj.language).count() == 1:

@fscherf fscherf force-pushed the fscherf/fix-draft-deletion-bug branch from 21a8672 to ea8db29 Compare January 30, 2024 12:26
@fscherf
Copy link
Contributor Author

fscherf commented Jan 30, 2024

@fsbraun Thanks for your review. I applied both requested changes

@fscherf fscherf requested a review from fsbraun January 30, 2024 12:32
@fscherf
Copy link
Contributor Author

fscherf commented Feb 5, 2024

@fsbraun: Is there anything else that I can do?

@fscherf
Copy link
Contributor Author

fscherf commented Feb 12, 2024

@fsbraun: What is the state of this PR? Can it be merged?

@fsbraun
Copy link
Member

fsbraun commented Feb 12, 2024

@fscherf LGTM! I will give it a test asap and merge afterwards!

@fsbraun
Copy link
Member

fsbraun commented Feb 12, 2024

So, tests should pass again!

@fsbraun
Copy link
Member

fsbraun commented Feb 12, 2024

@fscherf I can confirm, that deleting the alias draft does not remove all connected alias plugins. However, I fail to get the new message. Can you point me to where I should be seeing this?

@fscherf
Copy link
Contributor Author

fscherf commented Feb 12, 2024

@fsbraun: Ah! It seems, the message is not shown when using djangocms-versioning, because it overrides the delete method.

@fscherf
Copy link
Contributor Author

fscherf commented Feb 13, 2024

@fsbraun: It works, but the message does not show up in all cases. Is that enough, or do we need to change plans here?

@marksweb marksweb force-pushed the fscherf/fix-draft-deletion-bug branch from 4f3497c to dcb0a9a Compare February 13, 2024 21:27
@fscherf
Copy link
Contributor Author

fscherf commented Feb 27, 2024

What's the state of this PR?

fscherf and others added 2 commits March 9, 2024 11:29
Previously, `djangocms_alias.models.AliasContent.delete` deleted all plugins
related to `AliasContent.alias`, with the same language. Drafts and previously
published versions included.
This meant that the deletion of a draft resulted in plugins disappearing from
published pages.

This patch removes the mechanism entirely and instead adds a warning, notifying
the user that the last version or translation of a language got deleted.

Signed-off-by: Florian Scherf <[email protected]>
@marksweb marksweb force-pushed the fscherf/fix-draft-deletion-bug branch from 1706862 to 5eeb8da Compare March 9, 2024 11:29
@fsbraun fsbraun merged commit fa45af8 into django-cms:master Mar 10, 2024
12 checks passed
@fscherf fscherf deleted the fscherf/fix-draft-deletion-bug branch March 10, 2024 13:46
fscherf added a commit to fscherf/djangocms-alias that referenced this pull request Apr 9, 2024
…cms#215)

* fix djangocms-versioning draft deletion bug (django-cms#214)

Previously, `djangocms_alias.models.AliasContent.delete` deleted all plugins
related to `AliasContent.alias`, with the same language. Drafts and previously
published versions included.
This meant that the deletion of a draft resulted in plugins disappearing from
published pages.

This patch removes the mechanism entirely and instead adds a warning, notifying
the user that the last version or translation of a language got deleted.

Signed-off-by: Florian Scherf <[email protected]>

* ci: auto fixes from pre-commit hooks

for more information, see https://pre-commit.ci

---------

Signed-off-by: Florian Scherf <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
FinalAngel pushed a commit that referenced this pull request Apr 11, 2024
* fix djangocms-versioning draft deletion bug (#214) (#215)

* fix djangocms-versioning draft deletion bug (#214)

Previously, `djangocms_alias.models.AliasContent.delete` deleted all plugins
related to `AliasContent.alias`, with the same language. Drafts and previously
published versions included.
This meant that the deletion of a draft resulted in plugins disappearing from
published pages.

This patch removes the mechanism entirely and instead adds a warning, notifying
the user that the last version or translation of a language got deleted.

Signed-off-by: Florian Scherf <[email protected]>

* ci: auto fixes from pre-commit hooks

for more information, see https://pre-commit.ci

---------

Signed-off-by: Florian Scherf <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* tests: requirements: django42: fix broken djangocms-versioning URL

Signed-off-by: Florian Scherf <[email protected]>

---------

Signed-off-by: Florian Scherf <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
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.

2 participants