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

Elle - Ports #45

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

Elle - Ports #45

wants to merge 14 commits into from

Conversation

dev-elle-up
Copy link

Media Ranker

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
Describe a custom model method you wrote. I did not get to writing any custom methods. However, I would have written a method to find up to 10 items with the most votes matching a given category. I may have used something like top_ten_books = books.max_by(10) {
Describe how you approached testing that model method. What edge cases did you come up with? I would have tested cases where there were over 10 works that matched as well as less than 10 works that matched, and 0 works in the entire category with any votes at all.
What are session and flash? What is the difference between them? Session uses cookies to keep track of a logged-in user and persists through one browser session by default. Flash is used to display messages about an action to a user and persists through only one request-response cycle.
What was one thing that you gained more clarity on through this assignment? How to seed databases using a CSV file
What is the Heroku URL of your deployed application? https://elle-media-ranker.herokuapp.com/
Do you have any recommendations on how we could improve this project for the next cohort? My struggle with getting this done was not about ability or knowledge but about not having enough time. One thing that could help with the time crunch would be to have the controllers, controller tests, and models set up already and ask the future students to implement the rest of the app. These tasks seemed repetitive at this point, after having done essentially the same thing (with some refactors) for Ada Books, Task List, and Rails Ride Share. I will say that doing these things for the 5th time on bEtsy went very quickly!

…. Generated a Votes model. Configured routes.
…er. Also wrote destroy action for works controller.
…reated views for same. Form is not loginform is not displaying field or button.
…. Also created and modified views for each action.
…or each work. Added title field to work form (oopos, forgot last time) Started troubleshooting problem of a new work not being saved (create method not working, form not passing params but it is because the tests pass) - investigation continues.
@tildeee
Copy link

tildeee commented May 6, 2019

Media Ranker

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene x
Comprehension questions x
General
Rails fundamentals (RESTful routing, use of named paths) routes are set up... but many are not being used!
Views are well-organized (DRY, use of semantic HTML, use of partials) x, it may be interesting to see how a partial could be used on works#index
Errors are reported to the user flash is set, but never displayed on the page!
Business logic lives in the models
Models are thoroughly tested, including relations, validations and any custom logic
Controllers are thoroughly tested, including the login/logout process and multi-step workflows like voting for a work didn't get to voting
Wave 1 - Media
Splash page shows the three media categories x
Basic CRUD operations on media are present and functional x, but no buttons for "new"
Wave 2 - Users and Votes
Users can log in and log out i can test login by going to /login, but there's no login button. there's no logout button
The ID of the current user is stored in the session
A user cannot vote for the same media more than once
All media lists are ordered by vote count
Splash page contains a media spotlight
Wave 3 - Users and Votes
Media pages contain lists of voting users
Individual user pages and the user list are present
Optional - Styling
Bootstrap is used appropriately
Look and feel is similar to the original
Overall

Hi Elle! We've had a conversation about Media Ranker so I'll keep it brief:

Things I want to touch base on and make sure your working knowledge feels solid:

  • Showing feedback to the users through flash
  • Small bits about syntax that I've left a few comments on
  • Overall how voting logic would have gone

Things you did well on:

  • Controller filters
  • Overall controller logic and testing
  • Clear "thinking like a programmer" with the current work you have

My interesting observation is that you have some code that doesn't work while manually testing... for example, you've implemented flash, but never showed it. You have some pages/views that don't have buttons on the site. It might be interesting to think about what's going on, so I'll mention it briefly at our next one-on-one

Good work, though. Media Ranker is a huge project with a lot of moving parts!

# end

def create
new_user = User.new[:user_params]
Copy link

Choose a reason for hiding this comment

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

Instead of User.new[:user_params, you'll want to use the private method named user_params as an argument for the method new (which means (), not []), so new_user = User.new(user_params)

<h1>User Summary: <%= @user.username%> </h1>

<p>
<%= "Joined site @user.created_at" %>
Copy link

Choose a reason for hiding this comment

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

Didn't interpolate @user.created_at hehe

},
}

Work.new(valid_work_params)
Copy link

Choose a reason for hiding this comment

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

The format valid_work_params follows the structure for how Rails formats form data when it sends it to the controller via params and using our strong params method. Thus; it's not quite the right place to use this form (nested hash) for a model test

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