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 processing for OAuth flicker reduction #35

Merged
merged 1 commit into from
Aug 22, 2022

Conversation

mkevinosullivan
Copy link
Contributor

@mkevinosullivan mkevinosullivan commented Aug 6, 2022

WHY are these changes introduced?

With the addition of the embedded param coming from Shopify, we can optimize the processing of OAuth to avoid screen flickering (switching to full screen then back again to iFrame, etc.)

Fixes https://github.com/Shopify/first-party-library-planning/issues/393

WHAT is this pull request doing?

Added embedded_app_url to config to point to /ExitIframe, page in FE. Also added checking of embedded parameter from Shopify to determine if top-level or in an iframe. Will redirect (via App Bridge) to load app in iFrame if top-level.

Checklist

Note: once this PR is merged, it becomes a new release for this template.

@mkevinosullivan mkevinosullivan requested a review from a team as a code owner August 6, 2022 19:45
Copy link
Contributor

@paulomarg paulomarg left a comment

Choose a reason for hiding this comment

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

LGTM!

@geongeorge
Copy link

geongeorge commented Aug 14, 2022

I do not have access to your internal ticket to read more about this. it seems that when I tried this, I cannot set .embedded_app_url. Tried updating shopify_app gem. no luck

/Users/geongeorge/Work/storetools-apps/invoice/web/config/initializers/shopify_app.rb:23:in `block in <main>': undefined method `embedded_app_url=' for #<ShopifyApp::Configuration:0x0000000107dd2c20 @root_url="/api", @myshopify_domain="myshopify.com", @scripttags_manager_queue_name=nil, @webhooks_manager_queue_name=nil, @disable_webpacker=false, @webhooks=[{:topic=>"app/uninstalled", :address=>"api/webhooks/app_uninstalled"}], @application_name="My Shopify App", @old_secret="", @scope="write_products,read_orders,read_customers,read_draft_orders", @embedded_app=true, @after_authenticate_job=false, @api_version="2022-07", @reauth_on_access_scope_changes=true, @login_url="/api/auth", @login_callback_url="/api/auth/callback"> (NoMethodError)

      config.embedded_app_url = "/ExitIframe"
            ^^^^^^^^^^^^^^^^^^^
    Did you mean?  embedded_app=
                   embedded_app
                   embedded_app?

Added embedded_app_url to config to point to /ExitIframe, part of FE.
Also added checking of embedded parameter from Shopify to determine if
top-level or in an iframe.  Will redirect (via App Bridge) to load app
in iFrame if top-level.
@mkevinosullivan mkevinosullivan force-pushed the kos/add_exitiframe_route branch from 6332651 to 3f7545e Compare August 22, 2022 16:30
@mkevinosullivan mkevinosullivan merged commit 8fbce7d into main Aug 22, 2022
@mkevinosullivan mkevinosullivan deleted the kos/add_exitiframe_route branch August 22, 2022 16:35
@mkevinosullivan
Copy link
Contributor Author

@geongeorge It should work now ... it required a new version of the shopify_app gem that was just released this morning, and some updates to this PR too.

I do not have access to your internal ticket to read more about this. it seems that when I tried this, I cannot set .embedded_app_url. Tried updating shopify_app gem. no luck

/Users/geongeorge/Work/storetools-apps/invoice/web/config/initializers/shopify_app.rb:23:in `block in <main>': undefined method `embedded_app_url=' for #<ShopifyApp::Configuration:0x0000000107dd2c20 @root_url="/api", @myshopify_domain="myshopify.com", @scripttags_manager_queue_name=nil, @webhooks_manager_queue_name=nil, @disable_webpacker=false, @webhooks=[{:topic=>"app/uninstalled", :address=>"api/webhooks/app_uninstalled"}], @application_name="My Shopify App", @old_secret="", @scope="write_products,read_orders,read_customers,read_draft_orders", @embedded_app=true, @after_authenticate_job=false, @api_version="2022-07", @reauth_on_access_scope_changes=true, @login_url="/api/auth", @login_callback_url="/api/auth/callback"> (NoMethodError)

      config.embedded_app_url = "/ExitIframe"
            ^^^^^^^^^^^^^^^^^^^
    Did you mean?  embedded_app=
                   embedded_app
                   embedded_app?

@@ -9,8 +9,12 @@ class HomeController < ApplicationController
PROD_INDEX_PATH = Rails.public_path.join("dist")

def index
contents = File.read(File.join(Rails.env.production? ? PROD_INDEX_PATH : DEV_INDEX_PATH, "index.html"))
if ShopifyAPI::Context.embedded? && !params[:embedded].present? || params[:embedded] != "1"

Choose a reason for hiding this comment

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

I don't fully get the conditions here but I think there should be some brackets:
ShopifyAPI::Context.embedded? && (!params[:embedded].present? || params[:embedded] != "1")

Because false && false || true returns true while here I think we want it to return false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you ... fixed in the latest version on main.

@hrdwdmrbl
Copy link
Contributor

hrdwdmrbl commented Nov 28, 2022

Added embedded_app_url to config to point to /ExitIframe, page in FE.

What's FE?

@geongeorge
Copy link

@hrdwdmrbl Probably frontend

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.

5 participants