+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. ***

Patch applied cleanly, go forth and signoff

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

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

I'm working on UT, but you can already test.

Mathieu

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 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 &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.

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.

ooops, still a mistake, please don't test for the moment

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 !

Created attachment 165229 [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>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>

Removed the dependency on bug 20271, since this was reworked to go forward without it.

The code looks good, and QA test pass. Passing QA

(In reply to Emily Lamancusa from comment #86)
> Removed the dependency on bug 20271, since this was reworked to go forward
> without it.
> 
> The code looks good, and QA test pass. Passing QA

Thanks !

Filed this in 2013, it got 40 votes over time, so happy to see it finally resolved :)

I've filed bug 36714 and 36713 to cover invoices and order search.

Pushed for 24.05!

Well done everyone, thank you!

Not backported to 23.11.x

Hi Frido, could you maybe reconsider? I think this one could also be seen as a long awaited bug fix.

(In reply to Katrin Fischer from comment #91)
> Hi Frido, could you maybe reconsider? I think this one could also be seen as
> a long awaited bug fix.

Good point.

It is a small patch and applies without conflict so go ;)

Pushed to 23.11.x for 23.11.08

Not backporting to 23.05.x unless requested