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

Carets - Guillermina #23

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

Carets - Guillermina #23

wants to merge 93 commits into from

Conversation

murog
Copy link

@murog murog 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 method for the Work model that was able to determine a spotlight work. It did so by checking the length of the votes array for works across each type of category.
Describe how you approached testing that model method. What edge cases did you come up with? I made sure that the spotlight method raised errors if attempted to be used with hashes that did not contain works. I originally had the method set up so that it would return a spotlight without an argument, but I changed it so that it could be more easily tested.
Describe an edge case test you wrote for a controller It should render a 404 page if a user attempts to delete a work that does not exist.
What are session and flash? What is the difference between them? They are both temporary hash-like objects. Flash stores information that you could present to the user to inform them of successes and errors. Flash notices only exist through the following request. Sessions are stored in a browser's cookies and exist for as long as the browser window is open.
Describe a controller filter you wrote. In the application controller, before any action is acted upon, the application searches to see if a user is logged in. If they are logged in, that user is assigned to an instance variable.
What was one thing that you gained more clarity on through this assignment? Controller testing! Building the application became much more manageable once returning to the pseudocode-red-green-refactor cycle.
What is the Heroku URL of your deployed application https://gm-media-ranker.herokuapp.com/ (However, I am getting a 503 status code at the moment...)
Do you have any recommendations on how we could improve this project for the next cohort? I believe this one would have been more appropriate as a pair project, as opposed to ride-share.

@CheezItMan
Copy link

Media Ranker

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene Good number of commit and commit messages
Comprehension questions Check, I agree the compressed week made this individual project more difficult.
General
Rails fundamentals (RESTful routing, use of named paths) Good RESTful routes
Semantic HTML Good Semantic HTML
Errors are reported to the user Check, although now I see that you're using OAuth, I can't check trying to delete a work without logging in, or upvoting
Business logic lives in the models You've got some, but I think you should have sorting works in categories by their number of votes in the Model.
Models are thoroughly tested, including relations, validations and any custom logic You've got tests, but some are failing (2). I also left some notes in your code.
Controllers are thoroughly tested You need to include status codes in some of your controller tests. Also some more edge-case testing, but overall pretty good. I left some notes in your code. However I had 17 tests failing and 15 errors... Lots of things like unknown paths, and incorrect status codes etc. That seems to be because you're adding OAuth to your project locally. Probably should get your tests to pass in a branch before merging it into master.
Wave 1 - Media
Splash page shows the three media categories Check
Basic CRUD operations on media are present and functional Check
Wave 2 - Users and Votes
Users can log in and log out Check
The ID of the current user is stored in the session Check
Individual user pages and the user list are present Check
A user cannot vote for the same media more than once Check
All media lists are ordered by vote count Check
Splash page contains a media spotlight Check
Media pages contain lists of voting users Check
Wave 3 - Styling
Foundation is used appropriately Nicely done
Look and feel is similar to the original Check
Overall Well done. It seems you've added OAuth to your repo which made evaluating your tests difficult. I left some notes in your code. You hit most of the requirements with minor criticisms.

output.must_be_instance_of Hash
end

it "should return values that are arrays" do

Choose a reason for hiding this comment

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

I would also test that they are in the proper order...

validates :title, presence: true
validates :title, uniqueness: true

def self.all_works_by_title

Choose a reason for hiding this comment

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

Shouldn't this be by the # of votes?

let(:work) { Work.new }
let(:vote_1) { votes(:vote_1)}

it "must have a valid work and user to be valid" do

Choose a reason for hiding this comment

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

I would also add a test to verify that two votes can't have the same work_id and user_id.

So a user can't vote for the same work twice.

describe MainPageController do
it "should get index" do
get main_page_index_url
value(response).must_be :success?

Choose a reason for hiding this comment

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

you could instead do:

must_respond_with :success

post login_path, params: {username: "alyssa"}
}.must_change 'User.count', 1
must_respond_with :redirect
must_redirect_to root_path

Choose a reason for hiding this comment

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

I would also test:

id = find_by(username: 'alyssa').id
id.must_equal session[:user_id]

proc {
post login_path, params: {username: nil}
}.must_change 'User.count', 0
flash[:error].must_equal "User not logged in successfully"

Choose a reason for hiding this comment

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

Please use status codes.


describe SessionsController do

it "going to the login_form page is successful" do

Choose a reason for hiding this comment

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

Fix AUTOINDENT!

}.must_change 'Vote.count', 0
must_respond_with :redirect
must_redirect_to work_path(works(:anti).id)
flash[:error].must_equal "You've already upvoted this!"

Choose a reason for hiding this comment

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

I would also have a test voting for a work that doesn't exist, or voting if not logged in.

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