Pushup Social is a technology platform that allows you add a social network to any existing website in a matter of seconds. This plugin automatically inserts the Pushup Social snippet into the header of your website.

How to use

  1. Activate the module through the ‘Modules’ menu in Drupal
  2. Click "I have my code" button
  3. Enter in a community ID (51ad08922dh2cb1d1e01f1db), and turn the community state to "On"
  4. Browse to your site and you will see the Pushup social bar on the bottom of your screen.

Project page
https://drupal.org/sandbox/pushupsocial/2278407

Git
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/PushupSocial/2278407.git pushup_social

Automatic Review
http://pareview.sh/pareview/httpgitdrupalorgsandboxPushupSocial2278407git

Intended Drupal core
7.x

Comments

PA robot’s picture

Status: Needs review » Needs work

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.

joetoday’s picture

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

ankitgarg’s picture

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.

PushupSocial’s picture

Module has been updated to reflect suggested changes. Thanks!

PushupSocial’s picture

Issue summary: View changes
Status: Needs work » Needs review
martin_klima’s picture

Status: Needs review » Needs work

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).

PushupSocial’s picture

Issue summary: View changes
Status: Needs work » Needs review

Thanks for reviewing. The GIT link has been updated and the administrative path has been changed to "admin/config/pushupsocial".

As for testing the functionality, please ensure that all external links (e.g. sign up) go to https://pushup.com and use the given community ID (51ad08922dh2cb1d1e01f1db) to test the administrative form.

Thanks!

Ayesh’s picture

Status: Needs review » Needs work

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.

PushupSocial’s picture

Status: Needs work » Needs review

@Ayesh, sorry for the late response.

The following updates were made:

  • drupal_add_js was removed. It's now using hook_page_build()
  • Moved splash into heredoc
  • Changed pushupsocial_admin_settings_form() to pushupsocial_admin_settings_form($form, $form_state)
  • Updated description. Copy is more informative now.

Thanks for your feedback!

Ayesh’s picture

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!

Ruslan P’s picture

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.

Ruslan P’s picture

Status: Needs review » Needs work
Ayesh’s picture

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.

Ruslan P’s picture

Status: Needs work » Needs review

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

PushupSocial’s picture

Issue summary: View changes

Ruslan & Ayesh,

Thanks for your feedback! Please keep it coming. The module has been updated with some of the recommended changes.

1. format_string
sprintf was changed to Drupal's format_string()

2. README
I've updated the readme installation copy to exclude 'pushup_social.zip'

3. pushupsocial.onoffswitch.js
Refactored and switched to Drupal.behaviors. If this is not the ideal use, please advise.

Thanks guys for your help!

Ordasoft’s picture

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

naveenvalecha’s picture

Issue summary: View changes
naveenvalecha’s picture

Issue summary: View changes

Updating issue summary.

naveenvalecha’s picture

Status: Needs review » Needs work
Issue tags: +PAReview: security

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!

PushupSocial’s picture

Hi @naveenvalecha,

Thanks for reviewing our module. The issues with pareview.sh have been resolved and we've added licensing information to our module. The security issue that you outlined -- and please correct me if I'm wrong -- doesn't apply here. $_GET[PUSHUPSOCIAL_PAGESLUG_CONFIG] is solely used as a flag. The script just checks if the flag is set via PHP's isset function. The a user can make it equal to whatever they want, it will not affect the module.

Cheers!

PushupSocial’s picture

Status: Needs work » Needs review
Ayesh’s picture

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.

naveenvalecha’s picture

Status: Needs review » Needs work

@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.

Ayesh’s picture

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.

naveenvalecha’s picture

Issue tags: -PAReview: security

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.

PushupSocial’s picture

@naveenvalecha & @Ayesh,

Thanks for your input! The module is now using drupal_get_query_parameters instead of $_GET. The Logo licensing information will be provided soon. Thanks again.

PushupSocial’s picture

Status: Needs work » Needs review

Licensing of the pushup logo has been provided inside the img directory.

Ayesh’s picture

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.

rpsu’s picture

Status: Needs review » Needs work

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.

PA robot’s picture

Status: Needs work » Closed (won't fix)

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.