Skip to content

Conversation

@Aurea-Li
Copy link

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? Aurea: Logic behind keeping track of stock and quantity when things are being added and removed from the cart in the Order Items Controller method. Sigrid: The root page (CSS) and the merchant dashboard. Val: I wrote a lot of the model and controller tests. Katricia: Setting up OAuth and Sessions controller
Each team member: what is one thing you were primarily responsible for that you would like targeted feedback on? Aurea: Ways to possibly refactor all the if statements in OrderItems controller Sigrid: The merchant dashboard. Val: Feedback on OrderItem model tests, the custom methods. Katricia: DRYing up CSS
How did your team break up the work to be done? First, we each took a controller and worked independently on those. Once we had our basic setup we switched to breaking it up by user stories on trello.
How did your team utilize git to collaborate? We all made feature branches and decided to coordinate pushing and pulling so that we would all be present and push/pull at the same time.
What did your group do to try to keep your code DRY while many people collaborated on it? We communicated to ensure that no one was creating repetitive code, and also if we saw other members code that could be refactored, we would refactor it.
What was a technical challenge that you faced as a group? We encountered a huge issue with our app crashing after a deployment to Heroku. We spent a lot of time and effort trying to solve the issue, the problem was that the error in Heroku was very vague and general and we weren't sure how to debug. We ended up figuring out the issue was having require pry in our controllers.
What was a team/personal challenge that you faced as a group? Coordinating our workflow, keeping everyone on track during meetings.
What was your application's ERD? (include a link) https://www.lucidchart.com/documents/edit/b97ddb55-3eb0-4cd7-9748-588adbc400ba/0
What is your Trello URL? https://trello.com/b/9yc2upI0/betsy-katricia-sigrid-val-aurea
What is the Heroku URL of your deployed application? https://petsy-emporium.herokuapp.com/

@droberts-sea
Copy link

bEtsy

Aurea: Ways to possibly refactor all the if statements in OrderItems controller

  • The rules for this are complex, so complex code is at least somewhat expected. However, I think you're right that we could clean this up. Here is the approach I would take:
# Add an exception type and an instance method to class Order:
class InventoryError < StandardError; end


def add_item(product, quantity)
  Order.transaction do
    if quantity > product.stock
      raise InventoryError, "Not enough #{product.name} in stock (wanted #{quantity}, have #{product.stock})"
    end


    item = self.order_items.find_by(product_id: product.id)
    if item.nil?
      item = self.order_items.new(product: product, quantity: 0)
    end


    item.quantity += quantity
    item.save!
    product.stock -= quantity
    product.save!
  end
end


# Then in the controller action, wrap the call in a begin/rescue block
def create
  # ... find or create order and product (maybe do this in a filter?) ...
  begin
    order.add_item(product, params[:quantity])
  rescue StandardError => err
    flash[:status] = :error
    flash[:result_text] = err.message
    return
  end
  
  # If we got here, we've successfully updated the cart!
end

Note the transaction - that ensures that all the operations either succeed or fail as a group. See https://api.rubyonrails.org/classes/ActiveRecord/Transactions/ClassMethods.html

Sigrid: The merchant dashboard.

  • I love your use of model methods here - good work.
  • However, your authorization story could be improved. Currently, the merchant ID comes from the URL, and in the view you check that it matches the ID of the logged in merchant. This type of logic is very specific to running a webapp, which tells me it should probably live in the controller (e.g. set a flash message and redirect to the root page if the wrong person is logged in).
  • Or, we could go even further and change the route to prevent the problem in the first place. Since we already have the merchant ID in the session, we could make the URL be merchant/dashboard (removing the merchant ID), and display the dashboard for whoever's currently logged in. That way we don't even need to make the check, because there's no opportunity for the user to give us the wrong info.

Val: Feedback on OrderItem model tests, the custom methods.

  • These are a good start, but you're missing some pieces:
    • In addition to returning the new value, does the stock on the product change?
    • What if you try to reduce the stock past 0?
    • What if you call either method with a negative number?
  • Also, restock_product will work exactly the same if you omit its first line (line 16).

Katricia: DRYing up CSS

  • I don't see too many repeated styles. However, CSS is really hard to just sit down and read. Is there some specific piece that was troublesome?

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