[Issue 2899] Distribution by counties! #3328

cielf · 2023-01-15T17:32:15Z

Resolves #2899

Description

Ability to have show the distribution by counties. at a high level.

Addition of optional areas_served partial to partner profile. This allows partners to select the counties they serve, and assign a percentage to each.
Distribution by county 'report'. Note this is not currently exportable.
Button leading to distribution by county report added to dashboard
Import of US counties

NOTE: The Import of US counties job ( load_us_counties) will have to be run when this is promoted !!!!!!!

Some decisions I'm not 100% sure are the best:
1/ County has full name (e.g. Washington County, Vermont) and region (Vermont) only -- maybe it should have the short name (without region) also. But I think we could run a job to extract it in the future if we have need. Chose region rather than state, because we include US territories.
2/ I chose show, rather than index or something else for the action for distribution_by_county. It is an endpoint rather than a jumping off point, though I can imagine a world where we would want to have links on each county. Will show be confusing - i.e. will people think there is a corresponding ActiveRecord?
3/ Not 100% happy with the name "served_area" for the model. Service areas? Served_areas at least matches up with areas_served, which is the title of the partial. I thought about using client_share as the name, but served_area allows the possibility of having other things about the area in the future better.

I do expect we will be asked to expand this in the future, to show (for example) diapers by county. But let's see what happens -- that should not require more input.

Dependency:

gem "activerecord-import"
And it does use js, including select2.

Type of change

New feature (non-breaking change which adds functionality)

How Has This Been Tested?

New automatic tests
manual testing as I went along

Note that you will have to either bin/setup or just run the load_us_counties rake job in order to manually test .

cielf · 2023-01-20T23:41:58Z

Hey @dorner. Would love your review, but even if it passes muster, let's not merge it until what's in the current main gets deployed to production (I don't want any QA issues with this to hold up the current release.)

dorner

Wow, what a meaty update! Super impressed at all the work! Let me know if you have any questions about any of the comments.

dorner · 2023-02-08T01:07:03Z

app/assets/javascripts/area_served.js

+
+
+
+function calculate_client_share_total(){


The "new" way of doing things is using Stimulus controllers. You can look in app/javascript for some examples.

Yeah. I've taken a look at stimulus, and it's not obvious to me how to convert this. Might be a thing to pair on, but I'm inclined to let the thing that is working work - maybe put in a technical debt item for someone to work on converting both this and the distribution edits, which will be similar.

It has been converted.

dorner · 2023-02-08T01:07:28Z

app/assets/javascripts/area_served.js

+
+function calculate_client_share_total(){
+    total = 0;
+    console.log("*********mark 1 total is: " +total);


This should be console.debug or removed entirely.

Yes. Oversight. Will be corrected in next push.

dorner · 2023-02-08T01:13:46Z

app/assets/javascripts/area_served.js

+function calculate_client_share_total(){
+    total = 0;
+    console.log("*********mark 1 total is: " +total);
+    const client_shares = document.querySelectorAll('*[id$="client_share"]')


Kind of annoying, but JavaScript variables and functions should be in camelCase instead of snake_case.

Hm. Fine. Will be corrected in next push.

dorner · 2023-02-08T01:14:16Z

app/assets/javascripts/area_served.js

+        document.getElementById("partners-served-areas-client-share-total-warning").style.visibility= 'hidden';
+        //      document.getElementById("profile-update-button").style.visibility = 'visible';
+        //    document.getElementById("profile-update-button-disabled").style.visibility = 'hidden';
+        //  document.getElementById("partner-county-client-share-total-warning-footer").style.visibility= 'hidden';


Please remove commented out code if it's not going to be used.

Also removed in next push

dorner · 2023-02-08T01:14:55Z

app/controllers/distributions_by_county_controller.rb

+  def show
+    setup_date_range_picker
+    distributions = current_organization.distributions.includes(:partner).during(helpers.selected_range)
+    breakdown_hash = {}


This method is too long to be in a controller. It should be in a service class. I'd also recommend not using a hash at all here, but creating a strongly typed Struct to store these values.

Moving it to a service in the next push. Considering the advice re the struct. Will probably do that too, because my tests for the service revealed a whole lotta awkwardness.

dorner · 2023-02-08T01:25:02Z

lib/tasks/load_us_counties.rake

+namespace :db do
+  desc "Load US Counties"
+  task :load_us_counties => :environment do
+    if County.find_by(name:"Abbeville County", region: " South Carolina").nil?


This seems kind of arbitrary - should we just do if County.none??

Yeah... A situation where you need US counties and there are already counties, but none of them are US is rather unlikely [I can imagine it, because this is open source, but it's a stretch]. In next push.

dorner · 2023-02-08T01:25:44Z

Gemfile

@@ -35,6 +35,8 @@ gem "paper_trail"
 gem "rolify", "~> 6.0"
 # Enforces "safe" migrations.
 gem "strong_migrations", "1.4.1"
+# imports (used to set up counties)
+gem "activerecord-import"


Rails supports mass inserts now: https://blog.saeloun.com/2022/07/26/rails-6-insert-all.html

Alrighty. That's the last thing I'm adding in the "low hanging fruit" push I'll be doing shortly.

dorner · 2023-02-08T01:26:45Z

spec/requests/distributions_by_county_spec.rb

+      describe "show" do
+        it "returns http success" do
+          get distributions_by_county_show_path(default_params)
+          expect(response).to have_http_status(:success)


Should we have some additional tests besides just "it doesn't error"?

I added a bunch in on the service in the next push. Will that do?

Maybe just some check for a particular string, e.g. the county and distribution name?

spec/requests/partners/profiles_requests_county_spec.rb

dorner · 2023-02-08T01:28:25Z

spec/system/distributions_by_county_system_spec.rb

+    end
+  end
+
+  context "handles time ranges properly (not fully written yet)" do


does this need to be fixed up?

Just the context title. Next push.

dorner · 2023-02-08T01:31:49Z

With re the questions above:

You can add a migration that calls the Rake task.
It probably would be best to name it anything other than show since there is no corresponding model. You could even just call it distributions_by_county again.
I think ServedArea is a bit clunky but clear in what it represents. I don't think it needs to be changed.

cielf · 2023-02-08T15:48:33Z

Thanks, @dorner! I hope to address most of these this week, FYI. (though there is a lot, and it requires learning...)

…bution_report # Conflicts: # db/schema.rb

…r than hash

cielf · 2023-02-17T21:31:21Z

I think I've caught everything except the stimulus controller -- which is not yielding easily.

cielf · 2023-02-17T21:31:40Z

The tests are all passing on local.

…to county

cielf · 2023-02-24T16:47:10Z

Alas, there is still a problem with this -- the deletion is not working properly. Putting it to a WIP state.

…"removed" item zero, plus a little rubocop happiness

cielf · 2023-02-24T19:24:20Z

Ok. The problem with the removal of served areas has been solved -- I'm a little concerned with my solution, though. With the data-actions on delete_served_area_button in ui_helper.rb, can we be sure that the first click will complete before the second one starts, or are we going to have to go down the road of zeroing the value, and then somehow calling the enveloping controller. It seems to be working, but my sample set is small.

cielf · 2023-02-24T19:59:07Z

I'm also getting a fair number of cases where these new tests are failing when I run the whole test suite. It seems to be related to Faker.Address.unique.community calls. They always pass when I "Rerun Failed Tests". Setting it aside for the nonce, as I have some meaty support stuff to work on, but will want to revisit. It is not always the same test.

cielf · 2023-02-27T03:09:08Z

Per @awwaiid's vocal request, I've merged main in, and found one of the tests that sometimes fail as an example: the test is at ./spec/models/partners/served_area_spec.rb:23 and the error is
Faker::UniqueGenerator::RetryLimitExceeded: Retry limit exceeded for community

dorner · 2023-02-28T14:43:33Z

This just reinforces my dislike of using randomly generated data in tests. If there is a failure, you can't reliably reproduce it, and it makes writing the tests themselves a lot harder.

… community. Bleah!

awwaiid · 2023-03-26T19:04:39Z

spec/helpers/distributions_by_county_helper_spec.rb

+#     end
+#   end
+# end
+RSpec.describe DistributionsByCountyHelper, type: :helper do


Delete this file too if you delete the other

awwaiid · 2023-03-26T21:19:38Z

spec/models/partners/profile_spec.rb

+    context "no served areas" do
+      let(:profile) { FactoryBot.build(:partner_profile) }
+      it "has 0 client share" do
+        expect(profile.client_share_total).to eq(0)


I suggest a expect(profile.valid?).to eq(true) here too, as below.

Co-authored-by: Brock Wilcox <awwaiid@thelackthereof.org>

… line with merged main

dorner · 2023-04-14T20:18:42Z

@cielf is this ready for re-review? Haven't looked at it in a while.

cielf · 2023-04-14T21:32:35Z

@dorner Yes. I believe it is.

awwaiid

Everything looks good to me; I think getting it into user hands to see what feedback they have sounds good.

dorner

Looking great! A few further suggestions, but they are on the minor side.

dorner · 2023-04-19T00:23:12Z

app/javascript/controllers/area_served_controller.js

+    }
+    calculateClientShareTotal(){
+        let total = 0;
+        let share_targets = this.shareTargets


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

dorner · 2023-04-19T00:25:02Z

app/javascript/controllers/area_served_controller.js

+            this.warningTarget.style.color= 'red';
+        }
+        return total;
+    }


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 =

Still missing some spaces before = 😄

app/models/partners/profile.rb

dorner · 2023-04-19T00:28:51Z

app/models/partners/profile.rb

+        errors.add(:base, "Total client share must be 0 or 100")
+      end
+      check
+    end


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

dorner · 2023-04-19T00:30:20Z

app/services/distribution_by_county_report_service.rb

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


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)

dorner · 2023-04-19T00:36:57Z

app/services/partner_profile_update_service.rb

+      @return_value = @partner.valid?
+
+      if @return_value
+        @profile.served_areas.each(&:destroy!)


Can use destroy_all here.

dorner · 2023-04-19T00:40:24Z

spec/requests/distributions_by_county_spec.rb

+      describe "show" do
+        it "returns http success" do
+          get distributions_by_county_show_path(default_params)
+          expect(response).to have_http_status(:success)


Maybe just some check for a particular string, e.g. the county and distribution name?

dorner · 2023-04-19T00:42:13Z

spec/services/distributions_by_county_report_service_spec.rb

+      partner_profile: @partner_1.profile, client_share: 20)
+    @partner_2.profile.served_areas[0].county = @partner_1.profile.served_areas[0].county
+    @partner_2.profile.served_areas[0].save
+    @partner_2.reload


Is there a reason you're using instance variables here? Generally we prefer having data set up with let blocks or in a before block (if you don't need to reference them later).

dorner · 2023-04-19T00:43:19Z

spec/system/distributions_by_county_system_spec.rb

+
+  it "shows 'Unspecified 100%' if no served_areas" do
+    @distribution = create(:distribution, :with_items, item: item_1, organization: @user.organization)
+    visit distributions_by_county_report_path


Can we move most of these from system to request specs? System specs are only really necessary if we're testing multiple pages (via clicks) or JavaScript. Simple visits should be doable with request specs, which don't fire up headless Chrome and hence are much faster.

dorner · 2023-04-19T00:43:40Z

spec/system/distributions_by_county_system_spec.rb

+    end
+  end
+
+  def setup_overlapping_partners


Should this be in a shared file somewhere since it's used in multiple specs?

compressing summation Co-authored-by: Daniel Orner <daniel.orner@wishabi.com>

cielf · 2023-04-20T21:19:32Z

Hmmm.... that's odd -- it only did 1 check when I put the latest thing up -- I've not seen that before. Noting it to @dorner, and also requesting a re-review

dorner

Almost there! A couple more cleanup bits and I think we're ready!

dorner · 2023-04-21T19:19:12Z

app/javascript/controllers/area_served_controller.js

+    }
+    calculateClientShareTotal ( ){
+        let total = 0;
+        this.share_targets.forEach(


Sorry - this won't work 😅 Stimulus requires a specific format for the target variables - it has to follow the JavaScript "CamelCase" format rather than the Ruby "snake_case" format. So this has to read this.shareTargets.

dorner · 2023-04-21T19:19:31Z

app/javascript/controllers/area_served_controller.js

+            this.warningTarget.style.color= 'red';
+        }
+        return total;
+    }


