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

Rewrite plugin to v3.0 #69

Merged
merged 2 commits into from Nov 10, 2018

Conversation

Projects
None yet
7 participants
@mosu-forge
Copy link
Contributor

mosu-forge commented Sep 22, 2018

v3.0 includes major reworking of how the plugin works:

  • Ability to set number of confirms: 0 for zero conf, up to 60.
  • Amount owed in XMR gets locked in after the order for a configurable amount of time after which the order is invalid, default 60 minutes.
  • Shows transactions received along with the number of confirms right on the order success page, auto-updates through AJAX.
  • QR code generation is done with Javascript instead of sending payment details to a 3rd party.
  • Admin page for showing all transactions made to the wallet.
  • Logic is done via cron, instead of the user having to stay on the order page until payment is confirmed.
  • Payment details (along with the txid) are always visible on the customer's account dashboard on the my orders section.
  • Live prices are also run via cron, shortcodes for showing exchange rates.
  • Properly hooks into order confirmation email page.
@serhack

This comment has been minimized.

Copy link
Member

serhack commented Sep 22, 2018

Hi Mosu-forge,
I'm really sorry but I am going to close your pull request. The features you implemented are fantastic, but I can not approve your edits on code.

I CAN'T stand why you are putting your donation address. Monero Integrations is a project funded by community and managed by Monero Integrations team.

Thanks anyway.
I'm going to review in a deeper way.

Cheers,
SerHack

@mosu-forge

This comment has been minimized.

Copy link
Contributor

mosu-forge commented Sep 22, 2018

@serhack then remove my donation address if that is the only reason you won't merge.

Can you expand on what edits you think are low quality?

@@ -1,6 +1,7 @@
MIT License

Copyright (c) 2017-2018 Monero Integrations
Copyright (c) 2018, Ryo Currency Project

This comment has been minimized.

@serhack

serhack Sep 22, 2018

Member

Remove. You are free to contribute, respecting the license.

This comment has been minimized.

@fireice-uk

fireice-uk Sep 22, 2018

@mosu-forge Don't do that, you don't want certain people suing you =)

This comment has been minimized.

@rehrar

rehrar Oct 11, 2018

If this code is merged, then he is free to attribute his copyright to whatever project he wishes. If this is merged, then this must stay.

This comment has been minimized.

@mosu-forge

mosu-forge Oct 11, 2018

Contributor
Copyright (c) 2018, Ryo Currency Project
Portions Copyright (c) 2017-2018, Monero Integrations

may be reworded to:

Copyright (c) 2018, Ryo Currency Project
Copyright (c) 2017-2018, Monero Integrations

i.e. removing the word "Portions". I also do not care if Monero is moved above (c) Ryo Currency. This work was done under the funding of Ryo, so I would like for it to stay in the LICENSE file.

Edit: @rehrar, this wasn't directed at you just to clarify, I appreciate your input. Just commenting since I realize we never resolved this issue.

README.md Outdated

## Donating to the Devs :)
XMR Address : `44krVcL6TPkANjpFwS2GWvg1kJhTrN7y9heVeQiDJ3rP8iGbCd5GeA4f3c2NKYHC1R4mCgnW7dsUUUae2m9GiNBGT4T8s2X`
mosu-forge: 4A6BQp7do5MTxpCguq1kAS27yMLpbHcf89Ha2a8Shayt2vXkCr6QRpAXr1gLYRV5esfzoK3vLJTm5bDWk5gKmNrT6s6xZep

This comment has been minimized.

@serhack

serhack Sep 22, 2018

Member

Remove.

jQuery('#monero_toast').append(toast);
toast.animate({ "right": "12px" }, "fast");
setInterval(function() {
toast.animate({ "right": "-400px" }, "fast", function() {

This comment has been minimized.

@serhack

serhack Sep 22, 2018

Member

Right: -400px?

This comment has been minimized.

@mosu-forge

mosu-forge Sep 22, 2018

Contributor

Correct, negative values are valid for top, left, bottom, right and are used to hide something off screen.

This comment has been minimized.

@serhack

serhack Sep 22, 2018

Member

To hide something on screen: display: none.

This comment has been minimized.

@mosu-forge

mosu-forge Sep 22, 2018

Contributor

Alright, well you're not understanding why -400px is used... The notification is animated off-screen. Please don't resort to personal attacks.

This comment has been minimized.

@serhack

serhack Sep 22, 2018

Member

I'm sorry. My intent was not to attack you on personal. However I appreciate your work.

};
setInterval(monero_fetchDetails, 30000);
monero_updateDetails();
new ClipboardJS('.clipboard').on('success', function(e) {

This comment has been minimized.

@serhack

serhack Sep 22, 2018

Member

I do not know why you need to add some random JS library for "Clipboarding".

This comment has been minimized.

@mosu-forge

mosu-forge Sep 22, 2018

Contributor

Because it's not standard across browsers, and mobile platforms, which is why clipboard.js exists in the first place. Originally I did not use a library and it wasn't cross-platform.

This comment has been minimized.

@serhack

serhack Sep 22, 2018

Member

Why do you have to copy a string on mobile when you can click on a link monero:// which will open a Monero wallet (Monerujo and Cakewallet supports monero:// )?

This comment has been minimized.

@mosu-forge

mosu-forge Sep 22, 2018

Contributor

If this is a serious objection, then clipboarding can be done the way the masari plugin does it, however clipboard.js isn't a heavy framework, but I understand if you want to drop it.

Group=moneroservices
WorkingDirectory=/opt/monero-wallets
Type=simple
ExecStart=/opt/monero-bin/monero-wallet-rpc --wallet-file /opt/monero-wallets/woocommerce --rpc-bind-port 18080 --password-file /opt/monero-wallets/woocommerce.password --disable-rpc-login --log-file /var/log/monero-wallet.log

This comment has been minimized.

@serhack

serhack Sep 22, 2018

Member

--disable-rpc-login ? Do you know what will happen if the rpc-login is disabled?

This comment has been minimized.

@mosu-forge

mosu-forge Sep 22, 2018

Contributor

Am I missing where the original WP plugin used authorization? Not being snarky.

add_action('wp', 'monero_activate_cron');
function monero_activate_cron() {
if(!wp_next_scheduled('monero_update_event')) {
wp_schedule_event(time(), 'one_minute', 'monero_update_event');

This comment has been minimized.

@serhack

serhack Sep 22, 2018

Member

Waste of resources.

This comment has been minimized.

@mosu-forge

mosu-forge Sep 22, 2018

Contributor

The problem with the way the current plugin works is that it only does payment checking logic when the order confirmation page is refreshed. So if a user leaves the order confirmation page, then their order will never confirm. Cron is necessary for allowing users to leave the page, and still have their order confirmed, especially if a merchant sets confirms to 10+. If you feel once a minute is too often, that can be changed.

This comment has been minimized.

@serhack

serhack Sep 22, 2018

Member

Imagine having running 200 cronjobs at the same time. I still disagree for running cronjobs when users leaves the order confirmation page.

This comment has been minimized.

@mosu-forge

mosu-forge Sep 22, 2018

Contributor

To be clear, the plugin only runs one cronjob for all payments. It does two things:

  • Fetches the price so live exchange rates can be displayed on the site, or used for displaying prices in XMR on the frontend
  • Checks for pending orders, and if there are any, checks for payments

This comment has been minimized.

@serhack

serhack Sep 22, 2018

Member

We have already had a sort of cronjob for live exchange rates.

This comment has been minimized.

@mosu-forge

mosu-forge Sep 22, 2018

Contributor

I did not see that anywhere in the code grep -Ri cron monerowp returns nothing.

This may be a disagreement of how the plugin should work. In my opinion if a merchant wants to require 10 confirms, it's not nice to make the user stay on the order confirmation for 20 minutes (at least, could be more), and if they close it, the payment never confirms.

This comment has been minimized.

@mosu-forge

mosu-forge Sep 22, 2018

Contributor

I know this PR is closed, but for the time that you re-look at this code I wanted to add:

Right now the current plugin actually runs the same payment checking code that this cron job does every 17 seconds, so this code should actually be more efficient.

This comment has been minimized.

@rehrar

rehrar Oct 11, 2018

This, or something like this, is critical to include indeed. It is unacceptable that the page cannot be closed or the order will not fulfill.

@@ -1,9 +1,9 @@
=== Monero WooCommerce Extension ===
Contributors: serhack
Contributors: serhack, mosu-forge

This comment has been minimized.

@serhack

This comment has been minimized.

@rehrar

rehrar Oct 11, 2018

If this PR is merged, then he is indeed a contributor and this should be here.

@serhack

This comment has been minimized.

Copy link
Member

serhack commented Sep 22, 2018

In particular, you are using mixed content. Some variables are sent to database without a prevent escaping. "Rewriting" is a very big word. You should simplify things, not adding Javascript framework and libraries to random. Moving and splitting the code in multiple files is not "rewriting a payment gateway".

@mosu-forge

This comment has been minimized.

Copy link
Contributor

mosu-forge commented Sep 22, 2018

@serhack responded to all the comments above, except the license / author questions.

If there's a place I did not escape a MySql statement, that should be updated, can you point it out please?

There is a lot more than moving and splitting code, if you don't want to call it a re-write, then that's okay, just call it a version bump.

@serhack

This comment has been minimized.

Copy link
Member

serhack commented Sep 22, 2018

There is a lot more than moving and splitting code, if you don't want to call it a re-write, then that's okay, just call it a version bump.

I disagree on splitting code into multiple files.

@mosu-forge

This comment has been minimized.

Copy link
Contributor

mosu-forge commented Sep 22, 2018

I disagree on splitting code into multiple files.

There are only a few key places I split code:

  • Monero RPC class, and Monero Node tools: split because we only need to require one or the other file
  • Template files, it's not fun editing inlined HTML in PHP files
  • Admin settings options, this is a common configuration for Wordpress plugins
@serhack

This comment has been minimized.

Copy link
Member

serhack commented Sep 22, 2018

Anyway, at the moment, I am a little busy with others project related to Monero. I do not have the time currently to test this and edit it to enchant a little. I'll soon open a FFS request for doing maintenance of plugins on this organization (Monero Integrations). I'll appreciate your answers and your code, I think I could improve a lot this payment gateway implementing features described by you.

However, I will not merge your pull request.

My suggestion for the future is splitting this in multiple pull-requests so we could discuss more about each feature :- ) I do not want to offend you and I'm sorry if you think that.

Cheers,
SerHack.

@serhack serhack closed this Sep 22, 2018

@mosu-forge

This comment has been minimized.

Copy link
Contributor

mosu-forge commented Sep 22, 2018

I'm fine if you don't merge it, I just wanted to contribute back. I hope when you have time to properly evaluate the plugin, you spin it up on a server.

@serhack

This comment has been minimized.

Copy link
Member

serhack commented Sep 22, 2018

Naturally yes. Thanks for trying to understand me. I hope you'll have a good day.
Best regards,
SerHack

@Mattthev

This comment has been minimized.

Copy link

Mattthev commented Sep 22, 2018

It's simple! XMR payments by Mosu works like a charm.
This XMR plugin just not confirm orders... It generates new address every refresh and also amount. CSS is not working for me and loading it from non SSL... It looks really dumb on checkout when you want to pay with privacy coin and the SSL lock isn't green and locked...

@cryptochangements34

This comment has been minimized.

Copy link
Member

cryptochangements34 commented Sep 22, 2018

Personally I do agree with some of the splitting into several files. Having all the code (including inline HTML/CSS/JS) is rather messy

@serhack

This comment has been minimized.

Copy link
Member

serhack commented Sep 22, 2018

I disagree, but I love community input. I recognize Monero Integrations Payment gateway for woocommerce has some issues on UX.

@serhack serhack referenced this pull request Sep 22, 2018

Closed

Implementing new features #70

@lacksfish

This comment has been minimized.

Copy link

lacksfish commented Sep 23, 2018

This seems to be a larger rewrite, at first glance. Looks like a proper effort!

Maybe OP of the project and @mosu-forge can talk together and adapt this PR to a mergeable state, with respect to Copyright and small code cleanups and all that.

Otherwise why not fork it and keep developing anyways? :)

@serhack

This comment has been minimized.

Copy link
Member

serhack commented Sep 23, 2018

I'm reopening this.

@serhack serhack reopened this Sep 23, 2018

@serhack

This comment has been minimized.

Copy link
Member

serhack commented Sep 23, 2018

This will be the base for the next version the third one. I'll include some features like subaddresses integration and I'll try to avoid as much as possible any JS framework.

@rehrar

This comment has been minimized.

Copy link

rehrar commented Oct 11, 2018

This, or something like this, is critical to get working correctly. The current WP plugin is broken as is, and many of the griefs I had of the old one are addressed here.

@serhack serhack merged commit ad6fe19 into monero-integrations:master Nov 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment