[D7] Pushup Social

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

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.

Pareview is throwing a bunch of errors that you should probably check out, but a few highlights I see are:

Capitalize your readme.txt so it reads README.txt

A bunch of the lines go over the 80 character limit defined by drupal coding standards.

Generally Drupal tries to put markup in a tpl.php file instead of something like pushupsocial.splash.inc.

Not sure why "Drupal Module: Google Analytics" on line 5 of pushupsocial.module

Line 80 of pushupsocial.admin.inc, the function should be re-named to follow module naming conventions.

Probably no bueno adding javascript with the form API #suffix element on line 56 of pushupsocial.admin.inc, probably want to use the #attached FAPI property

the link to the git source of your project in form of:
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/pushupsocial/2278407.git pushupsocial
and the pareview link:
http://pareview.sh/pareview/httpgitdrupalorgsandboxpushupsocial2278407git
There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch.

Hi, thanks for your work. At first, please correct GIT link in description.
You have: git.drupal.org:sandbox/PushupSocial/2278407.git (bad URL)
Should be: http://git.drupal.org/sandbox/PushupSocial/2278407.git

Functionality:
I installed module and it works as described. I signed up to the community. Link in the confirmation e-mail does nothing, so I could not test more.

Administration:
In my opinion, configuration path (admin/settings/pushupsocial) clould be nested in the Configuration menu (f.e. admin/config/pushupsocial).

Sorry I didn't try the module in a test site yet, and I'm just looking through the code.

In your pushupsocial.module file, you load all the variables and run the drupal_add_js code in the global scope (instead of using a Drupal hook). It might work, but running arbitrary code in the global scope is not a good idea. Depending on certain caching configurations, bootstrapping modes, Ajax calls, and such situations, this code can lead to some problems.
Consider including this in a hook_page_build hook and attaching the JavaScript to the render-able array instead of calling drupal_add_js().

You are not offering a way to alter the output without hacking your module. The previous reviewer mentioned tpl.php files. Usually that refers to utilizing Drupal theme API to pass variables to the template file and get the output. Basically it's the same idea as you have implemented, but that is used almost everywhere in Drupal and most of the users are aware of that convention. Since that splash page is not really for end users and no need of customizing, you could simply embed it in a heredoc and include in the admin page (use #markup property).

Function signature in pushupsocial_admin_settings_form() has to be pushupsocial_admin_settings_form($form, $form_state). You get an empty array as the first argument.

Also a feedback in general: The description fields are too advertising IMHO. They should at least explain that it integrates the service with Drupal site by adding the necessary assets in their sites.

Thanks! Everything looks fine for me now. Let's wait for some other community reviews and/or a git admin to pick this up. Try to take a review bonus.

On a side note, format_string is the Drupal version of sprintf, but used widely in Drupal code. It's basically the translation-less version of t(). There is nothing wrong with using sprintf() at all, but format_string uses the same modifiers you have used several hundred times in t().
Good luck!

Hello PushupSocial .

This is my manual code review.

1. In description of this issue.

Git
http://git.drupal.org/sandbox/PushupSocial/2278407.git

Not Found.
Please replace to "git clone git://git.drupal.org/sandbox/PushupSocial/2278407.git"

2. README.txt
Upload `pushup_social.zip`
This is confusing. The filename will be different (with version numbers etc).

Instead, you can write like that
Install as usual, see https://www.drupal.org/node/895232 for further information.

3. SASS and CSS.
Can you add config.rb? Maybe should put these files in separate directories? When I downloaded the module my phpstorm automatically recompiled the css file.

4. pushupsocial.onoffswitch.js

  var $onoffSwitch = $(".onoffswitch-label");
  var $onoffCheckbox = $("#edit-pushupsocialenabled")

I would write as:

  var $onOffSwitch = $(".onoffswitch-label"),
    $onOffCheckbox = $("#edit-pushupsocialenabled");

Also can you use Drupal.behaviors?

5. UI
I'm not sure what to do design for the admin interface. Maybe it adds work for custom theme. This is purely my opinion.

Hi Ruslan, I'm not the author of this module, but with all due respect, I think we can simply suggest the changes to the module author, but we should should not mark this as "Needs Work" because of minor (?) changes that does not affect module functionality in terms of performance, security, etc.

Ayesh, I probably agree with you.
Sorry if I wrote too much.

Thank you for your work! Your module is nice and useful.
But I think it's worth to implement hook_block_view().

Updating issue summary.

Hi @PushupSocial,
First Thanks for your contribution.

Automated Review

Please fix the best practice issues identified by pareview.sh http://pareview.sh/pareview/httpgitdrupalorgsandboxPushupSocial2278407git

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

May be: Please confirm about the licensing of the pushup logo.I did not find it anywhere.Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

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

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes. If "no", list security issues identified.

1. In pushupsocial.admin.inc $_GET[PUSHUPSOCIAL_PAGESLUG_CONFIG] using 'PUSHUPSOCIAL_PAGESLUG_CONFIG' directly in a URL. This is user input, so it is possible for a user with the 'administer pushup social' to inject something naughty on the page. You need to check_plain() this.Yes the use of the $_GET is here not seems a security issue but it may be.The use of drupal_get_query_parameters is more preferable than getting the prarameter from $_GET. Thanks to @ayesh as suggested.
   
   Removed the pareview:security tag.

Coding style & Drupal API usage

Rest of the changes seemed good.

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.

As I am not a git administrator, so I would recommend you, please help to review other project applications to get a review bonus. This will put you on the high priority list, then git administrators will take a look at your project right away :-)

Thanks Again!

I can also confirm this is NOT really a security issue.
In the admin.inc file, the splash page is shown if the flag is set in the URL, configuration form otherwise. I don't think it will make any security problem.

@PushupSocial
Thanks for the updates. Please use the drupal_get_query_parameters instead of the directly getting the parameter via $_GET
would you please confirm about the licensing of the pushup logo inside the img directory as mentioned in the #19
Rest seems good to me. +1 for RTBC.
@Ayesh
It may be but this may overcome the XSS exploit with the user having 'administer pushup social' permissions.

I'm sorry I don't understand what difference it would make when you use drupal_get_query_parameters over $_GET. That function uses $_GET array anyway, so it wouldn't make a difference.

The form already has access restrictions enforced from hook_menu level. This $_GET value is only to switch the page contents but access permissions are required nonetheless.
I'm sorry if I'm missing anything, but as far as I can see, this $_GET use in particular is not a security issue.

Yes the use of the $_GET is here not seems a security issue but it may be.The use of drupal_get_query_parameters is more preferable than getting the prarameter from $_GET.
Removed the pareview:security tag.

I think ALL code and assets in the repo must be GPL. Even if the code is in perfect standards, I think your application won't be approved due to licensing issues.
See https://www.drupal.org/licensing/faq#q2
Logos probably an exception but I'm posting the link just FYI.

Thank you for your contribution! This module is fairly simple but serves clearly a spesific purpose. There are some issues, especially with the PushUp images license.

Automated Review

No best practice issues identified by pareview.sh & drupalcs & coder.

Manual Review

Individual user account

No: Does not follow the guidelines for individual user accounts. It looks like this is a "company account" instead of an individual account.

No duplication

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

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

No: Does not follow the licensing requirements. Project contains images with a "All rights reserved" -copyright notice (img/LICENSE), which isn't allowed - see Licensing FAQ #7. Maybe you could simple replace images with links to the PushUp.com images?

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. However project page would benefit of explaining Pushup Social a bit more - looking at the project page it is not clear what is it for and what kind of site could benefit from using it.

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. (*) Licensing issue (images contained in module) is needs to be solved
2. (+) You should add hook-information to docblocks: pushupsocial_page_build() and pushupsocial_menu() are using Drupal hooks, that should be documented like this:
   

   /**
    * Implements hook_page_build().
    *
    * Inject Pushup code into site header.
    */

3. It would be better to use l() i pushupsocial_splash() instead of adding paths inside <a>-tag using format_string()-function.

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.

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.