Add an option to control whether "in-kind value" of each item should be added in export of donations and distributions by nozomirin · Pull Request #5122 · rubyforgood/human-essentials

nozomirin · 2025-03-27T03:48:07Z

Resolves #4858

Sorry for leaving it untouched for a long time. Since it is far behind the main branch, I made a new PR.
the original one is #4897.

Description

add include_in_kind_values_in_exported_files column in Organizations table.
add radio buttons in view/organization/edit.html.erb.
make some changes to add in-kind value for each item in exported donation and distribution files when the Include in-kind value in donation and distribution exports? is set to yes.
add explanations in docs/user_guide/bank/exports.md

Note: I changed the "In-Kind Value" header to "In-Kind Total" in the exported donation file to avoid ambiguity. However, I used "Header + In-Kind Value" for the in-kind value of items, so perhaps the change is unnecessary. If there’s anything incorrect or unclear, please let me know.

Type of change

New feature (non-breaking change which adds functionality)
This change requires a documentation update

How Has This Been Tested?

bundle exec rspec spec/services/exports/export_distributions_csv_service_spec.rb 
bundle exec rspec spec/services/exports/export_donations_csv_service_spec.rb

Screenshots

…be added in export of donations and distributions

cielf

Hey @nozomirin It still works well!

I can see, though, that we're going to have a conflict with the work in #5116, where we are reorganizing the organizations page..

With that in place, I would want the new field here to be in its own section, "Exports" on those screens. (refer to #5116 for what weight the heading should be)

Ideally, I would want it that section just above the "Annual Survey" section.

This is a change that is needed before merge, but might be easiest to do once #5116 is merged into main (which I anticipate this week).

I don't see any reason that it would hold up our technical re-review, so over to @dorner or @awwaiid.

nozomirin · 2025-03-28T04:31:40Z

@cielf OK, I will make some adjustments after #5116 is merged.

…-in-export-of-donations-and-distributions-new

…view.

nozomirin · 2025-04-05T05:56:35Z

There were no errors when I ran bundle exec rspec on my local machine. It seems the issue came from the first merge from main.

nozomirin · 2025-04-05T06:01:46Z

@cielf Please check whether it fits your expectations.

cielf · 2025-04-05T19:40:21Z

Yup -- I saw some things that are out of scope, and will be addressed in a separate issue, but your changes LGTM on manual testing. Over to @dorner for technical input

cielf · 2025-04-05T19:45:57Z

@nozomirin One very small change to the docs --- please change "Configuring exports" to "Exports" - so it matches what the bank user will be seeing. Thanks

…ation.md

cielf · 2025-04-06T03:11:16Z

LGTM.

dorner · 2025-04-08T23:16:27Z

+class AddIncludeInKindValuesInExportedFilesToOrganizations < ActiveRecord::Migration[8.0]
+  def change
+    add_column :organizations, :include_in_kind_values_in_exported_files, :boolean
+    add_check_constraint :organizations, "include_in_kind_values_in_exported_files IS NOT NULL", name: "include_in_kind_values_in_exported_files_not_null", validate: false


This seems a very awkward way of just saying null: false...?

Adding the NOT NULL constraint directly will lock the table. This method helps avoid that. I don't know how big the table will be, so I think it's better to avoid it.

Organizations is a tiny table. You can lock it.

dorner · 2025-04-08T23:20:22Z

      @distribution_totals = DistributionTotalsService.call(Distribution.where(organization:).class_filter(filters))
+
+      @export_line_item_adder = Exports::ExportLineItemColumnsAdder.new
+      @export_line_item_adder.register(Exports::ExportInKindTotalValue) if @organization.include_in_kind_values_in_exported_files


This seems like an overly complex way of adding a column to an export. You're basically turning a single if statement into a factory pattern. Can you explain why you did it this way?

If you have other attributes that need to be added to the exported CSV, just create another class and register it here. There's no need to modify any other code. Since both distribution and donation share the same export functionality, I extracted it to avoid duplication.

This is a case of unnecessary DRY along with a big helping of YAGNI. As far as we know there are no additional attributes that will need this, and in general the idea that "attributes in a CSV" is a generic feature is probably not a helpful construct. I'd much prefer a simpler approach even if it means a little bit of duplicated code.

dorner · 2025-04-08T23:22:49Z

+      @registry = []
+    end
+
+    def headers(item_headers)


In general this code is written in an unusual manner... I'd need to see a bunch more comments here since I can't really understand what some of it is supposed to be doing. What does "registry" contain? What kind of "row" does get_row return? What additional_columns do we need and how does that relate to the registry...?

The registry contains classes that implement Exports::ExportColumnsBase. Each element in the registry adds a column after each "line_item" column.
get_row returns a row like this:
|item A|col1|col2|item B|col1|col2|...
These col1, col2, etc., are the additional_columns.

The explanation has to be in comments, not in the PR.

But in general, if you have to explain what the code is doing via comments, it might be doing things in a way that isn't straightforward.

dorner · 2025-04-08T23:24:14Z

+            distribution.created_at.strftime("%m/%d/%Y"),
+            distribution.issued_at.strftime("%m/%d/%Y"),
+            distribution.storage_location.name,
+            distribution.line_items.where(item_id: item_id).total,


Using calculated values in specs make it not only hard to follow but also a lot flakier. I've had many instances where a value passed because it happened to match the expected value, but was actually reading the wrong source. Can you explicitly create the things you're trying to test and expect the hardcoded values from those things?

Ok, I will make some changes.

It seems really annoying to do that with FactoryBot...

Please check whether it fits your expectations.

…-in-export-of-donations-and-distributions-new

dorner · 2025-04-16T15:47:18Z

+            "$#{expected_dist[:shipping_cost].to_f}",
+            expected_dist[:state],
+            expected_dist[:agency_rep],
+            expected_dist[:comment],


Is there a reason you can't just generate a flat CSV string and compare the results against it instead of going row by row?

This comment still stands - see e.g.

human-essentials/spec/requests/items_requests_spec.rb

Line 52 in d8fa236

it "generates csv with items that are also inactive" do

…ration.

…re CSV at once.

…-in-export-of-donations-and-distributions-new

nozomirin · 2025-04-30T02:14:37Z

If you have time, please review this. @dorner

dorner · 2025-05-02T19:38:25Z

+    let(:duplicate_item) { create(:item, name: "Dupe Item", value_in_cents: 300, organization: organization) }

-    let(:items_lists) do
+    let(:expected_distributions) {


Is there a reason you're not just using FactoryBot.build / FactoryBot.create here?

This feels a lot like a Go table test instead of a Rails test.

Please check whether it fits your expectations.

dorner · 2025-05-02T19:38:36Z

+            "$#{expected_dist[:shipping_cost].to_f}",
+            expected_dist[:state],
+            expected_dist[:agency_rep],
+            expected_dist[:comment],


This comment still stands - see e.g.

human-essentials/spec/requests/items_requests_spec.rb

Line 52 in d8fa236

it "generates csv with items that are also inactive" do

…of-donations-and-distributions-new' of https://github.com/nozomirin/human-essentials into 4858-add-an-option-to-include-in-kind-values-in-export-of-donations-and-distributions-new

dorner

A bit more explicitness on the tests please!

dorner · 2025-05-09T19:12:13Z

+
+      it 'should match the expected content without in-kind value of each item for the csv' do
+        csv = <<~CSV
+          #{expected_headers.join(",")}


Can we make the entire value explicit please? We should know/specify the partner names and the storage location names. We should also know the headers.

OK, I just thought it would be really long.

dorner · 2025-05-09T19:12:51Z

-        expect(subject[idx + 1]).to eq(row)
+        csv = CSV.generate do |csv|
+          csv << expected_headers
+          csv << ["Product Drive", "2025-01-01", source_name(donations[0]), storage_location.name, 15, 2, 94.0, "It's a fine day for diapers.", 7, "70.00", 0, "0.00", 0, "0.00", 8, "24.00", 0, "0.00"]


Here especially - we're calculating values rather than using them directly.

The Faker gem generates names with commas, so I used CSV.generate to prevent test failures.
I’ve now replaced the comma in the name with a semicolon—please check if that's OK.

…n tests.

dorner

I'm good with this. In the future, if you need to compare/assert against any value, I'd recommend hardcoding that value instead of relying on Faker.

"LGTM"

github-actions · 2025-05-18T14:06:57Z

@nozomirin: Your PR Add an option to control whether "in-kind value" of each item should be added in export of donations and distributions is part of today's Human Essentials production release: 2025.05.18.
Thank you very much for your contribution!

…be added in export of donations and distributions (rubyforgood#5122) * Add an option to control whether "in-kind value" of each item should be added in export of donations and distributions * Change 'Configuring exports' to 'Exports' in getting_started_customization.md * Fix: Explicitly define test data and address money conversion problems. * fix: linter errors. * Fix: simplify the include_in_kind_values_in_exported_files column migration. * Fix: simplify the way to add In-Kind Total Value in csv file. * Fix: change from asserting each line in the CSV to asserting the entire CSV at once. * Fix: use <<~CSV...CSV to replace array based csv construction. * Fix: solve test error when the donation_site name contains comma. * Fix: make export csv header more explicit in distribution and donation tests.

Add an option to control whether "in-kind value" of each item should …

e3f8594

…be added in export of donations and distributions

cielf requested review from cielf and dorner March 27, 2025 13:37

cielf previously requested changes Mar 27, 2025

View reviewed changes

cielf requested a review from awwaiid March 27, 2025 13:52

nozomirin and others added 2 commits March 30, 2025 12:19

Merge branch 'main' into 4858-add-an-option-to-include-in-kind-values…

49ce0a6

…-in-export-of-donations-and-distributions-new

Resolve merge conflict from main branch that changed My Organization …

11e5743

…view.

Change 'Configuring exports' to 'Exports' in getting_started_customiz…

3908867

…ation.md

dorner requested changes Apr 8, 2025

View reviewed changes

nozomirin and others added 3 commits April 9, 2025 14:34

Merge branch 'main' into 4858-add-an-option-to-include-in-kind-values…

ab3e61c

…-in-export-of-donations-and-distributions-new

Fix: Explicitly define test data and address money conversion problems.

6c26a56

fix: linter errors.

e205b5c

dorner reviewed Apr 16, 2025

View reviewed changes

nozomirin and others added 4 commits April 17, 2025 12:33

Fix: simplify the include_in_kind_values_in_exported_files column mig…

d653f8e

…ration.

Fix: simplify the way to add In-Kind Total Value in csv file.

62e076f

Fix: change from asserting each line in the CSV to asserting the enti…

63b333e

…re CSV at once.

Merge branch 'main' into 4858-add-an-option-to-include-in-kind-values…

2abbe5e

…-in-export-of-donations-and-distributions-new

dorner requested changes May 2, 2025

View reviewed changes

nozomirin added 3 commits May 3, 2025 22:58

Fix: use <<~CSV...CSV to replace array based csv construction.

b09b697

Merge branch '4858-add-an-option-to-include-in-kind-values-in-export-…

872544c

…of-donations-and-distributions-new' of https://github.com/nozomirin/human-essentials into 4858-add-an-option-to-include-in-kind-values-in-export-of-donations-and-distributions-new

Fix: solve test error when the donation_site name contains comma.

6dae33a

nozomirin force-pushed the 4858-add-an-option-to-include-in-kind-values-in-export-of-donations-and-distributions-new branch from 60d1fee to 6dae33a Compare May 3, 2025 15:39

dorner requested changes May 9, 2025

View reviewed changes

Fix: make export csv header more explicit in distribution and donatio…

2670caa

…n tests.

dorner approved these changes May 15, 2025

View reviewed changes

dorner merged commit 0acf065 into rubyforgood:main May 15, 2025
12 checks passed

Uh oh!

Conversation

nozomirin commented Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

How Has This Been Tested?

Screenshots

Uh oh!

cielf left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nozomirin commented Mar 28, 2025

Uh oh!

nozomirin commented Apr 5, 2025

Uh oh!

nozomirin commented Apr 5, 2025

Uh oh!

cielf commented Apr 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cielf commented Apr 5, 2025

Uh oh!

cielf commented Apr 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dorner Apr 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nozomirin commented Apr 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dorner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dorner left a comment

Choose a reason for hiding this comment

Uh oh!

nozomirin commented Mar 27, 2025 •

edited

Loading

cielf left a comment •

edited

Loading

cielf commented Apr 5, 2025 •

edited

Loading

dorner Apr 16, 2025 •

edited

Loading