Bug 24606 - Allow storing item values as a template for creating new items
Summary: Allow storing item values as a template for creating new items
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low new feature (vote)
Assignee: Kyle M Hall
QA Contact: Katrin Fischer
URL:
Keywords: release-notes-needed
Depends on:
Blocks: 32188
  Show dependency treegraph
 
Reported: 2020-02-06 18:26 UTC by Nick Clemens
Modified: 2022-11-22 19:12 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This new feature allows librarians to create and share "item templates" where one or more item field values can be set for a template. Templates can be applied on a one by one basis, or set for the remainder of the logged in session. Each template may be optionally shared ( read only ) to other catalogers. Librarians with the manage_item_editor_templates permission may edit any template regardless of who created it.
Version(s) released in:
22.11.00


Attachments
Bug 24606: Update database, add new schema file (5.42 KB, patch)
2022-10-11 16:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24606: Add Koha Object(s) with unit tests (8.96 KB, patch)
2022-10-11 16:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24606: Update existing schema files [DO NOT PUSH] (1.47 KB, patch)
2022-10-11 16:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24606: Implement item templates (15.26 KB, patch)
2022-10-11 16:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24606: Add item template editor permission (8.43 KB, patch)
2022-10-11 16:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24606: Update database, add new schema file (5.42 KB, patch)
2022-10-12 10:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24606: Add Koha Object(s) with unit tests (8.96 KB, patch)
2022-10-12 10:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24606: Implement item templates (15.26 KB, patch)
2022-10-12 10:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24606: Add item template editor permission (8.43 KB, patch)
2022-10-12 10:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24606: Update existing schema files [DO NOT PUSH] (1.47 KB, patch)
2022-10-12 10:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24606: (QA follow-up) Set existing sharing setting when updating existing template (2.82 KB, patch)
2022-10-17 13:35 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24606: Update database, add new schema file (5.46 KB, patch)
2022-10-17 18:55 UTC, David Nind
Details | Diff | Splinter Review
Bug 24606: Add Koha Object(s) with unit tests (9.00 KB, patch)
2022-10-17 18:55 UTC, David Nind
Details | Diff | Splinter Review
Bug 24606: Implement item templates (15.31 KB, patch)
2022-10-17 18:55 UTC, David Nind
Details | Diff | Splinter Review
Bug 24606: Add item template editor permission (8.48 KB, patch)
2022-10-17 18:55 UTC, David Nind
Details | Diff | Splinter Review
Bug 24606: Update existing schema files [DO NOT PUSH] (1.52 KB, patch)
2022-10-17 18:56 UTC, David Nind
Details | Diff | Splinter Review
Bug 24606: (QA follow-up) Set existing sharing setting when updating existing template (2.87 KB, patch)
2022-10-17 18:56 UTC, David Nind
Details | Diff | Splinter Review
Bug 24606: (QA follow-up) Fix QA script issues (2.61 KB, patch)
2022-10-31 15:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24606: (QA follow-up) Update js message translation style (963 bytes, patch)
2022-10-31 15:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24606: (QA follow-up) Fix up interface (6.19 KB, patch)
2022-10-31 16:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24606: (QA follow-up) Hide button when using multi-add (1.75 KB, patch)
2022-10-31 16:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24606: (QA follow-up) Disable 'Apply template' button if no template is selected (1.43 KB, patch)
2022-10-31 16:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24606: Update database, add new schema file (5.52 KB, patch)
2022-11-01 14:36 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 24606: Add Koha Object(s) with unit tests (9.06 KB, patch)
2022-11-01 14:36 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 24606: Implement item templates (15.36 KB, patch)
2022-11-01 14:36 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 24606: Add item template editor permission (8.54 KB, patch)
2022-11-01 14:36 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 24606: Update existing schema files [DO NOT PUSH] (1.58 KB, patch)
2022-11-01 14:36 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 24606: (QA follow-up) Set existing sharing setting when updating existing template (2.92 KB, patch)
2022-11-01 14:36 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 24606: (QA follow-up) Fix QA script issues (2.67 KB, patch)
2022-11-01 14:37 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 24606: (QA follow-up) Update js message translation style (1020 bytes, patch)
2022-11-01 14:37 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 24606: (QA follow-up) Fix up interface (6.24 KB, patch)
2022-11-01 14:37 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 24606: (QA follow-up) Hide button when using multi-add (1.81 KB, patch)
2022-11-01 14:37 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 24606: (QA follow-up) Disable 'Apply template' button if no template is selected (1.48 KB, patch)
2022-11-01 14:37 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 24606: (QA follow-up) Remove string variable (1.31 KB, patch)
2022-11-01 14:37 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 24606: (QA follow-up) Fix up save form a bit (4.51 KB, patch)
2022-11-02 17:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24606: Update database, add new schema file (5.52 KB, patch)
2022-11-09 18:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24606: Add Koha Object(s) with unit tests (9.06 KB, patch)
2022-11-09 18:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24606: Implement item templates (15.39 KB, patch)
2022-11-09 18:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24606: Add item template editor permission (8.54 KB, patch)
2022-11-09 18:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24606: (QA follow-up) Set existing sharing setting when updating existing template (2.92 KB, patch)
2022-11-09 18:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24606: (QA follow-up) Fix QA script issues (2.67 KB, patch)
2022-11-09 18:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24606: (QA follow-up) Update js message translation style (1020 bytes, patch)
2022-11-09 18:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24606: (QA follow-up) Fix up interface (6.24 KB, patch)
2022-11-09 18:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24606: (QA follow-up) Hide button when using multi-add (1.81 KB, patch)
2022-11-09 18:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24606: (QA follow-up) Disable 'Apply template' button if no template is selected (1.48 KB, patch)
2022-11-09 18:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24606: (QA follow-up) Remove string variable (1.31 KB, patch)
2022-11-09 18:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24606: (QA follow-up) Fix up save form a bit (4.51 KB, patch)
2022-11-09 18:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24606: Update existing schema files [DO NOT PUSH] (1.47 KB, patch)
2022-11-09 18:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24606: Regression tests (1.89 KB, patch)
2022-11-11 18:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24606: Fix encoding issues on decoded content (1.69 KB, patch)
2022-11-11 18:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24606: (QA follow-up) Remove duplicate include (702 bytes, patch)
2022-11-11 18:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24606: (QA follow-up) Fix encoding/decoding (1.43 KB, patch)
2022-11-17 10:17 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24606: (QA follow-up) Add test for encoding/decoding (1.75 KB, patch)
2022-11-17 10:17 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24606: (QA follow-up) Fix Template.t (1.43 KB, patch)
2022-11-22 15:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2020-02-06 18:26:42 UTC
This is a feature developed by Kyle...elsewhere...

