Results by cookie

[mkcode]

Hi @deski - Following up on our discussion in #78

This PR adds info from the peek cookie as discussed. It does not save Peek results unless the peek cookie is enabled. Always sets the cookie if peek is enabled.

In the case Peek was not enabled for a request, and is toggled on, Peek::Views with a data-defer-to are hidden and a refresh message is displayed:

[dewski]

Thanks for opening this up! I'll take a look at this over the next few days.

[dewski]

There should be an assertion here that the cookie isn't set.

[dewski]

Thanks for being patient with me, I gave this a round of review. Let me know what you think.

[mkcode]

Great feedback. I'll fix this up.

[mkcode]

Hi @dewski - I updated this branch according to your feedback.

I changed the the name of the cookie test from peek_enabled to peek_bar_visible: https://github.com/peek/peek/pull/79/files#diff-5fa9351cd8e19ee995f9f3c600861393R14 in response to this comment: #79 (comment). We can't reuse peek_enabled for this, because it is in a global context, while the payload flag is per request. peek_bar_visible is a better name for this now, but that can change if you like.

This test could be better: https://github.com/peek/peek/pull/79/files#diff-d89c7ee11ab586df930a3fb679d46386R10. It is redundant with the other tests now and is actually testing if the request is saved. Ideally, this would be testing that Peek._request_id is set, but I could not figure out how to test for this without mocking, because it is cleared after the request. Deleting that test works for me.

[dewski]

We can't reuse peek_enabled for this, because it is in a global context, while the payload flag is per request. peek_bar_visible is a better name for this now, but that can change if you like.

Could you clarify what you mentioned here? I'd like to have peek_enabled? be the one source for determining if the bar is to be displayed and tracked. If it's disabled it won't render the view and after the request it won't save the payload. Since both methods are in the same controller they should be able to call one another.

[dewski]

I don't think the test case changed here at all, just need to update the test body:

assert_empty Peek.request_id
get '/'
refute_empty Peek.request_id

[mkcode]

Ok! Will update this.

[mkcode]

Could you clarify what you mentioned here?

Sure. peek_enabled is meant to be overridden in descendant controllers. From the readme:

class ApplicationController < ActionController::Base
  def peek_enabled?
    current_user.staff?
  end
end

If we were to reuse peek_enabled, users would need to add a test for the cookie in their peek_enabled? in order to get the benefits of this PR. I see peek_enabled as the place to lock down which users should have access to peek.

Perhaps this is the behavior you would like, which would make this behavior opt-in and we should add this info to the README. It was my intention to turn this behavior on by default, because its zero-cognitive-load on the user and I cannot think of a case where a user would need the results when the bar is not visible.

Either way works!

[mkcode]

peek_enabled? would need to look like this if we were to reuse it:

class ApplicationController < ActionController::Base
  def peek_enabled?
    return false unless peek_bar_visible?
    current_user.staff?
  end
end

[mkcode]

Hi @dewski - Hoping we can resolve this soon. What are your thoughts on #79 (comment) ?

[dewski]

We shouldn't set the peek=true cookie for every request as it will ignore the user's preference if they've set it to false.

[dewski]

We should update this to check if payload[:peek_enabled].