Bug 7593 - merging bib records loses connection to order line
Summary: merging bib records loses connection to order line
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Mathieu Saby
QA Contact:
URL:
Keywords:
: 9564 (view as bug list)
Depends on:
Blocks: 8149 10270
  Show dependency treegraph
 
Reported: 2012-02-24 18:37 UTC by Nicole C. Engard
Modified: 2013-12-05 20:09 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
[PATCH] Bug 7593: Move orders on the destination record when merging 2 records (1.62 KB, patch)
2013-03-13 15:24 UTC, Mathieu Saby
Details | Diff | Splinter Review
[PATCH][Revised] Bug 7593: Move orders on the destination record when merging 2 records (1.68 KB, patch)
2013-03-18 20:43 UTC, Mathieu Saby
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7593: Move orders on the destination record when merging 2 records (1.75 KB, patch)
2013-03-19 08:48 UTC, Julian Maurice
Details | Diff | Splinter Review
[PATCH] Bug 7593 : Follow-up : store the biblioitemnumber of kept record into the order (1.53 KB, patch)
2013-04-03 10:07 UTC, Mathieu Saby
Details | Diff | Splinter Review
[PATCH][rewritten] Bug 7593: Move orders on the destination record when merging 2 records (6.60 KB, patch)
2013-04-24 16:42 UTC, Mathieu Saby
Details | Diff | Splinter Review
Bug 7593: Move orders on the destination record when merging 2 records (6.64 KB, patch)
2013-04-26 13:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PATCH 2/2] Bug 7593 : QA followup : restores deleted line in C4::Acquisition (928 bytes, patch)
2013-04-30 06:20 UTC, Mathieu Saby
Details | Diff | Splinter Review
[ALREADY PUSHED][PASSED QA] Bug 7593: Move orders on the destination record when merging 2 records (6.75 KB, patch)
2013-05-03 06:29 UTC, Katrin Fischer
Details | Diff | Splinter Review
[ALREADY PUSHED][PASSED QA] Bug 7593 : QA followup : restores deleted line in C4::Acquisition (1.06 KB, patch)
2013-05-03 06:29 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 7593: Fix Unit tests (1.23 KB, patch)
2013-05-13 10:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7593: Fix unit tests (1.22 KB, patch)
2013-05-13 12:01 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nicole C. Engard 2012-02-24 18:37:45 UTC
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.
Comment 1 Katrin Fischer 2012-02-24 19:39:06 UTC
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.
Comment 2 Nicole C. Engard 2012-02-26 20:49:33 UTC
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?
Comment 3 Katrin Fischer 2012-02-26 21:00:18 UTC
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.
Comment 4 Nicole C. Engard 2012-02-26 21:22:36 UTC
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
Comment 5 laurenthdl 2012-02-26 21:55:52 UTC
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.
Comment 6 Nicole C. Engard 2012-02-26 22:09:59 UTC
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!
Comment 7 Mathieu Saby 2013-02-22 08:13:06 UTC
Same problem as BZ 9564...

M. Saby
Comment 8 Katrin Fischer 2013-02-22 08:32:59 UTC
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.
Comment 9 Mathieu Saby 2013-02-22 08:34:41 UTC
Maybe...
We use 3.10, and it is not working. We create items when receiving.

M. Saby
Rennes 2 university
Comment 10 Mathieu Saby 2013-03-09 11:52:52 UTC
*** Bug 9564 has been marked as a duplicate of this bug. ***
Comment 11 Mathieu Saby 2013-03-13 15:24:32 UTC Comment hidden (obsolete)
Comment 12 Julian Maurice 2013-03-18 12:29:45 UTC
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.
Comment 13 Mathieu Saby 2013-03-18 12:32:06 UTC
Logical ;-)
I will rewrite it tomorrow.
But I am surprised it worked when I tested it last week...

