Bug 10758 - Show bibliographic information of deleted records in acquisitions
Summary: Show bibliographic information of deleted records in acquisitions
Status: NEW
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P4 enhancement (vote)
Target Milestone: ---
Assignee: Bugs List
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
: 6920 8638 (view as bug list)
Depends on:
Blocks:
 
Reported: 2013-08-19 09:23 UTC by Katrin Fischer
Modified: 2018-10-30 23:40 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Bot Control: ---
When did the bot last check this: 2013-09-24 00:00:00
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
[PATCH] Bug 10758 - Show bibliographic information of deleted records in acquisitions (21.28 KB, patch)
2013-09-21 17:27 UTC, mathieu saby
Details | Diff | Splinter Review
[PATCH] Bug 10758 - Show bibliographic information of deleted records in acquisitions (50.03 KB, patch)
2013-10-20 20:31 UTC, mathieu saby
Details | Diff | Splinter Review
[PATCH 1/2] Bug 10758 - Show bibliographic information of deleted records in acquisitions (51.75 KB, patch)
2014-04-05 09:26 UTC, mathieu saby
Details | Diff | Splinter Review
[PATCH 2/2] Bug 10758 - Fix Acquisition unit tests (3.21 KB, patch)
2014-04-05 15:07 UTC, mathieu saby
Details | Diff | Splinter Review
[PATCH 1/2] Bug 10758 - Show bibliographic information of deleted records in acquisitions (50.69 KB, patch)
2014-04-17 19:07 UTC, mathieu saby
Details | Diff | Splinter Review
[PATCH 1/2] Bug 10758 - Show bibliographic information of deleted records in acquisitions (50.19 KB, patch)
2014-04-19 19:27 UTC, mathieu saby
Details | Diff | Splinter Review
[PATCH 2/2] Bug 10758 - Fix Acquisition unit tests (3.33 KB, patch)
2014-04-19 19:27 UTC, mathieu saby
Details | Diff | Splinter Review
[PATCH 1/2] Bug 10758 - Show bibliographic information of deleted records in acquisitions (50.19 KB, patch)
2014-04-19 19:29 UTC, mathieu saby
Details | Diff | Splinter Review
[PATCH 2/2] Bug 10758 - Fix Acquisition unit tests (3.33 KB, patch)
2014-04-19 19:30 UTC, mathieu saby
Details | Diff | Splinter Review
[PATCH 1/2] Bug 10758 - Show bibliographic information of deleted records in acquisitions (50.55 KB, patch)
2014-05-28 11:39 UTC, mathieu saby
Details | Diff | Splinter Review
[PATCH 2/2] Bug 10758 - Fix Acquisition unit tests (3.33 KB, patch)
2014-05-28 11:42 UTC, mathieu saby
Details | Diff | Splinter Review
[PATCH 1/2] Bug 10758 - Show bibliographic information of deleted records in acquisitions (50.54 KB, patch)
2014-07-13 22:35 UTC, mathieu saby
Details | Diff | Splinter Review
[PATCH 1/2] Bug 10758 - Show bibliographic information of deleted records in acquisitions (50.66 KB, patch)
2014-08-06 09:31 UTC, mathieu saby
Details | Diff | Splinter Review
[PATCH] Bug 10758 - Show bibliographic information of deleted records in acquisitions (50.48 KB, patch)
2014-08-06 09:37 UTC, mathieu saby
Details | Diff | Splinter Review
[PATCH] Bug 10758 - Show bibliographic information of deleted records in acquisitions (50.83 KB, patch)
2014-08-08 12:39 UTC, mathieu saby
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2013-08-19 09:23:23 UTC
When a record is deleted the order information in acquisition is missing the title and other information otherwise shown. As we store the information in the deleted* tables we could use that for display.
Comment 1 mathieu saby 2013-08-19 09:27:57 UTC
+1 ;-)
Mathieu
Comment 2 mathieu saby 2013-08-19 12:46:49 UTC
I think it deserves P3 or P4 priority

Mathieu
Comment 3 mathieu saby 2013-09-01 22:14:16 UTC
I think we need to change the behavior of these subs in C4/Acquisitions.pm:
GetOrders
GetOrder
GetOrdersByBiblionumber
GetCancelledOrders
GetPendingOrders 
SearchOrder
GetLateOrders
GetInvoices 
GetInvoiceDetails 

maybe also :
GetHistory

There is no join of acqorders and biblio in other subs.

Katrin, I am not sure of the best way of doing it : do you think I can just edit the SQL for requesting deleted_biblio and deleted_biblioitem table? Or maybe it will have bad consequencies on performances? 

Mathieu
Comment 4 mathieu saby 2013-09-08 13:03:21 UTC
I think we also need a way to pass to the basket template the information about the "deletion status" of each record, in order to suppress the HTML link pointing to the biblio for each order (if the biblio is deleted, the title should be display in plain text without link).
The ideal would be that the "deletion status" of each record should be included in the results of GetOrders, for each order.

Currently there is a "ON DELETE SET NULL" constraint on biblio.biblionumber on table aqorders : 
  CONSTRAINT `aqorders_ibfk_2` FOREIGN KEY (`biblionumber`) REFERENCES `biblio` (`biblionumber`) ON DELETE SET NULL ON UPDATE CASCADE

I suppose one solution could be alter this constraint to suppress the "ON DELETE SET NULL", so that the biblionumber is kept in aqorders table, even if the biblio is deleted.

But maybe it would be better to keep this column unchanged, and to create a new one, "aqorders.deletedbiblionumber", to be filled when a record is suppressed.
The advantage would be to be able to know direclty if an order is linked to an existing record or to a deleted record.

In fact I wonder why we have deleted(biblio/biblioitems/items) tables. Would not have been simplier to keep suppressed items and records in the main table, and to add a colum "deleted" in those tables? So the subs about Orders, reserves, circulation... won't have to use union between deleted* and main tables.

M. Saby
Comment 5 Katrin Fischer 2013-09-09 15:48:08 UTC
Hm, I think the separate tables are there for reasons - I am not sure how to resolve the problem with the deleted biblionumbers. I tend to just keep the number, as we do in other places and add querying of the deleted* table if the information we are looking for is not found in the current table.

Maybe we can get some more opinions/ideas on this?
Comment 6 mathieu saby 2013-09-09 15:52:15 UTC
Where is it done elsewhere in code?
Comment 7 Katrin Fischer 2013-09-09 15:54:43 UTC
I think statistics, action_logs, etc. keep the biblionumber.
Comment 8 mathieu saby 2013-09-09 16:04:41 UTC
Yes, of course, statistics table keeps the itemnumber.
I will take a look.
I think stats reports page in staff interface do not JOIN on deleteditems, only on items.

I would like C4 subs to return the information "this order is / is not link to a deleted biblio". I am not sure it could be done in 1 SQL request, because if I make a "UNION ALL" between biblio and deletedbiblio, I think the resulting temporary table will just have the same fields as biblio and deletedbiblio, without any information about the deleted status of some biblios.
Am I wrong?
If not, the solution could be to make 2 or 3 requests (one with "select aqorders left join biblio using biblionumber where biblio.biblionumber is not null", the 2d with "select aqorders left join deletedbiblio using biblionumber where deletedbiblio.biblionumber is not null").

I am not a SQL specialist... So what do you think of it?
Maybe we could ask koha-devel?

Mathieu
Comment 9 mathieu saby 2013-09-09 20:47:56 UTC
In statistics table, there is no constraint on biblionumber at all.
I suppose it won't be good for safety reason to merely suppress existing constraint in aqorders, so I edited updatedatabse to suppress the "ON DELETE SET NULL", and keep only "ON UPDATE CASCADE".
It is the 1st time I try to do this kind of manipulation... and I did it wrong : when I tried to suppress a biblio record linked to an order, it raised a SQL error.
Could you tell me how to write the constraint to link aqorders.biblionumber with biblio.biblionumber, but to keep the value of  aqorders.biblionumber when the biblio is suppressed ?

