Bug 15522 - New interface for revamped circulation rules
Summary: New interface for revamped circulation rules
Status: Patch doesn't apply
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Bugs List
QA Contact: Testopia
URL: https://gitlab.com/joubu/Koha/commits...
Keywords:
Depends on: 21156 15520 18936
Blocks: 15523 18886
  Show dependency treegraph
 
Reported: 2016-01-07 21:54 UTC by Jesse Weaver
Modified: 2021-06-16 23:47 UTC (History)
31 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 15522: Add API for circulation rules (23.45 KB, patch)
2017-10-17 23:00 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 15522: Add JS build pipeline (95.20 KB, patch)
2017-10-17 23:00 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 15522: Add production JS build mode (4.71 KB, patch)
2017-10-17 23:00 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 15522: New interface for updated circulation rules (59.04 KB, patch)
2017-10-17 23:00 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 15522: Remove old circulation rules interface (85.27 KB, patch)
2017-10-17 23:00 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 15522: Add API for circulation rules (23.12 KB, patch)
2018-01-29 23:30 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 15522: Add JS build pipeline (95.20 KB, patch)
2018-01-29 23:30 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 15522: Add production JS build mode (4.71 KB, patch)
2018-01-29 23:30 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 15522: New interface for updated circulation rules (59.10 KB, patch)
2018-01-29 23:30 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 15522: Remove old circulation rules interface (87.32 KB, patch)
2018-01-29 23:30 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 15522: Remove old circulation rules interface (86.78 KB, patch)
2018-02-26 19:20 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 15522: Remove old circulation rules interface (87.26 KB, patch)
2018-02-26 19:35 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 15522: Add API for circulation rules (23.27 KB, patch)
2019-08-05 03:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15522: Add JS build pipeline (4.64 KB, patch)
2019-08-05 03:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15522: New interface for updated circulation rules (58.16 KB, patch)
2019-08-05 03:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15522: Remove old circulation rules interface (90.90 KB, patch)
2019-08-05 03:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15522: Add yarn.lock (autogenerated) (84.30 KB, patch)
2019-08-05 03:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15522: Fix permission name manage_circ_rules_from_any_libraries (2.85 KB, patch)
2019-08-05 03:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bud 15522: Restore categories filtering (1.61 KB, patch)
2019-08-05 03:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15522: Replace the obsoleted fixFloat occurrence to use Sticky (990 bytes, patch)
2019-08-05 03:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15522: Add API for circulation rules (23.27 KB, patch)
2019-10-15 12:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15522: Add JS build pipeline (4.64 KB, patch)
2019-10-15 12:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15522: New interface for updated circulation rules (58.16 KB, patch)
2019-10-15 12:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15522: Remove old circulation rules interface (91.24 KB, patch)
2019-10-15 12:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15522: Add yarn.lock (autogenerated) (84.30 KB, patch)
2019-10-15 12:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15522: Fix permission name manage_circ_rules_from_any_libraries (2.85 KB, patch)
2019-10-15 12:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bud 15522: Restore categories filtering (1.61 KB, patch)
2019-10-15 12:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15522: Replace the obsoleted fixFloat occurrence to use Sticky (990 bytes, patch)
2019-10-15 12:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15522: Add API for circulation rules (23.26 KB, patch)
2020-02-18 08:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15522: Add JS build pipeline (4.64 KB, patch)
2020-02-18 08:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15522: New interface for updated circulation rules (58.16 KB, patch)
2020-02-18 08:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15522: Remove old circulation rules interface (93.19 KB, patch)
2020-02-18 08:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15522: Add yarn.lock (autogenerated) (100.35 KB, patch)
2020-02-18 08:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15522: Fix permission name manage_circ_rules_from_any_libraries (2.85 KB, patch)
2020-02-18 08:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bud 15522: Restore categories filtering (1.61 KB, patch)
2020-02-18 08:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15522: Replace the obsoleted fixFloat occurrence to use Sticky (990 bytes, patch)
2020-02-18 08:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15522: Handle deprecated rules (1.45 KB, patch)
2020-02-18 08:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15522: (follow-up) Handle deprecated rules (1.43 KB, patch)
2020-02-18 08:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15522: Add API for circulation rules (23.31 KB, patch)
2020-02-18 18:42 UTC, Lisette Scheer
Details | Diff | Splinter Review
Bug 15522: Handle deprecated rules (1.50 KB, patch)
2020-02-18 18:42 UTC, Lisette Scheer
Details | Diff | Splinter Review
Bug 15522: Handle deprecated rules (1.50 KB, patch)
2020-02-18 18:43 UTC, Lisette Scheer
Details | Diff | Splinter Review
Bug 15522: Replace the obsoleted fixFloat occurrence to use Sticky (1.02 KB, patch)
2020-02-18 18:43 UTC, Lisette Scheer
Details | Diff | Splinter Review
Bud 15522: Restore categories filtering (1.67 KB, patch)
2020-02-18 18:44 UTC, Lisette Scheer
Details | Diff | Splinter Review
Bug 15522: Fix permission name manage_circ_rules_from_any_libraries (2.91 KB, patch)
2020-02-18 18:44 UTC, Lisette Scheer
Details | Diff | Splinter Review
Bug 15522: Add yarn.lock (autogenerated) (100.40 KB, patch)
2020-02-18 18:45 UTC, Lisette Scheer
Details | Diff | Splinter Review
Bug 15522: Remove old circulation rules interface (93.25 KB, patch)
2020-02-18 18:45 UTC, Lisette Scheer
Details | Diff | Splinter Review
Bug 15522: New interface for updated circulation rules (58.22 KB, patch)
2020-02-18 18:46 UTC, Lisette Scheer
Details | Diff | Splinter Review
Bug 15522: Add JS build pipeline (4.70 KB, patch)
2020-02-18 18:46 UTC, Lisette Scheer
Details | Diff | Splinter Review
Bug 15522: Add API for circulation rules (23.31 KB, patch)
2020-02-18 18:46 UTC, Lisette Scheer
Details | Diff | Splinter Review
Bug 15522: Add API for circulation rules (23.31 KB, patch)
2020-02-18 18:47 UTC, Lisette Scheer
Details | Diff | Splinter Review
Bug 15522: Add API for circulation rules (23.31 KB, patch)
2020-02-18 18:49 UTC, Lisette Scheer
Details | Diff | Splinter Review
Bug 15522: Add JS build pipeline (4.70 KB, patch)
2020-02-18 18:54 UTC, Lisette Scheer
Details | Diff | Splinter Review
Bug 15522: New interface for updated circulation rules (58.22 KB, patch)
2020-02-18 18:55 UTC, Lisette Scheer
Details | Diff | Splinter Review
Bug 15522: (follow-up) Handle deprecated rules (1.49 KB, patch)
2020-02-18 18:56 UTC, Lisette Scheer
Details | Diff | Splinter Review
Bug 15522: Add API for circulation rules (23.29 KB, patch)
2020-03-26 23:27 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 15522: Add JS build pipeline (4.70 KB, patch)
2020-03-26 23:28 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 15522: New interface for updated circulation rules (58.15 KB, patch)
2020-03-26 23:28 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 15522: Remove old circulation rules interface (92.80 KB, patch)
2020-03-26 23:29 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 15522: Add yarn.lock (autogenerated) (100.40 KB, patch)
2020-03-26 23:29 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 15522: Fix permission name manage_circ_rules_from_any_libraries (2.91 KB, patch)
2020-03-26 23:30 UTC, Lari Taskula
Details | Diff | Splinter Review
Bud 15522: Restore categories filtering (1.67 KB, patch)
2020-03-26 23:30 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 15522: Replace the obsoleted fixFloat occurrence to use Sticky (1.02 KB, patch)
2020-03-26 23:30 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 15522: Handle deprecated rules (1.50 KB, patch)
2020-03-26 23:31 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 15522: (follow-up) Handle deprecated rules (1.49 KB, patch)
2020-03-26 23:31 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 15522: Add API for circulation rules (23.29 KB, patch)
2020-03-27 03:10 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 15522: Add JS build pipeline (4.70 KB, patch)
2020-03-27 03:10 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 15522: New interface for updated circulation rules (58.15 KB, patch)
2020-03-27 03:10 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 15522: Remove old circulation rules interface (92.80 KB, patch)
2020-03-27 03:10 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 15522: Add yarn.lock (autogenerated) (100.45 KB, patch)
2020-03-27 03:10 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 15522: Fix permission name manage_circ_rules_from_any_libraries (2.91 KB, patch)
2020-03-27 03:11 UTC, Lari Taskula
Details | Diff | Splinter Review
Bud 15522: Restore categories filtering (1.72 KB, patch)
2020-03-27 03:11 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 15522: Replace the obsoleted fixFloat occurrence to use Sticky (1.08 KB, patch)
2020-03-27 03:11 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 15522: Handle deprecated rules (1.56 KB, patch)
2020-03-27 03:11 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 15522: (follow-up) Handle deprecated rules (1.54 KB, patch)
2020-03-27 03:11 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 15522: (follow-up) Fix circulation rules interface title inconsistency (2.44 KB, patch)
2020-03-27 03:11 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 15522: (follow-up) Add missing http return codes to api spec (2.39 KB, patch)
2020-03-27 03:11 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 15522: (follow-up) Fix 400 errors in circ_rules.t (1.73 KB, patch)
2020-03-27 03:11 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 15522: (follow-up) Fix 400 errors in circ_rules.t (1.73 KB, patch)
2020-03-27 03:15 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 15522: Add API for circulation rules (23.38 KB, patch)
2020-07-15 11:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 15522: Add JS build pipeline (4.70 KB, patch)
2020-07-15 11:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 15522: New interface for updated circulation rules (58.15 KB, patch)
2020-07-15 11:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 15522: Remove old circulation rules interface (94.47 KB, patch)
2020-07-15 11:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 15522: Add yarn.lock (autogenerated) (100.45 KB, patch)
2020-07-15 11:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 15522: Fix permission name manage_circ_rules_from_any_libraries (2.91 KB, patch)
2020-07-15 11:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bud 15522: Restore categories filtering (1.72 KB, patch)
2020-07-15 11:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 15522: Replace the obsoleted fixFloat occurrence to use Sticky (1.08 KB, patch)
2020-07-15 11:12 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 15522: Handle deprecated rules (1.56 KB, patch)
2020-07-15 11:12 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 15522: (follow-up) Handle deprecated rules (1.54 KB, patch)
2020-07-15 11:12 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 15522: (follow-up) Fix circulation rules interface title inconsistency (2.44 KB, patch)
2020-07-15 11:12 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 15522: (follow-up) Add missing http return codes to api spec (2.39 KB, patch)
2020-07-15 11:12 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 15522: (follow-up) Fix 400 errors in circ_rules.t (1.73 KB, patch)
2020-07-15 11:12 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 15522: (QA followup) Fix the swagger spec (1.24 KB, patch)
2020-07-15 11:12 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 15522: (Vue alternative) Remove old smart rules pages and scripts (93.68 KB, patch)
2020-10-20 17:17 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 15522: (Vue alternative) Add policy template and scripts (8.10 KB, patch)
2020-10-20 17:17 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 15522: (Vue alternative) Modify api endpoint for circ rules (29.56 KB, patch)
2020-10-20 17:17 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 15522: (Vue alternative) Set gulp script and Makefile.PL (5.04 KB, patch)
2020-10-20 17:17 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 15522: (Vue alternative) Add vue scripts (33.60 KB, patch)
2020-10-20 17:18 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 15522: (Vue alternative) Add yarn.lock (67.79 KB, patch)
2020-10-20 17:18 UTC, Agustín Moyano
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jesse Weaver 2016-01-07 21:54:18 UTC
This interface would allow editing individual rules for each branch/category/itemtype, and be reorganized to clearly show the specificity order of default rules. 

(See http://wiki.koha-community.org/wiki/Circulation_Rules_Interface_and_Backend_Revamp_RFC#Frontend for more information and a rough mockup.)
Comment 1 Jesse Weaver 2017-10-17 23:00:00 UTC Comment hidden (obsolete)
Comment 2 Jesse Weaver 2017-10-17 23:00:04 UTC Comment hidden (obsolete)
Comment 3 Jesse Weaver 2017-10-17 23:00:07 UTC Comment hidden (obsolete)
Comment 4 Jesse Weaver 2017-10-17 23:00:10 UTC Comment hidden (obsolete)
Comment 5 Jesse Weaver 2017-10-17 23:00:14 UTC Comment hidden (obsolete)
Comment 6 George Williams (NEKLS) 2018-01-09 18:08:33 UTC
Signed off based on video that Jesse put together for me.
Comment 7 Jonathan Druart 2018-01-09 19:00:28 UTC
(In reply to George Williams (NEKLS) from comment #6)
> Signed off based on video that Jesse put together for me.

Well, none of the dependencies apply cleanly, where are the patches ? :)
Comment 8 Jesse Weaver 2018-01-29 23:30:09 UTC Comment hidden (obsolete)
Comment 9 Jesse Weaver 2018-01-29 23:30:13 UTC Comment hidden (obsolete)
Comment 10 Jesse Weaver 2018-01-29 23:30:17 UTC Comment hidden (obsolete)
Comment 11 Jesse Weaver 2018-01-29 23:30:23 UTC Comment hidden (obsolete)
Comment 12 Jesse Weaver 2018-01-29 23:30:28 UTC Comment hidden (obsolete)
Comment 13 Jonathan Druart 2018-02-26 17:35:23 UTC
Waiting for depending bugs that are not signed off yet.

Jesse, can you provide a remote branch with all the patches applied?
Comment 14 Jesse Weaver 2018-02-26 19:20:57 UTC Comment hidden (obsolete)
Comment 15 Jesse Weaver 2018-02-26 19:35:39 UTC Comment hidden (obsolete)
Comment 16 Jesse Weaver 2018-02-26 19:44:12 UTC
(In reply to Jonathan Druart from comment #13)
> Waiting for depending bugs that are not signed off yet.
> 
> Jesse, can you provide a remote branch with all the patches applied?

https://github.com/pianohacker/koha , branch bz15522-withdeps .
Comment 17 Owen Leonard 2019-02-20 16:08:26 UTC
Is there still a way forward for this bug, assuming its dependencies are taken care of?
Comment 18 Jonathan Druart 2019-02-22 10:23:29 UTC
(In reply to Owen Leonard from comment #17)
> Is there still a way forward for this bug, assuming its dependencies are
> taken care of?

I am taking care of the dependencies. This patchset is to big to keep it up-to-date with all the deps.

If you want to give it a try you should use Jesse's remote branch.
Comment 19 Victor Grousset/tuxayo 2019-05-20 19:54:39 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2019-05-23 02:42:35 UTC
(In reply to Victor Grousset/tuxayo from comment #19)
> Hi :), I tried to test https://github.com/pianohacker/koha => branch
> bz15522-withdeps
> 
> As it is, before trying to rebase it on master.

Please do not do that. See the related bug reports, I am on it already. You should focus on bugs that are before this one in the dependency tree.
Comment 21 Victor Grousset/tuxayo 2019-05-24 15:42:39 UTC
Ok, I got confused by the previous message, I should have noticed that it was in "BLOCKED" and not in "Need Signoff"

So, I think I did all I could in bug 18930 and bug 18936.
I there something else doable in the dependencies of this bug?

Maybe trying to get one of my colleagues to QA Bug 18928 ?
Comment 22 Cab Vinton 2019-07-10 20:34:11 UTC
If it's not too late to add a suggestion, systems with multiple (N) branches often end up with the same rule duplicated N times, once for each library.

Ideally, this would be simplified to a single rule that applies to all branches.

Thinking a button to run a clean-up script might do the trick.
Comment 23 Jonathan Druart 2019-08-05 03:03:55 UTC Comment hidden (obsolete)
Comment 24 Jonathan Druart 2019-08-05 03:04:01 UTC Comment hidden (obsolete)
Comment 25 Jonathan Druart 2019-08-05 03:04:07 UTC Comment hidden (obsolete)
Comment 26 Jonathan Druart 2019-08-05 03:04:14 UTC Comment hidden (obsolete)
Comment 27 Jonathan Druart 2019-08-05 03:04:20 UTC Comment hidden (obsolete)
Comment 28 Jonathan Druart 2019-08-05 03:04:26 UTC Comment hidden (obsolete)
Comment 29 Jonathan Druart 2019-08-05 03:04:31 UTC Comment hidden (obsolete)
Comment 30 Jonathan Druart 2019-08-05 03:04:36 UTC Comment hidden (obsolete)
Comment 31 Jonathan Druart 2019-08-05 03:08:05 UTC
Screenshot: https://snag.gy/ujakOo.jpg
Comment 32 Jonathan Druart 2019-10-15 12:38:47 UTC Comment hidden (obsolete)
Comment 33 Jonathan Druart 2019-10-15 12:38:52 UTC Comment hidden (obsolete)
Comment 34 Jonathan Druart 2019-10-15 12:38:58 UTC Comment hidden (obsolete)
Comment 35 Jonathan Druart 2019-10-15 12:39:03 UTC Comment hidden (obsolete)
Comment 36 Jonathan Druart 2019-10-15 12:39:09 UTC Comment hidden (obsolete)
Comment 37 Jonathan Druart 2019-10-15 12:39:14 UTC Comment hidden (obsolete)
Comment 38 Jonathan Druart 2019-10-15 12:39:19 UTC Comment hidden (obsolete)
Comment 39 Jonathan Druart 2019-10-15 12:39:23 UTC Comment hidden (obsolete)
Comment 40 Michal Denar 2019-10-31 16:26:11 UTC
Hi Jonathan, can you rebase it on top of master?

Applying: Bug 15522: Add API for circulation rules
error: sha1 information is lacking or useless (Koha/CirculationRules.pm).
error: could not build fake ancestor
Patch failed at 0001 Bug 15522: Add API for circulation rules

Thank You.
Comment 41 Jonathan Druart 2019-11-26 14:00:32 UTC
Rebased patches are on the remote branch.
Comment 42 Martin Renvoize 2019-12-12 13:04:07 UTC
This all scans well, if you get a chance it would be great to have a final SO/QA by January as it's one I'm targetting for an early push.
Comment 43 Jonathan Druart 2020-01-22 14:22:43 UTC
Patches rebased.
Comment 44 Michal Denar 2020-01-22 20:18:41 UTC
It's working for me. 

Nice and useful work.

Thank You Jonathan.
Comment 45 Jonathan Druart 2020-01-23 11:05:06 UTC
(In reply to Michal Denar from comment #44)
> It's working for me. 
> 
> Nice and useful work.
> 
> Thank You Jonathan.

Hi Michal, can you explain what you tested exactly?
Did you think your tests cover bug 18936 as well?
Comment 46 Michal Denar 2020-01-23 11:20:20 UTC
Hi Jonamthan
I followed test plan above. If test bug 18936 is mandatory, I'll try to make it soon. Set this bug back as Need Signoff.
Comment 47 Jonathan Druart 2020-02-05 11:02:21 UTC
Remote branch rebased.
Comment 48 Hayley Pelham 2020-02-17 22:07:47 UTC
Hi,

Are any of the obsoleted patches here supposed to be not obsoleted? I see this Needs Signoff
Comment 49 Katrin Fischer 2020-02-17 22:45:23 UTC
Hi Hayley, I think this has a remote branch to be used instead of attached patches:
https://gitlab.com/joubu/Koha/commits/bug_15522
Comment 50 Hayley Pelham 2020-02-17 22:46:56 UTC
Ahh, that makes sense. Thanks Katrin!
Comment 51 Jonathan Druart 2020-02-18 08:20:43 UTC Comment hidden (obsolete)
Comment 52 Jonathan Druart 2020-02-18 08:20:48 UTC Comment hidden (obsolete)
Comment 53 Jonathan Druart 2020-02-18 08:20:53 UTC Comment hidden (obsolete)
Comment 54 Jonathan Druart 2020-02-18 08:20:58 UTC Comment hidden (obsolete)
Comment 55 Jonathan Druart 2020-02-18 08:21:03 UTC Comment hidden (obsolete)
Comment 56 Jonathan Druart 2020-02-18 08:21:08 UTC Comment hidden (obsolete)
Comment 57 Jonathan Druart 2020-02-18 08:21:13 UTC Comment hidden (obsolete)
Comment 58 Jonathan Druart 2020-02-18 08:21:17 UTC Comment hidden (obsolete)
Comment 59 Jonathan Druart 2020-02-18 08:21:22 UTC Comment hidden (obsolete)
Comment 60 Jonathan Druart 2020-02-18 08:21:27 UTC Comment hidden (obsolete)
Comment 61 Jonathan Druart 2020-02-18 08:22:07 UTC
Patches are now attached here! :)
Comment 62 Lisette Scheer 2020-02-18 18:38:53 UTC
(In reply to Jonathan Druart from comment #51)
> Created attachment 99148 [details] [review] [review]
> Bug 15522: Add API for circulation rules
> 
> Test plan:
>   1) Run t/db_dependent/api/v1/circ_rules.t .

kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove t/db_dependent/api/v1/circ_rules.t
t/db_dependent/api/v1/circ_rules.t .. 1/5
    #   Failed test '403 Forbidden'
    #   at t/db_dependent/api/v1/circ_rules.t line 128.
    #          got: '400'
    #     expected: '403'
    # Looks like you failed 1 test of 2.
t/db_dependent/api/v1/circ_rules.t .. 3/5
#   Failed test 'set_rules | unauthorized'
#   at t/db_dependent/api/v1/circ_rules.t line 132.

    #   Failed test '200 OK'
    #   at t/db_dependent/api/v1/circ_rules.t line 212.
    #          got: '403'
    #     expected: '200'
    # Looks like you failed 1 test of 6.

#   Failed test 'set_rules | restricted'
#   at t/db_dependent/api/v1/circ_rules.t line 223.
# Looks like you failed 2 tests of 5.
t/db_dependent/api/v1/circ_rules.t .. Dubious, test returned 2 (wstat 512, 0x200)
Failed 2/5 subtests

Test Summary Report
-------------------
t/db_dependent/api/v1/circ_rules.t (Wstat: 512 Tests: 5 Failed: 2)
  Failed tests:  3, 5
  Non-zero exit status: 2
Files=1, Tests=5,  4 wallclock secs ( 0.02 usr  0.00 sys +  2.86 cusr  0.23 csys =  3.11 CPU)
Result: FAIL

I got this message when I tried to do this step in my devbox. Everything else worked great for me.
Comment 63 Lisette Scheer 2020-02-18 18:42:04 UTC Comment hidden (obsolete)
Comment 64 Lisette Scheer 2020-02-18 18:42:44 UTC Comment hidden (obsolete)
Comment 65 Lisette Scheer 2020-02-18 18:43:03 UTC Comment hidden (obsolete)
Comment 66 Lisette Scheer 2020-02-18 18:43:36 UTC Comment hidden (obsolete)
Comment 67 Lisette Scheer 2020-02-18 18:44:08 UTC Comment hidden (obsolete)
Comment 68 Lisette Scheer 2020-02-18 18:44:45 UTC Comment hidden (obsolete)
Comment 69 Lisette Scheer 2020-02-18 18:45:15 UTC Comment hidden (obsolete)
Comment 70 Lisette Scheer 2020-02-18 18:45:47 UTC Comment hidden (obsolete)
Comment 71 Lisette Scheer 2020-02-18 18:46:14 UTC Comment hidden (obsolete)
Comment 72 Lisette Scheer 2020-02-18 18:46:33 UTC Comment hidden (obsolete)
Comment 73 Lisette Scheer 2020-02-18 18:46:59 UTC Comment hidden (obsolete)
Comment 74 Lisette Scheer 2020-02-18 18:47:48 UTC Comment hidden (obsolete)
Comment 75 Lisette Scheer 2020-02-18 18:49:04 UTC Comment hidden (obsolete)
Comment 76 Lisette Scheer 2020-02-18 18:54:59 UTC Comment hidden (obsolete)
Comment 77 Lisette Scheer 2020-02-18 18:55:14 UTC Comment hidden (obsolete)
Comment 78 Lisette Scheer 2020-02-18 18:56:05 UTC Comment hidden (obsolete)
Comment 79 Nick Clemens 2020-03-13 17:48:27 UTC
We are reviewing this one internally and plan to demo it to the sponsoring library to get some feedback before this continues
Comment 80 Arthur Suzuki 2020-03-26 10:26:02 UTC
Hi Nick,
I've been testing the new interface, there are few things that I really like:

- filters (circulation, fines, holds) -> very useful for ease of reading when lots of rules or when troubleshooting hold rules issues for example.

- remove duplicate rules -> got to spend a bit more time to try that one, does it have a warning showing the duplicates before removal? that would be great if there's not :)

- overall presentation is really nice.

Arthur
Comment 81 Arthur Suzuki 2020-03-26 10:42:56 UTC
Just seen the disclaimer :
"This should not change any circulation behavior, but you will be able to review these changes before saving."

Nice :)
Comment 82 Arthur Suzuki 2020-03-26 10:59:25 UTC
Tried the "Cleanup rules" button this way :

Set rule by category (defaults) to 5, 5, 5
Created a new one for "School" with same values.

click on "cleanup rules" -> get the disclaimer, click ok.

after ok, the rule for school still appears with yellowish colors (enabled for editing I guess?)

if I reload the page, it disappear from the view -> cleanup works.

If I don't reload the page but click on "save" instead, then the rule still appears, until a full page reload. -> this might lead people to thinks the rules has not been removed. (solved after a reload).

Overall, the stuff works but the process of reviewing the rules before actual deletion is not so clear.

Maybe any click on the "save button" could trigger a full page reload to ensure working with "fresh" data?

Happy to sign-off if this is the expected behavior.
Comment 83 Arthur Suzuki 2020-03-26 11:13:25 UTC
Also just tried the "Rules per categories and itemtype" section.
When there are a lot of rules it's still tough to read which field one is editing.
Would be great to reproduce something like this : http://output.jsbin.com/AjiJIGA/1

Nice to have one edit button on each field though.

The interrogation mark helps to find out from which rules the value is inherited, would be useful to have as well the field name in there.
Comment 84 Arthur Suzuki 2020-03-26 11:17:45 UTC
Also just tried the "Rules per categories and itemtype" section.
When there are a lot of rules it's still tough to read which field one is editing.
Would be great to reproduce something like this : http://output.jsbin.com/AjiJIGA/1

Nice to have one edit button on each field though.

The interrogation mark helps to find out from which rules the value is inherited, would be useful to have as well the field name in there.
Comment 85 Lari Taskula 2020-03-26 22:58:42 UTC
Not applying. Patches have gone in wrong order. I'll attempt to fix it.
Comment 86 Lari Taskula 2020-03-26 23:27:56 UTC Comment hidden (obsolete)
Comment 87 Lari Taskula 2020-03-26 23:28:21 UTC Comment hidden (obsolete)
Comment 88 Lari Taskula 2020-03-26 23:28:51 UTC Comment hidden (obsolete)
Comment 89 Lari Taskula 2020-03-26 23:29:21 UTC Comment hidden (obsolete)
Comment 90 Lari Taskula 2020-03-26 23:29:43 UTC Comment hidden (obsolete)
Comment 91 Lari Taskula 2020-03-26 23:30:04 UTC Comment hidden (obsolete)
Comment 92 Lari Taskula 2020-03-26 23:30:25 UTC Comment hidden (obsolete)
Comment 93 Lari Taskula 2020-03-26 23:30:45 UTC Comment hidden (obsolete)
Comment 94 Lari Taskula 2020-03-26 23:31:07 UTC Comment hidden (obsolete)
Comment 95 Lari Taskula 2020-03-26 23:31:27 UTC Comment hidden (obsolete)
Comment 96 Lari Taskula 2020-03-26 23:32:30 UTC
Fixed patch order, obsoleted duplicates, rebased on master.
Comment 97 Lari Taskula 2020-03-27 01:24:07 UTC
(In reply to Lari Taskula from comment #87)
> Created attachment 101919 [details] [review] [review]
> Bug 15522: Add JS build pipeline
> 
> This is not directly testable, but can be either used through the
> Makefile:
> 
> $ make js-build
> 
> (This will automatically install all dependencies.)
> 
> Or by manually installing yarn:
> 
> $ sudo apt install nodejs npm
> $ sudo npm install -g yarn
> $ yarn install
> $ yarn build
> 
> (There is also `make js-watch` and `yarn watch`, which automatically
> recompile when files are changed.)
> 
> JD note: the make rule does not work
> 
> Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

I had issues with this on Ubuntu 18.04, especially the step "yarn build".

koha@koha:~/Koha$ yarn build
yarn run v1.22.4
$ node_modules/.bin/gulp build
You are missing required Node modules; run `npm install`.

I did some digging and found a more detailed message:
ReferenceError: primordials is not defined

It worked when I explicitly chose to install node version 10.
Comment 98 Lari Taskula 2020-03-27 01:25:22 UTC Comment hidden (obsolete)
Comment 99 Lari Taskula 2020-03-27 01:30:08 UTC Comment hidden (obsolete)
Comment 100 Lari Taskula 2020-03-27 02:02:33 UTC
(In reply to Lari Taskula from comment #98)
> (In reply to Lisette Scheer from comment #62)
> > (In reply to Jonathan Druart from comment #51)
> > > Created attachment 99148 [details] [review] [review] [review] [review]
> > > Bug 15522: Add API for circulation rules
> > > 
> > > Test plan:
> > >   1) Run t/db_dependent/api/v1/circ_rules.t .
> > 
> > kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove
> > t/db_dependent/api/v1/circ_rules.t
> > t/db_dependent/api/v1/circ_rules.t .. 1/5
> >     #   Failed test '403 Forbidden'
> >     #   at t/db_dependent/api/v1/circ_rules.t line 128.
> >     #          got: '400'
> >     #     expected: '403'
> >     # Looks like you failed 1 test of 2.
> > t/db_dependent/api/v1/circ_rules.t .. 3/5
> > #   Failed test 'set_rules | unauthorized'
> > #   at t/db_dependent/api/v1/circ_rules.t line 132.
> > 
> >     #   Failed test '200 OK'
> >     #   at t/db_dependent/api/v1/circ_rules.t line 212.
> >     #          got: '403'
> >     #     expected: '200'
> >     # Looks like you failed 1 test of 6.
> > 
> > #   Failed test 'set_rules | restricted'
> > #   at t/db_dependent/api/v1/circ_rules.t line 223.
> > # Looks like you failed 2 tests of 5.
> > t/db_dependent/api/v1/circ_rules.t .. Dubious, test returned 2 (wstat 512,
> > 0x200)
> > Failed 2/5 subtests
> > 
> > Test Summary Report
> > -------------------
> > t/db_dependent/api/v1/circ_rules.t (Wstat: 512 Tests: 5 Failed: 2)
> >   Failed tests:  3, 5
> >   Non-zero exit status: 2
> > Files=1, Tests=5,  4 wallclock secs ( 0.02 usr  0.00 sys +  2.86 cusr  0.23
> > csys =  3.11 CPU)
> > Result: FAIL
> > 
> > I got this message when I tried to do this step in my devbox. Everything
> > else worked great for me.
> 
> I confirm this error. Will provide a patch to fix it.

Already fixed by "Bug 15522: Fix permission name manage_circ_rules_from_any_libraries", maybe squash this patch to the first one.
Comment 101 Lari Taskula 2020-03-27 02:11:30 UTC
(In reply to Lari Taskula from comment #99)
> Comment on attachment 101920 [details] [review] [review]
> Bug 15522: New interface for updated circulation rules
> 
> Review of attachment 101920 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: koha-tmpl/intranet-tmpl/prog/js/src/admin/policy/app.js
> @@ +75,5 @@
> > +        </div>;
> > +    }
> > +
> > +    componentDidMount() {
> > +        $( this.toolbarElem ).fixFloat();
> 
> Test plan step 3 fails, my browser (both FF and Chromium) console says:
> TypeError: $(...).fixFloat is not a functionapp.js:79:30

Fixed by "Bug 15522: Replace the obsoleted fixFloat occurrence to use Sticky", would be useful to squash this patch
Comment 102 Lari Taskula 2020-03-27 03:10:13 UTC
Created attachment 101934 [details] [review]
Bug 15522: Add API for circulation rules

Test plan:
  1) Run t/db_dependent/api/v1/circ_rules.t .

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Comment 103 Lari Taskula 2020-03-27 03:10:21 UTC
Created attachment 101935 [details] [review]
Bug 15522: Add JS build pipeline

This is not directly testable, but can be either used through the
Makefile:

$ make js-build

(This will automatically install all dependencies.)

Or by manually installing yarn:

$ sudo apt install nodejs npm
$ sudo npm install -g yarn
$ yarn install
$ yarn build

(There is also `make js-watch` and `yarn watch`, which automatically
recompile when files are changed.)

JD note: the make rule does not work

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Comment 104 Lari Taskula 2020-03-27 03:10:30 UTC
Created attachment 101936 [details] [review]
Bug 15522: New interface for updated circulation rules

Test plan:
  1) Make sure all dependencies are applied, and that their DB upgrades
     have been applied.
  2) Build JS (see previous patches for HOWTO).
  3) Load .../cgi-bin/koha/admin/policy.pl .
  4) Try changing an existing rule (one with a textbox or dropdown). Hit
     "Save rules", ensure that your changes are saved.
  5) Until changes are saved, all cells that are changed or affected by
     your changes should be highlighted in yellow.
  6) Same as above, but deleting a rule by hitting the "x" next to it.
  7) Add rows and columns using the "Choose rule..." and "Choose
     category/item type..." dropdowns.
  8) Add rules by clicking the pencil in a currently unset rule cell.
     Ensure changes are saved.
  9) Hit "Cleanup rules"; all redundant rules should be removed. As this
     only removes rules that are the same as the defaults, this should
     not change any effective rules.
 10) Use the filters at the top right ("Circulation", "Holds" and
     "Fines") to filter the kinds of rules that are shown.
 11) Ensure that any desired library can be shown, including (if
     selected) all at the same time.

