[D7] BIG FISH Payment Gateway - Übercart

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

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.

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

Master Branch

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.

• You have some non unicode chars in README.txt
• I'm not sure I understand the purpose of the large switch functions in uc_paymentgateway_method_fhb() for example, can you explain?
• Since your module requires the SDK to be loaded, check for it in hook_requirements() and consider using the Libraries API to load it.

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

Thanks for your review.

The bigfishdev is a shared account for our development team,
but currently I'm the only one, who using it and due to this policy nobody else
will be using it in the future.
I've created an individual account for myself. How can I become the owner of this project?

The version specific branch was created before, but now I deleted the master branch.
7.x-3.x is the default.

The large switches were just some skeleton codes for the different payment method submodules.
Those were unnecessary at the moment, so I removed them.

I've extended hook_requirements to check the library already at install phase.
I've checked the Libraries API also, but I think this module doesn't need it. The SDK
won't be shared, only this project will use it. Also the Libraries module doesn't help yet
the user implementing the third party code on the site. (except in the documentation)

Manual Review:

diff --git a/uc_paymentgateway.module b/uc_paymentgateway.module
index 9f59ff3..4e03592 100644
--- a/uc_paymentgateway.module
+++ b/uc_paymentgateway.module
@@ -297,29 +297,13 @@ function uc_paymentgateway_method_common($op, &$order, $form, &$form_state, $cal
       return TRUE;
 
     case 'cart-review':
-      break;
-
     case 'customer-view':
-      break;
-
     case 'order-delete':
-      break;

you can also use like this:

     case 'cart-review':
     case 'customer-view':
     case 'order-delete':
     break;

why do you want this blank condition???

@@ -466,10 +448,6 @@ function uc_paymentgateway_redirect() {
 
   $uc_paymentgateway = new UcPaymentGateway($order);
 
  if (!$uc_paymentgateway->getError() && $uc_paymentgateway->initTransaction()) {

  }

Thanks, Niteshp!

I removed the unnecessary breaks and the condition is not
blank anymore.

Hi, there's still a couple comments that are too long per pareview. Below is some feedback, of which only the inline comment are really git vetted blockers. The rest is just good information to know.

  protected function loadOrder($transaction_id) {
    $order_id = db_select(self::TABLE_NAME, 't')
      ->fields('t', array('order_id'))
      ->condition('transaction_id', $transaction_id)
      ->execute()
      ->fetchField(0);

It is unnecessary to list fetchField(0) as zero is the default.

function uc_paymentgateway_init() {
  module_load_include('inc', 'uc_paymentgateway');

  if (!_uc_paymentgateway_check_library()) {
    drupal_set_message(t('In order use the BIG FISH Payment Gateway module, You must download the proper SDK library from <a href="@url" target="_blank">here</a>.', array(
      '@url' => UC_PAYMENTGATEWAY_DOWNLOAD_URL,
    )), 'warning', FALSE);
  }

Consider not spamming the user in hook_init. In a local development environment where the payment gateway might not be set up, these types of messages on every page view become highly annoying.

function uc_paymentgateway_menu() {
  $items['uc_paymentgateway_redirect'] = array(
    'title' => 'Paymentgateway redirect page',
    'page callback' => 'uc_paymentgateway_redirect',
    'access arguments' => array('access content'),
    'type' => MENU_CALLBACK,
  );

The default type of MENU_CALLBACK isn't strictly necessary.

function uc_paymentgateway_settings_form() {
  $form = array();

  $form['uc_paymentgateway_store_name'] = array(
    '#title' => t('Online Store Name'),
    '#description' => '',
    '#type' => 'textfield',
    '#default_value' => variable_get('uc_paymentgateway_store_name', ''),
    '#size' => 60,

Empty description isn't necessary and the default size is already 60.

function uc_paymentgateway_uc_payment_gateway() {

  $gateways['paymentgateway'] = array(
    'title' => t('BIG FISH PaymentGateway'),
    'description' => t('BIG FISH PaymentGateway'),
    // 'settings' => 'uc_paymentgateway_gateway_settings_form',
    // 'credit' => 'uc_paymentgateway_charge',
  );

  return $gateways;
}

Remove commented out code and depend on the VCS to store history of changes.

/**
 * Initialize and start the transaction, then redirect the user to the Payment Gateway.
 */
function uc_paymentgateway_redirect() {
  $order_id = (int) $_SESSION['cart_order'];
  $order = uc_order_load($order_id);

  $uc_paymentgateway = new UcPaymentGateway($order);

  if (!$uc_paymentgateway->getError()) {
    /**
     * redirect user and exit
     */
    $uc_paymentgateway->initTransaction();
  }

  drupal_set_message($uc_paymentgateway->getErrorMessage(), 'error');
  drupal_goto('cart');
}

Inline comments should begin with two slashes (//) and end with a full stop.

I also struggle with saying this, but the project name seems a little, shall we say broad. There are many payment gateway options. Naming the project uc_paymentgateway makes me think this is an API project for creating payment gateways, not a payment gateway specifically purposed for Big Fish. Perhaps a project re-name is in order that includes some variation of Big Fish in the name.

Thanks for the review, heddn!

I fixed the code as you suggested, except the menu settings.
As the API document says, the default type is MENU_NORMAL_ITEM.
I also renamed the module to uc_bigfish_paymentgateway.

Not sure how I missed these earlier:
In uc_bigfish_paymentgateway_otp.inc, t() expects all strings to be in English.

.install:
public://uc_bigfish_paymentgateway' is created but never deleted on uninstall.

Non-blocking:
Have you considered the use of libraries module to wire in the external big fish PHP library? That way folks don't have to place the library within the module. Which makes version control easier.

I fixed the uninstall hook. Now it deletes the unnecessary folder.

Those strings can't be translated in English, so I removed all the t(),
but now Pareview shows warning about this. I hope it's not a blocker.

Let's see what the git administrators have to say about t(). Marking RTBCed.

OK. Thanks again for the review.

I think this is an issue that should definitely be fixed, but you clearly have a good understanding of the Drupal API, so it's not a blocker:

• Using uc_bigfish_paymentgateway_init() to load all those files is pretty heavy-handed, that hook gets executed on every page request. Much better performance would be to load those files only when needed by your functions.

The untranslated t() strings in uc_bigfish_paymentgateway_otp.inc are ok, I assume those are country-specific and therefore language-specific and would never need to be translated from Hungarian.

So we're left with the account name issue. We can only promote 1 account for this application, and it must be an individual. I see two options:

• confirm that you would like the david.juhasz account to be promoted instead of bigfishdev , OR
• update the bigfishdev account to include personal information, a name is usually sufficient, and confirm that it's no longer a shared account, and we can promote bigfishdev

Checked for use of the Drupal API, security, licensing, everything else seems ready to go!

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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

I fixed the heavy-handed file loading issue. The included files will be loaded only, if necessary.

Also our senior developer took over the bigfishdev account, and updated that filling out the profile with his name.
So we confirm that is not a shared account, and we ask you to promote it for us.

Typically moving the status to RTBC is an indication of a review by someone un-related to the code. Moving back to needs review for now.

None of those are blocking issues.

According to the guidelines, this issue should now be critical:

Critical: Applications with a status of needs review that have been awaiting response from a reviewer for 4+ weeks.

Your module still have some warning and error accroding to coding standard. Can you fix those? you can see the errors from the following link

http://pareview.sh/pareview/httpgitdrupalorgsandboxbigfishdev2059889git

It is always important to follow coding standard if you want to move your experimental module to full project.

I fixed the coding standard issues.

I gave your david.juhasz vetted git status.

Thanks for your contribution! Congratulations, you are now a vetted Git user. When you create new projects (typically as a sandbox to start) you can then promote them to 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 (here you get to ask fans of your project to help you!)

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 for the status update, but i'm not the owner of this project.
I'm only a maintainer, so i can't promote this module.

Is it possible to assign the project to me, or the bigfishdev user (the owner) to have a vetted git status also?

Hey David ... given the scenario (where you were approved as opposed to the submitter of the project), and that meaning that you aren't able to promote the sandbox, I'd suggest opening an issue in the Drupal.org Webmasters Queue with your request that they either transfer ownership of the module to you, or promote it from sandbox to full project on your behalf (in which case you'll need to supply them with your desired machine-name for the project as well).

The directory http://cgit.drupalcode.org/sandbox-bigfishdev-2059889/tree/images in the project repo is full of 3rd party company logos that is both copyrighted and trademarked. Don't even dream about licensing this intellectual property belonging to various third parties under the GPL.

Our policy on 3rd party materials in the repo is described in the 3rd party libraries and content on Drupal.org. It also appears in the Drupal Git Repository Usage policy you agreed to when you signed up for Git access, which you may want to re-read, to be sure you're not violating other terms.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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