Fix 5780

[guswhitten]

What github issue is this PR for, if any?

Resolves #5780

What changed, and why?

Add basic CRUD operations for case Placements.

How is this tested? (please write tests!) 💖💪

• index.html.erb_spec.rb
• edit.html.erb_spec.rb
• new.html.erb_spec.rb

Screenshots please :)

[elasticspoon]

Hey. This is a great start!

Overall, my comments are more about removal of code than anything.

I also would like to see a system spec to exercise the major work flows => creating, editing and deleting a placement.

[elasticspoon]

We should use the placement policy for this. You should be able to do something like Placement.authorize.

[thejonroberts]

I have to run before finished reviewing, have some minor suggestions.

[thejonroberts]

Suggested change

	transient { placement_types { ["Reunification", "Adoption", "Foster Care", "Kinship"] } }
	transient { placement_names { ["Reunification", "Adoption", "Foster Care", "Kinship"] } }

Confusing, these aren't placement types, they are just type names.

[thejonroberts]

I do think the placement_names change should be made at the least:

Suggested change

	after(:create) do |org, evaluator|
	evaluator.placement_types.each do |name|
	org.placement_types.create!(name: name)
	end
	end
	after(:create) do |org, evaluator|
	evaluator.placement_names.each do |name|
	org.placement_types.create!(name: name)
	end
	end

[thejonroberts]

Got a lot of comments, some are a little nit picky. But this is good overall! Will pull this down and try it out (probably tomorrow) before approval, but not requesting changes.

[thejonroberts]

This would break for nil user, or user with no casa org (all_casa_admin or some edge case).

Suggested change

	scope.joins(:casa_case).where(casa_cases: {casa_org_id: @user.casa_org_id})
	return scope.none unless @user&.casa_org
	scope.joins(:casa_case).where(casa_cases: {casa_org: @user.casa_org})

Also, can use casa_org object instead of casa_org_id (not urgent, more of a convention/nit I pick, prefer to use the object over the object_id. unless necessary).

[thejonroberts]

Suggested change

	<%= current_placement.placement_type.name %>
	<span><%= current_placement.placement_type.name %></span>

wrap with p or span some kind of html element. same with "Unknown" below.

I don't love all of these paragraphs being wrapped by a heading (h6) element. Should that h6 just be for "Current Placement"?

[guswhitten]

to keep it similar to other casa_case partials. the h6 includes Current Placement and the placement_type, but now excludes the link to "See All Placements" (though its CSS was being overridden anyway)

[thejonroberts]

Personally, I would prefer this set in the controller in @placement_types.

You couldn't use a policy scope because we don't have one for PlacementType, so this will do, but that is more likely to be fixed/updated with a scope if it's in the controller, rather than tucked away in this partial.

Not a must do, just a suggestion.

[thejonroberts]

Suggested change

	visit casa_case_placement_path(casa_case, placement)
	click_link("Edit")
	visit edit_casa_case_placement_path(casa_case, placement)

I think would work - can run bin/rails routes -g casa_case_placement to check it's there. no big deal, just one less page load for every test.

[elasticspoon]

I would argue that those are two different tests. Navigating to the index first catches any issues with the edit link not being present etc.

That being said that might already be covered in another test and be unnecessary.

[thejonroberts]

yes, but I would say that is a concern of the index system or view spec... it "displays link to edit" do or similar there... edit spec shouldn't worry about index (or the other ways we can get to this page) at all imo.

[thejonroberts]

Suggested change

	visit casa_case_placements_path(casa_case)
	click_link("New Placement")
	visit new_casa_case_placements_path(casa_case)

should work here.

[thejonroberts]

what's going on here? seems unrelated?

[guswhitten]

yeah this is unrelated. fixes a flaky test (from a previous PR of mine)

[guswhitten]

@thejonroberts i appreciate your effort, but it's functional and tested. this many refactorizations that change nothing about the actual product seems a bit ridiculous?

[thejonroberts]

@thejonroberts i appreciate your effort, but it's functional and tested. this many refactorizations that change nothing about the actual product seems a bit ridiculous?

@guswhitten I hid some of the comments that were less important imo (I can't resolve convos).

I do think that maintainability is important in addition to functionality - whether things are readable and modifiable in the future matters. That's the motivation behind most of the suggestions. But I take your point. I didn't block merge by requesting changes. The suggestions are my opinions, the decision to use them is yours. I will approve when I can pull down and try it out, regardless.

I also made formatted suggestions so that you could commit directly from GitHub if you agree with the change, requiring less effort for you. Try it out with the two placement_types/placement_names changes in the factory if you like. I think that is definitely confusing to a reader because placement_type is a model/factory, and therefore prone to breaking something if changed/used incorrectly.

[elasticspoon]

@guswhitten Hey sorry bout the delay reviewing this. Largely I am fine with the whole thing. It works nice in local testing.

I have 1 remaining concern which is mainly around not confusing users. Because the implementation order is a bit wack and orgs don't actually currently have a way to create placement types could you make this bit:

not show up. Either via flipper flag or just checking if an org has placements.

I'll leave it up to you on how much of the refactorings you want to accept but none of that stuff is blocking.

[elasticspoon]

Actually after thinking a bit my earlier comment still applies but I would rather have this in sooner rather than later even in a slightly imperfect form.

Feel free to add the conditional on the org having placement types and any other refactorings, otherwise I'll probably merge it in tomorrow and make an issue to not show the html until a organization has placements.

[thejonroberts]

Thank you for the scope and factory changes! Looks good!

[elasticspoon]

👍 thank you for the work @guswhitten and @thejonroberts for the in depth review.