-
Notifications
You must be signed in to change notification settings - Fork 51
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: add option to display subscriptionless memberships #3058
Conversation
@thomasguillot @kevinzweerink This one's kind of "it is what it is" visually, but I'd appreciate your thoughts on hiding the 'Cancel' button from the main Memberships table. Normally it shows in the list, but it seemed like extra clutter, and it wasn't consistent with how the Subscriptions list looked, when both were displaying. (I'd also appreciate any feedback on little things we can improve!). |
Can we hide the "next bill on" column since it's not applicable? |
@thomasguillot Oooo, good call! I just pushed an update -- here's what it looks like without: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@laurelfulford works well and with very clean code! Just a few minor suggestions below.
Adding the extra option in WooCommerce settings makes sense, but in practice we usually try not to add our own options into the WooCommerce settings UI. I think we should try to reimplement this new setting in the Newspack > Reader Activation > Advanced Settings wizard page. I'm okay with doing this as a separate follow-up PR so it doesn't block this one, though.
public static function get_memberships_without_subs() { | ||
if ( function_exists( 'wc_memberships_get_user_active_memberships' ) ) { | ||
$customer_id = \get_current_user_id(); | ||
$memberships_info = wc_memberships_get_user_active_memberships( $customer_id ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: this is not currently throwing any errors for me, but it's something we've run into occasionally, so this is a good practice to get into the habit of:
$memberships_info = wc_memberships_get_user_active_memberships( $customer_id ); | |
$memberships_info = \wc_memberships_get_user_active_memberships( $customer_id ); |
If you're working in a PHP file that has a namespace (as this one does), when calling any global function, it's a good idea to prefix it with \
. This tells PHP to look for this function in the global namespace. If you don't include the \
prefix, then PHP will first look for the function in the current namespace and then fall back to global if it doesn't exist in the current namespace. I've seen instances where this can cause fatal errors, so adding the \
sidesteps that whole issue and also makes the code a little clearer in a namespaced file (even if it's not required to avoid errors like in this case).
The exception to this rule is internal PHP functions, so if you can find the function in the PHP manual, you don't need to specify a namespace (global or otherwise) for it. In my code editor internal PHP functions are helpfully highlighted in a different color than other functions:
![Screenshot 2024-04-16 at 1 03 47 PM](https://private-user-images.githubusercontent.com/2230142/322967793-291a3576-8284-494e-9227-0228c9807ebc.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzk2NDcyNTAsIm5iZiI6MTczOTY0Njk1MCwicGF0aCI6Ii8yMjMwMTQyLzMyMjk2Nzc5My0yOTFhMzU3Ni04Mjg0LTQ5NGUtOTIyNy0wMjI4Yzk4MDdlYmMucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI1MDIxNSUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNTAyMTVUMTkxNTUwWiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9MDc4YzRmNDMxZmNmODUzZTQyMmFkMTlhYjVlMDk3NDZjZjRkZmZkYzg0YTRlMzI4NDliOWQxOWU4ODU4ZjkwNSZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QifQ.60eoK8D_zfs-LRALr6TN6-pqSrMA03U9HO4jQjWM2X0)
You also don't need to prefix the function name when checking whether it exists (like function_exists
) as this will check for the function in every namespace including the global one, unless you pass a specific namespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL -- nice! I didn't realize the purpose of the \
, I figured it had something to do with namespaced/not namespaced, but didn't realize what it actually did under the hood.
includes/reader-revenue/my-account/class-woocommerce-my-account.php
Outdated
Show resolved
Hide resolved
includes/reader-revenue/my-account/class-woocommerce-my-account.php
Outdated
Show resolved
Hide resolved
includes/reader-revenue/my-account/class-woocommerce-my-account.php
Outdated
Show resolved
Hide resolved
includes/reader-revenue/my-account/class-woocommerce-my-account.php
Outdated
Show resolved
Hide resolved
includes/reader-revenue/my-account/class-woocommerce-my-account.php
Outdated
Show resolved
Hide resolved
includes/reader-revenue/my-account/class-woocommerce-my-account.php
Outdated
Show resolved
Hide resolved
includes/reader-revenue/my-account/class-woocommerce-my-account.php
Outdated
Show resolved
Hide resolved
includes/reader-revenue/my-account/class-woocommerce-my-account.php
Outdated
Show resolved
Hide resolved
Thanks for this, @dkoo! This was all very helpful! I made the code corrections in 44698b9, and then took a swing at moving the option into the Reader Activation advanced settings in d130fae. It should now appear with the other Membership settings there, and, when the Memberships plugin isn't enabled, it shouldn't appear: (It's also off by default like the original, which my screenshot doesn't really convey!) Just let me know if it'd be better to roll back this bit and focus on the original set of changes, or if there's anything else that looks amiss -- thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes! Looks good and works well!
## [3.6.13](v3.6.12...v3.6.13) (2024-04-22) ### Bug Fixes * add option to display subscriptionless memberships ([#3058](#3058)) ([aa493c9](aa493c9))
🎉 This PR is included in version 3.6.13 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
# [3.7.0-alpha.7](v3.7.0-alpha.6...v3.7.0-alpha.7) (2024-04-22) ### Bug Fixes * add option to display subscriptionless memberships ([#3058](#3058)) ([aa493c9](aa493c9))
🎉 This PR is included in version 3.7.0-alpha.7 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
All Submissions:
Changes proposed in this Pull Request:
This PR is to work with an edge case where readers can have WooCommerce Memberships with expiration dates but without subscriptions. Because Newspack hides the Memberships tab in My Account, there's no way for readers to get information about these memberships.
In this specific case, we don't want to create associated subscriptions for these memberships because they'll send out a bunch of renewal emails. Instead, we just want a way for readers to be able to see when their memberships are expiring, so they can proactively renew them.
See: 1200550061930446-as-1206901401819708
How to test the changes in this Pull Request:
I wrote these test steps as though you'd be using different browser sessions to act as the admin and reader, but you can also use the User Switching plugin.
npm run build
.Other information: