Meetup Login module allows user to login to a Drupal 7 website using their Meetup.com login details. Once the account is integrated with meetup login details, users would be able to login using Meetup account details.

https://drupal.org/sandbox/sreyas/2086295

git clone --branch master sreyas@git.drupal.org:sandbox/sreyas/2086295.git meetup_login

Comments

Thank you for your contribution.

  • There is a coding errors.
  • Remove LICENSE.txt, it will be added by drupal.org packaging automatically.
  • Remove "version" from the ./meetup_login.info file, it will be added by drupal.org packaging automatically.
  • Remove "project" from the ./meetup_login.info file, it will be added by drupal.org packaging automatically.
  • Remove "datestamp" from the ./meetup_login.info file, it will be added by drupal.org packaging automatically.

See
http://pareview.sh/pareview/httpgitdrupalorgsandboxsreyas2086295git

Hi,

Please remove master branch and create new branch (like 7.x-1.x) then delete master branch also provide new link for git in issue summary.

Thanks,
Paritosh Gautam

Hi,

Please remove ventral comments find attached image displaying long list of comments.

Thanks,
Paritosh Gautam

Hi.
Attachment - the requested page could not be found.
Thanks,
Vitaliy Sytnik

Status:Needs review» Needs work

Hi sreyas,

Here's my review:

- Lot's of parereview errors, see drupal coding standards. http://pareview.sh/pareview/httpgitdrupalorgsandboxsreyas2086295git.
- There's a dependency of oauth_common, but this module has been has moved to it's new home at http://drupal.org/project/oauth.
- Would be nice to add (real) validation to OAuth Consumer key and OAuth Consumer secret fields.
- function meetup_account_settings: why are you creating all these variables, while on that point you don't even know if you even need them? I would say first check if condition "No Meetup accounts have been added yet" applies, if not start creating variables.
- Meetup provides api's for all languages. I would suggest to use the PHP api, using the libraries module: http://www.meetup.com/meetup_api/clients/
- function meetupapi_delete_confirm: you're defining $uid, but not using it for anything.
- meetup_login.module, line 303: $url_self = 'https://api.meetup.com/members.json/?relation=self'; shouldn't this be configurable?

Hope this helps.

Manual Review

1. In install file I didn't fount any install hook but found function meetup_login_schema which is never used.

2. In function meetup_account_settings() you coded like below. string should be enclodes in t().

<?php
if (!empty($rows)) {
 
$output = theme('table', array('header' => $header, 'rows' => $rows));
  return
$output;
}
else{
 
$output="No Meetup accounts have been added yet";
}
  return
$output;
?>

Replace with

<?php
if (!empty($rows)) {
  return
theme('table', array('header' => $header, 'rows' => $rows));
}
else{
  return
t('No Meetup accounts have been added yet');
}
?>

No $output variable needed.

3. All menu callback functions should be defined in different inc file.

4. The code below will accept all file which has .png in it's name

<?php
$results
= file_scan_directory($img_path, '/.png/');
?>

Should be replaced with
<?php
$results
= file_scan_directory($img_path, '/.*.png$/i');
?>

This will accept file ending with .png or .PNG

Hi Everyone..

Thank you for your valuable comments, will review the code again and make changes to it and update you the status shortly

Regards
Ciril

Hi

We have made full revamp of the drupal now. All the errors reported by coder module using drupal code sniffer has been corrected. also changed the login prompt to oauth popup.

Please every review the system now and let me know

Regards
Ciril

Status:Needs work» Needs review

Hi

We have made full revamp of the drupal now. All the errors reported by coder module using drupal code sniffer has been corrected. also changed the login prompt to oauth popup.

Please every review the system now and let me know

Regards
Ciril

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.

Hi,

1 - Exists a "function user_password($length = 10)" in user.module you can use instead of your "function function randompassword()", Anyway if you need to use a custom function It would be a good idea to add "_your_module" as a prefix.

2 - oauth_common dependency still exists. As beljaako says this module has been moved to https://drupal.org/project/oauth

3 - Files "meetup.inc" and "meetup.pages.inc" should be called "meetup_login.inc" and "meetup_login.pages.inc"

I am sorry I am sure there is a reason but I am not able to understand why are you calling "variable_get($user->name)" in "meetup_login.module" in line 359.

Hope this helps.

Regards.

Status:Needs review» Needs work

Status:Needs work» Needs review

Hi

1. We have updated the code to use randompassword
2. We have updated the code to use oauth module(but it seems for some drupal installation is looks for oauth_common)
3. We have renamed the files

This is kind of issue we faced with meetup. Actually meetup api just like twitter api wont return the email id to our drupal site. So once logged i user would need to set the email address at myaccount edit. Since there is no email password, user wont be able to use forgot password if they close the window. So we thought of this option. We saved the password inside variable table, And when a user logs in using the meetup_login and edit his account, if there is no email in his form this password from variable would come. so he can directly change password even without knowing the current password. Once the password is set that variable is cleared.

Let us know if you have other suggestions here for not showing password publicly, and also not able to send password information via mail:)

Regards
Ciril

Hi,
thanks for the explanation! I´ll take a look.

function randompassword still exists.

