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

Simple captca for signup page #4659

Merged
merged 2 commits into from Jan 6, 2014

Conversation

jaywink
Copy link
Contributor

@jaywink jaywink commented Jan 2, 2014

Adopted pull request by @qmaruf (#4626) - this is to replace that to finish captcha functionality. I kept the original commits but squashed them into one. Also fixed a few minor things like indentation in config example, reverted .rvmrc change and locked the captcha gem version in dependencies.

Will add tests asap to finish this.

@@ -2,70 +2,77 @@
<div class="container">

<div class="row">
<div class="span4" id="image-container">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of the changes here are indentation fixes by @qmaruf - it is the official correct indentation unlike what it was before, but imho it's not a good idea to bundle random indentation fixes into a feature pull - opinions? :)

Copy link
Member

Choose a reason for hiding this comment

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

I agree but editing those out now just increases the noise, so I'd say leave them in for now.

@jaywink
Copy link
Contributor Author

jaywink commented Jan 2, 2014

Now on my pod iliketoast.net:

pic

Also needs to be added to mobile signup page too.

@jaywink
Copy link
Contributor Author

jaywink commented Jan 3, 2014

@qmaruf - I couldn't figure out how to change your commit author easily, sorry :P

@MrZyx - test for the user model included - I think it actually verifies something ;) I couldn't think of anything to add to the controller test. There is no point in testing the captcha itself, in fact it auto-validates in test environment. If you have something in mind, please let me know.

@jaywink
Copy link
Contributor Author

jaywink commented Jan 3, 2014

Mobile version image from my pod (iliketoast.net):

pic

@Flaburgan
Copy link
Member

Nice work!

@jaywink
Copy link
Contributor Author

jaywink commented Jan 4, 2014

Added missing Changelog entry

AppConfig.settings.captcha.enable = true
@user.sign_up.should be_true
User.find_by_username("ohai").should == @user
end
Copy link
Member

Choose a reason for hiding this comment

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

I'd just make sure that the right methods are called:

@user.should_receive(:save_with_captcha).and_return(false)

and

@user.should_receive(:save).and_return(true)

Both are external dependencies and we assume they work correctly, we really just want to be sure that the right methods are called depending on the setting in case that gets more complex in the future.

maruf and others added 2 commits January 6, 2014 22:00
…rite simple_captcha views to haml. Revert .rvmrc, fix indentation in config examp$
@jaywink
Copy link
Contributor Author

jaywink commented Jan 6, 2014

Fixed test according to comments and squashed to two commits

jhass added a commit that referenced this pull request Jan 6, 2014
@jhass jhass merged commit 70f74dc into diaspora:develop Jan 6, 2014
@jhass
Copy link
Member

jhass commented Jan 6, 2014

Thank you both!

@Flaburgan
Copy link
Member

Awesome guys ;)

@jaywink
Copy link
Contributor Author

jaywink commented Jan 7, 2014

Thanks @qmaruf for working on this!

@jaywink jaywink deleted the simple-captca-for-signup-page branch January 7, 2014 13:21
@goobertron
Copy link

Nice work both of you. Thanks so much for getting this done.

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