Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comments
Comment #1
Vik CreditAttribution: Vik commentedThank 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
Comment #2
pgautam CreditAttribution: pgautam commentedHi,
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
Comment #3
pgautam CreditAttribution: pgautam commentedHi,
Please remove ventral comments find attached image displaying long list of comments.
Thanks,
Paritosh Gautam
Comment #4
Vik CreditAttribution: Vik commentedHi.
Attachment - the requested page could not be found.
Thanks,
Vitaliy Sytnik
Comment #5
beljaako CreditAttribution: beljaako commentedHi 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.
Comment #6
bappa.sarkar CreditAttribution: bappa.sarkar commentedManual 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().
Replace with
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
Should be replaced with
This will accept file ending with .png or .PNG
Comment #7
sreyas CreditAttribution: sreyas commentedHi 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
Comment #8
sreyas CreditAttribution: sreyas commentedHi
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
Comment #9
sreyas CreditAttribution: sreyas commentedHi
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
Comment #10
PA robot CreditAttribution: PA robot commentedWe 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.
Comment #11
Enxebre CreditAttribution: Enxebre commentedHi,
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.
Comment #12
Enxebre CreditAttribution: Enxebre commentedComment #13
sreyas CreditAttribution: sreyas commentedHi
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
Comment #14
Enxebre CreditAttribution: Enxebre commentedHi,
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.
Comment #15
rameshrasaiyan CreditAttribution: rameshrasaiyan commentedHi,
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.
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
Comment #16
sreyas CreditAttribution: sreyas commentedHi,
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
Comment #17
kamleshpatidar CreditAttribution: kamleshpatidar commentedHi,
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
Comment #18
sreyas CreditAttribution: sreyas commentedGot 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
Comment #19
kscheirerThe Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org.
meetup_login_
so they are namespaced to your module.This is not a complete review, I'm just out of time for now!
----
Top Shelf Modules - Crafted, Curated, Contributed.
Comment #20
sreyas CreditAttribution: sreyas commentedHave 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
Comment #21
harshil.maradiya CreditAttribution: harshil.maradiya commentedIf 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...
Comment #22
harshil.maradiya CreditAttribution: harshil.maradiya commentedComment #23
sreyas CreditAttribution: sreyas commentedHi 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
Comment #24
welly CreditAttribution: welly commentedConfirming 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.
Comment #25
Purushothaman Chinnadurai - Drupal Geeks CreditAttribution: Purushothaman Chinnadurai - Drupal Geeks commentedHi 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.
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...
Comment #26
sreyas CreditAttribution: sreyas commentedHello 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
Comment #27
sreyas CreditAttribution: sreyas commentedComment #28
manumilou CreditAttribution: manumilou commentedHi 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:
Comment #29
sreyas CreditAttribution: sreyas commentedComment #30
sreyas CreditAttribution: sreyas commentedWe have fixed these reported issues on this module now. Looking for any further feedback here.
Regards
Sreyas
Comment #31
manumilou CreditAttribution: manumilou commentedGood for me. Thank you!
Manu
Comment #32
sreyas CreditAttribution: sreyas commentedHi 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
Comment #33
sreyas CreditAttribution: sreyas commentedComment #34
pingwin4egHello @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.
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).
Comment #35
sreyas CreditAttribution: sreyas commentedSorry 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.
Comment #36
Rishi KulshreshthaAutomated 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
.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/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.
Comment #37
PA robot CreditAttribution: PA robot commentedClosing 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.