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

Ana Lisa Sutherland: Octos C9: MediaRanker-Revisited #31

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

Conversation

The-Beez-Kneez
Copy link

Currently, tests are all broken and some commented out (when I was attempting to hunt down what went wrong). Will fix later today.

…or Show Method. Also added in .env file to begun OmniAuth procedure
…xed bug in Works Controller changed render :not_found to :bad_request
…n user/vote pair registers a vote via user as well as work
… this, updated application view to support this.
@droberts-sea
Copy link

MediaRanker Revisted

What We're Looking For

Feature Feedback
General
Tests on WorksController yes
Tests on SessionsController yes
Tests on UsersController yes
Completed OAuth yes
Overall Great work overall! There are a couple of small mistakes, but that's why professional engineers use code reviews.

create_table :users do |t|
t.string :name
t.string :email
t.integer :uid, null: false # this is the identifier provided by GitHub

Choose a reason for hiding this comment

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

This project already has a table users, so creating a new table probably isn't what you want here. Instead you might edit the existing table using add_column.

Looks like Rails takes the last one if there are multiple migrations with the same name, so this may have worked accidentally.

it "succeeds with one media type absent" do
# Precondition: there is at least one media in two of the categories
%w(albums movies).each do |category|
Work.by_category(category).length.must_be :>,0
Work.by_category(:books).destroy_all

Choose a reason for hiding this comment

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

This is strange to me - inside the loop you're both checking that the category has members and destroying all the books. Probably the destroy_all wants to be outside the loop.

end

it "redirects to the work page after the user has logged out" do
work_id = Work.first.id

Choose a reason for hiding this comment

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

Why is this test nested under upvote?

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