This module integrates with rackspace webmail and provides a single sign-on for users.

It uses nusoap for Rackspace Webmail API integration.

It will create email account on rackspace webmail when new user register on Drupal site.

Example:--
if any user register with Drupal username: firstname.lastname
Rackspace webmail Inbox user will be: firstname.lastname@YOURSITENAME.COM

It will create a 'Mail information' tab on /user/* page, where you can see the total number of emails on your webmail and link to open your rackspace webmail.

Link to project (sandbox) page

https://www.drupal.org/sandbox/babusaheb.vikas/2508733

Git clone command

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/babusaheb.vikas/2508733.git rackspace_webmail_integration
cd rackspace_webmail_integration

Manual reviews of other projects:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

PA robot’s picture

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.

darol100’s picture

Status: Needs review » Reviewed & tested by the community

Automated Review

Pareview.sh - No errors, http://git.drupal.org/sandbox/babusaheb.vikas/2508733.git

Coder Review

rswebmail.module

  • Line 169: When labelling buttons, make it clear what the button does, "Submit" is too generic.
        '#value' => t('SUBMIT'),
  • Line 266: Potential problem: drupal_set_message() only accepts filtered text, be sure to use check_plain(), filter_xss() or similar to ensure your $variable is fully sanitized.
            drupal_set_message($num_messages_response);

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 assets/code
Yes: Follows the guidelines for 3rd party assets/code.
README.txt/README.md
Yes: Follows 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
Yes: Meets security requirements.
Coding style & Drupal API usage
List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:
  1. If this module dependency on the library nusoap.php, I think you should add the libraries_get_path. In addition, you should add Library module as a dependency in the .info file.
  2. You should provide the hook_help to allow site builders to find information about your module using Drupal UI.

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.

I did not see any blockers.

Ayesh’s picture

Status: Reviewed & tested by the community » Needs work

Darryl: Sorry about setting the application back to Needs Work. But this module has some issues that I believe should be fixed first.

Thanks for submitting this, Vikas. I installed the module, but I could not review well without purchasing the rackspace service. But I reviewed the code, and unfortunately there are few changes that needs done.

- This module does not integrate with the libraries module, which is fine, but the instructions are not clear on how to install the third party library. You are encouraged to use the libraries module, but the module should not use a common name to refer to files in the libraries folder. sites/all/libraries folder contains all other libraries. General practice is to have a folder named after the library name to store the files. "lib" is not a unique name. Also, the README.txt instructions could be more precise.

- Also, module uses require_once which throws a fatal error. Since the README.txt instructions were not precise, it's easier for anyone to experience this error, reducing the reliability of the module.

- The rswebmail_details function returns the last entered row in the database. Could you explain how it's supposed to work?

- There is a reference to a file in sites/default/files folder. Please provide all assets with the module. You can use drupal_get_path() to get the path to the module folder.

- The function name rswebmail_form is supposed to be a function for drupal_get_form(), but it also implements hook_form. Using a different unique name should fix any possible issues.

babusaheb.vikas’s picture

Thanks darol100 for your support and guidance.

I went through your points and fixed both Coder Review issues.

I have also fixed issues of Coding style & Drupal API usage
For this I have used libraries_get_path and added libraries module as a dependency in the .info file.

I have provided hook_help to allow site builders to find information about my module using Drupal UI.

babusaheb.vikas’s picture

Thanks Ayesh for your feedback and support. it is appreciated and I have fixed the issues.

I have added instruction about steps for install the third party library in README.txt file
which prevent anyone to experience this require_once error.

rswebmail_details function returns the last entered row in the database. === I have added a condition in rswebmail_create_form_submit() function.
if ($total_row == '0') then db_insert() will work otherwise db_update() will work
So, each time there will be only one row found in the database that will return using rswebmail_details().

I have used drupal_get_path() function and provide all assets with the module.

I have changed rswebmail_form name to rswebmail_create_form

babusaheb.vikas’s picture

Status: Needs work » Needs review
babusaheb.vikas’s picture

FileSize
14.47 KB
babusaheb.vikas’s picture

Issue summary: View changes
babusaheb.vikas’s picture

Issue tags: -drupal 7, -Needs Review, -task +PAreview: review bonus
BR0kEN’s picture

Status: Needs review » Needs work
Issue tags: -PAreview: review bonus
  1. No need to use quotes here.
  2. Correct hook comment is: Implements hook_HOOK().. So, change it here
  3. For what we need this blank lines?
  4. Better to define variable before use it.
  5. Wrong hook comment here.
  6. Why the hook_enable() needed here? You are not deleting the scheme in hook_uninstall(), and installation hook will be performed before enabling. So, your schema will be available.
  7. Incorrect hook comments throughout the file.
  8. The same, as in paragraph 4, here.
  9. Why you concatenate this variable? It is not used anywhere. The code should be like this:
    $form = drupal_get_form('rswebmail_create_form');
    
    return drupal_render($form);
    

Also, the comments for functions like this:

**
 * Used db_select.
 */

is very bad. Why we comment the code? We do this to help other developers work with our code. In your case, your comments only irritate me (IMHO).

Also, I cannot understand this loop. For what it needed?

You need to work a lot of time on this module before it will be published.

Good luck in your work!

babusaheb.vikas’s picture

Issue summary: View changes
babusaheb.vikas’s picture

