[Bug 18149] New: Move CountUsage calls to Koha namespace
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18149 Bug ID: 18149 Summary: Move CountUsage calls to Koha namespace Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: gmcharlt@gmail.com Reporter: m.de.rooy@rijksmuseum.nl QA Contact: testopia@bugs.koha-community.org Built on top of bug 9988, that introduces Koha::Authority::get_usage_count. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18149 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |15449 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15449 [Bug 15449] Move stuffs to the Koha namespace -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18149 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |9988 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9988 [Bug 9988] Leave larger authority merges to merge_authority cronjob (pref AuthorityMergeLimit) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18149 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |17908 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17908 [Bug 17908] Authority merge omnibus -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18149 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Assignee|gmcharlt@gmail.com |m.de.rooy@rijksmuseum.nl -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18149 Jacek Ablewicz <abl@biblos.pk.edu.pl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |abl@biblos.pk.edu.pl -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18149 Bug 18149 depends on bug 9988, which changed state. Bug 9988 Summary: Leave larger authority merges to merge_authorities cronjob (pref AuthorityMergeLimit) https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9988 What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18149 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18149 --- Comment #1 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 64720 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=64720&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18149 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18149 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #64720|0 |1 is obsolete| | --- Comment #2 from Josef Moravec <josef.moravec@gmail.com> --- Created attachment 65653 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=65653&action=edit [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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18149 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com QA Contact|testopia@bugs.koha-communit |tomascohen@gmail.com |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18149 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #3 from Tomás Cohen Arazi <tomascohen@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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18149 --- Comment #4 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18149 --- Comment #5 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
(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
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18149 --- Comment #6 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Tomás Cohen Arazi from comment #5) prints an error already. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18149 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18149 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18149 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #65653|0 |1 is obsolete| | --- Comment #7 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 66984 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66984&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18149 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org --- Comment #8 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- We really should remove Koha::Authorities->get_usage_count and Koha::Authorities->linked_biblionumbers, they are not method for the object set class. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18149 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master --- Comment #9 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Pushed to master for 17.11, thanks to everybody involved! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18149 --- Comment #10 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18149 Fridolin SOMERS <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |FIXED Status|Pushed to Master |RESOLVED CC| |fridolin.somers@biblibre.co | |m --- Comment #11 from Fridolin SOMERS <fridolin.somers@biblibre.com> --- Enhancement not pushed to 17.05.x -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org