JD edits: Removed the changes to filtered categories, will be done in a
follow-up

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Comment 105 Lari Taskula 2020-03-27 03:10:40 UTC
Created attachment 101937 [details] [review]
Bug 15522: Remove old circulation rules interface

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Comment 106 Lari Taskula 2020-03-27 03:10:51 UTC
Created attachment 101938 [details] [review]
Bug 15522: Add yarn.lock (autogenerated)

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Signed-off-by: Lari Taskula <lari.taskula@hypernova.fi>
Comment 107 Lari Taskula 2020-03-27 03:11:00 UTC
Created attachment 101939 [details] [review]
Bug 15522: Fix permission name manage_circ_rules_from_any_libraries

On 15520 followups have been provided to not use a negative permission.

Note that two api tests are failing (get 400). I would like some help to
investigate here why 400 is returned instead of 403.

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Comment 108 Lari Taskula 2020-03-27 03:11:09 UTC
Created attachment 101940 [details] [review]
Bud 15522: Restore categories filtering

Mimicking what is done for Branches

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Signed-off-by: Lari Taskula <lari.taskula@hypernova.fi>
Comment 109 Lari Taskula 2020-03-27 03:11:17 UTC
Created attachment 101941 [details] [review]
Bug 15522: Replace the obsoleted fixFloat occurrence to use Sticky

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Signed-off-by: Lari Taskula <lari.taskula@hypernova.fi>
Comment 110 Lari Taskula 2020-03-27 03:11:26 UTC
Created attachment 101942 [details] [review]
Bug 15522: Handle deprecated rules

For upgraded installs. Also add a log in the console if it happens.

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Signed-off-by: Lari Taskula <lari.taskula@hypernova.fi>
Comment 111 Lari Taskula 2020-03-27 03:11:34 UTC
Created attachment 101943 [details] [review]
Bug 15522: (follow-up) Handle deprecated rules

Previous use of inArray was wrong.

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Signed-off-by: Lari Taskula <lari.taskula@hypernova.fi>
Comment 112 Lari Taskula 2020-03-27 03:11:42 UTC
Created attachment 101944 [details] [review]
Bug 15522: (follow-up) Fix circulation rules interface title inconsistency

Follow-up to "Bug 15522: Remove old circulation rules interface"

Instead of:
"Circulation, fines and holds rules", use
"Circulation, fines, and holds rules" for consistency

Latter form already used in commit Bug 15522: New interface for updated
circulation rules as follows

koha-tmpl/intranet-tmpl/prog/en/includes/admin-menu.inc:37:
<li><a href="/cgi-bin/koha/admin/policy.pl">Circulation, fines, and holds rules</a></li>
koha-tmpl/intranet-tmpl/prog/en/modules/admin/admin-home.tt:88:
<dt><a href="/cgi-bin/koha/admin/policy.pl">Circulation, fines, and holds rules</a></dt>
Comment 113 Lari Taskula 2020-03-27 03:11:50 UTC
Created attachment 101945 [details] [review]
Bug 15522: (follow-up) Add missing http return codes to api spec
Comment 114 Lari Taskula 2020-03-27 03:11:59 UTC Comment hidden (obsolete)
Comment 115 Lari Taskula 2020-03-27 03:15:49 UTC
Created attachment 101947 [details] [review]
Bug 15522: (follow-up) Fix 400 errors in circ_rules.t

