V1.3 and up #36
V1.3 and up #36
Conversation
| @@ -1 +1 @@ | |||
| 1.3.0 | |||
| 2.0.0.rc1 | |||
There was a problem hiding this comment.
This should probably just be deleted because it isn't used anywhere anymore
| @@ -66,7 +66,11 @@ def self.customer_can_validate | |||
| end | |||
|
|
|||
| def is_true? | |||
| ActiveRecord::ConnectionAdapters::Column::TRUE_VALUES.include?(value) | |||
There was a problem hiding this comment.
This could probably be condensed to just
def is_true?
[true, 1, '1', 't', 'T', 'true', 'TRUE', 'on', 'ON'].include?(value)
endAlso curious as to where the variable value is defined.
There was a problem hiding this comment.
value is a column in the AvalaraPreference table
| @@ -22,7 +22,7 @@ def get_file_avalara_order | |||
| def erase_data | |||
| File.open("log/#{params['log_name']}.log", 'w') {} | |||
|
|
|||
| render nothing: true | |||
| render body: nil | |||
| @@ -63,14 +63,12 @@ def cache_key(order) | |||
|
|
|||
There was a problem hiding this comment.
Your cache key is very expensive, and it might be just easier to do without it. You are loading in Ruby all line items, all adjustments, the order, the shipments ... that's really a lot. Most of those propagate any changes to the order's updated at, so just use that.
There was a problem hiding this comment.
If we use updated_at it hits the API too many times. I just tested, going from delivery to payment in checkout, the difference between what we have now and changing it to check updated at and what we have now only hits the API once, and when I used updated_at, it hit the API four times.
There was a problem hiding this comment.
Manuel had worked on the cache key at a point in time before, trying to limit the api calls and complexity and this is what we have resulted with.
There was a problem hiding this comment.
It's then better to fetch the max(updated_at) out of the database directly for each of those collections - line items, shipments, etc ..
There was a problem hiding this comment.
Can this be created as an issue an tackled on another PR? This PR has to do with allowing the gem to work with v1.3 & up of solidus.
Gem works with solidus versions v1.3 & up
Need to delete branches v1.3, v1.4, & v2.0 after this is merged so there is no confusion.