[D7] Meetup Login

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

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

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

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.

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.

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

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

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.

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

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.

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

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

Good for me. Thank you!

Manu

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

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.

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.