Bug 36002 - Get rid of aqorders.purchaseordernumber
Summary: Get rid of aqorders.purchaseordernumber
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords: release-notes-needed
Depends on:
Blocks:
 
Reported: 2024-02-06 13:27 UTC by Marcel de Rooy
Modified: 2024-04-30 15:10 UTC (History)
1 user (show)

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


Attachments
Bug 36002: Database revision for aqorders.purchaseordernumber (2.21 KB, patch)
2024-02-06 13:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36002: DBIx schema changes for Aqorder (1.69 KB, patch)
2024-02-06 13:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36002: Remove unused subroutine GetParcels (3.20 KB, patch)
2024-02-06 13:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36002: Remove last references to purchaseordernumber (2.14 KB, patch)
2024-02-06 13:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36002: Database revision for aqorders.purchaseordernumber (2.23 KB, patch)
2024-04-16 14:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 36002: DBIx schema changes for Aqorder (1.71 KB, patch)
2024-04-16 14:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 36002: Remove unused subroutine GetParcels (3.22 KB, patch)
2024-04-16 14:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 36002: Remove last references to purchaseordernumber (2.16 KB, patch)
2024-04-16 14:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 36002: Database revision for aqorders.purchaseordernumber (2.29 KB, patch)
2024-04-23 13:01 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 36002: DBIx schema changes for Aqorder (1.76 KB, patch)
2024-04-23 13:01 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 36002: Remove unused subroutine GetParcels (3.28 KB, patch)
2024-04-23 13:01 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 36002: Remove last references to purchaseordernumber (2.22 KB, patch)
2024-04-23 13:01 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 36002: (follow-up) Save purchaseordernumbers from aqorders (1.67 KB, patch)
2024-04-30 11:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2024-02-06 13:27:39 UTC
This field is unused. Any attempt to use it was inactive since 2014.
Comment 1 Marcel de Rooy 2024-02-06 13:55:03 UTC
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>
Comment 2 Marcel de Rooy 2024-02-06 13:55:06 UTC
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>
Comment 3 Marcel de Rooy 2024-02-06 13:55:08 UTC
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>
Comment 4 Marcel de Rooy 2024-02-06 13:55:10 UTC
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>
Comment 5 Kyle M Hall 2024-04-16 14:00:04 UTC
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>
Comment 6 Kyle M Hall 2024-04-16 14:00:10 UTC
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>
Comment 7 Kyle M Hall 2024-04-16 14:00:12 UTC
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>
Comment 8 Kyle M Hall 2024-04-16 14:00:14 UTC
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>
Comment 9 Marcel de Rooy 2024-04-16 14:38:44 UTC
(In reply to Kyle M Hall from comment #8)
> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Great. Thanks.
Comment 10 Pedro Amorim 2024-04-23 13:01:45 UTC
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>
Comment 11 Pedro Amorim 2024-04-23 13:01:48 UTC
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>
Comment 12 Pedro Amorim 2024-04-23 13:01:54 UTC
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>
Comment 13 Pedro Amorim 2024-04-23 13:01:57 UTC
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>
Comment 14 Pedro Amorim 2024-04-23 13:02:12 UTC
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
Comment 15 Katrin Fischer 2024-04-29 14:36:30 UTC
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!
Comment 16 Marcel de Rooy 2024-04-30 11:47:36 UTC
(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?
Comment 17 Marcel de Rooy 2024-04-30 11:49:33 UTC
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>
Comment 18 Marcel de Rooy 2024-04-30 11:52:47 UTC
(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 ;)
Comment 19 Katrin Fischer 2024-04-30 11:54:17 UTC
> 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.
Comment 20 Katrin Fischer 2024-04-30 15:10:59 UTC
Pushed for 24.05!

Well done everyone, thank you!