search

gnikyt / laravel-shopify

A full-featured Laravel package for aiding in Shopify App development
MIT License
1.24k stars 374 forks source link

Fixed app-bridge redirect on fullpage_redirect auth view #1097

Closed olavoasantos closed 2 years ago

olavoasantos commented 2 years ago

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.

stevesweets commented 2 years ago

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.

olavoasantos commented 2 years ago

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(?)

olavoasantos commented 2 years ago

@lucasmichot @osiset Any updates on this?

gnikyt commented 2 years ago

I am out with covid atm will have to revisit when I can.

olavoasantos commented 2 years ago

@osiset Sorry to hear that. Hope you feel better soon.

APdevelopments commented 2 years ago

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?

stevesweets commented 2 years ago

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.

Kyon147 commented 2 years ago

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.

olavoasantos commented 2 years ago

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.

Kyon147 commented 2 years ago

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?

olavoasantos commented 2 years ago

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.

Kyon147 commented 2 years ago

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!

Kyon147 commented 2 years ago

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.

stevesweets commented 2 years ago

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.

gnikyt commented 2 years ago

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?

Kyon147 commented 2 years ago

Yeah the tests need to be updated a little - @stevesweets did you want to make these changes to the tests?

stevesweets commented 2 years ago

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)

Kyon147 commented 2 years ago

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...

stevesweets commented 2 years ago

@Kyon147 had to do a new pr

Kyon147 commented 2 years ago

Closing this PR as one was merged in that was finished by @stevesweets!