Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
ee27105
create needed_items table
isidzukuri Feb 19, 2024
29a4187
add needed_item_ids to children controller
isidzukuri Feb 19, 2024
791a882
add needed_items to Child model
isidzukuri Feb 19, 2024
781afa2
update views with needed_items
isidzukuri Feb 19, 2024
2284dcd
comment: TODO: drop items_needed_ids after issue #3797 released to prod
isidzukuri Feb 19, 2024
30ca7ac
update Child factory
isidzukuri Feb 19, 2024
f803c7d
enhance view of needed items in the lists
isidzukuri Feb 19, 2024
2ccbb64
remove item_needed_diaperid from seed.rb
isidzukuri Feb 19, 2024
36b5fce
replace item_needed_diaperid with needed_items in controllers
isidzukuri Feb 19, 2024
7b7a3a1
specs
isidzukuri Feb 19, 2024
166eb5e
rubocop
isidzukuri Feb 19, 2024
c18c5a4
rubocop
isidzukuri Feb 19, 2024
399683d
Merge branch 'main' into 3797_improving_the_child_functionality
isidzukuri Feb 19, 2024
3a43674
remove mention about not existing column items_needed_ids
isidzukuri Feb 20, 2024
8cee0f6
use {} instead of do..end with map in erb
isidzukuri Feb 20, 2024
a94971f
use shorter syntax for group_by
isidzukuri Feb 20, 2024
6227f72
use rails conventional approach to implement needed child items
isidzukuri Feb 21, 2024
0e957ef
migrations
isidzukuri Feb 21, 2024
912c6e9
revert unrelated automatic schema changes
isidzukuri Feb 21, 2024
2114d33
fix specs
isidzukuri Feb 21, 2024
7e9749d
remove db/migrate/20240219121949_create_needed_item.rb
isidzukuri Feb 21, 2024
e5ef123
Merge branch 'main' into 3797_improving_the_child_functionality
isidzukuri Feb 21, 2024
5958c6b
fix spec
isidzukuri Feb 21, 2024
2ef7ffd
fix spec
isidzukuri Feb 21, 2024
05c5142
fix spec
isidzukuri Feb 21, 2024
9513c9b
fix spec
isidzukuri Feb 21, 2024
a9f8bc2
fix spec
isidzukuri Feb 21, 2024
8a823fe
remove comment
isidzukuri Feb 22, 2024
cc81a69
remove useless wip migration
isidzukuri Feb 22, 2024
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
4 changes: 2 additions & 2 deletions app/controllers/partners/children_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -88,11 +88,11 @@ def child_params
:first_name,
:gender,
:health_insurance,
:item_needed_diaperid,
:last_name,
:race,
:archived,
child_lives_with: []
child_lives_with: [],
needed_item_ids: []
)
end

Expand Down
7 changes: 5 additions & 2 deletions app/controllers/partners/family_requests_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,12 @@ def create
end
end

children = current_partner.children.active.where(id: children_ids).where.not(item_needed_diaperid: [nil, 0])
children = current_partner.children.active
.where(id: children_ids)
.joins(:needed_items)
.select('children.*', :item_id)

children_grouped_by_item_id = children.group_by(&:item_needed_diaperid)
children_grouped_by_item_id = children.group_by(&:item_id)
family_requests_attributes = children_grouped_by_item_id.map do |item_id, item_requested_children|
{ item_id: item_id, person_count: item_requested_children.size, children: item_requested_children }
end
Expand Down
6 changes: 4 additions & 2 deletions app/models/partners/child.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ class Child < Base
serialize :child_lives_with, Array
belongs_to :family
has_many :child_item_requests, dependent: :destroy
has_and_belongs_to_many :needed_items, class_name: 'Item'
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This should still just be has_many, no?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

in that case items must have child_id. Or has_many .... through

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yep, can we change to has_many :through? That's more accurate for the actual relationship here.

Copy link
Copy Markdown
Contributor Author

@isidzukuri isidzukuri Feb 22, 2024

Choose a reason for hiding this comment

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

No, it does not.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ah, took another look and you're right - this is a HABTM. I'm not used to using this relationship because it's rare that it's legitimately cross-join. 

Still have a question though - the table is now called children_items but the relationship is called needed_items... I don't actually see the table being referenced. Is there some Rails magic I'm missing?