Mathieu
Comment 10 Katrin Fischer 2013-09-09 20:51:08 UTC
Sorry Mathieu, I would have to take a look at the documentation and experiment as well.
Comment 11 mathieu saby 2013-09-14 08:56:34 UTC
*** Bug 8638 has been marked as a duplicate of this bug. ***
Comment 12 Jonathan Druart 2013-09-16 08:22:32 UTC
(In reply to mathieu saby from comment #9)
> Could you tell me how to write the constraint to link aqorders.biblionumber
> with biblio.biblionumber, but to keep the value of  aqorders.biblionumber
> when the biblio is suppressed ?

No possible. To do that, I think you must add a new column in aqorders which point to deletedbiblio.biblionumber.
You have to delete the biblio, the aqorders.biblionumber will be set to NULL.
Next update the aqorders.deletedbiblionumber.
Comment 13 mathieu saby 2013-09-16 08:26:57 UTC
Thanks
I have looked at the documentation of MySQL, and that's I understood, but I was not sure of that...

Mathieu
Comment 14 mathieu saby 2013-09-18 07:54:29 UTC
*** Bug 6920 has been marked as a duplicate of this bug. ***
Comment 15 mathieu saby 2013-09-21 17:27:32 UTC Comment hidden (obsolete)
Comment 16 I'm just a bot 2013-09-24 05:37:54 UTC
Patch applied cleanly, go forth and signoff
Comment 17 I'm just a bot 2013-09-24 05:38:21 UTC
Patch applied cleanly, go forth and signoff
Comment 18 mathieu saby 2013-09-26 09:59:48 UTC
Note : Bug 10869 has been signed off.

Mathieu
Comment 19 mathieu saby 2013-10-01 22:30:40 UTC
I have just seen that Jonathan Druart has made an other choice (COALESCE biblio.title, deletedbiblio.title) for the same purpose in his bug for improving order search (bug 5336, attachment 21689 [details] [review]).
I am not a database expert, so could somebody check the code and tell me if my solution (UNION ALL...) is good?

M. Saby
Comment 20 mathieu saby 2013-10-02 07:03:13 UTC
(I suppose Jonathan's code is better)
Mathieu
Comment 21 Pierre Angot 2013-10-11 11:52:44 UTC
There is a problem on sandbox : 

Some problems occurred applying patches from bug 10758:
<h1>Something went wrong !</h1>Applying: Bug 10758 - Show bibliographic information of deleted records in acquisitions
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 10758 - Show bibliographic information of deleted records in acquisitions
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 10758 - Show bibliographic information of deleted records in acquisitions
Comment 22 mathieu saby 2013-10-11 11:55:43 UTC
Depends on: 	10869
You cannot test on sandbox

Mathieu
Comment 23 mathieu saby 2013-10-11 11:58:36 UTC
By the way, if Jonathan or Katrin could drop an eye on my code before somebody try to sign it off, I would appreciate (especially the queries changed in C4/Acquisitions)
If somebody sign it off and if QA tell me I need to rewrite it on a different base it afterwards, the tester would have work for nothing ;-)

Mathieu
Comment 24 Katrin Fischer 2013-10-11 12:08:43 UTC
Hi Mathieu,

I don't have a lot of time until after KohaCon probably - but it would really help this along if you can provide unit tests for the changed routines.
Comment 25 mathieu saby 2013-10-13 21:00:23 UTC
(In reply to Katrin Fischer from comment #24)
> Hi Mathieu,
> 
> I don't have a lot of time until after KohaCon probably - but it would
> really help this along if you can provide unit tests for the changed
> routines.

Ok.
I will take  the occasion for improving Biblio.t. It is failing with my unimarc database.

Mathieu
Comment 26 mathieu saby 2013-10-20 09:22:14 UTC
I won't make Biblio.t unimarc compliant, that's too complex. But I'm still working on writing UT.

Mathieu
Comment 27 mathieu saby 2013-10-20 12:59:31 UTC
I see UT for Acquisition subs in Acquisition.t and in separate files for TransferOrder, OrderFromSubscription and GetOrdersByBiblionumber.

If I want to add UT for NewOrder, DelOrder, GetOrder, GetOrders, etc etc etc , what is the better : use Acquisition.t, whith comments to make clear which Sub I am testing, or creating a new file of each one?

Mathieu
Comment 28 mathieu saby 2013-10-20 20:31:59 UTC Comment hidden (obsolete)
Comment 29 mathieu saby 2013-10-23 07:14:29 UTC
patch does not apply anymore. I'm working on that
Comment 30 mathieu saby 2013-11-06 09:33:56 UTC
I think factorizing code would make things much easier.
Katrin and Jonathan, could you take a look at Bug 11206 please?

Mathieu
Comment 31 mathieu saby 2013-11-09 13:51:11 UTC
In fact, the 1st step is adding UT to current master. Then, I will work again on that bug.
See Bug 11224 for new UTs

Mathieu
Comment 32 mathieu saby 2014-04-03 21:52:59 UTC
Bug 11224 is pushed
Comment 33 mathieu saby 2014-04-05 09:26:09 UTC Comment hidden (obsolete)
Comment 34 mathieu saby 2014-04-05 09:27:16 UTC
I'm working on UT, but you can already test.

Mathieu
Comment 35 mathieu saby 2014-04-05 15:07:26 UTC Comment hidden (obsolete)
Comment 36 mathieu saby 2014-04-17 19:07:21 UTC Comment hidden (obsolete)
Comment 37 mathieu saby 2014-04-17 19:08:10 UTC
Please note that bug 10869 is linked to this bug.

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 38 Koha Team Lyon 3 2014-04-18 12:45:49 UTC
test on a sandbox :
<h1>Something went wrong !</h1>Applying: Bug 10758 - Fix Acquisition unit tests
Applying: Bug 10758 - Show bibliographic information of deleted records in acquisitions
fatal: sha1 information is lacking or useless (installer/data/mysql/updatedatabase.pl).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Comment 39 mathieu saby 2014-04-18 16:28:56 UTC
I don't understand.
It applies properly against master on my VM, either with "git bz apply 10758" or by applying manually the 2 patches with "git am".

Maybe a sandbox issue?

I put it back to needs signoff. If the problem persists I hope someone could help me on that :(

Mathieu
Comment 40 mathieu saby 2014-04-19 19:27:17 UTC Comment hidden (obsolete)
Comment 41 mathieu saby 2014-04-19 19:27:54 UTC Comment hidden (obsolete)
Comment 42 mathieu saby 2014-04-19 19:29:54 UTC Comment hidden (obsolete)
Comment 43 mathieu saby 2014-04-19 19:30:19 UTC Comment hidden (obsolete)
Comment 44 mathieu saby 2014-05-28 11:39:32 UTC Comment hidden (obsolete)
Comment 45 mathieu saby 2014-05-28 11:42:55 UTC
Created attachment 28515 [details] [review]
[PATCH 2/2] Bug 10758 - Fix Acquisition unit tests
Comment 46 mathieu saby 2014-05-28 11:43:11 UTC
Patch rebased
Comment 47 Paola Rossi 2014-06-06 16:13:11 UTC
I apply against master 3.17.00.007.

I think that the first patch (Show bibliographic ...) has an error:
on updtedatabase.pl it requires this insertion:

+<<<<<<< HEAD

----------------------------------------------------
Anyway the Web installer Step 3 gives this error report on updating database structure:
Update report :
    Status: 500
    Content-type: text/html
    <h1>Software error:</h1>
    <pre>syntax error at /var/root-koha/bug-7162/installer/data/mysql/updatedatabase.pl line 8172, near &quot;-and&quot;
    (Might be a runaway multi-line &lt;&lt; string starting on line 8169)
    Can't find string terminator &quot;shareurl&quot; anywhere before EOF at /var/root-koha/bug-7162/installer/data/mysql/updatedatabase.pl line 8174.
    </pre>
    <p>
    For help, please send mail to the webmaster (<a href="mailto:webmaster@linux-test">webmaster@linux-test</a>), giving this error message
    and the time and date of the error.
    </p>

Update errors :

    [Fri Jun 6 17:46:50 2014] updatedatabase.pl: Use of bare << to mean <<"" is deprecated at /var/root-koha/bug-7162/installer/data/mysql/updatedatabase.pl line 8162.
    [Fri Jun 6 17:46:50 2014] updatedatabase.pl: Use of bare << to mean <<"" is deprecated at /var/root-koha/bug-7162/installer/data/mysql/updatedatabase.pl line 8169.
    [Fri Jun 6 17:46:50 2014] updatedatabase.pl: syntax error at /var/root-koha/bug-7162/installer/data/mysql/updatedatabase.pl line 8172, near "-and"
    [Fri Jun 6 17:46:50 2014] updatedatabase.pl: (Might be a runaway multi-line << string starting on line 8169)
    [Fri Jun 6 17:46:50 2014] updatedatabase.pl: Can't find string terminator "shareurl" anywhere before EOF at /var/root-koha/bug-7162/installer/data/mysql/updatedatabase.pl line 8174.
--------------------------------------------------------------------------- 
I pass the patch to "Patch doesn't apply" status.
Comment 48 mathieu saby 2014-07-13 22:35:43 UTC Comment hidden (obsolete)
Comment 49 Paola Rossi 2014-08-06 08:41:33 UTC
I've tried to apply the patches against master 3.17.00.011.

Applying: Bug 10758 - Show bibliographic information of deleted records in acquisitions
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/parcel.tt
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging acqui/neworderempty.pl
Auto-merging acqui/basket.pl
Auto-merging C4/Biblio.pm
CONFLICT (content): Merge conflict in C4/Biblio.pm
Auto-merging C4/Acquisition.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 10758 - Show bibliographic information of deleted records in acquisitions
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 50 mathieu saby 2014-08-06 09:31:06 UTC Comment hidden (obsolete)
Comment 51 mathieu saby 2014-08-06 09:32:06 UTC
ooops, still a mistake, please don't test for the moment
Comment 52 mathieu saby 2014-08-06 09:37:12 UTC Comment hidden (obsolete)
Comment 53 mathieu saby 2014-08-06 09:52:04 UTC
rebased again, it applies.
Comment 54 Paola Rossi 2014-08-08 09:56:34 UTC
Kind Mr. Mathieu Saby, I'm in doubt that the patch "Show..." that you filed on Aug the 6-th may not be the right one.

The date inside the "Show ..." patch is :
Date: Fri, 4 Apr 2014 09:36:52 +0200
and the patch is about 3.17.00.008 changing updatedatabase.pl .

Could you please verify? Thanks in advance.

Anyway today the patch doesn't apply [against master 3.17.00.013] :

Applying: Bug 10758 - Show bibliographic information of deleted records in acquisitions
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Failed to merge in the changes.
Patch failed at 0001 Bug 10758 - Show bibliographic information of deleted records in acquisitions

So I pass the patch to "Patch doesn't apply" status.
Comment 55 mathieu saby 2014-08-08 11:36:31 UTC
My machine does not update the date when I rebase a patch. I don't know if there is an option for that. So, all the versions of this patch - that are obsoleted - are dated from April 4.
I'm going to rebase it again and set manually a new date.
Comment 56 mathieu saby 2014-08-08 11:48:44 UTC
Well, in fact you were probably right :(
Comment 57 mathieu saby 2014-08-08 12:39:12 UTC
Created attachment 30623 [details] [review]
[PATCH] Bug 10758 - Show bibliographic information of deleted records in acquisitions

Rebased
I've tested it, it seems to work

Mathieu
Comment 58 mathieu saby 2014-08-09 16:03:39 UTC
Bug 7162 (passed QA) will have consequencies for this one, as it is refactoring DelOrder. I will have to make some adjusments. So do not test before 7162 is passed, it will be a waste of time...
I change the status to "in discussion".

Matheiu
Comment 59 Paola Rossi 2014-12-02 15:19:51 UTC
(In reply to mathieu saby from comment #58)
> Bug 7162 (passed QA) will have consequencies for this one, as it is
> refactoring DelOrder. I will have to make some adjusments. So do not test
> before 7162 is passed, it will be a waste of time...
> I change the status to "in discussion".
> 
> Matheiu

Hi Mathieu,

bug 7162 has been pushed to master. What about this bug now?
Comment 60 Nicole C. Engard 2015-05-14 15:37:06 UTC
Removing in discussion as it seems we can maybe get this in now? It's a big big deal to track what was ordered/deleted for reorder and accounting purposes.
Comment 61 mathieu saby 2015-05-14 21:55:59 UTC
Sorry Katrin, I'm to far from Koha now to continue my work on that issue. So if someone want to work on it, feel free to take my patches as a base. I'm unassinging me.

Mathieu
Comment 62 Sandre Cunha 2016-08-06 09:23:11 UTC
What is the current status of this issue?
Comment 63 Jonathan Druart 2016-08-08 09:23:50 UTC
(In reply to Sandre Cunha from comment #62)
> What is the current status of this issue?

The bug report is not assigned to anyone. A patch has been submitted few years ago but need to be rebased and certainly adapted to master.