<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">On 06/07/2013 12:15 PM, Tomas Babej
wrote:<br>
</div>
<blockquote cite="mid:51B1B2DF.3050101@redhat.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
<div class="moz-cite-prefix">On 05/31/2013 12:08 PM, Ana
Krivokapic wrote:<br>
</div>
<blockquote cite="mid:51A87688.2070107@redhat.com" type="cite">
<pre wrap="">Hello,
This patch addresses ticket <a moz-do-not-send="true" class="moz-txt-link-freetext" href="https://fedorahosted.org/freeipa/ticket/3635">https://fedorahosted.org/freeipa/ticket/3635</a>.
</pre>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
<pre wrap="">_______________________________________________
Freeipa-devel mailing list
<a moz-do-not-send="true" class="moz-txt-link-abbreviated" href="mailto:Freeipa-devel@redhat.com">Freeipa-devel@redhat.com</a>
<a moz-do-not-send="true" class="moz-txt-link-freetext" href="https://www.redhat.com/mailman/listinfo/freeipa-devel">https://www.redhat.com/mailman/listinfo/freeipa-devel</a></pre>
</blockquote>
<br>
Hi, the patch itself works as it should, I have only some
refactoring comments:<br>
<br>
1.) There already is a add_range method that is called from within
trust_add's execute()<br>
and handles some range validation (currently only whether domain's
SID of new and <br>
existing range are equal, my patch 67 add some more).<br>
<br>
Your patch introduces a new method validate_range() that is called
from execute(),<br>
which leads to some duplication of the logic, mainly two same
calls to the API (getting<br>
the info about the old range via idrange_show)<br>
<br>
I'd rather we keep all the validation in one place, preferably
inside the add_range method<br>
or refactored out of it in the new method that is called only from
add_range().<br>
<br>
2.) That being said, other parts of refactoring are nice and make
code more clear, +1.<br>
<br>
Tomas<br>
</blockquote>
<br>
My first instinct was to place this validation in the add_range()
method. However, add_range() is called after the trust itself is
added, and validation introduced by this patch needs to happen
before the trust is added (we need to prevent the addition of trust
in case the validation fails).<br>
<br>
As for the code duplication, you are right about that. I tried to
minimize duplication - resulting updated patch attached.<br>
<pre class="moz-signature" cols="80">--
Regards,
Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.</pre>
</body>
</html>