Skip to content

Conversation

@mllocs
Copy link
Collaborator

@mllocs mllocs commented Jun 13, 2018

giphy

Here I'm trying to make the "Remember me" option work, which will be the default option in the mobile app.

Instead of expiring the session the hard way, I'm using the timeoutable devise module. I'm also configuring the rememberable module to remember sessions for 4 weeks.

I also did a little cleanup on the alert bootstrap component, so, instead of using our own styled alert messages, I'm using bootstrap ones, which look exactly the same :D

I also updated devise from 4.4.1 to 4.4.3 🚀

UPDATE

How to test:

Change config.remember_for and config.timeout_in in your environment. In the gif, timeout_in was set to 5.seconds and remember_for to 10.seconds. With those values:

  • If I DON'T check Remember me, whenever I stay more than 5 seconds without doing a request, I'll be kicked out.
  • if I check Remember me, I can use the app without doing login again for 10 seconds.

In the gif you can also appreciate another case:

  • When you are kicked out and you go to a public page (like offers or demands), you'll see a flash message saying that your session expired but you are not redirected to the login.

  • Instead, if you navigate to a page where you need to be looged in to see, whenever your session expires you'll be redirected to the login page and you'll see the flash message there as well.

@mllocs mllocs force-pushed the fix/remember-me branch from 6258941 to 4a318ec Compare June 13, 2018 18:38
@@ -1,5 +1,7 @@
<% flash.each do |key, value| %>
<div class="alert alert-<%= key %>">
<% next if key == 'timedout' %>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mllocs mllocs changed the title [WIP] Fixed "Remember me" in Login Fixed "Remember me" in Login Jun 13, 2018
@@ -1,4 +1,6 @@
class PostsController < ApplicationController
before_action :authenticate_user!
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👋

Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 no, no? 😂

@mllocs
Copy link
Collaborator Author

mllocs commented Jun 13, 2018

This test fails OffersController GET #show with valid params without a logged in user assigns the requested offer to @offer. Do we want to allow logged out visitors to see individual offers? 🤔

cc @sseerrggii @enricostano

@sseerrggii
Copy link
Contributor

sseerrggii commented Jun 14, 2018

Yes, offers and demands (Post) are public, the view of post changes if you are logged in or logged out. Showing or not de member contact data

Copy link
Contributor

@enricostano enricostano left a comment

Choose a reason for hiding this comment

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

I think we need to:

  1. remove the authenication step on PostController
  2. check that the "remember me" think actually work, it wasn't last time I checked. Can we add sme step in the description on how to test this? Thanks!

@@ -1,4 +1,6 @@
class PostsController < ApplicationController
before_action :authenticate_user!
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 no, no? 😂

'alert-info'
end
end
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

@mllocs maybe we can move this to ApplicationHelper, I think we don't need an extra module only for this helper method (I'm also removing "blank" helpers in #361).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll create an issue where we can discuss this, I prefer to tackle it in a different PR since we already tested it 😄

Copy link
Collaborator

Choose a reason for hiding this comment

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

no prob @mllocs, I'll do it in #361 while rebasing develop


<div class="alert <%= alert_class(key) %>">
<button type="button" class="close" data-dismiss="alert">x</button>
<ul>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we fix indentation here 🙏 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll do it in the next PR 💪

@enricostano
Copy link
Contributor

@mllocs I'm testing this branch in staging, I set config.remember_for = 15.seconds and config.timeout_in = 5.seconds

When remember me:

  • IS NOT checked: it works
  • IS checked: I'm kicked out after 5 seconds of inactivity

@enricostano
Copy link
Contributor

Tested, it actually works 😬

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.

5 participants