Bug 5339 - Parcel closing in acq
Summary: Parcel closing in acq
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: 3.10
Hardware: All All
: P3 enhancement (vote)
Deadline: 2011-05-01
Assignee: Julian Maurice
QA Contact: Bugs List
URL: http://wiki.koha-community.org/wiki/P...
Keywords:
: 5344 6850 8424 (view as bug list)
Depends on: 5344
Blocks: 6504 7163 7288 7627
  Show dependency treegraph
 
Reported: 2010-11-02 02:39 UTC by Paul Poulain
Modified: 2014-05-26 21:04 UTC (History)
12 users (show)

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


Attachments
Update database patch (5.71 KB, patch)
2012-01-06 15:55 UTC, Julian Maurice
Details | Diff | Splinter Review
Invoices management improvement patch (85.83 KB, patch)
2012-01-06 16:03 UTC, Julian Maurice
Details | Diff | Splinter Review
Update database patch (5.59 KB, patch)
2012-02-03 14:16 UTC, Julian Maurice
Details | Diff | Splinter Review
Invoices management improvement patch (85.94 KB, patch)
2012-02-03 14:21 UTC, Julian Maurice
Details | Diff | Splinter Review
Update database patch (5.59 KB, patch)
2012-02-16 14:58 UTC, Julian Maurice
Details | Diff | Splinter Review
Invoices management improvement patch (85.94 KB, patch)
2012-02-16 14:59 UTC, Julian Maurice
Details | Diff | Splinter Review
Update database patch (5.68 KB, patch)
2012-02-16 15:02 UTC, Julian Maurice
Details | Diff | Splinter Review
Invoices management improvement patch (86.46 KB, patch)
2012-02-16 15:03 UTC, Julian Maurice
Details | Diff | Splinter Review
Update database patch (5.69 KB, patch)
2012-02-24 17:08 UTC, Julian Maurice
Details | Diff | Splinter Review
Invoices management improvement patch (89.48 KB, patch)
2012-02-24 17:09 UTC, Julian Maurice
Details | Diff | Splinter Review
7862: Update database patch (5.69 KB, patch)
2012-03-01 11:17 UTC, Julian Maurice
Details | Diff | Splinter Review
7863: Invoices management improvement patch (102.63 KB, patch)
2012-03-01 11:20 UTC, Julian Maurice
Details | Diff | Splinter Review
Update database patch (5.69 KB, patch)
2012-03-20 08:45 UTC, Julian Maurice
Details | Diff | Splinter Review
Invoices management improvement patch (103.07 KB, patch)
2012-03-20 08:45 UTC, Julian Maurice
Details | Diff | Splinter Review
Update database patch (5.65 KB, patch)
2012-03-20 13:22 UTC, Julian Maurice
Details | Diff | Splinter Review
Invoices management improvement patch (102.74 KB, patch)
2012-03-20 13:23 UTC, Julian Maurice
Details | Diff | Splinter Review
Update database patch (5.69 KB, patch)
2012-03-23 09:14 UTC, Julian Maurice
Details | Diff | Splinter Review
Invoices management improvement patch (105.46 KB, patch)
2012-03-23 09:15 UTC, Julian Maurice
Details | Diff | Splinter Review
Update database patch (5.69 KB, patch)
2012-03-28 13:09 UTC, Julian Maurice
Details | Diff | Splinter Review
Invoices management improvement patch (5.69 KB, patch)
2012-03-28 13:10 UTC, Julian Maurice
Details | Diff | Splinter Review
Invoices management improvement patch (105.07 KB, patch)
2012-04-04 07:17 UTC, Julian Maurice
Details | Diff | Splinter Review
Update database patch (5.69 KB, patch)
2012-04-06 12:09 UTC, Julian Maurice
Details | Diff | Splinter Review
Invoices management improvement patch (104.87 KB, patch)
2012-04-06 12:10 UTC, Julian Maurice
Details | Diff | Splinter Review
Rebase to master and added a few features (124.52 KB, patch)
2012-05-01 13:29 UTC, Elliott Davis
Details | Diff | Splinter Review
Bug 5339 rebased and rolled into one commit. Capstone for Elliott (123.48 KB, patch)
2012-06-10 15:55 UTC, MJ Ray (software.coop)
Details | Diff | Splinter Review
Bug 5339 rebased and rolled into one commit. Capstone for Elliott (123.52 KB, patch)
2012-06-10 15:56 UTC, MJ Ray (software.coop)
Details | Diff | Splinter Review
Bug 5339: Invoices management improvement (105.01 KB, patch)
2012-06-25 09:00 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 5339: Invoices management improvement (105.95 KB, patch)
2012-07-18 13:00 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 5339: Invoices management improvement (105.99 KB, patch)
2012-07-23 11:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 5339: Invoices management improvement (106.45 KB, patch)
2012-08-10 14:02 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 5339: perltidy on added files (15.51 KB, patch)
2012-08-13 11:28 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 5339: Invoices management improvement (106.31 KB, patch)
2012-09-17 09:37 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 5339: perltidy on added files (15.51 KB, patch)
2012-09-17 09:38 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 5339: ergonomic changes (6.56 KB, patch)
2012-09-19 14:33 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 5339: Unit tests for invoices related subroutines (5.45 KB, patch)
2012-09-19 14:33 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 5339: Fix t/db_dependant/Acquisition.t (876 bytes, patch)
2012-09-20 07:14 UTC, Julian Maurice
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Paul Poulain 2010-11-02 02:39:18 UTC
A new feature will be added to enable closing of a parcel. Once a parcel is closed, no item can be added.
Comment 1 Paul Poulain 2011-11-05 09:56:39 UTC
invoice management : In Koha 3.2, the amount of the items in the parcel is deduced from the budget. We will add 3 fields and a form to let the librarian add additional fees to the parcel, a description of the added fees and a budget. Those fees will be deduced from a selected budget. May overlap with http://wiki.koha-community.org/wiki/Capturing_information_on_handling,_postage,_service_charge_etc.
Comment 2 Paul Poulain 2011-11-05 10:56:39 UTC
If you want to participate to the upstreaming:
* assign the bug to yourself
* retrieve the patch(es) on git.biblibre.com
* the branch name can be found using
git branch -a|grep NNNN
Usually, it should be 
  biblibre/customer/ft/MTNNNN
