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

Introducing Logging for Customer Product Downloads and Advanced Reporting #16418

Closed
wants to merge 28 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@procifer
Contributor

procifer commented Aug 11, 2017

Completes #12517

Adds a new WC_Customer_Download_Log class, and supporting data store, database table, etc. When customer downloads take place, the events are now tracked in the log table. Additional reporting in the main section of WC reports is now available to view download logs filtered by a given permission.

Also includes unit tests for the new WC_Customer_Download_Log class, as well as the new functionality in WC_Customer_Download.

This is from my fork of the repo, so is against the master branch. Once ready for merging, this should likely go into a feature branch. Also, it targets version 3.3.0, but please review the target version once ready for merging.

@claudiosanches

This comment has been minimized.

Show comment
Hide comment
@claudiosanches

claudiosanches Aug 12, 2017

Member

This is a little big and there is some changes that does not sounds related to the main issue. So for sure will be hard to review each line of this PR.
Thanks for your help.

Member

claudiosanches commented Aug 12, 2017

This is a little big and there is some changes that does not sounds related to the main issue. So for sure will be hard to review each line of this PR.
Thanks for your help.

@procifer

This comment has been minimized.

Show comment
Hide comment
@procifer

procifer Aug 12, 2017

Contributor

@claudiosanches thanks for taking a look! Yea, it's fairly involved so definitely let me know if you have any questions or need me to explain anything in details as you go through. Also, since I'm on trial, this is my first PR so definitely let me know if there's a better approach to organizing it, i.e. if you think certain parts should be on their own, etc. Thanks!

Contributor

procifer commented Aug 12, 2017

@claudiosanches thanks for taking a look! Yea, it's fairly involved so definitely let me know if you have any questions or need me to explain anything in details as you go through. Also, since I'm on trial, this is my first PR so definitely let me know if there's a better approach to organizing it, i.e. if you think certain parts should be on their own, etc. Thanks!

@mikejolley

Leaving code review feedback. I've not 'tested' the functionality yet, just gone through the PR :)

