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

fix(compat): prevent missing donation data with Restrict Content Pro #2710

Closed
ravinderk opened this Issue Jan 25, 2018 · 13 comments

Comments

Projects
None yet
7 participants
@ravinderk
Collaborator

ravinderk commented Jan 25, 2018

Issue Overview

If you activate restrict content pro then you will see information missing on donation listing page in admin.

image

Related

Tasks

  • Implement solution
    • Renaming the meta typepayment to donation
    • Renaming the table {*}_give_paymentmeta to {*}_give_donationmeta
    • Renaming the table and payment_id -> donation_id column
    • Rename $wpdb property paymentmeta -> donationmeta
    • Add DB upgrade
    • Add backward compatibility
  • Tests
    • Check DB upgrade
    • Test with multiple addon
  • Add blog post
  • Add issue for document update

@ravinderk ravinderk self-assigned this Jan 25, 2018

@ravinderk ravinderk added bug labels Jan 25, 2018

@ravinderk ravinderk added this to the 2.0.1 milestone Jan 25, 2018

@ravinderk

This comment has been minimized.

Show comment
Hide comment
@ravinderk

ravinderk Jan 25, 2018

Collaborator

@DevinWalker

Cause of issue:

I found the conflict between RCP and Give. We have same $wpdb->paymentmeta table name param, so restrict content pro overwriting over table name or vice versa 😞
For ref:
https://github.com/WordPress/WordPress/blob/master/wp-includes/meta.php#L931
https://github.com/restrictcontentpro/restrict-content-pro/blob/master/restrict-content-pro.php#L371

Collaborator

ravinderk commented Jan 25, 2018

@DevinWalker

Cause of issue:

I found the conflict between RCP and Give. We have same $wpdb->paymentmeta table name param, so restrict content pro overwriting over table name or vice versa 😞
For ref:
https://github.com/WordPress/WordPress/blob/master/wp-includes/meta.php#L931
https://github.com/restrictcontentpro/restrict-content-pro/blob/master/restrict-content-pro.php#L371

@ravinderk ravinderk modified the milestones: 2.0.1, 2.2 Jan 25, 2018

@DevinWalker DevinWalker removed this from the 2.2 milestone Mar 6, 2018

@kevinwhoffman kevinwhoffman changed the title from Conflict with restrict content pro to fix(compat): prevent missing donation data with Restrict Content Pro Mar 11, 2018

@samsmith89

This comment has been minimized.

Show comment
Hide comment
@samsmith89

samsmith89 Mar 19, 2018

Having this issue currently with a customer. Was this fixed and reopened, or still waiting?
https://secure.helpscout.net/conversation/543868415/15603?folderId=1823994

samsmith89 commented Mar 19, 2018

Having this issue currently with a customer. Was this fixed and reopened, or still waiting?
https://secure.helpscout.net/conversation/543868415/15603?folderId=1823994

@kevinwhoffman kevinwhoffman added this to the Sprint: 2018/03/28 - 2018/04/10 milestone Mar 30, 2018

@kevinwhoffman

This comment has been minimized.

Show comment
Hide comment
@kevinwhoffman

kevinwhoffman Mar 30, 2018

Member

@ravinderk Please complete the following tasks but do not move forward with implementation until we discuss with @DevinWalker.

  • Post a possible solution in this issue, including whether a database upgrade routine would be necessary.
  • Update the task list based on your proposed solution.
  • Update the story point estimate based on your proposed solution.
  • Notify @DevinWalker and @kevinwhoffman to discuss next steps.
Member

kevinwhoffman commented Mar 30, 2018

@ravinderk Please complete the following tasks but do not move forward with implementation until we discuss with @DevinWalker.

  • Post a possible solution in this issue, including whether a database upgrade routine would be necessary.
  • Update the task list based on your proposed solution.
  • Update the story point estimate based on your proposed solution.
  • Notify @DevinWalker and @kevinwhoffman to discuss next steps.
@ravinderk

This comment has been minimized.

Show comment
Hide comment
@ravinderk

ravinderk Apr 2, 2018

Collaborator

@kevinwhoffman I already explained the problem here: #2710 (comment)

paymentdata is not a general table name prefix that was logic behind keeping. But conflict with Restrict content pro prove that In future, we will have the possibility to conflict with a plugin which can process payment own there own.

Thanks to Restrict content pro team which took the first step to resolve conflict (release/3.0): https://github.com/restrictcontentpro/restrict-content-pro/issues/1656

To resolve this problem we can take following steps:

@DevinWalker @kevinwhoffman Let me know what do you think.

Collaborator

ravinderk commented Apr 2, 2018

@kevinwhoffman I already explained the problem here: #2710 (comment)

paymentdata is not a general table name prefix that was logic behind keeping. But conflict with Restrict content pro prove that In future, we will have the possibility to conflict with a plugin which can process payment own there own.

Thanks to Restrict content pro team which took the first step to resolve conflict (release/3.0): https://github.com/restrictcontentpro/restrict-content-pro/issues/1656

To resolve this problem we can take following steps:

@DevinWalker @kevinwhoffman Let me know what do you think.

@kevinwhoffman

This comment has been minimized.

Show comment
Hide comment
@kevinwhoffman

kevinwhoffman Apr 3, 2018

Member

@ravinderk Thanks for the breakdown. RCP seems to have merged a fix on their end, but it is not slated for release until RCP 3.0, which does not look like it has a known release.

We should still adress it from our end, but this does not look like a minor fix, so I'd like to hear from @mathetos how many users this is affecting. Then we can discuss timeline.

