Skip to content

Conversation

@bsbonus
Copy link
Contributor

@bsbonus bsbonus commented May 22, 2025

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.

Parital #5152 - partially, will be broken up into multiple PRs

Description

This modifies the query used by the Donations export to also include inactive items and items that were not donated, so that the headers are consistent with the "Distributions" export logic.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  1. Ensured test data included inactive and non-donated items. Giving the item a unique name is super important.
  2. Exported out Distribution export as reference point
  3. Exported the Donations export before the changes
  4. Exported the Donations export after the changes, compared against Distributions for item columns to ensure match.
  5. Updated and shored up test coverage accordingly

PLease see attached XLXS files to help make sense of this.

This CSV will serve as a reference point for Donations for the item columns, at least.
Distributions_as_reference.csv

This is an example of the data BEFORE the fix -- note how many fewer item columns are in the export
donations_before.csv

This is the fixed version. Note that the total number of item columns matches the Distribution export
donations_after_fix.csv

@cielf
Copy link
Collaborator

cielf commented May 23, 2025

LGTM functionally. Over to @dorner for technical review.

@cielf cielf requested a review from dorner May 23, 2025 15:32
@bsbonus bsbonus requested a review from dorner May 29, 2025 19:43

# Check that the remaining columns match our expected case-insensitive sort
expect(item_columns).to eq(expected_order)
end
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See previous comments re hardcoded CSVs.

@awwaiid
Copy link
Collaborator

awwaiid commented Sep 13, 2025

For this, it looks like we should switch to a CSV fixture based spec, and then it should be ready to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants