This patch doesn't apply anymore on HEAD.

Adding bug 2894 to this - reading the description of this feature I hope it will fix the broken holds feature.

updatedatabase.pl can't create the two required new tables.

I get for example this error message: 

Can't create table './koha_devel/subscriptionroutinglist.frm' (errno: 150)

The above error arise because the new table and the existing one don't
have the same engine: InnoDB for the former, and MyISAM for the old.

This could be a solution:

 ALTER TABLE borrowers ENGINE = InnoDB;
 ALTER TABLE subscription ENGINE = InnoDB;

But I'm not sure it is good to force to InnoDB tables which may have
been in MyISAM for good reasons.

Another solution could be to modify updatedatabase.pl to set the new
table to the same engine as the related tables.

You can get it with:

  SELECT ENGINE
    FROM information_schema.TABLES
   WHERE TABLE_SCHEMA = DATABASE()
     AND TABLE_NAME = 'subscription'

Frédéric, in kohastructure.sql, the only table which use MyISAM engine is pending_offline_operations.
In my databases, subscription and borrowers table both use InnoDB, so why they don't in your database?

> Frédéric, in kohastructure.sql, the only table which use MyISAM engine
> is pending_offline_operations.  In my databases, subscription and
> borrowers table both use InnoDB, so why they don't in your database?

Good question. Since it's a library currently using Koha database, I'm
sure this issue will occur in real life for others. It means that your
enhancement won't work at all for some (a lot) of libraries. Maybe this
question could be raised on koha-devel list? pointing to this bug.

Engine used for all table:

    SELECT TABLE_NAME, ENGINE 
      FROM information_schema.TABLES
     WHERE TABLE_SCHEMA = DATABASE()

I have a lot of Koha DB mixing MyISAM and InnoDB. Why? I have no clue.

(In reply to comment #8)
> > Frédéric, in kohastructure.sql, the only table which use MyISAM engine
> > is pending_offline_operations.  In my databases, subscription and
> > borrowers table both use InnoDB, so why they don't in your database?
> 
> Good question. Since it's a library currently using Koha database, I'm
> sure this issue will occur in real life for others. It means that your
> enhancement won't work at all for some (a lot) of libraries. Maybe this
> question could be raised on koha-devel list? pointing to this bug.
> 
> Engine used for all table:
> 
>     SELECT TABLE_NAME, ENGINE 
>       FROM information_schema.TABLES
>      WHERE TABLE_SCHEMA = DATABASE()
> 
> I have a lot of Koha DB mixing MyISAM and InnoDB. Why? I have no clue.

Maybe that's because, during the upgrade from 2.x to 3.0, you missed some message speaking of a problem ? Because the update22to30.pl script update all tables to innoDB.
    # MOVE all tables TO UTF-8 and innoDB
    $sth = $dbh->prepare("show table status");
    $sth->execute;
    while ( my $table = $sth->fetchrow_hashref ) {
        next if $table->{Name} eq 'marc_word';
        next if $table->{Name} eq 'marc_subfield_table';
        next if $table->{Name} eq 'auth_word';
        next if $table->{Name} eq 'auth_subfield_table';
        if ($table->{Engine} ne 'InnoDB') {
            print "moving $table->{Name} to InnoDB\n";
            $dbh->do("ALTER TABLE $table->{Name} ENGINE = innodb");
        }
        unless ($table->{Collation} =~ /^utf8/) {
            print "moving $table->{Name} to utf8\n";
            $dbh->do("ALTER TABLE $table->{Name} CONVERT TO CHARACTER SET utf8");
            $dbh->do("ALTER TABLE $table->{Name} DEFAULT CHARACTER SET utf8 COLLATE utf8_general_ci");
            # FIXME : maybe a ALTER TABLE tbl_name CONVERT TO CHARACTER SET utf8 would be better, def char set seems to work fine. If any problem encountered, let's try with convert !
        } else {
        }
    }

If you have such problems in your database, the good option is to write a small script in misc/maintenance, that fixes the problem.
It's not the responsibility of this patch, imo, to fix it.

Something went wrong !
Applying: Bug 7957: Routing lists enhancements
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging C4/Items.pm
Auto-merging C4/Serials.pm
Auto-merging installer/data/mysql/kohastructure.sql
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/includes/serials-menu.inc
Auto-merging koha-tmpl/intranet-tmpl/prog/en/includes/serials-toolbar.inc
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/includes/serials-toolbar.inc
Removing koha-tmpl/intranet-tmpl/prog/en/modules/serials/routing-preview.tt
CONFLICT (delete/modify): koha-tmpl/intranet-tmpl/prog/en/modules/serials/routing.tt deleted in Bug 7957: Routing lists enhancements and modified in HEAD. Version HEAD of koha-tmpl/intranet-tmpl/prog/en/modules/serials/routing.tt left in tree.
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-collection.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-collection.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-search.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-search.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-detail.tt
Removing serials/reorder_members.pl
Removing serials/routing-preview.pl
Removing serials/routing.pl
Auto-merging serials/serials-collection.pl
Auto-merging serials/serials-search.pl
Auto-merging serials/subscription-detail.pl
Failed to merge in the changes.
Patch failed at 0001 Bug 7957: Routing lists enhancements
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".
Bug 7957 - Routing lists: manage several routing list for each subscription, and export them as CSV

Bug 7957: Routing lists enhancements
Apply? [yn] 
Patch left in /tmp/Bug-7957-Routing-lists-enhancements-8Hq0be.patch

Patch rebased on master

Applying: Bug 7957: Routing lists enhancements
Using index info to reconstruct a base tree...
M	C4/Items.pm
M	C4/Serials.pm
M	installer/data/mysql/kohastructure.sql
M	installer/data/mysql/updatedatabase.pl
M	koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-collection.tt
M	koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-search.tt
M	koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-detail.tt
M	serials/member-search.pl
M	serials/routing-preview.pl
M	serials/serials-collection.pl
M	serials/serials-search.pl
M	serials/subscription-detail.pl
A	t/db_dependent/lib/KohaTest/Serials.pm
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): t/db_dependent/lib/KohaTest/Serials.pm deleted in HEAD and modified in Bug 7957: Routing lists enhancements. Version Bug 7957: Routing lists enhancements of t/db_dependent/lib/KohaTest/Serials.pm left in tree.
Auto-merging serials/subscription-detail.pl
Auto-merging serials/serials-search.pl
Auto-merging serials/serials-collection.pl
Removing serials/routing.pl
CONFLICT (modify/delete): serials/routing-preview.pl deleted in Bug 7957: Routing lists enhancements and modified in HEAD. Version HEAD of serials/routing-preview.pl left in tree.
Removing serials/reorder_members.pl
Auto-merging serials/member-search.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-detail.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-search.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-collection.tt
Removing koha-tmpl/intranet-tmpl/prog/en/modules/serials/routing.tt
Removing koha-tmpl/intranet-tmpl/prog/en/modules/serials/routing-preview.tt
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/kohastructure.sql
Auto-merging C4/Serials.pm
Auto-merging C4/Items.pm
Patch failed at 0001 Bug 7957: Routing lists enhancements
The copy of the patch that failed is found in:
   /home/christopher/git/koha/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Patch rebased on master

Patch applied cleanly, go forth and signoff

Applying: Bug 7957: Routing lists enhancements
Using index info to reconstruct a base tree...
M	C4/Items.pm
M	C4/Serials.pm
M	installer/data/mysql/kohastructure.sql
M	installer/data/mysql/updatedatabase.pl
M	koha-tmpl/intranet-tmpl/prog/en/modules/serials/routing.tt
M	koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-collection.tt
M	koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-search.tt
M	serials/routing.pl
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): serials/routing.pl deleted in Bug 7957: Routing lists enhancements and modified in HEAD. Version HEAD of serials/routing.pl left in tree.
Removing serials/routing-preview.pl
Removing serials/reorder_members.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-search.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-collection.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-collection.tt
CONFLICT (modify/delete): koha-tmpl/intranet-tmpl/prog/en/modules/serials/routing.tt deleted in Bug 7957: Routing lists enhancements and modified in HEAD. Version HEAD of koha-tmpl/intranet-tmpl/prog/en/modules/serials/routing.tt left in tree.
Removing koha-tmpl/intranet-tmpl/prog/en/modules/serials/routing-preview.tt
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/kohastructure.sql
Auto-merging C4/Serials.pm
Auto-merging C4/Items.pm
Patch failed at 0001 Bug 7957: Routing lists enhancements
The copy of the patch that failed is found in:
   /home/christopher/git/koha/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

I've tried to apply the patch.

Applying: Bug 7957: Routing lists enhancements
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging serials/subscription-detail.pl
Auto-merging serials/serials-search.pl
CONFLICT (content): Merge conflict in serials/serials-search.pl
Auto-merging serials/serials-collection.pl
CONFLICT (modify/delete): serials/routing.pl deleted in Bug 7957: Routing lists enhancements and modified in HEAD. Version HEAD of serials/routing.pl left in tree.
CONFLICT (modify/delete): serials/routing-preview.pl deleted in Bug 7957: Routing lists enhancements and modified in HEAD. Version HEAD of serials/routing-preview.pl left in tree.
Removing serials/reorder_members.pl
Auto-merging serials/member-search.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-search.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-search.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-collection.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-collection.tt
Removing koha-tmpl/intranet-tmpl/prog/en/modules/serials/routing.tt
Removing koha-tmpl/intranet-tmpl/prog/en/modules/serials/routing-preview.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/includes/serials-toolbar.inc
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/kohastructure.sql
Auto-merging C4/Serials.pm
Auto-merging C4/Items.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 7957: Routing lists enhancements

So I pass the patch to "Patch doesn't apply" status.

Thanks, Julian.

I've applied the patch against master 3.19.00.010 head 13675.

On /cgi-bin/koha/serials/subscription-detail.pl I've selected the
"Create routing list" [/ "Edit routing list"]
at the left of the page.
I've received the "Create routing list" [/ "Routing list for ...."] page.

In both the 2 cases, then I've selected :
"Serial collection" at the left of this page.
I saw:

<h1>Software error:</h1>
<pre>Undefined subroutine &amp;main::check_routing called at /[....]/serials/serials-collection.pl line 132.
</pre>

So I pass the patch to "Failed QA" status.


NB-1) The patch needed a trivial rebase.

NB-2) Step 1:
Make a search, in the results list you have a column 'Routing lists'
    (if not, consider setting RoutingSerials syspref). If the
    subscription doesn't have a routing list, you should have a 'New'
    link. Otherwise you have an 'Edit(X)' link, where X is the number of
    routing lists.

RoutingSerials "Use"/"Don't use" is OK as required.
But at present the "column" is an "Actions".
Could you update the test plan please?
[A numbered indentation would be very helpful to me in this long test plan.]

Thank you very much, Julian.

I've applied the patches against master 3.19.00.010 head 13240.

The sw error in comment #22 is OK!

About:
>Run the updatedatabase.pl script
>Existing routing lists should have been imported in a routing list named
'import'

All existing routing lists were named "import", as required.
But the note field of the existing RLs were all lost.

So I pass the patch to "Failed QA" status.

I was not aware of this note field.
With this patch it should be retrieved during the updatedatabase process.

Thanks, Julian.

I've applied the 3 patches against master 3.19.00.010.

The existing imported-RLs' notes were present, as expected.

