|
Description
Nick Clemens (kidclamp)
2018-04-24 20:56:55 UTC
Created attachment 74829 [details] [review] Bug 20656: Don't print lines that have no balance on print summary To test: 1 - Add at least two fines to a patron 2 - Pay off one of them 3 - Print summary - all 3 lines show 4 - Apply patch 5 - Print summary - only line with balance shows Created attachment 74830 [details] [review] Bug 20656: Better formatting for print summary This patch is a slightly edited c&p from boraccount.tt 1 - Manually add a credit with no notes to patron 2 - Print summary 3 - Inspect the line 4 - Note that td has extra newlines and no words 5 - Apply patch 6 - Print summary again 7 - Note better formatting I'm not sure it won't be considered as a regression by some libraries. Before your patch, Print summary shows the fine and its payment. After your patch, the fine and payment info doesn't appear anymore. On Summary page, the table showing information is labelled Account fines and payments. So 'payments' seems to be expected. I think I like the new display better. As it's supposed to be a summary displaying only open credits and fines makes sense to me, but the heading should then be changed to match the new display. Signing, but waiting for follow-up and maybe another opinion here. Created attachment 76775 [details] [review] Bug 20656: Don't print lines that have no balance on print summary To test: 1 - Add at least two fines to a patron 2 - Pay off one of them 3 - Print summary - all 3 lines show 4 - Apply patch 5 - Print summary - only line with balance shows Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 76776 [details] [review] Bug 20656: Better formatting for print summary This patch is a slightly edited c&p from boraccount.tt 1 - Manually add a credit with no notes to patron 2 - Print summary 3 - Inspect the line 4 - Note that td has extra newlines and no words 5 - Apply patch 6 - Print summary again 7 - Note better formatting Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Please rebase on actual master. Created attachment 79002 [details] [review] Bug 20656: Don't print lines that have no balance on print summary To test: 1 - Add at least two fines to a patron 2 - Pay off one of them 3 - Print summary - all 3 lines show 4 - Apply patch 5 - Print summary - only line with balance shows Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 79003 [details] [review] Bug 20656: Better formatting for print summary This patch is a slightly edited c&p from boraccount.tt 1 - Manually add a credit with no notes to patron 2 - Print summary 3 - Inspect the line 4 - Note that td has extra newlines and no words 5 - Apply patch 6 - Print summary again 7 - Note better formatting Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Condition is missing to display the biblio's title: account.accounttype != 'F' AND account.accounttype != 'FU' Created attachment 79024 [details] [review] Bug 20656: (follow-up) Don't display title for F or FU fines (it is in description) Comment on attachment 79003 [details] [review] Bug 20656: Better formatting for print summary Review of attachment 79003 [details] [review]: ----------------------------------------------------------------- ::: koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember-print.tt @@ +97,4 @@ > <caption>Account fines and payments</caption> > + <thead> > + <tr> > + <th class="title-string">Date</th> Why are you adding the class here? It is usually used for sorting in datatables, which is not this case. Created attachment 79095 [details] [review] Bug 20656: Don't print lines that have no balance on print summary To test: 1 - Add at least two fines to a patron 2 - Pay off one of them 3 - Print summary - all 3 lines show 4 - Apply patch 5 - Print summary - only line with balance shows Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 79096 [details] [review] Bug 20656: Better formatting for print summary This patch is a slightly edited c&p from boraccount.tt 1 - Manually add a credit with no notes to patron 2 - Print summary 3 - Inspect the line 4 - Note that td has extra newlines and no words 5 - Apply patch 6 - Print summary again 7 - Note better formatting Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 79097 [details] [review] Bug 20656: (follow-up) Don't display title for F or FU fines (it is in description) (In reply to Josef Moravec from comment #12) > > + <tr> > > + <th class="title-string">Date</th> > > Why are you adding the class here? It is usually used for sorting in > datatables, which is not this case. Bad copy/paste during rebase Created attachment 79138 [details] [review] Bug 20656: Don't print lines that have no balance on print summary To test: 1 - Add at least two fines to a patron 2 - Pay off one of them 3 - Print summary - all 3 lines show 4 - Apply patch 5 - Print summary - only line with balance shows Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 79139 [details] [review] Bug 20656: Better formatting for print summary This patch is a slightly edited c&p from boraccount.tt 1 - Manually add a credit with no notes to patron 2 - Print summary 3 - Inspect the line 4 - Note that td has extra newlines and no words 5 - Apply patch 6 - Print summary again 7 - Note better formatting Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 79140 [details] [review] Bug 20656: (follow-up) Don't display title for F or FU fines (it is in description) Signed-off-by: Josef Moravec <josef.moravec@gmail.com> (In reply to Nick Clemens from comment #16) > (In reply to Josef Moravec from comment #12) > > > + <tr> > > > + <th class="title-string">Date</th> > > > > Why are you adding the class here? It is usually used for sorting in > > datatables, which is not this case. > > Bad copy/paste during rebase Thanks fir quick fix, passing QA Created attachment 79176 [details] [review] Bug 20656: (RM follow-up) Add missing filter Awesome work all! Pushed to master for 18.11 (In reply to Katrin Fischer from comment #4) > I think I like the new display better. As it's supposed to be a summary > displaying only open credits and fines makes sense to me, but the heading > should then be changed to match the new display. > > Signing, but waiting for follow-up and maybe another opinion here. I am happy to fix this on a follow-up bug, can you suggest a new title? Pushed to 18.05.x for 18.05.05. 17.11.x is still using GetMemberAccountRecords instead of Koha::Account::Lines so patch does not applies. Is is relevant for 17.11.x ? |