Created attachment 8615 [details] [review]
[SIGNED-OFF] Bug 7757 - Edit basket vendor after it has been created

Signed-off-by: mveron <veron@veron.ch>

Works as expected. With patch applied I have an additional drop down where I can choose th vendor in Edit basket and can change vendor as appropiate.

Template [% IF %] are not permissible inside HTML tags, as they can break translations.  xt/tt_valid.t fails.

Marking Failed QA.

Hi Kyle, 

It seems you have unfortunately deleted a 'o' character  in your patch :)
Your patch replaces the zoom css properties with 'zom' (which does not exist).

Fixed, thanks for the heads up.

(In reply to comment #6)
> Hi Kyle, 
> 
> It seems you have unfortunately deleted a 'o' character  in your patch :)
> Your patch replaces the zoom css properties with 'zom' (which does not
> exist).

QA comment:

there's something strange when I apply the 2nd patch : I get:
            [% FOREACH b IN booksellers %]
llers %]
                   [% IF booksellerid == b.id %]
 b.id %]
       <option value="[% b.id %]" selected="selected">[% b.name %]</option>
/option>
                   [% ELSE %]
 ELSE %]
       <option value="[% b.id %]">[% b.name %]</option>
/option>

(yes, truncated code) does others have the same problem ?

Created attachment 9588 [details] [review]
Bug 7757 - Followup - Move IF outside of html tag

Paul, try with it (I removed tabulation characters)

(In reply to comment #11)
> Created attachment 9588 [details] [review]
> Bug 7757 - Followup - Move IF outside of html tag
> 
> Paul, try with it (I removed tabulation characters)

Not better

I've rewritten the patch

QA comment:
 * patch #2 = tiny patch, necessary, passed QA
 * patch #1 = 
   * improve some code, like 
-my $booksellerid;
-$booksellerid = $input->param('booksellerid');
+my $booksellerid = $input->param('booksellerid');
   * coding guidelines OK
   * I'm not sure 
-        ModBasketHeader($input->param('basketno'),$input->param('basketname'),$input->param('basketnote'),$input->param('basketbooksellernote'),$input->param('basketcontractnumber'));
+        ModBasketHeader( $input->param('basketno'), $input->param('basketname'), $input->param('basketnote'), $input->param('basketbooksellernote'), $input->param('basketcontractnumber') || undef, $input->param('basketbooksellerid') );
is the best way to deal with an empty basketcontractnumber ( $input->param('basketcontractnumber') || undef ), but there's nothing about that in the coding guidelines, so I won't object

passed QA

About parameters: I think we should define a rule for new subs, about how we pass parameters. I think passing hashes would gracefully solve this kind of problem. So what about a rule like = identifier/mandatory fields are passed directly, others are passed through a hash
The ModBasketHeader would then become:
ModBasketHeader($basketno, 
         { basketname => $basketname, 
         basketnote => $basketnote, 
         basketbooksellernote => $basketbooksellernote, 
         basketcontractnumber => basketcontractnumber, 
         basketbooksellerid=> $basketbooksellerid}
);

Kyle, if you agree/like this idea, I can start a discussion on koha-devel !

> About parameters: I think we should define a rule for new subs, about how we
> pass parameters. I think passing hashes would gracefully solve this kind of
> problem. So what about a rule like = identifier/mandatory fields are passed
> directly, others are passed through a hash
> The ModBasketHeader would then become:
> ModBasketHeader($basketno, 
>          { basketname => $basketname, 
>          basketnote => $basketnote, 
>          basketbooksellernote => $basketbooksellernote, 
>          basketcontractnumber => basketcontractnumber, 
>          basketbooksellerid=> $basketbooksellerid}
> );
> 
> Kyle, if you agree/like this idea, I can start a discussion on koha-devel !

Paul, I am 110% behind this idea. I've been using hashes for parameter passing in most of my new code. I think it makes it much easier to extend subroutines after the fact. Right new we have subs that look like sub( $param1, $parm2, undef, $param3, 1, $param4 ) and this would make our code far better.

I just re-read your comment. Do you think it's really necessary to pass required params directly? I really like the passing a single hash as the param list like so: 

mysub(
    param1 => $param1,
    param2 => $param2,
    param3 => $param3,
);

mysub {
  my %params = @_;

  my $param1 = $params{'param1'};
  my $param2 = $params{'param2'};
  my $param3 = $params{'param3'};

  return unless ( $param1 && $param2 )

  ## Do Stuff
}

(In reply to comment #13)
> > About parameters: I think we should define a rule for new subs, about how we
> > pass parameters. I think passing hashes would gracefully solve this kind of
> > problem. So what about a rule like = identifier/mandatory fields are passed
> > directly, others are passed through a hash
> > The ModBasketHeader would then become:
> > ModBasketHeader($basketno, 
> >          { basketname => $basketname, 
> >          basketnote => $basketnote, 
> >          basketbooksellernote => $basketbooksellernote, 
> >          basketcontractnumber => basketcontractnumber, 
> >          basketbooksellerid=> $basketbooksellerid}
> > );
> > 
> > Kyle, if you agree/like this idea, I can start a discussion on koha-devel !
> 
> Paul, I am 110% behind this idea. I've been using hashes for parameter
> passing in most of my new code. I think it makes it much easier to extend
> subroutines after the fact. Right new we have subs that look like sub(
> $param1, $parm2, undef, $param3, 1, $param4 ) and this would make our code
> far better.

This is especially better when a sub requires one of multiple options ( we have a number of subroutines that require either $param1 OR $param2, but not necessarily both ).

New feature, held for 3.10

There have been no further reports of problems so I am marking this bug resolved.