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

[Boutique Inventory] Counts all stock that exists #1468

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kotp
Copy link
Member

@kotp kotp commented Nov 30, 2022

This corrects a bug that would not account for all items in some
inventory that would otherwise pass the tests.

@kotp kotp self-assigned this Nov 30, 2022
@kotp kotp requested a review from a team November 30, 2022 19:24
@kotp
Copy link
Member Author

kotp commented Nov 30, 2022

Incoming fly-by e-mail patch submitted, but I can not approve.

@iHiD
Copy link
Member

iHiD commented Nov 30, 2022

I can't see what this is testing. Do you have an example of code that previously was incorrectly passing pls?

@kotp
Copy link
Member Author

kotp commented Nov 30, 2022

It is testing that we indeed get a total count for some products. Student code was passing but would not pass with this additional test.

  def total_stock
    stock = items.map { |item| item[:quantity_by_size] }.first
    stock.nil? || stock.empty? ? OUT_OF_STOCK : stock.each_value.inject(:+)
  end

Would pass the current tests, but would fail this situation as presented in the tests. The test was really testing that we got the total in the first listed type of product, rather than the indicated "total of some products".

The additional test drove this solution for this student:

  def total_stock
    items.inject(0) do |counter, item|
      counter + item[:quantity_by_size].inject(0) { |sum, values| sum + values.last }
    end
  end

This corrects a bug that would not account for all items in some
inventory that would otherwise pass the tests.
@kotp kotp force-pushed the boutique_inventory_bug_fix branch from 6ba9260 to dd240f4 Compare October 3, 2024 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants