Log installments errors by default #235
Conversation
kennyadsl
force-pushed
the
kennyadsl/log-installments-errors-by-default
branch
from
c2324d2 to
40905bd
Compare
|
🔴 Linting failures are not related to this PR. |
| error_handler = SolidusSubscriptions.configuration.processing_error_handler | ||
|
|
||
| if error_handler | ||
| proc_or_method = error_handler.is_a?(Proc) ? error_handler : error_handler.method(:call) |
There was a problem hiding this comment.
Are we adding this arity check for compatibility with older versions? What's the purpose here?
I feel that this adds a bit of extra complexity (specially when comparing an arity of -2), and handling some logic which should not be the responsibility of this job.
Some users might also override the default class with one which requires both the installment and the error, in which case the arity would be 2 🤔
There was a problem hiding this comment.
I think you are right. I thought that an existing proc that only had one argument (like proc { |e| raise e }) wouldn't work but it seems that it handles not having the second argument without breaking. I'll remove the extra code, thanks!
There was a problem hiding this comment.
Yeah, IIR my meta-programming correctly, Procs do not arity check, whilst lambdas do 😉
There was a problem hiding this comment.
Oh, so we need to hope that people didn't use a lambda for their handler?
There was a problem hiding this comment.
Yes. But users who're upgrading should be able to deal with this right?
There was a problem hiding this comment.
Yes but if they didn't notice the change they'll end up with an exception in production, not ideal.
kennyadsl
force-pushed
the
kennyadsl/log-installments-errors-by-default
branch
from
40905bd to
ca3dcaf
Compare
lib/solidus_subscriptions/processing_error_handlers/rails_logger.rb
Outdated
Show resolved
Hide resolved
At the moment, by default, all installment prceissing errors are swallowed and there's no way for a developer to understand what's happening. Of course they can create a custom handler with the processing_error_handler configuration provided but usually, when the first errors happen, it's too late and those errors are lost. We are not raising errors of this job because if there's a retry mechanism in place for Active Job, the installment will be reprocessed twice. Once by Active Job and once by the installment retry mechanism already provided by the extension. Logging to Rails.error is a middle-ground that allows to intercept the message of the exception. Still, creating a custom handler based on the bug tracker/exception handler used is the suggested option here.
This way we can provide more flexibility on things that can be done when an error occurs. In the provided RailsLogger handler, we'll also print the ID of the installment.
kennyadsl
force-pushed
the
kennyadsl/log-installments-errors-by-default
branch
from
ca3dcaf to
0317e22
Compare
At the moment, by default, all installment processing errors are swallowed and there's no way for a developer to understand what's happening. Of course, they can create a custom handler with the
processing_error_handlerconfiguration provided but usually, when the first errors happen, it's too late and those errors are lost.We are not raising errors of this job because if there's a retry mechanism in place for Active Job, the installment will be reprocessed twice. Once by Active Job and once by the installment retry mechanism already provided by the extension.
Logging to
Rails.erroris a middle-ground that allows to intercept the message of the exception. Still, creating a custom handler based on the bug tracker/exception handler used is the suggested option here.This PR also changes the signature of the handler call method (or proc) to allow passing the installment along with the exception.