Skip to content

Browsing without login by setting environment variable - #93

Open
kitsuyui wants to merge 1 commit into
cookpad:masterfrom
kitsuyui:browsing-without-login
Open

Browsing without login by setting environment variable#93
kitsuyui wants to merge 1 commit into
cookpad:masterfrom
kitsuyui:browsing-without-login

Conversation

@kitsuyui

@kitsuyui kitsuyui commented Dec 9, 2017

Copy link
Copy Markdown
Contributor

@hogelog hogelog left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could you write spec?

helper_method :current_user

before_action :require_login
if ENV.has_key? 'DMEMO_ALLOW_ANONYMOUS_TO_READ'

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

DMEMO_ prefix is unneccessary.

@favorite_tables = TableMemo.where(id: current_user.favorite_tables.pluck(:table_memo_id))
else
@favorite_tables = []
end

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I want to hide favorite tables without login user.

image

Comment thread app/views/shared/_navbar.html.haml Outdated
%li
= link_to google_oauth2_path(state: url_for(params.merge(only_path: true))) do
%i.fa.fa-sign-in
Login

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Comment thread app/views/users/index.html.haml Outdated
%i.fa.fa-check
%td
- if current_user.editable_user?(user.id)
- if current_user and current_user.editable_user?(user.id)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I like current_user.try!(:editable_user?, user.id)

@kitsuyui

Copy link
Copy Markdown
Contributor Author

@hogelog
Thank you!
I'll solve that you pointed out.

@kitsuyui

Copy link
Copy Markdown
Contributor Author

@hogelog
I have tried to write RSpec for testing top page.
Is this RSpec style OK?
If its direction is OK, I can write more for filling coverage.

@hogelog

hogelog commented Feb 16, 2018

Copy link
Copy Markdown
Collaborator

Sorry too late response.

Please use context like http://www.betterspecs.org/#contexts and other specs.f

- Enable readonly browsing when DMEMO_ALLOW_ANONYMOUS_TO_READ is set
- Hide favorite tables without login user.
- Update RSpec for anonymous login
@kitsuyui
kitsuyui force-pushed the browsing-without-login branch from 5c736a2 to c2e62c0 Compare May 6, 2019 09:30
@kitsuyui

kitsuyui commented May 6, 2019

Copy link
Copy Markdown
Contributor Author

@hogelog

I'm sorry to have kept you waiting.

I've updated the rspec and rebased these commits.
Would you please review this?

(CI Hakiri shows warning because of Nokogiri v1.10.3 (CVE-2019-11068).
But it should be solved in another pull-request, I think.)

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.

2 participants