Many libraries follow this process: 1. Enter brief record in Aquisitions basket 2. Receive items in Aquisitions 3. Cataloger pulls in full record 4. Merge full record and Acq record Now, if you do this and choose the new full record as your base record then you end up losing the connection to the order line because in essence the bib record that was on that order has been deleted. We need some sort of check in place to prevent this from happening. The easiest I can think of is to update the bib number in the order line as part of the merge record process, but I'm up for any suggestion that does not mean that we have to trust the user to choose the Acq order record as their base and then check all the boxes for all the fields on the full record by hand.
Perhaps overwriting the record instead of merging would be a good work around? And easier to do because you don't have to worry about which bibnumber to keep.
Which record do you overwrite? Right now when you use the merge tool it asks which record to keep as the newly merged record and which to delete - so how would Koha pick which one to keep?
What I meant was editing the acquisition record in cataloguing and overwriting it there with a z39.50 search. This way you don't need to create a second record and the bibnumber problem will not happen.
That's a process change for some libraries where the cataloging and acq happen in different departments. Many libraries subscribe to services like OCLC and import batches of records to overlay the brief records entered in acquisitions and the overlaying process is not foolproof (as we know) and so sometimes you're left with duplicates. The merging process should check before deleting a record to make sure that it's not breaking anything (like the fix we put in place that merges holds when records are merged). Nicole
My 2 cents : rather than asking for one more automated check, why not asking for a warning when merging records, to show which has orders / issues / reserves / subscriptions depending on it so that librarian would be aware of what he does. And yes, the merge could move those from one record to an other. But it is quite unclear which module the functions to move the biblionumber from a to b should then belong to. If this would be in Acquisition, Circulation, Reserves, Serials, then the script may end up loading quite a wide range of modules. If those functions should belong to the Biblio module, then it would edit some "unrelated" tables. We could go on adding some SQL to THAT pl file, but this is not an option.
A warning to tell the librarian which of the records they're merging have things attached to them would be great. Anything to help prevent them from deleting the wrong thing would be great!
Same problem as BZ 9564... M. Saby
Is it possible that this worked in some versions? I tested it for a library of ours like 2 months ago on a 3.6.3 or 3.6.10 and the bibnumber in the aqorders table was updated correctly according to my notes. The library creates items when placing orders.
Maybe... We use 3.10, and it is not working. We create items when receiving. M. Saby Rennes 2 university
*** Bug 9564 has been marked as a duplicate of this bug. ***
Created attachment 16093 [details] [review] [PATCH] Bug 7593: Move orders on the destination record when merging 2 records Note : this patch uses Acquisition::GetOrdersByBiblionumber created by bug 9780, which is not signed off yet. So you need to apply bug 9780 before testing this one. At present, merging records breaks the link order/record. This patch moves existing orders from deleted record to destination record. To test : 1. create a basket with one order 2. put the record used by this order in a list 3. put an other record in the list 4. merge the 2 records 5. check if the order in the basket is updated M. Saby
When I merge the two records, orders are not updated. I think this is because GetOrdersByBiblionumber is called after DelBiblio and aqorders table has a constraint "ON DELETE SET NULL". So no orders are returned by GetOrdersByBiblionumber. moving your added lines above the "#Deleting the other record" block may fix the problem.
Logical ;-) I will rewrite it tomorrow. But I am surprised it worked when I tested it last week... M. Saby
Created attachment 16311 [details] [review] [PATCH][Revised] Bug 7593: Move orders on the destination record when merging 2 records Revision : execute code before deleting the record Note : this patch uses Acquisition::GetOrdersByBiblionumber created by bug 9780. So you need to apply bug 9780 before testing this one. At present, merging records breaks the link order/record. This patch moves existing orders from deleted record to destination record. To test : 1. create a basket with one order 2. put the record used by this order in a list 3. put an other record in the list 4. merge the 2 records 5. check if the order in the basket is updated M. saby
Created attachment 16322 [details] [review] [SIGNED-OFF] Bug 7593: Move orders on the destination record when merging 2 records Revision : execute code before deleting the record Note : this patch uses Acquisition::GetOrdersByBiblionumber created by bug 9780. So you need to apply bug 9780 before testing this one. At present, merging records breaks the link order/record. This patch moves existing orders from deleted record to destination record. To test : 1. create a basket with one order 2. put the record used by this order in a list 3. put an other record in the list 4. merge the 2 records 5. check if the order in the basket is updated Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
It works better now ;-) Signed off.
QA comment: + $myorder->{'biblioitemnumber'} = $tobiblio; #maybe useless is wrong! I think this field is not used, but should not be equal to biblionumber. Marked as Failed QA.
It seems that my patch is no longer needed : in current master, the problem it was made for seems to be fixed by an other patch (I don't know which one...). I will test again later, and if I am wrong I will re-write my patch. M. Saby
Jonathan, In fact I think the problem described in bug 7593 is still present in master. My VM was probably corrupted when I tested during Hackfest. You still need to apply BZ 9780 to test this patch. BZ 9780 is signed off but did not yet pass QA. If 7595 is more needed than 9780, I can copy the function GetOrdersByBiblionumber in 7595. If I suppress the line + $myorder->{'biblioitemnumber'} = $tobiblio; #maybe useless the biblioitemnumber of the suppressed record will still be present in aqorders.biblioitemnumber, and that is not clean. I thought the simpliest thing to do was to copy the biblionumber of the record we keep in aqorders.biblionumber and aqorders.biblioitemnumber, because I assumed biblionumber and biblioitemnumber were always the same. But maybe I am wrong, so I used Acquisition::GetBiblioItemByBiblioNumber to get the biblioitemnumber of $tobiblio record. I am not sure I did it in a "perlish" way. Could you take a look at my followup please?
Created attachment 17162 [details] [review] [PATCH] Bug 7593 : Follow-up : store the biblioitemnumber of kept record into the order With this followup, the biblioitemnumber of $tobiblio is stored into the orderline. To test : 1. create 2 records A and B 2. use record A in an order. 3. merge A and B (keep B) 4. check your basket : record B sould be used in order 5. check your database (using a report or in mysql) : SELECT biblioitemnumber FROM aqorders WHERE biblionumber= [Biblionumber of record B] => the result of the request should be the biblioitemnumber of record B (usually the same value as its biblionumber)
(In reply to comment #19) > If I suppress the line > + $myorder->{'biblioitemnumber'} = $tobiblio; #maybe useless > the biblioitemnumber of the suppressed record will still be present in > aqorders.biblioitemnumber, and that is not clean. In which cases?
in all cases : in my tests, without my patch, when you suppress a record, the biblioitemnumber is never suppressed from acqorders (only the biblionumber). Mathieu
(In reply to comment #22) > in all cases : in my tests, without my patch, when you suppress a record, > the biblioitemnumber is never suppressed from acqorders (only the > biblionumber). > > Mathieu I am under the impression that this field is unused. Maybe the good way is to set to null this value.
OK for me if you are sure it is not used, and not going to be used... But if you are right, why not suppressing it from the database? Mathieu
(In reply to comment #24) > OK for me if you are sure it is not used, and not going to be used... > But if you are right, why not suppressing it from the database? > > Mathieu cf bug 9987
So I will wait until 9780 and 9987 are pushed into master to re-write my patches for bz7593. M. Saby
Note : I have just discovered that the link between orders and biblio record is NOT always broken. If some items of the suppressed record are linked to acqorders table, the function Items::MoveItemFromBiblio (called for moving the items) updates the biblionumber used in the order. Now I understand why during the hackfest I thought my patch was useless : I was testing this special case without knowing it... So a patch is still needed if items are created on reception. M. Saby
Created attachment 17657 [details] [review] [PATCH][rewritten] Bug 7593: Move orders on the destination record when merging 2 records I merged previous patchs, took some parts of BZ 9780, and change the test plan... So this patch need to be Signed off again. Revised patch according to QA comments. No more dependent from bz 9780. At present, merging records breaks the link order/record, except if an item of the deleted record is used in the order. This is a serious issue for libraries creating items on receipt. This patch moves existing orders from deleted record to destination record. It creates a new function Acquisitions::GetOrdersByBiblionumber, that could be used by other patches later. To test : Check the problem : 1. Set syspref AcqCreateItem = Create an item when receiving an order 1. Create a basket with one order 2. Put the record used by this order in a list 3. Put an other record in the list 4. Merge the 2 records, keeping as a reference the record NOT used in the order 5. In the order, you will see for that order "Deleted bibliographic information..." 6. Apply the patch 7. Repeat steps 1-4 8. In the order, you will see the title/author of the kept record. 9. Set syspref AcqCreateItem = Create an item when placing an order 10. Repeat steps 1-4 (an item will be created) 11. In the oreder, you will see the title/author of the kept record (it is already the case at present. the patch should not alter this behavior) M. Saby
Created attachment 17700 [details] [review] Bug 7593: Move orders on the destination record when merging 2 records Revised patch according to QA comments. No more dependent from bz 9780. At present, merging records breaks the link order/record, except if an item of the deleted record is used in the order. This is a serious issue for libraries creating items on receipt. This patch moves existing orders from deleted record to destination record. It creates a new function Acquisitions::GetOrdersByBiblionumber, that could be used by other patches later. To test : Check the problem : 1. Set syspref AcqCreateItem = Create an item when receiving an order 1. Create a basket with one order 2. Put the record used by this order in a list 3. Put an other record in the list 4. Merge the 2 records, keeping as a reference the record NOT used in the order 5. In the order, you will see for that order "Deleted bibliographic information..." 6. Apply the patch 7. Repeat steps 1-4 8. In the order, you will see the title/author of the kept record. 9. Set syspref AcqCreateItem = Create an item when placing an order 10. Repeat steps 1-4 (an item will be created) 11. In the oreder, you will see the title/author of the kept record (it is already the case at present. the patch should not alter this behavior) Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment on attachment 17700 [details] [review] Bug 7593: Move orders on the destination record when merging 2 records Review of attachment 17700 [details] [review]: ----------------------------------------------------------------- ::: C4/Acquisition.pm @@ -53,5 @@ > &ModBasketgroup &NewBasketgroup &DelBasketgroup &GetBasketgroup &CloseBasketgroup > &GetBasketgroups &ReOpenBasketgroup > > - &NewOrder &DelOrder &ModOrder &GetPendingOrders &GetOrder &GetOrders > - &GetOrderNumber &GetLateOrders &GetOrderFromItemnumber Hi Mathieu, I think you accidentally removed the line exporting GetOrderNumber, GetLateOrdes and GetOrderFromItemnumber - that looks like it would cause trouble. Can you please take a look?
Created attachment 17833 [details] [review] [PATCH 2/2] Bug 7593 : QA followup : restores deleted line in C4::Acquisition You are completely right! Here is a followup which restores that line. Mathieu
Created attachment 17913 [details] [review] [ALREADY PUSHED][PASSED QA] Bug 7593: Move orders on the destination record when merging 2 records Revised patch according to QA comments. No more dependent from bz 9780. At present, merging records breaks the link order/record, except if an item of the deleted record is used in the order. This is a serious issue for libraries creating items on receipt. This patch moves existing orders from deleted record to destination record. It creates a new function Acquisitions::GetOrdersByBiblionumber, that could be used by other patches later. To test : Check the problem : 1. Set syspref AcqCreateItem = Create an item when receiving an order 1. Create a basket with one order 2. Put the record used by this order in a list 3. Put an other record in the list 4. Merge the 2 records, keeping as a reference the record NOT used in the order 5. In the order, you will see for that order "Deleted bibliographic information..." 6. Apply the patch 7. Repeat steps 1-4 8. In the order, you will see the title/author of the kept record. 9. Set syspref AcqCreateItem = Create an item when placing an order 10. Repeat steps 1-4 (an item will be created) 11. In the oreder, you will see the title/author of the kept record (it is already the case at present. the patch should not alter this behavior) Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Test plan, test suite and QA script pass.
Created attachment 17914 [details] [review] [ALREADY PUSHED][PASSED QA] Bug 7593 : QA followup : restores deleted line in C4::Acquisition This patch restores the line exporting GetOrderNumber that I accidentally suppressed. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Restores a line that was deleted by the first patch. Was not sure if patches should be squashed.
This patch has been pushed to master and 3.12.x.
I suspect this is going to break unit tests on jenkins, in which case I will be reverting it. This line: + isa_ok( ref @listorders, 'ARRAY','GetOrdersByBiblionumber : result is an array' ); seems problematic. Perhaps you want to check that $#listorders > 1, Mathieu?
I would prefer not to revert this if I could get a follow-up fixing the broken test, which is a problem only in the unit test and not in the code itself. If someone can provide a follow-up fixing the unit test by May 16, I will not revert these patches.
Created attachment 18113 [details] [review] Bug 7593: Fix Unit tests
Hello Thank you Jonathan for the fix. Are you sure of the "41" instead of "42" ? (I did not check) Mathieu
In fact I wanted to check if the result is an array, and not something else (hash or scalar). Can you tell me why this line is wrong? isa_ok( ref @listorders, 'ARRAY','GetOrdersByBiblionumber : result is an array' ); I don't know the meaning of "$#listorders ". what is it supposed to check? Mathieu
(In reply to comment #38) > Hello > Thank you Jonathan for the fix. > Are you sure of the "41" instead of "42" ? (I did not check) % prove t/db_dependent/Acquisition.t t/db_dependent/Acquisition.t .. ok All tests successful. Files=1, Tests=41, 0 wallclock secs ( 0.02 usr 0.00 sys + 0.40 cusr 0.02 csys = 0.44 CPU) Result: PASS (In reply to comment #39) > In fact I wanted to check if the result is an array, and not something else > (hash or scalar). > Can you tell me why this line is wrong? > isa_ok( ref @listorders, 'ARRAY','GetOrdersByBiblionumber : result is an > array' ); The parameter should be a reference (http://perldoc.perl.org/functions/ref.html). Else the value is always an empty string. > I don't know the meaning of "$#listorders ". what is it supposed to check? $#array returns the max index of the array @array. So in fact the test I proposed is false. I will provide another patch (we just want to check if the returned value is defined).
Created attachment 18114 [details] [review] Bug 7593: Fix unit tests
If you think this test is not very useful, you can supress the line. Mathieu
We are still getting failures from jenkins on master. I am going to need to revert this today if we (well, someone who isn't me) can't figure out and fix the problem quickly.
Did you test the patch of Jonathan (18114)? I suppose I cannot sign it off, as I am the main contributor to this bug? Mathieu
(In reply to comment #45) > Did you test the patch of Jonathan (18114)? > I suppose I cannot sign it off, as I am the main contributor to this bug? Yes. I pushed it, but we're still getting errors. Could it be a data issue? Is there particular data that is needed for those steps?
I do not know how Jenkins is working. Does it precise which test is wrong? The reason why I added unit test is that P. Poulain said me "new function, new unit test" during Hackfest (the new function was on an other bug at that time) ;-) So, you can suppress some of these tests if you think they are not necessary... Mathieu
(In reply to comment #47) > I do not know how Jenkins is working. Does it precise which test is wrong? Jenkins just has all the sample data on a Debian Squeeze server and runs the entire build/test process every time there is a change to the source code. http://wiki.koha-community.org/wiki/Continuous_Integration If you look at http://jenkins.koha-community.org/job/Koha_master/lastCompletedBuild/testReport/ it gives you a summary, and you can get a bit more information in the log at http://jenkins.koha-community.org/job/Koha_master/lastCompletedBuild/console > The reason why I added unit test is that P. Poulain said me "new function, > new unit test" during Hackfest (the new function was on an other bug at that > time) ;-) > > So, you can suppress some of these tests if you think they are not > necessary... The test is definitely necessary, and looks good. I would not have pushed it without unit tests.
OK, so if I understand Jenkins logs, the 2 tests are failing. Maybe my function is wrong, or maybe the 2 lines in unit test "my $testorder = @$orders[0]; my $testbiblio = $testorder->{'biblionumber'};" I am looking at it right now. Mathieu
I updated my Koha instance, and launch $prove -v Acquisition.t Acquisition.t .. ok 5 - GetOrdersByBiblionumber : no argument, return undef ok 6 - GetOrdersByBiblionumber : result is defined ok 7 - GetOrdersByBiblionumber : result contains at least one element I don't understand : How is it possible that Jenkins fails and not my instance? Mathieu
(In reply to comment #50) > I don't understand : How is it possible that Jenkins fails and not my > instance? Theses tests are db dependent. As Jared said, you should insert the sample data before launching them.
I checked everything is correct by adding warns in my Acquisition.t warn 'record number : '.$testbiblio; my @listorders = GetOrdersByBiblionumber($testbiblio); ok( @listorders ,'GetOrdersByBiblionumber : result is defined' ); ok( scalar (@listorders) >0,'GetOrdersByBiblionumber : result contains at least one element' ); warn 'nborders in listorders : '.scalar (@listorders); warn 'record number in listorders[0] : '.@listorders[0]->{biblionumber}; => everything is correct record number : 2090 at Acquisition.t line 45. ok 6 - GetOrdersByBiblionumber : result is defined ok 7 - GetOrdersByBiblionumber : result contains at least one element nborders in listorders : 1 at Acquisition.t line 49. record number in listorders[0] : 2090 at Acquisition.t line 50. So, as you say, the pbm must come from the test database. Is there a way to look at its data ? Mathieu
Jonathan I did not see you answer. I never did that, and I am not sure it could be done in a few lines (need to find a biblio or create it, find a fund, a vendor etc). So if it is necessary, I could not do that today, and Jared need to revert the bug. Maybe the easier solution would be to take a look at Jenkins sample database, but I don't know where it is. I suppose there must already be some orders in it, because other tests of Acquisiton.t do not fail. Mathieu
(In reply to comment #48) > Jenkins just has all the sample data on a Debian Squeeze server and runs the > entire build/test process every time there is a change to the source code. > http://wiki.koha-community.org/wiki/Continuous_Integration Hello Jared, In fact I don't understand at all why tests fail. Maybe I missed something. What are sample data? Is it sql files in installer/data/mysql/en/optional? I don't find any insert into the acquisition tables and t/db_dependent/Acquisition.t does not add orders. How is created this order? I suspect that the biblionumber key (l.44) is empty but I cannot check it :-/ (I include Chris in the loop)
(In reply to comment #54) > (In reply to comment #48) > > Jenkins just has all the sample data on a Debian Squeeze server and runs the > > entire build/test process every time there is a change to the source code. > > http://wiki.koha-community.org/wiki/Continuous_Integration > > Hello Jared, > In fact I don't understand at all why tests fail. > Maybe I missed something. > What are sample data? Is it sql files in installer/data/mysql/en/optional? > I don't find any insert into the acquisition tables and > t/db_dependent/Acquisition.t does not add orders. How is created this order? > > I suspect that the biblionumber key (l.44) is empty but I cannot check it :-/ > > (I include Chris in the loop) The tests fail, because they are dependent on orders existing for an existing supplier, but the test does not set up any of this up, so they work coincidentally on systems that have it set up, skip on others that have no orders, fail on others that have some orders but not the ones the test is expecting. I will make jenkins have data to make these tests pass, in future it would be good to make the tests test creating and deleting orders too, so that they can pass on any database.
(In reply to comment #55) Chris, Thank you for your quick reply! > The tests fail, because they are dependent on orders existing for an > existing supplier, but the test does not set up any of this up, so they work > coincidentally on systems that have it set up, skip on others that have no > orders, fail on others that have some orders but not the ones the test is > expecting. So what was the case for the DB used by jenkins? Was an order exist ? I think yes: Yesterday, the jenkins report for t/db_dependent/Acquisition.t was [...] ok 2 - NewBasket( 1 , 1 ) returns 18575 ok 3 - GetBasket(18575) returns HASH(0x3b1a960) ok 4 - The reference isa ARRAY ok 5 - GetOrdersByBiblionumber : no argument, return undef not ok 6 - GetOrdersByBiblionumber : result is defined not ok 7 - GetOrdersByBiblionumber : result contains at least one element ok 8 - all orders match ok 9 - AddClaim : Check claimed_date ok 10 - This order has a basketno field ok 11 - This order has a biblioitemnumber field ok 12 - This order has a biblionumber field ok 13 - This order has a invoiceid field [...] If no orders existed we should have: ok 1 - use C4::Acquisition; ok 2 - NewBasket( 1 , 1 ) returns 92 ok 3 - GetBasket(92) returns HASH(0x402ecc8) ok 4 - The reference isa ARRAY ok 5 - GetOrdersByBiblionumber : no argument, return undef ok 6 # skip No Orders, cannot test GetOrdersByBiblionumber ok 7 # skip No Orders, cannot test GetOrdersByBiblionumber ok 8 # skip No Orders, cannot test GetOrdersByBiblionumber ok 9 - AddClaim : Check claimed_date ok 10 # skip No relevant orders in database, cannot test baskets ok 11 # skip No relevant orders in database, cannot test baskets ok 12 # skip No relevant orders in database, cannot test baskets So yesterday an order existed before tests was launched, but I don't find why. It was not inserted by sample data (which I know) or unit test itself. I would like to unravel this mystery :) > I will make jenkins have data to make these tests pass, in future it would > be good to make the tests test creating and deleting orders too, so that > they can pass on any database. Yes, it is what I do and I think Mathieu should propose another patch to go in that way. Otherwise, it will be even more complicated to understand how tests are launched and how launching them with a local DB.
> > I will make jenkins have data to make these tests pass, in future it would > > be good to make the tests test creating and deleting orders too, so that > > they can pass on any database. > > Yes, it is what I do and I think Mathieu should propose another patch to go > in that way. Otherwise, it will be even more complicated to understand how > tests are launched and how launching them with a local DB. It may be beyond my competencies :( I think the best would be some real good developper create a strong and comprehensive set of tests for Acquisition.t. A lot of functions are not tested at that time. Mathieu
(In reply to comment #55) > The tests fail, because they are dependent on orders existing for an > existing supplier, but the test does not set up any of this up, so they work > coincidentally on systems that have it set up, skip on others that have no > orders, fail on others that have some orders but not the ones the test is > expecting. > > I will make jenkins have data to make these tests pass, in future it would > be good to make the tests test creating and deleting orders too, so that > they can pass on any database. Hi Chris, and thx for the DB fix. Is it written somewhere how the jenkins database is filled ? I thought it was filled with default datas, and default datas only, but we're not sure, because other acq tests should have failed. what are we missing ? (about having datas to make there tests pass, that could be Kenza main work. 2 weeks to wait before she's onboard ;-) -and maybe 2-3 more weeks before she can really produce high value code - )
I signed off BZ 10270 Mathieu
Jared, can you confirm this is definitly pushed? Mathieu
Yes, this and all the follow-ups are in 3.12.x and master.
Great. My colleagues will be really happy! Mathieu