This is an Ubercart payment gateway which allows for integration of Ubercart with Element Payment Services' "Element Express" payment system (http://www.elementps.com/). There is currently no other Drupal integration with this payment system through Ubercart.

Project page: https://www.drupal.org/sandbox/daniel.moberly/2303615

Git clone command:
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/Daniel.Moberly/2303615.git ubercart_element_payment_gateway

Automatic Review :

http://pareview.sh/pareview/httpgitdrupalorgsandboxdanielmoberly2303615git

Comments

gwprod’s picture

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

Please ensure that your module passes automated tests.

Daniel.Moberly’s picture

Should now pass automated testing

gwprod’s picture

Status: Needs review » Needs work

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.

Daniel.Moberly’s picture

Project updated. Obviously I am a bit new to the translation stuff - never needed to do it before! Addressed your comments as follows:

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).

I have attempted to wrap all string literals as translatable where appropriate.

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

Updated the documentation for this function.

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.

I have moved the HTML outside of the translations.

On line 505, Your error message is passed as a replacement to t(). This defeats the purpose of t(). That is also true on lines 536, 570, 646, 661, 873, 912, 926

Fixed this by performing the translation on the initial error message rather than in the drupal_set_message calls

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

Added @param and @return documentation to custom functions.

Daniel.Moberly’s picture

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

Example:

$error = _uc_element_response($avs_transaction_response_code) . ': ' . $soap_avs_response->response->ExpressResponseMessage;
    drupal_set_message(check_plain($error), 'error');
    return array('success' => FALSE, 'message' => $error);

The $soap_avs_response object is a SOAP object returned from the payment processor and the ExpressResponseMessage would be a string value with the error message.

Daniel.Moberly’s picture

Status: Needs work » Needs review
gwprod’s picture

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

PA robot’s picture

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.

gisle’s picture

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.

Daniel.Moberly’s picture

That was my thought as well, but you are correct - the payment processor does change their error strings occasionally. Thanks for the input.

stitchzdotnet’s picture

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.

pingwin4eg’s picture

@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.

Daniel.Moberly’s picture

I have updated uc_element_uninstall to explicitly delete variables using variable_del rather than using a db_query command. I have also modified uc_element_form_alter to use format_date() instead of date().

Additionally, I have made formatting changes to uc_element.css and README.txt to better conform to guidelines.

naveenvalecha’s picture

Assigned: Unassigned » naveenvalecha

Assigning to myself for next review that may be tomorrow.

naveenvalecha’s picture

Assigned: naveenvalecha » Unassigned
Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +PAReview: Ubercart

Thanks for your contribution!

Automated Review

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

  • 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.

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 :)

Daniel.Moberly’s picture

Status: Needs work » Needs review

I have addressed all of the issues reported by naveenvalecha above. Thanks for the feedback!

To address the issue on permissions you listed under Secure Code, I have added a custom permission. However, I have added the permission check into the _uc_element_validate_user function rather than as an access callback as we still need to allow users to view/edit their own cards.

All issues under coding style have been addressed as requested

PA robot’s picture

Status: Needs review » Closed (duplicate)
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.