3251 limit distribution date #3453

cielf · 2023-03-14T17:24:49Z

Resolves #3251

Description

This change will prevent people from entering issued at dates before the year 2000 for purchases, donations, distributions, and anywhere else that uses issued_at
Per the issue, we have several instances where people have entered dates that are before our "All Time" date window.

Still to be done:

migration to correct the outstanding distribution issued_at dates that are bad.
migration to correct the outstanding donation issued_at dates that are bad.
manual verification is blocked, because new distributions don't currently save the date properly. (There is a proto-issue for this -- I felt it was out of scope for this )

NOTE: This (the migration) will require communication to the affected banks.

Type of change

Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

automated tests on the model
modified other tests regarding the "All Time" range
outstanding: manual confirmation (blocked)

…on date save issue fixed to confirm manually / with system tests

cielf · 2023-03-14T19:44:59Z

One issue we have is that at least one of the distributions has items that are not available at the indicated storage location. This is going to take a bit more thought -- or we reach out to them and say "can we delete this record"? But that has potential run-on effects too. Or -- I can just do an update_attribute rather than an update! That'll work.

app/models/concerns/issued_at.rb

dorner · 2023-03-17T17:33:56Z

db/migrate/20230314192630_update_bad_distribution_issued_ats.rb

+    Distribution.unscoped.where("issued_at <  ?","2000-01-01").find_each do |dist|
+      # Using update_attribute, rather than update! because some of the distributions
+      # will not validate for other reasons (storage locations missing items)
+      dist.update_attribute(:issued_at, dist.created_at)


You can actually do this in a single SQL call which might be less error prone:

Distribution.unscoped.where("issued_at < ?","2000-01-01"). update_all('issued_at=created_at, updated_at=NOW()')

Is the intent of updated at when the user last touched it, or when it was changed in the db? Because if the former, we shouldn't touch it. I know we've had some reports that have been based on the updated_at -- I think in error, but I am feeling a bit cautious about touching it here.

I'm pretty sure from my inventory drift investigations that there are some occasions when we are not touching it in error also... (sigh) We should, perhaps, have someone dig into that.

Yes, updated_at should, by convention, always be when the record was updated for any normal reason even if not caused by a human interaction. So like a background job or something should update it. Probably a good best-practice to update it as part of this change.

Yeah --That's what I would have assumed -- but I noticed that we aren't in some cases when I was working on the inventory drift problem. So I'm concerned that we may be depending on it not changing. I'm now also wondering how this will should interact with the historical inventory on the storage location. Enough that I'm going to throw a WIP onto this until I can check that out.

I think I do need to noodle the historical inventory and distributions a bit more. In most cases, though, it's the created_at date that's going to be matching up to the inventory, not the updated_at. The exception will be any corrections -- which I presume would see the inventory nudged appropriately at the updated_at date.

All of which is to say we likely won't be making any issues worse by setting the updated to now

So this change should still happen then?

spec/models/distribution_spec.rb

spec/system/dashboard_system_spec.rb

dorner · 2023-03-17T17:35:20Z

spec/support/date_range_picker_shared_example.rb

@@ -17,7 +17,7 @@ def date_range_picker_select_range(range_name)
    described_class.destroy_all
  end

-  let!(:very_old) { create(described_class.to_s.underscore.to_sym, date_field.to_sym => Time.zone.local(1919, 7, 31)) }
+  let!(:very_old) { create(described_class.to_s.underscore.to_sym, date_field.to_sym => Time.zone.local(2000, 7, 31)) }  # We are no longer allowing dates before 2000


Not sure that comment is necessary? 2000 is still "very old" :)

cielf · 2023-04-03T00:58:21Z

I think it's ready for another look, then.

app/models/donation.rb

dorner · 2023-04-03T20:52:41Z

db/migrate/20230314192630_update_bad_distribution_issued_ats.rb

+    Distribution.unscoped.where("issued_at <  ?","2000-01-01").find_each do |dist|
+      # Using update_attribute, rather than update! because some of the distributions
+      # will not validate for other reasons (storage locations missing items)
+      dist.update_attribute(:issued_at, dist.created_at)


So this change should still happen then?

dorner · 2023-04-03T20:53:05Z

db/migrate/20230314202847_update_bad_donation_issued_ats.rb

@@ -0,0 +1,5 @@
+class UpdateBadDonationIssuedAts < ActiveRecord::Migration[7.0]
+  Donation.unscoped.where("issued_at <  ?","2000-01-01").find_each do |donation|
+    donation.update_attribute(:issued_at, donation.created_at)


This is just the same migration twice?

Nope - one's distribution, one's donation. We don't need one for purchases.

