The database table notifys and the code related to it is as far as I can tell no longer used in Koha. I am not sure how this used to work, so we would need to check very carefully before removing it. There are some things still visible in the interface: - fines table in patron account shows columns notify id and level - similar information shows in the overdues with fines report In all databases I checked there was no information stored in the databaes table or columns - they were always 0. Remaining code I found: - database table notifys - notify_id and notify_level in accountlines - C4/Overdues.pm - C4/Accounts.pm - C4/Members.pm
*** Bug 7081 has been marked as a duplicate of this bug. ***
Now that bug 10256 has been pushed to master, we should also remove the database parts of it.
Most of this is solved in the accounts rewrite - bug 6427
Created attachment 54194 [details] [review] Bug 10021: Drop table notifys and related code This patch drops the notifys table and its related code in C4::Overdues. A second patch should remove the 2 columns notify_id and notify_level from the accountlines table.
(In reply to Jonathan Druart from comment #4) > A second patch should remove the 2 columns notify_id and notify_level > from the accountlines table. But I am not familiar enough with this module to do it.
There are still code in C4::Account about accountlines.notify_id (at least).
Created attachment 68262 [details] [review] Bug 10021: Drop table notifys and related code This patch drops the notifys table and its related code in C4::Overdues. A second patch should remove the 2 columns notify_id and notify_level from the accountlines table.
Created attachment 68263 [details] [review] Bug 10021: Drop columns notify_id and notify_level from accountlines It appears that has never worked. Could someone confirm?
There seems to be an issue with the second patch: Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 10021: Drop table notifys and related code Applying: Bug 10021: Drop columns notify_id and notify_level from accountlines error: sha1 information is lacking or useless (C4/Overdues.pm). error: could not build fake ancestor Patch failed at 0001 Bug 10021: Drop columns notify_id and notify_level from accountlines The copy of the patch that failed is found in: .git/rebase-apply/patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-10021-Drop-columns-notifyid-and-notifylevel-fr-hdUZOE.patch
Created attachment 68419 [details] [review] Bug 10021: Drop table notifys and related code This patch drops the notifys table and its related code in C4::Overdues. A second patch should remove the 2 columns notify_id and notify_level from the accountlines table.
Created attachment 68420 [details] [review] Bug 10021: Drop columns notify_id and notify_level from accountlines It appears that has never worked. Could someone confirm?
Created attachment 68421 [details] [review] Bug 10021: Update DB changes
Created attachment 68422 [details] [review] Bug 10021: Remove accountlines columns - DBIC Schema changes
I have problem applying the patch on current master - probably because of bug 12768, could you please rebase one more time? Thanks
Created attachment 68477 [details] [review] Bug 10021: Drop table notifys and related code This patch drops the notifys table and its related code in C4::Overdues. A second patch should remove the 2 columns notify_id and notify_level from the accountlines table.
Created attachment 68478 [details] [review] Bug 10021: Drop columns notify_id and notify_level from accountlines It appears that has never worked. Could someone confirm?
Created attachment 68479 [details] [review] Bug 10021: Update DB changes
Created attachment 68480 [details] [review] Bug 10021: Remove accountlines columns - DBIC Schema changes
Code changes read okay. But given that I don't know how to trigger all the changes, I'm not going to sign off. I'll leave that for someone else more fearless.
Good job Jonathan, just few small things: 1) There is still remaining one column "Notified by" in table on page branchtransfers (line 41 of branchtransfers.tt", it is using values overdue1, overdue2, overdue3, which were removed from branchtransfers.pl 2) The column "Overdue status" should be removed too 3) Any owed fees are not shown on pay.pl page Otherwise it looks good for me.
Created attachment 68505 [details] [review] Bug 10021: Remove notify columns in circ/branchoverdues.tt
Created attachment 68506 [details] [review] Bug 10021: Restore "Pay fines" list Previous changes were wrong, the notify_id was always equal to 1 and GetBorNotifyAcctRecord was used to retrieved the account lines to pay
Created attachment 68793 [details] [review] Bug 10021: Drop table notifys and related code This patch drops the notifys table and its related code in C4::Overdues. A second patch should remove the 2 columns notify_id and notify_level from the accountlines table. Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 68794 [details] [review] Bug 10021: Drop columns notify_id and notify_level from accountlines It appears that has never worked. Could someone confirm? Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 68795 [details] [review] Bug 10021: Update DB changes Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 68796 [details] [review] Bug 10021: Remove accountlines columns - DBIC Schema changes Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 68797 [details] [review] Bug 10021: Remove notify columns in circ/branchoverdues.tt Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 68798 [details] [review] Bug 10021: Restore "Pay fines" list Previous changes were wrong, the notify_id was always equal to 1 and GetBorNotifyAcctRecord was used to retrieved the account lines to pay Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Moving this to enh
FAIL C4/Members.pm FAIL pod coverage POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666 FAIL C4/Overdues.pm FAIL pod coverage POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666 No reason to immediately move this to FQA ! When you remove a sub from a module without full coverage, this is the result. Something for QA tools ?
(In reply to Jonathan Druart from comment #8) > Created attachment 68263 [details] [review] [review] > Bug 10021: Drop columns notify_id and notify_level from accountlines > > It appears that has never worked. > Could someone confirm? Did you have any response whatsoever ?
QA Comment: GetOverduesForBranch Please add a test. You make some fundamental changes here. koha-tmpl/intranet-tmpl/prog/en/modules/members/pay.tt - <input type="hidden" name="totals[% line.accountlines_id %]" value="[% line.totals %]" /> Not sure. Why do yo remove this line ? New code: + my $accounts = Koha::Account::Lines->search({ borrowernumber => $borrowernumber, }, { order_by => ['accounttype'] }); + my @accounts; + while ( my $account = $accounts->next ) { This does not look very nice. Can we get more confusion on $account, @accounts or $accounts ? Renaming @accounts here might be the least we can do? No blocker on itself.
(In reply to Marcel de Rooy from comment #30) > When you remove a sub from a module without full coverage, this is the > result. Something for QA tools ? Yes, could be improved. POD failures must be considered as warnings. (In reply to Marcel de Rooy from comment #31) > (In reply to Jonathan Druart from comment #8) > > It appears that has never worked. > > Could someone confirm? > > Did you have any response whatsoever ? I think we discussed it on IRC, but I do not find the logs. (In reply to Marcel de Rooy from comment #32) > QA Comment: > > GetOverduesForBranch > Please add a test. You make some fundamental changes here. Fundamental? I do not understand what you want me to test. > koha-tmpl/intranet-tmpl/prog/en/modules/members/pay.tt > - <input type="hidden" name="totals[% line.accountlines_id %]" value="[% > line.totals %]" /> > Not sure. Why do yo remove this line ? Yes it could have been done somewhere else, it's not directly related to these changes. There is no "totals*" variables retrieved from the pl script. > New code: > + my $accounts = Koha::Account::Lines->search({ borrowernumber => > $borrowernumber, }, { order_by => ['accounttype'] }); > + my @accounts; > + while ( my $account = $accounts->next ) { > This does not look very nice. Can we get more confusion on $account, > @accounts or $accounts ? > Renaming @accounts here might be the least we can do? > No blocker on itself. Will fix.
Created attachment 68920 [details] [review] Bug 10021: (QA follow-up) Rename variables To avoid $account, $accounts and @accounts variables in the same scope
(In reply to Jonathan Druart from comment #33) > (In reply to Marcel de Rooy from comment #32) > > QA Comment: > > > > GetOverduesForBranch > > Please add a test. You make some fundamental changes here. > > Fundamental? I do not understand what you want me to test. Maybe non-trivial would have been a better word ?
Created attachment 68953 [details] [review] Bug 10021: Drop table notifys and related code This patch drops the notifys table and its related code in C4::Overdues. A second patch should remove the 2 columns notify_id and notify_level from the accountlines table. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 68954 [details] [review] Bug 10021: Drop columns notify_id and notify_level from accountlines It appears that has never worked. Could someone confirm? Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 68955 [details] [review] Bug 10021: Update DB changes Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 68956 [details] [review] Bug 10021: Remove accountlines columns - DBIC Schema changes Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 68957 [details] [review] Bug 10021: Remove notify columns in circ/branchoverdues.tt Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 68958 [details] [review] Bug 10021: Restore "Pay fines" list Previous changes were wrong, the notify_id was always equal to 1 and GetBorNotifyAcctRecord was used to retrieved the account lines to pay Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 68959 [details] [review] Bug 10021: (QA follow-up) Rename variables To avoid $account, $accounts and @accounts variables in the same scope Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Would have preferred to see a test for GetOverduesForBranch, but it is not a blocker for me. A rename of this sub would be nice too (see FIXME). Something like GetOverduesWithFines ? Similarly, it is weird that the script branchoverdues.pl (whats in a name) does not list Overdues with fines in the header but just Overdues. Apart from these observations, it is wonderful to see dead code moving out !
Pushed to master for 18.05, thanks to everybody involved!
Created attachment 69695 [details] [review] Bug 10021: Remove notify headers from pay*.tt
Created attachment 69696 [details] [review] Bug 10021: Remove notify headers from pay*.tt
Created attachment 69698 [details] [review] Bug 10021: Only display outstanding fines in 'Pay fines'
(In reply to Jonathan Druart from comment #46) > Created attachment 69696 [details] [review] [review] > Bug 10021: Remove notify headers from pay*.tt (In reply to Jonathan Druart from comment #47) > Created attachment 69698 [details] [review] [review] > Bug 10021: Only display outstanding fines in 'Pay fines' These last 2 patches have been pushed to master.
Awesome work all! Skipping for 17.11, enhancement