Bug 25044

Summary: No need to define koha_object[s]_class for standard object class names
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Katrin Fischer <katrin.fischer>
Severity: normal    
Priority: P5 - low CC: 1joynelson, agustinmoyano, martin.renvoize, tomascohen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00
Circulation function:
Bug Depends on: 24356    
Bug Blocks:    
Attachments: Bug 25044: Remove the need to define koha_object[s]_class for standard object class names
Bug 25044: Remove the need to define koha_object[s]_class for standard object class names
Bug 25044: Remove the need to define koha_object[s]_class for standard object class names
Bug 25044: Remove the need to define koha_object[s]_class for standard object class names
Bug 25044: (follow-up) Fix wrong tests

Description Jonathan Druart 2020-04-02 14:02:01 UTC
In Koha::Object[s]->prefetch_whitelist, there is a call to the koha_object[s]_class DBIC resultset class. We should not, otherwise those 2 methods will have to be defined all the time, even when we can guess it easily.

Koha::Item <> Schema::Result::Item => standard
Koha::Acquisition::Order <> Schema::Result::Aqorder => non-standard

sub _get_object_class {
    my ( $type ) = @_; 
    return unless $type;

    if( $type->can('koha_object_class') ) { 
        return $type->koha_object_class;
    }   
    $type =~ s|Schema::Result::||;
    return ${type};
}
Comment 1 Jonathan Druart 2020-04-02 14:04:41 UTC
Created attachment 102297 [details] [review]
Bug 25044: Remove the need to define koha_object[s]_class for standard object class names

In Koha::Object[s]->prefetch_whitelist, there is a call to the koha_object[s]_class DBIC resultset class. We should not, otherwise those 2 methods will have to be defined all the time, even when we can guess it easily.

Koha::Item <> Schema::Result::Item => standard
Koha::Acquisition::Order <> Schema::Result::Aqorder => non-standard

sub _get_object_class {
    my ( $type ) = @_;
    return unless $type;

    if( $type->can('koha_object_class') ) {
        return $type->koha_object_class;
    }
    $type =~ s|Schema::Result::||;
    return ${type};
}

Test plan:
 % prove t/db_dependent/Koha/Object.t t/db_dependent/Koha/Objects.t
should return green before and after this patch
Comment 2 Tomás Cohen Arazi (tcohen) 2020-04-02 14:11:22 UTC
Good catch, I think we missed that method. Still, wouldn't it be more efficient to just have this information as schema metadata? (not even a method, just metadata in the big schema hash)
Comment 3 Jonathan Druart 2020-04-02 14:21:30 UTC
Created attachment 102298 [details] [review]
Bug 25044: Remove the need to define koha_object[s]_class for standard object class names

In Koha::Object[s]->prefetch_whitelist, there is a call to the koha_object[s]_class DBIC resultset class. We should not, otherwise those 2 methods will have to be defined all the time, even when we can guess it easily.

Koha::Item <> Schema::Result::Item => standard
Koha::Acquisition::Order <> Schema::Result::Aqorder => non-standard

sub _get_object_class {
    my ( $type ) = @_;
    return unless $type;

    if( $type->can('koha_object_class') ) {
        return $type->koha_object_class;
    }
    $type =~ s|Schema::Result::||;
    return ${type};
}

Test plan:
 % prove t/db_dependent/Koha/Object.t t/db_dependent/Koha/Objects.t
should return green before and after this patch
Comment 4 Tomás Cohen Arazi (tcohen) 2020-04-08 04:19:34 UTC
Created attachment 102529 [details] [review]
Bug 25044: Remove the need to define koha_object[s]_class for standard object class names

In Koha::Object[s]->prefetch_whitelist, there is a call to the koha_object[s]_class DBIC resultset class. We should not, otherwise those 2 methods will have to be defined all the time, even when we can guess it easily.

Koha::Item <> Schema::Result::Item => standard
Koha::Acquisition::Order <> Schema::Result::Aqorder => non-standard

sub _get_object_class {
    my ( $type ) = @_;
    return unless $type;

    if( $type->can('koha_object_class') ) {
        return $type->koha_object_class;
    }
    $type =~ s|Schema::Result::||;
    return ${type};
}

Test plan:
 % prove t/db_dependent/Koha/Object.t t/db_dependent/Koha/Objects.t
should return green before and after this patch

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 5 Katrin Fischer 2020-04-12 19:28:47 UTC
Created attachment 102774 [details] [review]
Bug 25044: Remove the need to define koha_object[s]_class for standard object class names

In Koha::Object[s]->prefetch_whitelist, there is a call to the koha_object[s]_class DBIC resultset class. We should not, otherwise those 2 methods will have to be defined all the time, even when we can guess it easily.

Koha::Item <> Schema::Result::Item => standard
Koha::Acquisition::Order <> Schema::Result::Aqorder => non-standard

sub _get_object_class {
    my ( $type ) = @_;
    return unless $type;

    if( $type->can('koha_object_class') ) {
        return $type->koha_object_class;
    }
    $type =~ s|Schema::Result::||;
    return ${type};
}

Test plan:
 % prove t/db_dependent/Koha/Object.t t/db_dependent/Koha/Objects.t
should return green before and after this patch

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 6 Katrin Fischer 2020-04-12 19:29:40 UTC
Trusting Tomas and Jonathan here as experts - not a big code change and tests pass.
Comment 7 Tomás Cohen Arazi (tcohen) 2020-04-13 17:32:31 UTC
(In reply to Katrin Fischer from comment #6)
> Trusting Tomas and Jonathan here as experts - not a big code change and
> tests pass.

Yeah, we are just 'doing it right' on something Agustin and I overlooked a bit.
Comment 8 Martin Renvoize (ashimema) 2020-04-14 07:37:29 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 9 Jonathan Druart 2020-04-14 09:36:07 UTC
Tomas, one test is failing:

t/Koha/REST/Plugin/Query.t .. 1/6
    #   Failed test 'exact match for JSON Pointer "/prefetch/0"'
    #   at t/Koha/REST/Plugin/Query.t line 350.
    #     Structures begin differing at:
    #          $got = HASH(0x55daaa8747c8)
    #     $expected = 'biblio'
    # Looks like you failed 1 test of 10.

I do not understand what was supposed to do the code prior to this change.

Koha::Schema::Result::Biblio does not have a koha_object_class, so the try was failing, and undef was assigned.

I am suspecting the tests to be wrong, but it would be good if you or Agustin could confirm it.
Comment 10 Tomás Cohen Arazi (tcohen) 2020-04-14 11:58:51 UTC
Created attachment 102927 [details] [review]
Bug 25044: (follow-up) Fix wrong tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 11 Martin Renvoize (ashimema) 2020-04-14 14:24:03 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 12 Joy Nelson 2020-05-05 20:42:31 UTC
missing dependencies.  not backported to 19.11.x