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

Set access_scopes column to string by default #1636

Merged
merged 4 commits into from
Mar 21, 2023

Conversation

kirillplatonov
Copy link
Contributor

@kirillplatonov kirillplatonov commented Jan 6, 2023

What this PR does

Access scopes can't be nil anymore as it will trigger a sorbet error in ShopifyAPI 10+.

Parameter 'scope': Expected type T.any(String, T::Array[String]), got type NilClass
Caller: /usr/local/bundle/gems/sorbet-runtime-0.5.9878/lib/types/private/methods/call_validation.rb:117
Definition: /usr/local/bundle/gems/shopify_api-10.0.2/lib/shopify_api/auth/session.rb:52

Resolves #1411

Checklist

Before submitting the PR, please consider if any of the following are needed:

  • Update CHANGELOG.md if the changes would impact users
  • Update README.md, if appropriate.
  • Update any relevant pages in /docs, if necessary
  • For security fixes, the Disclosure Policy must be followed.

@kirillplatonov kirillplatonov changed the title Set access_scopes column to string by default Set access_scopes column to string by default Jan 6, 2023
@nelsonwittwer nelsonwittwer merged commit 5be04c3 into Shopify:main Mar 21, 2023
@vfonic
Copy link
Contributor

vfonic commented Jun 21, 2023

I understand I'm half a year late to the party. Just wanted to understand what's the general guidance on making such changes that are not backwards compatible?
For example, if I had an app which already had the access scopes migration (without default "" and null: true), how can I upgrade to the new gem version? Do I need to manually create a new migration adding default "" and null: false?

Is there any reason why this isn't the default guidance/workflow?

Also, unrelated to this particular issue, I strongly believe that @kirillplatonov should be paid for his work on Shopify projects. @nelsonwittwer, you're getting paid for your work, correct?
Why would @kirillplatonov volunteer to upgrade your infrastructure and the only benefit he'd get is to not have to fork the repo to have his fixes in? ...but I guess that's not the status quo, it's expected from outside developers to contribute for free... (that's why I stopped contributing)
https://github.com/Shopify/shopify_app/pulls?q=author%3Akirillplatonov

@vfonic
Copy link
Contributor

vfonic commented Jun 23, 2023

I just ran rails generate shopify_app and realized that these changes were not applied. It's been half a year since @kirillplatonov created this PR and more than 3 months since the PR was merged into main. What is going on?

EDIT: Not sure what the issue was, but bundle update shopify_app would not update shopify_app gem from 21.2.0 to 21.5.0. I tried specifying version restriction in several different ways, even removing '~> 21.2'. The only thing that worked was explicitly setting gem 'shopify_app', '~> 21.5'.

@kirillplatonov kirillplatonov deleted the access-scope-defaults branch October 11, 2023 21:46
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.

V19.0.1 - sorbet error?
3 participants