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

CPAN Pull Request Challenge #1

Open
wants to merge 13 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@pauloscustodio

pauloscustodio commented May 26, 2015

In the context of the CPAN Pull Request Challenge, I've been assigned your module for review.
See http://perlmaven.com/2015-cpan-pull-request-challenge

I have done all the changes in a separate branch (cpan_prc), and each on a separate commit, so that you are able to easily select the corrections you wish to apply.

Summary of the fixes:
Fix #39864: Allow selection of number of characters/numbers
Fix #79948: Broken Captcha Image on UI if generated code matches code in db
Fix #93798 - pod encoding
Fix #104697: Spelling in README
Fix #104698: Add standard .gitignore for Perl modules
Fix #104718: Tests leave temporary directory behind

The last three are new, submitted by myself during the review.

I left #7224 open, as the fix is not trivial and the need for it not clear.

Below you can find the complete git log, that you can copy-paste into the resolution description while closing the RT tickets.

commit 483167e
Author: pauloscustodio pauloscustodio@gmail.com
Date: Tue May 26 23:20:21 2015 +0100

Updated README with latest POD

commit f715d18
Author: pauloscustodio pauloscustodio@gmail.com
Date: Tue May 26 23:09:04 2015 +0100

Fix #39864: Allow selection of number of characters/numbers

Done as suggested: added a numbers_percentage field, that can hold 0..100, initialized by default with 25.
Added pod documentation and tests.

commit 571d2af
Author: pauloscustodio pauloscustodio@gmail.com
Date: Tue May 26 22:31:35 2015 +0100

Fix #79948: Broken Captcha Image on UI if generated code matches code in db

Reproduced the code from the supplied patch (not directly, patch was for a different Captcha.pm version).
Added tests to certify the bug does not occur.

There is still a problem that cannot be solved with this implementation:
In the very remote case that two different codes generate the same random md5 token, then only the code generated
will give a positive match with check_code().
The older code with the same md5 is overwritten in the database.
To the user it will appear as if she has written the code incorrectly.

This failure is more acceptable than the previous "image not found" one.

commit 1e5c246
Author: pauloscustodio pauloscustodio@gmail.com
Date: Tue May 26 21:23:54 2015 +0100

Fix #104718: Tests leave temporary directory behind

Remove the temporary directory created at the start of the test script at the end of it.

commit d106e8a
Author: pauloscustodio pauloscustodio@gmail.com
Date: Mon May 25 23:24:49 2015 +0100

Formatting of the Changes file.

commit 8c1239a
Author: pauloscustodio pauloscustodio@gmail.com
Date: Mon May 25 22:34:00 2015 +0100

Fix #104698: Add standard .gitignore for Perl modules

commit 47638f5
Author: pauloscustodio pauloscustodio@gmail.com
Date: Mon May 25 22:50:27 2015 +0100

Fix #93798 - pod encoding

Done as suggested.

commit 3840a5d
Author: Lubomir Rintel lubo.rintel@gooddata.com
Date: Tue Aug 14 12:19:56 2012 +0200

Fix #104697: Spelling in README

Spelling error in the README file (Turning instead of Turing) fixed.

lkundrak and others added some commits Aug 14, 2012

Fix #104697: Spelling in README
Spelling error in the README file (Turning instead of Turing) fixed.
Fix #93798 - pod encoding
Done as suggested.
Fix #104718: Tests leave temporary directory behind
Remove the temporary directory created at the start of the test script at the end of it.
Fix #79948: Broken Captcha Image on UI if generated code matches code…
… in db

Reproduced the code from the supplied patch (not directly, patch was for a different Captcha.pm version).
Added tests to certify the bug does not occur.

There is still a problem that cannot be solved with this implementation:
In the very remote case that two different codes generate the same random md5 token, then only the code generated
will give a positive match with check_code().
The older code with the same md5 is overwritten in the database.
To the user it will appear as if she has written the code incorrectly.

This failure is more acceptable than the previous "image not found" one.
Fix #39864: Allow selection of number of characters/numbers
Done as suggested: added a numbers_percentage field, that can hold 0..100, initialized by default with 25.
Added pod documentation and tests.
@pauloscustodio

This comment has been minimized.

Show comment
Hide comment
@pauloscustodio

pauloscustodio May 26, 2015

Updated pull request with update of the Changes file

pauloscustodio commented May 26, 2015

Updated pull request with update of the Changes file

pauloscustodio added some commits May 26, 2015

Fix #39864 (2): Allow selection of number of characters/numbers
Added test for numbers_percentage() getters and setters
Use Test::TempDir::Tiny
to create temporary directories for tests that are deleted automatically when tests pass.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment