Updates from May, 2012 Toggle Comment Threads | Keyboard Shortcuts

  • 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(
      	array(
      		'key'       => 'site_name',
      		'label'     => __( 'Site Name', 'ad-code-manager' ),
      		'editable'  => true,
      	),
      	array(
      		'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'] )
      		continue;
      	$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(
            array(
                'key'       => 'site_name',
                'label'     => '__( 'Site Name', 'ad-code-manager' ),
                'editable'  => true,
                'required'  => true,
            ),
            array(
                '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] ) {
                    return;
                }
                $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

          Hey,

          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 12:31 am on May 15, 2012 Permalink | Reply
    Tags: v0.2.1   

    Tagged and released v0.2.1 which solves these issues:

    • Flush the cache whenever an ad code is created or deleted so you don’t have to wait for a timeout with persistent cache
    • Bug fix: Default to priority 10 when querying for ad codes if there is no priority set

    Thanks to @rinatkhaziev for helping me out with this… I think we should do v0.2.2 a few weeks from now and include bulk editing/deleting ad codes, etc.

     
  • Daniel Bachhuber 11:11 pm on May 7, 2012 Permalink | Reply
    Tags:   

    Version 0.2 is out the door… nice work guys!

    Here’s my announcement post for tomorrow, please give it a read and let me know what you think. I’ll publish tomorrow morning PT.

    I already found one bug 🙂

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

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

     
c
Compose new post
j
Next post/Next comment
k
Previous post/Previous comment
r
Reply
e
Edit
o
Show/Hide comments
t
Go to top
l
Go to login
h
Show/Hide help
shift + esc
Cancel