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

Branches - Amal, Angele, Dora, Julia #97

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

Conversation

dora1405
Copy link

@dora1405 dora1405 commented Nov 2, 2019

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

Question Answer
Each team member: what is one thing you were primarily responsible for that you're proud of? Amal: the product show page; Angele: Browsing products by category and making extra categories show up in a dropdown list; Dora: OAuth signin/signout; Julia: the cart and adjusting the stock and retired status of products (also css which I now feel more comfortable with)
Each team member: what is one thing you were primarily responsible for that you would like targeted feedback on? Amal: if we should have used the money gem; Angele: determine_wizard controller filter and how it's used for authorization, or drying up the form to use it for both new and edit (wasn't able to do) ; Dora: OAuth login and why it didn't work as well once on heroku (Jared knows the issue); Julia: wizard dashboard
How did your team break up the work to be done? Set up database, seeds and migrations together to match team's ERD design. Each user story was made into a Trello card, which was sectioned into Waves. Team members can then assign themselves to the card they want to/are working on as long as it's the wave we are currently working on. Tough ones are done with pair programming.
How did your team utilize git to collaborate? Used git branching, merging and fetching. Submitting PRs for review and getting approval before it can be merged onto origin master.
What did your group do to try to keep your code DRY while many people collaborated on it? Communication helped with this, so we didn't have the issue and prevented overlap, etc. We were all coding with/next to each other so communication is very high within our group.
What was a technical challenge that you faced as a group? We had a couple of challenges with heroku; mainly logging in and having to move a gem to a different category in the Gemfile.
What was a team/personal challenge that you faced as a group? Team challenge: getting comfortable with git (git stash, fetch, merge, etc).
What was your application's ERD? (include a link) https://www.lucidchart.com/documents/view/9cd206a3-0a07-43a8-b896-41d0c16d0d33/0_0
What is your Trello URL? https://trello.com/b/dl5ykI7O/betsy-lumos
What is the Heroku URL of your deployed application? http://shop-lumos.herokuapp.com/

geli-gel and others added 30 commits October 24, 2019 15:19
…d updated methods to be more easily testable
…ed with migration to move retired from order to product
Added hyperlinks for homepage and link style
Updated retired to correct model with migrations
geli-gel and others added 29 commits October 31, 2019 17:48
 update product stock validation to be greater than zero
fixed css for forms and carousel
Retire (merged with overall style, that one should be first)
@jmaddox19
Copy link

Team 3 bEtsy WIP

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 I don't see how
Create a product in that category with stock 10 x
Add the product you created to your cart x
Add it again (should update quantity) No, added it as a new item
Verify unable to increase quantity beyond stock No, added it as a new item, even when out of stock
Add another merchant's product x
Check out Works in general. However, when I failed to checkout because I added too many of an item, the stock still went down and the order for the one that was in stock is placed.
Check that stock was reduced Yes, even when checkout fails, as noted in the above row
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 x
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) yes! Great job with the routes!
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)
Card does not use parameterized routes but the merchant dashboard does. It would be an improvement to adjust that.
Controllers
Controller-filter to require login by default No, not sure why but it is used for products
Helper methods or filters to find logged-in user, cart, product, etc x
No excessive business logic In general, it looks pretty good. The OrderItemsController create method stands out to me as an opportunity to refactor and figure out how to pull some logic into the model but I see how that would be difficult to do. Creating this separation likely would've made it easier/cleaner to preform logic like making sure an item that is already in the cart is not added as a duplicate.
Business logic that ought to live in the model
Add / remove / update product on order Similar to what I mentioned above, a lot of this logic is currently exclusively in the OrderItemsController. It'd be good to pull some of it out into the model.
Merchant's total revenue yes :)
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
The tests that are there are in the OrderItemsController but several of these tests are missing.
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
Only one test here. More edge case testing would be important in a production environment.
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)
The "Product already in cart (should update quantity)" test is the one what would've caught the issue I had. More edge case testing around retired product, etc. is also missing.
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 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 injected lots of your own fun creativity into this project and coordinated so well together on a whole team with 4 members pushing to the same repo, especially given it's y'all's first time! Also y'all's route organization is great!

I do see some room for improvement around more thorough edge case testing (both manual testing and unit testing) and putting a bit more logic into the model. (Though I will say that in general it seems like y'all made a good habit of that.)

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