I have to say that this is sponsored by our client, http://www.ccsr.qc.ca/.

Also, this isn't necessary to get the patch pushed, but it would be awesome to also show the librarian/branch who collected the money (maybe a future enhancement to this feature).

Comment on attachment 5641 [details] [review]
New patch with date format fixes and better indentation

I'm afraid that this was not meant to be sent yet.
I will update this bug once we can really share it.

Looks like I forgot to update the bug.
I hope it stills applies to master.

Is this code unencumbered, license-wise, and suitable for inclusion at this time into Koha's GPL codebase?

Yes. Sorry if I was unclear. Also sorry to have copied the header form another file. The Katipo copyright is of course wrong and should be removed.

The report works only with "Pay fines" 
and not when we create manual invoice or credit

QA comments (even if the patch is not signed off, I watched it with Mathilde a few minuts ago):

+    Join borrowers t2 On t1.borrowernumber=t2.borrowernumber
+    Where amount<0  AND accounttype like 'Pay' AND CAST(t1.date AS DATE) between ? AND ?
=> you must write SQL keywords in capital cases (WHERE, JOIN,...)

(In reply to comment #10)
> The report works only with "Pay fines" 
> and not when we create manual invoice or credit

A "manual invoice" should not appear in this report, we agree. but a manual credit should, that's why the patch failed QA

The SQL request should not be "accounttype ='Pay' " but "accounttype in('L','Pay')"

Hi Maxime, 
Thanks for your patch.
Sorry but can you repropose it with tabulation characters replaced with 4 spaces please ?
You can read the Koha guidelines on the wiki: http://wiki.koha-community.org/wiki/Coding_Guidelines#Perl

(In reply to comment #12)
> (In reply to comment #10)
> > The report works only with "Pay fines" 
> > and not when we create manual invoice or credit
> 
> A "manual invoice" should not appear in this report, we agree. but a manual
> credit should, that's why the patch failed QA
> 
> The SQL request should not be "accounttype ='Pay' " but "accounttype
> in('L','Pay')"

Just realized that I blindly copy and pasted this, but I think it's wrong. L is for lost. Manual credits are 'C' or 'FOR'.
I will send a new patch soon if this makes sense to you.

Changing this bug's status back to "assigned" since it appears we're waiting for a new patch?

We made many changes to this report recently. I will try to send a new patch soon.

Comment on attachment 36476 [details]
[SIGNED-OFF] Bug 6934 New report Cash Register Statistics

0 of attachment 36476 [details]:
-----------------------------------------------------------------

Hi Simith,

I have added a few comments from code review - please fix in a follow-up. I have only marked the first occurrence of each, please also look below and above to make sure to get them all. 

Thx!

::: koha-tmpl/intranet-tmpl/prog/en/includes/reports-menu.inc
@@ +10,4 @@
>  		<li><a href="/cgi-bin/koha/reports/catalogue_stats.pl">Catalog</a></li>
>  		<li><a href="/cgi-bin/koha/reports/issues_stats.pl">Circulation</a></li>
>          <li><a href="/cgi-bin/koha/reports/serials_stats.pl">Serials</a></li>
> +        <li><a href="/cgi-bin/koha/reports/cash_register_stats.pl">Cash Register</a></li>

Please fix capitalization here and in the other template files to obey our rules. Thx!

::: koha-tmpl/intranet-tmpl/prog/en/modules/reports/cash_register_stats.tt
@@ +7,5 @@
> +<script type="text/javascript" src="[% themelang %]/js/datatables.js"></script>
> +<script type="text/javascript" id="js">$(document).ready(function() {
> + $(document).ready(function() {
> +    $("#tbl_cash_register_stats").dataTable($.extend(true, {}, dataTablesDefaults, {
> +        "iDisplayLength": 50

Be careful, without adding a paging option here, the paging on the table is broken (see recent 'paging' bugs for examples on how to fix)

@@ +143,5 @@
> +                        <option value="F">Fine</option>
> +                        [% END %]
> +
> +                        [% IF transaction_type == "FU" %]
> +                        <option value="FU" selected="selected">Fine - long period</option>

Please check fine descriptions with those in the other templates - I think this one is not quite right.

@@ +233,5 @@
> +        <thead>
> +        <tr>
> +            <th>Manager name</th>
> +            <th>Borrower cardnumber</th>
> +            <th>Borrower name</th>

Please use patron instead of borrower.

@@ +234,5 @@
> +        <tr>
> +            <th>Manager name</th>
> +            <th>Borrower cardnumber</th>
> +            <th>Borrower name</th>
> +            <th>Branch</th>

Please use library instead of branch.

@@ +240,5 @@
> +            <th>Transaction type</th>
> +            <th>Amount</th>
> +            <th>Biblio title</th>
> +            <th>Barcode</th>
> +            <th>Document type</th>

Please use item type.

::: reports/cash_register_stats.pl
@@ +14,5 @@
> +# with Koha; if not, write to the Free Software Foundation, Inc.,
> +# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> +
> +use strict;
> +use warnings;

Please use Modern::Perl; instead.

@@ +22,5 @@
> +use C4::Reports;
> +use C4::Output;
> +use C4::Koha;
> +use C4::Circulation;
> +use C4::Dates qw/format_date format_date_in_iso/;

Please avoid use of C4::Dates as it's deprecated.

@@ +45,5 @@
> +my $output           = $input->param("output");
> +my $basename         = $input->param("basename");
> +my $transaction_type = $input->param("transaction_type") || 'ACT';
> +my $branchcode       = $input->param("branch") || C4::Context->userenv->{'branch'};
> +our $sep = ",";

I think the separator should not be hardcoded, it would be better to use a pull down like on the other reports or using the delimiter system preference.

@@ +49,5 @@
> +our $sep = ",";
> +
> +$template->param(
> +    do_it => $do_it,
> +    DHTMLcalendar_dateformat => C4::Dates->DHTMLcalendar(),

We are no longer using DHTML calendar, please use the TT Dates plugin instead.

Hi Genevieve

Sorry but patch fails

This is when invalid date is enter in both dates.

Marc can help you with this kind of issues. Or see bugs related with Remove C4::Dates.

Software error:

The 'month' parameter ("32") to DateTime::new did not pass the 'an integer between 1 and 12' callback
 at /usr/lib/i386-linux-gnu/perl5/5.20/DateTime.pm line 197.
	DateTime::new(undef, "month", 32, "minute", 0, "hour", 0, "year", 2015, ...) called at /var/koha/koha/Koha/DateUtils.pm line 164
	Koha::DateUtils::dt_from_string("12/32/2015", "metric") called at /var/koha/koha/reports/cash_register_stats.pl line 75

Another think is patch does not follow dateformat sypref at all

I can see "Cash Register statistics 2015-11-12 to 2015-11-12" in form title
but in interval from 12/11/2015 to 12/11/2015, both most display equal.

Regards

Fix capitalization in form title "Cash Register statistics 2015-11-12 to 2015-11-12"

Patch still apply, cvs exported successful data in screen display OK.

(In reply to Héctor Eduardo Castro Avalos from comment #26)
(...)
> Marc can help you with this kind of issues. Or see bugs related with Remove
> C4::Dates.
(...)

- Jonathan introduced Bug 15166 - Make output_pref accept a string as parameter.
  An example is to be found in
  acqui/invoice.pl  line 84 (with Bug 14946 applied).

- Todays date as dt: dt_from_string without any params. See example from Bug
  14946, acqui/histsearch.pl, line 81:  	

my $from_placed_on = eval { dt_from_string( $input->param('from') ) } || dt_from_string;

according to comment #26 it's more failed QA than need discussion

Thanks for the tips, I obsoleted the previous patch and I fix the rest in it.
I understood the KohaDates better, thanks really.

There are some features and improvements that I need to add, it will come in another path soon.

Thanks for the tips, I obsoleted the previous patch and I fix the rest in it.
I understood the KohaDates better, thanks really.

There are some features and improvements that I need to add, it will come in another path soon.

Sorry, I didn't notice the comments #33 was duplicated.

This patch is ready to be tested :)

QA comments:
1/ +function isNull(f,noalert) {
not used later

2/ The datepicker from/to should be done using the datepickerfrom and datepickerto css class (see acqui/histsearch.tt for an example).

3/ Why do we have to fill a date?

4/ Ergonomic: the date interval is in a table with only 1 column and 1 row, it could be better to move them in the same fieldset as the 2 dropdown lists.

5/ $fullreportname is only used once, not needed.

6/ +my $fromDate = dt_from_string;
and later
+    beginDate        => dt_from_string($fromDate),
You are passing a DT to dt_from_string, a string is expected.

7/ Remove the unneeded commented lines ("^###")

8/ The csv file should be generated using Text::CSV::Encoded

9/ The column names should be retrieved using C4::Reports::Guided to get the translated values

Created attachment 48642 [details]
Cash register image

Hi Rémi

Still capitalization in reports-menu.inc

>        <li><a href="/cgi-bin/koha/reports/serials_stats.pl">Serials</a></li>
>        <li><a href="/cgi-bin/koha/reports/cash_register_stats.pl">Cash Register</a></li>

I could observe this:

When you start both calendars are set with the current date, i.e., 2016-03-03, but if you change the "From:" date to 2016-01-01, the date in "To:" change also and if you try to change it, for example, to the current date (2016-03-03), you cannot do it at all with the calendar, just manually.

See image attached.

Of course otherwise works correctly. About QA comments (comment 38) we need to await Jonathan's reply.

Regards

(In reply to Héctor Eduardo Castro Avalos from comment #41)
> Of course otherwise works correctly. About QA comments (comment 38) we need
> to await Jonathan's reply.

No :)
Please signoff if everything works as expected.
Then I, or another QAer, will be able to qa it again.

Hi Rémi

About my comment 40, i don't know if is that the behavior expected, or, needs to be corrected?

Is the patch ready to sign-off? If yes, I'll sign-off immediately.

Regards

This bug should be ready to be signed-off now, thank you for noticing that the calendar weren't functionning as they should have. I Fixed that now.

(In reply to Rémi Mayrand-Provencher from comment #45)
> This bug should be ready to be signed-off now, thank you for noticing that
> the calendar weren't functionning as they should have. I Fixed that now.

Sorry Rémi but doesn't work

The message presented is:

Form not submitted because of the following problem(s)
-------------------------------------------------------------------


- Dates cannot be empty

Probably about this bug: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15084

Really sorry, it was my mistake, I should have tested it again just before sending my patch.

Rémi just another issue, when exporting there are problem with utf-8:

branchname	date	accounttype	amount	title	barcode	itype
Midway	2016-01-25T00:00:00	Rent	5	A lot like love /	2015-0004	
Midway	2016-01-28T00:00:00	Rent	5	Antología de entremeses del Siglo de Oro /	2016-0001	

But I think IMHO this could be treated in another bug.

(In reply to Héctor Eduardo Castro Avalos from comment #52)
> Rémi just another issue, when exporting there are problem with utf-8:
> 
> branchname	date	accounttype	amount	title	barcode	itype
> Midway	2016-01-25T00:00:00	Rent	5	A lot like love /	2015-0004	
> Midway	2016-01-28T00:00:00	Rent	5	Antología de entremeses del Siglo de Oro
> /	2016-0001	
> 
> But I think IMHO this could be treated in another bug.

No, an enhancement won't be pushed with known encoding issues.

I agree with Jonathan - please fix the encoding issue!

Fixed encoding, issue, changing status to needs signoff, thank you!

There are some complaints from the QA script, I can take care of the tabs, but not sure about the last complaint:

 FAIL	reports/cash_register_stats.pl
   OK	  critic
   FAIL	  forbidden patterns
		forbidden pattern: tab char (line 176)
		forbidden pattern: tab char (line 159)
		forbidden pattern: tab char (line 162)
		forbidden pattern: tab char (line 160)
		forbidden pattern: tab char (line 156)
		forbidden pattern: tab char (line 163)
		forbidden pattern: tab char (line 179)
		forbidden pattern: tab char (line 178)
		forbidden pattern: tab char (line 157)
		forbidden pattern: tab char (line 180)
		forbidden pattern: tab char (line 164)
		forbidden pattern: tab char (line 174)
		forbidden pattern: tab char (line 181)
		forbidden pattern: tab char (line 177)
		forbidden pattern: tab char (line 158)
		forbidden pattern: tab char (line 171)
		forbidden pattern: tab char (line 161)
		forbidden pattern: tab char (line 175)
   OK	  pod
   OK	  spelling
		0
   FAIL	  valid
		Useless use of hash element in void context

I will upload a follow-up for fixing tabs and a capitalization issue. 

But there are some other things that need to be looked at and fixed by the author:
- Remaining QA tools complaint
- What is the definition of an "Active transaction"? (was not clear to me, so asking :) )
- The CSV when exported won't open automatically in LibreOffice for me as all the other reports do - I think something is odd there. Firefox reports it as 'html document'. It's also missing the option to choose a file format and separator for the file as the other reports do.
- when you limit the search, the total amount is no longer correct. On the acq start page there is an example where this works, but not a blocker now.
- itype column is empty in the exported CSV file.
- Please make the CSV header column translatable - take a look at acq for examples (late orders or similar).
- Please use the new TT plugin for formatting the amounts - examples can be found in the acquisition module.
- Please use the TT plugin to display the item type descriptions instead of the codes.

Please come back with this and a follow-up!

Hi Bouzid

For me the patch fails in master when apply

>Applying: Bug 6934 New report Cash Register Statistics
>Using index info to reconstruct a base tree...
>Falling back to patching base and 3-way merge...
>Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/reports/reports-home.tt
>CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/reports/reports-home.tt
>Failed to merge in the changes.
>Patch failed at 0001 Bug 6934 New report Cash Register Statistics

Do I need to apply all patches or just few of them?

Regards

Hi Hèctor,
You need apply all patch.
For this conflict, I will rebase it.
Bouzid.

Created attachment 53582 [details] [review]
[SIGNED-OFF] Bug 6934 New report Cash Register Statistics

This patch creates a new report that outputs cash register stats for a specific period.

Changes:

koha-tmpl/intranet-tmpl/prog/en/includes/reports-menu.inc               - Added cash register report link
koha-tmpl/intranet-tmpl/prog/en/modules/reports/cash-register_stats.tt  - new file
koha-tmpl/intranet-tmpl/prog/en/modules/reports/reports-home.tt         - Added cash register report link
reports/cash-register_stats.pl                                          - new file

Testing:

I Apply the patch

0) Go to reports home page (Home › Reports);
1) Validate link << Cash Register >> under Statistics wizards;
2) Click link;
3) Validate page with three fieldsets;
4) Chose a date interval, a transaction type, a library and an output;
5) Validade the result.

Signed-off-by: Nicole <nicole@bywatersolutions.com>

Created attachment 53583 [details] [review]
[SIGNED-OFF]Bug 6934: Fix code in CashRegisterStats (dataTables pagination, more accurate descriptions, add a delimiter pull down, change C4::Dates to Koha::DateUtils)

I cleaned up the code according to comment #23. I got rid of DHTMLcalendar_dateformat,
beacause it wasn't use in cash_register_stats.tt. C4::Dates is deprecated,
Koha::DateUtils is now used.
Some column names are changed and the pagination for the dataTables is fixed.

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>

Created attachment 53584 [details] [review]
[SIGNED-OFF]Bug 6934: New features, note added to transaction information, total caculated for negative types of transaction

To be more specific, the column note from accountlines is now displayed in the table of the transactions.
The grand total is now calculated for neagtive types like Credit or Payment and isn't counted for Write off types.
Credit (return item) 'CR' has been added to drop down of transaction type

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>
Works as advertised

Created attachment 53585 [details] [review]
[SIGNED-OFF]Bug 6934: QA Follow-up

Tried to address every single thing that QA mentioned :
1)Removed isNull function
2)Changed datepicker properly
3)We have to fill in two dates not to be flooded with old entries if we don't want to.
4)The first two fieldsets are now in one bigger field set
5)Removed the use of $fullreportname variable
6)Removed the use of dt_from_string when it already recieved a DT.
7)Removed comments starting with "###" in cash_register_stats.pl
8)Used Text::CSV::Encoded to generate the CSV file (used guided_reports.pl as an example)
9)I am not sure if using C4::Reports::Guided is the right choice here. I instead used the same column names as the one recieved by the query in cash_register_stats because this report uses columns from 5 different tables in addition of using both firstname and surname twice.

For the ninth point, I would like to know more about C4::Reports::Guided if you still think using it would be a better solution. Thank you!

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>

Created attachment 53586 [details] [review]
[SIGNED-OFF]Bug 6934 Fixed Calendar and removed used of unexisting and unused C4::Budgets getCurrency

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>

Created attachment 53587 [details] [review]
[SIGNED-OFF]Bug 6934: second fix to calendars

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>

Created attachment 53588 [details] [review]
[SIGNED-OFF]Bug 6934: fixed encoding issue

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>
Encoding issue fixed

fixed conflicts, patches should apply correctly.

Created attachment 53650 [details] [review]
[SIGNED-OFF]Bug 6934: QA follow-up

- fixed tabs
- fixed capitalization on reports start page

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>

Created attachment 53651 [details] [review]
[SIGNED-OFF]Bug 6934: fix problem when export it open in LibreOffice - show values of itype and use TT plugin

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>

Created attachment 53652 [details] [review]
[SIGNED-OFF]Bug 6934: Make CSV header column translatable and add total amounts in csv file

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>

Created attachment 53653 [details] [review]
[SIGNED-OFF]Bug 6934: Fix the href for datables in link rel stylesheet

Fix the new path for 'datatables.css'

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>

Hi Bouzid

For me this is Ok.

Just explain what does "Active transaction" mean? QA comment 59

Regards

Miraculously, this still applies :) Can you please answer the question asked in comment#92?

Please also fix these errors pointed out by the QA script. Other problems can be ignored for now ("Tie::Hash::FIELDS"...)

 FAIL	koha-tmpl/intranet-tmpl/prog/en/modules/reports/reports-home.tt
   FAIL	  forbidden patterns
		forbidden pattern: tab char (line 39)
		forbidden pattern: tab char (line 31)
		forbidden pattern: tab char (line 36)
		forbidden pattern: tab char (line 32)
		forbidden pattern: tab char (line 34)
		forbidden pattern: tab char (line 37)
		forbidden pattern: tab char (line 33)
		forbidden pattern: tab char (line 35)
   OK	  spelling
   OK	  tt_valid
   OK	  valid_template

 FAIL	reports/cash_register_stats.pl
   FAIL	  critic
		"$q_errors" is declared but not used at line 140, column 9. Unused variables clutter code and make it harder to read.
		"$content" is declared but not used at line 146, column 9. Unused variables clutter code and make it harder to read.

I will keep an eye on this and try to continue as soon as the follow up is there.

Tiny thing: the total amount in the last grey line of the table is not formatted.

(In reply to Katrin Fischer from comment #59)

> - What is the definition of an "Active transaction"? (was not clear to me,
> so asking :) )

The idea behind "Active transactions" is to help balance cash register transactions. In other words, how much goes in or how much goes out the cash register. The active transactions group what patrons paid or gave as credit to the library, which means what went IN the register.

The code works this way : it takes all transactions where account types  are NOT "F" (Fine), "FU" (Fine unpaid), "FOR" (Forgiven), "M" (Sundry), "L" (Lost).

Hi Bouzid

One comment:

In bug 16964 I fix the capitalization up for "Report Plugins", right now the bug is in Passed QA. I will try to fix it again in your last patch.

Regards

Hi Héctor,
ok, thanks.
Regards

Passing QA-Test Tool I got:

FAIL	reports/cash_register_stats.pl
   OK	  critic
   OK	  forbidden patterns
   OK	  pod
   OK	  spelling
		0
   FAIL	  valid
		Name "Cache::RemovalStrategy::FIELDS" used only once: possible typo 
		Name "Tie::Hash::FIELDS" used only once: possible typo 
		Name "Cache::RemovalStrategy::LRU::FIELDS" used only once: possible typo 
		Use of uninitialized value $_[0] in lc

But I think this is a problem caused for bug 11998 and explained for Jonathan in bug 16104

if you test with: $ perl -cw reports/cash_register_stats.pl
you obtain:
Use of uninitialized value $_[0] in lc at /usr/share/perl5/Cache.pm line 485.
Use of uninitialized value $_[0] in lc at /usr/share/perl5/Cache.pm line 485.
Use of uninitialized value $_[0] in lc at /usr/share/perl5/Cache.pm line 485.
Name "Tie::Hash::FIELDS" used only once: possible typo at /usr/share/perl/5.14/fields.pm line 135.
Name "Cache::RemovalStrategy::FIELDS" used only once: possible typo at /usr/share/perl/5.14/fields.pm line 135.
Name "Cache::RemovalStrategy::LRU::FIELDS" used only once: possible typo at /usr/share/perl/5.14/fields.pm line 135.
reports/cash_register_stats.pl syntax OK

