Skip to content

Ports - Kate#44

Open
goblineer wants to merge 67 commits intoAda-C11:masterfrom
goblineer:master
Open

Ports - Kate#44
goblineer wants to merge 67 commits intoAda-C11:masterfrom
goblineer:master

Conversation

@goblineer
Copy link

@goblineer goblineer commented Apr 30, 2019

Media Ranker

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
Describe a custom model method you wrote. The methods that create the front-page content (top ten lists, spotlight media, etc.) are all custom methods of the Works model. I created them to sort the data so that views could pull up the pre-sorted lists.
Describe how you approached testing that model method. What edge cases did you come up with?
What are session and flash? What is the difference between them? Session is used to save information over a users session keeping track of activity once logged in through cookies. Flash is a one time message shown to the users telling the user something happened. It is also a way to display error messages.
--
What was one thing that you gained more clarity on through this assignment? I'm starting to "see" the RESTful routes much more instinctively. By the end of the project, I could see some wacky design choices I'd made at the beginning of the project, which tells me that I had a few big a-ha moments along the way.
What is the Heroku URL of your deployed application? http://mediaranks.herokuapp.com/
Do you have any recommendations on how we could improve this project for the next cohort?

@CheezItMan
Copy link

Media Ranker

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene Good number of commits and commit messages
Comprehension questions Check, I'm glad you have a better understanding of RESTful routes. There are some questions unanswered
General
Rails fundamentals (RESTful routing, use of named paths) Check
Views are well-organized (DRY, use of semantic HTML, use of partials) Check
Errors are reported to the user Check
Business logic lives in the models Check
Models are thoroughly tested, including relations, validations and any custom logic NOPE
Controllers are thoroughly tested, including the login/logout process and multi-step workflows like voting for a work NOPE
Wave 1 - Media
Splash page shows the three media categories Check
Basic CRUD operations on media are present and functional Check
Wave 2 - Users and Votes
Users can log in and log out Check
The ID of the current user is stored in the session Check
A user cannot vote for the same media more than once Can't vote!
All media lists are ordered by vote count Check
Splash page contains a media spotlight Check
Wave 3 - Users and Votes
Media pages contain lists of voting users Check
Individual user pages and the user list are present Check
Optional - Styling
Bootstrap is used appropriately Check, nice!
Look and feel is similar to the original Check
Overall Not bad, not perfect either. Things to work on include the upvote and working to improve testing. Check out my inline comments and let me know any questions you have.

Copy link

@CheezItMan CheezItMan left a comment

Choose a reason for hiding this comment

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

Not bad, not perfect, but mostly working. Things to improve: Testing in all categories. Some things seem broken on Heroku.


it "should get logout" do
post logout_path
must_respond_with :success

Choose a reason for hiding this comment

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

This test is failing: Logout should respond with :redirect

You should also test the flash notices.

get login_path
must_respond_with :success
end

Choose a reason for hiding this comment

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

You're missing a test for submitting the login form.

let(:user) { users(:one) }

describe 'validations' do
it 'is valid when all fields are present' do

Choose a reason for hiding this comment

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

You should also be testing the reasons the validations fail.

end
end

describe 'has_voted?' do

Choose a reason for hiding this comment

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

Good business logic method.

expect(result).must_equal true
end

it 'requires user_id' do

Choose a reason for hiding this comment

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

These validations come with the relationships.

You should however have a validation to ensure that the combination of user_id and work_id is unique.

@@ -0,0 +1,79 @@
class WorksController < ApplicationController
before_action :find_work, only: [:show, :edit, :update, :destroy, :upvote]

Choose a reason for hiding this comment

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

Nice filter!

end

def update
if @work.update(work_params)

Choose a reason for hiding this comment

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

You should also check to see if @work.nil just to see if it found that work.

def upvote
if @super_user.nil?
flash[:warning] = "Please log in so your vote can be counted."
redirect_to login_path

Choose a reason for hiding this comment

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

For some reason Heroku is crashing here, not sure why. I would also suggest using redirect_back, but I'm not sure why.

post 'works/:id/upvote', to: 'works#upvote', as: 'upvote'
resources :works

resources :users, only: [:index, :show]

Choose a reason for hiding this comment

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

👍

flash[:warning] = "Sorry, you can't vote twice on the same work."
redirect_back fallback_location: works_path
else
Vote.create(user_id: @super_user.id, work_id: @work.id)

Choose a reason for hiding this comment

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

This is failing because it's complaining about can't write unknown attribute vote_count``

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