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!
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!
Created attachment 108799 [details] [review] Bug 26268: Remove final references to paidfor db field
if( !column_exists( 'biblio', 'biblionumber' ) ) {
Created attachment 108807 [details] [review] Bug 26268: Remove marc framework mappings to items.paidfor
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!
Created attachment 108809 [details] [review] Bug 26268: Remove final references to paidfor db field
Created attachment 108810 [details] [review] Bug 26268: Remove marc framework mappings to items.paidfor Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
(In reply to Kyle M Hall from comment #3) > if( !column_exists( 'biblio', 'biblionumber' ) ) { Doh! Fixed now
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>
Created attachment 108813 [details] [review] Bug 26268: Remove final references to paidfor db field Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
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>
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 %]
(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 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
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>
Created attachment 109675 [details] [review] Bug 26268: Remove final references to paidfor db field Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
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>
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.
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).
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
(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.
= Only remove the mapping if there is no data in the column etc.
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>
Created attachment 109931 [details] [review] Bug 26268: Remove final references to paidfor db field Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
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>
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.
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.
Follow-up added, back to NQA
Created attachment 110801 [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> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 110802 [details] [review] Bug 26268: Remove final references to paidfor db field Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 110803 [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. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 110804 [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. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 110805 [details] [review] Bug 26268: Correct mapping for internal note to items.itemnotes_nonpublic Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 110806 [details] [review] Bug 26268: Remove invalid mappings from MARC frameworks when items.paidfor is removed Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Hi Martin, 952$x is the internal note field that was mapped to items.paidfor in the past before we added a proper column for it: itemsnotes_nonpublic. In some frameworks the old mappings had survived, but we only want to correct the mapping, not remove the subfield leaving these languages with no internal note field in their items. So I made some changes: - I killed your patch changing the framework files as you deleted the 952$x lines instead of just correcting the mapping. I replaced it by a follow-up patch - it was just a little easier this way. - I changed your database update. If the column items.paidfor is successfully removed, we don't care which field was mapped to it - we want to reset it as it's now an invalid mapping. Can you please have a look at my follow-ups and sign-off/PQA if you agree?
Created attachment 110836 [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> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 110837 [details] [review] Bug 26268: Remove final references to paidfor db field Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 110838 [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. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 110839 [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. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 110840 [details] [review] Bug 26268: Correct mapping for internal note to items.itemnotes_nonpublic Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 110841 [details] [review] Bug 26268: Remove invalid mappings from MARC frameworks when items.paidfor is removed Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
All works well and I agree with the follow-ups. Setting to PQA as requested :).. Yeay!
Created attachment 110935 [details] [review] Bug 26268: Fix deleteditems query in DB rev
Pushed to master for 20.11, thanks to everybody involved!
enhancement will not be backported to 20.05.x
(In reply to Martin Renvoize from comment #27) > Created attachment 109934 [details] [review] [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. Unfortunately, it seems like this conditional removal can lead to inconsistent database schemas... issues_stats.pl is breaking because my items table has 47 columns and deleteditems table has 46 columns. The "paidfor" warning message must've been missed in the CLI output at upgrade time. Not sure what the answer is here. Maybe an audit script...