Skip to content
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

Memberships: Product creation fail message #12226

Merged
merged 2 commits into from
May 6, 2019

Conversation

artpi
Copy link
Contributor

@artpi artpi commented May 1, 2019

For some reasons, product creation fails for certain sites. #9802 (review)
The error message is terrible. We have captured extensive logs on wpcom side, but error is not returned properly.

This change is only executed on WPCOM side and its already merged there.
This PR syncs it.
No testing necessary.

@artpi artpi requested a review from a team May 1, 2019 15:54
@artpi artpi self-assigned this May 1, 2019
@artpi artpi added [Status] Needs Review This PR is ready for review. [Type] Bug When a feature is broken and / or not performing as intended labels May 1, 2019
@jetpackbot
Copy link
Collaborator

jetpackbot commented May 1, 2019

Warnings
⚠️ "Testing instructions" are missing for this PR. Please add some
⚠️ "Proposed changelog entry" is missing for this PR. Please include any meaningful changes

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against b1436c2

@kraftbj kraftbj added this to the 7.3 milestone May 1, 2019
@artpi
Copy link
Contributor Author

artpi commented May 2, 2019

@kraftbj I don't think this needs cherry-pick. This code here is never executed in Jetpack. I am getting it into JP for Fusion's sake, but it makes no difference for the users and their sites. Its merged to wpcom where it makes sense.

Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

While it works to avoid the first error, I think we would also need to update the Membership block itself to support use-cases when no product can be created. Right now, when running into this error you end up getting the following in the block editor:

Warning: Each child in an array or iterator should have a unique "key" prop.
Check the render method of `MembershipsButtonEdit`.

This seems to happen because this.state.products is an empty array.

@jeherve jeherve added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels May 2, 2019
@kraftbj
Copy link
Contributor

kraftbj commented May 2, 2019

@kraftbj I don't think this needs cherry-pick. This code here is never executed in Jetpack. I am getting it into JP for Fusion's sake, but it makes no difference for the users and their sites. Its merged to wpcom where it makes sense.

Understood it isn't needed, but I do like to keep any code related to a new feature in Jetpack going out at the same time, even if it doesn't run. If it is a bug fix for something unreleased that either fixes something seen or has no liability (in this case), rather cherry-pick than not.

Cherry-picking is not something we expect or want you, as the originating developer, to do. It's a quick thing on my side as I'm working through the beta week.

@jeherve jeherve added the [Block] Payment Button aka Recurring Payments label May 3, 2019
@artpi
Copy link
Contributor Author

artpi commented May 3, 2019

@jeherve I added b1436c2 which passes the error causing it to display:

Zrzut ekranu 2019-05-3 o 14 18 01

@artpi artpi added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels May 3, 2019
@artpi artpi requested a review from jeherve May 3, 2019 12:27
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

The error message isn't especially helpful since it does not tell you what to do next, but it's better than the current version in master.

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels May 6, 2019
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello artpi! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D27845-code before merging this PR. Thank you!

@artpi
Copy link
Contributor Author

artpi commented May 6, 2019

The error message isn't especially helpful since it does not tell you what to do next

That is because I honestly don't know why this error occurs on your site :D
Once we solve core issue, the message should not even be necessary.

@artpi
Copy link
Contributor Author

artpi commented May 6, 2019

D27845-code is deployed.

@jeherve
Copy link
Member

jeherve commented May 6, 2019

@artpi Let's take that opportunity to bring the file in sync to solve those Fusion issues. See D27848-code.

@artpi
Copy link
Contributor Author

artpi commented May 6, 2019

@jeherve Fusion PR is waiting patiently for when this right here lands.
See D27731-code

@matticbot matticbot removed the [Status] Ready to Merge Go ahead, you can push that green button! label May 6, 2019
@jeherve jeherve deleted the memberships/fix-product-creation-message branch May 6, 2019 10:31
jeherve pushed a commit that referenced this pull request May 6, 2019
* Better product craetion message, port changes from WPCOM

* Better error passing
@jeherve
Copy link
Member

jeherve commented May 6, 2019

Cherry-picked to branch-7.3 in 32f1e95

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Payment Button aka Recurring Payments Touches WP.com Files [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants