woocommerce / pinterest-for-woocommerce

A native Pinterest integration for WooCommerce. Development is managed by Ventures.
https://woocommerce.com/products/pinterest-for-woocommerce/
GNU General Public License v3.0
22 stars 9 forks source link

922 retry oauth #925

Closed budzanowski closed 6 months ago

budzanowski commented 7 months ago

Changes proposed in this Pull Request:

Closes #922 .

When token procedure fails retry 3 more times in 5, 10 and 15 minutes.

Testing:

budzanowski commented 6 months ago

@ecgan

I think we need to reset or delete the note when we are disconnecting Pinterest connection or when we are uninstalling the plugin (using uninstall.php file).

We don't because disconnecting or deleting ( installing later ) always puts merchants on the path of regular manual connection requirement which is not an error scenario.

budzanowski commented 6 months ago

@ecgan

💡 I think during the action run, we need to check whether the store is already connected with v5 token. If yes, we can stop the action and no need to schedule another retry.

Correct! This should now be handled by ee4038f

Please retry.

ecgan commented 6 months ago

@budzanowski ,

I think we need to reset or delete the note when we are disconnecting Pinterest connection or when we are uninstalling the plugin (using uninstall.php file).

We don't because disconnecting or deleting ( installing later ) always puts merchants on the path of regular manual connection requirement which is not an error scenario.

But what if users did what I did, e.g. disconnecting Pinterest connection, uninstall Pinterest plugin (with v5 API), install old Pinterest plugin (with v3 API), and then update Pinterest plugin (with v5 API), and then for some reasons there is an error scenario there, but the note is not shown because it is already actioned in the database?

Also, according to https://developer.wordpress.org/plugins/plugin-basics/uninstall-methods/:

When your plugin is uninstalled, you’ll want to clear out any plugin options and/or settings specific to the plugin, and/or other database entities such as tables.

I suppose this is also applicable to WC Admin notes too?

budzanowski commented 6 months ago

@ecgan

But what if users did what I did, e.g. disconnecting Pinterest connection, uninstall Pinterest plugin (with v5 API), install old Pinterest plugin (with v3 API), and then update Pinterest plugin (with v5 API), and then for some reasons there is an error scenario there, but the note is not shown because it is already actioned in the database?

That is a very obscure scenario. The moment you disconnect, uninstall and go to install old version just to install new version sounds sketchy. We can reinforce the plugin for this scenario but it starts too feel like an overkill.

budzanowski commented 6 months ago

@ecgan please check 2cd9dcf it deletes the note on success and uninstall making the code ready to generate a new note in case it will be needed.

ecgan commented 6 months ago

@ecgan please check https://github.com/woocommerce/pinterest-for-woocommerce/pull/925/commits/2cd9dcffb5c53bdac55f19c353182fb812ee7b2f it deletes the note on success and uninstall making the code ready to generate a new note in case it will be needed.

I just noticed this message. I'll test it out.

ecgan commented 6 months ago

@ecgan please check 2cd9dcf it deletes the note on success and uninstall making the code ready to generate a new note in case it will be needed.

I just noticed this message. I'll test it out.

Something's weird. I tried out the latest code here with a "broken" version, and I don't see the retry scheduled action, no "action required" banner, the token exchange did not happened, and I need to manually connect Pinterest account in the plugin settings page.

I was expecting to see retry in scheduled action, and "action required" banner.

I'll try this out again tomorrow.

ecgan commented 6 months ago

Something's weird. I tried out the latest code here with a "broken" version, and I don't see the retry scheduled action, no "action required" banner, the token exchange did not happened, and I need to manually connect Pinterest account in the plugin settings page.

I was expecting to see retry in scheduled action, and "action required" banner.

The reason why it was not working like how I expected is because the pinterest_for_woocommerce_pinterest_api_version option is still in the database with the value of 'v5' even after uninstallation.

Screenshot 2024-03-13 at 3 51 18 PM

When I was doing the test with the "broken" version, the code sees the 'v5' option value, and decides not to do the retry. See the relevant code here: https://github.com/woocommerce/pinterest-for-woocommerce/pull/925/files#diff-9087ae5f2a84d37a915583363d0231c72410aa9b1ac515734de92a14b191e8e4R331-R334. That's why there was no retry scheduled action.

I manually removed the options from the database and tested it again, it works as expected now, I can see retry in scheduled action, and the "action required" banner.

Note that removing the option upon uninstallation is being addressed in PR https://github.com/woocommerce/pinterest-for-woocommerce/pull/932.