friendly vault, snippet and user path, clean up controller, test#13
friendly vault, snippet and user path, clean up controller, test#13mrvncaragay wants to merge 2 commits intocodervault:masterfrom
Conversation
| def set_vault | ||
| @vault = params[:vault_id] ? Vault.find_by_id(params[:vault_id]) : Vault.find_by_id(params[:id]) | ||
| redirect_to root_url if @vault.nil? || @vault.user_username != params[:username] | ||
| @vault = params[:vault_id] ? Vault.find_vault(params[:vault_id]) : Vault.find_vault(params[:id]) |
There was a problem hiding this comment.
What happens if @vault is nil?
There was a problem hiding this comment.
@CGA1123 def authenticate_username ... is called first. It checks if the user is valid and checks if the user owns the vault. Therefore, if the that pass no need to authenticate set_vault
There was a problem hiding this comment.
It seems unwise to couple those two methods together that closely.
It would make more sense to make set_vault check for correct ownership, do you not think?
There was a problem hiding this comment.
I see what you saying @CGA1123. is there trade-off going this way or is just a bad practice and should be avoided?
There was a problem hiding this comment.
I don't know whether it's bad practice or not, it just seems more logical to keep the validation of vault in set_vault.
There was a problem hiding this comment.
okay, will update this change. the code below belongs to SalGant and i just moved it to application since they're both the same code.
There was a problem hiding this comment.
You could still push those changes!
| end | ||
|
|
||
| def check_edit_permissions | ||
| if @vault.user_id != current_user.id then |
|
|
||
| def check_edit_permissions | ||
| if @vault.user_id != current_user.id then | ||
| flash[:alert] = t(:permissions_error) |
There was a problem hiding this comment.
You can write redirect_to root_url, alert: t(:permissions_error)
| <div class="col-md-12"> | ||
| <div class="panel panel-default panel-interactive"> | ||
| <%= link_to "", vault_path(vault, username: vault.user.username), class: "overlay" %> | ||
| <%= link_to "", owner_vault_path(vault, username: vault.user.username), class: "overlay" %> |
There was a problem hiding this comment.
Could use vault.user_username here, to keep things consistent
2 participants
No description provided.