Bug 13040 - Improve exporter to allow multiple branch selections
Summary: Improve exporter to allow multiple branch selections
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Robin Sheat
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 14709
  Show dependency treegraph
 
Reported: 2014-10-08 06:12 UTC by Robin Sheat
Modified: 2016-06-21 21:36 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 13040 - multiple branch selections in exporter (7.99 KB, patch)
2014-10-08 06:17 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 13040 - multiple branch selections in exporter (7.99 KB, patch)
2015-01-05 01:29 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 13040 - multiple branch selections in exporter (9.62 KB, patch)
2015-01-05 04:25 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 13040 - multiple branch selections in exporter (9.67 KB, patch)
2015-01-15 03:27 UTC, Thomas Wright
Details | Diff | Splinter Review
[PASSED QA] Bug 13040 - multiple branch selections in exporter (9.74 KB, patch)
2015-01-23 13:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13040 [QA Followup] - Fix koha-qa.pl issues (3.75 KB, patch)
2015-01-23 13:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13040 [Proposed Followup] - Use multi-select (2.34 KB, patch)
2015-01-23 14:07 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13040 [CSS follow-up] Improve exporter to allow multiple branch selections (1.10 KB, patch)
2015-01-23 15:11 UTC, Owen Leonard
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Robin Sheat 2014-10-08 06:12:05 UTC
At the moment you can only select all branches or one branch for export. This will add the ability to select a set of branches for export.

By default all branches will be selected.
Comment 1 Robin Sheat 2014-10-08 06:17:02 UTC Comment hidden (obsolete)
Comment 2 Paola Rossi 2014-10-22 14:35:49 UTC
I've applied the patch against master 3.17.00.029

The test plan is OK.

But in case I filled the "Don't export fields" field (f.e. writing 650), either by the "File format" marc or by the "File format" xml koha wrote in the output file:

<h1>Software error:</h1>
<pre>Arguments must be MARC::Field object at [....]/tools/export.pl line 395
</pre>
<p>
For help, please send mail to the webmaster (<a href="mailto:webmaster@linux-test">webmaster@linux-test</a>), giving this error message 
and the time and date of the error.

</p>

On the contrary, master is OK on this case.

So I pass the patch to "Failed QA" status.
Comment 3 Robin Sheat 2014-10-23 00:57:18 UTC
(In reply to Paola Rossi from comment #2)
> <pre>Arguments must be MARC::Field object at [....]/tools/export.pl line 395
> </pre>

This actually looks like an error in general. I have tested it against master and can reproduce it (though on current master it's line 396.)

Looking at the code, this can't work:

if ( $f =~ m/^(\d{3})(.)?$/ ) {
    my ( $field, $subfield ) = ( $1, $2 );

    # skip if this record doesn't have this field
    if ( defined $record->field($field) ) {
        if ( defined $subfield ) {
            my @tags = $record->field($field);
            foreach my $t (@tags) {
                $t->delete_subfields($subfield);
            }
        }
        else {
            $record->delete_fields($field);
        }
    }
}

where the 'delete_fields' line is the one causing the error. It can't take a tag like the regex pulls out, it must take a MARC::Field. So, I'm going to put this back to Needs Signoff as this is an unrelated bug.
Comment 4 Robin Sheat 2014-10-23 00:59:03 UTC
Bug 12860 covers it.
Comment 5 Paola Rossi 2014-10-23 16:08:47 UTC
Thanks, Robin Sheat, for the answer in comment 3. I've made some further tests. I agree with you: the problem is the same on MASTER.
And the error managed by Bug 12860 happens when the MARC field XXX occurs at least in one of the records selected for the export. Otherwise, no abort occurs.

Today I've applied the patch against master 3.17.00.030.
I've found the following problems.

1) I selected no branch [so all branches were selected]. Then I've selected a value for the two input fields:
From item call number
To item call number

Koha has answered only about the current selected library, not about all the libraries as required.

2) > By default all branches will be selected.
No, the default was the current selected library (that I could uncheck, anyway).

3) There were some records biblio in my DB, all the biblios had at least one item. Exporting all records [no branch checked, item type ALL, the other input fields empty] was OK. Then I catalogued a new biblio record [Default framework] without items. And then I edited an old item changing the barcode, inventory and something else. 
Exporting in the same way as above, not all the records were exported, but only some of them: in particular, the biblio record of the changed item was not exported.
[I went back to master on the same DB. Exporting [ALL Library selected, item type ALL, the other input fields empty], all the records were exported: the biblio record without items too, as well as the biblio record whose item I changed.]

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

NB-1. On the template of export, "Accession date" is shown, but the field is shown as the item's "d - Date acquired" on CAT.
NB-2. If I didn't selected "From biblio number" and "To biblio number", in some cases the export file is not sorted on biblio number. This seemed strange to me: the sort of the export file could be an important information for me, but I didn't recognize what it was.
NB-3. If no record was selected, an output file was created, empty. Perhaps the answer could be given to the user in intranet.
Comment 6 Paola Rossi 2014-12-23 15:49:49 UTC
I've tried to apply the patch against master 3.19.00.002

Applying: Bug 13040 - multiple branch selections in exporter
Using index info to reconstruct a base tree...
<stdin>:28: new blank line at EOF.
+
warning: 1 line adds whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging tools/export.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/tools/export.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/css/staff-global.css
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/css/staff-global.css
Failed to merge in the changes.
Patch failed at 0001 Bug 13040 - multiple branch selections in exporter