include Filterable
include Exportable
Expand Down Expand Up @@ -88,7 +89,7 @@ def display_name
def self.csv_export_headers
%w[
id first_name last_name date_of_birth gender child_lives_with race agency_child_id
health_insurance comments created_at updated_at family_id item_needed_diaperid active archived
health_insurance comments created_at updated_at family_id needed_item_ids active archived
].freeze
end

Expand All @@ -107,11 +108,12 @@ def csv_export_attributes
created_at,
updated_at,
family_id,
item_needed_diaperid,
needed_item_ids,
active,
archived
]
end
end
end

# TODO: drop item_needed_diaperid after issue #3797 released to prod
2 changes: 1 addition & 1 deletion app/views/partners/children/_child.json.jbuilder
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
json.extract! child, :id, :first_name, :last_name, :date_of_birth, :gender, :child_lives_with, :race, :agency_child_id, :health_insurance, :item_needed_diaperid, :comments, :created_at, :updated_at
json.extract! child, :id, :first_name, :last_name, :date_of_birth, :gender, :child_lives_with, :race, :agency_child_id, :health_insurance, :needed_item_ids, :comments, :created_at, :updated_at
json.url child_url(child, format: :json)
8 changes: 4 additions & 4 deletions app/views/partners/children/_form.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@
<%= form.text_field :last_name, class: "form-control" %>

<%= form.label :item_needed, "Diaper/Item Used" %>
<%= form.select :item_needed_diaperid, @formatted_requestable_items,
{include_blank: 'Select an item'},
{class: 'form-control'} %>
<br>
<span>(select multiple by holding `ctrl`)</span>
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@cielf thoughts about this? Typically we have an interface similar to the line item where we give them an "add new" button. Is there going to be confusion / difficulty using the interface if it requires a select-click?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Just saw this - will try to take a look tomorrow.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

My take:

I like the aesthetics of the select-many approach better, but I think having to use 'ctrl' does have the potential to be confusing . One issue is that the key that is acting as 'ctrl' is actually 'command' on Macs -- using 'control', which is what you'd think would work, doesn't. (I tried it out)

The add new pattern is relatively clunky, aesthetically, but since having more than 2 items per child is relatively rare, I think it's workable.

<%= form.select :needed_item_ids, @formatted_requestable_items,
{include_blank: false, include_hidden: false, selected: child.needed_item_ids},
{class: 'form-control', multiple: true} %>

<%= form.input :date_of_birth, as: :date, start_year: 20.years.ago.year, end_year: Time.current.year, label: "Date of Birth" %>
<%= form.input :gender, collection: ["Male", "Female"], class: "form-control" %>
Expand Down
6 changes: 5 additions & 1 deletion app/views/partners/children/show.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,11 @@
<dd><%= @child.health_insurance %></dd>

<dt>Item needed:</dt>
<dd><%= current_partner.organization.item_id_to_display_string_map[@child.item_needed_diaperid] %></dd>
<dd>
<%= @child.needed_item_ids.map{|item_id|
current_partner.organization.item_id_to_display_string_map[item_id]
}.join(', ') %>
</dd>

<dt>Comments:</dt>
<dd><%= @child.comments %></dd>
Expand Down
8 changes: 5 additions & 3 deletions app/views/partners/family_requests/_list.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,17 @@
<%= child.first_name %> <%= child.last_name %>
</td>
<td>
<% if child.item_needed_diaperid %>
<%= current_partner.organization.item_id_to_display_string_map[child.item_needed_diaperid] %>
<% if child.needed_item_ids.present? %>
<%= child.needed_item_ids.map{|item_id|
current_partner.organization.item_id_to_display_string_map[item_id]
}.join(', ') %>
<% else %>
<%= "N/A" %>
<% end %>
</td>
<td>
<div class="custom-control custom-switch form-switch">
<% if child.item_needed_diaperid %>
<% if child.needed_item_ids.present? %>
<%= check_box_tag "child-#{child.id}", child.active, child.active,
class: "custom-control-input",
id: "child-#{child.id}" %>
Expand Down
12 changes: 12 additions & 0 deletions db/migrate/20240221072117_create_join_table_child_item.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
class CreateJoinTableChildItem < ActiveRecord::Migration[7.0]
def change
create_table :children_items do |t|
t.references :child, null: false, foreign_key: true
t.references :item, null: false, foreign_key: true

