Skip to content
This repository has been archived by the owner on Feb 23, 2022. It is now read-only.

Add Feature: On-site modal checkout #26

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

adiled
Copy link

@adiled adiled commented Apr 10, 2019

Feature works by switching the external redirect at checkout to btcpay.showInvoice call

Expected flow

If activated in settings

  • On clicking Proceed to Checkout button, payment modal pops up
  • If modal is closed before completing payment, checkout page is reloaded
  • If payment is made and modal is closed, the page redirects as it would in external server checkout page.

4/2019 Changelog

  • Include btcpay.js script in assets/js
  • Enqueue btcpay.js script
  • Add wp-ajax custom endpoint for retrieving order status in context of btcpay invoice
  • Enqueue script for appropriately handling modal close using btcpay.onModalWillLeave and custom wp-ajax endpoint
  • Add wp settings based checkbox[yes,no] option to btcpay settings page for feature toggle

Please excuse the bulky commit, I had made the changes in distribution build and then ported to repo.

Credits to @astupidmoose for sponsoring and live testing

@NicolasDorier
Copy link
Member

Awesome, I know that @astupidmoose worked on it also.

However, I think you should remove the btcpay.js from here and instead cross reference the one configured on the btcpayserver configured for the store. By doing so, you don't need to use setApiUrlPrefix at all. Also, bug fixes on btcpayserver will not need to make a new plugin version.

Copy link
Member

@Kukks Kukks left a comment

Choose a reason for hiding this comment

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

Thanks for the PR :) In the future try and avoid formatting the files differently as it causes the diff to be harder to read.

@@ -0,0 +1,135 @@
/* jshint browser: true, strict: false, maxlen: false, maxstatements: false */
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to include this file here. Each btcpay host ships with this js file e.g. https://btcpay.kukks.org/modal/btcpay.js

function enqueue_modal_js()
{
$ajax_url = admin_url( 'admin-ajax.php' );
wp_enqueue_script( 'btcpay-modal', plugin_dir_url( __FILE__ ) . 'assets/js/btcpay.js', array( 'jquery' ), null, false );
Copy link
Member

Choose a reason for hiding this comment

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

use remote btcpay script file

wp_enqueue_script( 'btcpay-modal', plugin_dir_url( __FILE__ ) . 'assets/js/btcpay.js', array( 'jquery' ), null, false );

wp_add_inline_script( 'btcpay-modal',
"btcpay.setApiUrlPrefix('".get_option('woocommerce_btcpay_url')."')
Copy link
Member

Choose a reason for hiding this comment

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

You do not need this if you load the script remotely.

@astupidmoose
Copy link
Contributor

I could be wrong as I didn't actually code it, but I think it was required to bring in the btcpay.js for the closing and redirection functionality. Else, that functionality would be needed to be added into btcpay.js in the master repo.

When calling it remotely, the window was not closing properly and would not redirect on payment success.

@rockstardev
Copy link
Member

Hey @astupidmoose what we talked about doesn't apply here. It is really design question - do we want separate btcpay.js as part of woocommerce plugin or we should keep that modal file in BTCPayServer repository.

I discussed with Mike about us providing paid event... right now the way modal library works is that it only provides close event and then on clientside you should query server and ensure invoice is paid before redirecting.

@adiled
Copy link
Author

adiled commented Apr 10, 2019

@Kukks @NicolasDorier

btcpay.js is indeed a redundancy that I missed documenting while my review. I have exposed a function in it to retrieve id of the invoice being shown, which for the purpose @astupidmoose pointed out, enables the status checking of order without making cross origin requests. Would you suggest a PR on btcpayserver? I suppose that would cause a btcpayserver version dependency for the feature though.

@rockstardev
Copy link
Member

I think we should have paid event in btcpay.js... then after redirection merchant can server-side verify that invoice is paid before confirming order. We just leave comment in JS library to ensure that people don't auto-ship orders on Javascript events that can easily be faked.

@Kukks
Copy link
Member

Kukks commented Apr 10, 2019

@m-adilshaikh You can print the invoice id as a js var as well though which would give you global access and not require you to depend on that event to give you the id

@adiled
Copy link
Author

adiled commented Apr 10, 2019

@Kukks Yes I was expecting to enqueue order details but the flow is boxed up with WC Gateway API, the request goes async and response is handled by checkout.js I believe, couldn't figure where to intercept.

@adiled
Copy link
Author

adiled commented Apr 10, 2019

@Kukks Actually I just realized I am already calling a js function as redirect... I will look into this.

@NicolasDorier
Copy link
Member

So do we need to modify btcpay.js on btcpayserver to make things simpler?

@adiled
Copy link
Author

adiled commented Apr 12, 2019

@NicolasDorier Not required after d76a23d

@astupidmoose
Copy link
Contributor

astupidmoose commented Apr 13, 2019

@m-adilshaikh I can't get this latest PR to install.

Parse error: syntax error, unexpected '}', expecting ';' in /var/www/html/wp-content/plugins/woocommerce-btcpay/class-wc-gateway-btcpay.php on line 628

@astupidmoose
Copy link
Contributor

Tried installing now

Plugin could not be activated because it triggered a fatal error.

End up getting this error:
Fatal error: Uncaught Exception: The BTCPay payment plugin was not installed correctly or the files are corrupt. Please reinstall the plugin. If this message persists after a reinstall, contact the BTCPay team through http://slack.btcpayserver.org with this message. in /var/www/html/wp-content/plugins/btcpay-for-woocommerce/class-wc-gateway-btcpay.php:35 Stack trace: #0 /var/www/html/wp-admin/includes/plugin.php(2050): include() #1 /var/www/html/wp-admin/plugins.php(175): plugin_sandbox_scrape('btcpay-for-wooc...') #2 {main} thrown in /var/www/html/wp-content/plugins/btcpay-for-woocommerce/class-wc-gateway-btcpay.php on line 35

@NicolasDorier
Copy link
Member

Any idea @m-adilshaikh ?

@NicolasDorier
Copy link
Member

ping @m-adilshaikh (also need rebase)

@Kukks
Copy link
Member

Kukks commented May 10, 2019

What's the status here @m-adilshaikh? Shall we review again? :)

@astupidmoose
Copy link
Contributor

I need to check this out again. The original plugin @m-adilshaikh sent me works and is live in production, but I think it somehow re-added the libs and css file that were previously stripped. The updated versions do not work however.

I'm not sure how. I need to setup another test site to check the latest version.

@astupidmoose
Copy link
Contributor

astupidmoose commented Jul 31, 2020

This code is pretty old and there was a bunch of flaws with it. I think modal checkout would be useful, but I don't think it's safe to push this PR through.

Not sure why it was "approved"

remove built css file

modal checkout: add global variable for invoice id at checkout, use for validation

fix get_btcpay_redirect_modal
@adiled
Copy link
Author

adiled commented Jul 31, 2020

I have rebased and cleaned up the mess of reformatting I made, now I recall this had issues but we were very close, it was pending manual testing, I may have not committed the revisions after review. Perhaps someone can take it from here, I've lost context.

@adiled adiled marked this pull request as draft July 31, 2020 10:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants