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

Allow configuration of Cookies. #51

Merged
merged 9 commits into from Jul 15, 2014
Merged

Allow configuration of Cookies. #51

merged 9 commits into from Jul 15, 2014

Conversation

gotjosh
Copy link
Contributor

@gotjosh gotjosh commented Jul 12, 2014

Allow the Lotus#configuration to inject the Lotus::Action::Cookies
module in your application’s controller.

When the application is loading the framework,
Lotus::Loader.load_frameworks! will inject the before mentioned
module if the configuration is set.

Closes #1.

Signed-off-by: Josue Abreu josh@pixelpt.com

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) when pulling 65c1849 on gotjosh:allow-cookies-configuration into 3bd5f24 on lotus:master.

# called without, it will return the already set value, or the default.
#
# @overload cookies(value)
# Instructs lotus#load_frameworks! to inject Lotus::Action:Cookies.
Copy link
Member

Choose a reason for hiding this comment

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

Documentation should be high level, if we change the loader internals we should change this line too.
What do you think about: Enable cookies (disabled by default)?

@gotjosh
Copy link
Contributor Author

gotjosh commented Jul 13, 2014

Thank you very much for the feedback @jodosha, will start tidying up immediately. Do you want me to amend the mistakes to keep the history clean or want me to put a commit on top of it?

# Gets the boolean value or returns false if it's not set.
# @return [Boolean]
#
# @since 0.1.0
Copy link
Member

Choose a reason for hiding this comment

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

is it?

@jodosha
Copy link
Member

jodosha commented Jul 13, 2014

@gotjosh Thank you for your first PR. It looks good.

Before to merge it:

  1. I would like you to add integration tests for this. Please have a look at the test/integration directory and to the cookies test of Lotus::Controller.
  2. You will discover that Rack::Cookies should be injected in the middleware stack, otherwise it won't work. ;)
  3. We need this feature to be documented in README.

Thanks :shipit:

@jodosha
Copy link
Member

jodosha commented Jul 13, 2014

@gotjosh Please add more commits, don't worry about the clean history.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) when pulling c6f7c3e on gotjosh:allow-cookies-configuration into 3bd5f24 on lotus:master.

@gotjosh
Copy link
Contributor Author

gotjosh commented Jul 14, 2014

@jodosha Could you point me in the right direction when it comes to where you'd like me to add the integration tests? From here, it's uncertain whenever you want me to add in middleware or any of the other apps... 😁

@jodosha
Copy link
Member

jodosha commented Jul 14, 2014

@gotjosh Please have a look at test/integration/middleware_stack_test.rb.

You should create a tiny app like MiddlewareStack::Application and use cookies from the action(s).
In the test code, use rack-test to verify that the cookies are correctly received from the client. Hint: inspect them with request.cookies.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) when pulling 16eeaa8 on gotjosh:allow-cookies-configuration into 3bd5f24 on lotus:master.

Allow the `Lotus#configuration` to inject the `Lotus::Action::Cookies`
module in your application’s controller.

When the application is loading the framework,
`Lotus::Loader.load_frameworks!` will inject the before mentioned
module if the configuration is set.

Closes #1.

Signed-off-by: Josue Abreu josh@pixelpt.com
- Documentation should be high level.
- Replace boolean with TrueClass & FalseClass.
- Removing redundant overloads.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) when pulling e815e85 on gotjosh:allow-cookies-configuration into e13b532 on lotus:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) when pulling 7da7370 on gotjosh:allow-cookies-configuration into e13b532 on lotus:master.

@@ -410,6 +410,10 @@ module Bookshelf
#
routes 'config/routes'

# The mapping set (optional) (alternative usage)
Copy link
Member

Choose a reason for hiding this comment

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

This is out of scope here, can you please remove it?

@jodosha jodosha self-assigned this Jul 15, 2014
@jodosha
Copy link
Member

jodosha commented Jul 15, 2014

@gotjosh This looks good 👍 Can you please remove that mapping documentation so that we can merge it? Thanks.

@jodosha
Copy link
Member

jodosha commented Jul 15, 2014

@gotjosh I've also tried that Cookies::Application in the browser. Can you please use the same key for the cookies? Eg. cookies[:foo], so that fixture can be used as example for developers who want to try it.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) when pulling 90ce4c7 on gotjosh:allow-cookies-configuration into e13b532 on lotus:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) when pulling 90ce4c7 on gotjosh:allow-cookies-configuration into e13b532 on lotus:master.

@gotjosh
Copy link
Contributor Author

gotjosh commented Jul 15, 2014

@jodosha Everything should be good to go!

jodosha added a commit that referenced this pull request Jul 15, 2014
@jodosha jodosha merged commit 3b1be6f into hanami:master Jul 15, 2014
@jodosha
Copy link
Member

jodosha commented Jul 15, 2014

@gotjosh Thanks for your work! 👍

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.

Allow to configure cookies
4 participants