Bug 7791 - Deleting basket should not delete all existing orders without warning
Summary: Deleting basket should not delete all existing orders without warning
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: mathieu saby
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-22 10:21 UTC by Marijana Glavica
Modified: 2015-06-04 23:23 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 7791 : Give the ability to delete records when deleting a basket (8.62 KB, patch)
2013-09-19 08:36 UTC, mathieu saby
Details | Diff | Splinter Review
[PATCH 2] Bug 7791 Followup: delete warns, add missing curly braket and make a simpler message if basket is void (6.18 KB, patch)
2013-09-19 09:22 UTC, mathieu saby
Details | Diff | Splinter Review
Bug 7791 : Give the ability to delete records when deleting a basket (8.13 KB, patch)
2013-09-19 14:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7791 Followup: delete warns, add missing curly braket and make a simpler message if basket is void (6.19 KB, patch)
2013-09-19 14:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PATCH 3] Bug 7791 Followup: add warning after deletion if some records were not deleted (7.25 KB, patch)
2013-09-30 22:04 UTC, mathieu saby
Details | Diff | Splinter Review
[PATCH 3/3] Bug 7791 Followup: add warning after deletion if some records were not deleted (7.25 KB, patch)
2013-10-11 14:37 UTC, mathieu saby
Details | Diff | Splinter Review
Bug 7791 Followup: add warning after deletion if some records were not deleted (8.68 KB, patch)
2013-10-18 09:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7791 : Give the ability to delete records when deleting a basket (8.18 KB, patch)
2013-10-20 18:26 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 7791 Followup: delete warns, add missing curly braket and make a simpler message if basket is void (6.10 KB, patch)
2013-10-20 18:26 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 7791 Followup: add warning after deletion if some records were not deleted (8.71 KB, patch)
2013-10-20 18:27 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 7791 : Give the ability to delete records when deleting a basket (8.28 KB, patch)
2013-11-07 12:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 7791 Followup: delete warns, add missing curly braket and make a simpler message if basket is void (6.19 KB, patch)
2013-11-07 12:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 7791 Followup: add warning after deletion if some records were not deleted (8.80 KB, patch)
2013-11-07 12:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marijana Glavica 2012-03-22 10:21:20 UTC
When deleting orders in a basket, orders will not be deleted completely but will be marked as canceled. Also, in this case, system is taking care of biblios and items records as described in bug 5680.

But, when you delete the whole basket, everything will be deleted without warning!

The system should at least warn you that you will loose all your orders if you delete basket, or it should not let you to delete baskets which are not empty.

Also, it would be nice if you could enter the reason for deleting your basket.
Comment 1 mathieu saby 2013-09-17 19:01:01 UTC
Patch coming soon

M. Saby
Comment 2 mathieu saby 2013-09-19 08:36:09 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2013-09-19 08:49:20 UTC
Mathieu,
Some warns are in french and some indentations should be fixed.
Comment 4 mathieu saby 2013-09-19 08:50:45 UTC
Oups, I was not suppose to keep warns. It was just for me...
followup coming

Mathieu
Comment 5 mathieu saby 2013-09-19 09:22:28 UTC Comment hidden (obsolete)
Comment 6 cedric.vita 2013-09-19 13:48:25 UTC
Signing patches on sandbox (test1) did not work.
Thank you to "Sign-off" on.
Comment 7 Jonathan Druart 2013-09-19 14:33:33 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2013-09-19 14:33:38 UTC Comment hidden (obsolete)
Comment 9 Marcel de Rooy 2013-09-30 07:31:11 UTC
QA: Looking at this one now.
Comment 10 Marcel de Rooy 2013-09-30 07:42:05 UTC
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.
Comment 11 mathieu saby 2013-09-30 09:56:58 UTC
(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
Comment 12 Marcel de Rooy 2013-09-30 10:16:00 UTC
(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.
Comment 13 mathieu saby 2013-09-30 19:58:00 UTC
(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
Comment 14 mathieu saby 2013-09-30 22:04:10 UTC Comment hidden (obsolete)
Comment 15 mathieu saby 2013-09-30 22:04:58 UTC
Patch 1 and 2 were already signed off.
Comment 16 I'm just a bot 2013-10-01 18:43:12 UTC
Patch applied cleanly, go forth and signoff
Comment 17 Jonathan Druart 2013-10-11 12:46:24 UTC
fatal: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt).
Comment 18 mathieu saby 2013-10-11 13:32:52 UTC
zut
Comment 19 mathieu saby 2013-10-11 13:34:44 UTC
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
Comment 20 Jonathan Druart 2013-10-11 13:45:55 UTC
Tested again, I cannot apply your patch, sorry.
Comment 21 mathieu saby 2013-10-11 14:05:47 UTC
strange
Do you know what attachment is causing the problem?

Mathieu
Comment 22 Jonathan Druart 2013-10-11 14:14:04 UTC
(In reply to mathieu saby from comment #21)
The last one.
Comment 23 mathieu saby 2013-10-11 14:37:12 UTC Comment hidden (obsolete)
Comment 24 Pierre Angot 2013-10-17 09:10:35 UTC
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
Comment 25 mathieu saby 2013-10-17 09:24:42 UTC
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
Comment 26 mathieu saby 2013-10-17 09:27:50 UTC
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
Comment 27 mathieu saby 2013-10-17 16:56:37 UTC
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
Comment 28 Jonathan Druart 2013-10-18 09:45:34 UTC Comment hidden (obsolete)
Comment 29 mathieu saby 2013-10-18 10:07:20 UTC
Hello Jonathan
Did you do something with this attachment? What was the issue ?

Mathieu
Comment 30 Jonathan Druart 2013-10-18 10:23:10 UTC
(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.
Comment 31 mathieu saby 2013-10-18 10:53:13 UTC
Thank you, I did not know the trick.
Mathieu
Comment 32 Chris Cormack 2013-10-20 18:26:44 UTC Comment hidden (obsolete)
Comment 33 Chris Cormack 2013-10-20 18:26:58 UTC Comment hidden (obsolete)
Comment 34 Chris Cormack 2013-10-20 18:27:08 UTC Comment hidden (obsolete)
Comment 35 Marcel de Rooy 2013-11-07 12:15:07 UTC
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>
Comment 36 Marcel de Rooy 2013-11-07 12:15:14 UTC
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>
Comment 37 Marcel de Rooy 2013-11-07 12:15:23 UTC
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>
Comment 38 Marcel de Rooy 2013-11-07 12:46:52 UTC
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.
Comment 39 mathieu saby 2013-11-07 12:50:47 UTC
(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
Comment 40 Galen Charlton 2013-12-02 16:20:54 UTC
Pushed to master.  Thanks, Mathieu!
Comment 41 Fridolin Somers 2013-12-13 15:15:45 UTC
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")