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.)
Created attachment 68231 [details] [review] Bug 15522: Add API for circulation rules Test plan: 1) Run t/db_dependent/api/v1/circ_rules.t .
Created attachment 68232 [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.)
Created attachment 68233 [details] [review] Bug 15522: Add production JS build mode To use, run yarn build (or make js-build) with PRODUCTION=1.
Created attachment 68234 [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.
Created attachment 68235 [details] [review] Bug 15522: Remove old circulation rules interface
Signed off based on video that Jesse put together for me.
(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 ? :)
Created attachment 71048 [details] [review] Bug 15522: Add API for circulation rules Test plan: 1) Run t/db_dependent/api/v1/circ_rules.t .
Created attachment 71049 [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.)
Created attachment 71050 [details] [review] Bug 15522: Add production JS build mode To use, run yarn build (or make js-build) with PRODUCTION=1.
Created attachment 71051 [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.
Created attachment 71052 [details] [review] Bug 15522: Remove old circulation rules interface
Waiting for depending bugs that are not signed off yet. Jesse, can you provide a remote branch with all the patches applied?
Created attachment 72239 [details] [review] Bug 15522: Remove old circulation rules interface
Created attachment 72242 [details] [review] Bug 15522: Remove old circulation rules interface
(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 .
Is there still a way forward for this bug, assuming its dependencies are taken care of?
(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.
Hi :), I tried to test https://github.com/pianohacker/koha => branch bz15522-withdeps As it is, before trying to rebase it on master. But I can't successfully load a demo dump and do an upgrade database successfully. The last tag on the branch is v17.11.00 with means that it was from master during v18.05 development So perldeps for 18.05 dumps tried: - MARC 21 latest (v3.1900003) http://git.koha-community.org/gitweb/?p=contrib/global.git;a=history;f=sandbox/sql/sandbox1.sql.gz;h=19268bccb43b2a33d5644b7d86cbb1abb323016b;hb=HEAD I get these errors [1] with the atomic updates Other dumps tried http://git.koha-community.org/gitweb/?p=contrib/global.git;a=history;f=sandbox/sql/sandbox2.sql.gz;hb=HEAD - UNIMARC last 17.11.00 (this one should have worked) - UNIMARC last 17.05.00 - UNIMARC first 17.11.00 What did I miss to get a compatible state for the branch bz15522-withdeps ? [1] Upgrade to 17.12.00.020 done (Bug 20082 - Update descriptions of Vietnamese language) Upgrade to 17.12.00.021 done (Bug 19290 - Add system preference BrowseResultSelection) DEV atomic update: bug_15520-add_manage_circ_rules_restricted.sql DEV atomic update: bug_18887.perl DBD::mysql::db do failed: Can't create table `koha`.`circulation_rules` (errno: 150 "Foreign key constraint is incorrectly formed") [for Statement " CREATE TABLE `circulation_rules` ( `id` int(11) NOT NULL auto_increment, `branchcode` varchar(10) NULL default NULL, `categorycode` varchar(10) NULL default NULL, `itemtype` varchar(10) NULL default NULL, `rule_name` varchar(32) NOT NULL, `rule_value` varchar(32) NOT NULL, PRIMARY KEY (`id`), KEY `branchcode` (`branchcode`), KEY `categorycode` (`categorycode`), KEY `itemtype` (`itemtype`), KEY `rule_name` (`rule_name`), UNIQUE (`branchcode`,`categorycode`,`itemtype`,`rule_name`), FOREIGN KEY (branchcode) REFERENCES branches (branchcode) ON DELETE CASCADE ON UPDATE CASCADE, FOREIGN KEY (categorycode) REFERENCES categories (categorycode) ON DELETE CASCADE ON UPDATE CASCADE, FOREIGN KEY (itemtype) REFERENCES itemtypes (itemtype) ON DELETE CASCADE ON UPDATE CASCADE ) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci; "] at (eval 1177) line 4. DBD::mysql::db do failed: Table 'koha.circulation_rules' doesn't exist [for Statement " INSERT INTO circulation_rules ( branchcode, categorycode, itemtype, rule_name, rule_value ) SELECT branchcode, categorycode, NULL, 'max_holds', COALESCE( max_holds, '' ) FROM branch_borrower_circ_rules "] at (eval 1177) line 24. DBD::mysql::db do failed: Table 'koha.circulation_rules' doesn't exist [for Statement " INSERT INTO circulation_rules ( branchcode, categorycode, itemtype, rule_name, rule_value ) SELECT NULL, categorycode, NULL, 'max_holds', COALESCE( max_holds, '' ) FROM default_borrower_circ_rules "] at (eval 1177) line 29. DBD::mysql::db do failed: Can't DROP 'max_holds'; check that column/key exists [for Statement " ALTER TABLE branch_borrower_circ_rules DROP COLUMN max_holds "] at (eval 1177) line 34. DBD::mysql::db do failed: Can't DROP 'max_holds'; check that column/key exists [for Statement " ALTER TABLE default_borrower_circ_rules DROP COLUMN max_holds "] at (eval 1177) line 38. Upgrade to XXX done (Bug 18887 - Introduce new table 'circulation_rules', use for 'max_holds' rules) DEV atomic update: bug_18925.perl DBD::mysql::db do failed: Table 'koha.circulation_rules' doesn't exist [for Statement " INSERT INTO circulation_rules ( categorycode, branchcode, itemtype, rule_name, rule_value ) SELECT categorycode, branchcode, NULL, 'patron_maxissueqty', COALESCE( maxissueqty, '' ) FROM branch_borrower_circ_rules "] at (eval 1178) line 4. DBD::mysql::db do failed: Table 'koha.circulation_rules' doesn't exist [for Statement " INSERT INTO circulation_rules ( categorycode, branchcode, itemtype, rule_name, rule_value ) SELECT categorycode, branchcode, NULL, 'patron_maxonsiteissueqty', COALESCE( maxonsiteissueqty, '' ) FROM branch_borrower_circ_rules "] at (eval 1178) line 9. DBD::mysql::db do failed: Table 'koha.circulation_rules' doesn't exist [for Statement " INSERT INTO circulation_rules ( categorycode, branchcode, itemtype, rule_name, rule_value ) SELECT categorycode, NULL, NULL, 'patron_maxissueqty', COALESCE( maxissueqty, '' ) FROM default_borrower_circ_rules "] at (eval 1178) line 18. DBD::mysql::db do failed: Table 'koha.circulation_rules' doesn't exist [for Statement " INSERT INTO circulation_rules ( categorycode, branchcode, itemtype, rule_name, rule_value ) SELECT categorycode, NULL, NULL, 'patron_maxonsiteissueqty', COALESCE( maxonsiteissueqty, '' ) FROM default_borrower_circ_rules "] at (eval 1178) line 23. DBD::mysql::db do failed: Table 'koha.circulation_rules' doesn't exist [for Statement " INSERT INTO circulation_rules ( categorycode, branchcode, itemtype, rule_name, rule_value ) SELECT NULL, NULL, NULL, 'patron_maxissueqty', COALESCE( maxissueqty, '' ) FROM default_circ_rules "] at (eval 1178) line 32. DBD::mysql::db do failed: Table 'koha.circulation_rules' doesn't exist [for Statement " INSERT INTO circulation_rules ( categorycode, branchcode, itemtype, rule_name, rule_value ) SELECT NULL, NULL, NULL, 'patron_maxonsiteissueqty', COALESCE( maxonsiteissueqty, '' ) FROM default_circ_rules "] at (eval 1178) line 37. DBD::mysql::db do failed: Table 'koha.circulation_rules' doesn't exist [for Statement " INSERT INTO circulation_rules ( categorycode, branchcode, itemtype, rule_name, rule_value ) SELECT NULL, branchcode, NULL, 'patron_maxissueqty', COALESCE( maxissueqty, '' ) FROM default_branch_circ_rules "] at (eval 1178) line 46. [...]
(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.
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 ?
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.
Created attachment 91977 [details] [review] Bug 15522: Add API for circulation rules Test plan: 1) Run t/db_dependent/api/v1/circ_rules.t .
Created attachment 91978 [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
Created attachment 91979 [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
Created attachment 91980 [details] [review] Bug 15522: Remove old circulation rules interface
Created attachment 91981 [details] [review] Bug 15522: Add yarn.lock (autogenerated)
Created attachment 91982 [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.
Created attachment 91983 [details] [review] Bud 15522: Restore categories filtering Mimicking what is done for Branches
Created attachment 91984 [details] [review] Bug 15522: Replace the obsoleted fixFloat occurrence to use Sticky
Screenshot: https://snag.gy/ujakOo.jpg
Created attachment 94201 [details] [review] Bug 15522: Add API for circulation rules Test plan: 1) Run t/db_dependent/api/v1/circ_rules.t .
Created attachment 94202 [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
Created attachment 94203 [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
Created attachment 94204 [details] [review] Bug 15522: Remove old circulation rules interface
Created attachment 94205 [details] [review] Bug 15522: Add yarn.lock (autogenerated)
Created attachment 94206 [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.
Created attachment 94207 [details] [review] Bud 15522: Restore categories filtering Mimicking what is done for Branches
Created attachment 94208 [details] [review] Bug 15522: Replace the obsoleted fixFloat occurrence to use Sticky
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.
Rebased patches are on the remote branch.
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.
Patches rebased.
It's working for me. Nice and useful work. Thank You Jonathan.
(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?
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.
Remote branch rebased.
Hi, Are any of the obsoleted patches here supposed to be not obsoleted? I see this Needs Signoff
Hi Hayley, I think this has a remote branch to be used instead of attached patches: https://gitlab.com/joubu/Koha/commits/bug_15522
Ahh, that makes sense. Thanks Katrin!
Created attachment 99148 [details] [review] Bug 15522: Add API for circulation rules Test plan: 1) Run t/db_dependent/api/v1/circ_rules.t .
Created attachment 99149 [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
Created attachment 99150 [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
Created attachment 99151 [details] [review] Bug 15522: Remove old circulation rules interface
Created attachment 99152 [details] [review] Bug 15522: Add yarn.lock (autogenerated)
Created attachment 99153 [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.
Created attachment 99154 [details] [review] Bud 15522: Restore categories filtering Mimicking what is done for Branches
Created attachment 99155 [details] [review] Bug 15522: Replace the obsoleted fixFloat occurrence to use Sticky
Created attachment 99156 [details] [review] Bug 15522: Handle deprecated rules For upgraded installs. Also add a log in the console if it happens.
Created attachment 99157 [details] [review] Bug 15522: (follow-up) Handle deprecated rules Previous use of inArray was wrong.
Patches are now attached here! :)
(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.
Created attachment 99199 [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>
Created attachment 99200 [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>
Created attachment 99201 [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>
Created attachment 99202 [details] [review] Bug 15522: Replace the obsoleted fixFloat occurrence to use Sticky Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Created attachment 99203 [details] [review] Bud 15522: Restore categories filtering Mimicking what is done for Branches Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Created attachment 99204 [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>
Created attachment 99205 [details] [review] Bug 15522: Add yarn.lock (autogenerated) Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Created attachment 99206 [details] [review] Bug 15522: Remove old circulation rules interface Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Created attachment 99207 [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>
Created attachment 99208 [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>
Created attachment 99209 [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>
Created attachment 99210 [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>
Created attachment 99211 [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>
Created attachment 99213 [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>
Created attachment 99214 [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>
Created attachment 99215 [details] [review] Bug 15522: (follow-up) Handle deprecated rules Previous use of inArray was wrong. Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
We are reviewing this one internally and plan to demo it to the sponsoring library to get some feedback before this continues
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
Just seen the disclaimer : "This should not change any circulation behavior, but you will be able to review these changes before saving." Nice :)
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.
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.
Not applying. Patches have gone in wrong order. I'll attempt to fix it.
Created attachment 101918 [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>
Created attachment 101919 [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>
Created attachment 101920 [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>
Created attachment 101921 [details] [review] Bug 15522: Remove old circulation rules interface Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Created attachment 101922 [details] [review] Bug 15522: Add yarn.lock (autogenerated) Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Created attachment 101923 [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>
Created attachment 101924 [details] [review] Bud 15522: Restore categories filtering Mimicking what is done for Branches Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Created attachment 101925 [details] [review] Bug 15522: Replace the obsoleted fixFloat occurrence to use Sticky Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Created attachment 101926 [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>
Created attachment 101927 [details] [review] Bug 15522: (follow-up) Handle deprecated rules Previous use of inArray was wrong. Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Fixed patch order, obsoleted duplicates, rebased on master.
(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.
(In reply to Lisette Scheer from comment #62) > (In reply to Jonathan Druart from comment #51) > > Created attachment 99148 [details] [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.
Comment on attachment 101920 [details] [review] Bug 15522: New interface for updated circulation rules Review of attachment 101920 [details] [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
(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.
(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
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>
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>
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>
Created attachment 101937 [details] [review] Bug 15522: Remove old circulation rules interface Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
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>
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>
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>
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>
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>
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>
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>
Created attachment 101945 [details] [review] Bug 15522: (follow-up) Add missing http return codes to api spec
Created attachment 101946 [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
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 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.
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?
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.
Sponsoring library has approved, removing discussion status
(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?
(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?
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!
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.
If I try to test via the linked branch all I get is 'Loading, please wait...'.. am I doing something wrong?
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.
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>
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>
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>
Created attachment 106916 [details] [review] Bug 15522: Remove old circulation rules interface Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
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>
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>
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>
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>
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>
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>
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>
Created attachment 106924 [details] [review] Bug 15522: (follow-up) Add missing http return codes to api spec
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
Created attachment 106926 [details] [review] Bug 15522: (QA followup) Fix the swagger spec
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.
Actually.. it's growing on me.. but I think a few of the grouping terms could be improved to clarify the views.
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? :)
Can you confirm you tried the yarn commands?
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.
This needs to be rebased (conflict on gulpfile.js - see bug 26228)
(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"
Created attachment 112056 [details] [review] Bug 15522: (Vue alternative) Remove old smart rules pages and scripts
Created attachment 112057 [details] [review] Bug 15522: (Vue alternative) Add policy template and scripts
Created attachment 112058 [details] [review] Bug 15522: (Vue alternative) Modify api endpoint for circ rules
Created attachment 112059 [details] [review] Bug 15522: (Vue alternative) Set gulp script and Makefile.PL
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
Created attachment 112061 [details] [review] Bug 15522: (Vue alternative) Add yarn.lock
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.
screenshot: https://snipboard.io/jIwD0s.jpg
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.
(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.
(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.
> 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.
(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.
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.
(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.
(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.
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.
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?
Hi, we would like to test this next week during the hackfest Marseille. Is someone still working on this ? Cheers, Sonia
Discussed in current hackfest, now that there is vue in Koha via the ERM patches, there isn't anything to salvage from the vue patch submitted here. Mostly because here it's vue 2 (ERM is vue 3) and some other technical reason IIUC. But there is also the fact that the vue patch here wasn't a clone of the react patch. But different UX also. So first step is to settle on what is wanted for circ rules interface.