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

Leanne R #33

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

Leanne R #33

wants to merge 32 commits into from

Conversation

leannerivera
Copy link

@leannerivera leannerivera commented Oct 15, 2018

Media Ranker

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
Describe a custom model method you wrote. voted? to check if user has already voted on work or not.
Describe how you approached testing that model method. What edge cases did you come up with? making sure that each work has only one vote per user.
What are session and flash? What is the difference between them? session stores the information on a user's time on the app, while flash are messages that can be called to display quick info to user
Describe a controller filter you wrote. to find instance via id (before_action)
What was one thing that you gained more clarity on through this assignment? flashes and warnings and using rails to simplify html in views
What is the Heroku URL of your deployed application? https://radiant-citadel-31716.herokuapp.com/
Do you have any recommendations on how we could improve this project for the next cohort? i liked it, but could have done well with more instruction on active model methods, particularly help with sorting

WHY DOES IT ALL WORK ON LOCALHOST BUT IS BROKEN IN HEROKU!?!

@dHelmgren
Copy link

Media Ranker

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene It looks like you tried to get this done in one day, which is not really advisable.
Comprehension questions Good
General
Rails fundamentals (RESTful routing, use of named paths) Mostly good, see comments
Views are well-organized (DRY, use of semantic HTML, use of partials) Your HTML is good, but not always doing what you want it to.
Errors are reported to the user Mostly, see comments
Business logic lives in the models Yes
Models are thoroughly tested, including relations, validations and any custom logic Mostly, your works tests could use bolstering, see comments.
Wave 1 - Media
Splash page shows the three media categories No, see comments
Basic CRUD operations on media are present and functional Mixed bag, see comments.
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
A user cannot vote for the same media more than once Yes
All media lists are ordered by vote count No
Splash page contains a media spotlight No
Wave 3 - Users and Votes
Media pages contain lists of voting users Yes
Individual user pages and the user list are present yes
Optional - Styling
Bootstrap is used appropriately yes
Look and feel is similar to the original yes
Overall Looking at the outcome, I feel like you had 2 problems during this project. 1) Overall, it seems rushed. A lot of the code doesn't seem like it got tested, and because of that several of your crud ops didn't work. 2) It seems like you put in a good amount of time on the CSS rather than making sure that your basic functionality was present/working on Heroku. The site looks good at a surface level, but I'd much prefer to see your time go to the base project before the extensions. I'm not sure what kept you from being able to complete the project, but if there's any support we can give you to make sure that you have the time or focus you need during the week, let's talk.

# get 'works/new'

resources :works
resources :users

Choose a reason for hiding this comment

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

I don't think you need all 7 restful routes for users - only index and show are needed for this project.

</tr>
</thead>
<tbody>
<% @works.each do |work| %>

Choose a reason for hiding this comment

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

Unfortunately, this doesn't account for categories.

</h2>
<ul class="list-group top-ten__list">
<% @top_works.each do |work| %>
<% if work.category == category.name %>

Choose a reason for hiding this comment

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

This might well work, but your works don't get created with categories, and it is impossible to create new works with categories.

@@ -0,0 +1,18 @@
class Vote < ApplicationRecord
belongs_to :user
belongs_to :work

Choose a reason for hiding this comment

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

You can't delete a work that has votes! This is because you've created a link between the two tables at the database level, and deleting the work would leave behind invalid votes. You might want to check out the dependent argument to has_many.

expect(valid).must_equal false
expect(other_work.errors.messages).must_include :title
end

Choose a reason for hiding this comment

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

There's a lot of interesting test cases for your custom Work methods missing here. For top_work, I would wonder:

  • What happens if there are no works?
  • What happens if there are works but no votes?
  • What happens if two works have the same number of votes?

Similarly for your category and top ten methods, I would ask:

  • What if there are no works of that category?
  • What if there are less than 10 works?
  • What if there's a tie for last place, e.g. works 9, 10 and 11 all have 0 votes?

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