|
Description
Marijana Glavica
2012-03-22 10:21:20 UTC
Patch coming soon M. Saby Created attachment 21224 [details] [review] Bug 7791 : Give the ability to delete records when deleting a basket Currently, when a basket is deleted, all the orders are deleted (there is a foreign key in aqorders table on basketno). This could be dangerous, and there is no warn. After the deletion, unused biblios are left in the catalogue. This patch - adds a more detailed message describing the consequences of deletion - give the choice of also deleting biblio records if possible To test : Test A : 1. create a basket with 4 orders: - an order from a new record A - an order from a record B which has already an item - an order from a record C used in a subscription - an order from a record D used in an other order 2. note the biblionumbers of the records used (or open them in other tabs in yout browser) 3. click on "Delete basket" 4. choose button "Delete basket and orders" 5. check the catalogue : records A,B,C,D must still be there Test B: 1. create a basket with 4 orders: - an order from a new record A - an order from a record B which has already an item - an order from a record C used in a subscription - an order from a record D used in an other order 2. note the biblionumbers of the records used (or open them in other tabs in yout browser) 3. click on "Delete basket" 4. choose button "Delete basket, orders and records" 5. check the catalogue : records B,C,D must still be there. Record A must be deleted Mathieu, Some warns are in french and some indentations should be fixed. Oups, I was not suppose to keep warns. It was just for me... followup coming Mathieu Created attachment 21225 [details] [review] [PATCH 2] Bug 7791 Followup: delete warns, add missing curly braket and make a simpler message if basket is void This patch - delete warns - add a missing } - add a condition in template of avoiding asking to suppress orders or records if the basket is void To test : 1. Make the same tests as defined in test plan of main patch. It should behave the same way 2. Try to delete a basket with no records inside. You will only have a "Delete basket" button, with less warnings Signing patches on sandbox (test1) did not work. Thank you to "Sign-off" on. Created attachment 21245 [details] [review] Bug 7791 : Give the ability to delete records when deleting a basket Currently, when a basket is deleted, all the orders are deleted (there is a foreign key in aqorders table on basketno). This could be dangerous, and there is no warn. After the deletion, unused biblios are left in the catalogue. This patch - adds a more detailed message describing the consequences of deletion - give the choice of also deleting biblio records if possible To test : Test A : 1. create a basket with 4 orders: - an order from a new record A - an order from a record B which has already an item - an order from a record C used in a subscription - an order from a record D used in an other order 2. note the biblionumbers of the records used (or open them in other tabs in yout browser) 3. click on "Delete basket" 4. choose button "Delete basket and orders" 5. check the catalogue : records A,B,C,D must still be there Test B: 1. create a basket with 4 orders: - an order from a new record A - an order from a record B which has already an item - an order from a record C used in a subscription - an order from a record D used in an other order 2. note the biblionumbers of the records used (or open them in other tabs in yout browser) 3. click on "Delete basket" 4. choose button "Delete basket, orders and records" 5. check the catalogue : records B,C,D must still be there. Record A must be deleted Signed-off-by: Cedric Vita <cedric.vita@dracenie.com> Created attachment 21246 [details] [review] Bug 7791 Followup: delete warns, add missing curly braket and make a simpler message if basket is void This patch - delete warns - add a missing } - add a condition in template of avoiding asking to suppress orders or records if the basket is void To test : 1. Make the same tests as defined in test plan of main patch. It should behave the same way 2. Try to delete a basket with no records inside. You will only have a "Delete basket" button, with less warnings Signed-off-by: Cedric Vita <cedric.vita@dracenie.com> QA: Looking at this one now. QA Comment: This looks quite good to me. No complaints from qa tools. I just have one question: I think it would be even more friendly to the user if you could report the errors DelBiblio returns on refused deletions (because of items not connected to orders). At this time your code does not look at a possible return value of DelBiblio. Would it be possible for you to add this in a followup? (But I do not think this should block your patch..) And I got struck by another problem when testing your patch, but it does not depend on your changes but is connected to booksellers.pl. If I delete a basket, the return page is booksellers.pl. Every time I get the Firefox message: A script on this page may be busy, or it may have stopped responding. You can stop the script now, or you can continue to see if the script will complete. Script: http://[servername]/intranet-tmpl/lib/jquery/jquery.js:4 Just mentioning it here for completeness. Changing the bug status for now. (In reply to M. de Rooy from comment #10) > QA Comment: > This looks quite good to me. No complaints from qa tools. > I just have one question: I think it would be even more friendly to the user > if you could report the errors DelBiblio returns on refused deletions > (because of items not connected to orders). > At this time your code does not look at a possible return value of DelBiblio. > Would it be possible for you to add this in a followup? (But I do not think > this should block your patch..) Hi I will look if it is possible and add a followup. > > And I got struck by another problem when testing your patch, but it does not > depend on your changes but is connected to booksellers.pl. > If I delete a basket, the return page is booksellers.pl. Every time I get > the Firefox message: A script on this page may be busy, or it may have > stopped responding. You can stop the script now, or you can continue to see > if the script will complete. > Script: http://[servername]/intranet-tmpl/lib/jquery/jquery.js:4 > Just mentioning it here for completeness. To be clear : does it occur in master without my patch? Mathieu (In reply to mathieu saby from comment #11) > To be clear : does it occur in master without my patch? Hi Matthieu, Yes it does. So nothing to do with your patch.. Note that if I am in a basket from some supplier and I delete it, I actually would like to return to the basket list of that supplier instead of getting a list of all suppliers. But that is outside the scope of this report. (In reply to M. de Rooy from comment #12) > (In reply to mathieu saby from comment #11) > > To be clear : does it occur in master without my patch? > Hi Matthieu, > Yes it does. So nothing to do with your patch.. > > Note that if I am in a basket from some supplier and I delete it, I actually > would like to return to the basket list of that supplier instead of getting > a list of all suppliers. But that is outside the scope of this report. I filled a new bug for that 10978 ;-) Mathieu Created attachment 21684 [details] [review] [PATCH 3] Bug 7791 Followup: add warning after deletion if some records were not deleted Here is the followup. This followup add some warnings after deletion if some items were not deleted. 4 types of messages are possible : - x item(s) attached. - x subscription(s) attached. - x order(s) attached. - Unknown error. To test : test a 1. create a basket with - an order using a record A which has already an item, which is used in a subscription, and in other order (in an other basket) - an order using a record B used nowhere elese 2. click on "Delete basket" 3. choose button "Delete basket, orders and records" 4. you must see a page anouncing basket deletion, and that record A was not deleted because of its item, subscription and order. 5. check the link around the title of record B takes you to the record 6. check the link under the warning box ("Click here to go back to booksellers page") takes you to booksellers page 5. check record B is deleted test b 1. suppress the subscription linked with record A 2. create an other basket using record A 3. delete the basket on the same way as for test a 4. you must see a page anouncing basket deletion, and that record A was not deleted because of its item and order test c 1. suppress the item attached under record A 2. create an other basket using record A 3. delete the basket on the same way as for test a 4. you must see a page anouncing basket deletion, and that record A was not deleted because of its order M. Saby Patch 1 and 2 were already signed off. Patch applied cleanly, go forth and signoff fatal: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt). zut Jonathan, I don't understand, I don't have any problem : $git bz apply 7791 Bug 7791 - Deleting basket should not delete all existing orders without warning Bug 7791 : Give the ability to delete records when deleting a basket Apply? [yn] y Applying: Bug 7791 : Give the ability to delete records when deleting a basket Bug 7791 Followup: delete warns, add missing curly braket and make a simpler message if basket is void Apply? [yn] y Applying: Bug 7791 Followup: delete warns, add missing curly braket and make a simpler message if basket is void [PATCH 3] Bug 7791 Followup: add warning after deletion if some records were not deleted Apply? [yn] y Applying: Bug 7791 Followup: add warning after deletion if some records were not deleted Mathieu Tested again, I cannot apply your patch, sorry. strange Do you know what attachment is causing the problem? Mathieu (In reply to mathieu saby from comment #21) The last one. Created attachment 21969 [details] [review] [PATCH 3/3] Bug 7791 Followup: add warning after deletion if some records were not deleted I applied it, and regenerate a patch with format pach. Mathieu The sandbox you've requested is not ready. Some problems occurred applying patches from bug 7791: <h1>Something went wrong !</h1>Applying: Bug 7791 : Give the ability to delete records when deleting a basket Applying: Bug 7791 Followup: delete warns, add missing curly braket and make a simpler message if basket is void Applying: Bug 7791 Followup: add warning after deletion if some records were not deleted fatal: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 7791 Followup: add warning after deletion if some records were not deleted 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. Bug 7791 - Deleting basket should not delete all existing orders without warning 21245 - Bug 7791 : Give the ability to delete records when deleting a basket 21246 - Bug 7791 Followup: delete warns, add missing curly braket and make a simpler message if basket is void 21969 - [PATCH 3/3] Bug 7791 Followup: add warning after deletion if some records were not deleted Same error Jonathan pointed out. I regenerated the patch and it applies cleany on my machine (using git am -3iu) Sorry, I don't understand this sha1 issue, and can't resolve it :( Mathieu I could mabye try to merge the 3 patches, maybe it will resolve the issue. But I would not prefer, as the first 2 patches were already signed off, and not the 3rd... Mathieu This bug applies correctly on current master in my VM : $git bz apply 7791 Bug 7791 - Deleting basket should not delete all existing orders without warning Bug 7791 : Give the ability to delete records when deleting a basket Apply? [yn] y Applying: Bug 7791 : Give the ability to delete records when deleting a basket Bug 7791 Followup: delete warns, add missing curly braket and make a simpler message if basket is void Apply? [yn] y Applying: Bug 7791 Followup: delete warns, add missing curly braket and make a simpler message if basket is void [PATCH 3/3] Bug 7791 Followup: add warning after deletion if some records were not deleted Apply? [yn] y Applying: Bug 7791 Followup: add warning after deletion if some records were not deleted Maybe the problem comes from the sandbox ? Mathieu Created attachment 22046 [details] [review] Bug 7791 Followup: add warning after deletion if some records were not deleted This followup add some warnings after deletion if some items were not deleted. 4 types of messages are possible : - x item(s) attached. - x subscription(s) attached. - x order(s) attached. - Unknown error. To test : test a 1. create a basket with - an order using a record A which has already an item, which is used in a subscription, and in other order (in an other basket) - an order using a record B used nowhere elese 2. click on "Delete basket" 3. choose button "Delete basket, orders and records" 4. you must see a page anouncing basket deletion, and that record A was not deleted because of its item, subscription and order. 5. check the link around the title of record B takes you to the record 6. check the link under the warning box ("Click here to go back to booksellers page") takes you to booksellers page 5. check record B is deleted test b 1. suppress the subscription linked with record A 2. create an other basket using record A 3. delete the basket on the same way as for test a 4. you must see a page anouncing basket deletion, and that record A was not deleted because of its item and order test c 1. suppress the item attached under record A 2. create an other basket using record A 3. delete the basket on the same way as for test a 4. you must see a page anouncing basket deletion, and that record A was not deleted because of its orderBug 7791 Followup: add warning after deletion if some records were not deleted Content-Type: text/plain; charset="utf-8" This followup add some warnings after deletion if some items were not deleted. 4 types of messages are possible : - x item(s) attached. - x subscription(s) attached. - x order(s) attached. - Unknown error. To test : test a 1. create a basket with - an order using a record A which has already an item, which is used in a subscription, and in other order (in an other basket) - an order using a record B used nowhere elese 2. click on "Delete basket" 3. choose button "Delete basket, orders and records" 4. you must see a page anouncing basket deletion, and that record A was not deleted because of its item, subscription and order. 5. check the link around the title of record B takes you to the record 6. check the link under the warning box ("Click here to go back to booksellers page") takes you to booksellers page 5. check record B is deleted test b 1. suppress the subscription linked with record A 2. create an other basket using record A 3. delete the basket on the same way as for test a 4. you must see a page anouncing basket deletion, and that record A was not deleted because of its item and order test c 1. suppress the item attached under record A 2. create an other basket using record A 3. delete the basket on the same way as for test a 4. you must see a page anouncing basket deletion, and that record A was not deleted because of its order Hello Jonathan Did you do something with this attachment? What was the issue ? Mathieu (In reply to mathieu saby from comment #29) > Hello Jonathan > Did you do something with this attachment? What was the issue ? Mathieu, I applied the third patch using 'git apply --reject' and I manually fix the conflict. Thank you, I did not know the trick. Mathieu Created attachment 22091 [details] [review] Bug 7791 : Give the ability to delete records when deleting a basket Currently, when a basket is deleted, all the orders are deleted (there is a foreign key in aqorders table on basketno). This could be dangerous, and there is no warn. After the deletion, unused biblios are left in the catalogue. This patch - adds a more detailed message describing the consequences of deletion - give the choice of also deleting biblio records if possible To test : Test A : 1. create a basket with 4 orders: - an order from a new record A - an order from a record B which has already an item - an order from a record C used in a subscription - an order from a record D used in an other order 2. note the biblionumbers of the records used (or open them in other tabs in yout browser) 3. click on "Delete basket" 4. choose button "Delete basket and orders" 5. check the catalogue : records A,B,C,D must still be there Test B: 1. create a basket with 4 orders: - an order from a new record A - an order from a record B which has already an item - an order from a record C used in a subscription - an order from a record D used in an other order 2. note the biblionumbers of the records used (or open them in other tabs in yout browser) 3. click on "Delete basket" 4. choose button "Delete basket, orders and records" 5. check the catalogue : records B,C,D must still be there. Record A must be deleted Signed-off-by: Cedric Vita <cedric.vita@dracenie.com> Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Created attachment 22092 [details] [review] Bug 7791 Followup: delete warns, add missing curly braket and make a simpler message if basket is void This patch - delete warns - add a missing } - add a condition in template of avoiding asking to suppress orders or records if the basket is void To test : 1. Make the same tests as defined in test plan of main patch. It should behave the same way 2. Try to delete a basket with no records inside. You will only have a "Delete basket" button, with less warnings Signed-off-by: Cedric Vita <cedric.vita@dracenie.com> Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Created attachment 22093 [details] [review] Bug 7791 Followup: add warning after deletion if some records were not deleted This followup add some warnings after deletion if some items were not deleted. 4 types of messages are possible : - x item(s) attached. - x subscription(s) attached. - x order(s) attached. - Unknown error. To test : test a 1. create a basket with - an order using a record A which has already an item, which is used in a subscription, and in other order (in an other basket) - an order using a record B used nowhere elese 2. click on "Delete basket" 3. choose button "Delete basket, orders and records" 4. you must see a page anouncing basket deletion, and that record A was not deleted because of its item, subscription and order. 5. check the link around the title of record B takes you to the record 6. check the link under the warning box ("Click here to go back to booksellers page") takes you to booksellers page 5. check record B is deleted test b 1. suppress the subscription linked with record A 2. create an other basket using record A 3. delete the basket on the same way as for test a 4. you must see a page anouncing basket deletion, and that record A was not deleted because of its item and order test c 1. suppress the item attached under record A 2. create an other basket using record A 3. delete the basket on the same way as for test a 4. you must see a page anouncing basket deletion, and that record A was not deleted because of its orderBug 7791 Followup: add warning after deletion if some records were not deleted Content-Type: text/plain; charset="utf-8" This followup add some warnings after deletion if some items were not deleted. 4 types of messages are possible : - x item(s) attached. - x subscription(s) attached. - x order(s) attached. - Unknown error. To test : test a 1. create a basket with - an order using a record A which has already an item, which is used in a subscription, and in other order (in an other basket) - an order using a record B used nowhere elese 2. click on "Delete basket" 3. choose button "Delete basket, orders and records" 4. you must see a page anouncing basket deletion, and that record A was not deleted because of its item, subscription and order. 5. check the link around the title of record B takes you to the record 6. check the link under the warning box ("Click here to go back to booksellers page") takes you to booksellers page 5. check record B is deleted test b 1. suppress the subscription linked with record A 2. create an other basket using record A 3. delete the basket on the same way as for test a 4. you must see a page anouncing basket deletion, and that record A was not deleted because of its item and order test c 1. suppress the item attached under record A 2. create an other basket using record A 3. delete the basket on the same way as for test a 4. you must see a page anouncing basket deletion, and that record A was not deleted because of its order Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Created attachment 22764 [details] [review] Bug 7791 : Give the ability to delete records when deleting a basket Currently, when a basket is deleted, all the orders are deleted (there is a foreign key in aqorders table on basketno). This could be dangerous, and there is no warn. After the deletion, unused biblios are left in the catalogue. This patch - adds a more detailed message describing the consequences of deletion - give the choice of also deleting biblio records if possible To test : Test A : 1. create a basket with 4 orders: - an order from a new record A - an order from a record B which has already an item - an order from a record C used in a subscription - an order from a record D used in an other order 2. note the biblionumbers of the records used (or open them in other tabs in yout browser) 3. click on "Delete basket" 4. choose button "Delete basket and orders" 5. check the catalogue : records A,B,C,D must still be there Test B: 1. create a basket with 4 orders: - an order from a new record A - an order from a record B which has already an item - an order from a record C used in a subscription - an order from a record D used in an other order 2. note the biblionumbers of the records used (or open them in other tabs in yout browser) 3. click on "Delete basket" 4. choose button "Delete basket, orders and records" 5. check the catalogue : records B,C,D must still be there. Record A must be deleted Signed-off-by: Cedric Vita <cedric.vita@dracenie.com> Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 22765 [details] [review] Bug 7791 Followup: delete warns, add missing curly braket and make a simpler message if basket is void This patch - delete warns - add a missing } - add a condition in template of avoiding asking to suppress orders or records if the basket is void To test : 1. Make the same tests as defined in test plan of main patch. It should behave the same way 2. Try to delete a basket with no records inside. You will only have a "Delete basket" button, with less warnings Signed-off-by: Cedric Vita <cedric.vita@dracenie.com> Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 22766 [details] [review] Bug 7791 Followup: add warning after deletion if some records were not deleted This followup add some warnings after deletion if some items were not deleted. 4 types of messages are possible : - x item(s) attached. - x subscription(s) attached. - x order(s) attached. - Unknown error. To test : test a 1. create a basket with - an order using a record A which has already an item, which is used in a subscription, and in other order (in an other basket) - an order using a record B used nowhere elese 2. click on "Delete basket" 3. choose button "Delete basket, orders and records" 4. you must see a page anouncing basket deletion, and that record A was not deleted because of its item, subscription and order. 5. check the link around the title of record B takes you to the record 6. check the link under the warning box ("Click here to go back to booksellers page") takes you to booksellers page 5. check record B is deleted test b 1. suppress the subscription linked with record A 2. create an other basket using record A 3. delete the basket on the same way as for test a 4. you must see a page anouncing basket deletion, and that record A was not deleted because of its item and order test c 1. suppress the item attached under record A 2. create an other basket using record A 3. delete the basket on the same way as for test a 4. you must see a page anouncing basket deletion, and that record A was not deleted because of its orderBug 7791 Followup: add warning after deletion if some records were not deleted Content-Type: text/plain; charset="utf-8" This followup add some warnings after deletion if some items were not deleted. 4 types of messages are possible : - x item(s) attached. - x subscription(s) attached. - x order(s) attached. - Unknown error. To test : test a 1. create a basket with - an order using a record A which has already an item, which is used in a subscription, and in other order (in an other basket) - an order using a record B used nowhere elese 2. click on "Delete basket" 3. choose button "Delete basket, orders and records" 4. you must see a page anouncing basket deletion, and that record A was not deleted because of its item, subscription and order. 5. check the link around the title of record B takes you to the record 6. check the link under the warning box ("Click here to go back to booksellers page") takes you to booksellers page 5. check record B is deleted test b 1. suppress the subscription linked with record A 2. create an other basket using record A 3. delete the basket on the same way as for test a 4. you must see a page anouncing basket deletion, and that record A was not deleted because of its item and order test c 1. suppress the item attached under record A 2. create an other basket using record A 3. delete the basket on the same way as for test a 4. you must see a page anouncing basket deletion, and that record A was not deleted because of its order Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> QA Comment: Works as advertised. Nice to have. No complaints from qa tools. Code looks good to me. Includes string changes, so candidate for 3.16. I am passing QA on these patches, but have some hesitations as to design and navigation, as described in the following points: 1) The design of the page showing that the basket has been deleted, but also listing a warning c/should look better (layman's perspective :) 2) This same error page gives me the link Go back to booksellers page (note the problems reported earlier about its performance). Since I came from a basket of a specific bookseller, I would rather go back to the basket list of THAT bookseller. You opened up a new report for that, I understand. I think that this set of patches would be even more attractive with such a link. 3) A successful deletion actually does the same and sends me back to all booksellers. Please give me my particular bookseller instead :) Thanks. PS I downgraded the bug severity. I can understand that you feel this is an important issue, but critical is really another category. (In reply to M. de Rooy from comment #38) > QA Comment: > Works as advertised. Nice to have. > No complaints from qa tools. Code looks good to me. > Includes string changes, so candidate for 3.16. > I am passing QA on these patches, but have some hesitations as to design and > navigation, as described in the following points: > > 1) The design of the page showing that the basket has been deleted, but also > listing a warning c/should look better (layman's perspective :) > 2) This same error page gives me the link Go back to booksellers page (note > the problems reported earlier about its performance). Since I came from a > basket of a specific bookseller, I would rather go back to the basket list > of THAT bookseller. You opened up a new report for that, I understand. I > think that this set of patches would be even more attractive with such a > link. > 3) A successful deletion actually does the same and sends me back to all > booksellers. Please give me my particular bookseller instead :) > > Thanks. > > PS I downgraded the bug severity. I can understand that you feel this is an > important issue, but critical is really another category. Thank you For the points 1-3 it could be improved by other patches ;-) Mathieu Pushed to master. Thanks, Mathieu! Those patches has been pushed to 3.14.x, will be in 3.14.1. (integration into master added a commit : "1be884f Bug 7791: (follow-up) adjust wording of basket deletion confirmation dialog") |