-
Notifications
You must be signed in to change notification settings - Fork 23
Add timeouts and an enable/disable switch #16
Add timeouts and an enable/disable switch #16
Conversation
Wasn't being used and slows things down.
Store these configs in the database so that they can be applied to all app servers instantly and without restarting.
4abb0e1
to
0f94bad
Compare
Note: Having the enable/disable switch inside of solidus_avatax in this way feels a bit weird to me, but I'm OK with it if others are. I could also see us just trying to make it really easy to add your own enable/disable logic inside of the applications themselves if people thought that was better. |
tax_svc.canceltax(params) | ||
end | ||
end | ||
|
||
def tax_svc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this become private?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yah it could but I was thinking it's probably nice to be able to access this from a console & etc, or from custom code if someone using Avatax is using an endpoint that we're not covering.
looking good to me! and nice cleanups along the way. |
@@ -11,6 +13,17 @@ class << self | |||
attr_accessor :sales_invoice_generate_error_handler | |||
attr_accessor :sales_invoice_commit_error_handler | |||
attr_accessor :sales_invoice_cancel_error_handler | |||
|
|||
def timeout | |||
(config = last) ? config.timeout : DEFAULT_TIMEOUT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes me just a tad uncomfortable to use last
without an order
. I know that this will order by id by default, but personally I'd love to be more explicit about this. Not a blocker, just a thought.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'm fine with adding a most_recent
helper method or something like that.
Nice work, thank you. 👍 |
@cbrunsdon or @jhawthorn or @gmacdougall any thoughts on the general approach here before I start doing tweaks & cleanups? |
More explicit name and more explicit order clause And add some comments.
Explicitly focus the timeout code on Avatax API calls to discourage it from being used for other purposes.
bd49267
to
4142a5c
Compare
👍 🚢 🇮🇹 |
…itch Add timeouts and an enable/disable switch
PR for #15.
cc @magnusvk