[D7] SharpSpring Webforms

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.

There seems to be some errors in the module as reported by pareview:

http://pareview.sh/pareview/httpgitdrupalorgsandboxmojiferous2480987git

Automated Review

http://pareview.sh/pareview/httpgitdrupalorgsandboxmojiferous2480987git

Git errors:

Git default branch is not set, see the documentation on setting a default branch.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

Nice Module. you are near RTBC
Git errors:

• Git default branch is not set, see the documentation on setting a default branch.

Review of the 7.x-1.x branch (commit 1387216):

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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. Remove the .gitignore file from the module directory.
2. Add more information on the project page.See the tips https://www.drupal.org/node/997024
3. Readme.txt is Nice!
4. Is module workable with both webform apis 3 and 4 ?
5. Move the configuration form sharpspring_webforms_config_form into a seprate file sharpspring_webforms.admin.inc and move the direct queries into chunks of function to make it reusable.It will give performance gain.
6. (*)sharpspring_webforms_config_form_submit : $db_name = 'sharpspring_webforms'; This variable holds the name of table so can we make it more relevant like $table_name . Nice use of drupal_write_record. Line 238,239 : Don't do check_plain while saving the endpoint and base_uri to database
7. (*)sharpspring_webforms_form_alter : Line 64,68. The base_uri and endpoint are untrusted.So use check_plain on them.
8. sharpspring_webforms_config_form : Use static query instead of dynamic query. It will give performance hits.
9. sharpspring_webforms_form_alter : Its not a good practice to attach js in $form['#suffix']. Use $form['#attached'] instead

Please take another review bonus for the next review.

Thanks!

Thanks! for the changes.
Review of the 7.x-1.x branch (commit f989df6):

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

  
  FILE: ...ites/all/modules/contrib/pareview_temp/sharpspring_webforms.admin.inc
  ---------------------------------------------------------------------------
  FOUND 1 ERROR AFFECTING 1 LINE
  ---------------------------------------------------------------------------
   6 | ERROR | [x] Doc comment long description must end with a full stop
  ---------------------------------------------------------------------------
  PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
  ---------------------------------------------------------------------------
  
  Time: 277ms; Memory: 6.5Mb
  

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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. Add a hook_help in module at the top of all functions in .module file.
2. sharpspring_webforms_form_alter :
   

         $result_check = db_select('sharpspring_webforms', 'sw')
           ->fields('sw')
           ->condition('nid', $webform_id, '=')
           ->condition('status', 1)
           ->execute()
           ->fetchAll();

   Put these code in some helper function so that it can be reusable.

3. /**
    * @file
    * sharpspring_webforms.admin.inc
    *
    * Provides admin configuration for the SharpSpring Webforms module
    */

   The first line should be the summary of the file.So make it simple like

   /**
    * @file
    * Provides admin configuration for the SharpSpring Webforms module
    */

4. sharpspring_webforms_config_form : Move the html tag out from t function. '#description' => t("<p>Add unique tracking IDs to each webform on the site. For each webform you want to track, enter the form's endpoint ID as it appears in the unique tracking code. It should be formatted like xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx (8-4-4-4-12).</p>"), It should be '#description' => '<p>' . t("Add unique tracking IDs to each webform on the site. For each webform you want to track, enter the form's endpoint ID as it appears in the unique tracking code. It should be formatted like xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx (8-4-4-4-12).") . '</p>', Similarly do the same at other places.
5. '#description' => t("The form's baseURI will appear in the tracking code as: <pre>__ss_noform.push(['baseURI', '<strong>https://app-XXXXXX.sharpspring.com/webforms/receivePostback/XXXXXXXX/</strong>']);</pre>"), Use placeholders in the t() function. See https://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/t/7 Please correct at other places too.

After fixing this I'll love to RTBC this :)

Review of the 7.x-1.x branch (commit 71c8b05):

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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. Change the project url in Readme.txt https://www.drupal.org/sandbox/mojiferous/2480987 when the project will be promoted.Just keep it as side note.
2. Specify about the #8.9 with its reason #9.9 in the issue summary.
3. sharpspring_webforms_form_alter : variable_get('sharpspring_domain') Specify the default value of the variable as well and pass it as second parameter.Code from sharpspring module variable_get('sharpspring_domain', 'koi-000000.sharpspring.com')This is not trustable, but i did not find any way to do some hack with this.

Otherwise looks good to me.
Assigning to @mpdonadio to give it a final look if he has time.

Automated Review

Review of the 7.x-1.x branch (commit 71c8b05):

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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

(*) sharpspring_webforms_config_form, line 60, is XSS vulnerable. $webform->title needs to be run through check_plain().

(*) sharpspring_webforms_config_form() is vulnerable to node access bypass. Line 19 just queries the {webform} table. This should really be
a db_select that joins the node table and then does and ->addTag('node_access'). This is mitigated by the fact that users
need to have the 'administer sharpspring' permission. Or, define your own permission and use the 'restrict access' option in the
hook_permission.

sharpspring_webforms_schema() should have FK entries for documentation.

So every time you add a webform, you need to goto the config to enable it? Think about a better long term solution.

drupal_add_js() is deprecated. Use #attached.

Use NODE_PUBLISHED instead of 1 when checking node.status.

This was a pretty quick look after finding the two security problems...

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

Oops I missed simple catch #14.1 and #14.2 :(
/me Very bad to review code in tonight

Automated Review

Review of the 7.x-1.x branch (commit 1b0ae08):

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

  
  FILE: /Users/matt/PAR/pareview_temp/sharpspring_webforms.admin.inc
  ---------------------------------------------------------------------------
  FOUND 1 ERROR AFFECTING 1 LINE
  ---------------------------------------------------------------------------
   19 | ERROR | [x] Comma not allowed after last value in single-line array
      |       |     declaration
  ---------------------------------------------------------------------------
  PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
  ---------------------------------------------------------------------------
  
  Time: 284ms; Memory: 6.75Mb
  

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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

The XSS is fixed.

(*) node_load_multiple() is not sufficient to protect against access bypass. You need to ->addTag('node_access') w/ db_select(). See https://www.phase2technology.com/drupal-7-node-access-grants-locks-and-k...

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

I read `git diff 1b0ae08` and am satisfied.

Thanks for your contribution, Mojiferous!

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.