Bug 7757 - Edit basket vendor after it has been created
Summary: Edit basket vendor after it has been created
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: 3.10
Hardware: All All
: P3 enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Paul Poulain
URL:
Keywords:
Depends on:
Blocks: 8247
  Show dependency treegraph
 
Reported: 2012-03-20 13:22 UTC by Kyle M Hall
Modified: 2013-12-05 20:04 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 7757 - Edit basket vendor after it has been created (6.23 KB, patch)
2012-03-23 17:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7757 - Edit basket vendor after it has been created (6.32 KB, patch)
2012-03-23 19:35 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 7757 - Followup - Move IF outside of html tag (1.42 KB, patch)
2012-05-02 17:36 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7757 - Followup - Move IF outside of html tag (1.28 KB, patch)
2012-05-04 15:43 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7757 - Followup - Move IF outside of html tag (1.33 KB, patch)
2012-05-07 01:00 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 7757 - Followup - Move IF outside of html tag (1.34 KB, patch)
2012-05-15 14:49 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2012-03-20 13:22:40 UTC
In the Acquisitions module, add the ability to change the vendor for an individual basket.
Comment 1 Kyle M Hall 2012-03-23 17:00:57 UTC Comment hidden (obsolete)
Comment 2 Marc Véron 2012-03-23 19:35:38 UTC
Created attachment 8615 [details] [review]
[SIGNED-OFF] Bug 7757 - Edit basket vendor after it has been created

Signed-off-by: mveron <veron@veron.ch>
Comment 3 Marc Véron 2012-03-23 19:38:04 UTC
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.
Comment 4 Ian Walls 2012-03-25 21:48:24 UTC
Template [% IF %] are not permissible inside HTML tags, as they can break translations.  xt/tt_valid.t fails.

Marking Failed QA.
Comment 5 Kyle M Hall 2012-05-02 17:36:26 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2012-05-04 14:32:27 UTC
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).
Comment 7 Kyle M Hall 2012-05-04 15:43:06 UTC Comment hidden (obsolete)
Comment 8 Kyle M Hall 2012-05-04 15:43:54 UTC
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).
Comment 9 Chris Cormack 2012-05-07 01:00:19 UTC Comment hidden (obsolete)
Comment 10 Paul Poulain 2012-05-15 14:39:08 UTC
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 ?
Comment 11 Jonathan Druart 2012-05-15 14:49:01 UTC
Created attachment 9588 [details] [review]
Bug 7757 - Followup - Move IF outside of html tag

Paul, try with it (I removed tabulation characters)
Comment 12 Paul Poulain 2012-05-24 11:44:23 UTC
(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 !
Comment 13 Kyle M Hall 2012-05-24 12:24:06 UTC
> 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.
Comment 14 Kyle M Hall 2012-05-24 12:28:19 UTC
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.
Comment 15 Kyle M Hall 2012-05-24 12:30:36 UTC
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 ).
Comment 16 Chris Cormack 2012-05-24 21:06:49 UTC
New feature, held for 3.10
Comment 17 Jared Camins-Esakov 2012-12-31 01:06:47 UTC
There have been no further reports of problems so I am marking this bug resolved.