Skip to content
This repository has been archived by the owner on Oct 2, 2019. It is now read-only.

[Delivers #102267428] pick highest pending approver on the stack #650

Merged
merged 4 commits into from
Oct 5, 2015
Merged

[Delivers #102267428] pick highest pending approver on the stack #650

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
335f97b
tweek CSV reporting to pick highest pending approver on the stack
Sep 25, 2015
bf1170c
fix approver email method (again) to check first for any pending appr…
Sep 26, 2015
1aa3fee
Updated #current_approver_email_address to output the correct email a…
phirefly Oct 4, 2015
98fbfb6
Merge pull request #659 from 18F/102267428-approver-email-rv
pkarman Oct 5, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 7 additions & 2 deletions app/models/ncr/work_order.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,8 +114,13 @@ def approving_official
self.approvers.first
end

def approving_official_email_address
approving_official ? approving_official.email_address : self.system_approver_emails.first
# the highest approver on the stack, pending preferred if status indicates
Copy link
Contributor

Choose a reason for hiding this comment

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

what do you think about not having a comment here? I think this method's tests make it clear why it works this way, and having the comment could lead to more complications down the line.

For more details on this line of thinking, this is a killer blog post on why code comments can be tricky https://robots.thoughtbot.com/letting-your-code-speak-for-itself by the one and only @adarsh

Copy link

Choose a reason for hiding this comment

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

:thumbs_up:

On Monday, October 5, 2015, Jessie A. Young notifications@github.com
wrote:

In app/models/ncr/work_order.rb
#650 (comment):

@@ -114,8 +114,13 @@ def approving_official
self.approvers.first
end

  • def approving_official_email_address
  •  approving_official ? approving_official.email_address : self.system_approver_emails.first

  • the highest approver on the stack, pending preferred if status indicates

what do you think about not having a comment here? I think this method's
tests make it clear why it works this way, and having the comment could
lead to more complications down the line.

For more details on this line of thinking, this is a killer blog post on
why code comments can be tricky
https://robots.thoughtbot.com/letting-your-code-speak-for-itself by the
one and only @adarsh https://github.com/adarsh

—
Reply to this email directly or view it on GitHub
https://github.com/18F/C2/pull/650/files#r41155349.

@adarshp http://www.twitter.com/adarshp
313.454.1515 (mobile)
See when I am free: http://bit.ly/adarshcal

def current_approver_email_address
if self.pending?
self.individual_approvals.where(status: 'actionable').first.user.email_address
else
self.approving_official.email_address
end
end

def email_approvers
Expand Down
2 changes: 1 addition & 1 deletion lib/ncr/reporter.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ def self.make_csv_row(proposal)
[
self.proposal_public_url(proposal),
proposal.requester.email_address,
proposal.client_data.approving_official_email_address,
proposal.client_data.current_approver_email_address,
proposal.client_data.cl_number,
proposal.client_data.function_code,
proposal.client_data.soc_code,
Expand Down
15 changes: 15 additions & 0 deletions spec/models/ncr/work_order_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -453,4 +453,19 @@
expect(wo.building_id).to be_nil
end
end

describe "#current_approver_email_address" do
it "returns the first pending approval's email address" do
Copy link
Contributor

Choose a reason for hiding this comment

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

when you say first -- what are users being ordered by? just id?

wo = FactoryGirl.create(:ncr_work_order, :with_approvers)
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be outside the scope of this PR, but what do you think about include FactoryGirl syntax methods so this can just be wo = create(:ncr_work_order, :with_approvers) ? Would be a lot less typing!!

https://github.com/thoughtbot/factory_girl/blob/master/GETTING_STARTED.md#configure-your-test-suite

expect(wo.current_approver_email_address).to eq(wo.individual_approvals.first.user.email_address)
wo.individual_approvals.first.approve!
expect(wo.current_approver_email_address).to eq(wo.individual_approvals.last.user.email_address)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

add a newline between tests?

it "returns the first approver when fully approved" do
wo = FactoryGirl.create(:ncr_work_order, :with_approvers)
wo.individual_approvals.first.approve!
wo.reload.individual_approvals.last.approve!
expect(wo.current_approver_email_address).to eq(wo.individual_approvals.first.user.email_address)
end
end
end