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 #4897 · rubyforgood/human-essentials

nozomirin · 2024-12-31T15:57:07Z

Resolves #4858

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

cielf

Light testing looks good. User manual changes for the organization bit are outstanding, though.

The relevant area for that updating are: docs/user_guide/bank/getting_started_customization.md

I'll review the user manual stuff later.

cielf

I'm suggesting some expanded text around the instructions.

cielf · 2025-01-06T00:48:53Z


 [!NOTE] This includes inactive Items as well as active ones. 

+### Add In-Kind Value for each item


Please add something like "The default export includes the number of items exported, but not the values. If you want to also have the export include the in-kind value for each item in the distributions, you can set that option." before the instructions.

And after, please add a "[!NOTE] Setting this affects both the donation and distribution exports."

cielf · 2025-01-06T00:49:50Z


+### Add In-Kind Value for each item
+Click "My Organization" in the left hand manu. Click "Edit" button. Set the "Include in-kind value in donation and distribution exports?" to "yes".
+


Please add similar text as noted above in the distribution section.

cielf · 2025-01-06T00:51:34Z


+### Add In-Kind Value for each item
+Click "My Organization" in the left hand manu. Click "Edit" button. Set the "Include in-kind value in donation and distribution exports?" to "yes".
+


to "yes", then click "Save".

nozomirin · 2025-01-06T14:14:42Z

Light testing looks good. User manual changes for the organization bit are outstanding, though.

The relevant area for that updating are: docs/user_guide/bank/getting_started_customization.md

I'll review the user manual stuff later.

What contents should I add in this file?

cielf · 2025-01-06T17:05:55Z

The relevant area for that updating are: docs/user_guide/bank/getting_started_customization.md
I'll review the user manual stuff later.

What contents should I add in this file?

Well -- I would add a section, at the same level as "customizing the distribution printout", called "Configuring exports", then a subsection for the field. In that, I would talk about what the default distribution and donation exports are (as you did in the exports section), and the effect that setting this field to "yes' will have.

IIRC, your new field is just before "Distribution Email Content". If that's right, put the new section there.

nozomirin · 2025-01-07T05:43:29Z

The relevant area for that updating are: docs/user_guide/bank/getting_started_customization.md
I'll review the user manual stuff later.

What contents should I add in this file?

Well -- I would add a section, at the same level as "customizing the distribution printout", called "Configuring exports", then a subsection for the field. In that, I would talk about what the default distribution and donation exports are (as you did in the exports section), and the effect that setting this field to "yes' will have.

IIRC, your new field is just before "Distribution Email Content". If that's right, put the new section there.

It seems odd to me that there is a second-level section between two fourth-level sections.
I made a commit. Please check if it meets your expectations.

cielf · 2025-01-07T17:52:26Z

The relevant area for that updating are: docs/user_guide/bank/getting_started_customization.md
I'll review the user manual stuff later.

What contents should I add in this file?

Well -- I would add a section, at the same level as "customizing the distribution printout", called "Configuring exports", then a subsection for the field. In that, I would talk about what the default distribution and donation exports are (as you did in the exports section), and the effect that setting this field to "yes' will have.
IIRC, your new field is just before "Distribution Email Content". If that's right, put the new section there.

It seems odd to me that there is a second-level section between two fourth-level sections. I made a commit. Please check if it meets your expectations.

Yeah (haven't checked yet) We need to do some reorganization of that screen so that same-concepted things are together - but that's out of scope for this change.

cielf

A couple of tweaks, please, and over to @dorner for technical input.

cielf · 2025-01-08T01:46:11Z


+## Configuring exports
+
+### Add In-Kind Value for each item


Make this the whole field name -- i.e. Include in-kind value in donation and distribution exports?

This matches the practice for the field-level sections in the rest of the file.

cielf · 2025-01-08T01:47:06Z

+
+### Add In-Kind Value for each item
+
+You can configure whether the export file includes the "In-Kind Value" column for each item. By default, these values are excluded. To include them, set the "Include in-kind value in donation and distribution exports?" option to "Yes".


please change "the export file includes " to "the exports for donations and distributions include "

…be added in export of donations and distributions

Addressed

cielf

Hey @nozomirin . This looks pretty good to me from a functional pov.

One note -- when you do a force push it makes it harder for the reviewers, because they can't see the incremental changes.

Over to @dorner for technical review

dorner

Overall looks great - I had a request for the tests.

dorner · 2025-01-10T01:07:27Z

@@ -0,0 +1,6 @@
+class AddIncludeInKindValuesInExportedFilesToOrganizations < ActiveRecord::Migration[7.2]
+  def change
+    add_column :organizations, :include_in_kind_values_in_exported_files, :boolean


This should be NOT NULL. Otherwise it's got three values, true false and null. :)

dorner · 2025-01-10T01:09:25Z

+            distribution.comment
+          ]
+
+          row += quantities_and_values_of_item.flat_map { |item| [item[:quantity], item[:value].to_f] }


We really should be setting up the data using explicit values rather than trying to calculate aggregations of random values. If it's not too much work, would you be able to make that change?

@nozomirin FYI, we will be addressing those inconsistencies in PR4924. I'd like to get this one in first if we can.

I will change this test later in another PR.

dorner · 2025-01-10T01:09:43Z

+
+          row += item_quantities
+
+          expect(item_quantities.sum).to eq(total_item_quantity)


nozomirin · 2025-01-11T16:00:33Z

Hey @nozomirin . This looks pretty good to me from a functional pov.

