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

Alena's MediaRanker #26

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

Conversation

Spatterjaaay
Copy link

@Spatterjaaay Spatterjaaay commented Apr 15, 2017

Media Ranker

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
Describe a custom model method you wrote. I wrote a custom model for displaying the top ten works of each category. It selects all works that belong to a specific category, then orders them by vote_count in descending order, and finally I limit it to the first 10.
Describe how you approached testing that model method. What edge cases did you come up with? I have not yet tested that, but I plan to do that as soon as possible on Saturday. I plan to test that it displays the work in a specific category that has the highest vote count, that it displays only 10 works if there is more, and all of them if there is less than 10. I will also check if it displays work of the correct category and what will be returned if there are no works of that category (empty array in this case).
Describe an edge case test you wrote for a controller For example for create in the Works controller, I am checking that it shows a flash message if I attempt to create a work with no title, which would violate the validations, and I also check that no new object was added to the database.
What are session and flash? What is the difference between them? Flash is a hash-like object that we use to send one time messages from the controller to the views. It persists through one request cycle (as opposed to flash.now that does not). It is usually used to tell the user if they succeeded or failed to do something (such as create a new object). Session is also a hash_like object, but contrary to flash it persists thought multiple request cycles. It is used to keep track of data, for example login information, through what is called a "user session." Session usually ends by closing the browser.
Describe a controller filter you wrote. I hope to learn more about that next week :)
What was one thing that you gained more clarity on through this assignment? Custom routes, redirecting and rendering. I also learned a lot about testing, but that was also new this week, so I don't know if that counts :)
What is the Heroku URL of your deployed application https://alena-media-ranker.herokuapp.com/ Hopefully soon with some CSS too!
Do you have any recommendations on how we could improve this project for the next cohort? I feel like session and flash was explained to us very clearly and I had no problems implementing them in my app. I wish we did spend more time on testing, because most materials I find are not written for minitest and sometimes I have hard time adjusting the syntax (or don't know if I am not creating some horrible hybrids :)) Another recommendation might be to tell us what is the best practice or industry standard? I am still not sure whether I should have had one works controller for all categories or if having separate controllers for books, album and movies is the way. I understand that both have benefits and challenges but maybe one is preferred for reason that are not immediately obvious? I am also not sure if it's okay to pass an argument from a model to an action in the controller.

…he back button reflectsthe cartegory of the work and redirects accordingly
…ion in users controller and created show page for users
…me as well suring a session, added header and log in/out to the application.html
…del in a do block, added tests for user model
…ion fixed path in test testing delete method
… new in respestice controllers, and tested upvoting
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.

Nice work overall I like how the site looks much like the original.


it "If a name is given the user is valid" do
user = users(:user_two)
user.valid?

Choose a reason for hiding this comment

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

probably should be user.valid?.must_equal true

end

it "You can create a user" do
user = users(:user_one)

Choose a reason for hiding this comment

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

Didn't the test above cover this?


it "should redirect to list after adding an album" do
post albums_path, params: { work:
{ title: "Running with Scissors",

Choose a reason for hiding this comment

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

Great album

proc {
post upvote_path(works(:goldman).id)
}.must_change 'Vote.count', 0
end

Choose a reason for hiding this comment

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

I would also test the redirect & flash message here!

@CheezItMan
Copy link

Media Ranker

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene Good regular commits & branching
Comprehension questions Good answers to the questions. I would say that it might be a good idea to create separate controllers for books, albums, movies etc, but perhaps use inheritance so that similar functionality can be pulled from an inherited method, DRYing up your code. I am uncertain how you can pass an argument from a model to a controller as the router calls the controller action and that controller action then interacts with models.
General
Rails fundamentals (RESTful routing, use of named paths) Looks Good
Semantic HTML Good use of semantic HTML, especially the <header> element in the layout. Nice use of tables in the category pages.
Errors are reported to the user Good use of flash notices through the layout. Also good use of the 404 page. One issue is what happens when there is an error deleting a resource? Also the upvote error message isn't being exactly clear about why they cannot upvote. It should probably express their name not their user_id.
Business logic lives in the models Good work putting the top-10 and spotlight methods in the work model. I will say that upvote should probably also be a method in either the vote or work models.
Model testing Testing is done, but the test for returns work with the most votes overall is failing as of now. This is because you're sorting works by votes_count and some of your fixtures don't have a value for it. It would be better to not have a field called votes_count in the model and instead use .votes.count to count all the votes that belong to this work.
Controller testing You do have an error in the movie index page, which the testing exposed because it used a local variable album which doesn't exist. Otherwise good work on the controller testing. I did notice you used assert_equal in a few places. Just a note you could use flash[:error].must_equal "A problem occurred: Could not create movie" Similar with other expectations. It's a little odd to be using assertions mixed in with it and describe. I also love the show test for a resource where they don't exist ex: get user_path("stupid") I also like how you created the helper method to help them sign-in before trying to upvote
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
Individual user pages and the user list are present Check
A user cannot vote for the same media more than once Check
All media lists are ordered by vote count Check
Splash page contains a media spotlight Check
Media pages contain lists of voting users Check
Wave 3 - Styling
Foundation is used appropriately Well done!
Look and feel is similar to the original Very similar, good work
Overall The site is nicely done, a few bugs, but it imitates the original well. I put in a few notices above about your testing and model methods, but this is overall well done.

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