[Freeipa-devel] [PATCH] 0004 User life cycle: support of MODRDN to a new superior

[Jan Cholasta]

Dne 12.4.2015 v 18:51 thierry bordaz napsal(a):
> On 04/09/2015 12:42 PM, thierry bordaz wrote:
>> On 04/08/2015 03:33 PM, Jan Cholasta wrote:
>>> Dne 8.4.2015 v 15:00 thierry bordaz napsal(a):
>>>> On 04/08/2015 08:34 AM, Jan Cholasta wrote:
>>>>> Hi,
>>>>>
>>>>> Dne 1.4.2015 v 17:40 thierry bordaz napsal(a):
>>>>>> Hello,
>>>>>>
>>>>>>     In user life cycle, Active entries are moved to Delete
>>>>>> container and
>>>>>>     Delete entries can be moved back to Staging container.
>>>>>>     This requires a LDAP modrdn with new superior that is not
>>>>>> supported
>>>>>>     in ldap2.
>>>>>
>>>>> Since update_entry_rdn() is used only in one spot in baseldap, I think
>>>>> we can merge it and move_entry_newsuperior() into a single method
>>>>> move_entry():
>>>>>
>>>>>     def move_entry(self, dn, new_dn, del_old=True):
>>>>>
>>>>> We can easily detect whether the superior needs to be updated by
>>>>> comparing dn[1:] and new_dn[1:].
>>>>
>>>> Hello Jan,
>>>>
>>>> Yes that is a good idea to merge those two methods. They both rely on
>>>> modrdn and a single method is enough.
>>>
>>> Well, I had something like this in mind:
>>>
>>>     def move_entry(self, dn, new_dn, del_old=True):
>>>         assert isinstance(dn, DN)
>>>         assert isinstance(new_dn, DN)
>>>
>>>         if new_dn == dn:
>>>             raise errors.EmptyModlist()
>>>
>>>         new_rdn = new_dn[0]
>>>         if new_rdn == dn[0]:
>>>             new_rdn = None
>>>
>>>         new_superior = new_dn[1:]
>>>         if new_superior == dn[1:]:
>>>             new_superior = None
>>>
>>>         with self.error_handler():
>>>             self.conn.rename_s(dn, new_rdn, new_superior, int(del_old))
>>>             time.sleep(.3)  # Give memberOf plugin a chance to work
>>>
>>> so that you don't have to care if you should change the RDN or the
>>> superior and it just does the right thing.
>>>
>>>>
>>>>>
>>>>> Maybe we can also get rid of del_old, if it's always gonna be True in
>>>>> our code?
>>>>
>>>> I think it is better to get this interface as close as possible as the
>>>> MODRDN call, so that del_old option will be already available for
>>>> future
>>>> usage.
>>>> I agree that currently del_old is always true in case of IPA but it
>>>> could be the default value.
>>>
>>> OK, it's not a big piece of code, so I guess we can leave it.
>>>
>> Thank for the reviews and the help. Here is a new patch.
>>
>> thierry
>>
> Hello,
>
> After additional tests, the previous patch was incomplete...
>
> thierry

Please wrap long lines:

                     new_dn = DN((self.obj.primary_key.name,
                                  entry_attrs[self.obj.primary_key.name]),
                                 *entry_attrs.dn[1:])
                     self._exc_wrapper(keys, options, ldap.move_entry)(
                         entry_attrs.dn, new_dn)

and:

             self.conn.rename_s(dn, new_rdn, newsuperior=new_superior,
                                delold=int(del_old))


Also, you don't need to include your login in the author header (it's 
part of your email address) or the reviewed by line in the commit 
message (it's automatically added by ipatool when the commit is pushed).

-- 
Jan Cholasta