Make purchase helper pick default storage location #4722

lenikadali · 2024-10-14T19:11:09Z

Resolves #4436

Description

Type of change

Bug fix (non-breaking change which fixes an issue)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Screenshots

Changed the purchase helper to use the default storage location of the organization first, before using any other storage locations that are available. Also added a (failing) spec to check that we can see the option on the page. Should be fixed in the next commit

spec/requests/purchases_requests_spec.rb

Fixed the failing test by using the extended format of the test expectation and used `subject` to make an assertion regarding the response body.

lenikadali · 2024-10-15T18:27:09Z

The next step here is to apply the default storage location (if set) to donations.

I see that the donations form was implemented differently from the purchases form in that we directly set the location in the form as shown here while the purchases form uses a helper function as shown here.

The open question: should we switch the donation form to use a (new) method declared in app/helpers/donations_helper.rb or directly check that the organization has a default storage location in the form?

dorner · 2024-10-22T13:44:50Z

Wouldn't switching the donation form to use the same helper as purchases do what we need here?

cielf · 2024-10-22T19:22:19Z

I don't know of any functional reason they shouldn't behave exactly the same, fwiw.

Added a test that when we begin to make a new donation, we have a default storage location set.

Switched to use the helper method from PurchasesHelper to set the default storage location for a donation

lenikadali · 2024-10-23T16:19:24Z

Wouldn't switching the donation form to use the same helper as purchases do what we need here?

I think so. Why I would be hestitant to use it is separation of concerns etc.

Also something strange: in commit 231b516, I added a test to check that the default storage location was being used by checking the content of the form. The test passed 🤔 which made me think that there may be something I am missing regarding the behaviour of the new donation form versus the new purchases form.

Anyway in the subsequent commit e048651, I updated the form to use the method from the PurchasesHelper module and the test still passes.

This should be good for another review 🙏🏿

cielf · 2024-10-23T17:13:33Z

Hey @lenikadali -- when a PR is ready for review, please change its status to "Open".

…onation

cielf

LGTM from a functional POV

dorner

Minor suggestion, otherwise looks good.

app/views/donations/_donation_form.html.erb

Addressed review feedback namely: * Renaming new_purchase_default_location to set_default_location and moving it to ApplicationHelper. This allows us to use across both the donation and purchase forms. * Updated the tests and forms accordingly * Added specs to ApplicationHelper to test that we are able to successfully set the default storage location for purchases and donations.

lenikadali · 2024-11-13T10:55:05Z

Hi @dorner

Thanks a lot for the review 🙌 I think you can give it another look and it should be good 🤞

app/helpers/application_helper.rb

dorner · 2024-11-16T23:12:36Z

spec/helpers/application_helper_spec.rb

+    end
+  end
+
+  describe "#set_default_location for donation" do


I don't think we need to copy the same test for all things that have storage locations - all we're doing is referencing the storage_location_id of that thing.

I've opted to keep one test as per commit 2716706 so that we guarantee the functionality of the method and any subsequent changes to it.

Let me know if that is in keeping with what you were suggesting or your proposal was to remove the test altogether.

Renamed the method as per review suggestion and keep only one test since we only need to test that the default storage location is successfully set

…onation

dorner

Looks good!

dorner

oof... @lenikadali tests are failing now. :(

lenikadali · 2024-11-22T13:06:31Z

oof... @lenikadali tests are failing now. :(

Should be green in the next run @dorner

It seems I forgot to update the other usages of set_default_location to `default_location. Commit b88b36c should let the next run pass 🤞

lenikadali · 2024-11-25T19:33:25Z

Found that I hadn't update the view code to use the new method; hopefully the pipeline passes now 🤞
Thanks a lot for your patience everybody 🙌

lenikadali · 2024-11-29T15:16:16Z

Took a look at the last failing spec and I am not sure how to fix it. Currently if you make the following changes to the spec/requests/donations_requests_spec.rb:

-        expect(response.body).to include("<option selected=\"selected\" value=\"#{edited_source_drive_participant.id}\">#{edited_source_drive_participant_business_name}</option>")
+        expect(response.body).to include("<option value=\"#{edited_source_drive_participant.id}\">#{edited_source_drive_participant_business_name}</option>")

The spec will pass. Probably because we've changed the form's behaviour:

-                        selected: donation_form.product_drive_participant_id,
+                        selected: default_location(@donation),

Should I update the spec? (I am not sure how or what the desired behaviour in this case would be)

dorner · 2024-11-30T23:09:03Z

@lenikadali yes, let's update the spec so it matches the new behavior.

Updated spec to match the new behaviour of the donation form.

lenikadali · 2024-12-02T19:40:55Z

Updated the spec. The pipeline should be green now 🤞

lenikadali force-pushed the 4436-default-storage-location-for-purchase-donation branch from 8f44b71 to bf3a652 Compare October 14, 2024 19:15

lenikadali commented Oct 14, 2024

View reviewed changes

spec/requests/purchases_requests_spec.rb Show resolved Hide resolved

Fix test by using extended format and subject

fbab592

Fixed the failing test by using the extended format of the test expectation and used `subject` to make an assertion regarding the response body.

lenikadali added 2 commits October 23, 2024 18:36

Add test that new donation has default storage location

231b516

Added a test that when we begin to make a new donation, we have a default storage location set.

Switch to use helper to set default storage location

e048651

Switched to use the helper method from PurchasesHelper to set the default storage location for a donation

lenikadali marked this pull request as ready for review October 23, 2024 18:45

Merge branch 'main' into 4436-default-storage-location-for-purchase-d…

7a0ccd4

…onation

cielf self-requested a review November 7, 2024 14:19

cielf reviewed Nov 7, 2024

View reviewed changes

dorner requested changes Nov 9, 2024

View reviewed changes

app/views/donations/_donation_form.html.erb Outdated Show resolved Hide resolved

dorner requested changes Nov 16, 2024

View reviewed changes

lenikadali added 2 commits November 18, 2024 19:37

Rename method, have only one test

2716706

Renamed the method as per review suggestion and keep only one test since we only need to test that the default storage location is successfully set

Merge branch 'main' into 4436-default-storage-location-for-purchase-d…

dfd41ca

…onation

dorner approved these changes Nov 22, 2024

View reviewed changes

dorner requested changes Nov 22, 2024

View reviewed changes

Update set_default_location to default_location

b88b36c

Update method references in view code

7cf2b84

Update spec to match new behaviour

f2bd834

Updated spec to match the new behaviour of the donation form.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make purchase helper pick default storage location #4722

Make purchase helper pick default storage location #4722

lenikadali commented Oct 14, 2024

lenikadali commented Oct 15, 2024

dorner commented Oct 22, 2024

cielf commented Oct 22, 2024

lenikadali commented Oct 23, 2024

cielf commented Oct 23, 2024

cielf left a comment

dorner left a comment

lenikadali commented Nov 13, 2024

dorner Nov 16, 2024

lenikadali Nov 18, 2024

dorner left a comment

dorner left a comment

lenikadali commented Nov 22, 2024

lenikadali commented Nov 25, 2024

lenikadali commented Nov 29, 2024 •

edited

Loading

dorner commented Nov 30, 2024

lenikadali commented Dec 2, 2024

Make purchase helper pick default storage location #4722

Are you sure you want to change the base?

Make purchase helper pick default storage location #4722

Conversation

lenikadali commented Oct 14, 2024

Description

Type of change

How Has This Been Tested?

Screenshots

lenikadali commented Oct 15, 2024

dorner commented Oct 22, 2024

cielf commented Oct 22, 2024

lenikadali commented Oct 23, 2024

cielf commented Oct 23, 2024

cielf left a comment

Choose a reason for hiding this comment

dorner left a comment

Choose a reason for hiding this comment

lenikadali commented Nov 13, 2024

dorner Nov 16, 2024

Choose a reason for hiding this comment

lenikadali Nov 18, 2024

Choose a reason for hiding this comment

dorner left a comment

Choose a reason for hiding this comment

dorner left a comment

Choose a reason for hiding this comment

lenikadali commented Nov 22, 2024

lenikadali commented Nov 25, 2024

lenikadali commented Nov 29, 2024 • edited Loading

dorner commented Nov 30, 2024

lenikadali commented Dec 2, 2024

lenikadali commented Nov 29, 2024 •

edited

Loading