Make purchase helper pick default storage location

[lenikadali]

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

[lenikadali]

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]

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

[cielf]

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

[lenikadali]

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]

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

[lenikadali]

Hi @dorner

Thanks a lot for the review :raised_hands: I think you can give it another look and it should be good :crossed_fingers:

[lenikadali]

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 :crossed_fingers:

[lenikadali]

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

[lenikadali]

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]

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

[lenikadali]

Updated the spec. The pipeline should be green now :crossed_fingers:

[github-actions[bot]]

@lenikadali: Your PR Make purchase helper pick default storage location is part of today's Human Essentials production release: 2024.12.15. Thank you very much for your contribution!