This report adds three cases where you could want to require a note when placing a hold. Each of these cases may be enabled separately via a system preference. (The hold note itself is triggered by OpacHoldNotes.) The three cases are: serial indication in the leader, collection indication in the leader, and MARC 300$a contains an indication of multiple parts/volumes (identified via a regular expression). A new sub in Reserves handles these cases and will be easily extensible. A unit test will be provided too.
Created attachment 46687 [details] [review] Bug 15545: Database revision for HoldNoteReasons pref Adds a new preference. Test plan: Run the db revision. Check the pref in the interface.
Created attachment 46688 [details] [review] Bug 15545: Optionally require notes when placing a hold This report adds three cases where you could want to require a note when placing a hold. Each of these cases may be enabled separately via the system preference HoldNoteReasons. (The hold note itself is triggered by OpacHoldNotes.) These three cases are: [1] serial indication in the leader (called LEADER_SERIAL) [2] collection indication in the leader (LEADER_COLLECTION) [3] MARC 300$a contains an indication of multiple parts/volumes, as identified by a regular expression (MARC300A_PARTS). The new sub IsHoldNoteRequired in Reserves.pm handles these cases. And will be easily extensible. Also this patch removes some unused code in the reserves template for mandatory hold notes reasons. Test plan: [1] Enable OpacHoldNotes. Set HoldNoteReasons to LEADER_SERIAL. [2] In OPAC, place a hold on a serial record (with correct leader). Verify that the hold notes are required. [3] Set HoldNoteReasons to MARC300A_PARTS. Verify that the hold notes are no longer required for the record in the preceding step. (This actually depends on your 300$a..) Cancel the hold you placed. [4] Add "2 vols" to MARC 300$a in the same record. Try to place the hold again. The hold notes should be required again. [5] Place a multiple hold request with the same record and another one. Click Place Hold while more-options is collapsed (blank notes). Is the correct hold note made visible?
Created attachment 46689 [details] [review] Bug 15545: Unit test for IsHoldNoteRequired Adds unit test. Test plan: Run the test t/db../Reserves/IsHoldNoteRequired.t
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?
(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).
Created attachment 49084 [details] [review] Bug 15545: Database revision for HoldNoteReasons pref Adds a new preference. Test plan: Run the db revision. Check the pref in the interface.
Created attachment 49085 [details] [review] Bug 15545: Optionally require notes when placing a hold This report adds three cases where you could want to require a note when placing a hold. Each of these cases may be enabled separately via the system preference HoldNoteReasons. (The hold note itself is triggered by OpacHoldNotes.) These three cases are: [1] serial indication in the leader (called LEADER_SERIAL) [2] collection indication in the leader (LEADER_COLLECTION) [3] MARC 300$a contains an indication of multiple parts/volumes, as identified by a regular expression (MARC300A_PARTS). The new sub IsHoldNoteRequired in Reserves.pm handles these cases. And will be easily extensible. Also this patch removes some unused code in the reserves template for mandatory hold notes reasons. Test plan: [1] Enable OpacHoldNotes. Set HoldNoteReasons to LEADER_SERIAL. [2] In OPAC, place a hold on a serial record (with correct leader). Verify that the hold notes are required. [3] Set HoldNoteReasons to MARC300A_PARTS. Verify that the hold notes are no longer required for the record in the preceding step. (This actually depends on your 300$a..) Cancel the hold you placed. [4] Add "2 vols" to MARC 300$a in the same record. Try to place the hold again. The hold notes should be required again. [5] Place a multiple hold request with the same record and another one. Click Place Hold while more-options is collapsed (blank notes). Is the correct hold note made visible?
Created attachment 49086 [details] [review] Bug 15545: Unit test for IsHoldNoteRequired Adds unit test. Test plan: Run the test t/db../Reserves/IsHoldNoteRequired.t
Rebased
Back to Assigned: Making some changes.
Created attachment 51832 [details] Koha Plugin: Rijks_HoldNoteRequired_Extent and _Leader
Created attachment 51833 [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.
Created attachment 51834 [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.
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
(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.
It may well be because of the dependency, I missed that. I'll have another go. :) Cheers, Liz
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
(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.
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
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>
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>
(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..
to be fair it says "OpacHoldNotes" so... yeah I just wasn't putting two and two together. :)
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.
(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.
(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?
(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?
(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.
I'm kind of exhausted in finding new ways to get this simple change in Koha.
(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.
(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 :)
(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.
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>
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>
Rebase for C4/Reserves
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?
(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 :)
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 ; )
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
Blocked (temporarily) in favor of bug 18037. Actually splitting the patch set.
Gonna remove the remainders of this bug report here now.
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>
Trivial: self SO. Just housekeeping.
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>
Pushed to master for 22.11. Nice work everyone, thanks!