Ubercart Paymill Integration

Module for Ubercart integration with Paymill payment gateway.

Sandbox project: https://drupal.org/sandbox/topsitemakers/1846242

Git clone command: git clone http://git.drupal.org/sandbox/topsitemakers/1846242.git uc_paymill

Automatic code review results

Coder and [pareview.sh] checks are already performed and reported only 2 issues:

  1. @file block missing in uc_paymill.js - it is there already, not sure if this is a glitch in Coder module
  2. Unused variable $get_active_keys - this is left only for developer clarity and make it easier to understand what for is that parameter

Reviews of other projects

  1. Rasp (theme)
    https://drupal.org/node/2094859#comment-7885949
    https://drupal.org/node/2094859#comment-7888647
  2. Node Revision Bulk Delete (module)
    https://drupal.org/node/2092851#comment-7888207
  3. TestimonialBlocks (module)
    https://drupal.org/node/2087221#comment-7888375
  4. D7 Mobile boilerplate (theme)
    https://drupal.org/node/2078603#comment-7888899

Thanks!

Comments

kscheirer’s picture

Status: Needs review » Postponed (maintainer needs more info)
PAReview: Individual user account
It seems you are using a non-individual account.
All user accounts are for individuals. Accounts created for more than one user or those using anonymous mail services will be blocked when discovered (see Get a Drupal.org account).

Can you confirm that the topsitemakers account is a single user? Please change your account information and enter your realname.

If you prefer, we can also promote this sandbox to a full project without granting you "git vetted user" status.

----
Top Shelf Modules - Crafted, Curated, Contributed.

aramboyajyan’s picture

This account is individual, and I'm the sole user of this account.

To avoid confusion, I updated the "sponsored by" line on project landing page and in README.txt file.
I also updated my real name here on Drupal.org.

Let me know if this is sufficient.

Thanks.

kscheirer’s picture

Status: Postponed (maintainer needs more info) » Needs review

Yup, that's good, thanks!

TR’s picture

Priority: Critical » Normal
sanchiz’s picture

Status: Needs review » Needs work

I can not turn the module through Drush, because in .info file no Ubercart in dependencies. But this is reaaly problem, as
in reality module Ubercart does not exist, this is only module package.

In file uc_paymill.install no need to use quotes, Coder confirms this.

db_query("DELETE FROM {variable} WHERE `name` LIKE :variables", array(':variables' => 'uc_paymill_%'));

to

db_query("DELETE FROM {variable} WHERE name LIKE :variables", array(':variables' => 'uc_paymill_%'));

The rest looks good.

aramboyajyan’s picture

Status: Needs work » Needs review

Thanks for the feedback.

  1. Drush - I assume that everyone interested in this module will have Ubercart already installed. However, to avoid any confusion I updated the README.txt as well as the project page to explicitly say "download with Drush" rather than "install with Drush" and that Ubercart won't be downloaded with Drush automatically.
  2. Quotes removed and code updated.
klausi’s picture

Assigned: Unassigned » patrickd
Status: Needs review » Reviewed & tested by the community
Issue tags: -PAreview: review bonus

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

  • DrupalPractice has found some issues with your code, but could be false positives.
    
    FILE: /home/klausi/pareview_temp/uc_paymill.module
    --------------------------------------------------------------------------------
    FOUND 0 ERROR(S) AND 11 WARNING(S) AFFECTING 10 LINE(S)
    --------------------------------------------------------------------------------
     121 | WARNING | Unused variable $get_active_keys.
     247 | WARNING | Unused variable $get_active_keys.
     282 | WARNING | The extra check_plain() is not necessary for placeholders, "@"
         |         | and "%" will automatically run check_plain()
     282 | WARNING | The extra check_plain() is not necessary for placeholders, "@"
         |         | and "%" will automatically run check_plain()
     294 | WARNING | The extra check_plain() is not necessary for placeholders, "@"
         |         | and "%" will automatically run check_plain()
     297 | WARNING | The extra check_plain() is not necessary for placeholders, "@"
         |         | and "%" will automatically run check_plain()
     298 | WARNING | The extra check_plain() is not necessary for placeholders, "@"
         |         | and "%" will automatically run check_plain()
     299 | WARNING | The extra check_plain() is not necessary for placeholders, "@"
         |         | and "%" will automatically run check_plain()
     310 | WARNING | The extra check_plain() is not necessary for placeholders, "@"
         |         | and "%" will automatically run check_plain()
     311 | WARNING | The extra check_plain() is not necessary for placeholders, "@"
         |         | and "%" will automatically run check_plain()
     312 | WARNING | The extra check_plain() is not necessary for placeholders, "@"
         |         | and "%" will automatically run check_plain()
    --------------------------------------------------------------------------------
    

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:

  1. uc_paymill_uninstall(): better use explicit variable_del() calls to not accidentially delete anything from other (sub-)modules and to also clear the appropriate caches.
  2. _uc_paymill_get_error_text(): I don't think you need to call check_plain() here, since you are using the return value in placeholders anyway, which will escape the text for you.

But that are not blockers, otherwise looks RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Assigning to patrickd as he might have time to take a final look at this.

aramboyajyan’s picture

Thank you very much for the review.

Automatic review comments

  1. Unnecessary instances of check_plain() have been removed and committed.
  2. Unused variable $get_active_keys is left on purpose for developer clarity.

Manual review comments

  1. Good point regarding the removal of module variables. Updated and committed to the repo.
  2. Unnecessary check_plain() for recognizing Paymill response messages has been removed and committed.
patrickd’s picture

Status: Reviewed & tested by the community » Fixed

omg!! so many inline comments :)

well done, I've nothing to complain about

Thanks for your contribution, topsitemakers!

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:

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.

aramboyajyan’s picture

Hi Patrick,

Thanks for the positive feedback, updating my account and posting recommended readings :)

I hope the module will be useful for the community. I plan on contributing more projects and some of them are already being prepared.

Cheers!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.