search

boomerdigital / solidus_avatax_certified

Improve your Solidus store's sales tax decision automation with Avalara AvaTax
BSD 3-Clause "New" or "Revised" License
9 stars 44 forks source link

Fix NameError error when using Spring #160

Closed sbader closed 2 years ago

sbader commented 4 years ago

We're encountering an issue with this gem when using it with Spring. Specifically, when Spring reloads due to changes in the application, and the SolidusAvataxCertified::OrderAdjuster is called (as part of the tax adjustment calculation), we receive a NameError:

NameError (uninitialized constant SolidusAvataxCertified::Spree::Tax)

I was able to more easily reproduce this without Spring by testing with the sample application here: https://github.com/sbader/solidus_avatax_certified_sandbox.

When the current version of the gem is used, running bin/rails console and accessing the OrderAdjuster causes the NameError.

> bin/rails console
Loading development environment (Rails 6.0.3.3)
>> SolidusAvataxCertified::OrderAdjuster
Traceback (most recent call last):
        1: from (irb):1
NameError (uninitialized constant SolidusAvataxCertified::Spree::Tax)

When updating the gem to our fork with the fix: 

bin/rails console
Loading development environment (Rails 6.0.3.3)
>> SolidusAvataxCertified::OrderAdjuster
=> SolidusAvataxCertified::OrderAdjuster

Not sure if there's any good way to reproduce this in an automated test, but if there are any suggestions I'd be happy to try.

blocknotes commented 3 years ago

Same problem here, I was going to propose the same PR 😅 I hope it will be merged soon.

dhonig commented 3 years ago

Thanks @sbader we'll get this integrated as soon as we can sort out the CI situation.

sbader commented 3 years ago

@dhonig We're pretty far passed this Spring change, and are unfortunately relying on our fork to make this original change and now other things with Rails 6 work properly. Ideally we can get all of these changes in so we don't need to rely on the fork, but just let me know if you'd want me to change this so I'm not throwing all this in one PR.

dhonig commented 3 years ago

I've gone through the changes. I don't have a problem with them. But it looks like the Circle CI fails on postgres. If we don't think that the postgres failure is something that will affect users, then I'd be fine to merge this in now.

On Wed, Jan 20, 2021 at 12:35 PM Scott Bader notifications@github.com wrote:

@dhonig https://github.com/dhonig We're pretty far passed this Spring change, and are unfortunately relying on our fork to make this original change and now other things with Rails 6 work properly. Ideally we can get all of these changes in so we don't need to rely on the fork, but just let me know if you'd want me to change this so I'm not throwing all this in one PR.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/boomerdigital/solidus_avatax_certified/pull/160#issuecomment-763813871, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABZTL6CSZKQ6Y777BWCTVDS24H7TANCNFSM4RIAZOUA .

sbader commented 3 years ago

Yeah, looking at it I think there will need to be some updates to the spec setup to make it work properly with the changes to solidus_support (including the need to add solidus_dev_support).

I'm going to be working on some additional changes to our fork of this in a week or so. No promises, but I may be able to fix the tests at that time. Probably best to leave this open until after that.