Commit "Bug 15522: Fix permission name manage_circ_rules_from_any_libraries"
renames a permission by one that negates the previous.

However the commit above incorrectly negates more than is supposed to.

The test wants to check whether unauthorized and restricted user can store
circulation rules.

The libriarian is restricted (to their homebranch) when they are circ rules
authorized (by "manage_circ_rules" permission) but do not have permission
"manage_circ_rules_from_any_libraries".

To test:
1. prove t/db_dependent/api/v1/circ_rules.t
Comment 116 Lari Taskula 2020-03-27 03:39:39 UTC
Comment on attachment 101934 [details] [review]
Bug 15522: Add API for circulation rules

Review of attachment 101934 [details] [review]:
-----------------------------------------------------------------

::: api/v1/swagger/paths/circulation-rules.json
@@ +28,5 @@
> +          "schema": {
> +            "$ref": "../definitions.json#/error"
> +          }
> +        }
> +      }

No permissions required for GET?

@@ +40,5 @@
> +      ],
> +      "parameters": [{
> +        "name": "body",
> +        "in": "body",
> +        "description": "A JSON object containing rules to set; null for a rule_value means to delete the rule",

I think this wrong functionality for POST method that is supposed to create resources.

Also, by definition POST is not idempotent, but here it seems to be. (If I understand the API correctly, here making the same request twice is the same as making it only once, right?)

It should then be PUT. But PUT requires the whole object as a parameter.
Comment 117 Jonathan Druart 2020-03-27 06:49:46 UTC
Lari, the remote branch needs to be used to testing. The patches were not up-to-date on purpose, new rules have been added to the table, and need to be added to the interface, did you deal with them?
Comment 118 Martin Renvoize 2020-04-06 13:28:58 UTC
Removed from 20.05 targets as it appears to still need a little work after usability testing.. We've got lots of the dependencies pushed, so hopefully this can get some attention and make it in time for 20.11.
Comment 119 Nick Clemens 2020-06-03 14:53:38 UTC
Sponsoring library has approved, removing discussion status
Comment 120 Jonathan Druart 2020-06-03 15:11:10 UTC
(In reply to Nick Clemens from comment #119)
> Sponsoring library has approved, removing discussion status

Could you share a summary of what has been tested and discussed?
Comment 121 Katrin Fischer 2020-06-21 16:44:22 UTC
(In reply to Jonathan Druart from comment #120)
> (In reply to Nick Clemens from comment #119)
> > Sponsoring library has approved, removing discussion status
> 
> Could you share a summary of what has been tested and discussed?

+1

For further testing - git branch or attached patches?
Comment 122 Jessie Zairo 2020-06-23 15:39:07 UTC
I spent some time exploring the new layout, here is a summary of my notes. 

It is nice that if you don’t need to use a column in a certain table, you can set the default to null, save, and the column will be removed. This then creates a dropdown at the end that will allow all the users to add the column back in. Really makes for a clean interface if you have unused columns. 

It is great that the header floats, staff can easily save, cleanup rules, or apply filters wherever they are on the policy page. 

For large libraries with multiple branches or consortiums, the initial page, cgi-bin/koha/admin/policy.pl, is slow to load or reflect changes. 

It would be nice to have the left column and the header float when you are editing the rules. Currently, there is no way to see what rule/column you are editing (without scrolling back and forth). 

For the rules by category, when creating a new rule, it inserts the new rule alphabetically. For longer lists, you have to go back to find it to make any edits. 

Overall, this is a fantastic improvement to the matrix!
Comment 123 Christopher Brannon 2020-06-23 16:31:15 UTC
Could columns be accordioned like rows?  Maybe accordion each column, or groups according to topic?  Then maybe a button/link that shows all or return to accordion?  Sometimes we like to scan the page, and having all show as we scroll is good, but we don't want to scroll all the time.
Comment 124 Martin Renvoize 2020-07-15 08:42:15 UTC
If I try to test via the linked branch all I get is 'Loading, please wait...'.. am I doing something wrong?
Comment 125 Martin Renvoize 2020-07-15 09:08:06 UTC
Same result if I apply the patches here.. it's like the JS just doesn't load at all.. Not seeing any errors in the console and not failures in the networking tab.
Comment 126 Martin Renvoize 2020-07-15 11:11:19 UTC
Created attachment 106913 [details] [review]
Bug 15522: Add API for circulation rules

Test plan:
  1) Run t/db_dependent/api/v1/circ_rules.t .

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Comment 127 Martin Renvoize 2020-07-15 11:11:25 UTC
Created attachment 106914 [details] [review]
Bug 15522: Add JS build pipeline

This is not directly testable, but can be either used through the
Makefile:

$ make js-build

(This will automatically install all dependencies.)

Or by manually installing yarn:

$ sudo apt install nodejs npm
$ sudo npm install -g yarn
$ yarn install
$ yarn build

(There is also `make js-watch` and `yarn watch`, which automatically
recompile when files are changed.)

JD note: the make rule does not work

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Comment 128 Martin Renvoize 2020-07-15 11:11:31 UTC
Created attachment 106915 [details] [review]
Bug 15522: New interface for updated circulation rules

Test plan:
  1) Make sure all dependencies are applied, and that their DB upgrades
     have been applied.
  2) Build JS (see previous patches for HOWTO).
  3) Load .../cgi-bin/koha/admin/policy.pl .
  4) Try changing an existing rule (one with a textbox or dropdown). Hit
     "Save rules", ensure that your changes are saved.
  5) Until changes are saved, all cells that are changed or affected by
     your changes should be highlighted in yellow.
  6) Same as above, but deleting a rule by hitting the "x" next to it.
  7) Add rows and columns using the "Choose rule..." and "Choose
     category/item type..." dropdowns.
  8) Add rules by clicking the pencil in a currently unset rule cell.
     Ensure changes are saved.
  9) Hit "Cleanup rules"; all redundant rules should be removed. As this
     only removes rules that are the same as the defaults, this should
     not change any effective rules.
 10) Use the filters at the top right ("Circulation", "Holds" and
     "Fines") to filter the kinds of rules that are shown.
 11) Ensure that any desired library can be shown, including (if
     selected) all at the same time.

