From what I can tell, _simpleads_load_ads() currently:

1. Loads the number of ads specified by $limit.
2. Checks to see if the ad is active, and displays it if so.

Say you set limit to 1, and simpleads loads an inactive ad. It will not process it as it is not active. Then it will end up returning an empty array when it should have loaded a different ad in the first place.

This can be fixed by checking the active field in hook_simpleads_order:

function simpleads_simpleads_order($delta, $term_ids, $limit) {
  $query = db_select('node', 'n');
  $query->join('taxonomy_index', 'ti', 'n.nid = ti.nid');
  $query->join('field_data_field_ad_status', 'a', 'n.nid = a.entity_id');
  $query->fields('n', array('nid'));
  $query->condition('n.type', 'simpleads');
  $query->condition('ti.tid', $term_ids, 'IN');
  $query->condition('a.field_ad_status_value', 1, '='); 
  $query->range(0, $limit);
  $query->addTag('node_access');
  .....

I'm not sure if this has implications for the rest of the module though.

Happy to roll a patch if you think this approach is okay.

CommentFileSizeAuthor
#5 fix_blank_ad_spaces-1760048-5.patch837 bytesPete B

Comments

soulston’s picture

soulston commented

I'm having this issue too.

I am not sure what the thinking behind the "This ad is active" tickbox and why the node publish state is not used as this is what the simpleads_simpleads_order() function currently checks for:

/**
 * Implements hook_simpleads_order().
 */
function simpleads_simpleads_order($delta, $term_ids, $limit) {

  $query = db_select('node', 'n');
  $query->join('taxonomy_index', 'ti', 'n.nid = ti.nid');
  $query->fields('n', array('nid'));
  $query->condition('n.type', 'simpleads');
  $query->condition('ti.tid', $term_ids, 'IN');
  $query->condition('n.status', 1);
  $query->range(0, $limit);
  $query->addTag('node_access');

Maybe it's a better option to use the node status and then we can do away with the "This ad is active" tickbox altogether?

samhassell’s picture

samhassell commented

Looks like the active checkbox got added in http://drupal.org/node/1401812

As a summary, It used to use unpublish but you had to set the Active Date to a future date in order for it to work.

Seems like there should be a more Drupally solution that makes unpublish work correctly.

soulston’s picture

soulston commented

ah - of course, so the on/off date would need to trigger the published state.

At the moment it's counter intuitive as an ad that is unpublished is still loaded. Suppose the "This ad is active" could still be used to turn the ad on and off but the published status would be the thing that is triggered by the date.

It's probably the wording that needs re-thinking. Enabled/Disabled would be better with a help text to say that an Enabled advert just means it's available to the system, much like contexts or rules can be enabled/disabled - just because a context is enabled doesn't mean it's doing anything unless the conditions are met.

Maybe it could work like this:

  • Adverts have a state of Enabled/Disabled
  • On node save check the start/end date
  • If it should not be active set the published state to 0
  • hook_cron will take care of the rest
  • Only show the ad if it's enabled and published

Thoughts?

liquidcms’s picture

liquidcms commented

i stumbled upon same issue. Inactive ads are loading blank. Pretty sure this is incorrect.

As for pub/unpub versus active/inactive; what is the purpose of enabled/disabled if not to show in ad rotation? i am not sure what the use case is for 4 states; perhaps someone could define these? failing that i don't much care if a checkbox or simply use node status (pub/unpub) - but it is confusing as it is now.

Pete B’s picture

Pete B commented
StatusFileSize
new fix_blank_ad_spaces-1760048-5.patch837 bytes

As stated above, #2 and #5 from Issue 1401812 make a good case for having a separate "active" control here.

Therefore I think the OP's approach was correct. I have rolled a patch of the change he suggested, which I have tested on my install.

Respect to samhassell.

Thanks,
Pete

minnur’s picture

minnur commented
Issue summary: View changes
Status: Active » Closed (won't fix)