Bug 18201 - Export data -Fix "Remove non-local items" option and add "Removes non-local records" option for existing functionality
Summary: Export data -Fix "Remove non-local items" option and add "Removes non-local r...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-03-02 21:11 UTC by Nick Clemens
Modified: 2018-06-04 20:15 UTC (History)
7 users (show)

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


Attachments
Bug 18201 - Export data -Fix "Remove non-local items" option and add "Removes non-local records" option for existing functionality (3.48 KB, patch)
2017-04-13 14:29 UTC, Nick Clemens
Details | Diff | Splinter Review
[SIGNED OFF ]Bug 18201 - Export data -Fix "Remove non-local items" option and add "Removes non-local records" option for existing functionality (3.58 KB, patch)
2017-04-13 20:15 UTC, Christopher Brannon
Details | Diff | Splinter Review
Bug 18201 - Followup (5.00 KB, patch)
2017-08-24 11:48 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18201 - Export data -Fix "Remove non-local items" option and add "Removes non-local records" option for existing functionality (5.31 KB, patch)
2017-10-04 12:12 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18201 - Unit tests (3.18 KB, patch)
2017-10-04 12:12 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18201 - Export data -Fix "Remove non-local items" option and add "Removes non-local records" option for existing functionality (5.36 KB, patch)
2017-10-24 18:46 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18201 - Unit tests (3.24 KB, patch)
2017-10-24 18:46 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18201 - Export data -Fix "Remove non-local items" option and add "Removes non-local records" option for existing functionality (5.36 KB, patch)
2017-12-21 11:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18201 - Unit tests (3.29 KB, patch)
2017-12-21 11:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18201: Tidy tests (4.95 KB, patch)
2017-12-21 15:43 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 Nick Clemens 2017-03-02 21:11:53 UTC
When exporting records and selecting branches many users (and myself) assume that 'Remove non-local items' means:
remove items not belonging the branches i am exporting
But it actually means:
Remove items not belonging to the branch I am logged in as

At a minimum I think we need to update the language, ideally I think we should provide the expected behavior as an option.
Comment 1 Nick Clemens 2017-04-05 13:48:29 UTC
It's worse, it means 
'remove titles that don't contain items belonging to the branch I am signed in as' 
which means you get all items on the records, even if those specific items are from otherbranches

Clarification:
Record A has items from branches A B C
Record B has items from branches B C
Record C has items from branches A C

Signed in a branch b
Remove nonlocal items
Get:
Record A with all items
Record B with all items


Expected:
Record A with item from branch B
Record B with item from branch B
Comment 2 Nick Clemens 2017-04-13 14:29:58 UTC
Created attachment 62149 [details] [review]
Bug 18201 - Export data -Fix "Remove non-local items" option and add "Removes non-local records" option for existing functionality

It turns out the modules had the option expected  built in, we just didn't call
it. This patch set expands the options for passing to the export.

To test:
1 - Go to Tools->Export data
2 - Perform an export and check 'Remove non-local items'
3 - Note that file contains only 'local' records, but includes all items
on those records
4 - Apply patch
5 - Note Tools->Export data has a new option to remove records not owned
by logged in branch
6 - Export as before checking records option, file should be as before
7 - Now check 'Remove items not owned by logged in branch'
8 - File should now only have local items (may have empty records)
9 - Check both boxes and recieve only 'local' records and items
Comment 3 Christopher Brannon 2017-04-13 20:15:22 UTC
Created attachment 62152 [details] [review]
[SIGNED OFF ]Bug 18201 - Export data -Fix "Remove non-local items" option and add "Removes non-local records" option for existing functionality

It turns out the modules had the option expected  built in, we just didn't call
it. This patch set expands the options for passing to the export.

To test:
1 - Go to Tools->Export data
2 - Perform an export and check 'Remove non-local items'
3 - Note that file contains only 'local' records, but includes all items
on those records
4 - Apply patch
5 - Note Tools->Export data has a new option to remove records not owned
by logged in branch
6 - Export as before checking records option, file should be as before
7 - Now check 'Remove items not owned by logged in branch'
8 - File should now only have local items (may have empty records)
9 - Check both boxes and recieve only 'local' records and items