On master, on Patrons the tag “Routing lists” of a patron lists the subscriptions’
 RL-s the patron-borrower is in.

On the contrary, after having applied the patches, the tag “Routing lists” of a patron lists no (neither new nor imported) RL-s the patron is in:

0 Subscription routing lists
Patron does not belong to any subscription routing lists.

I think this could be an error.

For now I pass the patch to "Failed QA".

Thanks for your comments Paola, I didn't even know this page.

Thanks, Julian.

[And the name of each RL is shown in the RL-s' table, added against master!]

I was trying to "Add a borrower" to a RL.
On /cgi-bin/koha/serials/member-search.pl, I:

A) have selected a Library and a Category, and these input values have been crossed in AND [OK]:
Library AND Category

B) have selected a Library, a Category and a Name, and these input values have been crossed in (?):
Name OR (Library AND Category)
Name OR Library
Name OR Category

while I was expecting
Name AND Library AND Category
Name AND Library
Name AND Category

I think this could be an error.

So I pass to "Failed QA" status.

NB-1) On /cgi-bin/koha/serials/member-search.pl, I selected a library, and the library [code] appeared in the table result, and IMO this is a benefit for the user.
On the contrary, if I selected a category, the category didn't appear in the table result. Perhaps it could be added?

NB-2) On /cgi-bin/koha/serials/routinglists.pl, the RL is shown to be:
Routing lists for X,
where X is the serial-bibliographic-record's title.
But the RL is about 1 subscription, and a serial-biblio-record can have some subscriptions.
In other parts of koha [serials/serials-collection.pl], koha shows the serial-bibliographic-record title and the "Subscription num." #numbercode's link.
Perhaps it could be helpful.

This search generates a quite long SQL query: http://pastebin.com/qYULAHsm where the search is done on firstname, surname, email, othernames and cardnumber.
But basically you're right, it does something like (Name OR (Library AND Category))
I don't know if this is intended or not, and as the behaviour is the same as master, I suggest to open a new bug for that.

(In reply to Julian Maurice from comment #34)
> This search generates a quite long SQL query: http://pastebin.com/qYULAHsm
> where the search is done on firstname, surname, email, othernames and
> cardnumber.
> But basically you're right, it does something like (Name OR (Library AND
> Category))
> I don't know if this is intended or not, and as the behaviour is the same as
> master, I suggest to open a new bug for that.

I opened the new bug 13716 on this topic.

I know this is old code, so not sure about DBIX or use of the new Koha Object, but quite sure we should have tests! These patches only remove a few tests, but none are added for the new module.

Julian, please rebase, I will try to have a look.

On updating the DB, I got several lines of this type:

DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha`.`subscriptionrouting`, CONSTRAINT `subscriptionrouting_ibfk_2` FOREIGN KEY (`borrowernumber`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE CASCADE) at /home/koha/src/installer/data/mysql/atomicupdate/bug_7957.perl line 62.

DBD::mysql::st execute failed: Duplicate entry '2-7363' for key 'PRIMARY' at /home/koha/src/installer/data/mysql/atomicupdate/bug_7957.perl line 62.

Any ideas of why?

MariaDB [koha]> select * from subscriptionroutinglist;
+-----------+----------------+---------+----------------+
| routingid | borrowernumber | ranking | subscriptionid |
+-----------+----------------+---------+----------------+
[SKIP]
|       195 |           7874 |      38 |              2 |
|       196 |           7874 |      39 |              2 |
[SKIP]

I don't know if it's a DB data error or if it comes from the updatedb entry.

It seems you have a problem with your database.
On master, subscriptionroutinglist has UNIQUE (`subscriptionid`, `borrowernumber`) so you shouldn't be able to have 2 rows with same borrowernumber and subscriptionid.
Also, subscriptionroutinglist has a foreign key constraint on borrowernumber; the "Cannot add or update a child row" error should not happen

I could "hide" those errors by adding IGNORE to the INSERT query, but I don't know if it's the best thing to do.

Conflicts with bug 9809.
Don't forget to uptade AddReserve calls.

Applying: Bug 7957: Routing lists enhancements
fatal: sha1 information is lacking or useless (C4/Items.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 7957: Routing lists enhancements

Patches rebased on master

(In reply to Aleisha Amohia from comment #63)
> Created attachment 47664 [details] [review] [review]
> [SIGNED-OFF] Bug 7957: Redirect after creating a new routing list
> 
> This avoids creating the same list multiple times when reloading the
> page
> 
> Signed-off-by: Aleisha <aleishaamohia@hotmail.com>
> 
> Have made a couple of small changes to your patch.
> 1) "There is no routing lists for this subscription" -> changed to "There
> are no"
> 2) When I click Preview and choose a serial (up to Step 9 in test plan),
> there is a link to another system preference which I assume$
...which I assume is where the text comes from. Have changed this in syspref.sql to take the <a> tags out around the syspref because html tags won't show up nicely in a sql file

This is a bug with a long history :( And it currently doesn't apply, please fix:

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 7957: Routing lists enhancements
Using index info to reconstruct a base tree...
M	C4/Items.pm
M	installer/data/mysql/kohastructure.sql
M	koha-tmpl/intranet-tmpl/prog/en/modules/serials/routing-preview-slip.tt
M	koha-tmpl/intranet-tmpl/prog/en/modules/serials/routing-preview.tt
M	koha-tmpl/intranet-tmpl/prog/en/modules/serials/routing.tt
M	koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-collection.tt
M	koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-search.tt
M	members/routing-lists.pl
Falling back to patching base and 3-way merge...
Removing serials/routing.pl
Removing serials/routing-preview.pl
Removing serials/reorder_members.pl
Auto-merging members/routing-lists.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-search.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-collection.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-collection.tt
CONFLICT (modify/delete): koha-tmpl/intranet-tmpl/prog/en/modules/serials/routing.tt deleted in Bug 7957: Routing lists enhancements and modified in HEAD. Version HEAD of koha-tmpl/intranet-tmpl/prog/en/modules/serials/routing.tt left in tree.
CONFLICT (modify/delete): koha-tmpl/intranet-tmpl/prog/en/modules/serials/routing-preview.tt deleted in Bug 7957: Routing lists enhancements and modified in HEAD. Version HEAD of koha-tmpl/intranet-tmpl/prog/en/modules/serials/routing-preview.tt left in tree.
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/serials/routing-preview-slip.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/serials/routing-preview-slip

Created attachment 50226 [details] [review]
Bug 7957: Routing lists enhancements

- Manage several routing list per subscription
- Export a routing list as CSV

Test plan:

Run the updatedatabase.pl script
Existing routing lists should have been imported in a routing list named
'import'

From the serials-home.pl page:
  1 Make a search, in the results list you have a column 'Actions' that
    should contain a "New/Edit routing list" link" (if not, consider
    setting RoutingSerials syspref). If the subscription doesn't have a
    routing list, you should have a 'New routing list' link. Otherwise
    you have an 'Edit routing lists (X)' link, where X is the number of
    routing lists.
  2 Choose a subscription which have no routing lists and click on 'New
    routing list'
  3 You are on the routing list creation page. Enter a title for the
    routing list and click on 'Save'.
  4 The routing list has been created and now you are on the routing
    list modification page. The behaviour of this page is quite the same
    as before. A sensible change is that reordering borrowers doesn't
    reload the page each time you make a change.
  5 Click on 'Save' to finish routing list modifications.
  6 Now you are on the routing lists management page. You can edit a
    routing list by clicking on its title, preview it, export it, and
    delete it by clicking on the corresponding link.
  7 Click on 'Preview'.
  8 You will be asked to choose a serial. Pick the one you want to print
    the routing list for and click on 'Continue'.
  9 If RoutingListAddReserves is OFF, the routing list is displayed and
    you just have to click on 'Print'. If the syspref is ON, you will
    have to click on 'Confirm and print', an alert will show telling you
    that holds will be placed on the serial you chose. Click 'OK' and
    you will be able to print the slip. Now check if the holds have been
    placed. (It works only if an item is attached to this serial).
 10 Go back to the routing lists management page (routinglists.pl) and
    click on 'Export'. Download the CSV and check that it contains
    correct data.

From serials-collection.pl page:
 11 In the serials list, click on 'Print list'.
 12 You will be asked to choose a routing list. Pick one and click on
    'Continue'.
 13 ... (same behaviour as above)

Try to add and delete routing lists to show if links are correctly
updated in serials-home.pl, serials-collection.pl and
subscription-detail.pl

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Signed-off-by: Aleisha <aleishaamohia@hotmail.com>

Created attachment 50227 [details] [review]
Bug 7957: Add unit tests for new subroutines

Signed-off-by: Aleisha <aleishaamohia@hotmail.com>

Created attachment 50228 [details] [review]
Bug 7957: Redirect after creating a new routing list

This avoids creating the same list multiple times when reloading the
page

Signed-off-by: Aleisha <aleishaamohia@hotmail.com>

Have made a couple of small changes to your patch.
1) "There is no routing lists for this subscription" -> changed to "There are no"
2) When I click Preview and choose a serial (up to Step 9 in test plan), there is a link to another system preference which I assume$

Hi Julian,

there are a couple of problems here noted by he QA script:
- tabs
- license
- Name "Cache::RemovalStrategy::LRU::FIELDS" used only once: possible typo (can be ignored)

From looking at the code:

1) This is a translation issue - please take a look at how other CSV exports solve the problem.
+    my @headers = ("Subscription title", "Routing list", qw(Surname Firstname Notes));

2) Little typo: 
<p id="noborrowersp">There is no borrowers in this routing list.</p>
There are no... :)

3) How can you edit a routing list description? I tried to change the routing lists created by the update - but failed to find a link.

4) I can add patrons to the subscription list, but the borrowers don't save. Could you please check?

4) might be related to my database, but would be nice if you could verify if it works for you.

Created attachment 50265 [details] [review]
Bug 7957: QA fixes (tabs, POD, GPL version)

Created attachment 50266 [details] [review]
Bug 7957: Fix typo "There {is -> are} no borrowers"

(In reply to Katrin Fischer from comment #69)
> there are a couple of problems here noted by he QA script:
> - tabs
> - license
> - Name "Cache::RemovalStrategy::LRU::FIELDS" used only once: possible typo
> (can be ignored)
tabs and license fixed, but git grep 'Cache::RemovalStrategy::LRU::FIELDS' returns nothing. I don't know what this error is about.

> 
> From looking at the code:
> 
> 1) This is a translation issue - please take a look at how other CSV exports
> solve the problem.
> +    my @headers = ("Subscription title", "Routing list", qw(Surname
> Firstname Notes));
I have seen different ways of doing CSV export, but I assume you are talking about the one which use a .tt file.
Is this really necessary ? We could just wrap the strings into calls to Koha::I18N::gettext().
I didn't see much enthusiasm about having translatable strings in .pl/.pm (cf. bug 11904) but I think it's one of the cases that could greatly benefit from Koha::I18N.

> 3) How can you edit a routing list description? I tried to change the
> routing lists created by the update - but failed to find a link.
By 'description' do you mean the 'title' field ? If so, it's not editable at the time, but it's not really intended. It could be changed.

> 4) I can add patrons to the subscription list, but the borrowers don't save.
> Could you please check?
You mean you can add them in the interface, but when you click on 'Save' the list of borrowers is lost ? It works for me. Could you check the logs to see if you have any SQL error ?

Hi Julian,

the Cache::RemovalStrategy::LRU::FIELDS is a general problem right now and will be fixed with another patch (it's somewhere in the PQA queue).

I am pretty sure 4) was my problem - I will verify that with a new database.

By description I meant the field you fill out when creating a new routing list - the one that is set to "import" (would be nice to find something better) for routing lists you had before. I think it should absolutely be possible to edit that. You pick the routing list by that name from the list if you have more than one and libraries will want to use that to store information - fix typos, etc. - or just change the 'import' to something more meaningful.

Otherwise I thought this looked good so far - but I still have some things to test like the routing list holds (will be happy to see bug 2894 resolved!)

For the CSV:
1) the solution you propose will require a new po file to be set up on pootle - so I would not mix this change in here to avoid further discussion. We should resolve the question of using Koha:I18N separately. I'd like to see this bug resolved after more than 4 years! :)
2) there is an existing solution to make this work in other places - so that would be an easy solution for now. We could also move this to a separate bug report if totally necessary.

Created attachment 50270 [details] [review]
Bug 7957: Make routing list title editable

Created attachment 50273 [details] [review]
Bug 7957: Make CSV header translatable

(In reply to Katrin Fischer from comment #74)
> For the CSV:
> 1) the solution you propose will require a new po file to be set up on
> pootle - so I would not mix this change in here to avoid further discussion.
> We should resolve the question of using Koha:I18N separately. I'd like to
> see this bug resolved after more than 4 years! :)
> 2) there is an existing solution to make this work in other places - so that
> would be an easy solution for now. We could also move this to a separate bug
> report if totally necessary.

I'm wondering why Koha::I18N was accepted in the first place if nobody want to use it :)
But you are right about the pootle thing, so I used a .tt file for translatability

Resetting status to Signed off as these patches are minor changes

Ok, some hopefully last notes:
- I don't like "import" so much as the default name for the original routing list. What do you think of just naming it '1'?
- On the routing list preview the dateformat of the issue is wrong. I think this was fixed some time ago and the fix got undone by the patch accidentally.
- Without the patch it seems possible to print routing lists from the serial collection page without 'Routing' permission, with the patch, this is no longer possible. The Routing permission is not clear here, it just reads 'Routing' - so maybe to be sure we could try and avoid the behaviour change.

Something I think would be a good follow-up (unless it's easy to do):
- When you have only 1 routing list defined, it would be great if the 'Print list' link could take you directly to the preview/print screen as selecting/confirming from a list of 1 adds unnecessary additional clicks. I think this would make things more ergonomic and less of a change to libraries using the routing lists now.

>Something I think would be a good follow-up (unless it's easy to do):
I meant this could could possibly go on a separate bug - but would be nice to have.

Ah sorry, one last thing I wanted to fix in a QA follow-up: take care to use patrons in stead of borrowers in the GUI!

Holds will be placed for all borrowers in this list.
<th>No. of borrowers</th>

Looking at the patches, is the idea that you'd have a routing list per issue?

(In reply to David Cook from comment #81)
> Looking at the patches, is the idea that you'd have a routing list per issue?

Multiple routing lists per subscription.

Created attachment 97519 [details] [review]
Bug 7957: Routing lists enhancements

- Manage several routing list per subscription
- Export a routing list as CSV

Test plan:

Run the updatedatabase.pl script
Existing routing lists should have been imported in a routing list named
'import'

From the serials-home.pl page:
  1 Make a search, in the results list you have a column 'Actions' that
    should contain a "New/Edit routing list" link" (if not, consider
    setting RoutingSerials syspref). If the subscription doesn't have a
    routing list, you should have a 'New routing list' link. Otherwise
    you have an 'Edit routing lists (X)' link, where X is the number of
    routing lists.
  2 Choose a subscription which have no routing lists and click on 'New
    routing list'
  3 You are on the routing list creation page. Enter a title for the
    routing list and click on 'Save'.
  4 The routing list has been created and now you are on the routing
    list modification page. The behaviour of this page is quite the same
    as before. A sensible change is that reordering borrowers doesn't
    reload the page each time you make a change.
  5 Click on 'Save' to finish routing list modifications.
  6 Now you are on the routing lists management page. You can edit a
    routing list by clicking on its title, preview it, export it, and
    delete it by clicking on the corresponding link.
  7 Click on 'Preview'.
  8 You will be asked to choose a serial. Pick the one you want to print
    the routing list for and click on 'Continue'.
  9 If RoutingListAddReserves is OFF, the routing list is displayed and
    you just have to click on 'Print'. If the syspref is ON, you will
    have to click on 'Confirm and print', an alert will show telling you
    that holds will be placed on the serial you chose. Click 'OK' and
    you will be able to print the slip. Now check if the holds have been
    placed. (It works only if an item is attached to this serial).
 10 Go back to the routing lists management page (routinglists.pl) and
    click on 'Export'. Download the CSV and check that it contains
    correct data.

From serials-collection.pl page:
 11 In the serials list, click on 'Print list'.
 12 You will be asked to choose a routing list. Pick one and click on
    'Continue'.
 13 ... (same behaviour as above)

Try to add and delete routing lists to show if links are correctly
updated in serials-home.pl, serials-collection.pl and
subscription-detail.pl

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Signed-off-by: Aleisha <aleishaamohia@hotmail.com>

Created attachment 97520 [details] [review]
Bug 7957: Change name routing list created by updatedb

'import' -> '1'

Created attachment 97521 [details] [review]
Bug 7957: Fix date format in routing-preview-slip.pl

Created attachment 97522 [details] [review]
Bug 7957: Do not require 'routing' permission for preview

Anyone one with at least one serials permission should be able to see
the routing list preview

Created attachment 97523 [details] [review]
Bug 7957: Skip the routing list selection screen if not necessary

Created attachment 97524 [details] [review]
Bug 7957: Reword 'borrowers' to 'patrons'

Rebased/Squashed all patches on master + added small patches for the last QA comments.
Patches were quite old, so I think this bug should go back to 'Needs signoff' status.

Sorry, the patch no longer applies 8-(... 

Applying: Bug 7957: Routing lists enhancements
.git/rebase-apply/patch:146: trailing whitespace.
                 FROM     serial 
warning: 1 line adds whitespace errors.
Using index info to reconstruct a base tree...
M	C4/Serials.pm
M	installer/data/mysql/kohastructure.sql
M	serials/routing-preview.pl
.git/rebase-apply/patch:146: trailing whitespace.
                 FROM     serial 
warning: 1 line adds whitespace errors.
Falling back to patching base and 3-way merge...
Removing serials/routing.pl
CONFLICT (modify/delete): serials/routing-preview.pl deleted in Bug 7957: Routing lists enhancements and modified in HEAD. Version HEAD of serials/routing-preview.pl left in tree.
Removing serials/reorder_members.pl
Removing koha-tmpl/intranet-tmpl/prog/en/modules/serials/routing.tt
Removing koha-tmpl/intranet-tmpl/prog/en/modules/serials/routing-preview.tt
Auto-merging installer/data/mysql/kohastructure.sql
Auto-merging C4/Serials.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 7957: Routing lists enhancements

(In reply to Katrin Fischer from comment #82)
> Multiple routing lists per subscription.

What is the purpose/use of having multiple routing lists per subscription?

(In reply to David Cook from comment #91)
> (In reply to Katrin Fischer from comment #82)
> > Multiple routing lists per subscription.
> 
> What is the purpose/use of having multiple routing lists per subscription?

I think there are different use case. Some libraries don't want to use x subscriptions, while they still receive x issues for one. So having differnt routing lists could go on multiple issues of the same subscription.

In other cases libraries might want to keep the lists shorter / more anonymous, thus using different ones.