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

Earth/Fire - Anna, Christina, Genevieve, Stacy #82

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

Conversation

geneminde
Copy link

@geneminde geneminde commented Nov 26, 2020

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

Prompt Response
Each team member: what is one thing you were primarily responsible for that you're proud of? Anna: partials to dry up similar code. Christina: order fulfillment page that shows only user's items in shared order. Genevieve: I love all of my code equally. However, I did enjoy reading up on named scopes for queries, though I only implemented a simple one. Stacy: figuring out seeding and how to save for image links that could be blocked by the original site.
Each team member: what is one thing you were primarily responsible for that you would like targeted feedback on? Anna: any place to dry up code more? Some of the before_action items better ways to consolidate if any. Christina targeted feedback: where to place order_item methods regarding relationships and ActiveRecord methods (i.e. in the Controller? in the model?) Genevieve: For Order, is there anything that should be a model method that isn't? Stacy: nested and/or cascading code, often get into a loop that is hard to get out of
.
How did your team break up the work to be done? We assigned each person one or more model(s) to implement. From there we worked more closely to finalize design.
How did your team utilize git to collaborate? We created a lot of branches for different features, and we held daily (sometimes multiple times) merge-fests
What did your group do to try to keep your code DRY while many people collaborated on it? If we were to do this project over, this one area we would like to focus more on.
What was a technical challenge that you faced as a group? latency due to merging at specific times
What was a team/personal challenge that you faced as a group? It can be difficult to coordinate four people's schedules
What was your application's ERD? (upload this to Google Drive, change the share settings to viewable by everyone with a link, and include a link) https://drive.google.com/file/d/1Cqq7vXKx2YD1RVkTIsM9FaRX1uxxPXxs/view?usp=sharing
What is your Trello URL? https://trello.com/b/rn1TMUiL/betsy
What is the Heroku URL of your deployed application? https://young-reaches-98324.herokuapp.com

annakim93 and others added 30 commits November 19, 2020 22:43
… out of stock items) and set available to false if is_retired OR quantity == 0
… Order#mark_shipped method so if all order items are marked shipped, order status will change to complete.
…ems in shared orders on order fulfillment page
StacyLundquist and others added 29 commits November 25, 2020 09:24
…t exceed product quantity unless order is already paid, complete, or cancelled
S feedback check whoo hoo dropdown search by merchant!
Copy link

@tildeee tildeee left a comment

Choose a reason for hiding this comment

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

bEtsy

Functional Requirements: Manual Testing

Workflow yes / no
Before logging in
Browse all products, by category, by merchant ✔️
Leave a review ✔️
Verify unable to create a new product ✔️, redirect + flash message
After logging in
Create a category ✔️
Create a product in that category with stock 10 ✔️
Add the product you created to your cart ✔️
Add it again (should update quantity) ✔️
Verify unable to increase quantity beyond stock ✔️
Add another merchant's product ✔️
Check out ✔️
Check that stock was reduced ✔️
Change order-item's status on dashboard ✔️
Verify unable to leave a review for your own product ✔️
Verify unable to edit another merchant's product by manually editing URL ✔️
Verify unable to see another merchant's dashboard by manually editing URL ✔️

Major Learning Goals/Code Review

Criteria yes / no
90% reported coverage for all controller and model classes using SimpleCov ✔️, 95.6%!
Routes
No un-needed routes generated (check reviews) ✔️ yes in general, but reviews likely doesn't need an edit, update, or delete actions, does it?
Routes not overly-nested (check products and merchants) ✔️
Merchant dashboard and cart page use a non-parameterized routes (should pull merchant or cart ID from session) ✔️
Controllers
Controller-filter to require login is applied to all merchant-specific actions (update/add item, add category, view merchant dashboard, etc.) - filter method is not duplicated across multiple files ✔️, nice, I think it was cool that y'all leveraged this and put most/all? of them in the application controller. It ends up reading well imo
Helper methods or filters to find logged-in user, cart, product, etc ✔️, same note as above
No excessive business logic ✔️, well done, this is hard to pull off but your order items controller looks great in this regard!!
Business logic that ought to live in the model
Add / remove / update product on order ✔️, your code is reasonable about this, but if there's something to keep refactoring: In the OrderItemsController, even lines like Order.create() or order.order_items << order_item expose/show the controller some details about the Order and OrderItem model's internals (such as "order.order_items is an array"). The next step of refactoring would be to make these model methods (no matter how small! But this will make it so when we read the code, the controller doesn't have to know that order_items is an array, and we could have more generic/expressive lines like order.add(order_item)
Checkout -> decrease inventory ✔️
Merchant's total revenue ✔️, nice! Not wrong, but to be honest, I might've expected this to be an instance method on the User model. (the feature is to find the total revenue of all orders based around one instance of merchant). Just a small thought!
Find all orders for this merchant (instance method on Merchant) ✔️, you all approached this as a scope on Order. As the rubric states, I might've expected this as an instance method on Merchant, but this scope is pretty cool!
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
because of the nature of how y'all implemented this, I don't think there are model tests
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
because of the nature of how y'all implemented this, I don't think there are tests?
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)
✔️, I didn't see all of these cases, but overall, I see this functionality and I see test coverage around it
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
✔️

Code Style Bonus Awards

Was the code particularly impressive in code style for any of these reasons (or more...?)

Quality Yes?
Perfect Indentation ✅ from the code I read, there's perfect indentation, which is a huge feat for a group project! hahaha
Elegant/Clever
Descriptive/Readable
Concise
Logical/Organized

Overall Feedback

GREAT WORK, team!! 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!

Y'all did a great job; one overall theme that I want to call out is that y'all broke out of the Ada patterns to make the code do what you wanted to do best, and I'm so happy with that. I'm so glad you felt ownership and used your own flash keys, imagined your own helper methods, and created so many good controller filters, partials, and other things for this project. Y'all applied some great concepts into ways that was meaningful for your project; I hope y'all always keep that spirit up when you can!

Y'alls code style overall is great; it felt very confident and clean throughout. I also want to shout out that the entire feature of order fulfillment (logic, views, etc) was a delight to read through.

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