[Freeipa-devel] [PATCH] 0005 Verify otptoken timespan is valid

[Jan Cholasta]

Dne 24.7.2014 v 10:00 David Kupka napsal(a):
>
>
> On 07/23/2014 05:07 PM, Jan Cholasta wrote:
>> Hi,
>>
>> On 23.7.2014 15:46, David Kupka wrote:
>>> https://fedorahosted.org/freeipa/ticket/4244
>>
>> 1) Use "isinstance(X, Y)" instead of "type(X) is Y".
>
> Thanks for advice, will try to remember.
>>
>> 2) When is "type(not_before) is str" or "type(not_after) is str" true?
>> The values coming from command options or LDAP should always be
>> datetime, never str.
>
> Actually, it is never true. I don't  know why I thought that there is
> such option.
>>
>> 3) There are some misindentations:
>>
>> +            raise ValidationError(name='not_after',
>> +                                    error='is before not_before!')
>>
>> +                raise ValidationError(name='not_after',
>> +                                    error='is before not_before!')
>>
>> +                raise ValidationError(name='not_before',
>> +                                    error='is after not_after!')
>>
>> 4) We don't do exclamation marks in errors messages.
>
> You re right, it's probably better not to shout at customer :)
>>
>> 5) Generally, when you want to validate command options, you should look
>> into "options", not "entry_attrs".
>>
>> 6) This is not right:
>>
>> +            result = self.api.Command.otptoken_find(ipatokenuniqueid=
>> +                entry_attrs.get('ipatokenuniqueid', None))['result']
>>
>> This is:
>>
>> +            result = self.api.Command.otptoken_show(keys[-1])['result']
>
> Both works, but Martin explained me why is otptoken_show better and how
> it actually works.
>>
>> Honza
>>
>

Few more nitpicks:

1) You can make _check_interval a little bit shorter by removing a 
redundant if statement:

+def _check_interval(not_before, not_after):
+    if not_before and not_after:
+        return not_before <= not_after
+    return True

2) Please keep the 2 line space between the last global function and 
first class in the module.

3) Error messages are for users' eyes, please don't use internal 
identifiers in them.

4) You don't need to check if the result of otptoken_show is empty, it 
never is.

-- 
Jan Cholasta