Built on top of bug 9988, that introduces Koha::Authority::get_usage_count.
Created attachment 64720 [details] [review] Bug 18149: Move CountUsage calls to Koha namespace After the introduction of Koha::Authorities->get_usage_count with bug 9988, we can now replace the remaining occurrences of CountUsage. At the same time we remove CountUsageChildren. This was an empty sub. The typo get_count_usage in a subtest title is adjusted. Test plan: [1] Run t/db_dependent/Koha/Authorities.t [2] Perform a search on authorities-home.pl and verify that you see plausible numbers for 'used in xx records'. [3] Click on Details for one authority. See the same number? [4] Do the same as in 2/3 for Authority search on OPAC. [5] Remember the authid and enter this in the record numbers box on tools/batch_delete_records.pl. Select Authorities and click Continue. The next form shows a column "Used in". Do you see the same count again? [6] Git grep CountUsage. You should see just one hit in a comment that can be kept in Koha/Authorities.pm. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 65653 [details] [review] [SIGNED-OFF] Bug 18149: Move CountUsage calls to Koha namespace After the introduction of Koha::Authorities->get_usage_count with bug 9988, we can now replace the remaining occurrences of CountUsage. At the same time we remove CountUsageChildren. This was an empty sub. The typo get_count_usage in a subtest title is adjusted. Test plan: [1] Run t/db_dependent/Koha/Authorities.t [2] Perform a search on authorities-home.pl and verify that you see plausible numbers for 'used in xx records'. [3] Click on Details for one authority. See the same number? [4] Do the same as in 2/3 for Authority search on OPAC. [5] Remember the authid and enter this in the record numbers box on tools/batch_delete_records.pl. Select Authorities and click Continue. The next form shows a column "Used in". Do you see the same count again? [6] Git grep CountUsage. You should see just one hit in a comment that can be kept in Koha/Authorities.pm. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Marcel, I agree with the removal, but you seem to be fixing a bug in the meantime (which I'm not against per-se) but I think if falls short, as we should be warning or similar about those non-existing authority records.
(In reply to Tomás Cohen Arazi from comment #3) > Marcel, I agree with the removal, but you seem to be fixing a bug in the > meantime (which I'm not against per-se) but I think if falls short, as we > should be warning or similar about those non-existing authority records. This is about two checks? [1] opac-authoritiesdetail -my $authtypecode = $authority->authtypecode; +my $authtypecode = $authority ? $authority->authtypecode : q{}; This does not fall short. It actually is only theoretical. If GetAuthority fails, the script redirects to 404. [2] authorities/detail.pl -my $authtypecode = Koha::Authorities->find($authid)->authtypecode; +my $authobj = Koha::Authorities->find($authid); +my $authtypecode = $authobj ? $authobj->authtypecode: q{}; Similar here. This check just prevents an ISE on authtypecode while a few lines later the output of GetAuthority is checked and an error is printed via the template. So no, it does not fall short. Please explain why this fails QA.
(In reply to Marcel de Rooy from comment #4) > (In reply to Tomás Cohen Arazi from comment #3) > > Marcel, I agree with the removal, but you seem to be fixing a bug in the > > meantime (which I'm not against per-se) but I think if falls short, as we > > should be warning or similar about those non-existing authority records. > > This is about two checks? > [1] opac-authoritiesdetail > -my $authtypecode = $authority->authtypecode; > +my $authtypecode = $authority ? $authority->authtypecode : q{}; > This does not fall short. It actually is only theoretical. If GetAuthority > fails, the script redirects to 404. > > [2] authorities/detail.pl > -my $authtypecode = Koha::Authorities->find($authid)->authtypecode; > +my $authobj = Koha::Authorities->find($authid); > +my $authtypecode = $authobj ? $authobj->authtypecode: q{}; > Similar here. This check just prevents an ISE on authtypecode while a few > lines later the output of GetAuthority is checked and an error is printed > via the template. So no, it does not fall short. > > Please explain why this fails QA. It is not a big deal, I just would like the script to warn (in the logs) in addition to the fallback to empty string.
(In reply to Tomás Cohen Arazi from comment #5) > (In reply to Marcel de Rooy from comment #4) > > (In reply to Tomás Cohen Arazi from comment #3) > > > Marcel, I agree with the removal, but you seem to be fixing a bug in the > > > meantime (which I'm not against per-se) but I think if falls short, as we > > > should be warning or similar about those non-existing authority records. > > > > This is about two checks? > > [1] opac-authoritiesdetail > > -my $authtypecode = $authority->authtypecode; > > +my $authtypecode = $authority ? $authority->authtypecode : q{}; > > This does not fall short. It actually is only theoretical. If GetAuthority > > fails, the script redirects to 404. > > > > [2] authorities/detail.pl > > -my $authtypecode = Koha::Authorities->find($authid)->authtypecode; > > +my $authobj = Koha::Authorities->find($authid); > > +my $authtypecode = $authobj ? $authobj->authtypecode: q{}; > > Similar here. This check just prevents an ISE on authtypecode while a few > > lines later the output of GetAuthority is checked and an error is printed > > via the template. So no, it does not fall short. > > > > Please explain why this fails QA. > > It is not a big deal, I just would like the script to warn (in the logs) in > addition to the fallback to empty string. We do not need these warns. As mentioned, the script either redirects to 404 or prints an error already.
Created attachment 66984 [details] [review] Bug 18149: Move CountUsage calls to Koha namespace After the introduction of Koha::Authorities->get_usage_count with bug 9988, we can now replace the remaining occurrences of CountUsage. At the same time we remove CountUsageChildren. This was an empty sub. The typo get_count_usage in a subtest title is adjusted. Test plan: [1] Run t/db_dependent/Koha/Authorities.t [2] Perform a search on authorities-home.pl and verify that you see plausible numbers for 'used in xx records'. [3] Click on Details for one authority. See the same number? [4] Do the same as in 2/3 for Authority search on OPAC. [5] Remember the authid and enter this in the record numbers box on tools/batch_delete_records.pl. Select Authorities and click Continue. The next form shows a column "Used in". Do you see the same count again? [6] Git grep CountUsage. You should see just one hit in a comment that can be kept in Koha/Authorities.pm. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
We really should remove Koha::Authorities->get_usage_count and Koha::Authorities->linked_biblionumbers, they are not method for the object set class.
Pushed to master for 17.11, thanks to everybody involved!
(In reply to Jonathan Druart from comment #8) > We really should remove Koha::Authorities->get_usage_count and > Koha::Authorities->linked_biblionumbers, they are not method for the object > set class. Yes, I did put them here temporarily. When refactoring goes further and we have these single objects in place, it will be trivial to move the code from the plural class to the singular class.
Enhancement not pushed to 17.05.x