Bug 7180

Summary: Order from staged file improvements
Product: Koha Reporter: Julian Maurice <julian.maurice>
Component: AcquisitionsAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Katrin Fischer <katrin.fischer>
Severity: enhancement    
Priority: P5 - low CC: chris, claire.hernandez, gitbot, gmcharlt, jonathan.druart, katrin.fischer, kyle, marjorie.barry-vila, mathsabypro, mtj, nengard, paul.poulain, tredok.pierre
Version: master   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9063
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10501
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10302
Change sponsored?: Sponsored Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 5335, 10522    
Bug Blocks: 13180, 7163, 9063, 10875, 10877, 12074, 13214    
Attachments: addorderiso2709.pl
Bug 7180: Order from staged file improvements
Bug 7180: Order from staged file improvements
Bug 7180: Order from staged file improvements
Bug 7180: Order from staged file improvements
Bug 7180: Followup Show the discount value
Bug 7180: Fix typo, budget_code is a var, not a string...
Bug 7180: FIX ergonomic issue on selecting record
Bug 7180: Order from staged file improvements
Bug 7180: FIX getAuthValueDropbox should display the selected value
Bug 7180: FIX getAuthValueDropbox should display the selected value
Test
Bug 7180: FIX getAuthValueDropbox should display the selected value
Bug 7180: Add UT for C4::Budgets::GetBudgetByCode
Bug 7180: Order from staged file improvements
Bug 7180: FIX getAuthValueDropbox should display the selected value
Bug 7180: Add UT for C4::Budgets::GetBudgetByCode
Bug 7180: QA followup
Bug 7180: Order from staged file improvements
Bug 7180: FIX getAuthValueDropbox should display the selected value
Bug 7180: Add UT for C4::Budgets::GetBudgetByCode
Bug 7180: QA followup
Bug 7180: The default quantity is 1
proposition of evolution M. Saby
Bug 7180: Order from staged file improvements
Bug 7180: FIX getAuthValueDropbox should display the selected value
Bug 7180: Add UT for C4::Budgets::GetBudgetByCode
Bug 7180: QA followup
Bug 7180: The default quantity is 1
Bug 7180: Follow-up fixing minor issues
Bug 7180: Follow-up fixing minor issues
Bug 7180: Change syspref description
Bug 7180: Adapt neworderempty code (js function moved).
Bug 7180: Adapt neworderempty code (js function moved).
Bug 7180: FIX License version
Bug 7180: QA followup
Bug 7180 - QA Followup - Correct minor language issues
Bug 7180: Restore the "Add order" link
Bug 7180: Remove useless rrp variable
Bug 7180: Order from staged file improvements
Bug 7180: FIX getAuthValueDropbox should display the selected value
Bug 7180: Add UT for C4::Budgets::GetBudgetByCode
Bug 7180: QA followup
Bug 7180: The default quantity is 1
Bug 7180: Follow-up fixing minor issues
Bug 7180: Change syspref description
Bug 7180: Adapt neworderempty code (js function moved).
Bug 7180: FIX License version
Bug 7180: QA followup
Bug 7180 - QA Followup - Correct minor language issues
Bug 7180: Restore the "Add order" link
Bug 7180: Remove useless rrp variable
Bug 7180: FIX: discount was divided by 100 twice.
Bug 7180: Order from staged file improvements
Bug 7180: FIX getAuthValueDropbox should display the selected value
Bug 7180: Add UT for C4::Budgets::GetBudgetByCode
Bug 7180: QA followup
Bug 7180: The default quantity is 1
Bug 7180: Follow-up fixing minor issues
Bug 7180: Change syspref description
Bug 7180: Adapt neworderempty code (js function moved).
Bug 7180: FIX License version
Bug 7180: QA followup
Bug 7180 - QA Followup - Correct minor language issues
Bug 7180: Restore the "Add order" link
Bug 7180: Remove useless rrp variable
Bug 7180: FIX: discount was divided by 100 twice.
Bug 7180: Order from staged file improvements
Bug 7180: FIX getAuthValueDropbox should display the selected value
Bug 7180: Add UT for C4::Budgets::GetBudgetByCode
Bug 7180: QA followup
Bug 7180: The default quantity is 1
Bug 7180: Follow-up fixing minor issues
Bug 7180: Change syspref description
Bug 7180: Adapt neworderempty code (js function moved).
Bug 7180: FIX License version
Bug 7180: QA followup
Bug 7180 - QA Followup - Correct minor language issues
Bug 7180: Restore the "Add order" link
Bug 7180: Remove useless rrp variable
Bug 7180: FIX: discount was divided by 100 twice.
Bug 7180: Order from staged file improvements
Bug 7180: FIX getAuthValueDropbox should display the selected value
Bug 7180: Add UT for C4::Budgets::GetBudgetByCode
Bug 7180: QA followup
Bug 7180: The default quantity is 1
Bug 7180: Follow-up fixing minor issues
Bug 7180: Change syspref description
Bug 7180: Adapt neworderempty code (js function moved).
Bug 7180: FIX License version
Bug 7180: QA followup
Bug 7180 - QA Followup - Correct minor language issues
Bug 7180: Restore the "Add order" link
Bug 7180: Remove useless rrp variable
Bug 7180: FIX: discount was divided by 100 twice.
Bug 7180: Order from staged file improvements
Bug 7180: FIX getAuthValueDropbox should display the selected value
Bug 7180: Add UT for C4::Budgets::GetBudgetByCode
Bug 7180: QA followup
Bug 7180: The default quantity is 1
Bug 7180: Follow-up fixing minor issues
Bug 7180: Change syspref description
Bug 7180: Adapt neworderempty code (js function moved).
Bug 7180: FIX License version
Bug 7180: QA followup
Bug 7180 - QA Followup - Correct minor language issues
Bug 7180: Restore the "Add order" link
Bug 7180: Remove useless rrp variable
Bug 7180: FIX: discount was divided by 100 twice.
Bug 7180: fetch_sort_dropbox.pl is not in use anymore
Bug 7180: Order from staged file improvements
Bug 7180: FIX getAuthValueDropbox should display the selected value
Bug 7180: Add UT for C4::Budgets::GetBudgetByCode
Bug 7180: QA followup
Bug 7180: The default quantity is 1
Bug 7180: Follow-up fixing minor issues
Bug 7180: Change syspref description
Bug 7180: Adapt neworderempty code (js function moved).
Bug 7180: FIX License version
Bug 7180: QA followup
Bug 7180 - QA Followup - Correct minor language issues
Bug 7180: Restore the "Add order" link
Bug 7180: Remove useless rrp variable
Bug 7180: FIX: discount was divided by 100 twice.
Bug 7180: fetch_sort_dropbox.pl is not in use anymore
New staged order page
Bug 7180: QA followup 2
Bug 7180: QA followup 2
Bug 7180: QA followup 3
New staged order page - with improvements
Bug 7180: Order from staged file improvements
Bug 7180: FIX getAuthValueDropbox should display the selected value
Bug 7180: Add UT for C4::Budgets::GetBudgetByCode
Bug 7180: QA followup
Bug 7180: The default quantity is 1
Bug 7180: Follow-up fixing minor issues
Bug 7180: Change syspref description
Bug 7180: Adapt neworderempty code (js function moved).
Bug 7180: FIX License version
Bug 7180: QA followup
Bug 7180 - QA Followup - Correct minor language issues
Bug 7180: Restore the "Add order" link
Bug 7180: Remove useless rrp variable
Bug 7180: FIX: discount was divided by 100 twice.
Bug 7180: fetch_sort_dropbox.pl is not in use anymore
Bug 7180: QA followup 2
Bug 7180: QA followup 3
Bug 7180: Reset statistic values on reloading the page
Bug 7180: Reset statistic values on reloading the page
[PASSED QA] Bug 7180: Order from staged file improvements
[PASSED QA] Bug 7180: FIX getAuthValueDropbox should display the selected value
[PASSED QA] Bug 7180: Add UT for C4::Budgets::GetBudgetByCode
[PASSED QA] Bug 7180: QA followup
[PASSED QA] Bug 7180: The default quantity is 1
[PASSED QA] Bug 7180: Follow-up fixing minor issues
[PASSED QA] Bug 7180: Change syspref description
[PASSED QA] Bug 7180: Adapt neworderempty code (js function moved).
[PASSED QA] Bug 7180: FIX License version
[PASSED QA] Bug 7180: QA followup
[PASSED QA] Bug 7180 - QA Followup - Correct minor language issues
[PASSED QA] Bug 7180: Restore the "Add order" link
[PASSED QA] Bug 7180: Remove useless rrp variable
[PASSED QA] Bug 7180: FIX: discount was divided by 100 twice.
[PASSED QA] Bug 7180: fetch_sort_dropbox.pl is not in use anymore
[PASSED QA] Bug 7180: QA followup 2
[PASSED QA] Bug 7180: QA followup 3
[PASSED QA] Bug 7180: Reset statistic values on reloading the page
Bug 7180: Order from staged file improvements
Bug 7180: FIX getAuthValueDropbox should display the selected value
Bug 7180: Add UT for C4::Budgets::GetBudgetByCode
Bug 7180: QA followup
Bug 7180: The default quantity is 1
Bug 7180: Follow-up fixing minor issues
Bug 7180: Change syspref description
Bug 7180: Adapt neworderempty code (js function moved).
Bug 7180: FIX License version
Bug 7180: QA followup
Bug 7180 - QA Followup - Correct minor language issues
Bug 7180: Restore the "Add order" link
Bug 7180: Remove useless rrp variable
Bug 7180: FIX: discount was divided by 100 twice.
Bug 7180: fetch_sort_dropbox.pl is not in use anymore
Bug 7180: QA followup 2
Bug 7180: QA followup 3
Bug 7180: Reset statistic values on reloading the page
Bug 10875: Add order from a staged file should not display inactive funds
Bug 7180: Reset statistic values on reloading the page
Bug 7180: Order from staged file improvements
Bug 7180: FIX getAuthValueDropbox should display the selected value
Bug 7180: Add UT for C4::Budgets::GetBudgetByCode
Bug 7180: QA followup
Bug 7180: The default quantity is 1
Bug 7180: Follow-up fixing minor issues
Bug 7180: Change syspref description
Bug 7180: Adapt neworderempty code (js function moved).
Bug 7180: FIX License version
Bug 7180: QA followup
Bug 7180 - QA Followup - Correct minor language issues
Bug 7180: Restore the "Add order" link
Bug 7180: Remove useless rrp variable
Bug 7180: FIX: discount was divided by 100 twice.
Bug 7180: fetch_sort_dropbox.pl is not in use anymore
Bug 7180: QA followup 2
Bug 7180: QA followup 3
Bug 7180: Reset statistic values on reloading the page