JD edits: Removed the changes to filtered categories, will be done in a
follow-up

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Comment 129 Martin Renvoize 2020-07-15 11:11:38 UTC
Created attachment 106916 [details] [review]
Bug 15522: Remove old circulation rules interface

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Comment 130 Martin Renvoize 2020-07-15 11:11:44 UTC
Created attachment 106917 [details] [review]
Bug 15522: Add yarn.lock (autogenerated)

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Signed-off-by: Lari Taskula <lari.taskula@hypernova.fi>
Comment 131 Martin Renvoize 2020-07-15 11:11:50 UTC
Created attachment 106918 [details] [review]
Bug 15522: Fix permission name manage_circ_rules_from_any_libraries

On 15520 followups have been provided to not use a negative permission.

Note that two api tests are failing (get 400). I would like some help to
investigate here why 400 is returned instead of 403.

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Comment 132 Martin Renvoize 2020-07-15 11:11:55 UTC
Created attachment 106919 [details] [review]
Bud 15522: Restore categories filtering

Mimicking what is done for Branches

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Signed-off-by: Lari Taskula <lari.taskula@hypernova.fi>
Comment 133 Martin Renvoize 2020-07-15 11:12:01 UTC
Created attachment 106920 [details] [review]
Bug 15522: Replace the obsoleted fixFloat occurrence to use Sticky

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Signed-off-by: Lari Taskula <lari.taskula@hypernova.fi>
Comment 134 Martin Renvoize 2020-07-15 11:12:07 UTC
Created attachment 106921 [details] [review]
Bug 15522: Handle deprecated rules

