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

Conversation

pkarman
Copy link
Contributor

@pkarman pkarman commented Sep 25, 2015

@phirefly
Copy link
Contributor

Looks like this is blowing up https://gist.github.com/phirefly/4f725ae6de6a4b43933f#file-error-2015-09-25-L7

likely because these records don't have pending approvals:
https://github.com/18F/C2/blob/master/app/mailers/report_mailer.rb#L20-L22

…ovals before accessing one, falling back to approving_official if !pending exists
@pkarman
Copy link
Contributor Author

pkarman commented Sep 26, 2015

I think bf1170c should fix it, whenever we return to QAing this.

@yozlet
Copy link
Contributor

yozlet commented Oct 1, 2015

:shipit:

1 similar comment
@annalee
Copy link
Contributor

annalee commented Oct 1, 2015

:shipit:

@pkarman
Copy link
Contributor Author

pkarman commented Oct 5, 2015

@phirefly if/when this passes for you in stage, feel free to merge.

@@ -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

pkarman added a commit that referenced this pull request Oct 5, 2015
[Delivers #102267428] pick highest pending approver on the stack
@pkarman pkarman merged commit 9efc770 into master Oct 5, 2015
@pkarman pkarman deleted the 102267428-approver-email branch October 5, 2015 15:42
@pkarman pkarman mentioned this pull request Oct 6, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants