[D7] webClinicPro

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: https://www.drupal.org/node/2673664

Project 2: https://www.drupal.org/node/2582131

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

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

Fixed the git clone URL in the issue summary for non-maintainer users.

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.

Automated Review

There is some coding standard errors to fix : http://pareview.sh/pareview/httpgitdrupalorgsandboxe-mailit2667034git

Individual user account
Yes

Duplication
No

Master Branch
Yes. Branch 7.x-1.x

Licensing
Yes. Follow

3rd party assets/code
Yes follow. Do not contains any 3dr party.

README.txt/README.md
Yes :

Code long/complex enough for review
Yes. Well documented. Extending views sort, fields and argument.

Secure code
None security issue found. Looks good.

Coding style & Drupal API usage
After a manuel Review, everything seems to be ok. Perhaps, just some recommandations :
1. (*) In webclinicpro.module file function webclinicpro_flush_caches() has if condition with else. if no use of else (line 205) ,should be removed.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

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.

@jdrichmond
some of suggestions from manual review of your module

1. You have set some of variables in variables table Ex: webclinicpro_block_seal, webclinicpro_forced_ssl, webclinicpro_subscriber_key.. So you need to remove those variables while module Un install. By implementing hook_uninstall().

2. You need to pass default value for variable_get() while it enters first time in your function webclinicpro_admin_settings() on line 35 and 55. By default variable get have the value NULL, in your module
Refer : https://api.drupal.org/api/drupal/includes!bootstrap.inc/function/variab...

3. Line 56 $options should have key and values.
Refer : https://www.drupal.org/node/325766

4. In function webclinicpro_admin_settings_submit() you have used Curl function. Need to use curl time out function may remote server down your application wont affect its failure.

curl_setopt($ch, CURLOPT_CONNECTTIMEOUT , TIME_IN_SECOND); // For connection establish.
curl_setopt($ch, CURLOPT_TIMEOUT, TIME_IN_SECOND); // For fetch data.

5. In webclinicpro_block_info(), can use description it help to additional block details.

6. Line 138 hook_block_info() implemented but hook_block_view() and block render data not used.

Hi
I found the following issues/suggestion in your module.

Issues
1. Variable used in the application like webclinicpro_subscriber_key not getting deleted from the variable table when we uninstall the module. You can create a .install file in your module and can use variable_del() where required.
2. When I tried to set the webClinic Pro Subscriber KEY I got a warning message saying "Notice: Undefined index: webclinicpro_block_seal in webclinicpro_admin_settings_submit() (line 102 of /var/www/iclick_test7/drupal/sites/all/modules/custom/2581971/webclinicpro.module).
3. In your .module file Line 130 : you can add an entry to watchdog, same on Line 206.

Suggestion
1. Please use hook_help in your .module file, and you can add the admin configuration link from there.

Hi jdrichmond,

you should consider to move the admin settings form to a separate webclinicpro.admin.inc file. Please delete your variables used as settings storage in your hook_uninstall() (that you have already implemented). You should print the content of your README.txt in your hook_help().

Your webclinicpro.install must begin with <?php to be php executable

Best regards

Hi Jdrichmond

In .install file for variable delete have you also read the comments on Proper way to bulk-delete variables on module uninstall? You should not delete variables directly from db.

You should do something like this:

global $conf;
$module_string = "meet_our_team_";
$vars = array_intersect_key($conf, array_flip(preg_grep("/^{$module_string}.*/", array_keys($conf))));
foreach (array_keys($vars) as $var) {
  variable_del($var);
}

Hi
Please don't create a package just for your module in your info file
Thanks
Nitin

Hi jdrichmond

Manual Review:

I reviewed your module, following thing have need to fixed.

1. Please add configure link to the module in .info file.
2. remove .css file from .info file and only add it where it needed.
3. if user submit blank setting form it do not show any error, Please add validation to the setting .
Thanks
Nitin

@NitinSP: why should check_url() not be used? It is perfectly fine as sanitization function when you output a URL? Or did you see a special situation here where it should not be used?

There is some coding standard errors to fix :

EDIT: removed long pareview.sh dump.

Source: http://pareview.sh/ - PAReview.sh online service

@jdrichmond Please ignore previous comment.

Please wrap user facing things in t() not the key :

Ex :

$options = array(
      t('None') => 'None',
      t('1') => 'Style #1 - Black Text with Transparent Background',
      t('2') => 'Style #2 - White Text with Transparent Background',
    );

To

$options = array(
      'None' => t('None'),
      '1' => t('Style #1 - Black Text with Transparent Background'),
      '2' => t('Style #2 - White Text with Transparent Background'),
    );

Also fix the issue reported on https://pareview.sh/node/90

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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

1. Please remove logo, unless it will be licensed GPL 2+
2. webclinicpro.module#n73 don't use target="_blank" for URL's (UX and in some cases a security risk)

https://www.drupal.org/project/securepages does the same job.

We don't close applications as duplicates of existing projects. Duplication is not an application stopper.

Automated Review

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

• Your README.txt does not follow best practices (headings need to be uppercase). See https://www.drupal.org/node/2181737 .
  • The INTRODUCTION section is missing.
  • The REQUIREMENTS section is missing.
  • The INSTALLATION section is missing.
  • The CONFIGURATION section is missing.
• 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.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

No: Causes module duplication and/or fragmentation. https://www.drupal.org/project/securepages

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

No: Does not follow the guidelines for 3rd party assets/code. 3rd party logo included

README.txt/README.md

No: Does not follow the guidelines for in-project documentation and/or the README Template. See pareview review

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage

1. (*) your commits are listed as Unattributed/Michael Redman not jdrichmond. Did you copy the repository or are your git settings incorrect? Please follow the instruction in your drupal.org profile -> Git access

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

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.

If you are still working on this application, you should fix all known problems and set the status to Needs review. (See also the project application workflow.)
Please don't change status of this application if you aren't sure you have time to dedicate to this application, or it will be closed again as won't fix.

I am closing this application due to lack of activity.

I also see that many commits are done from Michael Redman, not the user who applied. In this case, this project cannot be use for this application.