Conversation
|
Passes functional check. Over to @dorner for technical review. |
dorner
left a comment
There was a problem hiding this comment.
Thanks so much for all your help - had a few questions which are repeats of other PRs. :)
| # needed rather than depending on external code to do it for me. | ||
| # This makes this code more self contained and efficient! | ||
| @purchases = Purchase.includes( | ||
| @organization = organization |
There was a problem hiding this comment.
Does this need to be an instance variable?
| private | ||
|
|
||
| attr_reader :purchases | ||
| attr_reader :purchases, :item_headers |
There was a problem hiding this comment.
Is this read from outside the class?
| let(:unused_item) { create(:item, name: "Unused Item", organization: organization) } | ||
| let(:generated_csv_data) do | ||
| # Force unused_item to be created first | ||
| unused_item |
There was a problem hiding this comment.
Can we replace line 121 with let! to avoid having to do this?
| let(:generated_csv) { described_class.new(purchase_ids: purchase_ids, organization: organization).generate_csv } | ||
|
|
||
| it "returns a valid CSV string" do | ||
| expect(generated_csv).to be_a(String) |
There was a problem hiding this comment.
Can we do a test against "real" CSV data instead of just testing bits of it?
There was a problem hiding this comment.
Totally can if you want -- it's kinda unnecessary imo.
Most of the test contentions are against generated_csv_data which is what builds the "real" csv data, and this block is testing the generate_csv method -- which is a very thin wrapper around CSV.generate. So writing tests against that method are kinda just testing CSV.generate?
I rearranged the file a bit so you can see this more clearly, with a #generate_csv_data and a #generate_csv context blocks. If you still want that in, let me know :)
There was a problem hiding this comment.
I think it's really kind of turning a set of unit tests into an integration test. Each individual part might be tested, but until we see a full CSV that we're comparing, with as few dynamic values as possible, we can't really know that we're doing it right. So we create data with specific values, generate the CSV, and compare against our result. Our comparison CSV shouldn't have anything dynamic except auto-generated IDs.
dorner
left a comment
There was a problem hiding this comment.
I have to write something here even though I already made a comment thank you GitHub
|
For this, it looks like we should switch to a CSV fixture based spec, and then it should be ready to merge. |
|
@bsbonus Hi! It looks like we have one outstanding comment here: #5201 (comment). Would you be able to take a look? Thanks! |
…export-with-inactive-items
Per review: compare the full generated CSV against fixture files in spec/fixtures/files/ instead of order-independent header/index lookups, so any change to column order or content fails loudly. - Pin vendor names, dates, amounts, and item names (previously Faker- random) so the fixtures can be fully static - Rewrite the inactive-item and unused-item tests (bug rubyforgood#5152 coverage) as full-CSV fixture comparisons - Convert the case-insensitive column sort test to a fixture comparison and drop the loose generate_csv wrapper tests, which the fixture assertions cover exactly Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BNy2e1J81K8xbvXraCeN8e
|
@bsbonus: Your PR |
Checklist:
X I have performed a self-review of my own code,
X I have commented my code, particularly in hard-to-understand areas,
X I have made corresponding changes to the documentation,
X I have added tests that prove my fix is effective or that my feature works,
X New and existing unit tests pass locally with my changes ("bundle exec rake"),
X Title include "WIP" if work is in progress.
X I acknowledge that I will not force push my branch once reviews have started.
Partial #5152 - partially, see main thread on this issue
Description
Updates the Purchases export to include inactive, unpurchased items in the export headers for consistency with the Distributions export
Type of change
How Has This Been Tested?
See attached CSVs as reference
Distributions_as_reference.csv
Purchases export before fix:
purchases_before_fix.csv
Purchases after fix:
purchases after_fix.csv