t.timestamps
end

add_index :children_items, [:child_id, :item_id], unique: true
end
end
7 changes: 7 additions & 0 deletions db/migrate/20240221072118_populate_needed_items.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
class PopulateNeededItems < ActiveRecord::Migration[7.0]
def change
Partners::Child.find_each do |child|
child.needed_item_ids = [child.item_needed_diaperid] if child.item_needed_diaperid.present?
end
end
end
14 changes: 13 additions & 1 deletion db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema[7.0].define(version: 2024_02_07_202431) do
ActiveRecord::Schema[7.0].define(version: 2024_02_21_072118) do
# These are extensions that must be enabled in order to support this database
enable_extension "plpgsql"

Expand Down Expand Up @@ -188,6 +188,16 @@
t.index ["family_id"], name: "index_children_on_family_id"
end

create_table "children_items", force: :cascade do |t|
t.bigint "child_id", null: false
t.bigint "item_id", null: false
t.datetime "created_at", null: false
t.datetime "updated_at", null: false
t.index ["child_id", "item_id"], name: "index_children_items_on_child_id_and_item_id", unique: true
t.index ["child_id"], name: "index_children_items_on_child_id"
t.index ["item_id"], name: "index_children_items_on_item_id"
end

create_table "counties", force: :cascade do |t|
t.string "name"
t.string "region"
Expand Down Expand Up @@ -861,6 +871,8 @@
add_foreign_key "child_item_requests", "children"
add_foreign_key "child_item_requests", "item_requests"
add_foreign_key "children", "families"
add_foreign_key "children_items", "children"
add_foreign_key "children_items", "items"
add_foreign_key "distributions", "partners"
add_foreign_key "distributions", "storage_locations"
add_foreign_key "donations", "manufacturers"
Expand Down
4 changes: 2 additions & 2 deletions db/seeds.rb
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,7 @@ def random_record_for_org(org, klass)
comments: Faker::Lorem.paragraph,
active: Faker::Boolean.boolean,
archived: false,
item_needed_diaperid: p.organization.item_id_to_display_string_map.key(Partners::Child::CHILD_ITEMS.sample)
needed_item_ids: [p.organization.item_id_to_display_string_map.key(Partners::Child::CHILD_ITEMS.sample)]
)
end

Expand All @@ -322,7 +322,7 @@ def random_record_for_org(org, klass)
comments: Faker::Lorem.paragraph,
active: Faker::Boolean.boolean,
archived: false,
item_needed_diaperid: p.organization.item_id_to_display_string_map.key(Partners::Child::CHILD_ITEMS.sample)
needed_item_ids: [p.organization.item_id_to_display_string_map.key(Partners::Child::CHILD_ITEMS.sample)]
)
end
end
Expand Down
2 changes: 1 addition & 1 deletion spec/factories/partners/child.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,6 @@
first_name { Faker::Name.first_name }
last_name { Faker::Name.last_name }
gender { Faker::Gender.binary_type }
item_needed_diaperid { Item.all.sample.id }
needed_item_ids { [Item.all.sample.id] }
end
end
1 change: 1 addition & 0 deletions spec/models/partners/child_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
describe 'associations' do
it { should belong_to(:family) }
it { should have_many(:child_item_requests).dependent(:destroy) }
it { should have_and_belong_to_many(:needed_items).class_name('Item') }
end

