UC File download quota

A couple of us looked at the code for 15 minutes and it makes good use of the Drupal API. Neither of us is experienced with the Ubercart codebase to RTBC this, so it would be great if someone familiar with the Ubercart internals could review:

1. The code around the comment that says:

        // FIXME ! Need better integration with uc_recurring to grab this from uc_recurring_user table

which directly queries the uc_recurring_users table.

2. The other direct queries of Ubercart tables: uc_file_products, uc_files_products (is this the same as uc_file_products?), uc_file_users. Since Ubercart has been around for a while and has a core API it seems like it would be good to utilize the API as much as possible.

3. The ddr_check_access() function is doing a lot which makes it hard to review. Is it possible to break this down into more manageable chunks? Possibly pull out the "check if the user subscription is still valid" portion into a separate function?

4. Running the code through Coder would be great. It will point out some coding style issues.

Sounds good. Looks like the status is still "needs review" so if you are working on it and don't want it to be reviewed again until it's ready, please change the status to "needs work." If you'd like to have it looked at again now, I wonder if there's a way to reach out to the Ubercart community to find someone to look at this.

'Drupie'?

I think it's better to have module names that are descriptive rather than idiosyncratic... surely something like uc_file_download_quota would describe what it does and be unique.

I agree with @joachim on this one. Now's the time to change it before everyone starts using it.

I'm with joachim on something like uc_file_download_quota as this is relevant to how Ubercart modules are commonly named. With the ever expanding list of modules hosted on drupal.org, it is becoming difficult enough for users to find the module they're looking for. I absolutely agree that it's best to provide a descriptive name like this especially when it integrates with another module like Ubercart. uc_what_this_module_does is the common format here.

Updating tags

Needs work to conform to Drupal coding standards. Read http://drupal.org/coding-standards .

Also, please ensure you've reviewed the documentation on the Review Process for Full Project Applications: What To Expect and Tips for Ensuring a Smooth Review pages before marking your project as 'needs review'.

* do not include the "version" in the info file, see http://drupal.org/node/231036
* README.txt is missing
* uc_file_download_quota.install: extra "*" on line 35 is not needed
* the comments on top of uc_file_download_quota.module belong in the README
* "Implementation of hook_perm" ==> "()" is missing
* same for hook_menu
* uc_file_download_quota_configuration_form() doc block needs to be fixed
* no newline between doc block and function definition
* theme_ddr_limit_form(): the element to be themed should be name spaced with your module name
* do not implement hook_init() to include a file!!! Inlude it when you need it.

README.txt still missing.

The repository hasn't been changed since July - the issues mentioned in #14 and #16 are all still in there.

* Git release branch missing, see http://drupal.org/node/1015226
* line breaks in README.txt are not correct ('\r'), you should use unix line endings. see http://drupal.org/node/318#indenting
* uc_file_download_quota_help(): there should be no empty line between function and code block. Also applies to other functions.
* hook_help(): All end user text in your module should be passed through t() for translation
* do not break code lines, they can be as long as needed
* "// unlimited user fix" Comments should start with a capital letter and end with a "."

* Push all your recent changes to the 6.x-1.x branch
* Remove the 6.x-1.x-dev branch to avoid confusion
* README.txt line endings are still broken in all branches

• lines in README.txt should not exceed 80 characters
• uc_file_download_quota_update_6100(): doc block formatting error, "**/" should be "*/"
• @file doc block missing, see http://drupal.org/node/1354#files
• uc_file_download_quota_uninstall(): hook implementation doc block missing
• uc_file_download_quota_help(): there should be no new line between function and doc block. Also for many others.
• ddr_check_access(): all functions in your module must be prefixed with your module name to avoid name clashes.
• ddr_check_access(): indentation errors for the "elseif" blocks at the end.
• uc_file_download_quota_theme(): indentation errors on array formatting, the closing ")," should be one level deeper.
• "//debug purposes" Remove all debugging code form the repository
• Array indentation errors in theme_uc_file_download_quota_limit_form().

Review of the 6.x-1.x branch:

• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Go and review some other project applications, so we can get back to yours sooner.

manual review:

