Send order to merchant as an attachment

Hi there,

Thanks for highlighting this bug. I can confirm that your second issue "Title of payment option:" doesn't work and have attached a patch to fix this.

As for your other question, I'm not entirely sure what the intention of "Send order to merchant as an attachment" is off the top of my head, as I did not create the module. However I will look into it to see if I can figure it out. In the meantime, hopefully psynaptic (the original creator of the module) will be able to help clear things up.

Thanks again for your post. Can you please try the patch and let me know if that fixes that issue?

Hi Leon,

Thanks for that, it's much appreciated. Since this patch is quite small I won't ask you to re-submit but in the future can you try be aware of the coding practices of Drupal? For instance using spaces (2) instead of tabs. Also spacing such as if() should be if ().

Please refer to the coder module (http://drupal.org/project/coder) as it is a very helpful tool to make sure all the code is consistent.

I'm not too sure what the patch is supposed to do (as I didn't really know what the initial problem was), so could you please let me know how I can test it to make sure it works? What settings do I need to enable? Where can I make sure that the setting is actually working? Then I will commit it to HEAD.

Thanks again, much appreciated.

Marc

Setting the status of this "postponed (maintainer needs more info)" as I don't believe I have sufficient information on how to successfully test the patch in #3.

As soon as I know how then I will test and, assuming everything works, will commit to HEAD.

Hey Leon,

Thanks so much for that detailed description (and the patch itself of course!!). It all looks and sounds good to me. I will contact psynaptic (main maintainer of this module) to make sure he is happy with it before committing. But all looks good as far as I can see, so should be committed soon. I will update on here when it's committed.

Thanks for spotting the bug and providing a patch!! It's much appreciated :)

Cheers,

Marc

