exception_notification icon indicating copy to clipboard operation
exception_notification copied to clipboard

Published 20 hours ago • verified publisher icon smartinez87

Make backtrace cleaning optional

Open vala opened this issue 8 years ago • 7 comments

This is a new take of the previous https://github.com/smartinez87/exception_notification/pull/242 pull request by @chancancode.

I borrowed the tests and adapted the implementation he wrote at the time to work with the current gem's code.

But, to make the tests pass, I had to add the ability to clean the backtracke to the #background_exception_notification since the EmailNotifier tests are written with the background notification.

This seems bad to me, since we wouldn't have the ability to disable backtrace cleaning only for the controller-exception backed email notifier.

Which way should I go ? Write a separate test class to test the exception with a controller exception ?

This brings be to think about the whole backtrace cleaning strategy, is it really useful ? Can't we just, in the HTML e-mail, differenciate app and gem backtrace lines ?

Thanks for your feedback !

vala avatar Feb 26 '16 14:02 vala

Before fixing the failing test for https://travis-ci.org/smartinez87/exception_notification/builds/112021045, I'd like that we address the questions in my PR ! Thanks !

vala avatar Feb 26 '16 15:02 vala

hi @vala, thanks for this.

Maybe we can go with writing a separate test class to test the exception with a controller exception for now, to have the feature available.

smartinez87 avatar Mar 10 '16 14:03 smartinez87

Can you also please add an entry to the changelog with this, mentioning you and @chancancode as the authors?

smartinez87 avatar Mar 10 '16 14:03 smartinez87

Hello. This has been open for over a year. Anything I can do to help get it merged? cc @vala @smartinez87

monfresh avatar Nov 02 '17 20:11 monfresh

If you want to take the PR over don't hesitate, I'm unsure I'll be able to work on that soon. Sorry for the lack of follow-up on this

vala avatar Nov 03 '17 10:11 vala

@monfresh Would you mind taking care of this PR if it's still a desired feature for you. Otherwise, I'll try to move this forward.

Thank you

FLarra avatar Jan 30 '19 19:01 FLarra

I discovered this PR while looking for any existing issue or PR for adding backtrace cleaning for background_exception_notification. Currently backtrace cleaning is only done in the exception_notification that is called as part of a web request, but I couldn't understand why anyone wouldn't also want clean backtraces for background exception notification as well to be consistent.

So I am glad to find this PR makes both notifications clean backtraces and hope it gets merged soon.

TylerRick avatar May 21 '19 17:05 TylerRick