laravel-shopify
laravel-shopify copied to clipboard
gnikyt
Fixed app-bridge redirect on fullpage_redirect auth view
Summary
Shopify is deprecating EASDK and the package was making making an EASDK calls (ie Shopify.API.remoteRedirect). This triggered the message for the apps consuming the library.
Closes #1094
What was done?
This PR refactors the EASDK call to redirect the app made on the auth/fullpage_redirect.blade.php view to use App Bridge instead.
This is exactly what I did too, and mine works. I'm hoping that what is referenced in this comment (https://github.com/osiset/laravel-shopify/issues/1094#issuecomment-1055579657) will help establish if there is an issue still or this resolves it.
This makes this exactly the same as is found in koa-shopify-auth, so can't imagine this being a problem.
https://github.com/Shopify/koa-shopify-auth/blob/32517fae738234e6159c0e699bfcdffb0bcded11/src/auth/redirection-page.ts
*edit, unless the issue will still come up because it uses window['app-bridge'], that is.
Can you fix the CI @olavoasantos ?
Fixed the related issue. FYI, there's a test failing locally that has nothing to do with my changes (BillingControllerTest::testSendsShopToBillingScreen). I'm guessing it's something with my local environment(?)
This code do not redirect user to the new full page , instead that it open a page into SHopify admin in iframe... How to redirect to full page?
Just an extra check in to say that I implemented this on my live app, and the warning has now disappeared, suggesting that this is entirely fit for purpose.
~~I'm wondering if the full page redirect is even needed anymore. As my PR (#1075) works by just redirecting straight to the url we need to. So it could be we reduce the overhead of having to redirect to a page just to do a redirect.~~
~~It might need to be test by someone other than me though - @lucasmichot / @olavoasantos can you give it a go?~~
Update: it is still needed.
I think we have two solutions for the same issue. @Kyon147 's PR (#1075) is an alternative to this solution as it also removes the deprecated call. So I guess we'd need to choose between them. @lucasmichot @osiset Could you evaluate the two approaches? It's important to move forward with one of these solutions soon.
Hey @olavoasantos - this is still needed and would love to get this merged in and get a release out. There's one test failing and wondered if you could take a look?
Hey. Sorry... I've been busy the last little while and I haven't been able to fix the tests. I'll try to get to this asap.
Hey. Sorry... I've been busy the last little while and I haven't been able to fix the tests. I'll try to get to this asap.
No problem at all, we all have lives outside of here - totally understand 👍 thanks for taking a look!
Hey @olavoasantos - just checking to see if you have time for this, as we can try and get it out the door with the Laravel 9 support now @osiset is good with the PR once ready.
Hey @olavoasantos - just checking to see if you have time for this, as we can try and get it out the door with the Laravel 9 support now @osiset is good with the PR once ready.
i'd be happy to pick this up. kind of need it for our app anyway so makes sense.
I am good with the code itself, but have not tested. Looks like the tests may need to be updated to expect a new flow?
Yeah the tests need to be updated a little - @stevesweets did you want to make these changes to the tests?
Yeah the tests need to be updated a little - @stevesweets did you want to make these changes to the tests?
after offering, i immediately got flooded in some other tasks. on it tomorrow am (gmt)
Yeah the tests need to be updated a little - @stevesweets did you want to make these changes to the tests?
after offering, i immediately got flooded in some other tasks. on it tomorrow am (gmt)
No worries, let me know if you can't and I'll try fit it into my schedule at some point this month...