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

Remove scholarship validation from application model #33524

Merged
merged 5 commits into from Mar 10, 2020

Conversation

bencodeorg
Copy link
Contributor

@bencodeorg bencodeorg commented Mar 9, 2020

Education team noticed a bug where no updates (of any kind) could be made to CSP scholarships with the new EIR scholarship status.

Problem is a validation on the application that I missed, which validates the scholarship status. My approach here was to remove this validation -- I think it is redundant because:

Testing story

[manual] After reproducing the original issue locally, I've tested that it no longer occurs on an existing CSP or CSD application. Also checked that I can create an application (via factory), and update the scholarship status via the application dashboard.

[automated, per Clare's suggestion] added test that failed prior to this change, and passes with it.

Reviewer Checklist:

  • Tests provide adequate coverage
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@bencodeorg bencodeorg requested review from clareconstantine and a team March 9, 2020 23:11
@@ -65,12 +64,6 @@ class TeacherApplicationBase < ApplicationBase
pd_workshop_id
)

def scholarship_status_valid
unless scholarship_status.nil? || Pd::ScholarshipInfoConstants::SCHOLARSHIP_STATUSES.include?(scholarship_status)
Copy link
Contributor

Choose a reason for hiding this comment

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

did you check if these constants are used incorrectly anywhere else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good thought -- I just went through and double checked that all uses of Pd::ScholarshipInfoConstants::something are of individual constants (not lists) that still exist (eg, Pd::ScholarshipInfoConstants::NO).

I think I may have confused myself the first time around when I used things like Pd::ScholarshipInfo::YES_CDO, which is possible because Pd::ScholarshipInfoConstants is included at the top of the Pd::ScholarshipInfo model. I've moved references to the more confusing use of constants to the less confusing version in my most recent commit.

Copy link
Contributor

@molly-moen molly-moen left a comment

Choose a reason for hiding this comment

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

LGTM

@bencodeorg bencodeorg merged commit b2a95e0 into staging Mar 10, 2020
@bencodeorg bencodeorg deleted the allow-updates-to-eir-applications branch March 10, 2020 18:02
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.

None yet

2 participants