[CPDLP-3228] NPQ MVP Prep - Disable NPQ endpoints in ECF specific separation environment #5027

mooktakim · 2024-07-10T11:08:07Z

Context

Ticket: https://dfedigital.atlassian.net.mcas.ms/browse/CPDLP-3228

Changes proposed in this pull request

disable_npq_endpoints feature added
NpqApiEndpointRoute created to constraint NPQ routes
For NPQ & ECF shared declarations routes:
- Declarations create action returns error if course_identifier includes npq course
- ParticipantDeclarations::Index updated to not return NPQ declarations if feature enabled
- Api::V3::ParticipantDeclarationsQuery updated to not return NPQ declarations if feature enabled

Guidance to review

github-actions · 2024-07-10T11:18:44Z

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

cwrw

could we also disable NPQ statements as well from the statements joint endpoint?

cwrw · 2024-07-10T16:17:02Z

app/controllers/api/v1/participant_declarations_controller.rb

-        service = RecordDeclaration.new({ cpd_lead_provider: }.merge(permitted_params["attributes"] || {}))
+        attributes = permitted_params["attributes"] || {}
+
+        if FeatureFlag.active?(:disable_npq_endpoints) && attributes[:course_identifier].to_s.starts_with?("npq-")


I understand why we might have this at the controller level, is there an argument of having this check at the record service level instead 🤔

As we can identify the NPQ declarations, I think it's best to return as soon as possible; that way, we don't interfere with the service classes.

The feature check is now done in RecordDeclaration service class

config/routes.rb

leandroalemao

Hi @mooktakim
Think /api/v{1}{2}{3}/participant-declarations/{id}/void is a shared endpoint as well as the statements one like @cwrw said?
As an idea, maybe we could return a deprecation warning do LPs rather than a 404 error?

cwrw · 2024-07-11T10:39:00Z

As an idea, maybe we could return a deprecation warning do LPs rather than a 404 error?

@leandroalemao Good question, LPs asked for a "sandbox" like environment without the NPQ endpoints, so we have to disable them, also this will probably be used when we switch off the service so we need to completely disable those

mooktakim · 2024-07-12T13:38:49Z

Hi @mooktakim Think /api/v{1}{2}{3}/participant-declarations/{id}/void is a shared endpoint as well as the statements one like @cwrw said? As an idea, maybe we could return a deprecation warning do LPs rather than a 404 error?

@leandroalemao added void endpoints now too

cwrw · 2024-07-15T08:27:12Z

lib/npq_api_endpoint_route.rb

+
+class NpqApiEndpointRoute
+  def self.matches?(_request)
+    !FeatureFlag.active?(:disable_npq_endpoints)


I wonder if we should change this to check the ENV is separation instead and switch it to the feature flag later to avoid hitting the db everytime those URLs are hit? let's discuss

leandroalemao

Looking good.. thanks @mooktakim 👍 😄

cwrw · 2024-07-17T17:33:20Z

app/services/api/v3/finance/statements_query.rb

@@ -23,6 +23,10 @@ def statements
              cohort_id: cohorts.map(&:id),
            )

+          if NpqApiEndpoint.disable_npq_endpoints?


I wonder if we can use statement_class and only return Finance::Statement::ECF there instead to avoid the extra queries here might be simpler, similar to the declaration query?

cwrw

thank you @mooktakim ! Looks great, after this is tested we will need to wait for the separation env to be ready then we can switch the config to that from review and merge 🙌

cwrw · 2024-07-23T17:55:28Z

config/environments/migration.rb

@@ -8,4 +8,9 @@

  # Used to handle HTTP_X_WITH_SERVER_DATE header for server side datetime overwrite
  config.middleware.use TimeTraveler
+
+  # Enable/disable aspects of the separation environment


sorry could this move to separation instead of migration env? :D

…aration environment

…fig.separation[:disable_npq_endpoints]

mooktakim force-pushed the 3228-npq-mvp-prep-disable-npq-endpoints-in-ecf-specific-separation-environment branch from df70c3f to bda6e60 Compare July 10, 2024 11:10

mooktakim temporarily deployed to review July 10, 2024 11:15 — with GitHub Actions Inactive

mooktakim requested a review from a team July 10, 2024 12:20

cwrw assigned leandroalemao Jul 10, 2024

cwrw reviewed Jul 10, 2024

View reviewed changes

config/routes.rb Outdated Show resolved Hide resolved

cwrw reviewed Jul 10, 2024

View reviewed changes

config/routes.rb Outdated Show resolved Hide resolved

leandroalemao requested changes Jul 11, 2024

View reviewed changes

mooktakim force-pushed the 3228-npq-mvp-prep-disable-npq-endpoints-in-ecf-specific-separation-environment branch from bda6e60 to 1f6b485 Compare July 11, 2024 13:53

mooktakim temporarily deployed to review July 11, 2024 13:57 — with GitHub Actions Inactive

mooktakim requested review from leandroalemao and cwrw July 11, 2024 14:09

mooktakim force-pushed the 3228-npq-mvp-prep-disable-npq-endpoints-in-ecf-specific-separation-environment branch from 1f6b485 to 4bc13c6 Compare July 12, 2024 13:09

mooktakim temporarily deployed to review July 12, 2024 13:14 — with GitHub Actions Inactive

mooktakim temporarily deployed to review July 12, 2024 13:25 — with GitHub Actions Inactive

cwrw reviewed Jul 15, 2024

View reviewed changes

mooktakim force-pushed the 3228-npq-mvp-prep-disable-npq-endpoints-in-ecf-specific-separation-environment branch from 1dc4034 to 7fb238f Compare July 15, 2024 10:07

mooktakim temporarily deployed to review July 15, 2024 10:11 — with GitHub Actions Inactive

mooktakim force-pushed the 3228-npq-mvp-prep-disable-npq-endpoints-in-ecf-specific-separation-environment branch from 765078d to b2435b7 Compare July 15, 2024 15:54

mooktakim temporarily deployed to review July 15, 2024 15:58 — with GitHub Actions Inactive

mooktakim temporarily deployed to review July 15, 2024 16:17 — with GitHub Actions Inactive

mooktakim force-pushed the 3228-npq-mvp-prep-disable-npq-endpoints-in-ecf-specific-separation-environment branch from a624d14 to 9eb7a51 Compare July 15, 2024 16:22

mooktakim temporarily deployed to review July 15, 2024 16:26 — with GitHub Actions Inactive

mooktakim requested a review from cwrw July 15, 2024 16:36

leandroalemao approved these changes Jul 16, 2024

View reviewed changes

mooktakim force-pushed the 3228-npq-mvp-prep-disable-npq-endpoints-in-ecf-specific-separation-environment branch from 9eb7a51 to a2f4991 Compare July 17, 2024 08:41

mooktakim force-pushed the 3228-npq-mvp-prep-disable-npq-endpoints-in-ecf-specific-separation-environment branch from a2f4991 to fda99b8 Compare July 17, 2024 15:54

mooktakim temporarily deployed to review July 17, 2024 15:58 — with GitHub Actions Inactive

cwrw reviewed Jul 17, 2024

View reviewed changes

mooktakim temporarily deployed to review July 17, 2024 17:56 — with GitHub Actions Inactive

mooktakim force-pushed the 3228-npq-mvp-prep-disable-npq-endpoints-in-ecf-specific-separation-environment branch from 44dcc9c to da1fdea Compare July 17, 2024 18:17

mooktakim temporarily deployed to review July 17, 2024 18:22 — with GitHub Actions Inactive

cwrw approved these changes Jul 18, 2024

View reviewed changes

mooktakim added the blocked-merge Do not merge this PR label Jul 18, 2024

mooktakim force-pushed the 3228-npq-mvp-prep-disable-npq-endpoints-in-ecf-specific-separation-environment branch from da1fdea to bb30096 Compare July 23, 2024 13:38

mooktakim temporarily deployed to review July 23, 2024 13:43 — with GitHub Actions Inactive