As pgautam says seems you are working con master branch. You should move on to a candidate realease branch:
https://drupal.org/node/1659588
https://drupal.org/node/1127732

You have the files with the new names but you still have the old files.

Have you considered to use Drupal behaviours in your .js files?

In meetup_login.module, line 273, you don´t need to apply check_plain function() to your string

Regards.

Status:Needs review» Needs work

Hi,

I manually reviewed your code and following are few of my suggestions.

1) you have included JS files in hook_init function using drupal_add_js(), instead you can add those in .info files. It is a better approach to having the JS file in .info file, if the JS is going to be there in all the pages.

  drupal_add_js(drupal_get_path('module', 'meetup_login') . '/js/meetup_login.js');
  drupal_add_js(drupal_get_path('module', 'meetup_login') . '/js/jquery.oauthpopup.js');

2) Did not validate the secret and api key if it is empty in config page while i'm clicking "sign in with meetup" image in login block. So please show the Sign in image after the validation.

3) Please create the 7.x-1.x branch and move all your files and delete the master branch. Please see https://drupal.org/node/1066342 for more information.

Thanks,
Ramesh

Status:Needs work» Needs review

Hi,

1. Have created new branch for this project.
2. Have removed the unnecessary files from repository
3. Have added validation to the keys.
4. Have moved javascript to info file.

Let me know if any further changes are required:)

Regards
Ciril

Status:Needs review» Needs work

Hi,
Please review your project with pareview.

http://pareview.sh/pareview/httpgitdrupalorgsandboxsreyas2086295git

It reflect number of Meetup Login Project's errors, mainly deals with Drupal Coding Standard. you must test your module with coder module itself on your local.

Kamlesh Patidar

Status:Needs work» Needs review

Got it completely reviewed by pareview and all the warning there has solved..Now it looks good..

Also new features added.
1. Field mapping from meetup to drupal
2. User picture taken from meetup

Regards
Sreyas

Issue summary:View changes
Status:Needs review» Needs work
Project page
Please take a moment to make your project page follow tips for a great project page.
PAReview: 3rd party code
jquery.oauthpopup.js and meetupapi.lib.php appear to be 3rd party code. 3rd party code is not generally allowed on Drupal.org and should be deleted. This policy is described in the getting involved handbook. It also appears in the terms and conditions you agreed to when you signed up for Git access, which you may want to re-read, to be sure you're not violating other terms.

The Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org.

  • Your commit messages are not useful :) You should briefly describe what was done in each commit.
  • All your variables should start with meetup_login_ so they are namespaced to your module.
  • meetup_login_update_7401() and meetup_login_update_7403() are empty - can this be deleted?
  • The fields added in meetup_login_update_7400() and meetup_login_update_7402() should also be added to hook_schema(). When the module is installed, it's expected to have the latest configuration and not require any updates to be run.

This is not a complete review, I'm just out of time for now!

----
Top Shelf Modules - Crafted, Curated, Contributed.

Status:Needs work» Needs review

Have updated the module to use external library, whose link is provided in the README.txt file. Could you please check this and let us know.

Regards
Ciril

If we put third party client library(Meetup php client) in hook_requirements it will be usefull to detect weather library is installed or not at any point of execution time .

more reference https://api.drupal.org/api/drupal/modules!system!system.api.php/function...

Status:Needs review» Needs work

Status:Needs work» Needs review

Hi Harshil,

We can confirm hook_requirements is already in the install file. When we did the pareview, it mentioned keep the requirements in .install file.

Regards
Ciril

Confirming hook_requirements is in the install file, however when enabling the module I get the following error:

Fatal error: Call to undefined function libraries_get_libraries() in drupal-7.25/sites/all/modules/meetup_login/meetup_login.install on line 146

I think this still needs a little work and testing.

Status:Needs review» Needs work

Hi sreyas,

Upon manual review,

- Use hook_page_build() instead of hook_init() which is recommended.

- Use system_settings_form($form) to create an admin form where variables will be set if the form elements names are same as variable names.

- Use drupal method user_password() instead of custom method meetup_login_randompassword() for generating random password which will allow to reduce the code.

- Use drupal_get_message() instead of unset($_SESSION['messages']); in which it will clear the messages.

- Regarding select empty option, use drupal features for empty options.

function meetup_login_user_properties() {
  $common = array(
    '_none' => t('Select'),

Refer these links - https://api.drupal.org/api/drupal/developer!topics!forms_api_reference.h..., https://api.drupal.org/api/drupal/developer!topics!forms_api_reference.h...

Hello Purushothaman

We have resolved this issue here. Actually the lirary module was required for installing our module. But it was not auto enabled. We have corrected the module by auto enabling Library module when our module is installed. Please do check this now and let me know.

Regards
Sreyas

Status:Needs work» Needs review

Status:Needs review» Needs work

Hi sreyas,

Upon manual review, I found the followings in meetup_login.module:

- Would be better to use user_is_logged_in() instead of your current test at line 160. It does pretty much the same thing, but is more standard.

- Duplicated @file tag at line 91.

- As mentionned in #25, you should be using system_settings_form function in your admin form meetup_login_admin_settings_form.

Also, please update the git clone command in this issue summary with the proper one:

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/sreyas/2086295.git meetup_login