Skip to content

Conversation

@daynew
Copy link
Contributor

@daynew daynew commented Sep 6, 2020

Resolves #1803

Description

When a distribution of materials is scheduled, we want to be able to record how the materials will be delivered. Are the materials being picked up at the bank? Is the bank delivering the materials to the partner organization? Distributions will now be marked as either "Delivery" or "Pick up".

  • Adds thedelivery_method column to the Distribution model.
  • Adds validation that creating or updating a Distribution requires a delivery_method value
    • Existing Distributions will not be back filled, but if an edit to one is made, a delivery_method must be defined.

Follow up work

  • In a follow up PR I will update e-mail templates to change depending the selected delivery method.

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

  • Manually tested by creating, editing, and viewing Distributions.
  • Updated Distribution unit tests to use the new required field.

Screenshots

/diaper_bank/distributions

image

/diaper_bank/distributions/22

image

/diaper_bank/distributions/22/edit

image

/diaper_bank/distributions/pick_ups

image

/diaper_bank/distributions/pickup_day?filters[during]=2020-09-10

image

Pulling from rubyforgood/diaper
@daynew daynew requested a review from edwinthinks September 6, 2020 22:36
@scooter-dangle scooter-dangle added Ruby For Good 2020 Virtual Created for Ruby for Good 2020 Virtual Repo First-timer This Pull Request is a user's first contribution to Diaperbase labels Sep 12, 2020
@daynew daynew requested a review from edwinthinks September 12, 2020 18:46
Copy link
Collaborator

@edwinthinks edwinthinks left a comment

Choose a reason for hiding this comment

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

Hey @daynew -- sorry for the late review. I think this is a great start. I noticed that there are a few other places that we'll want to change before merging in (this can be in another PR into this one).

When you click on the events in the calendar, it brings you to a page that includes pick-up related text. I think we'll need to update the logic on this page.
Screen Shot 2020-09-13 at 9 46 54 AM
Screen Shot 2020-09-13 at 9 47 06 AM

Apologies, I didn't realize that there was another page that would need to get updated.

@daynew
Copy link
Contributor Author

daynew commented Sep 13, 2020

Hey @daynew -- sorry for the late review. I think this is a great start. I noticed that there are a few other places that we'll want to change before merging in (this can be in another PR into this one).

When you click on the events in the calendar, it brings you to a page that includes pick-up related text. I think we'll need to update the logic on this page.
Screen Shot 2020-09-13 at 9 46 54 AM
Screen Shot 2020-09-13 at 9 47 06 AM

Apologies, I didn't realize that there was another page that would need to get updated.

I have updated the screenshots which address the pages you linked. Let me know if the language is okay.

Copy link
Collaborator

@albertchae albertchae left a comment

Choose a reason for hiding this comment

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

LGTM, but will let @edwinthinks make the final call since he reviewed a lot of it before

<div class="row mb-2">
<div class="col-sm-7">
<% content_for :title, "Distribution Pick-Ups - #{current_organization.name}" %>
<% content_for :title, "Distribution Schedule - #{current_organization.name}" %>
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably rename this file too, but that can be a separate PR

Copy link
Collaborator

Choose a reason for hiding this comment

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

@albertchae sounds good. Could either of you create the issue to handle that and label it as tech-debt after this has been merged in?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Finally got around to creating this issue. #1942


# TODO: This should probably be in the Request resource specs, not Distribution
context "When creating a distrubition from a request" do
context "When creating a distribution from a request" do
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice 🔍

@daynew daynew force-pushed the issue-1803-add-delivery-option branch from 4f8d457 to a61dbba Compare September 15, 2020 22:09
@daynew daynew force-pushed the issue-1803-add-delivery-option branch from a61dbba to f9ecb7f Compare September 16, 2020 07:10
Copy link
Collaborator

@edwinthinks edwinthinks left a comment

Choose a reason for hiding this comment

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

Looks good! There are just a few copy changes I can make:

"Your Distribution Has Changed" to "Your Distribution Date Has Changed"

<div class="row mb-2">
<div class="col-sm-7">
<% content_for :title, "Distribution Pick-Ups - #{current_organization.name}" %>
<% content_for :title, "Distribution Schedule - #{current_organization.name}" %>
Copy link
Collaborator

Choose a reason for hiding this comment

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

@albertchae sounds good. Could either of you create the issue to handle that and label it as tech-debt after this has been merged in?

@edwinthinks edwinthinks merged commit 8f1fc0a into rubyforgood:master Sep 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Repo First-timer This Pull Request is a user's first contribution to Diaperbase Ruby For Good 2020 Virtual Created for Ruby for Good 2020 Virtual

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Give the option to select that a distribution will be picked-up or delivered

4 participants