Navigation Menu

Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Google reCaptcha feature #13011

Merged
merged 1 commit into from Feb 20, 2017
Merged

Conversation

ShreyasSinha
Copy link
Contributor

Signed-off-by: Shreyas Sinha shreyas.sinha14@gmail.com

Solves issue #12983

Before submitting pull request, please check that every commit:

  • Has proper Signed-Off-By
  • Has commit message which describes it
  • Is needed on it's own, if you have just minor fixes to previous commits, you can squash them
  • Any new functionality is covered by tests

@ShreyasSinha ShreyasSinha force-pushed the google_captcha branch 2 times, most recently from bd3461b to 47b51c3 Compare February 17, 2017 19:53
@codecov-io
Copy link

codecov-io commented Feb 17, 2017

Codecov Report

Merging #13011 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #13011      +/-   ##
==========================================
+ Coverage   54.25%   54.25%   +<.01%     
==========================================
  Files         466      466              
  Lines       69644    69645       +1     
==========================================
+ Hits        37783    37784       +1     
  Misses      31861    31861

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff38179...c433b57. Read the comment docs.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, This looks good too.

Copy link
Member

@ibennetch ibennetch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I tried this without reCAPTCHA enabled or configured, the Go button is disabled and I am unable to log in.

It does seem to work as expected when the Captcha is enabled, so nice work on that.

js/functions.js Outdated
/*
* Function to enable the 'Go' button on login.
*/
function captcha_enable() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this implies enabling the captcha itself, not the "Go" button.

Perhaps a better function name would be login_go_button_enable() or something else more descriptive.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that will be better.

js/functions.js Outdated
/*
* Function to disable the 'Go' button on login.
*/
function captcha_disable() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise, this function name is also a bit misleading.

js/functions.js Outdated
@@ -5022,6 +5022,28 @@ function toggleDatepickerIfInvalid($td, $input_field) {
}
}

/*
* Function to enable the 'Go' button on login.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please align the asterisks by indenting by one space, like

/*
 * Function...
 */

That way it matches the style of the rest of the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@ghost
Copy link

ghost commented Feb 18, 2017

Sadly, I am not a programmer. But do have still plenty ideas about new functions on phpMyAdmin.
Maybe @ShreyasSinha could finish the job.

@ibennetch
Copy link
Member

@edwarddekker Your input is valuable here. Thanks for taking an interest in improving the project.

To be clear in case I caused confusion, my feedback was specifically meant about the code that ShreyasSinha has proposed. You can feel free to keep making suggestions :)

Signed-off-by: Shreyas Sinha <shreyas.sinha14@gmail.com>
@ShreyasSinha
Copy link
Contributor Author

@ibennetch I think this is ready for review.

@nijel nijel self-assigned this Feb 20, 2017
@nijel nijel merged commit c433b57 into phpmyadmin:master Feb 20, 2017
nijel added a commit that referenced this pull request Feb 20, 2017
- use camel case for function names
- move initialization to existing on load block

Signed-off-by: Michal Čihař <michal@cihar.com>
nijel added a commit that referenced this pull request Feb 20, 2017
Signed-off-by: Michal Čihař <michal@cihar.com>
@nijel
Copy link
Contributor

nijel commented Feb 20, 2017

Merged with coding style cleanup in 1f819d8.

@nijel nijel added this to the 4.8.0 milestone Feb 20, 2017
@ibennetch
Copy link
Member

Thanks @ShreyasSinha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants