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

Christina MedRanRev #38

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

Christina MedRanRev #38

wants to merge 6 commits into from

Conversation

Peacegypsy
Copy link

All tests are passing but I haven't run simplecov yet. I don't think the user is being logged in. Will keep working at it to find it.

@CheezItMan
Copy link

MediaRanker Revisted

What We're Looking For

Feature Feedback
General
Appropriate Tests on WorksController Basic tests on the works controller. However only one test on authenticated users using the controller.
Appropriate Tests on SessionsController You have no tests here, instead it looks like you copied your controller methods into the test file. This is highly concerning.
Appropriate Tests on UsersController No Tests
Completed OAuth It doesn't work since the sessions controller doesn't set session. You do have pieces of it. See my comments.
Overall This continues a troubling trend. I want you to review this project with Charles or myself. You need to know how to: 1. Write controller tests, 2. See a Rails project through to completion. Thus far I have not seen this from you.

@@ -1,34 +1,59 @@
class SessionsController < ApplicationController
def login_form
end
def create

Choose a reason for hiding this comment

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

You don't seem to be setting session[:user_id] = user.id here?


end
expect{
post upvote_path(id)}.wont_change 'Vote.count'

Choose a reason for hiding this comment

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

Indentation is a little off here.


end
it "redirects to the work page after the user has logged out" do

Choose a reason for hiding this comment

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

empty test


it "redirects to the work page if the user has already voted for that work" do
it "succeeds for a logged-in user and a fresh user-vote pair" do

Choose a reason for hiding this comment

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

There's no logging in here.

@@ -1,5 +1,40 @@
require "test_helper"

describe SessionsController do
def create

Choose a reason for hiding this comment

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

Why do you have methods here? This is the test file. You should have it blocks and expectations.

I do not understand what's going on here.

end

it "redirects to the work page if the user has already voted for that work" do
user = users(:grace)

Choose a reason for hiding this comment

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

This is a pretty good attempt at trying to test the login functionality along with voting.

Problems: 1. Your sessions controller isn't setting sessions[:user_id]
2. Somehow the post upvote_path(id) is clearing session completely erasing everything in it. I'm not sure how.

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