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.

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

Fix for Perkins loan caps, unsubsidized graduate loan error message #269

Merged
merged 7 commits into from
Aug 8, 2016

Conversation

mistergone
Copy link
Member

@mistergone mistergone commented Aug 8, 2016

This PR implements two fixes:

  • Updates to student-debt-calc v2.5.11, which fixes errors with Perkins Loan caps.
  • Fixes error content for graduate unsubsidized loans

Changes

  • Update to student-debt-calc v2.5.11
  • Moves calculation of caps and updating of content regarding caps to financialView.updateViewWithProgram() which is much better timing

Testing

  • Passes unit tests!

Review

Checklist

  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows the standards laid out in the front end playbook
  • Passes all existing automated tests
  • New functions include new tests
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged
  • Visually tested in supported browsers and devices
  • Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)

Animated GIF

When I notice how poorly I coded something in the recent past:

spock-dazzling-logic

…when the

user goes over a federal limit in one of the loan fields. The logic was
happening too early before - now it will occur when the view updates using the
program data, as it should.
@saintsoup52
Copy link

Does this pull request deal with perkins loans that exceed cost of attendance as well?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 1c36ac8 on perks into 86e39b2 on master.

@mistergone
Copy link
Member Author

@saintsoup52 - Yes

@@ -63,7 +63,6 @@ var app = {
} );
}
// set financial caps based on data
financialView.setCaps( getFinancial.values() );
Copy link
Member

Choose a reason for hiding this comment

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

This line needs to remain in order for the About This Tool page to display loan and grant caps.

Copy link
Member Author

Choose a reason for hiding this comment

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

I replaced this, and I'll push a commit up.

@marteki
Copy link
Member

marteki commented Aug 8, 2016

On focusout, the Grad PLUS loan over cost of attendance error is flashing off screen before it can be read.

@@ -351,9 +351,9 @@
"resolved": "https://registry.npmjs.org/bluebird/-/bluebird-3.4.1.tgz"
},
"bn.js": {
"version": "4.11.6",
"version": "4.11.5",
Copy link
Member

Choose a reason for hiding this comment

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

In your reducing of merge conflicts, you might have downgraded a lot of dependencies in our shrinkwrapping? I just upgraded a bunch of our NPM packages.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I might have. There were conflicts, so I regenerated the file. I don't know why that wouldn't keep the dependency tree...

@mistergone
Copy link
Member Author

On focusout, the Grad PLUS loan over cost of attendance error is flashing off screen before it can be read.

@marteki - Was this previously addressed? It's possible a merge messed it up.

@marteki
Copy link
Member

marteki commented Aug 8, 2016

Focusout: Yeah. I just had a PR merged today to address it. 😞 #267

@mistergone
Copy link
Member Author

mistergone commented Aug 8, 2016

@marteki - I can't account for that. Your changes from #267 are still in my PR, and I don't immediately see what could have broken them.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 111065d on perks into 8508958 on master.

…when the

user focusouts from the relevant input.
@mistergone
Copy link
Member Author

@marteki - The latest commit should fix your error messages not-appearing bug. 😄

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling bbeb339 on perks into 8508958 on master.

@marteki
Copy link
Member

marteki commented Aug 8, 2016

It does! 🙌

👍

@higs4281
Copy link
Member

higs4281 commented Aug 8, 2016

👍 perkins caps show correctly for grad and undergrad perkins schools.

Does this pull request deal with perkins loans that exceed cost of attendance as well?

Working for me:

perkins_error_msgs

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling d67376a on perks into 8508958 on master.

@mistergone
Copy link
Member Author

A commit was also pushed to patch a bug in "About this page"

@mistergone mistergone merged commit 3f97e2b into master Aug 8, 2016
@mistergone mistergone deleted the perks branch August 18, 2017 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants