Bug 10869 - Can't cancel order line if title deleted
Summary: Can't cancel order line if title deleted
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: mathieu saby
QA Contact: Paul Poulain
URL:
Keywords:
: 5781 22554 (view as bug list)
Depends on:
Blocks: 11617
  Show dependency treegraph
 
Reported: 2013-09-10 16:43 UTC by Nicole C. Engard
Modified: 2019-03-21 14:30 UTC (History)
17 users (show)

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


Attachments
deleted title. (65.41 KB, image/png)
2013-09-10 16:43 UTC, Nicole C. Engard
Details
[PATCH] Bug 10869: Allow the deletion of order line if the record is deleted (10.08 KB, patch)
2013-09-14 10:39 UTC, mathieu saby
Details | Diff | Splinter Review
Bug 10869: Allow the deletion of order line if the record is deleted (9.78 KB, patch)
2013-09-21 15:53 UTC, mathieu saby
Details | Diff | Splinter Review
[PATCH] Bug 10869: Allow the deletion of order line if the record is deleted (9.67 KB, patch)
2013-09-21 16:15 UTC, mathieu saby
Details | Diff | Splinter Review
Bug 10869: Allow the deletion of order line if the record is deleted (9.68 KB, patch)
2013-09-26 09:53 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 10869 follow-up: indent & term (2.27 KB, patch)
2013-10-20 19:58 UTC, Paul Poulain
Details | Diff | Splinter Review
[PATCH 1/2] Bug 10869: Allow the deletion of order line if the record is deleted (9.75 KB, patch)
2013-10-22 21:45 UTC, mathieu saby
Details | Diff | Splinter Review
[PATCH] Bug 10869: Allow the deletion of order line if the record is deleted (10.29 KB, patch)
2013-10-22 22:02 UTC, mathieu saby
Details | Diff | Splinter Review
[PATCH 2/2] Bug 10869 Followup : change DelOrder and adds UT (10.07 KB, patch)
2013-10-23 20:56 UTC, mathieu saby
Details | Diff | Splinter Review
[PATCH 2/2] Bug 10869 Followup : change DelOrder and adds UT (10.02 KB, patch)
2013-11-03 18:38 UTC, mathieu saby
Details | Diff | Splinter Review
[PATCH 2/2] Bug 10869 Followup : change DelOrder and adds UT (10.03 KB, patch)
2013-12-28 00:11 UTC, mathieu saby
Details | Diff | Splinter Review
Bug 10869: Allow the deletion of order line if the record is deleted (10.29 KB, patch)
2014-01-27 12:29 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10869 Followup: Unit tests for DelOrder (8.23 KB, patch)
2014-01-27 12:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[PATCH] Bug 10869: Allow the deletion of order line if the record is deleted (10.33 KB, patch)
2014-02-02 22:51 UTC, mathieu saby
Details | Diff | Splinter Review
Bug 10869 Followup: Unit tests for DelOrder (8.23 KB, patch)
2014-02-03 10:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10869 Followup: Unit tests for DelOrder (8.17 KB, patch)
2014-02-03 10:40 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10869 Followup: Unit tests for DelOrder (8.17 KB, patch)
2014-02-03 10:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10869 Followup: Unit tests for DelOrder (8.08 KB, patch)
2014-02-03 10:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[PATCH] Bug 10869: Allow the deletion of order line if the record is deleted (12.26 KB, patch)
2014-03-30 22:00 UTC, mathieu saby
Details | Diff | Splinter Review
[PATCH 2/2] Bug 10869 Followup: Unit tests for DelOrder (8.22 KB, patch)
2014-03-30 22:21 UTC, mathieu saby
Details | Diff | Splinter Review
[PATCH 3/3] Followup : update the receipt page (parcel.tt) (3.51 KB, patch)
2014-04-17 18:37 UTC, mathieu saby
Details | Diff | Splinter Review
[PATCH 3/3] Bug 10869: Followup : update the receipt page (parcel.tt) (3.66 KB, patch)
2014-04-18 16:19 UTC, mathieu saby
Details | Diff | Splinter Review
[PATCH 2/2] Bug 10869 Followup: Unit tests for DelOrder (8.17 KB, patch)
2014-05-28 11:48 UTC, mathieu saby
Details | Diff | Splinter Review
[PATCH 2/2] Bug 10869 Followup: Unit tests for DelOrder (8.22 KB, patch)
2014-07-13 23:16 UTC, mathieu saby
Details | Diff | Splinter Review
Bug 10869: Allow the deletion of order line if the record is deleted (5.92 KB, patch)
2018-09-15 17:28 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 10869: Followup : update the receipt page (parcel.tt) (3.45 KB, patch)
2018-09-15 17:28 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 10869 Followup: Unit tests for DelOrder (4.98 KB, patch)
2018-09-15 17:28 UTC, Chris Cormack
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nicole C. Engard 2013-09-10 16:43:09 UTC
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
Comment 1 mathieu saby 2013-09-12 10:55:50 UTC
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
Comment 2 Nicole C. Engard 2013-09-12 13:20:50 UTC
Showing the title is great, but the line should still be allowed to be deleted.
Comment 3 mathieu saby 2013-09-12 13:26:30 UTC
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
Comment 4 mathieu saby 2013-09-14 10:38:21 UTC
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
Comment 5 mathieu saby 2013-09-14 10:39:03 UTC Comment hidden (obsolete)
Comment 6 mathieu saby 2013-09-14 10:40:47 UTC
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
Comment 7 mathieu saby 2013-09-17 19:00:07 UTC
*** Bug 5781 has been marked as a duplicate of this bug. ***
Comment 8 mathieu saby 2013-09-21 15:53:10 UTC Comment hidden (obsolete)
Comment 9 mathieu saby 2013-09-21 16:15:46 UTC Comment hidden (obsolete)
Comment 10 I'm just a bot 2013-09-24 05:37:19 UTC
Patch applied cleanly, go forth and signoff
Comment 11 Pierre Angot 2013-09-26 09:50:50 UTC
Is it a duplicate of 9780 ? It seems redundant.
Comment 12 sandboxes@biblibre.com 2013-09-26 09:53:12 UTC
Patch tested with a sandbox, by Cedric Vita <cedric.vita@dracenie.com>
Comment 13 sandboxes@biblibre.com 2013-09-26 09:53:32 UTC Comment hidden (obsolete)
Comment 14 mathieu saby 2013-09-26 09:58:10 UTC
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
Comment 15 mathieu saby 2013-09-26 09:58:31 UTC
And thank you Cédric!
Comment 16 mathieu saby 2013-10-20 09:21:04 UTC
Could someone QA, so that depending patch (Bug 10758) could be signed on sandbox once this one is pushed?

