Bug 30939

Summary: remove_unused_authorities.pl is broken
Product: Koha Reporter: Fernando <biblioteca>
Component: Architecture, internals, and plumbingAssignee: Katrin Fischer <katrin.fischer>
Status: CLOSED FIXED QA Contact: Fridolin Somers <fridolin.somers>
Severity: major    
Priority: P5 - low CC: arthur.suzuki, david, fridolin.somers, lucas, tomascohen, victor
Version: Main   
Hardware: All   
OS: Linux   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
This fixes the ./misc/migration_tools/remove_unused_authorities.pl script so that it now works and deletes unused authority records. Before this, it generated an error message at the first unused authority record and stopped (without deleting any unused authority records).
Version(s) released in:
22.11.00, 22.05.04, 21.11.11
Bug Depends on: 17600    
Bug Blocks:    
Attachments: Bug 30939: Fix use statement for DelAuthority
Bug 30939: Fix use statement for DelAuthority
Bug 30939: Fix use statement for DelAuthority

Description Fernando 2022-06-10 08:06:35 UTC
When I try to remove unused authorithies I get:
authid=1 type=PERSO_NAME : used 6 time(s)
authid=2 type=TOPIC_TERM : used 23 time(s)
authid=3 type=PERSO_NAME : used 3 time(s)
authid=4 type=PERSO_NAMEUndefined subroutine &main::DelAuthority called at /usr/share/koha/bin/migration_tools/remove_unused_authorities.pl line 98.

When I try only a test works OK.
Thank you
Fernando
Comment 1 Katrin Fischer 2022-06-17 22:32:56 UTC
Confirmed on maser with the sample database:

authid=172 type=CORPO_NAME : used 2 time(s)
authid=173 type=PERSO_NAMEUndefined subroutine &main::DelAuthority called at ./misc/migration_tools/remove_unused_authorities.pl line 98.
Comment 2 Katrin Fischer 2022-06-17 22:41:28 UTC
Created attachment 136320 [details] [review]
Bug 30939: Fix use statement for DelAuthority

Without this patch, the script won't delete any
unused authorities, but gives an error instead
and dies:

Undefined subroutine &main::DelAuthority called at ./misc/migration_tools/remove_unused_authorities.pl line 98.

To test:
- Run from koha-shell:
  ./misc/migration_tools/remove_unused_authorities.pl -t
- Verify several authorities are reported as unused
- ./misc/migration_tools/remove_unused_authorities.pl -c
- Verify the error message is shown when the first unused
  authority is found and the script stops
- Apply patch and rerun:
  ./misc/migration_tools/remove_unused_authorities.pl -t
- Verify the error is gone, the script finishes and auhorities
  are deleted

https://bugs.koha-community.org/show_bug.cgi?id=30936
Comment 3 David Nind 2022-06-18 21:07:25 UTC
Created attachment 136323 [details] [review]
Bug 30939: Fix use statement for DelAuthority

Without this patch, the script won't delete any
unused authorities, but gives an error instead
and dies:

Undefined subroutine &main::DelAuthority called at ./misc/migration_tools/remove_unused_authorities.pl line 98.

To test:
- Run from koha-shell:
  ./misc/migration_tools/remove_unused_authorities.pl -t
- Verify several authorities are reported as unused
- ./misc/migration_tools/remove_unused_authorities.pl -c
- Verify the error message is shown when the first unused
  authority is found and the script stops
- Apply patch and rerun:
  ./misc/migration_tools/remove_unused_authorities.pl -t
- Verify the error is gone, the script finishes and auhorities
  are deleted

https://bugs.koha-community.org/show_bug.cgi?id=30936

Signed-off-by: David Nind <david@davidnind.com>
Comment 4 Fridolin Somers 2022-06-27 21:30:44 UTC
Created attachment 136661 [details] [review]
Bug 30939: Fix use statement for DelAuthority

Without this patch, the script won't delete any
unused authorities, but gives an error instead
and dies:

Undefined subroutine &main::DelAuthority called at ./misc/migration_tools/remove_unused_authorities.pl line 98.

To test:
- Run from koha-shell:
  ./misc/migration_tools/remove_unused_authorities.pl -t
- Verify several authorities are reported as unused
- ./misc/migration_tools/remove_unused_authorities.pl -c
- Verify the error message is shown when the first unused
  authority is found and the script stops
- Apply patch and rerun:
  ./misc/migration_tools/remove_unused_authorities.pl -t
- Verify the error is gone, the script finishes and auhorities
  are deleted

https://bugs.koha-community.org/show_bug.cgi?id=30936

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 5 Fridolin Somers 2022-06-27 21:37:55 UTC
How could be search for thoses ?

I try :
git grep 'use C4::.*;' | egrep -v '^(C4|Koha|t)/' | egrep -v 'qw ?(\(|/)' | grep -v Context

If found for example :
tools/batch_delete_records.pl:use C4::AuthoritiesMarc;

Is it a correct match ?
Comment 6 Tomás Cohen Arazi 2022-06-28 12:33:23 UTC
(In reply to Fridolin Somers from comment #5)
> How could be search for thoses ?
> 
> I try :
> git grep 'use C4::.*;' | egrep -v '^(C4|Koha|t)/' | egrep -v 'qw ?(\(|/)' |
> grep -v Context
> 
> If found for example :
> tools/batch_delete_records.pl:use C4::AuthoritiesMarc;
> 
> Is it a correct match ?

Nope. Because the script is using fully-qualified method names (e.g. C4::AuthoritiesMarc::GetAuthority( $record_id ) ) so not importing the function names into the package scope is not a real issue. We need to check that C4::AuthoritiesMarc defines a method that is used in a script.
Comment 7 Tomás Cohen Arazi 2022-06-28 12:41:27 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 8 Lucas Gass 2022-07-29 17:00:39 UTC
Backported to 22.05.x for 22.05.04
Comment 9 Arthur Suzuki 2022-08-11 13:06:30 UTC
applied to 21.11.x for 21.11.11
Comment 10 Victor Grousset/tuxayo 2022-08-13 16:30:51 UTC
Missing dependencies for 21.05.x, it shouldn't be affected, no backport.

Nothing to document, marking resolved.