Member

kevinwhoffman commented Apr 3, 2018

@ravinderk Thanks for the breakdown. RCP seems to have merged a fix on their end, but it is not slated for release until RCP 3.0, which does not look like it has a known release.

We should still adress it from our end, but this does not look like a minor fix, so I'd like to hear from @mathetos how many users this is affecting. Then we can discuss timeline.

@DevinWalker DevinWalker removed this from the Sprint: 2018/03/28 - 2018/04/10 milestone Apr 3, 2018

@mathetos

This comment has been minimized.

Show comment
Hide comment
@mathetos

mathetos Apr 3, 2018

Member

@kevinwhoffman Only two customers reporting as far as I've seen so far. But the urgency is related to the fact that Give 2.0+ is completely unusable for any user that is also running RCP. Additionally, I can imagine this problem would happen with other plugins that call paymentmeta using wpdb as well.

Member

mathetos commented Apr 3, 2018

@kevinwhoffman Only two customers reporting as far as I've seen so far. But the urgency is related to the fact that Give 2.0+ is completely unusable for any user that is also running RCP. Additionally, I can imagine this problem would happen with other plugins that call paymentmeta using wpdb as well.

@Benunc Benunc added 3-reported and removed 1-reported labels Apr 4, 2018

@Benunc

This comment has been minimized.

Show comment
Hide comment
@Benunc

Benunc Apr 4, 2018

Member

This issue also breaks functionality with Zapier and Give, and I have a user reporting that, and I replicated the problem on my test (live) install.

Findings

WHen RCP and Give are active on the same site, in addition to the above reference (donor missing) issue, Zapier is unable to connect to the api URL at all.

Additionally, the API returns incomplete and incorrect information:

Before activating RCP:


screen shot 2018-04-04 at 4 11 42 pm


After Activating RCP:


screen shot 2018-04-04 at 4 12 12 pm


So, in addition to removing all relevant data from the API, it also incorrectly attributes the donations to the POST "Thought 53" which isn't a form at all.

Member

Benunc commented Apr 4, 2018

This issue also breaks functionality with Zapier and Give, and I have a user reporting that, and I replicated the problem on my test (live) install.

Findings

WHen RCP and Give are active on the same site, in addition to the above reference (donor missing) issue, Zapier is unable to connect to the api URL at all.

Additionally, the API returns incomplete and incorrect information:

Before activating RCP:


screen shot 2018-04-04 at 4 11 42 pm


After Activating RCP:


screen shot 2018-04-04 at 4 12 12 pm


So, in addition to removing all relevant data from the API, it also incorrectly attributes the donations to the POST "Thought 53" which isn't a form at all.

@ravinderk

This comment has been minimized.

Show comment
Hide comment
@ravinderk

ravinderk May 11, 2018

Collaborator

@WordImpress/support I think this issue fixed with latest https://github.com/restrictcontentpro/restrict-content-pro/releases/tag/2.9.12

I tested it. Can you confirm?

Collaborator

ravinderk commented May 11, 2018

@WordImpress/support I think this issue fixed with latest https://github.com/restrictcontentpro/restrict-content-pro/releases/tag/2.9.12

I tested it. Can you confirm?

@samsmith89

This comment has been minimized.

Show comment
Hide comment
@samsmith89

samsmith89 May 11, 2018

I will check on this today.

samsmith89 commented May 11, 2018

I will check on this today.

@mathetos

This comment has been minimized.

Show comment
Hide comment
@mathetos

mathetos May 11, 2018

Member

@ravinderk Two things:

  1. It looks like that PR on RCP was merged into version 3.0, but that's not their current release.
  2. Even if they fix it on their end, I believe we need a way to avoid this happening with other plugins as well. Is there any prefixing or namespacing we can do to avoid this problem in the future?
Member

mathetos commented May 11, 2018

@ravinderk Two things:

  1. It looks like that PR on RCP was merged into version 3.0, but that's not their current release.
  2. Even if they fix it on their end, I believe we need a way to avoid this happening with other plugins as well. Is there any prefixing or namespacing we can do to avoid this problem in the future?
@ravinderk

This comment has been minimized.

Show comment
Hide comment
@ravinderk

ravinderk May 14, 2018

Collaborator

@mathetos yes, I will be fixing in release assign to this issue.

Collaborator

ravinderk commented May 14, 2018

@mathetos yes, I will be fixing in release assign to this issue.

ravinderk added a commit to impress-org/give-database-healthcheck that referenced this issue May 22, 2018

@DevinWalker DevinWalker modified the milestones: Sprint: 2018/05/08 - 2018/05/22, Sprint: 2018/05/08 - 2018/06/05 May 23, 2018

DevinWalker added a commit that referenced this issue May 24, 2018

Merge pull request #3243 from /issues/2710
fix(compat): prevent missing donation data with Restrict Content Pro #2710
@mehul0810

This comment has been minimized.

Show comment
Hide comment
@mehul0810

mehul0810 May 28, 2018

Contributor

@ravinderk I can see an issue on 2.2 Give core branch even after running DB upgrade.
image

Contributor

mehul0810 commented May 28, 2018

@ravinderk I can see an issue on 2.2 Give core branch even after running DB upgrade.
image

@mehul0810 mehul0810 reopened this May 28, 2018

@ravinderk

This comment has been minimized.

Show comment
Hide comment
@ravinderk

ravinderk May 28, 2018

Collaborator

@mehul0810 Close this issue because it fixed.

Collaborator

ravinderk commented May 28, 2018

@mehul0810 Close this issue because it fixed.

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