Description
Edgar Fuß
2010-09-28 18:59:57 UTC
Comment on attachment 2653 [details] [review] Patch to enable acquisition orders by email correcting mime type A basic translation of the mail template: Hello. <item><<orders.quantity>> copies of <<orders.full_title>> by <<orders.author>>, ISBN <<orders.isbn>> for <<orders.ecost>> each (list price <<orders.listprice>>), total <<orders.quantity_ecost>></item> <total>Total <<orders.total.quantity>> item(s), total price <<orders.total.ecost_gsti>> incl. <<orders.total.gstrate>>% VAT This order is based on a rebate of <<orders.total.discount>>%. All prices in <<orders.total.currency>>.</total> Good bye. [done by Edgar Fuß] Working on reformatting this so it applies on master Created attachment 4067 [details] [review] First patch of 2, changes reformatted for master Created attachment 4068 [details] [review] Follow up patch containing template changes The ran into some problems, will send a follow up. Also trying to make use of the message_queue. Created attachment 4781 [details] [review] Revised patch for 3.4.3 I'll attach a revised patch for 3.4.3. This time in GIT format in order to (hopefully) make all GIT people happy. As before, written by Edgar Fuß, Mathematisches Institut der Universität Bonn. Hello, sorry I have tested, and when you click on "Send Mail" no requester to ask you to confirm action, and more uncomfortable IMO no popup to say "operation successfull" or "mail sent" or other... regards. Edgar, your patch does not seems to be in git format a git patch starts with : (for example) From 1b633c25236c62d43e938dac02f01897ba073747 Mon Sep 17 00:00:00 2001 From: Chris Cormack <chrisc@catalyst.net.nz> Date: Tue, 10 May 2011 09:30:06 +1200 Subject: [PATCH] Bug 5260 : Patch for template changes your revised patch #4781 starts by : diff --git a/acqui/basketgroup.pl b/acqui/basketgroup.pl index 08356f8..4a4a9f2 100755 --- a/acqui/basketgroup.pl +++ b/acqui/basketgroup.pl QA comment: I also see that there are many commented warns or print, like +#print "order_mail\n"; +#print "booksellerid: $booksellerid\n", "mail_orders: ", Dumper($mail_orders); could you remove them before submitting, otherwise it will "fail QA" later Thanks I don't know if somebody is still working on that, but that would be a nice improvement! M. Saby Still nobody working on that? Mathieu Edgar Fuß wrote me. He is no working on Koha. So, could an other dev take in charge this feature? Mathieu (In reply to mathieu saby from comment #12) > Edgar Fuß wrote me. He is no working on Koha. > So, could an other dev take in charge this feature? > If you find me some funding, I'd be more than happy to (In reply to Chris Cormack from comment #13) > (In reply to mathieu saby from comment #12) > > Edgar Fuß wrote me. He is no working on Koha. > > So, could an other dev take in charge this feature? > > > If you find me some funding, I'd be more than happy to Maybe the work made here can be reused. I'm taking a look... Created attachment 22171 [details] [review] [PATCH] Bug 5260 : Send basketgroup by mail to the bookseller 2 Patchs originally written by Edgar Fuß <ef@math.uni-bonn.de> in 2011 Merged, rebased and ceaned-up by Mathieu Saby on 21st oct 2013 Do not test for the moment I rebased everything and clean tabs and trailing whistepaces. It applies, but probably won't work well, as master has evolved since 2011. So, setting it in dissussion for the moment. Mathieu (In reply to mathieu saby from comment #16) > I rebased everything and clean tabs and trailing whistepaces. > It applies, but probably won't work well, as master has evolved since 2011. > So, setting it in dissussion for the moment. > > Mathieu I would say that this bug is very relevant to all larger Libraries. Is it going to be released anytime soon? Hello I'm not working on it anymore. If you or someone else is interested, you can maybe use the existing patch as a base or a source of inspiration. Mathieu Has anybody plans to work on this enhancement? I talked to Katrin about this topic recently, she said she has a proof of concept. Working on an alternative patch for implementing this feature - will upload patches in the next few days! Ok, back to working on this patch set... hope to be able to really upload something soon this time. Comment on attachment 22171 [details] [review] [PATCH] Bug 5260 : Send basketgroup by mail to the bookseller >From 10d994598c4cbcae260aabe818cd0209a938d370 Mon Sep 17 00:00:00 2001 >From: =?UTF-8?q?Edgar=20Fu=C3=9F?= <ef@math.uni-bonn.de> >Date: Mon, 21 Oct 2013 20:08:11 +1200 >Subject: [PATCH] Bug 5260 : Send basketgroup by mail to the bookseller > >2 Patchs originally written by Edgar Fuà <ef@math.uni-bonn.de> in 2011 >Merged, rebased and ceaned-up by Mathieu Saby on 21st oct 2013 > >Do not test for the moment > > >--- > acqui/basketgroup.pl | 181 +++++++++++++++++++- > koha-tmpl/intranet-tmpl/prog/en/js/acq.js | 8 + > .../prog/en/modules/acqui/basketgroup.tt | 3 +- > .../intranet-tmpl/prog/en/modules/tools/letter.tt | 5 + > tools/letter.pl | 33 ++++ > 5 files changed, 225 insertions(+), 5 deletions(-) > >diff --git a/acqui/basketgroup.pl b/acqui/basketgroup.pl >index 6fe4db2..7ffff0b 100755 >--- a/acqui/basketgroup.pl >+++ b/acqui/basketgroup.pl >@@ -293,6 +293,171 @@ sub printbasketgrouppdf{ > > } > >+sub order_mail { >+#use Data::Dumper; >+ my ($booksellerid, $mail_orders) = @_; >+#print "order_mail\n"; >+#print "booksellerid: $booksellerid\n", "mail_orders: ", Dumper($mail_orders); >+ my $mail_order_total = shift @$mail_orders; >+ >+ my $bookseller = GetBookSellerFromId($booksellerid); >+ >+ my $letter = C4::Letters::getletter('orders', 'ORDERMAIL') || return; >+#print "letter: ", Dumper(\$letter); >+ >+ eval "use Mail::Sendmail"; >+ eval "use C4::Log"; >+ eval "use Carp"; >+ eval "use Encode"; >+ >+ # branch info >+ my $userenv = C4::Context->userenv; >+ C4::Letters::parseletter($letter, 'branches', $userenv->{branch}); >+ >+ # librarian name >+ $letter->{content} =~ s/<<LibrarianFirstname>>/$userenv->{firstname}/g; >+ $letter->{content} =~ s/<<LibrarianSurname>>/$userenv->{surname}/g; >+ $letter->{content} =~ s/<<LibrarianEmailaddress>>/$userenv->{emailaddress}/g; >+ >+ # booksellers >+ C4::Letters::parseletter($letter, 'aqbooksellers', $booksellerid); >+ >+ # items and total >+ return unless $letter->{'content'} =~ m/(<item>(.*)<\/item>)/sm; >+ my $item_place = $1; my $item_format = $2; >+ my ($total_place, $total_format); >+ if ($letter->{'content'} =~ m/(<total>(.*)<\/total>)/sm) { >+ $total_place = $1; $total_format = $2; >+ } >+ >+ my @items; >+ foreach my $mail_order (@$mail_orders) { >+ my $item = $item_format; >+ while (my ($key, $value) = each %$mail_order) { >+#print "$key: $value\n"; >+ $item =~ s/<<orders.$key>>/$value/g; >+ } >+ push @items, $item; >+ } >+#print "items: ", Dumper(@items); >+ $letter->{'content'} =~ s/\Q$item_place\E/join "\n",@items/e; >+ if ($total_format) { >+#print "total_place: $total_place\n"; >+#print "total_format: $total_format\n"; >+ my $total = $total_format; >+ while (my ($key, $value) = each %$mail_order_total) { >+ $total =~ s/<<orders.total.$key>>/$value/g; >+ } >+#print "total: $total\n"; >+ $letter->{'content'} =~ s/\Q$total_place\E/$total/; >+ } >+ my %mail = ( >+ To => $bookseller->{bookselleremail} || >+ $bookseller->{contemail} || >+ $userenv->{emailaddress}, >+ From => $userenv->{emailaddress}, >+ Subject => $letter->{title}, >+ Message => Encode::encode("UTF-8", $letter->{content}), >+ 'Content-Type' => 'text/plain; charset="utf8"', >+ ); >+#print "mail: ", Dumper(\%mail); >+ sendmail(%mail) or carp $Mail::Sendmail::error; >+ logaction( >+ "ORDER", >+ "Send email order", >+ undef, >+ "To=%mail{To}\nSubject=%mail{Subject}\nMessage=%mail{Message}" >+ ) if C4::Context->preference("LetterLog"); >+} >+ >+sub mailbasketgroup { >+ my ($basketgroupid) = @_; >+ >+ eval "use C4::Branch"; >+ eval "use C4::Biblio"; >+ eval "use C4::Koha"; >+ eval "use Number::Format qw(format_number format_price)"; >+ >+ my $num = FormatNumber; # C4::Output >+ >+ my $itemtypes = GetItemTypes(); >+ >+ my $basketgroup = GetBasketgroup($basketgroupid); >+ my $booksellerid = $basketgroup->{booksellerid}; >+ my $bookseller = GetBookSellerFromId($booksellerid); >+ my $baskets = GetBasketsByBasketgroup($basketgroupid); >+ >+ my $gstrate = $bookseller->{gstrate} || C4::Context->preference("gist") || 0; >+ my $discount = $bookseller->{'discount'} / 100; >+ >+ my $total_ecost; # Total, its value will be assigned to $total_ecost_gsti or $total_ecost_gste depending of $bookseller->{'listincgst'} >+ my $total_ecost_gsti; # Total, GST included >+ my $total_ecost_gste; # Total, GST excluded >+ my $total_quantity; # Total quantity >+ >+ my @mail_orders; >+ for my $basket (@$baskets) { >+ my $basketno = $basket->{basketno}; >+ my @orders = &GetOrders($basketno); >+ for my $order (@orders) { >+ my %mail_order; >+ >+ my $quantity = $order->{quantity} || 0; >+ next if $quantity <= 0; >+ my $ecost = $order->{ecost} || 0; >+ >+ for (qw(quantity quantityreceived author title volume seriestitle isbn publishercode)) { >+ $mail_order{$_} = $order->{$_} if defined $order->{$_}; >+ } >+ for (qw(listprice ecost)) { >+ $mail_order{$_} = $num->format_price($order->{$_}) if defined $order->{$_}; >+ } >+ my $full_title = $order->{title}; >+ $full_title .= (" " . $order->{seriestitle}) if $order->{seriestitle}; >+ $full_title .= (" " . $order->{volume}) if $order->{volume}; >+ $mail_order{full_title} = $full_title; >+ if ($order->{biblionumber}) { >+ my $bibliodata = GetBiblioData($order->{biblionumber}); >+ if ($bibliodata->{itemtype}) { >+ $mail_order{itemtype} = $itemtypes->{$bibliodata->{itemtype}}->{description}; >+ } >+ } >+ >+ my $quantity_ecost = $quantity * $ecost; >+ $mail_order{quantity_ecost} = $num->format_price($quantity_ecost); >+ $mail_order{basketno} = $basketno; >+ $total_ecost += $quantity_ecost; >+ $total_quantity += $quantity; >+ push @mail_orders, \%mail_order; >+ } >+ } >+ >+ my %total; >+ $total{quantity} = $total_quantity; >+ $total{gstrate} = $num->format_number($gstrate); >+ $total{currency} = $bookseller->{listprice}; >+ $total{discount} = $num->format_number($bookseller->{discount}); >+ >+ my $total_gist; >+ >+ if ($bookseller->{listincgst}) { # prices already includes GST >+ $total_ecost_gsti = $total_ecost; >+ $total_ecost_gste = $total_ecost_gsti / ($gstrate + 1); >+ $total_gist = $total_ecost_gsti - $total_ecost_gste; >+ } else { # prices does not include GST >+ $total_ecost_gste = $total_ecost; >+ $total_gist = $total_ecost_gste * $gstrate; >+ $total_ecost_gsti = $total_ecost_gste + $total_gist; >+ } >+ $total{ecost_gste} = $num->format_price($total_ecost_gste); >+ $total{ecost_gsti} = $num->format_price($total_ecost_gsti); >+ $total{gist} = $num->format_number($total_gist); >+ >+ unshift @mail_orders, \%total; >+ >+ order_mail($booksellerid, \@mail_orders); >+} >+ > my $op = $input->param('op') || 'display'; > my $booksellerid = $input->param('booksellerid'); > $template->param(booksellerid => $booksellerid); >@@ -409,6 +574,14 @@ if ( $op eq "add" ) { > > printbasketgrouppdf($basketgroupid); > exit; >+} elsif ( $op eq 'closeandmail') { >+ my $basketgroupid = $input->param('basketgroupid'); >+ >+ CloseBasketgroup($basketgroupid); >+ >+ mailbasketgroup($basketgroupid); >+ >+ print $input->redirect('/cgi-bin/koha/acqui/basketgroup.pl?booksellerid=' . $booksellerid); > }elsif ($op eq 'print'){ > my $basketgroupid = $input->param('basketgroupid'); > >@@ -422,19 +595,19 @@ if ( $op eq "add" ) { > ); > print GetBasketGroupAsCSV( $basketgroupid, $input ); > exit; >+}elsif ($op eq 'mail'){ >+ my $basketgroupid = $input->param('basketgroupid'); >+ mailbasketgroup($basketgroupid); >+ print $input->redirect('/cgi-bin/koha/acqui/basketgroup.pl?booksellerid=' . $booksellerid); > }elsif( $op eq "delete"){ > my $basketgroupid = $input->param('basketgroupid'); > DelBasketgroup($basketgroupid); > print $input->redirect('/cgi-bin/koha/acqui/basketgroup.pl?booksellerid=' . $booksellerid); >- > }elsif ( $op eq 'reopen'){ > my $basketgroupid = $input->param('basketgroupid'); > my $booksellerid = $input->param('booksellerid'); >- > ReOpenBasketgroup($basketgroupid); >- > print $input->redirect('/cgi-bin/koha/acqui/basketgroup.pl?booksellerid=' . $booksellerid . '#closed'); >- > } elsif ( $op eq 'attachbasket') { > > # Getting parameters >diff --git a/koha-tmpl/intranet-tmpl/prog/en/js/acq.js b/koha-tmpl/intranet-tmpl/prog/en/js/acq.js >index 3fde6f1..d3e758e 100644 >--- a/koha-tmpl/intranet-tmpl/prog/en/js/acq.js >+++ b/koha-tmpl/intranet-tmpl/prog/en/js/acq.js >@@ -382,6 +382,14 @@ function closeandprint(bg){ > } > } > >+function closeandmail(bs, bg){ >+ if(document.location = '/cgi-bin/koha/acqui/basketgroup.pl?op=closeandmail&booksellerid=' + bs + '&basketgroupid=' + bg ){ >+ setTimeout("window.location.reload();",3000); >+ }else{ >+ alert('Error downloading the file'); >+ } >+} >+ > //function that lets the user unclose a basketgroup as long as he hasn't submitted the changes to the page. > function unclosegroup(bgid){ > var div = document.getElementById('basketgroup-'+bgid+'-closed').parentNode; >diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basketgroup.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basketgroup.tt >index b6cdc52..4613cc7 100644 >--- a/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basketgroup.tt >+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basketgroup.tt >@@ -237,7 +237,6 @@ function submitForm(form) { > </div> > </div> > [% ELSE %] >- > <div id="toolbar" class="btn-toolbar"> > <div class="btn-group"><a href="/cgi-bin/koha/acqui/basketgroup.pl?op=add&booksellerid=[% booksellerid %]" class="btn btn-small" id="newbasketgroup"><i class="icon-plus"></i> New basket group</a></div> > </div> >@@ -278,6 +277,7 @@ function submitForm(form) { > <td>[% basketgroup.basketsqty %]</td> > <td> > <input type="button" onclick="closeandprint([% basketgroup.id %])" value="Close and print" /> >+ <a href="/cgi-bin/koha/acqui/basketgroup.pl?op=closeandmail&booksellerid=[% basketgroup.booksellerid %]&basketgroupid=[% basketgroup.id %]" class="yui-button yui-link-button">Close & Send Mail</a> > <form action="/cgi-bin/koha/acqui/basketgroup.pl" method="get"><input type="hidden" name="op" value="add" /><input type="hidden" name="booksellerid" value="[% basketgroup.booksellerid %]" /><input type="hidden" name="basketgroupid" value="[% basketgroup.id %]" /><input type="submit" value="Edit" /></form> > <form action="/cgi-bin/koha/acqui/basketgroup.pl" method="get"><input type="hidden" name="op" value="delete" /><input type="hidden" name="booksellerid" value="[% basketgroup.booksellerid %]" /><input type="hidden" name="basketgroupid" value="[% basketgroup.id %]" /><input type="submit" value="Delete" /></form> > </td> >@@ -318,6 +318,7 @@ function submitForm(form) { > <form action="/cgi-bin/koha/acqui/basketgroup.pl" method="get"><input type="hidden" name="op" value="reopen" /><input type="hidden" name="booksellerid" value="[% basketgroup.booksellerid %]" /><input type="hidden" name="basketgroupid" value="[% basketgroup.id %]" /><input type="submit" value="Reopen" /></form> > <form action="/cgi-bin/koha/acqui/basketgroup.pl" method="get"><input type="hidden" name="op" value="print" /><input type="hidden" name="basketgroupid" value="[% basketgroup.id %]" /><input type="submit" value="Print" /></form> > <form action="/cgi-bin/koha/acqui/basketgroup.pl" method="get"><input type="hidden" name="op" value="export" /><input type="hidden" name="basketgroupid" value="[% basketgroup.id %]" /><input type="submit" value="Export as CSV" /></form> >+ <a href="/cgi-bin/koha/acqui/basketgroup.pl?op=mail&booksellerid=[% basketgroup.booksellerid %]&basketgroupid=[% basketgroup.id %]" class="yui-button yui-link-button">Send Mail</a> > </td> > </tr> > [% END %] >diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/letter.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/letter.tt >index 52e2b83..3f81aca 100644 >--- a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/letter.tt >+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/letter.tt >@@ -304,6 +304,11 @@ $(document).ready(function() { > [% ELSE %] > <option value="suggestions">Suggestions</option> > [% END %] >+ [% IF ( orders ) %] >+ <option value="orders" selected="selected">Orders</option> >+ [% ELSE %] >+ <option value="orders">Orders</option> >+ [% END %] > </select> > </li> > <li> >diff --git a/tools/letter.pl b/tools/letter.pl >index 7582e66..d3df297 100755 >--- a/tools/letter.pl >+++ b/tools/letter.pl >@@ -172,6 +172,29 @@ sub add_form { > } > > my $field_selection; >+ if ( $module eq "suggestions" ) { >+ push @SQLfieldname, column_picks('borrowers'), >+ column_picks('suggestions'), >+ column_picks('aqbooksellers'), >+ column_picks('biblio'), >+ column_picks('items'); >+ } >+ elsif ( $module eq "reserves" ) { >+ push @SQLfieldname, column_picks('borrowers'), >+ column_picks('reserves'), >+ column_picks('biblio'), >+ column_picks('items'); >+ } >+ elsif ( $module eq "orders" ) { >+ push @SQLfieldname, column_picks('aqbooksellers'), >+ column_picks('aqorders'), >+ column_picks('biblio'), >+ column_picks('items'); >+ } >+ elsif ( index( $module, "acquisition" ) > 0 ) { # FIXME: imprecise comparison >+ push @SQLfieldname, column_picks('aqbooksellers'), column_picks('aqorders'); >+ # add issues specific tables >+ } > push @{$field_selection}, add_fields('branches'); > if ($module eq 'reserves') { > push @{$field_selection}, add_fields('borrowers', 'reserves', 'biblio', 'items'); >@@ -179,6 +202,16 @@ sub add_form { > elsif ($module eq 'claimacquisition') { > push @{$field_selection}, add_fields('aqbooksellers', 'aqorders', 'biblio', 'biblioitems'); > } >+ elsif ($module eq 'orders') { >+ push @{$field_selection}, add_fields('aqbooksellers', 'aqorders', 'biblio', 'items'); >+ push @{$field_selection}, { value => q{}, text => '---ORDER ITEMS---' }; >+ push @{$field_selection}, { value => "orders.$_", text => "orders.$_" } >+ foreach (qw/listprice quantity ecost quantityreceived author title volume seriestitle isbn publishercode/); >+ push @{$field_selection}, { value => "orders.$_", text => "orders.$_" } >+ foreach (qw/full_title itemtype quantity_ecost basketno/); >+ push @{$field_selection}, { value => "orders.total.$_", text => "orders.total.$_" } >+ foreach (qw/quantity gstrate currency discount ecost_gste ecost_gsti gstrate/); >+ } > elsif ($module eq 'claimissues') { > push @{$field_selection}, add_fields('aqbooksellers', 'serial', 'subscription'); > push @{$field_selection}, >-- >1.7.9.5 > Created attachment 56449 [details] [review] Bug 5260 - Add option to send an order by e-mail to the acquisition module With this patch it will be possible to send order information to the vendor by e-mail. For now this feature can be triggered manually with a button before closing the basket. The order e-mail is based on the acquisition claim feature, but uses a new notice template. Test plan: 1) Vendors A new checkbox "Contact when ordering?" was added to the vendor page. - Add a vendor and/or edit an existing vendor - Verify the new option is saved correctly - Verify the new option displays on the vendor summary page after saving 2) Notices The feature works with a new notice template: ACQORDER It works with the same formatting/fields etc. as the acq claim notice. - Add a new notice template ACQORDER in module 'Claim/order aquisition' - Make sure to use fields from the various offered tables in your notice - Verify it is saved correctly 3) Basket - Turn on LetterLog system preference - Create multiple order lines - Click the 'Send order' button in the toolbar - Verify error or success message - Verify you received the e-mail - Verify there is a new entry with about the sent notice in your action_logs table 4) Regression testing... - Verify order claims still work - Verify serial claims still work - Verify new serial issue notices still work ... (I can provide additional test plans if needed) Created attachment 56450 [details] [review] Bug 5260 - Schema changes Schema changes for new column in the aqcontacts table. Created attachment 56483 [details] [review] Bug 5260 - Schema changes Schema changes for new column in the aqcontacts table. Created attachment 56484 [details] [review] Bug 5260 - Add option to send an order by e-mail to the acquisition module With this patch it will be possible to send order information to the vendor by e-mail. For now this feature can be triggered manually with a button before closing the basket. The order e-mail is based on the acquisition claim feature, but uses a new notice template. Test plan: 1) Vendors A new checkbox "Contact when ordering?" was added to the vendor page. - Add a vendor and/or edit an existing vendor - Verify the new option is saved correctly - Verify the new option displays on the vendor summary page after saving 2) Notices The feature works with a new notice template: ACQORDER It works with the same formatting/fields etc. as the acq claim notice. - Add a new notice template ACQORDER in module 'Claim/order aquisition' - Make sure to use fields from the various offered tables in your notice - Verify it is saved correctly 3) Basket - Turn on LetterLog system preference - Create multiple order lines - Click the 'Send order' button in the toolbar - Verify error or success message - Verify you received the e-mail - Verify there is a new entry with about the sent notice in your action_logs table 4) Regression testing... - Verify order claims still work - Verify serial claims still work - Verify new serial issue notices still work ... (I can provide additional test plans if needed) Aware of missing tests - working on it, but would love some first feedback if someone would like to take a look. I take a look of code and for me is good. I undeline that we need to update the description of ClaimsBccCopy sys preference. With this patch it becames: "[... ] blind copy (BCC) to logged in user when sending an acquisition order my mail or a claim notices for serial or acquisitions.". Zeno, thx for the feedback. I am still working on the tests and have to rebase now on Marcel's bug. New patches coming soon. As the name of the system preference is specific to claiming I have decided to not make it work for orders also. If there is a need for a feature like this, we could add it later on. Hi Katrin, in my humble opinion the option to recieve a Bcc copy of e-mail is quite important. If we can develop it with this patch for me is better. Created attachment 56549 [details] [review] Bug 5260 - Add option to send an order by e-mail to the acquisition module With this patch it will be possible to send order information to the vendor by e-mail. For now this feature can be triggered manually with a button before closing the basket. The order e-mail is based on the acquisition claim feature, but uses a new notice template. Test plan: 1) Vendors A new checkbox "Contact when ordering?" was added to the vendor page. - Add a vendor and/or edit an existing vendor - Verify the new option is saved correctly - Verify the new option displays on the vendor summary page after saving 2) Notices The feature works with a new notice template: ACQORDER It works with the same formatting/fields etc. as the acq claim notice. - Add a new notice template ACQORDER in module 'Claim/order aquisition' - Make sure to use fields from the various offered tables in your notice - Verify it is saved correctly 3) Basket - Turn on LetterLog system preference - Create multiple order lines - Click the 'Send order' button in the toolbar - Verify error or success message - Verify you received the e-mail - Verify there is a new entry with about the sent notice in your action_logs table 4) Regression testing... - Verify order claims still work - Verify serial claims still work - Verify new serial issue notices still work ... (I can provide additional test plans if needed) Created attachment 56550 [details] [review] Bug 5260: Unit tests for new functionality in SendAlert Created attachment 56551 [details] [review] Bug 5260 - Schema changes Schema changes for new column in the aqcontacts table. Ready for testing now! Created attachment 56560 [details] [review] Bug 5260 - Add option to send an order by e-mail to the acquisition module With this patch it will be possible to send order information to the vendor by e-mail. For now this feature can be triggered manually with a button before closing the basket. The order e-mail is based on the acquisition claim feature, but uses a new notice template. Test plan: 1) Vendors A new checkbox "Contact when ordering?" was added to the vendor page. - Add a vendor and/or edit an existing vendor - Verify the new option is saved correctly - Verify the new option displays on the vendor summary page after saving 2) Notices The feature works with a new notice template: ACQORDER It works with the same formatting/fields etc. as the acq claim notice. - Add a new notice template ACQORDER in module 'Claim/order aquisition' - Make sure to use fields from the various offered tables in your notice - Verify it is saved correctly 3) Basket - Turn on LetterLog system preference - Create multiple order lines - Click the 'Send order' button in the toolbar - Verify error or success message - Verify you received the e-mail - Verify there is a new entry with about the sent notice in your action_logs table 4) Regression testing... - Verify order claims still work - Verify serial claims still work - Verify new serial issue notices still work ... (I can provide additional test plans if needed) Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 56561 [details] [review] Bug 5260: Unit tests for new functionality in SendAlert Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 56562 [details] [review] Bug 5260 - Schema changes Schema changes for new column in the aqcontacts table. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Comment on attachment 56560 [details] [review] Bug 5260 - Add option to send an order by e-mail to the acquisition module Review of attachment 56560 [details] [review]: ----------------------------------------------------------------- Code looks generally good (given the area it's been added, it conforms to the script style), in testing I cannot find any fallout issues and the feature behaves as expected. ::: C4/Letters.pm @@ +368,2 @@ > > +Parameters: This block needs properly indenting please ;) (In reply to Zeno Tajoli from comment #32) > Hi Katrin, > in my humble opinion the option to recieve a Bcc copy of e-mail is quite > important. > If we can develop it with this patch for me is better. I agree that having the option to receive a BCC is good, but I think we shouldn't mix it in with the claims. I filed bug 17455 for this. (In reply to Martin Renvoize from comment #40) > Comment on attachment 56560 [details] [review] [review] > Bug 5260 - Add option to send an order by e-mail to the acquisition module > > Review of attachment 56560 [details] [review] [review]: > ----------------------------------------------------------------- > > Code looks generally good (given the area it's been added, it conforms to > the script style), in testing I cannot find any fallout issues and the > feature behaves as expected. > > ::: C4/Letters.pm > @@ +368,2 @@ > > > > +Parameters: > > This block needs properly indenting please ;) Adding a follow-up - was this what you were thinking about? Created attachment 56568 [details] [review] Bug 5260: Follow-up - Reindenting POD of SendAlerts Is there a good reason not to add the new ACQORDER notice? (In reply to Jonathan Druart from comment #44) > Is there a good reason not to add the new ACQORDER notice? Without the letter created I get "Order e-mail was sent to the vendor." on the interface, but No orderacquisition ACQORDER letter transported by email at /home/koha/src/C4/Letters.pm line 689. in the logs. Created attachment 56679 [details] [review] Bug 5260: QA follow-up: Fix error when no notice template is defined When no notice template ACQORDER was defined, you'r receive a false positive "email sent" message. Now it will display a specific error message instead. Also includes 2 unit tests to test for the warn and new error code. Hi Jonathan,
thx for the review and catching this! I have (hopefully) fixed the problem. Can you please take a look at the follow-up patch?
> Is there a good reason not to add the new ACQORDER notice?
No good reason really, more an oversight! I will work on another follow-up covering it.
Created attachment 56680 [details] [review] Bug 5260: QA follow-up - Add sample ACQORDER notice to sample notices Adds the new notice ACQORDER to all web installers. Verify by testing the SQL contains no errors and by running: perl xt/sample_notices.t NOTE: - de-DE is already translated, others are in English. - xt/sample_notices.t doesn't check fr-CA yet. I didn't fix this here as the file is missing other notices as well which should be fixed separately. Created attachment 56686 [details] [review] Bug 5260: make tests pass even without patron cat 'PT' Created attachment 56687 [details] [review] Bug 5260: Insert the new letter for existing installs Created attachment 56688 [details] [review] Bug 5260: simplify script and error handling No need to redirect, just sent the params to the template directly Created attachment 56689 [details] [review] Bug 5260: Use post params instead of get to avoid putting op=email in the browser history Katrin, Could you have a look at the 4 patches please? They should not introduce any regression. I am wondering if we should not add a confirmation box as the button will sent an email. I also think that the button should not be displayed if the vendor does not have any email address defined, don't you think? (In reply to Jonathan Druart from comment #53) > Katrin, > Could you have a look at the 4 patches please? They should not introduce any > regression. > I am wondering if we should not add a confirmation box as the button will > sent an email. > I also think that the button should not be displayed if the vendor does not > have any email address defined, don't you think? Hi Jonathan, thx for the follow-ups! I will test them later, but they all look good to me and make sense. I was modeling this from the way we send e-mails for claims. I am not against a confirmation box, I can try to take a look at how this is done on other pages. Arguments for showing the button: The user gets an informative message, that they need to edit a vendor contact in order to make it work. Also: I am not sure how to achieve hiding it correctly :( The logic here is not as simple as checking for an email address in the vendor, as there can be multiple contacts and the SendAlerts give priority to the one that is acqprimary and has the right option checked (Take a look at the SQL in SendAlerts). Created attachment 56705 [details] [review] [SIGNED OFF] Bug 5260: make tests pass even without patron cat 'PT' Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 56706 [details] [review] [SIGNED OFF] Bug 5260: Insert the new letter for existing installs Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 56707 [details] [review] [SIGNED OFF] Bug 5260: simplify script and error handling No need to redirect, just sent the params to the template directly Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 56708 [details] [review] [SIGNED OFF] Bug 5260: Use post params instead of get to avoid putting op=email in the browser history Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 56709 [details] [review] Bug 5260: QA follow-up - Ask for confirmation before sending e-mail With this patch there will be a confirmation alert when clicking on 'E-mail order'. Hi Jonathan, I have added a patch asking for confirmation, but honestly.. I am not persuaded by it. I think this will get annoying quite fast when you are working with the acquisition module on a daily basis. Also: we don't ask for confirmation for sending out a claim email neither in serials nor acquisitions. Created attachment 56786 [details] [review] Bug 5260 - Add option to send an order by e-mail to the acquisition module With this patch it will be possible to send order information to the vendor by e-mail. For now this feature can be triggered manually with a button before closing the basket. The order e-mail is based on the acquisition claim feature, but uses a new notice template. Test plan: 1) Vendors A new checkbox "Contact when ordering?" was added to the vendor page. - Add a vendor and/or edit an existing vendor - Verify the new option is saved correctly - Verify the new option displays on the vendor summary page after saving 2) Notices The feature works with a new notice template: ACQORDER It works with the same formatting/fields etc. as the acq claim notice. - Add a new notice template ACQORDER in module 'Claim/order aquisition' - Make sure to use fields from the various offered tables in your notice - Verify it is saved correctly 3) Basket - Turn on LetterLog system preference - Create multiple order lines - Click the 'Send order' button in the toolbar - Verify error or success message - Verify you received the e-mail - Verify there is a new entry with about the sent notice in your action_logs table 4) Regression testing... - Verify order claims still work - Verify serial claims still work - Verify new serial issue notices still work ... (I can provide additional test plans if needed) Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 56787 [details] [review] Bug 5260: Unit tests for new functionality in SendAlert Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 56788 [details] [review] Bug 5260 - Schema changes Schema changes for new column in the aqcontacts table. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 56789 [details] [review] Bug 5260: Follow-up - Reindenting POD of SendAlerts Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 56790 [details] [review] Bug 5260: QA follow-up: Fix error when no notice template is defined When no notice template ACQORDER was defined, you'r receive a false positive "email sent" message. Now it will display a specific error message instead. Also includes 2 unit tests to test for the warn and new error code. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 56791 [details] [review] Bug 5260: QA follow-up - Add sample ACQORDER notice to sample notices Adds the new notice ACQORDER to all web installers. Verify by testing the SQL contains no errors and by running: perl xt/sample_notices.t NOTE: - de-DE is already translated, others are in English. - xt/sample_notices.t doesn't check fr-CA yet. I didn't fix this here as the file is missing other notices as well which should be fixed separately. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 56792 [details] [review] Bug 5260: make tests pass even without patron cat 'PT' Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 56793 [details] [review] Bug 5260: Insert the new letter for existing installs Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 56794 [details] [review] Bug 5260: simplify script and error handling No need to redirect, just sent the params to the template directly Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 56795 [details] [review] Bug 5260: Use post params instead of get to avoid putting op=email in the browser history Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> (In reply to Katrin Fischer from comment #60) > Hi Jonathan, I have added a patch asking for confirmation, but honestly.. I > am not persuaded by it. > I think this will get annoying quite fast when you are working with the > acquisition module on a daily basis. Also: we don't ask for confirmation for > sending out a claim email neither in serials nor acquisitions. Agreed, I obsoleted it. Pushed to master for 16.11, thanks Katrin, Jonathan! Katrin, could you have a look, it seems that the tests are failing now Which tests does it break? t/db_dependent/Letters.t? (In reply to Katrin Fischer from comment #74) > Which tests does it break? t/db_dependent/Letters.t? Yes. Ok, I will try... :( I think I need help to fix the tests. :( My guess right now is that the "from" address, which for claims and orders is the email of the librarian sending it, is somehow generated and not valid - which could break the tests like that: # found carped warning: Bad or missing From address: '' found carped warning: Bad or missing From address: 'SSi32Vsm4u' The tests did run nicely when I was developing this, I haven't been able to locate when and why they broke :( New feature, skipping for 16.05.x |