Closed guswhitten closed 3 weeks ago
@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 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.
:+1: thank you for the work @guswhitten and @thejonroberts for the in depth review.
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 :)