describe "#display_name" do
Expand Down
6 changes: 3 additions & 3 deletions spec/requests/partners/children_requests_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
agency_child_id: "Agency McAgence",
health_insurance: "Private insurance",
comments: "Some comment",
item_needed_diaperid: nil,
needed_item_ids: nil,
active: true,
archived: false,
family: family)
Expand All @@ -31,7 +31,7 @@
agency_child_id: "Agency McAgence",
health_insurance: "Private insurance",
comments: "Some comment",
item_needed_diaperid: nil,
needed_item_ids: nil,
active: true,
archived: false,
family: family)
Expand All @@ -51,7 +51,7 @@
headers = {"Accept" => "text/csv", "Content-Type" => "text/csv"}
get partners_children_path, headers: headers
csv = <<~CSV
id,first_name,last_name,date_of_birth,gender,child_lives_with,race,agency_child_id,health_insurance,comments,created_at,updated_at,family_id,item_needed_diaperid,active,archived
id,first_name,last_name,date_of_birth,gender,child_lives_with,race,agency_child_id,health_insurance,comments,created_at,updated_at,family_id,needed_item_ids,active,archived
#{child1.id},John,Smith,2019-01-01,Male,"mother,grandfather",Other,Agency McAgence,Private insurance,Some comment,#{child1.created_at},#{child1.updated_at},#{family.id},"",true,false
#{child2.id},Jane,Smith,2018-01-01,Female,father,Hispanic,Agency McAgence,Private insurance,Some comment,#{child2.created_at},#{child2.updated_at},#{family.id},"",true,false
CSV
Expand Down
4 changes: 2 additions & 2 deletions spec/requests/partners/family_requests_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,9 @@
describe 'POST #create' do
before do
# Set one child as deactivated and the other as active but
# without a item_needed_diaperid
# without a needed items
children[0].update(active: false)
children[1].update(item_needed_diaperid: nil)
children[1].needed_item_ids = []
end
subject { post partners_family_requests_path, params: params }

Expand Down
20 changes: 14 additions & 6 deletions spec/system/partners/family_requests_system_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,13 @@
end

describe "for children with different items, from different families" do
let(:item_id) { Item.all.sample.id }
let(:item_ids) { Item.pluck(:id).sample(2).sort }
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can we just create new items? I'm not a fan of using auto-generated data for testing. (In other words, auto-generated data is great when you *don't care* about the values - but if you're testing that the values are correct, it's much better to set them yourself.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

right. Also spec file should be self sufficient and describe all dependencies at least with shared_context or shared_examples. imho

Copy link
Copy Markdown
Contributor Author

@isidzukuri isidzukuri Feb 22, 2024

Choose a reason for hiding this comment

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

i like the "boy scout rule". But here its relatively complex change because of current Rspec - FactoryBot - integration tests setup. It will lead to multiline change across multiple files. Looks like its behind of the scope of current PR. Maybe is better to leave item_ids as it is and try to improve tests suite in standalone PR. I guess tests can be speeded up drastically if not auto-generate data if they are not really needed...

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Sorry if I was unclear - I don't think we have to stop creating the other data when it's created automatically. But here at least we can create 2 new items that we can test against.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I understood you perfectly. I don't want to do it to be honest.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

OK... can you explain why?

let!(:children) do
[
create(:partners_child, family: family),
create(:partners_child, family: family, item_needed_diaperid: item_id),
create(:partners_child, family: family, item_needed_diaperid: item_id),
create(:partners_child, family: other_family, item_needed_diaperid: item_id),
create(:partners_child, family: family, needed_item_ids: item_ids),
create(:partners_child, family: family, needed_item_ids: item_ids),
create(:partners_child, family: other_family, needed_item_ids: item_ids),
create(:partners_child, family: other_family)
]
end
Expand All @@ -28,8 +28,16 @@
expect(page).to have_text("Request Details")
click_link "Your Previous Requests"
expect(page).to have_text("Request History")
expect(Partners::ChildItemRequest.pluck(:child_id)).to match_array(children.pluck(:id))
expect(Partners::ItemRequest.pluck(:item_id)).to match_array(children.pluck(:item_needed_diaperid).uniq)

expect(children[0].child_item_requests.size).to eq(1)
expect(children[1].child_item_requests.size).to eq(2)
expect(children[2].child_item_requests.size).to eq(2)
expect(children[3].child_item_requests.size).to eq(2)
expect(children[4].child_item_requests.size).to eq(1)
expect(children[1].child_item_requests.map(&:item_request).map(&:item_id).sort).to eq(item_ids)
expect(children[2].child_item_requests.map(&:item_request).map(&:item_id).sort).to eq(item_ids)
expect(children[3].child_item_requests.map(&:item_request).map(&:item_id).sort).to eq(item_ids)
expect(Partners::ItemRequest.pluck(:item_id)).to match_array(children.map(&:needed_item_ids).flatten.uniq)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

None of the specs actually show that we now support multiple items... let's make sure we have some examples of that in all the specs that are relevant.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

addressed 2114d33

end
end

Expand Down