Still missing some spaces before = 😄

dorner · 2023-04-21T19:20:26Z

app/models/partners/profile.rb

+      # 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
+      if client_share_total != 0 && client_share_total != 100


Can we add a variable here to act as a cache? Otherwise it'll sum the client shares twice instead of once.

dorner · 2023-04-21T19:21:55Z

spec/requests/distributions_by_county_spec.rb

+        it "returns http success" do
+          get distributions_by_county_report_path(default_params)
+          expect(response).to have_http_status(:success)
+          expect(response.body).to include("Estimated Distributions by County for ")


Um - this doesn't have any actual data that's it checking for. Can we validate that we're at least viewing the page for the right distribution etc?

Removing the plain jane test, as, if it would fail, the other tests will also fail.

dorner · 2023-04-21T19:23:33Z

spec/requests/distributions_by_county_spec.rb

+      context "basic behaviour with served areas" do
+        it "handles multiple partners with overlapping service areas properly" do
+          @distribution_p1 = create(:distribution, :with_items, item: item_1, organization: @user.organization, partner: partner_1, issued_at: issued_at_present)
+          @distribution_p2 = create(:distribution, :with_items, item: item_1, organization: @user.organization, partner: partner_2, issued_at: issued_at_present)


Do these need to be instance variables? (Also line 30)

dorner

LGTM! Let's fix the Gemfile.lock and this is good to merge for me.

cielf · 2023-04-28T15:37:01Z

Looks like we've got a real conflict here! We've taken somewhat different approaches, etc.

cielf · 2023-04-28T19:16:07Z

Hey @dorner -- I think the test that is failing is actually in error -- that it should be checking for blankness rather than nilness. Shouldn't it?

dorner · 2023-05-05T19:53:43Z

@cielf all looks good - now we have a conflict with schema.rb 😆

cielf · 2023-05-05T20:46:07Z

@dorner Yeah. I can handle that. Just use the most recent one.

cielf · 2023-05-05T21:04:51Z

Holding my breath and pushing merge! (We don't have anything else that's big in the pipe for this release)

cielf added 2 commits January 15, 2023 11:45

Distribution by counties!

40acd69

clearing factory_bot:lint error (all tests still pass)

444ccfc

cielf requested a review from dorner January 20, 2023 23:40

cielf requested a review from awwaiid February 6, 2023 20:35

dorner requested changes Feb 8, 2023

View reviewed changes

cielf added 6 commits February 16, 2023 19:16

Low hanging fruit from review of distribution counties feature

3a99547

Distribution by counties! rebasing onto main

94a1860

clearing factory_bot:lint error (all tests still pass)

d195feb

Low hanging fruit from review of distribution counties feature

c22c5d9

Merge remote-tracking branch 'origin/Distribution_report' into Distri…

825acd5

…bution_report # Conflicts: # db/schema.rb

reworking distribution_by_county_report_service to use a struct rathe…

ac03d8c

…r than hash

