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.
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
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
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>
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.
(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
(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.
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?
(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
(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.
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
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.
That would work for me! Thanks, Katrin! Peggy
Created attachment 66412 [details] [review] Bug 18201 - Followup
Followup provided - it will need unit tests, but wanted to check if this functionality works for everyone
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:
Nick, maybe we should squash the patches and require another signoff?
(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
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
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
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>
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>
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>
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>
Created attachment 70058 [details] [review] Bug 18201: Tidy tests
Pushed to master for 18.05, thanks to everybody involved!
Awesome work all! Pushed to stable for 17.11.02
Pushed to 17.05.x for v17.05.08