-
Notifications
You must be signed in to change notification settings - Fork 50
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
Switch Admin UI to generating in batches, using WC's BatchProcessor tool #136
Conversation
This way the progress bar will still update even when the browser window is not focused.
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.
Great work!
Approving. This could be merged right away but I left some mostly optional comments/nits that I thought you should see first of all.
A few other observations (not all are directly related to your work here, though I thought they were worth jotting down):
- Partly because of my settings, I found that generating a lot of products was bottle-necked by the slew of
woocommerce_run_product_attribute_lookup_update_callback
jobs that were consequently created. Perhaps this will be addressed by other work in progress for WC Core, however. - If I generate a set of products, then that control is disabled while product generation is in progress ... but the order generation button is also enabled. I'm not sure if this is deliberate, but I do think it could be handy for users to be able to set both in motion (whether or not the work happens concurrently, it would save them from having to remember to check back in and trigger the second piece of work).
Again, not stuff to address here, just capturing the thoughts.
Thank you, @coreymckrill!
<progress | ||
id="smoothgenerator-progress-bar" | ||
max="<?php echo esc_attr( $current_job->amount ); ?>" | ||
value="<?php echo $current_job->processed ? esc_attr( $current_job->processed ) : ''; ?>" | ||
style="width: 560px;" | ||
> |
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.
Very snazzy! 🎷
🤔 I wonder if it would be worth disabling that lookup table altogether when we start a product generation process. And then re-enabling it afterward? Maybe that would cause other issues? I guess maybe it would depend on what kind of testing you were planning to do with the products...
I think you mean the order generation button is also disabled? That's how it should be anyhow. Yeah, having multiple concurrent generation background tasks would probably be a nice enhancement, especially since you can already kind of do that with the CLI. Maybe something we could look at doing during #120 |
The same train of thoughts was going through my head. I'm not really sure what's best. Also not directly related to this work, so 🤷🏼 |
It's Friday and I can no longer type correctly, but yes. Thank you for logging it! |
Changes proposed in this Pull Request:
This changes the approach to generating stuff used in the admin UI. Instead of creating a separate Action Scheduler event for every product or order that should get generated, it leverages the batch methods introduced in #125 (and refined by subsequent PRs) to generate batches of items in one event. It also makes use of WooCommerce's BatchProcessor tool to handle batch management when more than one batch is needed (if you want to generate hundreds of products, for example).
To go along with this improved background generation process, this makes some improvements to the UI on the Smooth Generator page in WP Admin. It adds a progress bar while a generation job is running, which is updated periodically via WP's heartbeat system. Plus some other miscellaneous tweaks to improve the UX of the page.
Fixes #121
How to test the changes in this Pull Request:
Changelog entry