Bug 5336 - Acq history search
Summary: Acq history search
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low new feature (vote)
Deadline: 2011-05-01
Assignee: Jonathan Druart
QA Contact: Katrin Fischer
URL: http://wiki.koha-community.org/wiki/H...
Keywords:
Depends on: 7169 7583
Blocks: 5355 7163 7294 8230 8667 11169 11170
  Show dependency treegraph
 
Reported: 2010-11-02 02:33 UTC by Paul Poulain
Modified: 2014-05-26 21:04 UTC (History)
10 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 5336:Order search (search and UI enhancements) (34.22 KB, patch)
2012-08-14 13:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5336:Order search (search and UI enhancements) (34.20 KB, patch)
2012-08-14 13:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5336: Replace IFNULL with COALESCE (1.22 KB, patch)
2012-08-23 14:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5336:Order search (search and UI enhancements) (33.23 KB, patch)
2012-12-13 09:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5336:Order search (search and UI enhancements) (33.21 KB, patch)
2012-12-18 13:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5336:Order search (search and UI enhancements) (25.68 KB, patch)
2012-12-20 15:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5336:Order search (search and UI enhancements) (25.40 KB, patch)
2013-03-11 08:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5336:Order search (search and UI enhancements) (23.17 KB, patch)
2013-04-23 13:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5336:Order search (search and UI enhancements) (23.30 KB, patch)
2013-04-23 13:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5336: Late orders search: don't display deleted order. (987 bytes, patch)
2013-04-23 13:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5336: Add unit tests for 3 acq routines (3.78 KB, patch)
2013-04-23 14:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5336:Order search (search and UI enhancements) (23.96 KB, patch)
2013-04-24 12:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5336: Late orders search: don't display deleted order. (987 bytes, patch)
2013-04-24 12:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5336: Add unit tests for 3 acq routines (3.78 KB, patch)
2013-04-24 12:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5336: Merge parent ordernumber and ordernumber columns (2.08 KB, patch)
2013-05-02 08:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5336: Add unit tests for 3 acq routines (4.08 KB, patch)
2013-05-17 09:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5336:Order search (search and UI enhancements) (24.03 KB, patch)
2013-05-27 08:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5336: Late orders search: don't display deleted order. (987 bytes, patch)
2013-05-27 08:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5336: Merge parent ordernumber and ordernumber columns (2.08 KB, patch)
2013-05-27 08:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5336: Add unit tests for 3 acq routines (4.08 KB, patch)
2013-05-27 08:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5336: Add unit tests for 3 acq routines (4.15 KB, patch)
2013-05-30 08:11 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 5336:Order search (search and UI enhancements) (24.09 KB, patch)
2013-05-30 08:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5336: Late orders search: don't display deleted order. (1.02 KB, patch)
2013-05-30 08:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5336: Merge parent ordernumber and ordernumber columns Signed-off-by: Cedric Vita <cedric.vita@dracenie.com> (2.14 KB, patch)
2013-05-30 08:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5336: Add unit tests for 3 acq routines (4.14 KB, patch)
2013-05-30 08:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5336:Order search (search and UI enhancements) (24.03 KB, patch)
2013-07-01 14:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5336: Late orders search: don't display deleted order. (1.02 KB, patch)
2013-07-01 14:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5336: Merge parent ordernumber and ordernumber columns Signed-off-by: Cedric Vita <cedric.vita@dracenie.com> (2.06 KB, patch)
2013-07-01 14:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5336: Add unit tests for 3 acq routines (4.14 KB, patch)
2013-07-01 14:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5336: QA followup (2.30 KB, patch)
2013-07-01 14:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5336: Change numbers with understandable codes for orderstatus (11.83 KB, patch)
2013-07-11 08:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5336:Order search (search and UI enhancements) (24.00 KB, patch)
2013-09-09 08:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5336: Late orders search: don't display deleted order. (1.02 KB, patch)
2013-09-09 08:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5336: Merge parent ordernumber and ordernumber columns Signed-off-by: Cedric Vita <cedric.vita@dracenie.com> (2.06 KB, patch)
2013-09-09 08:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5336: Add unit tests for 3 acq routines (3.28 KB, patch)
2013-09-09 08:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5336: QA followup (2.30 KB, patch)
2013-09-09 08:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5336: Change numbers with understandable codes for orderstatus (11.80 KB, patch)
2013-09-09 08:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5336:Order search (search and UI enhancements) (24.05 KB, patch)
2013-10-01 03:32 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 5336: Late orders search: don't display deleted order. (1.09 KB, patch)
2013-10-01 03:32 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 5336: Merge parent ordernumber and ordernumber columns Signed-off-by: Cedric Vita <cedric.vita@dracenie.com> (2.11 KB, patch)
2013-10-01 03:32 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 5336: Add unit tests for 3 acq routines (3.35 KB, patch)
2013-10-01 03:32 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 5336: QA followup (2.34 KB, patch)
2013-10-01 03:32 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 5336: Change numbers with understandable codes for orderstatus (11.85 KB, patch)
2013-10-01 03:32 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
my orders... (64.84 KB, image/png)
2013-10-01 06:05 UTC, Katrin Fischer
Details
... and no pending orders (56.27 KB, image/png)
2013-10-01 06:05 UTC, Katrin Fischer
Details
Bug 5336: FIX: partial status was not correctly set (1.70 KB, patch)
2013-10-01 12:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5336: FIX: partial status was not correctly set (1.78 KB, patch)
2013-10-02 08:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5336: Multiple QA fixes (6.41 KB, patch)
2013-10-02 08:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5336: Multiple QA fixes (6.52 KB, patch)
2013-10-02 08:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PASSED QA] Bug 5336:Order search (search and UI enhancements) (24.20 KB, patch)
2013-10-03 11:03 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 5336: Late orders search: don't display deleted order. (1.14 KB, patch)
2013-10-03 11:03 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 5336: Merge parent ordernumber and ordernumber columns Signed-off-by: Cedric Vita <cedric.vita@dracenie.com> (2.17 KB, patch)
2013-10-03 11:03 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 5336: Add unit tests for 3 acq routines (3.40 KB, patch)
2013-10-03 11:04 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 5336: QA followup (2.40 KB, patch)
2013-10-03 11:04 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 5336: Change numbers with understandable codes for orderstatus (11.92 KB, patch)
2013-10-03 11:04 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 5336: FIX: partial status was not correctly set (1.85 KB, patch)
2013-10-03 11:04 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 5336: Multiple QA fixes (9.63 KB, patch)
2013-10-03 11:04 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 Paul Poulain 2010-11-02 02:33:38 UTC
New options will be added to the history search (isbn, library, budget, invoicenumber)
Comment 1 Paul Poulain 2011-11-05 10:57:23 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 5840
Comment 2 Jonathan Druart 2012-08-14 13:23:05 UTC Comment hidden (obsolete)
Comment 3 Katrin Fischer 2012-08-14 13:30:20 UTC
Comment on attachment 11603 [details] [review]
Bug 5336:Order search (search and UI enhancements)

