Updates from April, 2013 Toggle Comment Threads | Keyboard Shortcuts

  • Paul Gibbs 1:26 pm on April 10, 2013 Permalink | Reply  

    Hi all,

    On a dotcom site that uses Ad Code Manager, we’re seeing Google try to crawl ?preview= URLs (I have CSV reports from the site owner of the URLs that Google is failing to access). Not super sure how Google is finding those, but @danielbachhuber suggested it might be something like when an author is previewing a post, this triggers AdSense, AdSense reports back to Google the URL the code was triggered on, and Google tries to call it, and it errors.

    If this sounds plausible, I’d like to suggest that adsense is disabled (i.e. not output) when is_preview() === true. ?

    googletag.pubads().set(“page_url”, “URL”); is another option, but as page slugs/URLs may change during successive previews, invalid URLs might still occur.

  • Daniel Bachhuber 9:12 pm on February 28, 2013 Permalink | Reply
    Tags: ,   

    … and I’m back into it. Let’s discuss a launch date for v0.4. I think the easy Google AdSense / Google DFP configuration is a good highlight feature.

    @rinatkhaziev @jeremyfelt Of the other features we lined up for the milestone, is there anything you’d like to try to finish up before release?

    I think two or three weeks from now would be a good date to hit.

  • Daniel Bachhuber 11:56 pm on June 25, 2012 Permalink | Reply

    Synced over the extent of this commit to WordPress.com VIP (except the unit tests).

    If you could commit against an issue when you’re making improvements, it would be much appreciated. It’s much easier to refer to a series of commits this way 🙂

    Thanks guys

    • Rinat K 4:25 am on June 26, 2012 Permalink | Reply

      @danielbachhuber I closed the outstanding issue and tagged 0.2.3 on github, wanted to push to WPORG but found out that I have no write privileges. So either give me a write or tag it yourself, please 🙂

      • Daniel Bachhuber 7:30 pm on June 26, 2012 Permalink | Reply

        Sorry about that, I’ve given both you and @jeremyfelt commit access.

        When you tag the version, can you also remove the .js and .css files from jqGrid that are no longer in use? I’ve been unzipping the new version into the directory, which means changed files get updated and new files get added, but old files aren’t deleted…

        I’ve already removed the old files from the VIP repo.

        • Rinat K 8:44 pm on June 26, 2012 Permalink

          I found out that rsync comes in pretty handy in those situations

        • Rinat K 4:09 am on June 27, 2012 Permalink

          on the other hand, rm & svn rm are ok too 🙂

        • Mo Jangda 4:11 am on June 27, 2012 Permalink

          rsync -rv --delete --exclude='.svn' /from /to
          cd /to
          svn status | grep '^!' | awk '{print $2}' | xargs svn rm
          svn status | grep '^?' | awk '{print $2}' | xargs svn add


        • Rinat K 4:14 am on June 27, 2012 Permalink

          Ha, glad that you’re still with us and thanks for the snippet 🙂

        • Jeremy Felt 3:06 am on June 28, 2012 Permalink

          It should be known that Mo’s rsync comment made way for a beautiful addition to my local bash profile

  • Rinat K 7:21 pm on June 7, 2012 Permalink | Reply  

    Hey guys, there’s one issue left: https://github.com/Automattic/Ad-Code-Manager/issues/26

    What are your thoughts on it, @danielbachhuber, @jeremyfelt

  • Jeremy Felt 12:05 am on May 22, 2012 Permalink | Reply  

    I ran into an issue with all columns being required when creating/editing an ad code – notes are up on GitHub.

    My current thought is to solve this by adding a filter to action_load_providers() that sets something like $optional_provider_columns that we can check alongside $ad_code[$slug] in create_ad_code() and edit_ad_code().

    Does that sound reasonable? I can’t find anything that would break as a result of this change, but I’m still hunting.

    • Daniel Bachhuber 4:51 pm on May 23, 2012 Permalink | Reply

      To be honest, I’m not entirely sure of the origin of that code. It seems like the best solution might be to keep the fields required for an ad code independent of the columns that appear, but then have them influence which columns appear. Something like this:

      $this->ad_code_args = array(
      		'key'       => 'site_name',
      		'label'     => __( 'Site Name', 'ad-code-manager' ),
      		'editable'  => true,
      		'key'       => 'zone1',
      		'label'     => __( 'zone1', 'ad-code-manager' ),
      		'editable'  => true,
      $this->columns = array();
      foreach( $this->ad_code_args as $ad_code_arg ) {
      	if ( !$ad_code_arg['editable'] )
      	$column = array(
      		$ad_code_arg['key'] => $ad_code_arg['label'],
      	$this->columns = array_merge( $this->columns, $column );
      $this->columns = apply_filters( 'acm_provider_columns', $this->columns );

      I think we’d want to standardize it a bit more and stick it in the base class so it auto-populated $this->columns based on the args. Or, drop $this->columns altogether and build our columns based on $this->ad_code_args

      • Jeremy Felt 5:58 pm on May 23, 2012 Permalink | Reply

        If I’m reading it right, the structure is on the right track, but we need another argument for ‘required’ when defining the codes. Rereading my GitHub comment, I’m not even sure I conveyed the problem correctly. 🙂

        $this->ad_code_args = array(
                'key'       => 'site_name',
                'label'     => '__( 'Site Name', 'ad-code-manager' ),
                'editable'  => true,
                'required'  => true,
                'key'       => 'zone1',
                'label'     => __( 'zone1', 'ad-code-manager' ),
                'editable'  => true,
                'required'  => false,

        In the case above, both site_name and zone1 would be editable, but the user could leave zone1 blank when saving the code.

        The second part of the problem occurs in create_ad_code() and edit_ad_code(), where we would need to check that required flag.

        function create_ad_code( $ad_code = array() ) {
            $titles = array();
            foreach ( $this->current_provider->columns as $slug => $col_title ) {
                // We shouldn't create an ad code,
                // If any of required fields is not set
                if ( ! $ad_code[$slug] ) {
                $titles[] = $ad_code[$slug];

        An additional check could be added to if( ! $ad_code[$slug] ) or we could take that out entirely and find a way to catch the field as a required input before the form was submitted.

        • Daniel Bachhuber 6:15 pm on May 23, 2012 Permalink

          You’re right, I missed the ‘required’ argument. The create_ and edit_ functions would obviously need to be updated to properly validate the ad codes. Eventually we could add a ‘type’ argument to ad codes and/or a whitelisted set of values to offer better form validation.

        • Rinat K 9:29 pm on May 23, 2012 Permalink

          Sorry guys, I kind of got out of the loop on this one. I like $this->ad_code_args solution!

        • Rinat K 4:15 am on May 25, 2012 Permalink


          I did some work on known issues, so this is only one outstanding for 0.2.2 release. Well, there’s another one, but it’s related to it.

        • Rinat K 6:26 am on June 7, 2012 Permalink

          Hey @danielbachhuber @jeremyfelt, i made a swing, it is in a branch refactor-providers.
          I didn’t test thoroughly but my fairly rudimentar test suite (in a branch tests) shows that at least creating and getting ad codes working as expected.

  • Daniel Bachhuber 2:53 pm on May 4, 2012 Permalink | Reply

    @rinatkhaziev @jeremyfelt how are you guys feeling about v0.2? Release today or Monday?

  • Daniel Bachhuber 6:00 pm on April 30, 2012 Permalink | Reply

    Finished refactoring the WP List Table implementation this morning. Here’s what I’ve identified as needing work for v0.2:

    @jeremyfelt want to take a swing at updating the readme and including screenshots? @rinatkhaziev have time to start testing? I’m cool with releasing this as soon as I get the checkoffs from you guys 🙂

  • Daniel Bachhuber 1:26 am on April 5, 2012 Permalink | Reply
    Tags: conditionals   

    In a recent commit, Rinat modifies the following code:

    // If the ad code doesn't have any conditionals
    // we should add it to the display list
    if ( empty( $ad_code['conditionals'] ) ) {
    	if ( $this->logical_operator == 'AND' ) {
    		$display_codes[] = $ad_code;


    // If the ad code doesn't have any conditionals
    // we should add it to the display list
    if ( empty( $ad_code['conditionals'] ) ) {
    	$display_codes[] = $ad_code;

    This means that each ad code will display on the site as soon as it’s added in the admin if it doesn’t have any conditionals applied. I think that this is probably a bad thing and that users will think it a bug as soon as we push the code that works “correctly”.

    Instead, we might do one or more things:

    1) Have an “ALL” logical operator which means that ad code are loaded regardless of conditionals. Something like:

    if ( empty( $ad_code['conditionals'] ) && $this->logical_operator == 'ALL' )

    2) Have ‘published’ vs. ‘draft’ states for ad codes that will allow some amount of pre-publication control.

    Thoughts? Feel free to weigh in before I create a ticket on this.

  • Daniel Bachhuber 7:01 pm on March 23, 2012 Permalink | Reply  

    Moved the Ad Code Manager repo so you’ll want to update your remote repository accordingly 🙂

    @rinatkhaziev any progress on the updated UI? can I help at all?

  • Daniel Bachhuber 3:05 am on February 27, 2012 Permalink | Reply

    @rinatkhaziev I just committed an implementation of priorities. Mind pulling the commit and checking it out in your local environment? The priorities work much like they do with actions and filters in core.

    • Rinat K 2:05 am on March 1, 2012 Permalink | Reply

      @danielbachhuber Finally had a chance to test it, found one issue and pushed a fix to develop branch. Other than that, looks good! Logical operator check is also removed now.

      • Daniel Bachhuber 12:42 am on March 2, 2012 Permalink | Reply

        Sweet, thanks for the fix. As a FYI, you can reference and close issues with syntax like “See #14” and “Closes #4”

Compose new post
Next post/Next comment
Previous post/Previous comment
Show/Hide comments
Go to top
Go to login
Show/Hide help
shift + esc