New options will be added to the history search (isbn, library, budget, invoicenumber)
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
Created attachment 11603 [details] [review] Bug 5336:Order search (search and UI enhancements) You can now search orders by - order status - branchcode - budget New authorised value 'ORDRSTATUS' is created with following values: 0 => New 1 => Requested 2 => Partial 3 => Complete 4 => Deleted New field aqorders.parent_ordernumber is added (which is mainly used by others enhancement). It allows to know the main(=parent) order for an order (if an order is partially received, Koha creates another order. But it is the same order. We want to know this information). To test: Search and check if results are consistent in histsearch.pl /!\ the search by branchcode is not available now. We have to wait the Bug 7294.
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.
Created attachment 11604 [details] [review] Bug 5336:Order search (search and UI enhancements) You can now search orders by - order status - branchcode - budget New authorised value 'ORDRSTATUS' is created with following values: 0 => New 1 => Ordered 2 => Partial 3 => Complete 4 => Deleted New field aqorders.parent_ordernumber is added (which is mainly used by others enhancement). It allows to know the main(=parent) order for an order (if an order is partially received, Koha creates another order. But it is the same order. We want to know this information). To test: Search and check if results are consistent in histsearch.pl
Created attachment 11792 [details] [review] Bug 5336: Replace IFNULL with COALESCE
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".
Created attachment 14067 [details] [review] Bug 5336:Order search (search and UI enhancements) You can now search orders by - order status - branchcode - budget New authorised value 'ORDRSTATUS' is created with following values: 0 => New 1 => Ordered 2 => Partial 3 => Complete 4 => Deleted New field aqorders.parent_ordernumber is added (which is mainly used by others enhancement). It allows to know the main(=parent) order for an order (if an order is partially received, Koha creates another order. But it is the same order. We want to know this information). To test: Search and check if results are consistent in histsearch.pl
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
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)
Maybe my install is corrupted. I will test again later... M. Saby
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
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
Created attachment 14188 [details] [review] Bug 5336:Order search (search and UI enhancements) You can now search orders by - order status - branchcode - budget New authorised value 'ORDRSTATUS' is created with following values: 0 => New 1 => Ordered 2 => Partial 3 => Complete 4 => Deleted New field aqorders.parent_ordernumber is added (which is mainly used by others enhancement). It allows to know the main(=parent) order for an order (if an order is partially received, Koha creates another order. But it is the same order. We want to know this information). To test: Search and check if results are consistent in histsearch.pl
- 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
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.
(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.
Created attachment 14213 [details] [review] Bug 5336:Order search (search and UI enhancements) You can now search orders by - order status - branchcode - budget New field aqorders.orderstatus, it can contains following values: 0 => New 1 => Ordered 2 => Partial 3 => Complete 4 => Deleted New field aqorders.parent_ordernumber is added (which is mainly used by others enhancement). It allows to know the main(=parent) order for an order (if an order is partially received, Koha creates another order. But it is the same order. We want to know this information). To test: Search and check if results are consistent in histsearch.pl
REMEMBER: You must have the field aqorders.branchcode in your DB (created by Bug 7294).
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
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.
So I will test this patch again when BZ 7294 is pushed (current status is "Failed QA"). It is "cleaner" for me. M. Saby
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
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
Created attachment 16013 [details] [review] Bug 5336:Order search (search and UI enhancements) You can now search orders by - order status - branchcode - budget New field aqorders.orderstatus, it can contains following values: 0 => New 1 => Ordered 2 => Partial 3 => Complete 4 => Deleted New field aqorders.parent_ordernumber is added (which is mainly used by others enhancement). It allows to know the main(=parent) order for an order (if an order is partially received, Koha creates another order. But it is the same order. We want to know this information). To test: Search and check if results are consistent in histsearch.pl
By the way, applying 7294 does not update the database. Pbm in updatedatabase.pl (3.09.XXX should be 3.11.XXX) M. Saby
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
This patch should be apply on top of bug 7294
Created attachment 17612 [details] [review] 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
This last patch doesn't depend on bug 7294.
Created attachment 17613 [details] [review] 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
Created attachment 17615 [details] [review] 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.
Created attachment 17617 [details] [review] Bug 5336: Add unit tests for 3 acq routines Unit tests In C4::Acquisition for: - CloseBasket - ReopenBasket - GetBiblioCountByBasketno
Created attachment 17648 [details] [review] 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
Created attachment 17649 [details] [review] 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.
Created attachment 17650 [details] [review] Bug 5336: Add unit tests for 3 acq routines Unit tests In C4::Acquisition for: - CloseBasket - ReopenBasket - GetBiblioCountByBasketno
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
(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...
Created attachment 17883 [details] [review] Bug 5336: Merge parent ordernumber and ordernumber columns
Created attachment 18187 [details] [review] Bug 5336: Add unit tests for 3 acq routines Unit tests In C4::Acquisition for: - CloseBasket - ReopenBasket - GetBiblioCountByBasketno
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
Created attachment 18411 [details] [review] 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
Created attachment 18412 [details] [review] 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.
Created attachment 18413 [details] [review] Bug 5336: Merge parent ordernumber and ordernumber columns
Created attachment 18414 [details] [review] Bug 5336: Add unit tests for 3 acq routines Unit tests In C4::Acquisition for: - CloseBasket - ReopenBasket - GetBiblioCountByBasketno
Patch tested with a sandbox, by Cedric Vita <cedric.vita@dracenie.com>
Created attachment 18496 [details] [review] 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>
Created attachment 18497 [details] [review] 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>
Created attachment 18498 [details] [review] 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>
Created attachment 18499 [details] [review] Bug 5336: Merge parent ordernumber and ordernumber columns Signed-off-by: Cedric Vita <cedric.vita@dracenie.com>
Created attachment 18500 [details] [review] 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>
Last 4 patches are signoff patch for CĆ©dric Vita. Thanks CĆ©dric!
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?
Created attachment 19317 [details] [review] 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>
Created attachment 19318 [details] [review] 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>
Created attachment 19319 [details] [review] Bug 5336: Merge parent ordernumber and ordernumber columns Signed-off-by: Cedric Vita <cedric.vita@dracenie.com>
Created attachment 19320 [details] [review] 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>
Created attachment 19321 [details] [review] Bug 5336: QA followup - fix capitalization - fix update database message
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.
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.
(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
Right, was confusing it with the other acq related patch with cancellationreason.
Started QA on this.
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.
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.
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.
Jonathan, could you take a look at the my comments before I proceed here?
Created attachment 19550 [details] [review] 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.
(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.
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.
Created attachment 20894 [details] [review] 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>
Created attachment 20895 [details] [review] 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>
Created attachment 20896 [details] [review] Bug 5336: Merge parent ordernumber and ordernumber columns Signed-off-by: Cedric Vita <cedric.vita@dracenie.com>
Created attachment 20897 [details] [review] 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>
Created attachment 20898 [details] [review] Bug 5336: QA followup - fix capitalization - fix update database message
Created attachment 20899 [details] [review] 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.
Rebased patches!
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 ?
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.
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
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.
Thank's for your answers ^^
Pierre, if you could confirm that partially received orders are correctly recognized by the database update that would be helpful.
Well, Pierre or anyone else :)
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 :)
Ok. I'll test this afternoon ^^
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.
Any hints appreciated... the logs are clean. I am not seeing what I am doing wrong.
I can't test on sandbox, there is something wrong with them. I'm waiting for Jonathan ^^
Patch tested with a sandbox, by Pierre Angot <tredok.pierre@gmail.com>
Created attachment 21689 [details] [review] 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>
Created attachment 21690 [details] [review] 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>
Created attachment 21691 [details] [review] 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>
Created attachment 21692 [details] [review] 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>
Created attachment 21693 [details] [review] Bug 5336: QA followup - fix capitalization - fix update database message Signed-off-by: Pierre Angot <tredok.pierre@gmail.com>
Created attachment 21694 [details] [review] 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>
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
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"?
Created attachment 21695 [details] my orders... Maybe someone spots something I don't see?
Created attachment 21696 [details] ... and no pending orders
On sandboxes, AcqCreateItems is "placing an order". I haven't tested with others :/
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.
I've made a test with AcqCreateItem on "Receive an order". Everything is correct. All my pending orders are there. No one is missing.
I will investigate the code and the database tonight - another idea I have is permissions - are you superlibrarian?
Yes, I'm SuperLibriarian ^^ You can check on sandbox nĀ°3 : http://pro.test3.biblibre.com/
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.
(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.
Created attachment 21697 [details] [review] 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).
Katrin, I let you switch to needs signoff if you think it is needed.
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 ?
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.
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.
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 :)
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.
Created attachment 21707 [details] [review] 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.
Created attachment 21708 [details] [review] 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
(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.
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?
Created attachment 21709 [details] [review] 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
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.
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.
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>
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>
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>
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>
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>
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>
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.
Hi Jonathan, guess it's ok that I retested, but please let me know if I missed something :)
(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.
Yeah ! Now, we are waiting for pushing. I can start to test 8230 ^^
Can you push this patch ? We are waiting it to test 8230.
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!
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.
(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!