512357 - Use Paypal IPN

Status: RESOLVED FIXED

(addons.mozilla.org Graveyard :: Collections, defect)

Description

[Wil Clouser [:clouserw]]

We want to start using Paypal's IPN stuff.  There is example code here: https://www.paypal.com/cgi-bin/webscr?cmd=p/xcl/rec/ipn-code-outside and we just want to throw a script in /webroot/services/

https://www.paypal.com/cgi-bin/webscr?cmd=p/xcl/rec/ipn-techview-outside has all the details.  And make it pretty 'cause jbalogh is a tough reviewer. Thanks!

Comment 1

[RJ Walsh [:rjwalsh]]

Attachment: v1

Testing was basically non-existent since I don't have a paypal test account - but this seems like it should do the trick.  Note the 49xxx migration you need to run on stats_contributions.

Comment 2

[Jeff Balogh (:jbalogh)]

Comment on attachment 396369 [details] [diff] [review]
v1

Is there a SERVICE_URL . '/paypal.php' handler that didn't make it in here?

Comment 3

[RJ Walsh [:rjwalsh]]

Attachment: v2

Comment 4

[Scott McCammon [:mccammos]]

RJ: I think there is at least one query in the stats component that would be affected by the stats_contributions schema change.

Comment 5

[RJ Walsh [:rjwalsh]]

(In reply to comment #4)
> RJ: I think there is at least one query in the stats component that would be
> affected by the stats_contributions schema change.

Scott: This is the only one I found, please let me know if there are more:
-                     WHERE addon_id='{$addon_id}' AND uuid IS NULL AND amount > 0
+                     WHERE addon_id='{$addon_id}' AND completed = 1 AND amount > 0

Comment 6

[Jeff Balogh (:jbalogh)]

Comment on attachment 396465 [details] [diff] [review]
v2

>Index: webroot/services/paypal.php
>===================================================================
>+/**
>+ * CONFIG
>+ *
>+ * Require site config.
>+ */
>+require_once(dirname(__FILE__).'/../../config/config.php');
>+require_once(dirname(__FILE__).'/../../config/constants.php');
>+
>+// Most of this is sample code from the paypal site 
>+// read the post from PayPal system and add 'cmd'
>+$req = 'cmd=_notify-validate';
>+
>+foreach ($_POST as $key => $value) {
>+    $value = urlencode(stripslashes($value));
>+    $req .= "&$key=$value";
>+}

clouseroo prefers the {$key} syntax.

>+// post back to PayPal system to validate
>+$header .= "POST /cgi-bin/webscr HTTP/1.0\r\n";
>+$header .= "Content-Type: application/x-www-form-urlencoded\r\n";
>+$header .= "Content-Length: " . strlen($req) . "\r\n\r\n";
>+$fp = fsockopen ('ssl://www.paypal.com', 443, $errno, $errstr, 30);

(high level languages)++

controllers/components/httplib.php uses curl to do HTTP POSTs, so we don't muck around with file handles and fgets and crap like that.

>+
>+// assign posted variables to local variables
>+$item_name = $_POST['item_name'];
>+$item_number = $_POST['item_number'];
>+$payment_status = $_POST['payment_status'];
>+$payment_amount = $_POST['mc_gross'];
>+$payment_currency = $_POST['mc_currency'];
>+$txn_id = $_POST['txn_id'];
>+$receiver_email = $_POST['receiver_email'];
>+$payer_email = $_POST['payer_email'];

We don't need most of these, right?

>+if (!$fp) {
>+    die('HTTP Error');

This script gets pretty morbid.

Will any of these messages be logged somewhere?

>+ } else {
>+    fputs ($fp, $header . $req);
>+    while (!feof($fp)) {
>+        $res = fgets ($fp, 1024);
>+        if (strcmp ($res, "VERIFIED") == 0) {
>+
>+            // Try to connect to the DB
>+            $dbh = @mysql_connect(DB_HOST.':'.DB_PORT, DB_USER, DB_PASS);
>+            if (!is_resource($dbh)) die('Could not connect to DB!');
>+            if (!@mysql_select_db(DB_NAME, $dbh)) die('Could not select DB!');
>+            
>+            // Make sure the payment is valid and yet unprocessed
>+            if ($payment_stats != 'Completed') die('Payment not completed!');

$payment_stats is unbound.  Did you mean $payment_status?

>+            $exists = "SELECT COUNT(*) FROM `stats_contributions` WHERE `uuid` = '{$txn_id}' AND `completed` = 1";
>+            $result = @mysql_query($query);
>+            if (!$result) die('Could not determine if contribution already logged');
>+            if (@mysql_num_rows($result)!==0) die('Transaction already processed!');

Won't there always be a single "row" of results, saying count is 0 or 1 or 7?

>+                      
>+            // Build the query - item_nuxcmber is the add-on ID

- item_number is the uuid we use to correlate pre-payment and post-payment data

>+            $query = "UPDATE `stats_contributions` SET `completed` = 1, `uuid` = '{$txn_id}' WHERE `uuid` = '{$item_number}'";

Why not add a new transaction_id column?  This code is confusing me by changing uuid and txn_id.  Completed transactions would have a non-null transaction id.

The thing we really want to save is mc_gross.  That's why we're jumping through all these hoops.

We should probably save all the posted data in our db as a text field, at least to start with, so we can see everything paypal is dumping on us.

Comment 7

[Scott McCammon [:mccammos]]

(In reply to comment #5)
> Scott: This is the only one I found, please let me know if there are more:
> ...

RJ, here is what I've found :-)

[smccammon@khan controllers]$ grep -n stats_contributions components/stats.php 
343:                    FROM `stats_contributions` 
476:            $data = $this->controller->Addon->query("SELECT DISTINCT DATE(`created`) AS `date` FROM `stats_contributions` WHERE `addon_id`={$addon_id} AND `uuid` IS NULL AND `amount` > 0 ORDER BY `date`", true);

Comment 8

[RJ Walsh [:rjwalsh]]

Attachment: v3

Comment 9

[Jeff Balogh (:jbalogh)]

Comment on attachment 396494 [details] [diff] [review]
v3

You don't need to urlencode the POST data, httplib->post will do that for you.

Do we still need completed if we're maintaining the txn_id?  Are those redundant?

You have a space between strcmp and the paren.

Can you include any URLs you've been referencing as comments in the code?  The paypal docs are a nightmare to navigate.

You probably need to change the URL to https, not ssl.

http->post returns the tuple (content, request_info), but you're treating the whole array as a string.

Instead of a final_amount column, can we just replace into the amount column?

Comment 10

[Jeff Balogh (:jbalogh)]

Attachment: fixes for paypal.php

Comment 11

[RJ Walsh [:rjwalsh]]

Comment on attachment 396583 [details] [diff] [review]
fixes for paypal.php

Cool, I'll post a merged patch with these changes

Comment 12

[RJ Walsh [:rjwalsh]]

Attachment: v4 with fixes from jbalogh

Comment 13

[Jeff Balogh (:jbalogh)]

Comment on attachment 396597 [details] [diff] [review]
v4 with fixes from jbalogh

nice

Comment 14

[RJ Walsh [:rjwalsh]]

Committed revision r50112.

Comment 15

[RJ Walsh [:rjwalsh]]

Attachment: Remove final_amount

Comment 16

[Jeff Balogh (:jbalogh)]

Comment on attachment 396629 [details] [diff] [review]
Remove final_amount

Why is your query line so long?  Is your return key broken?

Comment 17

[Brian King [:kinger]]

Wrong component?

Comment 18

[Wil Clouser [:clouserw]]

(In reply to comment #17)
> Wrong component?

We could use some new components but I haven't had time to make a list.  In the mean time, I sure wish Paypal's IPN would work.

Comment 19

[Wil Clouser [:clouserw]]

What?  this thing is fixed.