For example:
   remotes/origin/stetienne/ft/MT6522

This patch is in branch number 6533
Comment 3 Julian Maurice 2011-11-06 06:30:39 UTC
This feature is more than just the possibility to close a parcel.
It provides a new submodule for searching and managing invoices.
It will be possible to set a shipment cost for an invoice, and affect it to a specific fund. And also a billing date.
Comment 4 Julian Maurice 2012-01-06 15:55:50 UTC Comment hidden (obsolete)
Comment 5 Julian Maurice 2012-01-06 16:03:09 UTC Comment hidden (obsolete)
Comment 6 Julian Maurice 2012-01-13 13:20:56 UTC
*** Bug 5344 has been marked as a duplicate of this bug. ***
Comment 7 Katrin Fischer 2012-01-22 20:15:29 UTC
Hi Julian, 

I looked at the code and did some tests. These are my findings:

1) I would be nice if you could add documentation for the new tables in kohastructure.sql. I wonder if it would be better to name the table aqinvoices. This would make it more consistent with the other table names.

2) Remove MySQLisms
+  `closedate` date default NULL,
Coding guidelines: don't use ` in table or column names (this is a mySQLism) 

3) Only a note: Patch moves invoice information from aqorders into the new table invoices. I think testing this with a larger database would be good - my test database is only very small and has only a few invoices.

4) Perhaps it would be good to check that all invoicenumbers have been moved correctly before dropping the column. The datatype in invoices is smaller than before: mediumtext vs. varchar(80).
+    $dbh->do("
+        ALTER TABLE aqorders
+        DROP COLUMN booksellerinvoicenumber
+    ");

5) 1st patch includes kohaversion, but file should not be in the patch.

