Bug 10021 - Remove dead code related to notifys
Summary: Remove dead code related to notifys
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
: 7081 (view as bug list)
Depends on: 6427
Blocks: 7315
  Show dependency treegraph
 
Reported: 2013-04-10 09:16 UTC by Katrin Fischer
Modified: 2018-12-03 20:04 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 10021: Drop table notifys and related code (9.50 KB, patch)
2016-08-09 10:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10021: Drop table notifys and related code (9.19 KB, patch)
2017-10-18 15:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10021: Drop columns notify_id and notify_level from accountlines (23.79 KB, patch)
2017-10-18 15:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10021: Drop table notifys and related code (9.19 KB, patch)
2017-10-23 15:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10021: Drop columns notify_id and notify_level from accountlines (22.35 KB, patch)
2017-10-23 15:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10021: Update DB changes (1.16 KB, patch)
2017-10-23 15:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10021: Remove accountlines columns - DBIC Schema changes (1.66 KB, patch)
2017-10-23 15:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10021: Drop table notifys and related code (9.19 KB, patch)
2017-10-24 15:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10021: Drop columns notify_id and notify_level from accountlines (21.69 KB, patch)
2017-10-24 15:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10021: Update DB changes (1.16 KB, patch)
2017-10-24 15:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10021: Remove accountlines columns - DBIC Schema changes (1.66 KB, patch)
2017-10-24 15:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10021: Remove notify columns in circ/branchoverdues.tt (5.06 KB, patch)
2017-10-24 21:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10021: Restore "Pay fines" list (3.54 KB, patch)
2017-10-24 21:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10021: Drop table notifys and related code (9.22 KB, patch)
2017-10-28 07:40 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 10021: Drop columns notify_id and notify_level from accountlines (21.68 KB, patch)
2017-10-28 07:40 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 10021: Update DB changes (1.21 KB, patch)
2017-10-28 07:40 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 10021: Remove accountlines columns - DBIC Schema changes (1.71 KB, patch)
2017-10-28 07:40 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 10021: Remove notify columns in circ/branchoverdues.tt (5.10 KB, patch)
2017-10-28 07:40 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 10021: Restore "Pay fines" list (3.58 KB, patch)
2017-10-28 07:40 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 10021: (QA follow-up) Rename variables (1.81 KB, patch)
2017-11-03 16:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10021: Drop table notifys and related code (9.31 KB, patch)
2017-11-06 09:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10021: Drop columns notify_id and notify_level from accountlines (21.78 KB, patch)
2017-11-06 09:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10021: Update DB changes (1.30 KB, patch)
2017-11-06 09:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10021: Remove accountlines columns - DBIC Schema changes (1.80 KB, patch)
2017-11-06 09:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10021: Remove notify columns in circ/branchoverdues.tt (5.20 KB, patch)
2017-11-06 09:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10021: Restore "Pay fines" list (3.68 KB, patch)
2017-11-06 09:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10021: (QA follow-up) Rename variables (1.90 KB, patch)
2017-11-06 09:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10021: Remove notify headers from pay*.tt (1.83 KB, patch)
2017-12-11 16:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10021: Remove notify headers from pay*.tt (1.85 KB, patch)
2017-12-11 16:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10021: Only display outstanding fines in 'Pay fines' (1.05 KB, patch)
2017-12-11 16:58 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2013-04-10 09:16:41 UTC
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
Comment 1 Katrin Fischer 2013-04-13 23:15:23 UTC
*** Bug 7081 has been marked as a duplicate of this bug. ***
Comment 2 Katrin Fischer 2013-07-07 09:38:22 UTC
Now that bug 10256 has been pushed to master, we should also remove the database parts of it.
Comment 3 Nicole C. Engard 2015-03-05 14:26:56 UTC
Most of this is solved in the accounts rewrite - bug 6427
Comment 4 Jonathan Druart 2016-08-09 10:29:51 UTC
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.
Comment 5 Jonathan Druart 2016-08-09 10:30:39 UTC
(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.
Comment 6 Jonathan Druart 2017-10-11 13:44:11 UTC
There are still code in C4::Account about accountlines.notify_id (at least).
Comment 7 Jonathan Druart 2017-10-18 15:24:39 UTC
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.
Comment 8 Jonathan Druart 2017-10-18 15:24:44 UTC
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?
Comment 9 Katrin Fischer 2017-10-21 22:36:57 UTC
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
Comment 10 Jonathan Druart 2017-10-23 15:28:41 UTC
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.
Comment 11 Jonathan Druart 2017-10-23 15:28:46 UTC
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?
Comment 12 Jonathan Druart 2017-10-23 15:29:00 UTC
Created attachment 68421 [details] [review]
Bug 10021: Update DB changes
Comment 13 Jonathan Druart 2017-10-23 15:29:05 UTC
Created attachment 68422 [details] [review]
Bug 10021: Remove accountlines columns - DBIC Schema changes
Comment 14 Josef Moravec 2017-10-23 22:21:14 UTC
I have problem applying the patch on current master - probably because of bug 12768, could you please rebase one more time? Thanks
Comment 15 Jonathan Druart 2017-10-24 15:42:56 UTC
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.
Comment 16 Jonathan Druart 2017-10-24 15:43:00 UTC
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?
Comment 17 Jonathan Druart 2017-10-24 15:43:05 UTC
Created attachment 68479 [details] [review]
Bug 10021: Update DB changes
Comment 18 Jonathan Druart 2017-10-24 15:43:09 UTC
Created attachment 68480 [details] [review]
Bug 10021: Remove accountlines columns - DBIC Schema changes
Comment 19 Mark Tompsett 2017-10-24 16:44:50 UTC
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.
Comment 20 Josef Moravec 2017-10-24 17:49:45 UTC
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.
Comment 21 Jonathan Druart 2017-10-24 21:48:38 UTC
Created attachment 68505 [details] [review]
Bug 10021: Remove notify columns in circ/branchoverdues.tt
Comment 22 Jonathan Druart 2017-10-24 21:48:43 UTC
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
Comment 23 Josef Moravec 2017-10-28 07:40:22 UTC
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>
Comment 24 Josef Moravec 2017-10-28 07:40:30 UTC
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>
Comment 25 Josef Moravec 2017-10-28 07:40:36 UTC
Created attachment 68795 [details] [review]
Bug 10021: Update DB changes

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 26 Josef Moravec 2017-10-28 07:40:41 UTC
Created attachment 68796 [details] [review]
Bug 10021: Remove accountlines columns - DBIC Schema changes

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 27 Josef Moravec 2017-10-28 07:40:47 UTC
Created attachment 68797 [details] [review]
Bug 10021: Remove notify columns in circ/branchoverdues.tt

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 28 Josef Moravec 2017-10-28 07:40:55 UTC
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>
Comment 29 Marcel de Rooy 2017-11-03 08:29:46 UTC
Moving this to enh
Comment 30 Marcel de Rooy 2017-11-03 08:33:26 UTC
 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 ?
Comment 31 Marcel de Rooy 2017-11-03 08:55:25 UTC
(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 ?
Comment 32 Marcel de Rooy 2017-11-03 08:58:59 UTC
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.
Comment 33 Jonathan Druart 2017-11-03 16:39:12 UTC
(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.
Comment 34 Jonathan Druart 2017-11-03 16:41:44 UTC
Created attachment 68920 [details] [review]
Bug 10021: (QA follow-up) Rename variables

To avoid $account, $accounts and @accounts variables in the same scope
Comment 35 Marcel de Rooy 2017-11-06 08:54:14 UTC
(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 ?
Comment 36 Marcel de Rooy 2017-11-06 09:16:12 UTC
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>
Comment 37 Marcel de Rooy 2017-11-06 09:16:16 UTC
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>
Comment 38 Marcel de Rooy 2017-11-06 09:16:20 UTC
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>
Comment 39 Marcel de Rooy 2017-11-06 09:16:25 UTC
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>
Comment 40 Marcel de Rooy 2017-11-06 09:16:29 UTC
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>
Comment 41 Marcel de Rooy 2017-11-06 09:16:34 UTC
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>
Comment 42 Marcel de Rooy 2017-11-06 09:16:38 UTC
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>
Comment 43 Marcel de Rooy 2017-11-06 09:22:35 UTC
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 !
Comment 44 Jonathan Druart 2017-12-11 14:34:54 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 45 Jonathan Druart 2017-12-11 16:32:28 UTC
Created attachment 69695 [details] [review]
Bug 10021: Remove notify headers from pay*.tt
Comment 46 Jonathan Druart 2017-12-11 16:34:23 UTC
Created attachment 69696 [details] [review]
Bug 10021: Remove notify headers from pay*.tt
Comment 47 Jonathan Druart 2017-12-11 16:58:01 UTC
Created attachment 69698 [details] [review]
Bug 10021: Only display outstanding fines in 'Pay fines'
Comment 48 Jonathan Druart 2017-12-11 17:53:36 UTC
(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.
Comment 49 Nick Clemens 2017-12-14 17:03:36 UTC
Awesome work all!  Skipping for 17.11, enhancement