Bug 15734

Summary: Audio Alerts broken
Product: Koha Reporter: Kyle M Hall <kyle>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Tomás Cohen Arazi <tomascohen>
Severity: normal    
Priority: P5 - low CC: fridolin.somers, jonathan.druart, josef.moravec, martin.renvoize, nick, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22378
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 13618, 19655    
Bug Blocks: 15733    
Attachments: Bug 15734 - Audio Alerts issues in master
Bug 15734 - Audio Alerts broken
[SIGNED-OFF]Bug 15734: Audio Alerts broken
Bug 15734: Audio Alerts broken
Bug 15734: Fix filters on AUDIO_ALERTS
Bug 15734: Update missing filters test for json
Bug 15734: Fix filters on AUDIO_ALERTS
Bug 15734: Update missing filters test for json
Bug 15734: Fix filters on AUDIO_ALERTS
Bug 15734: Update missing filters test for json
Bug 15734: Fix filters on AUDIO_ALERTS
Bug 15734: Update missing filters test for json
[ALTERNATIVE-PATCH] Bug 15734: Use To.json to filter audio alerts
Bug 15734: Use To.json to filter audio alerts
Bug 15734: Use To.json to filter audio alerts

Description Kyle M Hall 2016-02-03 19:36:53 UTC
From Nick:
In master there is a JSON error thrown when AudioAlerts are enabled.  When checking in I get:
SyntaxError: JSON.parse: expected property name or '}' at line 1 column 3 of the JSON data

This does not happen in 3.22
Comment 1 Kyle M Hall 2016-02-03 19:40:08 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2016-02-03 19:41:29 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2016-02-04 10:21:07 UTC
*** Bug 15733 has been marked as a duplicate of this bug. ***
Comment 4 Héctor Eduardo Castro Avalos 2016-02-04 14:56:28 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2016-02-08 11:16:31 UTC
Created attachment 47735 [details] [review]
Bug 15734: Audio Alerts broken

In master there is a JSON error thrown when AudioAlerts are enabled.
This is caused by changes in master from bug 13618.

Test Plan:
1) Enable audio alerts
2) Note audio alerts don't work
3) Apply this patch
4) Note audio alerts now work

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>
Works as described

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 6 Jonathan Druart 2016-02-12 09:39:12 UTC
Depends on bug 13618, which has been reverted from master.
Comment 7 Josef Moravec 2018-10-05 07:01:32 UTC
As bug 13618 is pushed in other way, this bug is here again
Comment 8 Josef Moravec 2018-10-05 07:03:09 UTC
Created attachment 79997 [details] [review]
Bug 15734: Fix filters on AUDIO_ALERTS

Test Plan:
1) Enable audio alerts
2) Note audio alerts don't work
3) Apply this patch
4) Note audio alerts now work
Comment 9 Josef Moravec 2018-10-05 07:03:14 UTC
Created attachment 79998 [details] [review]
Bug 15734: Update missing filters test for json

0) Apply only the first patch of this bug report
1) Run qa tools on the first patch - should return missing filters on
AUDIO_ALERTS line
2) Apply this patch
3) Run qa test tools on both patches - should be OK
Comment 10 Kyle M Hall 2018-10-05 12:02:15 UTC
Created attachment 80052 [details] [review]
Bug 15734: Fix filters on AUDIO_ALERTS

Test Plan:
1) Enable audio alerts
2) Note audio alerts don't work
3) Apply this patch
4) Note audio alerts now work

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 11 Kyle M Hall 2018-10-05 12:02:26 UTC
Created attachment 80053 [details] [review]
Bug 15734: Update missing filters test for json

0) Apply only the first patch of this bug report
1) Run qa tools on the first patch - should return missing filters on
AUDIO_ALERTS line
2) Apply this patch
3) Run qa test tools on both patches - should be OK

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 12 Owen Leonard 2018-10-05 12:23:44 UTC
Created attachment 80056 [details] [review]
Bug 15734: Fix filters on AUDIO_ALERTS

Test Plan:
1) Enable audio alerts
2) Note audio alerts don't work
3) Apply this patch
4) Note audio alerts now work

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 13 Owen Leonard 2018-10-05 12:23:48 UTC
Created attachment 80057 [details] [review]
Bug 15734: Update missing filters test for json

0) Apply only the first patch of this bug report
1) Run qa tools on the first patch - should return missing filters on
AUDIO_ALERTS line
2) Apply this patch
3) Run qa test tools on both patches - should be OK

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 14 Katrin Fischer 2018-10-06 19:27:26 UTC
Created attachment 80164 [details] [review]
Bug 15734: Fix filters on AUDIO_ALERTS

Test Plan:
1) Enable audio alerts
2) Note audio alerts don't work
3) Apply this patch
4) Note audio alerts now work

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 15 Katrin Fischer 2018-10-06 19:27:31 UTC
Created attachment 80165 [details] [review]
Bug 15734: Update missing filters test for json

0) Apply only the first patch of this bug report
1) Run qa tools on the first patch - should return missing filters on
AUDIO_ALERTS line
2) Apply this patch
3) Run qa test tools on both patches - should be OK

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 16 Jonathan Druart 2018-10-08 19:47:54 UTC
I did not test but maybe we should use To.json instead
Comment 17 Jonathan Druart 2018-10-09 13:20:06 UTC
[Testing something]
Comment 18 Jonathan Druart 2018-10-09 13:49:00 UTC
Created attachment 80258 [details] [review]
[ALTERNATIVE-PATCH] Bug 15734: Use To.json to filter audio alerts

Test Plan:
1) Enable audio alerts
2) Note audio alerts don't work
3) Apply this patch
4) Note audio alerts now work

Use complex selectors to test:
  #circ_returns p.problem:contains('Not checked out.')
  #doc3 > #bd
Comment 19 Jonathan Druart 2018-10-09 13:50:53 UTC
I do not want to block this but wondering what would be the best approach.
We already have a To.json method so I think it would make sense to use it instead of the TT json filter.

Letting authors, testers and RM decide.
Comment 20 Jonathan Druart 2018-10-09 13:51:37 UTC
(In reply to Jonathan Druart from comment #18)
> Use complex selectors to test:
>   #circ_returns p.problem:contains('Not checked out.')
>   #doc3 > #bd

=> Both approaches seem to behave identically.
Comment 21 Josef Moravec 2018-10-09 14:03:49 UTC
Created attachment 80259 [details] [review]
Bug 15734: Use To.json to filter audio alerts

Test Plan:
1) Enable audio alerts
2) Note audio alerts don't work
3) Apply this patch
4) Note audio alerts now work

Use complex selectors to test:
  #circ_returns p.problem:contains('Not checked out.')
  #doc3 > #bd

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>


Current status: Signed Off
Comment 22 Josef Moravec 2018-10-09 14:06:31 UTC
(In reply to Jonathan Druart from comment #19)
> I do not want to block this but wondering what would be the best approach.
> We already have a To.json method so I think it would make sense to use it
> instead of the TT json filter.
> 
> Letting authors, testers and RM decide.

I like your version better:
 - it does not add new thinks to code
 - the code is more readable for me - as it removes the neccesarity of replace quotes
Comment 23 Tomás Cohen Arazi 2018-10-09 14:28:22 UTC
Created attachment 80277 [details] [review]
Bug 15734: Use To.json to filter audio alerts

Test Plan:
1) Enable audio alerts
2) Note audio alerts don't work
3) Apply this patch
4) Note audio alerts now work

Use complex selectors to test:
  #circ_returns p.problem:contains('Not checked out.')
  #doc3 > #bd

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 24 Nick Clemens (kidclamp) 2018-10-09 16:27:24 UTC
Awesome work all!

Pushed to master for 18.11
Comment 25 Martin Renvoize 2018-10-15 13:54:27 UTC
Pushed to 18.05.x for 18.05.05
Comment 26 Fridolin Somers 2018-11-09 07:17:00 UTC
Pushed to 17.11.x for 17.11.12