6) Patch adds new subs to C4. It would be nice if you could provide unit tests for those.
+        &GetInvoices
+        &GetInvoice
+        &GetInvoiceDetails
+        &AddInvoice
+        &ModInvoice
+        &CloseInvoice
+        &ReopenInvoice

7) From looking at the code I wondered about date formatting, but couldn't check the display. Perhaps you want to make use of the new TT filter for dates.
http://wiki.koha-community.org/wiki/Coding_Guidelines#Displaying_dates 

7) Both patches don't apply any longer. 
Can you please fix them and resubmit? Thx!
Comment 8 Julian Maurice 2012-02-03 14:16:51 UTC Comment hidden (obsolete)
Comment 9 Julian Maurice 2012-02-03 14:21:52 UTC Comment hidden (obsolete)
Comment 10 Katrin Fischer 2012-02-03 14:29:52 UTC
Hi Julian, 

thx for fixing the patches. I am still thinking about the table name - one part of acquisitions that I like is that it's easy to spot the tables by their prefix 'aq'. So I wonder if we should stick with this pattern?
Comment 11 Elliott Davis 2012-02-10 15:19:41 UTC
*** Bug 6850 has been marked as a duplicate of this bug. ***
Comment 12 Julian Maurice 2012-02-16 14:58:13 UTC Comment hidden (obsolete)
Comment 13 Julian Maurice 2012-02-16 14:59:31 UTC Comment hidden (obsolete)
Comment 14 Julian Maurice 2012-02-16 15:01:43 UTC
Oops sorry, I resubmitted the same patches. Correct patches will follow.
Comment 15 Julian Maurice 2012-02-16 15:02:57 UTC Comment hidden (obsolete)
Comment 16 Julian Maurice 2012-02-16 15:03:27 UTC Comment hidden (obsolete)
Comment 17 Julian Maurice 2012-02-16 15:33:32 UTC
Hmm.. Paul is telling me that table name should be singular... I will resubmit patches
Comment 18 Julian Maurice 2012-02-16 15:49:23 UTC
It seems there is no final decision about singular or plural table names, so leaving it as is until a decision is made.
Comment 19 Paul Poulain 2012-02-17 14:39:11 UTC
The script devel/misc/testbugzillapatches.pl has detected that this bug does not apply anymore, so i'm marking this bug as "does not apply".

The script is supposed to handle conflicts related to updatedatabase, but no other cases of merge conflict. So the conflict may be trivial to fix.

If it happens that the patch attached does apply cleanly, please send me an email, there's something wrong in the script. It's possible, as it's a recent script.
If the patch applies does not apply as I think, please rebase and resubmit, thanks
Comment 20 Julian Maurice 2012-02-24 17:08:41 UTC Comment hidden (obsolete)
Comment 21 Julian Maurice 2012-02-24 17:09:06 UTC Comment hidden (obsolete)
Comment 22 Katrin Fischer 2012-02-26 06:41:25 UTC
Hi Julian, 

I started testing and thought of some other places where we are using the invoicenumber:

1) /cgi-bin/koha/acqui/spent.pl?fund=1&fund_code=BOOK
This page is broken now - the sql has to be changed to adapt to the new database structure.

2) /cgi-bin/koha/acqui/histsearch.pl
[Sun Feb 26 07:18:18 2012] [error] [client 127.0.0.1] [Sun Feb 26 07:18:18 2012] histsearch.pl: DBD::mysql::st fetchrow_hashref failed: fetch() without execute() at /home/katrin/kohaclone/C4/Acquisition.pm line 1684., referer: http://localhost:8080/cgi-bin/koha/acqui/histsearch.pl

3) /cgi-bin/koha/catalogue/moredetail.pl?biblionumber=9820
Can't use an undefined value as an ARRAY reference at /home/katrin/kohaclone/acqui/parcel.pl line 154.


For 3) perhaps you can take a look at the patches for bug 7235 - I think there will be a small conflict between both bugs.
Comment 23 Julian Maurice 2012-03-01 11:17:08 UTC Comment hidden (obsolete)
Comment 24 Julian Maurice 2012-03-01 11:20:03 UTC Comment hidden (obsolete)
Comment 25 Elliott Davis 2012-03-15 19:58:34 UTC
I was attempting to apply this earlier and it appears that it is out of date again(specifically updatedatabase.pl and parcel.tt).
Comment 26 Koha Team University Lyon 3 2012-03-19 15:01:09 UTC
Something went wrong !
Applying: Bug 5339: Invoices management improvement
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging installer/data/mysql/kohastructure.sql
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 5339: Invoices management improvement
Comment 27 Paul Poulain 2012-03-19 15:18:33 UTC
(In reply to comment #26)
> Something went wrong !
> Applying: Bug 5339: Invoices management improvement
> Using index info to reconstruct a base tree...
> Falling back to patching base and 3-way merge...
> Auto-merging installer/data/mysql/kohastructure.sql
> 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 5339: Invoices management improvement

When a patch has a merge conflict on update database, and is tested with the sandbox system, it must not be set to "does not apply".
Just report the problem on the entry, saying you've tried to test it with sandboxes, and it is not possible.
The patch will have to be tested manually

So... switching back to "needs signoff"
Comment 28 Elliott Davis 2012-03-19 17:09:06 UTC
From http://129.114.128.202:8080/cgi-bin/koha/acqui/parcel.pl when you click on receive you get a hash reference in the URL for invoice so when you click save it fails giving the error message:

Can't use an undefined value as an ARRAY reference at /home/koha/kohaclone/acqui/parcel.pl line 154.

with the URL http://129.114.128.202:8080/cgi-bin/koha/acqui/parcel.pl?invoiceid=

I can provide screenshots if needed.  Marking failed for now.
Comment 29 Julian Maurice 2012-03-20 08:45:27 UTC Comment hidden (obsolete)
Comment 30 Julian Maurice 2012-03-20 08:45:57 UTC Comment hidden (obsolete)
Comment 31 Julian Maurice 2012-03-20 09:04:41 UTC
I was not able to reproduce the problem described in comment 28 and can receive items without problems. Did you execute the updatedatabase?
Comment 32 Koha Team University Lyon 3 2012-03-20 12:33:40 UTC
I have the same error than in comment28 : Software error:

Can't use an undefined value as an ARRAY reference at /home/koha/src/acqui/parcel.pl line 154.

For help, please send mail to the webmaster (webmaster@test4-sandbox), giving this error message and the time and date of the error.
Comment 33 Julian Maurice 2012-03-20 13:22:46 UTC Comment hidden (obsolete)
Comment 34 Julian Maurice 2012-03-20 13:23:20 UTC Comment hidden (obsolete)
Comment 35 Julian Maurice 2012-03-20 13:26:24 UTC
The last patch fix a bug in pagination (parcel.pl) and show only supplier invoices in parcels.pl (all invoices were shown).

If you are testing on a Sandbox, be sure to replace the database by another (option 1 or 2) or make it empty (option 0) and then play the updatedatabase process.
Comment 36 Elliott Davis 2012-03-20 15:57:26 UTC
Tested Receiving orders from inported records and new records.  Billing date/Shipping Date modifications work.  Shipment price works.
Comment 37 Elliott Davis 2012-03-22 18:41:21 UTC
Due to the recent changes in master this patch is now out of date.  The files that I am showing in conflict are:

catalogue/moredetail.pl
koha-tmpl/intranet-tmpl/prog/en/modules/acqui/parcel.tt
koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/moredetail.tt
Comment 38 Julian Maurice 2012-03-23 09:14:44 UTC Comment hidden (obsolete)
Comment 39 Julian Maurice 2012-03-23 09:15:25 UTC Comment hidden (obsolete)
Comment 40 Katrin Fischer 2012-03-25 22:04:28 UTC
There is a conflict in parcel.tt that I am not sure how to solve. Julian, can you please rebase again?
Comment 41 Julian Maurice 2012-03-28 13:09:52 UTC Comment hidden (obsolete)
Comment 42 Julian Maurice 2012-03-28 13:10:22 UTC Comment hidden (obsolete)
Comment 43 Elliott Davis 2012-03-30 19:21:03 UTC
Rebased to apply again...Signed off
Comment 44 Elliott Davis 2012-04-03 17:30:56 UTC
The two attached patches are the same.  I'm sure this patch will need rebased again to go with the current master.  Also the sql in the update database batch should have quoted fields.  

I think it would be helpful of both of these patches got merged since they are interconnected.
Comment 45 Julian Maurice 2012-04-04 07:17:34 UTC Comment hidden (obsolete)
Comment 46 Julian Maurice 2012-04-04 07:24:24 UTC
Elliott, if you're talking about «`» quotes, this is a mysqlism and it's forbidden in new patches (see http://wiki.koha-community.org/wiki/Coding_Guidelines#Database (SQL6))
Comment 47 Elliott Davis 2012-04-04 20:36:48 UTC
on the parcel.pl page the add notes link is broken.  It is passing a hash as the invoice.  Also, this may be unrelated but when you save that note you get a software error "Biblionumber is required at /home/koha/kohaclone/C4/Acquisition.pm line 1000."

Also, this is out of date again.  I'm terribly sorry Julian.  I try my best to test it as soon as you rebase but apparently I'm not fast enough.
Comment 48 Julian Maurice 2012-04-06 12:09:36 UTC Comment hidden (obsolete)
Comment 49 Julian Maurice 2012-04-06 12:10:02 UTC Comment hidden (obsolete)
Comment 50 Julian Maurice 2012-04-06 12:13:18 UTC
'Add note' link fixed by the second patch. With that fix I don't have "Biblionumber is required" error.

P.S.: Elliot, don't feel sorry about this. I'm glad to have someone testing my patches ;-)
Comment 51 Elliott Davis 2012-05-01 13:29:46 UTC Comment hidden (obsolete)
Comment 52 Julian Maurice 2012-06-04 14:08:23 UTC
Elliott, next time you'll submit changes like this, could you just submit a follow-up patch instead of the whole patch + your changes? It's easier to see what you changed, and then easier to test.
Comment 53 Elliott Davis 2012-06-04 14:20:10 UTC
Yeah I can Julian.  Sorry about messing that up.

(In reply to comment #52)
> Elliott, next time you'll submit changes like this, could you just submit a
> follow-up patch instead of the whole patch + your changes? It's easier to
> see what you changed, and then easier to test.
Comment 54 MJ Ray (software.coop) 2012-06-10 15:55:26 UTC Comment hidden (obsolete)
Comment 55 MJ Ray (software.coop) 2012-06-10 15:56:51 UTC Comment hidden (obsolete)
Comment 56 Mason James 2012-06-25 05:45:53 UTC
QA comments

1) parcel.tt and parcels.tt appear to have existing code globally reformatted with pretty-formatting/auto-indentation

2) patch does not apply to current master '19d30206df473ff6d801f15063f6837225b004bf' with some big conflicts 
(nor does it apply to master at the date of the last rebased patch? '2012-06-10')

