Bug 32242 - The job has not been sent to the message broker: (Wide character in syswrite ... )
Summary: The job has not been sent to the message broker: (Wide character in syswrite ...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on: 31351
Blocks:
  Show dependency treegraph
 
Reported: 2022-11-17 09:25 UTC by Marcel de Rooy
Modified: 2023-12-28 20:43 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00, 22.05.08


Attachments
Bug 32242: Resolve encoding issue when sending to Net::Stomp (1.52 KB, patch)
2022-11-17 09:34 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32242: Resolve encoding issue when sending to Net::Stomp (1.72 KB, patch)
2022-11-17 09:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32242: Resolve encoding issue when sending to Net::Stomp (1.73 KB, patch)
2022-11-17 11:47 UTC, David Nind
Details | Diff | Splinter Review
Bug 32242: Add selenium tests (3.45 KB, patch)
2022-11-17 13:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32242: Resolve encoding issue when sending to Net::Stomp (1.78 KB, patch)
2022-11-17 17:46 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 32242: Add selenium tests (3.50 KB, patch)
2022-11-17 17:46 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 32242: [22.05] fix selenium tests for 22.05 (1.54 KB, patch)
2022-12-21 16:42 UTC, Lucas Gass
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2022-11-17 09:25:25 UTC
The full version:

[2022/11/17 08:51:39] [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.) at /usr/share/koha/Koha/BackgroundJob.pm line 147.
Comment 1 Marcel de Rooy 2022-11-17 09:27:07 UTC
I think we should encode via Net::Stomp but I will first add a quick fix.
Comment 2 Marcel de Rooy 2022-11-17 09:34:12 UTC
Created attachment 143986 [details] [review]
Bug 32242: Resolve encoding issue when sending to Net::Stomp

This is meant as a quick fix. It should be enhanced later on by
letting Net::Stomp do the UTF8 conversion.

Test plan:
Run batch mod items. Add a Unicode char in the notes.
Check if job runs and results are correct.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 3 Marcel de Rooy 2022-11-17 09:38:53 UTC
FYI Tomas: The json to UTF8 conversion was handled correctly at the database layer but this bug is about sending json to RabbitMQ. That conversion is not handled properly (yet). See also bug 32243.
Comment 4 Marcel de Rooy 2022-11-17 09:49:54 UTC
Created attachment 143987 [details] [review]
Bug 32242: Resolve encoding issue when sending to Net::Stomp

This is meant as a quick fix. It should be enhanced later on by
letting Net::Stomp do the UTF8 conversion.

Test plan:
Run batch mod items. Add a Unicode char in the notes.
Check if job runs and results are correct.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 5 Marcel de Rooy 2022-11-17 09:50:33 UTC
Still added a use Encode
Comment 6 David Nind 2022-11-17 11:47:59 UTC
Created attachment 143994 [details] [review]
Bug 32242: Resolve encoding issue when sending to Net::Stomp

This is meant as a quick fix. It should be enhanced later on by
letting Net::Stomp do the UTF8 conversion.

Test plan:
Run batch mod items. Add a Unicode char in the notes.
Check if job runs and results are correct.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Nind <david@davidnind.com>
Comment 7 David Nind 2022-11-17 11:56:09 UTC
Testing notes (using KTD):

1. Use Batch item modification (Cataloging > Batch editing) to edit two records:
   - Add a Unicode character, such as 〼, in the non-public note and public note fields
2. View the details of job - no records modified
3. Check the items - no change was made
4. Check the /var/log/koha/kohadev/plack-intranet-error.log - there is a warning:
[2022/11/17 11:43:13] [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/Hand    le.pm line 200.
5. Apply the patch (and restart all the things)
6. Repeat steps 1-4 - the records should be successfully modified and there should be no warning message in the logs.
Comment 8 Jonathan Druart 2022-11-17 12:07:40 UTC
It's fixing the problem.

Any good reasons to not import encode_utf8 from Encode?
Comment 9 Marcel de Rooy 2022-11-17 12:40:35 UTC
(In reply to Jonathan Druart from comment #8)
> It's fixing the problem.
> 
> Any good reasons to not import encode_utf8 from Encode?

I didnt need to import it. Main reason ;)
Comment 10 Jonathan Druart 2022-11-17 12:51:11 UTC
That's not a discussion for this bug report, but I thought we agreed on the following pattern:

use Module qw( sub list );
then call Module::list() of list()
Comment 11 Marcel de Rooy 2022-11-17 12:55:21 UTC
(In reply to Jonathan Druart from comment #10)
> That's not a discussion for this bug report, but I thought we agreed on the
> following pattern:
> 
> use Module qw( sub list );
> then call Module::list() of list()

It is conform that agreement with the list being empty ;)
Comment 12 Jonathan Druart 2022-11-17 13:34:09 UTC
Created attachment 143998 [details] [review]
Bug 32242: Add selenium tests

This is not testing the thing at the correct level, but at least we test
the whole workflow.
Comment 13 Tomás Cohen Arazi 2022-11-17 17:46:08 UTC
Created attachment 144016 [details] [review]
Bug 32242: Resolve encoding issue when sending to Net::Stomp

This is meant as a quick fix. It should be enhanced later on by
letting Net::Stomp do the UTF8 conversion.

Test plan:
Run batch mod items. Add a Unicode char in the notes.
Check if job runs and results are correct.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 14 Tomás Cohen Arazi 2022-11-17 17:46:13 UTC
Created attachment 144017 [details] [review]
Bug 32242: Add selenium tests

This is not testing the thing at the correct level, but at least we test
the whole workflow.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 15 Tomás Cohen Arazi 2022-11-17 17:47:35 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 16 Tomás Cohen Arazi 2022-11-18 20:09:31 UTC
This broke tests:

kohadev-koha@kohadevbox:/kohadevbox/koha$ prove t/db_dependent/Koha/BackgroundJobs.t
t/db_dependent/Koha/BackgroundJobs.t .. 1/15
    #   Failed test 'No jobs found without userenv'
    #   at t/db_dependent/Koha/BackgroundJobs.t line 135.
    #          got: '2'
    #     expected: '0'
    # Looks like you failed 1 test of 3.

#   Failed test 'search_limited'
#   at t/db_dependent/Koha/BackgroundJobs.t line 142.
...

Reverting makes tests pass:

git revert 022034a1c7b91adac539c7ed16fc7b48d566912e

Please take your time to fix it for the release.
Comment 17 Tomás Cohen Arazi 2022-11-18 23:22:20 UTC
(In reply to Tomás Cohen Arazi from comment #16)
> This broke tests:
> 
> kohadev-koha@kohadevbox:/kohadevbox/koha$ prove
> t/db_dependent/Koha/BackgroundJobs.t
> t/db_dependent/Koha/BackgroundJobs.t .. 1/15
>     #   Failed test 'No jobs found without userenv'
>     #   at t/db_dependent/Koha/BackgroundJobs.t line 135.
>     #          got: '2'
>     #     expected: '0'
>     # Looks like you failed 1 test of 3.
> 
> #   Failed test 'search_limited'
> #   at t/db_dependent/Koha/BackgroundJobs.t line 142.
> ...
> 
> Reverting makes tests pass:
> 
> git revert 022034a1c7b91adac539c7ed16fc7b48d566912e
> 
> Please take your time to fix it for the release.

I was wrong. My bisect was flawed because of the reset_all I was using.
Comment 18 Marcel de Rooy 2022-11-21 09:35:29 UTC
I am seeing this now Tomas, but it should not be related to this report:

# Subtest: purge
    1..9
    ok 1 - Only the old finished type1 job would be purged
    not ok 2 - All finished old jobs would be purged with job_types = all
    #   Failed test 'All finished old jobs would be purged with job_types = all'
    #   at t/db_dependent/Koha/BackgroundJobs.t line 162.
    #          got: '5'
    #     expected: '2'
    ok 3 - All jobs still left in queue
    ok 4 - Only the old finished type1 job is purged
    ok 5 - 3 jobs still left in queue
    not ok 6 - The remaining old finished jobs is purged
    #   Failed test 'The remaining old finished jobs is purged'
    #   at t/db_dependent/Koha/BackgroundJobs.t line 183.
    #          got: '4'
    #     expected: '1'
    ok 7 - 2 jobs still left in queue
    ok 8 - Unfinished job still left in queue
    ok 9 - Recent finished job still left in queue
    # Looks like you failed 2 tests of 9.
not ok 15 - purge
Comment 19 Marcel de Rooy 2022-11-21 09:46:38 UTC
(In reply to Marcel de Rooy from comment #18)
>     not ok 2 - All finished old jobs would be purged with job_types = all
>     #   Failed test 'All finished old jobs would be purged with job_types =
> all'
>     #   at t/db_dependent/Koha/BackgroundJobs.t line 162.
>     #          got: '5'
>     #     expected: '2'

It seems that this test assumes that there are no other old jobs. So this should be corrected by removing them all first ?
Opened bug 32304.
Comment 20 Marcel de Rooy 2022-11-25 08:22:33 UTC
(In reply to Tomás Cohen Arazi from comment #17)
> > kohadev-koha@kohadevbox:/kohadevbox/koha$ prove
> > t/db_dependent/Koha/BackgroundJobs.t
> > t/db_dependent/Koha/BackgroundJobs.t .. 1/15
> >     #   Failed test 'No jobs found without userenv'
> >     #   at t/db_dependent/Koha/BackgroundJobs.t line 135.
> >     #          got: '2'
> >     #     expected: '0'
> >     # Looks like you failed 1 test of 3.
> > 
> > #   Failed test 'search_limited'
> > #   at t/db_dependent/Koha/BackgroundJobs.t line 142.

Added a fix for this one on bug 32304 too now.
Comment 21 Jonathan Druart 2022-12-05 11:29:27 UTC
This should have been backported into 22.05.x
Comment 22 Lucas Gass 2022-12-21 16:42:07 UTC
Created attachment 144772 [details] [review]
Bug 32242: [22.05] fix selenium tests for 22.05
Comment 23 Lucas Gass 2022-12-21 16:47:17 UTC
Backported to 22.05.x for upcoming 22.05.08 release
Comment 24 Arthur Suzuki 2022-12-22 11:21:17 UTC
This one seems not to apply to 21.11 branch since it depends on 31351 (only in 22.05.x).
Won't backport