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.
This module provides interface for admin to add email address if Admin want to
get mail notification when new comment has been posted using facebook comment
module.
Multiple email addresses can be added separated by comma.
https://www.drupal.org/sandbox/pragna/2580865
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/pragna/2580865.git fb_comment_notification
cd fb_comment_notification
I have solved all issues by testing from pareview.sh
http://pareview.sh/pareview/httpgitdrupalorgsandboxpragna2580865git
Comment | File | Size | Author |
---|---|---|---|
#14 | Screen Shot 2016-03-15 at 3.39.10 PM.png | 48.75 KB | shaktik |
Comments
Comment #2
pragna CreditAttribution: pragna commentedComment #3
PA robot CreditAttribution: PA robot commentedFixed the git clone URL in the issue summary for non-maintainer users.
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.
Comment #4
Helgi Andri Jónsson CreditAttribution: Helgi Andri Jónsson commentedHi Pragna,
Automated Review looks ok.
Manual Review
Basic Application Checks
[Yes] - Ensure that the application contains a repository and project page link.
[Yes] - Ensure it´s not a duplication.
[Yes] - Ensure the user does not have multiple applications.
Basic Repository Checks
[Yes] - Ensure the repository actually contains code.
[Yes] - Ensure the applicant is working in a version specific branch.
[Yes] - Ensure branches and tags are following naming conventions.
Licensing
[Yes] - Ensure the repository does not contain a ‘LICENSE.txt’ file.
[Yes] - Ensure the repository does not contain any 3rd party (non-GPL) code.
Documentation
[No] You have a detailed README.txt please make sure the project page contains at least the same information. - Ensure the project page contains detailed information.
[Yes] - Ensure the repository contains a detailed README.txt
[Yes] - Ensure the code contains a well-balanced amount of inline-comments
Coding Standards & Style
[Yes] - If not done yet run an automated review and ensure there are no major issues.
API Review & Security
[No] - Ensure the applicant is using Drupals API correctly.
One of your hook_menu items, send/fb-mail-notify, does not have a title. As this menu path is also used through an ajax post request bound to a facebook event in jQuery it should also be of type MENU_CALLBACK.
I am also concerned here that no access validation is performed on this path. Any site that has this module installed is vulnerable to fake post requests and DDOS attacks. Drupal has a token api to create tokens and verify tokens which you could check in a custom access callback with drupal_get_token() and drupal_valid_token(). The drupal token api however relies upon user sessions so any site behind a reverse proxy like varnish would fail to cache because of the sessions. But in theory you can take a look at drupal_get_token() and generate a token based on something other than the session_id() and use a custom validation function.
I am also concerned about this function:
I recommend you should at least use filter_xss() on $_POST['urlofpage'] and $_POST['comment_msg']. You could also add url validation to $_POST['urlofpage']. Not sure why you use drupal_set_message in this callback if it is access through an ajax call.
Thanks for your work!
Best Regards,
Helgi.
Comment #5
Helgi Andri Jónsson CreditAttribution: Helgi Andri Jónsson commentedHi pragna,
Also please add links to your manual reviews before tagging your application with the PAReview: review bonus.
Best Regards,
Helgi.
Comment #6
klausiyep, please add manual reviews that you did to the issue summary before adding the review bonus tag.
Comment #7
pragna CreditAttribution: pragna commentedThank you Helgi and Klausi.
I will solve all issues as soon as possible.
Comment #8
pragna CreditAttribution: pragna commentedHi Helgi,
I have fixed the issues and commited the code.
About "send/fb-mail-notify" callback menu, I have already passes access argument so there is no need to set extra validation.
Do you have any example of drupal_get_token() and drupal_valid_token() with menu?
Thank you,
Pragna
Comment #9
Helgi Andri Jónsson CreditAttribution: Helgi Andri Jónsson commentedHi pragna,
The token workflow was only a suggestion. I don't think it would be a blocking issue but I think you should at least create a custom permission by implementing hook_permission() to give the site administrator the option of allowing/restricting access.
Best Regards,
Helgi.
Comment #10
pragna CreditAttribution: pragna commentedComment #11
pragna CreditAttribution: pragna commentedComment #12
shinde_kashmira CreditAttribution: shinde_kashmira at Intelliswift commentedHi Pragna,
Thanks for contributing this module.
Automated as well as manual review look fine to me.
Comment #13
shaktikHi Pragna,
1. I reviewed your module, you need to add email validation for Facebook comments notification settings also attached screentshots.
http://cgit.drupalcode.org/sandbox-pragna-2580865/tree/fb_comment_notifi...
2. Also as an example,please upload the screenshot of your project page.
https://www.drupal.org/sandbox/pragna/2580865
Thanks
Shakti
Comment #14
shaktikComment #14
Comment #15
klausiThat validation and the screenshot for the project page are surely not application blockers, anything else that you found or should this be RTBC instead?
Comment #16
vikas_jain CreditAttribution: vikas_jain commentedHi Pragna,
Please Move your admin setting form functionality in .admin.inc.
Thanks.
Comment #17
vikas_jain CreditAttribution: vikas_jain commentedComment #18
klausiUsing an admin.inc file is optional and surely not an application blocker. Anything else that you found or should this be RTBC instead?
Comment #19
vikas_jain CreditAttribution: vikas_jain commentedThere should be also uninstall hook in which you will delete all of the variables (variable_get()) by variable_del($name).
Thanks.
Comment #20
heykarthikwithuComment #21
muthukumar sri CreditAttribution: muthukumar sri at Ameex-Drupal Geeks commentedHi,
Remove the variable fb_comment_notification_admin_email when your module has uninstall.
Refer:
https://api.drupal.org/api/drupal/modules!system!system.api.php/function...
Comment #22
hesnvabr CreditAttribution: hesnvabr commented1 warning that affects 1 line of code i.e unused variable $msg.
Comment #23
pragna CreditAttribution: pragna commentedHi Guys,
I will try to solve all bugs as soon as possible.
Thanks,
Pragna
Comment #24
pragna CreditAttribution: pragna commentedHi Guys,
I have added the install file to delete the variables and removed unused variable.
Please check it.
Thank you,
Pragna
Comment #25
ARUN AK CreditAttribution: ARUN AK as a volunteer and commentedHi pragna,
I did a manual review.
Currently module is triggering emails when the users comments through Facebook comments block. It is because you are adding/appending your js to the pages through Facebook comments block.
But the Facebook comments module is providing an extra comment form other than block. If user is commenting through that comment form, no emails will trigger.
You can keep your js in to separate file and can include it using hook_page_build(). Then mail will work for both Facebook comments block and Facebook comment form.
Thanks,
ARUN AK
Comment #26
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.
Comment #27
pragna CreditAttribution: pragna commentedComment #28
apadernoComment #29
pragna CreditAttribution: pragna commentedHi ARUN AK,
I have solved issue, now it is working on both form.
Thank you for your review.
Comment #30
3ssom CreditAttribution: 3ssom commentedHello pragna,
Reviewers here have done great work ,, and you followed that so I'll do a review to see if this is a RTBC.
Automated Review
found some error ,, not much @pareview.sh:
http://pareview.sh/pareview/httpgitdrupalorgsandboxpragna2580865git
Manual Review
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.
although I see no blockers here so I think this is a RTBC :)
Thank you
Comment #31
apadernoI will update 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!
Thank you, 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 go the dedicated reviewer(s) as well.
Comment #32
pragna CreditAttribution: pragna commentedHi,
I have solved all issues which are found on pareview.sh
Thank you,
Pragna