[Bug] Fixes session.create_permission_url() to omit scope param if not required
#757
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Modified `create_permission_url` to make `scope` optional, allowing it to be omitted when specified in the app's configuration (TOML). Updated the README to reflect this change and clarify usage. This improves flexibility and simplifies configuration management.
Removed an unnecessary extra space in the method signature of `create_permission_url`.
|
I have signed the CLA. |
|
hi @tylerj117, thanks for your contribution! Could you please update the tests in |
Updated tests to improve clarity and consistency in naming and arguments. Modified `create_permission_url` calls to match new positional order for `redirect_uri` and `scope`. Enhanced assertion coverage for edge cases like empty scopes and added tests for state parameter handling.
|
Apologies @Arkham;
|
|
@tylerj117 thanks! it seems there are just a couple of linting errors and then we can merge this. |
Removes extra white space in parameters in session_test.py and changes conditional formatting in shopify/session.py.
Removes extra white space in parameters in session_test.py and changes conditional formatting in shopify/session.py.
|
@Arkham Linting errors fixed, thanks! |
Arkham
approved these changes
Jan 20, 2025
|
Thanks for your contribution! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
WHY are these changes introduced?
To align with Shopify API Docs for 3xx redirects.
Fixes #756
3xx redirects for non-embedded apps using Shopify Managed Installation do not require scopes in the query parameter if provided by the app's TOML, per Shopify Docs.
WHAT is this pull request doing?
Adding scopes to auth_url query param only if provided in functional, otherwise, omitting it.
Checklist