https://github.com/turingschool/black_thursday_spec_harness/blob/2dcddb9eaf6bf6162766c895d1d6fa325c335f98/spec/spec_helper.rb#L37
@memcmahon

Hey Lovisa,

In this test on Iteration 4, line 12, the date used is only ever in the data for an Invoice Updated At date ("2011-02-27"). Should this method query updated_at dates? Shouldn't it be created_at dates?

Summary

Iteration 4 #most_sold_item_for_merchant returns the most sold item spec does not match a common sense understanding of what that method would return.

Additional Detail

The Iteration 4 spec includes the following interaction pattern under the description which item sold most in terms of quantity and revenue:

sa = SalesAnalyst.new

sa.most_sold_item_for_merchant(merchant_id)
#=> [item] (in terms of quantity sold) 
# or, if there is a tie, [item, item, item]

sa.best_item_for_merchant(merchant_id)
#=> item (in terms of revenue generated)

With regards to quantity, this implies that for a Merchant that sold four forks in a single invoice and seven spoons across seven invoices, the method would return the spoon. However, for this method to work, students have had to find the InvoiceItem with the highest quantity for a particular merchant (and return the fork). There only seems to cause a discrepancy in the spec on line 105 of the iteration_4_spec document (assume that the earlier examples using the merchants on line 90 and 97 are not impacted by the two different ways of making this calculation).

Recommendation

Appears that this part of the spec harness is looking to see that ties are calculated correctly. Believe that means that we will need to find another Merchant who has items tied for their best selling item based on count.

Also believe that the #best_item_for_merchant method is calculated the same way. Fewer students have indicated an issue, likely because they use the pattern they found for #most_sold when they implement `#best_item.

Summary

Iteration 4 #best_item_for_merchant returns the item which generated most revenue for the given merchant spec does not expect an Item related to the Merchant identified as an argument.

Believe this may also impact #35.

Additional Detail

The Iteration 4 spec harness test includes the following at line 108:

it "#best_item_for_merchant returns the item which generated most revenue for the given merchant" do
  merchant_id = 12334189
  expected = sales_analyst.best_item_for_merchant(merchant_id)

  expect(expected.id).to eq 263516130
  expect(expected.class).to eq Item

  merchant_id = 12337105
  expected = sales_analyst.best_item_for_merchant(merchant_id)

  expect(expected.id).to eq 263463003
  expect(expected.class).to eq Item
end

The first expectation seems to indicate that the Item with the ID of 263516130 is the best selling item for the Merchant with the id of 12334189. However, the merchant_id associated with that Item (description: To Our Son the Chattan - Jewish Wedding Greeting Card) in items.csv is 12334863. It's difficult to understand how an Item not sold by a Merchant could be that Merchant's best selling item.

My best guess is that the merchant_id determined in the items table has been adjusted in some way and/or not checked against the item_ids included in the invoice_items table.

Students have been able to get the answer expected in the spec by first finding all of the Invoices associated with a Merchant, all the InvoiceItems associated with those Invoices, and then all of the Items associated with those InvoiceItems. However, if a student first tries to find all of the Items associated with a particular merchant and then all of the InvoiceItems associated with those Items, they will never find the greeting card, since that Item is not associated with the Merchant indicated.

Recommendation

One of three things:

1. Clean the data to ensure that all the Items associated with a Merchant via the merchant_id on the items table are also the items associated with a Merchant when you traverse invoices/invoice_items to get to items.
2. Indicate to students that this is an issue and they will need to go through the invoices table for this particular method.
3. Adjust the spec harness to find an Item that has the same Merchant going directly to items, or through invoices/invoice_items.

Summary

Iteration 4 indicates that an invoice is considered pending if none of its transactions are successful. However, that description is misleading due to the fact that not all Invoices have Transactions.

Additional Detail

Line 59 of the Iteration 4 tests of the Spec Harness indicates that there are 467 Merchants that have pending invoices:

it "#merchants_with_pending_invoices returns merchants with pending invoices" do
  expected = sales_analyst.merchants_with_pending_invoices

  expect(expected.length).to eq 467
  expect(expected.first.class).to eq Merchant
end

However, not all Invoices have Transactions. If a student attempts to complete this method by finding all of the Invoices that do not have a successful transaction (including those invoices that have no transaction), they will not get the 467 count. Instead, they need to look for invoices only failed transactions (they must have transactions, but each of those transactions must be a failure).

I'm less sure of the following: I believe that the count of 467 might only include Merchants who only have pending Invoices, and not all Merchants who have at least one pending Invoice, which seems to go against the text description of the method.

Recommendation

1. Confirm the issue of all pending invoices vs. at least one pending invoice.
2. Adjust the description in the spec, or adjust the spec harness to match the existing description.