-
Notifications
You must be signed in to change notification settings - Fork 9
feat: auto-complete orders for OpenedX courses & virtual products, suppressing duplicate emails #117
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
base: main
Are you sure you want to change the base?
Conversation
…ppressing duplicate emails
24e9538
to
b22a884
Compare
@MaferMazu kindly review this PR, 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.
@amirnafees88 thanks for this contribution!
I tested it and it works. Furthermore, I think we can implement this more effectively if we treat the Open edX course product as a virtual product, which I believe is the root of the problem (with this change, the email and order status should not generate issues). Additionally, WooCommerce will treat the product as virtual, which is more accurate in our case.
@amirnafees88 @muhammadali286 I'll open an issue regarding this, but I really would like your thoughts about this.
* | ||
* @return bool | ||
*/ | ||
public function disable_processing_email_for_virtual_and_courses( $enabled, $order ) { |
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.
When I saw this, my first thought was that we shouldn't modify the email behavior of other items that are not Open edX courses. So I try to simplify the logic of this function, and I obtain that the best way is to ask if $analysis['has_physical_products'] then enable the email, else: disable
. However, it is not clear with this approach that I am only modifying the open edx course behavior (because here you can't see that the base behavior of the virtual and downloadable items has this email disabled).
In summary, this fix is good, but I think it is better to make improvements in this plugin so that WooCommerce treats the Open edX Course as a virtual product, which is more accurate. What do you think @amirnafees88?
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.
I know we previously discussed how to implement this feature, but upon implementation, I realized that we are not specifically checking if the enrollment was successful (that could be more complicated); instead, we only verify if the enrollment exists. As a result, the behavior of the order status and email is more similar to that of a virtual product, and perhaps the answer lies in that approach.
Issue: #119
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.
@MaferMazu, I am already handling the check for Open edX Course, Virtual, Downloadable, and Physical products in my code lines(573 to 586).
if ( $analysis['has_physical_products'] ) {
return $enabled;
}
if ( $analysis['has_openedx_courses'] ) {
$enrollment_metadata = $this->get_enrollment_metadata( $order_id, $analysis['items'] );
if ( empty( $enrollment_metadata ) ) {
return false;
}
}
if ( $analysis['has_regular_virtual_products'] ) {
return false;
}
About making the Open edX Course as a Virtual product, I added comments in this issue.
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.
@amirnafees88 thanks a lot for your answer in #119. I mixed concepts, but focusing on the email issue, the product type that doesn't pass through the "processing" state is the downloadable one. So that's the only product type that doesn't send the processing email by default.
So I suggest changing:
if ( $analysis['has_regular_virtual_products'] ) {
return false;
}
To $enabled
.
And regarding the conditions for sending or not sending the processing email for an Open edX course, it is our decision. It makes sense to me that we checked the enrollment before deciding whether to send or not the emails. But it wouldn't sound like a bad idea to me to turn it off if there are only open edx courses, regardless of whether the enrollment was successful or not, and manage failed registrations with a new feature so that if there is an error with the enrollment, an email is sent to the store owner notifying them, and that's it (out of the scope of this PR).
However, my primary concern is modifying the behavior of products that are outside our scope.
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.
@MaferMazu Thank you. I’ll handle it for the Open edX course only, without alter the default WooCommerce behavior for other product types.
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.
I'm sorry for my late review of this.
I have been thinking a lot about how to improve this behavior, as the goal is to manage order status and email sending without altering the WC behavior for the rest of the product. We want to ensure that we scope our work to what is related to the Open edX course products, to avoid unwanted changes in behavior that are outside our scope.
Regarding the order status
- As it is right now, we are changing the behavior of the virtual products (if we have virtual products without the downloadable check), they should stay in processing. - Issue in lines 549 and 474. Argument: #119 (comment)
- About the enrollment check based on line 542, we are only checking that at least one of the courses has an enrollment associated with it. So, if I have three courses in my order and only one enrollment, it will be marked as complete.
- Related to enrollments, we only check that we have the object created, not if it was successful. It is possible that I make the purchase and the enrollment request is created, but the API response is 500, or the user cannot be found and the enrollment is not performed, or the enrollment date has already passed, and I didn't have force enrollment enabled.
Something we can have is:
Marked as completed only when the order contains successful course enrollments or downloadable products. For each product, check whether it is an Open edX course, downloadable, a physical item, or a virtual product. The order will only be auto-completed if all items are either courses with successful enrollments or downloadables. If any course enrollment fails, or if the order includes products requiring manual attention (like physical or virtual products), the order remains in processing, ensuring that only fully and successfully fulfilled orders are automatically completed.
Perhaps it would be a good idea to have this process as part of the enrollment creation process, to check immediately when the enrollments are being created. Here: https://github.com/openedx/openedx-wordpress-ecommerce/blob/main/admin/class-openedx-commerce-admin.php#L432
Or at least relate them, but only if it helps.
What do you think?
Regarding the email
My only comment here is that we should change the behavior (turn off the processing email) only if it is an open edx course, because we are currently turning off emails for the virtual-only products. (line 474)
Summary
Current Issues:
Proposed Solution:
Closing Note: Regarding emails, I’ll handle them for Open edX course products, and I agree with your approach. Please let me know if this approach works for you so I can proceed with the implementation. |
@amirnafees88 thanks for the summary. You nailed it. ✨ About the proposed solution for the order status:
I would take into account that this is true, only if I have only open edx courses in my order, or if I have a combination of open edx courses + (virtual downloadable products).
Checking the documentation, WooCommerce handles the completion if the order only contains virtual downloadable products https://woocommerce.github.io/code-reference/classes/WC-Order.html#method_needs_processing (the virtual-only and downloadable-only still need to be processed)
Totally. And regarding the email part:
Yes, we should scope our custom behavior. |
Description
This PR updates the order processing flow to automatically mark orders containing OpenedX courses or regular virtual/downloadable products as Completed.
If an order contains physical products, it will remain in the Processing status until completed manually.
Additionally, this PR disables the Order Processing email for orders that are auto-marked as Completed (to avoid sending both "Processing" and "Completed" emails to the purchaser at the same time).
Reasoning for Email Change:
When an order is marked as Completed immediately (OpenedX courses or virtual products only), sending both the Processing and Completed emails creates unnecessary duplication for the purchaser. This PR ensures only the Completed email is sent in such cases.
Testing instructions
Single Item Combinations:
Two-Item Combinations:
Three-Item Combination: