Stacy's Media Ranker submission by StacyLundquist · Pull Request #45 · Ada-C14/media-ranker

StacyLundquist · 2020-11-16T20:05:31Z

Media Ranker

Congratulations! You're submitting your assignment!

Reflection//Comprehension Questions

Question	Answer
Describe a custom model method you wrote.	In the work model I created a (currently non-working) method to change the vote button to a remove vote if the person had already voted.
Describe how you approached testing that model method. What edge cases did you come up with?	The idea was to test what happens when the user isn't logged in, or when the vote is nil.
What are `session` and `flash`? What is the difference between them?	Session is the browsing session, and is connected to a particular user. Flash is a message that lasts for a cycle.
What was one thing that you gained more clarity on through this assignment?	Ummmm, I gained more confusion of a lot of things! I gained clarity on exactly how much coffee I can drink in a day and how to generate models and controllers.
What is the Heroku URL of your deployed application?	http://adiec14-media-ranker.herokuapp.com/

Assignment Submission: Media Ranker

Congratulations! You're submitting your assignment. Please reflect on the assignment with these questions.

…ly?)

CheezItMan

Media Ranker

Functional Requirements: Manual Testing

Criteria	yes/no
Before logging in	--
1. On index page, there are at most 10 pieces of media on three lists, and a Media Spotlight	✔️, mostly, you don't have a movie category. I'm uncertain as to why.
2. Can go into a work's show page	✔️
3. Verify unable to vote on a work, and get a flash message	✔️, I just can't vote if I'm not logged in.
4. Can edit this work successfully, and get a flash message	✔️
5. Can go to "View all media" page and see three lists of works, sorted by vote	✔️, I can see the 2 categories you have
6. 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	✔️, you have front-end validations, nice, but I can't test the flash notice. See my works controller notes.
7. Can create a new work successfully. Note the URL for this work's show page	✔️
8. Can delete this work successfully	⚠️ I can't delete a work!
9. 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)	✔️
10. Verify that the "View all users" page lists no users	✔️
Log in	--
11. Logging in with a valid name changes the UI to "Logged in as" and "Logout" buttons	✔️
12. Your username is listed in "View all users" page	✔️
13. Verify that number of votes determines the Media Spotlight	✔️
14. Voting on several different pieces of media affects the "Votes" tables shown in the work's show page and the user's show page	✔️
15. Voting on the same work twice produces an error and flash message, and there is no extra vote	✔️, you don't present the vote button, but you have the validation as well.
Log out	--
16. Logging out showed a flash message and changed the UI	✔️
17. Logging in as a new user creates a new user	✔️
18. Logging in as an already existing user has a specific flash message	✔️

Major Learning Goals/Code Review

Criteria	yes/no
1. Sees the full development cycle including deployment, and the app is deployed to Heroku	✔️
2. Practices full-stack development and fulfilling story requirements: the styling, look, and feel of the app is similar to the original Media Ranker	⚠️ Not quite there although you have a good start on it
3. Practices git with at least 25 small commits and meaningful commit messages	✔️

Previous Rails learning, Building Complex Model Logic, DRYing up Rails Code

Criteria	yes/no
4. Routes file uses `resources` for works	✔️
5. Routes file shows intention in limiting routes for voting, log-in functionality, and users	✔️, one minor note
6. The homepage view, all media view, and new works view use semantic HTML	✔️
7. The homepage view, all media view, and new works view use partials when appropriate	⚠️, you have partials for forms but no others. You can use partials to dry up the repeated lists of works you have in several views.
8. The model for media (likely named `work.rb`) `has_many` votes	✔️
9. 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	✔️, You have it, but you waaay overcomplicated the methods required.
10. Some controller, likely the `ApplicationController`, has a controller filter for finding a logged in user	✔️
11. Some controller, likely the `WorksController`, has a controller filter for finding a work	✔️
12. The `WorksController` uses strong params	✔️
13. The `WorksController`'s code style is clean, and focused on working with requests, responses, `params`, `session`, `flash`	⚠️ Some improvement in the error handling and use of flash, and the destroy action.

Testing Rails Apps

Criteria	yes/no
14. There are valid fixtures files used for users, votes, and works	⚠️
15. User model has tests with sections on validations (valid and invalid) and relationships (has votes)	⚠️, no real tests
16. Vote model has tests with sections on validations (valid and invalid) and relationships (belongs to a user, belongs to a vote)	⚠️ Very little testing
17. Work model has tests with sections on validations (valid and invalid) and relationships (has votes)	⚠, Some errors here especially around validations
18. Work model has tests with a section on all business logic methods in the model, including their edge cases	⚠️ Missing tests

Overall Feedback

Overall Feedback	Criteria	yes/no
Red (Not at Standard)	0-10 in Code Review or 0-11 in Functional Reqs, or assignment is breaking/doesn’t run with less than 5 minutes of debugging, or the instructor judges that this project needs special attention	♥️

Code Style Bonus Awards

Was the code particularly impressive in code style for any of these reasons (or more...?)

Quality	Yes?
Perfect Indentation	✅
Elegant/Clever	✅

Summary

Yes, I'm labeling this Red, but you are closer than you think to Green. Most of your issues relate to testing. I've left extensive comments in the code. Take a look. You have a lot of the functionality in place.

Areas to improve on include:

Writing Validation and custom method tests for models
Simplifying your custom model methods. You overcomplicated your life with the work.rb file. See my inline notes here.
Practice debugging in Rails and asking for help. I suspect your sharp brain let you do more with less help than other students in regular Ruby, but Rails is a bit bigger and you seemed to have run into a bug and moved on without calling out for help. I'm happy to sit with you and review this project at some point and practice some debugging skills.

As time allows doing practice with controller tests as well.

Areas you did well:

Controller filters! Awesome! Well done.
Complicated validations, really clever ones
Front-end Validations
Lots of clever front-end things like hiding the vote button when I can't vote.
Pretty clean routes.rb file.

Due to the rubric this is a red, but you're actually pretty close to Green. If testing had been done, I think it would have been Green.

CheezItMan · 2020-11-20T02:31:01Z

+  end
+
+  describe "validations" do
+    let work = works(:test_book)


This isn't how a let is written

Suggested change

let work = works(:test_book)

let (:work) { works(:test_book) }

CheezItMan · 2020-11-20T02:32:42Z

+    it "can get the new user" do
+
+      # Act
+      get new_user_path


This test is failing because in routes.rb you don't have the route given a nickname.

CheezItMan · 2020-11-20T02:33:05Z

+  end
+
+  resources :users, only: [:index, :show]
+  get '/signup', to: 'users#new'


Suggested change

get '/signup', to: 'users#new'

get '/signup', to: 'users#new', as: "new_user"

CheezItMan · 2020-11-20T02:34:53Z

+  describe "show" do
+    it "can get a valid user" do
+          # Act
+          get user_path(first.id)


Missing The model class

Suggested change

get user_path(first.id)

get user_path(User.first.id)

CheezItMan · 2020-11-20T02:36:07Z

+  end
+
+  def show
+    @user = User.find_by(id: params[:id])


You need to do something if the user doesn't exist. Otherwise @user.votes is nil.

Suggested change

@user = User.find_by(id: params[:id])

@user = User.find_by(id: params[:id])

if @user.nil?

redirect_to root_path

flash[:error] = "User not found"

return

end

CheezItMan · 2020-11-20T03:12:58Z

+  before_action :find_work, only: [:show, :edit, :update, :destroy]
+  before_action :find_work_votes, only: :show


Awesome! Filters!

CheezItMan · 2020-11-20T03:14:35Z

+      flash.now[:error] = "Work edits not saved!"
+      @work.errors.messages.each do |field, messages|
+        flash[field] = messages
+      end
+      render :edit
+    end


If you're rendering use flash.now Also don't forget the status code!

Suggested change

flash.now[:error] = "Work edits not saved!"

@work.errors.messages.each do |field, messages|

flash[field] = messages

end

render :edit

end

flash.now[:error] = "Work edits not saved!"

@work.errors.messages.each do |field, messages|

flash.now[field] = messages

end

render :edit, status: :bad_request

end

CheezItMan · 2020-11-20T03:15:29Z

+      flash[:error] = "Work not created!"
+      @work.errors.messages.each do |field, messages|
+        flash[field] = messages
+      end
+      redirect_to new_work_path
+    end


You should instead render the new view.

Suggested change

flash[:error] = "Work not created!"

@work.errors.messages.each do |field, messages|

flash[field] = messages

end

redirect_to new_work_path

end

flash.now[:error] = "Work not created!"

@work.errors.messages.each do |field, messages|

flash.now[field] = messages

end

render :new, status: :bad_request

end

CheezItMan · 2020-11-20T03:21:14Z

+    if @work.nil?
+      @deleted_work = @work.destroy
+      flash[:success] = "#{@work.title} deleted"
+      redirect_to root_path
+    else
+      flash[:failure] = "Work #{@work.title} not destroyed."
+      redirect_back fallback_location: root_path
+    end


I think you want to destroy a work if it's not nil.

Suggested change

if @work.nil?

@deleted_work = @work.destroy

flash[:success] = "#{@work.title} deleted"

redirect_to root_path

else

flash[:failure] = "Work #{@work.title} not destroyed."

redirect_back fallback_location: root_path

end

unless @work.nil?

@deleted_work = @work.destroy

flash[:success] = "#{@work.title} deleted"

redirect_to root_path

else

flash[:failure] = "Work #{params[:id]} not destroyed."

redirect_back fallback_location: root_path

end

CheezItMan · 2020-11-20T03:40:35Z

+  root to: 'works#home'
+
+  resources :works do
+    resources :votes, only: [:create, :destroy]


I don't know that you need to nest destroy

CheezItMan

Media Ranker

Functional Requirements: Manual Testing

Criteria	yes/no
Before logging in	--
1. On index page, there are at most 10 pieces of media on three lists, and a Media Spotlight	✔️
2. Can go into a work's show page	✔️
3. Verify unable to vote on a work, and get a flash message	✔️
4. Can edit this work successfully, and get a flash message	✔️
5. Can go to "View all media" page and see three lists of works, sorted by vote	✔️
6. 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	✔️
7. Can create a new work successfully. Note the URL for this work's show page	✔️
8. Can delete this work successfully	✔️
9. 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)	✔️
10. Verify that the "View all users" page lists no users	✔️
Log in	--
11. Logging in with a valid name changes the UI to "Logged in as" and "Logout" buttons	✔️
12. Your username is listed in "View all users" page	✔️
13. Verify that number of votes determines the Media Spotlight	✔️
14. Voting on several different pieces of media affects the "Votes" tables shown in the work's show page and the user's show page	✔️
15. Voting on the same work twice produces an error and flash message, and there is no extra vote	✔️, you don't present the vote button, but you have the validation as well.
Log out	--
16. Logging out showed a flash message and changed the UI	✔️
17. Logging in as a new user creates a new user	✔️
18. Logging in as an already existing user has a specific flash message	✔️

Major Learning Goals/Code Review

Criteria	yes/no
1. Sees the full development cycle including deployment, and the app is deployed to Heroku	✔️
2. Practices full-stack development and fulfilling story requirements: the styling, look, and feel of the app is similar to the original Media Ranker	✔️
3. Practices git with at least 25 small commits and meaningful commit messages	✔️

Previous Rails learning, Building Complex Model Logic, DRYing up Rails Code