Description Julian Maurice 2011-11-06 05:23:27 UTC
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
Comment 1 Jonathan Druart 2012-10-24 12:14:49 UTC Comment hidden (obsolete)
Comment 2 Mathieu Saby 2013-02-02 20:07:53 UTC
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
Comment 3 Jonathan Druart 2013-02-04 08:58:11 UTC Comment hidden (obsolete)
Comment 4 Mathieu Saby 2013-02-04 17:00:23 UTC
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
Comment 5 Jonathan Druart 2013-02-05 08:46:33 UTC Comment hidden (obsolete)
Comment 6 Mathieu Saby 2013-02-22 11:45:12 UTC
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
Comment 7 Jonathan Druart 2013-02-22 11:55:01 UTC
(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.
Comment 8 Mathieu Saby 2013-02-22 12:58:21 UTC
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
Comment 9 Jonathan Druart 2013-02-22 13:03:16 UTC Comment hidden (obsolete)
Comment 10 Mathieu Saby 2013-02-22 19:43:02 UTC
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
Comment 11 Jonathan Druart 2013-02-26 10:26:54 UTC
(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.
Comment 12 Jonathan Druart 2013-02-26 10:28:29 UTC Comment hidden (obsolete)
Comment 13 Mathieu Saby 2013-03-06 22:51:21 UTC
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
Comment 14 Jonathan Druart 2013-03-07 15:11:46 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2013-03-07 15:11:50 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2013-03-07 15:13:21 UTC
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
Comment 17 Mathieu Saby 2013-03-09 13:36:50 UTC
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
Comment 18 Jonathan Druart 2013-03-11 08:31:43 UTC Comment hidden (obsolete)
Comment 19 Mathieu Saby 2013-03-20 06:51:11 UTC
Patch not working when a matching rule is selected.

M. Saby & P. Angot
Comment 20 Pierre Angot 2013-03-20 11:09:01 UTC
I've tested and there is a little problem.
When i import a file with a free sort 2, it  does'nt work.
Comment 21 Jonathan Druart 2013-03-20 11:35:11 UTC Comment hidden (obsolete)
Comment 22 Paul Poulain 2013-03-20 12:47:59 UTC
Patch tested with a sandbox, by Pierre Angot <tredok.pierre@gmail.com>
Comment 23 Paul Poulain 2013-03-20 12:48:19 UTC Comment hidden (obsolete)
Comment 24 Mathieu Saby 2013-03-20 16:10:55 UTC
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
Comment 25 Pierre Angot 2013-03-20 16:15:08 UTC
syspref : 

price: 920$a
quantity: 920$q
budget_code: 920$b
rrp: 010$d
discount: 920$t
sort1: 920$c
sort2: 920$d
Comment 26 Pierre Angot 2013-03-20 16:17:31 UTC
Created attachment 16544 [details]
Test
Comment 27 Paul Poulain 2013-03-21 08:03:39 UTC
Patch tested with a sandbox, by Pierre Angot <tredok.pierre@gmail.com>
Comment 28 Paul Poulain 2013-03-21 08:04:07 UTC Comment hidden (obsolete)
Comment 29 Katrin Fischer 2013-03-21 10:26:20 UTC
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!
Comment 30 Jonathan Druart 2013-03-21 12:28:19 UTC Comment hidden (obsolete)
Comment 31 Jonathan Druart 2013-03-21 12:29:16 UTC
I don't have the jenkins's DB, so I cannot test if they pass... :-/
Comment 32 Jonathan Druart 2013-06-17 11:45:58 UTC Comment hidden (obsolete)
Comment 33 Jonathan Druart 2013-06-17 11:46:04 UTC Comment hidden (obsolete)
Comment 34 Jonathan Druart 2013-06-17 11:46:08 UTC Comment hidden (obsolete)
Comment 35 Jonathan Druart 2013-06-17 11:47:03 UTC
Rebased patch.
Unit tests from t/db_dependent/Budgets.t pass.
Comment 36 Katrin Fischer 2013-06-30 21:33:03 UTC
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.
Comment 37 Jonathan Druart 2013-07-01 14:02:48 UTC Comment hidden (obsolete)
Comment 38 Jonathan Druart 2013-07-01 14:07:52 UTC
(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.
Comment 39 Jonathan Druart 2013-07-09 08:06:06 UTC
bug 10522 is pushed to master. This feature can be QAed.
Comment 40 Mathieu Saby 2013-07-12 10:32:12 UTC
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
Comment 41 Jonathan Druart 2013-07-12 10:37:21 UTC Comment hidden (obsolete)
Comment 42 Jonathan Druart 2013-07-12 10:37:27 UTC Comment hidden (obsolete)
Comment 43 Jonathan Druart 2013-07-12 10:37:33 UTC Comment hidden (obsolete)
Comment 44 Jonathan Druart 2013-07-12 10:37:40 UTC Comment hidden (obsolete)
Comment 45 Jonathan Druart 2013-07-12 10:39:53 UTC
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
Comment 46 Mathieu Saby 2013-07-12 11:32:46 UTC
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
Comment 47 Mathieu Saby 2013-07-12 11:44:03 UTC
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
Comment 48 Katrin Fischer 2013-07-12 11:57:01 UTC
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.
Comment 49 Mathieu Saby 2013-07-12 12:18:52 UTC
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
Comment 50 Katrin Fischer 2013-07-12 20:16:22 UTC
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.
Comment 51 Katrin Fischer 2013-07-14 16:48:11 UTC
Jonathan, I think Mathieu has some good points here for the duplicate check and the quantity - could you take a look?
Comment 52 Jonathan Druart 2013-07-15 08:06:23 UTC Comment hidden (obsolete)
Comment 53 Jonathan Druart 2013-07-15 08:07:13 UTC
(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.
Comment 54 Mathieu Saby 2013-07-15 10:25:05 UTC
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
Comment 55 Mathieu Saby 2013-07-15 10:42:51 UTC
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
Comment 56 Katrin Fischer 2013-07-15 10:50:24 UTC
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.
Comment 57 Mathieu Saby 2013-07-15 11:23:24 UTC
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
Comment 58 Mathieu Saby 2013-07-15 11:23:56 UTC
READ : 
"- the isbn13 changed to isbn10 after the Title. need a little fix ?"
Comment 59 Jonathan Druart 2013-07-15 12:14:30 UTC
(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.
Comment 60 Mathieu Saby 2013-07-15 12:32:14 UTC
> 
> 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
Comment 61 Katrin Fischer 2013-07-15 13:06:55 UTC
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.
Comment 62 Mathieu Saby 2013-07-15 13:28:53 UTC
(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
Comment 63 Katrin Fischer 2013-09-08 20:34:34 UTC
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.
Comment 64 Jonathan Druart 2013-09-09 08:16:59 UTC Comment hidden (obsolete)
Comment 65 Jonathan Druart 2013-09-09 08:17:07 UTC Comment hidden (obsolete)
Comment 66 Jonathan Druart 2013-09-09 08:17:12 UTC Comment hidden (obsolete)
Comment 67 Jonathan Druart 2013-09-09 08:17:17 UTC Comment hidden (obsolete)
Comment 68 Jonathan Druart 2013-09-09 08:17:23 UTC Comment hidden (obsolete)
Comment 69 Jonathan Druart 2013-09-09 08:17:53 UTC
Rebased patches!
Comment 70 Katrin Fischer 2013-09-09 20:34:03 UTC
Currently working on this.
Comment 71 Katrin Fischer 2013-09-09 20:57:29 UTC Comment hidden (obsolete)
Comment 72 Katrin Fischer 2013-09-09 21:02:45 UTC
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).
Comment 73 Jonathan Druart 2013-09-10 08:37:16 UTC Comment hidden (obsolete)
Comment 74 Jonathan Druart 2013-09-10 08:37:22 UTC Comment hidden (obsolete)
Comment 75 Jonathan Druart 2013-09-10 08:37:27 UTC Comment hidden (obsolete)
Comment 76 Jonathan Druart 2013-09-10 08:39:54 UTC
(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.
Comment 77 Katrin Fischer 2013-09-10 21:27:06 UTC
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)
Comment 78 Katrin Fischer 2013-09-10 21:33:15 UTC
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.
Comment 79 Jonathan Druart 2013-09-11 07:37:41 UTC
(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.
Comment 80 Jonathan Druart 2013-09-11 07:38:12 UTC Comment hidden (obsolete)
Comment 81 Jonathan Druart 2013-09-11 07:38:17 UTC Comment hidden (obsolete)
Comment 82 Katrin Fischer 2013-09-11 20:36:44 UTC
Hi Jonathan,

one of the new tests doesn't pass for me:
not ok 4 - GetBudgetPeriod(0) returned undef : noactive BudgetPeriod

Continueing testing...
Comment 83 Katrin Fischer 2013-09-11 20:41:49 UTC
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?
Comment 84 Katrin Fischer 2013-09-11 21:15:54 UTC
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?
Comment 85 Jonathan Druart 2013-09-12 09:44:03 UTC
(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.
Comment 86 Jonathan Druart 2013-09-12 09:46:20 UTC Comment hidden (obsolete)
Comment 87 Kyle M Hall 2013-09-12 12:37:49 UTC Comment hidden (obsolete)
Comment 88 Kyle M Hall 2013-09-12 12:44:42 UTC
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.
Comment 89 Jonathan Druart 2013-09-12 12:53:50 UTC
(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...
Comment 90 Paul Poulain 2013-09-12 13:11:47 UTC
(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 )
Comment 91 Jonathan Druart 2013-09-12 13:18:33 UTC Comment hidden (obsolete)
Comment 92 Mathieu Saby 2013-09-12 13:24:39 UTC
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
Comment 93 Mathieu Saby 2013-09-13 19:31:21 UTC
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
Comment 94 Katrin Fischer 2013-09-13 19:56:06 UTC
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?
Comment 95 Mathieu Saby 2013-09-13 20:41:11 UTC
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
Comment 96 Mathieu Saby 2013-09-13 20:45:16 UTC
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
Comment 97 Jonathan Druart 2013-09-16 10:44:47 UTC
(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).
Comment 98 Jonathan Druart 2013-09-16 10:46:57 UTC Comment hidden (obsolete)
Comment 99 Jonathan Druart 2013-09-16 10:50:33 UTC
(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
Comment 100 Jonathan Druart 2013-09-16 10:52:10 UTC
Katrin wants a second signoff on this feature.
Back to Needs Signoff
Comment 101 I'm just a bot 2013-09-18 10:37:45 UTC
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".
Comment 102 Jonathan Druart 2013-09-18 12:03:42 UTC
(In reply to I'm just a bot from comment #101)
> CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
easy to solve.
Comment 103 Jonathan Druart 2013-09-26 07:25:34 UTC Comment hidden (obsolete)
Comment 104 Jonathan Druart 2013-09-26 07:25:41 UTC Comment hidden (obsolete)
Comment 105 Jonathan Druart 2013-09-26 07:25:46 UTC Comment hidden (obsolete)
Comment 106 Jonathan Druart 2013-09-26 07:25:52 UTC Comment hidden (obsolete)
Comment 107 Jonathan Druart 2013-09-26 07:25:57 UTC Comment hidden (obsolete)
Comment 108 Jonathan Druart 2013-09-26 07:26:03 UTC Comment hidden (obsolete)
Comment 109 Jonathan Druart 2013-09-26 07:26:08 UTC Comment hidden (obsolete)
Comment 110 Jonathan Druart 2013-09-26 07:26:18 UTC Comment hidden (obsolete)
Comment 111 Jonathan Druart 2013-09-26 07:26:24 UTC Comment hidden (obsolete)
Comment 112 Jonathan Druart 2013-09-26 07:26:30 UTC Comment hidden (obsolete)
Comment 113 Jonathan Druart 2013-09-26 07:26:35 UTC Comment hidden (obsolete)
Comment 114 Jonathan Druart 2013-09-26 07:26:41 UTC Comment hidden (obsolete)
Comment 115 Jonathan Druart 2013-09-26 07:26:46 UTC Comment hidden (obsolete)
Comment 116 Mathieu Saby 2013-09-26 07:52:13 UTC
I am testing it on sandbox 3, thank you for the rebasing.

Mathieu
Comment 117 Mathieu Saby 2013-09-26 08:15:00 UTC
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
Comment 118 Mathieu Saby 2013-09-26 09:15:52 UTC
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
Comment 119 Mathieu Saby 2013-09-26 09:19:49 UTC
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
Comment 120 Jonathan Druart 2013-09-27 14:53:25 UTC
(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.
Comment 121 Jonathan Druart 2013-09-27 14:54:17 UTC Comment hidden (obsolete)
Comment 122 Mathieu Saby 2013-09-27 22:22:40 UTC
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
Comment 123 I'm just a bot 2013-09-29 04:54:32 UTC
Patch applied cleanly, go forth and signoff
Comment 124 Mathieu Saby 2013-10-13 08:31:30 UTC
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
Comment 125 I'm just a bot 2013-10-30 07:10:20 UTC
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".
Comment 126 Jonathan Druart 2013-10-30 08:44:53 UTC Comment hidden (obsolete)
Comment 127 Jonathan Druart 2013-10-30 08:45:04 UTC Comment hidden (obsolete)
Comment 128 Jonathan Druart 2013-10-30 08:45:11 UTC Comment hidden (obsolete)
Comment 129 Jonathan Druart 2013-10-30 08:45:16 UTC Comment hidden (obsolete)
Comment 130 Jonathan Druart 2013-10-30 08:45:22 UTC Comment hidden (obsolete)
Comment 131 Jonathan Druart 2013-10-30 08:45:28 UTC Comment hidden (obsolete)
Comment 132 Jonathan Druart 2013-10-30 08:45:33 UTC Comment hidden (obsolete)
Comment 133 Jonathan Druart 2013-10-30 08:45:39 UTC Comment hidden (obsolete)
Comment 134 Jonathan Druart 2013-10-30 08:45:45 UTC Comment hidden (obsolete)
Comment 135 Jonathan Druart 2013-10-30 08:45:50 UTC Comment hidden (obsolete)
Comment 136 Jonathan Druart 2013-10-30 08:45:56 UTC Comment hidden (obsolete)
Comment 137 Jonathan Druart 2013-10-30 08:46:03 UTC Comment hidden (obsolete)
Comment 138 Jonathan Druart 2013-10-30 08:46:09 UTC Comment hidden (obsolete)
Comment 139 Jonathan Druart 2013-10-30 08:46:16 UTC Comment hidden (obsolete)
Comment 140 Mathieu Saby 2013-11-05 18:44:07 UTC
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
Comment 141 Jonathan Druart 2013-11-07 10:28:57 UTC
(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])
Comment 142 Mathieu Saby 2013-11-29 10:57:48 UTC
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])
Comment 143 Jonathan Druart 2014-01-09 11:50:02 UTC Comment hidden (obsolete)
Comment 144 Jonathan Druart 2014-01-09 11:50:14 UTC Comment hidden (obsolete)
Comment 145 Jonathan Druart 2014-01-09 11:50:23 UTC Comment hidden (obsolete)
Comment 146 Jonathan Druart 2014-01-09 11:50:33 UTC Comment hidden (obsolete)
Comment 147 Jonathan Druart 2014-01-09 11:50:42 UTC Comment hidden (obsolete)
Comment 148 Jonathan Druart 2014-01-09 11:50:52 UTC Comment hidden (obsolete)
Comment 149 Jonathan Druart 2014-01-09 11:51:01 UTC Comment hidden (obsolete)
Comment 150 Jonathan Druart 2014-01-09 11:51:11 UTC Comment hidden (obsolete)
Comment 151 Jonathan Druart 2014-01-09 11:51:20 UTC Comment hidden (obsolete)
Comment 152 Jonathan Druart 2014-01-09 11:51:30 UTC Comment hidden (obsolete)
Comment 153 Jonathan Druart 2014-01-09 11:51:39 UTC Comment hidden (obsolete)
Comment 154 Jonathan Druart 2014-01-09 11:51:50 UTC Comment hidden (obsolete)
Comment 155 Jonathan Druart 2014-01-09 11:52:00 UTC Comment hidden (obsolete)
Comment 156 Jonathan Druart 2014-01-09 11:52:10 UTC Comment hidden (obsolete)
Comment 157 Jonathan Druart 2014-01-09 11:52:55 UTC
Patches rebased.
Comment 158 Jonathan Druart 2014-01-23 09:09:19 UTC Comment hidden (obsolete)
Comment 159 Jonathan Druart 2014-01-23 09:09:28 UTC Comment hidden (obsolete)
Comment 160 Jonathan Druart 2014-01-23 09:09:35 UTC Comment hidden (obsolete)
Comment 161 Jonathan Druart 2014-01-23 09:09:42 UTC Comment hidden (obsolete)
Comment 162 Jonathan Druart 2014-01-23 09:09:49 UTC Comment hidden (obsolete)
Comment 163 Jonathan Druart 2014-01-23 09:09:57 UTC Comment hidden (obsolete)
Comment 164 Jonathan Druart 2014-01-23 09:10:05 UTC Comment hidden (obsolete)
Comment 165 Jonathan Druart 2014-01-23 09:10:13 UTC Comment hidden (obsolete)
Comment 166 Jonathan Druart 2014-01-23 09:10:23 UTC Comment hidden (obsolete)
Comment 167 Jonathan Druart 2014-01-23 09:10:31 UTC Comment hidden (obsolete)
Comment 168 Jonathan Druart 2014-01-23 09:10:38 UTC Comment hidden (obsolete)
Comment 169 Jonathan Druart 2014-01-23 09:10:45 UTC Comment hidden (obsolete)
Comment 170 Jonathan Druart 2014-01-23 09:10:52 UTC Comment hidden (obsolete)
Comment 171 Jonathan Druart 2014-01-23 09:11:00 UTC Comment hidden (obsolete)
Comment 172 Jonathan Druart 2014-01-23 09:11:44 UTC
Patches rebased.
Comment 173 Kyle M Hall 2014-01-24 17:32:26 UTC Comment hidden (obsolete)
Comment 174 Kyle M Hall 2014-01-24 17:32:42 UTC Comment hidden (obsolete)
Comment 175 Kyle M Hall 2014-01-24 17:32:47 UTC Comment hidden (obsolete)
Comment 176 Kyle M Hall 2014-01-24 17:32:52 UTC Comment hidden (obsolete)
Comment 177 Kyle M Hall 2014-01-24 17:32:56 UTC Comment hidden (obsolete)
Comment 178 Kyle M Hall 2014-01-24 17:33:00 UTC Comment hidden (obsolete)
Comment 179 Kyle M Hall 2014-01-24 17:33:06 UTC Comment hidden (obsolete)
Comment 180 Kyle M Hall 2014-01-24 17:33:11 UTC Comment hidden (obsolete)
Comment 181 Kyle M Hall 2014-01-24 17:33:15 UTC Comment hidden (obsolete)
Comment 182 Kyle M Hall 2014-01-24 17:33:19 UTC Comment hidden (obsolete)
Comment 183 Kyle M Hall 2014-01-24 17:33:23 UTC Comment hidden (obsolete)
Comment 184 Kyle M Hall 2014-01-24 17:33:28 UTC Comment hidden (obsolete)
Comment 185 Kyle M Hall 2014-01-24 17:33:32 UTC Comment hidden (obsolete)
Comment 186 Kyle M Hall 2014-01-24 17:33:42 UTC Comment hidden (obsolete)
Comment 187 Jonathan Druart 2014-03-06 16:20:14 UTC Comment hidden (obsolete)
Comment 188 Katrin Fischer 2014-04-09 23:11:17 UTC
Hi Jonathan, could you rebase this please? I would like to try and tackle this beginning of next week.
Comment 189 Jonathan Druart 2014-04-10 07:46:06 UTC Comment hidden (obsolete)
Comment 190 Jonathan Druart 2014-04-10 07:46:14 UTC Comment hidden (obsolete)
Comment 191 Jonathan Druart 2014-04-10 07:46:20 UTC Comment hidden (obsolete)
Comment 192 Jonathan Druart 2014-04-10 07:46:26 UTC Comment hidden (obsolete)
Comment 193 Jonathan Druart 2014-04-10 07:46:31 UTC Comment hidden (obsolete)
Comment 194 Jonathan Druart 2014-04-10 07:46:36 UTC Comment hidden (obsolete)
Comment 195 Jonathan Druart 2014-04-10 07:46:42 UTC Comment hidden (obsolete)
Comment 196 Jonathan Druart 2014-04-10 07:46:47 UTC Comment hidden (obsolete)
Comment 197 Jonathan Druart 2014-04-10 07:46:52 UTC Comment hidden (obsolete)
Comment 198 Jonathan Druart 2014-04-10 07:46:58 UTC Comment hidden (obsolete)
Comment 199 Jonathan Druart 2014-04-10 07:47:05 UTC Comment hidden (obsolete)
Comment 200 Jonathan Druart 2014-04-10 07:47:11 UTC Comment hidden (obsolete)
Comment 201 Jonathan Druart 2014-04-10 07:47:16 UTC Comment hidden (obsolete)
Comment 202 Jonathan Druart 2014-04-10 07:47:22 UTC Comment hidden (obsolete)
Comment 203 Jonathan Druart 2014-04-10 07:47:28 UTC Comment hidden (obsolete)
Comment 204 Jonathan Druart 2014-04-10 08:36:24 UTC
Patches rebased against master.
Comment 205 Katrin Fischer 2014-04-15 09:47:10 UTC
Starting on this now... wish me luck? :)
Comment 206 Katrin Fischer 2014-04-15 12:46:53 UTC
Created attachment 27128 [details]
New staged order page

A screenshot - testing comments later.
Comment 207 Katrin Fischer 2014-04-15 13:32:11 UTC
** 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
Comment 208 Jonathan Druart 2014-04-15 14:45:41 UTC Comment hidden (obsolete)
Comment 209 Jonathan Druart 2014-04-15 14:46:35 UTC
(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.
Comment 210 Katrin Fischer 2014-04-15 14:57:19 UTC
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 :)
Comment 211 Katrin Fischer 2014-04-15 15:25:41 UTC
Resetting failed for now, until we can figure out what's wrong with the card view.
Comment 212 Jonathan Druart 2014-04-15 15:29:19 UTC Comment hidden (obsolete)
Comment 213 Jonathan Druart 2014-04-15 15:30:22 UTC
(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).
Comment 214 Katrin Fischer 2014-04-15 20:19:27 UTC
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 :)
Comment 215 Jonathan Druart 2014-04-16 09:03:28 UTC Comment hidden (obsolete)
Comment 216 Katrin Fischer 2014-04-19 09:21:48 UTC
*** Bug 10501 has been marked as a duplicate of this bug. ***
Comment 217 Katrin Fischer 2014-04-20 15:26:08 UTC
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.
Comment 218 Katrin Fischer 2014-04-20 15:30:28 UTC
Created attachment 27325 [details]
New staged order page - with improvements
Comment 219 Katrin Fischer 2014-04-20 16:46:45 UTC
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.
Comment 220 Jonathan Druart 2014-04-22 15:41:38 UTC Comment hidden (obsolete)
Comment 221 Jonathan Druart 2014-04-22 15:41:45 UTC Comment hidden (obsolete)
Comment 222 Jonathan Druart 2014-04-22 15:41:52 UTC Comment hidden (obsolete)
Comment 223 Jonathan Druart 2014-04-22 15:41:58 UTC Comment hidden (obsolete)
Comment 224 Jonathan Druart 2014-04-22 15:42:04 UTC Comment hidden (obsolete)
Comment 225 Jonathan Druart 2014-04-22 15:42:10 UTC Comment hidden (obsolete)
Comment 226 Jonathan Druart 2014-04-22 15:42:17 UTC Comment hidden (obsolete)
Comment 227 Jonathan Druart 2014-04-22 15:42:22 UTC Comment hidden (obsolete)
Comment 228 Jonathan Druart 2014-04-22 15:42:28 UTC Comment hidden (obsolete)
Comment 229 Jonathan Druart 2014-04-22 15:42:34 UTC Comment hidden (obsolete)
Comment 230 Jonathan Druart 2014-04-22 15:42:39 UTC Comment hidden (obsolete)
Comment 231 Jonathan Druart 2014-04-22 15:42:45 UTC Comment hidden (obsolete)
Comment 232 Jonathan Druart 2014-04-22 15:42:51 UTC Comment hidden (obsolete)
Comment 233 Jonathan Druart 2014-04-22 15:42:57 UTC Comment hidden (obsolete)
Comment 234 Jonathan Druart 2014-04-22 15:43:03 UTC Comment hidden (obsolete)
Comment 235 Jonathan Druart 2014-04-22 15:43:09 UTC Comment hidden (obsolete)
Comment 236 Jonathan Druart 2014-04-22 15:43:14 UTC Comment hidden (obsolete)
Comment 237 Jonathan Druart 2014-04-22 15:43:58 UTC
(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?
Comment 238 Katrin Fischer 2014-04-23 05:52:17 UTC
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 :)
Comment 239 Jonathan Druart 2014-04-23 07:59:49 UTC Comment hidden (obsolete)
Comment 240 Katrin Fischer 2014-04-24 05:46:14 UTC
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.
Comment 241 Jonathan Druart 2014-04-24 07:30:28 UTC
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.
Comment 242 Jonathan Druart 2014-04-24 07:32:05 UTC
(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!
Comment 243 Katrin Fischer 2014-04-25 05:21:56 UTC
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.
Comment 244 Katrin Fischer 2014-04-25 05:22:49 UTC Comment hidden (obsolete)
Comment 245 Katrin Fischer 2014-04-25 05:22:54 UTC Comment hidden (obsolete)
Comment 246 Katrin Fischer 2014-04-25 05:22:58 UTC Comment hidden (obsolete)
Comment 247 Katrin Fischer 2014-04-25 05:23:03 UTC Comment hidden (obsolete)
Comment 248 Katrin Fischer 2014-04-25 05:23:08 UTC Comment hidden (obsolete)
Comment 249 Katrin Fischer 2014-04-25 05:23:13 UTC Comment hidden (obsolete)
Comment 250 Katrin Fischer 2014-04-25 05:23:18 UTC Comment hidden (obsolete)
Comment 251 Katrin Fischer 2014-04-25 05:23:23 UTC Comment hidden (obsolete)
Comment 252 Katrin Fischer 2014-04-25 05:23:28 UTC Comment hidden (obsolete)
Comment 253 Katrin Fischer 2014-04-25 05:23:34 UTC Comment hidden (obsolete)
Comment 254 Katrin Fischer 2014-04-25 05:23:39 UTC Comment hidden (obsolete)
Comment 255 Katrin Fischer 2014-04-25 05:23:44 UTC Comment hidden (obsolete)
Comment 256 Katrin Fischer 2014-04-25 05:23:49 UTC Comment hidden (obsolete)
Comment 257 Katrin Fischer 2014-04-25 05:23:54 UTC Comment hidden (obsolete)
Comment 258 Katrin Fischer 2014-04-25 05:23:59 UTC Comment hidden (obsolete)
Comment 259 Katrin Fischer 2014-04-25 05:24:04 UTC Comment hidden (obsolete)
Comment 260 Katrin Fischer 2014-04-25 05:24:09 UTC Comment hidden (obsolete)
Comment 261 Katrin Fischer 2014-04-25 05:24:14 UTC Comment hidden (obsolete)
Comment 262 Galen Charlton 2014-04-30 16:25:46 UTC
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.
Comment 263 Jonathan Druart 2014-05-01 07:20:35 UTC Comment hidden (obsolete)
Comment 264 Jonathan Druart 2014-05-01 07:20:41 UTC Comment hidden (obsolete)
Comment 265 Jonathan Druart 2014-05-01 07:20:47 UTC Comment hidden (obsolete)
Comment 266 Jonathan Druart 2014-05-01 07:20:52 UTC Comment hidden (obsolete)
Comment 267 Jonathan Druart 2014-05-01 07:20:57 UTC Comment hidden (obsolete)
Comment 268 Jonathan Druart 2014-05-01 07:21:03 UTC Comment hidden (obsolete)
Comment 269 Jonathan Druart 2014-05-01 07:21:09 UTC Comment hidden (obsolete)
Comment 270 Jonathan Druart 2014-05-01 07:21:16 UTC Comment hidden (obsolete)
Comment 271 Jonathan Druart 2014-05-01 07:21:21 UTC Comment hidden (obsolete)
Comment 272 Jonathan Druart 2014-05-01 07:21:26 UTC Comment hidden (obsolete)
Comment 273 Jonathan Druart 2014-05-01 07:21:33 UTC Comment hidden (obsolete)
Comment 274 Jonathan Druart 2014-05-01 07:21:38 UTC Comment hidden (obsolete)
Comment 275 Jonathan Druart 2014-05-01 07:21:43 UTC Comment hidden (obsolete)
Comment 276 Jonathan Druart 2014-05-01 07:21:48 UTC Comment hidden (obsolete)
Comment 277 Jonathan Druart 2014-05-01 07:21:56 UTC Comment hidden (obsolete)
Comment 278 Jonathan Druart 2014-05-01 07:22:03 UTC Comment hidden (obsolete)
Comment 279 Jonathan Druart 2014-05-01 07:22:11 UTC Comment hidden (obsolete)
Comment 280 Jonathan Druart 2014-05-01 07:22:18 UTC Comment hidden (obsolete)
Comment 281 Jonathan Druart 2014-05-01 07:22:23 UTC Comment hidden (obsolete)
Comment 282 Jonathan Druart 2014-05-01 07:23:05 UTC
Last patches fix conflicts with bug 12089 and bug 12138.
Comment 283 Jonathan Druart 2014-05-01 07:24:06 UTC Comment hidden (obsolete)
Comment 284 Jonathan Druart 2014-05-01 08:02:07 UTC
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>
Comment 285 Jonathan Druart 2014-05-01 08:02:16 UTC
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>
Comment 286 Jonathan Druart 2014-05-01 08:02:25 UTC
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>
Comment 287 Jonathan Druart 2014-05-01 08:02:58 UTC
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>
Comment 288 Jonathan Druart 2014-05-01 08:03:09 UTC
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>
Comment 289 Jonathan Druart 2014-05-01 08:03:29 UTC
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>
Comment 290 Jonathan Druart 2014-05-01 08:03:39 UTC
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>
Comment 291 Jonathan Druart 2014-05-01 08:03:56 UTC
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>
Comment 292 Jonathan Druart 2014-05-01 08:04:07 UTC
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>
Comment 293 Jonathan Druart 2014-05-01 08:04:16 UTC
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>
Comment 294 Jonathan Druart 2014-05-01 08:04:35 UTC
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>
Comment 295 Jonathan Druart 2014-05-01 08:04:48 UTC
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>
Comment 296 Jonathan Druart 2014-05-01 08:05:05 UTC
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>
Comment 297 Jonathan Druart 2014-05-01 08:05:17 UTC
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>
Comment 298 Jonathan Druart 2014-05-01 08:05:25 UTC
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>
Comment 299 Jonathan Druart 2014-05-01 08:05:42 UTC
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>
Comment 300 Jonathan Druart 2014-05-01 08:05:58 UTC
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>
Comment 301 Jonathan Druart 2014-05-01 08:06:25 UTC
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.
Comment 302 Jonathan Druart 2014-05-01 08:07:42 UTC
I reuploaded patches from my local branch, so without the Katrin's SO.
Last patch set contains the SO!
Sorry for the confusion.
Comment 303 Galen Charlton 2014-05-04 20:40:52 UTC
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.
Comment 304 Galen Charlton 2014-05-04 21:58:03 UTC
Pushed to master.  Thanks, Jonathan!
Comment 305 Katrin Fischer 2014-11-02 08:37:57 UTC
*** Bug 9597 has been marked as a duplicate of this bug. ***