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

Leaves - Georgina #37

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

Leaves - Georgina #37

wants to merge 78 commits into from

Conversation

geomsb
Copy link

@geomsb geomsb commented Oct 21, 2019

Media Ranker

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
Describe a custom model method you wrote. I wrote a self method named spotlight. The method looks up for all the votes, then makes groups using the work_id, then counts all the votes for each work_id, after that it uses an enumerator, max_by, in order to get the work with more votes.
Describe how you approached testing that model method. What edge cases did you come up with? I created some works, users, and votes and I tested if the method returned the work with more votes based on what I defined at my test. About the edge case, the method returns nil if there are no votes.
What are session and flash? What is the difference between them? They are very similar, are used to to send messages to the views. We use flash for success, errors or failures at the controllers. We use session to keep track of data throughout a users "session", which normally ends when they close their browser.
What was one thing that you gained more clarity on through this assignment? routes and paths
What is the Heroku URL of your deployed application? https://georgina-media.herokuapp.com/
Do you have any recommendations on how we could improve this project for the next cohort? It's a lot of repeating work. Maybe a shorter version with fewer pages would be better.

@beccaelenzil
Copy link

Media Ranker

What We're Looking For

Manual Testing

Workflow yes / no
Deployed to Heroku yes
Optional: Look and feel is similar to the original (consider styling and layout) yes
Before logging in
On index page, there are at most 10 pieces of media on three lists, and a Media Spotlight
Can go into a work's show page Somehow your lists got cut short.
Verify unable to vote on a work, and get a flash message yes
Can edit this work successfully, and get a flash message yes -- but the flash message covers the header for the page. seems like a css issue -- also edit redirects to list of works. redirecting the show page would make more sense.
Can go to "View all media" page and see three lists of works, sorted by vote One cannot examine if these were sorted by vote because vote is not shown on this page.
Verify unable to create a new work when the form is empty, and details about the validation errors are visible to the user through a flash message yes
Can create a new work successfully. Note the URL for this work's show page yes
Can delete this work successfully yes
Going back to the URL of this deleted work's show page produces a 404 or some redirect behavior (and does not try to produce a broken view) yes
Verify that the "View all users" page lists no users (except what is produced by db seeding) yes
Log in
Logging in with a valid name changes the UI to "Logged in as" and "Logout" buttons yes
Your username is listed in "View all users" page yes
Verify that number of votes determines the Media Spotlight yes
Voting on several different pieces of media affects the "Votes" tables shown in the work's show page and the user's show page yes
Voting on the same work twice produces an error/flash message, and there is no extra vote yes
Log out
Logging out showed a flash message and changed the UI yes
Logging in as a new user creates a new user yes
Logging in as an already existing user has a specific flash message yes

Targeted Code Review

Area yes / no
git commits were small and atomic, with useful messages yes
Routes file uses resources for works yes
Routes file shows intention in limiting routes for voting, log-in functionality, and users yes
The homepage view, all media view, and new works view use semantic HTML yes
The homepage view, all media view, and new works view use partials when appropriate see comment
The model for media (likely named work.rb) has_many votes yes
The model for media has methods to describe business logic, specifically for top ten and top media, possibly also for getting works by some category I see you implemented top ten and top media in the vote model. These could have been more easily implemented in work.rb
Some controller, likely the ApplicationController, has a controller filter for finding a logged in user Finding a logged in user is good functionality to put in a controller filter because it may be used multiple places in the future.
Some controller, likely the WorksController, has a controller filter for finding a work
The WorksController uses strong params no
The WorksController's code style is clean, and focused on working with requests, responses, params, session, flash yes

Targed Test Review

Area yes / no
There are valid fixtures files used for users, votes, and works consider using votes fixtures in your tests. For example:
vote1:
   user: user1
   work: work1

User model has tests with sections on validations (valid and invalid) and relationships (has votes) | yes
Vote model has tests with sections on validations (valid and invalid) and relationships (belongs to a user, belongs to a vote) | yes
Work model has tests with sections on validations (valid and invalid) and relationships (has votes) | yes
Work model has tests with a section on all business logic methods in the model, including their edge cases |yes

Overall Feedback

Good work completing a full stack with complex relationship and actions and thoroughly tested models. A few things to focus on for next time: consider what model business logic should belong to. In this case, top_ten and spotlight would have been simpler to implement in work.rb. Also, make sure to consider the redirect behavior that makes the most sense for each action. There seems to be one central bug: the top media page does not show the top ten for each work. Take a look and let me know if you have questions.

</tr>
</thead>
<tbody>
<% Work.albums_work.each do |work| %>

Choose a reason for hiding this comment

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

You should use a partial view or iteration to DRY up your code.

end

if @work.update(
category: params[:work][:category],

Choose a reason for hiding this comment

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

could you use your strong params here?

geomsb and others added 2 commits November 6, 2019 10:16
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