Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show age on application #584

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Show age on application #584

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8444749
Add an age field to the applications index page
sylphiae May 3, 2021
16fc9fe
WIP-add css class and logic for warning. Logic is still broken.
sylphiae May 8, 2021
fa9d110
Fix highlighting for age column for 2 months overdue applications
sylphiae May 9, 2021
e41766f
Add helper for choosing highlight class
sylphiae May 10, 2021
c602f88
Change highlight warning number of weeks to the correct number
sylphiae May 10, 2021
69ac98a
Add spec, refactor method, and refactor css classes
sylphiae May 16, 2021
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
2 changes: 1 addition & 1 deletion app/assets/stylesheets/membership_application.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,4 +35,4 @@ body.applications.show {
display: block;
}
}
}
}
sylphiae marked this conversation as resolved.
Show resolved Hide resolved
14 changes: 14 additions & 0 deletions app/helpers/membership_application_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
module MembershipApplicationHelper

def choose_highlight_class(age)
if age >= 7.weeks && age < 2.months
"bg-info"
elsif age > 2.months
"bg-danger"
end
end

def age_in_days(age)
(age/ 1.day).floor
end
end
4 changes: 4 additions & 0 deletions app/models/application.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,10 @@ def sufficient_votes?
enough_yes || !few_nos
end

def age
Time.now - submitted_at
Copy link
Member

@anaulin anaulin May 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in_days doesn't seem to work in the rails console at all. I get an undefined method error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in_days does not work in the template either. Is there anything I need to do to activate it?

Copy link
Member

Choose a reason for hiding this comment

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

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.

end

def self.to_approve
all.map { |x| x if x.approvable? && x.state == "submitted" }.compact.sort_by { |x| x.submitted_at }
end
Expand Down
6 changes: 6 additions & 0 deletions app/views/members/applications/index.html.haml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
%th Yes
%th No
%th ?
%th Age
%tbody
- @applicants_submitted.each do |applicant|
- application = applicant.application
Expand All @@ -48,6 +49,11 @@
%td= application.yes_votes.size
%td= application.no_votes.size
%td= application.not_voted_count
%td{:class => choose_highlight_class(application.age)}
= "#{age_in_days(application.age)} days"




%h3
%a{"data-toggle" => "collapse" , "href" => "#collapseEmails"} Applicant Email Addresses <b class="caret"></b>
Expand Down
25 changes: 25 additions & 0 deletions spec/features/application_age_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
require "spec_helper"

describe "diplaying age on application" do

before do
@submitted_application = create(:user, state: :applicant).application
@submitted_application.update_attribute(:state, "submitted")
@submitted_application.update_attribute(:submitted_at, 1.week.ago)
end

before do
page.set_rack_session(user_id: member.id)
end

context "when logged in as a voting member" do
let(:member) { create(:voting_member) }

it "displays the correct application age" do
visit members_applications_path
expect(page).to have_content "Age"
#This spec controls the age of the application so the age can be hardcoded in
expect(page).to have_content "7 days"
end
end
end