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 7.x-1.x http://git.drupal.org/sandbox/sreyas/2086295.git meetup_login
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Vik’s picture

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

pgautam’s picture

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

pgautam’s picture

Hi,

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

Thanks,
Paritosh Gautam

Vik’s picture

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

beljaako’s picture

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.

bappa.sarkar’s picture

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

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

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

$results = file_scan_directory($img_path, '/.png/');

Should be replaced with

$results = file_scan_directory($img_path, '/.*.png$/i');

This will accept file ending with .png or .PNG

sreyas’s picture

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

sreyas’s picture

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

sreyas’s picture

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

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.

Enxebre’s picture

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.

Enxebre’s picture

Status: Needs review » Needs work
sreyas’s picture

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

Enxebre’s picture

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.

rameshrasaiyan’s picture

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

sreyas’s picture

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

kamleshpatidar’s picture

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

sreyas’s picture

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

kscheirer’s picture

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.

sreyas’s picture

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

harshil.maradiya’s picture

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

harshil.maradiya’s picture

Status: Needs review » Needs work
sreyas’s picture

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

welly’s picture

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.

Purushothaman Chinnadurai - Drupal Geeks’s picture

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

sreyas’s picture

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

sreyas’s picture

Status: Needs work » Needs review
manumilou’s picture

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
sreyas’s picture

Issue summary: View changes
sreyas’s picture

Status: Needs work » Needs review

We have fixed these reported issues on this module now. Looking for any further feedback here.

Regards
Sreyas

manumilou’s picture

Good for me. Thank you!

Manu

sreyas’s picture

Hi Manu,

Waiting for team further feedback if there is any

How can I promote this module to full project if every thing looks good?

Regards
Sreyas

sreyas’s picture

Status: Needs review » Reviewed & tested by the community
pingwin4eg’s picture

Status: Reviewed & tested by the community » Needs work

Hello @sreyas

There are still some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxsreyas2086295git

Though below is the only release blocker, you should also take care of other coding style issues.

FILE: /var/www/drupal-7-pareview/pareview_temp/meetup_login.install
-------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-------------------------------------------------------------------------------
69 | WARNING | Messages are user facing text and must run through t() for
| | translation
-------------------------------------------------------------------------------

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

P.S.: Don't RTBC yourself. 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).

sreyas’s picture

Status: Needs work » Needs review

Sorry I missed out the change.. Have updated the same now and pareview looks good.

Earlier also pareview was all good so i think some policy were updated later.

Let me know if there is anything more.

Rishi Kulshreshtha’s picture

Status: Needs review » Needs work

Automated Review

Best practice issues identified by pareview.sh. There are still issues with your code, please go through this and try to get a solution for them.

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
Yes: Follows the licensing requirements
3rd party code
Yes: Follows the guidelines for 3rd party code.
README.txt/README.md
No: Does not follows the guidelines for in-project documentation and the README Template. I have found that your README.txt is not following the README Template, please go through and mentioned link that will surely help you to bring out a well detailed README.txt file.
Code long/complex enough for review
Yes: Follows the guidelines for project length and complexity.
Secure code
Yes.
Coding style & Drupal API usage
  1. In your .install file, can you please explain on line no 177. Why are you asking users to download a specific but OLD branch https://github.com/blobaugh/Meetup-API-client-for-PHP/tree/804a5e9b0b82b... though there is a new branch available there? https://github.com/blobaugh/Meetup-API-client-for-PHP/
  2. Also can you please specify is it really necesaary to use the $_GET & $_SESSION kind of variables?
  3. On line no 124 in your pages.inc file why you have used this? $row[] = $dellink . '          ' . $unlink; it doesn't seems to be a good practice.

Also I've found that your project page seems to be incomplete, please go through each and every steps mentioned on this page https://www.drupal.org/node/1011698 as its provide all the information necessary to understand your module accordingly.

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.