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

Change how we check if app is embedded #1458

Merged
merged 4 commits into from
Jun 22, 2022
Merged

Conversation

mkevinosullivan
Copy link
Contributor

@mkevinosullivan mkevinosullivan commented Jun 20, 2022

WHY are these changes introduced?

We want to support unframed apps. Note that this only changes the redirect functionality within the app; there is no change to the frontend which is still using App Bridge v2.

WHAT is this pull request doing?

Using appBridgeUtils.isShopifyEmbedded() rather than manually checking window.top. isShopifyEmbedded is a more complex check that supports unframed.

Also, upgrade to App Bridge 3.1.1 (from 2.0.12) as this is required, in the redirect functionality only.

Closes https://github.com/Shopify/first-party-library-planning/issues/366

Reviewer's guide to testing

Test environment:

  • Ruby 3.1.2p20
  • Rails 7.0.3
  • Bundler 2.3.16
  • Node v18.3.0
  1. Clone this branch
  2. In a terminal, use Shopify CLI to create and configure a new rails app:
    SHOPIFY_CLI_STACKTRACE=1 shopify app create rails
  3. Change into the app directory
  4. In an editor, open the Gemfile. Modify the entry to shopify_app to point to this branch in your local environment:
    gem 'shopify_app', path: '~/src/github.com/Shopify/shopify_app'
    
    and run bundle install.
  5. Keep an eye on the output of the shopify app create rails command - you may need to run any commands that didn't complete successfully. For example, in my case, I needed to run bin/rails importmap:install, followed by bin/rails turbo:install stimulus:install, and bin/rails generate shopify_app --new-shopify-cli-app.
  6. Start your app server
    SHOPIFY_CLI_STACKTRACE=1 shopify app serve
  7. Open the ngrok url outputted by the app serve command
  8. You could add the code snippet below to app/assets/javascripts/shopify_app/redirect.js to see more output in browser console:
    console.log('***** DEBUG *****: app-bridge v3', {
      'targetInfo.url': targetInfo.url,
      'appBridgeUtils.isShopifyEmbedded()': appBridgeUtils.isShopifyEmbedded(),
    });

Test new installation flow

  1. In a browser, follow the ngrok url to install the app on a test store. It should look similar to this:
    https://82f1-72-141-27-79.ngrok.io/login?shop=my-dev-store.myshopify.com
    
  2. Expect the app is installed as normal to the test shop

Test the login page on shop already has the app installed

  1. Open the login url again
  2. Expect the app is installed as normal to the test shop

Test re-authentication. Ex: cookies is expired

  1. Open the database console
    bin/rails db
  2. Turn on headers, look at the entries in the shops table
    sqlite> .headers on
    sqlite> select * from shops;
    
  3. Delete the entry associated with the development store, e.g.,
    sqlite> delete from shops where id = 1;
    
  4. Open the embedded app from the apps list in your test shop
  5. Expect the app is opened as normal

Browsers

Perform the tests above for Chrome, Firefox and Safari. Note that you will likely need to disable any third-party cookie blocking that Firefox and/or Safari have enabled, as well as any "enhanced protection" settings (Firefox).

Checklist

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

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

@mkevinosullivan mkevinosullivan marked this pull request as ready for review June 21, 2022 20:22
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.

I had some issues tophatting, but the new code worked well as far as I could see. We can fix the other issues as fast follows.

Copy link
Member

@henrytao-me henrytao-me left a comment

Choose a reason for hiding this comment

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

LGTM

@mkevinosullivan mkevinosullivan merged commit 684d872 into main Jun 22, 2022
@mkevinosullivan mkevinosullivan deleted the kos/add_app_bridge_3 branch June 22, 2022 15:20
@shopify-shipit shopify-shipit bot temporarily deployed to rubygems July 4, 2022 20:31 Inactive
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.

None yet

3 participants