This module adds www.reactor.am tracking and communication service to your drupal website. Reactor.am is a tool for marketing automation, crm and user analytics.

Module code is based on the segmentio module.

Clone command

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/martin.smid/2434189.git reactor_am
cd reactor_am

Project url
https://www.drupal.org/sandbox/martin.smid/2434189

Automated Review

Manual reviews of other projects

https://www.drupal.org/node/2363081#comment-9668751

Comments

smido’s picture

Issue summary: View changes
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/httpgitdrupalorgsandboxmartinsmid2434189git

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.

smido’s picture

Issue summary: View changes
smido’s picture

Issue summary: View changes
smido’s picture

Automated review
http://pareview.sh/pareview/httpgitdrupalorgsandboxmartinsmid2434189git-...

It seems to try to apply coding style to the markdown file. Also, i cannot remove master branch from sandbox project.
No other problems detected

smido’s picture

Status: Needs work » Needs review
smido’s picture

Issue summary: View changes
Issue tags: +PAReview: review bonus
mpdonadio’s picture

Issue summary: View changes
mpdonadio’s picture

Assigned: Unassigned » mpdonadio

@smido, these two links should help you to fix the master and default branches [#1659588] and [#1127732].

The README should be wrapped to 80 cols.

Assigning to myself for a proper review, which will be in the next day or two.

mpdonadio’s picture

Assigned: mpdonadio » naveenvalecha
Issue tags: +PAReview: security

Automated Review

Git errors:

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

  • Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
    
    FILE: /Users/mattd/Dropbox/Drupal/par/pareview_temp/README.md
    ----------------------------------------------------------------------
    FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 3 LINES
    ----------------------------------------------------------------------
      3 | WARNING | Line exceeds 80 characters; contains 117 characters
      9 | WARNING | Line exceeds 80 characters; contains 96 characters
     12 | WARNING | Line exceeds 80 characters; contains 116 characters
    ----------------------------------------------------------------------
    
    Time: 157ms; Memory: 5.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

Individual user account
Yes: Follows the guidelines for individual user accounts.
No duplication
Yes: Does not cause module duplication and/or fragmentation.
Master Branch
No: Does not follow the guidelines for master branch. You have a 7.x-1.x branch, but still have master. You need to remove it.
Licensing
[Yes: Follows / No: Does not follow] the licensing requirements.
3rd party assets/code
Yes: Follows the guidelines for 3rd party assets/code.
README.txt/README.md
No: Does not follow the guidelines for in-project documentation and/or the README Template. This is a really lean README. Is there anything else you can add?
Code long/complex enough for review
Yes: Follows the guidelines for project length and complexity.
Secure code

This module has a few security problems, one of which is directly exploitable. As part of Review Admin mentoring, I am assigning to naveenvalecha to find them, in particular the direct exploit.

Coding style & Drupal API usage

What is release.sh for? I don't think you need this in the repo?

You don't need to set a variable in the install. Just pass the default to variable_get().

In reactor_admin_settings_form(), wrapping single fields with collapsible kinda seems like overkill.

Your non-hooks need proper doclocks, eg reactor_get_picture_url().

In reactor_page_alter(), use user_is_anonymous() instead of checking the uid.

User tracking isn't an emergency; log config problems as errors.

Using #attached is preferred over drupal_add_js().

I get a consistent warning

    Notice: Undefined variable: profile_url in reactor_get_picture_url() (line 47 of /home/s04358c6ec93c294/www/sites/default/modules/2434189/reactor.module).
    

You should have a hook_help()

You should have a hook_requirements() to check the config.

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.

smido’s picture

Thank for your review !

  • the master branch has been successfully deleted
  • README was cut to 80 characters per line, enriched and formated according to template
  • What is release.sh for? I don't think you need this in the repo?
    Removed.
  • You don't need to set a variable in the install. Just pass the default to variable_get().
    Removed.
  • In reactor_admin_settings_form(), wrapping single fields with collapsible kinda seems like overkill.
    Overkill removed ;)
  • Your non-hooks need proper doclocks, eg reactor_get_picture_url().
    Added simple description.
  • In reactor_page_alter(), use user_is_anonymous() instead of checking the uid.
    Used.
  • User tracking isn't an emergency; log config problems as errors.
    Loglevel changed.
  • Using #attached is preferred over drupal_add_js().
    I dont know how to use this.
  • I get a consistent warning

    Notice: Undefined variable: profile_url in reactor_get_picture_url() (line 47 of /home/s04358c6ec93c294/www/sites/default/modules/2434189/reactor.module).

    Fixed

  • You should have a hook_help()
    Added
  • You should have a hook_requirements() to check the config.
    Added
naveenvalecha’s picture

Assigned: naveenvalecha » Unassigned
Issue summary: View changes
Status: Needs review » Needs work

First,Thanks for your contribution.

Automated Review

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

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.


FILE: ...s/naveeen/Documents/pareviewsh/pareview_temp/inc/reactor.admin.inc
---------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 2 WARNING(S) AFFECTING 2 LINE(S)
---------------------------------------------------------------------------
 14 | WARNING | A comma should follow the last multiline array item.
    |         | Found: )
 30 | WARNING | A comma should follow the last multiline array item.
    |         | Found: )
---------------------------------------------------------------------------
UPGRADE TO PHP_CODESNIFFER 2.0 TO FIX ERRORS AUTOMATICALLY
---------------------------------------------------------------------------


FILE: /Users/naveeen/Documents/pareviewsh/pareview_temp/reactor.module
---------------------------------------------------------------------------
FOUND 3 ERROR(S) AFFECTING 3 LINE(S)
---------------------------------------------------------------------------
 136 | ERROR | Whitespace found at end of line
 148 | ERROR | Whitespace found at end of line
 150 | ERROR | Files must end in a single new line character
---------------------------------------------------------------------------
UPGRADE TO PHP_CODESNIFFER 2.0 TO FIX ERRORS AUTOMATICALLY
---------------------------------------------------------------------------


FILE: /Users/naveeen/Documents/pareviewsh/pareview_temp/README.md
---------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
---------------------------------------------------------------------------
 19 | WARNING | Line exceeds 80 characters; contains 111 characters
---------------------------------------------------------------------------
UPGRADE TO PHP_CODESNIFFER 2.0 TO FIX ERRORS AUTOMATICALLY
---------------------------------------------------------------------------

Manual Review : Read 10c19a0d7be3...

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
No: Meets the security requirements. / No: List of security issues identified.]
  • reactor_page_alter :
          $script .= "
            _rcr('collect', {
              user_id: '" . $user->uid . "', 
              username_s: '" . $user->name . "',
              email_s: '" . $user->mail . "',
              profile_picture_s: '" . reactor_get_picture_url($user->picture) . "',
              registration_date_d: " . $user->created . ",
              active_b: " . $user->status . ",
            });
          ";

    You should sanitize the data using proper sanitization functions before passing to drupal_add_js .http://drupal.stackexchange.com/questions/10280/is-check-plain-enough

Coding style & Drupal API usage
  1. (*) Notice: Undefined variable: output in reactor_help() (line 149 of /Applications/MAMP/htdocs/d7.dev/sites/all/modules/sandbox/reactor_am/reactor.module).
  2. Project page needs some improvements.
  3. reactor_requirements : The hook_requirements is not used in the module.So remove it.
  4. reactor_page_alter : You are adding js file using drupal_add_js rather use #attached to add the JS file
    with page rendered array.
  5. reactor_page_alter : you are adding js on every page.Please add a comment.I would suggest for long term create a seperate file.
  6. reactor_page_alter :
      // Do not collect anonymous users.
      if (user_is_anonymous()) {
        return;
      }

    Please specify on the project page regarding this that the module is not collecting data for anonymous users.

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.

mpdonadio’s picture

This is what I had:

Secure code

The admin form is XSS prone. If I use

  ', alert('XSS'), '
  

as the app ID, I get a popup. I think you need to make this a proper behavior, and add the app ID as a setting.

You are also using values from the $user object directly. I could not directly exploit these, because of validation, but these should also be passed via settings, which are escaped properly by core when added to a page.

smido’s picture

Status: Needs work » Needs review
  1. You should sanitize the data using proper sanitization functions before passing to drupal_add_js
    Output sanitized
  2. (*) Notice: Undefined variable: output in reactor_help() (line 149 of /Applications/MAMP/htdocs/d7.dev/sites/all/modules/sandbox/reactor_am/reactor.module).
    Fixed
  3. Project page needs some improvements.
    Improved
  4. reactor_requirements : The hook_requirements is not used in the module.So remove it.
    Removed
  5. reactor_page_alter : You are adding js file using drupal_add_js rather use #attached to add the JS file

    with page rendered array.
    Fixed
  6. reactor_page_alter : you are adding js on every page.Please add a comment.I would suggest for long term create a seperate file.
    reactor_page_alter :

    &nbsp; // Do not collect anonymous users.<br>&nbsp; if (user_is_anonymous()) {<br>&nbsp;&nbsp;&nbsp; return;<br>&nbsp; }

    Please specify on the project page regarding this that the module is not collecting data for anonymous users.

    Comment added.

klausi’s picture

Issue summary: View changes
Issue tags: -PAReview: review bonus

Removing review bonus tag, you have not done all manual reviews. Make sure to read through the source code of the other projects, as requested on the review bonus page.

naveenvalecha’s picture

Assigned: Unassigned » heddn
Status: Needs review » Reviewed & tested by the community

Manual Review : Read 47bd61...
The Git commits are not connected to your user account. You need to specify an email address. See https://www.drupal.org/node/1022156 andhttps://www.drupal.org/node/1051722

  1. reactor.admin.inc : reactor_admin_settings_form : Wrong doc comment.This is not a hook.
  2. reactor_page_alter : Please specify on the project page that the module not track anonymous users.For long term make it configurable at admin part and choose the admin to track which roles.
  3. reactor_page_alter : No need to sanitize uid,email,picture,created and status Remove the check_plain from them.check_plain($user->name)Use format_username that will also work later when module will enable the tracking for anonymous users.
  4. If reactorid will be numeric always then you can restrict it to numeric at the admin configuration as well.Then there will be no need to sanitize this variable as well.

Assigning to heddn for final review if he has time.

smido’s picture

Hi, thanks for your review. Here are my comments

* Git config set
* Doc comment changed.
* The comment was already on the project page.
* Sanitizing was requested. How do I know who to listen to ? This is already second request to change something back. Hmm now I am looking that it was you who requested it :) Fixed.
* reactor_app_id is not numeric

naveenvalecha’s picture

Status: Reviewed & tested by the community » Fixed

Manual Review : Read 82a55b...
Your git commits are still not connected to your user account. You need to specify an email address. See https://www.drupal.org/node/1022156 and https://www.drupal.org/node/1051722 if you have changed the email address then file an issue in the webmasters issue queue to update your commits specified with that email address.
No Objections from more than 1 week.
No blockers.
Thanks for your contribution, @smido!

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:

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.

naveenvalecha’s picture

Assigned: heddn » Unassigned
smido’s picture

@naveenvalecha Thank you. I will try to fix the git issues.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.