[D7] Ubercart Element Payment Gateway

Fails automated testing.
http://pareview.sh/pareview/httpgitdrupalorgsandboxdanielmoberly2303615git

Please ensure that your module passes automated tests.

Please make sure your string literals are wrapped in t(), such as 'Stored Card' on line 74. (there are a LOT of these that are incorrect).

uc_element_delete_form doesn't implement hook_form. hook_form is a Node API hook, not a Form API hook.

don't use HTML in t(), such as on line 125. And 'em' is better than 'i'. This is also true of
on line 687.

On line 505, Your error message is passed as a replacement to t(). This defeats the purpose of t().

$error = 'Please choose a valid payment method';
    drupal_set_message(t('@error', array('@error' => $error)), 'error');

That is also true on lines 536, 570, 646, 661, 873, 912, 926

It is a good practice to always include @param for parameters unless directly implementing an API defined hook.

Wrapping your responses in t() and then issuing them through drupal_set_message should be fine.

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.

Additional - any thoughts on how to handle translating error messages returned by the payment processor? Currently I am just returning those as untranslated strings.

In practice, that's all you can do. t() only works with static strings, not variables.

When not passing through t() it is important to sanitize the string (it looks as if you already do this, I just mention it for the record).

You could have anticipated all the strings returned by the payment processor and then created a dummy function to wrap those in t() to facilitate translation - but that would be overkill and would also break if the payment processor in the future added or changed strings.

Automated Review

Best practice issues identified by pareview.sh, these should be corrected.

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/README.md

No: Follows the guidelines for in-project documentation and the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes.

Coding style & Drupal API usage

1. (+) uc_element_uninstall(): You should not use db_query to delete data, use db_delete instead. You could do something like this: db_delete('variable')->condition('name', db_like('uc_element_') . '%', 'LIKE')->execute(); Then make sure to clear the cached variables, i.e. cache_clear_all('variables', 'cache_bootstrap');. Alternatively, you could loop through your variables and call variable_del($name) for each.
2. uc_element_form_alter(): Use format_date() instead of date().

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.

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.

@stitchzdotnet wrote:

You could do something like this: db_delete('variable')->condition('name', db_like('uc_element_') . '%', 'LIKE')->execute(); Then make sure to clear the cached variables, i.e. cache_clear_all('variables', 'cache_bootstrap');

THIS IS WRONG! Do not ever do like that. Every module must delete its variables with variable_del() no matter how many them in the system.

Assigning to myself for next review that may be tomorrow.

Thanks for your contribution!

Automated Review

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Maybe: Meets the security requirements.

1. _uc_element_validate_user : This function has hard-coded administer role name.That might cause security implications.I would suggest to provide a seperate permission by the module and add that in access arguments of the menus user/%/cards , user/%/cards/delete .

Coding style & Drupal API usage

1. (*)Use #attached instead of adding css/js in the .info file #attached to add the CSS file where these actually required.
2. uc_element_user_card_list : drupal_set_title(t("@user's Stored Credit Cards", array('@user' => $user->name)));use single quotes where possible for Drupal code standards and a very slight performance benefit.
3. uc_element_delete_form : Remove the html from the translation function $form['areyousure'] = array('#markup' => t('Are you sure you want to delete this card?<br/>'));
4. (*)uc_element_charge :
   

       $error = t('Please choose a valid payment method');
       drupal_set_message(check_plain($error), 'error');

   No need of check_plain here.because the text is not coming from 3rd party and allso t function is doing sanitization for you.The usage of check_plain is wrong at lot of places.

5. _uc_element_create_pass : Use drupal_strtolower instead of strtolower.
6. _uc_element_create_pass : Use drupal_substr instead of substr.Similarly in the function _uc_element_generate_pass_account, _uc_element_generate_pass_request,_uc_element_generate_request
7. _uc_element_create_pass : Use drupal_strlen instead of strlen.
8. _uc_element_card_get : Use db_query for simple queries.
9. uc_element.rules.inc : Use format_date instead of date.

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.

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.

I would recommend you, please help to review other project applications to get a review bonus back. This will put you on the high priority list, then git administrators will take a look at your project right away :)

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: https://www.drupal.org/node/2503291

Project 2: https://www.drupal.org/node/2303639

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

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