cwrw reviewed Jul 23, 2024

View reviewed changes

mooktakim added 10 commits July 24, 2024 10:04

[CPDLP-3228] NPQ MVP Prep - Disable NPQ endpoints in ECF specific sep…

016a72e

…aration environment

[CPDLP-3228] Declerations create check moved to RecordDeclaration

766f12a

[CPDLP-3228] Feature enable VoidParticipantDeclaration

3f8bbe8

[CPDLP-3228] Declerations index query fix for v3

b1f3268

[CPDLP-3228] Removed FeatureFlag, instead using Rails.application.con…

b44c0f5

…fig.separation[:disable_npq_endpoints]

[CPDLP-3228] RecordDeclaration will now return only one npq course error

5a2e274

[CPDLP-3228] Use 'config.npq_separation'

3a922fd

[CPDLP-3228] /api/v3/finance/statements feature flagged

1019c32

[CPDLP-3228] logic used to choose correct class

bee4a99

[CPDLP-3228] move over npq_separation settings to the separation env

f8e2ce8

mooktakim force-pushed the 3228-npq-mvp-prep-disable-npq-endpoints-in-ecf-specific-separation-environment branch from bb30096 to f8e2ce8 Compare July 24, 2024 09:04

mooktakim temporarily deployed to review July 24, 2024 09:08 — with GitHub Actions Inactive

cwrw approved these changes Jul 24, 2024

View reviewed changes

mooktakim added this pull request to the merge queue Jul 24, 2024

Merged via the queue into main with commit 0a14dd2 Jul 24, 2024
31 checks passed

mooktakim deleted the 3228-npq-mvp-prep-disable-npq-endpoints-in-ecf-specific-separation-environment branch July 24, 2024 14:04

mooktakim temporarily deployed to review July 24, 2024 14:04 — with GitHub Actions Inactive

mooktakim removed the blocked-merge Do not merge this PR label Jul 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CPDLP-3228] NPQ MVP Prep - Disable NPQ endpoints in ECF specific separation environment #5027

[CPDLP-3228] NPQ MVP Prep - Disable NPQ endpoints in ECF specific separation environment #5027

mooktakim commented Jul 10, 2024 •

edited

Loading

github-actions bot commented Jul 10, 2024

cwrw left a comment

cwrw Jul 10, 2024

mooktakim Jul 10, 2024

mooktakim Jul 11, 2024

leandroalemao left a comment

cwrw commented Jul 11, 2024 •

edited

Loading

mooktakim commented Jul 12, 2024

cwrw Jul 15, 2024

leandroalemao left a comment

cwrw Jul 17, 2024

cwrw left a comment

cwrw Jul 23, 2024

[CPDLP-3228] NPQ MVP Prep - Disable NPQ endpoints in ECF specific separation environment #5027

[CPDLP-3228] NPQ MVP Prep - Disable NPQ endpoints in ECF specific separation environment #5027

Conversation

mooktakim commented Jul 10, 2024 • edited Loading

Context

Changes proposed in this pull request

Guidance to review

github-actions bot commented Jul 10, 2024

cwrw left a comment

Choose a reason for hiding this comment

cwrw Jul 10, 2024

Choose a reason for hiding this comment

mooktakim Jul 10, 2024

Choose a reason for hiding this comment

mooktakim Jul 11, 2024

Choose a reason for hiding this comment

leandroalemao left a comment

Choose a reason for hiding this comment

cwrw commented Jul 11, 2024 • edited Loading

mooktakim commented Jul 12, 2024

cwrw Jul 15, 2024

Choose a reason for hiding this comment

leandroalemao left a comment

Choose a reason for hiding this comment

cwrw Jul 17, 2024

Choose a reason for hiding this comment

cwrw left a comment

Choose a reason for hiding this comment

cwrw Jul 23, 2024

Choose a reason for hiding this comment

mooktakim commented Jul 10, 2024 •

edited

Loading

cwrw commented Jul 11, 2024 •

edited

Loading