Would need to be reworked to fit into modern Koha

https://github.com/liblime/LibLime-Koha/commit/ac11f3d86772891e605b58ab7b5d9eac3be5b605
Comment 1 Kyle M Hall 2022-10-11 16:48:53 UTC
Created attachment 141613 [details] [review]
Bug 24606: Update database, add new schema file
Comment 2 Kyle M Hall 2022-10-11 16:49:04 UTC
Created attachment 141614 [details] [review]
Bug 24606: Add Koha Object(s) with unit tests
Comment 3 Kyle M Hall 2022-10-11 16:49:08 UTC
Created attachment 141615 [details] [review]
Bug 24606: Update existing schema files [DO NOT PUSH]
Comment 4 Kyle M Hall 2022-10-11 16:49:12 UTC
Created attachment 141616 [details] [review]
Bug 24606: Implement item templates

This patch set implements item editor templates for community Koha.

Test Plan:
1) Apply this patch set
2) Run updatedatabase.pl
3) Restart all the things!
4) prove t/db_dependent/Koha/Item/Template*
5) As a non superlibrarian, enter the item editor
6) Set some item fields, save as a new template using the buttom and
   form below the editor.
7) Test loading a template without remembering for the session
8) Test loading a template while remembering for the session
9) Test deleting a template
10) Test updating a template
11) Create one or more shared templates
12) Log in as another non superlibrarian without the new permission manage_item_editor_templates,
    verify you cannot edit/delete templates shared to you
13) Enable the new permission manage_item_editor_templates,
    verify you can now edit and delete templates shared to you
Comment 5 Kyle M Hall 2022-10-11 16:49:16 UTC
Created attachment 141617 [details] [review]
Bug 24606: Add item template editor permission
Comment 6 David Nind 2022-10-11 20:52:44 UTC
Patch no longer applies:

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 24606: Update database, add new schema file
Applying: Bug 24606: Add Koha Object(s) with unit tests
Applying: Bug 24606: Update existing schema files [DO NOT PUSH]
Using index info to reconstruct a base tree...
M	Koha/Schema/Result/Borrower.pm
Falling back to patching base and 3-way merge...
Auto-merging Koha/Schema/Result/Borrower.pm
CONFLICT (content): Merge conflict in Koha/Schema/Result/Borrower.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 24606: Update existing schema files [DO NOT PUSH]
Comment 7 Kyle M Hall 2022-10-12 10:57:35 UTC
Created attachment 141717 [details] [review]
Bug 24606: Update database, add new schema file
Comment 8 Kyle M Hall 2022-10-12 10:58:12 UTC
Created attachment 141718 [details] [review]
Bug 24606: Add Koha Object(s) with unit tests
Comment 9 Kyle M Hall 2022-10-12 10:58:16 UTC
Created attachment 141719 [details] [review]
Bug 24606: Implement item templates

This patch set implements item editor templates for community Koha.

Test Plan:
1) Apply this patch set
2) Run updatedatabase.pl
3) Restart all the things!
4) prove t/db_dependent/Koha/Item/Template*
5) As a non superlibrarian, enter the item editor
6) Set some item fields, save as a new template using the buttom and
   form below the editor.
7) Test loading a template without remembering for the session
8) Test loading a template while remembering for the session
9) Test deleting a template
10) Test updating a template
11) Create one or more shared templates
12) Log in as another non superlibrarian without the new permission manage_item_editor_templates,
    verify you cannot edit/delete templates shared to you
13) Enable the new permission manage_item_editor_templates,
    verify you can now edit and delete templates shared to you
Comment 10 Kyle M Hall 2022-10-12 10:58:20 UTC
Created attachment 141720 [details] [review]
Bug 24606: Add item template editor permission
Comment 11 Kyle M Hall 2022-10-12 10:58:23 UTC
Created attachment 141721 [details] [review]
Bug 24606: Update existing schema files [DO NOT PUSH]
Comment 12 David Nind 2022-10-14 21:41:09 UTC
The only thing I noticed is around editing and updating an already shared template.

If you are not the creator, and you can have manage_item_editor_templates permissions, if you don't select 'Share template' then you make it unshared and can no longer see it.

Maybe if it is a shared template, the default when updating the template is for the share template option to be selected by default.

To replicate:
1. Have two librarian users, one with manage_item_editor_templates permissions (henry) the other without (casey).
2. Create and share a template as henry.
3. As casey, apply the shared template and make some changes, for example the shelving location
4. Casey can only save as a new template, can't update the existing one - as expected.
5. Change permissions for casey, so they have manage_item_editor_templates.
6. Repeat step 3
7. Select save as template and then select the template from under update shared template.
5. Don't tick the share template option.
6. Now you can't see or choose that template - it is only visible to the original creator (henry).
7. Log in as henry - you can see the template, as creator.

Everything else seems to work as per the test plan.
Comment 13 Kyle M Hall 2022-10-17 11:01:56 UTC
Good catch! I'll get that fixed asap!

Kyle

(In reply to David Nind from comment #12)
> The only thing I noticed is around editing and updating an already shared
> template.
> 
> If you are not the creator, and you can have manage_item_editor_templates
> permissions, if you don't select 'Share template' then you make it unshared
> and can no longer see it.
> 
> Maybe if it is a shared template, the default when updating the template is
> for the share template option to be selected by default.
> 
> To replicate:
> 1. Have two librarian users, one with manage_item_editor_templates
> permissions (henry) the other without (casey).
> 2. Create and share a template as henry.
> 3. As casey, apply the shared template and make some changes, for example
> the shelving location
> 4. Casey can only save as a new template, can't update the existing one - as
> expected.
> 5. Change permissions for casey, so they have manage_item_editor_templates.
> 6. Repeat step 3
> 7. Select save as template and then select the template from under update
> shared template.
> 5. Don't tick the share template option.
> 6. Now you can't see or choose that template - it is only visible to the
> original creator (henry).
> 7. Log in as henry - you can see the template, as creator.
> 
> Everything else seems to work as per the test plan.
Comment 14 Kyle M Hall 2022-10-17 13:35:54 UTC
Created attachment 141972 [details] [review]
Bug 24606: (QA follow-up) Set existing sharing setting when updating existing template
Comment 15 David Nind 2022-10-17 18:55:42 UTC
Created attachment 142003 [details] [review]
Bug 24606: Update database, add new schema file

Signed-off-by: David Nind <david@davidnind.com>
Comment 16 David Nind 2022-10-17 18:55:48 UTC
Created attachment 142004 [details] [review]
Bug 24606: Add Koha Object(s) with unit tests

Signed-off-by: David Nind <david@davidnind.com>
Comment 17 David Nind 2022-10-17 18:55:54 UTC
Created attachment 142005 [details] [review]
Bug 24606: Implement item templates

This patch set implements item editor templates for community Koha.

Test Plan:
1) Apply this patch set
2) Run updatedatabase.pl
3) Restart all the things!
4) prove t/db_dependent/Koha/Item/Template*
5) As a non superlibrarian, enter the item editor
6) Set some item fields, save as a new template using the buttom and
   form below the editor.
7) Test loading a template without remembering for the session
8) Test loading a template while remembering for the session
9) Test deleting a template
10) Test updating a template
11) Create one or more shared templates
12) Log in as another non superlibrarian without the new permission manage_item_editor_templates,
    verify you cannot edit/delete templates shared to you
13) Enable the new permission manage_item_editor_templates,
    verify you can now edit and delete templates shared to you

Signed-off-by: David Nind <david@davidnind.com>
Comment 18 David Nind 2022-10-17 18:55:59 UTC
Created attachment 142006 [details] [review]
Bug 24606: Add item template editor permission

Signed-off-by: David Nind <david@davidnind.com>
Comment 19 David Nind 2022-10-17 18:56:05 UTC
Created attachment 142007 [details] [review]
Bug 24606: Update existing schema files [DO NOT PUSH]

Signed-off-by: David Nind <david@davidnind.com>
Comment 20 David Nind 2022-10-17 18:56:10 UTC
Created attachment 142008 [details] [review]
Bug 24606: (QA follow-up) Set existing sharing setting when updating existing template

