Bug 15381 - Move the authority types related code to Koha::Authority::Type[s] - part 2
Summary: Move the authority types related code to Koha::Authority::Type[s] - part 2
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 15380
Blocks: 15379 15467 15469 15470 15473 15550 16010 16056 17118 17582 19683
  Show dependency treegraph
 
Reported: 2015-12-15 14:38 UTC by Jonathan Druart
Modified: 2017-11-29 17:00 UTC (History)
3 users (show)

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


Attachments
Bug 15381: Use Koha::Authority::Types in some other places (5.29 KB, patch)
2015-12-15 15:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15381: Remove getauthtypes and getauthtype (28.60 KB, patch)
2015-12-15 15:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15381: Remove GetAuthType and GetAuthTypeCode (17.28 KB, patch)
2015-12-15 15:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15381: Use Koha::Authority::Types in some other places (5.43 KB, patch)
2015-12-22 08:58 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 15381: Remove getauthtypes and getauthtype (28.74 KB, patch)
2015-12-22 08:59 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 15381: Remove GetAuthType and GetAuthTypeCode (17.42 KB, patch)
2015-12-22 09:00 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 15381: Fix error in SearchAuthorities when no authtypecode (1.02 KB, patch)
2015-12-22 09:01 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 15381: Use Koha::Authority::Types in some other places (5.50 KB, patch)
2015-12-30 13:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 15381: Remove getauthtypes and getauthtype (28.80 KB, patch)
2015-12-30 13:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 15381: Remove GetAuthType and GetAuthTypeCode (17.49 KB, patch)
2015-12-30 13:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 15381: Fix error in SearchAuthorities when no authtypecode (1.08 KB, patch)
2015-12-30 13:35 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2015-12-15 14:38:24 UTC
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
Comment 1 Jonathan Druart 2015-12-15 15:20:49 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2015-12-15 15:20:58 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2015-12-15 15:21:00 UTC Comment hidden (obsolete)
Comment 4 Frédéric Demians 2015-12-21 08:33:21 UTC
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.
Comment 5 Jonathan Druart 2015-12-21 09:34:31 UTC
(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.
Comment 6 Frédéric Demians 2015-12-22 08:58:05 UTC Comment hidden (obsolete)
Comment 7 Frédéric Demians 2015-12-22 08:59:55 UTC Comment hidden (obsolete)
Comment 8 Frédéric Demians 2015-12-22 09:00:48 UTC Comment hidden (obsolete)
Comment 9 Frédéric Demians 2015-12-22 09:01:18 UTC Comment hidden (obsolete)
Comment 10 Martin Renvoize 2015-12-23 06:06:56 UTC
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?
Comment 11 Martin Renvoize 2015-12-23 06:06:57 UTC
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?
Comment 12 Frédéric Demians 2015-12-23 07:22:38 UTC
(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.
Comment 13 Martin Renvoize 2015-12-30 13:35:37 UTC
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>
Comment 14 Martin Renvoize 2015-12-30 13:35:41 UTC
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>
Comment 15 Martin Renvoize 2015-12-30 13:35:44 UTC
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>
Comment 16 Martin Renvoize 2015-12-30 13:35:46 UTC
Created attachment 46081 [details] [review]
Bug 15381: Fix error in SearchAuthorities when no authtypecode

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Martin Renvoize 2015-12-30 13:36:22 UTC
I've been convinced, code looks good and I can't find any regressions :)
Comment 18 Brendan Gallagher 2015-12-31 19:02:57 UTC
Pushed to master - Awesome - Thanks
Comment 19 Jonathan Druart 2016-01-12 11:10:51 UTC
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.