Hook nodeapi in LM PayPal Paid Adverts uses lm_paypal_paid_adverts_has_sub() to test the status of a node requiring payment.

This function checks the status on line 338 of lm_paypal_paid_adverts.module, but does not return the status to lm_paypal_paid_adverts_nodeapi. Rather it always returns true, which means that the pages are always visible in spite of the payment status:

  $paidfor = lm_paypal_node_subscribed($node->nid, $subid);
  return TRUE;

The return statement should be changed to:

  return ((empty($paidfor)) ? FALSE : $paidfor);

so that the reults of the test are made available to hook_nodeapi for the 'load' op. The empty() test is required to prevent the return of NULL if there is no subscription record for the node.

Comments

lyricnz’s picture

lyricnz commented
Status: Needs review » Postponed (maintainer needs more info)

The function lm_paypal_paid_adverts_has_sub() is performing as documented:

/**
 * Does the node require a subscription
 *
 * @param $node
 *   The node to check
 * @param $subid
 *   If the node is subscription based and subscribed this is the subid
 * @param $paidfor
 *   If the node is subscription based this is TRUE if the node has been paid for
 * @return
 *   TRUE if the node does require a subscription
 */
function lm_paypal_paid_adverts_has_sub($node, &$subid, &$paidfor) {
  ...
}

So it returns whether or not a subscription is attached to the given node. Additionally, it writes into two of it's (by-reference) parameters, which $subid is attached, and whether that has been paid for this node (in $paidfor).

The hook_nodeapi() function also appears to be performing "as designed", in that it trashes the node title/teaser/body on load, when (a) the node does not belong to the current user, and (b) the node has not been paid for:

      $has_sub = lm_paypal_paid_adverts_has_sub($node, $subid, $paidfor);
      if (!$has_sub) {
        return;
      }
      if ($node->uid != $user->uid && !$paidfor) {
        // Make sure nothing useful is visible
        $overwrite['title'] = $short_title;
        $overwrite['teaser'] = $overwrite['body'] = t('<em>Not yet published.</em>');
        return $overwrite;
      }
      return;

Are you having a specific problem here?

Triskelion’s picture

Triskelion commented

Missed the 'by reference'. Spent my time last night working on the views integration, which gives me a work-around to my problem.#113716: lm_paypal Views filter.

I have several paid ad types on the site, which are, with one exception (ed_classified), standard user-defined node types. They are all visible even to visitors who are not logged in, even when not paid. The views integration means I can restrict the navigation, but anyone with the uri can view the content. The ed_classified module uses node_prepare(), not hook node_api, and I am going to have to find a work-around. The funny thing is that ed_classified recommends LM PayPal, yet does not appear to work with it. On their project page they state:

Paid classified ads may be implemented by installing the lm_paypal module and limiting ed_classified node creation to paying users using the lm_paypal_paid_adverts or subscriptions feature. Other schemes may work as well, but the ed_classified module does not handle paid ad creation for you - this is beyond the scope of the module at present, although tighter lm_paypal integration is being investigated.

I don't know what I am missing here, but the frustration is growing. Is there some reason that LM PayPal does not toggle the node->status flag rather than doing an overwrite?

tomws’s picture

tomws commented

subscribing

lyricnz’s picture

lyricnz commented
Title: Paid Adverts implementation of hook nodeapi broken in 5.x-i.x-dev » Paid Adverts implementation of hook nodeapi unexpected behaviour

> Is there some reason that LM PayPal does not toggle the node->status flag rather than doing an overwrite?

Presumably that was a deliberate design decision, so that a node could show a message to the author saying "hey, you need to pay for this to make it visible". Hopefully it will be easier to integrate with this workflow in our new version - perhaps via Drupal 6 Actions (though perhaps not useful for your D5 project).

Why doesn't LM Paypal still work with this node-type? Is prepare called *after* our hook? You could always "fix" this with theming? I'm probably not going to make big changes to D5, but feel free to file a D6 feature request around this stuff, for the 6.x-2 branch.

Can we make this issue as "by design"?

Triskelion’s picture

Triskelion commented
Status: Postponed (maintainer needs more info) » Closed (works as designed)

I have a 'My Listings' menu item for logged in users which shows a view of their own content filtered on uid. I can show them both published and unpublished content, and give them payment links to alter the ad status. Using the node->status flag does not prevent the user from being prompted to pay.

I have changed this to 'By Design', but I would still appreciate some help here. I know that the ed_classified module is going to have to be hacked, but I first want to find a way to prevent other content from being available to the public. I am still deep in the code here (with limited time, but I'll figure it out).

Thanks for your input so far.

lyricnz’s picture

lyricnz commented

Yes, node->status is not used at all by the paid advertisement module - it keeps track of this itself. Are you trying to remove the created-but-not-paid items from some view? They should be getting modified on-load to include a message, but hiding them completely is better.

It seems that exposing the "paid" logic as a views field is an adequate solution for now, and I think that using node->status might be a better option for the future. What do you think?

Triskelion’s picture

Triskelion commented

I am kicking myself for not being a little more thorough. I inserted a case 'view': print_r($node); in your hook nodeapi, and I can confirm that the body and teaser are changed.

My problem is that all of the really important information on the paid nodes is defined in other fields (CCK, CCK_Taxonomy, Location, Gmap etc), which do not get overwritten. Customized content cannot be used as paid content with only title and body overwriting.

In my case it is going to be absolutely necessary to use node->status. I think what you were discussing with pounard is a good idea. It is relatively simple to add a 'toggle_node_status' field to the lm_paypal_subscriptions table, and to give the developer the option of using this method, on a case by case basis, in the subscriptions edit form. This would preserve the current API and extend the functionality to use of node->status if chosen. In future, I don't see any way that this module will have more than limited use if you do not use node->status.

My problem is trying to understand the work flow used to update the subscription status. I assume kind=0 and kind=2 refer to role and og respectively and the logic is fairly clear. I am having a problem following the 'kind=1' logic. I would appreciate it if you could nudge me in the proper direction. If I can get the two methods working side by side, you will be able to compare them and make up your own mind on the issue.

Triskelion’s picture

Triskelion commented

OK. The source of my confusion is now clear, and I have some specific questions.

1. Role and Group subscriptions are being granted in the code when the the subscr_signup ipn is received. If the subscr_payment completed ipn is never received, the subscription is active anyway. This is fine for trial periods with no payment required. But what if the payment is never received for a period requiring payment? I cannot find any checks here. Hook cron appears to be used only for sending email.

2. Does PayPal send a cancellation ipn at the end of the subscription, or when they receive the cancellation from the client? The subscriptions are being dropped on receipt of the ipn with no checks to see if there is any remaining time on the subscription.

3. Does PayPal send a cancellation ipn at the end of a subscription if the client simply does not renew? Again, I cannot find any cron function to expire subscriptions at end of term.

4. It seems to me that payments made for an active subscription are being rejected. What if the user wishes to pay for an additional fixed term. Should the payment not be accepted, and the expiry adjusted accordingly? I am wondering about setting an expiry date field in lm_paypal_subscribers.

The basic coding is done for a parallel publishing system using node->status. It was pretty simple to do. If you could give me answers to these questions, I can code the synchronization of the two systems.

Thanks.

LeeMcL’s picture

LeeMcL commented

Do remember that LM_PayPal is not a general subscription management system. It entirely relies on PayPal to handle starting and stopping subscriptions. It does not cross check that PayPal is correct - it just obeys incoming IPN's. I found PayPal's rules a little odd in places and quite limiting in others. I was sorely tempted on many occasions to just bite the bullet and write and entire subscriptions management system. But its a huge amount of work.

Also PayPal does not guarantee the order of the initial IPN's. Signup's and payments might arrive in any order and, as you mention, in the case of subscriptions with a trial period the first payment might not arrive for months. So you have to obey the signup - the payment isn't important. Also some messages can be received multiple times - so be very careful with using IPNs to toggle flags!

So:

1. Yes it just obeys the signup
2. Yes it just obeys the cancel
3. Yes PayPal sends a cancel
4. PayPal doesn't allow a subscriber to top up a subscription.

Unless PayPal has changed things you cannot change the rate of an existing subscription either. Nor is it possible to cancel a subscription other than by logging into the PayPal website. The PayPal subscription cancellation rules are a little odd too. You cannot get a list a users existing subscriptions. All this makes it very difficult to cross-check what PayPal says to you.

Lee

Triskelion’s picture

Triskelion commented

Lee

Thank you for responding. The PayPal Order Management Integration Guide is a wealth of incomplete information! It tells me what the variables are, but says nothng about when, or if, they will be sent.

I wanted to make sure that I understood how things were done, and to make sure that I didn't 'fix what was not broke'.

@lyricnz

I'm off to the sandbox for a while, then I'll post a patch.

Triskelion’s picture

Triskelion commented

Patch posted at #362438: Node->status implementation for LM PayPal