Commerce Google Checkout

Welcome!

• Sandbox link is missing in the issue summary.
• It is strongly recommended to review 3 other project applications as outlined in https://drupal.org/node/1011698
• You need to set the status to "needs review" if you want to get a review.

Automated Review

http://ventral.org/pareview/httpgitdrupalorgsandboxalasdair861497002git-...
Review of the master branch:

• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See attachment.

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. Get a review bonus and we will come back to your application sooner.

Manual Review

Your Master branch has more code than your 7.x-1.x branch code. Your 7.x-1.x branch code is completely missing the Drupal Commerce hooks for payment method, checkout pane, etc.

commerce_google_checkout.module
commerce_google_checkout_menu
- Your access callback on the admin item is TRUE, meaning anonymous users have access to this page.
- This same menu item requires the file commerce_google_checkout.admin.inc, which does not exist

commerce_google_checkout_commerce_checkout_pane_info
- Your checkout pane requires the file includes/commerce_google_checkout.checkout_pane.inc yet that file doesn't exist

commerce_google_checkout_redirect_form
- Why are you using a redirect payment method when there is no UI for the user to be redirected to? The payment is handled on-site (through XML). Redirect forms are for off-site payment methods that require user input on a separate website.

commerce_google_checkout_order_form
- This creates a submit button from an image that is not stored locally, consider storing it in your module as it won't necessarily always be available on the Google servers.
- Why are you creating a submit button when Commerce checkout creates the "continue to next step" button for you which submits your form?

commerce_google_checkout_order_submit
- Commerce Checkout should handle the redirection for you, although unnecessary as this is not an external payment method.

commerce_google_checkout_send_request
- You call variable_get('commerce_google_checkout_merchant_id', '') twice for no reason. Just call it once outside of the IF statement

commerce_google_checkout_headers
- The initial IF statement is completely useless. You set the merchant_id and key to exactly the same values regardless of what the value of commerce_google_checkout_server is.

commerce_google_checkout_order_state_change
- Rather than destroy the site logs with constant messages from watchdog, consider just adding messages to the order log.

commerce_google_checkout_charge
- You call commerce_google_checkout_get_google_number which is not defined.

commerce_google_checkout_build_transaction
- Inline comments should begin with //, not /*
- Your switch statement can be shrunk to the following:

switch ($transaction_status) {
  case 'CHARGING':
  case 'CHARGEABLE':
    $transaction->status = COMMERCE_PAYMENT_STATUS_PENDING;
    break;
  case 'CHARGED':
  case 'DELIVERED':
    $transaction->status = COMMERCE_PAYMENT_STATUS_SUCCESS;
    break;
  default:
    $transaction->status = COMMERCE_PAYMENT_STATUS_FAILURE;
}

This is not a 100% thorough review, just an initial delve into the review.

You should add the GIT-URL to this page: http://git.drupal.org/sandbox/alasdair86/1497002.git

There are some spelling errors in README.txt:

A Payment gateway for Google Checkout. To use this module you will need to have
an account set up with Google Checkout and have recieved a merhacnt key as well
as a merchant ID. There is a sandbox option you can use.

commerce_google_checkout.module:
Line 424+505: "return TRUE;" is unreachable at end of function, maybe that line should be removed.

Line 433:

 * @return boolean
 *   TRUE if an order corresponding to the Google order number was found and
 * updated.

could be formatted nicer.

commerce_google_checkout.inc:

      if ($success) {
        commerce_google_checkout_notification_acknowledgement(check_plain($serial_number));
      }
      else {
      commerce_google_checkout_notification_error();
      }

=> bad ident

Except, those tiny things I did not find anything "disturbing", and the code looks very clean.

commerce_google_checkout_order_form
- This creates a submit button from an image that is not stored locally, consider storing it in your module as it won't necessarily always be available on the Google servers.

There is still the loading of an external image: "http://sandbox.google.com/checkout/buttons/checkout.gif?merchant_id=1234..."

I think alasdaircf is right, copying the image locally and using that copy would be better.

Is this Module working for use on a site?
Thanks

Hi, alasdaircf!

Everything passed automated review. Manual review:

• commerce_google_checkout.info:1 - System name might be trimmed to Google Checkout, to match other payment gateway modules of its kind
• commerce_google_checkout.info:3 - Package name should be Commerce (contrib) per http://www.drupalcommerce.org/development/standards#packages
• commerce_google_checkout.info:8 - Comment cruft; can be removed.
• commerce_google_checkout.pages.inc:44 - commerce_google_checkout_refund_order() is never defined.

Subscribing

@14 - Please use the Follow button instead. "Subscribe" comments will unnecessarily bump the issue.

I tried out your module, and it works fine so far. I had to make one mod since I use Commerce Customizable Product and lineitems types can be named something other than 'product'. See below, I just changed "if ($lineitem->type == 'product') {" to if ($lineitem->type != 'shipping') {"

if ($lineitem->commerce_total['und'][0]['amount'] != '0') {
// Add product info.
if ($lineitem->type != 'shipping') {

Your a setting variables, but you do not have any uninstall tasks - which means that the variables will still be in the database on deactivation of the module, you should add uninstall tasks to remove them. Else this look good to me. Mark as RTBC.

@alasdaircf: Thanks for contributing this module! Thank you also for your patience with the review process.

#18 is an important change, in my view, so if someone would please add this as an issue and help get this committed, I would appreciate it. MiSc is very busy, so please don't wait on him for this.

@dallas.masters: Thank you very much for reporting that the code is in a working state. Would you please also post an issue, hopefully even a patch, to the project so that this change will be committed?

I would like to see at least one or two more reviews that say it's working (Designer Optics?). I added Commerce's review tag, so hopefully that will draw some more eyeballs.

Thanks a lot for the reviews from everyone so far! I look forward to vetting this.

Technically, this module works. However, it does not adhere to Google's ToS.

Google REQUIRES you put a Google Checkout button next to any other checkout button you have. At a minimum, this requires you place it on the Cart page. This probably means you need a custom block. It cannot be just another payment type in the normal Commerce flow, as they explicitly forbid it.

View section #4. https://checkout.google.com/seller/policies.html

What Google demands have nothing to do with the project application, rather an issue in issue queue for the module. Setting is back as need review.

In this case, when working with things as sensitive as ecommerce, it should. If Google has certain demands regarding any integration done, then those demands should be met before being viewed as complete. It would be a bad idea for anyone to actually use this module as it is currently written.

If the module is claiming to solve a problem (integration with Google Checkout), then it should actually solve the problem correctly.

This module, as-is, does not really solve the problem it claims to solve yet. It's a good start, with the messaging intact. But it is incomplete.

No I do not think it is stoppers in this process. We are not here to meet Google demands, just Drupal best practice, security, coding standard and so on.

I thought the module had to meet a need not met before? That is one of the criteria for acceptance and getting promoted as a full project.

So if a module claims to implement an API, I would think proper implementation of that API should be a factor. Currently, it doesn't just NOT implement some requirements, it is in direct opposition to Google's terms, doing something they explicitly tell you not to do.

No one should use this module with the way it is currently implemented. If a merchant has the risk of having their Google account flagged or deactivated or their Google Checkout privileges revoked for using this module, that should definitely be a factor into whether this should be a full project or not.

And that's all I'm going to say about it.

You are still talking about Google demands, and that is not relevant here. Sure it would be good if the module use the demands from Google, but it is not relevant here in this issue queue.

@25 But it is directly relevant to the usefulness of the module to the community. Installing a module that violates a payment gateway's TOS results in every user installing it opening a big can of worms, a serious quality (not to mention liability) issue. Agree with #20 that this is still CNW.

@davidwatson: No, It is not relevant in the review process (http://drupal.org/node/1587704), external demands should not be an issue to get access to publish full projects on d.o. It is an issue for the module itself, not an issue for this process.

@27 It's not an "external demand" or a feature request, it's a major issue with the functional correctness of the module. The list you linked to--however valuable to new contributors--is far from exhaustive, as it says itself I believe. If it doesn't implement Google's spec (and in fact violates their TOS by doing so), it is effectively non-functional, and there is already a policy on incomplete submissions.

External functional correctness is not a part of the review process, and it is not a incomplete submission.

I disagree with you, but I am tired of switching the status of this all the time. We wait for input from the developer.

@29 - Sounds fair, and I'm more than willing to agree to disagree rather than get into a revert war. I'd also be very interested in hearing from a Git admin's take on it.

@davidwatson: I am a git admin.

@31 I'm willing to defer to your experience handling similar issues, then. I'm still not sure if I agree, but that's better left to IRC, methinks. :]

Hi, when using this module the customer is charged for the purchase, but the order never appears in Drupal.

Is there an "API callback URL" that should be configured?

Thanks

@Mike Chelen, this is not a finished module yet, and maybe will not be (depending on the applicant), but please do a review of it, to get this process forward.

@MiSc
From other posts it seems like the module might be working, so I was wondering if the problem I'm having is due my specific website and setup, or the module itself. The payment goes through ok, but the order is stuck in the "Checkout - Payment" phase.

I had an old version of the module https://drupal.org/sandbox/paul.linney/1246398 installed first so maybe that is causing problems?

Do not RTBC your own issues, please

PAReview found a few errors: http://ventral.org/pareview/httpgitdrupalorgsandboxalasdair861497002git

1) While checking out we get to the page:

"Proceed to Google checkout"
"Use the button below to proceed to the payment server"

There is no button below. The 'Proceed to Google checkout' is the button, and the .gif image you're using for it isn't showing up because you've prefixed the #src attribute in your form submit array with a '/'. See the attached patch for details.

2) After payment there was no redirect back to the site. Not sure how difficult this would be, but it would be nice to have. Maybe a feature request after project promotion :)

On the plus side the transaction appears to work. I tested it on a Commerce Kickstart 2.0 environment. Good luck!

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

I have tested this on an ecommerce site that I am currently building. The site has 3 other payment options which all appear as radio button options inside the payment wrapper of the checkout. These options are Redeem a coupon (custom credit voucher module), Paypal and Credit Card (commerce_nab_transact).

As the Google Checkout button appears as a separate button outside of these radio button options, when I click it to checkout, the checkout form still tries to validate and errors.

Does the Google Checkout API allow you to not have it as a separate button and integrate it more with how Commerce treats payments?

Also, you are missing the Git repo address in your description. Ran the repo through ventral.org and found no errors.

Other than this the code looks very clean and tidy.

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

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

Changed to add link to sandbox