[Freeipa-devel] Change the behaviour of addattr/setattr parameters

[Adam Young]

On 08/16/2010 09:30 AM, Adam Young wrote:
> I'm going to top post this to make sure it gets seen.
> Just ran the details patch on top of this, and the user details page 
> does not work with it.
>>
>> If you submit the page even with a minor edit to the full name you 
>> get an error: 'login' is required.
>>
>> I've tested it out with the JSON and CURL:  Here is the JSON it is 
>> sending:
>>
>> {"method":"user_mod","params":[["zoe"],{"all":true,"setattr":["cn=Zoe 
>> MacLeod","gidnumber=1044896486","title=","displayname=","initials=","uid=","mail=","street=","location=","postalcode=","ou=","carlicense="],"addattr":[],"givenname":"Zoe","sn":"MacPhearson","uidnumber":"1044896486","homedirectory":"/home/zoe","sizelimit":100}],"id":4} 
>>
>>
>>
>> The message 'login' is required is coming from the call I added to
>>     value = self.params[attr](value)
>> As it is the message inside RequirementError, which gets called from 
>> parameters.py, specifically:
>>
>> def validate(self, value):
>>         """
>>         Check validity of ``value``.
>>
>>         :param value: A proposed value for this parameter.
>>         """
>>         if value is None:
>>             if self.required:
>>                 raise RequirementError(name=self.cli_name)
>>
>>
>> My guess is the correct change is to skip this call if  value is 
>> null, which seems to be what is happening.  But I suspect we are 
>> sending in bogus values to setattr.  Notice this part of the JSON
>>
>> "setattr":["cn=Zoe 
>> MacLeod","gidnumber=1044896486","title=","displayname=","initials=","uid=","mail=","street=","location=","postalcode=","ou=","carlicense="] 
>>
>>
>>
>> My guess is that the details page shouldn't send any unset values.  
>> "uid=" in particular is probably a mistake.  REmoving that from the 
>> JSON gets us to: an internal error has occurred.
>>
>> Here's the stack trace:
>>
>> ipa: ERROR: non-public: TypeError: 'NoneType' object is not iterable
>> Traceback (most recent call last):
>>   File "/home/ayoung/devel/freeipa/ipaserver/rpcserver.py", line 206, 
>> in wsgi_execute
>>     result = self.Command[name](*args, **options)
>>   File "/home/ayoung/devel/freeipa/ipalib/frontend.py", line 401, in 
>> __call__
>>     ret = self.run(*args, **options)
>>   File "/home/ayoung/devel/freeipa/ipalib/frontend.py", line 674, in run
>>     return self.execute(*args, **options)
>>   File "/home/ayoung/devel/freeipa/ipalib/plugins/baseldap.py", line 
>> 431, in execute
>>     addset = set(get_attributes(options.get('addattr', [])))
>>   File "/home/ayoung/devel/freeipa/ipalib/plugins/baseldap.py", line 
>> 52, in get_attributes
>>     for attr in attrs:
>>
>>
>> Again, this is from the __call__ code, which means it is from the 
>> code I added, although now I'm not sure which parameter, if any is 
>> tripped the code.
>>
>>
>> I ran the full body of unit tests on the code as commited, and they 
>> all pass, as did the set of tests that Rob cobbled up (reminder: Lets 
>> get those added) so I don't think the call to 
>> self.params[attr](value) is wrong, but that it catches input errors 
>> that would have bitten us.
>
> Thinking about it afterwards, I wondered if the extra information was 
> messing it up. I tried it with just the following json and it seems to 
> have worked.
>
> {"method":"user_mod","params":[["zoe"],{"all":true,"setattr":["cn=Zoe 
> MacLeod","gidnumber=1044896486"],"sizelimit":100}],"id":4}
>
>
> So I think we need to identify values that have changed on the screen, 
> and only send that subset in the setattr portion.  Only send values in 
> as addattr if they were previously unset.  And avoid setting any 
> values as parameters outside of  setaddr or addattr.

THe attached patch does this:  when a field that has been changed loses 
focus, the new value is added to a collection (changed_detail).  Then, 
when the user clicks "update" the values in the changed_detail 
collection is added to the setattr collection of the JSON request.

I'm not too clear on the difference between setattr and addattr, so I 
suspect that the patch is incomplete.  I just wanted to get the idea out 
there.  THis patch should apply on top of pzuna's two outstanding UI 
patches.

>
> _______________________________________________
> Freeipa-devel mailing list
> Freeipa-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/freeipa-devel

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-catch-update-for-setattr.patch
Type: text/x-patch
Size: 4637 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20100816/2144fa5d/attachment.bin>