[FEATURE] Allow essential banks to review their distribution before submitting it

[cielf]

Summary

Add a confirmation step to distributions

Justification

Non-profits are often staffed with folks with limited technical ability and sometimes make mistakes with their distributions This puts and extra burden on the org admin person as they then need to go in and correct the distribution. We need to add in a confirmation page summarizing the distribution and asking for confirmation from the bank user before this distribution is finalized.

Details

It will show a list of the items in the distribution and number for each, along with the text "Please confirm that the above list is what you meant to request" and buttons "No, I need to change something." and "Yes, it's right"

Clicking "No..." will take them back to the distributions page they came from so that they can adjust their ask.

Clicking "Yes" will submit the distribution and show the submitted page for that distribution as it does now.

Things we should know

There is a similar issue in the works for partners confirming requests #3052

Criteria for Completion

• [ ] Add in a confirmation page when banks enter a distribution, as described above
• [ ] Add in relevant tests
• [ ] Tests pass

[HankSml]

Hi, I would like to work on this issue please! :)

[cielf]

Hi, I would like to work on this issue please! :)

@HankSml Please do.

[github-actions[bot]]

This issue has been inactive for 243 hours (10.13 days) and will be automatically unassigned after 117 more hours (4.88 days).

[github-actions[bot]]

This issue has been inactive for 363 hours (15.13 days) and is past the limit of 360 hours (15.00 days) so is being unassigned.

[tacoda]

Hi, I'd like to pick up this issue.

[cielf]

@tacoda Please do!

[github-actions[bot]]

This issue has been inactive for 252 hours (10.50 days) and will be automatically unassigned after 108 more hours (4.50 days).

[github-actions[bot]]

This issue has been inactive for 372 hours (15.50 days) and is past the limit of 360 hours (15.00 days) so is being unassigned.

[tacoda]

👋 I didn't get the chance to reply to this before it unsigned me. I'm still working on this.

[github-actions[bot]]

This issue has been inactive for 249 hours (10.38 days) and will be automatically unassigned after 111 more hours (4.63 days).

[tacoda]

Still working on this. I've got some time set aside for this in the next few days to get it moved forward.

[github-actions[bot]]

This issue has been inactive for 245 hours (10.21 days) and will be automatically unassigned after 115 more hours (4.79 days).

[github-actions[bot]]

This issue has been inactive for 365 hours (15.21 days) and is past the limit of 360 hours (15.00 days) so is being unassigned.

[lokisk1155]

I can take this one! #3052 is in its final leg!

[github-actions[bot]]

This issue has been inactive for 262 hours (10.92 days) and will be automatically unassigned after 98 more hours (4.08 days).

[github-actions[bot]]

This issue is marked as stale due to no activity within 30 days. If no further activity is detected within 7 days, it will be unassigned.

[github-actions[bot]]

Automatically unassigned after 7 days of inactivity.

[danielabar]

I'd like to pick this up.

[dorner]

Go for it!

[danielabar]

What should happen in the following scenario?

1. User fills out new distribution form and clicks Save
2. They get navigated to the confirmation view
3. They click the "No, I need to change something" button
4. They get navigated to the Edit Distribution view where they can make changes and click Save

