[D7] SweetCaptcha

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxamreana2154001git

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

While giving GIT clone command, please provide the non-maintainer command : "git clone --branch 7.x-1.x http://git.drupal.org/sandbox/amreana/2154001.git sweetcaptcha"

PAReview Passed: http://pareview.sh/pareview/httpgitdrupalorgsandboxamreana2154001git

Manual Review:

1. Instead of Defining two fieldset i.e. 'sweetcaptcha_registration' in sweetcaptcha.admin.inc file, you could have just defined it once and depending on your condition shown the description.
2. Are website, email, language, Design compulsory fields? They've "*" icon next to them. And that is given in title. If these are required fields you can make them as #required using Drupal Form API itself, which will automatically add "*" icon next to them.
3. On module configuration page, "already" text spelling is wrong, "If you allready own an account simply fill the fields below."
4. My account was supposed to be created automatically after I fill required details on configuration page, tick the checkbox "I wish to register a new username" and submit the form? Because the line says "SweetCaptcha is currently UNREGISTERED. Please use the form below to register."
5. If Yes for account creation, even after filling all these details my account won't get created on SweetCaptcha site.
6. I don't find any use of showing "I wish to register a new username" checkbox, if user is already registered i.e. filled proper API details.
7. If you're defining your own submit button, you don't need to pass your $form array to "system_settings_form()" function. Because you're already doing what this function does i.e. variable_set(). Also you don't need to define '#submit' for your form with "FORM_ID_submit" as submit handler, by default Drupal will call FORM_ID_submit().
   

   <?php
     $form = system_settings_form($form);
     $form['#submit'][] = 'sweetcaptcha_settings_form_submit';
     return $form;
   ?>
   

   So the above code will be change to:

   <?php
     return $form;
   ?>
   

8. Remove extra line in SweetCaptcha.admin.inc file on line 8.
9. Code on, line 198 - 215 in SweetCaptch.admin.inc file and line 74 - 95 in swcore.inc file does same thing, you can merge this code in one function and call that function from these two files with proper arguments. That will reduce line of code.
10. Suggestion: In swcore.inc file "API_URL", try getting it dynamically may be using variable_get, because if the SweetCaptcha changes its API URL to suppose, api.sweetcaptcha.com then it would be just configuring API URL using settings UI. Not sure if this would work here. Same for "SWEETCAPTCHA_API_URL" in sweetcaptcha.module file on line 20.
11. The comment for @file, should end with "." period in sweetcaptcha.module file on line 5.
12. Please remove extra spaces for "'type' => MENU_CALLBACK," in hook_menu on line 51 and 58 in sweetcaptcha.module file.
13. Not sure, if I've correctly configured SweetCaptcha or not, but it was not allowing me to save comment. I was trying with anonymous user with permission to post comment. The math captcha allowed me to save comment. Please find the screenshot attached.

> description = SweetCaptcha is a free Captcha service putting the users and your website at first priority. This is a new and fresh graphical Captcha focused on enhanced user experience, so instead of using difficult and boring text, SweetCaptcha offers a fun and interactive friendly user experience.

That's really long for the module admin page.

Also, I have no idea what "putting the users and your website at first priority" means. The text should state what the module does in clear terms, rather then sound like marketing copy.

> class Sweetcaptcha {

This class's methods have incomplete docs.

if (variable_get('sweetcaptcha_folder') !== '') {
  $folder = '/' . variable_get('sweetcaptcha_folder');
}
else {
  $folder = '';
}
// ...
define('SWEETCAPTCHA_PUBLIC_URL', $base_root . $folder . '/swcore');

Are you having the user put files in their webroot? That doesn't strike me as very clean. Though I don't fully understand what's going on here. What is the sweetcaptcha_folder variable doing? Documentation!

> $items['swajax'] = array(

Best to prefix your root-level paths with the whole module name to avoid collisions.

>

Use the warning CSS class that's built into Drupal. Or use drupal_set_message(), which is even better UI.

> $description = '

' . t('SweetCaptcha is REGISTERED.') . '

';

Do you need to say this on the admin form every time? Maybe this belongs in hook_requirements().

  variable_set('sweetcaptcha_folder', $form_state['values']['sweetcaptcha_folder']);
  variable_set('sweetcaptcha_reg_website', $form_state['values']['sweetcaptcha_reg_website']);
  variable_set('sweetcaptcha_reg_email', $form_state['values']['sweetcaptcha_reg_email']);
  variable_set('sweetcaptcha_reg_design', $form_state['values']['sweetcaptcha_reg_design']);
...

You might be able to use system_settings_form() to make this code simpler.

> 2. Added a more detailed description to the Sweetcaptcha class.

More than this, every method and function needs to document its parameters and return value.

• Readme has some issues under install steps you miss the upper-case letter at the start of sentence and dots from the end.
• t function in sweetcaptcha_swajax($arg = '') seems unnecessary.
• == "true" seems like something that might fail in some cases.
• You should uninstall variables one by one instead of deleting them from database directly.
• $t = get_t(); shouldn't this be st() instead?
• Is the module package something global or your own creation. I would drop my own creation.

> Is the module package something global or your own creation. I would drop my own creation.

That must have changed since I reviewed this the other day. I checked for this and it looked like it was the same module package as the Captcha module.

1- Unable to access form from the URL: admin/config/people/captcha/sweetcaptcha. if i enable it using drush en command.
2- Not pursesting the value I wish to register a new account element and related form.
3- If i will not input the value nither Website* nor Email* than it by pass the validation of Website element to next email element.

Hi Amreana,

I verified your fixed but fund validation issue is exists now.
1- If i input invalid email id without filling the website field value it save it and does not return any validation error.
If i put any value in website field with invalid email id it returns a message: invalid email dev.firoza.
2- If i input valid email id and invalid website (e.g. drupal). It returns an error invalid website drupal. if both fields are required than error message should be populate in sequence.
3- If i input email id in both fileds (email and website). It returns an error "This website is already registered with the email your have provided. Click here to have us send your crdentials to your email" But i found that no registration has been done.

I am unable to reproduce the point no. 1 in issue Id #19.

Removed automated reviews.

Review of the 7.x-1.x branch:

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  
  FILE: /home/klausi/pareview_temp/sweetcaptcha.install
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   19 | WARNING | Format should be "* Implements hook_foo().", "* Implements
      |         | hook_foo_BAR_ID_bar() for xyz_bar().", or "* Implements
      |         | hook_foo_BAR_ID_bar() for xyz_bar.tpl.php.".
  --------------------------------------------------------------------------------
  

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

1. swcore.inc: appears to be 3rd party code. 3rd party code is not generally allowed on Drupal.org and should be deleted. This policy is described in the getting involved handbook. It also appears in the terms and conditions you agreed to when you signed up for Git access, which you may want to re-read, to be sure you're not violating other terms. The Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org.
2. sweetcaptcha_swajax(): doc block is confusing, this function does not generate a URL and does not return it?
3. sweetcaptcha_swajax(): what is $command and why do you send it through t()? Please add a comment.
4. sweetcaptcha_settings_form(): the values of $designs and $languages should run through t() for translation.
5. "drupal_set_message($api_call_result['error'], 'error', FALSE);": this is vulnerable to XSS exploits. The API call result stems from a third party source and therefore it must be considered to be untrusted user provided input. You need to sanitize all user provided input before printing. Make sure to read https://drupal.org/node/28984 again. But I guess users will have to fully trust the service anyway, because in sweetcaptcha_captcha() you are inserting HTML from that source, so they can already XSS you. So I think this is not a security issue.
6. sweetcaptcha_captcha(): why the special handling for maintenance mode? Please add a comment.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Manaul Review:

You have removed the * from Website and Email but it is returning email validation error. so this field should be mandatory and you should be used drupal inbuild function 'valid_email_address($mail)'.

You can send the Ajax request using the jQuery. So we no need to create the request object.
So you need to remove the function customGetter.
Call a ajax request like:

(function ($) {
        Drupal.behaviors.ajax_example = {
        attach:function (context) {
            // If the site name is present set it to the username.
                if ($('#element-id', context).length) {
                    $.ajax({
                    url: '/ajax/xyz',
                    success: function(data) {

                    // Change site name to current user name.
                    $('#site-name a span').html(data);
                    }
                    });
                }
            }
        }
    })(jQuery);

manual review:

1. sweetcaptcha.module: do not execute PHP in the global scope. You should only include the library when you actually need it, not on every single page request. That would mean in sweetcaptcha_captcha(). And why do you need to define the constants that are just variable_get() anyway?
2. sweetcaptcha_menu(): so the file swcore.inc does not exist anymore for the path sweetcaptcha_core? Looks like that page callback is not used anyway and can be removed?

But otherwise looks RTBC to me.

Assiging to stBorchert who might have time to take a final look at this.

Some minor notes from me:

sweetcaptcha.js

• I suggest prefixing the behavior names with "sweetcaptcha" (or simply "sc") to prevent possible accidently duplicates in other scripts.
• var sckey is never used in customLoop so it may be removed.

Apart from this it looks very good so I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here 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 lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, 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.

Thanks to the dedicated reviewer(s) as well.