• uc_file_download_quota_schema(): is hard to read, please use the usual formatting of the arrays on several lines.
• "Implements of hook_help()." should be "Implements hook_help().". Please check all you functions.
• "WHERE u.uid=%d": there should be a space before and after the "=".
• $b and $a are not really self explaining variable names. Better use something like $allowed_limit, $time_left, $remaining_seconds etc.
• "drupal_goto(variable_get('uc_file_download_quota_no_role_redirect', 'membership-plans'));": indentation error, this should be 2 spaces to the left.
• "$model = $form_state['clicked_button']['#post']['sku'];": do not access raw POST data, use $form_state['values'] instead.
• "$sql = "INSERT INTO {uc_file_download_quota} (fid, uid, timestamp) VALUES ('%s', %d, %d)";": you should use drupal_write_record() here.

Otherwise I think this is nearly ready.

Review of the 6.x-1.x branch:

• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Get a review bonus and we will come back to your application sooner.

FILE: ...all/modules/pareview_temp/test_candidate/uc_file_download_quota.install
--------------------------------------------------------------------------------
FOUND 15 ERROR(S) AND 4 WARNING(S) AFFECTING 15 LINE(S)
--------------------------------------------------------------------------------
 14 | ERROR   | Spaces must be used to indent lines; tabs are not allowed
 15 | ERROR   | Spaces must be used to indent lines; tabs are not allowed
 16 | ERROR   | Spaces must be used to indent lines; tabs are not allowed
 16 | WARNING | A comma should follow the last multiline array item. Found:
    |         | TRUE
 19 | ERROR   | Spaces must be used to indent lines; tabs are not allowed
 20 | ERROR   | Spaces must be used to indent lines; tabs are not allowed
 21 | ERROR   | Spaces must be used to indent lines; tabs are not allowed
 22 | ERROR   | Spaces must be used to indent lines; tabs are not allowed
 22 | WARNING | A comma should follow the last multiline array item. Found: 255
 25 | ERROR   | Spaces must be used to indent lines; tabs are not allowed
 26 | ERROR   | Spaces must be used to indent lines; tabs are not allowed
 27 | ERROR   | Spaces must be used to indent lines; tabs are not allowed
 28 | ERROR   | Spaces must be used to indent lines; tabs are not allowed
 28 | WARNING | A comma should follow the last multiline array item. Found: 255
 31 | ERROR   | Spaces must be used to indent lines; tabs are not allowed
 32 | ERROR   | Spaces must be used to indent lines; tabs are not allowed
 33 | ERROR   | Spaces must be used to indent lines; tabs are not allowed
 34 | ERROR   | Spaces must be used to indent lines; tabs are not allowed
 34 | WARNING | A comma should follow the last multiline array item. Found: 255
--------------------------------------------------------------------------------


FILE: .../all/modules/pareview_temp/test_candidate/uc_file_download_quota.module
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 254 | ERROR | Whitespace found at end of line
--------------------------------------------------------------------------------

Source: http://ventral.org/pareview - PAReview.sh online service

Manual review:

- Array doesn't follow coding standard http://drupal.org/coding-standards#array

    'fields' => array(
      'did' => array(
      	'type' => 'serial',
      	'not null' => TRUE,
      	'unsigned' => TRUE
      ),
      'fid' => array(
      	'type' => 'varchar',
      	'not null' => TRUE,
      	'default' => '',
      	'length' => 255
      ),
      'uid' => array(
      	'type' => 'int',
      	'not null' => TRUE,
      	'default' => '',
      	'length' => 255
      ),
      'timestamp' => array(
      	'type' => 'int',
      	'not null' => TRUE,
      	'default' => '',
      	'length' => 255
      ),
    ),

- Should add t function for the text for example:

    case 'admin/help#uc_file_download_quota':
      $output = "uc file download quota is a add-on module for uc_file which enable admin to limit product download
                 Per role and per time limit";
      return $output;

    case 'admin/modules#description':
      return 'file download quota is a add-on module for uc_file which enable admin to limit product download
                 Per role and per time limit';

I am not a Ubercart expert but here's what I've seen to be problematic:

1. Review of the 6.x-1.x branch:
   • Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See attachment.

   This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Get a review bonus and we will come back to your application sooner.

   FILE: .../all/modules/pareview_temp/test_candidate/uc_file_download_quota.module
   --------------------------------------------------------------------------------
   FOUND 4 ERROR(S) AFFECTING 4 LINE(S)
   --------------------------------------------------------------------------------
      5 | ERROR | Additional blank line found at the end of doc comment
    104 | ERROR | If the line declaring an array spans longer than 80 characters,
        |       | each element should be broken into its own line
    227 | ERROR | BREAK statements must be followed by a single blank line
    230 | ERROR | BREAK statements must be followed by a single blank line
   --------------------------------------------------------------------------------
   

   Source: http://ventral.org/pareview - PAReview.sh online service

2. You have an @file in your module file but it's quite rudimentary
3. I couldn't find it in the Coding Standards but I feel like your code could use a newline here and there. I like to use newlines before and after branches and loops to make them more prominent.
4. Description on the project page seems to be outdated, it says
   

   3. Install drupie_download_role, configure

I've checked out the 6.x-1.x branch and looked at your code. Some of your lines are too long (e.g. line 14 in the module file).

As I said I am not a Ubercart expert but the logic I've seen and understood seems to be good. Good Job!

Your project page is not very detailed, please have a look at the tips for a great project page, you may also use HTML-tags for better structure. Same with your README, make sure it follows the guidelines for in-project documentation (also have a look at the common Styles).

1. .module, 14, 19, 336:
   

   t('file download quota is a add-on module for uc_file which enable admin to limit product download
                    Per role and per time limit');

   , please don't do such linebreaks and indentation within t() strings. as these strings will be detected automatically for translations, this is really ugly to translate.

2. .module, 103: array('1' => 'Daily', '7' => 'Weekly', '30' => 'Monthly', '90' => 'Quarterly', '360' => 'Yearly'),, You should translate this strings with t(). - also if you want integers - use them like integers NOT like strings! Using them your way makes it necessary to cast these values everytime you want to calculate something with them.
3. uc_file_download_quota_check_access():
   

   return 1 if the user has no acceptable role
    * return 2 if the user quota is depleted
    * return 3 if user still has quota for download

   , using such hardcoded strings/numbers is a bad programming practice, please define proper constants.

4. Your function documentation is not very consistent, please make them follow the doxygen documentation standarts for functions. Also make sure all your comments start uppercase and end with . ? !
5. Please avoid using time() if not really necessary, REQUEST_TIME constant contains the timestamp of when the script was executed and provides the same information without producing higher load
6. uc_file_download_quota_uninstall(): Actually I don't think there's a need to use a wildcard query to delete all your variables as the names are pretty static, however, make sure to clear the variable caches properly if you want to do it your way (cache_clear_all('variables', 'cache');)
7. Are there other modules using the "Ubercart - extra" package?
8. you should add ubercart as dependency - if drush tries to lookup the submodule uc_file it is not able to download it because it does not know it's contained in ubercart

I was not able to enable your module:

Invalid default value for 'uid'
query: CREATE TABLE uc_file_download_quota (
`did` INT unsigned NOT NULL auto_increment, 
`fid` VARCHAR(255) NOT NULL DEFAULT '', 
`uid` INT NOT NULL DEFAULT '', 
`timestamp` INT NOT NULL DEFAULT '', 
PRIMARY KEY (did)
) /*!40100 DEFAULT CHARACTER SET utf8 */ in _db_query() (line 134 of
/home/patrickd/www/drupal-6-test/includes/database.mysqli.inc).

Because your setting a string ('') as default value for the integer field uid for your table.

Make sure to make a full test of your module's functionality after you applied changes! (Within installation and uninstallation!)

> 3. uc_file_download_quota_check_access(): return value; That is your personal opinion?

I'd say that's generally held standard practice...

Also:

return 1 if the user has no acceptable role
* return 2 if the user quota is depleted
* return 3 if user still has quota for download

Do you actually need 3 values? The main question is surely 'can this user download more?' and the answer is either yes or no -- you need return only TRUE or FALSE.

Note that drupal uses constants for this not 0 or 1
See
NODE_PUBLISHED
NODE_NOT_PUBLISHED

I won't argue about programming best practices here, the reason I did not rbtc this issue was because the module was simply not working. That this code was "written almost 2 years ago" is no reason that this module is not working correctly - this is an issue you always should take care of.

Seems ok for me, setting as RTBC.

aww, let's get this done

There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch,

git checkout 6.x-1.x
git branch -D master
git push origin :master

Would you mind cleaning up your project page text a bit?

Thanks for your contribution and finally welcome to the community of project contributors on drupal.org!

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best (however be careful with this).

Thanks, also, for your enormous patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.

Thanks to the dedicated reviewer(s) as well.