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

[Issue 2899] Distribution by counties! #3328

Merged
merged 27 commits into from
May 5, 2023
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
40acd69
Distribution by counties!
cielf Dec 17, 2022
444ccfc
clearing factory_bot:lint error (all tests still pass)
cielf Jan 15, 2023
3a99547
Low hanging fruit from review of distribution counties feature
cielf Feb 9, 2023
94a1860
Distribution by counties! rebasing onto main
cielf Dec 17, 2022
d195feb
clearing factory_bot:lint error (all tests still pass)
cielf Jan 15, 2023
c22c5d9
Low hanging fruit from review of distribution counties feature
cielf Feb 9, 2023
825acd5
Merge remote-tracking branch 'origin/Distribution_report' into Distri…
cielf Feb 17, 2023
ac03d8c
reworking distribution_by_county_report_service to use a struct rathe…
cielf Feb 17, 2023
a6322c7
Renamed served_areas table to partner_served_areas and added indices …
cielf Feb 19, 2023
9b4d82e
converted to stimulus controller
cielf Feb 24, 2023
676f088
Merge branch 'main' into Distribution_report
cielf Feb 24, 2023
d0f635b
fixing issue with removal of served areas by making the value on the …
cielf Feb 24, 2023
0457a8f
Merge branch 'main' into Distribution_report
cielf Feb 27, 2023
d88f440
Changed the factory for counties to be a sequence instead of a unique…
cielf Mar 6, 2023
ebc3707
Merge branch 'main' into Distribution_report
cielf Mar 26, 2023
f83c4a5
Update lib/tasks/load_us_counties.rake
cielf Mar 27, 2023
cfeef34
Update app/views/distributions_by_county/report.html.erb
cielf Mar 27, 2023
925fc9c
General cleanup and responses to reviews
cielf Apr 1, 2023
8fca5cf
Merge branch 'main' into Distribution_report
cielf Apr 3, 2023
ae485a9
changing field prefix in tests from "profile" to "partner_profile" in…
cielf Apr 4, 2023
43e7e49
Update app/models/partners/profile.rb
cielf Apr 19, 2023
03df861
changes from @dorners' review
cielf Apr 19, 2023
640b583
changes from @dorner's review of 2023-04-21
cielf Apr 21, 2023
cdcad77
renaming spec to match tested service
cielf Apr 28, 2023
7cb003f
Merge branch 'main' into Distribution_report
cielf Apr 28, 2023
871cd56
tweeked profiles_requests_spec to match empty string instead of nil
cielf Apr 28, 2023
6acbf89
Merge branch 'main' into Distribution_report
cielf May 5, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions app/assets/stylesheets/custom.scss
Original file line number Diff line number Diff line change
Expand Up @@ -63,3 +63,11 @@
.modal-body-warning-text {
color: #ee5034
}

.numeric {
text-align: right;
}

.percent {
text-align: right;
}
10 changes: 10 additions & 0 deletions app/controllers/distributions_by_county_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
class DistributionsByCountyController < ApplicationController
include DateRangeHelper
include DistributionHelper

def report
setup_date_range_picker
distributions = current_organization.distributions.includes(:partner).during(helpers.selected_range)
@breakdown = DistributionByCountyReportService.new.get_breakdown(distributions)
end
end
11 changes: 9 additions & 2 deletions app/controllers/partners/profiles_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,19 @@ module Partners
class ProfilesController < BaseController
def show; end

def edit; end
def edit
@counties = County.all
@client_share_total = current_partner.profile.client_share_total
end