Signed-off-by: David Nind <david@davidnind.com>
Comment 21 Katrin Fischer 2022-10-30 12:04:47 UTC
1) QA test tools:

 OK	Koha/Item/Template.pm
 FAIL	Koha/Item/Templates.pm
   FAIL	  pod
		*** WARNING: =head3 without preceding higher level  in file Koha/Item/Templates.pm
		*** WARNING: =head3 without preceding higher level  in file Koha/Item/Templates.pm
		*** WARNING: =head3 without preceding higher level  in file Koha/Item/Templates.pm

 OK	Koha/Schema/Result/Borrower.pm
 FAIL	Koha/Schema/Result/ItemEditorTemplate.pm
   FAIL	  pod coverage
		POD is missing for koha_object_class
		POD is missing for koha_objects_class

 OK	cataloguing/additem.pl
 OK	installer/data/mysql/atomicupdate/bug_24606.pl
 FAIL	installer/data/mysql/kohastructure.sql
   FAIL	  tinyint_has_boolean_flag
		The new column item_editor_templates.is_shared is using TINYINT(1) as type but is not defined as boolean in the schema file (Koha/Schema/Result/ItemEditorTemplate.pm), see the SQL12 coding guideline

 OK	koha-tmpl/intranet-tmpl/prog/en/includes/permissions.inc
 OK	koha-tmpl/intranet-tmpl/prog/en/includes/str/cataloging_additem.inc
 FAIL	koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/additem.tt
   FAIL	  filters
		missing_filter at line 275 (                    <option data-editor="1" data-shared="[% t.is_shared %]" value="[% t.id | html %]">[% t.name | html %][% IF t.is_shared %] (shared)[% END %]</option>)

2) Why the sub in cataloguing/additem.pl instead of a module?

+sub get_item_from_template 

3) New cookie alert!

ItemEditorSessionTemplateId

If this gets pushed, please don't forget updating https://wiki.koha-community.org/wiki/Use_of_Cookies. 

4) Translatability

You used the old style here, which is no longer necessary:

+        alert(MSG_TEMPLATE_NAME_REQUIRED);

You can just put the message into the .js using the double-underscore: __("")

Not a blocker for me, as it looks like the other messages on this template haven't been moved yet, but not opposed to a fix either :)

5) The GUI doesn't work well with the new staff interface design. The elements don't align nicely (especialy the save template one is jumping up and down a lot with the elements) and we are missing padding. I believe we should not push without having this sorted or on the way to be sorted. - We want to advertise and document this feature and then it should look nice!

5) When using "Add multiple items" all other buttons disappear, but the "Save as template" remains. Should this also be hidden?

6) 'Delete template' is greyed out, when no template is selected, but 'Apply template' is always active. Should we make 'Apply template' also depend on having a template selected first?
Comment 22 Kyle M Hall 2022-10-31 15:38:14 UTC
Created attachment 142833 [details] [review]
Bug 24606: (QA follow-up) Fix QA script issues
Comment 23 Kyle M Hall 2022-10-31 15:49:28 UTC
Created attachment 142834 [details] [review]
Bug 24606: (QA follow-up) Update js message translation style
Comment 24 Kyle M Hall 2022-10-31 16:00:09 UTC
Created attachment 142835 [details] [review]
Bug 24606: (QA follow-up) Fix up interface
Comment 25 Kyle M Hall 2022-10-31 16:05:40 UTC
Created attachment 142836 [details] [review]
Bug 24606: (QA follow-up) Hide button when using multi-add
Comment 26 Kyle M Hall 2022-10-31 16:17:34 UTC
Created attachment 142837 [details] [review]
Bug 24606: (QA follow-up) Disable 'Apply template' button if no template is selected
Comment 27 Kyle M Hall 2022-10-31 16:19:57 UTC
(In reply to Katrin Fischer from comment #21)
> 1) QA test tools:

Fixed with followup!

> 2) Why the sub in cataloguing/additem.pl instead of a module?

To remain consistent with how this script was developed and operates. It could use an overhaul, but that's outside the scope of this bug.

> 3) New cookie alert!

Done!

> 4) Translatability

Fixed with followup!

> 5) The GUI doesn't work well with the new staff interface design. The
> elements don't align nicely (especialy the save template one is jumping up
> and down a lot with the elements) and we are missing padding. I believe we
> should not push without having this sorted or on the way to be sorted. - We
> want to advertise and document this feature and then it should look nice!

Fixed with followup!
> 
> 5) When using "Add multiple items" all other buttons disappear, but the
> "Save as template" remains. Should this also be hidden?

Seems reasonable to me. Fixed with a followup!

> 
> 6) 'Delete template' is greyed out, when no template is selected, but 'Apply
> template' is always active. Should we make 'Apply template' also depend on
> having a template selected first?

