search

rubyforgood / human-essentials

Human Essentials is an inventory management system for diaper, incontinence, and period-supply banks. It supports them in distributing to partners, tracking inventory, and reporting stats and analytics.
https://humanessentials.app
MIT License
436 stars 449 forks source link

3090 v2 distribution confirmation #4367

Closed danielabar closed 3 days ago

danielabar commented 1 month ago

Resolves #3090

Description

Introduce a Stimulus controller for the new distribution confirmation feature. It intercepts the Save form submission (only for New distributions, not Edit, as per earlier disucssion).

This controller extracts key information from the new distribution form including:

And displays these in a modal asking the user if they're sure this is what they intend.

The user can click "No...", which will close the modal and then they're still on the new form where they can continue filling it out, making changes, etc.

Or the user can click "Yes..." in which case the form is submitted, and then the existing server-side flow runs.

Note: There's a previous PR from an earlier attempt which introduced a new pending status for the Distribution model and saved the distribution in this state as part of the confirmation flow: https://github.com/rubyforgood/human-essentials/pull/4341. But after some discussion, it was decided to go with a simpler client-side JS approach as per this PR.

Type of change

New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Screenshots

Here is an example of the new distribution confirmation modal, shown after user clicks Save: image

cielf commented 1 month ago

I'm waiting to do any more functional reviews until the issue with bin/setup gets addressed. (Just setting expectations)

cielf commented 1 month ago

@daniealbar Thank you very much for your hard work on this so far! (I really appreciate your thoroughness)

Alas, During the earlier discussions, I failed to catch that the confirmation would be before basic error checking and the combination of multiples, etc.

Does it have to be that way, with this approach? (I'm concerned that it won't meet the need to have useful numbers for their final review)

(also calling @dorner into the discussion)

danielabar commented 1 month ago

@cielf Currently the server side part of this performs validations and saves. And this being client side, it runs before all that.

Would a more ideal flow would be, from clicking Save:

  1. Run validations (but do not create new distribution, even if valid)
  2. If invalid, render the form with errors
  3. If valid, render the form with the modal confirmation dialog
  4. From clicking "yes" on modal, again submit to server for validation and creation (should be valid by this point, but just in case since its still a form coming from the client)

It might be possible to introduce a new endpoint for validation only. It may also be possible to cause some Stimulus code to trigger as a result of render (saw something like this in a tutorial, could look into this).

A tricky thing will be the validation though, I ran into this earlier when working on the first attempt pure server side. For basic validation like is partner and storage location filled in, that's straightforward. But for inventory, I it seems to be coupled with actually decrementing the inventory amount in this line in DistributionCreateService: distribution.storage_location.decrease_inventory(distribution.line_item_values).

There's also some validation about this distribution already being fulfilled (if it was created from a request) that also happens together in the creation service, making it difficult to tease apart creation from validation.

There's another complexity with a further inventory check that happens after creation success wrt on-hand minimums. Although that may be only a flash alert after the fact, but doesn't stop creation.

So this might require some server side restructuring of the code to pull out all the distribution validation logic into one place that can be called independently of saving/updating anything.

cielf commented 1 month ago

@danielabar My gut says that would be better. I had hoped to do a bit of UX research today to see if I'm totally off base on this, but did not get to it.

If I'm understanding correctly, It sounds like this would be a bit of a mess, I mean challenge... g and that it that could make the code harder to understand.

The distribution confirmation is a "nice to have" , rather than core functionality. We'll have to make a judgement call on whether the benefit derived is worth the increased complexity of the code. I'd like to get @dorner 's take on this, see if there's an angle you haven't considered, but also to get his read on maintainablity.

I know you've put a lot of work into this one.

dorner commented 1 month ago

I think the idea of separating out validation from creation is a good one. I think it'd make it easier to understand, not harder, because we aren't shoving them all together. I think that should happen in a separate PR though. There also should be a focus on event sourcing, so maybe validating inventory items is not quite as important. (I don't think there's a direct method to say "initialize this event and check if the inventory can handle it", but it's not hard to do - maybe 2-3 lines of code.)

Once we have them separated, the flow becomes a lot easier to work with and we can more easily have that confirmation page.

The fact is that without a way to validate the distribution without creating it, this ticket is pretty much impossible to complete. Whether this is the most important thing for Daniela to spend her time on is a judgment call for @cielf I think.

cielf commented 1 month ago

IIUC, the stuff that we'd have to do to make this happen is a good idea anyway?

I am, however, starting a conversation with @danielabar over in slack about the partner profile rework - which is in the important but we're having trouble getting to it bucket.

dorner commented 1 month ago

OK - I think the action for this should be to create another issue around separating validation from creation logic, and blocking this issue on that one. Prioritization is a separate question.

cielf commented 4 weeks ago

I'm not sure I know enough about this to write it up -- though one question would be whether we want to separate it just for distributions, or across the board of itemizables? (for consistency's sake, I would think the latter)