I was thinking, maybe ordered would be better then requested? Not sure here, I think we use requested for orders and suggestions, but ordered in acq.
Comment 4 Jonathan Druart 2012-08-14 13:49:03 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2012-08-23 14:40:40 UTC Comment hidden (obsolete)
Comment 6 Mathieu Saby 2012-12-12 16:39:03 UTC
Patch does not apply.
Could you rebase it please?
M. Saby


Applying: Bug 5336:Order search (search and UI enhancements)
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/histsearch.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/acqui/histsearch.tt
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/kohastructure.sql
CONFLICT (content): Merge conflict in installer/data/mysql/kohastructure.sql
Auto-merging installer/data/mysql/de-DE/optional/auth_val.sql
CONFLICT (content): Merge conflict in installer/data/mysql/de-DE/optional/auth_val.sql
Auto-merging acqui/histsearch.pl
CONFLICT (content): Merge conflict in acqui/histsearch.pl
Auto-merging C4/Acquisition.pm
CONFLICT (content): Merge conflict in C4/Acquisition.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 5336:Order search (search and UI enhancements)
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".
Comment 7 Jonathan Druart 2012-12-13 09:58:29 UTC Comment hidden (obsolete)
Comment 8 Mathieu Saby 2012-12-13 13:04:19 UTC
Still a conflit :

CONFLICT (content): Merge conflict in installer/data/mysql/en/optional/auth_val.sql
Auto-merging installer/data/mysql/de-DE/optional/auth_val.sql
CONFLICT (content): Merge conflict in installer/data/mysql/de-DE/optional/auth_val.sql
Failed to merge in the changes.


M. Saby
Comment 9 Jonathan Druart 2012-12-13 13:36:28 UTC
Humm, it works here:

$ git remote update
$ git checkout -b BZ5336 origin/master
$ git bz apply 5336
Bug 5336 - Acq history search

Bug 5336:Order search (search and UI enhancements)
Apply? [yn] y

Applying: Bug 5336:Order search (search and UI enhancements)
Comment 10 Mathieu Saby 2012-12-13 13:38:30 UTC
Maybe my install is corrupted. I will test again later...
M. Saby
Comment 11 Mathieu Saby 2012-12-18 13:07:27 UTC
Same error in sandbox nĀ°3 : 

CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Failed to merge in the changes.
Patch failed at 0001 Bug 5336:Order search (search and UI enhancements)
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".
Bug 5336 - Acq history search


M. Saby
Comment 12 Mathieu Saby 2012-12-18 13:08:47 UTC
Sorry, not the SAME error, but patch doesn't apply in sandbox.

The complete message

Applying: Bug 5336:Order search (search and UI enhancements)
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 5336:Order search (search and UI enhancements)
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".
Bug 5336 - Acq history search


M. Saby
Comment 13 Jonathan Druart 2012-12-18 13:31:39 UTC Comment hidden (obsolete)
Comment 14 Mathieu Saby 2012-12-19 10:54:03 UTC
- In search results, the name of the budget and the branchcode for each line does not show up
=> I cannot check if the search by budget/branchcode is working well. Could you add these informations ?


I think I found a problem concerning Order status :
- create 1 basket with 2 order lines (one book ordered in 1 items, on a fund A, the other in 2 items, on a fund B)
- search for the 2 titles => they should have status "New", 
=> OK
- search for the orders with status "New" => the list should contain the 2 titles
=> OK

- close the basket, create a basketgroup with it
- search for the 2 titles => they should have status "Ordered"
=> Error : The 2 lines still have status "New"

Before testing with 2 lines, I tested with 1 line in my basket, and it seemed to be good...


M. Saby
Rennes 2 University
Comment 15 Katrin Fischer 2012-12-19 21:11:16 UTC
Hi Jonathan and Mathieu, 