Also fixed with a followup!
Comment 28 Katrin Fischer 2022-11-01 11:48:23 UTC
Back here.
Comment 29 Katrin Fischer 2022-11-01 14:36:32 UTC
Created attachment 142877 [details] [review]
Bug 24606: Update database, add new schema file

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 30 Katrin Fischer 2022-11-01 14:36:38 UTC
Created attachment 142878 [details] [review]
Bug 24606: Add Koha Object(s) with unit tests

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 31 Katrin Fischer 2022-11-01 14:36:43 UTC
Created attachment 142879 [details] [review]
Bug 24606: Implement item templates

This patch set implements item editor templates for community Koha.

Test Plan:
1) Apply this patch set
2) Run updatedatabase.pl
3) Restart all the things!
4) prove t/db_dependent/Koha/Item/Template*
5) As a non superlibrarian, enter the item editor
6) Set some item fields, save as a new template using the buttom and
   form below the editor.
7) Test loading a template without remembering for the session
8) Test loading a template while remembering for the session
9) Test deleting a template
10) Test updating a template
11) Create one or more shared templates
12) Log in as another non superlibrarian without the new permission manage_item_editor_templates,
    verify you cannot edit/delete templates shared to you
13) Enable the new permission manage_item_editor_templates,
    verify you can now edit and delete templates shared to you

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 32 Katrin Fischer 2022-11-01 14:36:48 UTC
Created attachment 142880 [details] [review]
Bug 24606: Add item template editor permission

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 33 Katrin Fischer 2022-11-01 14:36:53 UTC
Created attachment 142881 [details] [review]
Bug 24606: Update existing schema files [DO NOT PUSH]

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 34 Katrin Fischer 2022-11-01 14:36:59 UTC
Created attachment 142882 [details] [review]
Bug 24606: (QA follow-up) Set existing sharing setting when updating existing template

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 35 Katrin Fischer 2022-11-01 14:37:04 UTC
Created attachment 142883 [details] [review]
Bug 24606: (QA follow-up) Fix QA script issues

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 36 Katrin Fischer 2022-11-01 14:37:10 UTC
Created attachment 142884 [details] [review]
Bug 24606: (QA follow-up) Update js message translation style

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 37 Katrin Fischer 2022-11-01 14:37:16 UTC
Created attachment 142885 [details] [review]
Bug 24606: (QA follow-up) Fix up interface

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 38 Katrin Fischer 2022-11-01 14:37:22 UTC
Created attachment 142886 [details] [review]
Bug 24606: (QA follow-up) Hide button when using multi-add

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 39 Katrin Fischer 2022-11-01 14:37:28 UTC
Created attachment 142887 [details] [review]
Bug 24606: (QA follow-up) Disable 'Apply template' button if no template is selected

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 40 Katrin Fischer 2022-11-01 14:37:33 UTC
Created attachment 142888 [details] [review]
Bug 24606: (QA follow-up) Remove string variable

This string is no longer needed as the __() is used
in the .js file directly.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 41 Katrin Fischer 2022-11-01 14:42:03 UTC
I think this a nice addition and have passed QA on this. 

BUT: The GUI still needs a little bit more work, so this can shine on release:
https://snipboard.io/IaM4VW.jpg 

I noticed that it reloads the page every time you apply or clean a template, it might be a nice improvement to rely on an API call here in the future.
Comment 42 Kyle M Hall 2022-11-02 17:15:35 UTC
Created attachment 143020 [details] [review]
Bug 24606: (QA follow-up) Fix up save form a bit
Comment 43 Kyle M Hall 2022-11-09 18:17:10 UTC
Created attachment 143569 [details] [review]
Bug 24606: Update database, add new schema file

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 44 Kyle M Hall 2022-11-09 18:17:30 UTC
Created attachment 143570 [details] [review]
Bug 24606: Add Koha Object(s) with unit tests

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 45 Kyle M Hall 2022-11-09 18:17:34 UTC
Created attachment 143571 [details] [review]
Bug 24606: Implement item templates

This patch set implements item editor templates for community Koha.

Test Plan:
1) Apply this patch set
2) Run updatedatabase.pl
3) Restart all the things!
4) prove t/db_dependent/Koha/Item/Template*
5) As a non superlibrarian, enter the item editor
6) Set some item fields, save as a new template using the buttom and
   form below the editor.
7) Test loading a template without remembering for the session
8) Test loading a template while remembering for the session
9) Test deleting a template
10) Test updating a template
11) Create one or more shared templates
12) Log in as another non superlibrarian without the new permission manage_item_editor_templates,
    verify you cannot edit/delete templates shared to you
13) Enable the new permission manage_item_editor_templates,
    verify you can now edit and delete templates shared to you

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 46 Kyle M Hall 2022-11-09 18:17:38 UTC
Created attachment 143572 [details] [review]
Bug 24606: Add item template editor permission

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 47 Kyle M Hall 2022-11-09 18:17:43 UTC
Created attachment 143573 [details] [review]
Bug 24606: (QA follow-up) Set existing sharing setting when updating existing template

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 48 Kyle M Hall 2022-11-09 18:17:47 UTC
Created attachment 143574 [details] [review]
Bug 24606: (QA follow-up) Fix QA script issues

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 49 Kyle M Hall 2022-11-09 18:17:52 UTC
Created attachment 143575 [details] [review]
Bug 24606: (QA follow-up) Update js message translation style

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 50 Kyle M Hall 2022-11-09 18:17:56 UTC
Created attachment 143576 [details] [review]
Bug 24606: (QA follow-up) Fix up interface

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 51 Kyle M Hall 2022-11-09 18:18:00 UTC
Created attachment 143577 [details] [review]
Bug 24606: (QA follow-up) Hide button when using multi-add

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 52 Kyle M Hall 2022-11-09 18:18:05 UTC
Created attachment 143578 [details] [review]
Bug 24606: (QA follow-up) Disable 'Apply template' button if no template is selected

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 53 Kyle M Hall 2022-11-09 18:18:12 UTC
Created attachment 143579 [details] [review]
Bug 24606: (QA follow-up) Remove string variable

This string is no longer needed as the __() is used
in the .js file directly.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 54 Kyle M Hall 2022-11-09 18:18:16 UTC
Created attachment 143580 [details] [review]
Bug 24606: (QA follow-up) Fix up save form a bit
Comment 55 Kyle M Hall 2022-11-09 18:18:22 UTC
Created attachment 143581 [details] [review]
Bug 24606: Update existing schema files [DO NOT PUSH]
Comment 56 Tomás Cohen Arazi 2022-11-10 17:51:26 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 57 Jonathan Druart 2022-11-11 14:03:04 UTC
https://snipboard.io/0uEloj.jpg - shouldn't we hide if there is no template defined?
Comment 58 Jonathan Druart 2022-11-11 14:04:40 UTC
Use statements appear twice
+use URI::Escape qw( uri_escape_utf8 );
+use URI::Escape qw( uri_escape_utf8 );
Comment 59 Jonathan Druart 2022-11-11 14:16:53 UTC
There is an encoding issue when you are applying a template that has ♥ in one of its subfield

Wide character in subroutine entry at /kohadevbox/koha/Koha/Item/Template.pm line 58

    in Koha::Item::Template::decoded_contents at /kohadevbox/koha/Koha/Item/Template.pm line 58
Comment 60 Tomás Cohen Arazi 2022-11-11 18:43:02 UTC
Created attachment 143794 [details] [review]
Bug 24606: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 61 Tomás Cohen Arazi 2022-11-11 18:43:41 UTC
Created attachment 143795 [details] [review]
Bug 24606: Fix encoding issues on decoded content

This patch makes the `decoded_contents` method UTF-8 encode the data
before calling the relevant decode_json, as expected by the JSON library
[1].

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Item/Template.t
=> FAIL: Tests fail!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!

[1] `Any decoding or encoding (e.g. to UTF-8 or UTF-16) needs to be done
yourself, e.g. using the Encode module.` https://metacpan.org/pod/JSON

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 62 Tomás Cohen Arazi 2022-11-11 18:49:17 UTC
Created attachment 143796 [details] [review]
Bug 24606: (QA follow-up) Remove duplicate include
Comment 63 Tomás Cohen Arazi 2022-11-11 19:55:39 UTC
(In reply to Jonathan Druart from comment #58)
> Use statements appear twice
> +use URI::Escape qw( uri_escape_utf8 );
> +use URI::Escape qw( uri_escape_utf8 );

Done.

(In reply to Jonathan Druart from comment #59)
> There is an encoding issue when you are applying a template that has ♥ in
> one of its subfield
> 
> Wide character in subroutine entry at /kohadevbox/koha/Koha/Item/Template.pm
> line 58

Done.
Comment 64 Tomás Cohen Arazi 2022-11-11 19:56:55 UTC
(In reply to Jonathan Druart from comment #57)
> https://snipboard.io/0uEloj.jpg - shouldn't we hide if there is no template
> defined?

Kyle, this is not strictly a blocker, but we really need you to fix it for the release. Separate bug please.
Comment 65 Katrin Fischer 2022-11-13 22:34:55 UTC
(In reply to Tomás Cohen Arazi from comment #64)
> (In reply to Jonathan Druart from comment #57)
> > https://snipboard.io/0uEloj.jpg - shouldn't we hide if there is no template
> > defined?
> 
> Kyle, this is not strictly a blocker, but we really need you to fix it for
> the release. Separate bug please.

Filed it as: Bug 32188 - Only show template controls above item form if templates have been defined
Comment 66 Marcel de Rooy 2022-11-16 15:25:11 UTC
The following follow-up looks highly arguable to me:

Bug 24606: Fix encoding issues on decoded content

-    return decode_json( $self->contents ) if $self->contents;
+    return decode_json( encode_utf8($self->contents) ) if $self->contents;

Note a few lines above (in ->store):
    if ( ref( $self->contents ) eq 'HASH' ) {
        $self->contents( encode_json( $self->contents ) );
    }

The reference to "Do it yourself" in the commit message is not very specific and out of its context.

Since we are storing strings to the database here, I think we should refer the utf8 encoding to that layer. And we should encode/decode in the middle of the process.

So I would quote another part of the JSON docs:
utf8 flag disabled
When utf8 is disabled (the default), then encode/decode generate and expect Unicode strings, that is, characters with high ordinal Unicode values (> 255) will be encoded as such characters, and likewise such characters are decoded as-is, no changes to them will be done, except "(re-)interpreting" them as Unicode codepoints or Unicode characters, respectively (to Perl, these are the same thing in strings unless you do funny/weird/dumb stuff).
This is useful when you want to do the encoding yourself (e.g. when you want to have UTF-16 encoded JSON texts) or when some other layer does the encoding for you (for example, when printing to a terminal using a filehandle that transparently encodes to UTF-8 you certainly do NOT want to UTF-8 encode your data first and have Perl encode it another time).

What does practically mean?
You should switch from using decode_json and encode_json TO from_json and to_json. Leave the strings in Perl internal format during the process.

encode_json Converts the given Perl data structure to a UTF-8 encoded, binary string.
to_json Converts the given Perl data structure to a Unicode string by default. 

This needs further attention imo. (But cant fail it anymore :)
Comment 67 Marcel de Rooy 2022-11-16 15:26:01 UTC
And we should encode/decode in the middle of the process.

And we should NOT
Comment 68 Tomás Cohen Arazi 2022-11-16 17:11:57 UTC
(In reply to Marcel de Rooy from comment #66)
> What does practically mean?
> You should switch from using decode_json and encode_json TO from_json and
> to_json. Leave the strings in Perl internal format during the process.

The docs and you say the same: decode_json expects to be passed a string with Perl's internal format representing UTF-8 characters. And that's what encode_utf8 does, and why the patch introduces it at that exact place. 

> This needs further attention imo. (But cant fail it anymore :)

You can't fail what is correct :-P

The cool thing is that you can propose some other solution. But I'd vote against something that adds more steps to the caller.
Comment 69 Tomás Cohen Arazi 2022-11-16 17:15:25 UTC
(In reply to Marcel de Rooy from comment #66)
> encode_json Converts the given Perl data structure to a UTF-8 encoded,
> binary string.
> to_json Converts the given Perl data structure to a Unicode string by
> default. 

"
This module also exports to_json and from_json for backward compatibility. These are slower
"
Comment 70 Jonathan Druart 2022-11-17 06:39:41 UTC
We are using a cookie when localStorage should be used, see bug 12410.
Comment 71 Jonathan Druart 2022-11-17 06:45:16 UTC
(In reply to Tomás Cohen Arazi from comment #69)
> (In reply to Marcel de Rooy from comment #66)
> > encode_json Converts the given Perl data structure to a UTF-8 encoded,
> > binary string.
> > to_json Converts the given Perl data structure to a Unicode string by
> > default. 
> 
> "
> This module also exports to_json and from_json for backward compatibility.
> These are slower
> "

It's not working, so it's wrong.

[2022/11/17 06:44:05] [WARN] The job has not been sent to the message broker: (Wide character in syswrite at /usr/lib/x86_64-linux-gnu/perl-base/IO/Handle.pm line 200.

To recreate, see bug 31351
Comment 72 Marcel de Rooy 2022-11-17 06:46:58 UTC
(In reply to Tomás Cohen Arazi from comment #68)
> The docs and you say the same: decode_json expects to be passed a string
> with Perl's internal format representing UTF-8 characters. And that's what
> encode_utf8 does, and why the patch introduces it at that exact place. 

Perl internal format is certainly not UTF-8. Perhaps you are saying that here, but you could well mean something else too.. Not sure what you mean. But it is clear to me that we should not use decode_json here.

> You can't fail what is correct :-P
That is true. But this is absolutely not correct ;)

> The cool thing is that you can propose some other solution. But I'd vote
> against something that adds more steps to the caller.
If the right thing would take one more step, you should just take it.
Comment 73 Marcel de Rooy 2022-11-17 06:47:44 UTC
The correct pattern is in BackgroundJob:

sub decoded_data {
    my ($self) = @_;

    return $self->data ? $self->json->decode( $self->data ) : undef;
}

sub set_encoded_data {
    my ( $self, $data ) = @_;

    return $self->data( $data ? $self->json->encode($data) : undef );
}
Comment 74 Marcel de Rooy 2022-11-17 06:48:17 UTC
(In reply to Tomás Cohen Arazi from comment #69)
> This module also exports to_json and from_json for backward compatibility.
> These are slower

Thats true. We should switch to OO interface.
Comment 75 Marcel de Rooy 2022-11-17 06:49:36 UTC
From Backgrount too

sub json {
    my ( $self ) = @_;
    $self->{_json} //= JSON->new->utf8(0); # TODO Should we allow_nonref ?
    return $self->{_json};
}

I remember that you were objecting against it too. But it is the right approach. Do not set utf8, but keep everything in Perl internal format in the middle of the process..
Comment 76 Marcel de Rooy 2022-11-17 07:03:52 UTC
(In reply to Jonathan Druart from comment #71)
> [2022/11/17 06:44:05] [WARN] The job has not been sent to the message
> broker: (Wide character in syswrite at
> /usr/lib/x86_64-linux-gnu/perl-base/IO/Handle.pm line 200.
> 
> To recreate, see bug 31351

Bug 31351 has been pushed.
How do you recreate the problem? And explain the connection to this report please. This is different code?
Comment 77 Jonathan Druart 2022-11-17 07:40:41 UTC
(In reply to Marcel de Rooy from comment #76)
> (In reply to Jonathan Druart from comment #71)
> > [2022/11/17 06:44:05] [WARN] The job has not been sent to the message
> > broker: (Wide character in syswrite at
> > /usr/lib/x86_64-linux-gnu/perl-base/IO/Handle.pm line 200.
> > 
> > To recreate, see bug 31351
> 
> Bug 31351 has been pushed.
> How do you recreate the problem? And explain the connection to this report
> please. This is different code?

Different code? The first patch is replacing encode_json calls with encode, and remove the utf8 encoding :)

To recreate, batch mod items, add ❤ in the note, submit the job, go to the job detail => it fails
go to the koha logs => see the "Wide char" error
Comment 78 Marcel de Rooy 2022-11-17 07:59:07 UTC Comment hidden (obsolete)
Comment 79 Jonathan Druart 2022-11-17 09:01:46 UTC
For the record, my comments didn't make sense. I hadn't noticed that Tomas was not modifying Koha::BackgroundJob.
Comment 80 Marcel de Rooy 2022-11-17 09:21:42 UTC
(In reply to Marcel de Rooy from comment #78)
> (In reply to Jonathan Druart from comment #77)
> > (In reply to Marcel de Rooy from comment #76)
> > > (In reply to Jonathan Druart from comment #71)
> > > > [2022/11/17 06:44:05] [WARN] The job has not been sent to the message
> > > > broker: (Wide character in syswrite at
> > > > /usr/lib/x86_64-linux-gnu/perl-base/IO/Handle.pm line 200.
> > > > 
> > > > To recreate, see bug 31351
> > > 
> > > Bug 31351 has been pushed.
> > > How do you recreate the problem? And explain the connection to this report
> > > please. This is different code?
> > 
> > Different code? The first patch is replacing encode_json calls with encode,
> > and remove the utf8 encoding :)
> > 
> > To recreate, batch mod items, add ❤ in the note, submit the job, go to the
> > job detail => it fails
> > go to the koha logs => see the "Wide char" error
> 
> Okay. You actually mean that Tomas is right.
> I am on it today. Please wait a bit.

We complicated this discussion a bit by mixing two separate issues.
Comment 81 Marcel de Rooy 2022-11-17 09:26:29 UTC
Opened bug 32242 for the Background issue now.
Comment 82 Marcel de Rooy 2022-11-17 09:42:26 UTC
Tomás: I will add a patch here for your consideration now. (By way of exception :)
Comment 83 Marcel de Rooy 2022-11-17 10:17:09 UTC
Created attachment 143989 [details] [review]
Bug 24606: (QA follow-up) Fix encoding/decoding

We should use OO interface and leave encoding to database
layer.

Test plan:
Add a new item template containing real Unicode chars.
Save. Apply on new item.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 84 Marcel de Rooy 2022-11-17 10:17:16 UTC
Created attachment 143990 [details] [review]
Bug 24606: (QA follow-up) Add test for encoding/decoding

NOTE: I have the impression that this unit test is quite
rudimentary. I do not see the apply function tested?

Test plan:
Run t/db_dependent/Koha/Item/Templates.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 85 Marcel de Rooy 2022-11-18 07:15:07 UTC
Changed status to get attention from RM for last two follow-ups. Thx.
Comment 86 Tomás Cohen Arazi 2022-11-18 14:01:30 UTC
(In reply to Marcel de Rooy from comment #85)
> Changed status to get attention from RM for last two follow-ups. Thx.

Thanks for the follow-ups, Marcel.
Comment 87 Marcel de Rooy 2022-11-22 15:43:32 UTC
Template.t fails now. But the code is wrong in the test. I'll have a look.
Comment 88 Marcel de Rooy 2022-11-22 15:50:50 UTC
Created attachment 144161 [details] [review]
Bug 24606: (QA follow-up) Fix Template.t

The test cases here still assumed UTF-8 enocding in the middle
of the process. Can be much simpler.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 89 Tomás Cohen Arazi 2022-11-22 16:34:20 UTC
(In reply to Marcel de Rooy from comment #88)
> Created attachment 144161 [details] [review] [review]
> Bug 24606: (QA follow-up) Fix Template.t
> 
> The test cases here still assumed UTF-8 enocding in the middle
> of the process. Can be much simpler.
> 
> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Follow-up pushed, thanks!