Bug 13320 - Some prices calculation are done in template side
Summary: Some prices calculation are done in template side
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 13001
Blocks: 12826
  Show dependency treegraph
 
Reported: 2014-11-21 11:29 UTC by Jonathan Druart
Modified: 2015-12-03 22:11 UTC (History)
2 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 13320: Move price calculation from tt to pl (4.57 KB, patch)
2014-11-21 11:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13320: Fix "Tax inc." vs "Tax exc." (1.99 KB, patch)
2014-12-01 13:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13320: Move price calculation from tt to pl (4.63 KB, patch)
2014-12-28 07:54 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 13320: Fix "Tax inc." vs "Tax exc." (2.04 KB, patch)
2014-12-28 07:54 UTC, Chris Cormack
Details | Diff | Splinter Review
invoice - master (72.74 KB, image/png)
2014-12-28 09:48 UTC, Katrin Fischer
Details
parcel - patch applied (64.16 KB, image/png)
2014-12-28 09:49 UTC, Katrin Fischer
Details
on master (34.47 KB, image/png)
2014-12-29 13:12 UTC, Jonathan Druart
Details
Bug 13320: Move price calculation from tt to pl (4.66 KB, patch)
2015-01-21 08:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13320: Fix "Tax inc." vs "Tax exc." (1.50 KB, patch)
2015-01-21 08:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Screenshot of parcel page - TOTAL wrong? (14.55 KB, image/png)
2015-01-27 21:36 UTC, Katrin Fischer
Details
[PASSED QA] Bug 13320: Move price calculation from tt to pl (4.73 KB, patch)
2015-01-27 21:55 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 13320: Fix "Tax inc." vs "Tax exc." (1.56 KB, patch)
2015-01-27 21:56 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2014-11-21 11:29:01 UTC

    
Comment 1 Jonathan Druart 2014-11-21 11:37:15 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2014-12-01 13:19:28 UTC Comment hidden (obsolete)
Comment 3 Chris Cormack 2014-12-28 07:54:02 UTC Comment hidden (obsolete)
Comment 4 Chris Cormack 2014-12-28 07:54:16 UTC Comment hidden (obsolete)
Comment 5 Katrin Fischer 2014-12-28 09:48:40 UTC
Created attachment 34759 [details]
invoice - master

Hi Jonathan, 

I hope I applied all the dependencies correctly:
12896
12969
13001
13320

I see a difference in the total, the actual cost seems not to be used for the total amount, I am attaching 2 screenshots - one for master and one with the patches applied.
Comment 6 Katrin Fischer 2014-12-28 09:49:49 UTC
Created attachment 34760 [details]
parcel - patch applied

Both screenshots show the parcel page.
Comment 7 Jonathan Druart 2014-12-29 13:12:54 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2014-12-29 13:17:06 UTC
Comment on attachment 34812 [details]
on master

forget that.
Comment 9 Jonathan Druart 2014-12-29 13:34:44 UTC
(In reply to Katrin Fischer from comment #5)
> Created attachment 34759 [details]
> invoice - master
> 
> Hi Jonathan, 
> 
> I hope I applied all the dependencies correctly:
> 12896
> 12969
> 13001
> 13320

Bug 13001 depends on bug 12976, which is missing.
The issue is fixed when the patches from bug 12976 are applied.
Comment 10 Katrin Fischer 2015-01-04 23:15:52 UTC
As some of the dependencies still need QA, I will work on them first.
Comment 11 Katrin Fischer 2015-01-21 07:24:47 UTC
Hi Jonathan, 

it looks like all dependencies are in master now, but I got problems applying this patch:

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 13320: Move price calculation from tt to pl
fatal: sha1 information is lacking or useless (acqui/parcel.pl).

Can you take a look?
Comment 12 Jonathan Druart 2015-01-21 08:38:24 UTC
Bug 13001 is not in master, the status seems to be wrong.
Comment 13 Jonathan Druart 2015-01-21 08:58:59 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2015-01-21 08:59:02 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2015-01-21 08:59:24 UTC
Conflicts with bug 11944 fixed.
Comment 16 Katrin Fischer 2015-01-27 21:36:28 UTC
Created attachment 35563 [details]
Screenshot of parcel page - TOTAL wrong?

Hi Jonathan,

this applies cleanly now and the values are the same as before - so this is looking good. But I am still wondering - could you take a look at the attachd screenshot?

The sum of the TOTAL column doesn't make sense?
Comment 17 Katrin Fischer 2015-01-27 21:55:58 UTC
Created attachment 35564 [details] [review]
[PASSED QA] Bug 13320: Move price calculation from tt to pl

In parcel.tt, total are calculated for subtotal.

This could be done in the pl script for more consistency.

Test plan:
Go on a parcel page with several already received orders.
Orders must be linked to different funds.
If possible ecost and unitprice (price on ordering and on receiving)
should changed (different values will be displayed in the table).

The values displayed before and after the patch must be the same.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 18 Katrin Fischer 2015-01-27 21:56:14 UTC
Created attachment 35565 [details] [review]
[PASSED QA] Bug 13320: Fix "Tax inc." vs "Tax exc."

The parcel page always displayes "Tax exc." even when values don't
include taxes.

Test plan:
On the parcel page, verify that the string "Tax *" is correct.
This appears in the already received order table.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 19 Katrin Fischer 2015-01-27 21:59:01 UTC
Patch doesn't change display apart from optional formatting of the amounts. 
So I am passing it - still curious about the TOTAL column, maybe something for yet another tax bug?
Comment 20 Tomás Cohen Arazi 2015-02-04 18:55:46 UTC
Patches pushed to master.

Thanks Jonathan!
Comment 21 Tomás Cohen Arazi 2015-02-05 17:42:14 UTC
(In reply to Katrin Fischer from comment #19)
> Patch doesn't change display apart from optional formatting of the amounts. 
> So I am passing it - still curious about the TOTAL column, maybe something
> for yet another tax bug?

Jonathan?
Comment 22 Katrin Fischer 2015-02-05 22:07:12 UTC
Jonathan and I discussed it on IRC and I think we agreed that some other patch in the series will fix it, so there was no need for a new bug report
Comment 23 Jonathan Druart 2015-02-06 08:52:29 UTC
Yes, as I noted in the commit msg "The values displayed before and after the patch must be the same".
I tested on the 13323 branch, and the values are fixed.