dorner · 2023-04-03T20:53:55Z

spec/system/dashboard_system_spec.rb

-        ["All Time",     test_time - 100.years,                   test_time]
+        ["This Year",    test_time.beginning_of_year,             test_time.end_of_year]
+        #  We now can't test the lower limit of All Time, because the earliest possible date is 2000-01-01
+        #  ["All Time",     test_time - 100.years,                   test_time]


Sorry - shouldn't we still have "All Time" be "since 2000"?

Pretty sure the same thing is going to apply here -- that the tests will try to create things that can't be created in order to check that All Time excludes them.

Yup. confirmed that.

We don't have to check that All Time excludes something - at the very least we can test that it includes something that the other values don't.

I've just been avoiding tinkering with the "do this for all the preset ranges" code, but we could do a single test for all time outside of that, I suppose.

It turned out to be easier to remove testing one end of "All Time" than to add in the 5 or 6 extra tests that would have been needed. This spec could use some attention overall though, imo.

cielf · 2023-04-03T22:05:42Z

Mea culpa. I failed to actually push the updated migrations. I have now.

dorner

one last suggestion!

dorner · 2023-04-14T20:17:26Z

@cielf this looks good to me! There are some conflicts but otherwise I think we should be fine. It's still listed as [WIP] - is that good to be removed?

cielf · 2023-04-23T18:16:22Z

Hey @cielf - Just letting you know this is in production now. Thanks!

ModelTests written. Other tests modified and passing. Need distributi…

b3dc314

…on date save issue fixed to confirm manually / with system tests

Migrations to re-date out of range issued_ats

267e472

cielf requested a review from dorner March 17, 2023 14:14

cielf changed the title ~~[WIP] 3251 limit distribution date~~ 3251 limit distribution date Mar 17, 2023

dorner requested changes Mar 17, 2023

View reviewed changes

moving validation to concern, plus rubucop happiness

456f59d

cielf changed the title ~~3251 limit distribution date~~ [WIP] 3251 limit distribution date Mar 26, 2023

better migrations

e197edc

cielf requested a review from dorner April 3, 2023 00:58

dorner requested changes Apr 3, 2023

View reviewed changes

cielf requested a review from dorner April 3, 2023 22:08

dorner requested changes Apr 4, 2023

View reviewed changes

tests for All Time modified to skip checks from before valid range

55fbf3f

cielf requested a review from dorner April 4, 2023 17:52

dorner approved these changes Apr 14, 2023

View reviewed changes

cielf changed the title ~~[WIP] 3251 limit distribution date~~ 3251 limit distribution date Apr 14, 2023

Merge branch 'main' into 3251-limit-distribution-date

0ef6f8a

cielf merged commit 09c1adf into rubyforgood:main Apr 17, 2023

cielf deleted the 3251-limit-distribution-date branch April 23, 2023 18:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

3251 limit distribution date #3453

3251 limit distribution date #3453

cielf commented Mar 14, 2023 •

edited

Loading

cielf commented Mar 14, 2023 •

edited

Loading

dorner Mar 17, 2023

cielf Mar 17, 2023

cielf Mar 17, 2023

awwaiid Mar 26, 2023

cielf Mar 26, 2023

cielf Apr 2, 2023 •

edited

Loading

cielf Apr 2, 2023

dorner Apr 3, 2023

dorner Mar 17, 2023

cielf Mar 18, 2023

cielf commented Apr 3, 2023

dorner Apr 3, 2023

dorner Apr 3, 2023

cielf Apr 3, 2023

dorner Apr 3, 2023

cielf Apr 3, 2023

cielf Apr 3, 2023

dorner Apr 4, 2023

cielf Apr 4, 2023

cielf Apr 4, 2023 •

edited

Loading

cielf commented Apr 3, 2023

dorner left a comment

dorner commented Apr 14, 2023

cielf commented Apr 23, 2023

3251 limit distribution date #3453

3251 limit distribution date #3453

Conversation

cielf commented Mar 14, 2023 • edited Loading

Description

Type of change

How Has This Been Tested?

cielf commented Mar 14, 2023 • edited Loading

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 Apr 2, 2023 • edited Loading

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 3, 2023

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 Apr 4, 2023 • edited Loading

Choose a reason for hiding this comment

cielf commented Apr 3, 2023

dorner left a comment

Choose a reason for hiding this comment

dorner commented Apr 14, 2023

cielf commented Apr 23, 2023

cielf commented Mar 14, 2023 •

edited

Loading

cielf commented Mar 14, 2023 •

edited

Loading

cielf Apr 2, 2023 •

edited

Loading

cielf Apr 4, 2023 •

edited

Loading