Created attachment 62170 [details] [review] Bug 18433: Allow to select results to export in item search This adds a column of checkboxes in the results table to be able to select the items to be exported Test plan: 1. Go to item search and click 'Search' 2. Without checking any checkbox, verify that the export still works (it should export all results) 3. Tick some checkboxes and re-export, verify that only selected items are exported
Created attachment 62181 [details] [review] Bug 18433: Allow to select results to export in item search This adds a column of checkboxes in the results table to be able to select the items to be exported Test plan: 1. Go to item search and click 'Search' 2. Without checking any checkbox, verify that the export still works (it should export all results) 3. Tick some checkboxes and re-export, verify that only selected items are exported Followed test plan, works as expected. Signed-off-by: Marc Véron <veron@veron.ch>
Something sounds wrong here, I'd suggest that GetCategoryByKohaField should be replaced with Koha::AuthorisedValues->search_by_koha_field->categories, do not you think? Failing QA because of CamelCase for GetCategoryByKohaField and lack of tests. Note: I did not test but I'd expect my @items = Koha::Items->search({ itemnumber => { -in => \@itemnumbers } }); to be faster and easier to read than: my @items; foreach my $itemnumber (@itemnumbers) { my $item = Koha::Items->find($itemnumber); if ($item) { push @items, $item; } }
You should also use Text::CSV to build the CSV content.
(In reply to Jonathan Druart from comment #3) > Something sounds wrong here, I'd suggest that GetCategoryByKohaField should > be replaced with Koha::AuthorisedValues->search_by_koha_field->categories, > do not you think? I'm not sure, what would be the return value of Koha::AuthorisedValues->search_by_koha_field('items.barcode') for example ? A list of the authorised values of the category linked to items.barcode ? What I want is a single AV category, so for me it makes sense to have a Koha::AVCategories->findBy... method. > > Failing QA because of CamelCase for GetCategoryByKohaField and lack of tests. > All Koha::Template::Plugin::* methods are in CamelCase. I think you meant findByKohaField, right ? > Note: I did not test but I'd expect > my @items = Koha::Items->search({ itemnumber => { -in => \@itemnumbers } }); > > to be faster and easier to read than: > > my @items; > foreach my $itemnumber (@itemnumbers) { > my $item = Koha::Items->find($itemnumber); > if ($item) { > push @items, $item; > } > } Probably, I'm not familiar with the search() parameters yet, that's why I dit it that way. (In reply to Jonathan Druart from comment #4) > You should also use Text::CSV to build the CSV content. It's not really CSV, just a simple text file with one barcode per line, so what's the benefit of using Text::CSV ?
> (In reply to Jonathan Druart from comment #4) > > You should also use Text::CSV to build the CSV content. > > It's not really CSV, just a simple text file with one barcode per line, so > what's the benefit of using Text::CSV ? Sorry, I confused with bug 18394, but still, the CSV is generated by Template::Toolkit (for translatability), so, same question: what's the benefit of Text::CSV ? :)
Created attachment 62358 [details] [review] Bug 18433: QA fixes - Use snake_case for Koha::AuthorisedValueCategories method name - Replace a loop of Koha::Items->find by a single call to Koha::Items->search - Add unit tests for Koha::AuthorisedValueCategories::find_by_koha_field
Koha::Item->biblioitem should be moved to its own bug report and covered by tests.
When I export in CSV there are 2 spaces at the beginning of each line.
Created attachment 62388 [details] [review] Bug 18433: Allow to select results to export in item search This adds a column of checkboxes in the results table to be able to select the items to be exported Test plan: 1. Go to item search and click 'Search' 2. Without checking any checkbox, verify that the export still works (it should export all results) 3. Tick some checkboxes and re-export, verify that only selected items are exported
Created attachment 62389 [details] [review] Bug 18433: QA fixes - Use snake_case for Koha::AuthorisedValueCategories method name - Replace a loop of Koha::Items->find by a single call to Koha::Items->search - Add unit tests for Koha::AuthorisedValueCategories::find_by_koha_field
Rebased patches on top of bug 18459
Created attachment 62390 [details] [review] Bug 18433: Remove indentation from CSV output
Created attachment 62415 [details] [review] Bug 18433: Use existing pattern for find_by_koha_field
Created attachment 62416 [details] [review] Bug 18433: Add new subroutine GetDescriptionByKohaField to AuthorisedValues TT plugin To allow quick access to AV description
Created attachment 62417 [details] [review] Bug 18433: Use the TT plugin for itemsearch_item.json.inc TODO: includes/catalogue/itemsearch_item.csv.inc modules/catalogue/itemsearch.tt
I think I finally found what was wrong. Take a look at the 2 last patches. What you actually want is Koha::AuthorisedValues->get_description_by_koha_field. But you missed a TT plugin for an easy access. This method is cached and so certainly much powerful than what your were trying to do. Let me know if you have any questions. If you agree we will need to use this sub from the 2 other files, remove the tests and the new method you added.
(In reply to Jonathan Druart from comment #17) > What you actually want is > Koha::AuthorisedValues->get_description_by_koha_field. Yes, exactly! :-) Thanks
Created attachment 62449 [details] [review] Bug 18433: Remove Koha::AuthorisedValueCategories::find_by_koha_field Use Koha::AuthorisedValues::*_by_koha_field instead
Comment on attachment 62415 [details] [review] Bug 18433: Use existing pattern for find_by_koha_field Obsoleted this patch because it's not needed (method removed)
Back to Signed off
Created attachment 62472 [details] [review] Bug 18433: Allow to select results to export in item search This adds a column of checkboxes in the results table to be able to select the items to be exported Test plan: 1. Go to item search and click 'Search' 2. Without checking any checkbox, verify that the export still works (it should export all results) 3. Tick some checkboxes and re-export, verify that only selected items are exported Followed test plan, works as expected. Signed-off-by: Marc Véron <veron@veron.ch>
Squashed all patches for readability
Hum, I get "Template process failed: undef error - The method biblioitem is not covered by tests! at /home/vagrant/kohaclone/C4/Templates.pm line 121." when I export to CSV. Something else I completely missed before: if you select rows then switch to another page the behaviour will not work as expected.
(In reply to Jonathan Druart from comment #24) > Hum, I get "Template process failed: undef error - The method biblioitem is > not covered by tests! at /home/vagrant/kohaclone/C4/Templates.pm line 121." > when I export to CSV. If I apply the patch on a fresh master, I don't get this error, as Koha::Item::biblioitem is defined. You said on bug 18459 that the patch isn't in master, but it is (http://git.koha-community.org/gitweb/?p=koha.git;a=commit;h=6527b3b67506db0289997eb9bb98647fa1967a55) so a git fetch + git rebase should fix the problem > > Something else I completely missed before: if you select rows then switch to > another page the behaviour will not work as expected. What are you expecting exactly ? That the hidden rows keep their 'selected' state ? Or that the "Export selected results to" label changes back to "Export all results to" because there is no visible selected rows anymore ?
(In reply to Julian Maurice from comment #25) > (In reply to Jonathan Druart from comment #24) > > Hum, I get "Template process failed: undef error - The method biblioitem is > > not covered by tests! at /home/vagrant/kohaclone/C4/Templates.pm line 121." > > when I export to CSV. > > If I apply the patch on a fresh master, I don't get this error, as > Koha::Item::biblioitem is defined. You said on bug 18459 that the patch > isn't in master, but it is > (http://git.koha-community.org/gitweb/?p=koha.git;a=commit; > h=6527b3b67506db0289997eb9bb98647fa1967a55) so a git fetch + git rebase > should fix the problem Hum, not sure what happened here, certainly a lack of caffeine... > > Something else I completely missed before: if you select rows then switch to > > another page the behaviour will not work as expected. > > What are you expecting exactly ? That the hidden rows keep their 'selected' > state ? Or that the "Export selected results to" label changes back to > "Export all results to" because there is no visible selected rows anymore ? I do not really know but the current behaviour is wrong. I think the better would be to keep the selected state. Ccing Owen to get his point of view.
I agree with Jonathan that the best behavior would be to allow the user to select items on multiple pages and have it remember across pages. I say this not knowing how difficult it might be to implement it. At the very least the "Export selected results" button should return to its default state if the first page of selections is getting lost, but I don't think it's enough.
Maybe this should be splitted into one bug changing Koha::Template::Plugin::AuthorisedValues and this bug focusing on changes in item search.
(In reply to Fridolin SOMERS from comment #28) > Maybe this should be splitted into one bug changing > Koha::Template::Plugin::AuthorisedValues and this bug focusing on changes in > item search. The 2 methods added to the TT plugin are part of master now (since bug 9573).
(In reply to Jonathan Druart from comment #29) > (In reply to Fridolin SOMERS from comment #28) > > Maybe this should be splitted into one bug changing > > Koha::Template::Plugin::AuthorisedValues and this bug focusing on changes in > > item search. > > The 2 methods added to the TT plugin are part of master now (since bug 9573). Ah super. Only needs rebase then.
Created attachment 95380 [details] [review] Bug 18433: Allow to select results to export in item search This adds a column of checkboxes in the results table to be able to select the items to be exported Test plan: 1. Go to item search and click 'Search' 2. Without checking any checkbox, verify that the export still works (it should export all results) 3. Tick some checkboxes and re-export, verify that only selected items are exported Followed test plan, works as expected. Signed-off-by: Marc Véron <veron@veron.ch>
Patch rebased on master
(In reply to Owen Leonard from comment #27) > I agree with Jonathan that the best behavior would be to allow the user to > select items on multiple pages and have it remember across pages. I say this > not knowing how difficult it might be to implement it. > > At the very least the "Export selected results" button should return to its > default state if the first page of selections is getting lost, but I don't > think it's enough. This is still valid.
Created attachment 97479 [details] [review] Bug 18433: Remember item search results selected rows in session storage Session storage is not shared between browser tabs, and is cleared when a tab is closed, so it seems like a good fit for storing selected rows Test plan: 1. Go to item search (intranet) and submit the form 2. Select some rows on the first page by clicking on checkboxes 3. Go to another page of results 4. Select more rows 5. Go back to the first page 6. Selected rows should still be checked 7. Play with sorting and filtering, and see that selected rows are always checked 8. Export as CSV and as a barcodes file, verify that the export contains all selected rows 9. Click on 'Clear selection', it should deselect all rows from all pages 10. Select some rows again 11. Click on 'Edit search' and submit the same search 12. No row should be selected 13. Select some rows again 14. Refresh the page and submit again the same search 15. No row should be selected
(In reply to Jonathan Druart from comment #33) > This is still valid. Not anymore ;)
Created attachment 99123 [details] [review] Bug 18433: Allow to select results to export in item search This adds a column of checkboxes in the results table to be able to select the items to be exported Test plan: 1. Go to item search and click 'Search' (Home > Search > Item search) 2. Without checking any checkbox, verify that the export still works (it should export all results) 3. Tick some checkboxes and re-export, verify that only selected items are exported Followed test plan, works as expected. Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 99124 [details] [review] Bug 18433: Remember item search results selected rows in session storage Session storage is not shared between browser tabs, and is cleared when a tab is closed, so it seems like a good fit for storing selected rows Test plan: 1. Go to item search (intranet) and submit the form 2. Select some rows on the first page by clicking on checkboxes 3. Go to another page of results 4. Select more rows 5. Go back to the first page 6. Selected rows should still be checked 7. Play with sorting and filtering, and see that selected rows are always checked 8. Export as CSV and as a barcodes file, verify that the export contains all selected rows 9. Click on 'Clear selection', it should deselect all rows from all pages 10. Select some rows again 11. Click on 'Edit search' and submit the same search 12. No row should be selected 13. Select some rows again 14. Refresh the page and submit again the same search 15. No row should be selected Signed-off-by: David Nind <david@davidnind.com>
Note for others: - This is on the 'Item search' page rather than the normal 'Search the catalog' - Go to Staff interface homepage > Search dropdown list in the header > Item search
Created attachment 99159 [details] [review] Bug 18433: Allow to select results to export in item search This adds a column of checkboxes in the results table to be able to select the items to be exported Test plan: 1. Go to item search and click 'Search' 2. Without checking any checkbox, verify that the export still works (it should export all results) 3. Tick some checkboxes and re-export, verify that only selected items are exported Followed test plan, works as expected. Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 99160 [details] [review] Bug 18433: Remember item search results selected rows in session storage Session storage is not shared between browser tabs, and is cleared when a tab is closed, so it seems like a good fit for storing selected rows Test plan: 1. Go to item search (intranet) and submit the form 2. Select some rows on the first page by clicking on checkboxes 3. Go to another page of results 4. Select more rows 5. Go back to the first page 6. Selected rows should still be checked 7. Play with sorting and filtering, and see that selected rows are always checked 8. Export as CSV and as a barcodes file, verify that the export contains all selected rows 9. Click on 'Clear selection', it should deselect all rows from all pages 10. Select some rows again 11. Click on 'Edit search' and submit the same search 12. No row should be selected 13. Select some rows again 14. Refresh the page and submit again the same search 15. No row should be selected Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 99161 [details] [review] Bug 18433: Add missing filter Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 99162 [details] [review] Bug 18433: Display the number of selected items Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
(In reply to Julian Maurice from comment #35) > (In reply to Jonathan Druart from comment #33) > > This is still valid. > > Not anymore ;) Awesome Julian, thanks for that follow-up! (In reply to Jonathan Druart from comment #42) > Created attachment 99162 [details] [review] [review] > Bug 18433: Display the number of selected items > > Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Do you agree with that patch?
(In reply to Jonathan Druart from comment #43) > Do you agree with that patch? Yep. I haven't tested it, but patch looks good.
Nice work everyone! Pushed to master for 20.05
enhancement not backported to 19.11.x
This change causes the regression reported in Bug 27565.