def update
if current_partner.update(partner_params) && current_partner.profile.update(profile_params)
@counties = County.all
result = PartnerProfileUpdateService.new(current_partner, partner_params, profile_params).call
if result.success?
flash[:success] = "Details were successfully updated."
redirect_to partners_profile_path
else
flash[:error] = "There is a problem. Try again: %s" % result.error
render :edit
end
end
Expand Down Expand Up @@ -89,6 +95,7 @@ def profile_params
:enable_child_based_requests,
:enable_individual_requests,
:enable_quantity_based_requests,
served_areas_attributes: %i[county_id client_share _destroy],
documents: []
).select { |_, v| v.present? }
end
Expand Down
10 changes: 8 additions & 2 deletions app/controllers/profiles_controller.rb
Original file line number Diff line number Diff line change
@@ -1,14 +1,19 @@
class ProfilesController < ApplicationController
def edit
@partner = current_organization.partners.find(params[:id])

@counties = County.all
@client_share_total = @partner.profile.client_share_total
end

def update
@counties = County.all
@partner = current_organization.partners.find(params[:id])
if @partner.update(edit_partner_params) && @partner.profile.update(edit_profile_params)
result = PartnerProfileUpdateService.new(@partner, edit_partner_params, edit_profile_params).call
if result.success?
redirect_to partner_path(@partner) + "#partner-information", notice: "#{@partner.name} updated!"
else
flash[:error] = "Something didn't work quite right -- try again?"
flash[:error] = "Something didn't work quite right -- try again? %s " % result.error
render action: :edit
end
end
Expand Down Expand Up @@ -100,6 +105,7 @@ def edit_profile_params
:enable_child_based_requests,
:enable_individual_requests,
:enable_quantity_based_requests,
served_areas_attributes: %i[county_id client_share _destroy],
documents: []
).select { |_, v| v.present? }
end
Expand Down
2 changes: 2 additions & 0 deletions app/helpers/distributions_by_county_helper.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
module DistributionsByCountyHelper
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 we should delete this file; it was probably added automatically.

end
25 changes: 25 additions & 0 deletions app/helpers/ui_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -178,4 +178,29 @@ def optional_data_text(field)
tag.span("Not-Provided", class: "text-muted font-weight-light")
end
end

def add_served_area_button(form, node, options = {})
text = options[:text] || "Add another county"
size = options[:size] || "md"
type = options[:type] || "primary"
partial = options[:partial] || "served_areas/served_area_fields"
link_to_add_association form, :served_areas,
data: {
association_insertion_node: node,
association_insertion_method: "append"
}, id: "__add_partner_served_area", class: "btn btn-#{size} btn-#{type} add-partner-served_area", partial: partial do
fa_icon "plus", text: text
end
end

def delete_served_area_button(form, options = {})
text = options[:text] || "Remove"
size = options[:text] || "sm"
type = options[:type] || "danger"

link_to_remove_association form, class: "btn btn-#{size} btn-#{type} remove_served_area", style: "width: 100px;",
"data-action": "click->served-area#zeroShareValue click->area-served#calculateClientShareTotal" do
fa_icon "trash", text: text
end
end
end
28 changes: 28 additions & 0 deletions app/javascript/controllers/area_served_controller.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import { Controller } from "@hotwired/stimulus"
export default class extends Controller {
static targets= ["share", "total", "warning"]
connect () {
this.calculateClientShareTotal()
}
calculateClientShareTotal(){
let total = 0;
let share_targets = this.shareTargets
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line isn't really necessary since the next line can just be this.shareTargets.forEach(

share_targets.forEach( share_target =>{
Copy link
Collaborator

Choose a reason for hiding this comment

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

The whitespace formatting/indent of this whole file seems a bit off - please switch to 2-spaces and on line-10 here add in a newline I think. [this is not a required change, but if you're in here anyway....]

if(share_target.value){
total += parseInt(share_target.value);
}
}
)

this.totalTarget.innerHTML = total + " %"

if(total == 0 || total == 100){
awwaiid marked this conversation as resolved.
Show resolved Hide resolved
this.warningTarget.style.visibility= 'hidden';
}else {
this.warningTarget.style.visibility= 'visible';
this.warningTarget.style.color= 'red';
}
return total;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

In general there are quite a lot of spacing fixes I'd put here (we don't have a JS linter):

  • A space before and after =>
  • A space before and after { and } 
  • A space before ( and after )
  • A space before and after =

Copy link
Collaborator

Choose a reason for hiding this comment

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

Still missing some spaces before = 😄


}
11 changes: 11 additions & 0 deletions app/javascript/controllers/served_area_controller.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import { Controller } from "@hotwired/stimulus"
export default class extends Controller {
static targets= ["share"]

connect(){}
zeroShareValue(){
let share_target = this.shareTarget
share_target.value = 0
}

}
13 changes: 13 additions & 0 deletions app/models/county.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
# == Schema Information
#
# Table name: counties
#
# id :bigint not null, primary key
# name :string
# region :string
# created_at :datetime not null
# updated_at :datetime not null
#
class County < ApplicationRecord
has_many :served_areas, class_name: "Partners::ServedArea", dependent: :destroy
end
1 change: 1 addition & 0 deletions app/models/organization.rb
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ def upcoming
['Agency Stability', 'agency_stability'],
['Organizational Capacity', 'organizational_capacity'],
['Sources of Funding', 'sources_of_funding'],
['Area Served', 'area_served'],
['Population Served', 'population_served'],
['Executive Director', 'executive_director'],
['Pickup Person', 'diaper_pick_up_person'],
Expand Down
2 changes: 2 additions & 0 deletions app/models/partner.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ class Partner < ApplicationRecord

belongs_to :organization
belongs_to :partner_group, optional: true

has_many :item_categories, through: :partner_group
has_many :requestable_items, through: :item_categories, source: :items
has_one :profile, class_name: 'Partners::Profile', dependent: :destroy
Expand Down Expand Up @@ -101,6 +102,7 @@ class Partner < ApplicationRecord
agency_stability
organizational_capacity
sources_of_funding
area_served
population_served
executive_director
diaper_pick_up_person
Expand Down
28 changes: 28 additions & 0 deletions app/models/partners/profile.rb
Original file line number Diff line number Diff line change
Expand Up @@ -87,10 +87,17 @@ class Profile < Base

has_one_attached :proof_of_partner_status
has_one_attached :proof_of_form_990

has_many :served_areas, foreign_key: "partner_profile_id", class_name: "Partners::ServedArea", dependent: :destroy, inverse_of: :partner_profile

accepts_nested_attributes_for :served_areas, allow_destroy: true

has_many_attached :documents

validates :no_social_media_presence, acceptance: {message: "must be checked if you have not provided any of Website, Twitter, Facebook, or Instagram."}, if: :has_no_social_media?

validate :client_share_is_0_or_100

self.ignored_columns = %w[
evidence_based_description
program_client_improvement
Expand All @@ -108,5 +115,26 @@ class Profile < Base
def has_no_social_media?
website.blank? && twitter.blank? && facebook.blank? && instagram.blank?
end

def client_share_total
tot = 0
served_areas.each do |served_area|
tot += served_area.client_share
end
tot
cielf marked this conversation as resolved.
Show resolved Hide resolved
end

def client_share_is_0_or_100
# business logic: the client share has to be 0 or 100 -- although it is an estimate only, making it 0 (not
# specified at all) or 100 means we won't have people overallocating (> 100) and that they think about what
# their allocation actually is
value = client_share_total
check = (value == 0 || value == 100)
if !check

errors.add(:base, "Total client share must be 0 or 100")
awwaiid marked this conversation as resolved.
Show resolved Hide resolved
end
check
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be tidied up:

def client_share_is_0_or_100
  value = client_share_total
  if value != 0 && value != 100
    errors.add(:base, "Total client share must be 0 or 100")
  end
end

end
end
20 changes: 20 additions & 0 deletions app/models/partners/served_area.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# == Schema Information
#
# Table name: partner_served_areas
#
# id :bigint not null, primary key
# client_share :integer
# created_at :datetime not null
# updated_at :datetime not null
# county_id :bigint not null
# partner_profile_id :bigint not null
#
module Partners
class ServedArea < ApplicationRecord
awwaiid marked this conversation as resolved.
Show resolved Hide resolved
self.table_name = "partner_served_areas"
belongs_to :partner_profile, class_name: "Partners::Profile"
belongs_to :county
validates :client_share, numericality: {only_integer: true}
validates :client_share, inclusion: {in: 1..100}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great -- so this validates on the server side, only the fast-feedback JS needs to have the new validation.

end
end
33 changes: 33 additions & 0 deletions app/services/distribution_by_county_report_service.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
# frozen_string_literal: true

class DistributionByCountyReportService
def get_breakdown(distributions)
breakdown_struct = Struct.new(:name, :region, :num_items, :amount)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd have this as a constant above the method rather than defined in the method:

Breakdown = Struct.new(:name, :region, :num_items, :amount)
...
breakdowns['Unspecified'] = Breakdown.new("Unspecified", "ZZZ", 0, 0.00)

breakdowns = {}
breakdowns["Unspecified"] = breakdown_struct.new("Unspecified", "ZZZ", 0, 0.00)
distributions.each do |distribution|
served_areas = distribution.partner.profile.served_areas
num_items_for_distribution = distribution.line_items.total
value_of_distribution = distribution.line_items.total_value
if served_areas.size == 0
breakdowns["Unspecified"].num_items += num_items_for_distribution
breakdowns["Unspecified"].amount += value_of_distribution
else
served_areas.each do |served_area|
name = served_area.county.name
percentage = served_area.client_share / 100.0
if !breakdowns[name]
breakdowns[name] = breakdown_struct.new(name, served_area.county.region,
(num_items_for_distribution * percentage).round(0), value_of_distribution * percentage)
else
breakdowns[name].num_items = breakdowns[name].num_items + (num_items_for_distribution * percentage).round(0)
breakdowns[name].amount = breakdowns[name].amount + value_of_distribution * percentage
end
end
end
end

breakdown_array = breakdowns.sort_by { |k, v| [v[:region], k] }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use .region instead of [:region], that's why structs are neat 😄

@breakdown = breakdown_array.map { |a| a[1] }
end
end
48 changes: 48 additions & 0 deletions app/services/partner_profile_update_service.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
class PartnerProfileUpdateService
include ServiceObjectErrorsMixin
attr_reader :error
def initialize(old_partner, new_partner_params, new_profile_params)
@partner = old_partner
@profile = @partner.profile
@partner_params = new_partner_params
@profile_params = new_profile_params
end

def call
@return_value = false
perform_profile_service do
@partner.update(@partner_params)
@return_value = @partner.valid?

if @return_value
@profile.served_areas.each(&:destroy!)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can use destroy_all here.

@profile.reload
@profile.update!(@profile_params)
@profile.reload
end
end
end

def perform_profile_service(&block)
begin
@profile.transaction do
yield block
end
rescue ActiveRecord::RecordNotFound => e
Rails.logger.error "[!] #{self.class.name} failed to update profile #{@profile.id} because it does not exist"
set_error(e)
rescue => e
Rails.logger.error "[!] #{self.class.name} failed to update profile for #{@profile.id}: #{@profile.errors.full_messages} [#{e.inspect}]"
set_error(e)
end
self
end

def success?
@error.nil?
end

def set_error(error)
@error = error.to_s
end
end
2 changes: 2 additions & 0 deletions app/views/dashboard/index.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,8 @@
<div class="position-relative mb-4">
<div class="box-body text-center float-center">
<%= new_button_to new_distribution_path, {text: "New Distribution"} %>
<%= print_button_to distributions_by_county_report_path(filters: { date_range: date_range_params }), {text: "Distributions by County", size: "md"} %>

<h3 class="text-center">
<span class="total_distributed">
<%= total_distributed %>
Expand Down
Loading