cielf and others added 3 commits February 18, 2023 22:42

Renamed served_areas table to partner_served_areas and added indices …

a6322c7

…to county

converted to stimulus controller

9b4d82e

Merge branch 'main' into Distribution_report

676f088

cielf requested a review from dorner February 24, 2023 16:04

cielf changed the title ~~[Issue 2899] Distribution by counties!~~ [Issue 2899] [WIP] Distribution by counties! Feb 24, 2023

fixing issue with removal of served areas by making the value on the …

d0f635b

…"removed" item zero, plus a little rubocop happiness

cielf changed the title ~~[Issue 2899] [WIP] Distribution by counties!~~ [Issue 2899] Distribution by counties! Feb 24, 2023

Merge branch 'main' into Distribution_report

0457a8f

Changed the factory for counties to be a sequence instead of a unique…

d88f440

… community. Bleah!

awwaiid reviewed Mar 26, 2023

View reviewed changes

cielf and others added 3 commits March 27, 2023 10:14

Update lib/tasks/load_us_counties.rake

f83c4a5

Co-authored-by: Brock Wilcox <awwaiid@thelackthereof.org>

Update app/views/distributions_by_county/report.html.erb

cfeef34

Co-authored-by: Brock Wilcox <awwaiid@thelackthereof.org>

General cleanup and responses to reviews

925fc9c

cielf requested a review from awwaiid April 2, 2023 16:22

cielf and others added 2 commits April 3, 2023 07:42

Merge branch 'main' into Distribution_report

8fca5cf

changing field prefix in tests from "profile" to "partner_profile" in…

ae485a9

… line with merged main

awwaiid approved these changes Apr 16, 2023

View reviewed changes

dorner requested changes Apr 19, 2023

View reviewed changes

cielf and others added 2 commits April 18, 2023 20:52

Update app/models/partners/profile.rb

43e7e49

compressing summation Co-authored-by: Daniel Orner <daniel.orner@wishabi.com>

changes from @Dorners' review

03df861

cielf requested a review from dorner April 20, 2023 21:19

dorner requested changes Apr 21, 2023

View reviewed changes

changes from @dorner's review of 2023-04-21

640b583

cielf requested a review from dorner April 23, 2023 17:42

dorner approved these changes Apr 26, 2023

View reviewed changes

cielf and others added 2 commits April 28, 2023 13:10

renaming spec to match tested service

cdcad77

Merge branch 'main' into Distribution_report

7cb003f

tweeked profiles_requests_spec to match empty string instead of nil

871cd56

Merge branch 'main' into Distribution_report

6acbf89

cielf merged commit 320d29f into rubyforgood:main May 5, 2023




		function calculate_client_share_total(){

[Issue 2899] Distribution by counties! #3328

[Issue 2899] Distribution by counties! #3328

Conversation

cielf commented Jan 15, 2023 • edited Loading

Description

Type of change

How Has This Been Tested?

cielf commented Jan 20, 2023

dorner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dorner commented Feb 8, 2023

cielf commented Feb 8, 2023 • edited Loading

cielf commented Feb 17, 2023

cielf commented Feb 17, 2023

cielf commented Feb 24, 2023

cielf commented Feb 24, 2023

cielf commented Feb 24, 2023 • edited Loading

cielf commented Feb 27, 2023

dorner commented Feb 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dorner commented Apr 14, 2023

cielf commented Apr 14, 2023

awwaiid left a comment

Choose a reason for hiding this comment

dorner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cielf commented Apr 20, 2023

dorner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dorner left a comment

Choose a reason for hiding this comment

cielf commented Apr 28, 2023

cielf commented Apr 28, 2023

dorner commented May 5, 2023

cielf commented May 5, 2023

cielf commented May 5, 2023

cielf commented Jan 15, 2023 •

edited

Loading

cielf commented Feb 8, 2023 •

edited

Loading

cielf commented Feb 24, 2023 •

edited

Loading