Description
Kyle M Hall
2016-02-03 19:36:53 UTC
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 |