[D7] Twilio OTP

Git clone failed for http://git.drupal.org/sandbox/rajatdkte/2826044.git while invoking http://pareview.sh/pareview/httpgitdrupalorgsandboxrajatdkte2826044git

Git clone failed. Aborting.

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.

Please review your code on "https://pareview.sh" site. We found some issue in your code. Here I have attached the link for your error code.

https://pareview.sh/node/141

Likewise, please provide the "git clone URL" in your issue queue.

Fixed the git clone URL in the issue summary for non-maintainer users.

I'm a robot and this is an automated message from Project Applications Scraper.

I have tested this module and it's working in my site... great module.

Hi,
I tested the module.

On Line 68 of the file twilio_otp.module:
if ($_SESSION['twilio_otp'] != $twilio_otp) {

This will generate an undefined index notice because $_SESSION['twilio_otp'] isn't defined/initialized at this point.

More:
1. "Send OTP" form element should use a "#limit_validation_errors" property so that clicking on it doesn't validate the whole form but mobile number.
2. There should be a way to let users know whether the OTP was sent successfully or it failed after clicking on "Send OTP". You can dynamically add a message to the form in your ajax callback.

Hi kawal,

Please recheck issue resolved.

The more functionality we will add after some time.

Hi kawal,

I got some free time today, So added more changes.

Automated Review

Git errors:

The last commit message is just one word, you should provide a meaningful short summary what you changed. See https://www.drupal.org/node/52287
Review of the 7.x-1.x branch (commit e4640d3):

No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.
This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

Manual Review

1. In the function "twilio_otp_user_insert" after inserting the otp in db the SESSION variable should be unset. And the query should be made with the db to validate the OTP.
2. Provide the hook to change the message content before sending at function twilio_otp_send_otp_callback line number 60 on twillio_otp.module file.
3. Watchdog logging of error on twilio_otp_send_otp_callback in case the otp was not send or any error.

@vaibhavdev: looks like you forgot to change the status. Is this now RTBC after your review or are there application blockers left and this should be set to "needs work"?

Marking the project as needs work as per the comments of https://www.drupal.org/node/2830269#comment-11855989

Hi vaibhavdev ,

I had made the changes.

Please do not assign ticket yourself. See the workflow https://www.drupal.org/node/532400

Hi Rajat,

Moving this to RTBC. This looks good, the code is fine, and it works as desired.
I believe, README should contain some information about this module. However, it is not mandatory though.

Hi,

I have done three reviews:
https://www.drupal.org/node/2843486#comment-11896040
https://www.drupal.org/node/2844305#comment-11896065
https://www.drupal.org/node/2786217#comment-11896103

manual review:

1. README.txt is empty, please fill it out. See also https://www.drupal.org/node/2181737
2. project page: is the OTP code needed for every login or just for the registration? Is this similar to the 2FA module https://www.drupal.org/project/tfa or not? Please expand the project page, see also https://www.drupal.org/node/997024
3. twilio_otp_form_alter(): if you are only altering one specific form you should use hook_form_FORM_ID_alter() instead. See https://api.drupal.org/api/drupal/modules!system!system.api.php/function...
4. twilio_otp_user_insert(): this will throw PHP warnings if a user account is inserted programmatically and no data is set in $_SESSION. Make sure to check for those first.

Otherwise looks god to me.

Thanks for your contribution, Rajat!

I updated 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:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, 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 to the dedicated reviewer(s) as well.

Hi klausi,

Thanks for review,
I had made changes as you told.
It is not similar to 2FA module as it will provide field on user registration not on user login page.