For upgraded installs. Also add a log in the console if it happens.

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Signed-off-by: Lari Taskula <lari.taskula@hypernova.fi>
Comment 135 Martin Renvoize 2020-07-15 11:12:13 UTC
Created attachment 106922 [details] [review]
Bug 15522: (follow-up) Handle deprecated rules

Previous use of inArray was wrong.

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Signed-off-by: Lari Taskula <lari.taskula@hypernova.fi>
Comment 136 Martin Renvoize 2020-07-15 11:12:19 UTC
Created attachment 106923 [details] [review]
Bug 15522: (follow-up) Fix circulation rules interface title inconsistency

Follow-up to "Bug 15522: Remove old circulation rules interface"

Instead of:
"Circulation, fines and holds rules", use
"Circulation, fines, and holds rules" for consistency

Latter form already used in commit Bug 15522: New interface for updated
circulation rules as follows

koha-tmpl/intranet-tmpl/prog/en/includes/admin-menu.inc:37:
<li><a href="/cgi-bin/koha/admin/policy.pl">Circulation, fines, and holds rules</a></li>
koha-tmpl/intranet-tmpl/prog/en/modules/admin/admin-home.tt:88:
<dt><a href="/cgi-bin/koha/admin/policy.pl">Circulation, fines, and holds rules</a></dt>
Comment 137 Martin Renvoize 2020-07-15 11:12:25 UTC
Created attachment 106924 [details] [review]
Bug 15522: (follow-up) Add missing http return codes to api spec
Comment 138 Martin Renvoize 2020-07-15 11:12:31 UTC
Created attachment 106925 [details] [review]
Bug 15522: (follow-up) Fix 400 errors in circ_rules.t

Commit "Bug 15522: Fix permission name manage_circ_rules_from_any_libraries"
renames a permission by one that negates the previous.

However the commit above incorrectly negates more than is supposed to.

The test wants to check whether unauthorized and restricted user can store
circulation rules.

The libriarian is restricted (to their homebranch) when they are circ rules
authorized (by "manage_circ_rules" permission) but do not have permission
"manage_circ_rules_from_any_libraries".

To test:
1. prove t/db_dependent/api/v1/circ_rules.t
Comment 139 Martin Renvoize 2020-07-15 11:12:37 UTC
Created attachment 106926 [details] [review]
Bug 15522: (QA followup) Fix the swagger spec
Comment 140 Martin Renvoize 2020-07-15 11:23:15 UTC
Rebased and swagger spec fixed so it can all be applied easily.

Jury is out on whether it's an improvement in my opinion.. but I'm not a librarian  working with it.
Comment 141 Martin Renvoize 2020-07-15 11:33:33 UTC
Actually.. it's growing on me.. but I think  a few of the grouping terms could be improved to clarify the views.
Comment 142 Katrin Fischer 2020-07-30 22:10:47 UTC
I tried with the remote branch (Got the Loading...) and I tried the patches here - do not apply. 

Which is the preferred for testing at the moment? And can I please get a rebase or fix? :)
Comment 143 Jonathan Druart 2020-07-31 07:56:14 UTC
Can you confirm you tried the yarn commands?
Comment 144 Martin Renvoize 2020-08-11 08:24:06 UTC
I can confirm the branch does work as expected (when you remember to run the yarn commands 'yarn install && yarn build')

We've had this branch checked out for a few days with PTFS-E staff testing, and even had a feedback session with ByWater staff.

