I've applied the (7) patches against master 3.17.00.023

I selected a vendor. On acqui/supplier.pl, I could select "+New" contract or "+New" vendor without errors. But, if I selected "+New" basket this Software error occurred:

Can't use string ("") as a subroutine ref while "strict refs" in use at /var/root-koha/bug-XXXX/acqui/basketheader.pl line 109

Same error occurred selecting "+New basket" on the list of the vendors, on /acqui/booksellers.pl.

So I pass the patch to "Failed QA".

Today I've applied the patches against master 3.17.00.023

About step 4/ I tried:
t/db_dependent/Letters.t
t/db_dependent/Bookseller.t
t/db_dependent/Acquisition/Invoices.t
t/db_dependent/Acquisition.t
t/db_dependent/Acquisition/NewOrder.t
t/db_dependent/Budgets.t
t/db_dependent/Acquisition/CancelReceipt.t
t/db_dependent/Acquisition/GetBasketsInfosByBookseller.t
t/db_dependent/Acquisition/GetOrdersByBiblionumber.t
t/db_dependent/Acquisition/OrderFromSubscription.t
t/db_dependent/Acquisition/TransferOrder.t
t/db_dependent/Acquisition/close_reopen_basket.t
t/db_dependent/Contract.t

>perl t/db_dependent/Acquisition/OrderFromSubscription.t 
1..13
ok 1 - use C4::Acquisition;
ok 2 - use C4::Biblio;
ok 3 - use C4::Budgets;
ok 4 - use C4::Serials;
Undefined subroutine &C4::Bookseller::AddBookseller called at t/db_dependent/Acquisition/OrderFromSubscription.t line 18.
# Looks like you planned 13 tests but ran 4.
# Looks like your test exited with 255 just after 4.

So I pass the patch to "Failed QA" status.

Thanks, Jonathan.

Today I've tried to apply the patches against master 3.17.00.025

The first patch is 5342.
 
Applying: Bug 5342: Serial claiming improvements: add a counter
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/Acquisition/OrderFromSubscription.t
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/kohastructure.sql
Failed to merge in the changes.
Patch failed at 0001 Bug 5342: Serial claiming improvements: add a counter

I've resolved the conflict about updatedatabase.pl.
Then , being in doubt, I immediately proved all the 5342's *.t. 
One error occurred:

prove t/db_dependent/Acquisition/OrderFromSubscription.t
t/db_dependent/Acquisition/OrderFromSubscription.t .. 1/13 # Looks like you planned 13 tests but ran 12.
t/db_dependent/Acquisition/OrderFromSubscription.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 1/13 subtests 
 
Test Summary Report
-------------------
t/db_dependent/Acquisition/OrderFromSubscription.t (Wstat: 65280 Tests: 12 Failed: 0)
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 13 tests but ran 12.
Files=1, Tests=12,  1 wallclock secs ( 0.00 usr  0.02 sys +  0.84 cusr  0.22 csys =  1.08 CPU)
Result: FAIL
-----------------------------------------------------------------------------
So I pass the patch to "Patch doesn't apply" status.

Bug 5342 rebased. Should be ok now.

Now I've tried to apply the patches against master 3.17.00.025 again.

The first patch was 5342, and it was OK, thanks.

Having read:
<http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12827#c22>,

and having seen some errors occurred proving:
prove t/db_dependent/Letters.t

I applied the patch 12953, and so the t/db_dependent/Letters.t became OK.

Next applied patch was then 12830:
Applying: Bug 12830: Move the order-related code into Koha::Acquisition::Order
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/Letters.t
CONFLICT (content): Merge conflict in t/db_dependent/Letters.t
Auto-merging t/db_dependent/Acquisition/OrderFromSubscription.t
Failed to merge in the changes.
Patch failed at 0001 Bug 12830: Move the order-related code into Koha::Acquisition::Order

And so I pass the patch 12896 to "Patch doesn't apply" status.

(In reply to Paola Rossi from comment #9)
> And so I pass the patch 12896 to "Patch doesn't apply" status.

Now bug 12830 depends on bug 12953.
This one should apply correctly on top.

Looks like there are a number of merge conflicts with the unit test file.

Forgot to apply the dependancies.

Bug 12852 should be applied on top of bug 5342, and then this patch.

Hi Paola, I have tried working my way through the dependencies here, but I must be missing something. Can you still get this to apply? Or maybe you still have your testing branch and could take a look at the sequence you applied the patches in?

(In reply to Katrin Fischer from comment #15)
> Hi Paola, I have tried working my way through the dependencies here, but I
> must be missing something. Can you still get this to apply? Or maybe you
> still have your testing branch and could take a look at the sequence you
> applied the patches in?

Kind Katrin, I'm sorry, I'm not able to help you.

I signed off on Sep the 19-th, against master 3.17.00.025, attaching to bugzilla an 0004* file.
So I suppose I followed exactly the Jonathan's note of comment 10:

>> Now bug 12830 depends on bug 12953.
>>This one should apply correctly on top.

Today [3.17.00.033] the patch 12953 (1) is already pushed to master, whilst the patch 12830 (2) not.
So, applying against master, the two 12830 are OK, but this patch 12896 is not OK [as it should be expected after Sep 23-th]. This is the conflict:

Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/Acquisition/OrderFromSubscription.t
CONFLICT (content): Merge conflict in t/db_dependent/Acquisition/OrderFromSubscription.t
Auto-merging acqui/addorderiso2709.pl
Failed to merge in the changes.
Patch failed at 0001 Bug 12896: Move the bookseller-related code into Koha::Acquisition::Bookseller
--------------------------------------------------------------------
The 5342's dependency was required later, on the 23-th. But I see conflict:
1) either applying 5342 soon after 12830, 
2) or applying 12852/29-th [as Jonathan asked in comment 14] after 12830, 
3) or applying 12852/PassedQA Oct 11 (after 12830 or without 12830) 

So I'm sorry, I can't help you.
----------------------------------------------
5342's conflict after applying 12830:
Applying: Bug 5342: Serial claiming improvements: add a counter
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
error: The following untracked working tree files would be overwritten by merge:
        t/db_dependent/Serials/Claims.t
Please move or remove them before you can merge.
Aborting
Failed to merge in the changes.
Patch failed at 0001 Bug 5342: Serial claiming improvements: add a counter
-----------------------------
The conflict of the patch 12852, version of Sep, the 29-th,  after applying 12830:
Applying: Bug 12852: The "preview" param can be removed serials/claims.pl
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging serials/claims.pl
CONFLICT (content): Merge conflict in serials/claims.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/serials/claims.tt
Failed to merge in the changes.
Patch failed at 0001 Bug 12852: The "preview" param can be removed serials/claims.pl
======================================================================

Hi Paola, thx for trying.  I think we will have to wait for Jonathan then to help. I have QA'd some of the other patches and will if I can do more without this one.

I've tried to apply against master 3.17.00.035 (head bug 13088)

At first I applied 12852 (OK).

Then:
Applying: Bug 12896: Move the bookseller-related code into Koha::Acquisition::Bookseller
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
error: The following untracked working tree files would be overwritten by merge:
        Koha/Acquisition/Bookseller.pm
Please move or remove them before you can merge.
Aborting
Failed to merge in the changes.
Patch failed at 0001 Bug 12896: Move the bookseller-related code into Koha::Acquisition::Bookseller

I keep the patch in the "Patch doesn't apply" status.

(In reply to Paola Rossi from comment #18)

There only was a small conflict on the number of unit tests.

> Falling back to patching base and 3-way merge...
> error: The following untracked working tree files would be overwritten by
> merge:
>         Koha/Acquisition/Bookseller.pm
> Please move or remove them before you can merge.
> Aborting
> Failed to merge in the changes.
> Patch failed at 0001 Bug 12896: Move the bookseller-related code into
> Koha::Acquisition::Bookseller

You should remove the file and retry to apply this patch.

Back to needs signoff.

(In reply to Jonathan Druart from comment #20)
> Back to needs signoff.

I meant to Signed off!

Applies cleanly on master now :) But will have to continue tomorrow.

Hi Jonathan, 

I tested all files included in the patch and they work fine... but:

git grep GetBookSeller
serials/claims.pl:use C4::Bookseller qw( GetBookSeller );
serials/claims.pl:    @supplierinfo=GetBookSeller($supplierid);
t/db_dependent/Bookseller.t:#Test GetBookSeller
t/db_dependent/Bookseller.t:    "Get Supplier1 (GetBookSellerFromId)" );

... and serials claims actually explodes when you try to open it.

(In reply to Katrin Fischer from comment #23)
> Hi Jonathan, 
> 
> I tested all files included in the patch and they work fine... but:
> 
> git grep GetBookSeller
> serials/claims.pl:use C4::Bookseller qw( GetBookSeller );
> serials/claims.pl:    @supplierinfo=GetBookSeller($supplierid);
> t/db_dependent/Bookseller.t:#Test GetBookSeller
> t/db_dependent/Bookseller.t:    "Get Supplier1 (GetBookSellerFromId)" );
> 
> ... and serials claims actually explodes when you try to open it.

git remote update
git checkout -b bug_12896 origin/master
git bz apply 12852
git bz apply 12896
git grep GetBookSeller
misc/release_notes/release_notes_3_10_0.txt:    8223    minor   Properly format SQL query in C4::Bookseller::GetBookSeller
t/db_dependent/Bookseller.t:#Test GetBookSeller
t/db_dependent/Bookseller.t:    "Get Supplier1 (GetBookSellerFromId)" );

You certainly forgot to apply 12852 before.

You are totally right... dependencies dependencies... will retest tonight. All other things I tested worked nicely, I am being carefully optimistic :)

 C4/Acquisition.pm                                  |   4 +-
 C4/Bookseller.pm                                   |  53 +--------
 Koha/Acquisition/Bookseller.pm                     | 124 +++++++++++++++++++++
 acqui/addorderiso2709.pl                           |   No regression found.
 acqui/basket.pl                                    |   No regression found.
 acqui/basketgroup.pl                               |   No regression found.
 acqui/basketheader.pl                              |   No regression found.
 acqui/booksellers.pl                               |   No regression found.
 acqui/finishreceive.pl                             |   No regression found.
 acqui/invoice.pl                                   |   No regression found.
 acqui/invoices.pl                                  |   No regression found.
 acqui/modordernotes.pl                             |   No regression found.
 acqui/neworderbiblio.pl                            |   No regression found.
 acqui/neworderempty.pl                             |   No regression found.
 acqui/newordersubscription.pl                      |   No regression found.
 acqui/newordersuggestion.pl                        |   No regression found.
 acqui/orderreceive.pl                              |   No regression found.
 acqui/parcel.pl                                    |   No regression found.
 acqui/parcels.pl                                   |   No regression found.
 acqui/supplier.pl                                  |   No regression found.
 acqui/transferorder.pl                             |   No regression found.
 acqui/uncertainprice.pl                            |   No regression found.
 acqui/z3950_search.pl                              |   No regression found.
 admin/aqcontract.pl                                |   No regresison found.
 catalogue/moredetail.pl                            |   No regression found.
 serials/acqui-search-result.pl                     |   No regression found.
 serials/subscription-detail.pl                     |   No regression found.
 t/db_dependent/Acquisition.t                       |   Pass
 t/db_dependent/Acquisition/Invoices.t              |   Pass
 t/db_dependent/Acquisition/OrderFromSubscription.t |   Pass
 t/db_dependent/Bookseller.t                        |   Pass
 t/db_dependent/Letters.t                           |   Pass
 
Serials
 - Subscription search
 - Subscription detail - vendor shows
 - Subscription edit - vendor can be searched and linked
 
Acquisitons - vendors
 - searching for vendors
 - vendor details, editing a vendors

Acquisitions - baskets & basket groups
 - adding a new basket group
 - printing PDF

Acquisitons - uncertain prices
- viewing uncertain prices list
 
Acquisitions - contracts
 - adding, editing, deleting a contract
 
Acquisition - baskets
- adding a new basket
- changing basket header details
- order from z39.50
- order from 'new'
- order from existing
- order from staged
- order from subscription
- order from suggestion
- transferring an order
- adding vendor and internal note
 
Acquisition - receive shipment
- receive a new shipment
- reveiving orders
- undo receipt
- canceling an order - giving a reason (new)
- finish receiving

Acquisitions - invoices
- Searching for invoices
- Closing an invoice
- Viewing an invoice

 Catalog - detail page
 - Acquisition tab
 - Items tab - display of bookseller and acquisition information
 
Also:
git grep GetBookSeller - no references found.
git grep GetBookSellerFromId - no references found.

Created attachment 33855 [details] [review]
Bug 12896: Move the bookseller-related code into Koha::Acquisition::Bookseller

The C4::Acquisition module should be exploded in order to add
readability and maintainability to this part of the code.

This patch is a POC, it introduces a new Koha::Acquisition::Bookseller module and put in
it the code from GetBookSeller and GetBookSellerFromId.

Test plan:
1/ Create a bookseller, modify it.
2/ Add contacts for this bookseller
3/ Create an order, receive it, transfer it
4/ Launch the prove command on all unit tests modified by this patch and
verify that all pass.

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 33856 [details] [review]
Bug 12896: Remove 2 occurrences in comments

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Passes tests and QA script.
Full test report on the bug report.

Patches rebased.

Patches pushed to master.

Thanks Jonathan!

Major refactoring, and moving of code, so I wont be pushing this to a stable branch.