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

Browsing without login by setting environment variable #93

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kitsuyui
Copy link
Contributor

@kitsuyui kitsuyui commented Dec 9, 2017

Copy link
Collaborator

@hogelog hogelog left a comment

Choose a reason for hiding this comment

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

Could you write spec?

@@ -4,6 +4,9 @@ class ApplicationController < ActionController::Base
helper_method :current_user

before_action :require_login
if ENV.has_key? 'DMEMO_ALLOW_ANONYMOUS_TO_READ'
Copy link
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
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

%li
= link_to google_oauth2_path(state: url_for(params.merge(only_path: true))) do
%i.fa.fa-sign-in
Login
Copy link
Collaborator

Choose a reason for hiding this comment

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

@@ -27,6 +27,6 @@
- if user.admin?
%i.fa.fa-check
%td
- if current_user.editable_user?(user.id)
- if current_user and current_user.editable_user?(user.id)
Copy link
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
Contributor Author

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

@kitsuyui
Copy link
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
Copy link
Collaborator

hogelog commented Feb 16, 2018

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
Copy link
Contributor Author

kitsuyui commented May 6, 2019

@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