BCH integration - Design

shivaenigma commented 3 years ago

This is an issue to start thinking about design of BCH integration into prestashop. Please refer to woocommerce design and BCH WHMCS design

URL Structure and Navigation

The payment controller submit is currently a POST request. This creates an issue. If user goes to crypto selection screen and then comes back, browser will ask to resend data (This is not desirable)
Design of crypto selection URLs that follow prestashop controller design guidelines
Cart duplication and address reuse should work

DB Changes

What DB table changes are required and what is the migration strategy? We should always follow prestashop best practices

BCH Information

BCH address/txid information should be shown in invoices and other places where we are currently showing bitcoin information. Merchant should clearly find out that the invoice was paid via BTC/BCH and not get confused

Admin UI

Prestashop UI already allows for different section in settings (so we don't need to design tabs). A new section in Settings with checkboxes should work. Test Setup can be moved to this new section. Lets create mockup in prestashop so that we approve this

cnohall commented 3 years ago

Sounds good, thanks for the comments regarding the structure for the coming changes. I'm looking forward to realizing this

cnohall commented 3 years ago

This is a rough draft:

But, I wanted to check that I'm heading in the right direction for the new Admin UI. Please let me know your thoughts.

cnohall commented 3 years ago

Perhaps using switch like in the older versions of the module might be preferable? Otherwise, the currencies section looks quite empty.

cnohall commented 3 years ago

Regarding the Save and Test Setup buttons. Should they save the data that has been changed only in that section or all data on the page?

So for example, if a user changes Time Period and immediately clicks Test Setup, without clicking the Save button, should it be saved?

DarrenWestwood commented 3 years ago

Hi @cnohall, the UI with the switch looks good.

Regarding the Save and Test Setup buttons. Should they save the data that has been changed only in that section or all data on the page?

Let's keep this functionality separate for now. The tabs look independent and it appears clear which button belongs/operates each section.

Design of crypto selection URLs that follow prestashop controller design guidelines

We should try to copy the design of the URLs directly from the woocommerce plugin.

what is the migration strategy? We should always follow prestashop best practices

There is Prestashop built-in upgrade functionality to help handle updating the database table between versions. https://github.com/blockonomics/prestashop-plugin/tree/1.7/upgrade

shivaenigma commented 3 years ago

Few changes here

I actually prefer the checkbox style. The red in switch UI is catching the eye and BTC/BCH being disabled feels like an error which it is not
Test Setup should automatically Save all Settings first. Otherwise this will create problems like Test Setup giving stale output for the input APIKey

cnohall commented 3 years ago

One last question about buttons, just to be sure. How about if a user changes the BCH and BTC options and presses Save? Should the selected cryptos also be saved then? It's fairly easy to implement, just want to double-check so that we're on the same page.

To summarize the progress so far. This is the design at the moment:

I've connected the checkboxes so that they're saved in the Database in the ps_configuration table under the names BLOCKONOMICS_BTC and BLOCKONOMICS_BCH as booleans. I'm currently working on making the messages appear above the correct section (e.g. messages related to testSetup should appear above the Currencies section but below Settings section ). After that it's probably suitable to work on the crypto-selection screen.

Do you have any suggestions on how to organize the PRs? It seems like it may easily become a big PR. However, it's not all that clear on where we may split the code into several PRs.

shivaenigma commented 3 years ago

Do you have any suggestions on how to organize the PRs? It seems like it may easily become a big PR. However, it's not all that clear on where we may split the code into several PRs.

Yes, I agree this will be quite big. Lets first finalize the design. One thought could be do Test Setup first in a PR. Then we can do Payment Screen and Callbacks

cnohall commented 3 years ago

Sounds good. I have found a way of placing the messages above the correct the section. However, it requires both sections to be split into separate functions and quite many lines of code will have to be moved. As you said, it's probably best to create an Test Setup PR first.

This is how it looks like now:

DarrenWestwood commented 3 years ago

I agree we should handle the Admin UI and Test Setup in one PR, and the Checkout and DB changes in another.

The payment controller submit is currently a POST request. This creates an issue. If user goes to crypto selection screen and then comes back, browser will ask to resend data (This is not desirable)

The request to the validation controller cannot be changed to a GET request as the form method is set in the theme template files, however, using Tools::redirect() from the validation controller fixes the issues with refreshing and going back as the redirect is not added to the browser history.

URL Structure

Prestashop uses the cart to fetch the order and not the order id or any other parameters. This means the only additional parameter required to add to the URL is the crypto parameter.

There will be 3 Prestashop controllers.

Validation fc=module&module=blockonomics&controller=validation&id_lang=1
Payment crypto=btc&fc=module&module=blockonomics&controller=payment&id_lang=1
Select Crypto fc=module&module=blockonomics&controller=select_crypto&id_lang=1

The Validation controller will check how many cryptos are activated and redirect the user to the select crypto controller, or the payment controller with the additional crypto parameter added.

$blockonomics = $this->module;
$active_cryptos = $this->getActiveCryptos();
// Check how many crypto currencies are activated
if (count($active_cryptos) > 1) {
    Tools::redirect($this->context->link->getModuleLink($blockonomics->name, 'select_crypto', array(), true));
} elseif (count($active_cryptos) === 1) {
    Tools::redirect($this->context->link->getModuleLink($blockonomics->name, 'payment', array("crypto"=>array_keys($active_cryptos)[0]), true));
} else if (count($active_cryptos) === 0) {
    $this->setTemplate('module:blockonomics/views/templates/front/no_crypto.tpl');
}

The Payment controller will contain the existing functionality with the addition of collecting the crypto param from the URL to use when checking for address reuse, generating a new address, and saving to the database. The Select Crypto controller will contain the functionality to collect the active cryptos and display the select_crypto template. This requires its own controller to fix the POST request behavior of the validation controller.

DB Changes

The existing blockonomics_bitcoin_orders database table will hold both BCH and BTC addresses. A new crypto column is required to know the type of each crypto address. This will allow for address reuse based on the crypto and allow a user to go back and select another crypto for the same order. Each id_cart can have two blockonomics_bitcoin_orders rows. One for the BTC address and one for BCH. Address reuse should now happen based on the id_cart and selected crypto: https://github.com/blockonomics/prestashop-plugin/blob/1.7/controllers/front/validation.php#L105

shivaenigma commented 3 years ago

Overall the design looks good to me.

The request to the validation controller cannot be changed to a GET request as the form method is set in the theme template files, however, using Tools::redirect() from the validation controller fixes the issues with refreshing and going back as the redirect is not added to the browser history.

Can you elaborate more on this. Will this require another encrypted order id parameter in URL like in WHMCS ?

DarrenWestwood commented 3 years ago

WHMCS uses the id_order from the request to collect the blockonomics_order, whereas Prestashop uses the cart_id from the customer context This means we are not required to add/encrypt the order id in the URL as it is not used to fetch the order from the database in Prestashop. By redirecting from validation.php to payment.php we solve the form submission issue, without any further changes. Tools::redirect($this->context->link->getModuleLink($blockonomics->name, 'payment', array(), true));

Pull request to fix form submission, separate to the bch changes: Fix issue with form submission on refresh #108

blockonomics commented 3 years ago

WHMCS uses the id_order from the request to collect the blockonomics_order, whereas Prestashop uses the cart_id from the customer context This means we are not required to add/encrypt the order id in the URL as it is not used to fetch the order from the database in Prestashop. By redirecting from validation.php to payment.php we solve the form submission issue, without any further changes. Tools::redirect($this->context->link->getModuleLink($blockonomics->name, 'payment', array(), true));

Pull request to fix form submission, separate to the bch changes: Fix issue with form submission on refresh #108

Ok understood. Looks clear and elegant. Just to confirm, pressing F5 on payment or select controller page will work normally without any browser warning ?

DarrenWestwood commented 3 years ago

Just to confirm, pressing F5 on payment or select controller page will work normally without any browser warning ?

Yes this is correct. Refreshing/F5 will work normally without any browser warning on the payment and select controller.

blockonomics / prestashop-plugin