Bug 21411 - Advanced cataloging editor - rancor - Allow configuration of Keyboard shortcuts
Summary: Advanced cataloging editor - rancor - Allow configuration of Keyboard shortcuts
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Nick Clemens
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on:
Blocks: 17179 23396 17178
  Show dependency treegraph
 
Reported: 2018-09-26 12:05 UTC by Nick Clemens
Modified: 2019-08-02 18:56 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
This enhancement lets you globally redefine the keyboard shortcuts used in rancor.
Version(s) released in:
19.05.00


Attachments
Bug 21411 - Advanced cataloging editor - rancor - Allow configuration of Keyboard (17.02 KB, patch)
2018-09-26 12:10 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21411: DO NOT PUSH - Schema changes (2.01 KB, patch)
2018-09-26 12:10 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21411 - Advanced cataloging editor - rancor - Allow configuration of Keyboard (22.15 KB, patch)
2019-02-20 15:26 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21411: DO NOT PUSH - Schema changes (2.01 KB, patch)
2019-02-20 15:26 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21411 - Advanced cataloging editor - rancor - Allow configuration of Keyboard (22.08 KB, patch)
2019-02-20 17:37 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21411: DO NOT PUSH - Schema changes (1.88 KB, patch)
2019-02-20 17:37 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21411 - Advanced cataloging editor - rancor - Allow configuration of Keyboard (22.26 KB, patch)
2019-03-04 16:24 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21411: DO NOT PUSH - Schema changes (1.88 KB, patch)
2019-03-04 16:24 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21411: (follow-up) Missing filters and unused variable (2.97 KB, patch)
2019-03-04 16:41 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21411 - Advanced cataloging editor - rancor - Allow configuration of Keyboard (22.30 KB, patch)
2019-03-04 17:44 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 21411: DO NOT PUSH - Schema changes (1.92 KB, patch)
2019-03-04 17:44 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 21411: (follow-up) Missing filters and unused variable (3.02 KB, patch)
2019-03-04 17:44 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 21411: Advanced cataloging editor - rancor - Allow configuration of Keyboard (22.29 KB, patch)
2019-04-23 12:32 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21411: (follow-up) Missing filters and unused variable (3.02 KB, patch)
2019-04-23 12:32 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21411: Address QA concerns (19.93 KB, patch)
2019-04-23 12:32 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21411: DO NOT PUSH - SCHEMA CHANGES (1.72 KB, patch)
2019-04-23 12:32 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21411: (QA follow-up) fix tests (800 bytes, patch)
2019-04-30 14:22 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 21411: (follow-up) Clean up for QA (7.71 KB, patch)
2019-05-01 11:29 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21411: Advanced cataloging editor - rancor - Allow configuration of Keyboard (22.35 KB, patch)
2019-05-07 08:14 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 21411: (follow-up) Missing filters and unused variable (3.07 KB, patch)
2019-05-07 08:14 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 21411: Address QA concerns (19.99 KB, patch)
2019-05-07 08:14 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 21411: (QA follow-up) fix tests (857 bytes, patch)
2019-05-07 08:15 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 21411: (follow-up) Clean up for QA (7.77 KB, patch)
2019-05-07 08:15 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 21411: (QA follow-up) Fix permission checks on admin page and menu (3.78 KB, patch)
2019-05-07 08:15 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2018-09-26 12:05:21 UTC
These patches will add a link tot he keyboard shortcuts dropdown in the advanced cataloging editor to a page where the mappings can be redefined
Comment 1 Nick Clemens 2018-09-26 12:10:37 UTC
Created attachment 79408 [details] [review]
Bug 21411 - Advanced cataloging editor - rancor - Allow configuration of Keyboard

This patch allows for redfining the keyboard shortcuts used in rancor.
For now these are set globally (per instance)
Ctrl-S is a codemirror setting and cannot be altered

To test:
 1 - Apply patches
 2 - update database
 3 - Load the advanced cataloging editor
 4 - Click the "Keyboard shortcuts" button
 5 - Ensure the list looks correct
 6 - Note that 'Ctrl-S' note sit cannot be remapped
 7 - Note the 'Redefine shortcuts' link at the top of the menu
 8 - Confirm all the listed shortcuts work as expected
 9 - Click the 'Redefine' link
10 - Ensure current mappings load correctly
11 - Read explanation and verify it makes sense
12 - Remap some functions
13 - Return to the cataloging editor
14 - Confirm your mappings work
Comment 2 Nick Clemens 2018-09-26 12:10:40 UTC
Created attachment 79409 [details] [review]
Bug 21411: DO NOT PUSH - Schema changes
Comment 3 Josef Moravec 2018-09-26 15:36:17 UTC
Hi Nick, nice enhancement, just few quick comments:

