This field is unused. Any attempt to use it was inactive since 2014.
Created attachment 161779 [details] [review] Bug 36002: Database revision for aqorders.purchaseordernumber Test plan: Run dbrev. Check if field is no longer there. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 161780 [details] [review] Bug 36002: DBIx schema changes for Aqorder No test plan. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 161781 [details] [review] Bug 36002: Remove unused subroutine GetParcels That did refer to our aqorders.purchaseordernumber.. Test plan: Git grep for GetParcels. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 161782 [details] [review] Bug 36002: Remove last references to purchaseordernumber Test plan: Git grep for it. Ignore update22to30.pl and atomicupdate. Verify that you can still create/update/delete an acq order. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 164938 [details] [review] Bug 36002: Database revision for aqorders.purchaseordernumber Test plan: Run dbrev. Check if field is no longer there. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 164939 [details] [review] Bug 36002: DBIx schema changes for Aqorder No test plan. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 164940 [details] [review] Bug 36002: Remove unused subroutine GetParcels That did refer to our aqorders.purchaseordernumber.. Test plan: Git grep for GetParcels. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 164941 [details] [review] Bug 36002: Remove last references to purchaseordernumber Test plan: Git grep for it. Ignore update22to30.pl and atomicupdate. Verify that you can still create/update/delete an acq order. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
(In reply to Kyle M Hall from comment #8) > Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Great. Thanks.
Created attachment 165386 [details] [review] Bug 36002: Database revision for aqorders.purchaseordernumber Test plan: Run dbrev. Check if field is no longer there. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 165387 [details] [review] Bug 36002: DBIx schema changes for Aqorder No test plan. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 165388 [details] [review] Bug 36002: Remove unused subroutine GetParcels That did refer to our aqorders.purchaseordernumber.. Test plan: Git grep for GetParcels. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 165389 [details] [review] Bug 36002: Remove last references to purchaseordernumber Test plan: Git grep for it. Ignore update22to30.pl and atomicupdate. Verify that you can still create/update/delete an acq order. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Code makes sense. Tested creating/editing and deleting a purchase order through UI, all works. QA script is showing what I believe is a false positive for POD coverage: Before applying patchset: perl -MPod::Coverage=C4::Acquisition -e666 C4::Acquisition has a Pod::Coverage rating of 0.94 The following are uncovered: GetOrderFromItemnumber, NotifyOrderUsers, ReOpenBasketgroup After applying patchset: perl -MPod::Coverage=C4::Acquisition -e666 C4::Acquisition has a Pod::Coverage rating of 0.938775510204082 The following are uncovered: GetOrderFromItemnumber, NotifyOrderUsers, ReOpenBasketgroup
I have to admit that we have used this column in migrations to allow for reporting. And I feel we are not the only ones. Right now we just drop the column without checking for any contents. Could there be a more clever way of doing this, that raises a warning first? In light of us wanting to do more clean-up I think it would be worth to agree on how to handle this in general and then apply it to every removal moving forward. Maybe something like we did for items.paidfor? if ( column_exists( 'items', 'paidfor' ) ) { my ($count) = $dbh->selectrow_array( q| SELECT COUNT(*) FROM items WHERE paidfor IS NOT NULL AND paidfor <> "" | ); if ($count) { warn "Warning - Cannot remove column items.paidfor. At least one value exists"; } else { $dbh->do(q|ALTER TABLE items DROP COLUMN paidfor|); $dbh->do(q|UPDATE marc_subfield_structure SET kohafield = '' WHERE kohafield = 'items.paidfor'|); } } Database update is missing the "success" say out line. Otherwise this does a good clean-up :) Didn't find any other references and amazed we missed removing GetParcels!
(In reply to Katrin Fischer from comment #15) > I have to admit that we have used this column in migrations to allow for > reporting. And I feel we are not the only ones. Right now we just drop the > column without checking for any contents. Did you really? :) > Could there be a more clever way of doing this, that raises a warning first? Well, will add a follow-up to meet your request. But have to admit that I was a bit hesitant to do so. You could theoretically say that people filling Koha columns with their own data should not depend on the development team to recover that data. > In light of us wanting to do more clean-up I think it would be worth to > agree on how to handle this in general and then apply it to every removal > moving forward. I do not hope that this argument will hijack this simple cleanup operation. Please start that discussion elsewhere. > Maybe something like we did for items.paidfor? I chose another approach. > Database update is missing the "success" say out line. I have never seen that as a requirement. Which coding guideline is that?
Created attachment 165853 [details] [review] Bug 36002: (follow-up) Save purchaseordernumbers from aqorders We copy them here to another (temporary) table. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Marcel de Rooy from comment #16) > > Database update is missing the "success" say out line. > > I have never seen that as a requirement. Which coding guideline is that? DEV atomic update /usr/share/koha/installer/data/mysql/atomicupdate/bug_36002.pl [11:41:13]: Bug 36002 - Remove aqorders.purchaseordernumber We found 1 order lines where field purchaseordernumber was filled! These records have been copied to table: zzaqorders_purchaseordernumber. Please examine the data and remove this table. The last line here is kind of a success line btw ;)
> I have never seen that as a requirement. Which coding guideline is that? It's in our skeleton.pl template with tons of examples. We tell what we are going to do before running the SQL and we tell what we did after. This is a standard thing, nothing I made up.
Pushed for 24.05! Well done everyone, thank you!
I prefer not to impact stable version 23.11.x
This looks like an architecture change. Nothing to add/edit in the Koha manual.