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

Prevent import of inventory if storage location already has items in it and double importing #4300

Conversation

vincent-truong-dev
Copy link
Contributor

@vincent-truong-dev vincent-truong-dev commented Apr 22, 2024

Fixes #3687

Partially borrowed from #3696

Description

Importing storage location inventory can have 2 weird behaviours

  • If imported in two separate tabs of the same storage location, the inventory is doubled when it should be one import. This isn't handled by the current front end check
  • If Import CSV is double clicked, there is double inventory when it should be just one import.

The fix is

  • A back end check that redirects to the page with an error if the storage location already has inventory
  • Disabling the Import CSV button with a greyed out look and text once clicked.

Type of change

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

How Has This Been Tested?

Double tab issue

  1. Make sure a storage location has already been setup
  2. Navigate to http://localhost:3000/diaper_bank/storage_locations/<id> to show an individual storage location in both tabs
  3. Click `Import Baseline Inventory' in both tabs
  4. Upload a sample csv
  5. Click Import CSV in both tabs
  6. In first tab, see the import is successful
  7. In second tab, see error message in screenshot is displayed

Double click issue

  1. Make sure a storage location has already been setup
  2. Navigate to http://localhost:3000/diaper_bank/storage_locations/<id> to show an individual storage location in one tab
  3. Click `Import Baseline Inventory' in
  4. Upload a sample csv
  5. Try to double click Import CSV in one tab
  6. See you can't double click the button as it is disabled and says Importing like in screenshot.

Screenshots

Screenshot 2024-04-23 at 5 25 50 PM Screenshot 2024-04-23 at 5 32 20 PM

<i class="fa fa-upload"></i> Import CSV
<% end %>
<li style="margin-bottom: 27px;">Then click the "Import CSV" button to import your <%= import_type.downcase %>.</li>
<%= button_tag class: "btn btn-md btn-info", data: { disable_with: "Importing" } do %>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only thing changed was adding data: { disable_with: "Importing" }, otherwise rearranging the button tag to be in its own line.

Looked into writing a system spec for this but it's near impossible to trigger a double click the way we want . The various double click methods (.click.click, double_click, or using js trigger or execute_script) don't actually let you double click. Neither double clicks results in two backend calls, only once.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that's fine - if we are validating this on the backend, that's the most important check. Frontend is more of a nice to have.

@vincent-truong-dev vincent-truong-dev marked this pull request as ready for review April 23, 2024 23:35
@vincent-truong-dev vincent-truong-dev changed the title Prevent import of inventory if storage location already has items in it Prevent import of inventory if storage location already has items in it and double importing Apr 23, 2024
@@ -132,6 +132,11 @@ def self.import_csv(csv, organization)
# @param loc [Integer] StorageLocation ID
# @return [void]
def self.import_inventory(filename, org, loc)
storage_location = StorageLocation.find(loc.to_i)
storage_location_exists = storage_location.size > 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need this to work both with and without event sourcing. Can you use the method empty_inventory?instead to check this? That's works with both.

<i class="fa fa-upload"></i> Import CSV
<% end %>
<li style="margin-bottom: 27px;">Then click the "Import CSV" button to import your <%= import_type.downcase %>.</li>
<%= button_tag class: "btn btn-md btn-info", data: { disable_with: "Importing" } do %>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that's fine - if we are validating this on the backend, that's the most important check. Frontend is more of a nice to have.

spec/models/storage_location_spec.rb Outdated Show resolved Hide resolved
spec/requests/storage_locations_requests_spec.rb Outdated Show resolved Hide resolved
cielf
cielf previously requested changes Apr 27, 2024
Copy link
Collaborator

@cielf cielf left a comment

Choose a reason for hiding this comment

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

Hey @vincent-truong-dev - I noticed that rubocop is failing with your latest -- you'll need to address that.

@vincent-truong-dev
Copy link
Contributor Author

@cielf @dorner all green now. I encountered a transient system spec failure in https://github.com/rubyforgood/human-essentials/actions/runs/8882912643/job/24388616782, worth creating a ticket for?

@cielf
Copy link
Collaborator

cielf commented Apr 30, 2024

@cielf @dorner all green now. I encountered a transient system spec failure in https://github.com/rubyforgood/human-essentials/actions/runs/8882912643/job/24388616782, worth creating a ticket for?

Thanks for pointing it out! I'm pretty sure that's the one that PR #4268 is addressing, so no need to create a ticket.

@dorner
Copy link
Collaborator

dorner commented May 2, 2024

All good!

@dorner dorner dismissed cielf’s stale review May 2, 2024 21:06

Rubocop is fixed

@dorner dorner merged commit 8ce0db4 into rubyforgood:main May 2, 2024
19 checks passed
Copy link
Contributor

github-actions bot commented May 5, 2024

@vincent-truong-dev: Your PR Prevent import of inventory if storage location already has items in it and double importing is part of today's Human Essentials production release: 2024.05.05.
Thank you very much for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Inventory imports should prevent modifying already-existing inventory
3 participants