At this point, should the Distribution just be saved and user navigated to Show view? OR Should they again be navigated to the Confirmation view? (i.e. let them go through the edit -> confirm -> edit loop as many times as they'd like)

[cielf]

A fine question -- I can see arguments for either way. Navigating to the confirmation view again is the path of least surprise, so let's do that (also, I suspect the simpler solution).

[danielabar]

I have the overall flow working but the inventory update is not quite right. If the create and update actions were only creating/updating the distribution model, it might be more straightforward.

But both the create and update actions do more than that. There's also optional association to a request, inventory incrementing/decrementing, and that inventory maintenance seems tied to associating the line items with the distribution model, and potentially sending notifications.

So to split up the process into a Confirm step (that also has a button that goes to the Edit view) may require reviewing all the code in the controller create and update actions, and associated services and deciding which of that code needs to run:

1. In the transition from creating a new distribution to the confirmation view
2. From clicking "Yes it's right" in the confirm view
3. From clicking "No, I need to change something" in the confirm view, making the change, and then clicking Save, which goes to the Confirm view again.

The problem I'm running into with inventory quantity is in the third scenario above, where user goes back to edit something on the pending distribution. In this case, the DistributionCreateService hasn't yet run (because that sends notifications), which means it hasn't yet run distribution.storage_location.decrease_inventory(distribution.line_item_values). But there could be a line item with a quantity such as 10 on the pending distribution. Then let's say user changes it to 20.

Then from the edit screen, they click Save which runs the update action. This calls DistributionUpdateService, which runs this code:

    ItemizableUpdateService.call(
        itemizable: distribution,
        params: @params,
        type: :decrease,
        event_class: DistributionEvent
      )

This ends up only decrementing the inventory quantity by 10 rather than 20 because its doing a comparison of what's already saved on the distribution vs the new amount. Another thing - I tried skipping this code for pending distribution but then it doesn't associate the new line items from the form params. It seems like the above code both increments/decrements inventory and associates line items from the form params.

Then the user is again taken to Confirm screen and if they click "Yes it's right", then it runs my new code which will call DistributionCreateService, which performs a decrement on the new line item value (which would be 20 in this case) so the inventory amount is incorrect.

[dorner]

The inventory stuff is a lot less onerous with event sourcing turned on. Can you try it with that and see if there are still issues? If it's easier, we can delay releasing this until event sourcing is available to everyone. We could use a feature flag if it'll be ready sooner than we can work with it.

[danielabar]

I may have found the issue, was even before inventory updating. There's an AR callback on the distribution model:

before_save :combine_distribution, :reset_shipping_cost

Which ends up calling this method in the Itemizable concern:

    def combine!
        # Bail if there's nothing
        return if size.zero?

        # First we'll collect all the line_items that are used
        combined = {}
        parent_id = first.itemizable_id
        each do |line_item|
          next unless line_item.valid?
          next unless line_item.quantity != 0

          combined[line_item.item_id] ||= 0
          combined[line_item.item_id] += line_item.quantity
        end
        # Delete all the existing ones in this association -- this
        # method aliases to `delete_all`
        clear
        # And now recreate a new array of line_items using the corrected totals
        combined.each do |item_id, qty|
          build(quantity: qty, item_id: item_id, itemizable_id: parent_id)
        end
      end

But if this logic runs for a pending distribution that never yet ran through the creation service, the line_item quantities will be incorrect. For example:

1. Create a distribution with one item A, quantity 10, click Save
2. Behind the scenes, distribution gets created in pending status, but does NOT call the creation service (because that affects inventory and sends notifications). This is in a new create_maybe action in controller (probably needs a better name).
3. User is now on the confirmation screen that shows item A, quantity 10
4. User clicks "No I want to make changes"
5. Now they're on the edit distribution view (because the distribution has an id at this point).
6. User changes item A quantity to 20, click Save
7. Behind the scenes, the existing update logic runs (except I changed it to not update inventory because user hasn't confirmed yet).
8. As part of existing update logic, that before_save callback runs. It sees distribution has item A, quantity 10, then it sees item A, quantity 20 in params, and adds them for a new line item A, quantity 30
9. Now the confirmation view will show quantity 30, which is not what the user meant. When editing a pending distribution, they wanted to replace the quantity of 10 with quantity of 20.

I have a fix with a different callback to run in the case of a pending that replaces line items instead of combining.

And after that the previously failing system test ./spec/system/distribution_system_spec.rb:616 # Distributions allows completion of corrected distribution with depleted inventory item is now passing.

The overall code could definitely use some improvement, although at this point I was thinking to try and get all system tests passing first, then could come back and improve the code.

Let me know if it would be helpful to submit a Draft PR to review work in progress.

[dorner]

That behavior seems pretty weird. Why would editing a distribution and changing a line item quantity result in adding the old and new together?

@cielf any ideas?

[danielabar]

That combine! method got introduced from this 2017 PR: https://github.com/rubyforgood/human-essentials/pull/137 Although that was a refactor so maybe the logic was there from even earlier than that.

[cielf]

This is the code that combines the quantities if you have multiple entries with the same item, right.
Well, If we were editing (at least in pre-event sourcing days), I'm pretty sure the line items get deleted and re-added. [That's why we've run into so many problems with people not being able to edit purchases and donations even when they aren't changing the numbers).

[cielf]

I guess the question is why would we have both the old and the new line items when we are stepping through them here? (I'm assuming that's what's happening)

[github-actions[bot]]

Automatically unassigned after 7 days of inactivity.

[danielabar]

I'm still working on this, discussion has moved to the first attempt PR, and I'm WIP on the second solution: https://github.com/rubyforgood/human-essentials/pull/4341#issuecomment-2105745006