M. Saby
Comment 14 Mathieu Saby 2013-03-18 20:43:21 UTC Comment hidden (obsolete)
Comment 15 Julian Maurice 2013-03-19 08:48:38 UTC Comment hidden (obsolete)
Comment 16 Julian Maurice 2013-03-19 08:49:40 UTC
It works better now ;-)
Signed off.
Comment 17 Jonathan Druart 2013-03-20 14:52:43 UTC
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.
Comment 18 Mathieu Saby 2013-03-21 15:06:55 UTC
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
Comment 19 Mathieu Saby 2013-04-03 10:06:16 UTC
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?
Comment 20 Mathieu Saby 2013-04-03 10:07:11 UTC Comment hidden (obsolete)
Comment 21 Jonathan Druart 2013-04-03 12:14:18 UTC
(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?
Comment 22 Mathieu Saby 2013-04-03 12:24:15 UTC
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
Comment 23 Jonathan Druart 2013-04-04 08:46:40 UTC
(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.
Comment 24 Mathieu Saby 2013-04-04 08:51:15 UTC
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
Comment 25 Jonathan Druart 2013-04-04 08:57:42 UTC
(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
Comment 26 Mathieu Saby 2013-04-06 09:41:46 UTC
So I will wait until 9780 and 9987 are pushed into master to re-write my patches for bz7593.

M. Saby
Comment 27 Mathieu Saby 2013-04-24 16:01:32 UTC
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
Comment 28 Mathieu Saby 2013-04-24 16:42:53 UTC Comment hidden (obsolete)
Comment 29 Kyle M Hall 2013-04-26 13:09:37 UTC Comment hidden (obsolete)
Comment 30 Katrin Fischer 2013-04-30 06:10:31 UTC
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?
Comment 31 Mathieu Saby 2013-04-30 06:20:43 UTC Comment hidden (obsolete)
Comment 32 Katrin Fischer 2013-05-03 06:29:11 UTC
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.
Comment 33 Katrin Fischer 2013-05-03 06:29:24 UTC
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.
Comment 34 Jared Camins-Esakov 2013-05-12 14:31:11 UTC
This patch has been pushed to master and 3.12.x.
Comment 35 Jared Camins-Esakov 2013-05-12 14:48:19 UTC
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?
Comment 36 Jared Camins-Esakov 2013-05-13 00:46:40 UTC
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.
Comment 37 Jonathan Druart 2013-05-13 10:03:58 UTC Comment hidden (obsolete)
Comment 38 Mathieu Saby 2013-05-13 10:24:15 UTC
Hello
Thank you Jonathan for the fix.
Are you sure of the "41" instead of "42" ? (I did not check)

Mathieu
Comment 39 Mathieu Saby 2013-05-13 11:25:13 UTC
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
Comment 40 Jonathan Druart 2013-05-13 11:59:49 UTC
(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).
Comment 41 Jonathan Druart 2013-05-13 12:01:02 UTC
Created attachment 18114 [details] [review]
Bug 7593: Fix unit tests
Comment 42 Mathieu Saby 2013-05-13 12:29:39 UTC
If you think this test is not very useful, you can supress the line.

Mathieu
Comment 43 Jared Camins-Esakov 2013-05-14 17:46:40 UTC
This patch has been pushed to master and 3.12.x.
Comment 44 Jared Camins-Esakov 2013-05-16 11:14:31 UTC
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.
Comment 45 Mathieu Saby 2013-05-16 11:17:50 UTC
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
Comment 46 Jared Camins-Esakov 2013-05-16 11:20:59 UTC
(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?
Comment 47 Mathieu Saby 2013-05-16 11:47:24 UTC
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
Comment 48 Jared Camins-Esakov 2013-05-16 11:58:57 UTC
(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.
Comment 49 Mathieu Saby 2013-05-16 12:26:32 UTC
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
Comment 50 Mathieu Saby 2013-05-16 12:35:49 UTC
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
Comment 51 Jonathan Druart 2013-05-16 12:43:37 UTC
(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.
Comment 52 Mathieu Saby 2013-05-16 12:45:52 UTC
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
Comment 53 Mathieu Saby 2013-05-16 12:52:14 UTC
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
Comment 54 Jonathan Druart 2013-05-16 13:55:49 UTC
(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)
Comment 55 Chris Cormack 2013-05-16 19:49:18 UTC
(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.
Comment 56 Jonathan Druart 2013-05-17 07:29:28 UTC
(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.
Comment 57 Mathieu Saby 2013-05-17 07:59:44 UTC
> > 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
Comment 58 Paul Poulain 2013-05-17 09:01:09 UTC
(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 - )
Comment 59 Mathieu Saby 2013-05-17 10:14:15 UTC
I signed off BZ 10270
Mathieu
Comment 60 Mathieu Saby 2013-05-19 13:19:01 UTC
Jared, can you confirm this is definitly pushed?

Mathieu
Comment 61 Jared Camins-Esakov 2013-05-19 13:32:40 UTC
Yes, this and all the follow-ups are in 3.12.x and master.
Comment 62 Mathieu Saby 2013-05-19 13:52:29 UTC
Great. My colleagues will be really happy!

Mathieu