NPQ capping: add funded place to accept npq application endpoint #4726
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Review app deployed to https://cpd-ecf-review-4726-web.test.teacherservices.cloud |
pmanrubia
force-pushed
the
CPDLP-2938-npq-capping-add-funded-place-to-accept-npq-application-endpoint
branch
from
9f5645b
to
968e4d7
Compare
pmanrubia
force-pushed
the
CPDLP-2938-npq-capping-add-funded-place-to-accept-npq-application-endpoint
branch
from
cc672ca
to
56fd640
Compare
cwrw
reviewed
May 2, 2024
cwrw
reviewed
May 2, 2024
cwrw
reviewed
May 2, 2024
app/models/npq_application.rb
Outdated
| @@ -79,6 +79,14 @@ class NPQApplication < ApplicationRecord | |||
| delegate :id, :name, to: :npq_course, prefix: true | |||
| delegate :id, :name, to: :npq_lead_provider, prefix: true | |||
|
|
|||
| def current_contract | |||
There was a problem hiding this comment.
contracts are provider specific, and tied to statements, we need to think about where we could query this, and if this should be on application or on the npq provider/statement instead, happy to chat through this
cwrw
reviewed
May 2, 2024
| def funding_cap_available | ||
| return unless FeatureFlag.active?("npq_capping") | ||
| return if errors.any? | ||
| return if funded_place.nil? |
There was a problem hiding this comment.
I was wondering if it actually should fail here since we are expecting it to be populated and not nil?
cwrw
reviewed
May 2, 2024
|
|
||
| return if funded_place && contract.funding_cap.present? && contract.funding_cap.positive? | ||
|
|
||
| errors.add(:npq_application, I18n.t("npq_application.no_funding_cap_available")) |
There was a problem hiding this comment.
I think maybe the check here will probably be that this field is required since there is a cap on the contract 🤔 otherwise it's ok?
cwrw
reviewed
May 2, 2024
| context "when feature flag `npq_capping` is disabled" do | ||
| before { FeatureFlag.deactivate(:npq_capping) } | ||
|
|
||
| it "updates funded place attribute" do |
There was a problem hiding this comment.
Suggested change
| it "updates funded place attribute" do | |
| it "does not update funded place attribute" do |
mooktakim
force-pushed
the
CPDLP-2938-npq-capping-add-funded-place-to-accept-npq-application-endpoint
branch
from
e2120c1
to
0df2ac1
Compare
mooktakim
force-pushed
the
CPDLP-2938-npq-capping-add-funded-place-to-accept-npq-application-endpoint
branch
from
13f1f44
to
07c5ca1
Compare
mooktakim
force-pushed
the
CPDLP-2938-npq-capping-add-funded-place-to-accept-npq-application-endpoint
branch
from
a6ecaac
to
0bb5bbb
Compare
cwrw
reviewed
May 13, 2024
| @@ -24,6 +27,9 @@ def call | |||
| npq_application.update!(lead_provider_approval_status: "accepted") | |||
| other_applications_in_same_cohort.update(lead_provider_approval_status: "rejected") # rubocop:disable Rails/SaveBang | |||
| deduplicate_by_trn! | |||
| if FeatureFlag.active?("npq_capping") | |||
There was a problem hiding this comment.
can we add the check that if funded place is set as well, incase it's in previous cohorts?
|
|
||
| def accept_permitted_params | ||
| params.require(:data).permit(:type, attributes: %i[funded_place]) | ||
| rescue ActionController::ParameterMissing => e |
There was a problem hiding this comment.
I was testing with this and was thinking we avoid this because of past years where we won't need to pass in this request vs 2024, otherwise providers will get an error. Happy to chat through
mooktakim
force-pushed
the
CPDLP-2938-npq-capping-add-funded-place-to-accept-npq-application-endpoint
branch
from
0bb5bbb
to
70d0806
Compare
mooktakim
force-pushed
the
CPDLP-2938-npq-capping-add-funded-place-to-accept-npq-application-endpoint
branch
from
70d0806
to
cf1aa89
Compare
cwrw
approved these changes
May 13, 2024
mooktakim
force-pushed
the
CPDLP-2938-npq-capping-add-funded-place-to-accept-npq-application-endpoint
branch
from
3277cc6
to
6bc9cea
Compare
mooktakim
force-pushed
the
CPDLP-2938-npq-capping-add-funded-place-to-accept-npq-application-endpoint
branch
from
6bc9cea
to
9c30630
Compare
cwrw
approved these changes
May 16, 2024
mooktakim
deleted the
CPDLP-2938-npq-capping-add-funded-place-to-accept-npq-application-endpoint
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
Updates
acceptend point for NPQ capping in all versions of the API.The code is behind the feature flag:
npq_capping.Related PRs: #4690, #4697, #4701