[CPDLP-3815] NPQ Post Separation Cleanup - Remove NPQ API disable feature flag from ECF #5372

mooktakim · 2024-12-16T18:03:16Z

Context

Ticket: https://dfedigital.atlassian.net/browse/CPDLP-3815

Changes proposed in this pull request

removed disable_npq feature flag.
removed NpqApiEndpoint.

Guidance to review

github-actions · 2024-12-16T18:12:07Z

Review app deployed to https://cpd-ecf-review-5372-web.test.teacherservices.cloud

leandroalemao

Looks good @mooktakim 👍 😄 tks

ethax-ross

Looks good 👍 I think there's quite a bit of overlap with Leandro's PR, might be worth one branching off the other

ethax-ross · 2024-12-17T14:12:58Z

app/services/record_declaration.rb

@@ -286,13 +285,4 @@ def create_participant_outcome!
  def check_mentor_completion
    ParticipantDeclarations::HandleMentorCompletion.call(participant_declaration:)
  end
-
-  def validate_if_npq_course_supported


Do we not want to keep this? 🤔

agree to keep

Yes, will change this, i saw it on Leandro's PR👍

ethax-ross · 2024-12-17T14:20:24Z

config/routes.rb

@@ -198,7 +196,6 @@
    # Keeping the urls to old guidance urls, but they need to lead to new api-reference ones
    get "/guidance/home", to: redirect("/api-reference")
    get "/guidance/ecf-usage", to: redirect("/api-reference/ecf-usage")


A bunch of these other pages don't seem to exist either, we should maybe ditch the redirects at some point. There are also some translations we could bin here

https://github.com/DFE-Digital/early-careers-framework/blob/main/config/locales/en.yml#L600

I think let's keep the other redirects for now as not sure about those for now and they are not NPQ related.
In terms of other translations I think we need to do another sweep after those tickets to see whats left and break down into tickets to avoid too many changes

ethax-ross · 2024-12-17T14:21:56Z

spec/models/finance/schedule/ecf_spec.rb

@@ -32,7 +32,7 @@
    end
  end

-  Cohort.where(start_year: 2021..).find_each do |cohort|
+  Cohort.where(start_year: 2021..2024).find_each do |cohort|


Why did this need to change, of interest?

We discussed this on call. The schedules.csv file creates up to 2024. We will change this after we add 2025 to the csv. I'll add a comment.

cwrw · 2024-12-17T20:38:13Z

app/services/oneoffs/npq/migrate_declarations_between_statements.rb

-
-require "has_recordable_information"
-
-module Oneoffs::NPQ


didn't those go with the other pr 🤔

cwrw · 2024-12-17T20:45:42Z

spec/services/npq/change_to_pending_spec.rb

@@ -142,22 +142,6 @@
      end
    end

-    # Should fail
-    %w[eligible payable paid].each do |dec_state|


not sure why this had to go?

tried to keep minimum change just for this ticket

cwrw

looks good, a few minor comments. I agree with @ethax-ross around overlap, let's all chat tomorrow to agree way forward :)

…n PR 5371

cwrw · 2024-12-18T13:46:27Z

app/jobs/participant_outcomes/batch_send_latest_outcomes_job.rb

-# frozen_string_literal: true
-
-module ParticipantOutcomes
-  class BatchSendLatestOutcomesJob < ApplicationJob


can we remove this from the cron job?

cwrw · 2024-12-18T13:47:20Z

app/jobs/participant_outcomes/stream_api_requests_to_big_query_job.rb

-# frozen_string_literal: true
-
-module ParticipantOutcomes
-  class StreamApiRequestsToBigQueryJob < ApplicationJob


can we remove this from ParticipantOutcomeApiRequest?

mooktakim temporarily deployed to review December 16, 2024 18:08 — with GitHub Actions Inactive

cwrw assigned leandroalemao and ethax-ross Dec 16, 2024

leandroalemao approved these changes Dec 17, 2024

View reviewed changes

ethax-ross reviewed Dec 17, 2024

View reviewed changes