1) you should also provide sql installer data - for new installs, 

2) there is missing file tools/adveditorshortcuts.pl

3) I am not sure if it should be part of tools module...

4) in file adveditorshortcuts.tt you are referencing the adveditorshortcuts.pl as reviews/adveditorshortcuts.pl - in breadcrumbs

5) could you please use bootstrap for layout of edit shortcuts page?
Comment 4 Nick Clemens 2019-02-20 15:26:28 UTC
Created attachment 85365 [details] [review]
Bug 21411 - Advanced cataloging editor - rancor - Allow configuration of Keyboard

This patch allows for redfining the keyboard shortcuts used in rancor.
For now these are set globally (per instance)
Ctrl-S is a codemirror setting and cannot be altered

To test:
 1 - Apply patches
 2 - update database
 3 - Load the advanced cataloging editor
 4 - Click the "Keyboard shortcuts" button
 5 - Ensure the list looks correct
 6 - Note that 'Ctrl-S' note sit cannot be remapped
 7 - Note the 'Redefine shortcuts' link at the top of the menu
 8 - Confirm all the listed shortcuts work as expected
 9 - Click the 'Redefine' link
10 - Ensure current mappings load correctly
11 - Read explanation and verify it makes sense
12 - Remap some functions
13 - Return to the cataloging editor
14 - Confirm your mappings work
Comment 5 Nick Clemens 2019-02-20 15:26:31 UTC
Created attachment 85366 [details] [review]
Bug 21411: DO NOT PUSH - Schema changes
Comment 6 Nick Clemens 2019-02-20 17:37:55 UTC
Created attachment 85397 [details] [review]
Bug 21411 - Advanced cataloging editor - rancor - Allow configuration of Keyboard

This patch allows for redfining the keyboard shortcuts used in rancor.
For now these are set globally (per instance)
Ctrl-S is a codemirror setting and cannot be altered

To test:
 1 - Apply patches
 2 - update database
 3 - Load the advanced cataloging editor
 4 - Click the "Keyboard shortcuts" button
 5 - Ensure the list looks correct
 6 - Note that 'Ctrl-S' note sit cannot be remapped
 7 - Note the 'Redefine shortcuts' link at the top of the menu
 8 - Confirm all the listed shortcuts work as expected
 9 - Click the 'Redefine' link
10 - Ensure current mappings load correctly
11 - Read explanation and verify it makes sense
12 - Remap some functions
13 - Return to the cataloging editor
14 - Confirm your mappings work
Comment 7 Nick Clemens 2019-02-20 17:37:58 UTC
Created attachment 85398 [details] [review]
Bug 21411: DO NOT PUSH - Schema changes
Comment 8 Nick Clemens 2019-03-04 16:24:14 UTC
Created attachment 86012 [details] [review]
Bug 21411 - Advanced cataloging editor - rancor - Allow configuration of Keyboard

This patch allows for redfining the keyboard shortcuts used in rancor.
For now these are set globally (per instance)
Ctrl-S is a codemirror setting and cannot be altered

To test:
 1 - Apply patches
 2 - update database
 3 - Load the advanced cataloging editor
 4 - Click the "Keyboard shortcuts" button
 5 - Ensure the list looks correct
 6 - Note that 'Ctrl-S' note sit cannot be remapped
 7 - Note the 'Redefine shortcuts' link at the top of the menu
 8 - Confirm all the listed shortcuts work as expected
 9 - Click the 'Redefine' link
10 - Ensure current mappings load correctly
11 - Read explanation and verify it makes sense
12 - Remap some functions
13 - Return to the cataloging editor
14 - Confirm your mappings work
Comment 9 Nick Clemens 2019-03-04 16:24:16 UTC
Created attachment 86013 [details] [review]
Bug 21411: DO NOT PUSH - Schema changes
Comment 10 Nick Clemens 2019-03-04 16:41:45 UTC
Created attachment 86014 [details] [review]
Bug 21411: (follow-up) Missing filters and unused variable

POD warnings from QA tool are false positives (basic functionsn POD not
needed)
Comment 11 Liz Rea 2019-03-04 17:44:04 UTC
Created attachment 86018 [details] [review]
Bug 21411 - Advanced cataloging editor - rancor - Allow configuration of Keyboard

This patch allows for redfining the keyboard shortcuts used in rancor.
For now these are set globally (per instance)
Ctrl-S is a codemirror setting and cannot be altered

To test:
 1 - Apply patches
 2 - update database
 3 - Load the advanced cataloging editor
 4 - Click the "Keyboard shortcuts" button
 5 - Ensure the list looks correct
 6 - Note that 'Ctrl-S' note sit cannot be remapped
 7 - Note the 'Redefine shortcuts' link at the top of the menu
 8 - Confirm all the listed shortcuts work as expected
 9 - Click the 'Redefine' link
10 - Ensure current mappings load correctly
11 - Read explanation and verify it makes sense
12 - Remap some functions
13 - Return to the cataloging editor
14 - Confirm your mappings work

Signed-off-by: Liz Rea <liz@catalyst.net.nz>
Comment 12 Liz Rea 2019-03-04 17:44:07 UTC
Created attachment 86019 [details] [review]
Bug 21411: DO NOT PUSH - Schema changes

Signed-off-by: Liz Rea <liz@catalyst.net.nz>
Comment 13 Liz Rea 2019-03-04 17:44:10 UTC
Created attachment 86020 [details] [review]
Bug 21411: (follow-up) Missing filters and unused variable

POD warnings from QA tool are false positives (basic functionsn POD not
needed)

Signed-off-by: Liz Rea <liz@catalyst.net.nz>
Comment 14 Liz Rea 2019-03-04 17:50:21 UTC
I've got a bad git email address hiding out somewhere in my git config, so sorry if those look like my old email address at Catalyst. I'm still signing off and if you're inclined you can change them to wizzyrea@gmail.com. I don't want to clutter the bug by fixing it but I'll fix it before I do my next one.

Cheers,
Liz
Comment 15 Katrin Fischer 2019-04-10 22:02:32 UTC
Hi Nick, 

having a first look here:

1) FAIL	Koha/KeyboardShortcuts.pm
   OK	  critic
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  pod
   FAIL	  pod coverage
		POD is missing for 'object_class'

2) No tests?

3) Translation issues

a) Ctrl is not labelled Ctrl on German keyboards, it's Strg - so people might not understand. Currently I can fix this in translation, but with the new system that will not work. We could add another column for a 'spelled out' mapping or we could add some replacing in the template that can be translated. Not sure here. (Not quite blocker... but meh)

b) The new file is missing from the translated installers. (blocker)
I'd like to suggest a different approach here. As you can't invent your own keyboard shortcuts, I'd deal with this like with the permissions. Have one SQL file with the mandatory values on top level (not in language directories) and deal with the description translation in the templates.

4) I am wondering if tools is the right place for this. I think I would put it with Sound alerts and column configuration in Administration.

5) If we leave it in tools: It's missing from tools sidebar and start page. If we move to admin, we'll also need sidebar and start page.
As the navigation of tools shows on the tool page, I think that makes it currently a bit confusing.

6) Permissions: currently it doesn't require any permission:
flagsrequired   => {},
Do we want to leave it like this?
As the keyboard shortcuts are global and not per user I can imagine libraries would not want anyone be able to reconfigure them.

7) "If combing multiple keys " - interesting idea :)

8) The button is a bit close to the table, could use a little template clean-up.
Comment 16 Nick Clemens 2019-04-23 12:32:10 UTC
Created attachment 88465 [details] [review]
Bug 21411: Advanced cataloging editor - rancor - Allow configuration of Keyboard

This patch allows for redfining the keyboard shortcuts used in rancor.
For now these are set globally (per instance)
Ctrl-S is a codemirror setting and cannot be altered

To test:
 1 - Apply patches
 2 - update database
 3 - Load the advanced cataloging editor
 4 - Click the "Keyboard shortcuts" button
 5 - Ensure the list looks correct
 6 - Note that 'Ctrl-S' note sit cannot be remapped
 7 - Note the 'Redefine shortcuts' link at the top of the menu
 8 - Confirm all the listed shortcuts work as expected
 9 - Click the 'Redefine' link
10 - Ensure current mappings load correctly
11 - Read explanation and verify it makes sense
12 - Remap some functions
13 - Return to the cataloging editor
14 - Confirm your mappings work

Signed-off-by: Liz Rea <liz@catalyst.net.nz>
Comment 17 Nick Clemens 2019-04-23 12:32:13 UTC
Created attachment 88466 [details] [review]
Bug 21411: (follow-up) Missing filters and unused variable

POD warnings from QA tool are false positives (basic functionsn POD not
needed)

Signed-off-by: Liz Rea <liz@catalyst.net.nz>
Comment 18 Nick Clemens 2019-04-23 12:32:15 UTC
Created attachment 88467 [details] [review]
Bug 21411: Address QA concerns

