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

Mw cart style #73

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

Conversation

MyriamWD
Copy link

No description provided.

piffer59 and others added 29 commits May 9, 2019 23:16
…orderitem model, and worked on styling home page and nav bar
Mw Deleted filter by pending order status from merchant dashboard
Added Merchant model test, updated merchant dashboard product views
update gitignore file to include coverage folder and seed file
Mw Filtering out pending orders from merchant order fulfillment page in dashboard
@tildeee
Copy link

tildeee commented May 20, 2019

bEtsy

What We're Looking For

Manual testing

Workflow yes / no
Deployed to Heroku x
Before logging in
Browse all products, by category, by merchant x
Leave a review x
Verify unable to create a new product x
After logging in
Create a category x
Create a product in that category with stock 10 x
Add the product you created to your cart Project logic here says you can't shop if you're logged in!
Add it again (should update quantity) x
Verify unable to increase quantity beyond stock x
Add another merchant's product x
Check out x
Check that stock was reduced x
Change order-item's status on dashboard x
Verify unable to leave a review for your own product x
Verify unable to edit another merchant's product by manually editing URL I can do this!
Verify unable to see another merchant's dashboard by manually editing URL x

Code Review

Area yes / no
Routes
No un-needed routes generated (check reviews) x
Routes not overly-nested (check products and merchants) x
Merchant dashboard and cart page use a non-parameterized routes (should pull merchant or cart ID from session) The project relies on the route get "/merchants/dashboard/:id", to: "merchants#dashboard", as: "dashboard"
Controllers
Controller-filter to require login by default x
Helper methods or filters to find logged-in user, cart, product, etc x, but there are several places that call Order.find(session[:order_id]) so this should be refactored out (this is even called in a view!)
No excessive business logic A lot of the logic about orders and order items in the OrderItemsController could be moved out. Small bits like default values for a Review could be moved to the model
Business logic that ought to live in the model
Add / remove / update product on order Interesting model method to itemize the order items neatly :D You all are on the right track here -- there are more places that could be pulled out (like generically updating the orderitems on the order)
Checkout -> decrease inventory
Merchant's total revenue Missing... also, the Merchant class has a method to calculate total for an OrderItem...
Find all orders for this merchant (instance method on Merchant) x, well done!
Selected Model Tests
Add item to cart:
- Can add a good product
- Can't add a product w/o enough stock
- Can't add a retired product
- Can't add to an order that's not in cart mode
- Logic specific to this implementation
Has mostly the positive test cases
Get orders for this merchant:
- Includes all orders from this merchant
- Doesn't include orders from another merchant
- Orders are not included more than once
- Does something reasonable when there are no orders for this merchant
Has mostly the positive test cases
Selected Controller Tests
Add item to cart:
- Empty cart (should be created)
- Cart already exists (should add to same order)
- Product already in cart (should update quantity)
- Bad product ID, product is retired, quantity too high, or something like that (error)
Missing some
Leave a review:
- Works when not logged in
- Works when logged in as someone other than the product's merchant
- Doesn't work if logged in as this product's merchant
- Doesn't work if validations fail
x

Overall Feedback

Great 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!.

I am particularly impressed by the way that y'all:

  • did a great job with the custom business logic and moving it from the controllers into the models! Even though there's still more refactoring you can do, overall, you all did a GREAT job on this!
  • Had fantastic attention to detail: you all had really cool details, like the random item feature, and the default values for some other things were delightful to discover
  • Went all in on Bootstrap styling. Nice work

I do see some room for improvement around:

  • Utilizing some of the Active Record methods and opting to make class methods instead. For example, in Merchant, you have a method def self.show_all_products(merchant) ... return products = merchant.products ... end, which actually Merchants can do already as an instance method with merchant.products!
  • In the views I see a lot of <br> tags to make space... In the future, that would be a CSS thing ;)
  • I think overall the routes could have been given a review... It would be a nice refactoring to make the dashboard route consistent

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.

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.

5 participants