Bug 18149 - Move CountUsage calls to Koha namespace
Summary: Move CountUsage calls to Koha namespace
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: Marcel de Rooy
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on: 9988
Blocks: 15449 17908
  Show dependency treegraph
 
Reported: 2017-02-22 08:58 UTC by Marcel de Rooy
Modified: 2018-06-04 20:10 UTC (History)
4 users (show)

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


Attachments
Bug 18149: Move CountUsage calls to Koha namespace (6.15 KB, patch)
2017-06-29 12:42 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 18149: Move CountUsage calls to Koha namespace (6.18 KB, patch)
2017-08-08 14:46 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18149: Move CountUsage calls to Koha namespace (6.25 KB, patch)
2017-09-08 12:13 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2017-02-22 08:58:09 UTC
Built on top of bug 9988, that introduces Koha::Authority::get_usage_count.
Comment 1 Marcel de Rooy 2017-06-29 12:42:30 UTC
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>
Comment 2 Josef Moravec 2017-08-08 14:46:40 UTC
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>
Comment 3 Tomás Cohen Arazi 2017-09-01 18:43:37 UTC
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.
Comment 4 Marcel de Rooy 2017-09-04 06:45:54 UTC
(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.
Comment 5 Tomás Cohen Arazi 2017-09-04 10:04:02 UTC
(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.
Comment 6 Marcel de Rooy 2017-09-04 10:08:29 UTC
(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.
Comment 7 Tomás Cohen Arazi 2017-09-08 12:13:48 UTC
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>
Comment 8 Jonathan Druart 2017-09-19 13:53:49 UTC
We really should remove Koha::Authorities->get_usage_count and Koha::Authorities->linked_biblionumbers, they are not method for the object set class.
Comment 9 Jonathan Druart 2017-09-19 14:49:27 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 10 Marcel de Rooy 2017-10-02 08:49:59 UTC
(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.
Comment 11 Fridolin Somers 2017-10-23 14:51:19 UTC
Enhancement not pushed to 17.05.x