[D7] User recurring subscription

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxratanphp2304373git

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

@ratanphp, thanks for contribution!

Manual Review:
1. Your module looks so complex but did not contains hook_help(), it should be there.
2. In user_recurring_subscription.module file, most of functions lacking comments like user_recurring_subscription_load() and user_recurring_subscription_form_user_register_form_alter() etc, as it should be.
3. At most of places you are using SELECT query with '*' instead of fetching specific fields, please add your comment on this.
4. In user_recurring_subscription_nvp_to_array() function, you are passing curl response, it should be sanitized using check_plain or some other recommend functions for safe output.
5. In following function you are calling user_recurring_subscription_profile() function where you checked if (!empty($profile->profile_id)) { but

function user_recurring_subscription_user_view($account, $view_mode, $langcode) {
  $profile = user_recurring_subscription_profile($account->uid);
  if (!empty($profile->profile_id)) {
  ....
  }
}

this function does not return object in every case as you have used return statement inside if condition. I think either you use isset instead of !empty in above function and also return some variable outside if statement in below function.

function user_recurring_subscription_profile($uid) {
  if (!empty($uid)) {
    $object = db_query('SELECT * FROM {recurring} WHERE uid=:uid', array(':uid' => $uid))->fetchObject();
    return $object ;
  }
}

6.You are using db_update function but still using UPDATE command that looks wired. Please fix it.

function user_recurring_subscription_user_delete($account) {
  $profile = user_recurring_subscription_profile($account->uid);
  if (!empty($profile->profile_id)) {
   if (user_recurring_subscription_profile_cancel($profile_id)) {
      db_update('UPDATE {recurring} SET status=:status WHERE profile_id=:profile_id', array(':status' => 2, ':profile_id' => $profile->profile_id));
    }
  }
} 

You must follow correct method to use this db_update(). It should be something like this:

if (user_recurring_subscription_profile_cancel($profile_id)) {
      db_update('recurring')
       ->fields(array(
    'status' => 2,
  ))
  ->condition('profile_id', $profile->profile_id)
  ->execute();
    }

Also user_recurring_subscription.test file is empty yet, I think you are planning to write some test cases for your module. This is good thing as your module little bit complex. Go ahead!

Thanks Again!

Looks Great Improvement!

Automated Review

Still number of notices are reported by http://pareview.sh/pareview/httpgitdrupalorgsandboxratanphp2304373git.
Coder Sniffer has found some issues with your code (please check the Drupal coding standards).

Manual Review

Individual user account
Yes: Follows the guidelines for individual user accounts.
No duplication
Yes: Does not cause module duplication and fragmentation.
Master Branch
Yes: Follows the guidelines for master branch.
Licensing
Yes: Follows the licensing requirements
3rd party code
Yes: Follows the guidelines for 3rd party code.
README.txt
Yes: Follows the guidelines for in-project documentation and the README.txt Template.
Code long/complex enough for review
Yes: Follows the guidelines for project length and complexity.
Secure code
Yes: Using check_plain and other recommended function required for safe output.
Coding style & Drupal API usage
Looks Good.

- You are using placeholder in all your custom queries that's quite good.
- Good use of inline comments.
- Readme.txt file contains small information, it would be good to extended it little bit more.

Currently there is no blocker from my end except automated reviews report. It would be good if you fix these on priority and let other reviewer reviews your code.

Personally I'll move this to RTBC once these issues get fixed!

Thanks again for contribution!

Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Can you outline how this is different than existing subscription modules, in particular ones that integrate with either Ubercart or Drupal Commerce?

Assigning to myself for next review.

Automated Review

Review of the 7.x-1.x branch (commit e67cf42):

Lots of PAReview errors and warnings: http://pareview.sh/pareview/httpgitdrupalorgsandboxratanphp2304373git

Please go through these and see what are real and what are false, and also read https://www.drupal.org/coding-standards

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Maybe: Does not cause module duplication and fragmentation. There are some corssover with UC and Commerce, but also differences
I would say no to duplication, but I want other admins to weigh in before final review.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

No: Doesn't follows the guidelines for 3rd party code. Where did the logos come from? Those are all registerred trademarks. They need to go.

README.txt/README.md

No: Doesn't follow the guidelines for in-project documentation and the README Template.

There is no info on how to use or confgure this, or how it works.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

No. If "no", list security issues identified.

In user_recurring_subscription_menu(), The administration menu callback should probably use "administer site configuration" - which implies the user can change something - rather than "access administration pages" which is about viewing but not changing configurations. A separate permission would be good, too.

md5() should not be used for anything security related. You should use drupal_hmac_base64() instead. Can you just use drupal_get_token() instead?

Is there no other way to use the Payflow API other than storing the username and password in the database? The password would be in plain text this way.

In your template file, you should really do the plaining in a preprocess. You should use placeholders intead of string concatenation with t(). I can't totally trace this out, but I think fee, price, and interval unit(?) may be going straight to output from user input.
The t() will plain these for you.

Coding style & Drupal API usage

You should leverage classes namedspaced to your module instead of IDs for CSS. Yours are too generic and may have namespace clash.

(+) Your variables aren't namespaced to your module. What if someone want to make a spearate payflow module?

Do any of those configuration options need to be required? If so, use #required in the form.

Do you mean "vendor" instead of "vandor"?

In theme_user_recurring_subscription_card_cvv_help(), Drupal behaviors are generally preferred over onclicks. You may also want to look into ctools modals instead of a popup.

When making links in t(), it is best to just pass in the URL. See https://api.drupal.org/api/drupal/includes%21common.inc/function/l/7

(+) In user_recurring_subscription_permission, your permission is too vague, both the machine name and title. You should also add a description.

user_recurring_subscription_currency_array() needs a proper docblock.

What is user_recurring_subscription_load() doing? That is a node hook. See https://api.drupal.org/api/drupal/modules!node!node.api.php/function/hoo...

Your schemas should have indexes for the columns that reference foreign keys (uid, etc).

user_recurring_subscription_form_user_register_form_alter implements hook_form_FORM_ID_alter().

Using #attached is preferred over drupal_load_css().

You should use drupal_http_request() instead of curl(), and drupal_http_build_query() for building up the query string.

Use ip_address() instead of $_SERVER['REMOTE_ADDR'];

Use REQUEST_TIME instead of time().

Don't check_plain() in user_recurring_subscription_nvp_to_array(), you only plain for output. See https://www.drupal.org/node/28984

In user_recurring_subscription_user_view(), build up a render array instead of calling theme() directly.

In user_recurring_subscription_add_form(), #default_value already gets plained. See link above.

Why are you resetting the $form_state['values']['description'] in user_recurring_subscription_add_form_validate()?

user_recurring_subscription_cvv_info() is not a good idea. Either use inline help or a ctools modal.

user_recurring_subscription_card_form() is a form builder, not a hook_form.

What is the array_walk for in user_recurring_subscription_card_form_validate? You don't seem to be outputting any of that.

I also may be dense here, but I don't see how the role that the user is purchasing actually gets saved to their account, or removed when they cancel the subscription? There are no calls to user_save() anywhere.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

user_recurring_subscription_manage() should return a render array and not themed output.

I decided to not do a second visual pass through this like I typically do, as the above list is substantial enough.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

Assigning to myself for next review.

Started review, but realized that the logos are still in the repo (the images/ subdirectory). Where did they come from? Those are all registered trademarks, so I think they need to go unless you can point to acceptable language that says they are usable.

I will continue with a review of a1d3c1b in the meantime. This is a decently length module, so it will take a little while.

Sorry for the delay, but with volunteer efforts, life/work gets in the way sometimes...

Automated Review

Git default branch is not set, see the documentation on setting a default branch.
Review of the 7.x-1.x branch (commit a1d3c1b):

• DrupaSecure barfed, but it looks like Coder was clean.
• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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. You have to get a review bonus to get a review from me.

Manual Review

See previous review in #12 for non-code related points.

Menu permissions are better.

Token is better.

I am going to defer on the password thing for a second opionion, but I think this is OK if there is no option and the documentation
clearly states that the username and password is stored in the database.

Variable names look better.

(+) I still think your permission machine names could be better.

Blocking issues from #12 have been addressed.

Make form fields required instead of validating them to be not empty.

You should use form_set_value() instead of directly updating $form_state['values'] in user_recurring_subscription_add_form_validate(). It also doesn't seem like you really need to do this?

(*) You also don't need to filter_xss() there. You filter when you output. https://www.drupal.org/node/28984

(*) You don't need to check_plain() in user_recurring_subscription_form_validate(). These are not being output here.

(+) I don't see what you are updating form state in user_recurring_subscription_form_submit(). Can't you just use one submit handler?

(*) In user_recurring_subscription_recurring(), it looks like $subscription->title goes from user input to output directly. This is XSS prone.

Automated Review

Git default branch is not set, see the documentation on setting a default branch.

Review of the 7.x-1.x branch (commit 588fe0d):

>No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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. You have to get a review bonus to get a review from me.

Manual Review

My blocking issues from #19 have been addressed.

I am not seeing any additional blocking issues, so I am setting RTBC. It needs a second set of eyes from a review admin, though, before approval.

OK, 13 days w/o further comments. No committed changes since my review in #20, so...

Thanks for your contribution, ratanphp!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, also, for your 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.

Thanks to the dedicated reviewer(s) as well.