garbanzito [garbanzito]

Hello, and thanks for applying for a CVS account. I am adding the review tags, and some volunteers will review your code, pointing out what needs to be changed.

As you reported links to projects that in someway allows to handle surveys, may you report the pro of using the proposed module versus using the existing projects?

Looks great, and I love all the commenting!

Few things found under code inspection:

In the _surveygizmo_request() method of the surveygizmo_api class you use file_get_contents() and have a TODO to use a more graceful solution. How about using drupal_http_request()? This allows for easily checking the response code. You can also use http://api.drupal.org/api/function/drupal_query_string_encode/6 to build the query part.

In surveygizmo_present_survey() how about wrapping the $fail message in t() so it can be translated? You can pass the $sgid and $survey variables as arguments.

You don't need the check_plain() in surveygizmo_manage_survey() on 'Manage survey', but you could use t() so it's translatable.

I think you need a return; or exit; after using drupal_not_found() in the functions surveygizmo_form_manage_survey() and surveygizmo_manage_survey()

Arguments in the $embed_description on line 190 of surveygizmo_form_manage_survey() appear to not actually be used in the string. I don't see the replacement @format used.

I don't see any, but maybe I'm missing it, but is there anything stopping me from setting a survey to an existing system path, like 'admin' or 'node/add/page' ? I don't think this is a blocker though cause it's a bit difficult to check for all the system paths. You could see what Views does, if anything. This could be handled later once the module is on contrib.

There are a few text arguments to l() in surveygizmo_dashboard() that should be run through t().

is there anything stopping me from setting a survey to an existing system path, like 'admin' or 'node/add/page'

Pathauto handles that with the below code. I just tried to create a node with a pattern of "user/[title-raw]" and a node title of "register" and Pathauto silently refuses to create the alias. I'm not sure being silent is right, but at least we know that the check that Pathauto does works ;)

function _pathauto_path_is_callback($path) {
  $menu = menu_get_item($path);
  if (isset($menu['path']) && $menu['path'] == $path) {
    return TRUE;
  }
  return FALSE;
}

I guess something similar could work here as well.

I've reviewed again, and installed and used the module and I believe garbanzito's app should be approved. As a coworker I don't want to RTBC but I do think it's RTBC status.

I will review the code tomorrow morning (less than 24 hours from now).

1.   if (!$fail) {
       drupal_set_title(check_plain((string)$survey->title));
       $output = (string)$survey->js_embedingcode;
       $output .= (string)$survey;
     }
     else {
       drupal_set_title(check_plain($fail));
       $output = "<p>" . t("This survey is not configured or is no longer valid.") . "</p>";
     }
   

   Error messages, as other type of messages, are shown with drupal_set_message().

2.   $form['presentation']['embed_token_description'] = array(
       '#value' => '<p><strong>' . t('Embedding') . ':</strong> ' . $embed_description . '</p>',
     );
   

   In this case, passing a single string to t() would give more context to who translates the strings. There are languages where the word embedding can be translated in different ways, depending on the context. Without such context, it would not be possible to translate the word correctly.

require_once('surveygizmo.api.php');

The code doesn't work, as it's trying to load the file from the Drupal root directory, which is the current directory set by Drupal. If the file is always included, then it is better to merge it with the module file.

Thank you for your contribution! I am going to update your account.
These are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, also, for your patience with the review process.
Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

I thank all the dedicated reviewers as well.