Criteria	yes/no
4. Routes file uses `resources` for works	✔️
5. Routes file shows intention in limiting routes for voting, log-in functionality, and users	✔️, one minor note
6. The homepage view, all media view, and new works view use semantic HTML	✔️
7. The homepage view, all media view, and new works view use partials when appropriate	⚠️, you have partials for forms but no others. You can use partials to dry up the repeated lists of works you have in several views.
8. The model for media (likely named `work.rb`) `has_many` votes	✔️
9. 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	✔️, You have it, but you waaay overcomplicated the methods required.
10. Some controller, likely the `ApplicationController`, has a controller filter for finding a logged in user	✔️
11. Some controller, likely the `WorksController`, has a controller filter for finding a work	✔️
12. The `WorksController` uses strong params	✔️
13. The `WorksController`'s code style is clean, and focused on working with requests, responses, `params`, `session`, `flash`	⚠️ Some improvement in the error handling and use of flash, and the destroy action.

Testing Rails Apps

Criteria	yes/no
14. There are valid fixtures files used for users, votes, and works	✔️
15. User model has tests with sections on validations (valid and invalid) and relationships (has votes)	✔️
16. Vote model has tests with sections on validations (valid and invalid) and relationships (belongs to a user, belongs to a vote)	✔️
17. Work model has tests with sections on validations (valid and invalid) and relationships (has votes)	✔️
18. Work model has tests with a section on all business logic methods in the model, including their edge cases	⚠️ Missing tests

Overall Feedback

Overall Feedback	Criteria	yes/no
Green (Meets/Exceeds Standards)	14+ in Functional Requirements: Manual Testing && 14+ in Code Review	💚

Code Style Bonus Awards

Was the code particularly impressive in code style for any of these reasons (or more...?)

Quality	Yes?
Perfect Indentation	✅
Elegant/Clever	✅

Summary

This is green and it mostly works. You do have a broken user show path, but that's minor. I've also noted that your controller tests are a bit of a mess and tried to give some feedback there even though it wasn't in the assignment. It's good that you attempted them, but I would get one test working before moving to the next. Otherwise things tend to build like your last commit notes.

As I noted last time, I think you overthought some things about the project, particularly the upvoting and downvoting logic you were working on. Keep it simple.

CheezItMan · 2020-12-01T20:28:45Z

@@ -1 +1 @@
-VALID_CATEGORIES = %w[album book].freeze
+VALID_CATEGORIES = %w[album book movie].freeze


CheezItMan · 2020-12-01T20:29:23Z

    end
-  end

+    describe "current" do


What about users who are not logged in?

CheezItMan · 2020-12-01T20:33:55Z

+    login_data = {
+        user: {
+            username: user.name,
+        },
+    }


Notice that the form you use to log in doesn't nest username under a user hash. Also in your controller you are using "name" as the key and not "username".

So this should be:

Suggested change

login_data = {

user: {

username: user.name,

},

}

login_data = {

name: user.name,

}

CheezItMan · 2020-12-01T20:40:55Z

+
+  get '/signup', to: 'users#new'
+
+  get "/users/current", to: "users#current", as: "current_user"


You don't have this controller method.

CheezItMan · 2020-12-01T20:41:39Z

+        perform_login(users(:second))
+
+        # Act
+        get current_user_path


This crashes because you don't have a current method in the users controller.

CheezItMan · 2020-12-01T20:49:25Z

+        post work_votes_path(work)
+      }.must_differ 'work.votes', 1
+
+      vote = Vote.find_by(user_id: user.id)


Since you're creating a new user you'll also need to find them again in the DB since now they have an id number

Suggested change

vote = Vote.find_by(user_id: user.id)

user = User.find_by(name: user.name)

vote = Vote.find_by(user_id: user.id)

CheezItMan · 2020-12-01T20:51:00Z

+      expect {
+        delete work_vote_path(work_id: work_hash, vote_id: 1)
+      }.wont_differ 'work.votes'


There is no work object here or vote for that matter and so it can't have votes. This test doesn't make much sense.

CheezItMan · 2020-12-01T20:53:39Z

+    end
+  end
+
+  describe 'destroy' do


For the destroy route, I don't think you need to use a nested route since you can identify the vote by it's id number.

CheezItMan · 2020-12-01T20:57:24Z

+      expect {
+        delete work_vote_path(work_id: work, vote_id: vote)
+      }.must_differ 'work.votes', -1


Suggested change

expect {

delete work_vote_path(work_id: work, vote_id: vote)

}.must_differ 'work.votes', -1

expect {

delete work_vote_path(work_id: work, id: vote)

}.must_differ 'work.votes.count', -1

CheezItMan · 2020-12-01T20:59:25Z

+    redirect_to work_path(params[:work_id])
+  end
+
+  def destroy


I don't think you need a destroy or "downvote" method.

I think you've overcomplicated this quite a bit. If you want to give them the abiltiy to downvote, you can check to see if the current user and work have a vote, if so remove it, otherwise add a vote.

StacyLundquist added 23 commits November 12, 2020 16:58

Initial Rails Setup

fab24b5

Set up controllers and homepage

fdfc6db

changed homepage

975d47d

CSV file and seed file

47f589f

created models, seeded file

e070d2f

Works_controller && work.rb

a418a2b

added votes, updated users_controller, user.rb

78ca4b7

more user_controller, user.rb

a2a2e86

works_controller, work.rb update

c136a62

updated votes_controller and vote.rb to include helper.

43a5022

connected work and votes. localhost running again, yay

d0d9a90

basic homepage styling

6164380

barebone tests, all around

73899e7

added fixtures

771ae5e

vote model test

86a8c46

additional tests

55dc31b

homepage, styling, partial form, index pages

a9ad81d

erb file framework, views#user/show, connected votes to user

c71604e

works show, added not found files (is it possible to have one file on…

c7b1f05

…ly?)

new and edit html.erb works, users

3ddb634

added sessions, trying to fix view all media bug

f64c815

all_works changes

bac87f5

fixed find_work in works_controller

1772edb

CheezItMan reviewed Nov 20, 2020

View reviewed changes

StacyLundquist added 4 commits November 28, 2020 22:43

multiple, so many tests

ea5ef56

styling to mimic website

6137114

fixed one, a couple others went wonky

3109b77

yikes, am I making it worse?

28c1e63

CheezItMan reviewed Dec 1, 2020

View reviewed changes

	let work = works(:test_book)
	let (:work) { works(:test_book) }

	get '/signup', to: 'users#new'
	get '/signup', to: 'users#new', as: "new_user"

		before_action :find_work, only: [:show, :edit, :update, :destroy]
		before_action :find_work_votes, only: :show

		@@ -1 +1 @@
		VALID_CATEGORIES = %w[album book].freeze
		VALID_CATEGORIES = %w[album book movie].freeze


		get '/signup', to: 'users#new'

		get "/users/current", to: "users#current", as: "current_user"

	vote = Vote.find_by(user_id: user.id)
	user = User.find_by(name: user.name)
	vote = Vote.find_by(user_id: user.id)

Uh oh!

Conversation

StacyLundquist commented Nov 16, 2020

Media Ranker

Reflection//Comprehension Questions

Assignment Submission: Media Ranker

Uh oh!

CheezItMan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Media Ranker

Functional Requirements: Manual Testing

Major Learning Goals/Code Review

Previous Rails learning, Building Complex Model Logic, DRYing up Rails Code

Testing Rails Apps

Overall Feedback

Code Style Bonus Awards

Summary

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CheezItMan Nov 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CheezItMan left a comment

Choose a reason for hiding this comment

Media Ranker

Functional Requirements: Manual Testing

Major Learning Goals/Code Review

Previous Rails learning, Building Complex Model Logic, DRYing up Rails Code

Testing Rails Apps

Overall Feedback

Overall Feedback

Code Style Bonus Awards

Summary

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

CheezItMan left a comment •

edited

Loading

CheezItMan Nov 20, 2020 •

edited

Loading