Maybe it could be fixed, but I have also a plan to display the title if the record is deleted (taking info from deletedbiblio & deletedbiblioitems ). See bug 10758 (I need to write a message to koha-devel, because I don't know the best way to do that...) M. Saby Showing the title is great, but the line should still be allowed to be deleted. Of course ;-) And it is probably muuuch easier to do if it only a javascript pbm! So if anyone want to make a patch for this bug I will test it. Mathieu It is not only a js pbm, because without biblionumber, addorder.pl cannot delete the order... The biblionumber is needed by Acquisitions::DelOrder sub. So I made changes to addorder.pl and to DelOrder sub (which it not used in code). Now, DelOrder sub takes the ordernumber before the biblionumber. And if there is no biblionumber given, it does not try to suppress items. ==== commit message This patch fixes 2 issues : (medium issue) It is not possible to delete an order in a basket if the biblio record is deleted. (minor issue) There is a useless hyperlink around "Deleted bibliographic notice", and "notice" is a frenchism for "record" I had to change the behavior of Acquisitions::DelOrder (which is used only by addorder.pl file), a line in addorder.pl, and a few lines in template basket.tt. To test : 1. Fill a basket with some records (some records with items or holds, some without items) 2. Supress one of the record 3. In the basket (refresh the screen) check that "Deleted bibliographic notice" line was replaced with "Deleted bibliographic record", without hyperlink arround 4. Delete this order with the link "Delete order" to the right column of the table 5. Check there is a new line in the "Deleted orders" table 6. Check there is no regression : felete an other order (one with no item), then check you cannot delete an order with an item (like before patch) Mathieu Created attachment 21106 [details] [review] [PATCH] Bug 10869: Allow the deletion of order line if the record is deleted Oups, by "(which it not used in code)" I meant "which it not used elsewhere in code" of course. So there should be no side effects. Mathieu *** Bug 5781 has been marked as a duplicate of this bug. *** Created attachment 21294 [details] [review] Bug 10869: Allow the deletion of order line if the record is deleted Same patch (fixing trailing whitespaces) M. Saby Created attachment 21295 [details] [review] [PATCH] Bug 10869: Allow the deletion of order line if the record is deleted Sorry, same patch again. I've just seen and fix 2 typos. M. Saby Patch applied cleanly, go forth and signoff Is it a duplicate of 9780 ? It seems redundant. Patch tested with a sandbox, by Cedric Vita <cedric.vita@dracenie.com> Created attachment 21480 [details] [review] Bug 10869: Allow the deletion of order line if the record is deleted This patch fixes 2 issues : (medium issue) It is not possible to delete an order in a basket if the biblio record is deleted. (minor issue) There is a useless hyperlink around "Deleted bibliographic notice", and "notice" is a frenchism for "record" I had to change the behavior of Acquisitions::DelOrder (which is used only by addorder.pl file), a line in addorder.pl, and a few lines in template basket.tt. To test : 1. Fill a basket with some records (some records with items or holds, some without items) 2. Supress one of the record 3. In the basket (refresh the screen) check that "Deleted bibliographic notice" line was replaced with "Deleted bibliographic record", without hyperlink arround 4. Delete this order with the link "Delete order" to the right column of the table 5. Check there is a new line in the "Deleted orders" table 6. Check there is no regression : felete an other order (one with no item), then check you cannot delete an order with an item (like before patch) Signed-off-by: Cedric Vita <cedric.vita@dracenie.com> Note : this patch is a bugfix, that should be applied to maintenance versions of Koha. But I also made an other patch with I consider as an ENH, which improves the behavior : http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10758 (which depends on 10869, so it can't be signed off on a sandbox) Mathieu And thank you Cédric! Could someone QA, so that depending patch (Bug 10758) could be signed on sandbox once this one is pushed? Mathieu QA comments: sorry guys, I think there's a regression: I could delete an order that has been already recieved. I tried without the patch, and I couldn't, which is the expected behaviour Otherwise, there's a tab that makes qa tools complain, I'll provide a follow-up also fixing some indentation & "bibnum" labeling Created attachment 22104 [details] [review] Bug 10869 follow-up: indent & term * removing a few tabs * fixed a few silly indents * replaced bibnumb by biblionumber in DelOrder sub, for consistency (In reply to Paul Poulain from comment #17) > QA comments: > sorry guys, I think there's a regression: I could delete an order that has > been already recieved. I tried without the patch, and I couldn't, which is > the expected behaviour > I don't understand : did you test on a closed or on an opened basket? In normal acq workflow, 1 you add books in your basket => You can cancel orders, or add new ones 2 you close the basket => You cannot cancel orders or add new ones 3 you receive items, or cancel items in receipt module by creating a mock invoice and clicking on "cancel order" in receipt page (you are not supposed to go again basket itself). So for me the issue is when you want to cancel an order before closing the basket (between steps 1 and 2). If someone delete the record, you cannot, that's an issue. Mathieu Maybe the issue is that some ppl in some libraries send orders before closing the basket. I think it should be discourage : closing the basket is needed for calculation of expected shiping date, so the "normal" workflow implies closing the basket. If there are too many ways to do the same operation, it is impossible to fix bugs like that, and to make improvements, because you could always find someone for who the bug is a normal behavior ;-) As Nicole is the person who fill the bug first, what's her opinion? Mathieu Created attachment 22299 [details] [review] [PATCH 1/2] Bug 10869: Allow the deletion of order line if the record is deleted Patch Rebased I suppose adding a UT for DelOrder could be a good point too. Mathieu Created attachment 22300 [details] [review] [PATCH] Bug 10869: Allow the deletion of order line if the record is deleted My last rebasing was wrong... Paul, I integrated your changes in the main patch, as they were obvious. I'd like to write a UT for DelOrder, but I see more and more different files in t/db_dependant/Acquisition/ When a UT does belong to db_dependant/Acquisition.t, and when to separate file? For me it would be more logic to put all the UT for a .pm file in a single .t file, so we could reuse mock records or orders created for the test. Is there a rule somewhere ? Mathieu In fact, passing the biblionumber to DelBiblio seems useless. It can be retreived inside the sub, with the ordernumber. I will rewrite the sub so that it takes only the ordernumber. Mathieu Created attachment 22342 [details] [review] [PATCH 2/2] Bug 10869 Followup : change DelOrder and adds UT Changes made to DelOrder : Returns undef if no param DelOrder does no more need a biblionumber. It can be retreived inside the sub. DelOrder is now cleaning aqorders_items. UT added for DelOrder To test : A. With syspref AcqCreateItem = on placing an order 1. Fill a basket with some orders (some records with already existing items or holds, some without items). New items linked with the order will be created 2. Keep the basket open 3. In the catalog, supress all the items attached to one of the record used in the basket, then suppress the record 4. In the basket, check you can delete the order that was using deleted record (click on link "Delete order" in the right column of the table) 5. After deleting the order, check there is a new line in the "Deleted orders" table 6. Regression test : try to delete other orders, and check the deletion of the order implies the deletion of items created when ordering a/ If no items were attached to the record before you created the basket, you can delete some other orders, AND their record. Try to delete order and record. b/ If some items were attached to the record before you created the basket, you can delete some other orders, BUT NOT their record. Try to delete only order. After you have deleted it, the items created when ordering must have been deleted. 7. Add some other orders to your basket (some records with already existing items or holds, some without items). New items linked with the order will be created 8. In the catalog, supress all the items attached to one of the record used in the basket, then suppress the record 9. Close the basket 10. Go on receipt page 11. Search the orders of your basket 12. In receipt page, check you can delete the order that was using deleted record (click on link "Delete order" in the right column of the table) 13. Regression test : try to delete other orders a/ If no items were attached to the record before you created the basket, you can delete some other orders, AND their record. Try to delete order and record. b/ If some items were attached to the record before you created the basket, you can delete some other orders, BUT NOT their record. Try to delete only order. After you have deleted it, the items created when ordering must have been deleted. B. With syspref AcqCreateItem = on receiving an order 1. Fill a basket with some orders (some records with already existing items or holds, some without items). 2. Keep the basket open 3. In the catalog, supress all the items attached to one of the record used in the basket, then suppress the record 4. In the basket, check you can delete the order that was using deleted record (click on link "Delete order" in the right column of the table) 5. After deleting the order, check there is a new line in the "Deleted orders" table 6. Regression test : try to delete other orders a/ If no items were attached to the record before you created the basket, you can delete some other orders, AND their record. Try to delete order and record. b/ If some items were attached to the record before you created the basket, you can delete some other orders, BUT NOT their record. Try to delete only order. 7. Add some other orders to your basket (some records with already existing items or holds, some without items). 8. In the catalog, supress all the items attached to one of the record used in the basket, then suppress the record 9. Close the basket 10. Go on receipt page 11. Search the orders of your basket 12. In receipt page, check you can delete the order that was using deleted record (click on link "Delete order" in the right column of the table) 13. Regression test : try to delete other orders a/ If no items were attached to the record before you created the basket, you can delete some other orders, AND their record. Try to delete order and record. b/ If some items were attached to the record before you created the basket, you can delete some other orders, BUT NOT their record. Try to delete only order. and to test the UT : prove t/db_dependant/Acquisition.t I ask for a 2d signoff, as my followup changes a lot of things in DelBiblio. Mathieu Patch applied cleanly, go forth and signoff Created attachment 22676 [details] [review] [PATCH 2/2] Bug 10869 Followup : change DelOrder and adds UT rebased Of course, UT cannot be tested on a sandbox, but the other aspects of the patch can. Mathieu Bug 10869 - can't delete order line if title deleted 22300 - [PATCH] Bug 10869: Allow the deletion of order line if the record is deleted 22676 - [PATCH 2/2] Bug 10869 Followup : change DelOrder and adds UT Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 10869: Allow the deletion of order line if the record is deleted Applying: Bug 10869 Followup : change DelOrder and adds UT fatal: sha1 information is lacking or useless (C4/Acquisition.pm). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 10869 Followup : change DelOrder and adds UT The copy of the patch that failed is found in: /home/christopher/git/koha/.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/PATCH-22-Bug-10869-Followup--change-DelOrder-and-a-dbVEZx.patch Created attachment 23839 [details] [review] [PATCH 2/2] Bug 10869 Followup : change DelOrder and adds UT rebased Just a *dumb* question: Why remove the biblio parameter and look it up in the database if you already had it in the first place? (In reply to M. de Rooy from comment #31) > Just a *dumb* question: Why remove the biblio parameter and look it up in > the database if you already had it in the first place? Well, you may be right ;-) I thought it was a good thing to have as less parameters as possible, to make the code in .pl files more simple. But it was maybe a bad idea, for performance reasons. The code without this followup was working well (not sure it is still the case), and was signed off. Paul failed QA in comment 17, but I answer him in comment 19. If you think the followup is not needed, I can obsolete it, and put back the bug in "Signed off" state. Mathieu (In reply to mathieu saby from comment #32) > Well, you may be right ;-) > I thought it was a good thing to have as less parameters as possible, to > make the code in .pl files more simple. But it was maybe a bad idea, for > performance reasons. > The code without this followup was working well (not sure it is still the > case), and was signed off. Paul failed QA in comment 17, but I answer him in > comment 19. > If you think the followup is not needed, I can obsolete it, and put back the > bug in "Signed off" state. I would suggest to combine them, since the followup also includes a unit test. (In reply to M. de Rooy from comment #33) > (In reply to mathieu saby from comment #32) > > Well, you may be right ;-) > > I thought it was a good thing to have as less parameters as possible, to > > make the code in .pl files more simple. But it was maybe a bad idea, for > > performance reasons. > > The code without this followup was working well (not sure it is still the > > case), and was signed off. Paul failed QA in comment 17, but I answer him in > > comment 19. > > If you think the followup is not needed, I can obsolete it, and put back the > > bug in "Signed off" state. > > I would suggest to combine them, since the followup also includes a unit > test. Are you sure it is the best way to proceed? as the 1st patch was already signed off, I wanted to keep the signoff. Mathieu (In reply to mathieu saby from comment #34) > > I would suggest to combine them, since the followup also includes a unit > > test. > > Are you sure it is the best way to proceed? as the 1st patch was already > signed off, I wanted to keep the signoff. In that case you should amend the second patch accordingly. (In reply to M. de Rooy from comment #35) > (In reply to mathieu saby from comment #34) > > > I would suggest to combine them, since the followup also includes a unit > > > test. > > > > Are you sure it is the best way to proceed? as the 1st patch was already > > signed off, I wanted to keep the signoff. > > In that case you should amend the second patch accordingly. I don't understand: I wrote it a long time ago, but I seem to remember that the UT added in the 2d patch was made to test the function DelOrder as amended by patches 1 and 2. So what do you want me to do? Mathieu Created attachment 24743 [details] [review] Bug 10869: Allow the deletion of order line if the record is deleted [I integrated a small followup by P. Poulain, changing the name of $bibnum to $biblionumber in DelOrder It was more simple for rebasing] This patch fixes 2 issues : (medium issue) It is not possible to delete an order in a basket if the biblio record is deleted. (minor issue) There is a useless hyperlink around "Deleted bibliographic notice", and "notice" is a frenchism for "record" I had to change the behavior of Acquisitions::DelOrder (which is used only by addorder.pl file), a line in addorder.pl, and a few lines in template basket.tt. To test : 1. Fill a basket with some records (some records with items or holds, some without items) 2. Supress one of the record 3. In the basket (refresh the screen) check that "Deleted bibliographic notice" line was replaced with "Deleted bibliographic record", without hyperlink arround 4. Delete this order with the link "Delete order" to the right column of the table 5. Check there is a new line in the "Deleted orders" table 6. Check there is no regression : felete an other order (one with no item), then check you cannot delete an order with an item (like before patch) Signed-off-by: Cedric Vita <cedric.vita@dracenie.com> Created attachment 24744 [details] [review] Bug 10869 Followup: Unit tests for DelOrder DelOrder now returns undef if no ordernumber passed. This patch also adds unit tests for DelOrder. Test plan: A. With syspref AcqCreateItem = on placing an order 1. Fill a basket with some orders (some records with already existing items or holds, some without items). New items linked with the order will be created 2. Keep the basket open 3. In the catalog, supress all the items attached to one of the record used in the basket, then suppress the record 4. In the basket, check you can delete the order that was using deleted record (click on link "Delete order" in the right column of the table) 5. After deleting the order, check there is a new line in the "Deleted orders" table 6. Regression test : try to delete other orders, and check the deletion of the order implies the deletion of items created when ordering a/ If no items were attached to the record before you created the basket, you can delete some other orders, AND their record. Try to delete order and record. b/ If some items were attached to the record before you created the basket, you can delete some other orders, BUT NOT their record. Try to delete only order. After you have deleted it, the items created when ordering must have been deleted. 7. Add some other orders to your basket (some records with already existing items or holds, some without items). New items linked with the order will be created 8. In the catalog, supress all the items attached to one of the record used in the basket, then suppress the record 9. Close the basket 10. Go on receipt page 11. Search the orders of your basket 12. In receipt page, check you can delete the order that was using deleted record (click on link "Delete order" in the right column of the table) 13. Regression test : try to delete other orders a/ If no items were attached to the record before you created the basket, you can delete some other orders, AND their record. Try to delete order and record. b/ If some items were attached to the record before you created the basket, you can delete some other orders, BUT NOT their record. Try to delete only order. After you have deleted it, the items created when ordering must have been deleted. B. With syspref AcqCreateItem = on receiving an order 1. Fill a basket with some orders (some records with already existing items or holds, some without items). 2. Keep the basket open 3. In the catalog, supress all the items attached to one of the record used in the basket, then suppress the record 4. In the basket, check you can delete the order that was using deleted record (click on link "Delete order" in the right column of the table) 5. After deleting the order, check there is a new line in the "Deleted orders" table 6. Regression test : try to delete other orders a/ If no items were attached to the record before you created the basket, you can delete some other orders, AND their record. Try to delete order and record. b/ If some items were attached to the record before you created the basket, you can delete some other orders, BUT NOT their record. Try to delete only order. 7. Add some other orders to your basket (some records with already existing items or holds, some without items). 8. In the catalog, supress all the items attached to one of the record used in the basket, then suppress the record 9. Close the basket 10. Go on receipt page 11. Search the orders of your basket 12. In receipt page, check you can delete the order that was using deleted record (click on link "Delete order" in the right column of the table) 13. Regression test : try to delete other orders a/ If no items were attached to the record before you created the basket, you can delete some other orders, AND their record. Try to delete order and record. b/ If some items were attached to the record before you created the basket, you can delete some other orders, BUT NOT their record. Try to delete only order. And to test the UT: prove t/db_dependent/Acquisition.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Amended the patch as to removing biblio parameter in DelOrder. There is no need to remove the parameter if you will search for biblionumber later on. I also suggest to handle the deletes in aqorders_items via constraint. Added a followup on report 11617 for that purpose. (In reply to mathieu saby from comment #36) > I don't understand: > I wrote it a long time ago, but I seem to remember that the UT added in the > 2d patch was made to test the function DelOrder as amended by patches 1 and > 2. So what do you want me to do? Please have a look at the amended second patch. If you agree, please retest and put an additional signoff on it. Also I added a follow-up on report 11617 to realize the deletion of items in aqorders_items. You are welcome to test that too :) Thanks. OK, I did not understand your point of view. I thought you agreed for removing biblionumber param... I'm going to test your patch asap Mathieu (In reply to mathieu saby from comment #40) > OK, I did not understand your point of view. I thought you agreed for > removing biblionumber param... Hi Matthieu, My question about removing the biblionumber actually was inspired by the thought that it was better to keep it than remove it and look it up again with another database query.. Marcel I'll take a look next week Mathieu I'm not sure of the origin of the conflict. I prefer to investigate a little more to avoid a mistake. Mathieu In fact the conflict was trivial ;-) Created attachment 24992 [details] [review] [PATCH] Bug 10869: Allow the deletion of order line if the record is deleted Marcel, I got a bad result with prove t/db_dependent/Acquisition.t # Failed test 'DelOrders delete items linked with cancelled order in aqorders_items table' # at t/db_dependent/Acquisition.t line 273. # got: '2' # expected: '0' # Looks like you failed 1 test of 67. t/db_dependent/Acquisition.t .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/67 subtests If I understand well, it is because, as you stated in your comment, aqorders_items is not cleared automaticaly by DelOrder, but will be when Bug 11617 is pushed. As Bug 11617 is not yet pushed, and Katrin raised an objection to it, I would prefer to put this one in Discussion, with status 'Blocked by Bug 11617'. Do you agree with that? Mathieu Created attachment 25000 [details] [review] Bug 10869 Followup: Unit tests for DelOrder DelOrder now returns undef if no ordernumber passed. This patch also adds unit tests for DelOrder. Test plan: A. With syspref AcqCreateItem = on placing an order 1. Fill a basket with some orders (some records with already existing items or holds, some without items). New items linked with the order will be created 2. Keep the basket open 3. In the catalog, supress all the items attached to one of the record used in the basket, then suppress the record 4. In the basket, check you can delete the order that was using deleted record (click on link "Delete order" in the right column of the table) 5. After deleting the order, check there is a new line in the "Deleted orders" table 6. Regression test : try to delete other orders, and check the deletion of the order implies the deletion of items created when ordering a/ If no items were attached to the record before you created the basket, you can delete some other orders, AND their record. Try to delete order and record. b/ If some items were attached to the record before you created the basket, you can delete some other orders, BUT NOT their record. Try to delete only order. After you have deleted it, the items created when ordering must have been deleted. 7. Add some other orders to your basket (some records with already existing items or holds, some without items). New items linked with the order will be created 8. In the catalog, supress all the items attached to one of the record used in the basket, then suppress the record 9. Close the basket 10. Go on receipt page 11. Search the orders of your basket 12. In receipt page, check you can delete the order that was using deleted record (click on link "Delete order" in the right column of the table) 13. Regression test : try to delete other orders a/ If no items were attached to the record before you created the basket, you can delete some other orders, AND their record. Try to delete order and record. b/ If some items were attached to the record before you created the basket, you can delete some other orders, BUT NOT their record. Try to delete only order. After you have deleted it, the items created when ordering must have been deleted. B. With syspref AcqCreateItem = on receiving an order 1. Fill a basket with some orders (some records with already existing items or holds, some without items). 2. Keep the basket open 3. In the catalog, supress all the items attached to one of the record used in the basket, then suppress the record 4. In the basket, check you can delete the order that was using deleted record (click on link "Delete order" in the right column of the table) 5. After deleting the order, check there is a new line in the "Deleted orders" table 6. Regression test : try to delete other orders a/ If no items were attached to the record before you created the basket, you can delete some other orders, AND their record. Try to delete order and record. b/ If some items were attached to the record before you created the basket, you can delete some other orders, BUT NOT their record. Try to delete only order. 7. Add some other orders to your basket (some records with already existing items or holds, some without items). 8. In the catalog, supress all the items attached to one of the record used in the basket, then suppress the record 9. Close the basket 10. Go on receipt page 11. Search the orders of your basket 12. In receipt page, check you can delete the order that was using deleted record (click on link "Delete order" in the right column of the table) 13. Regression test : try to delete other orders a/ If no items were attached to the record before you created the basket, you can delete some other orders, AND their record. Try to delete order and record. b/ If some items were attached to the record before you created the basket, you can delete some other orders, BUT NOT their record. Try to delete only order. And to test the UT: prove t/db_dependent/Acquisition.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Amended the patch as to removing biblio parameter in DelOrder. There is no need to remove the parameter if you will search for biblionumber later on. I also suggest to handle the deletes in aqorders_items via constraint. Added a followup on report 11617 for that purpose. Follow-up resubmitted to restore patch order. Will still answer previous comment. Created attachment 25001 [details] [review] Bug 10869 Followup: Unit tests for DelOrder DelOrder now returns undef if no ordernumber passed. This patch also adds unit tests for DelOrder. Test plan: A. With syspref AcqCreateItem = on placing an order 1. Fill a basket with some orders (some records with already existing items or holds, some without items). New items linked with the order will be created 2. Keep the basket open 3. In the catalog, supress all the items attached to one of the record used in the basket, then suppress the record 4. In the basket, check you can delete the order that was using deleted record (click on link "Delete order" in the right column of the table) 5. After deleting the order, check there is a new line in the "Deleted orders" table 6. Regression test : try to delete other orders, and check the deletion of the order implies the deletion of items created when ordering a/ If no items were attached to the record before you created the basket, you can delete some other orders, AND their record. Try to delete order and record. b/ If some items were attached to the record before you created the basket, you can delete some other orders, BUT NOT their record. Try to delete only order. After you have deleted it, the items created when ordering must have been deleted. 7. Add some other orders to your basket (some records with already existing items or holds, some without items). New items linked with the order will be created 8. In the catalog, supress all the items attached to one of the record used in the basket, then suppress the record 9. Close the basket 10. Go on receipt page 11. Search the orders of your basket 12. In receipt page, check you can delete the order that was using deleted record (click on link "Delete order" in the right column of the table) 13. Regression test : try to delete other orders a/ If no items were attached to the record before you created the basket, you can delete some other orders, AND their record. Try to delete order and record. b/ If some items were attached to the record before you created the basket, you can delete some other orders, BUT NOT their record. Try to delete only order. After you have deleted it, the items created when ordering must have been deleted. B. With syspref AcqCreateItem = on receiving an order 1. Fill a basket with some orders (some records with already existing items or holds, some without items). 2. Keep the basket open 3. In the catalog, supress all the items attached to one of the record used in the basket, then suppress the record 4. In the basket, check you can delete the order that was using deleted record (click on link "Delete order" in the right column of the table) 5. After deleting the order, check there is a new line in the "Deleted orders" table 6. Regression test : try to delete other orders a/ If no items were attached to the record before you created the basket, you can delete some other orders, AND their record. Try to delete order and record. b/ If some items were attached to the record before you created the basket, you can delete some other orders, BUT NOT their record. Try to delete only order. 7. Add some other orders to your basket (some records with already existing items or holds, some without items). 8. In the catalog, supress all the items attached to one of the record used in the basket, then suppress the record 9. Close the basket 10. Go on receipt page 11. Search the orders of your basket 12. In receipt page, check you can delete the order that was using deleted record (click on link "Delete order" in the right column of the table) 13. Regression test : try to delete other orders a/ If no items were attached to the record before you created the basket, you can delete some other orders, AND their record. Try to delete order and record. b/ If some items were attached to the record before you created the basket, you can delete some other orders, BUT NOT their record. Try to delete only order. And to test the UT: prove t/db_dependent/Acquisition.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Amended the patch as to removing biblio parameter in DelOrder. There is no need to remove the parameter if you will search for biblionumber later on. Deleting records in aqorders_items is discussed on bug 11617. Updated the commit message of the second patch too. Created attachment 25002 [details] [review] Bug 10869 Followup: Unit tests for DelOrder DelOrder now returns undef if no ordernumber passed. This patch also adds unit tests for DelOrder. Test plan: A. With syspref AcqCreateItem = on placing an order 1. Fill a basket with some orders (some records with already existing items or holds, some without items). New items linked with the order will be created 2. Keep the basket open 3. In the catalog, supress all the items attached to one of the record used in the basket, then suppress the record 4. In the basket, check you can delete the order that was using deleted record (click on link "Delete order" in the right column of the table) 5. After deleting the order, check there is a new line in the "Deleted orders" table 6. Regression test : try to delete other orders, and check the deletion of the order implies the deletion of items created when ordering a/ If no items were attached to the record before you created the basket, you can delete some other orders, AND their record. Try to delete order and record. b/ If some items were attached to the record before you created the basket, you can delete some other orders, BUT NOT their record. Try to delete only order. After you have deleted it, the items created when ordering must have been deleted. 7. Add some other orders to your basket (some records with already existing items or holds, some without items). New items linked with the order will be created 8. In the catalog, supress all the items attached to one of the record used in the basket, then suppress the record 9. Close the basket 10. Go on receipt page 11. Search the orders of your basket 12. In receipt page, check you can delete the order that was using deleted record (click on link "Delete order" in the right column of the table) 13. Regression test : try to delete other orders a/ If no items were attached to the record before you created the basket, you can delete some other orders, AND their record. Try to delete order and record. b/ If some items were attached to the record before you created the basket, you can delete some other orders, BUT NOT their record. Try to delete only order. After you have deleted it, the items created when ordering must have been deleted. B. With syspref AcqCreateItem = on receiving an order 1. Fill a basket with some orders (some records with already existing items or holds, some without items). 2. Keep the basket open 3. In the catalog, supress all the items attached to one of the record used in the basket, then suppress the record 4. In the basket, check you can delete the order that was using deleted record (click on link "Delete order" in the right column of the table) 5. After deleting the order, check there is a new line in the "Deleted orders" table 6. Regression test : try to delete other orders a/ If no items were attached to the record before you created the basket, you can delete some other orders, AND their record. Try to delete order and record. b/ If some items were attached to the record before you created the basket, you can delete some other orders, BUT NOT their record. Try to delete only order. 7. Add some other orders to your basket (some records with already existing items or holds, some without items). 8. In the catalog, supress all the items attached to one of the record used in the basket, then suppress the record 9. Close the basket 10. Go on receipt page 11. Search the orders of your basket 12. In receipt page, check you can delete the order that was using deleted record (click on link "Delete order" in the right column of the table) 13. Regression test : try to delete other orders a/ If no items were attached to the record before you created the basket, you can delete some other orders, AND their record. Try to delete order and record. b/ If some items were attached to the record before you created the basket, you can delete some other orders, BUT NOT their record. Try to delete only order. And to test the UT: prove t/db_dependent/Acquisition.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Amended the patch as to removing biblio parameter in DelOrder. There is no need to remove the parameter if you will search for biblionumber later on. Deleting records in aqorders_items is discussed on bug 11617. Created attachment 25003 [details] [review] Bug 10869 Followup: Unit tests for DelOrder DelOrder now returns undef if no ordernumber passed. This patch also adds unit tests for DelOrder. Test plan: A. With syspref AcqCreateItem = on placing an order 1. Fill a basket with some orders (some records with already existing items or holds, some without items). New items linked with the order will be created 2. Keep the basket open 3. In the catalog, supress all the items attached to one of the record used in the basket, then suppress the record 4. In the basket, check you can delete the order that was using deleted record (click on link "Delete order" in the right column of the table) 5. After deleting the order, check there is a new line in the "Deleted orders" table 6. Regression test : try to delete other orders, and check the deletion of the order implies the deletion of items created when ordering a/ If no items were attached to the record before you created the basket, you can delete some other orders, AND their record. Try to delete order and record. b/ If some items were attached to the record before you created the basket, you can delete some other orders, BUT NOT their record. Try to delete only order. After you have deleted it, the items created when ordering must have been deleted. 7. Add some other orders to your basket (some records with already existing items or holds, some without items). New items linked with the order will be created 8. In the catalog, supress all the items attached to one of the record used in the basket, then suppress the record 9. Close the basket 10. Go on receipt page 11. Search the orders of your basket 12. In receipt page, check you can delete the order that was using deleted record (click on link "Delete order" in the right column of the table) 13. Regression test : try to delete other orders a/ If no items were attached to the record before you created the basket, you can delete some other orders, AND their record. Try to delete order and record. b/ If some items were attached to the record before you created the basket, you can delete some other orders, BUT NOT their record. Try to delete only order. After you have deleted it, the items created when ordering must have been deleted. B. With syspref AcqCreateItem = on receiving an order 1. Fill a basket with some orders (some records with already existing items or holds, some without items). 2. Keep the basket open 3. In the catalog, supress all the items attached to one of the record used in the basket, then suppress the record 4. In the basket, check you can delete the order that was using deleted record (click on link "Delete order" in the right column of the table) 5. After deleting the order, check there is a new line in the "Deleted orders" table 6. Regression test : try to delete other orders a/ If no items were attached to the record before you created the basket, you can delete some other orders, AND their record. Try to delete order and record. b/ If some items were attached to the record before you created the basket, you can delete some other orders, BUT NOT their record. Try to delete only order. 7. Add some other orders to your basket (some records with already existing items or holds, some without items). 8. In the catalog, supress all the items attached to one of the record used in the basket, then suppress the record 9. Close the basket 10. Go on receipt page 11. Search the orders of your basket 12. In receipt page, check you can delete the order that was using deleted record (click on link "Delete order" in the right column of the table) 13. Regression test : try to delete other orders a/ If no items were attached to the record before you created the basket, you can delete some other orders, AND their record. Try to delete order and record. b/ If some items were attached to the record before you created the basket, you can delete some other orders, BUT NOT their record. Try to delete only order. And to test the UT: prove t/db_dependent/Acquisition.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Amended the patch as to removing biblio parameter in DelOrder. There is no need to remove the parameter if you will search for biblionumber later on. Deleting records in aqorders_items is discussed on bug 11617. (In reply to mathieu saby from comment #46) > If I understand well, it is because, as you stated in your comment, > aqorders_items is not cleared automaticaly by DelOrder, but will be when Bug > 11617 is pushed. Yes. For now I removed this one test from your patch. It still checks in items, no longer in aqorders_items. Note that if there would be a cascaded delete, I think we should not have to check MySQL functionality either. So the test is no longer needed. > As Bug 11617 is not yet pushed, and Katrin raised an objection to it, I > would prefer to put this one in Discussion, with status 'Blocked by Bug > 11617'. Strictly speaking, they can be handled independently now. But the discussion on bug 11617 made me think somewhat more about it. If the user wants to delete an order, it is actually obvious that he wants to delete records in aqorders_items too. You did in the former version (and I moved it to another report :) But in the meantime we are now only deleting the real item records here. And the question is: Did the user understand that, and should he have the possibility to not remove the items (so only the order and aqorders_items)? Should we add two options: Delete order / Delete order and items. Note that Delete order and catalog record (or biblio) should obviously include items. BTW Would biblio record be a better description than catalog record in the second [or third] option? You can also misread it as follows now: First delete the order and then catalog [read: edit] the record. When you say biblio record, it is a noun and no verb. Sorry for all these comments.. ;) Setting to In Discussion, as you suggested. The questions on order level actually pertain to basket level also. If we delete a basket, we do warn the user that he will lose his items. But he does not have a choice between "Delete basket and orders" and "Delete baskets, orders and items". Is this getting too complicated now? (In reply to M. de Rooy from comment #54) > The questions on order level actually pertain to basket level also. If we > delete a basket, we do warn the user that he will lose his items. But he > does not have a choice between "Delete basket and orders" and "Delete > baskets, orders and items". > Is this getting too complicated now? We are/have been discussing deleting acquisition information on several reports now. The constraint on itemnumber in aqorders_items did not reach consensus (bug 11617). Please note also bug 8991. I think we should move the old completed baskets (with orders) to an archive table and offer the possibility to delete that too. What do you think? With respect to the angle of this report, I think it is somewhat strange to delete biblio and item records but leave an order (read: order line) in cancelled status with a Deleted bibliographic record, can't find title-line. There is much more information on a biblio record and we do not hesitate to delete it (with a copy in deletedbiblio) but we keep a deleted order (line) without title. Do we need some more community consensus on how to move such things further in Acquistion before deciding on several individual patches? I don't like BZ 8991 ;-) BZ 10869 was made to fix a bug reported by Nicole. And for me too it sounds like a bug, at least at the time I wrote it. But it is not the only patch I have written regarding those points. There is also: - BZ 11224 for adding new UT - BZ 10758 for showing bibliographic information of deleted records in acquisitions BZ 10758 needs to be rewritten, and depends on BZ 11224 My logic was to write or improve UT for current acq subs (11224), so that 10758 could be reviewed more easily. Personal note: I'm not working anymore officially on Koha, so I won't be able to spend much time to rewrite those patches (however I will do for BZ 11224). So if community does not like 10869, I won't fight for it... Mathieu (In reply to mathieu saby from comment #56) > But it is not the only patch I have written regarding those points. There is > also: > - BZ 11224 for adding new UT > - BZ 10758 for showing bibliographic information of deleted records in > acquisitions That is helpful! > Personal note: I'm not working anymore officially on Koha, so I won't be > able to spend much time to rewrite those patches (however I will do for BZ > 11224). So if community does not like 10869, I won't fight for it... Thanks for your time. I do not think that the community does not like it by the way. But everyone is busy.. It may be quite hard to know what 'the community' likes or dislikes :) Created attachment 26692 [details] [review] [PATCH] Bug 10869: Allow the deletion of order line if the record is deleted This patch fixes 2 issues : (medium issue) It is not possible to delete an order in a basket if the biblio record is deleted. (minor issue) There is a useless hyperlink around "Deleted bibliographic notice", and "notice" is a frenchism for "record" Biblionumber was passed to the function as 1st parameter, which made impossible to suppress an order whose associated biblio record was suppressed. Moreover, the biblionumber is not needed to get and suppress the order, but only to suppress items, in a second step. Change made to Acquisitions::DelOrder: - I changed the order of the 2 parameters: now $ordernumber as 1st parameter and $biblionumber as 2d parameter - I renamed $bibnum to $biblionumber - I removed $biblionumber from the main SQL query of the function - I suppressed useless $sth->finish - if no $biblionumber is provided, Koha does not try to suppress the items linked created by acq process Change made to the calls to Acquisitions::DelOrder in Koha's code, to change the order of the parameters - in addorder.pl - in basket.pl - in UT t/db_dependant/Acquisition.t Change made in template basket.tt. This bug was signed off by Cedric some time ago, but it needs a second sign off, because I had to made some little changes. To test : 1. Fill a basket with some records (some records with items or holds, some without items) 2. Supress one of the record 3. In the basket (refresh the screen) check that "Deleted bibliographic notice" line was replaced with "Deleted bibliographic record", without hyperlink arround 4. Delete this order with the link "Delete order" to the right column of the table 5. Check there is a new line in the "Deleted orders" table 6. Check there is no regression : delete an other order (one with no item), then check you cannot delete an order with an item (like before patch) Created attachment 26693 [details] [review] [PATCH 2/2] Bug 10869 Followup: Unit tests for DelOrder [The best would be to add a second sign-off to this patch, as I had to to a little change, to take into account the recent rewrite of t/db_dependent/Acquisition.t: $ordernumber3 is now $ordernumbers[2] Mathieu Saby] DelOrder now returns undef if no ordernumber passed. This patch also adds unit tests for DelOrder. Test plan: A. With syspref AcqCreateItem = on placing an order 1. Fill a basket with some orders (some records with already existing items or holds, some without items). New items linked with the order will be created 2. Keep the basket open 3. In the catalog, supress all the items attached to one of the record used in the basket, then suppress the record 4. In the basket, check you can delete the order that was using deleted record (click on link "Delete order" in the right column of the table) 5. After deleting the order, check there is a new line in the "Deleted orders" table 6. Regression test : try to delete other orders, and check the deletion of the order implies the deletion of items created when ordering a/ If no items were attached to the record before you created the basket, you can delete some other orders, AND their record. Try to delete order and record. b/ If some items were attached to the record before you created the basket, you can delete some other orders, BUT NOT their record. Try to delete only order. After you have deleted it, the items created when ordering must have been deleted. 7. Add some other orders to your basket (some records with already existing items or holds, some without items). New items linked with the order will be created 8. In the catalog, supress all the items attached to one of the record used in the basket, then suppress the record 9. Close the basket 10. Go on receipt page 11. Search the orders of your basket 12. In receipt page, check you can delete the order that was using deleted record (click on link "Delete order" in the right column of the table) 13. Regression test : try to delete other orders a/ If no items were attached to the record before you created the basket, you can delete some other orders, AND their record. Try to delete order and record. b/ If some items were attached to the record before you created the basket, you can delete some other orders, BUT NOT their record. Try to delete only order. After you have deleted it, the items created when ordering must have been deleted. B. With syspref AcqCreateItem = on receiving an order 1. Fill a basket with some orders (some records with already existing items or holds, some without items). 2. Keep the basket open 3. In the catalog, supress all the items attached to one of the record used in the basket, then suppress the record 4. In the basket, check you can delete the order that was using deleted record (click on link "Delete order" in the right column of the table) 5. After deleting the order, check there is a new line in the "Deleted orders" table 6. Regression test : try to delete other orders a/ If no items were attached to the record before you created the basket, you can delete some other orders, AND their record. Try to delete order and record. b/ If some items were attached to the record before you created the basket, you can delete some other orders, BUT NOT their record. Try to delete only order. 7. Add some other orders to your basket (some records with already existing items or holds, some without items). 8. In the catalog, supress all the items attached to one of the record used in the basket, then suppress the record 9. Close the basket 10. Go on receipt page 11. Search the orders of your basket 12. In receipt page, check you can delete the order that was using deleted record (click on link "Delete order" in the right column of the table) 13. Regression test : try to delete other orders a/ If no items were attached to the record before you created the basket, you can delete some other orders, AND their record. Try to delete order and record. b/ If some items were attached to the record before you created the basket, you can delete some other orders, BUT NOT their record. Try to delete only order. And to test the UT: prove t/db_dependent/Acquisition.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Amended the patch as to removing biblio parameter in DelOrder. There is no need to remove the parameter if you will search for biblionumber later on. Deleting records in aqorders_items is discussed on bug 11617. I had to make some little changes to the main patch and to the followup, so the best would be to test again those patch and add new sign-off. I know Sonia Bouis from Lyon3 was interested with this bug. Mathieu It's ok for the page of the basket but I can't delete the command on the receipt page if the title is suppressed.... Created attachment 27270 [details] [review] [PATCH 3/3] Followup : update the receipt page (parcel.tt) Sorry, I forgot to change parcel.tt page. Here is a followup In main patch, I forgot to made some changes in the parcel.tt page This patch fixes that bu giving the librarian the ability to cancel an order in receipt page if the record is deleted. It also displays the message "Deleted bibliographic record, can't find title" in Summary Note that this behavior will be improved by bug 10758 To test : - create a basket and an order - delete the record used by that order - go on receipt page - in the line matching this order, you should see instead of the title "Deleted bibliographic record, can't find title" - click on "Delete order" : you should be able to delete the order - go to the basket, to check the order was properly cancelled - in receipt page, try to delete an other order (always linked to a record), to be sure there is no regression Please note that bug 10758 is linked to this bug (it will display the titles of deleted records in orders, instead of "Deleted record, can't find title"). There is no dependancy strictly speaking, so both can be tested, but bug 10758 is "bigger", so I suppose bug 10869 will be pushed first, and I will have then to make some changes to bug 10758. Mathieu There's a display problem in the receipt page (parcel.pl). In the column "summary", the title is changed by "Deleted bibliographic record, can't find title" for all the orders. When you click on receive, you can see the good title. If I understand your comment, it is not a display problem: Currently, if the record is deleted, the title is currently not given at all in the table with all orders to receive (in the summary column you just have [Add a note]), and it is not given either if you click on "Receive" link. The only changes made by this followup are - the ability to delete the order - the display of a message "Deleted title" in the summary column. For the reception page of an specific order linked to a deleted reocrd, I did not change anything. But I think trying to receive an order when the record has been deleted could be highly problematic (I think Koha recreates a new record). It could be discuss in other bug. So, I change back the status to "need signoff" Mathieu Oups, ok, in fact there is a problem if the record is NOT deleted. The message "Deleted record" is displayed and should not. Created attachment 27292 [details] [review] [PATCH 3/3] Bug 10869: Followup : update the receipt page (parcel.tt) (Fixed after test by Lyon3 : a bad name was used for a variable. Now "Deleted record..." should NOT be displayed if the record has not been deleted) In main patch, I forgot to made some changes in the parcel.tt page This patch fixes that bu giving the librarian the ability to cancel an order in receipt page if the record is deleted. It also displays the message "Deleted bibliographic record, can't find title" in Summary Note that this behavior will be improved by bug 10758 To test : - create a basket and an order - delete the record used by that order - go on receipt page - in the line matching this order, you should see instead of the title "Deleted bibliographic record, can't find title" - click on "Delete order" : you should be able to delete the order - go to the basket, to check the order was properly cancelled - in receipt page, try to delete an other order (always linked to a record), to be sure there is no regression Created attachment 28516 [details] [review] [PATCH 2/2] Bug 10869 Followup: Unit tests for DelOrder Patch rebased we can't understand why we should return on the page of reception to delete the record already deleted in the basket : - go on receipt page - in the line matching this order, you should see instead of the title "Deleted bibliographic record, can't find title" We created a basket and an order, then deleted the record used by that order On the receipt page, in the line matching this order, you should see instead of the title "Deleted bibliographic record, can't find title" impossible to delete the order In the basket, it was impossible too to delete the deleted record. In in receipt page, it was possible to delete an other order (always linked to a record) (In reply to Levast from comment #70) > we can't understand why we should return on the page of reception to delete > the record already deleted in the basket : > - go on receipt page > - in the line matching this order, you should see instead of the title > "Deleted bibliographic record, can't find title" Hello It is NOT a normal procedure, but a way to fix some mistakes that sometimes happen. When you try to delete a record used in some order, Koha now prevents you to do so with this warning : "Warning: This record is used in 1 order(s). Deleting it could cause serious issues on acquisition module. Are you sure you want to delete this record?". But it is a very recent behavior introduced last year by bug 9780. Before that, a librarian could very easily delete a record used in an order. And technically, it is still possible to do so... So, it happened in some libraries that some orders are linked with deleted records, and Koha does not allow you to suppress these orders, either in baskets (when the basket is open), or in the reception module (when the basket is closed, and the order is pending). The aim of this bug is to allow you to cancel these orders linked with deleted records in the same way as any other orders. I'm going to check if it works well before changing status. Mathieu Created attachment 29668 [details] [review] [PATCH 2/2] Bug 10869 Followup: Unit tests for DelOrder Unit tests need a little rebase I tested the 3 patches again. For me it works as it should work : Without the patches, you cannot cancel an order in a basket page or the reception page if the record used by this order was deleted before (that's not good practice, I agree, but those kind of things happen). With the patches, you can. Could someone test again please ? Mathieu I've tried to apply the peatch against master 3.17.00.011 Applying: Bug 10869: Allow the deletion of order line if the record is deleted Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging t/db_dependent/Acquisition.t Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt Auto-merging acqui/basket.pl Auto-merging acqui/addorder.pl Auto-merging C4/Acquisition.pm CONFLICT (content): Merge conflict in C4/Acquisition.pm Failed to merge in the changes. Patch failed at 0001 Bug 10869: Allow the deletion of order line if the record is deleted When you have resolved this problem run "git am --resolved". If you would prefer to skip this patch, instead run "git am --skip". To restore the original branch and stop patching run "git am --abort". So I pass the patch to "Patch doesn't apply" status. DelOrder sub was modified by Bug 12583 - DelItem takes $dbh in parameter (commit ba81cdcdb2806e3b60786ed47258a189703ccffb) "To delete an item, only the itemnumber should be mandatory. The DelItem routine can retrieve the biblionumber from the itemnumber." That was exaclty wat was doing my 1st patch, but I rewrote it after the remarks made by Marcel (comment 31). So I am a bit disappointed :( I won't touch it again for the moment In fact bug 12853 has nothing to do with this one, BUT bug 7162 (passed QA) will have aftereffects, as it rewrites DelOrder. So, I'll take a look again when bug 7162 is pushed M. Saby (In reply to mathieu saby from comment #78) > In fact bug 12853 has nothing to do with this one, BUT bug 7162 (passed QA) > will have aftereffects, as it rewrites DelOrder. > So, I'll take a look again when bug 7162 is pushed > > M. Saby Hi Mathieu, the bug 7162 has been pushed to master. What about this bug now? (In reply to Paola Rossi from comment #79) > (In reply to mathieu saby from comment #78) > > In fact bug 12853 has nothing to do with this one, BUT bug 7162 (passed QA) > > will have aftereffects, as it rewrites DelOrder. > > So, I'll take a look again when bug 7162 is pushed > > > > M. Saby > > Hi Mathieu, > > the bug 7162 has been pushed to master. What about this bug now? Hello I probably won't have time before february :( Mathieu Comment on attachment 26692 [details] [review] [PATCH] Bug 10869: Allow the deletion of order line if the record is deleted Review of attachment 26692 [details] [review]: ----------------------------------------------------------------- ::: C4/Acquisition.pm @@ +1760,4 @@ > =cut > > sub DelOrder { > + my ($ordernumber,$biblionumber) = @_; Were attempts to NOT change the parameter order made? This seems like extra work. It would be great if someone could work on this patch again ! The problem is still valid. Created attachment 78862 [details] [review] Bug 10869: Allow the deletion of order line if the record is deleted This patch fixes 2 issues : (medium issue) It is not possible to delete an order in a basket if the biblio record is deleted. (minor issue) There is a useless hyperlink around "Deleted bibliographic notice", and "notice" is a frenchism for "record" Biblionumber was passed to the function as 1st parameter, which made impossible to suppress an order whose associated biblio record was suppressed. Moreover, the biblionumber is not needed to get and suppress the order, but only to suppress items, in a second step. Change made to Acquisitions::DelOrder: - I changed the order of the 2 parameters: now $ordernumber as 1st parameter and $biblionumber as 2d parameter - I renamed $bibnum to $biblionumber - I removed $biblionumber from the main SQL query of the function - I suppressed useless $sth->finish - if no $biblionumber is provided, Koha does not try to suppress the items linked created by acq process Change made to the calls to Acquisitions::DelOrder in Koha's code, to change the order of the parameters - in addorder.pl - in basket.pl - in UT t/db_dependant/Acquisition.t Change made in template basket.tt. This bug was signed off by Cedric some time ago, but it needs a second sign off, because I had to made some little changes. To test : 1. Fill a basket with some records (some records with items or holds, some without items) 2. Supress one of the record 3. In the basket (refresh the screen) check that "Deleted bibliographic notice" line was replaced with "Deleted bibliographic record", without hyperlink arround 4. Delete this order with the link "Delete order" to the right column of the table 5. Check there is a new line in the "Deleted orders" table 6. Check there is no regression : delete an other order (one with no item), then check you cannot delete an order with an item (like before patch) Signed-off-by: Cedric Vita <cedric.vita@dracenie.com> Created attachment 78863 [details] [review] Bug 10869: Followup : update the receipt page (parcel.tt) (Fixed after test by Lyon3 : a bad name was used for a variable. Now "Deleted record..." should NOT be displayed if the record has not been deleted) In main patch, I forgot to made some changes in the parcel.tt page This patch fixes that bu giving the librarian the ability to cancel an order in receipt page if the record is deleted. It also displays the message "Deleted bibliographic record, can't find title" in Summary Note that this behavior will be improved by bug 10758 To test : - create a basket and an order - delete the record used by that order - go on receipt page - in the line matching this order, you should see instead of the title "Deleted bibliographic record, can't find title" - click on "Delete order" : you should be able to delete the order - go to the basket, to check the order was properly cancelled - in receipt page, try to delete an other order (always linked to a record), to be sure there is no regression Created attachment 78864 [details] [review] Bug 10869 Followup: Unit tests for DelOrder [The best would be to add a second sign-off to this patch, as I had to to a little change, to take into account the recent rewrite of t/db_dependent/Acquisition.t: $ordernumber3 is now $ordernumbers[2] Mathieu Saby] DelOrder now returns undef if no ordernumber passed. This patch also adds unit tests for DelOrder. Test plan: A. With syspref AcqCreateItem = on placing an order 1. Fill a basket with some orders (some records with already existing items or holds, some without items). New items linked with the order will be created 2. Keep the basket open 3. In the catalog, supress all the items attached to one of the record used in the basket, then suppress the record 4. In the basket, check you can delete the order that was using deleted record (click on link "Delete order" in the right column of the table) 5. After deleting the order, check there is a new line in the "Deleted orders" table 6. Regression test : try to delete other orders, and check the deletion of the order implies the deletion of items created when ordering a/ If no items were attached to the record before you created the basket, you can delete some other orders, AND their record. Try to delete order and record. b/ If some items were attached to the record before you created the basket, you can delete some other orders, BUT NOT their record. Try to delete only order. After you have deleted it, the items created when ordering must have been deleted. 7. Add some other orders to your basket (some records with already existing items or holds, some without items). New items linked with the order will be created 8. In the catalog, supress all the items attached to one of the record used in the basket, then suppress the record 9. Close the basket 10. Go on receipt page 11. Search the orders of your basket 12. In receipt page, check you can delete the order that was using deleted record (click on link "Delete order" in the right column of the table) 13. Regression test : try to delete other orders a/ If no items were attached to the record before you created the basket, you can delete some other orders, AND their record. Try to delete order and record. b/ If some items were attached to the record before you created the basket, you can delete some other orders, BUT NOT their record. Try to delete only order. After you have deleted it, the items created when ordering must have been deleted. B. With syspref AcqCreateItem = on receiving an order 1. Fill a basket with some orders (some records with already existing items or holds, some without items). 2. Keep the basket open 3. In the catalog, supress all the items attached to one of the record used in the basket, then suppress the record 4. In the basket, check you can delete the order that was using deleted record (click on link "Delete order" in the right column of the table) 5. After deleting the order, check there is a new line in the "Deleted orders" table 6. Regression test : try to delete other orders a/ If no items were attached to the record before you created the basket, you can delete some other orders, AND their record. Try to delete order and record. b/ If some items were attached to the record before you created the basket, you can delete some other orders, BUT NOT their record. Try to delete only order. 7. Add some other orders to your basket (some records with already existing items or holds, some without items). 8. In the catalog, supress all the items attached to one of the record used in the basket, then suppress the record 9. Close the basket 10. Go on receipt page 11. Search the orders of your basket 12. In receipt page, check you can delete the order that was using deleted record (click on link "Delete order" in the right column of the table) 13. Regression test : try to delete other orders a/ If no items were attached to the record before you created the basket, you can delete some other orders, AND their record. Try to delete order and record. b/ If some items were attached to the record before you created the basket, you can delete some other orders, BUT NOT their record. Try to delete only order. And to test the UT: prove t/db_dependent/Acquisition.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Amended the patch as to removing biblio parameter in DelOrder. There is no need to remove the parameter if you will search for biblionumber later on. Deleting records in aqorders_items is discussed on bug 11617. I've rebased and tried to fix things Séverine, I don't know how well it works but if you could test it would be good Thanks Chris for the quick rebase ! I only tested the A part (AcqCreateItem = on placing an order), and failed on steps 4 and 11 when it tried to delete orders. When I highlight the link with the mouse, instead of having a url at the bottom of the screen I got "javascript:confirm_delete_item" with the ordernumber (?) in parenthesis. I don't know if it is the reason becuase it doesn't work, but wanted to signaled it just in case it can help. Tell me if you me to test the other parts of the patch before rebaqing it ! Hm, looking at this too, I think it needs a bit of a rewrite. We added the cancellation reasons since this was first written and the whole cancellation process is slightly different now (delete order should be cancel order). *** Bug 22554 has been marked as a duplicate of this bug. *** Can someone check if this is fixed by bug 26577? I'm sure it is. With bug 26577 and all its dependencies applied, I created a new bib and item by adding a new title to a cart. I deleted the item and then the bib without cancelling the order, reloaded the basket page, and clicked "Cancel order." I got this error: Cannot insert order: Mandatory parameter biblionumber is missing at /kohadevbox/koha/acqui/cancelorder.pl line 60. at /usr/share/perl/5.24/Carp.pm line 167 Still an issue today? This is still an issue on current master. To recreate: 1: Add an order to a basket, using the "From a new (empty) record" option to create a bib and item 2: Once your order is entered, delete the bib (via the cataloging interface, not via acquisitions) 3: reload your basket, see your order now says "Deleted bibliographic record, can't find title" 4: click Cancel Order, confirm cancellation 5: get this error: Cannot insert order: Mandatory parameter biblionumber is missing at /kohadevbox/koha/acqui/cancelorder.pl line 59. at /usr/lib/x86_64-linux-gnu/perl-base/Carp.pm line 289 Right.. so should we dissallow deletion of Biblio's that have orders attached.. or remove the key constraint from Orders? I would disallow the deletion and instead direct the user to first cancel the order. For comparison, if I have a bib with a hold on it and try to use the "Delete all items" option under Edit on the bib, I get the message "1 hold(s) on this record. You must delete all holds before deleting all items." So we have some precedent for just blocking a deletion like this and redirecting the user to something else that needs to happen first. We have an ON DELETE SET NULL on the foreign key.. whilst I like keys for integrity that's wrong if we actually want to keep the link when the biblio is moved to deleted_biblios.. This needs some thought... another case where merging tables would help. We could just remove the code level key requirement in th store routine for biblionumber if it doesn't already exist in storage? Brill.. will take a look at taking that approach tomorrow... thanks Andrew Hi This bug is assigned to me since 2013 but I have not been involved in the development of Koha for years, so I'm reseting Assignee to default. I am wondering if we can get a developer assigned to this one. It looks like it has a fix. Just needs someone to guide it through QA. I agree with Rhonda. This is problematic functionality and it would be great to get a fix. Coming here from 36030, I noticed that I could reopen a basket that had active order lines (first table) without a biblio record, then Cancel that line without any problem in master and 22.11. What am I overlooking here? (In reply to Andrew Fuerste-Henry from comment #95) > I would disallow the deletion and instead direct the user to first cancel > the order. This makes sense. Probably many Koha installs contain pollution due to allowing the deletion of biblio records while being used in ACQ. (In reply to Marcel de Rooy from comment #102) > (In reply to Andrew Fuerste-Henry from comment #95) > > I would disallow the deletion and instead direct the user to first cancel > > the order. > > This makes sense. Probably many Koha installs contain pollution due to > allowing the deletion of biblio records while being used in ACQ. if you want to cancel, that's ok. But if you want to delete the record because it has left the collection? I think the solution needs some more complexity. (In reply to Katrin Fischer from comment #103) > (In reply to Marcel de Rooy from comment #102) > > (In reply to Andrew Fuerste-Henry from comment #95) > > > I would disallow the deletion and instead direct the user to first cancel > > > the order. > > > > This makes sense. Probably many Koha installs contain pollution due to > > allowing the deletion of biblio records while being used in ACQ. > > if you want to cancel, that's ok. But if you want to delete the record > because it has left the collection? I think the solution needs some more > complexity. As suggested, if you want to delete, then remove the linked orders first; you can always add a new order.. (In reply to Marcel de Rooy from comment #104) > (In reply to Katrin Fischer from comment #103) > > (In reply to Marcel de Rooy from comment #102) > > > (In reply to Andrew Fuerste-Henry from comment #95) > > > > I would disallow the deletion and instead direct the user to first cancel > > > > the order. > > > > > > This makes sense. Probably many Koha installs contain pollution due to > > > allowing the deletion of biblio records while being used in ACQ. > > > > if you want to cancel, that's ok. But if you want to delete the record > > because it has left the collection? I think the solution needs some more > > complexity. > > As suggested, if you want to delete, then remove the linked orders first; > you can always add a new order.. Maybe we are misunderstanding each other. I hope this helps: If the orderstatus is "new" or "ordered" things are probably easy and we could do with the check and note. For standing orders it already gets a bit more complicated, so we need to add "partial" as well. But we will also need to think about how to handle deleting records for aqorders with orderstatus "cancelled" or "received". But this might be out of scope here I realize. Also you can't really 'remove linked orders'. We can only cancel or delete a whole basket. If you have multiple order lines in a basket, there is no way to remove just one (odd, right?) (In reply to Katrin Fischer from comment #105) > If the orderstatus is "new" or "ordered" things are probably easy and we > could do with the check and note. For standing orders it already gets a bit > more complicated, so we need to add "partial" as well. So, do not allow deletion of biblio record for new/ordered/partial. > But we will also need to think about how to handle deleting records for > aqorders with orderstatus "cancelled" or "received". But this might be out > of scope here I realize. Allow deletion. The biblionumber in aqorder will be set to NULL (by SQL constraint). > Also you can't really 'remove linked orders'. We can only cancel or delete a > whole basket. If you have multiple order lines in a basket, there is no way > to remove just one (odd, right?) Yeah when I wrote remove, i meant cancel here. There is inconsistency in allowing to remove whole baskets and not order lines. Not sure btw how to remove a whole basket (only via basketgroup)? I think we should not allow that (not cascade delete but set null). (In reply to Marcel de Rooy from comment #106) > Yeah when I wrote remove, i meant cancel here. There is inconsistency in > allowing to remove whole baskets and not order lines. Not sure btw how to > remove a whole basket (only via basketgroup)? I think we should not allow > that (not cascade delete but set null). It is not possible to delete a basketgroup with baskets linked. So there may be no problem here after all ? I can unlink the basket from the group and remove the group. But how should I remove the whole basket?? Opened bug 36066 for REST API. Check if status is cancelled, when deleting an order! See also bug 18360 (enh req) Allow deleting cancelled order lines. Hi Marcel, I'll reply here: (In reply to Marcel de Rooy from comment #106) > Allow deletion. The biblionumber in aqorder will be set to NULL (by SQL > constraint). And deleted_biblionumber will be set to the biblionumber, so we don't lose the connection :) > > Also you can't really 'remove linked orders'. We can only cancel or delete a > > whole basket. If you have multiple order lines in a basket, there is no way > > to remove just one (odd, right?) > > Yeah when I wrote remove, i meant cancel here. There is inconsistency in > allowing to remove whole baskets and not order lines. Not sure btw how to > remove a whole basket (only via basketgroup)? I think we should not allow > that (not cascade delete but set null). It's not about basket groups. As you noted, a basket group would block deleting. If you have an open basket, not linked to a basket group, there is a button in the toolbar: "Delete basket". It gives a choice to delete only the basket and order lines or to delete them + bibliographic records. What did you mean with "we should not allow that (not cascade delete but set NULL)"? (In reply to Katrin Fischer from comment #109) > If you have an open basket, not linked to a basket group, there is a button > in the toolbar: > "Delete basket". > It gives a choice to delete only the basket and order lines or to delete > them + bibliographic records. Somehow I missed that one. And it is a game changer.. Still thinking how to proceed now. Since the last releases actually solved this bug on itself (you can cancel it now), i am closing this report in favor of: 18360 Interface - Delete cancelled lines 36066 API - Allow only deleting cancelled lines 27893 Delete of biblio with order does not always warn The latter report should actually do more imo: if we delete a biblio, we should warn and also cancel attached order lines when confirmed. Will change that a bit there. |
Created attachment 20970 [details] deleted title. When looking at a basket in Acq you can't delete an order line if the title has already been deleted. You get no confirmation, no error, it just doesn't delete. It does delete if the title still exists. The problem is a missing parameter. If the title is there the link looks like this: javascript:confirm_delete_item(5547,) If the title doesn't exist the link looks like this: javascript:confirm_delete_item(5557,278226) I know it's better to delete the order line first, but sometimes people do things in cataloging that effect acq and it shouldn't leave this phantom order line - we should be able to delete it. Nicole