Skip to content
This repository has been archived by the owner on Apr 14, 2018. It is now read-only.

Carets- Julia Meier- Media-ranker-oath #39

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

julmeier
Copy link

No description provided.

@julmeier
Copy link
Author

I had some issues with testing the error codes. For example, in the works_controller_test:
#QUESTION from Julia: The error for the below test is:
# Expected response to be a <401: unauthorized>, but was a <302: Found> redirect to http://www.example.com/.
#This makes sense, because the before_action in the application controller redirects the user to the root_path if there is no logged in user. Should the test be changed to account for this. How do you test the 401 unauthorized?
it "returns 401 unauthorized if no user is logged in" do
work = works(:mariner)
start_vote_count = work.votes.count
post upvote_path(work)
must_respond_with :unauthorized

  work.votes.count.must_equal start_vote_count
end

@CheezItMan
Copy link

Just a note, it seems that they've changed how things work so a redirect with a error code other than 3XX

http://api.rubyonrails.org/classes/ActionController/Redirecting.html

@CheezItMan
Copy link

Hey Julia,

Just some feedback on your Media-Ranker-OAuth. It's odd, but the PR seems different than your github repo, it looks like you did it via a branch. When I checked out that branch I was unable to run rails db:reset with a validation error. I did fix it with something I noted in your PR.

You also still have a large number of tests, both models & controllers broken. Something to work on.

Overall it looks like you struggled a bit, but got most of the functionality, but little of the testing.

flash[:status] = :success
flash[:result_text] = "Successfully upvoted!"
status = :found
if @work.can_edit_or_delete_work?(session[:user_id])

Choose a reason for hiding this comment

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

I was wrong earlier in that if you use redirect you can't return a status code other than 3XX, (redirection).

You can instead render something and give a response code, or just redirect with a flash notice.

def edit
unless @work.can_edit_or_delete_work?(session[:user_id])
redirect_to work_path(@work.id)

Choose a reason for hiding this comment

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

It would be good to add a filter or at least an if statement to make sure a user can't access this action unless they are the user who created the work.

flash[:status] = :success
flash[:result_text] = "Successfully destroyed #{@media_category.singularize} #{@work.id}"
redirect_to root_path
if @work.can_edit_or_delete_work?(session[:user_id])

Choose a reason for hiding this comment

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

This is just the sort of thing you'd want to put in the edit action.

@@ -3,6 +3,6 @@

Choose a reason for hiding this comment

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

I would suggest something like this in your seeds file.

User.create!(username: "bob", email: "[email protected]", uid: 12345, provider: "google")
User.create!(username: "Ada", email: "[email protected]", uid: 12346, provider: "google")

CSV.foreach(media_file, headers: true, header_converters: :symbol, converters: :all) do |row|
  data = Hash[row.headers.zip(row.fields)]
  data[:user] = User.all.sample
  # puts data
  Work.create!(data)
end

@@ -27,7 +27,7 @@
<%= link_to "Logged in as #{@login_user.username}", user_path(@login_user), class: "button" %>
<%= link_to "Log Out", logout_path, method: :post, class: "button" %>
<% else %>
<%= link_to "Log In", login_path, class: "button float-right" %>
<%= link_to "Log in with Github", "/auth/github", class: "button" %>

Choose a reason for hiding this comment

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

You should also add something for flash notices!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants