Bug 15545

Summary: Remove remainders of unfinished reqholdnotes functionality
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: Architecture, internals, and plumbingAssignee: Marcel de Rooy <m.de.rooy>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: gmcharlt, jonathan.druart, kyle, liz, lucas, m.de.rooy, nick, sally.healey
Version: master   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9743
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00
Bug Depends on: 16586, 18037    
Bug Blocks:    
Attachments: Bug 15545: Database revision for HoldNoteReasons pref
Bug 15545: Optionally require notes when placing a hold
Bug 15545: Unit test for IsHoldNoteRequired
Bug 15545: Database revision for HoldNoteReasons pref
Bug 15545: Optionally require notes when placing a hold
Bug 15545: Unit test for IsHoldNoteRequired
Koha Plugin: Rijks_HoldNoteRequired_Extent and _Leader
Bug 15545: Add routine IsHoldNoteRequired to Reserves module
Bug 15545: Use IsHoldNoteRequired in opac-reserve.pl
Bug 15545: Add routine IsHoldNoteRequired to Reserves module
Bug 15545: Use IsHoldNoteRequired in opac-reserve.pl
Bug 15545: Add routine IsHoldNoteRequired to Reserves module
Bug 15545: Use IsHoldNoteRequired in opac-reserve.pl
Bug 15545: Remove reqholdnotes from opac-reserve
Bug 15545: Remove reqholdnotes from opac-reserve

Description Marcel de Rooy 2016-01-11 14:17:35 UTC

    
Comment 1 Marcel de Rooy 2016-01-11 14:37:12 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2016-01-15 10:19:20 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2016-01-15 10:19:26 UTC Comment hidden (obsolete)
Comment 4 Marcel de Rooy 2016-01-15 10:19:31 UTC Comment hidden (obsolete)
Comment 5 Owen Leonard 2016-01-15 15:23:47 UTC
Would the need for this feature be eliminated if it was possible to limit item-level holds to the types of records defined in this patch? That is, serials, titles with volumes, etc?

