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

Enhancement: Improve BillableMiddleware for SPAs #167

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

ggelashvili
Copy link

@ggelashvili ggelashvili commented May 24, 2023

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.

…e request is ajax it will respond with the proper redirect URL
@Kyon147
Copy link
Owner

Kyon147 commented May 25, 2023

@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 ggelashvili marked this pull request as ready for review May 25, 2023 18:43
@ggelashvili
Copy link
Author

@Kyon147 fixed

@Kyon147
Copy link
Owner

Kyon147 commented Jun 14, 2023

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
Copy link
Author

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

@rvibit
Copy link
Contributor

rvibit commented Jul 10, 2023

@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
Copy link
Owner

Kyon147 commented Jul 12, 2023

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

@ggelashvili your thoughts?

@ggelashvili
Copy link
Author

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

@rvibit
Copy link
Contributor

rvibit commented Aug 28, 2023

@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?

@ggelashvili
Copy link
Author

@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
Copy link
Contributor

rvibit commented Aug 28, 2023

@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
Copy link
Author

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

Copy link

@mdzahid-pro mdzahid-pro left a comment

Choose a reason for hiding this comment

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

I have tested this code in my recent app and this working for me.
i have used reactjs for frontend

also for making it workable i have to use this code inside my axios interceptor

    const resInterceptor = axios.interceptors.response.use(response => response, async function (errors){

if(errors?.response?.status === 402 && errors?.response?.data?.forceRedirectUrl){
navigate(errors.response.data.forceRedirectUrl);
}

        return Promise.reject(errors);
    });
    
    I would like to request that please mention this in wiki and readme file also

asad561

This comment was marked as resolved.

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