cwrw reviewed Dec 17, 2024

View reviewed changes

mooktakim force-pushed the 3815-npq-post-separation-cleanup-remove-npq-api-disable-feature-flag-from-ecf branch 2 times, most recently from ae79b0f to 590f9ba Compare December 18, 2024 11:01

mooktakim temporarily deployed to review December 18, 2024 11:06 — with GitHub Actions Inactive

mooktakim added 6 commits December 18, 2024 11:28

[CPDLP-3815] deleted NpqApiEndpoint and everything related to it

60f9919

[CPDLP-3815] removed remaining 'disable_npq' feature usage

5cf94aa

[CPDLP-3815] schedule seed csv only for 2021 to 2024

4f7753f

[CPDLP-3815] fixing some specs for disable_npq removal

031aa5b

[CPDLP-3815] undo NPQ course check for RecordDeclaration

f77aca1

[CPDLP-3815] comment for clarification

16dcfa4

mooktakim force-pushed the 3815-npq-post-separation-cleanup-remove-npq-api-disable-feature-flag-from-ecf branch from 79766b1 to 10b6337 Compare December 18, 2024 11:28

[CPDLP-3815] leaving RecordDeclaration unchanged so it can be fixed i…

766f7ee

…n PR 5371

mooktakim force-pushed the 3815-npq-post-separation-cleanup-remove-npq-api-disable-feature-flag-from-ecf branch from 10b6337 to 766f7ee Compare December 18, 2024 11:33

mooktakim temporarily deployed to review December 18, 2024 11:39 — with GitHub Actions Inactive

cwrw reviewed Dec 18, 2024

View reviewed changes

[CPDLP-3815] removed jobs from cron and model

94e1107

mooktakim had a problem deploying to review December 18, 2024 14:40 — with GitHub Actions Failure

mooktakim closed this Dec 18, 2024

mooktakim temporarily deployed to review December 18, 2024 15:22 — with GitHub Actions Inactive

mooktakim reopened this Dec 18, 2024

mooktakim temporarily deployed to review December 18, 2024 15:26 — with GitHub Actions Inactive

cwrw approved these changes Dec 18, 2024

View reviewed changes

mooktakim added this pull request to the merge queue Dec 18, 2024

Merged via the queue into main with commit ed2f5bd Dec 18, 2024
57 of 58 checks passed

mooktakim deleted the 3815-npq-post-separation-cleanup-remove-npq-api-disable-feature-flag-from-ecf branch December 18, 2024 16:06

mooktakim temporarily deployed to review December 18, 2024 16:06 — with GitHub Actions Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CPDLP-3815] NPQ Post Separation Cleanup - Remove NPQ API disable feature flag from ECF #5372

[CPDLP-3815] NPQ Post Separation Cleanup - Remove NPQ API disable feature flag from ECF #5372

mooktakim commented Dec 16, 2024

github-actions bot commented Dec 16, 2024

leandroalemao left a comment

ethax-ross left a comment

ethax-ross Dec 17, 2024

cwrw Dec 17, 2024

mooktakim Dec 18, 2024

ethax-ross Dec 17, 2024

cwrw Dec 17, 2024

ethax-ross Dec 17, 2024

mooktakim Dec 18, 2024

cwrw Dec 17, 2024

cwrw Dec 17, 2024 •

edited

Loading

mooktakim Dec 18, 2024

cwrw left a comment

cwrw Dec 18, 2024

cwrw Dec 18, 2024


		require "has_recordable_information"

		module Oneoffs::NPQ

[CPDLP-3815] NPQ Post Separation Cleanup - Remove NPQ API disable feature flag from ECF #5372

[CPDLP-3815] NPQ Post Separation Cleanup - Remove NPQ API disable feature flag from ECF #5372

Conversation

mooktakim commented Dec 16, 2024

Context

Changes proposed in this pull request

Guidance to review

github-actions bot commented Dec 16, 2024

leandroalemao left a comment

Choose a reason for hiding this comment

ethax-ross left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwrw Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwrw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwrw Dec 17, 2024 •

edited

Loading