Fix 342 hook to copy relations

django-cms / djangocms-versioning

General purpose versioning package for Django CMS 4 and above.

Other

33 stars 29 forks source link

Fix 342 hook to copy relations #343

Closed jrief closed 9 months ago

jrief commented 1 year ago

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

[x] I have opened this pull request against master
[ ] I have added or modified the tests when changing logic
[x] I have followed the conventional commits guidelines to add meaningful information into the changelog
[x] 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 commented 12 months ago

@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 commented 12 months ago

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 commented 12 months ago

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 commented 9 months ago

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