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

Add default value to Configuration.scope parameter to make it optional #1913

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

zzooeeyy
Copy link
Contributor

@zzooeeyy zzooeeyy commented Oct 4, 2024

Relates to

What this PR does

Fixes:

Background

The scope parameter was required as a part of the legacy OAuth flow (Authorization code grant).

  • It used to be a required parameter during the ShopifyAPI::Context setup process, but has been changed to be optional from this PR
  • ShopifyApp.configuration does not have explicit expectation for config.scope to be configured, but it will encounter an error during OAuth from ShopStrategy

shopify_error

Solution

Making ShopifyApp.configuration.scope to have a default value [] so it can be omitted during setup.

How has this been tested?

  • Linked a local version of ShopifyApp with my changes to my app
  • Removed config.scope setup in the ShopifyApp.configure method
  • Start installation process and observe no errors:
    • With Shopify managed install configured: config.scope values are ignored during installation
    • With Shopify managed install disabled: config.scope values are used during installation

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.

@zzooeeyy zzooeeyy marked this pull request as ready for review October 4, 2024 20:05
@zzooeeyy zzooeeyy requested a review from a team as a code owner October 4, 2024 20:05
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.

2 participants