One note -- when you do a force push it makes it harder for the reviewers, because they can't see the incremental changes.

Over to @dorner for technical review

I understand that I should not force push after a review. Got it—this won't happen again.

nozomirin · 2025-01-12T08:16:26Z

If the filter is set to item A, the total number is the number of A. But what is the 'total value'? Is it the value of A or the value of the entire distribution?

cielf · 2025-01-13T17:07:07Z

We recently merged a change to make it so that the filtered values on the index reflect only the values for the items in the filter. The export should match that. I don't recall immediately if the export was addressed at that time, but suggest you merge main into your branch and confirm.

nozomirin · 2025-01-14T06:21:05Z

I pulled and tested it on the main branch. The total value represents the value of the entire distribution.

cielf · 2025-01-14T13:50:25Z

Huh. I thought we'd addressed that. I'm going to have to track down what happened there (and I can't do that in the next few minutes. Maybe later today).

The export should match the index.

cielf · 2025-01-15T15:27:03Z

I've dug into this a bit more. For the scope of this PR, leave the total value as is.

We have some inconsistencies between how we are handling filtering by item and filtering by item category in the export that need to be resolved, but it's out of scope for this PR.

cielf · 2025-01-16T17:14:57Z

@nozomirin FYI, we will be addressing those inconsistencies in PR4924. I'd like to get this one in first if we can.

cielf · 2025-03-16T07:06:45Z

This seems to have fallen through the cracks.... and there have been a few changes since that are causing confilcts. In the meantime we addressed the question of total value regarding the item and item category filtering. @dorner Can you take a look at the resolution of your concerns above, then I think we may have a semi-gnarly conflict resolution to do.

dorner · 2025-03-21T19:16:05Z

I asked for a change to the tests which wasn't done.

cielf · 2025-03-21T23:18:27Z

@nozomirin -- Just a last check -- are you still working on this? There are still changes that were asked for and not changed. (If we don't hear from you by the 29th, we'll assume this is is abandoned and we can ask someone else to finish it up.)

cielf · 2025-03-27T13:16:23Z

Closing this PR, as @nozomirin has submitted a fresh one.

nozomirin force-pushed the 4858-add-an-option-to-include-in-kind-values-in-export-of-donations-and-distributions branch 4 times, most recently from 41722bc to a21bf2d Compare January 2, 2025 08:36

cielf requested changes Jan 5, 2025

View reviewed changes

cielf requested changes Jan 6, 2025

View reviewed changes

nozomirin force-pushed the 4858-add-an-option-to-include-in-kind-values-in-export-of-donations-and-distributions branch 2 times, most recently from 62279c2 to 4ae30dc Compare January 6, 2025 04:55

nozomirin force-pushed the 4858-add-an-option-to-include-in-kind-values-in-export-of-donations-and-distributions branch from 4ae30dc to d8051fe Compare January 7, 2025 05:40

cielf previously requested changes Jan 8, 2025

View reviewed changes

cielf requested a review from dorner January 8, 2025 01:49

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

a15b1de

…be added in export of donations and distributions

nozomirin force-pushed the 4858-add-an-option-to-include-in-kind-values-in-export-of-donations-and-distributions branch from d8051fe to a15b1de Compare January 9, 2025 10:16

cielf reviewed Jan 9, 2025

View reviewed changes

dorner requested changes Jan 10, 2025

View reviewed changes

Make include_in_kind_values_in_exported_files not null.

556a41d

nozomirin mentioned this pull request Mar 27, 2025

Add an option to control whether "in-kind value" of each item should be added in export of donations and distributions #5122

Merged

cielf closed this Mar 27, 2025


		[!NOTE] This includes inactive Items as well as active ones.

		### Add In-Kind Value for each item


		### Add In-Kind Value for each item
		Click "My Organization" in the left hand manu. Click "Edit" button. Set the "Include in-kind value in donation and distribution exports?" to "yes".


		### Add In-Kind Value for each item

		You can configure whether the export file includes the "In-Kind Value" column for each item. By default, these values are excluded. To include them, set the "Include in-kind value in donation and distribution exports?" option to "Yes".


		row += item_quantities

		expect(item_quantities.sum).to eq(total_item_quantity)

Uh oh!

Conversation

nozomirin commented Dec 31, 2024 • 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!

cielf 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!

nozomirin commented Jan 6, 2025

Uh oh!

cielf commented Jan 6, 2025

Uh oh!

nozomirin commented Jan 7, 2025

Uh oh!

cielf commented Jan 7, 2025

Uh oh!

cielf 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!

cielf left a comment

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!

nozomirin Jan 17, 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!

nozomirin commented Jan 11, 2025

Uh oh!

nozomirin commented Jan 12, 2025

Uh oh!

cielf commented Jan 13, 2025

Uh oh!

nozomirin commented Jan 14, 2025

Uh oh!

cielf commented Jan 14, 2025

Uh oh!

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

Uh oh!

Uh oh!

cielf commented Jan 16, 2025

Uh oh!

cielf commented Mar 16, 2025

Uh oh!

dorner commented Mar 21, 2025

Uh oh!

cielf commented Mar 21, 2025

Uh oh!

cielf commented Mar 27, 2025

Uh oh!

Reviewers

Assignees

nozomirin commented Dec 31, 2024 •

edited

Loading

cielf left a comment •

edited

Loading

nozomirin Jan 17, 2025 •

edited

Loading

cielf commented Jan 15, 2025 •

edited

Loading