|
Description
Stefan Berndtsson
2018-01-22 11:54:15 UTC
Created attachment 70791 [details] [review] Add option to send permanent_location as AQ in SIP response Created attachment 125364 [details] [review] Bug 20058: Add option to send permanent_location as AQ in SIP response Created attachment 125365 [details] [review] Bug 20058: Add tests Rebased against master and added tests. Created attachment 125366 [details] [review] Bug 20058: Add tests --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
@@ -655,10 +655,19 @@ Circulation:
- pref: StoreLastBorrower
default: 0
choices:
+ -
1: Store
0: "Don't store"
This change is a mistake no ?
Yes! Will fix this. Created attachment 125454 [details] [review] Bug 20058: Add option to send permanent_location as AQ in SIP response Created attachment 125455 [details] [review] Bug 20058: Add tests A quick observation here: 1) I notice that the new system preference is added under Holds policy, I think that doesn't fit well and should better bemoved to be under the new SIP2 heading. 2) As this is supposed to be used for sorting: there is now a configuration option specific to sort bins with the SIP2SortBinMapping system preference, but it's setting the CL field. Is this still required with this new development? Also adding Kyle and Martin as they know SIP2 well, I am not sure if we don't have a way to achieve that yet via the conf file. Hmm, well bug 20517 uses the CL field for sorter, which from memory is what sorters here use.. I'd have to look at the docs again to remind myself what AQ is for, but from the code this looks reasonable. The main difference for us, and the reason for making the patch in the first place, is that our sorting is done in each machine based on CT (branchcode) and AQ (permanent_location). CT is only set when the item is being sent to another library, so we first sort on that, and if it is not set, we sort on AQ for different destinations within the same library. Using CL is likely a functional way forward for sorting, but it moves the sorting rules to the server, which is not something that's setup yet, so I haven't looked into that here. When we first wrote this patch (in 2018 or so), Koha did not produce CT, so AQ was all we had. Now that Koha sets CT to homebranch when transferring, the default behaviour of also setting AQ to the same thing seems a bit redundant. The SIP name of the AQ field is "permanent location" (FID_PERM_LOCN), although what that technically means is of course an interpretation. To me, AQ and CL has two different meanings. One is information on the returned item (AQ), one is instructions to a machine (CL). Created attachment 138925 [details] [review] Bug 20058: Add option to send permanent_location as AQ in SIP response Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 138926 [details] [review] Bug 20058: Add tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Thanks for the explanation.. all works as described to signing off. Over to Kyle for a final QA run :) PS, whilst rebasing it to apply I moved the syspref into the SIP2 section for display. (In reply to Martin Renvoize from comment #15) > Thanks for the explanation.. all works as described to signing off. Over to > Kyle for a final QA run :) > > PS, whilst rebasing it to apply I moved the syspref into the SIP2 section > for display. Thanks! Created attachment 140068 [details] [review] Bug 20058: Add option to send permanent_location as AQ in SIP response Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 140069 [details] [review] Bug 20058: Add tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 140070 [details] [review] Bug 20058: (QA follow-up) Add exec flag to install file Created attachment 140071 [details] [review] Bug 20058: (QA follow-up) Simplify test code Pushed to master for 22.11. Nice work everyone, thanks! + yes: permanent_location + no: homebranch This is wrong, it must be 1 and 0, no "yes" and "no". There are more of these, I've filed a separate bug and provided a patch: Bug 33060 - Fix yes/no setting to 1/0 in system preference YAML files |