Mathieu
Comment 17 Paul Poulain 2013-10-20 19:57:58 UTC
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
Comment 18 Paul Poulain 2013-10-20 19:58:46 UTC Comment hidden (obsolete)
Comment 19 mathieu saby 2013-10-20 20:42:18 UTC
(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
Comment 20 mathieu saby 2013-10-20 20:51:33 UTC
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
Comment 21 mathieu saby 2013-10-22 21:45:04 UTC Comment hidden (obsolete)
Comment 22 mathieu saby 2013-10-22 22:02:54 UTC Comment hidden (obsolete)
Comment 23 mathieu saby 2013-10-23 07:13:46 UTC
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
Comment 24 mathieu saby 2013-10-23 20:56:46 UTC Comment hidden (obsolete)
Comment 25 mathieu saby 2013-10-23 20:57:44 UTC
I ask for a 2d signoff, as my followup changes a lot of things in DelBiblio.

Mathieu
Comment 26 I'm just a bot 2013-10-30 07:58:30 UTC
Patch applied cleanly, go forth and signoff
Comment 27 mathieu saby 2013-11-03 18:38:47 UTC Comment hidden (obsolete)
Comment 28 mathieu saby 2013-11-03 18:39:35 UTC
Of course, UT cannot be tested on a sandbox, but the other aspects of the patch can.

Mathieu
Comment 29 Chris Cormack 2013-12-27 20:06:07 UTC
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
Comment 30 mathieu saby 2013-12-28 00:11:21 UTC Comment hidden (obsolete)
Comment 31 Marcel de Rooy 2014-01-17 15:33:51 UTC
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?
Comment 32 mathieu saby 2014-01-17 16:32:12 UTC
(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
Comment 33 Marcel de Rooy 2014-01-20 09:22:40 UTC
(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.
Comment 34 mathieu saby 2014-01-22 08:48:13 UTC
(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
Comment 35 Marcel de Rooy 2014-01-22 12:03:30 UTC
(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.
Comment 36 mathieu saby 2014-01-26 17:52:09 UTC
(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
Comment 37 Marcel de Rooy 2014-01-27 12:29:45 UTC Comment hidden (obsolete)
Comment 38 Marcel de Rooy 2014-01-27 12:30:00 UTC Comment hidden (obsolete)
Comment 39 Marcel de Rooy 2014-01-27 12:33:28 UTC
(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.
Comment 40 mathieu saby 2014-01-27 20:41:35 UTC
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
Comment 41 Marcel de Rooy 2014-01-28 12:02:09 UTC
(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
Comment 42 mathieu saby 2014-01-31 15:46:01 UTC
I'll take a look next week

Mathieu
Comment 43 mathieu saby 2014-02-02 22:32:33 UTC
I'm not sure of the origin of the conflict. I prefer to investigate a little more to avoid a mistake.

Mathieu
Comment 44 mathieu saby 2014-02-02 22:49:56 UTC
In fact the conflict was trivial ;-)
Comment 45 mathieu saby 2014-02-02 22:51:02 UTC Comment hidden (obsolete)
Comment 46 mathieu saby 2014-02-02 23:11:09 UTC
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
Comment 47 Marcel de Rooy 2014-02-03 10:31:36 UTC Comment hidden (obsolete)
Comment 48 Marcel de Rooy 2014-02-03 10:32:19 UTC
Follow-up resubmitted to restore patch order. Will still answer previous comment.
Comment 49 Marcel de Rooy 2014-02-03 10:40:27 UTC Comment hidden (obsolete)
Comment 50 Marcel de Rooy 2014-02-03 10:40:56 UTC
Updated the commit message of the second patch too.
Comment 51 Marcel de Rooy 2014-02-03 10:49:39 UTC Comment hidden (obsolete)
Comment 52 Marcel de Rooy 2014-02-03 10:56:33 UTC Comment hidden (obsolete)
Comment 53 Marcel de Rooy 2014-02-03 11:10:27 UTC
(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.
Comment 54 Marcel de Rooy 2014-02-03 11:16:39 UTC
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?
Comment 55 Marcel de Rooy 2014-02-10 11:12:57 UTC
(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?
Comment 56 mathieu saby 2014-02-17 07:26:59 UTC
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
Comment 57 Marcel de Rooy 2014-02-17 07:34:38 UTC
(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 :)
Comment 58 mathieu saby 2014-03-30 22:00:33 UTC
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)
Comment 59 mathieu saby 2014-03-30 22:21:13 UTC Comment hidden (obsolete)
Comment 60 mathieu saby 2014-03-30 22:22:44 UTC
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
Comment 61 Koha Team Lyon 3 2014-04-17 14:50:29 UTC
 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....
Comment 62 mathieu saby 2014-04-17 18:37:45 UTC Comment hidden (obsolete)
Comment 63 mathieu saby 2014-04-17 18:40:22 UTC
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
Comment 64 Koha Team Lyon 3 2014-04-18 12:22:58 UTC
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.
Comment 65 mathieu saby 2014-04-18 16:10:34 UTC
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
Comment 66 mathieu saby 2014-04-18 16:13:36 UTC
Oups, ok, in fact there is a problem if the record is NOT deleted. The message "Deleted record" is displayed and should not.
Comment 67 mathieu saby 2014-04-18 16:19:40 UTC
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
Comment 68 mathieu saby 2014-05-28 11:48:11 UTC Comment hidden (obsolete)
Comment 69 mathieu saby 2014-05-28 11:49:43 UTC
Patch rebased
Comment 70 Levast 2014-06-03 08:49:01 UTC
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"
Comment 71 Lise 2014-06-03 08:49:48 UTC
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)
Comment 72 mathieu saby 2014-07-13 22:59:25 UTC
(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
Comment 73 mathieu saby 2014-07-13 23:16:42 UTC
Created attachment 29668 [details] [review]
[PATCH 2/2] Bug 10869 Followup: Unit tests for DelOrder

Unit tests need a little rebase
Comment 74 mathieu saby 2014-07-13 23:19:31 UTC
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
Comment 75 Paola Rossi 2014-08-06 09:39:12 UTC
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.
Comment 76 mathieu saby 2014-08-06 10:03:07 UTC
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 :(
Comment 77 mathieu saby 2014-08-06 10:08:59 UTC
I won't touch it again for the moment
Comment 78 mathieu saby 2014-08-08 12:13:43 UTC
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
Comment 79 Paola Rossi 2014-12-02 15:21:15 UTC
(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?
Comment 80 mathieu saby 2014-12-04 20:09:16 UTC
(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 81 M. Tompsett 2016-01-25 02:17:04 UTC
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.
Comment 82 Séverine Queune 2018-03-15 20:11:26 UTC
It would be great if someone could work on this patch again !
The problem is still valid.
Comment 83 Chris Cormack 2018-09-15 17:28:17 UTC
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>
Comment 84 Chris Cormack 2018-09-15 17:28:23 UTC
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
Comment 85 Chris Cormack 2018-09-15 17:28:29 UTC
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.
Comment 86 Chris Cormack 2018-09-15 17:30:29 UTC
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
Comment 87 Séverine Queune 2018-09-15 19:02:04 UTC
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 !
Comment 88 Katrin Fischer 2018-09-27 22:35:49 UTC
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).
Comment 89 Nick Clemens 2019-03-21 14:29:37 UTC
*** Bug 22554 has been marked as a duplicate of this bug. ***