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 a default value for the setup option. #25

Merged
merged 1 commit into from
May 19, 2015
Merged

Conversation

dylanahsmith
Copy link
Contributor

@kevinhughes27 & @EiNSTeiN- for review

Depends on and include pull #24, see the last commit for this pull request's changes

Problem

Right now we have boilerplate code generated for the omniauth setup option in shopify_app, which basically prevents us from making any improvements to it without updating all those apps. For instance, we have a fix_https method which seems to only exist for the purpose of working around old setup lambdas that don't prefix the url with http:// instead of https:// (e.g. commit aa533f0 fixed the setup lambda in the README).

Also, we should have a default just to make the gem simpler to use.

Solution

Use option :setup,... to set a default proc to use for the setup.

Possible Enhancements

We might want to consider allowing merchants to enter their shop name without the .myshopify.com suffix and having the setup handle that automatically for the authorization endpoint that just redirects to Shopify. For now I have just added a test to show how that can be done.

@kevinhughes27
Copy link

lgtm - I never even stopped to consider that this code was duped everywhere - nice fix!

@EiNSTeiN-
Copy link
Contributor

👍

dylanahsmith added a commit that referenced this pull request May 19, 2015
Add a default value for the setup option.
@dylanahsmith dylanahsmith merged commit 8c06113 into master May 19, 2015
@dylanahsmith dylanahsmith deleted the default-setup branch May 19, 2015 19:07
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.

3 participants