Bug 22847 - Specific circ rule by patron category is displaying the default (or not displaying)
Summary: Specific circ rule by patron category is displaying the default (or not displ...
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low critical (vote)
Target Milestone: ---
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 18887 18925
Blocks: 18928
  Show dependency treegraph
 
Reported: 2019-05-03 21:33 UTC by Jonathan Druart
Modified: 2019-06-13 14:27 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
19.05.00, 18.11.06


Attachments
Bug 22847: Correctly displayed circ rule values for max_holds and maxissue* (4.06 KB, patch)
2019-05-03 21:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22847: Correctly displayed circ rule values for max_holds and maxissue* (4.10 KB, patch)
2019-05-06 16:18 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 22847: Correctly displayed circ rule values for max_holds and maxissue* (4.16 KB, patch)
2019-05-06 23:18 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22847: Correctly displayed circ rule values for max_holds and maxissue* (4.21 KB, patch)
2019-05-07 14:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22847: [18.11] Correctly displayed circ rule values for max_holds (2.32 KB, patch)
2019-05-16 17:32 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2019-05-03 21:33:09 UTC
There are several things going wrong on the circulation rules screen. The global rule is displayed for specific libraries (even if the rule does not exist for the library), and max_holds is always blank.

I have trouble finding the root of the issue, but I will propose a patch to fix both problem.

Certainly a mismatch between bug 18887 and bug 18925.
Comment 1 Jonathan Druart 2019-05-03 21:35:40 UTC
Created attachment 89335 [details] [review]
Bug 22847: Correctly displayed circ rule values for max_holds and maxissue*

There are several things going wrong on the circulation rules screen. The global rule is displayed for specific libraries (even if the rule does not exist for the library), and max_holds is always blank.

I have trouble finding the root of the issue, but I will propose a patch to fix both problem.

Certainly a mismatch between bug 18887 and bug 18925.

Test plan:
0. Do not apply the patch
1. Notice the issue on the circulation rules screen
2. Apply the patch
3. Confirm that the value you set are correctly displayed (and not the propagated ones!)
Comment 2 Liz Rea 2019-05-06 16:18:26 UTC
Created attachment 89373 [details] [review]
Bug 22847: Correctly displayed circ rule values for max_holds and maxissue*

There are several things going wrong on the circulation rules screen. The global rule is displayed for specific libraries (even if the rule does not exist for the library), and max_holds is always blank.

I have trouble finding the root of the issue, but I will propose a patch to fix both problem.

Certainly a mismatch between bug 18887 and bug 18925.

Test plan:
0. Do not apply the patch
1. Notice the issue on the circulation rules screen
2. Apply the patch
3. Confirm that the value you set are correctly displayed (and not the propagated ones!)

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 3 Agustín Moyano 2019-05-06 23:18:12 UTC
Created attachment 89401 [details] [review]
Bug 22847: Correctly displayed circ rule values for max_holds and maxissue*

There are several things going wrong on the circulation rules screen. The global rule is displayed for specific libraries (even if the rule does not exist for the library), and max_holds is always blank.

I have trouble finding the root of the issue, but I will propose a patch to fix both problem.

Certainly a mismatch between bug 18887 and bug 18925.

Test plan:
0. Do not apply the patch
1. Notice the issue on the circulation rules screen
2. Apply the patch
3. Confirm that the value you set are correctly displayed (and not the propagated ones!)

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 4 Kyle M Hall 2019-05-07 14:33:48 UTC
Created attachment 89439 [details] [review]
Bug 22847: Correctly displayed circ rule values for max_holds and maxissue*

There are several things going wrong on the circulation rules screen. The global rule is displayed for specific libraries (even if the rule does not exist for the library), and max_holds is always blank.

I have trouble finding the root of the issue, but I will propose a patch to fix both problem.

Certainly a mismatch between bug 18887 and bug 18925.

Test plan:
0. Do not apply the patch
1. Notice the issue on the circulation rules screen
2. Apply the patch
3. Confirm that the value you set are correctly displayed (and not the propagated ones!)

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 5 Nick Clemens 2019-05-09 18:21:41 UTC
Awesome work all!

Pushed to master for 19.05
Comment 6 Jonathan Druart 2019-05-16 17:32:31 UTC
Created attachment 89863 [details] [review]
Bug 22847: [18.11] Correctly displayed circ rule values for max_holds

Here a version for 18.11, it needs a signoff!
Comment 7 Victor Grousset/tuxayo 2019-05-24 10:23:18 UTC
1. Fill the 3 fields of Default checkout, hold and return policy
2. Same for a particular library with different values
3. Unset the policy for the particular library
4. Then you will see that the field "Maximum total holds allowed (count)" ends up with the value for all libraries policy
5. Change the value for all libraries policy, then return to the particular library
6. You should see that the value was updated as well
7. Which is wrooooooong
8. Apply the patch
9. You should see that the particular library policy is empty
10. You should see in the all lib policy that the "Maximum total holds allowed (count)" field is not empty
    - Is that expected???
11. Refill all the fields
12. Then you should see that for both policies, "Maximum total holds allowed (count)" stays empty
13. Wait this is definitely not supposed to happen!
14. Nooooooooooo! T_T
Comment 8 Martin Renvoize 2019-05-24 13:01:47 UTC
Looks like this isn't ready for 18.11.x yet.. setting to needs-more-work
Comment 9 Victor Grousset/tuxayo 2019-05-24 13:20:54 UTC Comment hidden (obsolete)
Comment 10 Victor Grousset/tuxayo 2019-05-24 13:25:40 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2019-05-24 22:07:14 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2019-05-24 22:07:42 UTC
(In reply to Victor Grousset/tuxayo from comment #7)

I do not understand what you are trying to show.

> 10. You should see in the all lib policy that the "Maximum total holds
> allowed (count)" field is not empty
>     - Is that expected???

Yes of course, what do you expect? I contain the value you filled in before.

> 11. Refill all the fields

Which ones? For "all" they are already filled.

> 12. Then you should see that for both policies, "Maximum total holds allowed
> (count)" stays empty

Nope, I see what I just entered.

All good for me, please retest.
Comment 13 Martin Renvoize 2019-05-30 19:07:20 UTC
I'm going to go for it here.. I can't replicate any issues once Jonathans patch is applied.. Thanks for your efforts in reworking it for 18.11.x series.

Pushed to 18.11.x for 18.11.06
Comment 14 Victor Grousset/tuxayo 2019-05-31 02:22:46 UTC
(In reply to Jonathan Druart from comment #12)
> (In reply to Victor Grousset/tuxayo from comment #7)
> > 10. You should see in the all lib policy that the "Maximum total holds
> > allowed (count)" field is not empty
> >     - Is that expected???
> 
> Yes of course, what do you expect? I contain the value you filled in before.

Okay so the patch manages to display the value that in the previous version was still saved but not displayed. I didn't understood this, so it seemed that the patch was changing the config.

> > 11. Refill all the fields
> 
> Which ones? For "all" they are already filled.

"Default checkout, hold and return policy"
It was to test if changing the values still worked.

> > 12. Then you should see that for both policies, "Maximum total holds allowed
> > (count)" stays empty
> 
> Nope, I see what I just entered.
> 
> All good for me, please retest.

It might have an issue with my setup or misunderstood something functionally when trying to guess some details of the test plan.
Comment 15 Victor Grousset/tuxayo 2019-05-31 02:24:06 UTC
Thanks Joubu for the patch.
Thanks Martin for retesting.