Bug 26268 - Remove items.paidfor once and for all
Summary: Remove items.paidfor once and for all
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Martin Renvoize
QA Contact: Testopia
URL:
Keywords:
Depends on: 18501 23463 19919
Blocks:
  Show dependency treegraph
 
Reported: 2020-08-21 10:20 UTC by Martin Renvoize
Modified: 2020-09-11 11:53 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 26268: Remove items.paidfor from the database (3.38 KB, patch)
2020-08-21 10:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26268: Remove final references to paidfor db field (1.80 KB, patch)
2020-08-21 10:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26268: Remove marc framework mappings to items.paidfor (6.15 KB, patch)
2020-08-21 11:35 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26268: Remove items.paidfor from the database (3.37 KB, patch)
2020-08-21 11:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26268: Remove final references to paidfor db field (1.80 KB, patch)
2020-08-21 11:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26268: Remove marc framework mappings to items.paidfor (6.20 KB, patch)
2020-08-21 11:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26268: Remove items.paidfor from the database (3.44 KB, patch)
2020-08-21 12:26 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26268: Remove final references to paidfor db field (1.87 KB, patch)
2020-08-21 12:26 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26268: Remove marc framework mappings to items.paidfor (6.27 KB, patch)
2020-08-21 12:26 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26268: Remove items.paidfor from the database (3.43 KB, patch)
2020-09-04 16:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26268: Remove final references to paidfor db field (1.86 KB, patch)
2020-09-04 16:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26268: Remove marc framework mappings to items.paidfor (6.26 KB, patch)
2020-09-04 16:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26268: (QA follow-up) Split DB update checks (1.24 KB, patch)
2020-09-04 16:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26268: Remove items.paidfor from the database (3.43 KB, patch)
2020-09-11 11:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26268: Remove final references to paidfor db field (1.86 KB, patch)
2020-09-11 11:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26268: Remove marc framework mappings to items.paidfor (6.26 KB, patch)
2020-09-11 11:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26268: (QA follow-up) Split DB update checks (1.24 KB, patch)
2020-09-11 11:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26268: (QA follow-up) Conditionally remove (2.46 KB, patch)
2020-09-11 11:53 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2020-08-21 10:20:05 UTC
In bug 19919 we removed all references to setting, unsettling and referring to the paidfor field but we neglected to actually drop the field from the database (I think I intended to do this, but was asking for feedback on the approach before adding the actual db change.. then completely forgot when it went through SO/QA quickly).

In bug 23463 we inadvertently re-introduced some of the setting code during a rebase.

In bug 18501 we then moved that inadvertently re-introduced setter into Koha::Item->store.

We should kill it off once and for all!
Comment 1 Martin Renvoize 2020-08-21 10:29:53 UTC
Created attachment 108797 [details] [review]
Bug 26268: Remove items.paidfor from the database

In bug 19919 we removed all references to setting, unsettling and
referring to the paidfor field but we neglected to actually drop the
field from the database (I think I intended to do this, but was asking
for feedback on the approach before adding the actual db change.. then
completely forgot when it went through SO/QA quickly).

In bug 23463 we inadvertently re-introduced some of the setting code
during a rebase.

In bug 18501 we then moved that inadvertently re-introduced setter into
Koha::Item->store.

We should kill it off once and for all!
Comment 2 Martin Renvoize 2020-08-21 10:29:57 UTC
Created attachment 108799 [details] [review]
Bug 26268: Remove final references to paidfor db field
Comment 3 Kyle M Hall 2020-08-21 11:34:32 UTC
if( !column_exists( 'biblio', 'biblionumber' ) ) {
Comment 4 Kyle M Hall 2020-08-21 11:35:35 UTC
Created attachment 108807 [details] [review]
Bug 26268: Remove marc framework mappings to items.paidfor
Comment 5 Martin Renvoize 2020-08-21 11:51:56 UTC
Created attachment 108808 [details] [review]
Bug 26268: Remove items.paidfor from the database

In bug 19919 we removed all references to setting, unsettling and
referring to the paidfor field but we neglected to actually drop the
field from the database (I think I intended to do this, but was asking
for feedback on the approach before adding the actual db change.. then
completely forgot when it went through SO/QA quickly).

In bug 23463 we inadvertently re-introduced some of the setting code
during a rebase.

In bug 18501 we then moved that inadvertently re-introduced setter into
Koha::Item->store.

We should kill it off once and for all!
Comment 6 Martin Renvoize 2020-08-21 11:51:59 UTC
Created attachment 108809 [details] [review]
Bug 26268: Remove final references to paidfor db field
Comment 7 Martin Renvoize 2020-08-21 11:52:03 UTC
Created attachment 108810 [details] [review]
Bug 26268: Remove marc framework mappings to items.paidfor

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2020-08-21 11:52:26 UTC
(In reply to Kyle M Hall from comment #3)
> if( !column_exists( 'biblio', 'biblionumber' ) ) {

Doh!

Fixed now
Comment 9 Kyle M Hall 2020-08-21 12:26:43 UTC
Created attachment 108812 [details] [review]
Bug 26268: Remove items.paidfor from the database

In bug 19919 we removed all references to setting, unsettling and
referring to the paidfor field but we neglected to actually drop the
field from the database (I think I intended to do this, but was asking
for feedback on the approach before adding the actual db change.. then
completely forgot when it went through SO/QA quickly).

In bug 23463 we inadvertently re-introduced some of the setting code
during a rebase.

In bug 18501 we then moved that inadvertently re-introduced setter into
Koha::Item->store.

We should kill it off once and for all!

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 10 Kyle M Hall 2020-08-21 12:26:51 UTC
Created attachment 108813 [details] [review]
Bug 26268: Remove final references to paidfor db field

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 11 Kyle M Hall 2020-08-21 12:26:54 UTC
Created attachment 108814 [details] [review]
Bug 26268: Remove marc framework mappings to items.paidfor

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 12 Katrin Fischer 2020-08-22 10:46:14 UTC
I see that there is a lot of history here, but I am not quite following yet.

1) Before dropping the column: Did we move the information stored there in previous developments? 

If we didn't, we might just want to print a warning if there is any content to be found in the db and ask people to drop themselves. It has been done in the past with some of the columns in borrowers (ethnicity?) I think. If there is no data, we can drop it in the update.

2) Looking for "paidfor" I found some more references this patch is not taking care of:

catalogue/moredetail.pl:    # Add paidfor info
catalogue/moredetail.pl:                $item->{paidfor} = { patron => $patron, created_on => $payment_offset->created_on };

koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/moredetail.tt:                [% IF ( ITEM_DAT.paidfor ) %]<li><span class="label">Paid for?:</span> [% INCLUDE 'patron-title.inc' patron=ITEM_DAT.paidfor.patron hide_patron_infos_if_needed=1 %] on [% ITEM_DAT.paidfor.created_on | $KohaDates %]</li>[% END %]
Comment 13 Martin Renvoize 2020-09-02 15:43:24 UTC
(In reply to Katrin Fischer from comment #12)
> I see that there is a lot of history here, but I am not quite following yet.
> 
> 1) Before dropping the column: Did we move the information stored there in
> previous developments?

Nope, we didn't drop the data.. we just started using the fields it used to be populated by inconsistently to construct the display on on the fly. The data in the field is no longer maintained and historically wasn't accurate so I think we should be OK to drop it regardless.

> 
> If we didn't, we might just want to print a warning if there is any content
> to be found in the db and ask people to drop themselves. It has been done in
> the past with some of the columns in borrowers (ethnicity?) I think. If
> there is no data, we can drop it in the update.
> 
> 2) Looking for "paidfor" I found some more references this patch is not
> taking care of:

As noted above, this is where we now construct the data on the fly. `$item->{paidfor} = { patron => $patron, created_on => $payment_offset->created_on }` is an assignment for the template rather than a getter or setter for the DB data.

> 
> catalogue/moredetail.pl:    # Add paidfor info
> catalogue/moredetail.pl:                $item->{paidfor} = { patron =>
> $patron, created_on => $payment_offset->created_on };
> 
> koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/moredetail.tt:            
> [% IF ( ITEM_DAT.paidfor ) %]<li><span class="label">Paid for?:</span> [%
> INCLUDE 'patron-title.inc' patron=ITEM_DAT.paidfor.patron
> hide_patron_infos_if_needed=1 %] on [% ITEM_DAT.paidfor.created_on |
> $KohaDates %]</li>[% END %]
Comment 14 Julian Maurice 2020-09-04 07:25:19 UTC
Comment on attachment 108812 [details] [review]
Bug 26268: Remove items.paidfor from the database

Review of attachment 108812 [details] [review]:
-----------------------------------------------------------------

::: installer/data/mysql/atomicupdate/bug_26268.perl
@@ +1,4 @@
> +$DBversion = 'XXX'; # will be replaced by the RM
> +if( CheckVersion( $DBversion ) ) {
> +
> +    if( !column_exists( 'items', 'paidfor' ) ) {

It should be column_exists, not !column_exists.
And in my opinion we should also check for deleteditems.paidfor existence
Comment 15 Martin Renvoize 2020-09-04 16:07:03 UTC
Created attachment 109674 [details] [review]
Bug 26268: Remove items.paidfor from the database

In bug 19919 we removed all references to setting, unsettling and
referring to the paidfor field but we neglected to actually drop the
field from the database (I think I intended to do this, but was asking
for feedback on the approach before adding the actual db change.. then
completely forgot when it went through SO/QA quickly).

In bug 23463 we inadvertently re-introduced some of the setting code
during a rebase.

In bug 18501 we then moved that inadvertently re-introduced setter into
Koha::Item->store.

We should kill it off once and for all!

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 16 Martin Renvoize 2020-09-04 16:07:07 UTC
Created attachment 109675 [details] [review]
Bug 26268: Remove final references to paidfor db field

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 17 Martin Renvoize 2020-09-04 16:07:13 UTC
Created attachment 109676 [details] [review]
Bug 26268: Remove marc framework mappings to items.paidfor

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 18 Martin Renvoize 2020-09-04 16:07:17 UTC
Created attachment 109677 [details] [review]
Bug 26268: (QA follow-up) Split DB update checks

This patch splits the DB update such that the 'paidfor' field is checked
for distinctly in the deleteditems table rather than assuming if it
exists in the items table it will also exist in deleteditems.

We also correct the check as highlights by Julian.
Comment 19 Martin Renvoize 2020-09-04 16:08:09 UTC
Good catch, corrected in the followup (which should probably be merged on push, but kept it separate to keep it clear the work has been done).
Comment 20 Julian Maurice 2020-09-11 08:11:36 UTC
Sorry, but I think there is still one thing missing : the updatedatabase should take care of removing any remaining reference to 'items.paidfor' in marc_subfield_structure
Comment 21 Katrin Fischer 2020-09-11 09:22:24 UTC
(In reply to Julian Maurice from comment #20)
> Sorry, but I think there is still one thing missing : the updatedatabase
> should take care of removing any remaining reference to 'items.paidfor' in
> marc_subfield_structure

Hm, I think that could be an issue we haven't thought about yet. For a long time there was not column for internal note in items and the default mapping we shipped Koha with mapped 952$x to items.paidfor. I think this is another reason not to drop the data if the column still contains information and to only output a note. And we shoudl not remove the mapping. If people have not actively remapped (which requires server action etc.) they might still store the internal note there.
Comment 22 Katrin Fischer 2020-09-11 09:26:20 UTC
= Only remove the mapping if there is no data in the column etc.
Comment 23 Martin Renvoize 2020-09-11 11:53:04 UTC
Created attachment 109930 [details] [review]
Bug 26268: Remove items.paidfor from the database

In bug 19919 we removed all references to setting, unsettling and
referring to the paidfor field but we neglected to actually drop the
field from the database (I think I intended to do this, but was asking
for feedback on the approach before adding the actual db change.. then
completely forgot when it went through SO/QA quickly).

In bug 23463 we inadvertently re-introduced some of the setting code
during a rebase.

In bug 18501 we then moved that inadvertently re-introduced setter into
Koha::Item->store.

We should kill it off once and for all!

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 24 Martin Renvoize 2020-09-11 11:53:08 UTC
Created attachment 109931 [details] [review]
Bug 26268: Remove final references to paidfor db field

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 25 Martin Renvoize 2020-09-11 11:53:12 UTC
Created attachment 109932 [details] [review]
Bug 26268: Remove marc framework mappings to items.paidfor

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 26 Martin Renvoize 2020-09-11 11:53:17 UTC
Created attachment 109933 [details] [review]
Bug 26268: (QA follow-up) Split DB update checks

This patch splits the DB update such that the 'paidfor' field is checked
for distinctly in the deleteditems table rather than assuming if it
exists in the items table it will also exist in deleteditems.

We also correct the check as highlights by Julian.
Comment 27 Martin Renvoize 2020-09-11 11:53:22 UTC
Created attachment 109934 [details] [review]
Bug 26268: (QA follow-up) Conditionally remove

This patch adds conditions to prevent the removal of items.paidfor if it
is populated and also adds the removal fo the marc_subfield_structure
row if it exists.
Comment 28 Martin Renvoize 2020-09-11 11:53:49 UTC
Follow-up added, back to NQA