+++ uc_paypointNew.module	2010-02-18 16:03:04.000000000 +0000
@@ -57,7 +57,7 @@ function uc_paypoint_payment_method() {
-    'title' => t('PayPoint'),
+    'title' => variable_get('uc_paypoint_title', t('PayPoint')),

Addressing multiple issues in the same patch is a bit messy. It's better to keep these separate if possible.

I think we should probably not include this feature as once this is set it would be impossible to localize.

+++ uc_paypointNew.module	2010-02-18 16:03:04.000000000 +0000
@@ -298,6 +298,47 @@ function uc_paypoint_form_alter(&$form, 
+        $xml_order = "<order class='com.secpay.seccard.Order'> 
+                       <orderLines class='com.secpay.seccard.OrderLine'>";

We should use single quotes to encapsulate this, use double quotes for the attribute values.

I'm not keen on adding the newline and long whitespace before the start of the next line.

+++ uc_paypointNew.module	2010-02-18 16:03:04.000000000 +0000
@@ -298,6 +298,47 @@ function uc_paypoint_form_alter(&$form, 
+        $xml_order = "<order class='com.secpay.seccard.Order'> 

Remove trailing whitespace.

+++ uc_paypointNew.module	2010-02-18 16:03:04.000000000 +0000
@@ -298,6 +298,47 @@ function uc_paypoint_form_alter(&$form, 
+          $xml_order .= "<OrderLine><prod_code>" . $product->title . " (" . $product->model . ")";

Double quotes where it should be single but this would probably be better using:

$xml_order .= "<OrderLine><prod_code>$product->title($product->model)";

+++ uc_paypointNew.module	2010-02-18 16:03:04.000000000 +0000
@@ -298,6 +298,47 @@ function uc_paypoint_form_alter(&$form, 
+          

Trailing whitespace.

+++ uc_paypointNew.module	2010-02-18 16:03:04.000000000 +0000
@@ -298,6 +298,47 @@ function uc_paypoint_form_alter(&$form, 
+              $xml_order .= " - " . $attribute . ": " . $selection[0] . ",";

Same as above, this could be improved by either changing to single quotes or embedding variables in the double quotes.

$xml_order .= ' - ' . $attribute . ': ' . $selection[0] . ',';

or

$xml_order .= " - $attribute: $selection[0],";

I prefer the second for readability.

+++ uc_paypointNew.module	2010-02-18 16:03:04.000000000 +0000
@@ -298,6 +298,47 @@ function uc_paypoint_form_alter(&$form, 
+          

Trailing whitespace.

+++ uc_paypointNew.module	2010-02-18 16:03:04.000000000 +0000
@@ -298,6 +298,47 @@ function uc_paypoint_form_alter(&$form, 
+          $xml_order .= "</prod_code><item_amount>" . $product->price . "</item_amount>
+                         <quantity>" . $product->qty . "</quantity>
+                         </OrderLine>";

$xml_order .= "</prod_code>
<item_amount>$product->price</item_amount>
<quantity>$product->qty</quantity>
</OrderLine>";

or

$xml_order .= "</prod_code><item_amount>$product->price</item_amount><quantity>$product->qty</quantity></OrderLine>";

+++ uc_paypointNew.module	2010-02-18 16:03:04.000000000 +0000
@@ -298,6 +298,47 @@ function uc_paypoint_form_alter(&$form, 
+        $comments = uc_order_comments_load($order_id);
+        if (is_array($comments)) {
+          $xml_order .= "<OrderLine>
+                         <prod_code>COMMENTS - " . $comments[0]->message . "</prod_code>
+                         </OrderLine>";
+        }
+        foreach ($order->line_items as $line_item) {
+          switch ($line_item['type']) {
+            case "shipping":
+              $title = "SHIPPING";
+              break;
+            case "uc_discounts":
+              $title = "DISCOUNT";
+              break;
+          }
+          $xml_order .= "<OrderLine>
+                         <prod_code>" . $title . "</prod_code>
+                         <item_amount>" . abs($line_item['amount']) . "</item_amount>
+                         </OrderLine>";
+        }
+        $xml_order .= "</orderLines></order>";
+        $data += array('order'  =>  $xml_order, );
+      }

Same issues as above. I'll leave it as an exercise for you to find them!

Overall, good job. These are quite small, nit-picky things.

Powered by Dreditor.

Thanks for those detailed fixes psynaptic. If leon.nk doesn't have the time to do these by this weekend then (depending how busy I am) I may implement them as I'd like to get this committed ASAP.

By the way, with regards to the first issue:

-    'title' => t('PayPoint'),
+    'title' => variable_get('uc_paypoint_title', t('PayPoint')),

Why would you not want this committed? Isn't the point of this setting on the form to be able to change the title of "PayPoint" to whatever the user likes? And if not, shouldn't we perhaps change the form itself so it doesn't confuse anyone else? I thought that the default was "PayPoint" but the user is able to change it. Which is not currently the case, but does get fixed with the above change.

Can you explain please? Cheers :)

There isn't really much point to having the settings since it can be achieved by using the localization features in core. We're just duplicating functionality.

Sorry about this!

+++ uc_paypoint.module	2010-02-19 14:54:39.000000000 +0000
@@ -298,6 +298,37 @@ function uc_paypoint_form_alter(&$form, 
+      //Send Order Details to PayPoint

// Send order details to PayPoint.

+++ uc_paypoint.module	2010-02-19 14:54:39.000000000 +0000
@@ -298,6 +298,37 @@ function uc_paypoint_form_alter(&$form, 
+          if (! empty($product->data['attributes'])) {

if (!empty($product->data['attributes'])) {

+++ uc_paypoint.module	2010-02-19 14:54:39.000000000 +0000
@@ -298,6 +298,37 @@ function uc_paypoint_form_alter(&$form, 
+          $xml_order .= "<OrderLine><prod_code>COMMENTS -" . $comments[0]->message ."</prod_code></OrderLine>";

$xml_order .= "<OrderLine><prod_code>COMMENTS -$comments[0]->message</prod_code></OrderLine>";

+++ uc_paypoint.module	2010-02-19 14:54:39.000000000 +0000
@@ -298,6 +298,37 @@ function uc_paypoint_form_alter(&$form, 
+            case "shipping":

case 'shipping':

+++ uc_paypoint.module	2010-02-19 14:54:39.000000000 +0000
@@ -298,6 +298,37 @@ function uc_paypoint_form_alter(&$form, 
+              $title = "SHIPPING";

$title = 'SHIPPING';

+++ uc_paypoint.module	2010-02-19 14:54:39.000000000 +0000
@@ -298,6 +298,37 @@ function uc_paypoint_form_alter(&$form, 
+            case "uc_discounts":

case 'uc_discounts':

+++ uc_paypoint.module	2010-02-19 14:54:39.000000000 +0000
@@ -298,6 +298,37 @@ function uc_paypoint_form_alter(&$form, 
+              $title = "DISCOUNT";

$title = 'DISCOUNT';

+++ uc_paypoint.module	2010-02-19 14:54:39.000000000 +0000
@@ -298,6 +298,37 @@ function uc_paypoint_form_alter(&$form, 
+          $xml_order .= "<OrderLine><prod_code>$title</prod_code><item_amount>" . abs($line_item['amount']) . "</item_amount></OrderLine>";

$xml_order .= "<OrderLine><prod_code>$title</prod_code><item_amount>" . abs($line_item['amount']) . '</item_amount></OrderLine>';

+++ uc_paypoint.module	2010-02-19 14:54:39.000000000 +0000
@@ -298,6 +298,37 @@ function uc_paypoint_form_alter(&$form, 
+        $xml_order .= "</orderLines></order>";

$xml_order .= '</orderLines></order>';

Powered by Dreditor.

I have attached the exact same patch as in #10 with your latest minor changes psynaptic.

Leon could you please double check that it's still working as expected?
Psynaptic can we commit it if this is the case?

Cheers,

Marc

EDIT: One thing I did notice is this line appears to mix " with '.

$xml_order .= "<OrderLine><prod_code>$title</prod_code><item_amount>" . abs($line_item['amount']) . '</item_amount></OrderLine>';

Would it not be better written as:

$xml_order .= '<OrderLine><prod_code>$title</prod_code><item_amount>' . abs($line_item['amount']) . '</item_amount></OrderLine>';

Or is there a reason for this?

Ahh yes thanks for pointing that out Leon, I missed the variable there.

psynaptic is the last patch (#15) good to commit? Obviously some of your changes weren't implemented, but with reason (as described in #14)...

Yup, just go ahead. I don't want to cause too much friction. Although, there are other coding standards errors!

Marking this as fixed as it has been committed.

Thanks very much Leon for helping us squash these bugs!

+++ uc_paypoint/uc_paypoint.module	22 Feb 2010 14:47:42 -0000
@@ -277,6 +277,42 @@
+          $xml_order .= '<OrderLine><prod_code>COMMENTS - '.$comments[0]->message. '</prod_code></OrderLine>';

Spacing around concatenation operator is erroneous, twice.

Powered by Dreditor.