please resubmit patch without applying global pretty-formatting to existing template code

failing QA on issue 1

otherwise… it looks good. both 'prove xt' and 'perlcritic' pass ok
Comment 57 Julian Maurice 2012-06-25 09:00:57 UTC Comment hidden (obsolete)
Comment 58 Katrin Fischer 2012-07-10 06:34:17 UTC
Hi Julian, I woud like to test this next. Elliot's patch doesn't apply for me, so I obsoleted it for now to make this bug a little less confusing. When applying your patch, I have a conflict in koha-tmpl/intranet-tmpl/prog/en/modules/acqui/parcels.tt that I am not sure how to resolve - could you take a look?

Hi Elliott, can you provide a new patch for your functionality as follow up?
Comment 59 Katrin Fischer 2012-07-10 06:35:03 UTC
Comment on attachment 10137 [details] [review]
Bug 5339 rebased and rolled into one commit. Capstone for Elliott

Obsoleting to stop confusion as there is a newer patch with Julian's work on this.
Comment 60 Katrin Fischer 2012-07-10 20:57:00 UTC
I think having a separate table for invoices is a great idea and I am looking forward to see this implemented in Koha. I fixed some conflicts in the patch, but I found some problems and have some questions about the change of workflow:

1) On parcels.pl the date received is not showing in the datatables table. I suspect this is because the datereceived from aqorders is not moved into the aqinvoices' shipmentdate column. Shipmentdate is empty after the database update.

2) What is the billing date for? How is it set? 

The filters on the new invoices page offer filtering by billing date, but I don't understand how this works. Can you explain the different dates in the aqinvoices table within the workflow? (I read the documentation on the table, but still wondering)

invoiceid int(11) NOT NULL AUTO_INCREMENT, -- ID of the invoice, primary 
invoicenumber mediumtext NOT NULL,    -- Name of invoice
booksellerid int(11) NOT NULL,    -- foreign key to aqbooksellers
shipmentdate date default NULL,   -- date of shipment
billingdate date default NULL,    -- date of billing
closedate date default NULL,  -- invoice close date, NULL means the invoice is open
shipmentcost decimal(28,6) default NULL,  -- shipment cost
shipmentcost_budgetid int(11) default NULL,   -- foreign key to aqbudgets, link the shipment cost to a budget
PRIMARY KEY (invoiceid),

3) Old datepickers will have to be replaced by the new YUI datepickers.

4) If you open a invoice from the new invoices page, there are fields for shipment cost and budget. Those are not present when I create a new invoice. Is this feature implemented with this bug? It would be saving time and an additional step to have those fields available at the time the invoice is created on parcels.tt.

5) It would be nice if the invoices table was sortable. There are a lot of filter options, but I can imagine people will want to print a list of open invoices for a vendor. 

6) When viewing an invoice on the new page, there is no way to open the title. I think the title should maybe be linked with the catalog record? There is also a link to the "receipt page". I am not sure about the terms receipt/invoice/shipment and where we use what. This view seems to double up quite a bit of information. 

Failing QA because of 1 and 3 - sorry! :(
Comment 61 Paul Poulain 2012-07-11 08:00:22 UTC
(In reply to comment #60)
> Failing QA because of 1 and 3 - sorry! :(

Katrin, you said nothing to my mail on koha-devel saying "I think Katrin should be in the QA team, because she made very good QA comments".
This comment confirm this fact ;-)
Comment 62 Katrin Fischer 2012-07-11 08:19:57 UTC
Hi Paul, thx for the compliment, but I think I am in a good place right now as a bug wrangler. My time is limited and I still have to learn lots about Perl and programming. I have put my name on the list as a bug wrangler for 3.12 (http://wiki.koha-community.org/wiki/Roles_for_3.12) and I hope to get reelected :)
Comment 63 Julian Maurice 2012-07-18 13:00:14 UTC Comment hidden (obsolete)
Comment 64 Kyle M Hall 2012-07-23 11:38:03 UTC Comment hidden (obsolete)
Comment 65 Katrin Fischer 2012-07-23 11:54:38 UTC
Hi Kyle,

could you add some information about the tests you did?
Comment 66 Kyle M Hall 2012-07-30 15:49:29 UTC
My memory is a bit hazy, but I do recall testing the searching, and the ability to modify the shipping and billing dates.

Kyle

(In reply to comment #65)
> Hi Kyle,
> 
> could you add some information about the tests you did?
Comment 67 Julian Maurice 2012-08-10 14:02:28 UTC Comment hidden (obsolete)
Comment 68 Julian Maurice 2012-08-10 14:02:46 UTC
Patch rebased on master
Comment 69 Jonathan Druart 2012-08-13 07:46:26 UTC
QA Comment:
It would be better that someone not from BibLibre make QA on this patch.

To pass QA, some points would be great:
- mandatory: perltidy on all new files
- minor: you change prototype for ModReceiveOrder but you don't modify files into the test directory
- some unit tests for your invoices routines would be perfect. See Bug 5327, using DBD::Mock it is very simple.
Comment 70 Julian Maurice 2012-08-13 11:28:20 UTC Comment hidden (obsolete)
Comment 71 Julian Maurice 2012-08-13 11:33:58 UTC
I made a separate patch to see that nothing changes except indentation and spaces. Both patches can be squashed before going into master.
Comment 72 Julian Maurice 2012-08-13 12:17:18 UTC
(In reply to comment #69)
> QA Comment:
> - minor: you change prototype for ModReceiveOrder but you don't modify files
> into the test directory

It only adds an optional parameter in last position, so ModReceiveOrder should
still behave as expected.
However, ModReceiveOrder is used GetParcels.pm and GetParcel.pm
(t/db_dependant/Acquisition/). Is this used somewhere?


> - some unit tests for your invoices routines would be perfect. See Bug 5327,
> using DBD::Mock it is very simple.

I'm not writing tests with DBD::Mock since I don't know how to use it (see http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=5327#c219)
Comment 73 Chris Cormack 2012-08-13 19:50:16 UTC
Hopefully my answer there will help you. It's not for testing sql of course, you need db_dependent tests fir that. But you can test everything else by faking the db
Comment 74 Mason James 2012-09-14 14:47:28 UTC
> > - some unit tests for your invoices routines would be perfect. See Bug 5327,
> > using DBD::Mock it is very simple.
> 
> I'm not writing tests with DBD::Mock since I don't know how to use it (see
> http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=5327#c219)


hmmm, we are we waiting on a unit-test before this can be QA'ed

Jonathon, would you do a Mock test for this, before i QA?
Comment 75 Jonathan Druart 2012-09-14 14:52:09 UTC
Kyle,
The patch is waiting for 9 months then I think it can pass qa without UT.
But a followup will be required.
Comment 76 Mason James 2012-09-16 12:13:44 UTC
(In reply to comment #75)
> Kyle,
> The patch is waiting for 9 months then I think it can pass qa without UT.
> But a followup will be required.

Jonathan, hmmm, i an OK with a follow-up patch for unit-test

Julian, sorry, i had some small merge conflicts against master (ce03893) :/
if you rebase against master again, i will QA this the same day

#       both modified:      C4/Acquisition.pm
#       both modified:      acqui/finishreceive.pl
#       both modified:      acqui/parcel.pl


changing status to 'patch does not apply' (sorry)
Comment 77 Julian Maurice 2012-09-17 09:37:23 UTC Comment hidden (obsolete)
Comment 78 Julian Maurice 2012-09-17 09:38:07 UTC Comment hidden (obsolete)
Comment 79 Mason James 2012-09-18 04:00:23 UTC
> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

patch looks good, passing QA…

$ koha-qa.pl -c 2

testing 2 commit(s) (applied to commit 9acabe4)
 * 76125f0 Bug 5339: perltidy on added files
      acqui/invoice.pl
      acqui/invoices.pl

 * cc99ecf Bug 5339: Invoices management improvement
      C4/Acquisition.pm
      C4/Budgets.pm
      acqui/addorderiso2709.pl
      acqui/finishreceive.pl
      acqui/invoice.pl
      acqui/invoices.pl
      acqui/orderreceive.pl
      acqui/parcel.pl
      acqui/parcels.pl
      acqui/spent.pl
      catalogue/moredetail.pl
      installer/data/mysql/kohastructure.sql
      installer/data/mysql/updatedatabase.pl
      koha-tmpl/intranet-tmpl/prog/en/includes/acquisitions-menu.inc
      koha-tmpl/intranet-tmpl/prog/en/modules/acqui/histsearch.tt
      koha-tmpl/intranet-tmpl/prog/en/modules/acqui/invoice.tt
      koha-tmpl/intranet-tmpl/prog/en/modules/acqui/invoices.tt
      koha-tmpl/intranet-tmpl/prog/en/modules/acqui/orderreceive.tt
      koha-tmpl/intranet-tmpl/prog/en/modules/acqui/parcel.tt
      koha-tmpl/intranet-tmpl/prog/en/modules/acqui/parcels.tt
      koha-tmpl/intranet-tmpl/prog/en/modules/acqui/spent.tt
      koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/moredetail.tt

* acqui/invoice.pl                                                         OK
* acqui/invoices.pl                                                        OK
* C4/Acquisition.pm                                                        OK
* C4/Budgets.pm                                                            OK
* acqui/addorderiso2709.pl                                                 OK
* acqui/finishreceive.pl                                                   OK
* acqui/orderreceive.pl                                                    OK
* acqui/parcel.pl                                                          OK
* acqui/parcels.pl                                                         OK
* acqui/spent.pl                                                           OK
* catalogue/moredetail.pl                                                  OK
* installer/data/mysql/updatedatabase.pl                                   OK
* koha-tmpl/intranet-tmpl/prog/en/includes/acquisitions-menu.inc           OK
* koha-tmpl/intranet-tmpl/prog/en/modules/acqui/histsearch.tt              OK
* koha-tmpl/intranet-tmpl/prog/en/modules/acqui/invoice.tt                 OK
* koha-tmpl/intranet-tmpl/prog/en/modules/acqui/invoices.tt                OK
* koha-tmpl/intranet-tmpl/prog/en/modules/acqui/orderreceive.tt            OK
* koha-tmpl/intranet-tmpl/prog/en/modules/acqui/parcel.tt                  OK
* koha-tmpl/intranet-tmpl/prog/en/modules/acqui/parcels.tt                 OK
* koha-tmpl/intranet-tmpl/prog/en/modules/acqui/spent.tt                   OK
* koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/moredetail.tt          OK
Comment 80 Mason James 2012-09-18 04:08:25 UTC
(In reply to comment #76)
> (In reply to comment #75)
> > Kyle,
> > The patch is waiting for 9 months then I think it can pass qa without UT.
> > But a followup will be required.
> 
> Jonathan, hmmm, i an OK with a follow-up patch for unit-test
> 

but

we still need a unit-test patch as a follow-up for this patch...
Comment 81 Paul Poulain 2012-09-19 09:19:27 UTC
Patch pushed to master

The patch has been pushed, but the Unit Tests are expected soon.
Another ergonomic follow-up would also be cool:
I think librarians will be confused by having a button "Finish Recieve" (at the bottom of the page) and another link (at the top of the page) to reach invoice information and "close it". I suggest that we have only one button "Finish Reciev" at the bottom, that reaches invoices.pl page.
On the "invoices.pl" page, it would also be better to be able to enter values and close the invoice in 1 step (2 required now : modify + click save THEN click "close" link just below. You can do like for basketgroup: just before the "save button", add a checkbox [] close invoice
Comment 82 Julian Maurice 2012-09-19 14:33:02 UTC
Created attachment 12352 [details] [review]
Bug 5339: ergonomic changes

- "Finish receive" on parcel.pl now redirect to acqui/invoice.pl,
  allowing the librarian to modify invoice informations (freight,
  billing date, ...) and close invoice.
- The librarian can now close (or reopen) the invoice and modify invoice
  informations in 1 operation (instead of 2)
Comment 83 Julian Maurice 2012-09-19 14:33:48 UTC
Created attachment 12353 [details] [review]
Bug 5339: Unit tests for invoices related subroutines

These tests use DBD::Mock to check if SQL queries are correctly built.
Actually, we only check bound parameters.
Comment 84 Julian Maurice 2012-09-19 14:35:02 UTC
Obsoleted the two previous patches as they are in master.
Switching status to needs signoff.
Comment 85 Chris Cormack 2012-09-19 22:15:02 UTC
We also need to fix the test in 
t/db_dependent/Acquisition.t

not ok 9 - This order has a booksellerinvoicenumber field
#   Failed test 'This order has a booksellerinvoicenumber field'
#   at t/db_dependent/Acquisition.t line 82.
Comment 86 Julian Maurice 2012-09-20 07:14:44 UTC
Created attachment 12369 [details] [review]
Bug 5339: Fix t/db_dependant/Acquisition.t

replaced booksellerinvoicenumber by invoiceid
Comment 87 Paul Poulain 2012-09-20 09:06:04 UTC
small Follow-ups tested & pushed in one go
Comment 88 Julian Maurice 2012-10-11 08:22:44 UTC
Ok in master.
Comment 89 Katrin Fischer 2013-04-14 08:09:24 UTC
*** Bug 8424 has been marked as a duplicate of this bug. ***