Soulsearchers/India/Olga/Li/Taylor #85
Conversation
… master seed file
created merchant controller and model
Retire button added to the merchant dashboard
Ordermodeltesting
User test 2
product tests completed
bEtsyFunctional Requirements: Manual Testing
Major Learning Goals/Code Review
Code Style Bonus AwardsWas the code particularly impressive in code style for any of these reasons (or more...?)
Overall FeedbackGreat work Soul Searchers! You've built a fully functional web store from top to bottom. This represents a huge amount of work, and you should be proud of yourselves!. I am particularly impressed by the way that you controlled the user access to the different parts of the site. All of the checks were in place to ensure that only a user who has permissions to access or do something was able to! Nice work! I do see some room for improvement around improving the tests around unexpected use or incorrect use. I noticed that many of the tests ensure that valid actions are successful, and those tests are necessary and very useful, but it's also important to think through all of the disallowed use cases and ensure that those sequences fail. Here is a list of bugs that I found:
bEtsy is a huge project on a very short timeline, and this feedback should not at all diminish the magnitude of what you've accomplished. Keep up the hard work! Only the person who submitted the PR will get an email about this feedback. Please let the rest of your team know about it. |
| if user.save | ||
| flash[:success] = "Logged in as new user #{user.username}" | ||
| else | ||
| head :not_found |
There was a problem hiding this comment.
Line 24: head :note_found combined with the redirect down below on line 26 causes an error: "Render and/or redirect were called multiple times in this action. Please note that you may only call render OR redirect, and at most once per action. Also note that neither redirect nor render terminate execution of the action, so if you want to exit an action after redirecting, you need to do something like "redirect_to(...) and return". Removing this line fixes that issue.
| else | ||
| head :not_found | ||
| flash[:error] = "Could not create new user account: #{user.errors.messages}" | ||
| return redirect_to root_path |
There was a problem hiding this comment.
Stylistically, this should be two lines:
redirect_to root_path
return
| end | ||
|
|
||
| session[:user_id] = user.id | ||
| return redirect_to root_path |
| user = User.new | ||
| user.uid = auth_hash[:uid] | ||
| user.provider = "github" | ||
| user.username = auth_hash["info"]["name"] |
There was a problem hiding this comment.
This line causes the login bug that I mentioned in the feedback notes. In the auth_hash ["info"]["name"] is an optional field and can be nil. If it is nil (as mine was when I tested) this causes the validation that the username has to exist to fail. To fix this, I would suggest using auth_hash["info"]["login"] as an alternative to name, or removing the validation. It is important to check that if you are validating on something, it will always exist in a valid case.
Assignment Submission: bEtsy
Congratulations! You're submitting your assignment. Please reflect on the assignment with these questions. These should be answered by all members of your team, not by a single teammate.
Reflection