But all this errors are from bug 11998

Still
FAIL	koha-tmpl/intranet-tmpl/prog/en/modules/reports/reports-home.tt
   FAIL	  forbidden patterns
		forbidden pattern: tab char (line 36)
		forbidden pattern: tab char (line 37)
		forbidden pattern: tab char (line 35)
		forbidden pattern: tab char (line 34)
		forbidden pattern: tab char (line 31)
		forbidden pattern: tab char (line 33)
		forbidden pattern: tab char (line 39)
		forbidden pattern: tab char (line 32)

But I will fix it up in the last patch

Created attachment 53780 [details] [review]
[SIGNED-OFF]Bug6934: fix errors pointed out by the QA script

Also fix forbidden patterns in koha-tmpl/intranet-tmpl/prog/en/modules/reports/reports-home.tt

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>

(In reply to Francois Charbonnier from comment #96)
> (In reply to Katrin Fischer from comment #59)
> 
> > - What is the definition of an "Active transaction"? (was not clear to me,
> > so asking :) )
> 
> The idea behind "Active transactions" is to help balance cash register
> transactions. In other words, how much goes in or how much goes out the cash
> register. The active transactions group what patrons paid or gave as credit
> to the library, which means what went IN the register.
> 
> The code works this way : it takes all transactions where account types  are
> NOT "F" (Fine), "FU" (Fine unpaid), "FOR" (Forgiven), "M" (Sundry), "L"
> (Lost).

Sorry, I still don't get it :( The list of excluded fine types looks a bit arbitrary to me - what about custom MANUAL_INV values? And what is left by excluding those?

(In reply to Katrin Fischer from comment #102)
> (In reply to Francois Charbonnier from comment #96)
> > (In reply to Katrin Fischer from comment #59)
> > 
> > > - What is the definition of an "Active transaction"? (was not clear to me,
> > > so asking :) )
> > 
> > The idea behind "Active transactions" is to help balance cash register
> > transactions. In other words, how much goes in or how much goes out the cash
> > register. The active transactions group what patrons paid or gave as credit
> > to the library, which means what went IN the register.
> > 
> > The code works this way : it takes all transactions where account types  are
> > NOT "F" (Fine), "FU" (Fine unpaid), "FOR" (Forgiven), "M" (Sundry), "L"
> > (Lost).
> 
> Sorry, I still don't get it :( The list of excluded fine types looks a bit
> arbitrary to me - what about custom MANUAL_INV values? And what is left by
> excluding those?

Hey Katrin,

But you do get that "active transaction" groups all transactions where patrons give money to the library, right ?

The logic should be different. What do you think of grouping the relevant account types, "Pay" and "C" (are there other?) instead of excluding this list?

I think that sounds more logical! Also safer with the option of new types being added - not so likely we change the types for paying and credit.

Maybe we could also think about a more 'speaking' name.

> But you do get that "active transaction" groups all transactions where
> patrons give money to the library, right ?

I don't think "active transaction" has any inherent meaning. What about something like "All payments to the library" ?

Some other things I notice:

- The page is looking for datatables.js in the wrong place. It's now at intranet-tmpl/prog/js/datatables.js.

- The table header "Patron cardnumber" should be "Patron card number."

Please take a look at comment#103 - comment#105 for a hopefully last follow-up.

Created attachment 54010 [details] [review]
Bug6934: rename active transaction with  All payments to the library and grouping in it the relevant account types 'Pay' 'C'

    rename Patron carnumber on Patron card number
    fix src databales.js

Treating the last as qa follow-up.

Created attachment 54088 [details] [review]
[PASSED QA] Bug 6934: New report Cash Register Statistics

This patch creates a new report that outputs cash register stats for a specific period.

Changes:

koha-tmpl/intranet-tmpl/prog/en/includes/reports-menu.inc               - Added cash register report link
koha-tmpl/intranet-tmpl/prog/en/modules/reports/cash-register_stats.tt  - new file
koha-tmpl/intranet-tmpl/prog/en/modules/reports/reports-home.tt         - Added cash register report link
reports/cash-register_stats.pl                                          - new file

Testing:

I Apply the patch

0) Go to reports home page (Home › Reports);
1) Validate link << Cash Register >> under Statistics wizards;
2) Click link;
3) Validate page with three fieldsets;
4) Chose a date interval, a transaction type, a library and an output;
5) Validade the result.

