Fix #4172 donation validation display error #4483

jimmyli97 · 2024-06-28T00:13:00Z

Resolves #4172

Description

donation items are now correctly displayed after invalid edit would bring inventory to below distributed amount
rspec request test for above

Alternatives considered

in the rspec, not sure how specific to be in the error message test, I added tests for both using Events and not using Events
in the rspec, not sure if it's better to use factorybot create(:distribution) instead of PUT

Type of change

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

How Has This Been Tested?

manual testing
bundle exec rake

Screenshots

When edited donation amount plus available inventory is less than requested inventory for distribution, it should redirect to the edit donation page, display an error, the original donated item name, and the original amount

@Items

…t edit * direct call to render doesn't load @Items causing partial to be unable to match item IDs to item names, resulting in displaying numbers * replace with redirect to edit

Error message changes if using events or not, add cases for both

dorner

I think this approach won't do what we want. Can you verify?

dorner · 2024-07-04T00:00:50Z

app/controllers/donations_controller.rb

@@ -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)


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

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?

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

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.

…ate error * Page should still display original donation source pre-edit in the title and breadcrumbs

* Also keeps submitted incorrect quantity

dorner · 2024-07-12T19:24:57Z

app/controllers/donations_controller.rb

+    donation_params[:line_items_attributes].values.each { |attr|
+      attr.delete(:_destroy)
+      line_items.push(attr)
+    }


I think this would put back any line items that the user removed?

I think you would have been right, but I originally put this line to pass donation controller request rspec tests which sent the _destroy parameter. Those tests I found out are outdated, the way the donation form is currently implemented the _destroy parameter is never sent, the donation object is just overridden with the new line_items_attributes. updated those rspec tests and removed this line in 3560157

dorner · 2024-07-12T19:26:15Z

spec/requests/donations_requests_spec.rb

+        expect(response.body).to include("Editing Donation\n          <small>from #{original_source}")
+        expect(response.body).to include("<li class=\"breadcrumb-item\">\n            <a href=\"#\">Editing #{original_source}")
+        expect(response.body).to include("<option selected=\"selected\" value=\"#{edited_source}\">#{edited_source}</option>")
+        expect(response.body).to include("<option selected=\"selected\" value=\"#{edited_source_drive.id}\">#{edited_source_drive.name}</option>")


I'd prefer to specify the name etc. (anything we're actually checking) when creating the data. This makes the tests less flaky and more easily reproducible.

resolved by 82a76fd

* _destroy parameter is not sent when deleting line items when tested on commit [#f747006](rubyforgood@f747006) (could not find commit when this change occurred) * remove _destroy parameter reference in outdated rspec tests and donations controller code

dorner

Overall looks good, but there's a really huge test I'd like to try to sanitize if possible.

dorner · 2024-07-19T19:19:53Z

spec/requests/donations_requests_spec.rb

+          }
+        }
+
+        put donation_path(id: donation.id, donation: edited_donation)


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?

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.

resolved by ba5af77

* Use TestInventory instead of distribution endpoint in rspec * Use build(:line_item) instead of as_json merge

dorner · 2024-07-26T19:13:22Z

Looks good to me! @cielf did you want to take a look?

cielf · 2024-07-27T05:57:08Z

Passes my manual testing

Pending changes were addressed

awwaiid

All set, thank you!

github-actions · 2024-08-18T14:46:24Z

@jimmyli97: Your PR Fix #4172 donation validation display error is part of today's Human Essentials production release: 2024.08.11.
Thank you very much for your contribution!

jimmyli97 added 3 commits June 27, 2024 19:36

RED donation quantity edit validation request test

fc9cdb5

When edited donation amount plus available inventory is less than requested inventory for distribution, it should redirect to the edit donation page, display an error, the original donated item name, and the original amount

RED fix rubyforgood#4172 displays item name on invalid donation amoun…

cbb2f85

…t edit * direct call to render doesn't load @Items causing partial to be unable to match item IDs to item names, resulting in displaying numbers * replace with redirect to edit

Update bundler version

a25a0d2

jimmyli97 force-pushed the 4172-fix-donation-validation branch from 2b03b55 to e49ef32 Compare June 28, 2024 00:55

GREEN fix donations controller spec to expect redirect

0585857

Error message changes if using events or not, add cases for both

jimmyli97 force-pushed the 4172-fix-donation-validation branch from e49ef32 to 0585857 Compare June 28, 2024 01:25

