Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #4172 donation validation display error #4483

Merged
merged 11 commits into from
Aug 4, 2024
Merged
2 changes: 1 addition & 1 deletion Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -787,4 +787,4 @@ DEPENDENCIES
webmock (~> 3.23)

BUNDLED WITH
2.5.11
2.5.14
2 changes: 1 addition & 1 deletion app/controllers/donations_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ def update
redirect_to donations_path
rescue => e
flash[:alert] = "Error updating donation: #{e.message}"
render "edit"
redirect_back(fallback_location: edit_donation_path)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't this lose all the information you've entered?

Copy link
Contributor Author

@jimmyli97 jimmyli97 Jul 4, 2024

Choose a reason for hiding this comment

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

That is the existing behavior when I tested right now on the main branch commit e12311, I can try to change it and add a test so that it keeps all entered information except for the incorrect edited quantity if you want?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That seems to be the correct thing to do, yes.

Copy link
Contributor Author

@jimmyli97 jimmyli97 Jul 12, 2024

Choose a reason for hiding this comment

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

I ended up keeping all entered information including the incorrect edited quantity in commit 68cabfb, if that doesn't work let me know.

Note that per #4508 when testing READ_EVENTS must be set to true before a fixed donation can be submitted after an error.

end

def destroy
Expand Down
2 changes: 1 addition & 1 deletion spec/controllers/donations_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@
}
donation_params = { source: donation.source, storage_location: new_storage_location, line_items_attributes: line_item_params }
put :update, params: { id: donation.id, donation: donation_params }
expect(response).not_to redirect_to(anything)
expect(response).to redirect_to(edit_donation_path(donation))
expect(original_storage_location.size).to eq 5
expect(new_storage_location.size).to eq 0
expect(donation.reload.line_items.first.quantity).to eq 10
Expand Down
42 changes: 42 additions & 0 deletions spec/requests/donations_requests_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -172,5 +172,47 @@
expect(response.body).to_not include("you’ll need to make an adjustment to the inventory as well.")
end
end

# Bug fix - Issue #4172
context "when donated items are distributed to less than donated amount " \
"and you edit the donation to less than distributed amount" do
it "shows a warning and displays original names and amounts" do
item = create(:item, organization: organization, name: "Brightbloom Seed")
storage_location = create(:storage_location, :with_items, item: item, item_quantity: 0, organization: organization)
original_quantity = 100
donation = create(:donation, :with_items, item: item, item_quantity: original_quantity, organization: organization, storage_location: storage_location)
distribution = {
storage_location_id: storage_location.id,
partner_id: create(:partner).id,
delivery_method: :delivery,
line_items_attributes: {
"0": { item_id: item.id, quantity: 90 }
}
}

post distributions_path(distribution: distribution, format: :turbo_stream)

edited_donation = {
storage_location_id: storage_location.id,
line_items_attributes: {
"0": { item_id: item.id, quantity: 1 }
}
}
put donation_path(id: donation.id, donation: edited_donation)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is really what we're trying to hit, right? Why are we hitting all those other endpoints rather than just directly creating the data we want to interact with on this page?

Copy link
Contributor Author

@jimmyli97 jimmyli97 Jul 20, 2024

Choose a reason for hiding this comment

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

I think when I wrote this (several weeks ago at this point, I don't remember) I didn't know how the inventory system worked and that DistributionCreateService or TestInventory existed. I think this can be rewritten to just use TestInventory and set inventory to 0 before editing the donation, I will try that.

I am thinking of adding TestInventory info and linking to the wiki Event page in CONTRIBUTING.md so new contributors have a easier time writing tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved by ba5af77


expect(response).to redirect_to(edit_donation_path(donation))

get response.headers['Location']

if Event.read_events?(organization)
expect(flash[:alert]).to include("Error updating donation: Could not reduce quantity by 99 - current quantity is 10 for Brightbloom Seed")
else # TODO remove this branch when switching to events
expect(flash[:alert]).to include("Error updating donation: Requested items exceed the available inventory")
end

expect(response.body).to include(item.name)
expect(response.body).to include(original_quantity.to_s)
end
end
end
end