-
Notifications
You must be signed in to change notification settings - Fork 13
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
validation test #79
base: master
Are you sure you want to change the base?
validation test #79
Conversation
Cgseedfixture
Removed binding.pry
relationships to nicknames fix in fixtures
fixes fixture relation syntax
Order fulfillment (without tests)
Order inspection
add to cart from product index
Mk/review form layout
Fixed catgeory display on add product form
Vendor facing style
Gitignore cov folder
Updated prod controller tests
bEtsyWhat We're Looking ForManual testing
Code Review
Overall FeedbackGreat work overall! 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! There were some places where I was surprised by the code style. Some of this code style ranged from code that looked a lot like what we learned in classroom, to some things I've never seen before (cool custom validations!), to also some dead code. Overall, my biggest concerns that I have this project are:
You all have a lot of test weirdness (missing tests, broken tests, test comments that point out something weird going on), and I appreciate the awareness about it! I actually won't spend time on helping debug these tests, but I'll point out an observation for the future: My biggest concern is when tests are confusing. Tests that are confusing and point to a different file that's non-obvious, or something similar (I'm thinking things like "the tests for this functionality are in another controller test"), it usually points to a lot of Spaghetti Code that is all tangled up. Not much to do about this now besides reflect and look forward to another, larger project :) (In industry, I think this kind of observation would mean talking to a Tech Lead and insisting that time and strategy needs to be made for a major refactoring) I do like some of the details that I saw in flight. None of these were required, though. That being said, I noticed nice interesting logic and UX things like:
Overall, I see this as a successful project, where the functionality for all the intense features (logging in, dashboard, products, categories, users) are there. However, there are just enough tiny gaps (reviews, cohesive UX) and big ones (tests) that show areas for improvement. 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. |
bEtsy
Congratulations! You're submitting your assignment! These comprehension questions should be answered by all members of your team, not by a single teammate.
Comprehension Questions