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

Liv, Hannah, Corinna - Time&Space #30

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

Conversation

mulhoo
Copy link

@mulhoo mulhoo commented Jun 19, 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? LIV: I'm really proud of the testing. Testing was something I dreaded doing with every project, but was the primary thing I did on this one. I definitely feel a log more comfortable with that now.
CORINNA: using session to store our cart, including updating quantities and eventually creating an order.
HANNAH: Deployment. This time, I deployed early and often (every day). Especially, implementing OAuth on Heroku is something I learned and I’m proud of!
Each team member: what is one thing you were primarily responsible for that you would like targeted feedback on? LIV: the setup in the test helper, and where throughout the tests I could further utilize it to make my test files DRY-er.
CORINNA: Places where I could use additional test helpers or move methods to make the code cleaner and more efficient. I struggled to use things like before_action because my params often changed from ['id'] to ['format'] so accommodating for that, etc.
HANNAH: products_contoller & model! I mainly worked on products related controllers like ‘reviews_controller’ and ‘category_controller’. I made sure to use before_action, around_action to make my code DRY.
How did your team break up the work to be done? Once the first wave of major deliverables was complete, we broke off features one at a time, using Trello to keep track of who was working on what, to make the rest of the build more manageable. Corinna and Hannah handled the views and a lot of the controller stuff (ass well as CSS), as well as the products/users models. Corinna also did the ERD. Liv did initial setup, testing and other model stuff, also some work in order/order items controller. Hannah mainly focused on products, categories, reviews controllers and models and their relationship. Also, she debugged testings to improve test coverage and worked on deployment.
How did your team utilize git to collaborate? So many branches. Branches on branches. A forrest worth of branches.
What did your group do to try to keep your code DRY while many people collaborated on it? commenting out code that seemed extraneous or wordy with new code and clearing the remaining at the end? Because we knew the code worked, we were just cleaning. Also just communicating via slack about what we were doing. We added a method that mostly used every controller in application_controller, and used before_action and around_action to dry our code.
What was a technical challenge that you faced as a group? Deployment and Heroku debugging.
What was a team/personal challenge that you faced as a group? As a whole, not too many! I (Liv) personally really enjoyed working with both Corinna and Hannah - they're absolute work horses! The only one we could come up with was knowing when to cut ourselves off from upgrading features and optimizing.
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/1qkeYUZruSNNcNA6UCqpdBDmXa1Qs-Hjt/view?usp=sharing
What is your Trello URL? https://trello.com/invite/b/UPxP6Dnt/0e2a30755acb0cc446925e053c7f3aab/nooksy
What is the Heroku URL of your deployed application? http://nooksy.herokuapp.com

Copy link

@beccaelenzil beccaelenzil left a comment

Choose a reason for hiding this comment

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

LIV: I love how you divided your tests into what you can do unauthenticated and what you can do authenticated. This is a great way to use login from the test helper. Overall, I feel like your tests are quite clean and make good use of this test_helper method and fixtures. If there are specific places where you thought it could be DRYer, I'd be happy to chat about it. In general, tests are the one place not to worry too much about DRYness.

CORINNA: I left in an in-line comment about the controller filter conundrum. It's ok to not use controller filters if you're not writing the same code more than a couple times. Again, if there are specific places in the tests, I'd be happy to chat about how you might be able to write a test_helper method to make things more concise.

HANNAH: Great use of controller filters and custom model methods. I left one in-line comment of a small amount of logic you could consider putting in the model.

end

def ordered
@order = Order.find_by(id: params['id'])

Choose a reason for hiding this comment

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

Per your question, it's ok to not have a controller filter here since this code is only repeated twice. You could chose to use a controller filter for the 'id' one and not the 'format' one.

product = Product.find_by(id: params["format"]).inventory


session[:cart].each do |item|

Choose a reason for hiding this comment

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

Consider whether you can move any of this logic to a model method.

quantity = params["quantity"].to_i

session[:cart].each do |item|
if item['product_id'] == product.id

Choose a reason for hiding this comment

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

Consider whether you can move any of this logic to a model method.

return
end

if item["product_id"] == params['format'].to_i

Choose a reason for hiding this comment

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

Consider moving this business logic to the model.

@shipped_revenue = 0
@shipped_count = 0

Order.all.each do |order|

Choose a reason for hiding this comment

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

These calculations should be done in custom model methods.

order.order_items.each do |item|
merchant_product = Product.find_by(id: item.product_id)

if session[:user_id] == merchant_product.user_id

Choose a reason for hiding this comment

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

This logic seems like it could go into the merchant model so that you could use something like merchant.orders



def index
@products = Product.where(active: true).order(:name).paginate(:page=>params[:page],:per_page=>15)

Choose a reason for hiding this comment

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

you could consider during some of this work in a custom model method such as active_sorted_products.

@@ -0,0 +1,66 @@
class Product < ApplicationRecord

Choose a reason for hiding this comment

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

Great use of custom model methods

return order_total
end

def self.get_items(merchant_products)

Choose a reason for hiding this comment

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

I like this method! Do you use it?

@@ -0,0 +1,15 @@
<div class="products__index_product-container item">

Choose a reason for hiding this comment

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

Great use of a partial view

@beccaelenzil
Copy link

bEtsy

Functional Requirements: Manual Testing

Workflow yes / no
Deployed to Heroku
Before logging in
Browse all products, by category, by merchant ✔️
Leave a review ✔️
Verify unable to create a new product ✔️
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) ✔️(this will update when I check out)
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 OrderItems, Order, and Users controllers all have coverage that is a bit low. The lines that are not covered seem to include some business that could perhaps be delegated to a model method that could then be tested.
Routes
No un-needed routes generated (check reviews) ✔️
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 by default ✔️
Helper methods or filters to find logged-in user, cart, product, etc ✔️
No excessive business logic ✔️
Business logic that ought to live in the model
Add / remove / update product on order See in-line comments pointing out business logic in controller
Checkout -> decrease inventory See in-line comments pointing out business logic in controller
Merchant's total revenue See in-line comments pointing out business logic in controller
Find all orders for this merchant (instance method on Merchant) Is this the method get_items on the Order model -- I don't see where this is used.
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
This logic is largely in the controller and not thoroughly tested. This it's what's bringing your test coverage down on orders_controller.
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
✔️Similar to above, this logic is in the controller, and not thoroughly tested. By moving this logic to the model, it will likely be more straightforward to test.
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 see some of these tests, but some are 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
✔️thorough testing!

Code Style Bonus Awards

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

Quality Yes?
Perfect Indentation
Elegant/Clever
Descriptive/Readable
Concise
Logical/Organized

Overall Feedback

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.

4 participants