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

Fix CustomerAddress.all returning empty array #1192

Closed
wants to merge 3 commits into from

Conversation

cdmwebs
Copy link

@cdmwebs cdmwebs commented Aug 8, 2023

Description

Fixes #1018, #970

When fetching CustomerAddresses, the results are returned as addresses. This PR adds json_response_body_name to the class and updates tests.

How has this been tested?

Assertions added to count the returned results.

Checklist:

  • My commit message follow the pattern described in here
  • I have performed a self-review of my own code.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have updated the project documentation.
  • I have added a changelog line.


response = response.first if response.respond_to?(:first)

# Assert attributes are correctly typed preventing Sorbet errors downstream
if response.respond_to?(:original_state)
response&.original_state&.each do |key, value|
next if key == :default
Copy link
Author

Choose a reason for hiding this comment

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

I don't like this, but I'm not sure how to better handle. default is both an attribute and a method.

@cdmwebs cdmwebs marked this pull request as ready for review August 8, 2023 23:43
@cdmwebs
Copy link
Author

cdmwebs commented Aug 9, 2023

Upon further review, this isn't going to work. The keys are different between Customer and CustomerAddress. When fetching a Customer, the key is customer_addresses. When fetching CustomerAddress, it's address.

@cdmwebs cdmwebs marked this pull request as draft August 9, 2023 00:05
Previously, when fetching `all` on CustomerAddress it would return an
empty array. Two issues (Shopify#970, Shopify#1018) were opened outlining the problem.

Initial attempt was to override `json_response_body_name` which worked
for fetching arrays of results. It doesn't work for fetching individual
results though.

I wound up overriding `create_instances_from_response` for
CustomerAddress. It isn't ideal but it does seem to behave correctly.

Assertions have been added to the the tests as well.
@cdmwebs cdmwebs marked this pull request as ready for review August 17, 2023 18:33
@cdmwebs
Copy link
Author

cdmwebs commented Aug 17, 2023

Looking for comments here. If this makes sense, I'll update the other API versions as well.

@nelsonwittwer
Copy link
Contributor

Thanks for your contributions here! Unfortunately, we won't be able to use them as written as these resources are generated from a schema. Having said that, we can take this fix and apply it at the root layer so we don't regenerate these resources and lose your work.

We have plans to open source that REST resource generation logic to enable the community to be more proactive with these fixes, but for now we'll ask for a little more time as we make this fix. We have resources dedicated to closing out open issues, so it shouldn't be too much longer!

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.

ShopifyAPI::CustomerAddress.all always returns an empty array
2 participants