[D7] Social network user detection

Hi,

Please add git link corresponding to non-maintainer in the issue-summary like "git clone http://git.drupal.org/sandbox/Mschuddings/1786552.git social_network_user_detection".

Manual review of your module:

1. In .module file you have used variable social_network_user_detection_hyves_app_id which haven't uninstalled.

2. I think access argument access administration pages is not appropriate here.

review bonus ?

Hi,

I look like a very nice project that probably will help many drupal users.

The automatic test looks just fine

Please notice several issues that may consider to take care of:
1. There is an uninstall hook, but no install hook
2. social_network_user_detection_init(), usually I recommended to make a return if fails in check, and only then perform the task (+ you could avoid the local variable)
3. social_network_user_detection_page_alter(), I would first perform a check and return and then the code itself:
if (!isset(variable_get('googleanalytics_account', NULL))) {
drupal_set_message(t('Please configure Google Analytics and this module if required.'));
watchdog('social_network_user_detection', 'Please set an GoogleAnalytics ID');
return;
}
...
4. _social_network_user_detection_javascript_reporting_functionality(). I did not understand why it done on server side, and not embedded in the static javascript that you already serve

Best Regards,
Moshe

For me is ok.

Interesting module indeed. I looked through the code.

Automated testing:
Codesniffer comes up blank. Good job.

Manual testing:
Code looks solid, no appearant flaws, good use of comments.
There is alot of javascript embedded in the code. I would seem appropriate to put this in one or more js files, however I do not consider this required (It's not _that_ much.. )
README file lacks structure. Look at the examples here http://drupal.org/node/447604 for better structure. Information looks ok though.

Fix the README and I'll be happy to approve.

Multiple Applications

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

Project 1: http://drupal.org/node/1561062
Project 2: http://drupal.org/node/1793098

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.

in social_network_user_detection.info Line 6:
dependencies[] = googleanalytics
Should be:
dependencies[] = google_analytics

Found it while trying to enable your module using drush.
*EDIT* Just realized that the project page and the module name don't match... Let me look into it for a min
*EDIT* You may want to add a line in your readme that mentions if using drush the auto download of dependencies will not work due to the problem listed above.

Hi @Mschudders,

Automatic review:
Code standard is ok according to Coder Module.

Manual review:
It is a amazing module and worked great here, but something not much bad happened here.

I tryied to install the module and the following message were displayed duplicated: "Please configure Google Analytics and this module if required.". I know this message display because i need to configure my GA ID, but i couldn't find out why it displayed duplicated. I attached a picture.

I think you could also implement the hook_help explaining how to use your module. I got some confused trying to understand how to use it and what was the next step to configure the facebook app id, then i realized that there were instructions inside README.txt. Maybe you could also add this description inside the hook_help.

Well, it is just a idea. You did a good job.

Removing review bonus tag, you have not done any manual review, you just posted the output of an automated review tool. Make sure to read through the source code of the other projects.

http://git.drupal.org/sandbox/Mschuddings/1786552.git

manual reivew:

1. _social_network_user_detection_javascript_reporting_functionality(): Javscript should be put into dedicated *.js files.
2. Same in _social_network_user_detection_social_network_testing()
3. _social_network_user_detection_social_network_testing(): this is vulnerable to XSS exploits. If I enter foo', status : true, cookie : true, xfbml : true}); alert('XSS'); FB.init({ appId : 'foo as facebook app ID I get a nasty javascript popup. You need to sanitize user provided input before printing. Please read http://drupal.org/node/28984 again. And a even better solution: use Drupal.settings to pass down variables from PHP to Javscript where they will be transferred securely, see http://drupal.org/node/756722

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

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

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

• Bad line endings were found, always use unix style terminators. See http://drupal.org/coding-standards#indenting
  

  ./social_network_user_detection.module:    PHP script, ASCII text, with CRLF, LF line terminators
  social_network_user_detection.module
  

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

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.

Although you should definitively fix those issues I think they are not hard blockers, so I guess this RTBC. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Thanks for your contribution, Michael!

I updated your account to let you 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 get 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.