automattic / ad-code-manager Goto Github PK

So we have acm_list_table_columns and acm_provider_columns. All the difference between them is that acm_list_table_columns return all table columns (e.g. id, name, priority, conditionals) and acm_provider_columns returns only ad network specific ones. May be we could use one filter and just skip service columns.

Give the users an ability to utilize different ad providers. E.g. a user wants to display DFP ads above the fold, and some home brewed ads below the fold.

Hi,
When we looked at the code the specially for double click, we did not find any way to render iframe ads. Looks like right not it just supports javascript as. Is there a plan to incorporate iframe ads too?

Regards
Amit Sannad

basically, something like this:

add_filter( 'acm_list_table_columns', 'my_acm_list_table_columns' );
function my_acm_list_table_columns( $columns ) {
    $columns = array(
        'site_name'      => __( 'Site Name', 'ad-code-manager' ),
        'zone1'          => __( 'Zone1', 'ad-code-manager' ),
        'campaign'       => __( 'Campaign', 'ad-code-manager' ),
        'priority'       => __( 'Priority', 'ad-code-manager' ),
        'conditionals'   => __( 'Conditionals', 'ad-code-manager' ),
    );
    return $columns;
}

will break UI for good.

Conditionals could be very confusing for end-user. We need to add some kind of cheat sheet to UI, with explanations on how to use it.

Let's get this out as a bugfix release before v0.5

I got an idea this morning. We all know that repetition is bad. So instead of writing a readme section and then duplicating it as a contextual help, we could parse Markdown from readme.

Per our first real world tester Steve Baron:

It would be nice to have kind of responsive approach and paginate records only if their total rendered height exceeds available viewport space.

Currently it POSTS the parent form, which means the changes to your ad code are lost.

For example:

googletag.defineSlot("/1234567/COOL_AD_TAG", [1, 1],"derp").addService(googletag.pubads());

Google will try to crawl:

http://mycoolsite.wordpress.com/1234567/COOL_AD_TAG

And will return a 404.

After some googling, it appears the quickest dirtiest way to do it is treat the pub id as a directory and set it to not follow in robots.txt

This is related to the upgrade path from v0.1.3 to v0.2 and I unfortunately noticed it after I shipped v0.2. It shouldn't be too big of a problem because we have some redundant checks in place.

Basically, ad codes in v0.1.3 didn't have priorities set. In v0.2, our get_ad_codes() query doesn't account for this fact when it should. If the postmeta value returns empty, we should set the priority to 10.

The whole conditionals thing might be confusing for end users. We should provide inline help, so that whenever users selects a conditional we display a short description on how to use this conditional, what arguments it might take, etc. See #46 for discussion

It would be nice to have config scanner to check if the plugin is configured properly:

• provider file is present,
• ACM_WP_List_Table columns are properly defined,
• ad code args are set correctly, there’s at least one tag id,
• etc, you name it

Let's make sure the readme is all up to date with what's new and what we've changed. Going through our Github issues and the commit messages is a good way of tracking this down.

Rather than requiring each provider configuration to be loaded from a file in the ACM providers directory, a filter should be available that allows a provider and table class to be registered from an outside source.

Hey,

has_category produces incorrect results. For example, if we are on home page, and there's a loop with posts where has_category( 'news', $post_id ) equals true, the news ad code will be added to $display_codes

We don't have any control over which code to display:

$code_to_display = $display_codes[0];

If has_category conditional was evaluated before is_home conditional, wrong ad code will be rendered.

I've made a workaround for this issue: https://gist.github.com/56f5825bd42200b7b36d

I'm not sure of proper way to fix it, but we might want to implement priority.

<?php
$display_codes = array( '0' => array ( array( /* homepage ad code */ ) ),
                       '10' => array ( array( /* news ad code */ ) ),
                       );
// and then we just
$priority = min( array_keys( $display_codes ) ); // or max
$code_to_display = $display_codes[$priority][0]

As it's implemented now, it's kinda hackish and doesn't fully leverage that API. We should improve it a bit

For instance, is_singular or is_home

Let's make sure everything is still relevant.

In both edit_ad_code() and create_ad_code(), the process is aborted if we run into a column that has been registered for the provider, but not available as part of the ad code data.

Depending on the view, it is possible that not all ad codes will make use of every column. Ideally, we'd have a way for provider authors to specify which columns are required or optional.

Per @jeremyfelt:
it would be cool if we could modify the input fields for each conditional either through filters or just by default. For example – If is_category() is selected as the conditional, a drop down list of existing categories would be more pleasant than a text field.

To make ad tags more portable, we should allow them to be embedded in post and widget content with shortcodes

Setting the logical operator from OR to AND does not seem to result in the expected behaviour for displaying ads.

For example:
Adding the conditional 'has_category' with value 'category 1' results in the ads only being displayed on category 1 pages.
Adding the conditional 'is_archive' with value '1' results in the ads only being displayed on archive pages.
However, including both conditionals shows the ads on all archive pages (if has_category is followed by is_archive) or all category 1 pages (if is_archive is followed by has_category).
The expected behaviour would be to only show ads on category 1 archive pages.

If you try deleting the last conditional for an ad code, you'll get errors in your PHP log and the conditional won't disappear until you refresh the page.

If provider class doesn't have columns_{$custom_column} method, there will be no row actions presented. It would be nice to figure out default logic for the first column to display dem actions.

Initial conversation: http://adcodemanager.wordpress.com/2012/04/05/in-a-recent-commit-rinat-modifies-the-following/

Basically, the current behavior is to display an ad code if the logical operator is set to AND and the ad code has no conditionals. This is probably the incorrect behavior. The post describes adding an "ALL" logical operator, but "ALL" isn't really a logical operator. Let's make this behavior opt-in with a filter instead.

If you go to My Blogs on WPCOM and have ACM enabled you will see ACM columns in addition to My Blogs columns.

Our Javascript is a little haphazard. We should try to clean it up a bit. Once it's cleaned up, we should try to clean it up a bit more.

Take a look at https://github.com/Automattic/Ad-Code-Manager/blob/develop/ad-code-manager.php#L970.
If we don't have any conditionals and acm_display_ad_codes_without_conditionals returns true we add ad code to display list and then continue. In reality ad code without conditionals still gets to display_list.

Workaround would be:

            if ( empty( $ad_code['conditionals'] ) && ! apply_filters( 'acm_display_ad_codes_without_conditionals', false ) ) {
                continue;
            }

It works fine, but looks bizarre. It's the only one blocker i encountered so far while testing v0.2-alpha against sandboxed copy of a real site.

The link pointing to Github in the VIP lobby still points to the old Github URL. This might not be the best place to point it out, but it felt awkward firing off an e-mail to support@ just for that. :)

This is everything that was added, fixed or modified right before launch.

Give users an ability to fine-tweak ACM config via some UI.

Currently, we only let ad codes pass if the logical_operator is set to "AND". This was originally a hack to avoid ad codes without conditionals taking top priority.

We should implement priorities #6 and then remove this.

DFP using async tags now allows ad slots to be defined with variable dimensions. A single ad tag can, for example, display either a 300x250 or a 300x600 ad. The Async DFP provider script should support defining ad tags with multiple dimensions.

There might be a case when there are two tags registered with the same dimensions and the same tag id ( which is just a div id ). This confuses DFP Async, so we need to make sure that tags are unique

All of the AJAX actions should have caps checks

Although it's not a huge deal, we should have this cached anyway.

This plugin should have a readme describing how to set it up, etc.

Conditionals should be evaluated with OR logic, otherwise the plugin won't render codes properly.

Example:

will never produce correct ad code, because one of them never equals true

Similar to what we've done with Google DFP Async, it would be great if our Google AdSense provider file offered much simpler integration.

Basically, if you create multiple ad codes with exactly same postmeta (site_name and zone1 in DFP case), ads won't show up for the whole site_name. Suspect is get_ad_codes(), but needs testing.

Say I want one ad code to be evaluated as:

has_tag apple AND
is_singular

and another to be evaluated as

is_category OR
is_tag OR
is_archive

This currently isn't possible. I think the best way to accomodate is by adding another interface element to the admin screen.

Would often be useful to have a debug bar extension for Ad Code Manager rather than needing to view the source of a page to confirm that ad tags are displaying right.

So, if this were to happen, what's the thought on where - separate plugin entirely or part of ACM by default?

My initial thought is to make a separate plugin entirely just to avoid extra code in the main ACM codebase.

subj

The WP_List_Table is instantiated on every view currently, by using the 'current_screen' hook. It should just be instantiated on the template.

Before each ad tag is output, it undergoes a whitelist check on it's URL. This results in action_acm_tag() (ad-code-manager.php:867 ) and register_ad_code() (ad-code-manager.php:696) returning early if the ad tag has no URL.

The current workaround is to whitelist a URL then set it as default.

This should be probably a row action which either pops an overlay with previews or displays them inline

I thought of this as a way to abstract out the DFP-related code and also support other ad networks.

Basically, we'd have a file in a /providers/ directory for each ad network. This file would include all of the necessary defaults, configuration, whitelisted domains, etc. for that ad network. It would interact with the plugin by manipulating the filters we have set up. We'd make choosing your provider a code-level option or, if there were more than one, have a dropdown selector in the admin interface.

When you're editing an ad code with the inline edit functionality, you should be able to delete an existing conditional.

All that's needed for this implementation is a bit of JS that removes the input element from the DOM. When the user submits the form, the data will no longer exist and the conditional will be removed from the ad code

To show potential users what the admin interface looks like and how you should use it, we should include screenshots.

We should probably clean up the admin interface a bit first though.