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 342 hook to copy relations #343

Closed
wants to merge 2 commits into from

Conversation

jrief
Copy link
Contributor

@jrief jrief commented Jul 6, 2023

Description

Details on this can be found in #342.

Running tests on this PR will fail, because the signature of the CMS's PageContentExtension.copy_relations() does not fit here.

This has to be changed first in the devel_4 branch of django-cms itself.

Related resources

This fixes #342

Checklist

  • I have opened this pull request against master
  • 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 #workgroup-pr-review on
    Slack to find a “pr review buddy” who is going to review my pull request.

Unit tests are pending. See above for the reason.

@fsbraun
Copy link
Member

fsbraun commented Jul 6, 2023

@jrief If I look at this it seems that the logic of copying extensions should be invoked by something like

    new_extension = old_extension.copy(new_content_object, language=new_content_object.langauge)

copy will call copy_relations.

Already the current versioning code replicates code from the core.

In a separate PR I'd suggest to remove to deprecate the language parameter since it is not needed. (It's essentially passed on to the copy_relations function.)

@jrief
Copy link
Contributor Author

jrief commented Jul 6, 2023

In order to get this functionality working, we have to patch django-cms first and make the language parameter optional.

I'm unsure about the copy-method,….objects.create(**extension_fields) already copies the object, so what's the intention of a separate copy method?

We can leave it just in case it has to be overridden, but usually it can remain a noop, except for invoking copy_relations.

@fsbraun
Copy link
Member

fsbraun commented Jul 6, 2023

It’s already there , just needs to be called. Versioning duplicates it incorrectly (.objects.create) and will conflict with any extension that overrides it’s copy method.

@fsbraun
Copy link
Member

fsbraun commented Sep 27, 2023

Should be fixed by #344. If not, please reopen.

@fsbraun fsbraun closed this Sep 27, 2023
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.

PageContentExtension should offer hook copy_relations
2 participants