Show outdated Hide outdated includes/abstracts/abstract-wc-product.php
Show outdated Hide outdated includes/abstracts/abstract-wc-product.php
Show outdated Hide outdated includes/admin/class-wc-admin-reports.php
Show outdated Hide outdated includes/admin/meta-boxes/views/html-order-download-permission.php
Show outdated Hide outdated includes/admin/reports/class-wc-report-downloads.php
@@ -514,7 +514,7 @@ private static function get_schema() {
) $collate;
CREATE TABLE {$wpdb->prefix}woocommerce_downloadable_product_permissions (
permission_id BIGINT UNSIGNED NOT NULL auto_increment,
download_id varchar(32) NOT NULL,
download_id varchar(36) NOT NULL,

This comment has been minimized.

@mikejolley

mikejolley Aug 15, 2017

Member

Not a problem afaik but reason?

@mikejolley

mikejolley Aug 15, 2017

Member

Not a problem afaik but reason?

This comment has been minimized.

@procifer

procifer Aug 22, 2017

Contributor

Yea, this is due to the longer UUIDs we'll be using, which are 36 chars long, instead of the 32 char MD5 previously.

@procifer

procifer Aug 22, 2017

Contributor

Yea, this is due to the longer UUIDs we'll be using, which are 36 chars long, instead of the 32 char MD5 previously.

Show outdated Hide outdated includes/class-wc-install.php
Show outdated Hide outdated includes/class-wc-post-data.php
Show outdated Hide outdated includes/class-woocommerce.php
Show outdated Hide outdated woocommerce.php

@mikejolley mikejolley added this to the 3.3.0 milestone Aug 22, 2017

@procifer

This comment has been minimized.

Show comment
Hide comment
@procifer

procifer Aug 22, 2017

Contributor

@mikejolley and @mjangda, thanks for the code reviews; great feedback! I will be working on these changes this week and hope to have them wrapped up soon. Thanks!

Contributor

procifer commented Aug 22, 2017

@mikejolley and @mjangda, thanks for the code reviews; great feedback! I will be working on these changes this week and hope to have them wrapped up soon. Thanks!

@procifer

This comment has been minimized.

Show comment
Hide comment
@procifer

procifer Aug 23, 2017

Contributor

@mikejolley and @mjangda, thanks again for the reviews! I submitted some new changes which should address all the code review feedback. All unit tests should be passing and I tested new installations as well as upgrades, so things should be working properly. Let me know what next steps should be and/or if you see any other revisions to work on. Thanks!

Contributor

procifer commented Aug 23, 2017

@mikejolley and @mjangda, thanks again for the reviews! I submitted some new changes which should address all the code review feedback. All unit tests should be passing and I tested new installations as well as upgrades, so things should be working properly. Let me know what next steps should be and/or if you see any other revisions to work on. Thanks!

@mikejolley mikejolley requested a review from claudiulodro Sep 5, 2017

@mikejolley

This comment has been minimized.

Show comment
Hide comment
@mikejolley

mikejolley Sep 5, 2017

Member

Code looks good to me. I've requested a final review from @claudiulodro to check over everything.

Few minor changes after seeing how this all works:

  • In the download report, when there is no user, output "guest".
  • Would it be useful to break out some stats on the main download reports page e.g. most downloaded, most active IP/user, with the ability to view by those too?
  • I think we should tweak the wording and tab on which this report is found. I'm wondering if products is really the most appropriate place? Suggestions?
  • I wonder if we should call them 'customer downloads' to reflect that these are downloads from customers and require permissions.

@mjangda

Member

mikejolley commented Sep 5, 2017

Code looks good to me. I've requested a final review from @claudiulodro to check over everything.

Few minor changes after seeing how this all works:

  • In the download report, when there is no user, output "guest".
  • Would it be useful to break out some stats on the main download reports page e.g. most downloaded, most active IP/user, with the ability to view by those too?
  • I think we should tweak the wording and tab on which this report is found. I'm wondering if products is really the most appropriate place? Suggestions?
  • I wonder if we should call them 'customer downloads' to reflect that these are downloads from customers and require permissions.

@mjangda

@procifer

This comment has been minimized.

Show comment
Hide comment
@procifer

procifer Sep 5, 2017

Contributor

@mikejolley Thanks so much for the additional review! And great ideas. I can definitely make those revisions. Since this is approved now, should I make those revisions in a separate PR? Or would those be appropriate as additional commits on this one?

  • Re: breaking out the most downloaded, active IP/user, etc., are you thinking that should appear on the top of the report regardless of the current filter state? Or what if we have a separate sub-tab for a "Summary" view (similar to how the other report tabs work)? Then those summary lines could link to a filtered "Detail" view. This might take a bit more time though; should we put this in a subsequent PR to keep this one a little simpler? (Happy to put in here too, just wondering about best practice 😄 )

  • Excellent question re: the labeling and position in the reports UI. I struggled with that decision too. I didn't want to put them under the other taxonomies since it's definitely a separate data set we're talking about, but I totally agree that the "Products" heading isn't perfect either. I hate to clutter the top level tabs, but maybe just a "Customer Downloads" top level might be most appropriate. Then we could support those details/summary tabs I was mentioning under that. Would that work?

Thanks again for the feedback! Let me know best next steps.

@mjangda

Contributor

procifer commented Sep 5, 2017

@mikejolley Thanks so much for the additional review! And great ideas. I can definitely make those revisions. Since this is approved now, should I make those revisions in a separate PR? Or would those be appropriate as additional commits on this one?

  • Re: breaking out the most downloaded, active IP/user, etc., are you thinking that should appear on the top of the report regardless of the current filter state? Or what if we have a separate sub-tab for a "Summary" view (similar to how the other report tabs work)? Then those summary lines could link to a filtered "Detail" view. This might take a bit more time though; should we put this in a subsequent PR to keep this one a little simpler? (Happy to put in here too, just wondering about best practice 😄 )

  • Excellent question re: the labeling and position in the reports UI. I struggled with that decision too. I didn't want to put them under the other taxonomies since it's definitely a separate data set we're talking about, but I totally agree that the "Products" heading isn't perfect either. I hate to clutter the top level tabs, but maybe just a "Customer Downloads" top level might be most appropriate. Then we could support those details/summary tabs I was mentioning under that. Would that work?

Thanks again for the feedback! Let me know best next steps.

@mjangda

@mjangda

Flagged a couple of minor things but overall this looks good.

// Subtitle for permission if set.
if ( isset( $_GET['permission_id'] ) && is_numeric( $_GET['permission_id'] ) && $_GET['permission_id'] > 0 ) {
$permission_id = $_GET['permission_id'];

This comment has been minimized.

@mjangda

mjangda Sep 5, 2017

Contributor

Minor: if we expect an integer here should we use intval or absint?

@mjangda

mjangda Sep 5, 2017

Contributor

Minor: if we expect an integer here should we use intval or absint?

try {
$permission = new WC_Customer_Download( $permission_id );
$product = wc_get_product( $permission->product_id );
} catch ( Exception $e ) {

This comment has been minimized.

@mjangda

mjangda Sep 5, 2017

Contributor

We're using a generic exception here; will that always mean that the permission wasn't found? Or could other errors creep in here too? (Note: I haven't looked too closely into either WC_Customer_Download or wc_get_product)

@mjangda

mjangda Sep 5, 2017

Contributor

We're using a generic exception here; will that always mean that the permission wasn't found? Or could other errors creep in here too? (Note: I haven't looked too closely into either WC_Customer_Download or wc_get_product)

break;
}
edit_post_link( $product->get_formatted_name(), '', '', $product->get_id() );

This comment has been minimized.

@mjangda

mjangda Sep 5, 2017

Contributor

Minor: for consistency with order, can we move this inside the if and reverse the condition?

@mjangda

mjangda Sep 5, 2017

Contributor

Minor: for consistency with order, can we move this inside the if and reverse the condition?

// Ensure the permission and product exist.
try {
$permission = new WC_Customer_Download( $_GET['permission_id'] );

This comment has been minimized.

@mjangda

mjangda Sep 5, 2017

Contributor

To be on the safe side, we should assign $_GET['permission_id'] to a variable and cast using intval or absint.

@mjangda

mjangda Sep 5, 2017

Contributor

To be on the safe side, we should assign $_GET['permission_id'] to a variable and cast using intval or absint.

@claudiulodro

Overall everything looks good and works well. Good job keeping the code consistent with the rest of the WooCommerce code and following WordPress best practices.

Just a couple small changes that haven't been brought up yet.

*
* @return int Log ID
*/
public function save() {

This comment has been minimized.

@claudiulodro

claudiulodro Sep 5, 2017

Contributor

This method is exactly the same as WC_Data::save. You can delete it and just let the parent method do the work.

@claudiulodro

claudiulodro Sep 5, 2017

Contributor

This method is exactly the same as WC_Data::save. You can delete it and just let the parent method do the work.

// Convert timestamp to WC_DateTime using ISO 8601 for PHP 5.2 compat.
$dtStr = date("c", $set_to);
$wc_timestamp = new WC_DateTime($dtStr);

This comment has been minimized.

@claudiulodro

claudiulodro Sep 5, 2017

Contributor

Nitpick: Formatting

@claudiulodro

claudiulodro Sep 5, 2017

Contributor

Nitpick: Formatting

@mikejolley

This comment has been minimized.

Show comment
Hide comment
@mikejolley

mikejolley Sep 6, 2017

Member

Re: breaking out the most downloaded, active IP/user, etc., are you thinking that should appear on the top of the report regardless of the current filter state? Or what if we have a separate sub-tab for a "Summary" view (similar to how the other report tabs work)? Then those summary lines could link to a filtered "Detail" view. This might take a bit more time though; should we put this in a subsequent PR to keep this one a little simpler?

New PR on top of this one may be wise so it can be reviewed separately. I was thinking the 'main' view before filtering could just show show stats like the other overview reporting sections, the difference being rather than charts the main section is a table as you have now? Keeping it to one screen - this is only a small part of WC as a whole.

Excellent question re: the labeling and position in the reports UI. I struggled with that decision too. I didn't want to put them under the other taxonomies since it's definitely a separate data set we're talking about, but I totally agree that the "Products" heading isn't perfect either. I hate to clutter the top level tabs, but maybe just a "Customer Downloads" top level might be most appropriate. Then we could support those details/summary tabs I was mentioning under that. Would that work?

I'd probably think it fits under either orders or customers. You could argue either way. I think orders.

Member

mikejolley commented Sep 6, 2017

Re: breaking out the most downloaded, active IP/user, etc., are you thinking that should appear on the top of the report regardless of the current filter state? Or what if we have a separate sub-tab for a "Summary" view (similar to how the other report tabs work)? Then those summary lines could link to a filtered "Detail" view. This might take a bit more time though; should we put this in a subsequent PR to keep this one a little simpler?

New PR on top of this one may be wise so it can be reviewed separately. I was thinking the 'main' view before filtering could just show show stats like the other overview reporting sections, the difference being rather than charts the main section is a table as you have now? Keeping it to one screen - this is only a small part of WC as a whole.

Excellent question re: the labeling and position in the reports UI. I struggled with that decision too. I didn't want to put them under the other taxonomies since it's definitely a separate data set we're talking about, but I totally agree that the "Products" heading isn't perfect either. I hate to clutter the top level tabs, but maybe just a "Customer Downloads" top level might be most appropriate. Then we could support those details/summary tabs I was mentioning under that. Would that work?

I'd probably think it fits under either orders or customers. You could argue either way. I think orders.

@mikejolley mikejolley referenced this pull request Nov 13, 2017

Merged

Download reporting #17679

1 of 1 task complete
@mikejolley

This comment has been minimized.

Show comment
Hide comment
@mikejolley

mikejolley Nov 13, 2017

Member

Continuing this PR here #17679

Member

mikejolley commented Nov 13, 2017

Continuing this PR here #17679

@mikejolley mikejolley closed this Nov 13, 2017

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