If so, shouldn't we be solving that problem instead?
Comment 6 Marcel de Rooy 2016-01-18 07:48:15 UTC
(In reply to Owen Leonard from comment #5)
> Would the need for this feature be eliminated if it was possible to limit
> item-level holds to the types of records defined in this patch? That is,
> serials, titles with volumes, etc?
> 
> If so, shouldn't we be solving that problem instead?

We still feel the need for this feature in that case. As long as our data is not perfect (and all issuable materials have been split in the items table), we still want the user to say what he really needs instead of collecting the whole set. To illustrate: a multipart with 2 items a-d,e-h (which should indeed be 8 items).
Comment 7 Marcel de Rooy 2016-03-14 07:49:52 UTC Comment hidden (obsolete)
Comment 8 Marcel de Rooy 2016-03-14 07:49:59 UTC Comment hidden (obsolete)
Comment 9 Marcel de Rooy 2016-03-14 07:50:04 UTC Comment hidden (obsolete)
Comment 10 Marcel de Rooy 2016-03-14 07:50:18 UTC
Rebased
Comment 11 Marcel de Rooy 2016-05-25 12:26:48 UTC
Back to Assigned: Making some changes.
Comment 12 Marcel de Rooy 2016-05-26 12:27:29 UTC
Created attachment 51832 [details]
Koha Plugin: Rijks_HoldNoteRequired_Extent and _Leader
Comment 13 Marcel de Rooy 2016-05-26 12:32:17 UTC Comment hidden (obsolete)
Comment 14 Marcel de Rooy 2016-05-26 12:32:21 UTC Comment hidden (obsolete)
Comment 15 Liz Rea 2016-07-18 03:38:46 UTC
Hi,

I have both the test failing:

t/db_dependent/Reserves/IsHoldNoteRequired.t .. 
1..3
ok 1 - Should be false if plugins are disabled
not ok 2 - Should be true for biblionumber 2

#   Failed test 'Should be true for biblionumber 2'
#   at t/db_dependent/Reserves/IsHoldNoteRequired.t line 53.
#          got: undef
#     expected: '1'
ok 3 - Should be false for biblionumber 0
# Looks like you failed 1 test of 3.
Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/3 subtests 

Test Summary Report
-------------------
t/db_dependent/Reserves/IsHoldNoteRequired.t (Wstat: 256 Tests: 3 Failed: 1)
  Failed test:  2
  Non-zero exit status: 1
Files=1, Tests=3,  2 wallclock secs ( 0.01 usr  0.00 sys +  0.99 cusr  0.05 csys =  1.05 CPU)
Result: FAIL

And I can't seem to make it work functionally either, sorry.

Liz
Comment 16 Marcel de Rooy 2016-07-18 06:45:05 UTC
(In reply to Liz Rea from comment #15)
> I have both the test failing:
> 
> t/db_dependent/Reserves/IsHoldNoteRequired.t .. 
> 1..3
> ok 1 - Should be false if plugins are disabled
> not ok 2 - Should be true for biblionumber 2
I had the same result when I did not apply the dependency. Please first apply the (fairly simple) patch for bug 16586. When I run the test again, it passes.


> And I can't seem to make it work functionally either, sorry.
Could you explain further? Related to the missing depend ?

Thx for testing this. It did not yet receive much attention lately.
Comment 17 Liz Rea 2016-07-19 21:22:32 UTC
It may well be because of the dependency, I missed that. I'll have another go. :)

Cheers,
Liz
Comment 18 Liz Rea 2016-07-20 03:35:02 UTC
Hi,

With the dependency, I got the tests to pass. Yay!

However I still can't make the required hold notes work. Also was further confused by bug 16391, which makes your plugins not show up in the list when you go back to the page, even if they are installed. Removing method=tool from the url makes them turn up. That's not your fault, it just caused me trouble when I was testing.

The patches don't break anything that I can see. I was testing on current master, 3692f92.

Maybe you know something else I could be doing wrong?

Cheers,
Liz
Comment 19 Marcel de Rooy 2016-07-20 11:47:41 UTC
(In reply to Liz Rea from comment #18)

Hi,

> However I still can't make the required hold notes work. Also was further
> confused by bug 16391, which makes your plugins not show up in the list when
> you go back to the page, even if they are installed. Removing method=tool
> from the url makes them turn up. That's not your fault, it just caused me
> trouble when I was testing.
You are right. I made a remark on that report too. Could add a fake tool subroutine to make them show up, but that would not be a real solution.

> The patches don't break anything that I can see. I was testing on current
> master, 3692f92.
Well, that is a major goal :)
 
> Maybe you know something else I could be doing wrong?
First verify that the plugin is installed etc., so we know that it is called. Pick a marc record with available item, change the leader (probably replace nam by nas), i.e. make sure that position 7 is the character s.
Go to opac, place a hold on that specific record. If you unhide the options, you already should see the text line: Please enter additional information about the requested item. Pressing Place Hold should not work. Depending on the browser, the text box is marked red or something. If that is the case, you actually finished testing already (the hold note indeed is required now).

Alternatively, you could also edit the plugin's Leader.pm, add a "return 1;" in the check subroutine near the top. Because we are not focusing on testing the plugin here but only the routine in Reserves calling available plugins. In that case messing with records is not needed..

Thanks again.
Comment 20 Liz Rea 2016-07-20 21:45:09 UTC
Hi Marcel,

I think I see what I did wrong now - I was testing this in the staff client! How very silly of me. Will give it one more go today, you know, doing it properly. :)

Liz
Comment 21 Liz Rea 2016-07-20 23:28:03 UTC
Created attachment 53546 [details] [review]
Bug 15545: Add routine IsHoldNoteRequired to Reserves module

This routine will be used to determine if a hold note is required.
In the current approach this is implemented by use of Koha plugins.
A former approach included a syspref. This is no longer needed. This
approach is even more flexible.

The routine will use all plugins it finds with metadata tag
'implements' set to 'IsHoldNoteRequired'. If one of these plugins has a
check method that returns true, the hold note should be regarded as
required.

The plugins Rijks_HoldNoteRequired_Extent and _Leader offer an
implementation where the value of the leader or MARC21 300$a determines
if the hold note is required. Similarly, any other field of the
bibliographic record could be used in a new plugin.

Test plan:
Run unit test t/db_dependent/Reserves/IsHoldNoteRequired.t.

Signed-off-by: Liz Rea  <liz@catalyst.net.nz>
Comment 22 Liz Rea 2016-07-20 23:28:56 UTC
Created attachment 53547 [details] [review]
Bug 15545: Use IsHoldNoteRequired in opac-reserve.pl

This patch implements use of the new Reserves routine IsHoldNoteRequired,
making it possible to require a note when placing a hold.
Only a very small change in opac-reserve.pl is needed. As well as
corresponding changes in the template. I am removing some unused template
code related to a former approach.

Test plan:
[1] Disable UseKohaPlugins, enable OpacHoldNotes.
[2] Place a hold on a serial record (leader pos 7 == s). Verify that
    the hold note is not required.
[3] Enable UseKohaPlugins. Install the Rijks_HoldNoteRequired plugins.
    (Go to plugins/plugins-home.pl and upload Rijks_HoldNoteRequired.kpz.
    This file can be found as an attachment to this BZ report.)
[4] Again place a hold on a serial record. The hold note should be required.

Signed-off-by: Liz Rea  <liz@catalyst.net.nz>
Comment 23 Marcel de Rooy 2016-07-21 06:22:37 UTC
(In reply to Liz Rea from comment #20)
> I think I see what I did wrong now - I was testing this in the staff client!
> How very silly of me. Will give it one more go today, you know, doing it
> properly. :)

Thx Liz for your signoff. Should have mentioned opac in the latest commit message..
Comment 24 Liz Rea 2016-07-21 21:04:02 UTC
to be fair it says "OpacHoldNotes" so... yeah I just wasn't putting two and two together. :)
Comment 25 Jonathan Druart 2016-07-25 16:42:44 UTC
I don't think I am ready to QA this one.
Julian has announced a new plugin system few weeks ago and this patch will adds a precedent for the existing plugin system.
Do you have in mind other uses case? Did you plan to extend it to other behaviors?
It would be great to ping koha-devel to get other dev opinions.
Comment 26 Marcel de Rooy 2016-07-25 18:27:47 UTC
(In reply to Jonathan Druart from comment #25)
> I don't think I am ready to QA this one.
> Julian has announced a new plugin system few weeks ago and this patch will
> adds a precedent for the existing plugin system.
> Do you have in mind other uses case? Did you plan to extend it to other
> behaviors?
> It would be great to ping koha-devel to get other dev opinions.

Please follow the trace in the see also bug and see that it is already three years under way.
Comment 27 Jonathan Druart 2016-08-01 09:53:10 UTC
(In reply to Marcel de Rooy from comment #26)
> (In reply to Jonathan Druart from comment #25)
> > I don't think I am ready to QA this one.
> > Julian has announced a new plugin system few weeks ago and this patch will
> > adds a precedent for the existing plugin system.
> > Do you have in mind other uses case? Did you plan to extend it to other
> > behaviors?
> > It would be great to ping koha-devel to get other dev opinions.
> 
> Please follow the trace in the see also bug and see that it is already three
> years under way.

I do not understand, are you talking about bug 9743? How this bug report can answer my questions?
Comment 28 Marcel de Rooy 2016-08-01 10:02:08 UTC
(In reply to Jonathan Druart from comment #27)
> (In reply to Marcel de Rooy from comment #26)
> > (In reply to Jonathan Druart from comment #25)
> > > I don't think I am ready to QA this one.
> > > Julian has announced a new plugin system few weeks ago and this patch will
> > > adds a precedent for the existing plugin system.
> > > Do you have in mind other uses case? Did you plan to extend it to other
> > > behaviors?
> > > It would be great to ping koha-devel to get other dev opinions.
> > 
> > Please follow the trace in the see also bug and see that it is already three
> > years under way.
> 
> I do not understand, are you talking about bug 9743? How this bug report can
> answer my questions?

It didn't ? :)
My point is: why block something that I am waiting for over three years because something else was created a few months ago?
Comment 29 Jonathan Druart 2016-08-01 11:02:28 UTC
(In reply to Marcel de Rooy from comment #28)
> (In reply to Jonathan Druart from comment #27)
> > (In reply to Marcel de Rooy from comment #26)
> > > (In reply to Jonathan Druart from comment #25)
> > > > I don't think I am ready to QA this one.
> > > > Julian has announced a new plugin system few weeks ago and this patch will
> > > > adds a precedent for the existing plugin system.
> > > > Do you have in mind other uses case? Did you plan to extend it to other
> > > > behaviors?
> > > > It would be great to ping koha-devel to get other dev opinions.
> > > 
> > > Please follow the trace in the see also bug and see that it is already three
> > > years under way.
> > 
> > I do not understand, are you talking about bug 9743? How this bug report can
> > answer my questions?
> 
> It didn't ? :)
> My point is: why block something that I am waiting for over three years
> because something else was created a few months ago?

The version of the patch on bug 9743 and the first version on this bug report did not introduced a new way to modify behaviors using plugings. If they get pushed, we will have a precedent and new patches/plugins will use it. That will add more stuffs to modify if we decide the switch our plugin system.
Comment 30 Marcel de Rooy 2016-08-01 11:07:51 UTC
I'm kind of exhausted in finding new ways to get this simple change in Koha.
Comment 31 Jonathan Druart 2016-08-01 11:18:26 UTC
(In reply to Marcel de Rooy from comment #30)
> I'm kind of exhausted in finding new ways to get this simple change in Koha.

My goal was not to exhaust you, but just to make sure it was the best thing to do.
Sorry about that. My suggestion was to ask on koha-devel what others think.
Comment 32 Marcel de Rooy 2016-08-01 11:25:36 UTC
(In reply to Jonathan Druart from comment #31)
> (In reply to Marcel de Rooy from comment #30)
> > I'm kind of exhausted in finding new ways to get this simple change in Koha.
> 
> My goal was not to exhaust you, but just to make sure it was the best thing
> to do.
> Sorry about that. My suggestion was to ask on koha-devel what others think.

No problem. But I prefer not to keep this report pending for another longer period of time. Moving it to custom :)
Comment 33 Marcel de Rooy 2016-12-15 08:06:39 UTC
(In reply to Jonathan Druart from comment #29)
> The version of the patch on bug 9743 and the first version on this bug
> report did not introduced a new way to modify behaviors using plugings. If
> they get pushed, we will have a precedent and new patches/plugins will use
> it. That will add more stuffs to modify if we decide the switch our plugin
> system.

Reopened and back to SO qeueue. Still applies.
There has been no movement on the plugin system since the above was written.
And note that one of the two patches was written Dec 2014.
The argument that this "introduces a new way to use plugins" is imo not really valid. No dangerous precedents here. It would be quite trivial to adjust this routine to a new plugin system as should be done for others too.
Comment 34 Marcel de Rooy 2017-01-19 14:19:37 UTC
Created attachment 59252 [details] [review]
Bug 15545: Add routine IsHoldNoteRequired to Reserves module

This routine will be used to determine if a hold note is required.
In the current approach this is implemented by use of Koha plugins.
A former approach included a syspref. This is no longer needed. This
approach is even more flexible.

The routine will use all plugins it finds with metadata tag
'implements' set to 'IsHoldNoteRequired'. If one of these plugins has a
check method that returns true, the hold note should be regarded as
required.

The plugins Rijks_HoldNoteRequired_Extent and _Leader offer an
implementation where the value of the leader or MARC21 300$a determines
if the hold note is required. Similarly, any other field of the
bibliographic record could be used in a new plugin.

Test plan:
Run unit test t/db_dependent/Reserves/IsHoldNoteRequired.t.

Signed-off-by: Liz Rea  <liz@catalyst.net.nz>
Comment 35 Marcel de Rooy 2017-01-19 14:19:42 UTC
Created attachment 59253 [details] [review]
Bug 15545: Use IsHoldNoteRequired in opac-reserve.pl

This patch implements use of the new Reserves routine IsHoldNoteRequired,
making it possible to require a note when placing a hold.
Only a very small change in opac-reserve.pl is needed. As well as
corresponding changes in the template. I am removing some unused template
code related to a former approach.

Test plan:
[1] Disable UseKohaPlugins, enable OpacHoldNotes.
[2] Place a hold on a serial record (leader pos 7 == s). Verify that
    the hold note is not required.
[3] Enable UseKohaPlugins. Install the Rijks_HoldNoteRequired plugins.
    (Go to plugins/plugins-home.pl and upload Rijks_HoldNoteRequired.kpz.
    This file can be found as an attachment to this BZ report.)
[4] Again place a hold on a serial record. The hold note should be required.

Signed-off-by: Liz Rea  <liz@catalyst.net.nz>
Comment 36 Marcel de Rooy 2017-01-19 14:20:10 UTC
Rebase for C4/Reserves
Comment 37 Jonathan Druart 2017-01-27 11:36:45 UTC
I do not manage to clear up my mind about the use of this plugin.

I'd say it's the kind of feature that could be driven by a syspref. The quantity of code to wrap the plugin seems the same as the size of the plugin itself. It is also the kind of code we will not be able to know if it is used by someone in few years (what we do have with a syspref, cf Hea).

I would like to see the C4::Reserves sub moved to a Koha::Hold method at least.

Kyle, can we get your opinion on this one please?
Comment 38 Marcel de Rooy 2017-01-27 12:33:21 UTC
(In reply to Jonathan Druart from comment #37)
> I would like to see the C4::Reserves sub moved to a Koha::Hold method at
> least.

I would not block this for that reason; the change was already pending before Koha::Objects were invented :)
Comment 39 Kyle M Hall 2017-01-27 12:48:29 UTC
I'm really on the fence. I don't see anything wrong with extending the plugin functionality, but on the other hand, there is a *very* specific behavior. If it were an extension to control all the hold fields it would seem a bit less odd.

Marcel, considering the small size of the patches, I don't think moving the sub to Koha::Hold is too big of an ask ; )
Comment 40 Marcel de Rooy 2017-01-27 13:21:48 UTC
From khall on irc to clarify:

marcelr: pluggability should be for the controlling behavior, not for the action imo. I'm trying to say if the plugin hook could be used to decide if any given hold form field should be required it would make more sense to me. That being said, I'm not opposed to it the way it works atm. I'm wondering if maybe HEA should be able to track installed plugins, that would be useful
Comment 41 Marcel de Rooy 2017-02-02 13:27:41 UTC
Blocked (temporarily) in favor of bug 18037.
Actually splitting the patch set.
Comment 42 Marcel de Rooy 2022-08-29 13:38:52 UTC
Gonna remove the remainders of this bug report here now.
Comment 43 Marcel de Rooy 2022-08-29 13:48:57 UTC
Created attachment 139947 [details] [review]
Bug 15545: Remove reqholdnotes from opac-reserve

This never really made it. So let's remove all traces.

Test plan:
Look at code changes and git grep reqholdnotes.
Bonus: Place a hold with/without hold notes.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 44 Marcel de Rooy 2022-08-29 13:49:38 UTC
Trivial: self SO. Just housekeeping.
Comment 45 Kyle M Hall 2022-09-23 18:13:03 UTC
Created attachment 140960 [details] [review]
Bug 15545: Remove reqholdnotes from opac-reserve

This never really made it. So let's remove all traces.

Test plan:
Look at code changes and git grep reqholdnotes.
Bonus: Place a hold with/without hold notes.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 46 Tomás Cohen Arazi 2022-09-26 14:06:52 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!