cielf requested a review from dorner June 29, 2024 19:53

dorner requested changes Jul 4, 2024

View reviewed changes

RED edit rspec to ensure entered user info is kept after donation upd…

672e9b6

…ate error * Page should still display original donation source pre-edit in the title and breadcrumbs

jimmyli97 force-pushed the 4172-fix-donation-validation branch 5 times, most recently from 223b647 to 708bbbb Compare July 12, 2024 03:34

GREEN keeps all entered information after invalid form submission

68cabfb

* Also keeps submitted incorrect quantity

jimmyli97 force-pushed the 4172-fix-donation-validation branch from 708bbbb to 68cabfb Compare July 12, 2024 03:43

jimmyli97 requested a review from dorner July 12, 2024 03:55

dorner requested changes Jul 12, 2024

View reviewed changes

jimmyli97 force-pushed the 4172-fix-donation-validation branch 4 times, most recently from 1920943 to 44a6ef4 Compare July 17, 2024 03:24

jimmyli97 requested a review from dorner July 17, 2024 03:52

jimmyli97 added 3 commits July 17, 2024 19:45

Hardcode names in rspec to make test more reliable

82a76fd

Add to rspec to test if deleted items persist after validation error

dfaa0e7

jimmyli97 force-pushed the 4172-fix-donation-validation branch from 14dfdc6 to dfaa0e7 Compare July 17, 2024 19:45

dorner previously requested changes Jul 19, 2024

View reviewed changes

Simplify rspec

ba5af77

* Use TestInventory instead of distribution endpoint in rspec * Use build(:line_item) instead of as_json merge

jimmyli97 requested a review from dorner July 23, 2024 02:09

jimmyli97 mentioned this pull request Jul 23, 2024

[BUG]: (trying to reproduce) arcane error message #4549

Closed

4 tasks

Merge branch 'main' into 4172-fix-donation-validation

559b67d

awwaiid approved these changes Aug 4, 2024

View reviewed changes

awwaiid merged commit be14a90 into rubyforgood:main Aug 4, 2024
19 checks passed

jimmyli97 deleted the 4172-fix-donation-validation branch August 5, 2024 00:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #4172 donation validation display error #4483

Fix #4172 donation validation display error #4483

jimmyli97 commented Jun 28, 2024 •

edited

Loading

dorner left a comment

dorner Jul 4, 2024

jimmyli97 Jul 4, 2024 •

edited

Loading

dorner Jul 4, 2024

jimmyli97 Jul 12, 2024 •

edited

Loading

dorner Jul 12, 2024

jimmyli97 Jul 16, 2024 •

edited

Loading

dorner Jul 12, 2024

jimmyli97 Jul 17, 2024 •

edited

Loading

dorner left a comment

dorner Jul 19, 2024

jimmyli97 Jul 20, 2024 •

edited

Loading

jimmyli97 Jul 23, 2024

dorner commented Jul 26, 2024

cielf commented Jul 27, 2024

awwaiid left a comment

github-actions bot commented Aug 18, 2024

Fix #4172 donation validation display error #4483

Fix #4172 donation validation display error #4483

Conversation

jimmyli97 commented Jun 28, 2024 • edited Loading

Description

Alternatives considered

Type of change

How Has This Been Tested?

Screenshots

dorner left a comment

Choose a reason for hiding this comment

dorner Jul 4, 2024

Choose a reason for hiding this comment

jimmyli97 Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

dorner Jul 4, 2024

Choose a reason for hiding this comment

jimmyli97 Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

dorner Jul 12, 2024

Choose a reason for hiding this comment

jimmyli97 Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

dorner Jul 12, 2024

Choose a reason for hiding this comment

jimmyli97 Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

dorner left a comment

Choose a reason for hiding this comment

dorner Jul 19, 2024

Choose a reason for hiding this comment

jimmyli97 Jul 20, 2024 • edited Loading

Choose a reason for hiding this comment

jimmyli97 Jul 23, 2024

Choose a reason for hiding this comment

dorner commented Jul 26, 2024

cielf commented Jul 27, 2024

awwaiid left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 18, 2024

jimmyli97 commented Jun 28, 2024 •

edited

Loading

jimmyli97 Jul 4, 2024 •

edited

Loading

jimmyli97 Jul 12, 2024 •

edited

Loading

jimmyli97 Jul 16, 2024 •

edited

Loading

jimmyli97 Jul 17, 2024 •

edited

Loading

jimmyli97 Jul 20, 2024 •

edited

Loading