Bug 12896 - Move the bookseller-related code into its own module
Summary: Move the bookseller-related code into its own module
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 12830 12852
Blocks: 15449 12826 12969 13726
  Show dependency treegraph
 
Reported: 2014-09-09 13:58 UTC by Jonathan Druart
Modified: 2017-06-14 22:10 UTC (History)
6 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
Bug 12896: Move the bookseller-related code into Koha::Acquisition::Bookseller (45.96 KB, patch)
2014-09-09 14:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12896: Move the bookseller-related code into Koha::Acquisition::Bookseller (45.96 KB, patch)
2014-09-15 10:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12896: Move the bookseller-related code into Koha::Acquisition::Bookseller (46.87 KB, patch)
2014-09-18 07:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12896: Move the bookseller-related code into Koha::Acquisition::Bookseller (46.87 KB, patch)
2014-09-18 09:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12896: Move the bookseller-related code into Koha::Acquisition::Bookseller (48.25 KB, patch)
2014-09-19 09:05 UTC, Paola Rossi
Details | Diff | Splinter Review
Bug 12896: Move the bookseller-related code into Koha::Acquisition::Bookseller (46.95 KB, patch)
2014-11-03 14:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12896: Remove 2 occurrences in comments (1.35 KB, patch)
2014-11-06 09:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PASSED QA] Bug 12896: Move the bookseller-related code into Koha::Acquisition::Bookseller (47.02 KB, patch)
2014-11-06 18:34 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 12896: Remove 2 occurrences in comments (1.48 KB, patch)
2014-11-06 18:34 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 12896: Move the bookseller-related code into Koha::Acquisition::Bookseller (46.94 KB, patch)
2014-11-24 13:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12896: Remove 2 occurrences in comments (1.46 KB, patch)
2014-11-24 13:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PASSED QA] Bug 12969: Add a subroutine to calculate VAT and prices (16.72 KB, patch)
2014-12-27 19:13 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2014-09-09 13:58:17 UTC
The C4::Acquisition module should be exploded in order to add readability and maintainability to this part of the code.

I propose to create a new Koha::Acquisition::Bookseller module and to put in it the code related to the bookseller management.
Comment 1 Jonathan Druart 2014-09-09 14:19:11 UTC Comment hidden (obsolete)
Comment 2 Paola Rossi 2014-09-12 15:17:33 UTC
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".
Comment 3 Jonathan Druart 2014-09-15 10:06:07 UTC Comment hidden (obsolete)
Comment 4 Paola Rossi 2014-09-17 16:09:14 UTC
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.
Comment 5 Jonathan Druart 2014-09-18 07:15:02 UTC Comment hidden (obsolete)
Comment 6 Paola Rossi 2014-09-18 09:13:05 UTC
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.
Comment 7 Jonathan Druart 2014-09-18 09:21:17 UTC
Bug 5342 rebased. Should be ok now.
Comment 8 Jonathan Druart 2014-09-18 09:25:30 UTC Comment hidden (obsolete)
Comment 9 Paola Rossi 2014-09-18 12:09:09 UTC
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.
Comment 10 Jonathan Druart 2014-09-18 13:14:44 UTC
(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.
Comment 11 Paola Rossi 2014-09-19 09:05:12 UTC Comment hidden (obsolete)
Comment 12 Kyle M Hall 2014-09-19 12:47:32 UTC
Looks like there are a number of merge conflicts with the unit test file.
Comment 13 Kyle M Hall 2014-09-19 12:57:31 UTC
Forgot to apply the dependancies.
Comment 14 Jonathan Druart 2014-09-23 10:42:31 UTC
Bug 12852 should be applied on top of bug 5342, and then this patch.
Comment 15 Katrin Fischer 2014-10-27 22:08:55 UTC
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?
Comment 16 Paola Rossi 2014-10-28 11:31:43 UTC
(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
======================================================================
Comment 17 Katrin Fischer 2014-10-28 12:26:27 UTC
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.
Comment 18 Paola Rossi 2014-10-28 16:19:18 UTC
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.
Comment 19 Jonathan Druart 2014-11-03 14:46:48 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2014-11-03 14:47:58 UTC
(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.
Comment 21 Jonathan Druart 2014-11-03 14:48:28 UTC
(In reply to Jonathan Druart from comment #20)
> Back to needs signoff.

I meant to Signed off!
Comment 22 Katrin Fischer 2014-11-05 21:58:36 UTC
Applies cleanly on master now :) But will have to continue tomorrow.
Comment 23 Katrin Fischer 2014-11-05 22:57:07 UTC
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.
Comment 24 Jonathan Druart 2014-11-06 09:44:22 UTC
(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.
Comment 25 Jonathan Druart 2014-11-06 09:46:41 UTC Comment hidden (obsolete)
Comment 26 Katrin Fischer 2014-11-06 10:20:16 UTC
You are totally right... dependencies dependencies... will retest tonight. All other things I tested worked nicely, I am being carefully optimistic :)
Comment 27 Katrin Fischer 2014-11-06 18:26:43 UTC
 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.
Comment 28 Katrin Fischer 2014-11-06 18:34:15 UTC Comment hidden (obsolete)
Comment 29 Katrin Fischer 2014-11-06 18:34:30 UTC Comment hidden (obsolete)
Comment 30 Jonathan Druart 2014-11-24 13:55:43 UTC
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>
Comment 31 Jonathan Druart 2014-11-24 13:55:47 UTC
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.
Comment 32 Jonathan Druart 2014-11-24 13:56:11 UTC
Patches rebased.
Comment 33 Katrin Fischer 2014-12-27 19:13:05 UTC Comment hidden (obsolete)
Comment 34 Tomás Cohen Arazi 2014-12-31 19:55:50 UTC
Patches pushed to master.

Thanks Jonathan!
Comment 35 Chris Cormack 2015-01-03 23:31:33 UTC
Major refactoring, and moving of code, so I wont be pushing this to a stable branch.