Gyr1 608/remove adopted and buyer#5208
Conversation
|
Heroku app: https://gyr-review-app-5208-523b8c6a0d86.herokuapp.com/ |
| def self.show?(intake) | ||
| intake.had_dependents_yes? | ||
| end | ||
| def self.show?(intake) = false |
There was a problem hiding this comment.
these two controllers seem to be intentionally not removed, but rather changing show to false. what is the logic behind that? why not delete them?
There was a problem hiding this comment.
I thought about this yesterday and I think I'm leaning towards just disabling because I am reluctant to remove too much on my first ticket on unfamiliar code that isn't well socialized among the team. If you feel really strongly that I should fully remove controllers, remove tests, remove forms, and remove views I might be persuaded, but I currently am preferring a light touch.
There was a problem hiding this comment.
Good question here. I normally love-love-love tossing out code, but the light touch might be better here, for a couple reasions: this feels like an unusual situation: a bunch of last-minute changes that could, conceivably, be reversed again in next year's form -- or, heaven forbid, even reversed in some sort of 11th-hour update in the next few weeks (which is unlikely but not impossible). and also, keeping the per-ticket surface area of change small will help us meet the tight deadline we have. that said, i am open to discussing further 😄
There was a problem hiding this comment.
thanks to you both for explaining!
anisharamnani
left a comment
anisharamnani
left a comment
There was a problem hiding this comment.
also putting a mark of approval! sorry for the delayed review
3 participants
Link to pivotal/JIRA issue
Is PM acceptance required? (delete one)
Reminder: merge main into this branch and get green tests before merging to main
What was done?
::show?method to be false on both controllers to be double sure they won't pop up.How to test?
Screenshots (for visual changes)