I have a question - why introduce a new authorized value if the values are pretty much hardcoded? Or am I mistaken? If they were from the templates instead, the status would be translatable using po files.
Comment 16 Jonathan Druart 2012-12-20 15:14:45 UTC
(In reply to comment #14)
> - In search results, the name of the budget and the branchcode for each line
> does not show up
> => I cannot check if the search by budget/branchcode is working well. Could
> you add these informations ?

Done

> I think I found a problem concerning Order status 

Yes! Good catch!

(In reply to comment #15)
> I have a question - why introduce a new authorized value if the values are
> pretty much hardcoded? Or am I mistaken? If they were from the templates
> instead, the status would be translatable using po files.

It is an very old development. I think the goal was to be more flexible but I don't think it is really useful. The following patch hardcodes these values in the template.
Comment 17 Jonathan Druart 2012-12-20 15:15:11 UTC Comment hidden (obsolete)
Comment 18 Jonathan Druart 2012-12-20 15:17:16 UTC
REMEMBER: You must have the field aqorders.branchcode in your DB (created by  Bug 7294).
Comment 19 Mathieu Saby 2012-12-20 15:30:26 UTC
I did not realized that for bug 7294. That means I cannot test properly 5336 in a sandbox, and I need to apply manualy 7294 before 5336 ?

M. Saby
Comment 20 Jonathan Druart 2012-12-20 15:48:16 UTC
Unfortunately yes.
The search by branches needed the aqorders.branchcode. Now I display the branch name in all cases and the join is made on this field. If it does not exist, mysql will raise an error.
Comment 21 Mathieu Saby 2013-01-07 09:16:17 UTC
So I will test this patch again when BZ 7294 is pushed (current status is "Failed QA"). It is "cleaner" for me.

M. Saby
Comment 22 Mathieu Saby 2013-02-06 13:55:09 UTC
BZ 7294 has been signed off and should be soon in master. So I'm going to test 5336 again with  BZ 7294 applied first.

M. Saby
Comment 23 Mathieu Saby 2013-03-09 18:50:10 UTC
I applied the 2 patchs, but bz5336 does not apply:

CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/acqui/histsearch.tt
CONFLICT (content): Merge conflict in acqui/histsearch.pl

I reverted bz7294, and tried to apply only 5336, but I got the same errors.

Could you rebase it please?


M. Saby
Comment 24 Jonathan Druart 2013-03-11 08:26:54 UTC Comment hidden (obsolete)
Comment 25 Mathieu Saby 2013-03-11 10:27:33 UTC
By the way, applying 7294 does not update the database.
Pbm in updatedatabase.pl (3.09.XXX should be 3.11.XXX)

M. Saby
Comment 26 cedric.vita 2013-03-18 13:30:12 UTC
Something went wrong !
Applying: Bug 5336:Order search (search and UI enhancements)
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 5336:Order search (search and UI enhancements)
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".
Bug 5336 - Acq history search

Bug 5336:Order search (search and UI enhancements)
Apply? [yn]
Patch left in /tmp/Bug-5336Order-search-search-and-UI-enhancements-af0iuw.patch
Comment 27 Jonathan Druart 2013-03-18 14:17:48 UTC
This patch should be apply on top of bug 7294
Comment 28 Jonathan Druart 2013-04-23 13:06:35 UTC Comment hidden (obsolete)
Comment 29 Jonathan Druart 2013-04-23 13:08:39 UTC
This last patch doesn't depend on bug 7294.
Comment 30 Jonathan Druart 2013-04-23 13:10:50 UTC Comment hidden (obsolete)
Comment 31 Jonathan Druart 2013-04-23 13:31:46 UTC Comment hidden (obsolete)
Comment 32 Jonathan Druart 2013-04-23 14:51:25 UTC Comment hidden (obsolete)
Comment 33 Jonathan Druart 2013-04-24 12:57:38 UTC Comment hidden (obsolete)
Comment 34 Jonathan Druart 2013-04-24 12:57:48 UTC Comment hidden (obsolete)
Comment 35 Jonathan Druart 2013-04-24 12:57:51 UTC Comment hidden (obsolete)
Comment 36 Mathieu Saby 2013-05-01 11:20:41 UTC
My first tests are good, but in search results, you add an column for ordernumber in the html table.
Ordernumber is already present in the table on 1st column.
So I suppose you wanted to display funds, and not ordernumber ?

M. Saby
Comment 37 Jonathan Druart 2013-05-02 08:40:26 UTC
(In reply to comment #36)
> My first tests are good, but in search results, you add an column for
> ordernumber in the html table.
> Ordernumber is already present in the table on 1st column.
> So I suppose you wanted to display funds, and not ordernumber ?
> 
> M. Saby

Mathieu,
In fact the first column is the parent order number, not the order number.
I agree with you, it is not clear.
The fund column is already added by the first patch.
A patch is coming...
Comment 38 Jonathan Druart 2013-05-02 08:40:37 UTC Comment hidden (obsolete)
Comment 39 Jonathan Druart 2013-05-17 09:53:11 UTC Comment hidden (obsolete)
Comment 40 Mathieu Saby 2013-05-25 15:27:52 UTC
Sorry, patch does not apply :

CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/acqui/histsearch.tt
CONFLICT (content): Merge conflict in C4/Acquisition.pm

M. Saby
Comment 41 Jonathan Druart 2013-05-27 08:39:58 UTC Comment hidden (obsolete)
Comment 42 Jonathan Druart 2013-05-27 08:40:03 UTC Comment hidden (obsolete)
Comment 43 Jonathan Druart 2013-05-27 08:40:07 UTC Comment hidden (obsolete)
Comment 44 Jonathan Druart 2013-05-27 08:40:11 UTC Comment hidden (obsolete)
Comment 45 Paul Poulain 2013-05-30 08:11:19 UTC
Patch tested with a sandbox, by Cedric Vita <cedric.vita@dracenie.com>
Comment 46 Paul Poulain 2013-05-30 08:11:42 UTC Comment hidden (obsolete)
Comment 47 Jonathan Druart 2013-05-30 08:46:08 UTC Comment hidden (obsolete)
Comment 48 Jonathan Druart 2013-05-30 08:46:14 UTC Comment hidden (obsolete)
Comment 49 Jonathan Druart 2013-05-30 08:46:18 UTC Comment hidden (obsolete)
Comment 50 Jonathan Druart 2013-05-30 08:46:22 UTC Comment hidden (obsolete)
Comment 51 Jonathan Druart 2013-05-30 08:47:06 UTC
Last 4 patches are signoff patch for CĆ©dric Vita.

Thanks CĆ©dric!
Comment 52 Katrin Fischer 2013-06-30 22:58:17 UTC
Hi Jonathan,

I really like the idea of this. Starting with a code review and running the automated tests on the codes I found a few things:

1) The updatedatabase states:
+    print "Upgrade to $DBversion done (Add category ORDRSTATUS values in authorised_values table)\n";
But it does not actually add an authorised value? And it looks like we don't need one and can have the values in the template, so it might just be enough to correct the line. Please also add the bug number :)

2) Hm not sure about this line, does it maybe belong to 5394?
order.internalnotes ) %][% order.internalnotes %]<br />[% END %][% order.isbn %]</td>

3) Capitalization:
<legend>Search Orders</legend>
+            <label for="orderstatus">Order Status: </label>

4) Patches fail the QA script:
 FAIL	t/db_dependent/Acquisition/close_reopen_basket.t
   OK	  pod
   OK	  forbidden patterns
   FAIL	  valid
		Use of qw(...) as parentheses is deprecated 
   OK	  critic


5) Checking my database and comparing it to kohacstructure in your patch, the order of subscriptionid and parent_ordernumber are reversed. Not really sure how to explain that right now, so just noting.

Will continue to test as nothing is severe so far, but can you please provide a follow-up?
Comment 53 Jonathan Druart 2013-07-01 14:41:03 UTC Comment hidden (obsolete)
Comment 54 Jonathan Druart 2013-07-01 14:42:05 UTC Comment hidden (obsolete)
Comment 55 Jonathan Druart 2013-07-01 14:42:12 UTC Comment hidden (obsolete)
Comment 56 Jonathan Druart 2013-07-01 14:42:17 UTC Comment hidden (obsolete)
Comment 57 Jonathan Druart 2013-07-01 14:42:23 UTC Comment hidden (obsolete)
Comment 58 Jonathan Druart 2013-07-01 14:47:23 UTC
Thanks Katrin!

1/ Done in the followup.

2) I amended the first patch for that. I am afraid to change that in the followup (It it is pushed after bug 5394 we will loose the line).

3) Capitalization : done.

4) I never got this failure, really strange.

5) 3.09.00.043 and 3.11.00.116 did not use the "after column_name" in the alter table query. So it certainly comes from here.
Comment 59 Katrin Fischer 2013-07-01 14:53:51 UTC
Hi Jonathan, 

I think we both got the wrong bug number for 2)? :) Julian removed the internalnotes column now, so I think removing it now and readding the correct code later would be good.
Comment 60 Julian Maurice 2013-07-01 15:03:29 UTC
(In reply to Katrin Fischer from comment #59)
> Julian removed the internalnotes column now, so I think removing it now and
> readding the correct code later would be good.

Not sure of what you are talking about, but aqorders.internalnotes is added by Bug 5349, and I didn't remove anything from this patch yet
Comment 61 Katrin Fischer 2013-07-01 15:09:39 UTC
Right, was confusing it with the other acq related patch with cancellationreason.
Comment 62 Katrin Fischer 2013-07-03 22:10:43 UTC
Started QA on this.
Comment 63 Katrin Fischer 2013-07-04 06:26:01 UTC
Comment on attachment 19317 [details] [review]
Bug 5336:Order search (search and UI enhancements)

Review of attachment 19317 [details] [review]:
-----------------------------------------------------------------

::: C4/Acquisition.pm
@@ +2134,2 @@
>  
> +    $query .= " AND (datecancellationprinted is NULL or datecancellationprinted='0000-00-00') " if $orderstatus ne '4';

Checking for status 4 here...

@@ +2697,5 @@
> +    my $query        = "
> +        SELECT COUNT( DISTINCT( biblionumber ) )
> +        FROM   aqorders
> +        WHERE  basketno = ?
> +            AND (datecancellationprinted IS NULL OR datecancellationprinted='0000-00-00')

... but not checking for status 4 here?

::: koha-tmpl/intranet-tmpl/prog/en/modules/acqui/histsearch.tt
@@ +57,5 @@
> +                <option value="0">New</option>
> +                <option value="1">Ordered</option>
> +                <option value="2">Partial</option>
> +                <option value="3">Complete</option>
> +                <option value="4">Deleted</option>

Maybe instead of Deleted we should be using Cancelled. For easier and better translation we could also go a bit further and say:

New orders/?
Open orders/Pending orders
Partially received orders
Received orders
Cancelled orders

Only an idea, native speakers should check before implementing any change :)

New/ordered could lead to misunderstandings. I think new are not send to the vendor (basket still open) and ordered are supposed to have been sent out. But I am not sure how to reflect that better.
Comment 64 Katrin Fischer 2013-07-04 06:33:09 UTC
I have gone through the code again and I am going to start testing in the interface soon, Especially partial orders. I wonder if the update condition in the updatedatabase can work, as we split the orders for partial receives.

I also have been thinking about the status field, I see some advantages and disadvantages of doing it this way. The arguments I can think of right now are:

contra: we have another field to maintan where we have to make sure the value is set correctly at all times depending on actions and changing values in other fields. It also adds a bit more complexity to migrations as the status have to be mapped correctly.

pro: It will be easy to do SQL reports on aqorders based on the new status fields because you won't have to join tables and figure out the exact conditions.

I am a bit torn here, so leaving this as notes while I continue testing.
Comment 65 Katrin Fischer 2013-07-10 19:14:51 UTC
I'd like to suggest changing the 'magical numbers' into easier understandable codes. 

Also I would suggest renaming the status a bit:

- New
- On order
- Partially received
- Received
- Cancelled
- (Completed could later be used for closed and paid invoices maybe)

Not sure about the codes to use best, so this is more for the descriptions.
Comment 66 Katrin Fischer 2013-07-10 19:24:02 UTC
Jonathan, could you take a look at the my comments before I proceed here?
Comment 67 Jonathan Druart 2013-07-11 08:45:50 UTC Comment hidden (obsolete)
Comment 68 Jonathan Druart 2013-07-11 08:47:42 UTC
(In reply to Katrin Fischer from comment #66)
> Jonathan, could you take a look at the my comments before I proceed here?

Done in the last patch.
You have to remove the columns (ALTER TABLE aqorders DROP COLUMN orderstatus;) and execute the updatedatabase entry.
Comment 69 Katrin Fischer 2013-09-08 20:29:01 UTC
Jonathan, 

there are a couple of conflicts, could you please rebase? I will try to come back to this as quick as possible as soon as the patches apply again.
Comment 70 Jonathan Druart 2013-09-09 08:02:05 UTC Comment hidden (obsolete)
Comment 71 Jonathan Druart 2013-09-09 08:02:12 UTC Comment hidden (obsolete)
Comment 72 Jonathan Druart 2013-09-09 08:02:17 UTC Comment hidden (obsolete)
Comment 73 Jonathan Druart 2013-09-09 08:02:23 UTC Comment hidden (obsolete)
Comment 74 Jonathan Druart 2013-09-09 08:02:29 UTC Comment hidden (obsolete)
Comment 75 Jonathan Druart 2013-09-09 08:02:34 UTC Comment hidden (obsolete)
Comment 76 Jonathan Druart 2013-09-09 08:03:12 UTC
Rebased patches!
Comment 77 Pierre Angot 2013-09-29 08:34:30 UTC
Hi !

We are interrested by this patch and by 8230 :)

Did someone do the QA for this one ? 

Is it possible do have this patch for 3.10 and 3.12 ? Do we need to make other tests ?
Comment 78 Katrin Fischer 2013-09-29 09:26:26 UTC
This is a new feature waiting for QA - I have it on my list and hope to get some work done in it today. As it's a new feature it is very unlikely to get backported to older versions like 3.12 and 3.10.
Comment 79 Mathieu Saby 2013-09-29 09:32:15 UTC
I understand Katrin, but it's sad, because this feature has apparently been developped in 2011, and I first tested it one year ago...

Mathieu
Comment 80 Katrin Fischer 2013-09-29 09:35:03 UTC
It often makes me sad too to see things sit for a long time, but we can all only do as much as we can.
Comment 81 Pierre Angot 2013-09-29 10:26:43 UTC
Thank's for your answers ^^
Comment 82 Katrin Fischer 2013-09-29 12:08:43 UTC
Pierre, if you could confirm that partially received orders are correctly recognized by the database update that would be helpful.
Comment 83 Katrin Fischer 2013-09-29 12:12:36 UTC
Well, Pierre or anyone else :)
Comment 84 Katrin Fischer 2013-09-29 20:42:42 UTC
Still applies cleanly, all tests and QA script pass.
Have to prepare a database to test the databaes update and setting of correct status next.
If someone wants to help with additonal testing, noting the test plan and results - it would be very welcome :)
Comment 85 Pierre Angot 2013-09-30 05:48:16 UTC
Ok.

I'll test this afternoon ^^
Comment 86 Katrin Fischer 2013-09-30 21:11:25 UTC
I have finally started configuring a shiny new database with examples to test the update with. I am currently stuck on receiving records - there are no orders showing up for receiving in the table of pending orders. I have 2 closed baskets that are assigned to a closed basket group. It's too late to figure out what's going wrong here.
Comment 87 Katrin Fischer 2013-09-30 21:31:02 UTC
Any hints appreciated... the logs are clean. I am not seeing what I am doing wrong.
Comment 88 Pierre Angot 2013-10-01 02:41:04 UTC
I can't test on sandbox, there is something wrong with them. I'm waiting for Jonathan ^^
Comment 89 Biblibre Sandboxes 2013-10-01 03:31:13 UTC
Patch tested with a sandbox, by Pierre Angot <tredok.pierre@gmail.com>
Comment 90 Biblibre Sandboxes 2013-10-01 03:32:22 UTC Comment hidden (obsolete)
Comment 91 Biblibre Sandboxes 2013-10-01 03:32:27 UTC Comment hidden (obsolete)
Comment 92 Biblibre Sandboxes 2013-10-01 03:32:31 UTC Comment hidden (obsolete)
Comment 93 Biblibre Sandboxes 2013-10-01 03:32:35 UTC Comment hidden (obsolete)
Comment 94 Biblibre Sandboxes 2013-10-01 03:32:40 UTC Comment hidden (obsolete)
Comment 95 Biblibre Sandboxes 2013-10-01 03:32:44 UTC Comment hidden (obsolete)
Comment 96 Pierre Angot 2013-10-01 03:33:56 UTC
Well, it works on sandbox 3 ^^

Test plan :
- Add 2 orders, first with 3 items, second with 2
- Receive 2 of the first order
- cancel one order
- search order

Results :
- isbn : ok (isbn 10 and 13)
- author : ok
- title : ok, even if it is partial
- fund : ok
- basket : ok
- basketgroup : ok
- Bookseller invoice : ok, but label is not correct, it works with invoice name, not number
- order status : ok for all
Comment 97 Katrin Fischer 2013-10-01 05:59:55 UTC
Hi Pierre, thanks :) What I am trying to do is check the update procedure - but I am still stuck with receiving orders. I will try again tonight to see what's going wrong in my installation. It puzzles me quite a bit! Have you tested with AcqCreateItems "on order"?
Comment 98 Katrin Fischer 2013-10-01 06:05:11 UTC
Created attachment 21695 [details]
my orders...

Maybe someone spots something I don't see?
Comment 99 Katrin Fischer 2013-10-01 06:05:50 UTC
Created attachment 21696 [details]
... and no pending orders
Comment 100 Pierre Angot 2013-10-01 07:18:35 UTC
On sandboxes, AcqCreateItems is "placing an order". I haven't tested with others :/
Comment 101 Katrin Fischer 2013-10-01 07:30:54 UTC
Testing with that too - it's just I am missing all pending orders... until I get that working I can't really continue to test.
Comment 102 Pierre Angot 2013-10-01 09:38:27 UTC
I've made a test with AcqCreateItem on "Receive an order". Everything is correct.

All my pending orders are there. No one is missing.
Comment 103 Katrin Fischer 2013-10-01 09:40:10 UTC
I will investigate the code and the database tonight - another idea I have is permissions - are you superlibrarian?
Comment 104 Pierre Angot 2013-10-01 10:40:25 UTC
Yes, I'm SuperLibriarian ^^

You can check on sandbox nĀ°3 : http://pro.test3.biblibre.com/
Comment 105 Katrin Fischer 2013-10-01 11:26:06 UTC
Thx - but I really need to figure out what's wrong with my installation :) I am never testing with superlibrarian, because our users don't get this permission and I want to make sure it works right for them.
Comment 106 Jonathan Druart 2013-10-01 12:05:32 UTC
(In reply to Katrin Fischer from comment #105)
> Thx - but I really need to figure out what's wrong with my installation :) I
> am never testing with superlibrarian, because our users don't get this
> permission and I want to make sure it works right for them.

Katrin,
Sorry for the delay.
I think there is a bug on updating the orderstatus field.
I will try to submit a followup asap.
Comment 107 Jonathan Druart 2013-10-01 12:09:28 UTC Comment hidden (obsolete)
Comment 108 Jonathan Druart 2013-10-01 12:10:39 UTC
Katrin,
I let you switch to needs signoff if you think it is needed.
Comment 109 Pierre Angot 2013-10-01 12:11:02 UTC
Do you have check permission order_manage and order_receive ? Here, I give the 2 permissions.

I think I've maybe missed something. Did this patch do a modification in receive shipment ?
Comment 110 Katrin Fischer 2013-10-01 12:58:43 UTC
Hi Pierre, I am sorry - I think I confused you. I can't receive anything in my normal installation without these patches. It's hard to test anything in acq when you have such a problem. I will figure it out, don't worry. This patch is not applied yet.
Comment 111 Katrin Fischer 2013-10-01 13:01:04 UTC
Hi Jonathan, thx for the follow-up - I remember from reading the code I had a feeling there might be a problem for the partial receives. Give me a chance to fix my installation and I will continue testing this.
Comment 112 Katrin Fischer 2013-10-01 16:58:40 UTC
I used git bisect to locate the problem:
04b4afbd92ab952671e5ea6124e9e95da4a05874 is the first bad commit
Bug 9130 follow up: add statesave cookie and "all" option for parcel drop-down menu

After deleting all cookies, the pending orders show again. So maybe a broken cookie caused the problem? Can continue testing now :)
Comment 113 Katrin Fischer 2013-10-01 19:47:23 UTC
Hi Jonathan,

I am pretty impressed with this :) 

But there are some issues, that need fixing:

1) Database update - I end up with too many partials. My test cases:

 - New orders, basket still open
   NOT OK Expected: new, Result: partial
 - Ordered orders, basket and basketgroup closed
   NOT OK Expected: ordered, Result: partial 
 - Partially received orders
   OK Expected: partial/complete, Result: partial/complete
 - Received orders
   OK: Expected: complete, Result: complete
 - Cancelled orders
   * cancelled from open basket before order
   OK: Expected: cancelled, Result: cancelled
   * cancelled from closed basket before receive
   OK: Expected: cancelled, Result: cancelled

I have saved my 'pre-updatedatabase' and can repeat the test anytime you have a follow up.

2) Testing search functionality

  a) Order search - result list
  Order search shows a new column, I think it would be a bit better if the
  status there was singular instead of plural - new order, partially received
  order etc. - maybe we could even leave out the 'order'? (minor)

  The column seems a bit lost in the middle, not sure where it would make more
  sense (just saying)

  b) Order search - advanced search form
  The advanced search form now shows a new filter: Order status
  All new status can be searched.
  - an "empty" search will find all but cancelled orders
  - searching for cancelled orders works correctly and shows results
  Search works correctly, no regressions found.

  When a search gives no results, there is no message. Also, all filters and
  fields are cleared so you can't modify or check you previous search
  (minor)

3) Testing setting of status after applying the patch

   a) Create a new order with 2 items - status is new. OK
   b) Close the basket - status is ordered. OK
   c) Receive both items - status is completed. OK
   d) Undo receipt - status is ordered. OK
   e) Receive only 1 item - order is split up into 2 orders:
       - status is partial OK
       - status is completed OK
   f) Undo receipt of received item - order is combined into 1 again
       - status is ordered OK!
   g) Receive only 1 item again - status ordered/partial.
   h) Delete order.
       - status is completed
       - status is cancelled
      OK!
   i) Undo receipt of 1 item again.
      Refresh page. Error message is shown:
      
Cannot cancel receipt. Possible reasons :
    The order line you trying to cancel was created from a partial receipt of another order line which is already received. Try to cancel this one first and retry.
    The order line you trying to cancel was created from a partial receipt of another order line which has been deleted. Cancellation is not possible.

BUT: The receipt is undon, you are left with an order
     line with 2 items, a cancellation date and the status ordered.

Because of the cancellation date the order is not visible in pending orders.

Summary: The database update is not quite right yet and there is an edge case where you are left with the wrong status. Also singulars in the orders search result page would be nice.

Hope you can take another look - I like this.
Comment 114 Jonathan Druart 2013-10-02 08:02:07 UTC Comment hidden (obsolete)
Comment 115 Jonathan Druart 2013-10-02 08:02:15 UTC Comment hidden (obsolete)
Comment 116 Jonathan Druart 2013-10-02 08:06:27 UTC
(In reply to Katrin Fischer from comment #113)
> Hi Jonathan,

Hi Katrin, Thanks for this great feedback :)

> 1) Database update - I end up with too many partials. My test cases:

Yep, The previous followup introduces some issues on updating status. I think this one is good.

> 2) Testing search functionality
> 
>   a) Order search - result list
>   Order search shows a new column, I think it would be a bit better if the
>   status there was singular instead of plural - new order, partially received
>   order etc. - maybe we could even leave out the 'order'? (minor)

Fixed.

> 
>   The column seems a bit lost in the middle, not sure where it would make
> more
>   sense (just saying)

Fixed, moved to the beginning of the table.

>   When a search gives no results, there is no message. Also, all filters and
>   fields are cleared so you can't modify or check you previous search
>   (minor)

Fixed. The issue only existed for fund and order status.

>    i) Undo receipt of 1 item again.
>       Refresh page. Error message is shown:
>       
> Cannot cancel receipt. Possible reasons :
>     The order line you trying to cancel was created from a partial receipt
> of another order line which is already received. Try to cancel this one
> first and retry.
>     The order line you trying to cancel was created from a partial receipt
> of another order line which has been deleted. Cancellation is not possible.
> 
> BUT: The receipt is undon, you are left with an order
>      line with 2 items, a cancellation date and the status ordered.
> 
> Because of the cancellation date the order is not visible in pending orders.

I reproduce the issue with the master branch. It is not from these patches.
Could you confirm that?

I will try to have a look and propose a followup on another report.
Comment 117 Katrin Fischer 2013-10-02 08:16:04 UTC
I can't test before tonight - so if you could confirm it's also on master I would be ok with that. Could you go ahead and file the new bug?
Comment 118 Jonathan Druart 2013-10-02 08:51:42 UTC Comment hidden (obsolete)
Comment 119 Katrin Fischer 2013-10-03 10:28:38 UTC
Hi Jonathan, I wanted to come back to this but see you have set Failed QA still - is it because of the edge case?

I think we could deal with that as a separate bug, as the behaviour is buggy on master as well. - If that is the only problem remaining.
Comment 120 Katrin Fischer 2013-10-03 11:03:35 UTC
Created attachment 21751 [details] [review]
[PASSED QA] Bug 5336:Order search (search and UI enhancements)

You can now search orders by
- order status
- fund

New field aqorders.orderstatus, it can contains following values:
  0 => New
  1 => Ordered
  2 => Partial
  3 => Complete
  4 => Deleted

To test: Search and check if results are consistent in histsearch.pl
Signed-off-by: Cedric Vita <cedric.vita@dracenie.com>

Signed-off-by: Pierre Angot <tredok.pierre@gmail.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comments on last patch.
Note: status are no longer numeric, but strings now.
Comment 121 Katrin Fischer 2013-10-03 11:03:47 UTC
Created attachment 21752 [details] [review]
[PASSED QA] Bug 5336: Late orders search: don't display deleted order.

Test plan:
Check that deleted orders are not listed in the late orders search
results.
Signed-off-by: Cedric Vita <cedric.vita@dracenie.com>

Signed-off-by: Pierre Angot <tredok.pierre@gmail.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 122 Katrin Fischer 2013-10-03 11:03:56 UTC
Created attachment 21753 [details] [review]
[PASSED QA] Bug 5336: Merge parent ordernumber and ordernumber columns Signed-off-by: Cedric Vita <cedric.vita@dracenie.com>

Signed-off-by: Pierre Angot <tredok.pierre@gmail.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 123 Katrin Fischer 2013-10-03 11:04:07 UTC
Created attachment 21754 [details] [review]
[PASSED QA] Bug 5336: Add unit tests for 3 acq routines

Unit tests In C4::Acquisition for:
- CloseBasket
- ReopenBasket
- GetBiblioCountByBasketno

Signed-off-by: Cedric Vita <cedric.vita@dracenie.com>
Signed-off-by: Pierre Angot <tredok.pierre@gmail.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 124 Katrin Fischer 2013-10-03 11:04:20 UTC
Created attachment 21755 [details] [review]
[PASSED QA] Bug 5336: QA followup

- fix capitalization
- fix update database message

Signed-off-by: Pierre Angot <tredok.pierre@gmail.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 125 Katrin Fischer 2013-10-03 11:04:30 UTC
Created attachment 21756 [details] [review]
[PASSED QA] Bug 5336: Change numbers with understandable codes for orderstatus

This patch uses understandable codes instead of magical numbers for the
aqorders.orderstatus field.

+ execute sql queries in unit tests into a transaction.

Signed-off-by: Pierre Angot <tredok.pierre@gmail.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 126 Katrin Fischer 2013-10-03 11:04:42 UTC
Created attachment 21757 [details] [review]
[PASSED QA] Bug 5336: FIX: partial status was not correctly set

An order (o1) is considered as partially received if (at least) another
order (o2) has the o1.ordernumber == o2.parent_ordernumber and the
quantity received is < to the quantity (to receive) and o1.ordernumber
!= o1.parent_ordernumber.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 127 Katrin Fischer 2013-10-03 11:04:52 UTC
Created attachment 21758 [details] [review]
[PASSED QA] Bug 5336: Multiple QA fixes

- add a message if the search returns no result
- keep selected values if the search returns no result (for fund and
  order status)
- remove plurals in order status
- move the order status column in the search results table

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes all tests and QA script.
New tests also pass:
    t/db_dependent/Acquisition/close_reopen_basket.t

1) Database update - I end up with too many partials. My test cases:

 - New orders, basket still open
   OK Expected: new, Result: partial
 - Ordered orders, basket and basketgroup closed
   OK Expected: ordered, Result: partial
 - Partially received orders
   OK Expected: partial/complete, Result: partial/complete
 - Received orders
   OK: Expected: complete, Result: complete
 - Cancelled orders
   * cancelled from open basket before order
   OK: Expected: cancelled, Result: cancelled
   * cancelled from closed basket before receive
   OK: Expected: cancelled, Result: cancelled

All aqorders where updated with the correct status.

I have saved my 'pre-updatedatabase' and can repeat the test anytime you have a follow up.

2) Testing search functionality

  a) Order search - result list
  Order search shows a new column, I think it would be a bit better if the
  status there was singular instead of plural - new order, partially received
  order etc. - maybe we could even leave out the 'order'? (minor)

  The column seems a bit lost in the middle, not sure where it would make more
  sense (just saying)

  b) Order search - advanced search form
  The advanced search form now shows a new filter: Order status

  All new status can be searched.
  - an "empty" search will find all but cancelled orders
  - searching for cancelled orders works correctly and shows results
  Search works correctly, no regressions found.

  If no result is found a message is shown.
  All search input is kept, so you can modify your search terms easily.

3) Testing setting of status after applying the patch

   a) Create a new order with 2 items - status is new. OK
   b) Close the basket - status is ordered. OK
   c) Receive both items - status is completed. OK
   d) Undo receipt - status is ordered. OK
   e) Receive only 1 item - order is split up into 2 orders:
       - status is partial OK
       - status is completed OK
   f) Undo receipt of received item - order is combined into 1 again
       - status is ordered OK!
   g) Receive only 1 item again - status ordered/partial.
   h) Delete order.
       - status is completed
       - status is cancelled
      OK!

   i) Undo receipt of 1 item again.
      Refresh page.

This results in the following behaviour, that has been reported as
bug 10984. After refreshing the page follow message is shown:
  Cannot cancel receipt. Possible reasons :
    - The order line you trying to cancel was created from a partial
    receipt of another order line which is already received.
    Try to cancel this one first and retry.
    - The order line you trying to cancel was created from a partial
    receipt of another order line which has been deleted.
    Cancellation is not possible.
  BUT: The receipt is undone, but you are left with a
    line with 2 items, a cancellation date and the status ordered.
    Because of the cancellation date the order is not visible in pending orders.

The status is correct - so I feel this should not stop this patch.
Comment 128 Katrin Fischer 2013-10-03 11:05:56 UTC
Hi Jonathan, guess it's ok that I retested, but please let me know if I missed something :)
Comment 129 Jonathan Druart 2013-10-04 08:25:49 UTC
(In reply to Katrin Fischer from comment #128)
> Hi Jonathan, guess it's ok that I retested, but please let me know if I
> missed something :)

Yep, thanks! It seems I missed to change the status.
Comment 130 Pierre Angot 2013-10-04 11:22:18 UTC
Yeah ! Now, we are waiting for pushing. I can start to test 8230 ^^
Comment 131 Pierre Angot 2013-10-17 06:55:03 UTC
Can you push this patch ?

We are waiting it to test 8230.
Comment 132 Galen Charlton 2013-10-22 13:53:03 UTC
Pushed to master, along with some follow-ups.  The most significant one I believe resolves the concern Katrin had about too many orders ending up in the partial status when 'ordered' or 'new' would have been better.  I'd appreciate review of that in case that particular follow-up needs to be modified.

Thanks, Jonathan!
Comment 133 Katrin Fischer 2013-10-22 14:08:17 UTC
Argh. I see that when I updated my test plan I put in OK everywhere, but I missed updating the line. The second test I did with Jonathan's changed updatedatabase procuded perfect results for all test data I had created - so the partials were correctly set, before I set to passed QA.
Comment 134 Jonathan Druart 2013-10-22 14:51:01 UTC
(In reply to Galen Charlton from comment #132)
> Pushed to master, along with some follow-ups.  The most significant one I
> believe resolves the concern Katrin had about too many orders ending up in
> the partial status when 'ordered' or 'new' would have been better.  I'd
> appreciate review of that in case that particular follow-up needs to be
> modified.

I agree with your followups. Thanks!