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.
+1 ;-) Mathieu
I think it deserves P3 or P4 priority Mathieu
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
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
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?
Where is it done elsewhere in code?
I think statistics, action_logs, etc. keep the biblionumber.
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
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
Sorry Mathieu, I would have to take a look at the documentation and experiment as well.
*** Bug 8638 has been marked as a duplicate of this bug. ***
(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.
Thanks I have looked at the documentation of MySQL, and that's I understood, but I was not sure of that... Mathieu
*** Bug 6920 has been marked as a duplicate of this bug. ***
Created attachment 21296 [details] [review] [PATCH] Bug 10758 - Show bibliographic information of deleted records in acquisitions This patch must be applied on top of patch 10869 Aim of the patch : display information stored in deletedbiblio and deletedbiblioitems tables in basket.tt and neworderempty.tt pages Content : - modification of database structure : new column "deletedbiblionumber" in aqorders - modification of GetOrder, GetOrders and GetCancelledOrders in C4::Acquisition - modification of DelBiblio in C4::Biblio - modification of the 2 templates Maybe some changes must be made to UT, and maybe the performance of my queries is not optimal. But please first test to check if it is doing what it is meant for... To test: - create a basket with 5 orders using records A,B,C,D (you can use existing records or create new ones, it does not matter) - delete 2 orders A & B, but keep the records - in the catalogue, suppress record A (used by a cancelled order) & D (used by an active order) - refresh the basket page : you should see the suppressed orders in Cancelled Orders block, with readable title and authors of deleted records, and a note (Deleted record) - click on "Modifiy" for order D. You should see the title, author, publisher, date... without the link [Edit Record] but with a message (Deleted record, non editable) - click on "Modify" for record C. You should have [Edit record] link - create an order E. - delete order E, and the record (link 'delete order and record') - refresh the basket. you should see the order in the Cancelled Orders, with a readable title and author M. Saby
Patch applied cleanly, go forth and signoff
Note : Bug 10869 has been signed off. Mathieu
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
(I suppose Jonathan's code is better) Mathieu
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
Depends on: 10869 You cannot test on sandbox Mathieu
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
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.
(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
I won't make Biblio.t unimarc compliant, that's too complex. But I'm still working on writing UT. Mathieu
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
Created attachment 22106 [details] [review] [PATCH] Bug 10758 - Show bibliographic information of deleted records in acquisitions Some modification to the original patch + 2 UT I still need to write 2 UT. Mathieu
patch does not apply anymore. I'm working on that
I think factorizing code would make things much easier. Katrin and Jonathan, could you take a look at Bug 11206 please? Mathieu
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
Bug 11224 is pushed
Created attachment 26830 [details] [review] [PATCH 1/2] Bug 10758 - Show bibliographic information of deleted records in acquisitions No more dependancy Fully testable, but a second patch will be provided for fixing and improving the UTs Aim of the patch : when a record is deleted but used in an order, display information stored in deletedbiblio and deletedbiblioitems tables on some pages of acquisition module Note that this patch will only have real effects for records deleted AFTER its application. Changes : - modification of database structure : new column "deletedbiblionumber" in aqorders We keep the constraint between aqorders.biblionumber and biblio.biblionumber, but we create a new one between aqorders.deletedbiblionumber and deletedbiblio.biblionumber With this system, we can make a JOIN between aqorders and deleted(biblio/biblioitems) table, and retreive information. - modification of GetOrder, GetOrders, GetCancelledOrders and SearchOrders in C4::Acquisition : Those subs are now also getting bibliographic information from deleted tables deletedbiblio and deletedbiblioitems Only usefull or potentially usefull fields are retreived (no more biblio.* and biblioitems.*) For each field, I used COALESCE in SQL query. Ex: COALESCE (biblio.title,deletedbiblio.title) AS title - modification of DelBiblio in C4::Biblio Wen a record is deleted with DelBiblio, the sub checks if it used in an order If so, the biblionumber is deleted from aqorders.biblionumber (as before), but it is copied to aqorder.deletedbiblionumber - modification of files in acqui : basket.pl, neworderempty.pl To transmit the new key "deletedbiblionumber" to the templates - modification of templates in intranet/module/acqui : basket.tt, histsearch.tt, lateorders.tt, neworderempty.tt, parcel.tt Test plan : A. BEFORE applying the patch - create a basket with at least 4 orders,using different records - in the catalog, delete one of the record used by 1st order, whithout cancelling the order (there will be an alert, but you can do that) - return to the basket page (or refresh it) : the information about the record is lost - if you want, try to receive orders or to search orders : in those pages too the information for this record will be lost B. Apply the patch and run updatedatabase.pl C. Go back to your basket and refresh it - the information of the record deleted before the application of the patch is still lost ("Can't find title") - in the catalog, delete one of the record used by 2d order, whithout cancelling the order - return to the basket page (or refresh it) : the information about the record is still visible, with the mention (Deleted record) - Try to modify the 2d order : click on "Modifiy" on the right column of the table - you will get on neworderempty.pl, but with a message "(Deleted record, not editable)". You will be able to change accounting details but not bibliographic information (In a future development, we could imagine to recreate a record and in order to make the order editable even if the original record is deleted, but it would be dangerous to do so now) - in the basket, cancel the 3rd order AND the record (click on "Delete order and record") - in the "Cancelled orders", you still could see bibliographic information about the cancelled order, with the mention "(Deleted record)" D. Go on late orders page - check you can see information about 2d order (which is linked with a deleted record) E. Go on order advanced search page - fill the form with the title of 2d order and click on Search - check Koha can retreive the order, and show correct information F. Try to receive orders from the vendor used for your basket - in the filters on the left of the page which displays the table of pending orders, search the title of the 2d order - Koha should retreive the 2d order - you must have a mention "Deleted record, order cannot be received" above the title - you must not have a link "Receive" on the left of the table (In a future development, we could imagine to recreate a record to do the receipt, but it would be dangerous to do so now. The best to do if a record was deleted and the order not yet received is probably to cancel the order, and to recreate it by hand)
I'm working on UT, but you can already test. Mathieu
Created attachment 26832 [details] [review] [PATCH 2/2] Bug 10758 - Fix Acquisition unit tests This patch fixes Acquisition UT Maybe more work could be done to write specific UT for testing the new features introduced by bug 10758 To test : prove -v t/db_dependant/Acquisition.t
Created attachment 27273 [details] [review] [PATCH 1/2] Bug 10758 - Show bibliographic information of deleted records in acquisitions (I removed the changes to Koha/Schema/Result/Aqorder.pm, as it should not be integrated in the main patch)
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
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.
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
Created attachment 27312 [details] [review] [PATCH 1/2] Bug 10758 - Show bibliographic information of deleted records in acquisitions Rebased
Created attachment 27313 [details] [review] [PATCH 2/2] Bug 10758 - Fix Acquisition unit tests
Created attachment 27314 [details] [review] [PATCH 1/2] Bug 10758 - Show bibliographic information of deleted records in acquisitions
Created attachment 27315 [details] [review] [PATCH 2/2] Bug 10758 - Fix Acquisition unit tests
Created attachment 28514 [details] [review] [PATCH 1/2] Bug 10758 - Show bibliographic information of deleted records in acquisitions
Created attachment 28515 [details] [review] [PATCH 2/2] Bug 10758 - Fix Acquisition unit tests
Patch rebased
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 "-and" (Might be a runaway multi-line << string starting on line 8169) Can't find string terminator "shareurl" 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.
Created attachment 29667 [details] [review] [PATCH 1/2] Bug 10758 - Show bibliographic information of deleted records in acquisitions Indeed, that horrible ">>>HEAD" was breaking everything! It's fixed. Mathieu
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.
Created attachment 30551 [details] [review] [PATCH 1/2] Bug 10758 - Show bibliographic information of deleted records in acquisitions 1st patch rebased
ooops, still a mistake, please don't test for the moment
Created attachment 30552 [details] [review] [PATCH] Bug 10758 - Show bibliographic information of deleted records in acquisitions
rebased again, it applies.
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.
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.
Well, in fact you were probably right :(
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
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
(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?
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.
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
What is the current status of this issue?
(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.
It would be great if this got some attention. If it would be possible to just display the info in the Acquisitions side rather than just deleting the title and other information. Maybe remove the links and add an internal note (item deleted) or something to that effect?
Created attachment 95782 [details] [review] Bug 10758: WIP Started working on this but then realised we do not have Koha::Old::Biblios classes. It is getting complicated and will put a spoke in bug 20271's wheel
I'd really love to see progress here, it's a big issue in the acquisition module as we display the cancelled orders so prominently. I get asked to manually delete them on a regular basis by one our libraries as you can't see what was cancelled.
That should be done on top of bug 20271.
(In reply to Jonathan Druart from comment #67) > That should be done on top of bug 20271. We could do it in a different way too - I know that we got some logic in a table now where 2 biblionumbers are stored in different columns - that way FK can be set. Martin did it, I just fail to remember the bug report and table.
(In reply to Katrin Fischer from comment #68) > (In reply to Jonathan Druart from comment #67) > > That should be done on top of bug 20271. > > We could do it in a different way too - I know that we got some logic in a > table now where 2 biblionumbers are stored in different columns - that way > FK can be set. Martin did it, I just fail to remember the bug report and > table. Yes, but it is adding more stuffs to unravel for bug 20271.
This is definitely an ongoing issue - users have no way of telling what was cancelled.
+1 - Displaying the bib info for cancelled orders is essential functionality.
Hi I proposed a patch for that bug a loooong time ago. I suppose it is obsolete now, but I am a bit sad to see that the situation has not evolved. Is it reasonable to expect a solution in the foreseeable future? M. Saby
+1 and voted :-)
+1 This would be a fantastic feature to have.
We would also be interested in this feature
Hi Is there still a will to work on that? Or should https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20271 be pushed first?
I believe this has been kept open for way too long, we need to find a fix independent of bug 20271 because we keep losing data and bug 20271 is no quick fix. The least we need to do is make sure the biblionumber is not lost from aqorders when the record is deleted.
Now that 33262 has been pushed, we could possibly fix this. I have seen some work on deleted objects on other bugs, maybe we could even benefit from that work here.
I am gonna try start moving this again by limiting the scope first. Lets start here with showing the deleted title on the basket page.
Created attachment 162016 [details] [review] Bug 10758: Show title of deleted biblio on basket page Test plan: Find a completed order line and a cancelled one with deleted biblios. Goto acqui/basket.pl Check if you see the title if deleted_biblionumber is filled.
(In reply to Marcel de Rooy from comment #79) > I am gonna try start moving this again by limiting the scope first. Lets > start here with showing the deleted title on the basket page. So, if we get this further, lets open an omnibus and record the occurrences in ACQ where we might wanna see it too.
(In reply to Marcel de Rooy from comment #79) > I am gonna try start moving this again by limiting the scope first. Lets > start here with showing the deleted title on the basket page. +1 :)
Created attachment 162034 [details] [review] Bug 10758: Show title of deleted biblio on basket page Test plan: Find a completed order line and a cancelled one with deleted biblios. Goto acqui/basket.pl Check if you see the title if deleted_biblionumber is filled. Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Thanks Marcel !