-
Notifications
You must be signed in to change notification settings - Fork 16
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.
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
FE admin claims #3144
FE admin claims #3144
Conversation
@@ -396,7 +396,7 @@ def important_notes | |||
end | |||
|
|||
def award_amount_with_topups | |||
topups.sum(:award_amount) + award_amount | |||
topups.sum(:award_amount) + (award_amount || 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feeling ambivalent about this one
@@ -55,7 +55,7 @@ def self.create_with_claims!(claims, topups, attrs = {}) | |||
# associates the payment to the Topup, so we know it's payrolled | |||
group_topups = grouped_items.select { |i| i.is_a?(Topup) } | |||
|
|||
award_amount = grouped_items.sum(&:award_amount) | |||
award_amount = grouped_items.map(&:award_amount).compact.sum(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks ugly but fixes deprecation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just left one comment about the policy short name
@@ -827,6 +827,12 @@ en: | |||
feedback_email: "[email protected]" | |||
support_email_address: "[email protected]" | |||
claim_subject: "Further education payment" | |||
policy_acronym: FE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went for "TRIP" in my PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so TRIPFE
? otherwise TRIP
also includes EY
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah good point, I guess TRIPFE makes sense, maybe something to run by policy, but given it's just in the admin area it's not a blocker
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'll leave for now and sort it later
config/locales/en.yml
Outdated
@@ -827,6 +827,12 @@ en: | |||
feedback_email: "[email protected]" | |||
support_email_address: "[email protected]" | |||
claim_subject: "Further education payment" | |||
policy_acronym: FE | |||
policy_short_name: Further education payments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In figma I think this is now "Targeted Retention Incentive Payment For FE Teachers"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's called policy_short_name
😂
40248b3
to
c13eb34
Compare
c13eb34
to
e430dba
Compare
Context