Created attachment 6234 [details] addorderiso2709.pl This development provide a way to create multiple order lines at a time from a staged file. You can choose what biblio you want to order, and set a quantity, a price and a budget for each. See screenshot attached. BibLibre infos: Repository: git://git.biblibre.com/koha_biblibre.git Branch: unilim/ft/MT6843
Created attachment 13019 [details] [review] Bug 7180: Order from staged file improvements This patch adds: - 1 syspref MarcFieldsToOrder - 1 ajax script acqui/ajax-getauthvaluedropbox.pl - 1 routine C4::Budgets::GetBudgetByCode Before this patch you were not able to order 1 or all the records from your staged file. You were allowed to specify some information ("Import All" and "Accounting details" areas) for the order. With this patch, the previous behaviours still exist. But now you can *select* which records you want to ordered. For these ones you can specify independently informations (Quantity, price, budget, sort1 and sort2). The cherry on the cake is that you can pre-fill these information with values Test plan: 1. Fill the new syspref MarcFieldsToOrder with something like: ==BEGIN== price: 947$c quantity: 969$h budget_code: 922$a rrp: 010$d discount: 969$d sort1: 923$a sort2: 924$a ==END== The empty line at the end is mandatory! The budget (corresponding to your budget_code) can be filled with authorized value categories (statistic 1 and 2). The sort1 and sort2 values can be filled with the an authorized value (of the category previously selected) 2. Choose randomly one or more biblio(s) and fill fields with what is relevant. 3. Export the biblio and import it (with the "Stage MARC records for import" tool). 4. Go on a basket and add an order from a staged file. Select your staged file. 5. Well. Now you can see your biblio (or biblios if your had exported more than one). For each one, informations should be pre-filled with the biblio values. The budget should be selected on the budget corresponding to the budget_code (in the field 922$a) and the "planning values" too (with fields 923$a and 924$a). You can modify these values (or not) and choose a default value for budget and planning values (in the "Accounting details" area). 6. Save and check the prices values. Modify the order and check that budget and sort* are good Prices are calculated following some parameters: if there is no price => listprice = 0 else => - the gstrate value for your order is the gstrate value of the bookseller - discount = if filled : the discount value / 100 else: the discount value of the bookseller - if the bookseller includes tax( List item price includes tax: Yes ) if a discount exists: ecost = price rrp = ecost / ( 1 - discount ) else: # a discount does not exist ecost = price * ( 1 - discount ) rrp = price else # the bookseller does not include tax if a discount exists: ecost = price / ( 1 + gstrate ) rrp = ecost / ( 1 - discount ) else: # a discount does not exist rrp = price / ( 1 + gstrate ) ecost = rrp * ( 1 - discount ) - in all cases: listprice = rrp / currency rate unitprice = ecost total = ecost * quantity 7. Retry with different parameters 8. Check the 'Import all' action still works
Hello I tried to test your patch but it does not apply : Applying: Bug 7180: Order from staged file improvements Using index info to reconstruct a base tree... <stdin>:357: new blank line at EOF. + warning: 1 line adds whitespace errors. Falling back to patching base and 3-way merge... error: The following untracked working tree files would be overwritten by merge: acqui/ajax-getauthvaluedropbox.pl Please move or remove them before you can merge. Aborting Failed to merge in the changes. Patch failed at 0001 Bug 7180: Order from staged file improvements M. Saby
Created attachment 15023 [details] [review] Bug 7180: Order from staged file improvements Rebased patch.
Hello Jonathan. I got a new error : fatal: sha1 information is lacking or useless (installer/data/mysql/updatedatabase.pl). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 7180: Order from staged file improvements 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". M. Saby
Created attachment 15048 [details] [review] Bug 7180: Order from staged file improvements
I want to test this afternoon, but new error : Could you please rebase it again? <stdin>:347: new blank line at EOF. + warning: 1 line adds whitespace errors. Falling back to patching base and 3-way merge... error: The following untracked working tree files would be overwritten by merge: acqui/ajax-getauthvaluedropbox.pl Please move or remove them before you can merge. Aborting Failed to merge in the changes. Patch failed at 0001 Bug 7180: Order from staged file improvements 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". Patch left in /tmp/Bug-7180-Order-from-staged-file-improvements-CiHSV3.patch M. Saby
(In reply to comment #6) > error: The following untracked working tree files would be overwritten by > merge: > acqui/ajax-getauthvaluedropbox.pl > Please move or remove them before you can merge. Try to delete this file before applying this patch.
new conflit, but maybe my VM is not clean... CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/css/staff-global.css M. saby
Created attachment 15617 [details] [review] Bug 7180: Order from staged file improvements This patch adds: - 1 syspref MarcFieldsToOrder - 1 ajax script acqui/ajax-getauthvaluedropbox.pl - 1 routine C4::Budgets::GetBudgetByCode Before this patch you were not able to order 1 or all the records from your staged file. You were allowed to specify some information ("Import All" and "Accounting details" areas) for the order. With this patch, the previous behaviours still exist. But now you can *select* which records you want to ordered. For these ones you can specify independently informations (Quantity, price, budget, sort1 and sort2). The cherry on the cake is that you can pre-fill these information with values Test plan: 1. Fill the new syspref MarcFieldsToOrder with something like: ==BEGIN== price: 947$c quantity: 969$h budget_code: 922$a rrp: 010$d discount: 969$d sort1: 923$a sort2: 924$a ==END== The empty line at the end is mandatory! The budget (corresponding to your budget_code) can be filled with authorized value categories (statistic 1 and 2). The sort1 and sort2 values can be filled with the an authorized value (of the category previously selected) 2. Choose randomly one or more biblio(s) and fill fields with what is relevant. 3. Export the biblio and import it (with the "Stage MARC records for import" tool). 4. Go on a basket and add an order from a staged file. Select your staged file. 5. Well. Now you can see your biblio (or biblios if your had exported more than one). For each one, informations should be pre-filled with the biblio values. The budget should be selected on the budget corresponding to the budget_code (in the field 922$a) and the "planning values" too (with fields 923$a and 924$a). You can modify these values (or not) and choose a default value for budget and planning values (in the "Accounting details" area). 6. Save and check the prices values. Modify the order and check that budget and sort* are good Prices are calculated following some parameters: if there is no price => listprice = 0 else => - the gstrate value for your order is the gstrate value of the bookseller - discount = if filled : the discount value / 100 else: the discount value of the bookseller - if the bookseller includes tax( List item price includes tax: Yes ) if a discount exists: ecost = price rrp = ecost / ( 1 - discount ) else: # a discount does not exist ecost = price * ( 1 - discount ) rrp = price else # the bookseller does not include tax if a discount exists: ecost = price / ( 1 + gstrate ) rrp = ecost / ( 1 - discount ) else: # a discount does not exist rrp = price / ( 1 + gstrate ) ecost = rrp * ( 1 - discount ) - in all cases: listprice = rrp / currency rate unitprice = ecost total = ecost * quantity 7. Retry with different parameters 8. Check the 'Import all' action still works
In some tests it seems to work, and in others, not :( I have not yet find why... (I tried to change the syspref, and tested different records. and maybe my acq framework was not including the fields mapped in the syspref...) But I can already make some remarks : What happens if a non numeric value is defined for quantity or discount? In particulary for discount. I believe there is no warn. So, as discount is invisible, it could be a problem. If is is imported, maybe the discount value should be visible for each record? What happens if a different value for fund, sort1, sort2, is defined in some records and in the Accounting details bloc at the bottom of the page? What happens if one field mapped in the syspref does not exist in the imported record? (eg : no 947$c in the record). I suppose all the other fields will be imported, but I am not sure... What happens if one field mapped in the syspref does not exist in the default/acq framework? In the syspref description, librarians will need the list of the usable values (in your example you give price, quantitity, budget_code, rrp, discout, sort1, sort2, but maybe there are others?) For example, is it possible to use the syspref for importing - gst rate - acquisition notes? I won't be able to make an other test until March 6h. M Saby
(In reply to comment #10) > But I can already make some remarks : > What happens if a non numeric value is defined for quantity or discount? In > particulary for discount. I believe there is no warn. So, as discount is > invisible, it could be a problem. > If is is imported, maybe the discount value should be visible for each > record? Will be displayed in a followup. > What happens if a different value for fund, sort1, sort2, is defined in some > records and in the Accounting details bloc at the bottom of the page? If a specific value is filled for a record, we take this one. Else the value will be the "default value" (value in the Accounting details). > What happens if one field mapped in the syspref does not exist in the > imported record? (eg : no 947$c in the record). I suppose all the other > fields will be imported, but I am not sure... Yes, It should work like that. > What happens if one field mapped in the syspref does not exist in the > default/acq framework? The mapping is not based on a framework. So the field will be updated with the given value. > In the syspref description, librarians will need the list of the usable > values (in your example you give price, quantitity, budget_code, rrp, > discout, sort1, sort2, but maybe there are others?) The list is exhaustive for this development. But the syspref could be used by another development (using some others fields). > For example, is it possible to use the syspref for importing > - gst rate > - acquisition notes? No, but it could be possible later.
Created attachment 15677 [details] [review] Bug 7180: Followup Show the discount value
I tested again on my VM and sandbox 3 => Failed Budget, sort1 and sort2 are not imported from records. Here is the detail : connected to Koha with a bib user (not the admin user) no acq framework default framework includes 947$c, 969$h, 922$a, 010$d, 969$d, 923$a, 924$a currency : EUR vendor with - gst = 0 - discount = 5 - list price include tax budget : "test", with fund code "budgetok" (fund name "budgetok") No importing matching rule parameters for MarcFieldsToOrder : price: 947$c quantity: 969$h budget_code: 922$a rrp: 010$d discount: 969$d sort1: 923$a sort2: 924$a ------ Batch of 7 records 200 1 $a Eudora Welty $b Texte imprimé $f edited and with an introduction by Harold Bloom,... 010 $d 11 EUR 922 $a budgetok 947 $c 10.00 969 $a 3 $h 3 200 1 $a Le théâtre du Marais $h 1 $i La période de gloire et de fortune, 1634(1629)-1648 $b Texte imprimé $f S. Wilma Deierkauf-Holsboer 010 $d 10,50 EUR 922 $a budgetbidon 923 $a stat1 924 $a stat2 947 $c 10.50 969 $a 2 $h 2 200 1 $a Paul Valéry et Pascal $b Texte imprimé $f Antonio G. Rodriguez 010 $d 23,70 EUR 923 $a stat1 947 $c 23.70 969 $a 1 $h 1 200 1 $a Arthur Miller's "Death of a salesman" $f ed. and with an introd. by Harold Bloom 010 $d 100.00 EUR 922 $a budgetok 923 $a stat1 924 $a stat2 947 $c 90.00 969 $a 3 $d 50 $h 3 200 1 $a Didactique de l'orthographe française $b Texte imprimé $e méthode, expériences et exercices pédagogiques $f Michel Gey 010 $d 12.50 EUR 922 $a budgetok 923 $a tri1 969 $a 1 $h 1 $d 10.50 200 1 $a Histoire de la sculpture $b Texte imprimé $f par Luc Benoist 922 $a budgetok 923 $a tri1 947 $c 15.50 969 $a 1 $h 1 $d 10 200 1 $a Premiers exercices de grammaire $f Sabine Dupré la Tour et Geneviève-Dominique de Salins 010 $d $ 10.00 922 $a budgetok 923 $a tri1 947 $c 10.50 969 $a 2 $d 20.50 $h 2 M. Saby
Created attachment 15934 [details] [review] Bug 7180: Fix typo, budget_code is a var, not a string...
Created attachment 15935 [details] [review] Bug 7180: FIX ergonomic issue on selecting record Before this patch, all labels were active for the first checkbox.
Hello Mathieu, Very stupid error, my fault. Fixed in the first followup. However for sort1 and sort2, you must to fill the statistic fields (sort1 and sort2) for your budget with an authorized values categories (like asort1, LOC, DAMAGED, etc.). Then fill 923$a and 924$a with a corresponding authorized value. e.g. budget.sort1 = LOC and 923$a=CART
I cannot test. Conflit : CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/css/staff-global.css Auto-merging installer/data/mysql/updatedatabase.pl Failed to merge in the changes. M. Saby
Created attachment 16014 [details] [review] Bug 7180: Order from staged file improvements Rebased and squashed patch
Patch not working when a matching rule is selected. M. Saby & P. Angot
I've tested and there is a little problem. When i import a file with a free sort 2, it does'nt work.
Created attachment 16505 [details] [review] Bug 7180: FIX getAuthValueDropbox should display the selected value If the no AV match the name/category or if no category is defined, the input field should contain the value.
Patch tested with a sandbox, by Pierre Angot <tredok.pierre@gmail.com>
Created attachment 16511 [details] [review] Bug 7180: FIX getAuthValueDropbox should display the selected value If the no AV match the name/category or if no category is defined, the input field should contain the value. Signed-off-by: Pierre Angot <tredok.pierre@gmail.com>
We tested with other values in syspref (using subfields of unimarc 920 field). We changed our default framework to make visible subfields of 920, so we were able to check in MARC view. In syspref we had : - 920c mapped to sort1. In Koha, for our budget, sort1 was mapped with LOC authorized values. - 920d mapped to sort2. In Koha, for our budget, sort2 was free In our record, we put : - in 920c a value present in LOC. - in 920d a free value, (tested with and without an accent) Both 920c and 920d were imported in DB as sort1 and sort2. We tested with AND without using a matching rule (by ISBN) when staging record, and verything is correct : il the imported record match a record already in Koha, the existing record is used for order, and the values of 920 are used to fill the order. P. Angot & M. Saby
syspref : price: 920$a quantity: 920$q budget_code: 920$b rrp: 010$d discount: 920$t sort1: 920$c sort2: 920$d
Created attachment 16544 [details] Test
Created attachment 16566 [details] [review] Bug 7180: FIX getAuthValueDropbox should display the selected value If the no AV match the name/category or if no category is defined, the input field should contain the value. Signed-off-by: Pierre Angot <tredok.pierre@gmail.com>
Hi Jonthan or Julian - can you please add a test for the new sub C4::Budgets::GetBudgetByCode and switch back to signed off? Hope I didn't miss it!
Created attachment 16613 [details] [review] Bug 7180: Add UT for C4::Budgets::GetBudgetByCode
I don't have the jenkins's DB, so I cannot test if they pass... :-/
Created attachment 19066 [details] [review] Bug 7180: Order from staged file improvements This patch adds: - 1 syspref MarcFieldsToOrder - 1 ajax script acqui/ajax-getauthvaluedropbox.pl - 1 routine C4::Budgets::GetBudgetByCode Before this patch you were not able to order 1 or all the records from your staged file. You were allowed to specify some information ("Import All" and "Accounting details" areas) for the order. With this patch, the previous behaviours still exist. But now you can *select* which records you want to ordered. For these ones you can specify independently informations (Quantity, price, budget, sort1 and sort2). The cherry on the cake is that you can pre-fill these information with values Test plan: 1. Fill the new syspref MarcFieldsToOrder with something like: ==BEGIN== price: 947$c quantity: 969$h budget_code: 922$a rrp: 010$d discount: 969$d sort1: 923$a sort2: 924$a ==END== The empty line at the end is mandatory! The budget (corresponding to your budget_code) can be filled with authorized value categories (statistic 1 and 2). The sort1 and sort2 values can be filled with the an authorized value (of the category previously selected) 2. Choose randomly one or more biblio(s) and fill fields with what is relevant. 3. Export the biblio and import it (with the "Stage MARC records for import" tool). 4. Go on a basket and add an order from a staged file. Select your staged file. 5. Well. Now you can see your biblio (or biblios if your had exported more than one). For each one, informations should be pre-filled with the biblio values. The budget should be selected on the budget corresponding to the budget_code (in the field 922$a) and the "planning values" too (with fields 923$a and 924$a). You can modify these values (or not) and choose a default value for budget and planning values (in the "Accounting details" area). 6. Save and check the prices values. Modify the order and check that budget and sort* are good Prices are calculated following some parameters: if there is no price => listprice = 0 else => - the gstrate value for your order is the gstrate value of the bookseller - discount = if filled : the discount value / 100 else: the discount value of the bookseller - if the bookseller includes tax( List item price includes tax: Yes ) if a discount exists: ecost = price rrp = ecost / ( 1 - discount ) else: # a discount does not exist ecost = price * ( 1 - discount ) rrp = price else # the bookseller does not include tax if a discount exists: ecost = price / ( 1 + gstrate ) rrp = ecost / ( 1 - discount ) else: # a discount does not exist rrp = price / ( 1 + gstrate ) ecost = rrp * ( 1 - discount ) - in all cases: listprice = rrp / currency rate unitprice = ecost total = ecost * quantity 7. Retry with different parameters 8. Check the 'Import all' action still works
Created attachment 19067 [details] [review] Bug 7180: FIX getAuthValueDropbox should display the selected value If the no AV match the name/category or if no category is defined, the input field should contain the value. Signed-off-by: Pierre Angot <tredok.pierre@gmail.com>
Created attachment 19068 [details] [review] Bug 7180: Add UT for C4::Budgets::GetBudgetByCode
Rebased patch. Unit tests from t/db_dependent/Budgets.t pass.
I have tested this, but there are a lot of small and bigger things I am not quite happy with. I really feel like this needs a bit more work. 1) The green color for selected lines seems debatable given the overall look and feel of the Koha GUI. Can you please reconsider? 2) The form has a few design issues - the labels and fields are not properly aligned in a smaller window, making it hard to figure out what goes where. It might fit a bit better into 2 columns and smaller fields for quantity, price and discount. I think the fields should ideally match the formatting on the normal order page. Also required fields should be highlighted and the input should be validated. Like there is no complaint if you don't input the quantity. 3) The label budget is wrong and should be fund instead. Also the list shows funds from inactive budgets. 4) In comparison to the normal form we are missing the option to set the currency in the form. I think it might be safe to assume that all records in that list will be one and the same currency, but it's not clear how to indicate the currency to be used. 5) When you select a fund that is connected to an authorised value list and then undo the selection, you end up with the string "undefined" in the input field. Some ideas: - I think instead of having the full form on each entry it would save a lot of space if we could only display it for selected records and keep the smaller layout for when now records are selected. - There is no option to see more about the record and we are only displaying 245$a it seems, which is not really helpful if you have lots of similar records. Showing a link to the ISBD/MARC preview might help that. - Instead of no_match we should be displaying a nicer text for the librarian. - It might be worth adding an option to pick the right tax rate.
Created attachment 19316 [details] [review] Bug 7180: QA followup Ergonomic improvements: - Remove the green color the selected record. - Use fieldset.rows (and legend). - Use the required css class for quantity inputs. - Replace "budget" with "fund". - fix the "undefined" string - Add a "show MARC" link - replace "no_match" with a text.
(In reply to Katrin Fischer from comment #36) Thanks Katrin for this helpful feedback! > 1) The green color for selected lines seems debatable given the overall look > and feel of the Koha GUI. Can you please reconsider? I removed the green color. > 2) The form has a few design issues - the labels and fields are not properly > aligned in a smaller window, making it hard to figure out what goes where. > It might fit a bit better into 2 columns and smaller fields for quantity, > price and discount. Done, now I use the fieldset with the "rows" css class. > Also required fields should be highlighted and the > input should be validated. Like there is no complaint if you don't input the > quantity. The quantity label now use the "required" css class. I added a javascript check too. > 3) The label budget is wrong and should be fund instead. Fixed. > Also the list shows funds from inactive budgets. Yes but since the record select a value in the dropdown list, the fix is not quite easy. I prefer to do it later. > 4) In comparison to the normal form we are missing the option to set the > currency in the form. I think it might be safe to assume that all records in > that list will be one and the same currency, but it's not clear how to > indicate the currency to be used. The currency used, for all records, is the default currency. Which is the one selected in the "Accounting details" block. I don't know how to make that much clearer. > 5) When you select a fund that is connected to an authorised value list and > then undo the selection, you end up with the string "undefined" in the input > field. Fixed. > Some ideas: > > - I think instead of having the full form on each entry it would save a lot > of space if we could only display it for selected records and keep the > smaller layout for when now records are selected. Hum, I tried something, but I am not a great ergonomist... > - There is no option to see more about the record and we are only displaying > 245$a it seems, which is not really helpful if you have lots of similar > records. Showing a link to the ISBD/MARC preview might help that. Done. > - Instead of no_match we should be displaying a nicer text for the librarian. Done. > - It might be worth adding an option to pick the right tax rate. Mathieu Saby already asked for that (comment 10), but it is not planned for now. Note: This patch is impacted by the bug 10522, it introduces side-effects with this feature.
bug 10522 is pushed to master. This feature can be QAed.
I wanted to test this patch to check if it solved the problem raised in BZ 7129, but it does not apply on sandbox CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/acquisitions.pref Failed to merge in the changes. Mathieu
Created attachment 19587 [details] [review] Bug 7180: Order from staged file improvements This patch adds: - 1 syspref MarcFieldsToOrder - 1 ajax script acqui/ajax-getauthvaluedropbox.pl - 1 routine C4::Budgets::GetBudgetByCode Before this patch you were not able to order 1 or all the records from your staged file. You were allowed to specify some information ("Import All" and "Accounting details" areas) for the order. With this patch, the previous behaviours still exist. But now you can *select* which records you want to ordered. For these ones you can specify independently informations (Quantity, price, budget, sort1 and sort2). The cherry on the cake is that you can pre-fill these information with values Test plan: 1. Fill the new syspref MarcFieldsToOrder with something like: ==BEGIN== price: 947$c quantity: 969$h budget_code: 922$a rrp: 010$d discount: 969$d sort1: 923$a sort2: 924$a ==END== The empty line at the end is mandatory! The budget (corresponding to your budget_code) can be filled with authorized value categories (statistic 1 and 2). The sort1 and sort2 values can be filled with the an authorized value (of the category previously selected) 2. Choose randomly one or more biblio(s) and fill fields with what is relevant. 3. Export the biblio and import it (with the "Stage MARC records for import" tool). 4. Go on a basket and add an order from a staged file. Select your staged file. 5. Well. Now you can see your biblio (or biblios if your had exported more than one). For each one, informations should be pre-filled with the biblio values. The budget should be selected on the budget corresponding to the budget_code (in the field 922$a) and the "planning values" too (with fields 923$a and 924$a). You can modify these values (or not) and choose a default value for budget and planning values (in the "Accounting details" area). 6. Save and check the prices values. Modify the order and check that budget and sort* are good Prices are calculated following some parameters: if there is no price => listprice = 0 else => - the gstrate value for your order is the gstrate value of the bookseller - discount = if filled : the discount value / 100 else: the discount value of the bookseller - if the bookseller includes tax( List item price includes tax: Yes ) if a discount exists: ecost = price rrp = ecost / ( 1 - discount ) else: # a discount does not exist ecost = price * ( 1 - discount ) rrp = price else # the bookseller does not include tax if a discount exists: ecost = price / ( 1 + gstrate ) rrp = ecost / ( 1 - discount ) else: # a discount does not exist rrp = price / ( 1 + gstrate ) ecost = rrp * ( 1 - discount ) - in all cases: listprice = rrp / currency rate unitprice = ecost total = ecost * quantity 7. Retry with different parameters 8. Check the 'Import all' action still works
Created attachment 19588 [details] [review] Bug 7180: FIX getAuthValueDropbox should display the selected value If the no AV match the name/category or if no category is defined, the input field should contain the value. Signed-off-by: Pierre Angot <tredok.pierre@gmail.com>
Created attachment 19589 [details] [review] Bug 7180: Add UT for C4::Budgets::GetBudgetByCode
Created attachment 19590 [details] [review] Bug 7180: QA followup Ergonomic improvements: - Remove the green color the selected record. - Use fieldset.rows (and legend). - Use the required css class for quantity inputs. - Replace "budget" with "fund". - fix the "undefined" string - Add a "show MARC" link - replace "no_match" with a text.
Conflicts on koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/acquisitions.pref, koha-tmpl/intranet-tmpl/prog/en/css/staff-global.css and installer/data/mysql/updatedatabase.pl Easy to fix, switch back to Signed Off
This patch has evolved since I tested it during Hackfest. I am a bit surprised, because when I selected all the records and tried to add them all in a batch ("Save button"), Koha asked me to define a quantity for each book of my imported MARC file. I think "1" should be the default quantity. Maybe I did something wrong? Mathieu
An other remark : currently in master, when you choose to add each document individually, Koha can alert you if there is a duplicate. Apparently we loose this function. Do you think it is a problem ? M. Saby
Hi Mathieu, I think maybe we could have a default of 1 but display that in the quantity field? Having empty default to 1 might be unexpected behaviour and a bit confusing. About the other - yes, I think it's a problem.
What I see is : - in master, I can import a file of 20 records in one click ("Save") - with this patch, I have to define the quantity of each line (20 times!) before clicking "Save" So the process is much longer than before. Currently, there is a default quantity of 1. And I think we should keep that. Mathieu
Hi, I think we are not disagreeing :) I meant as we have a quantity field now, maybe we should put the 1 in there by default, but so it can be changed before you create the orders. But we will need Jonathan to take a look to tell us what can be done - maybe I was wrong in my tests when I assumed that you have to fill in quantity, I don't remember if it created the order when I had left it empty, but I thought it had not. Hm.
Jonathan, I think Mathieu has some good points here for the duplicate check and the quantity - could you take a look?
Created attachment 19641 [details] [review] Bug 7180: The default quantity is 1
(In reply to Katrin Fischer from comment #51) > Jonathan, I think Mathieu has some good points here for the duplicate check > and the quantity - could you take a look? If no quantity is defined, the default value is now 1. Back to Signed Off.
Hello Jonathan, hello Katrin This is working better, but I think small improvements can still be made to improve usability : - Question for Jonathan : I suppose the value in () after the title is the isbn. I found strange to see "Titre01 (2845622201)", as the ISBN for my record is "9782845622203". Why do you "translate" my isbn13 in isbn 10 ? - This page needs a little text explaing to the average librarian how to use it, especially the use of the "Accounting detail bloc" : as the fund, and the 2 planning values can be defined either in each orders or in "Accounting detail bloc", the librarians needs to know what to do, and what behavior to expect if the value for a fund in an order is NOT the same as the one in "Accounting detail bloc". I have made a try with Planning value 1 : the value defined in the order seems to have a greater priority than the value defined in the precise order. If it is normal, we could add a text in the beggining of the page. The following remarks could be the core of further improvements AFTER this patch is pushed, so the page would be the more "librarian friendly" possible. Could you give me an opinion ? - When I click on "check all", my 19 orders are "unfolded", but if I use the "Acounting detail" block at the end of the page, I dont need to put information in each order, but only in the "Acounting detail" at the end of the page. So, maybe the simple fact of checking a order should not unfold the bloc for this order? Maybe we show the order informations in a compact way, in 1 line, just after "( Show MARC | Card )". So there would be no need to unfold the bloc, except if we want to change some of these informations. For example, we could have for each line : "Title01 (2845622201) ( Show MARC | Card ) Quantity: Price: Discount: Fund: Planning value1: Planning value2: " and a little button to change those values if we want Or maybe we could replace the folding/unfolding block with a table structure like that : Selected | Title (show marc) | Qty | Price | Discount | Fund | Planning 1 | Planning 2 With an INPUT in each cell of the table, so we could directly put information into the table I don't know if I am clear enough? Mathieu
Created attachment 19645 [details] proposition of evolution M. Saby Here is a capture of a proposition (made with firebug). - explanation text - all the orders in a table (I kept the LABEL markup, but it could be deleted or just hidden if they are needed for accessibilty) Do you think it would be an UX improvement ? Mathieu
Hm, not sure a table layout without using datatables or similar would work well for bigger lists or smaller screens. The labels in each line might be a bit much in that case.
that's just an idea. of course if we are to change the display, we would need to make a nicer table. But this could be done after this patch is pushed... What I would like to be checked before it is pushed is : - the isbn13 changed to isbn13 after the Title. need a little fix ? - adding a few lines explaining how to use the page (mainly, the "Accounting detail blocs" versus individual orders) - the disappearing of the individual duplicate check. I know some french libraries are using it (we don't). Maybe the fact that it is possible to avoid duplicates by defining an import rule is sufficient? I am not sure of that, because we don't use this feature for the moment here. Is it working well. Do you think all librarians will be fine with that? Mathieu
READ : "- the isbn13 changed to isbn10 after the Title. need a little fix ?"
(In reply to mathieu saby from comment #47) > currently in master, when you choose to add each document individually, Koha > can alert you if there is a duplicate. Apparently we loose this function. Do > you think it is a problem ? The feature was introduced by bug 5782. The needed behavior is for adding a record from an external source. Modifications were made in neworderempty (where the current link "Add order" redirects), so I don't think the behavior was needed for the addorderiso2709.pl page. (In reply to mathieu saby from comment #54) > - Question for Jonathan : I suppose the value in () after the title is the > isbn. I found strange to see "Titre01 (2845622201)", as the ISBN for my > record is "9782845622203". Why do you "translate" my isbn13 in isbn 10 ? I confirm the issue. The biblioitems.isbn field is filled with an isbn 10 by the import tool. It is not in this scope, but it is worth to open a new report. > - This page needs a little text explaing to the average librarian how to use > it, especially the use of the "Accounting detail bloc" : as the fund, and > the 2 planning values can be defined either in each orders or in "Accounting > detail bloc", the librarians needs to know what to do, and what behavior to > expect if the value for a fund in an order is NOT the same as the one in > "Accounting detail bloc". > I have made a try with Planning value 1 : the value defined in the order > seems to have a greater priority than the value defined in the precise > order. If it is normal, we could add a text in the beggining of the page. It is the same behavior with or without these patchs: If a value is given for a biblio, it is kept. Else the "default value" (i.e. the value in the "Accounting detail bloc) is used.
> > It is the same behavior with or without these patchs: If a value is given > for a biblio, it is kept. Else the "default value" (i.e. the value in the > "Accounting detail bloc) is used. Ok, so it may be outside the scope of this patch, and fixed later with a followup, but I still think more explainations are needed, with or without your patch... This is not obvious at all that the Accounting details are "default value" and are not overriding specific values of each line. This kind of behavior can be explained in detail in online help, but some basic information must be provided in the page itself. That's my librarian-point-of-view ;-) IMO, that's the case in some other pages of staff interface, where the use of some parameters is not "obvious" for the average end-user and could deserve 1 or 2 lines of explanations (not to speak of the inventory tool...). If I have time enough I will propose patches. Mathieu
Hm, I agree that we shoudl try to have interfaces as intuitive as possible, but I am not sure adding lots of text to the templates will help the case. I woud rather see something like a tooltip or reference to the help files. When you work in acquisitions every day you might get tired of the explanations and the space they take up pretty soon.
(In reply to Katrin Fischer from comment #61) > Hm, I agree that we shoudl try to have interfaces as intuitive as possible, > but I am not sure adding lots of text to the templates will help the case. I > woud rather see something like a tooltip I like the idea of tooltips. or reference to the help files. Maybe you are true for help files, but we don't use them in my library : they are not very well written (often very verbose), not very well translated in french (and not completely), not always up to date, and more important, sometime they don't fit our workflow, or our Marc flavor :( I know we can edit them. Maybe one day... > When you work in acquisitions every day you might get tired of the > explanations and the space they take up pretty soon. Some colleagues are working with some module everyday. Other colleagues only a few time a year. Some colleagues understand everything very quickly and don't like to see too much explanations, whereas other need a lot of explanations and still make big mistakes... My job as admin is to make it possible to prevent mistakes, improve speed of work, and reduce headaches. Not always easy, but I hope the evolution of Koha will be towards this goal! But that's a general discussion, whose scope is too large for this bug ;-) Mathieu
Jonathan, I am sorry, but this doesn't apply. There are conflicts in koha-tmpl/intranet-tmpl/prog/en/modules/acqui/addorderiso2709.tt that I think would be better if you take a look at. I am adding myself as QA contact - so I can come back to this asap.
Created attachment 20900 [details] [review] Bug 7180: Order from staged file improvements This patch adds: - 1 syspref MarcFieldsToOrder - 1 ajax script acqui/ajax-getauthvaluedropbox.pl - 1 routine C4::Budgets::GetBudgetByCode Before this patch you were not able to order 1 or all the records from your staged file. You were allowed to specify some information ("Import All" and "Accounting details" areas) for the order. With this patch, the previous behaviours still exist. But now you can *select* which records you want to ordered. For these ones you can specify independently informations (Quantity, price, budget, sort1 and sort2). The cherry on the cake is that you can pre-fill these information with values Test plan: 1. Fill the new syspref MarcFieldsToOrder with something like: ==BEGIN== price: 947$c quantity: 969$h budget_code: 922$a rrp: 010$d discount: 969$d sort1: 923$a sort2: 924$a ==END== The empty line at the end is mandatory! The budget (corresponding to your budget_code) can be filled with authorized value categories (statistic 1 and 2). The sort1 and sort2 values can be filled with the an authorized value (of the category previously selected) 2. Choose randomly one or more biblio(s) and fill fields with what is relevant. 3. Export the biblio and import it (with the "Stage MARC records for import" tool). 4. Go on a basket and add an order from a staged file. Select your staged file. 5. Well. Now you can see your biblio (or biblios if your had exported more than one). For each one, informations should be pre-filled with the biblio values. The budget should be selected on the budget corresponding to the budget_code (in the field 922$a) and the "planning values" too (with fields 923$a and 924$a). You can modify these values (or not) and choose a default value for budget and planning values (in the "Accounting details" area). 6. Save and check the prices values. Modify the order and check that budget and sort* are good Prices are calculated following some parameters: if there is no price => listprice = 0 else => - the gstrate value for your order is the gstrate value of the bookseller - discount = if filled : the discount value / 100 else: the discount value of the bookseller - if the bookseller includes tax( List item price includes tax: Yes ) if a discount exists: ecost = price rrp = ecost / ( 1 - discount ) else: # a discount does not exist ecost = price * ( 1 - discount ) rrp = price else # the bookseller does not include tax if a discount exists: ecost = price / ( 1 + gstrate ) rrp = ecost / ( 1 - discount ) else: # a discount does not exist rrp = price / ( 1 + gstrate ) ecost = rrp * ( 1 - discount ) - in all cases: listprice = rrp / currency rate unitprice = ecost total = ecost * quantity 7. Retry with different parameters 8. Check the 'Import all' action still works
Created attachment 20901 [details] [review] Bug 7180: FIX getAuthValueDropbox should display the selected value If the no AV match the name/category or if no category is defined, the input field should contain the value. Signed-off-by: Pierre Angot <tredok.pierre@gmail.com>
Created attachment 20902 [details] [review] Bug 7180: Add UT for C4::Budgets::GetBudgetByCode
Created attachment 20903 [details] [review] Bug 7180: QA followup Ergonomic improvements: - Remove the green color the selected record. - Use fieldset.rows (and legend). - Use the required css class for quantity inputs. - Replace "budget" with "fund". - fix the "undefined" string - Add a "show MARC" link - replace "no_match" with a text.
Created attachment 20904 [details] [review] Bug 7180: The default quantity is 1
Rebased patches!
Currently working on this.
Created attachment 20926 [details] [review] Bug 7180: Follow-up fixing minor issues - Fixed some labels from Budget to Fund - Added the system preference to sysprefs.sql - Fixed some untranslatable Javascript strings
Hi Jonathan, I have started with a code review and fixed some minor issues in a follow-up patch. Could you take a look at the patch please and add your sign-off if the changes are ok? 2 concerns so far: 1) I think the use of the new system preference is not clear from the description, can we give an example maybe? I am also not so happy about the blank line requirement. Not sure, but could it be handled similar to opachiddenitems? 2) I started with regression testing: When I add a new order by searching for a record from the catalog (neworderempty.pl) and choose my fund, the statistic values are not updated. There is a Javascript error on the page: ReferenceError: fetchSortDropbox is not defined fetchSortDropbox(this.form) I tried refreshing the page multiple times. Failed for 2).
Created attachment 20933 [details] [review] Bug 7180: Follow-up fixing minor issues - Fixed some labels from Budget to Fund - Added the system preference to sysprefs.sql - Fixed some untranslatable Javascript strings Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 20934 [details] [review] Bug 7180: Change syspref description - the blank line is now useless - add an example for the syspref value
Created attachment 20935 [details] [review] Bug 7180: Adapt neworderempty code (js function moved). The js function fetchSortDropbox has been deleted in previous patch. The new function is getAuthValueDropbox. Test plan: - link some av to some funds - open an existing order and verify value are correctly filled in the sort1 and sort2 values - create a new order and verify behavior is the same as before Note: This patch generates 2 ajax queries (max) if the budget is linked to 2 av categories for sort1 and sort2. This could be improved using a template plugin for values display on load.
(In reply to Katrin Fischer from comment #72) > Hi Jonathan, Hi Katrin, > I have started with a code review and fixed some minor issues in a follow-up > patch. Could you take a look at the patch please and add your sign-off if > the changes are ok? Yep, thank you for this patch! > 1) I think the use of the new system preference is not clear from the > description, can we give an example maybe? I am also not so happy about the > blank line requirement. Not sure, but could it be handled similar to > opachiddenitems? Yes, I added a followup for that: an example is added to the syspref description and the blank line is useless (added by the script). > 2) I started with regression testing: When I add a new order by searching > for a record from the catalog (neworderempty.pl) and choose my fund, the > statistic values are not updated. There is a Javascript error on the page: > > ReferenceError: fetchSortDropbox is not defined > fetchSortDropbox(this.form) Fixed in the last patch.
Continueing testing, but found something small: FAIL acqui/ajax-getauthvaluedropbox.pl OK pod FAIL forbidden patterns forbidden pattern: Koha is now under the GPLv3 license (line 9)
Sorry, found something bigger: - Create 2 funds - use 2 authorized values for the first, none for the other. - Create an order, switch to first fund first. - The pull downs now show up correctly - don't touch them. - Realize you have picked the wrong fund, change it to the second. - Notice that the authorized value codes remain and the fields are not cleared.
(In reply to Katrin Fischer from comment #78) > Sorry, found something bigger: > > - Create 2 funds - use 2 authorized values for the first, none for the other. > - Create an order, switch to first fund first. > - The pull downs now show up correctly - don't touch them. > - Realize you have picked the wrong fund, change it to the second. > - Notice that the authorized value codes remain and the fields are not > cleared. Yep, good catch, new patch coming.
Created attachment 20978 [details] [review] Bug 7180: Adapt neworderempty code (js function moved). The js function fetchSortDropbox has been deleted in previous patch. The new function is getAuthValueDropbox. Test plan: - link some av to some funds - open an existing order and verify value are correctly filled in the sort1 and sort2 values - create a new order and verify behavior is the same as before Note: This patch generates 2 ajax queries (max) if the budget is linked to 2 av categories for sort1 and sort2. This could be improved using a template plugin for values display on load. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 20979 [details] [review] Bug 7180: FIX License version Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Hi Jonathan, one of the new tests doesn't pass for me: not ok 4 - GetBudgetPeriod(0) returned undef : noactive BudgetPeriod Continueing testing...
Question: The new example has this line: price: 947$a|947$c What does happen in this case? Is the program using the first field that is available in the record?
When selecting a batch to import from, there are lots of warns in the logs: [Wed Sep 11 23:10:05 2013] [error] [client 127.0.0.1] [Wed Sep 11 23:10:05 2013] addorderiso2709.pl: Use of uninitialized value in split at /home/katrin/kohaclone/acqui/addorderiso2709.pl line 505., referer: http://localhost:8080/cgi-bin/koha/acqui/addorderiso2709.pl?booksellerid=11&basketno=33 When I import a single record, by only updating the quantity from 1 to 2, not changing any other value (relying on the fact that it might take the default from the bottom?) - no warning is issued, but the order line is not added to my basket. The logs show lots of unhappiness with me trying that. Should the selections at the bottom be taken into account if you don't select anything for the single record? I think we need to warn that you have to select a budget on the single record or take the global into account?
(In reply to Katrin Fischer from comment #82) > Hi Jonathan, Hi Katrin, > one of the new tests doesn't pass for me: > not ok 4 - GetBudgetPeriod(0) returned undef : noactive BudgetPeriod Will be fixed (there is no transaction for this ut file, we should remove existing funds/budgets). (In reply to Katrin Fischer from comment #83) > Question: The new example has this line: > price: 947$a|947$c > What does happen in this case? Is the program using the first field that is > available in the record? Yes: If 947$a is defined, we take this value, else we take 947$c if defined. (In reply to Katrin Fischer from comment #84) > When selecting a batch to import from, there are lots of warns in the logs: Will be fixed. > When I import a single record, by only updating the quantity from 1 to 2, > not changing any other value (relying on the fact that it might take the > default from the bottom?) - no warning is issued, but the order line is not > added to my basket. The logs show lots of unhappiness with me trying that. Ho, good catch! I don't see the logs but there was a js bug. > Should the selections at the bottom be taken into account if you don't > select anything for the single record? I think we need to warn that you have > to select a budget on the single record or take the global into account? I change the ergonomic of the page in order to be much more easy to understand. I will add 3 tabs: Select to import Items information Default accounting details In the first one, the user has to select the records to import and to define specific values. The second one allow to add some field values for *all* selected record. The third one offers to define "default" values for *all* selected record. In fact a defined value in this tab is used only if no one is filled for the record. I hope it will be more understandable like that.
Created attachment 21004 [details] [review] Bug 7180: QA followup - fix unit tests (use a transaction). - add 3 tabs on the page in order to be more understandable. - fix a warn in logs
Created attachment 21014 [details] [review] Bug 7180 - QA Followup - Correct minor language issues
A couple questions: 1) Should the "Fund" pulldown still be in the "Default accounting details" section, since it's now defined per-record? 2) There doesn't seem to be a link to neworderempty.pl anymore, which is still useful if you are ordering multiple items from a single record. It would make sense to re-add this after the Show MARC / Show Card links.
(In reply to Kyle M Hall from comment #88) > A couple questions: > > 1) Should the "Fund" pulldown still be in the "Default accounting details" > section, since it's now defined per-record? Yes, if you have 100 records and you want to set the same fund for each, it is easy to select 1 value rather than 100 times the same one :) > 2) There doesn't seem to be a link to neworderempty.pl anymore, which is > still useful if you are ordering multiple items from a single record. It > would make sense to re-add this after the Show MARC / Show Card links. Yes, patch coming...
(In reply to Jonathan Druart from comment #89) > (In reply to Kyle M Hall from comment #88) > > A couple questions: > > > > 1) Should the "Fund" pulldown still be in the "Default accounting details" > > section, since it's now defined per-record? > > Yes, if you have 100 records and you want to set the same fund for each, it > is easy to select 1 value rather than 100 times the same one :) And the behaviour is that, if you don't enter anything in one line, it's filled with the default accounting details. Maybe it's worth a line of comment on the screen. Something like: "values entered here will be used for all selected lines that don't have accounting details specified" ? (kyle, you can fix the spelling before joubu's patch :D )
Created attachment 21017 [details] [review] Bug 7180: Restore the "Add order" link
Hello I did not read all the last comments, but I pointed out some UX issue in previous comments (54...) The behavior of this page in current master is not obvious, and my collegues had to take some time to learn how to use the different fields. I would appreciate the enhancement, but I don't want it to become more difficult for them to understand what to do on this page. So adding a few lines in the online help or in the page itself may help ;-) I will test the patch again this week end. Mathieu
Hello Jonathan At first it seemed to work, but with further tests I found some issues : -- issues : When I click on "Card" in front of each line, I have a software error : Empty String at /home/msaby/kohamaster/src/catalogue/showmarc.pl line 64. I also tried to set "rrp: 010$d", without a "price..." line in syspref. I was surprise to see a blank field for "price". Because some price was imported, from $010$d. So the librarian must see it is ok. Otherwise, he will be confused and fill himself manually a price. In fact, the price was imported, but wrongly calculated My vendor had these settings : List item price includes tax: Yes Invoice item price includes tax: Yes Discount: 10.0 % Tax rate: 5.5 % My record had "20.00 EUR" in 010$d. So RRP = 20.00 So the listprice would have been "20 EUR", and the ecost "20 - 2 = 18" Instead, I had in my basket a line with - RRP tax inc = 22.22 - ecost tax inc = 20 - ecost tax ecl = 18.96 So something went wrong. RRP should have been taken from the record. Of course, my main currency is EUR. -- Some questions: Regarding prices : currently, "unitprice" is set to 0 when we fill an order. The unitprice (aka unit cost) is filled when we receive the document. It must be equal to the price actually printed on the invoice joined with the parcel. We have always been said it was not used before in Koha. But with your patch, unitprice is filled when we create the basket. Is it normal or not? Could it be a source of mistake in Koha price calculation? (I am always a bit lost with price management in Koha...) I suppressed mapping of price and rrp in the syspref, and a price was still imported, from $010 I suppose. Is it still hardcoded somehow ? Is it a wanted behavior? -- Some remarks : There are 2 buttons "Save" on the 1st and the 3rd tab. I suppose they do the same action (not a problem for me) ? I like the tabs, and the renaming "DEFAULT accounting details". It could help libs to understand that these values are overridden by the values defined in each line. I think restoring "Add order" link was needed to avoid a functional regression for ppl using the duplicate check feature. But maybe the "Add order" link for each line could be deleted in a further development, because it will be strange from user point of view to have the ability to check a box in front of a line, OR to click on a link. Librarians will need to know clearly that : - they don't need to fill the field "discount" in a line if it is void (because it is defined at vendor level) - they can click on "Add order" to add the order with a duplicate check before (like in current master, but this has never been obvious for us...) I don't konw if it should be written in online help or in the template itself. === Could you check the software error please and the prices questions? Mathieu
Mathieu, thank you for testing. I had done mostly regression tests until now and didn't get to the import configuration. We have changed quite a lot on this patch since I started testing and I feel it would be good if it got another sign off before I continue QA - would you be able to continue working on this with Jonathan? I will still keep my eyes on this waiting for it to be ready. I think having the link and the checkbox should be no big problem - and I really like the link restored - it was something I had been thinking about and was not sure. I think it always imported the price and that this has been improved lately - so maybe we shouldn't change it as it could be seen as a regression? Or maybe we could have it as default mapping in the system preference?
I did not realized it was signed off in fact ;-) Well, let's wait for Jonathan's answer. Maybe I misunderstood the price management, but I want to be sure... Mathieu
To be clear : my main concern is the fact that Koha calculates a RRP price > 20 EUR from a record with "20.00 EUR" in 010$d field. Mathieu
(In reply to mathieu saby from comment #93) > Hello Jonathan > At first it seemed to work, but with further tests I found some issues : > > -- > issues : > When I click on "Card" in front of each line, I have a software error : > Empty String at /home/msaby/kohamaster/src/catalogue/showmarc.pl line 64. I don't reproduce. It is the same 'Card' link as everywhere in Koha. > I also tried to set "rrp: 010$d", without a "price..." line in syspref. > I was surprise to see a blank field for "price". Because some price was > imported, from $010$d. So the librarian must see it is ok. Otherwise, he > will be confused and fill himself manually a price. > In fact, the price was imported, but wrongly calculated > My vendor had these settings : > List item price includes tax: Yes > Invoice item price includes tax: Yes > Discount: 10.0 % > Tax rate: 5.5 % > > My record had "20.00 EUR" in 010$d. So RRP = 20.00 > So the listprice would have been "20 EUR", and the ecost "20 - 2 = 18" > Instead, I had in my basket a line with > - RRP tax inc = 22.22 > - ecost tax inc = 20 > - ecost tax ecl = 18.96 > > So something went wrong. RRP should have been taken from the record. Of > course, my main currency is EUR. In fact, the rrp value is never used. The price value is retrieve from the record and the rrp and ecost values are calculated from the price, quantity, discount and gstrate values. > Regarding prices : currently, "unitprice" is set to 0 when we fill an order. > The unitprice (aka unit cost) is filled when we receive the document. It > must be equal to the price actually printed on the invoice joined with the > parcel. We have always been said it was not used before in Koha. > But with your patch, unitprice is filled when we create the basket. > Is it normal or not? Could it be a source of mistake in Koha price > calculation? I think listprice is never used, it is only used for display. > (I am always a bit lost with price management in Koha...) Everybody is completely lost with price managament in Koha... > Some remarks : > > There are 2 buttons "Save" on the 1st and the 3rd tab. I suppose they do the > same action (not a problem for me) ? It is the same button (outside of tabs).
Created attachment 21117 [details] [review] Bug 7180: Remove useless rrp variable
(In reply to mathieu saby from comment #96) > To be clear : my main concern is the fact that Koha calculates a RRP price > > 20 EUR from a record with "20.00 EUR" in 010$d field. Forget the rrp value, it is not used by this patch. In order to avoid to resubmit all patches, I don't modify the test plan in the first patch. Please read: price: 947$c quantity: 969$h budget_code: 922$a discount: 969$d sort1: 923$a sort2: 924$a
Katrin wants a second signoff on this feature. Back to Needs Signoff
Applying: Bug 7180: Order from staged file improvements Using index info to reconstruct a base tree... M installer/data/mysql/updatedatabase.pl Falling back to patching base and 3-way merge... Auto-merging installer/data/mysql/updatedatabase.pl CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl Patch failed at 0001 Bug 7180: Order from staged file improvements The copy of the patch that failed is found in: /home/christopher/git/koha/.git/rebase-apply/patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort".
(In reply to I'm just a bot from comment #101) > CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl easy to solve.
Created attachment 21462 [details] [review] Bug 7180: Order from staged file improvements This patch adds: - 1 syspref MarcFieldsToOrder - 1 ajax script acqui/ajax-getauthvaluedropbox.pl - 1 routine C4::Budgets::GetBudgetByCode Before this patch you were not able to order 1 or all the records from your staged file. You were allowed to specify some information ("Import All" and "Accounting details" areas) for the order. With this patch, the previous behaviours still exist. But now you can *select* which records you want to ordered. For these ones you can specify independently informations (Quantity, price, budget, sort1 and sort2). The cherry on the cake is that you can pre-fill these information with values Test plan: 1. Fill the new syspref MarcFieldsToOrder with something like: ==BEGIN== price: 947$c quantity: 969$h budget_code: 922$a rrp: 010$d discount: 969$d sort1: 923$a sort2: 924$a ==END== The empty line at the end is mandatory! The budget (corresponding to your budget_code) can be filled with authorized value categories (statistic 1 and 2). The sort1 and sort2 values can be filled with the an authorized value (of the category previously selected) 2. Choose randomly one or more biblio(s) and fill fields with what is relevant. 3. Export the biblio and import it (with the "Stage MARC records for import" tool). 4. Go on a basket and add an order from a staged file. Select your staged file. 5. Well. Now you can see your biblio (or biblios if your had exported more than one). For each one, informations should be pre-filled with the biblio values. The budget should be selected on the budget corresponding to the budget_code (in the field 922$a) and the "planning values" too (with fields 923$a and 924$a). You can modify these values (or not) and choose a default value for budget and planning values (in the "Accounting details" area). 6. Save and check the prices values. Modify the order and check that budget and sort* are good Prices are calculated following some parameters: if there is no price => listprice = 0 else => - the gstrate value for your order is the gstrate value of the bookseller - discount = if filled : the discount value / 100 else: the discount value of the bookseller - if the bookseller includes tax( List item price includes tax: Yes ) if a discount exists: ecost = price rrp = ecost / ( 1 - discount ) else: # a discount does not exist ecost = price * ( 1 - discount ) rrp = price else # the bookseller does not include tax if a discount exists: ecost = price / ( 1 + gstrate ) rrp = ecost / ( 1 - discount ) else: # a discount does not exist rrp = price / ( 1 + gstrate ) ecost = rrp * ( 1 - discount ) - in all cases: listprice = rrp / currency rate unitprice = ecost total = ecost * quantity 7. Retry with different parameters 8. Check the 'Import all' action still works
Created attachment 21463 [details] [review] Bug 7180: FIX getAuthValueDropbox should display the selected value If the no AV match the name/category or if no category is defined, the input field should contain the value. Signed-off-by: Pierre Angot <tredok.pierre@gmail.com>
Created attachment 21464 [details] [review] Bug 7180: Add UT for C4::Budgets::GetBudgetByCode
Created attachment 21465 [details] [review] Bug 7180: QA followup Ergonomic improvements: - Remove the green color the selected record. - Use fieldset.rows (and legend). - Use the required css class for quantity inputs. - Replace "budget" with "fund". - fix the "undefined" string - Add a "show MARC" link - replace "no_match" with a text.
Created attachment 21466 [details] [review] Bug 7180: The default quantity is 1
Created attachment 21467 [details] [review] Bug 7180: Follow-up fixing minor issues - Fixed some labels from Budget to Fund - Added the system preference to sysprefs.sql - Fixed some untranslatable Javascript strings Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 21468 [details] [review] Bug 7180: Change syspref description - the blank line is now useless - add an example for the syspref value
Created attachment 21469 [details] [review] Bug 7180: Adapt neworderempty code (js function moved). The js function fetchSortDropbox has been deleted in previous patch. The new function is getAuthValueDropbox. Test plan: - link some av to some funds - open an existing order and verify value are correctly filled in the sort1 and sort2 values - create a new order and verify behavior is the same as before Note: This patch generates 2 ajax queries (max) if the budget is linked to 2 av categories for sort1 and sort2. This could be improved using a template plugin for values display on load. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 21470 [details] [review] Bug 7180: FIX License version Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 21471 [details] [review] Bug 7180: QA followup - fix unit tests (use a transaction). - add 3 tabs on the page in order to be more understandable. - fix a warn in logs
Created attachment 21472 [details] [review] Bug 7180 - QA Followup - Correct minor language issues
Created attachment 21473 [details] [review] Bug 7180: Restore the "Add order" link
Created attachment 21474 [details] [review] Bug 7180: Remove useless rrp variable
I am testing it on sandbox 3, thank you for the rebasing. Mathieu
Well, Pierre Angot is testing, I let him work on that. But I will make 2 UX remark : - in syspref description or example, we need to know the list of the data that could be mapped with MARC (Not only price, budget_code and quantity as shown on the example, but also sort1,sort2,discount) - in the syspref description, or in the online help, we need to know too which kind of "price" is supposed to be "price" (vendor catalog price? rrp price?). For us who are testing it's not a big issue, but once it is pushed, it could cause great problems if the average librarian understand it the wrong way (and there are maaaany ways to misunderstand Koha price management). M. Saby
Discount are not taken into account. I put "10" in 966$d field and map it with "discount" in syspref. In the table for this record in "addorderiso2709.pl", I see "discount: 10" I saved the form In the resulting order, the discount is "9.00000" So the standard discount for the vendor applied, instead of the discount defined in the record. Is it normal or not (I suppose not)? Mathieu
2 other issues (the same as when I tested some days ago) - Koha is still adding VAT to price taken in the order, even if the vendor is defined with "list price include tax" - a unitcost is still calculated. It must not be. Everywhere in Koha unitcost is the real cost define at receipt, never when ordering. So I failed QA. Mathieu
(In reply to mathieu saby from comment #118) > Discount are not taken into account. > Is it normal or not (I suppose not)? No, there is a bug, the discount value was divided by 100 twice. A followup coming. (In reply to mathieu saby from comment #119) > 2 other issues (the same as when I tested some days ago) > > - Koha is still adding VAT to price taken in the order, even if the vendor > is defined with "list price include tax" with the following: - syspref: price: 947$c - a biblio with 947$c=100 - a supplier with "list price include tax = 0", I got a rrp (include tax) = 94.79 (gst is 5.5%). - a supplier with "list price include tax = 1", I got a rrp (include tax) = 100 It is what is expected. See the test plan, step 6. > - a unitcost is still calculated. It must not be. Everywhere in Koha > unitcost is the real cost define at receipt, never when ordering. The unitcosts can be filled when ordering, so I don't considered that as an issue.
Created attachment 21514 [details] [review] Bug 7180: FIX: discount was divided by 100 twice.
Thank you for corecting the discount > with the following: > - syspref: price: 947$c > - a biblio with 947$c=100 > - a supplier with "list price include tax = 0", I got a rrp (include tax) = > 94.79 (gst is 5.5%). > - a supplier with "list price include tax = 1", I got a rrp (include tax) = > 100 > > It is what is expected. See the test plan, step 6. I continue to find that strange. For me, if the vendor price in 947c is 100 EUR and this price "does NOT include tax", it means the book price is 100 EUR without tax, so 105.5 with tax. I cannot understand it in an other way... For unitprice, it is clearly NOT a big issue (I'm more concerned with taxes management), but for Koha it means "actual price". So how could I know the "actual" price before receipt? Do you know libraries which are using this field when ordering? Here we don't use it, and we were always said it must be left at "0" and will be automaticaly filled by Koha at receipt. Mathieu
Patch applied cleanly, go forth and signoff
Jonathan, I remembered of MT10590 : You thought BZ7180 would be the solution for an price calculation issue, but you had to write an bug 9506 to fix it. So, maybe 7180 causes a regression because it does not take into account 9506 ? I did not check the detail. Mathieu
Applying: Bug 7180: Order from staged file improvements Using index info to reconstruct a base tree... M installer/data/mysql/updatedatabase.pl M koha-tmpl/intranet-tmpl/prog/en/js/acq.js Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/intranet-tmpl/prog/en/js/acq.js Auto-merging installer/data/mysql/updatedatabase.pl CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl Patch failed at 0001 Bug 7180: Order from staged file improvements The copy of the patch that failed is found in: /home/christopher/git/koha/.git/rebase-apply/patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort".
Created attachment 22546 [details] [review] Bug 7180: Order from staged file improvements This patch adds: - 1 syspref MarcFieldsToOrder - 1 ajax script acqui/ajax-getauthvaluedropbox.pl - 1 routine C4::Budgets::GetBudgetByCode Before this patch you were not able to order 1 or all the records from your staged file. You were allowed to specify some information ("Import All" and "Accounting details" areas) for the order. With this patch, the previous behaviours still exist. But now you can *select* which records you want to ordered. For these ones you can specify independently informations (Quantity, price, budget, sort1 and sort2). The cherry on the cake is that you can pre-fill these information with values Test plan: 1. Fill the new syspref MarcFieldsToOrder with something like: ==BEGIN== price: 947$c quantity: 969$h budget_code: 922$a rrp: 010$d discount: 969$d sort1: 923$a sort2: 924$a ==END== The empty line at the end is mandatory! The budget (corresponding to your budget_code) can be filled with authorized value categories (statistic 1 and 2). The sort1 and sort2 values can be filled with the an authorized value (of the category previously selected) 2. Choose randomly one or more biblio(s) and fill fields with what is relevant. 3. Export the biblio and import it (with the "Stage MARC records for import" tool). 4. Go on a basket and add an order from a staged file. Select your staged file. 5. Well. Now you can see your biblio (or biblios if your had exported more than one). For each one, informations should be pre-filled with the biblio values. The budget should be selected on the budget corresponding to the budget_code (in the field 922$a) and the "planning values" too (with fields 923$a and 924$a). You can modify these values (or not) and choose a default value for budget and planning values (in the "Accounting details" area). 6. Save and check the prices values. Modify the order and check that budget and sort* are good Prices are calculated following some parameters: if there is no price => listprice = 0 else => - the gstrate value for your order is the gstrate value of the bookseller - discount = if filled : the discount value / 100 else: the discount value of the bookseller - if the bookseller includes tax( List item price includes tax: Yes ) if a discount exists: ecost = price rrp = ecost / ( 1 - discount ) else: # a discount does not exist ecost = price * ( 1 - discount ) rrp = price else # the bookseller does not include tax if a discount exists: ecost = price / ( 1 + gstrate ) rrp = ecost / ( 1 - discount ) else: # a discount does not exist rrp = price / ( 1 + gstrate ) ecost = rrp * ( 1 - discount ) - in all cases: listprice = rrp / currency rate unitprice = ecost total = ecost * quantity 7. Retry with different parameters 8. Check the 'Import all' action still works
Created attachment 22547 [details] [review] Bug 7180: FIX getAuthValueDropbox should display the selected value If the no AV match the name/category or if no category is defined, the input field should contain the value. Signed-off-by: Pierre Angot <tredok.pierre@gmail.com>
Created attachment 22548 [details] [review] Bug 7180: Add UT for C4::Budgets::GetBudgetByCode
Created attachment 22549 [details] [review] Bug 7180: QA followup Ergonomic improvements: - Remove the green color the selected record. - Use fieldset.rows (and legend). - Use the required css class for quantity inputs. - Replace "budget" with "fund". - fix the "undefined" string - Add a "show MARC" link - replace "no_match" with a text.
Created attachment 22550 [details] [review] Bug 7180: The default quantity is 1
Created attachment 22551 [details] [review] Bug 7180: Follow-up fixing minor issues - Fixed some labels from Budget to Fund - Added the system preference to sysprefs.sql - Fixed some untranslatable Javascript strings Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 22552 [details] [review] Bug 7180: Change syspref description - the blank line is now useless - add an example for the syspref value
Created attachment 22553 [details] [review] Bug 7180: Adapt neworderempty code (js function moved). The js function fetchSortDropbox has been deleted in previous patch. The new function is getAuthValueDropbox. Test plan: - link some av to some funds - open an existing order and verify value are correctly filled in the sort1 and sort2 values - create a new order and verify behavior is the same as before Note: This patch generates 2 ajax queries (max) if the budget is linked to 2 av categories for sort1 and sort2. This could be improved using a template plugin for values display on load. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 22554 [details] [review] Bug 7180: FIX License version Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 22555 [details] [review] Bug 7180: QA followup - fix unit tests (use a transaction). - add 3 tabs on the page in order to be more understandable. - fix a warn in logs
Created attachment 22556 [details] [review] Bug 7180 - QA Followup - Correct minor language issues
Created attachment 22557 [details] [review] Bug 7180: Restore the "Add order" link
Created attachment 22558 [details] [review] Bug 7180: Remove useless rrp variable
Created attachment 22559 [details] [review] Bug 7180: FIX: discount was divided by 100 twice.
I forgot to set status in Failed QA after my last test. Tax management and handling of unitprice must be fixed before this patch can be signed off. Mathieu
(In reply to mathieu saby from comment #122) > Thank you for corecting the discount > > > with the following: > > - syspref: price: 947$c > > - a biblio with 947$c=100 > > - a supplier with "list price include tax = 0", I got a rrp (include tax) = > > 94.79 (gst is 5.5%). > > - a supplier with "list price include tax = 1", I got a rrp (include tax) = > > 100 > > > > It is what is expected. See the test plan, step 6. > > I continue to find that strange. > For me, if the vendor price in 947c is 100 EUR and this price "does NOT > include tax", it means the book price is 100 EUR without tax, so 105.5 with > tax. > I cannot understand it in an other way... The value in 947$c *includes* the tax. The rrp and ecost values are calculated depending on the supplier configuration. I rewrite the algorithm: So, the imported file always contains: price = include tax with the discount discount = the discount ecost : unit price with discount RRP : unit price If the supplier includes tax: ecost = unit price with discount, tax include RRP = unit price, tax include If a discount exists in the file: ecost = price RRP = ecost/(1-discount) If no discount in the file: RRP = price ecost = price*(1-discount [from supplier]) If the supplier does not include tax: ecost = unit price with discount, tax exclude RRP = unit price, tax exclude If a discount exists in the file: ecost = price/(1+tax) RRP = ecost/1-discount If no discount in the file: RRP = price/(1+tax) ecost = RRP(1-discount [from supplier])
Hello I did not test, but made comments on your algorithm. I still have reserves on some points. Mathieu > > The value in 947$c *includes* the tax. > The rrp and ecost values are calculated depending on the supplier > configuration. I did not realize that... So if you have a RRP in 010$d and a price in 947$c, you recalculate RRP from the value of 947$c? Maybe it is useful for old books where the value of 010 is obsolete or written in Francs and not Euros? > > I rewrite the algorithm: > > So, the imported file always contains: Not always ;-) Only records from some vendors will contains price and discount. If those data are not provided, I suppose the behavior should be the same as before (ecost calculated from the value of the field containing RRP : 010$b and 345$d)? By the way, could you tell me if there will be a way to define the syspref to take a type of price (RRP or "price") from 2 fields, like "010 or 345" or "347 or 345". It is important, as DVD prices are only stored in 345 field, which is not important for books. So, currently, DVD are correctly taken into account by Koha, and it must still be the case with your patch. > price = include tax with the discount > discount = the discount > ecost : unit price with discount > RRP : unit price > > If the supplier includes tax: > ecost = unit price with discount, tax include > RRP = unit price, tax include > > If a discount exists in the file: > ecost = price > RRP = ecost/(1-discount) Just "translating" it with numeric values, to be sure I understand: price (in 347 field for ex.) : 94.95 € incl. tax. discount (in some field): 10 ecost : calculated from price : 94.95 € incl. tax. RRP : calculated from price : 105.5 € (=94.95 / (1-0.10)) incl. tax. > > If no discount in the file: > RRP = price > ecost = price*(1-discount [from supplier]) > So when you say "no discount", you mean "no value of discount stored in some field" AND "no discount taken into account in the calculation of price asked by vendor". So you suppose that if, for ex. 969$h is void, 947 will contain a RRP price. Are we sure it should be an absolute rule? Maybe for some vendors, but we can probably imagine a vendor just giving us the discounted price in some field, without giving us the rate of the discount. > > If the supplier does not include tax: > ecost = unit price with discount, tax exclude > RRP = unit price, tax exclude > > If a discount exists in the file: > ecost = price/(1+tax) > RRP = ecost/1-discount price (in 347 field for ex.) : 90 € excl. tax. discount (in some field): 10 ecost : calculated from price : 90 € excl. tax. RRP : calculated from price : 100 € (=94.95 / (1-0.10)) excl. tax. So, your RRP will be an excl. tax. RRP. But in 010$d the record may contain a incl. tax. RRP, which is used currenty by Koha. So you are changing Koha's behavior regarding RRP. That is what surprises me in my tests. And I still don't agree. I think we must be clear on *what* is exactly a RRP in Koha. Could RRP be with or without tax depending on the preferences? I don't like this idea, as RRP means "the price in any bookseller, outside the libraries". So RRP should not be with/without tax depending on library's preferences. I would prefer a rule saying "RRP is always with tax", so if it is calculated from a "vendor price" without tax, tax must be added. And if needed (to avoid rounding errors for ex), we can calcule and store an second version of RRP, without tax, that we may call "RRPe" > > If no discount in the file: > RRP = price/(1+tax) > ecost = RRP(1-discount [from supplier])
Created attachment 24052 [details] [review] Bug 7180: Order from staged file improvements This patch adds: - 1 syspref MarcFieldsToOrder - 1 ajax script acqui/ajax-getauthvaluedropbox.pl - 1 routine C4::Budgets::GetBudgetByCode Before this patch you were not able to order 1 or all the records from your staged file. You were allowed to specify some information ("Import All" and "Accounting details" areas) for the order. With this patch, the previous behaviours still exist. But now you can *select* which records you want to ordered. For these ones you can specify independently informations (Quantity, price, budget, sort1 and sort2). The cherry on the cake is that you can pre-fill these information with values Test plan: 1. Fill the new syspref MarcFieldsToOrder with something like: ==BEGIN== price: 947$c quantity: 969$h budget_code: 922$a rrp: 010$d discount: 969$d sort1: 923$a sort2: 924$a ==END== The empty line at the end is mandatory! The budget (corresponding to your budget_code) can be filled with authorized value categories (statistic 1 and 2). The sort1 and sort2 values can be filled with the an authorized value (of the category previously selected) 2. Choose randomly one or more biblio(s) and fill fields with what is relevant. 3. Export the biblio and import it (with the "Stage MARC records for import" tool). 4. Go on a basket and add an order from a staged file. Select your staged file. 5. Well. Now you can see your biblio (or biblios if your had exported more than one). For each one, informations should be pre-filled with the biblio values. The budget should be selected on the budget corresponding to the budget_code (in the field 922$a) and the "planning values" too (with fields 923$a and 924$a). You can modify these values (or not) and choose a default value for budget and planning values (in the "Accounting details" area). 6. Save and check the prices values. Modify the order and check that budget and sort* are good Prices are calculated following some parameters: if there is no price => listprice = 0 else => - the gstrate value for your order is the gstrate value of the bookseller - discount = if filled : the discount value / 100 else: the discount value of the bookseller - if the bookseller includes tax( List item price includes tax: Yes ) if a discount exists: ecost = price rrp = ecost / ( 1 - discount ) else: # a discount does not exist ecost = price * ( 1 - discount ) rrp = price else # the bookseller does not include tax if a discount exists: ecost = price / ( 1 + gstrate ) rrp = ecost / ( 1 - discount ) else: # a discount does not exist rrp = price / ( 1 + gstrate ) ecost = rrp * ( 1 - discount ) - in all cases: listprice = rrp / currency rate unitprice = ecost total = ecost * quantity 7. Retry with different parameters 8. Check the 'Import all' action still works
Created attachment 24053 [details] [review] Bug 7180: FIX getAuthValueDropbox should display the selected value If the no AV match the name/category or if no category is defined, the input field should contain the value. Signed-off-by: Pierre Angot <tredok.pierre@gmail.com>
Created attachment 24054 [details] [review] Bug 7180: Add UT for C4::Budgets::GetBudgetByCode
Created attachment 24055 [details] [review] Bug 7180: QA followup Ergonomic improvements: - Remove the green color the selected record. - Use fieldset.rows (and legend). - Use the required css class for quantity inputs. - Replace "budget" with "fund". - fix the "undefined" string - Add a "show MARC" link - replace "no_match" with a text.
Created attachment 24056 [details] [review] Bug 7180: The default quantity is 1
Created attachment 24057 [details] [review] Bug 7180: Follow-up fixing minor issues - Fixed some labels from Budget to Fund - Added the system preference to sysprefs.sql - Fixed some untranslatable Javascript strings Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 24058 [details] [review] Bug 7180: Change syspref description - the blank line is now useless - add an example for the syspref value
Created attachment 24059 [details] [review] Bug 7180: Adapt neworderempty code (js function moved). The js function fetchSortDropbox has been deleted in previous patch. The new function is getAuthValueDropbox. Test plan: - link some av to some funds - open an existing order and verify value are correctly filled in the sort1 and sort2 values - create a new order and verify behavior is the same as before Note: This patch generates 2 ajax queries (max) if the budget is linked to 2 av categories for sort1 and sort2. This could be improved using a template plugin for values display on load. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 24060 [details] [review] Bug 7180: FIX License version Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 24061 [details] [review] Bug 7180: QA followup - fix unit tests (use a transaction). - add 3 tabs on the page in order to be more understandable. - fix a warn in logs
Created attachment 24062 [details] [review] Bug 7180 - QA Followup - Correct minor language issues
Created attachment 24063 [details] [review] Bug 7180: Restore the "Add order" link
Created attachment 24064 [details] [review] Bug 7180: Remove useless rrp variable
Created attachment 24065 [details] [review] Bug 7180: FIX: discount was divided by 100 twice.
Patches rebased.
Created attachment 24636 [details] [review] Bug 7180: Order from staged file improvements This patch adds: - 1 syspref MarcFieldsToOrder - 1 ajax script acqui/ajax-getauthvaluedropbox.pl - 1 routine C4::Budgets::GetBudgetByCode Before this patch you were not able to order 1 or all the records from your staged file. You were allowed to specify some information ("Import All" and "Accounting details" areas) for the order. With this patch, the previous behaviours still exist. But now you can *select* which records you want to ordered. For these ones you can specify independently informations (Quantity, price, budget, sort1 and sort2). The cherry on the cake is that you can pre-fill these information with values Test plan: 1. Fill the new syspref MarcFieldsToOrder with something like: ==BEGIN== price: 947$c quantity: 969$h budget_code: 922$a rrp: 010$d discount: 969$d sort1: 923$a sort2: 924$a ==END== The empty line at the end is mandatory! The budget (corresponding to your budget_code) can be filled with authorized value categories (statistic 1 and 2). The sort1 and sort2 values can be filled with the an authorized value (of the category previously selected) 2. Choose randomly one or more biblio(s) and fill fields with what is relevant. 3. Export the biblio and import it (with the "Stage MARC records for import" tool). 4. Go on a basket and add an order from a staged file. Select your staged file. 5. Well. Now you can see your biblio (or biblios if your had exported more than one). For each one, informations should be pre-filled with the biblio values. The budget should be selected on the budget corresponding to the budget_code (in the field 922$a) and the "planning values" too (with fields 923$a and 924$a). You can modify these values (or not) and choose a default value for budget and planning values (in the "Accounting details" area). 6. Save and check the prices values. Modify the order and check that budget and sort* are good Prices are calculated following some parameters: if there is no price => listprice = 0 else => - the gstrate value for your order is the gstrate value of the bookseller - discount = if filled : the discount value / 100 else: the discount value of the bookseller - if the bookseller includes tax( List item price includes tax: Yes ) if a discount exists: ecost = price rrp = ecost / ( 1 - discount ) else: # a discount does not exist ecost = price * ( 1 - discount ) rrp = price else # the bookseller does not include tax if a discount exists: ecost = price / ( 1 + gstrate ) rrp = ecost / ( 1 - discount ) else: # a discount does not exist rrp = price / ( 1 + gstrate ) ecost = rrp * ( 1 - discount ) - in all cases: listprice = rrp / currency rate unitprice = ecost total = ecost * quantity 7. Retry with different parameters 8. Check the 'Import all' action still works
Created attachment 24637 [details] [review] Bug 7180: FIX getAuthValueDropbox should display the selected value If the no AV match the name/category or if no category is defined, the input field should contain the value. Signed-off-by: Pierre Angot <tredok.pierre@gmail.com>
Created attachment 24638 [details] [review] Bug 7180: Add UT for C4::Budgets::GetBudgetByCode
Created attachment 24639 [details] [review] Bug 7180: QA followup Ergonomic improvements: - Remove the green color the selected record. - Use fieldset.rows (and legend). - Use the required css class for quantity inputs. - Replace "budget" with "fund". - fix the "undefined" string - Add a "show MARC" link - replace "no_match" with a text.
Created attachment 24640 [details] [review] Bug 7180: The default quantity is 1
Created attachment 24641 [details] [review] Bug 7180: Follow-up fixing minor issues - Fixed some labels from Budget to Fund - Added the system preference to sysprefs.sql - Fixed some untranslatable Javascript strings Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 24642 [details] [review] Bug 7180: Change syspref description - the blank line is now useless - add an example for the syspref value
Created attachment 24643 [details] [review] Bug 7180: Adapt neworderempty code (js function moved). The js function fetchSortDropbox has been deleted in previous patch. The new function is getAuthValueDropbox. Test plan: - link some av to some funds - open an existing order and verify value are correctly filled in the sort1 and sort2 values - create a new order and verify behavior is the same as before Note: This patch generates 2 ajax queries (max) if the budget is linked to 2 av categories for sort1 and sort2. This could be improved using a template plugin for values display on load. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 24644 [details] [review] Bug 7180: FIX License version Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 24645 [details] [review] Bug 7180: QA followup - fix unit tests (use a transaction). - add 3 tabs on the page in order to be more understandable. - fix a warn in logs
Created attachment 24646 [details] [review] Bug 7180 - QA Followup - Correct minor language issues
Created attachment 24647 [details] [review] Bug 7180: Restore the "Add order" link
Created attachment 24648 [details] [review] Bug 7180: Remove useless rrp variable
Created attachment 24649 [details] [review] Bug 7180: FIX: discount was divided by 100 twice.
Created attachment 24708 [details] [review] Bug 7180: Order from staged file improvements This patch adds: - 1 syspref MarcFieldsToOrder - 1 ajax script acqui/ajax-getauthvaluedropbox.pl - 1 routine C4::Budgets::GetBudgetByCode Before this patch you were not able to order 1 or all the records from your staged file. You were allowed to specify some information ("Import All" and "Accounting details" areas) for the order. With this patch, the previous behaviours still exist. But now you can *select* which records you want to ordered. For these ones you can specify independently informations (Quantity, price, budget, sort1 and sort2). The cherry on the cake is that you can pre-fill these information with values Test plan: 1. Fill the new syspref MarcFieldsToOrder with something like: ==BEGIN== price: 947$c quantity: 969$h budget_code: 922$a rrp: 010$d discount: 969$d sort1: 923$a sort2: 924$a ==END== The empty line at the end is mandatory! The budget (corresponding to your budget_code) can be filled with authorized value categories (statistic 1 and 2). The sort1 and sort2 values can be filled with the an authorized value (of the category previously selected) 2. Choose randomly one or more biblio(s) and fill fields with what is relevant. 3. Export the biblio and import it (with the "Stage MARC records for import" tool). 4. Go on a basket and add an order from a staged file. Select your staged file. 5. Well. Now you can see your biblio (or biblios if your had exported more than one). For each one, informations should be pre-filled with the biblio values. The budget should be selected on the budget corresponding to the budget_code (in the field 922$a) and the "planning values" too (with fields 923$a and 924$a). You can modify these values (or not) and choose a default value for budget and planning values (in the "Accounting details" area). 6. Save and check the prices values. Modify the order and check that budget and sort* are good Prices are calculated following some parameters: if there is no price => listprice = 0 else => - the gstrate value for your order is the gstrate value of the bookseller - discount = if filled : the discount value / 100 else: the discount value of the bookseller - if the bookseller includes tax( List item price includes tax: Yes ) if a discount exists: ecost = price rrp = ecost / ( 1 - discount ) else: # a discount does not exist ecost = price * ( 1 - discount ) rrp = price else # the bookseller does not include tax if a discount exists: ecost = price / ( 1 + gstrate ) rrp = ecost / ( 1 - discount ) else: # a discount does not exist rrp = price / ( 1 + gstrate ) ecost = rrp * ( 1 - discount ) - in all cases: listprice = rrp / currency rate unitprice = ecost total = ecost * quantity 7. Retry with different parameters 8. Check the 'Import all' action still works Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 24709 [details] [review] Bug 7180: FIX getAuthValueDropbox should display the selected value If the no AV match the name/category or if no category is defined, the input field should contain the value. Signed-off-by: Pierre Angot <tredok.pierre@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 24710 [details] [review] Bug 7180: Add UT for C4::Budgets::GetBudgetByCode Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 24711 [details] [review] Bug 7180: QA followup Ergonomic improvements: - Remove the green color the selected record. - Use fieldset.rows (and legend). - Use the required css class for quantity inputs. - Replace "budget" with "fund". - fix the "undefined" string - Add a "show MARC" link - replace "no_match" with a text. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 24712 [details] [review] Bug 7180: The default quantity is 1 Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 24713 [details] [review] Bug 7180: Follow-up fixing minor issues - Fixed some labels from Budget to Fund - Added the system preference to sysprefs.sql - Fixed some untranslatable Javascript strings Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 24714 [details] [review] Bug 7180: Change syspref description - the blank line is now useless - add an example for the syspref value Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 24715 [details] [review] Bug 7180: Adapt neworderempty code (js function moved). The js function fetchSortDropbox has been deleted in previous patch. The new function is getAuthValueDropbox. Test plan: - link some av to some funds - open an existing order and verify value are correctly filled in the sort1 and sort2 values - create a new order and verify behavior is the same as before Note: This patch generates 2 ajax queries (max) if the budget is linked to 2 av categories for sort1 and sort2. This could be improved using a template plugin for values display on load. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 24716 [details] [review] Bug 7180: FIX License version Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 24717 [details] [review] Bug 7180: QA followup - fix unit tests (use a transaction). - add 3 tabs on the page in order to be more understandable. - fix a warn in logs Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 24718 [details] [review] Bug 7180 - QA Followup - Correct minor language issues Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 24719 [details] [review] Bug 7180: Restore the "Add order" link Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 24720 [details] [review] Bug 7180: Remove useless rrp variable Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 24721 [details] [review] Bug 7180: FIX: discount was divided by 100 twice. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 25908 [details] [review] Bug 7180: fetch_sort_dropbox.pl is not in use anymore Test plan: git grep fetch_sort_dropbox should return no result.
Hi Jonathan, could you rebase this please? I would like to try and tackle this beginning of next week.
Created attachment 26950 [details] [review] Bug 7180: Order from staged file improvements This patch adds: - 1 syspref MarcFieldsToOrder - 1 ajax script acqui/ajax-getauthvaluedropbox.pl - 1 routine C4::Budgets::GetBudgetByCode Before this patch you were not able to order 1 or all the records from your staged file. You were allowed to specify some information ("Import All" and "Accounting details" areas) for the order. With this patch, the previous behaviours still exist. But now you can *select* which records you want to ordered. For these ones you can specify independently informations (Quantity, price, budget, sort1 and sort2). The cherry on the cake is that you can pre-fill these information with values Test plan: 1. Fill the new syspref MarcFieldsToOrder with something like: ==BEGIN== price: 947$c quantity: 969$h budget_code: 922$a rrp: 010$d discount: 969$d sort1: 923$a sort2: 924$a ==END== The empty line at the end is mandatory! The budget (corresponding to your budget_code) can be filled with authorized value categories (statistic 1 and 2). The sort1 and sort2 values can be filled with the an authorized value (of the category previously selected) 2. Choose randomly one or more biblio(s) and fill fields with what is relevant. 3. Export the biblio and import it (with the "Stage MARC records for import" tool). 4. Go on a basket and add an order from a staged file. Select your staged file. 5. Well. Now you can see your biblio (or biblios if your had exported more than one). For each one, informations should be pre-filled with the biblio values. The budget should be selected on the budget corresponding to the budget_code (in the field 922$a) and the "planning values" too (with fields 923$a and 924$a). You can modify these values (or not) and choose a default value for budget and planning values (in the "Accounting details" area). 6. Save and check the prices values. Modify the order and check that budget and sort* are good Prices are calculated following some parameters: if there is no price => listprice = 0 else => - the gstrate value for your order is the gstrate value of the bookseller - discount = if filled : the discount value / 100 else: the discount value of the bookseller - if the bookseller includes tax( List item price includes tax: Yes ) if a discount exists: ecost = price rrp = ecost / ( 1 - discount ) else: # a discount does not exist ecost = price * ( 1 - discount ) rrp = price else # the bookseller does not include tax if a discount exists: ecost = price / ( 1 + gstrate ) rrp = ecost / ( 1 - discount ) else: # a discount does not exist rrp = price / ( 1 + gstrate ) ecost = rrp * ( 1 - discount ) - in all cases: listprice = rrp / currency rate unitprice = ecost total = ecost * quantity 7. Retry with different parameters 8. Check the 'Import all' action still works Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 26951 [details] [review] Bug 7180: FIX getAuthValueDropbox should display the selected value If the no AV match the name/category or if no category is defined, the input field should contain the value. Signed-off-by: Pierre Angot <tredok.pierre@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 26952 [details] [review] Bug 7180: Add UT for C4::Budgets::GetBudgetByCode Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 26953 [details] [review] Bug 7180: QA followup Ergonomic improvements: - Remove the green color the selected record. - Use fieldset.rows (and legend). - Use the required css class for quantity inputs. - Replace "budget" with "fund". - fix the "undefined" string - Add a "show MARC" link - replace "no_match" with a text. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 26954 [details] [review] Bug 7180: The default quantity is 1 Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 26955 [details] [review] Bug 7180: Follow-up fixing minor issues - Fixed some labels from Budget to Fund - Added the system preference to sysprefs.sql - Fixed some untranslatable Javascript strings Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 26956 [details] [review] Bug 7180: Change syspref description - the blank line is now useless - add an example for the syspref value Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 26957 [details] [review] Bug 7180: Adapt neworderempty code (js function moved). The js function fetchSortDropbox has been deleted in previous patch. The new function is getAuthValueDropbox. Test plan: - link some av to some funds - open an existing order and verify value are correctly filled in the sort1 and sort2 values - create a new order and verify behavior is the same as before Note: This patch generates 2 ajax queries (max) if the budget is linked to 2 av categories for sort1 and sort2. This could be improved using a template plugin for values display on load. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 26958 [details] [review] Bug 7180: FIX License version Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 26959 [details] [review] Bug 7180: QA followup - fix unit tests (use a transaction). - add 3 tabs on the page in order to be more understandable. - fix a warn in logs Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 26960 [details] [review] Bug 7180 - QA Followup - Correct minor language issues Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 26961 [details] [review] Bug 7180: Restore the "Add order" link Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 26962 [details] [review] Bug 7180: Remove useless rrp variable Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 26963 [details] [review] Bug 7180: FIX: discount was divided by 100 twice. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 26964 [details] [review] Bug 7180: fetch_sort_dropbox.pl is not in use anymore Test plan: git grep fetch_sort_dropbox should return no result.
Patches rebased against master.
Starting on this now... wish me luck? :)
Created attachment 27128 [details] New staged order page A screenshot - testing comments later.
** Automated tests and QA script ** Passes all tests in t, xt, and t/db_dependent/Budgets.t t/db_dependent/Acquisition.t t/db_dependent/Acquisition/ Passes QA script. New GetBudgetByCode() covered by tests in t/db_dependent/Budgets.t. ** Code review ** + <span class="checkall"><a id="checkAll" href="#">Check All</a></span> + <span class="uncheckall"><a id="unCheckAll" href="#">Uncheck All</a></span> BUG (trivial) Capitalization, please fix in a follow up. + <label for="sort1_record_[% biblio.import_record_id %]">Planning value 1: </label> + <label for="sort2_record_[% biblio.import_record_id %]">Planning value 2: </label> + <label for="sort2">Planning value 2: </label> BUG (trivial) Should be "Statistic 1" "Statistic 2" to match neworderempty.pl, please fix in a follow up. ** Database update Adds a new system preference MarcFieldsToOrder, inludes changes to sysprefs.sql, database update and .pref file. syspref.sql and database update are identical. acquisitions.pref + - "For example:<br/>price: 947$a|947$c<br/>quantity: 969$h<br/>budget_code: 922$a<rb/>" BUG (minor) Fix </rb>. ** Regression testing ** * Current functionality (some notes for comparision) File list / addorderiso2709.pl - List of files that include 'staged' records is shown, details include file name, comments, status, and staged date - Bug: # Bibs is not shown (bug 9063) List of staged records / addorderiso2709.pl - Information in the list only includes title, isbn/issn, there is no way to preview a record - Clicking on 'Add order' will import the record into the catalog and set the status to 'imported'. - You can choose to import all records at once, using the Accounting details section. Options are: currency, budget (should be fund), notes, planning value1, planning value2 - Bug: Fund list shows all funds, including inactive funds (bug 10302) - Bug: Statistic values don't reload on selecting a fund, only when refreshing the page New order form / neworderempty.pl - Replacing ajax-getauthvaluedropbox.pl with fetch_sort_dropbox.pl Used on neworderempty to show the correct values for Statistic 1 + 2 on selecting a fund. * Retested with patches applied File list / addorderiso2709.pl - List of files that include 'staged' records is shown, details include file name, comments, status, and staged date -> Still ok. - Bug: # Bibs is not shown (bug 9063) -> Still broken. List of staged records / addorderiso2709.pl - Information in the list only includes title, isbn/issn, there is no way to preview a record -> MARC preview and Card preview are added BUG (normal): clicking on the 'Card' view, results in a 404 error page. - Clicking on 'Add order' will import the record into the catalog and set the status to 'imported'. -> Still works the same. - You can choose to import all records at once, using the Accounting details section. Options are: currency, budget (should be fund), notes, planning value1, planning value2 -> Options are on a new tab: - Bug: Fund list shows all funds, including inactive funds (bug 10302) -> Still broken. - Bug: Statistic values don't reload on selecting a fund, only when refreshing the page -> BUG (normal): Switching funds works now, but when one of the Statistic values is a free text (no linked authorized value) an 'undefined' text appears in the input field. Only happens on the Default accounting details tab, neworderempty and the details for single orders are ok. -> BUG (normal): Importing and leaving the statistic value free text field empty, will write an 'undefined' into aqorders.sort* New order form / neworderempty.pl - Replacing ajax-getauthvaluedropbox.pl with fetch_sort_dropbox.pl Used on neworderempty to show the correct values for Statistic 1 + 2 on selecting a fund. -> Works. ** New functionality ** * GUI - BUG (trivial): The border of the order details section shows without having the details open (see screenshot). Please take a look. - SUGGESTION: Remove some of the 'bold', maybe only keep the titles bold, but not the links? - SUGGESTION: Change links to read: ( MARC | Card | Add order ) instead of ( Show MARC | Card ; Add order ) - BUG (trivial): Change formatting of the discount field to match formatting on neworderempty.pl, bit shorter and adding % - BUG (minor): Pull discount value from the vendor. I was not sure if the vendor discount would be used now or not, this way it would be clear. - BUG/SUGGESTION Add a note on the Default accounting details tab to clarify when these settings will be taken into account - only for all/selected titles? Similar to the Item information tab. - BUG (major): In my tests I added 12.00 as the price for one record, and 10 as percentage for the discount. When looking at the record in the basket, the list price shows as: -1.21 * MarcFieldsToOrder - Still needs to be tested! QUESTION Are there other fields available not listed in the example? Could we improve the description to give a complete list? price: 947$a|947$c quantity: 969$h budget_code: 922$a
Created attachment 27131 [details] [review] Bug 7180: QA followup 2 This patch takes into account all QA failures raised in comment 207.
(In reply to Katrin Fischer from comment #207) > + <span class="checkall"><a id="checkAll" href="#">Check All</a></span> > + <span class="uncheckall"><a id="unCheckAll" href="#">Uncheck All</a></span> > BUG (trivial) Capitalization, please fix in a follow up. Fixed. > BUG (trivial) Should be "Statistic 1" "Statistic 2" to match > neworderempty.pl, > please fix in a follow up. Fixed. > acquisitions.pref > + - "For example:<br/>price: 947$a|947$c<br/>quantity: > 969$h<br/>budget_code: 922$a<rb/>" > BUG (minor) Fix </rb>. Fixed. > - Bug: # Bibs is not shown (bug 9063) > -> Still broken. I am going to provide a patch for bug 9063. > List of staged records / addorderiso2709.pl > - Information in the list only includes title, isbn/issn, there is > no way to preview a record > -> MARC preview and Card preview are added > BUG (normal): clicking on the 'Card' view, results in a 404 error page. I cannot reproduce, the link is catalogue/showmarc.pl?viewas=card&id=230 and the preview works. > - Bug: Fund list shows all funds, including inactive funds (bug 10302) > -> Still broken. See bug 10875 for a patch. > -> BUG (normal): Switching funds > works now, but when one of the Statistic values is a free text > (no linked authorized value) an 'undefined' text appears in the > input field. Only happens on the Default accounting details tab, > neworderempty and the details for single orders are ok. Fixed. > -> BUG (normal): Importing and leaving the statistic value free > text field empty, will write an 'undefined' into aqorders.sort* I cannot reproduce. Maybe it is fixed with previous fix. > ** New functionality ** > > * GUI > - BUG (trivial): The border of the order details section shows without > having the details open (see screenshot). Please take a look. See comment 36: "- I think instead of having the full form on each entry it would save a lot of space if we could only display it for selected records and keep the smaller layout for when now records are selected." > - SUGGESTION: Remove some of the 'bold', maybe only keep the titles > bold, but not the links? Done. > - SUGGESTION: Change links to read: ( MARC | Card | Add order ) instead > of ( Show MARC | Card ; Add order ) Done. > - BUG (trivial): Change formatting of the discount field to match > formatting on neworderempty.pl, bit shorter and adding % Done. > - BUG (minor): Pull discount value from the vendor. I was not sure > if the vendor discount would be used now or not, this way it would > be clear. I am not sure to understand, could you detail? > - BUG/SUGGESTION Add a note on the Default accounting details tab > to clarify when these settings will be taken into account - only > for all/selected titles? Similar to the Item information tab. I tried to fix that... > - BUG (major): In my tests I added 12.00 as the price for one record, > and 10 as percentage for the discount. When looking at the record > in the basket, the list price shows as: -1.21 Fixed! The discount was divided by 100 if > 100. Which is wrong! It should be divided by 100 if > 1. > QUESTION Are there other fields available not listed in the example? > Could we improve the description to give a complete list? Done.
Hi Jonathan, the card views are all broken for me, tried several, my link is: http://localhost:8080/cgi-bin/koha/catalogue/showmarc.pl?viewas=card&id=644 Could it be MARC flavour specific? I remember we had problems with the card view in the past where this seemed to be part of the problem. > - BUG (minor): Pull discount value from the vendor. I was not sure > if the vendor discount would be used now or not, this way it would > be clear. I am not sure to understand, could you detail? The discount fields are all empty - so I was not sure what to do - put 0 in it to disable the vendor discount? Do I have to enter my normal discount? It would be a bit more clear when the dicount fields were prefilled with the standard dicsount from the vendor. > ** New functionality ** > > * GUI > - BUG (trivial): The border of the order details section shows without > having the details open (see screenshot). Please take a look. See comment 36: "- I think instead of having the full form on each entry it would save a lot of space if we could only display it for selected records and keep the smaller layout for when now records are selected." I still think that :) It's just cosmetig, I think the doubled up border for collapsed entries looks a bit strange, it's all ok, once you open the entry. But this can totally be fixed later - trivial :)
Resetting failed for now, until we can figure out what's wrong with the card view.
Created attachment 27136 [details] [review] Bug 7180: QA followup 2 This patch takes into account all QA failures raised in comment 207.
(In reply to Katrin Fischer from comment #210) > Hi Jonathan, > > the card views are all broken for me, tried several, my link is: > http://localhost:8080/cgi-bin/koha/catalogue/showmarc.pl?viewas=card&id=644 > > Could it be MARC flavour specific? I remember we had problems with the card > view in the past where this seemed to be part of the problem. Looking at the code (catalogue/showmarc.pl), the redirect to 404 is done if new_from_usmarc or GetMarcBiblio returns something undefined. It should not append. if ($importid) { my ($marc) = GetImportRecordMarc($importid); $record = MARC::Record->new_from_usmarc($marc); } else { $record =GetMarcBiblio($biblionumber); } if(!ref $record) { print $input->redirect("/cgi-bin/koha/errors/404.pl"); exit; } Could you attach the imported record to this report? > > - BUG (minor): Pull discount value from the vendor. I was not sure > > if the vendor discount would be used now or not, this way it would > > be clear. > > I am not sure to understand, could you detail? > > The discount fields are all empty - so I was not sure what to do - put 0 in > it to disable the vendor discount? Do I have to enter my normal discount? It > would be a bit more clear when the dicount fields were prefilled with the > standard dicsount from the vendor. Hum, it could be a good idea. But if a user wants to import a batch with a specific discount, (s)he will have to remove values manually and add the default one in the accounting defails tab. > > * GUI > > - BUG (trivial): The border of the order details section shows without > > having the details open (see screenshot). Please take a look. > > See comment 36: "- I think instead of having the full form on each entry it > would save a lot of space if we could only display it for selected records > and keep the smaller layout for when now records are selected." > > I still think that :) It's just cosmetig, I think the doubled up border for > collapsed entries looks a bit strange, it's all ok, once you open the entry. > But this can totally be fixed later - trivial :) Ho yes sorry, I read that too quickly. Fixed (as I can).
Hi Jonathan, I noticed that some of my cards displayed, but the wrong record - I took a look at the code and I think id should be importid in the link? ... http://localhost:8080/cgi-bin/koha/catalogue/showmarc.pl?viewas=card&importid=651 Works for me, while http://localhost:8080/cgi-bin/koha/catalogue/showmarc.pl?viewas=card&id=651 shows the wrong record. And you are right about the discount, for usability it's better like it is now. What about adding a 'grey' note behind the field? 'If empty, discount rate from vendor will be used'? (or maybe we can think of something shorter) Can you check my solution from above? It's too late now for doing acquisitions... I will come back to this tomorrow :)
Created attachment 27160 [details] [review] Bug 7180: QA followup 3 1/ showmarc.pl takes the importid for staged files. 2/ small note to indicate that the vendor's discount will be used if no discount is filled.
*** Bug 10501 has been marked as a duplicate of this bug. ***
Hi Jonathan, we need an update to this to account for the changes done by 7180 - patches conflict on current master. I will continue testing the new patches on top of my old branch for now.
Created attachment 27325 [details] New staged order page - with improvements
Hi Jonathan, we are almost there, only 2 problems found + testing the internal note/vendor note after rebase. If you can also rebase bug 10875 I will include it in my next test. ** Automated tests and QA script ** Passes all tests in t, xt, and t/db_dependent/Budgets.t t/db_dependent/Acquisition.t t/db_dependent/Acquisition/ Passes QA script. New GetBudgetByCode() covered by tests in t/db_dependent/Budgets.t. ** Code review ** No Capitalization problems found. No translation problems found. Terminology: Planning value 1 and 2 should be Statistic 1 and 2. BUG (trivial) Fixed for picking individual records, but still appears on the default accounting details tab. ** Database update Adds a new system preference MarcFieldsToOrder, inludes changes to sysprefs.sql, database update and .pref file. syspref.sql and database update are identical. ** Regression testing ** Comparing current functionality with functionality after applying the patches. File list - addorderiso2709.pl - List of files that include 'staged' records is shown, details include file name, comments, status, and staged date - Bug: # Bibs is not shown (bug 9063) * No regresion, still works like it should. New patch for bug 9063 also fixes the # Bibs problem. List of staged records - addorderiso2709.pl - Information in the list only includes title, isbn/issn, there is no way to preview a record * Improved: MARC preview and card preview - Clicking on 'Add order' will import the record into the catalog and set the status to 'imported'. * No regression, status is correctly set. - You can choose to import all records at once, using the Accounting details section. Options are: currency, budget (should be fund), notes, planning value1, planning value2 * Fund now correctly labelled. Options are now available for batch operation and for individual orders. Options include: - Individual order: quantity, price, discount, fund, statistic 1 + 2 - Batch: currency, fund, notes, planning value 1 + 2 No regression found. - Bug: Fund list shows all funds, including inactive funds (bug 10302) * No regression. Patches for bug 10302 currently don't apply. - Bug: Statistic values don't reload on selecting a fund, only when refreshing the page * BUG (normal): On the default accounting details page the refreshing doesn't work. For individual records it works alright, also on the neworderempty.pl page. New order form / neworderempty.pl - Replacing ajax-getauthvaluedropbox.pl with fetch_sort_dropbox.pl Used on neworderempty to show the correct values for Statistic 1 + 2 on selecting a fund. * No regression, works like it should. ** MarcFieldsToOrder New system preference allows to include acq data in the uploaded MARC file. System preference settings used for testing: price: 933$a|933$c quantity: 933$q sort1: 933$1 sort2: 933$2 discount: 933$d budget_code: 933$b Lines included in my records: 933 $a 12.00 $q 12 $b BOOKS1310 $d 5 $1 sort1 $2 sort2 933 $c 12.99 $a 9.00 $q 12 $b BOOKS1310 $d 0.05 $1 sort1 $2 sort2 Looking at the individual details for the first 2 records revealed: all settings have been imported correctly. Discount should be given as $, so 0.05 is way too low, 5 is better.
Created attachment 27408 [details] [review] Bug 7180: Order from staged file improvements This patch adds: - 1 syspref MarcFieldsToOrder - 1 ajax script acqui/ajax-getauthvaluedropbox.pl - 1 routine C4::Budgets::GetBudgetByCode Before this patch you were not able to order 1 or all the records from your staged file. You were allowed to specify some information ("Import All" and "Accounting details" areas) for the order. With this patch, the previous behaviours still exist. But now you can *select* which records you want to ordered. For these ones you can specify independently informations (Quantity, price, budget, sort1 and sort2). The cherry on the cake is that you can pre-fill these information with values Test plan: 1. Fill the new syspref MarcFieldsToOrder with something like: ==BEGIN== price: 947$c quantity: 969$h budget_code: 922$a rrp: 010$d discount: 969$d sort1: 923$a sort2: 924$a ==END== The empty line at the end is mandatory! The budget (corresponding to your budget_code) can be filled with authorized value categories (statistic 1 and 2). The sort1 and sort2 values can be filled with the an authorized value (of the category previously selected) 2. Choose randomly one or more biblio(s) and fill fields with what is relevant. 3. Export the biblio and import it (with the "Stage MARC records for import" tool). 4. Go on a basket and add an order from a staged file. Select your staged file. 5. Well. Now you can see your biblio (or biblios if your had exported more than one). For each one, informations should be pre-filled with the biblio values. The budget should be selected on the budget corresponding to the budget_code (in the field 922$a) and the "planning values" too (with fields 923$a and 924$a). You can modify these values (or not) and choose a default value for budget and planning values (in the "Accounting details" area). 6. Save and check the prices values. Modify the order and check that budget and sort* are good Prices are calculated following some parameters: if there is no price => listprice = 0 else => - the gstrate value for your order is the gstrate value of the bookseller - discount = if filled : the discount value / 100 else: the discount value of the bookseller - if the bookseller includes tax( List item price includes tax: Yes ) if a discount exists: ecost = price rrp = ecost / ( 1 - discount ) else: # a discount does not exist ecost = price * ( 1 - discount ) rrp = price else # the bookseller does not include tax if a discount exists: ecost = price / ( 1 + gstrate ) rrp = ecost / ( 1 - discount ) else: # a discount does not exist rrp = price / ( 1 + gstrate ) ecost = rrp * ( 1 - discount ) - in all cases: listprice = rrp / currency rate unitprice = ecost total = ecost * quantity 7. Retry with different parameters 8. Check the 'Import all' action still works Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 27409 [details] [review] Bug 7180: FIX getAuthValueDropbox should display the selected value If the no AV match the name/category or if no category is defined, the input field should contain the value. Signed-off-by: Pierre Angot <tredok.pierre@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 27410 [details] [review] Bug 7180: Add UT for C4::Budgets::GetBudgetByCode Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 27411 [details] [review] Bug 7180: QA followup Ergonomic improvements: - Remove the green color the selected record. - Use fieldset.rows (and legend). - Use the required css class for quantity inputs. - Replace "budget" with "fund". - fix the "undefined" string - Add a "show MARC" link - replace "no_match" with a text. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 27412 [details] [review] Bug 7180: The default quantity is 1 Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 27413 [details] [review] Bug 7180: Follow-up fixing minor issues - Fixed some labels from Budget to Fund - Added the system preference to sysprefs.sql - Fixed some untranslatable Javascript strings Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 27414 [details] [review] Bug 7180: Change syspref description - the blank line is now useless - add an example for the syspref value Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 27415 [details] [review] Bug 7180: Adapt neworderempty code (js function moved). The js function fetchSortDropbox has been deleted in previous patch. The new function is getAuthValueDropbox. Test plan: - link some av to some funds - open an existing order and verify value are correctly filled in the sort1 and sort2 values - create a new order and verify behavior is the same as before Note: This patch generates 2 ajax queries (max) if the budget is linked to 2 av categories for sort1 and sort2. This could be improved using a template plugin for values display on load. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 27416 [details] [review] Bug 7180: FIX License version Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 27417 [details] [review] Bug 7180: QA followup - fix unit tests (use a transaction). - add 3 tabs on the page in order to be more understandable. - fix a warn in logs Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 27418 [details] [review] Bug 7180 - QA Followup - Correct minor language issues Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 27419 [details] [review] Bug 7180: Restore the "Add order" link Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 27420 [details] [review] Bug 7180: Remove useless rrp variable Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 27421 [details] [review] Bug 7180: FIX: discount was divided by 100 twice. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 27422 [details] [review] Bug 7180: fetch_sort_dropbox.pl is not in use anymore Test plan: git grep fetch_sort_dropbox should return no result.
Created attachment 27423 [details] [review] Bug 7180: QA followup 2 This patch takes into account all QA failures raised in comment 207.
Created attachment 27424 [details] [review] Bug 7180: QA followup 3 1/ showmarc.pl takes the importid for staged files. 2/ small note to indicate that the vendor's discount will be used if no discount is filled.
(In reply to Katrin Fischer from comment #219) > Hi Jonathan, we are almost there, only 2 problems found + testing the > internal note/vendor note after rebase. If you can also rebase bug 10875 I > will include it in my next test. All patches rebased against master. > Terminology: Planning value 1 and 2 should be Statistic 1 and 2. > BUG (trivial) Fixed for picking individual records, but still appears on > the default accounting details tab. QA followup 2 amended. > * BUG (normal): On the default accounting details page the refreshing > doesn't work. > For individual records it works alright, also on the neworderempty.pl > page. It works for me. Could you retest with last patches please?
Hi Jonathan, Everything looks good now, I also checked the new vendor/internal notes save correctly. There is just one more problem: the reload of the fund on the accounting details tab is still not working for me. I guess it must be something in the data that makes the difference for you, so I am trying to provide a bit more information on my current setup: - I only have 1 fund with an authorized value category linked to Statistic 2, the other funds currently have no authorized value categories linked to them. - The fund with the link appears first in my list of funds, so is selected when you get on the page. - Makes me think, maybe we need a 'select fund' option here like neworderempty.pl has? - Page shows: Statistic 1: free text field, Statistic 2: CCODE list - When I switch to another fund, it looks like no new request for a new list is done and the Statistic 2 is not cleared up. - Switching back to the first fund, I can see the request for the CCODE list in Firebug. - It works perfectly fine for individual order lines. Does that make sense? I think it's the last bit. I have confirmed it still happens with 10875 applied. ... I have a branch with 17 patches marked [PASSED QA], hope I can push that up tonight :)
Created attachment 27451 [details] [review] Bug 7180: Reset statistic values on reloading the page On the "Default accounting details", if a dropdown list was created for a statistic value, on reloading the page it still exist. It should not given the fund value is reset.
I am sorry Jonathan, but I still can't get it to work on Accounting details. :( I have tried it with and without bug 10875 applied. Some more information with 10875 applied: - with the patch for 10875 I only have 1 fund: Books, it's got: Statistic 1: no authority category Statistic 2: CCODE - when I open the staged order list and go straight to the Accounting details tab the fund pull down shows: Select fund But: Statistic 2 already shows CCODE? - when I switch to the only other option: select funds - nothing changes - when I check the checkbox to display the other funds and switch to the other funds showing up, the display of Statistic 2 still won't change Without 10875 applied: - Page also shows select fund + statistic 2: ccode from the beginning - Switching the funds won't change it Maybe it somehwo still picks the values of the first fund in the list and then fails to reset it? For a test I created another fund in the active budget, I named it Bbooks, to make sure it sorts first and before Books: - Statistic 1: no authority category - Statistic 2: CCODE - when I open the staged order list now and goto the Accounting tabs both statistic values show as empty, no fund is selected yet - switching around works Hope that helps.
Created attachment 27520 [details] [review] Bug 7180: Reset statistic values on reloading the page On the "Default accounting details", if a dropdown list was created for a statistic value, on reloading the page it still exist. It should not given the fund value is reset. The CGIsort variable is useless and can be remove: the dropdown list is generated using the ajax call.
(In reply to Katrin Fischer from comment #240) > I am sorry Jonathan, but I still can't get it to work on Accounting details. > :( Ok, got it! I amended the last patch and the behavior should be correct now. > Hope that helps. Yes, very helpful, thanks!
And here are my final testing notes: ** Automated tests and QA script ** Passes all tests in t, xt, and t/db_dependent/Budgets.t t/db_dependent/Acquisition.t t/db_dependent/Acquisition/ Passes QA script. New GetBudgetByCode() covered by tests in t/db_dependent/Budgets.t. ** Code review ** No Capitalization problems found. No translation problems found. Terminology: * Planning value 1 and 2 > Statistic 1 and 2 * Budget > Fund ** Database update Adds a new system preference MarcFieldsToOrder, inludes changes to sysprefs.sql, database update and .pref file. syspref.sql and database update are identical. ** Regression testing ** Comparing current functionality with functionality after applying the patches. File list - addorderiso2709.pl - List of files that include 'staged' records is shown, details include file name, comments, status, and staged date - Bug: # Bibs is not shown (bug 9063) * No regresion, still works like it should. New patch for bug 9063 also fixes the # Bibs problem. List of staged records - addorderiso2709.pl - Information in the list only includes title, isbn/issn, there is no way to preview a record * Improved: MARC preview and card preview - Clicking on 'Add order' will import the record into the catalog and set the status to 'imported'. * No regression, status is correctly set. - You can choose to import all records at once, using the Accounting details section. Options are: currency, budget (should be fund), notes, planning value1, planning value2 * Fund now correctly labelled. Options are now available for batch operation and for individual orders. Options include: - Individual order: quantity, price, discount, fund, statistic 1 + 2 - Batch: currency, fund, notes, planning value 1 + 2 No regression found. - Bug: Fund list shows all funds, including inactive funds (bug 10302) * No regression. Bug 10302 should fix the existing remaining problems. * Statistic values on work correctly now for individual order lines and on the accounting details tab. New order form / neworderempty.pl - Replacing ajax-getauthvaluedropbox.pl with fetch_sort_dropbox.pl Used on neworderempty to show the correct values for Statistic 1 + 2 on selecting a fund. * No regression, works like it should. ** MarcFieldsToOrder New system preference allows to include acq data in the uploaded MARC file. System preference settings used for testing: price: 933$a|933$c quantity: 933$q sort1: 933$1 sort2: 933$2 discount: 933$d budget_code: 933$b Lines included in my records: 933 $a 12.00 $q 12 $b BOOKS1310 $d 5 $1 sort1 $2 sort2 933 $c 12.99 $a 9.00 $q 12 $b BOOKS1310 $d 0.05 $1 sort1 $2 sort2 Looking at the individual details for the first 2 records revealed: all settings have been imported correctly. Discount should be given as $, so 0.05 is way too low, 5 is better.
Created attachment 27561 [details] [review] [PASSED QA] Bug 7180: Order from staged file improvements This patch adds: - 1 syspref MarcFieldsToOrder - 1 ajax script acqui/ajax-getauthvaluedropbox.pl - 1 routine C4::Budgets::GetBudgetByCode Before this patch you were not able to order 1 or all the records from your staged file. You were allowed to specify some information ("Import All" and "Accounting details" areas) for the order. With this patch, the previous behaviours still exist. But now you can *select* which records you want to ordered. For these ones you can specify independently informations (Quantity, price, budget, sort1 and sort2). The cherry on the cake is that you can pre-fill these information with values Test plan: 1. Fill the new syspref MarcFieldsToOrder with something like: ==BEGIN== price: 947$c quantity: 969$h budget_code: 922$a rrp: 010$d discount: 969$d sort1: 923$a sort2: 924$a ==END== The empty line at the end is mandatory! The budget (corresponding to your budget_code) can be filled with authorized value categories (statistic 1 and 2). The sort1 and sort2 values can be filled with the an authorized value (of the category previously selected) 2. Choose randomly one or more biblio(s) and fill fields with what is relevant. 3. Export the biblio and import it (with the "Stage MARC records for import" tool). 4. Go on a basket and add an order from a staged file. Select your staged file. 5. Well. Now you can see your biblio (or biblios if your had exported more than one). For each one, informations should be pre-filled with the biblio values. The budget should be selected on the budget corresponding to the budget_code (in the field 922$a) and the "planning values" too (with fields 923$a and 924$a). You can modify these values (or not) and choose a default value for budget and planning values (in the "Accounting details" area). 6. Save and check the prices values. Modify the order and check that budget and sort* are good Prices are calculated following some parameters: if there is no price => listprice = 0 else => - the gstrate value for your order is the gstrate value of the bookseller - discount = if filled : the discount value / 100 else: the discount value of the bookseller - if the bookseller includes tax( List item price includes tax: Yes ) if a discount exists: ecost = price rrp = ecost / ( 1 - discount ) else: # a discount does not exist ecost = price * ( 1 - discount ) rrp = price else # the bookseller does not include tax if a discount exists: ecost = price / ( 1 + gstrate ) rrp = ecost / ( 1 - discount ) else: # a discount does not exist rrp = price / ( 1 + gstrate ) ecost = rrp * ( 1 - discount ) - in all cases: listprice = rrp / currency rate unitprice = ecost total = ecost * quantity 7. Retry with different parameters 8. Check the 'Import all' action still works Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27562 [details] [review] [PASSED QA] Bug 7180: FIX getAuthValueDropbox should display the selected value If the no AV match the name/category or if no category is defined, the input field should contain the value. Signed-off-by: Pierre Angot <tredok.pierre@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27563 [details] [review] [PASSED QA] Bug 7180: Add UT for C4::Budgets::GetBudgetByCode Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27564 [details] [review] [PASSED QA] Bug 7180: QA followup Ergonomic improvements: - Remove the green color the selected record. - Use fieldset.rows (and legend). - Use the required css class for quantity inputs. - Replace "budget" with "fund". - fix the "undefined" string - Add a "show MARC" link - replace "no_match" with a text. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27565 [details] [review] [PASSED QA] Bug 7180: The default quantity is 1 Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27566 [details] [review] [PASSED QA] Bug 7180: Follow-up fixing minor issues - Fixed some labels from Budget to Fund - Added the system preference to sysprefs.sql - Fixed some untranslatable Javascript strings Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27567 [details] [review] [PASSED QA] Bug 7180: Change syspref description - the blank line is now useless - add an example for the syspref value Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27568 [details] [review] [PASSED QA] Bug 7180: Adapt neworderempty code (js function moved). The js function fetchSortDropbox has been deleted in previous patch. The new function is getAuthValueDropbox. Test plan: - link some av to some funds - open an existing order and verify value are correctly filled in the sort1 and sort2 values - create a new order and verify behavior is the same as before Note: This patch generates 2 ajax queries (max) if the budget is linked to 2 av categories for sort1 and sort2. This could be improved using a template plugin for values display on load. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27569 [details] [review] [PASSED QA] Bug 7180: FIX License version Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27570 [details] [review] [PASSED QA] Bug 7180: QA followup - fix unit tests (use a transaction). - add 3 tabs on the page in order to be more understandable. - fix a warn in logs Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27571 [details] [review] [PASSED QA] Bug 7180 - QA Followup - Correct minor language issues Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27572 [details] [review] [PASSED QA] Bug 7180: Restore the "Add order" link Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27573 [details] [review] [PASSED QA] Bug 7180: Remove useless rrp variable Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27574 [details] [review] [PASSED QA] Bug 7180: FIX: discount was divided by 100 twice. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27575 [details] [review] [PASSED QA] Bug 7180: fetch_sort_dropbox.pl is not in use anymore Test plan: git grep fetch_sort_dropbox should return no result. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27576 [details] [review] [PASSED QA] Bug 7180: QA followup 2 This patch takes into account all QA failures raised in comment 207. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27577 [details] [review] [PASSED QA] Bug 7180: QA followup 3 1/ showmarc.pl takes the importid for staged files. 2/ small note to indicate that the vendor's discount will be used if no discount is filled. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27578 [details] [review] [PASSED QA] Bug 7180: Reset statistic values on reloading the page On the "Default accounting details", if a dropdown list was created for a statistic value, on reloading the page it still exist. It should not given the fund value is reset. The CGIsort variable is useless and can be remove: the dropdown list is generated using the ajax call. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Testing notes are on the bug report.
The first patch no longer applies, and the conflict doesn't appear non-trivial to resolve. Jonathan, please feel free to set status back to passed QA after you resolve the merge conflicts.
Created attachment 27829 [details] [review] Bug 7180: Order from staged file improvements This patch adds: - 1 syspref MarcFieldsToOrder - 1 ajax script acqui/ajax-getauthvaluedropbox.pl - 1 routine C4::Budgets::GetBudgetByCode Before this patch you were not able to order 1 or all the records from your staged file. You were allowed to specify some information ("Import All" and "Accounting details" areas) for the order. With this patch, the previous behaviours still exist. But now you can *select* which records you want to ordered. For these ones you can specify independently informations (Quantity, price, budget, sort1 and sort2). The cherry on the cake is that you can pre-fill these information with values Test plan: 1. Fill the new syspref MarcFieldsToOrder with something like: ==BEGIN== price: 947$c quantity: 969$h budget_code: 922$a rrp: 010$d discount: 969$d sort1: 923$a sort2: 924$a ==END== The empty line at the end is mandatory! The budget (corresponding to your budget_code) can be filled with authorized value categories (statistic 1 and 2). The sort1 and sort2 values can be filled with the an authorized value (of the category previously selected) 2. Choose randomly one or more biblio(s) and fill fields with what is relevant. 3. Export the biblio and import it (with the "Stage MARC records for import" tool). 4. Go on a basket and add an order from a staged file. Select your staged file. 5. Well. Now you can see your biblio (or biblios if your had exported more than one). For each one, informations should be pre-filled with the biblio values. The budget should be selected on the budget corresponding to the budget_code (in the field 922$a) and the "planning values" too (with fields 923$a and 924$a). You can modify these values (or not) and choose a default value for budget and planning values (in the "Accounting details" area). 6. Save and check the prices values. Modify the order and check that budget and sort* are good Prices are calculated following some parameters: if there is no price => listprice = 0 else => - the gstrate value for your order is the gstrate value of the bookseller - discount = if filled : the discount value / 100 else: the discount value of the bookseller - if the bookseller includes tax( List item price includes tax: Yes ) if a discount exists: ecost = price rrp = ecost / ( 1 - discount ) else: # a discount does not exist ecost = price * ( 1 - discount ) rrp = price else # the bookseller does not include tax if a discount exists: ecost = price / ( 1 + gstrate ) rrp = ecost / ( 1 - discount ) else: # a discount does not exist rrp = price / ( 1 + gstrate ) ecost = rrp * ( 1 - discount ) - in all cases: listprice = rrp / currency rate unitprice = ecost total = ecost * quantity 7. Retry with different parameters 8. Check the 'Import all' action still works Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 27830 [details] [review] Bug 7180: FIX getAuthValueDropbox should display the selected value If the no AV match the name/category or if no category is defined, the input field should contain the value. Signed-off-by: Pierre Angot <tredok.pierre@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 27831 [details] [review] Bug 7180: Add UT for C4::Budgets::GetBudgetByCode Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 27832 [details] [review] Bug 7180: QA followup Ergonomic improvements: - Remove the green color the selected record. - Use fieldset.rows (and legend). - Use the required css class for quantity inputs. - Replace "budget" with "fund". - fix the "undefined" string - Add a "show MARC" link - replace "no_match" with a text. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 27833 [details] [review] Bug 7180: The default quantity is 1 Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 27834 [details] [review] Bug 7180: Follow-up fixing minor issues - Fixed some labels from Budget to Fund - Added the system preference to sysprefs.sql - Fixed some untranslatable Javascript strings Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 27835 [details] [review] Bug 7180: Change syspref description - the blank line is now useless - add an example for the syspref value Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 27836 [details] [review] Bug 7180: Adapt neworderempty code (js function moved). The js function fetchSortDropbox has been deleted in previous patch. The new function is getAuthValueDropbox. Test plan: - link some av to some funds - open an existing order and verify value are correctly filled in the sort1 and sort2 values - create a new order and verify behavior is the same as before Note: This patch generates 2 ajax queries (max) if the budget is linked to 2 av categories for sort1 and sort2. This could be improved using a template plugin for values display on load. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 27837 [details] [review] Bug 7180: FIX License version Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 27838 [details] [review] Bug 7180: QA followup - fix unit tests (use a transaction). - add 3 tabs on the page in order to be more understandable. - fix a warn in logs Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 27839 [details] [review] Bug 7180 - QA Followup - Correct minor language issues Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 27840 [details] [review] Bug 7180: Restore the "Add order" link Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 27841 [details] [review] Bug 7180: Remove useless rrp variable Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 27842 [details] [review] Bug 7180: FIX: discount was divided by 100 twice. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 27843 [details] [review] Bug 7180: fetch_sort_dropbox.pl is not in use anymore Test plan: git grep fetch_sort_dropbox should return no result.
Created attachment 27844 [details] [review] Bug 7180: QA followup 2 This patch takes into account all QA failures raised in comment 207.
Created attachment 27845 [details] [review] Bug 7180: QA followup 3 1/ showmarc.pl takes the importid for staged files. 2/ small note to indicate that the vendor's discount will be used if no discount is filled.
Created attachment 27846 [details] [review] Bug 7180: Reset statistic values on reloading the page On the "Default accounting details", if a dropdown list was created for a statistic value, on reloading the page it still exist. It should not given the fund value is reset.
Created attachment 27847 [details] [review] Bug 10875: Add order from a staged file should not display inactive funds When adding an order from a staged file, a link "show all funds" is now added on the top of the page. All inactive funds are hidden by default.
Last patches fix conflicts with bug 12089 and bug 12138.
Created attachment 27848 [details] [review] Bug 7180: Reset statistic values on reloading the page On the "Default accounting details", if a dropdown list was created for a statistic value, on reloading the page it still exist. It should not given the fund value is reset.
Created attachment 27849 [details] [review] Bug 7180: Order from staged file improvements This patch adds: - 1 syspref MarcFieldsToOrder - 1 ajax script acqui/ajax-getauthvaluedropbox.pl - 1 routine C4::Budgets::GetBudgetByCode Before this patch you were not able to order 1 or all the records from your staged file. You were allowed to specify some information ("Import All" and "Accounting details" areas) for the order. With this patch, the previous behaviours still exist. But now you can *select* which records you want to ordered. For these ones you can specify independently informations (Quantity, price, budget, sort1 and sort2). The cherry on the cake is that you can pre-fill these information with values Test plan: 1. Fill the new syspref MarcFieldsToOrder with something like: ==BEGIN== price: 947$c quantity: 969$h budget_code: 922$a rrp: 010$d discount: 969$d sort1: 923$a sort2: 924$a ==END== The empty line at the end is mandatory! The budget (corresponding to your budget_code) can be filled with authorized value categories (statistic 1 and 2). The sort1 and sort2 values can be filled with the an authorized value (of the category previously selected) 2. Choose randomly one or more biblio(s) and fill fields with what is relevant. 3. Export the biblio and import it (with the "Stage MARC records for import" tool). 4. Go on a basket and add an order from a staged file. Select your staged file. 5. Well. Now you can see your biblio (or biblios if your had exported more than one). For each one, informations should be pre-filled with the biblio values. The budget should be selected on the budget corresponding to the budget_code (in the field 922$a) and the "planning values" too (with fields 923$a and 924$a). You can modify these values (or not) and choose a default value for budget and planning values (in the "Accounting details" area). 6. Save and check the prices values. Modify the order and check that budget and sort* are good Prices are calculated following some parameters: if there is no price => listprice = 0 else => - the gstrate value for your order is the gstrate value of the bookseller - discount = if filled : the discount value / 100 else: the discount value of the bookseller - if the bookseller includes tax( List item price includes tax: Yes ) if a discount exists: ecost = price rrp = ecost / ( 1 - discount ) else: # a discount does not exist ecost = price * ( 1 - discount ) rrp = price else # the bookseller does not include tax if a discount exists: ecost = price / ( 1 + gstrate ) rrp = ecost / ( 1 - discount ) else: # a discount does not exist rrp = price / ( 1 + gstrate ) ecost = rrp * ( 1 - discount ) - in all cases: listprice = rrp / currency rate unitprice = ecost total = ecost * quantity 7. Retry with different parameters 8. Check the 'Import all' action still works Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27850 [details] [review] Bug 7180: FIX getAuthValueDropbox should display the selected value If the no AV match the name/category or if no category is defined, the input field should contain the value. Signed-off-by: Pierre Angot <tredok.pierre@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27851 [details] [review] Bug 7180: Add UT for C4::Budgets::GetBudgetByCode Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27852 [details] [review] Bug 7180: QA followup Ergonomic improvements: - Remove the green color the selected record. - Use fieldset.rows (and legend). - Use the required css class for quantity inputs. - Replace "budget" with "fund". - fix the "undefined" string - Add a "show MARC" link - replace "no_match" with a text. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27853 [details] [review] Bug 7180: The default quantity is 1 Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27854 [details] [review] Bug 7180: Follow-up fixing minor issues - Fixed some labels from Budget to Fund - Added the system preference to sysprefs.sql - Fixed some untranslatable Javascript strings Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27855 [details] [review] Bug 7180: Change syspref description - the blank line is now useless - add an example for the syspref value Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27856 [details] [review] Bug 7180: Adapt neworderempty code (js function moved). The js function fetchSortDropbox has been deleted in previous patch. The new function is getAuthValueDropbox. Test plan: - link some av to some funds - open an existing order and verify value are correctly filled in the sort1 and sort2 values - create a new order and verify behavior is the same as before Note: This patch generates 2 ajax queries (max) if the budget is linked to 2 av categories for sort1 and sort2. This could be improved using a template plugin for values display on load. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27857 [details] [review] Bug 7180: FIX License version Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27858 [details] [review] Bug 7180: QA followup - fix unit tests (use a transaction). - add 3 tabs on the page in order to be more understandable. - fix a warn in logs Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27859 [details] [review] Bug 7180 - QA Followup - Correct minor language issues Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27860 [details] [review] Bug 7180: Restore the "Add order" link Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27861 [details] [review] Bug 7180: Remove useless rrp variable Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27862 [details] [review] Bug 7180: FIX: discount was divided by 100 twice. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27863 [details] [review] Bug 7180: fetch_sort_dropbox.pl is not in use anymore Test plan: git grep fetch_sort_dropbox should return no result. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27864 [details] [review] Bug 7180: QA followup 2 This patch takes into account all QA failures raised in comment 207. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27865 [details] [review] Bug 7180: QA followup 3 1/ showmarc.pl takes the importid for staged files. 2/ small note to indicate that the vendor's discount will be used if no discount is filled. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27866 [details] [review] Bug 7180: Reset statistic values on reloading the page On the "Default accounting details", if a dropdown list was created for a statistic value, on reloading the page it still exist. It should not given the fund value is reset. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Testing notes are on the bug report.
I reuploaded patches from my local branch, so without the Katrin's SO. Last patch set contains the SO! Sorry for the confusion.
Comment on attachment 27520 [details] [review] Bug 7180: Reset statistic values on reloading the page This patch somehow got dropped during the QA process. It's needed for 10875 to apply successfully.
Pushed to master. Thanks, Jonathan!
*** Bug 9597 has been marked as a duplicate of this bug. ***