Bug 15734 - Audio Alerts broken
Summary: Audio Alerts broken
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
: 15733 (view as bug list)
Depends on: 13618 19655
Blocks: 15733
  Show dependency treegraph
 
Reported: 2016-02-03 19:36 UTC by Kyle M Hall
Modified: 2019-03-15 22:54 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 15734 - Audio Alerts issues in master (1.27 KB, patch)
2016-02-03 19:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15734 - Audio Alerts broken (1.26 KB, patch)
2016-02-03 19:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF]Bug 15734: Audio Alerts broken (1.35 KB, patch)
2016-02-04 14:56 UTC, Héctor Eduardo Castro Avalos
Details | Diff | Splinter Review
Bug 15734: Audio Alerts broken (1.40 KB, patch)
2016-02-08 11:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15734: Fix filters on AUDIO_ALERTS (1.38 KB, patch)
2018-10-05 07:03 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 15734: Update missing filters test for json (1.18 KB, patch)
2018-10-05 07:03 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 15734: Fix filters on AUDIO_ALERTS (1.46 KB, patch)
2018-10-05 12:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15734: Update missing filters test for json (1.26 KB, patch)
2018-10-05 12:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15734: Fix filters on AUDIO_ALERTS (1.48 KB, patch)
2018-10-05 12:23 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 15734: Update missing filters test for json (1.29 KB, patch)
2018-10-05 12:23 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 15734: Fix filters on AUDIO_ALERTS (1.54 KB, patch)
2018-10-06 19:27 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 15734: Update missing filters test for json (1.34 KB, patch)
2018-10-06 19:27 UTC, Katrin Fischer
Details | Diff | Splinter Review
[ALTERNATIVE-PATCH] Bug 15734: Use To.json to filter audio alerts (1.46 KB, patch)
2018-10-09 13:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15734: Use To.json to filter audio alerts (1.51 KB, patch)
2018-10-09 14:03 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 15734: Use To.json to filter audio alerts (1.57 KB, patch)
2018-10-09 14:28 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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 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