Show age on application #584
Conversation
| @@ -125,6 +125,10 @@ def sufficient_votes? | |||
| enough_yes || !few_nos | |||
| end | |||
|
|
|||
| def age | |||
| Time.now - submitted_at | |||
There was a problem hiding this comment.
It is best to move logic out of the templates and into the code here (among other things, so that it can be tested), so I would change this to return the age in days, instead of just a difference between the two dates. Or you could move the computation into a helper. Either way, it would be good to remove it from the template.
Also, instead of using your own logic to compute the days, you could use in_days on a Rails Duration to get the number that you want to display in the UI. Documentation here: https://api.rubyonrails.org/classes/ActiveSupport/Duration.html#method-i-in_days
There was a problem hiding this comment.
Thanks for the feedback! I will remove the logic from the template and put it into a helper. I won't change age because it is being used in another helper's calculation as is, but make a new age_in_days variable. Thanks for the tip on Rails Duration - will look into that.
There was a problem hiding this comment.
in_days doesn't seem to work in the rails console at all. I get an undefined method error.
There was a problem hiding this comment.
in_days does not work in the template either. Is there anything I need to do to activate it?
There was a problem hiding this comment.
It's fine, you don't need to convert it to ActiveSupport::Duration if that doesn't work. The important part is to get the math/logic out of the view, and to add clarity, so e.g. if you leave this method as it is now, consider renaming it to age_secs (assuming secs is really what it is returning) or at least adding a comment indicating what is this returning.
There was a problem hiding this comment.
Thanks for adding this!
Before we merge this, it would be good to add one test to this PR to make sure that the age appears on the application page and looks right. Would you be up for that?
We already have a feature spec for voting on an application (https://github.com/doubleunion/arooo/blob/main/spec/features/voting_spec.rb), you could start a spec for displaying the application that follows the same steps as there (visit the application, etc), but instead verifies that the page has the age and it looks as expected.
Another idea is to add a new test case to https://github.com/doubleunion/arooo/blob/main/spec/controllers/members/applications_controller_spec.rb that asserts that the correct HTML with the age is returned in the response body.
Alternatively (or additionally), you could add a unit test for the helper, to make sure that it is making the selection of the background class correctly.
|
@sylphiae it looks like your code doesn't handle gracefully applications that have not yet been submitted. From the failed build at https://travis-ci.org/github/doubleunion/arooo/builds/770597863:
This will need fixing before this PR can be merged. |
Added a spec! |
What github issue is this PR for, if any?
Resolves #577
What does this code do, and why?
It adds an age field to the member applications overview page
It adds logic to highlight applications over 2 months old in orange, the overdue applications
It also adds logic to highlight applications that are a week away from the two-month deadline
How is this code tested?
Manually
Are any database migrations required by this change?
no
Are there any configuration or environment changes needed?
No
Screenshots please :)