This new feature allows for batch deletion of serials through the UI. The current usage is a one-by-one long game. Testing - Have at least three serials, with items - check the box to select two of them - Press the Remove selected issues button - Decide to delete their items as well or not - Validate.
Sponsored by : CCSR
Created attachment 57731 [details] [review] Bug 17674 - Allow UI to delete serials in batch This new feature allows for batch deletion of serials through the UI. The current usage is a one-by-one long game. Testing - Have at least three serials, with items - check the box to select two of them - Press the Remove selected issues button - Decide to delete their items as well or not - Validate. Sponsored by : CCSR
A test file for the changes in C4/Serials.pm will follow. This enhancement was clamored for on the Koha (French) list.
Created attachment 58262 [details] [review] Bug 17674 - Fix problem tabs
Created attachment 58263 [details] [review] Bug - Add unit tests
Created attachment 58264 [details] [review] Bug 17674 - Add unit tests
Created attachment 60925 [details] [review] Bug 17674 - Add unit tests
Patch tested with a sandbox, by Sonia BOUIS <sonia.bouis@univ-lyon3.fr>
Created attachment 61314 [details] [review] Bug 17674 - Allow UI to delete serials in batch This new feature allows for batch deletion of serials through the UI. The current usage is a one-by-one long game. Testing - Have at least three serials, with items - check the box to select two of them - Press the Remove selected issues button - Decide to delete their items as well or not - Validate. Sponsored by : CCSR Signed-off-by: Sonia BOUIS <sonia.bouis@univ-lyon3.fr>
Created attachment 61315 [details] [review] Bug 17674 - Fix problem tabs Signed-off-by: Sonia BOUIS <sonia.bouis@univ-lyon3.fr>
Created attachment 61316 [details] [review] Bug 17674 - Add unit tests https://bugs.koha-community.org/show_bug.cgi?id=17674 Signed-off-by: Sonia BOUIS <sonia.bouis@univ-lyon3.fr>
It works as described. I test with 1, 2, 3 issues deletion at one time asking for items to be deleting or not and the result is consistent :)
New subroutines should be covered by tests. It would be preferable not to add new subroutines to C4 whereas I am trying to move everything to Koha. Please consider adding a new method to Koha.
Created attachment 66565 [details] [review] Bug 17674 - Move subroutine GetSerialItemsInformations to Koha/Serials.pm
Hi Can you please rebase against the master branch, when I apply the patches attached to this bug report I get the following merge error which is being thrown by the t/db_dependent/Serials.t file Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 17674 - Allow UI to delete serials in batch Applying: Bug 17674 - Fix problem tabs Applying: Bug 17674 - Add unit tests Using index info to reconstruct a base tree... M t/db_dependent/Serials.t Falling back to patching base and 3-way merge... Auto-merging t/db_dependent/Serials.t CONFLICT (content): Merge conflict in t/db_dependent/Serials.t Failed to merge in the changes. Patch failed at 0001 Bug 17674 - Add unit tests The copy of the patch that failed is found in: /home/vagrant/kohaclone/.git/rebase-apply/patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-17674---Add-unit-tests-yO9YMv.patch
Created attachment 69007 [details] [review] Bug 17674 - Allow UI to delete serials in batch This new feature allows for batch deletion of serials through the UI. The current usage is a one-by-one long game. Testing - Have at least three serials, with items - check the box to select two of them - Press the Remove selected issues button - Decide to delete their items as well or not - Validate. Sponsored by : CCSR Signed-off-by: Sonia BOUIS <sonia.bouis@univ-lyon3.fr>
Created attachment 69010 [details] [review] Bug 17674 - Fix problem tabs Signed-off-by: Sonia BOUIS <sonia.bouis@univ-lyon3.fr>
Created attachment 69011 [details] [review] Bug 17674 - Add unit tests https://bugs.koha-community.org/show_bug.cgi?id=17674 Signed-off-by: Sonia BOUIS <sonia.bouis@univ-lyon3.fr>
Created attachment 69013 [details] [review] Bug 17674 - Move subroutine GetSerialItemsInformations to Koha/Serials.pm
*** Bug 12575 has been marked as a duplicate of this bug. ***
Created attachment 69976 [details] [review] Bug 17674 - Allow UI to delete serials in batch This new feature allows for batch deletion of serials through the UI. The current usage is a one-by-one long game. Testing - Have at least three serials, with items - check the box to select two of them - Press the Remove selected issues button - Decide to delete their items as well or not - Validate. Sponsored by : CCSR Signed-off-by: Sonia BOUIS <sonia.bouis@univ-lyon3.fr>
Created attachment 69977 [details] [review] Bug 17674 - Fix problem tabs Signed-off-by: Sonia BOUIS <sonia.bouis@univ-lyon3.fr>
Created attachment 69978 [details] [review] Bug 17674 - Add unit tests https://bugs.koha-community.org/show_bug.cgi?id=17674 Signed-off-by: Sonia BOUIS <sonia.bouis@univ-lyon3.fr>
Created attachment 69979 [details] [review] Bug 17674 - Move subroutine GetSerialItemsInformations to Koha/Serials.pm
Had conflicting git apply, rebased the patch on the current master.
Created attachment 71865 [details] [review] Bug 17674 - Allow UI to delete serials in batch This new feature allows for batch deletion of serials through the UI. The current usage is a one-by-one long game. Testing - Have at least three serials, with items - check the box to select two of them - Press the Remove selected issues button - Decide to delete their items as well or not - Validate. Sponsored by : CCSR Signed-off-by: Sonia BOUIS <sonia.bouis@univ-lyon3.fr>
Created attachment 71866 [details] [review] Bug 17674 - Fix problem tabs Signed-off-by: Sonia BOUIS <sonia.bouis@univ-lyon3.fr>
Created attachment 71867 [details] [review] Bug 17674 - Add unit tests https://bugs.koha-community.org/show_bug.cgi?id=17674 Signed-off-by: Sonia BOUIS <sonia.bouis@univ-lyon3.fr>
Created attachment 71868 [details] [review] Bug 17674 - Move subroutine GetSerialItemsInformations to Koha/Serials.pm
Created attachment 71869 [details] [review] Bug 17674 - Adapting Serials.t to the namespace change
Rebased once again on the current master. This time, though, a new patch was submitted to make sure our subtests respected the new namespace, and that no items were created without a biblioitemnumber. Just one more signoff to go.
1. You are not supposed to write SQL queries 2. You should read how Koha::Object-based objects work 3. method's names must be snake_case In short: forget the former C4 logic and rethink your new method. I can help you if you are stuck.
Created attachment 72985 [details] [review] Bug 17674 - Renamed GetSerialItemsInformations to get_serial_items_count and use new OOP logic Removed SQl and use snake_case. Same test plan as before. Might also want to test t/db_dependent/Serials.t
No conflict when applying the patch but I couldn't find where to find the new feature. Is it supposed to be on the /cgi-bin/koha/serials/serials-search.pl ? Or a new menu is created on Tools ?
(In reply to Séverine Queune from comment #34) > No conflict when applying the patch but I couldn't find where to find the > new feature. > Is it supposed to be on the /cgi-bin/koha/serials/serials-search.pl ? > Or a new menu is created on Tools ? Hi Severine, You have to go on serials-collection.pl?subscriptionid=?, you will see a button "Remove selected issue". See attached image. Regards, Marjorie
Created attachment 73032 [details] remove selected issues
My bad, I confused myself between "serial" and "subscription" so I expected a different feature and was looking at the wrong place ! (It's been a hard hackfest week in Marseille ^^). Thanks a lot Marjorie ! I 'm going to test it.
Created attachment 73035 [details] [review] Bug 17674 - Allow UI to delete serials in batch This new feature allows for batch deletion of serials through the UI. The current usage is a one-by-one long game. Testing - Have at least three serials, with items - check the box to select two of them - Press the Remove selected issues button - Decide to delete their items as well or not - Validate. Sponsored by : CCSR Signed-off-by: Sonia BOUIS <sonia.bouis@univ-lyon3.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 73036 [details] [review] Bug 17674 - Fix problem tabs Signed-off-by: Sonia BOUIS <sonia.bouis@univ-lyon3.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 73037 [details] [review] Bug 17674 - Add unit tests https://bugs.koha-community.org/show_bug.cgi?id=17674 Signed-off-by: Sonia BOUIS <sonia.bouis@univ-lyon3.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 73038 [details] [review] Bug 17674 - Move subroutine GetSerialItemsInformations to Koha/Serials.pm Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 73039 [details] [review] Bug 17674 - Adapting Serials.t to the namespace change Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 73040 [details] [review] Bug 17674 - Renamed GetSerialItemsInformations to get_serial_items_count and use new OOP logic Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
(In reply to Séverine Queune from comment #37) > My bad, I confused myself between "serial" and "subscription" so I expected > a different feature and was looking at the wrong place ! (It's been a hard > hackfest week in Marseille ^^). > Thanks a lot Marjorie ! I 'm going to test it. Great! Thanks for testing! Marjorie
Works as described. Thanks to all of you for this new feature !
Starting with a code review, patches apply, QA script passes: 1) Translatability issue + <h3>Are you sure delete <b> [% serialnumber %] </b> selected serials</h3> Please rethink this sentence. The <b> should not be used within a sentence as it will break this into several strings. I think it's not necessary here. Also sentence reads a bit strangely? :) Similar here: <h5>Number of items available <b>: [% countitems %] </b> </h5> That went well! Now testing: 2) I think if the serial is not set up to create items, we should not ask. Or if the number of items is 0. 3) A checkbox for deleting items would be a little nicer than the radio button. Could be fixed later, not blocker. 4) Number of items available : 0 No space there in English: available: 5) The appearing yellow dialogue pushes down the navigation bar on the right. Please check. Mostly tiny stuff, this looks quite good. Can you provide a quick follow-up and switch back to signed-off please?
Created attachment 74318 [details] [review] Bug 17674 (follow-up) Fix some HTML and JS inconsistencies This patch makes some HTML and JS changes to make the template more consistent with coding guidelines and other similar interfaces. - The confirmation dialog is now below the heading. Buttons are styled to match others like them. The layout of messages hopefully makes it a little more clear. - The "delete selected" event handler has been moved from an "onclick" attribute into the <script> tags. To test, apply the patch and follow the original test plan. - Confirm that the "Delete selected" button works correctly. - Confirm that the confirmation dialog looks good and that its form elements work correctly.
Hi Owen, did you forget to switch this back to "Need signoff"? It seems like your patch cover everything said Kat, yet the bug's status is still failed QA.
(In reply to Roch D'Amour from comment #48) > Hi Owen, did you forget to switch this back to "Need signoff"? I think this issue is still unresolved: > 2) I think if the serial is not set up to create items, we should not ask. > Or if the number of items is 0.
Created attachment 74359 [details] [review] Bug 17674: Remove delete related item checkbox when no items is available When the number of items available is 0, the confirmation dialog still ask is you also want to delete related items. This patch remove the checkbox in the confirmation dialog when the number of related items is 0. To test: - Try to delete some serials that has items available, the confirmation dialog does not ask if you want to delete related items since there are none. - Try to delete some serials which has items avaible. The confirmation dialog should show a checkbox and ask if you want to delete related items
(In reply to Owen Leonard from comment #47) > - Confirm that the "Delete selected" button works correctly. When serials of several years exist, only the button of the first tab works, not the buttons "Delete selected issues" displayed on the tabs of previous years. The other buttons work as expected on all tabs. Except that point, the rest of the patch works correctly, but I prefer not sign it.
For information: I have added a "modify selected serials" link on bug 18426, but on the other screen (subscription search result)
This if for serials issues, not subscriptons. I think it will be useful if libraries bind issues or regularly throw out older ones.
Created attachment 74472 [details] [review] Bug 17674 (follow-up) Fix some HTML and JS inconsistencies This patch makes some HTML and JS changes to make the template more consistent with coding guidelines and other similar interfaces. - The confirmation dialog is now below the heading. Buttons are styled to match others like them. The layout of messages hopefully makes it a little more clear. - The "delete selected" event handler has been moved from an "onclick" attribute into the <script> tags. To test, apply the patch and follow the original test plan. - Confirm that the "Delete selected" button works correctly. - Confirm that the confirmation dialog looks good and that its form elements work correctly. Edit: Make sure delete button works from any tab.
(In reply to Roch D'Amour from comment #50) > - Try to delete some serials that has items available, the confirmation > dialog does not ask There is a typo which is preventing this from working correctly: [% IF $countitems > 0 %] should be [% IF countitems > 0 %]
The patch no longer applies, can you please check? Maybe you did the last change without Owen's follow-up? Please also take a comment at c#55!
Oh, and please also fix git commit messages! https://wiki.koha-community.org/wiki/Commit_messages
(In reply to Katrin Fischer from comment #56) > The patch no longer applies, can you please check? The patches are out of order after my last update. "Remove delete related item checkbox when no items is available" should be applied after mine.
Roch, can you please provide a follow-up fixing the last issues? I'd like to retest before Friday deadline.
Created attachment 75643 [details] [review] Bug 17674: Allow UI to delete serials in batch This new feature allows for batch deletion of serials through the UI. The current usage is a one-by-one long game. Testing - Have at least three serials, with items - check the box to select two of them - Press the Remove selected issues button - Decide to delete their items as well or not - Validate. Sponsored by : CCSR Signed-off-by: Sonia BOUIS <sonia.bouis@univ-lyon3.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 75644 [details] [review] Bug 17674: Fix problem tabs Signed-off-by: Sonia BOUIS <sonia.bouis@univ-lyon3.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 75645 [details] [review] Bug 17674: Add unit tests https://bugs.koha-community.org/show_bug.cgi?id=17674 Signed-off-by: Sonia BOUIS <sonia.bouis@univ-lyon3.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 75646 [details] [review] Bug 17674: Move subroutine GetSerialItemsInformations to Koha/Serials.pm Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 75647 [details] [review] Bug 17674: Adapting Serials.t to the namespace change Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 75648 [details] [review] Bug 17674: Renamed GetSerialItemsInformations to get_serial_items_count and use new OOP logic Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 75649 [details] [review] Bug 17674: (follow-up) Fix some HTML and JS inconsistencies This patch makes some HTML and JS changes to make the template more consistent with coding guidelines and other similar interfaces. - The confirmation dialog is now below the heading. Buttons are styled to match others like them. The layout of messages hopefully makes it a little more clear. - The "delete selected" event handler has been moved from an "onclick" attribute into the <script> tags. To test, apply the patch and follow the original test plan. - Confirm that the "Delete selected" button works correctly. - Confirm that the confirmation dialog looks good and that its form elements work correctly. Edit: Make sure delete button works from any tab.
Rebased patches. I had to manually redo Owen's patch for some reason so I combined it with Roch's last patch
Created attachment 77745 [details] [review] Bug 17674 - Allow UI to delete serials in batch This new feature allows for batch deletion of serials through the UI. The current usage is a one-by-one long game. Testing - Have at least three serials, with items - check the box to select two of them - Press the Remove selected issues button - Decide to delete their items as well or not - Validate. Sponsored by : CCSR Signed-off-by: Sonia BOUIS <sonia.bouis@univ-lyon3.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 77746 [details] [review] Bug 17674 - Fix problem tabs Signed-off-by: Sonia BOUIS <sonia.bouis@univ-lyon3.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 77747 [details] [review] Bug 17674 - Add unit tests https://bugs.koha-community.org/show_bug.cgi?id=17674 Signed-off-by: Sonia BOUIS <sonia.bouis@univ-lyon3.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 77748 [details] [review] Bug 17674 - Move subroutine GetSerialItemsInformations to Koha/Serials.pm Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 77749 [details] [review] Bug 17674 - Adapting Serials.t to the namespace change Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 77750 [details] [review] Bug 17674 - Renamed GetSerialItemsInformations to get_serial_items_count and use new OOP logic Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 77751 [details] [review] Bug 17674 (follow-up) Fix some HTML and JS inconsistencies This patch makes some HTML and JS changes to make the template more consistent with coding guidelines and other similar interfaces. - The confirmation dialog is now below the heading. Buttons are styled to match others like them. The layout of messages hopefully makes it a little more clear. - The "delete selected" event handler has been moved from an "onclick" attribute into the <script> tags. To test, apply the patch and follow the original test plan. - Confirm that the "Delete selected" button works correctly. - Confirm that the confirmation dialog looks good and that its form elements work correctly. Edit: Make sure delete button works from any tab.
Created attachment 77861 [details] [review] Bug 17674: Allow UI to delete serials in batch This new feature allows for batch deletion of serials through the UI. The current usage is a one-by-one long game. Testing - Have at least three serials, with items - check the box to select two of them - Press the Remove selected issues button - Decide to delete their items as well or not - Validate. Sponsored by : CCSR Signed-off-by: Sonia BOUIS <sonia.bouis@univ-lyon3.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 77862 [details] [review] Bug 17674: Fix problem tabs Signed-off-by: Sonia BOUIS <sonia.bouis@univ-lyon3.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 77863 [details] [review] Bug 17674: Add unit tests https://bugs.koha-community.org/show_bug.cgi?id=17674 Signed-off-by: Sonia BOUIS <sonia.bouis@univ-lyon3.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 77864 [details] [review] Bug 17674: Move subroutine GetSerialItemsInformations to Koha/Serials.pm Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 77865 [details] [review] Bug 17674: Adapting Serials.t to the namespace change Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 77866 [details] [review] Bug 17674: Renamed GetSerialItemsInformations to get_serial_items_count and use new OOP logic Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 77867 [details] [review] Bug 17674: (follow-up) Fix some HTML and JS inconsistencies This patch makes some HTML and JS changes to make the template more consistent with coding guidelines and other similar interfaces. - The confirmation dialog is now below the heading. Buttons are styled to match others like them. The layout of messages hopefully makes it a little more clear. - The "delete selected" event handler has been moved from an "onclick" attribute into the <script> tags. To test, apply the patch and follow the original test plan. - Confirm that the "Delete selected" button works correctly. - Confirm that the confirmation dialog looks good and that its form elements work correctly. Edit: Make sure delete button works from any tab.
Created attachment 77868 [details] [review] Bug 17674: (follow-up) Fix wrong number of tests, make button match visually
I've fixed the commit messages Bug XXXX: instead of Bug XXXX -. I've also added a tiny follow-up to fix the styling of the button and the wrong number of tests. Generally this works well, so I am signing off. Things to improve: - When deleting multiple issues and one is expected, the expected one is not deleted, which is, I think, the correct behaviour. Without the expected issue the subscription would no longer work correctly. But we could improve the visual by warning the user maybe. - When deleting issues that have no items yet, you are still offered the option to delete items with them (Status late for example). Nothing bad happens, so not blocker for me.
Created attachment 79415 [details] [review] Bug 17674: (follow-up) Fix wrong number of tests, make button match visually
Created attachment 79420 [details] [review] Bug 17674: Allow UI to delete serials in batch This new feature allows for batch deletion of serials through the UI. The current usage is a one-by-one long game. Testing - Have at least three serials, with items - check the box to select two of them - Press the Remove selected issues button - Decide to delete their items as well or not - Validate. Sponsored by : CCSR Signed-off-by: Sonia BOUIS <sonia.bouis@univ-lyon3.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 79421 [details] [review] Bug 17674: Allow UI to delete serials in batch This new feature allows for batch deletion of serials through the UI. The current usage is a one-by-one long game. Testing - Have at least three serials, with items - check the box to select two of them - Press the Remove selected issues button - Decide to delete their items as well or not - Validate. Sponsored by : CCSR Signed-off-by: Sonia BOUIS <sonia.bouis@univ-lyon3.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 79422 [details] [review] Bug 17674: Fix problem tabs Signed-off-by: Sonia BOUIS <sonia.bouis@univ-lyon3.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 79423 [details] [review] Bug 17674: Add unit tests https://bugs.koha-community.org/show_bug.cgi?id=17674 Signed-off-by: Sonia BOUIS <sonia.bouis@univ-lyon3.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 79424 [details] [review] Bug 17674: Move subroutine GetSerialItemsInformations to Koha/Serials.pm Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 79425 [details] [review] Bug 17674: Adapting Serials.t to the namespace change Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 79426 [details] [review] Bug 17674: Renamed GetSerialItemsInformations to get_serial_items_count and use new OOP logic Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 79427 [details] [review] Bug 17674: (follow-up) Fix some HTML and JS inconsistencies This patch makes some HTML and JS changes to make the template more consistent with coding guidelines and other similar interfaces. - The confirmation dialog is now below the heading. Buttons are styled to match others like them. The layout of messages hopefully makes it a little more clear. - The "delete selected" event handler has been moved from an "onclick" attribute into the <script> tags. To test, apply the patch and follow the original test plan. - Confirm that the "Delete selected" button works correctly. - Confirm that the confirmation dialog looks good and that its form elements work correctly. Edit: Make sure delete button works from any tab.
Created attachment 79428 [details] [review] Bug 17674: (follow-up) Fix wrong number of tests, make button match visually
A rebase has been made.
Created attachment 79453 [details] [review] Bug 17674: Allow UI to delete serials in batch This new feature allows for batch deletion of serials through the UI. The current usage is a one-by-one long game. Testing - Have at least three serials, with items - check the box to select two of them - Press the Remove selected issues button - Decide to delete their items as well or not - Validate. Sponsored by : CCSR Signed-off-by: Sonia BOUIS <sonia.bouis@univ-lyon3.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com>
Created attachment 79454 [details] [review] Bug 17674: Fix problem tabs Signed-off-by: Sonia BOUIS <sonia.bouis@univ-lyon3.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com>
Created attachment 79455 [details] [review] Bug 17674: Add unit tests https://bugs.koha-community.org/show_bug.cgi?id=17674 Signed-off-by: Sonia BOUIS <sonia.bouis@univ-lyon3.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com>
Created attachment 79456 [details] [review] Bug 17674: Move subroutine GetSerialItemsInformations to Koha/Serials.pm Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com>
Created attachment 79457 [details] [review] Bug 17674: Adapting Serials.t to the namespace change Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com>
Created attachment 79458 [details] [review] Bug 17674: Renamed GetSerialItemsInformations to get_serial_items_count and use new OOP logic Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com>
Created attachment 79459 [details] [review] Bug 17674: (follow-up) Fix some HTML and JS inconsistencies This patch makes some HTML and JS changes to make the template more consistent with coding guidelines and other similar interfaces. - The confirmation dialog is now below the heading. Buttons are styled to match others like them. The layout of messages hopefully makes it a little more clear. - The "delete selected" event handler has been moved from an "onclick" attribute into the <script> tags. To test, apply the patch and follow the original test plan. - Confirm that the "Delete selected" button works correctly. - Confirm that the confirmation dialog looks good and that its form elements work correctly. Edit: Make sure delete button works from any tab. Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com>
Created attachment 79460 [details] [review] Bug 17674: (follow-up) Fix wrong number of tests, make button match visually Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com>
QA comments (reading the code): 1. I do not understand Koha::Serials::get_serial_items_count, several things are wrong: * you should not export, it must be a method and called like others: Koha::Module->method * my ( @self ) = @_; No, $self must be the object, then you pass the parameters. * + while ( my $s = $serialitems->next() ) { + $countitem++; + } => You actually want ->count * And finally, what is it supposed to do? and why? `git grep get_serial_items_count` returns only one occurrence: serials/serials-collection.pl:my @serialitemsinformation=get_serial_items_count(@ids); It returns an array of hashes (2 keys: countitems and serialid), countitems is used later, and serialid is not. Moreover I have no idea what the following is supposed to do 130 foreach my $line (@serialitemsinformation){ 131 DelItem($line); 132 } It certainly does not work. I would suggest you to rethink this as a method, for instance I am pretty sure you are looking for something like that: Koha::Serial->items which would return the items (Koha::Items!) for a given Koha::Serial object (already said, less explicitly, on comment 32) 2. You do not need to concat with '!' then split, you can pass several times a parameters (and retrieve them pl side with CGI->multi_param). It will simplify a lot the javascript code (function deleteIssues). 3. $delete and $confdelete should not be variables, but values of $op (usually "delete_confirm" and "delete_confirmed", see admin/cities.pl) 4. function generateReceive (serials-collection.tt) re-added, has been deleted previously by bug 18327. 5. Same for addsubscriptionid, delete by bug 19777. IMO it would be easier to redo these patches from scratch and abandon the too long patches history.
I really want the feature, but I see that Jonathan has made some very good points. This has been sitting too long and the code has evolved since :(
All patches have been removed and will be replaced by a new one as suggested. It will take in account the last suggestions.
Created attachment 80859 [details] [review] Bug 17674 - Allow UI to delete serials in batch This new feature allows for batch deletion of serials through the UI. The current usage is a one-by-one long game. Testing - Have at least three serials, with items - check the box to select two of them - Press the Remove selected issues button - Decide to delete their items as well or not - Validate. Sponsored by : CCSR
Items are not suppressed when I check the box.
Created attachment 80944 [details] [review] Bug 17674 - Allow UI to delete serials in batch This new feature allows for batch deletion of serials through the UI. The current usage is a one-by-one long game. Testing - Have at least three serials, with items - check the box to select two of them - Press the Remove selected issues button - Decide to delete their items as well or not - Validate. Sponsored by : CCSR
Created attachment 80955 [details] [review] Bug 17674 - Allow UI to delete serials in batch This new feature allows for batch deletion of serials through the UI. The current usage is a one-by-one long game. Testing - Have at least three serials, with items - check the box to select two of them - Press the Remove selected issues button - Decide to delete their items as well or not - Validate. Sponsored by : CCSR Signed-off-by: Michal Denar <black23@gmail.com>
Hi Pierre-Marc, some things here: 1) Please make sure all your patches pass the QA script before submitting them: https://gitlab.com/koha-community/qa-test-tools Currently, it fails: FAIL koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-collection.tt FAIL filters missing_filter at line 380 ( deleteIssues("[% subscriptionidlist %]");) missing_filter at line 39 ( <h5>Number of items available: [% countitems %]</h5>) missing_filter at line 46 ( <input type="hidden" name="subscriptionid" value="[% subscriptionid %]" />) missing_filter at line 48 ( <input type="hidden" name="serialid" value="[% serialid %]" />) missing_filter at line 148 ( <input type="button" value="Delete selected issues" class="delete-issues btn btn-default btn-sm" data-subscriptionids="[% subscriptionidlist %]" />) missing_filter at line 36 ( <h3>You are about to delete [% serialnumber %] serial(s). Do you want to continue?</h3>) missing_filter at line 45 ( <input type="hidden" name="biblionumber" value="[% biblionumber %]" />) missing_filter at line 53 ( <input type="hidden" name="subscriptionid" value="[% subscriptionid %]" />) 2) Please make sure that the subject lines of your commits are formatted correctly. The QA script will also help with that: * Commit title does not start with 'Bug XXXXX: ' - 12463c45e7 3) Please make sure all text is translatable: https://wiki.koha-community.org/wiki/Coding_Guidelines#JS2:_Enabling_translation_of_language_strings_in_embedded_JavaScript + alert("No serials selected");
Created attachment 82237 [details] [review] Bug 17674: Allow UI to delete serials in batch This new feature allows for batch deletion of serials through the UI. The current usage is a one-by-one long game. Testing - Have at least three serials, with items - check the box to select two of them - Press the Remove selected issues button - Decide to delete their items as well or not - Validate. Sponsored by : CCSR
I made sure that the QA tests passed and that the alert is translatable. These kinds of fixes should be gone now :)
Everything is correct when I deleted both serials and items, but I have a doubt about the behaviour when I keep the items : is it normal the 'Enumchron' field is emptied after deleting the linked serial ? I can't remember if it happened when I've previously tested it.
(In reply to Séverine Queune from comment #113) > Everything is correct when I deleted both serials and items, but I have a > doubt about the behaviour when I keep the items : is it normal the > 'Enumchron' field is emptied after deleting the linked serial ? > I can't remember if it happened when I've previously tested it. This doesn't sound good - I think enumchron is the only way to tell the items apart so needs to be kept.
I am not able to reproduce the problem. I will reupload the patch with a better test plan that will make sure there is no problem with the enumchron.
So... I find out when a serial is deleted with and without the patch, the enumchron lost only the date part. Maybe that was the observed problem, because otherwise the enumcron is not emptied. I will therefore proceed with the clearer test plan.
Created attachment 86689 [details] [review] Bug 17674: Allow UI to delete serials in batch This new feature allows for batch deletion of serials through the UI. The current usage is a one-by-one long game. Test plan : - Apply patch. - Create at least four serials with items. - Notice that new items have a numbering pattern and a date in the enumchron field. - Use the button Delete selected issues for the following steps. - Delete a serial and its associated item. - Confirm the serial and item are gone. - Delete a serial and not its item. - Confirm the serial is gone. - Confirm the item is still there and the date part in the enumchron field is gone. - Delete the two lasts serials. - Confirm they are gone. Sponsored by : CCSR
Sorry Pierre-mars, I still have the enumvhron th
Oups... Sorry, I still have the enumchron of the item emptied when I delete the serial.
Could this be an UNIMARC vs. MARC21 issue? Which format did you use for testing Severine?
(In reply to Katrin Fischer from comment #121) > Could this be an UNIMARC vs. MARC21 issue? Which format did you use for > testing Severine? Unimarc. I tested with Biblibre's sandbox#1, patch is still applied if you want to take a look : I used the subscription "Psychologies" (Call number: SER4)
I tried testing myself and could not reproduce the situation when the enumchron is emptied. I also tried testing with Biblibre's sandbox using Unimarc database and "Psychologies" subscription, but did not observe the problem. I wonder if I could trouble you for the exact steps you used to produce this error while testing?
Created attachment 95474 [details] [review] Bug 17674: Allow UI to delete serials in batch This new feature allows for batch deletion of serials through the UI. The current usage is a one-by-one long game. Test plan : - Apply patch. - Create at least four serials with items. - Notice that new items have a numbering pattern and a date in the enumchron field. - Use the button Delete selected issues for the following steps. - Delete a serial and its associated item. - Confirm the serial and item are gone. - Delete a serial and not its item. - Confirm the serial is gone. - Confirm the item is still there and the date part in the enumchron field is gone. - Delete the two lasts serials. - Confirm they are gone. Sponsored by : CCSR Signed-off-by: Michal Denar <black23@gmail.com>
If I receive a few serials and then delete the expected issue from the serials collection screen I can only see an expected issue until I receive the expected issue, then all previously received undeleted issues show again. To recreate: 1 - Create a subscription that creates items 2 - Receive two issues 3 - From serial collection select the 'Expected' issue and delete 4 - You only see the new expected issue If I try to delete a serial with an item that cannot be deleted the serial deletion completes but the item remains with no feedback To recreate 1 - Follow 1 and 2 above 2 - Check out one of the items 3 - Select the serial for this issue and delete 4 - Check the box to confirm deleting items 5 - The serial is deleted, the item remains, no error shown to user
Created attachment 99434 [details] [review] Bug 17674: Handling of special cases This patch adds handling for those 2 particular cases: 1. Deleting the expected issue 2. Deleting a serial and it's item, but one item cannot be deleted To test: 1.1 Create a subscription that creates items 1.2 Receive two issues 1.3 Use the new button to delete the 'Expected' issue 1.4 The issue is correctly deleted, but the new expected issue created to replace the one which has been deleted don't have a published or received date and is show in it's own tab. 1.5 Apply patch 1.6 Repeat steps 1.1 through 1.4 => The new expected issue is created correctly 2.1 Use the same serial as in 1 or repeat step 1.1 and 1.2 2.2 Check out one of the items 2.3 Delete the corresponding serial and check box to delete items 2.4 The serial is deleted, but not the item. => No feedback is show to the user 2.5 Apply patch 2.6 Repeat step 2.1 through 2.4 => As before, the serial is deleted, but not the item => This time, an error is shown to inform the user
Created attachment 99944 [details] [review] Bug 17674: Allow UI to delete serials in batch This new feature allows for batch deletion of serials through the UI. The current usage is a one-by-one long game. Test plan : - Apply patch. - Create at least four serials with items. - Notice that new items have a numbering pattern and a date in the enumchron field. - Use the button Delete selected issues for the following steps. - Delete a serial and its associated item. - Confirm the serial and item are gone. - Delete a serial and not its item. - Confirm the serial is gone. - Confirm the item is still there and the date part in the enumchron field is gone. - Delete the two lasts serials. - Confirm they are gone. Sponsored by : CCSR Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: sonia <sonia.bouis@univ-lyon3.fr>
Created attachment 99945 [details] [review] Bug 17674: Handling of special cases This patch adds handling for those 2 particular cases: 1. Deleting the expected issue 2. Deleting a serial and it's item, but one item cannot be deleted To test: 1.1 Create a subscription that creates items 1.2 Receive two issues 1.3 Use the new button to delete the 'Expected' issue 1.4 The issue is correctly deleted, but the new expected issue created to replace the one which has been deleted don't have a published or received date and is show in it's own tab. 1.5 Apply patch 1.6 Repeat steps 1.1 through 1.4 => The new expected issue is created correctly 2.1 Use the same serial as in 1 or repeat step 1.1 and 1.2 2.2 Check out one of the items 2.3 Delete the corresponding serial and check box to delete items 2.4 The serial is deleted, but not the item. => No feedback is show to the user 2.5 Apply patch 2.6 Repeat step 2.1 through 2.4 => As before, the serial is deleted, but not the item => This time, an error is shown to inform the user Signed-off-by: sonia <sonia.bouis@univ-lyon3.fr>
I've followed the test plan and everything looks Ok for me. 1. The next expected issue is correctly created when we delete the current expected issue. 2. If an item is checked out, you can't delete this item when you delete the serial issue. There's an alert to tell you that you can't delete the item.
I'd say you should not call DelItem directly, but DelItemCheck instead.
(In reply to Jonathan Druart from comment #130) > I'd say you should not call DelItem directly, but DelItemCheck instead. This is what has been done in the follow up patch.
Created attachment 100906 [details] [review] Bug 17674: Allow UI to delete serials in batch This new feature allows for batch deletion of serials through the UI. The current usage is a one-by-one long game. Test plan : - Apply patch. - Create at least four serials with items. - Notice that new items have a numbering pattern and a date in the enumchron field. - Use the button Delete selected issues for the following steps. - Delete a serial and its associated item. - Confirm the serial and item are gone. - Delete a serial and not its item. - Confirm the serial is gone. - Confirm the item is still there and the date part in the enumchron field is gone. - Delete the two lasts serials. - Confirm they are gone. Sponsored by : CCSR Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: sonia <sonia.bouis@univ-lyon3.fr> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 100907 [details] [review] Bug 17674: Handling of special cases This patch adds handling for those 2 particular cases: 1. Deleting the expected issue 2. Deleting a serial and it's item, but one item cannot be deleted To test: 1.1 Create a subscription that creates items 1.2 Receive two issues 1.3 Use the new button to delete the 'Expected' issue 1.4 The issue is correctly deleted, but the new expected issue created to replace the one which has been deleted don't have a published or received date and is show in it's own tab. 1.5 Apply patch 1.6 Repeat steps 1.1 through 1.4 => The new expected issue is created correctly 2.1 Use the same serial as in 1 or repeat step 1.1 and 1.2 2.2 Check out one of the items 2.3 Delete the corresponding serial and check box to delete items 2.4 The serial is deleted, but not the item. => No feedback is show to the user 2.5 Apply patch 2.6 Repeat step 2.1 through 2.4 => As before, the serial is deleted, but not the item => This time, an error is shown to inform the user Signed-off-by: sonia <sonia.bouis@univ-lyon3.fr> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
(In reply to Maryse Simard from comment #131) > (In reply to Jonathan Druart from comment #130) > > I'd say you should not call DelItem directly, but DelItemCheck instead. > > This is what has been done in the follow up patch. Indeed! I forgot to squash before reviewing them.
Nice work everyone! Pushed to master for 20.05
enhancement not backported to 19.11.x
*** Bug 12157 has been marked as a duplicate of this bug. ***
*** Bug 27611 has been marked as a duplicate of this bug. ***