Skip to content

Conversation

@knockknockhusthere
Copy link

Media Ranker

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
Describe a custom model method you wrote. A method that sorts through and returns the list of albums in order from most to least votes
Describe how you approached testing that model method. What edge cases did you come up with? I didn't get around to testing the business logic, but would ideally run the method, and check that it returns the right number of objects, and that the top few users are returned in the correct order. As an edge case, I would test how the method treats a work has no votes
What are session and flash? What is the difference between them? Flash is a hash-like feature used to send one-time messages to the view. Session on the other hand, keeps track of specified data for the entirety of a browser session
Describe a controller filter you wrote. I didn't have time to
What was one thing that you gained more clarity on through this assignment? There's so many moving parts as we learn more about rails and testing is easier to talk about than it is to
What is the Heroku URL of your deployed application? implementhttps://media-ranker-angela.herokuapp.com
Do you have any recommendations on how we could improve this project for the next cohort?
The waves are a little confusing. It's hard to picture building out a homepage without having voting function or something to sort the votes already.

@kariabancroft
Copy link

Media Ranker

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene For a project over several days, you should have many many more commits.
Comprehension questions Yes
General
Rails fundamentals (RESTful routing, use of named paths) Yes
Semantic HTML Yes
Errors are reported to the user Some - using the model errors in the forms, but missing use of flash for additional information. For example, deleting a work doesn't function but the user gets no information about that.
Business logic lives in the models Yes - I'm glad that you included these model methods. Each of them include a lot of duplicate code though, so I'm curious if you see a common pattern that you could use to create a shared method?
Models are thoroughly tested, including relations, validations and any custom logic Partially - I see some relationship testing but not all and missing custom model methods
Wave 1 - Media
Splash page shows the three media categories Yes - Though it doesn't have the links to the media options
Basic CRUD operations on media are present and functional Create and edit work - delete does not
Wave 2 - Users and Votes
Users can log in and log out Yes
The ID of the current user is stored in the session Yes
Individual user pages and the user list are present The user list is but not the individual user page
A user cannot vote for the same media more than once Not implemented
All media lists are ordered by vote count Yes
Splash page contains a media spotlight Yes
Media pages contain lists of voting users Yes
Wave 3 - Styling
Foundation is used appropriately Some
Look and feel is similar to the original Not quite
Overall You did a good job exercising lots of different CRUD actions and setting up the relationships here. I'd like to see you practice more testing in betsy as well as always using flash messages in your controller actions for additional information.

Copy link

@kariabancroft kariabancroft left a comment

Choose a reason for hiding this comment

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

sorry i just realized that i forgot to submit these comments along with the original review


get '/votes', to: 'votes#index', as: 'votes'

resources :users

Choose a reason for hiding this comment

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

Do you need all of these routes?

</spotlight>

<categories>
<top-movies>

Choose a reason for hiding this comment

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

I'm unsure about using these as HTML tags - is the goal here a semantic tag?

work = Work.find_by(id: params[:id])
work.vote.delete_all

if work

Choose a reason for hiding this comment

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

It would make sense to check for the work variable before you try to delete all of the votes. Additionally, if you set up the relationship to delete dependent entries in the model, you wouldn't need to explicitly do this delete operation here.

end

def show
@votes = Vote.all

Choose a reason for hiding this comment

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

Why would you use all of the votes in this view for a particular work?

@users = User.all
end

def new

Choose a reason for hiding this comment

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

It seems like you created the standard CRUD actions in the controller action, but don't actually need them (or use them)

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