[#3090] Introduce distribution confirmation

danielabar commented 1 month ago

Resolves #3090

Description

This modifies the flow of how new distributions are created as per the motivation explained in the ticket #3090.

The overall approach is to save these "intermediate" distributions in a new pending state in the database, rather than relying on request and/or session as a means of maintaining data across requests. This is because of some text fields on the distribution and many items can get added, that might exceed the max cookie or url size. There was some discussion about this on a previous PR.

After a user clicks Save from the New Distribution view, it will call a new create_pending action on the Distributions controller, which will create the distribution in a pending state (new enum value added), and then redirect to a new Confirmation view for this pending distribution.

Note that at this point, the DistributionCreateService has not yet been called because this would affect inventory and potentially send out notifications, which is not wanted at this point because the distribution hasn't yet been confirmed.

The confirmation view displays the partner the distribution is for, and lists the items/quantities and asks the user if they're sure this is what they want.

If user clicks Yes, then a new action update_from_confirm on the Distributions controller is called, which will now call the DistributionCreateService, which has been modified to update the state to scheduled (as part of the existing transaction so it should get rolled back to pending if anything goes wrong such as insufficient inventory).

If user clicks No, then the existing edit action on the Distributions controller is called. This is because at this point, the distribution has already been created and has an ID, so it's no longer the "new" view. Some existing update logic was modified to handle the pending state differently:

Do not update inventory yet
Replace rather than combine line_items (re: handling of accepts_nested_attributes_for in a one to many)

Note that when user clicks Save from Updating a pending distribution, they will still get the Confirmation view, they can go through this update/confirm/no loop as many times as they like.

Also all queries/views that show lists of distributions were modified to filter out pending.

A follow-on task will be needed to implement a scheduled clean-up job that removes old pending distributions. This is because it's possible for a user to "abandon" a distribution at the confirmation view.

Type of change

New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Many of the distribution system tests were modified to expect the new confirmation view. A new one was added to ensure that an abandoned pending distribution does not show up in the distribution index listing.

Screenshots

Here is the new confirmation view shown after user clicks Save for a new distribution. Notice the url here has the distribution id - because it has been created and saved to the database already:

cielf commented 1 month ago

Ok -- this is beyond what was asked for, but could we add in the storage location? "You are about to create a distribution for [partner name] from [storage location] ",

cielf commented 1 month ago

And... I just noticed that we're using the terminology "to request" -- the issue was cloned from a similar one for requests. Could we change that wording "Please confirm the above list is what you meant to request" to "Please confirm the above list matches the items distributed"?

cielf commented 1 month ago

(thinks) (not a request for change) So... if they walk away from this, we'll have a pending distribution in the database, temporarily. If that's the case, we need to check if there's anything that should not be including those that by default currently is.
There hasn't been anything with a status other than scheduled or completed added to the db since 2020, as far as I can tell -- we may very well have stuff that will inadvertently include these.

dorner commented 1 month ago

Given the pain points with line items and whatnot... should this be a completely separate model (DistributionPreviews) instead? Use a JSON column for the line items and a separate table so it doesn't get confused with "real" distributions?

danielabar commented 1 month ago

(thinks) (not a request for change) So... if they walk away from this, we'll have a pending distribution in the database, temporarily. If that's the case, we need to check if there's anything that should not be including those that by default currently is. There hasn't been anything with a status other than scheduled or completed added to the db since 2020, as far as I can tell -- we may very well have stuff that will inadvertently include these.

I've updated all the places I could find that list distributions to exclude the pending ones, although some manual testing to see if I missed anything could be helpful.

danielabar commented 1 month ago

Given the pain points with line items and whatnot... should this be a completely separate model (DistributionPreviews) instead? Use a JSON column for the line items and a separate table so it doesn't get confused with "real" distributions?

The challenge with a new model with JSON line items could be when user clicks the "No..." button - then that new model with JSON line items would have to get unpacked into a form. And even for the new case, it will still need the existing logic that pre-populates the items based on storage location, which might have to be re-written in a different way to work with the JSON items.

There's also the complexity of maintaining the associated request, because a new distribution can also be created from fulfilling an existing request.

I could try but this would be almost a new branch to try it on. And my gut sense from working on this so far is it might introduce a new set of problems and not necessarily any easier.

danielabar commented 1 month ago

Given the pain points with line items and whatnot... should this be a completely separate model (DistributionPreviews) instead? Use a JSON column for the line items and a separate table so it doesn't get confused with "real" distributions?

Another thought: Perhaps this could be done entirely with JavaScript. Brainstorming "out loud", something like:

Add a stimulus controller that intercepts the new distribution form submission
Since the project already has Bootstrap, use it to show a modal
The content of the modal will come from the DOM (not sure if its possible to do client-side templating in this context, needs some thought)
If user clicks "No..." from the modal, it simply closes, and user is still on the New form (since there was no form action, state, transition etc)
If user clicks "Yes...", then use JS to submit the form (although this will be xhr), need to figure out how to get the regular Rails render or redirect from here (because there still could be validation errors which would have to leave the user on New to fix them)

This might simplify things because then it doesn't require introduction of a new state or model. Everything is entirely held in client/browser until user clicks "Yes...". (the complexity then moves to the JS side as to how to render a nice looking modal with the content extracted from the DOM).

dorner commented 1 month ago

Been thinking through the different options and none of them are super great. I think the least terrible one might be to make sure that distributions become cognizant of state, and don't run certain callbacks unless the state is "confirmed" or similar. I don't love it, but all the other options seem worse. 😢

cielf commented 1 month ago

Pointing out, in case it makes a difference in the least of evils discussion -- that we are going to be doing a very similar thing for the requests as well.

danielabar commented 1 month ago

@dorner @cielf I've been experimenting with the StimulusJS solution and I think it can be done with a nice UX and lower risk to the project because the confirmation remains in the client/browser.

The server side solution ends up touching not just the distribution, but also the processing of the items that affects inventory, and request fulfillment. It also requires changes throughout the system to filter out pending distributions, and follow-on work to periodically clean them up. This branch does that (one system test was failing that needs more work).

However, I think the JS solution can be simpler - here is a commit that implements distribution confirmation it with a client side modal.

Here's a screenshot of what the user would see from clicking "Save" from a new distribution:

Notice the url - they're still on the /new view so nothing has changed on the server (yet). If they need to make changes, clicking "No" simply closes the modal.

If they click "Yes", then it proceeds with the existing distribution flow as usual.

We'll need to consider whether this modal should also show up when user is editing an existing distribution. In the server side solution I made the confirmation only show up for new/pending.

Let me know if I should proceed with the JS solution. btw - similar could be done for any entity that requires a confirmation. Currently it's specific to distribution confirmation, but potentially the stimulus controller could be made more generic to handle any itemizable confirmation.

cielf commented 1 month ago

On the surface of it, that sounds great, and I certainly want @dorner's take on it. Other possible concerns would be if we're going to have people having issue with browsers other than Chrome (even though we only officially support Chrome, we have a number of engaged users who use Firefox or Safari, and only switch to Chrome if something doesn't work).

As to whether it should also show up on edits -- that's a very good question. I'm leaning toward no, but I'd like to think about it. I can certainly see an argument both ways.

danielabar commented 1 month ago

I tried the JS solution with Safari and Firefox, it works.

dorner commented 1 month ago

OK, I'm sold. This definitely looks like a much less heavy way to solve it. Kudos!

I'm a little confused as to why the form was so heavily refactored though... don't we just need to add the modal and populate it?

danielabar commented 1 month ago

OK, I'm sold. This definitely looks like a much less heavy way to solve it. Kudos!

I'm a little confused as to why the form was so heavily refactored though... don't we just need to add the modal and populate it?

That was because the stimulus controller needs access to both the form and modal elements, and that form partial only had the form element:

<form>
  ...
</form>

Since I wasn't sure if its valid to put a modal in the form, I had modified it to add a wrapper div to attach the controller, which would then have access to both the form and modal:

<div data-controller="distribution-confirmation">
  <form>...</form>
  <modal>...</modal>
</div>

However, the data-controller can actually go in the new view that renders the form partial, then it will have access to both form and modal in the partial without requiring another wrapper div. This also solves the issue of new vs edit, in that the confirmation will only show up for the new case (although it could be added for edit later if that's also a requirement).

This commit shows the update with less changes to the form partial.

danielabar commented 1 month ago

Closing this PR as its now replaced by: https://github.com/rubyforgood/human-essentials/pull/4367

rubyforgood / human-essentials