Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade json_api_client gem #158

Merged
merged 56 commits into from
Oct 5, 2018
Merged

Conversation

adamdullenty
Copy link

@adamdullenty adamdullenty commented Sep 5, 2018

Related ticket: #157

This PR upgrades the json_api_client gem from 1.1.1 to 1.5.3 and makes relevant changes to support changes in the gem.

Note that the matalan-rails-site will not be compatible with this version - work needs to be done to bring this up to date (some issues exist with nested line items and out-of-date fixtures)

Deployment

  • Merge this PR and release an updated version of the gem - e.g. v0.6.50
  • Update this platform PR to point to this gem
  • Merge the platform PR into a release and carry out full system QA

super(klass)
self.connection = connection
self.path = path
def initialize(klass, opts = {})
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some changes to bring us in line with interface changes within the json_api_client gem we're extending

@@ -1,50 +0,0 @@
require "json_api_client/version"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dont we require this patch anymore?

@adamdullenty
Copy link
Author

Updating the matalan-rails-site to use the latest updates to the gem (it was locked to an older version and I missed this) has uncovered a whole new list of JSON parsing errors in the F/E test suite over the weekend.

I've decided to remove the gzip aspect of this work (this wasn't in the original ticket) and to add a separate ticket for that work. I'm taking this as a reminder not to let extra features sneak into tickets and nipping these things in the bud earlier in the development process.

@adamdullenty
Copy link
Author

I was seeing these specs fail:

bundle exec rspec ./spec/integration/tax_code_spec.rb:98 # FlexCommerce::TaxCode Creating a new tax code with invalid attributes should returns error messages
bundle exec rspec ./spec/integration/tax_code_spec.rb:150 # FlexCommerce::TaxCode Updating a tax code with invalid attributes should returns error messages

with the following error

image

This meant that the error details were returned against a key of :base instead of :code:

image

I originally thought this was due to this change in the json_api_client gem: JsonApiClient/json_api_client#277

However it was actually due to the fact we are overriding JsonApiClient::Resource which has had changes applied to it which we were missing. Adding these changes resolves the issue.

# @returns shipping methods with promotions applied
def shipping_methods
@shipping_methods ||= ShippingMethodsForCart.new(cart: cart, shipping_methods: shipping_method_model.all).call.sort_by(&:total)
end

def validate_shipping_method
unless cart.shipping_method_id.nil? || shipping_methods.any? {|sm| sm.id == cart.shipping_method_id} then
raise ::FlexCommerce::PaypalExpress::Exception::AccessDenied.new(I18n.t("payment_setup.shipping_method_not_available"))
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This translation wasn't loading and we were getting a translation missing error in the test runs. It was the only one in the locale file anyway, so I've moved it into the library. We could add another error class here as well (open to naming ideas)

@adamdullenty
Copy link
Author

adamdullenty commented Sep 25, 2018

Last issue on the list is the F/E specs failing with Not all prefix parameters specified:

image

I'm just looking through what's changed in json_api_client between 1.1.1 and 1.5.3 for anything which could be related to this, although it looks like there's already an unresolved ticket about this issue from 2016...? JsonApiClient/json_api_client#166

Edit: this is why #158 (comment)


# This allows us to set the `path` parameter automatically from the provided `container_id`
def self.path(params = nil, *args)
params["path"] = { "cart_id" => params["container_id"] } if params["container_id"]
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is required as we've removed the _prefix_path workaround to help with nested / multitenanted line items endpoint (here) which causes the upgraded json_api_client gem version to error.

We always have the container_id present so this saves us having to pass the path: { cart_id: ... } attribute everywhere.

The alternative is we make this a requirement and go through the F/E code adding path: { cart_id: ... } everywhere like we've done with nested coupons (and I'm not sure this would work if we're loading line items from a cart, updating them, then re-saving)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another fix would have been to update the platform to always include this path parameter in API responses. Not sure this is a good idea as we'd be adding it specifically for json_api_client but it isn't necessarily required for any other json API libraries.

errors.add(:base, error.title || error.detail)
end
end
fill_errors
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our patch needed updating to use the json_api_client gem's latest method of dealing with errors

@adamdullenty adamdullenty changed the title flex-ruby-gem - update json api client gem Upgrade json_api_client gem Oct 1, 2018
@adamdullenty adamdullenty merged commit 621299a into master Oct 5, 2018
praweb pushed a commit that referenced this pull request Feb 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants