Fixed (I hope) issue #30: Do not allow empty username and password when create user #37

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
2 participants
@adinapoli
Member

adinapoli commented Sep 13, 2012

Hi everyone,

this is an important step for me, my first contribution to an Haskell project :)
My patch is probably the most naive thing in the universe, and I've tested the code only from user side (aka in a new snap app) so expect more pull reqs with HUnit tests :)

As I said, it should work, now if you try to create a new empty user this is what Snap answers:

A web handler threw an exception. Details:
Username cannot be empty

I hope this is the expected behaviour and to have been useful!

And glad to be able to contribute, even in small part, to the Haskell ecosystem :)

Cheers,
Alfredo

ps. I've also update the .gitignore file, relaxing the constraint over the dist directory to be "dist*" :
since I'm a hsenv user, the tool creates a custom dist directory with as suffix the name of the sandbox environment. With my .gitignore we should be able to catch both cases (i.e. "plain" env and sandboxed env with hsenv).

@mightybyte

View changes

src/Snap/Snaplet/Auth/Handlers.hs
@@ -44,6 +44,7 @@ import Snap.Snaplet.Session
createUser :: Text -- ^ Username
-> ByteString -- ^ Password
-> Handler b (AuthManager b) AuthUser
+createUser "" _ = error "Username cannot be empty"

This comment has been minimized.

Show comment Hide comment
@mightybyte

mightybyte Sep 13, 2012

Owner

This isn't quite what we want here. The error function should be avoided as a general rule. Since this is runtime code, we want cleaner error handling. There are several possibilities. You could call pass or return a Maybe or an Either value. The latter two would force the caller to explicitly handle the error, which is probably a good thing. But don't be discouraged. This is a good first attempt. It may be better to put together some gists and discuss them in #snapframework until we come up with the right solution.

@mightybyte

mightybyte Sep 13, 2012

Owner

This isn't quite what we want here. The error function should be avoided as a general rule. Since this is runtime code, we want cleaner error handling. There are several possibilities. You could call pass or return a Maybe or an Either value. The latter two would force the caller to explicitly handle the error, which is probably a good thing. But don't be discouraged. This is a good first attempt. It may be better to put together some gists and discuss them in #snapframework until we come up with the right solution.

This comment has been minimized.

Show comment Hide comment
@adinapoli

adinapoli Sep 13, 2012

Member

I apologize, I Know almost nothing about how Snap works yet :) Tomorrow I'll dig inside the docs and I'll write a cleaner error handling :)
Please be patience :)

Ps. Thanks for your guidance, I'll post a gist tomorrow as soon as I have a solution worth looking at :)
Cheers,
A.

@adinapoli

adinapoli Sep 13, 2012

Member

I apologize, I Know almost nothing about how Snap works yet :) Tomorrow I'll dig inside the docs and I'll write a cleaner error handling :)
Please be patience :)

Ps. Thanks for your guidance, I'll post a gist tomorrow as soon as I have a solution worth looking at :)
Cheers,
A.

This comment has been minimized.

Show comment Hide comment
@mightybyte

mightybyte Sep 13, 2012

Owner

No worries. You gotta start somewhere. :)

@mightybyte

mightybyte Sep 13, 2012

Owner

No worries. You gotta start somewhere. :)

@adinapoli

This comment has been minimized.

Show comment Hide comment
@adinapoli

adinapoli Sep 15, 2012

Member

I though that before jumping into testing it would have been beneficial to check the code I wrote. The types are correct, so are the behavior inside Snap: the server doesn't raise nothing, but the user don't get saved. I've also make some little tweak because according to ghc-mod in some point of Handler.hs there was code that could have been written better using liftM. As regards the last point please tell me is my attitude was right or in general I should avoid to polish the code the way I did.
I resisted the temptation to go for an "eta reduce" because I'm my opinion it decreases readability of the code.

Very important: I've relaxed the constrains over http-conduit and http-types in the test cabal file. It seems to not break anything, please check for that!

Last but not least, I fixed the deprecated template inside the test directory, otherwise most tests would have failed.

Cheers,
A.

Member

adinapoli commented Sep 15, 2012

I though that before jumping into testing it would have been beneficial to check the code I wrote. The types are correct, so are the behavior inside Snap: the server doesn't raise nothing, but the user don't get saved. I've also make some little tweak because according to ghc-mod in some point of Handler.hs there was code that could have been written better using liftM. As regards the last point please tell me is my attitude was right or in general I should avoid to polish the code the way I did.
I resisted the temptation to go for an "eta reduce" because I'm my opinion it decreases readability of the code.

Very important: I've relaxed the constrains over http-conduit and http-types in the test cabal file. It seems to not break anything, please check for that!

Last but not least, I fixed the deprecated template inside the test directory, otherwise most tests would have failed.

Cheers,
A.

@mightybyte

This comment has been minimized.

Show comment Hide comment
@mightybyte

mightybyte Sep 16, 2012

Owner

Merged. Thanks.

Owner

mightybyte commented Sep 16, 2012

Merged. Thanks.

@mightybyte mightybyte closed this Sep 16, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment