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
Created attachment 47620 [details] [review] Bug 15734 - Audio Alerts issues in master 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
Created attachment 47621 [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
*** Bug 15733 has been marked as a duplicate of this bug. ***
Created attachment 47644 [details] [review] [SIGNED-OFF]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
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>
Depends on bug 13618, which has been reverted from master.
As bug 13618 is pushed in other way, this bug is here again
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
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
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>
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>
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>
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>
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>
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>
I did not test but maybe we should use To.json instead
[Testing something]
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
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.
(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.
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
(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
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>
Awesome work all! Pushed to master for 18.11
Pushed to 18.05.x for 18.05.05
Pushed to 17.11.x for 17.11.12