Works as described.

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Comment 4 Marcel de Rooy 2017-05-12 09:56:39 UTC
Not sure if we should classify this as a bug or an enhancement. It also includes strings. And if I read "Remove records", I suppose that will scare some people. Would it be better to talk about excluding records or items from an export ?

Another confusing thing is that we now have a library selection in the top of the form and an option "Remove .. not owned by library" in the bottom. It seems contradictory, or needs further explanation on the form.

What actually is a record not owned by a library? It makes sense for items, but how does it relate to biblios ?

Changing status to reflect need for feedback.
All by all, I think we need some more thought here and I would suggest to classify it as an enhancement.
Comment 5 Nick Clemens 2017-05-12 15:45:34 UTC
(In reply to Marcel de Rooy from comment #4)
> Not sure if we should classify this as a bug or an enhancement.
I assigned 'bug' because the current options don't do what people expect, it is essentially broken


> if I read "Remove records", I suppose that will scare
> some people. Would it be better to talk about excluding records or items
> from an export ?
'Exclude' is okay by me, but remove is the current language so I thought it should be preserved 

> Another confusing thing is that we now have a library selection in the top
> of the form and an option "Remove .. not owned by library" in the bottom. It
> seems contradictory, or needs further explanation on the form.
I take your point, but that is actually the current state, the checkbox just appears to have a different function, it actually limits to records owned by the library you are signed in at. Would you prefer to remove this option?

The only use case I see is that you can select one library at the top - check the box at the bottom and now get only records owned by both branches. Select all but your own branch at the top, check the box, and then you have a list of items you own that are not unique, may be useful in cosortia where unique items must be preserved but can be achieved via reports


> 
> What actually is a record not owned by a library? It makes sense for items,
> but how does it relate to biblios ?
'Owned by' in this context means 'hosts an item owned by' - A record is owned by a library if they own an item on that record

> Changing status to reflect need for feedback.
> All by all, I think we need some more thought here and I would suggest to
> classify it as an enhancement.
My goal was to preserve current functionality but provide expected functionality, I can accept if it is viewed as an enhancement but need some direction on what should be changed
Comment 6 Christopher Brannon 2017-05-12 18:35:20 UTC
(In reply to Marcel de Rooy from comment #4)
> Not sure if we should classify this as a bug or an enhancement. It also
> includes strings. 

It's main goal is to fix something that isn't working as expected.  Plumbing shouldn't change it to an enhancement in my opinion.

> And if I read "Remove records", I suppose that will scare
> some people. Would it be better to talk about excluding records or items
> from an export ?

I think the term "exclude" would be better.

> Another confusing thing is that we now have a library selection in the top
> of the form and an option "Remove .. not owned by library" in the bottom. It
> seems contradictory, or needs further explanation on the form.

I think the options should be up with the "Select records to export" section, and the "Remove non-local items" should say "Exclude bib records without items from signed in library".

Nick, you will hate this idea, but to be consistent and clear, the options would do well to read:

Exclude items records:
Exclude bib records without items from [SIGNED IN LIBRARY NAME]:
Exclude fields:

That's my two cents.
Comment 7 Marcel de Rooy 2017-05-24 14:36:04 UTC
Thanks Nick and Christopher.

Just another perspective:

I could imagine that the library selection in the top of the form would only affect item selection. (It would be good to move them to a separate box.)
Selecting some libraries would result in items from these libraries only, possibly resulting in a biblio without items.
Selecting all libraries would just mean all items. Selecting no libraries would result in no items.

An additional check box "Exclude records without items" could be available as part of record selection (not item selection).
Note that imo you cannot speak about biblio records from a branch within the current database setup.

That would mean that we could do without the confusing "Do not export items" as well as "Remove non-local items".

What do you think?
Comment 8 Nick Clemens 2017-06-09 15:55:12 UTC
(In reply to Marcel de Rooy from comment #7)
> Thanks Nick and Christopher.
> 
> Just another perspective:
> 
> I could imagine that the library selection in the top of the form would only
> affect item selection. (It would be good to move them to a separate box.)
> Selecting some libraries would result in items from these libraries only,
> possibly resulting in a biblio without items.
> Selecting all libraries would just mean all items. Selecting no libraries
> would result in no items.
> 
> An additional check box "Exclude records without items" could be available
> as part of record selection (not item selection).
> Note that imo you cannot speak about biblio records from a branch within the
> current database setup.
> 
> That would mean that we could do without the confusing "Do not export items"
> as well as "Remove non-local items".
> 
> What do you think?

So if I understand you Marcel, the work to be done is-
1 - Move all the checkboxes into a 'Select items to export' box
2 - Remove "Don't export items:" & "Remove non-local items:" from the 'Options' section 
3 - Add "Don't export records without items" to the 'Options' section
4 - The option in #3 would be applied after the item selections
Comment 9 Marcel de Rooy 2017-06-13 15:04:16 UTC
(In reply to Nick Clemens from comment #8)

> So if I understand you Marcel, the work to be done is-
> 1 - Move all the checkboxes into a 'Select items to export' box
> 2 - Remove "Don't export items:" & "Remove non-local items:" from the
> 'Options' section 
> 3 - Add "Don't export records without items" to the 'Options' section
> 4 - The option in #3 would be applied after the item selections

Well, depends of course on what you mean with "all the checkboxes" :)

If I am reading this again, I am still wondering if users will understand. 
Main problem might be: What means "itemtype BKS"? Or "library ABC"? Does this only affect item selection, or only biblio selection or both?
Itemtype BKS => all biblios having a BKS item (and not the other items) seems the most logical?
Library ABC => all biblios having a ABC item (not the other items) ? Note that having an ABC item should not(!) be worded as "owned by ABC".
So, in both cases we apply the selection to both biblio and item level.
Would you agree? Here we need some user feedback !

Sorry, this comment is indeed a move of position.
Comment 10 Peggy Thrasher 2017-08-10 19:16:18 UTC
As someone who is trying to do this right now...

The user chooses a bunch of bib records to export either by a filter or with a file (it would be awfully nice if it could be a file of barcodes as well as bib numbers...).  He should get all the bib records that are specified.

We could offer the option to export only bib records with items.

For items we need to give them the option of exporting no items, all items or choosing the items.  I think it makes the most sense for them to specify the branches that they want items for.  

Select Records to export
From biblio number ??  To bibionumber ??
From item call number?? to item call number ??
From Accession Start date ?? to Accession End date ??
Item type ??
Use a file  Biblionumbers or barcodes

Export bib records only if it has items ??

Choose items
Export no items
Export all items
Export items from these branches  A  B  C

Don't export these fields ??

File format
File name
Comment 11 Katrin Fischer 2017-08-16 16:48:32 UTC
I am worried that Peggy's and Marcel's suggestion will make it impossible to cover the current use case of the branch selection: Get all records, where one or more libraries have items. 

There could be done more, but for now my suggestion would be:

Rename "Library" to: "With items owned by the following libraries:". So just a clarification.

Remove the "Remove non-local items" as I understand it's current behavior is already covered by checking my library in the library selection. This will avoid a conflict between the 2 options.

Add a "Remove items not owned by selected libraries" - as this would be the expected behavior of "Remove non-local items", but then it would be clear and work in combination with the selection above.

Hope it makes sense.
Comment 12 Peggy Thrasher 2017-08-16 17:29:28 UTC
That would work for me!

Thanks, Katrin!

Peggy
Comment 13 Nick Clemens 2017-08-24 11:48:46 UTC
Created attachment 66412 [details] [review]
Bug 18201 - Followup
Comment 14 Nick Clemens 2017-08-24 11:50:17 UTC
Followup provided - it will need unit tests, but wanted to check if this functionality works for everyone
Comment 15 Katrin Fischer 2017-08-27 10:53:38 UTC
Looks better than before to me - I think at least it should have less surprises! 

These 2 options don't make so much sense to use together, but I think we can live with that until another iteration with a good idea:

Don't export items:
Remove items not owned by selected libraries:
Comment 16 Jonathan Druart 2017-09-27 17:04:13 UTC
Nick, maybe we should squash the patches and require another signoff?
Comment 17 Nick Clemens 2017-09-27 19:56:11 UTC
(In reply to Jonathan Druart from comment #16)
> Nick, maybe we should squash the patches and require another signoff?

Sounds good to me, will try to get tests and do that shortly
Comment 18 Nick Clemens 2017-10-04 12:12:51 UTC
Created attachment 67596 [details] [review]
Bug 18201 - Export data -Fix "Remove non-local items" option and add "Removes non-local records" option for existing functionality

It turns out the modules had the option expected  built in, we just didn't call
it. This patch set expands the options for passing to the export.

To test:
1 - Go to Tools->Export data
2 - Perform an export and check 'Remove non-local items'
3 - Note that file contains only 'local' records, but includes all items
on those records
4 - Apply patch
5 - Note Tools->Export data has a new option to remove records not owned
by logged in branch
6 - Export as before checking records option, file should be as before
7 - Now check 'Remove items not owned by logged in branch'
8 - File should now only have local items (may have empty records)
9 - Check both boxes and recieve only 'local' records and items
Comment 19 Nick Clemens 2017-10-04 12:12:54 UTC
Created attachment 67597 [details] [review]
Bug 18201 - Unit tests

To test:
1. prove -v t/db_dependent/Exporter/Record.t
2. Tests should pass/be green/make the day a little brighter
Comment 20 Nick Clemens 2017-10-24 18:46:29 UTC
Created attachment 68499 [details] [review]
Bug 18201 - Export data -Fix "Remove non-local items" option and add "Removes non-local records" option for existing functionality

It turns out the modules had the option expected  built in, we just didn't call
it. This patch set expands the options for passing to the export.

To test:
1 - Go to Tools->Export data
2 - Perform an export and check 'Remove non-local items'
3 - Note that file contains only 'local' records, but includes all items
on those records
4 - Apply patch
5 - Note Tools->Export data has a new option to remove records not owned
by logged in branch
6 - Export as before checking records option, file should be as before
7 - Now check 'Remove items not owned by logged in branch'
8 - File should now only have local items (may have empty records)
9 - Check both boxes and recieve only 'local' records and items

Signed-off-by: Scott Kehoe <scott@masslibsystem.org>
Comment 21 Nick Clemens 2017-10-24 18:46:33 UTC
Created attachment 68500 [details] [review]
Bug 18201 - Unit tests

To test:
1. prove -v t/db_dependent/Exporter/Record.t
2. Tests should pass/be green/make the day a little brighter

Signed-off-by: Scott Kehoe <scott@masslibsystem.org>
Comment 22 Kyle M Hall 2017-12-21 11:41:10 UTC
Created attachment 70002 [details] [review]
Bug 18201 - Export data -Fix "Remove non-local items" option and add "Removes non-local records" option for existing functionality

It turns out the modules had the option expected  built in, we just didn't call
it. This patch set expands the options for passing to the export.

To test:
1 - Go to Tools->Export data
2 - Perform an export and check 'Remove non-local items'
3 - Note that file contains only 'local' records, but includes all items
on those records
4 - Apply patch
5 - Note Tools->Export data has a new option to remove records not owned
by logged in branch
6 - Export as before checking records option, file should be as before
7 - Now check 'Remove items not owned by logged in branch'
8 - File should now only have local items (may have empty records)
9 - Check both boxes and recieve only 'local' records and items

Signed-off-by: Scott Kehoe <scott@masslibsystem.org>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 23 Kyle M Hall 2017-12-21 11:41:17 UTC
Created attachment 70003 [details] [review]
Bug 18201 - Unit tests

To test:
1. prove -v t/db_dependent/Exporter/Record.t
2. Tests should pass/be green/make the day a little brighter

Signed-off-by: Scott Kehoe <scott@masslibsystem.org>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 24 Jonathan Druart 2017-12-21 15:43:16 UTC
Created attachment 70058 [details] [review]
Bug 18201: Tidy tests
Comment 25 Jonathan Druart 2017-12-21 16:23:07 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 26 Nick Clemens 2018-01-15 13:19:46 UTC
Awesome work all! Pushed to stable for 17.11.02
Comment 27 Fridolin SOMERS 2018-01-15 15:45:19 UTC
Pushed to 17.05.x for v17.05.08