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

<pipes><Roxanne Agerone> <MediaRanker> #41

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

Conversation

RAgerone
Copy link

@RAgerone RAgerone commented Oct 16, 2017

Media Ranker

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
Describe a custom model method you wrote. I wrote a self method to get ten of each of the media types.
Describe how you approached testing that model method. What edge cases did you come up with? I wrote some to test that the user accepted a valid email.
Describe an edge case test you wrote for a controller I didn't get very far with controller methods, but I did get a couple of the media_instances tests to work. I tested for the status updating and for the create method working and not working.
What are session and flash? What is the difference between them? Session us used for the duration of your stay on the website. Flash resets between pages. They both require cookies.
Describe a controller filter you wrote. Didn't get there yet. I can see about 5 places where they need to be implemented in order to keep my code DRY.
What was one thing that you gained more clarity on through this assignment? This project was really frustrating for me. I gained more clarity on models and controllers, but I still don't feel like I get it. The foundations css formatting is not working in my Heroku app. I could not have done this project by myself. I needed A LOT of help and I still have more work to do.
What is the Heroku URL of your deployed application https://media-instances-ranker.herokuapp.com/
Do you have any recommendations on how we could improve this project for the next cohort? More project time in class.

@droberts-sea
Copy link

Media Ranker

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene yes
Comprehension questions yes
General
Rails fundamentals (RESTful routing, use of named paths) some - see inline comment, and text below
Semantic HTML yes
Errors are reported to the user some - You've got a strange hybrid of using flash and using errors.messages directly from the view. This is something it would be worth taking some time to study.
Business logic lives in the models yes - good work
Models are thoroughly tested, including relations, validations and any custom logic no - many of your tests passed without you testing anything (see inline). I would also like to see tests around model relations, and around the custom method you wrote on MediaInstance.
Controllers are thoroughly tested no - I was not able to get these tests to run, and it seems like you really struggled here. I suspect that all the complicated routing you did made this much more difficult than it should have been.
Wave 1 - Media
Splash page shows the three media categories yes
Basic CRUD operations on media are present and functional yes - requires user to be logged in, which doesn't match the demo site
Wave 2 - Users and Votes
Users can log in and log out yes, once rolled back to previous version (see below)
The ID of the current user is stored in the session yes
Individual user pages and the user list are present no
A user cannot vote for the same media more than once no
All media lists are ordered by vote count no
Splash page contains a media spotlight section is on the page, but just shows a random work
Media pages contain lists of voting users no
Wave 3 - Styling
Foundation is used appropriately some - seems like there is still a lot of ground to explore here
Look and feel is similar to the original some
Overall

This is a good start, but it feels like there's still many places with room for improvement. In particular, building routes and views on top of models with complex relations and behaviors seems to have been a challenge.

I am noticing that you've added a lot of functionality that isn't in the original site. For example, the advanced routing work around media_instances/:media_type is quite complex and not needed for the site as demoed. While I appreciate the spirit, I think that this ended up distracting from the core learning goals of the project, and made figuring out the rest of it more difficult.

I also had a lot of trouble running your site. It looks like you added a partial implementation of OAuth to your project and then pushed it to GitHub, and the PR picked up your changes (this happens automatically whenever you update a branch). I had to roll back to a previous commit in order to get anything to work.

Finally, it seems like you struggled a lot with testing. This is something we'll want to think about moving forward, but for now you should focus on shoring up your core rails understanding.

This is a big complex project with many moving pieces, so don't worry if you weren't quite able to get everything working the way you wanted. Still, I feel it would be worthwhile to meet on Monday to talk about how to do better going forward (and because I'm due for a 1-on-1 with you anyway). Please sign up for a slot on my calendar (same link as always).

config/routes.rb Outdated

resources :media_instances do
resources :users, only: [:index] do
resources :votes, only: [ :create, :destroy]

Choose a reason for hiding this comment

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

Why are these routes nested like this? I could see :create for users/:user/votes, but other than this these routes should be flat.

</body>

<footer>
&copy 2017

Choose a reason for hiding this comment

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

This <footer> should be inside the <body>


it "returns a success status when given a valid user_id" do
get user_media_instances_path(user.first)
must_respond_with :success

Choose a reason for hiding this comment

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

I don't understand what the user_media_instances_path is for, and I don't believe it's used anywhere in your application. This is usually a good indication that the route isn't needed.

end
it "will give an error message for not having media_instance_id nor user_id" do
vote.errors.messages.include? :media_instance_id
vote.errors.messages.include? :user_id

Choose a reason for hiding this comment

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

You don't actually test anything here. Try replacing .include? with .must_include.

media_instance.errors.messages.include? :title
end
it "must reject if there is no media type" do
media_instance.errors.messages.include? :media_type

Choose a reason for hiding this comment

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

Similarly, this doesn't actually check anything. When writing a test, you need to have some expectation (must_something or wont_something).

Going through the red-green-refactor cycle would have caught this. You would have tried to watch the test fail, and when it didn't, you would realize something is up.

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