Signed-off-by: Nicole <nicole@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 54089 [details] [review]
[PASSED QA] Bug 6934: Fix code in CashRegisterStats (dataTables pagination, more accurate descriptions, add a delimiter pull down, change C4::Dates to Koha::DateUtils)

I cleaned up the code according to comment #23. I got rid of DHTMLcalendar_dateformat,
beacause it wasn't use in cash_register_stats.tt. C4::Dates is deprecated,
Koha::DateUtils is now used.
Some column names are changed and the pagination for the dataTables is fixed.

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 54090 [details] [review]
[PASSED QA] Bug 6934: New features, note added to transaction information, total caculated for negative types of transaction

To be more specific, the column note from accountlines is now displayed in the table of the transactions.
The grand total is now calculated for neagtive types like Credit or Payment and isn't counted for Write off types.
Credit (return item) 'CR' has been added to drop down of transaction type

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>
Works as advertised

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 54091 [details] [review]
[PASSED QA] Bug 6934: QA Follow-up

Tried to address every single thing that QA mentioned :
1)Removed isNull function
2)Changed datepicker properly
3)We have to fill in two dates not to be flooded with old entries if we don't want to.
4)The first two fieldsets are now in one bigger field set
5)Removed the use of $fullreportname variable
6)Removed the use of dt_from_string when it already recieved a DT.
7)Removed comments starting with "###" in cash_register_stats.pl
8)Used Text::CSV::Encoded to generate the CSV file (used guided_reports.pl as an example)
9)I am not sure if using C4::Reports::Guided is the right choice here. I instead used the same column names as the one recieved by the query in cash_register_stats because this report uses columns from 5 different tables in addition of using both firstname and surname twice.

For the ninth point, I would like to know more about C4::Reports::Guided if you still think using it would be a better solution. Thank you!

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 54092 [details] [review]
[PASSED QA] Bug 6934: Fixed Calendar and removed used of unexisting and unused C4::Budgets getCurrency

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 54093 [details] [review]
[PASSED QA] Bug 6934: second fix to calendars

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 54094 [details] [review]
[PASSED QA] Bug 6934: fixed encoding issue

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>
Encoding issue fixed

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 54095 [details] [review]
[PASSED QA] Bug 6934: QA follow-up

- fixed tabs
- fixed capitalization on reports start page

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 54096 [details] [review]
[PASSED QA] Bug 6934: fix problem when export it open in LibreOffice - show values of itype and use TT plugin

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 54097 [details] [review]
[PASSED QA] Bug 6934: Make CSV header column translatable and add total amounts in csv file

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 54098 [details] [review]
[PASSED QA] Bug 6934: Fix the href for datables in link rel stylesheet

Fix the new path for 'datatables.css'

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 54099 [details] [review]
[PASSED QA] Bug 6934: fix errors pointed out by the QA script

Also fix forbidden patterns in koha-tmpl/intranet-tmpl/prog/en/modules/reports/reports-home.tt

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 54100 [details] [review]
[PASSED QA] Bug 6934: rename active transaction with All payments to the library and grouping in it the relevant account types 'Pay' 'C'

    rename Patron carnumber on Patron card number
    fix src databales.js

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Can someone clean up these patches a bit before I push? I see half are the signed off ones, half are passed qa, and one in the middle that is neither ; )

Created attachment 55939 [details] [review]
Bug 6934 - Fix problem  C4::Branch::GetBranchesLoop not exist

I clean all patches and fix a new minor problem, GetBranchesLoop not exist.

Created attachment 55988 [details] [review]
[PASSED QA] Bug 6934: New report Cash Register Statistics

This patch creates a new report that outputs cash register stats for a specific period.

Changes:

koha-tmpl/intranet-tmpl/prog/en/includes/reports-menu.inc               - Added cash register report link
koha-tmpl/intranet-tmpl/prog/en/modules/reports/cash-register_stats.tt  - new file
koha-tmpl/intranet-tmpl/prog/en/modules/reports/reports-home.tt         - Added cash register report link
reports/cash-register_stats.pl                                          - new file

Testing:

I Apply the patch

0) Go to reports home page (Home › Reports);
1) Validate link << Cash Register >> under Statistics wizards;
2) Click link;
3) Validate page with three fieldsets;
4) Chose a date interval, a transaction type, a library and an output;
5) Validade the result.

Signed-off-by: Nicole <nicole@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 55989 [details] [review]
[PASSED QA] Bug 6934: Fix code in CashRegisterStats (dataTables pagination, more accurate descriptions, add a delimiter pull down, change C4::Dates to Koha::DateUtils)

I cleaned up the code according to comment #23. I got rid of DHTMLcalendar_dateformat,
beacause it wasn't use in cash_register_stats.tt. C4::Dates is deprecated,
Koha::DateUtils is now used.
Some column names are changed and the pagination for the dataTables is fixed.

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 55990 [details] [review]
[PASSED QA] Bug 6934: New features, note added to transaction information, total caculated for negative types of transaction

To be more specific, the column note from accountlines is now displayed in the table of the transactions.
The grand total is now calculated for neagtive types like Credit or Payment and isn't counted for Write off types.
Credit (return item) 'CR' has been added to drop down of transaction type

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>
Works as advertised

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 55991 [details] [review]
[PASSED QA] Bug 6934: QA Follow-up

Tried to address every single thing that QA mentioned :
1)Removed isNull function
2)Changed datepicker properly
3)We have to fill in two dates not to be flooded with old entries if we don't want to.
4)The first two fieldsets are now in one bigger field set
5)Removed the use of $fullreportname variable
6)Removed the use of dt_from_string when it already recieved a DT.
7)Removed comments starting with "###" in cash_register_stats.pl
8)Used Text::CSV::Encoded to generate the CSV file (used guided_reports.pl as an example)
9)I am not sure if using C4::Reports::Guided is the right choice here. I instead used the same column names as the one recieved by the query in cash_register_stats because this report uses columns from 5 different tables in addition of using both firstname and surname twice.

For the ninth point, I would like to know more about C4::Reports::Guided if you still think using it would be a better solution. Thank you!

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 55992 [details] [review]
[PASSED QA] Bug 6934: Fixed Calendar and removed used of unexisting and unused C4::Budgets getCurrency

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 55993 [details] [review]
[PASSED QA] Bug 6934: second fix to calendars

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 55994 [details] [review]
[PASSED QA] Bug 6934: fixed encoding issue

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>
Encoding issue fixed

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 55995 [details] [review]
[PASSED QA] Bug 6934: QA follow-up

- fixed tabs
- fixed capitalization on reports start page

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 55996 [details] [review]
[PASSED QA] Bug 6934: fix problem when export it open in LibreOffice - show values of itype and use TT plugin

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 55997 [details] [review]
[PASSED QA] Bug 6934: Make CSV header column translatable and add total amounts in csv file

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 55998 [details] [review]
[PASSED QA] Bug 6934: Fix the href for datables in link rel stylesheet

Fix the new path for 'datatables.css'

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 55999 [details] [review]
[PASSED QA] Bug 6934: fix errors pointed out by the QA script

Also fix forbidden patterns in koha-tmpl/intranet-tmpl/prog/en/modules/reports/reports-home.tt

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 56000 [details] [review]
[PASSED QA] Bug 6934: rename active transaction with All payments to the library and grouping in it the relevant account types 'Pay' 'C'

rename Patron carnumber on Patron card number
    fix src databales.js

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 56001 [details] [review]
[SIGNED-OFF]Bug 6934 - Fix problem C4::Branch::GetBranchesLoop not exist

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>

Rebase all patches to avoid merge conflicts and signed-off last patch

Created attachment 56763 [details] [review]
[PASSED QA] Bug 6934 - Fix problem C4::Branch::GetBranchesLoop not exist

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>

Signed-off-by: Katrin Fischer  <katrin.fischer@bsz-bw.de>

Created attachment 56764 [details] [review]
Bug 6934: Fix tiny capitalization issue missed before

Account Management Fee

Marked the last patch PQA and added another follow-up... let's hope it's the last one!

Pushed to master for for 16.11, thanks Simith, Genevieve, Remi, Katrin, Bouzid, and Hector!

Enhancement, skipping for 16.05.x

For the records (from Bug 18734):
- Export to CSV leads to an Internal Server error, see Bug 18734
- It can be fixed by changing line 174 from exit(1); to exit(0);
  But then a new issue appears: The export has an empty line above the header line, and the header line is mixed with and without double quotes:

mfirstname;card number;firstname;branchname;date;accounttype;amount;title;barcode;"itype"

Noting it here to make the original authors of this feature aware.