In this second part, the following subroutines from C4::Koha will be removed by using methods from Koha::Authorit[y|ies] and Koha::Authority::Type[s]: - Koha::getauthtypes - Koha::getauthtype - C4::AuthoritiesMarc::GetAuthType - C4::AuthoritiesMarc::GetAuthTypeCode Then Koha::Authority::Type[s] will be used in different modules and scripts to replace SQL queries: - C4/AuthoritiesMarc.pm - admin/auth_subfields_structure.pl - admin/marc_subfields_structure.pl
Created attachment 45705 [details] [review] Bug 15381: Use Koha::Authority::Types in some other places There are 3 place where the auth_types table were requested directly from the script or a subroutine. These 3 occurrences are easy to replace with the new module. Test plan: 1/ Search for authorities and use the "did you mean" feature for authorities. Focus on the authtypecode 2/ Edit authority and biblio frameworks. The "Thesaurus" dropdown list should be correctly populated (UNIMARC only).
Created attachment 45706 [details] [review] Bug 15381: Remove getauthtypes and getauthtype These 2 subroutines are now not necessary anymore, we can use Koha::Authority::Types to retrieve the authority types. This patch won't be easy to test. Indeed, a lot of file are updated. To test it I would suggest to focus on the merge authority You should also confirm that the authtypecode and the authtypetext values are correctly displayed when navigating in the authority module.
Created attachment 45707 [details] [review] Bug 15381: Remove GetAuthType and GetAuthTypeCode Test this patch with the previous one.
For me what happens here is the same field as what you have observed with system preferences. We have information serialized in the relational DB model (so a lot of linked tables) which could be more easily represented in other data structures. Koha is requesting ad nauseam multiple tables. I fear that your patches are just overcomplexifying something which is already far too complex. It's becoming byzantines. Finally, you're creating multiples classes to access data found in one table (auth_types) which can contain up to 10 records. On bug 5572, I've tried to simplify the authorities merge() function. On the road, I introduced a new function that gather info from auth_type, auth_subfield_structure, and marc_subfield_structure. A data structure represents all authority types. It can be something like that: GEO: type: GEO tag: 151 letters: 68abvxyz bibtags: 691 PERSO_NAME: type: PERSO_NAME tag: 100 letters: 68abcdefghjklmnopqrstvxyz THEME: type: THEME tag: 150 letters: 68abvxyz bibtags: 690|691 This can be cached. From that, we have all info needed by Koha code manipulating authorities, without having to go back to DB tables containing authorities definitions.
(In reply to Frédéric Demians from comment #4) > For me what happens here is the same field as what you have observed with > system preferences. We have information serialized in the relational DB model > (so a lot of linked tables) which could be more easily represented in other > data structures. Koha is requesting ad nauseam multiple tables. I fear that > your patches are just overcomplexifying something which is already far too > complex. It's becoming byzantines. Finally, you're creating multiples classes > to access data found in one table (auth_types) which can contain up to 10 > records. I am following the same design for all admin scripts. I create a Koha::Object then use it for CRUD when it is possible to use it quickly. I have reduced the code to 2/3 and remove 4 subroutines in the deprecated C4 namespace, I think it is the way to go, and don't think it's overcomplexifying the area. If we will need to return auth types in another structure, it will be easy to create a new method in Koha::Authority::Types.
Created attachment 45900 [details] [review] Bug 15381: Use Koha::Authority::Types in some other places There are 3 place where the auth_types table were requested directly from the script or a subroutine. These 3 occurrences are easy to replace with the new module. Test plan: 1/ Search for authorities and use the "did you mean" feature for authorities. Focus on the authtypecode 2/ Edit authority and biblio frameworks. The "Thesaurus" dropdown list should be correctly populated (UNIMARC only). Signed-off-by: Frédéric Demians <f.demians@tamil.fr>
Created attachment 45901 [details] [review] Bug 15381: Remove getauthtypes and getauthtype These 2 subroutines are now not necessary anymore, we can use Koha::Authority::Types to retrieve the authority types. This patch won't be easy to test. Indeed, a lot of file are updated. To test it I would suggest to focus on the merge authority You should also confirm that the authtypecode and the authtypetext values are correctly displayed when navigating in the authority module. Signed-off-by: Frédéric Demians <f.demians@tamil.fr>
Created attachment 45902 [details] [review] Bug 15381: Remove GetAuthType and GetAuthTypeCode Test this patch with the previous one. Signed-off-by: Frédéric Demians <f.demians@tamil.fr>
Created attachment 45903 [details] [review] Bug 15381: Fix error in SearchAuthorities when no authtypecode
Comment on attachment 45903 [details] [review] Bug 15381: Fix error in SearchAuthorities when no authtypecode Review of attachment 45903 [details] [review]: ----------------------------------------------------------------- ::: C4/AuthoritiesMarc.pm @@ +291,5 @@ > my %newline; > $newline{authid} = $authid; > if ( !$skipmetadata ) { > + my $auth_tag_to_report; > + $auth_tag_to_report = Koha::Authority::Types->find($authtypecode)->auth_tag_to_report Does this not just move the error further downstream in the code, as you will now just be passing an undefined $auth_tag_to_report to line 298 instead. Under what circumstance might $authtypecode be undefined in the first place?
(In reply to Martin Renvoize from comment #11) > Comment on attachment 45903 [details] [review] [review] > Bug 15381: Fix error in SearchAuthorities when no authtypecode > > Review of attachment 45903 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: C4/AuthoritiesMarc.pm > @@ +291,5 @@ > > my %newline; > > $newline{authid} = $authid; > > if ( !$skipmetadata ) { > > + my $auth_tag_to_report; > > + $auth_tag_to_report = Koha::Authority::Types->find($authtypecode)->auth_tag_to_report > > Does this not just move the error further downstream in the code, as you > will now just be passing an undefined $auth_tag_to_report to line 298 > instead. Under what circumstance might $authtypecode be undefined in the > first place? This is how it was before Jonathan patch. The quick fix just restores the previous operation. Without the patch, it was failing for me on line 295: Koha::Authority::Types->find($authtypecode) was returning undef. With the patch, it works how it used to.
Created attachment 46078 [details] [review] Bug 15381: Use Koha::Authority::Types in some other places There are 3 place where the auth_types table were requested directly from the script or a subroutine. These 3 occurrences are easy to replace with the new module. Test plan: 1/ Search for authorities and use the "did you mean" feature for authorities. Focus on the authtypecode 2/ Edit authority and biblio frameworks. The "Thesaurus" dropdown list should be correctly populated (UNIMARC only). Signed-off-by: Frédéric Demians <f.demians@tamil.fr> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 46079 [details] [review] Bug 15381: Remove getauthtypes and getauthtype These 2 subroutines are now not necessary anymore, we can use Koha::Authority::Types to retrieve the authority types. This patch won't be easy to test. Indeed, a lot of file are updated. To test it I would suggest to focus on the merge authority You should also confirm that the authtypecode and the authtypetext values are correctly displayed when navigating in the authority module. Signed-off-by: Frédéric Demians <f.demians@tamil.fr> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 46080 [details] [review] Bug 15381: Remove GetAuthType and GetAuthTypeCode Test this patch with the previous one. Signed-off-by: Frédéric Demians <f.demians@tamil.fr> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 46081 [details] [review] Bug 15381: Fix error in SearchAuthorities when no authtypecode Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
I've been convinced, code looks good and I can't find any regressions :)
Pushed to master - Awesome - Thanks
These patches introduced some regressions: 1/ Tests failures, fixed by bug 15467 2/ At the intranet, the search box was broken, fixed by bu 15469 3/ At the OPAC, the search and detail were broken, will be fixed by bug 15550 4/ Not fixed yet: the merge authority is broken. It was already broken before and bug 15358 is going to fix it.