<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 05:49 PM, Ana Krivokapic
      wrote:<br>
    </div>
    <blockquote cite="mid:51B20107.4050405@redhat.com" type="cite">
      <meta content="text/html; charset=ISO-8859-1"
        http-equiv="Content-Type">
      <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>
    </blockquote>
    <br>
    While this is a nice improvement from the code repetition point of
    view,<br>
    the patch still has one flaw as I see it - the range validation
    happens at two separate places:<br>
    <br>
    Once here(already in the code):<br>
    <br>
    <meta name="qrichtext" content="1">
    <p style=" margin-top:0px; margin-bottom:0px; margin-left:0px;
      margin-right:0px; -qt-block-indent:0; text-indent:0px;"><!--StartFragment-->
      if old_range:</p>
    <p style=" margin-top:0px; margin-bottom:0px; margin-left:0px;
      margin-right:0px; -qt-block-indent:0; text-indent:0px;">
      old_dom_sid = old_range['result']['ipanttrusteddomainsid'][0];</p>
    <p style="-qt-paragraph-type:empty; margin-top:0px;
      margin-bottom:0px; margin-left:0px; margin-right:0px;
      -qt-block-indent:0; text-indent:0px;"><br>
    </p>
    <p style=" margin-top:0px; margin-bottom:0px; margin-left:0px;
      margin-right:0px; -qt-block-indent:0; text-indent:0px;"> if
      old_dom_sid == dom_sid:</p>
    <p style=" margin-top:0px; margin-bottom:0px; margin-left:0px;
      margin-right:0px; -qt-block-indent:0; text-indent:0px;"> return</p>
    <p style="-qt-paragraph-type:empty; margin-top:0px;
      margin-bottom:0px; margin-left:0px; margin-right:0px;
      -qt-block-indent:0; text-indent:0px;"><br>
    </p>
    <p style=" margin-top:0px; margin-bottom:0px; margin-left:0px;
      margin-right:0px; -qt-block-indent:0; text-indent:0px;"> raise
      errors.ValidationError(name=_('range exists'),</p>
    <p style=" margin-top:0px; margin-bottom:0px; margin-left:0px;
      margin-right:0px; -qt-block-indent:0; text-indent:0px;">
      error=_('ID range with the same name but different ' \</p>
    <p style=" margin-top:0px; margin-bottom:0px; margin-left:0px;
      margin-right:0px; -qt-block-indent:0; text-indent:0px;"> 'domain
      SID already exists. The ID range for ' \</p>
    <p style=" margin-top:0px; margin-bottom:0px; margin-left:0px;
      margin-right:0px; -qt-block-indent:0; text-indent:0px;"> 'the new
      trusted domain must be created manually.'))<!--EndFragment--></p>
    <meta http-equiv="Content-Type" content="text/html;
      charset=ISO-8859-1">
    <style type="text/css">
p, li { white-space: pre-wrap; }
</style><br>
    And once here(added by your patch):<br>
    <br>
    +        if not self.validate_range(*keys, **options):<br>
    +            raise errors.ValidationError(<br>
    +                name=_('id range'),<br>
    +                error=_(<br>
    +                    'An id range already exists for this trust. '<br>
    +                    'You should either delete the old range, or '<br>
    +                    'exclude --base-id/--range-size options from
    the command.'<br>
    <br>
    I'd suggest we have one common place, say validate_range() function
    as we have now,<br>
    that contains all the checks (more coming in the future for sure).<br>
    <br>
    That would mean that the whole trust-add command will fail in the
    case that "ID range <br>
    with the same name but different domain SID already exists", since
    we now call validate_range()<br>
    within execute() method. I checked with Alexander and we agreed that
    this is more desired behaviour.<br>
    <br>
    This would also result to reducement of the number of API calls,
    which is a nice benefit.<br>
    <br>
    Tomas<br>
  </body>
</html>