[Bug 15381] New: Move the authority types related code to Koha::Authority::Type[s] - part 2
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15381 Bug ID: 15381 Summary: Move the authority types related code to Koha::Authority::Type[s] - part 2 Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: ASSIGNED Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: jonathan.druart@bugs.koha-community.org Reporter: jonathan.druart@bugs.koha-community.org QA Contact: testopia@bugs.koha-community.org Depends on: 15379, 15380 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 Referenced Bugs: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15379 [Bug 15379] Move the authority types related code to Koha::Authority::Type[s] http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15380 [Bug 15380] Move the authority types related code to Koha::Authority::Type[s] - part 1 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15381 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |15379 Depends on|15379 | Referenced Bugs: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15379 [Bug 15379] Move the authority types related code to Koha::Authority::Type[s] -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15381 --- Comment #1 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 45705 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=45705&action=edit 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). -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15381 --- Comment #2 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 45706 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=45706&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15381 --- Comment #3 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 45707 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=45707&action=edit Bug 15381: Remove GetAuthType and GetAuthTypeCode Test this patch with the previous one. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15381 Frédéric Demians <frederic@tamil.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |frederic@tamil.fr --- Comment #4 from Frédéric Demians <frederic@tamil.fr> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15381 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff --- Comment #5 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15381 Frédéric Demians <frederic@tamil.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15381 Frédéric Demians <frederic@tamil.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #45705|0 |1 is obsolete| | --- Comment #6 from Frédéric Demians <frederic@tamil.fr> --- Created attachment 45900 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=45900&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15381 Frédéric Demians <frederic@tamil.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #45706|0 |1 is obsolete| | --- Comment #7 from Frédéric Demians <frederic@tamil.fr> --- Created attachment 45901 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=45901&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15381 Frédéric Demians <frederic@tamil.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #45707|0 |1 is obsolete| | --- Comment #8 from Frédéric Demians <frederic@tamil.fr> --- Created attachment 45902 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=45902&action=edit Bug 15381: Remove GetAuthType and GetAuthTypeCode Test this patch with the previous one. Signed-off-by: Frédéric Demians <f.demians@tamil.fr> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15381 --- Comment #9 from Frédéric Demians <frederic@tamil.fr> --- Created attachment 45903 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=45903&action=edit Bug 15381: Fix error in SearchAuthorities when no authtypecode -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15381 Frédéric Demians <frederic@tamil.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |5572 Referenced Bugs: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=5572 [Bug 5572] Improve C4::AuthoritiesMarc->merge() + syspref AuthCleanBiblios -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15381 Frédéric Demians <frederic@tamil.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on|5572 | Referenced Bugs: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=5572 [Bug 5572] Improve C4::AuthoritiesMarc->merge() + syspref AuthCleanBiblios -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15381 --- Comment #10 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Comment on attachment 45903 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=45903 Bug 15381: Fix error in SearchAuthorities when no authtypecode Review of attachment 45903: --> (http://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=15381&attachment=45903) ----------------------------------------------------------------- ::: 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? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15381 --- Comment #11 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Comment on attachment 45903 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=45903 Bug 15381: Fix error in SearchAuthorities when no authtypecode Review of attachment 45903: --> (http://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=15381&attachment=45903) ----------------------------------------------------------------- ::: 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? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15381 --- Comment #12 from Frédéric Demians <frederic@tamil.fr> --- (In reply to Martin Renvoize from comment #11)
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?
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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15381 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #45900|0 |1 is obsolete| | --- Comment #13 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 46078 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=46078&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15381 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #45901|0 |1 is obsolete| | --- Comment #14 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 46079 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=46079&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15381 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #45902|0 |1 is obsolete| | --- Comment #15 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 46080 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=46080&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15381 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #45903|0 |1 is obsolete| | --- Comment #16 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 46081 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=46081&action=edit Bug 15381: Fix error in SearchAuthorities when no authtypecode Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15381 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com Status|Signed Off |Passed QA --- Comment #17 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- I've been convinced, code looks good and I can't find any regressions :) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15381 Brendan Gallagher <brendan@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |brendan@bywatersolutions.co | |m Status|Passed QA |Pushed to Master --- Comment #18 from Brendan Gallagher <brendan@bywatersolutions.com> --- Pushed to master - Awesome - Thanks -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15381 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |15467 Referenced Bugs: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15467 [Bug 15467] AuthoritiesMarc.t is failing -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15381 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |15469 Referenced Bugs: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15469 [Bug 15469] The authority header search is broken -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15381 Frédéric Demians <frederic@tamil.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |15470 Referenced Bugs: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15470 [Bug 15470] Error when adding new authority with Default framework -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15381 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |15473 Referenced Bugs: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15473 [Bug 15473] Koha::Objects->find should find if the key is an empty string -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15381 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |15550 Referenced Bugs: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15550 [Bug 15550] Authority type pull down in OPAC authority search is empty -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15381 --- Comment #19 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15381 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=15358 -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org