Jump to:
| Project: | Drupal.org Project applications |
| Component: | module |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | reviewed & tested by the community |
Issue Summary
Sandbox: http://drupal.org/sandbox/thtas/1494902
Git repo: http://git.drupal.org/sandbox/thtas/1494902.git
git clone --recursive --branch 7.x-1.x http://git.drupal.org/sandbox/thtas/1494902.git commerce_justpay
A single page payment form
Provides a single page payment page where a user can enter payment information and an amount to pay.
Good for donations, settling invoices for services not managed by the current commerce system.
Provides an order with transactions against it much like a normal checkout process.
Provides a couple of alter hooks so additional form fields can be easily processed along with the order.
Notes from README.txt:
Only supports a single payment method at a time (but you could alter which method to use using provided hooks)
Requires a prodict be set up as the base product of the transaction. This is done automatically when you first access the payment form (if you have the proper permission)
requires the default setup for drupal commerce such that entity relationships exist like this:
$product->commerce_price
$order->commerce_customer_billing->commerce_customer_addressThe default product type "product" must exist
**Depends on most of the commerce modules**
INSTRUCTIONS:
1. Install module
2. The form will work at /justpay
3. There is also a Commerce Justpay block which you can set up to show the payment form wherever you like
4. Use commerce rules to configure what happens after payment, by default you just get returned to the form.
5. Configure the product dummy JUSTPAYMENT sku to set the currency of the payment
Sponsored by Eighty Options
Comments
#1
Hi,thtas
Manually Review
1) Remove unnecessary code like commented code.
2) Control statements should have one space between the control keyword and opening parenthesis,
if(!empty($form_state['values']['mail'])) {
Like
if (!empty($form_state['values']['mail'])) {
3)Put function comment in your module .
Mayank Kamothi
#2
1. Add git link like http://git.drupal.org/sandbox/thtas/1494902.git and version information in the issue summary.
2. Review your module from coder module and see the automated review here.
3. Remove the empty hook functions like hook_commerce_justpay_payment_method_alter(), hook_commerce_justpay_order_alter from commerce_justpay.api.php.\
4. when i installed the module the unrecoverable error occurred
Call to undefined function commerce_product_new() in /var/www/7/sites/all/modules/custom/commerce_justpay/commerce_justpay.install.
5. The menu link corresponding to justpay is not visible to user.
can use link like
$items['admin/config/people/justpay'] = array(
'title' => 'Pay',
'page callback' => 'drupal_get_form',
'page arguments' => array('commerce_justpay_form'),
'access arguments' => array('access commerce justpay form')
);
#3
Thanks for the comments. I've addressed all of the items mentioned.
#4
@thtas - I am still getting errors when I try to install/uninstall your module. It's happening because you haven't specified all the dependencies.
For example, you are calling commerce_product_load_by_sku() in commerce_justpay.install but that is in the commerce_product.module which in not in your commerce_justpay.info file's dependencies.
You need to test it on fresh drupal 7 install and keep adding required modules. Drush is the best way to enable/disable modules and test this.
Errors,
PHP Fatal error: Call to undefined function commerce_product_load_by_sku() in /var/www/d7/sites/all/modules/contrib/commerce_justpay/commerce_justpay.install on line 35
Notice: Undefined property: stdClass::$type in CommerceProductEntityController->save() (line 90 of /var/www/d7/sites/all/modules/contrib/commerce/modules/product/includes/commerce_product.controller.inc).
EntityMalformedException: Missing bundle property on entity of type commerce_product. in entity_extract_ids() (line 7562 of /var/www/d7/includes/common.inc).
#5
I think this is ready for another review.
I've done testing against a fresh Drupal install with the latest Drupal and the latest Commerce (as of this posting)
#6
Hello,
Please add a git clone link " git clone --recursive --branch 7.x-1.x http://git.drupal.org/sandbox/thtas/1494902.git commerce_justpay".
Manual review:
commerce_justpay.install: line 10, 17, 24, your hook function are empty, why did you leave them?
commerce_justpay.module: line 27, don’t use a MENU_NORMAL_ITEM as it used by default.
line 128: here should be a whitespace after 'commerce_justpay_submit_text' in variable_get().
line 151: else { must be in a new line, please fix all issues that automated test reports and read coding standarts as you have many errors with it.
See this report and please fix it http://ventral.org/pareview/httpgitdrupalorgsandboxthtas1494902git.
#7
Ok - all coding standards are fixed.
Can anybody tell me why function short description comments must be a single line of 80 characters?
What if i had more to say then could fit in to 80 characters, where would i put that text? README or something?
Edit: menu type removed, hooks in .install removed (there were there to set up configurations before i added an admin interface)
#8
Hi thtas,
I think your module is great! I am definitely going to use it. But there are a few critical issues i came up with and i think you need to fix them soon.
1. In commerce_justpay_form the payment details ajax refresh fails. It happens because the element's "payment_method" ajax callback which is "commerce_payment_pane_checkout_form_details_refresh" try to return $form['commerce_payment']['payment_details'] instead of $form['commerce_justpay_payment ']['payment_details']. I think an override of element's callback would fix this.
2. This is issue is more complex and difficult than the first one. When i enabled paypal as a payment method the checkout crashed. In function commerce_justpay_form_submit there is no functionality for payment methods that work with redirection like paypal WPS. I think that at this point your code is incomplete. You need to see how commerce handles these type of payment methods and modify your code accordingly.
Keep up your good work. I think you are close to create a very useful module.
#9
Thanks for checking out the module, it's much appreciated.
I'll see If i can come up with a solution to those problems...
#10
Ok I've added support for offsite payments with Paypal WPS.
Note: I've added a hook call in so that module authors can edit the return URLS for remote payments.
e.g. Paypal would return to checkout/XXX/payment, but we need to return to justpay/offsite instead.
So i've added a hook as follows
<?php/**
* Allows the offsite redirect form to be altered specifically for this module.
*
* @param array $form
* The just submitted single page checkout form
* @param object $order
* The order object about to be paid for
*
* @see commerce_justpay_offsite_payment_form()
*/
function hook_commerce_justpay_CALLBACK_alter(&$form, $order) {
// This example modifies the Paypal WPS return URLS.
$form['return']['#value'] = str_replace('checkout/' . $order->order_id . '/payment', 'justpay/offsite', $form['return']['#value']);
$form['cancel_return']['#value'] = str_replace('checkout/' . $order->order_id . '/payment', 'justpay/offsite', $form['cancel_return']['#value']);
}
?>
Remote payments really aren't as elegant a solution here as the direct web service calls, I'm not sure how much extra time I want to put in to this to support that feature...
#11
changing status
#12
Hi,
commerce_justpay.info:
Package should be 'Commerce (contrib)' instead of 'Commerce'.
Please end the description with a dot.
commerce_justpay.module:
I detected some style issues. This
function commerce_justpay_base_product($sku=NULL) {should be:
function commerce_justpay_base_product($sku = NULL) {I think you don't need to use filter_xss() if you already use placeholders inside t(), just use:
drupal_set_message(t('A product with SKU %sku has been automatically generated, if you want to use a different base product for Commerce Justpay, configure it !here', $vars));instead of:
$message = t('A product with SKU %sku has been automatically generated, if you want to use a different base product for Commerce Justpay, configure it !here', $vars);drupal_set_message(filter_xss($message));
#13
#14
I've addressed the first two issues, but the filter_xss function is there because the code review module gives this critical error without it:
Line 346: Potential problem: drupal_set_message() only accepts filtered text, be sure all !placeholders for $variables in t() are fully sanitized using check_plain(), filter_xss() or similar. (Drupal Docs)
drupal_set_message(t('A product with SKU %sku has been automatically generated, if you want to use a different base product for Commerce Justpay, configure it !here', $vars));
My solution to wrap the message in filter_xss seemed to make it happy, but it does look funny.
Is there a better way to include a URL in drupal_set_message?
#15
You used the bad !placeholder. Just use %here or @here and everything should be fine.
#16
I can't use % or @ because it will escape all the html in the link i'm trying to display and render it useless.
I decided to just copy how Drupal core deals with this scenario - see node.module line 75 in node_help
So mine ended up like this:
<?php$vars = array(
'@config_url' => url('admin/commerce/config/justpay'),
'%sku' => $sku,
);
drupal_set_message(t('A product with SKU %sku has been automatically generated, if you want to use a different base product for Commerce Justpay, configure it <a href="@config_url">here</a>', $vars));
?>
#17
There are some Drupal coding standard mistakes http://ventral.org/pareview/httpgitdrupalorgsandboxthtas1494902git
Rest seems ok
Nice module!
#18
We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)
#19
Hello thtas,
Thank you for great module.
Manual review:
1. Remove also variable "commerce_justpay_exlude_order_fields" what is present in file: commerce_justpay.admin.inc at line: 35 and other places.
2. In file: commerce_justpay.module at line: 182, function description in wrong "hook_submit().", I think this is form submit handler.
3. In file: commerce_justpay.module at line: 185, was initialized variable $user but it is not used in commerce_justpay_form_submit() function.
#20
changed status
#21
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.
#22
Thanks for the manual review Vladimir!
I've committed the fixes mentioned in #19
#23
Hi, check
http://ventral.org/pareview/httpgitdrupalorgsandboxthtas1494902git
and solve all errors/warnings.
Except that, code is functionality is working and it seems ok to me.
#24
Minor coding standard errors are not application blockers, please do a manual review.
#25
Fixed minor standards issues - Thanks erez111.
#26
Looks good to me and coder is happy as well.
I'd maybe remove the dependency on commerce_product because commerce_product_ui depends on that anyway, but I don't see that should stop this from being a full contrib module and I don't need to pick nits ;-)