In the era of smartphone, generating leads via website should be easy. If someone visits your website from her mobile, she should be easily reach you with her/his preferred communication system such as Facebook messenger, AIM messenger, yahoo messenger, Skype chat, SMS, Call or email. So this small module helps you enable a block which shows list of service link which opens appropriate apps in her/his mobile to connect with you instantly.

This module provide a configurable block to show the Instant Messenger Icons. Currently It Includes.

Phone call
SMS
Facebook Messenger
whatsapp (only IOS)
Apple facetime (only IOS)
Skype call
Skype chat
Email
Yahoo Messenger
AIM

When the user clicks on particular Icons, It will open appropriate application. Example : Clicking Skype icon will open a Skype program in your desktop. Opens SMS composing app in mobile, pre-filled with your number and message you configured.

Project
http://drupal.org/sandbox/quardz/1513112
http://git.drupal.org/sandbox/quardz/1513112.git

Git
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/quardz/1513112.git reach_us

Automated Review
http://pareview.sh/pareview/httpgitdrupalorgsandboxquardz1513112git

Project Reviewed
https://www.drupal.org/node/1666230#comment-6180296

Files: 
CommentFileSizeAuthor
#16 reach us.png2.52 KBalokvermaei

Comments

patrickd’s picture

welcome,

Please read about the Project Application Workflow.

You are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.

Also remove the license file, when you create a release it'll be added automatically and all code hosted on d.o is under GPL anyway.

As installation and usage instructions are quite important for us to review, please take a moment to make your project page follow tips for a great project page. Make sure your README.txt follows the guidelines for in-project documentation.

while waiting for an in-depht review of your module you can start out fixing some coding style issues detected by automated tools:
http://ventral.org/pareview/httpgitdrupalorgsandboxquardz1513112git

We do really need more hands in the application queue and highly recommend to get a review bonus so we can come back to your application sooner.

regards

spike22’s picture

Can you describe a little what the difference is between your module and the module you mentioned (http://drupal.org/project/on_the_web), please?...currently I can't see any difference.

patrickd’s picture

His module provides widgets for instant messengers (skype, msn.. ) - on the web provides widgets for social communities (g+, facebook)

I think that's the difference

spike22’s picture

Ok then...I've made a little digging in the code, looks ok, just few things got my eyes picked on:
-reach_us.module : line 130 , 171 and 217...

otherwise looks ok...hooks used normally, functions are ok, nothing critical!
just a TIP, but it's fine anyway: u can store more things in one variable, you don't need one for each

klausi’s picture

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

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

quardz’s picture

Status: Closed (won't fix) » Active

I will fix this in 10 days and make the module live. Sorry for the delay.

sreynen’s picture

Status: Active » Needs work

This doesn't seem to have changed since it was moved to "needs work" status.

klausi’s picture

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

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

If you reopen this please keep in mind that we are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

quardz’s picture

Issue summary: View changes
Status: Closed (won't fix) » Needs review

Added more features, Cleared all the issues from pareview.sh, here the demo of this module.

gaurav.pahuja’s picture

Getting 404 on demo link.

gaurav.pahuja’s picture

Status: Needs review » Needs work

Initial Code review:

Info file has typo for dependency

name = Reach Us
description = Block provides easy way to reach you via multiple mobile and desktop messaging and communication platform.
core = 7.x

dependancies[] = block

Module file:

No need to pass $delta to block function. In-fact whole function can be removed by calling reach_us_output function directly inside hook_block_view.

/**
 * Implements hook_block_view().
 */
function reach_us_block_view($delta = 0) {
  $block = array(
    'subject' => t('Reach Us On'),
    'content' => reach_us_display_block($delta),
  );
  return $block;
}

/**
 * Block Display.
 *
 * @return string
 *    Content for block.
 */
function reach_us_display_block($delta) {
  // Get the appropriate name to use in links.
  $linktype = variable_get('reach_us_display', 'icon');
  return reach_us_output($linktype);
}
gaurav.pahuja’s picture

Also, can you please check for XSS attacks?

I tried adding script tag to all the fields, though it saved but it also appeared on block also. All HTML tags should be handled properly and striped at time of saving only.

Only local images are allowed.

Also, did you tried using system_settings_form instead of saving all variables manually.
You can have a look at the example at:

https://www.drupal.org/node/1111260

One other small feedback:

Inside reach_us_display_block function, you are retrieving $linktype. This variable will always have some value as you are taking default value also.

But again while passing this value to reach_us_output($linktype = 'icon') function, you are assuming some default value which is kind of useless.

/**
 * Block Display.
 *
 * @return string
 *    Content for block.
 */
function reach_us_display_block($delta) {
  // Get the appropriate name to use in links.
  $linktype = variable_get('reach_us_display', 'icon');
  return reach_us_output($linktype);
}
/**
 * Outputs the rendered HTML of list of contact services.
 *
 * @linktype
 * 	  get either clickable to be"icon" or "link".
 *
 * @return string as HTML
 *    Content for block.
 */
function reach_us_output($linktype = 'icon') {
  // Attach the CSS file.
  drupal_add_css(drupal_get_path('module', 'reach_us') . '/reachus.css');
  $visibility = 'reach-visibility-' . variable_get('reach_us_visibility', 'horizontal');
  $output = '<div class = "reachus-wrap reachus-' . $linktype . ' ' . $visibility . '">';
  $defaultvalue = variable_get('reach_us_services');
  *
  *
  *
  *
}
quardz’s picture

thank gaurav for taking time and review module. But i fixed allabove issues. I cant use system_settings_form because the services are saved as serialize array(as per inc file array).
Here the demo link
Thanks again.

quardz’s picture

quardz’s picture

Status: Needs work » Needs review
alokvermaei’s picture

FileSize
2.52 KB

Hi quardz,
This is a great module ,please have a look on the below mentioned points
1. After saving the configuration form there is no message for form saving(you can see system form setting message) .
2. Please add validation. there should be some validation in the form.
3. There should be configuration for changing the icons so that user can upload images.
4. There should be configuration for adding more services as well.
5. Block should not appear as a blank content on the page if there is no service selected.
6. I will recommend you to use drupal system form settings.

naveenvalecha’s picture

Issue summary: View changes
naveenvalecha’s picture

Status: Needs review » Needs work

Hi @quardz,
Thanks for the contribution.

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder.
http://pareview.sh/pareview/httpgitdrupalorgsandboxquardz1513112git

There is still a master branch, make sure to set the correct default branch: https://www.drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in https://www.drupal.org/node/1127732

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

Manual Review

Individual user account
Yes: Follows the guidelines for individual user accounts.
No duplication
No: Does not cause module duplication and fragmentation.
Master Branch
No: Follows the guidelines for master branch.
Licensing
Yes: Follows the licensing requirements
3rd party code
Yes: Follows the guidelines for 3rd party code.
README.txt/README.md
No: Update the Read me file of the module.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/No. If "no", list security issues identified.
Coding style & Drupal API usage
  1. (*)reach_us.module : reach_us_menu is not registereing the menu. Declare $items as array before registering a menu.Add this $items = array();after this linefunction reach_us_menu() { will fix this.
  2. Remove the extra lines excluding line endings from reach_us.info
  3. reach_us.inc : Move the function reach_us_services() into reach_us.module file because it is including in the file and used in the module file and there is no extra function in the .inc file so remove that file.We keep it seperate so that the extra code will not compiled on the page load and we manage the code better.But in this module we are keeping only single function in the inc file and calling it in every file(.install and .module)
  4. Make a seperate file reach_us.admin.inc and move the admin page callback function reach_us_form and its submit function reach_us_form_submit defination into it.Because these functions are only needed at the admin callback page.After that add the file parameter in the hook_menu 'file' => 'reach_us.admin.inc'
  5. function reach_us_output($linktype) : Use the template to print the html by passing the variables so that it can be overrided in theme.

This review uses the Project Application Review Template.

As I am not a git administrator, so I would recommend you, should fix above issues first and then 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 :-)

Project page https://www.drupal.org/sandbox/quardz/1513112
Update the project page as read me and move the link 'more documentation here' into the documentation textbox by editing the project page so that it will display in the right sidebar and also then similarly move the 'Demo here' link in the 'Demo' text field under Resources section.Also update the installation,configuration text as readme file.

Thanks Again!

quardz’s picture

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

Thanks alokvermaei and naveenvalecha for your feedback

@naveenvalecha
Fixed the points 1,2,5,6. Point 3 can be changed using CSS, and point 4 can be added through hook, but more services are planned in next version

@naveenvalecha
Fixed all your recommendations, but keep the reach_us.inc since now it holds all the services and validations, just needs to edit this file if i want to add more services in future,

Thanks

samir_mankar’s picture

Category: Task » Bug report
Status: Needs review » Needs work

Automated Review

No issues found .

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

No: Does not cause module duplication and fragmentation.

Master Branch

No: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

No: Read me file needs changes as per 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.

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

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

sites/all/modules/reach_us/reach_us.module
reach_us.module

    severity: normalreview: style_string_spacingLine 160: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms [style_string_spacing]

        $varid = 'reach_us_serv_' . $key  . '_id';

sites/all/modules/reach_us/reach_us.admin.inc
reach_us.admin.inc

    severity: criticalreview: security_form_set_errorLine 164: Potential problem: form_set_error() and form_error() only accept filtered text, be sure to use check_plain(), filter_xss() or similar to ensure your $variable is fully sanitized. (Drupal Docs) [security_form_set_error]

            form_set_error($varid, $idvalid);

    severity: criticalreview: security_form_set_errorLine 169: Potential problem: form_set_error() and form_error() only accept filtered text, be sure to use check_plain(), filter_xss() or similar to ensure your $variable is fully sanitized. (Drupal Docs) [security_form_set_error]

              form_set_error($varlinktext, $errormsg);

quardz’s picture

Status: Needs work » Needs review

Thanks samir, fixed those pending issues, updated better readme file. I think almost ok now. waiting to get published as project

sendinblue’s picture

Status: Needs review » Fixed

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder. Please don't copy/paste all of the results unless they are short. If there are a lot, then post a link to the automated review and mention that problems should be addressed.

Manual Review

Individual user account
Yes: Follows the guidelines for individual user accounts.
No duplication
No .
Master Branch
Yes
Licensing
Yes/dd>
3rd party code
No.
README.txt/README.md
Yes.
Code long/complex enough for review
Yes.
Secure code
Yes.
Coding style & Drupal API usage

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.

klausi’s picture

Category: Bug report » Task
Status: Fixed » Needs review

quarz is not approved yet, so this is not fixed? See the workflow: https://www.drupal.org/node/532400

sendinblue’s picture

Status: Needs review » Reviewed & tested by the community

I think this module works correctly.

quardz’s picture

Thanks for the review, any possibilities of publishing this module sooner

heddn’s picture

Manual Review

3rd party assets/code
* No: Does not follow the guidelines for 3rd party assets/code. The images that are included are copyrighted.

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.

heddn’s picture

Status: Reviewed & tested by the community » Needs work
heddn’s picture

Issue tags: +PAReview: security

Automated review

FILE: ...sites/all/modules/reach_us/reach_us_content.tpl.php
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
2 | ERROR | Missing file doc comment
--------------------------------------------------------------------------------

FILE: ...sites/all/modules/reach_us/reach_us.module
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
152 | ERROR | Return comment indentation must be 2 additional spaces
194 | ERROR | Return comment must be on the next line
--------------------------------------------------------------------------------

Manual Review

Secure code
No: Does not meet the security requirements.
  1. (*) Values from reach_us_message variable are not sanitized before being printed in the theme layer. Viable options for resolving include a preprocess function to sanitize the text before displaying. It is preferable not to use sanitization calls directly in .tpl
Coding style & Drupal API usage
  1. .css file: property 'filter' is overwritten:
    .reachus-img:hover {
      opacity: 0.8;
      -webkit-filter: grayscale(100%);
      -moz-filter: grayscale(100%);
      filter: gray;
      filter: grayscale(100%);
    }
  2. admin.inc: spelling of default
      $form['settings']['reach_us_message'] = array(
        '#type' => 'textfield',
        '#title' => t('Detault message.'),
        '#default_value' => variable_get('reach_us_message', ''),
        '#description' => t('For some apps, Example FB messenger or SMS, its good to send just some short message, so you can back to them.'),
      );
  3. api.php: spelling of required & optional. * Make sure you added rquired css to you css. * Optinal but recommended, create a validate function for your service.

    also issues with spelling on line 19: components; 20: classes; 25: override; 26 & 47: description; 26: label; 36: skip; 41: numeric; 43: appear; default: 45; modifiable: 45;

    and a lot more.

  4. .module: Spelling of words in hook_help. easiliy Espacally
  5. admin.inc: Calling variable_get multiple times is inefficient (there's a couple examples of this). Assign it to a variable so it only needs to be called once.
    '#default_value' => trim(variable_get($varid)) ? variable_get($varid) : '',
  6. (*) .inc: None of the text is translated using t().
  7. .module: This is not necessary. Because you mention the file and path in the hook_menu.
    module_load_include('inc', 'reach_us', 'reach_us.admin');
  8. .module: Don't call variable_get multiple times on the same variable. Assign it to a variable if you need to call it more than once.

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.

quardz’s picture

Thanks Hedding, this review helps me understand more deeper and got more love of drupal community, i will fix the ASAP.

quardz’s picture

Status: Needs work » Needs review

Added the GPL licensed images from http://genericons.com and created few icons of my own, and available as GPL, and fixed all the issues pointed out. Thanks everyone to teach me good drupal coder.

heddn’s picture

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

A couple things I missed on my first pass, or that arose after your updates.

  1. admin.inc:
    else {
            if (!trim($form_state['values'][$varlinktext])) {
              $errormsg = filter_xss('Fill the Link text for ' . $services[$key]['label']);
              form_set_error($varlinktext, $errormsg);
            }
          }

    This should use t() on the "Fill..." and use argument replacement instead of filter_xss.

  2. admin.inc: It isn't important, but it is typical to end the page callback like this:
      $form['#theme'] = 'reach_us_form';
      return system_settings_form($form);
    
  3. .install: I think the module_load_include can be added to the hook_uninstall.
  4. .module: Please don't use markup in calls to t(). If you want to emphasize something, use %variable. If you need markup, then it should be inserted as argument replacements or concatenated to the string. If you don't, it makes l10n/i18n harder.
    $output .= t('By enable the <strong>Reach us</strong> block in admin page.');

However, none of these are release blockers. So I'm marking as RTBC. Assigning to mpdonadio to give this a final review, if he has time.

quardz’s picture

Thanks Hedding again, i fixed them too.

mpdonadio’s picture

Assigned: mpdonadio » Unassigned
Status: Reviewed & tested by the community » Needs work

Automated Review

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

  • Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
    
    FILE: /home/matt/PAR/pareview_temp/reach_us.module
    --------------------------------------------------------------------------------
    FOUND 4 ERRORS AFFECTING 4 LINES
    --------------------------------------------------------------------------------
     142 | ERROR | [x] Spaces must be used to indent lines; tabs are not allowed
     145 | ERROR | [x] Spaces must be used to indent lines; tabs are not allowed
     148 | ERROR | [x] Spaces must be used to indent lines; tabs are not allowed
     192 | ERROR | [x] Spaces must be used to indent lines; tabs are not allowed
    --------------------------------------------------------------------------------
    PHPCBF CAN FIX THE 4 MARKED SNIFF VIOLATIONS AUTOMATICALLY
    --------------------------------------------------------------------------------
    
    Time: 678ms; Memory: 8.25Mb
    
  • 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
Yes: Follows the guidelines for master branch.
Licensing
Yes: 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/or the README Template.
Code long/complex enough for review
Yes: Follows the guidelines for project length and complexity.
Secure code

(*) In _reach_us_content(), variable_get($varlinktext); is coming from user input and needs to be sanitized. It also looks the same for $display, $visibility, $iconsize.

Coding style & Drupal API usage
ALl of your variable_get() should have default values.

In reach_us_form(), why the filter_xss() on the description? This is hardcoded in the reach_us_services. Ah, because it is hookable.

(*) Since the services are hookable, they may be coming from user input, which means that you need to sanitize the label in reach_us_form() with check_plain. It is not directly exploitable, otherwise this would be a security issue.

(+) reach_us_form_validate is filtering values on input. This is not the Drupal way; you filter on output. See https://www.drupal.org/node/28984 and https://www.drupal.org/node/263002 It looks like you nust need to use t() with placeholders.

(+) module_load_include() should not be used the top level in the install file. See https://api.drupal.org/api/drupal/includes!module.inc/function/module_lo...

The print example at the bottom of the .api.php file is weird. Normally, you would just render array w/ a theme function.

Drupal has valid_email_address; no need for the PHP versions.

In reach_us_block_view, build up a render array instead of calling theme() directly.

#attached on the render array is preferred over drupal_add_css() for adding CSS.

reach_us_output() should really return a render array.

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.

quardz’s picture

Status: Needs work » Needs review

Thanks Matthew for that deep review, fixed them all.

heddn’s picture

Status: Needs review » Fixed

There are a few examples where the render array was converted to #markup render array. It could be done better; the render arrays should be formatted:

$render = array(
    '#theme' => 'reach_us_theme',
    '#example_variable' => $example_variable,
    '#prefix' => $prefix,
    '#suffix' => $suffix,
  );

That's not a release blocker...

Thanks for your contribution, quardz!

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.

quardz’s picture

Thanks Hedding, feeling awesome to be part of drupal community, thanks for all who take their time to make this project live now.

Status: Fixed » Closed (fixed)

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