Written feedback here to follow.
Comment 145 Julian Maurice 2020-08-28 10:55:21 UTC
This needs to be rebased (conflict on gulpfile.js - see bug 26228)
Comment 146 Martin Renvoize 2020-10-07 14:21:49 UTC
(In reply to Martin Renvoize from comment #144)
> I can confirm the branch does work as expected (when you remember to run the
> yarn commands 'yarn install && yarn build')
> 
> We've had this branch checked out for a few days with PTFS-E staff testing,
> and even had a feedback session with ByWater staff.
> 
> Written feedback here to follow.

OK.. so after conversing about this.. the tl:dr feedback was "We like it"
Comment 147 Agustín Moyano 2020-10-20 17:17:13 UTC
Created attachment 112056 [details] [review]
Bug 15522: (Vue alternative) Remove old smart rules pages and scripts
Comment 148 Agustín Moyano 2020-10-20 17:17:26 UTC
Created attachment 112057 [details] [review]
Bug 15522: (Vue alternative) Add policy template and scripts
Comment 149 Agustín Moyano 2020-10-20 17:17:35 UTC
Created attachment 112058 [details] [review]
Bug 15522: (Vue alternative) Modify api endpoint for circ rules
Comment 150 Agustín Moyano 2020-10-20 17:17:48 UTC
Created attachment 112059 [details] [review]
Bug 15522: (Vue alternative) Set gulp script and Makefile.PL
Comment 151 Agustín Moyano 2020-10-20 17:18:00 UTC
Created attachment 112060 [details] [review]
Bug 15522: (Vue alternative) Add vue scripts

This patch adds vue scripts for circulation rules

To test:
1. Apply all patches
2. restart_all
3. yarn install
4. yarn run build
5. go to circulation rules in staff interface

You'll notice the working area is divided in 3 parts..

On the left you've got a list area. That list area has a combo where you can choose
between libraries, patron categories and item types. What you choose
there are the rules you'll be watching and editing.

The middle area is where rules are shown. That area is divided by circulation rules, fines
rules and hold rules, and each of that sections are divided by rules
specific for libraries, libraries and categories, libraries and
item types or libraries, categories and item types. To edit a rule click
on the pencil icon in the section title. If you're editing default
values (Values for all libraries, categories or item types) the
corresponding edit control (input, select, etc) is shown. If you are
editing for a particular library, category or item type, above the
editing control you get a checkbox to use default value or to overwrite
that rule.

In the area to the right you've got a label with the currently selected
library, category or item type. If when selecting items in the list area
you do ctrl+click (cmd+click on mac) you can select multiple elements
from the list. All selected elements will appear in the right area.
Notice that only one of them is still selected. You can rapidly change
from one selected item to the other by clicking on those labels
Comment 152 Agustín Moyano 2020-10-20 17:18:11 UTC
Created attachment 112061 [details] [review]
Bug 15522: (Vue alternative) Add yarn.lock
Comment 153 Agustín Moyano 2020-10-20 17:22:42 UTC
I've just added Vue alternative for circulation rules.

I really, really, really don't like the uncomfortable huge rules table, so I took the liberty to propose a new design.

Hope you like it and have fun.
Comment 154 Agustín Moyano 2020-10-20 17:27:48 UTC
screenshot: https://snipboard.io/jIwD0s.jpg
Comment 155 Katrin Fischer 2020-10-20 18:51:37 UTC
The vue patches apply nicely, but the the other POC patches have too many conflicts. Would love to be able to compare a bit on the different design approaches.
Comment 156 Jonathan Druart 2020-11-02 08:08:34 UTC
(In reply to Katrin Fischer from comment #155)
> The vue patches apply nicely, but the the other POC patches have too many
> conflicts. Would love to be able to compare a bit on the different design
> approaches.

Use the remote branch at https://gitlab.com/joubu/Koha/commits/bug_15522
Not up-to-date but you will be able to compare the 2 approaches.
Comment 157 Katrin Fischer 2020-11-02 09:57:03 UTC
(In reply to Jonathan Druart from comment #156)
> (In reply to Katrin Fischer from comment #155)
> > The vue patches apply nicely, but the the other POC patches have too many
> > conflicts. Would love to be able to compare a bit on the different design
> > approaches.
> 
> Use the remote branch at https://gitlab.com/joubu/Koha/commits/bug_15522
> Not up-to-date but you will be able to compare the 2 approaches.

Augustin made a branch for me after I posted this and I had a look - I think both have advantages so it's really not an easy one.

What I miss is an overview kind of page like we have now - not to use it for editing, but to easily see what applies and to be able to export/print when needed even. Not sure if that is something that has been considered as a possible part of the developments.

I think Augustin's does a little nicer job in showing which rule applies easily, but you have to make the necessary selections first. The first one I like better for a better/faster editing/adding experience.

It's hard.
Comment 158 Kyle M Hall 2020-11-02 13:19:35 UTC
> I think Augustin's does a little nicer job in showing which rule applies
> easily, but you have to make the necessary selections first. The first one I
> like better for a better/faster editing/adding experience.
> 
> It's hard.

I think there is one *big* difference that weighs heavily in my mind. Jesse Weaver is no longer an active member in the Koha community, whereas Augustin is. Augustin will be able to assist other developers in learning how to make changes to the rules editor. If we go with Piano's version, we'll be left to explore and understand it on our own.
Comment 159 Katrin Fischer 2020-11-02 13:23:38 UTC
(In reply to Kyle M Hall from comment #158)
> > I think Augustin's does a little nicer job in showing which rule applies
> > easily, but you have to make the necessary selections first. The first one I
> > like better for a better/faster editing/adding experience.
> > 
> > It's hard.
> 
> I think there is one *big* difference that weighs heavily in my mind. Jesse
> Weaver is no longer an active member in the Koha community, whereas Augustin
> is. Augustin will be able to assist other developers in learning how to make
> changes to the rules editor. If we go with Piano's version, we'll be left to
> explore and understand it on our own.

Hi Kyle, I believe we might want to have the discussion about Angular / Vue and the functionality/GUI separately because both are different aspects. At the moment Augustin's patch is a completely new way of doing this both technically and in how the GUI is done. It's not a reimplementation of Jesse's work.

We might come to the conclusion that we want something similar to Jesse's but written in Vue.
Comment 160 Kyle M Hall 2020-11-02 14:02:47 UTC
Of course. My honest opinion is that React vs Vue is not really an issue. I believe that we as developers are perfectly capable of using either one. On the other hand, a discussion of licensing and other social issue has merit.

(In reply to Katrin Fischer from comment #159)
> Hi Kyle, I believe we might want to have the discussion about Angular / Vue
> and the functionality/GUI separately because both are different aspects. At
> the moment Augustin's patch is a completely new way of doing this both
> technically and in how the GUI is done. It's not a reimplementation of
> Jesse's work.
> 
> We might come to the conclusion that we want something similar to Jesse's
> but written in Vue.
Comment 161 Katrin Fischer 2020-11-02 14:09:54 UTC
(In reply to Kyle M Hall from comment #160)
> Of course. My honest opinion is that React vs Vue is not really an issue. I
> believe that we as developers are perfectly capable of using either one. On
> the other hand, a discussion of licensing and other social issue has merit.

Glad to hear - I don't have any experience or opinion on either, but hope that we can figure out what is best long term.
Comment 162 Agustín Moyano 2020-11-02 15:11:48 UTC
(In reply to Katrin Fischer from comment #161)
> (In reply to Kyle M Hall from comment #160)
> > Of course. My honest opinion is that React vs Vue is not really an issue. I
> > believe that we as developers are perfectly capable of using either one. On
> > the other hand, a discussion of licensing and other social issue has merit.
> 
> Glad to hear - I don't have any experience or opinion on either, but hope
> that we can figure out what is best long term.

I think we've got 2 discussions.. one is which technology fits better our needs (React vs Vue) and the other is how do we want our circulation rules revamped.

What I don't like of current circulation rules is that it's hard (at least for me) to edit scrolling horizontally, plus if there are a lot of rules it's hard to read.

I feel that with the React proposal (the model, not the technology) could potentially happen the same.. if you edit different rules of a category/item type, you may need to scroll horizontally too.
Comment 163 Katrin Fischer 2020-11-11 21:22:28 UTC
I think there are different use cases for what you check the editor for:

1) Editing a single rule
2) Editing multiple rules: It's common that libraries have a lot (like over a 100) of rows in the current table. And if you have to change a lot of them to extend renewals because of COVID, you want something comfortable. And once you are done:
3) Checking your entries are for a set of rules (show me rules x y z for a certain category, all for a certain library... all for a certain itemtype) correct. For this use case, you will want to see "more"
4) Look up all rules applying to a certain case (like a checked out item where you wonder about how the fines are set)


I think Agustin's works well for 1) and 4). 
But I think the editable table styl works better for 2) and 3). It's quicker and more comfortable. I think usually you will go in to edit more than one rule.

I think maybe we could refine the suggested grouping of rules a bit more:

- checkout/loan period related
- renewal related
- fine related
- holds related
- article requests related

And reduce the need to scroll horizontally this way.
Comment 164 Victor Grousset/tuxayo 2021-06-16 21:38:36 UTC
IIUC, this needs a rebase and the continuation of comparing the two alternatives. 
No other blocker than playing with both and getting feedback right? (which is already quite some work ^^)

There isn't yet one that has been chosen right?