Kyon147 / laravel-shopify

A full-featured Laravel package for aiding in Shopify App development
MIT License
353 stars 102 forks source link

Enhancement: Improve BillableMiddleware for SPAs #167

Open ggelashvili opened 1 year ago

ggelashvili commented 1 year ago

Overview

This PR enhances the Billable middleware to provide better support for SPAs by allowing AJAX requests to receive a JSON response with a redirect URL. Currently, Billable middleware and the billing_enabled configuration have no use for SPAs, & causes confusion which this PR aims to rectify.

Changes

The updated Billable middleware now checks if the incoming request is an AJAX request and, if billing is enabled, returns a 403 status code alongside a JSON response containing a forceRedirectUrl field. This field provides a URL to which the SPA can redirect the user. The updated middleware assumes that the host parameter is included in the request, which can be easily implemented using an axios interceptor (if using axios on front-end).

By returning a JSON response and a 403 status code, the front-end can now hook into the response interceptor, catch 403 errors, extract the redirect URL from the response, and manage redirection on the front-end. This makes the Billable middleware more useful for SPAs.

I added my own middleware in a project I'm working on with React front-end & worked perfectly, so thought it might be useful to improve the original Billable middleware.

Note that the wiki would need to be updated if this gets approved/merged.

This should not be a breaking change since Billable middleware should not be used anyways for traditional SPAs.

Kyon147 commented 1 year ago

@ggelashvili this looks good, can you fix the linting and I'll give this a test locally to see how the flow works in app.

ggelashvili commented 1 year ago

@Kyon147 fixed

Kyon147 commented 1 year ago

Hey @ggelashvili

Codcov has dropped a fair bit which suggests we need to add some tests - could you take a look.

It looks like we need to have test that covers this mainly.

if ($request->ajax()) {
return response()->json(
  ['forceRedirectUrl' => route(...$args)],
     403
  );
 }
ggelashvili commented 1 year ago

@Kyon147 I'll add the test(s) sometime next week

rvibit commented 1 year ago

@ggelashvili @Kyon147 can we return 402 instead of 403? As per Shopify standard Http Status Codes.

403 is ok but Shopify may return 403 for some other reasons in that case SPA must also check for redirect URL is available with the response or not.

Kyon147 commented 1 year ago

@rvibit 402 does make more sense based on the logic of this PR.

@ggelashvili your thoughts?

ggelashvili commented 1 year ago

Yea I think 402 makes sense. I'll make the change. Thanks @rvibit

rvibit commented 1 year ago

@Kyon147 and @ggelashvili I use this https://github.com/gnikyt/laravel-shopify/pull/1275 solution in my SPA and it works fine.

The solution in this PR is correct but if the already available middleware can handle the redirection automatically without redirecting from JS then why take extra overhead to check in my SPA that if the response is 402 then I have to redirect to charge page?

ggelashvili commented 1 year ago

@Kyon147 and @ggelashvili I use this gnikyt/laravel-shopify#1275 solution in my SPA and it works fine.

The solution in this PR is correct but if the already available middleware can handle the redirection automatically without redirecting from JS then why take extra overhead to check in my SPA that if the response is 402 then I have to redirect to charge page?

I don't think the redirection can be handled from middleware for SPAs and don't think it should be the responsibility of the middleware. It's better to delegate that responsibility to front-end, that way the developer decides what to do in such scenario, maybe instead of redirect they want to do something else first & then redirect, etc.

rvibit commented 1 year ago

@Kyon147 and @ggelashvili I use this gnikyt/laravel-shopify#1275 solution in my SPA and it works fine. The solution in this PR is correct but if the already available middleware can handle the redirection automatically without redirecting from JS then why take extra overhead to check in my SPA that if the response is 402 then I have to redirect to charge page?

I don't think the redirection can be handled from middleware for SPAs and don't think it should be the responsibility of the middleware. It's better to delegate that responsibility to front-end, that way the developer decides what to do in such scenario, maybe instead of redirect they want to do something else first & then redirect, etc.

that's a good point, I didn't realize that there could be more things the dev wants to do other then just redirecting. The solution makes more sense to me now. Thanks

ggelashvili commented 11 months ago

@Kyon147 do I need to make any further changes here or are we good?