danielabar commented 3 weeks ago

An attempt at new issue title/description:

Title: Refactor Distribution Creation to Separate Validation from Creation

Label: refactor

Description: Currently validation and creation of a Distribution are coupled, making it difficult to insert a confirmation feature into the workflow as per issue #3090. (i.e. confirmation should only be displayed for a valid, but not yet created, distribution).

Details: When a new distribution form is submitted, the create method of app/controllers/distributions_controller.rb is called. This method creates an instance of a Distribution model from the form parameters, but doesn't call the valid? method on this model. Rather, it calls DistributionCreateService which, among other things does:

What we'd like to do is separate out all the validations so that they can be called separately from creation, ideally from a single call to distribution.valid?. This way when DistributionCreateService calls distribution.save! it runs the exact same set of validations as distribution.valid?.

The benefit is this would allow for a future endpoint that only does validation via distribution.valid?, to build the confirmation feature something like described in this comment.

TODO: Are there any significant differences between Itemizable validation line_items_exist_in_inventory and the validation that's embedded in StorageLocation#decrease_inventory?

As for expanding to all the Itemizables, I haven't had a look but it's possible that the business rules and validation implementation could be different on each, so it may keep the PR more manageable to have separate tickets, starting just with Distribution model, since we know it has the issue of validation coupled with creation.

dorner commented 3 weeks ago

Let's keep it to Distribution.

The inventory thing should not be a focus of this. In event world, we're going to get rid of inventory items entirely, so I'd ignore it as much as possible.

danielabar commented 3 weeks ago

Thanks for the clarification, this simplifies things in that it's only the "is there an associated request that's already fulfilled?" validation that isn't part of the distribution validations.

Thinking about this further, if distribution.valid? already does much of the validation, it might be possible to continue on my original stimulus branch, with having it call a new validation only endpoint. I'll attempt this (i.e. may not need that refactor ticket after all).

danielabar commented 3 weeks ago

@cielf I've made some changes so that first it will run some basic validations on the distribution before showing the modal (from user clicking Save on new distribution).

For now, this runs the existing validations defined on the Distribution model via distribution.valid?. I didn't change any of that logic given earlier comment wrt inventory.

And there might still be a case of "request already fulfilled", but I wasn't sure if that validation actually belongs in distribution model (eg: that would also run on update).

Also wanted to see if this is closer to the desired flow wrt validation. Can you try this out whenever you have a chance. For example, if you leave out Partner and try to Save, it won't show the modal, instead it will show the validation error. Then if you fix the error by filling out partner and Save again, then the modal will show.

cielf commented 3 weeks ago

You're on my list! -- it's a little long at the moment (managing expectations)

cielf commented 3 weeks ago

This is definitely closer! It feels a lot smoother to me at this point.

Is it plausible to handle the case where someone has entered the same item twice before we show the modal?

danielabar commented 3 weeks ago

re: dupe items - How is this handled currently?

I tried on main branch, to create a new distribution with dupe items and it was allowed, didn't see any warning/validation error: image image

cielf commented 2 weeks ago

Yeah -- it is allowed. They currently get smooshed together using the distribution's method combine_distribution, which is called before_save.

("No, it' not plausible" may be a valid answer!)

danielabar commented 2 weeks ago

I had a look at Itemizable#combine!, which is called by Distribution#combine_distribution. I don't think it will be possible to use that from the JS modal as that method is building ActiveRecord models to be associated with the distribution, just before the distribution gets saved. It actually deletes the existing ones it got from the form and builds new one based on the combination, by unique item_id.

But in the confirmation modal, it's client side and only has what it can parse from the DOM. It might be possible to write a similar combine method client side in JS, that would only be used for display purposes in the modal. I can look into this.

danielabar commented 2 weeks ago

Got the combine display working in the confirmation modal, see this commit for details.

danielabar commented 2 weeks ago

Just to clarify, the combining of line items in the modal is display only, it doesn't modify the model because it hasn't gone to the server yet. So if user clicks "No I need to make changes", the line items in the form will be exactly as the user originally entered them.

cielf commented 2 weeks ago

It may be tomorrow before I have a chance to try this out, but it sounds workable to me from a functionality pov

cielf commented 2 weeks ago

Functionality looks good. Over to @dorner for technical review.

danielabar commented 1 week ago

re: that most recent system test failure from CI run: https://github.com/rubyforgood/human-essentials/actions/runs/9536196385/job/26282824787?pr=4367

That test passes locally, and even looking at screenshot from CI artefact download, there is a "Distributions" link that the test should be able to click.

cielf commented 3 days ago

Looks good to me. We can merge after the release.

dorner commented 3 days ago

Awesome! Thanks so much for your perseverance here!