[feature] Implement copy/clone templates. #131, openwisp/openwisp-controller#134 #135
Conversation
| @@ -100,5 +103,21 @@ def clean(self, *args, **kwargs): | |||
| if self.type == 'vpn' and not self.config: | |||
| self.config = self.vpn.auto_client(auto_cert=self.auto_cert) | |||
|
|
|||
| def clone(self, user): | |||
| clone = copy(self) | |||
There was a problem hiding this comment.
There's an official way to clone model instances, please see https://docs.djangoproject.com/en/2.2/topics/db/queries/#copying-model-instances.
There was a problem hiding this comment.
I've read this, but this official way requires modification of the original object instance, IMO it would be quite confusing if a function named 'clone' behaved that way, it seems natural that function like that does not affect the original object, what do you think?
There was a problem hiding this comment.
I am not sure how the is the orginal one affected in the official way?
I may be missing something, can you please explain that, thanks! 😄
There was a problem hiding this comment.
The official way is to just change the primary key to None, do some changes and save() the original object instance. Change of the pk makes it a new object in database and when you are operating directly on the objects, it's good approach but lets see what happened if I implemented the helper clone() function this way:
Let's have some original Template
clone = original.clone()
The clone is created, but now, the original instance is equal to clone.
In the clone action implementation it doesn't make a difference since it does not longer use the original object, but do You not think that behavior may be confusing a bit if one wanted to reuse that clone function?
There was a problem hiding this comment.
Ah! I took me a really long while, been reading your message for 15 mins but I think now I get it! 😄
The self object would be changed!
CC: @nemesisdesign
There was a problem hiding this comment.
Sorry for being incomprehensible,
If you really want to do it similarly to the official way, It's also possible to change copy(self) to
self.__class__.objects.get(pk=self.pk), but IMO the copy() function is a way more readable.
pawelplsi
force-pushed
the
master
branch
2 times, most recently
from
be87f94
to
7914f53
Compare
|
Please fix the travis build as well! 😄 |
pawelplsi
force-pushed
the
master
branch
2 times, most recently
from
1bd305c
to
e547430
Compare
pawelplsi
force-pushed
the
master
branch
8 times, most recently
from
8a62a76
to
f9720ca
Compare
pawelplsi
force-pushed
the
master
branch
2 times, most recently
from
037dc7e
to
e6eb7c6
Compare
There was a problem hiding this comment.
I was testing and I got:
ValidationError at /admin/django_netjsonconfig/template/
{'name': ['Template with this Name already exists.']}
Why?
I was cloning a template named:
"default" when a template "default (CLONE)" existed from previous cloning.
Although it's not an application breaking problem, one can simply use "default (CLONE)" to create "default (CLONE) (CLONE)" but can you please look into it?
|
Fixed, now the next clone would be automaticaly named default (Clone 2), etc. |
|
I resolved conflicts and applied some minor improvements in #140. Will merge once openwisp/openwisp-controller#139 is ready. Follow up on my review on that one please @pawelplsi. |
Another PR to openwisp/openwisp-controller will implement also the wanted organization-selection feature when templates are cloned by multi-organization administrator.