3992 preserve storage location when editing purchase #4003

PhantomSean · 2023-12-28T11:59:52Z

Resolves #3992

Description

Introduced condition to determine whether a new purchase is being created or if an existing purchase is being edited. If the latter is true the current storage location is displayed rather than defaulting to the organizations storage location/first item on the drop-down.

Type of change

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

How Has This Been Tested?

Visual Test:

Log in as an Organization Admin.
Go to the Purchases page (http://127.0.0.1:3000/diaper_bank/purchases).
View a purchase that has a Storage Location of 'Pawnee Main Bank (Office)'.
Click the 'Make a Correction' button.
Confirm that the Storage Location is correct and that it is not set to the first item in the drop-down list.

Rspec Test:
Added a test to spec/requests/purchases_requests_spec.rb to confirm that storage location is preserved editing a purchase and that the organization's storage location default is selected when creating a purchase.

Screenshots

Before:

After:

dorner

Hi Sean - thanks so much for the PR! The issue here is that the default should be selected for new purchases. It should not be selected when editing existing purchases (instead, the current storage location should be). This PR appears to change it so for new purchases, no storage location is selected by default.

PhantomSean · 2023-12-29T03:54:59Z

Hi @dorner, thanks for the feedback. I've updated my code to allow for new purchases to still default to a storage location. Let me know if this satisfies your request 😄

seanmarcia · 2023-12-29T04:42:32Z

app/views/purchases/_purchase_form.html.erb

+                            collection: @storage_locations,
+                            label: "Storage Location",
+                            error: "Where is it being stored?",
+                            selected: current_organization.intake_location,


We can probably simplify this quite a bit with a ternary on the selected.
I imagine that something like:
selected: @purchase.new_record? ? current_organization.intake_location : @purchase.storage_location_id,
would work.

Hi @seanmarcia, yes you are correct. Thank you for the suggestion, I've implemented the change that you suggested and is working fine.

Thanks! I think the last part of the form to this would be to just move it into a helper :)

No problem, created a helper function for it now.

dorner · 2023-12-29T16:22:58Z

spec/system/purchase_system_spec.rb

+          storage1 = create(:storage_location, name: "storage1")
+          purchase = create(:purchase, storage_location: storage1)
+          visit edit_purchase_path(@organization.to_param, purchase)
+          expect(page).to have_content("storage1")


I'm pretty sure this test will pass regardless of which location is selected. To verify, try taking your code in the previous files out and run the test - if it's still green, the test isn't useful. You'd need to find a better way to indicate that the right storage location is selected (perhaps via an option selected matcher.

Yes you were correct, @dorner. I've updated the test to make sure the expected storage is selected. Thanks for all the feedback, let me know if there's anything else that can be improved on.

Bumps [rubocop-rails](https://github.com/rubocop/rubocop-rails) from 2.23.0 to 2.23.1. - [Release notes](https://github.com/rubocop/rubocop-rails/releases) - [Changelog](https://github.com/rubocop/rubocop-rails/blob/master/CHANGELOG.md) - [Commits](rubocop/rubocop-rails@v2.23.0...v2.23.1) --- updated-dependencies: - dependency-name: rubocop-rails dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Preserved storage location on purchase edit page Improved test Fixing test affected by my change Fixed to reintroduce default storage location for new purchases

Bumps [rubocop-performance](https://github.com/rubocop/rubocop-performance) from 1.19.1 to 1.20.1. - [Release notes](https://github.com/rubocop/rubocop-performance/releases) - [Changelog](https://github.com/rubocop/rubocop-performance/blob/master/CHANGELOG.md) - [Commits](rubocop/rubocop-performance@v1.19.1...v1.20.1) --- updated-dependencies: - dependency-name: rubocop-performance dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Simplified changes Fix issue causing factory_bot_lint failure Moved logic to a helper function

dorner · 2024-01-01T01:33:46Z

spec/system/purchase_system_spec.rb

+          purchase = create(:purchase, storage_location: storage1)
+          visit edit_purchase_path(@organization.to_param, purchase)
+          assert page.has_select? "Storage Location", selected: "storage1"
+        end


Agree with @seanmarcia (his comment has been abandoned since you force pushed the branch - please don't do that with open PRs that have comments!)

You can inspect the rendered response in a request test. If you're not interacting with it in any way, that should be fine. System tests should be limited to interactivity and JavaScript testing since it's slower and more brittle.

Can another test be added to ensure the new behavior is also as expected?

I've moved the test into requests, and added a new test to ensure the new behavior. Let me know if these address the changes you wanted.

dorner

Getting closer! 😄

dorner · 2024-01-05T19:39:34Z

spec/requests/purchases_requests_spec.rb

+        it "storage location defaults to organizations storage location" do
+        purchase = create(:purchase)
+        get edit_purchase_path(@organization.to_param, purchase)
+        assert_select('select#purchase_storage_location_id option[selected]').first[storage_location.name]


I'm not familiar with the assert_select method and not sure I understand what it's doing here specifically (the docs don't seem to indicate this usage). Can you match this selector with the way we do checks elsewhere?

I've updated the tests to use match as is used elsewhere in the tests.

dorner · 2024-01-05T19:39:39Z

spec/requests/purchases_requests_spec.rb

@@ -67,6 +67,12 @@

          expect(Purchase.last.amount_spent_in_cents).to eq 100_054
        end
+
+        it "storage location defaults to organizations storage location" do
+        purchase = create(:purchase)


Spacing seems off here?

Yep, fixed that now

dorner · 2024-01-07T01:46:18Z

@PhantomSean So close! There's a conflict that needs to be solved now.

…purchase

PhantomSean · 2024-01-08T10:05:14Z

@PhantomSean So close! There's a conflict that needs to be solved now.

@dorner Resolved conflict, hopefully all good now. Let me know if there's anything else to be done 😄

…purchase

dorner · 2024-01-08T15:59:41Z

lol now linting is breaking! 😁 I think it would be simpler to just leave the gemfile / gemfile.lock updates out of this PR. It's causing more headaches than it's worth.

…f github.com:PhantomSean/human-essentials into 3992-preserve-storage-location-when-editing-purchase

…purchase

PhantomSean · 2024-01-17T01:32:05Z

lol now linting is breaking! 😁 I think it would be simpler to just leave the gemfile / gemfile.lock updates out of this PR. It's causing more headaches than it's worth.

I believe I've resolved the issues if you're free to review

dorner · 2024-01-18T01:30:14Z

This looks good! Thanks very much!

PhantomSean · 2024-01-18T02:29:43Z

This looks good! Thanks very much!

No problem!

github-actions · 2024-01-21T15:38:33Z

@PhantomSean: Your PR 3992 preserve storage location when editing purchase is part of today's Human Essentials production release: 2024.01.21.
Thank you very much for your contribution!

dorner requested changes Dec 28, 2023

View reviewed changes

seanmarcia reviewed Dec 29, 2023

View reviewed changes

dorner requested changes Dec 29, 2023

View reviewed changes

dependabot bot and others added 3 commits December 30, 2023 11:02

Improving test

af40477

PhantomSean force-pushed the 3992-preserve-storage-location-when-editing-purchase branch from fb6e43b to af40477 Compare December 30, 2023 00:27

Removing trailing whitespace

2c82e31

dorner requested changes Jan 1, 2024

View reviewed changes

Moving tests into requests

7084425

dorner requested changes Jan 5, 2024

View reviewed changes

PhantomSean added 2 commits January 6, 2024 16:39

Changing selector to match other tests

26d1e6f

spacing fix

6127209

Merge branch 'main' into 3992-preserve-storage-location-when-editing-…

18756f3

…purchase

Merge branch 'main' into 3992-preserve-storage-location-when-editing-…

3f86e3e

…purchase

PhantomSean and others added 5 commits January 9, 2024 18:24

Fixing lint error

eb6f156

Merge branch '3992-preserve-storage-location-when-editing-purchase' o…

454fa89

…f github.com:PhantomSean/human-essentials into 3992-preserve-storage-location-when-editing-purchase

Merge branch 'main' into 3992-preserve-storage-location-when-editing-…

901d74a

…purchase

Fixing lint error

b6fb919

Merge branch 'main' into 3992-preserve-storage-location-when-editing-…

79a101b

…purchase

dorner approved these changes Jan 18, 2024

View reviewed changes

dorner merged commit e07d71e into rubyforgood:main Jan 18, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

3992 preserve storage location when editing purchase #4003

3992 preserve storage location when editing purchase #4003

PhantomSean commented Dec 28, 2023 •

edited

Loading

dorner left a comment

PhantomSean commented Dec 29, 2023

seanmarcia Dec 29, 2023 •

edited

Loading

PhantomSean Dec 29, 2023

seanmarcia Dec 29, 2023

PhantomSean Dec 29, 2023

dorner Dec 29, 2023

PhantomSean Dec 29, 2023

dorner Jan 1, 2024

dorner Jan 1, 2024

PhantomSean Jan 4, 2024

dorner left a comment

dorner Jan 5, 2024

PhantomSean Jan 6, 2024

dorner Jan 5, 2024

PhantomSean Jan 6, 2024

dorner commented Jan 7, 2024

PhantomSean commented Jan 8, 2024

dorner commented Jan 8, 2024

PhantomSean commented Jan 17, 2024

dorner commented Jan 18, 2024

PhantomSean commented Jan 18, 2024

github-actions bot commented Jan 21, 2024

3992 preserve storage location when editing purchase #4003

3992 preserve storage location when editing purchase #4003

Conversation

PhantomSean commented Dec 28, 2023 • edited Loading

Description

Type of change

How Has This Been Tested?

Screenshots

dorner left a comment

Choose a reason for hiding this comment

PhantomSean commented Dec 29, 2023

seanmarcia Dec 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dorner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dorner commented Jan 7, 2024

PhantomSean commented Jan 8, 2024

dorner commented Jan 8, 2024

PhantomSean commented Jan 17, 2024

dorner commented Jan 18, 2024

PhantomSean commented Jan 18, 2024

github-actions bot commented Jan 21, 2024

PhantomSean commented Dec 28, 2023 •

edited

Loading

seanmarcia Dec 29, 2023 •

edited

Loading