Thanks BROkEN for contributing your valuable time to review my module.
I have made all the changes as pointed out by you.

babusaheb.vikas’s picture

Status: Needs work » Needs review
babusaheb.vikas’s picture

Issue summary: View changes
BR0kEN’s picture

Status: Needs review » Needs work
  1. Too many unnecessary variables here. You can just write the same in one line:
    return '<pre>' . file_get_contents(dirname(__FILE__) . '/README.txt') . '</pre>';
    
  2. Better to leave an empty line before return statement.
  3. Why we need an empty lines in this function?
  4. Do not use @ for preventing warning. It takes the memory.
  5.   $result = db_select('rswebmail_config', 'rs')
              ->fields('rs', array(
              'organization_name', 'host_name', 'reseller_username', 'reseller_password',
              ))
              ->execute();
    
      $record = $result->fetch();
    

    looks not so good.

    $record = db_select('rswebmail_config', 'rs')
      ->fields('rs', array(
        'organization_name',
        'host_name',
        'reseller_username',
        'reseller_password',
      ))
      ->execute()
      ->fetch();
    

    Also, I see the same code here. To follow the DRY principles we need to create additional function for this.

klausi’s picture

Status: Needs work » Needs review

That minor issues are surely not application blockers, anything else that you found or should this be RTBC instead?

babusaheb.vikas’s picture

Issue summary: View changes
Issue tags: +PAreview: review bonus

Thanks BROkEN for your support and guidance.
I have made all the changes as pointed out by you.

babusaheb.vikas’s picture

Issue summary: View changes
babusaheb.vikas’s picture

@klausi, I hope this module is ready for RTBC status.

mpdonadio’s picture

Assigned: Unassigned » mpdonadio

Reviewing next.

mpdonadio’s picture

Issue summary: View changes
mpdonadio’s picture

Assigned: mpdonadio » Unassigned
Status: Needs review » Needs work
Issue tags: +PAreview: security

Automated Review

Git errors:

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

  • 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 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

(*) rswebmail_block_content() is sending output from the webservice directly to the user with drupal_set_message(). One, this should
be part of a translated message. Two, the webservice is third-party data, and considered unsafe. Use t() with proper placeholders and some
context for the errors.

Coding style & Drupal API usage
A full database schema for the config looks like overkill for this. Simple variables seem like they would suffice.

Use user_is_anonymous() instead of checking against $user->uid == 0.

There are a decent number of Drupal Coding standards that didn't get picked up by the PAReview process. Double check them when you get a chance.

Don't concatenate translated strings. Use placeholders.

rswebmail_block_content() should build up a render array and not markup directly.

You require the Libraries module, but you aren't really using the library API. Take a look at the docs for how to implement this properly.

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.

The blocking issues (*) are the unsanitized output of third-party data.

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.

babusaheb.vikas’s picture

Status: Needs work » Needs review

Thanks mpdonadio for contributing your valuable time to review.

I have done the changes suggested by you:

1) Git default branch is set now.
2) rswebmail_block_content() is sending output from the webservice directly to the user with drupal_set_message(). this is now part of a translated message. I have used t() with proper placeholders and some context for the errors.
3) Added simple variables for database schema.
4) Used user_is_logged_in() instead of $user->uid != 0.
5) Added placeholders instead of concatenate translated strings.
6) Used render array instead of markup directly in rswebmail_block_content().
7) Used libraries_get_path() in correct way.

klausi’s picture

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

manual review:

  1. rswebmail_form_fetch(): that page callback is not necessary. You can directly use "drupal_get_form" as page callback in hook_menu(). See node_menu() for an example.
  2. rswebmail_create_form_validate(): the first argument to form_set_error() should be the name of the field, in your case "host_name".
  3. "drupal_set_message(t('@fault', array('@fault' => $client->fault)));": this does not look useful for translators. Maybe add a prefix like "Could not get the number of messages, error: @fault. Response: @new_messages."

But otherwise looks good to me.

Assigning to mpdonadio since he might have time to take a final look at this.

mpdonadio’s picture

Status: Reviewed & tested by the community » Needs work

Automated Review

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

  • 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

My earlier security concerns look addressed.

(*) I just noticed that the 'admin/config/services/rswebmail' path is only protected by the 'access Rswebmail' permission. This
means that all users who have access to the Rswebmail page/block can update the system settings and get the reseller username/password. If you add a new permission, say 'administer Rswebmail', and update the access on this path, I will approve the application.

Leaving myself assigned so it stays in my queue.

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.

babusaheb.vikas’s picture

Thanks klausi for your review and support.
I have made all the changes as pointed out by you.

mpdonadio’s picture

Status: Needs work » Needs review

I will look at this tonight when I am on my primary Drupal dev machine.

babusaheb.vikas’s picture

Hi mpdonadio,

Thanks for contributing your valuable time to review my module.

I have added a new permission 'administer Rswebmail' and update the access on this path.
Now, 'admin/config/services/rswebmail' path is protected by the 'administer Rswebmail' permission.

mpdonadio’s picture

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

Manual Review

Review of the 7.x-1.x branch (commit 2472e3e).

Read `git diff 30918a4`. My concern was fixed.

mpdonadio’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for your contribution, babusaheb.vikas !

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.

Status: Fixed » Closed (fixed)

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