I pass the patch to "Patch doesn't apply" status.
Comment 7 Robin Sheat 2015-01-05 01:29:59 UTC Comment hidden (obsolete)
Comment 8 Robin Sheat 2015-01-05 03:50:57 UTC
(In reply to Paola Rossi from comment #5)
> 1) I selected no branch [so all branches were selected]. Then I've selected
> a value for the two input fields:
> From item call number
> To item call number
> 
> Koha has answered only about the current selected library, not about all the
> libraries as required.

Confirmed. This is an annoying interaction with the "strip non-local items" option. Going to have to think about how this should work for a minute.
 
> 2) > By default all branches will be selected.
> No, the default was the current selected library (that I could uncheck,
> anyway).

Confirmed. Turns out it was something that differed from using the database user vs. a real one, but it should behave like the old version which is to select "all" by default.

Haven't looked at the other things yet.
Comment 9 Robin Sheat 2015-01-05 04:23:06 UTC
(In reply to Paola Rossi from comment #5)
> 3) There were some records biblio in my DB, all the biblios had at least one
> item. Exporting all records [no branch checked, item type ALL, the other
> input fields empty] was OK. Then I catalogued a new biblio record [Default
> framework] without items. And then I edited an old item changing the
> barcode, inventory and something else. 
> Exporting in the same way as above, not all the records were exported, but
> only some of them: in particular, the biblio record of the changed item was
> not exported.
> [I went back to master on the same DB. Exporting [ALL Library selected, item
> type ALL, the other input fields empty], all the records were exported: the
> biblio record without items too, as well as the biblio record whose item I
> changed.]

I can't confirm this. I did these steps, but my edited item was exported correctly, along with its biblio.

> NB-2. If I didn't selected "From biblio number" and "To biblio number", in
> some cases the export file is not sorted on biblio number. This seemed
> strange to me: the sort of the export file could be an important information
> for me, but I didn't recognize what it was.

No ordering is provided to the SQL query, so it'll be whatever order the database responds in I think. This'll normally be the same order as the records were added, but that's not a guarantee.
Comment 10 Robin Sheat 2015-01-05 04:25:43 UTC Comment hidden (obsolete)
Comment 11 Robin Sheat 2015-01-05 04:26:37 UTC
This changes the behaviour slightly, I'm not sure if that'll be an issue or not. I'd appreciate a double-check on that.
Comment 12 Thomas Wright 2015-01-15 03:27:31 UTC Comment hidden (obsolete)
Comment 13 Jonathan Druart 2015-01-15 11:21:35 UTC
Don't you think a select multiple would be more adapted?
I cc Owen to get his opinion on this.
Comment 14 Robin Sheat 2015-01-15 12:18:32 UTC
No one knows that control+click is a thing you can do.
Comment 15 Jonathan Druart 2015-01-15 12:28:01 UTC
Have a look at the plugin I used for the OpacAdvSearchMoreOptions pref.
Comment 16 Jonathan Druart 2015-01-15 12:31:28 UTC
Robin, we have a customer with 42 branches (yes it's the right number).
I am not sure the checkboxes will be display correctly with as many branches.
Comment 17 Kyle M Hall 2015-01-23 12:55:37 UTC
 FAIL	koha-tmpl/intranet-tmpl/prog/en/includes/branch-selector.inc
   OK	  forbidden patterns
   FAIL	  tt_valid
		lines 12
   OK	  valid_template

 FAIL	koha-tmpl/intranet-tmpl/prog/en/modules/help/tools/export.tt
   FAIL	  forbidden patterns
		forbidden pattern: tab char (line 26)
		forbidden pattern: tab char (line 18)
   OK	  tt_valid
   OK	  valid_template
Comment 18 Kyle M Hall 2015-01-23 13:47:56 UTC
Created attachment 35505 [details] [review]
[PASSED QA] Bug 13040 - multiple branch selections in exporter

This allows the exporter (Tools -> Export) to have any combination of
branches selected, rather than it being all or only one.

Test Plan:
* Apply the patch
* Go to the exporter, see that instead of a dropdown you now have an
  elegently laid out grid of branches you can select from
* Select some branches, run the export
* Note that only records with items in the selected branches are
  returned.
* Repeat this with the item related options (as that code was refactored
  slightly) and make sure everything is sane.

Sponsored-By: South Taranaki District Libraries
Signed-off-by: Thomas <tomsStudy@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 19 Kyle M Hall 2015-01-23 13:48:05 UTC
Created attachment 35506 [details] [review]
Bug 13040 [QA Followup] - Fix koha-qa.pl issues

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 20 Kyle M Hall 2015-01-23 13:50:40 UTC
Tried using the multi-select, but it doesn't seem to be able to handle the pulldown stylistically. Also, given the number of possible options, the pulldown would fall far off the page. I haven't tested it with 40+ branches, but I'm afraid some browsers would stop the scrolldown from even making it the end of the list ( i.e. the bottom would fall off the visible page )!
Comment 21 Kyle M Hall 2015-01-23 14:07:31 UTC Comment hidden (obsolete)
Comment 22 Owen Leonard 2015-01-23 15:11:17 UTC
Created attachment 35510 [details] [review]
Bug 13040 [CSS follow-up] Improve exporter to allow multiple branch selections

This patch adds a little bit of CSS to style the branch checkbox boxes.
I hope that this helps the readability, especially in systems with a
large number of branches.
Comment 23 Tomás Cohen Arazi 2015-01-26 19:36:24 UTC
Patches pushed to master.

Thanks Robin!
Comment 24 Robin Sheat 2015-01-26 21:42:34 UTC
Thanks for the prettying, Owen.