Move to admin
Add a permission
Remove descriptions from table
Clean up template
Comment 19 Nick Clemens 2019-04-23 12:32:18 UTC
Created attachment 88468 [details] [review]
Bug 21411: DO NOT PUSH - SCHEMA CHANGES
Comment 20 Katrin Fischer 2019-04-27 15:29:55 UTC
Something is odd here :(

kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove -v t/db_dependent/Koha/KeyboardShortcuts.t
t/db_dependent/Koha/KeyboardShortcuts.t .. 
1..4
ok 1 - use Koha::KeyboardShortcuts;
DBIx::Class::Row::store_column(): No such column 'shortcut_desc' on Koha::Schema::Result::KeyboardShortcut at /home/vagrant/kohaclone/Koha/Object.pm line 75
# Looks like your test exited with 255 just after 1.
Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 3/4 subtests 

Test Summary Report
-------------------
t/db_dependent/Koha/KeyboardShortcuts.t (Wstat: 65280 Tests: 1 Failed: 0)
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 4 tests but ran 1.
Files=1, Tests=1,  2 wallclock secs ( 0.03 usr  0.01 sys +  1.50 cusr  0.37 csys =  1.91 CPU)
Result: FAIL

I've run updatedatabase, dbic, restart_all etc. :(

Functionality wise this seems ok so far, can you please have a look?
Comment 21 Liz Rea 2019-04-30 14:22:56 UTC
Created attachment 89119 [details] [review]
Bug 21411: (QA follow-up) fix tests

This patch removes erroneous shortcut_desc from the tests, which was left in accidentally.
Comment 22 Katrin Fischer 2019-05-01 10:02:14 UTC
Suggested follow-ups:

Hide hide the Keyboard settings when Rancor is not activated, similar like other settings are not visible (Plugins, Search engine configs) without the pref.
Comment 23 Katrin Fischer 2019-05-01 10:07:35 UTC
Some small stuff:

1) As we moved the sql out of hte en installer, this needs to go too:
installer/data/mysql/en/mandatory/keyboard_shortcuts.txt

2) Do you really want to give liblime copyright?

+-- Default keyboard shortcuts
+-- for Koha.
+--
+-- Copyright (C) 2007 LiblimeA
+-- Copyright 2018 Koha Development Team

3) Needs permission check:
+<a id="redefine_shortcuts" href="/cgi-bin/koha/admin/adveditorshortcuts.pl">Redefine shortcuts</a>

4) Hide hide the Keyboard settings when Rancor is not activated, similar like other settings are not visible (Plugins, Search engine configs) without the pref.
- Admin menu
- Admin home page
Comment 24 Nick Clemens 2019-05-01 11:29:09 UTC
Created attachment 89179 [details] [review]
Bug 21411: (follow-up) Clean up for QA
Comment 25 Katrin Fischer 2019-05-07 08:14:53 UTC
Created attachment 89416 [details] [review]
Bug 21411: Advanced cataloging editor - rancor - Allow configuration of Keyboard

This patch allows for redfining the keyboard shortcuts used in rancor.
For now these are set globally (per instance)
Ctrl-S is a codemirror setting and cannot be altered

To test:
 1 - Apply patches
 2 - update database
 3 - Load the advanced cataloging editor
 4 - Click the "Keyboard shortcuts" button
 5 - Ensure the list looks correct
 6 - Note that 'Ctrl-S' note sit cannot be remapped
 7 - Note the 'Redefine shortcuts' link at the top of the menu
 8 - Confirm all the listed shortcuts work as expected
 9 - Click the 'Redefine' link
10 - Ensure current mappings load correctly
11 - Read explanation and verify it makes sense
12 - Remap some functions
13 - Return to the cataloging editor
14 - Confirm your mappings work

Signed-off-by: Liz Rea <liz@catalyst.net.nz>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 26 Katrin Fischer 2019-05-07 08:14:56 UTC
Created attachment 89417 [details] [review]
Bug 21411: (follow-up) Missing filters and unused variable

POD warnings from QA tool are false positives (basic functionsn POD not
needed)

Signed-off-by: Liz Rea <liz@catalyst.net.nz>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 27 Katrin Fischer 2019-05-07 08:14:59 UTC
Created attachment 89418 [details] [review]
Bug 21411: Address QA concerns

Move to admin
Add a permission
Remove descriptions from table
Clean up template

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 28 Katrin Fischer 2019-05-07 08:15:03 UTC
Created attachment 89419 [details] [review]
Bug 21411: (QA follow-up) fix tests

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 29 Katrin Fischer 2019-05-07 08:15:06 UTC
Created attachment 89420 [details] [review]
Bug 21411: (follow-up) Clean up for QA

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 30 Katrin Fischer 2019-05-07 08:15:10 UTC
Created attachment 89421 [details] [review]
Bug 21411: (QA follow-up) Fix permission checks on admin page and menu

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 31 Katrin Fischer 2019-05-07 08:16:00 UTC
I realized I had missed the follow-up, sorry!
Comment 32 Nick Clemens 2019-05-10 19:12:03 UTC
Awesome work all!

Pushed to master for 19.05
Comment 33 Martin Renvoize 2019-05-24 13:27:57 UTC
Enhancement will not be backported to 18.11.x series.