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.

Sign up for GitHub

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

Jump to bottom

Fixes invalid typing of discount code errors #1205

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fixes invalid typing of discount code errors #1205

wants to merge 1 commit into from

Conversation

garethson
Copy link
Contributor

@garethson garethson commented Aug 28, 2023 •
edited

Description

Fixes #1202

It appears as though when #1153 was shipped, it set the errors type the ShopifyAPI::DiscountCode class to be T.nilable(T::Hash[T.untyped, T.untyped])) when it should have just been left as the type defined in ShopifyAPI::Rest::Base.

As reported by @martinsp, this results in an error when ShopifyAPI::Rest::Base attempts to @errors.errors << e.

Note that, specifically in 125b693, the nuance of the DiscountCode resource returning a list of serialized errors on a normal fetch was dealt with, but I think that PR then should have removed those explicit definitions from the DiscountCode class as I have here. The other solution may have been to actually hook in and create a ShopifyAPI::Rest::Base for the returned errors when listing DiscountCodes after a batch create, but that's a deeper change.

How has this been tested?

Tested manually and also added a test here. It's unclear to me how these tests are now auto-generated so I suspect my manually added test needs to go ... elsewhere?

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.

@garethson garethson force-pushed the fix-nil-discount-code-errors branch 2 times, most recently from 1626ad3 to a38e649 Compare August 28, 2023 20:45
@garethson garethson force-pushed the fix-nil-discount-code-errors branch 5 times, most recently from 2ec3952 to 0c21e9b Compare August 28, 2023 21:00
garethson
garethson commented Aug 28, 2023
View reviewed changes
test/rest/2023_07/discount_code_test.rb
@@ -324,4 +324,26 @@ def test_9()
end
end

sig do
Copy link
Contributor Author

@garethson garethson Aug 28, 2023 •
edited

Choose a reason for hiding this comment

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

I only put this one test in the latest API version, I doubt this is where this test belongs as it's supposed to be auto-generated. Seems to me that the auto-generation of these tests are missing some error cases.

@garethson garethson marked this pull request as ready for review August 28, 2023 21:09
@nelsonwittwer
Copy link
Contributor

Thanks for your contribution! Unfortunately, these REST resource files are auto generated upstream so we won't be able to merge this in as the next time we generate these resources your fixes will be reverted. Having said that, we can apply your fix upstream! We are working on a few other fixes in this area and will add these to our list!

@garethson
Copy link
Contributor Author

@nelsonwittwer Thanks! I'll watch for updates on this issue, appreciate you pulling this in. 👍

@garethson garethson requested a review from a team as a code owner December 1, 2023 19:20
@garethson garethson force-pushed the fix-nil-discount-code-errors branch 2 times, most recently from c128ae0 to 463ae2a Compare December 1, 2023 19:26
@garethson
Copy link
Contributor Author

@nelsonwittwer This has been rebased and everything is passing, is it possible to get this looked at?

Thanks!

@lizkenyon
Copy link
Contributor

Hi there 👋

Sorry for the sluggish responses on this. Tldr; we do want to get this merged.
We have a ton of upstream friction with the REST resources, and we would ideally make these changes upstream, like we outline in our contribution guidelines.

Though we are planning on merging this when the January API release goes out, and apply these changes.

@sle-c sle-c self-assigned this Feb 6, 2024
@sle-c sle-c mentioned this pull request Feb 7, 2024
Closed
5 tasks
@sle-c
Copy link
Contributor

sle-c commented Feb 14, 2024

hi @garethson , this is great! Would you mind updating the 2024-01 and 2024-04 versions? It'll be nice to apply changes to the latest versions too.

@garethson garethson force-pushed the fix-nil-discount-code-errors branch from 794067c to dbb913c Compare May 15, 2024 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@sle-c sle-c

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug in error handling in Shopify::Rest::Base#save
4 participants
@garethson @nelsonwittwer @lizkenyon @sle-c