|
Description
Nick Clemens (kidclamp)
2020-02-06 18:26:42 UTC
Created attachment 141613 [details] [review] Bug 24606: Update database, add new schema file Created attachment 141614 [details] [review] Bug 24606: Add Koha Object(s) with unit tests Created attachment 141615 [details] [review] Bug 24606: Update existing schema files [DO NOT PUSH] 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 Created attachment 141617 [details] [review] Bug 24606: Add item template editor permission 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] Created attachment 141717 [details] [review] Bug 24606: Update database, add new schema file Created attachment 141718 [details] [review] Bug 24606: Add Koha Object(s) with unit tests 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 Created attachment 141720 [details] [review] Bug 24606: Add item template editor permission Created attachment 141721 [details] [review] Bug 24606: Update existing schema files [DO NOT PUSH] 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. 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. Created attachment 141972 [details] [review] Bug 24606: (QA follow-up) Set existing sharing setting when updating existing template Created attachment 142003 [details] [review] Bug 24606: Update database, add new schema file Signed-off-by: David Nind <david@davidnind.com> Created attachment 142004 [details] [review] Bug 24606: Add Koha Object(s) with unit tests Signed-off-by: David Nind <david@davidnind.com> 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> Created attachment 142006 [details] [review] Bug 24606: Add item template editor permission Signed-off-by: David Nind <david@davidnind.com> Created attachment 142007 [details] [review] Bug 24606: Update existing schema files [DO NOT PUSH] Signed-off-by: David Nind <david@davidnind.com> 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> 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? Created attachment 142833 [details] [review] Bug 24606: (QA follow-up) Fix QA script issues Created attachment 142834 [details] [review] Bug 24606: (QA follow-up) Update js message translation style Created attachment 142835 [details] [review] Bug 24606: (QA follow-up) Fix up interface Created attachment 142836 [details] [review] Bug 24606: (QA follow-up) Hide button when using multi-add Created attachment 142837 [details] [review] Bug 24606: (QA follow-up) Disable 'Apply template' button if no template is selected (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! Back here. 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> 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> 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> 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> 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> 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> Created attachment 142883 [details] [review] Bug 24606: (QA follow-up) Fix QA script issues Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> 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> Created attachment 142885 [details] [review] Bug 24606: (QA follow-up) Fix up interface Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> 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> 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> 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> 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. Created attachment 143020 [details] [review] Bug 24606: (QA follow-up) Fix up save form a bit 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> 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> 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> 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> 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> Created attachment 143574 [details] [review] Bug 24606: (QA follow-up) Fix QA script issues Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> 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> Created attachment 143576 [details] [review] Bug 24606: (QA follow-up) Fix up interface Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> 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> 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> 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> Created attachment 143580 [details] [review] Bug 24606: (QA follow-up) Fix up save form a bit Created attachment 143581 [details] [review] Bug 24606: Update existing schema files [DO NOT PUSH] Pushed to master for 22.11. Nice work everyone, thanks! https://snipboard.io/0uEloj.jpg - shouldn't we hide if there is no template defined? Use statements appear twice +use URI::Escape qw( uri_escape_utf8 ); +use URI::Escape qw( uri_escape_utf8 ); 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
Created attachment 143794 [details] [review] Bug 24606: Regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> 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> Created attachment 143796 [details] [review] Bug 24606: (QA follow-up) Remove duplicate include (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. (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. (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 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 :) And we should encode/decode in the middle of the process. And we should NOT (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. (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 " We are using a cookie when localStorage should be used, see bug 12410. (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 (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. 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 );
}
(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. 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..
(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? (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 (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. For the record, my comments didn't make sense. I hadn't noticed that Tomas was not modifying Koha::BackgroundJob. (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. Opened bug 32242 for the Background issue now. Tomás: I will add a patch here for your consideration now. (By way of exception :) 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> 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> Changed status to get attention from RM for last two follow-ups. Thx. (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. Template.t fails now. But the code is wrong in the test. I'll have a look. 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> (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! |
