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

[CPDLP-3586] NPQ Internal API - Implement DQT API calls in NPQ #1753

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mooktakim
Copy link
Contributor

@mooktakim mooktakim commented Sep 30, 2024

Context

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

Changes proposed in this pull request

  • Created Dqt::V1::Teacher client
  • Created Dqt::RecordCheck
  • Updated ParticipantValidator to use DQT when ecf_api_disabled is on.

Copy link

Copy link
Collaborator

@ethax-ross ethax-ross left a comment

Choose a reason for hiding this comment

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

Looks good just a couple of minor comments/questions

@@ -0,0 +1,25 @@
module Dqt
module V1
Copy link
Collaborator

Choose a reason for hiding this comment

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

The ticket mentions using v3 instead of v3, did something change/is this something different?

Copy link
Collaborator

Choose a reason for hiding this comment

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

we discussed this with the TRA team and unfortunately V3 is missing some features so we had to go back to V1 for now. Since this will all be deprecated soon they agreed for us to go with V1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, we had a meeting about this and found v3 doesn't include some things we need. We're sticking with v1 for migration, and likely replace all this in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should have refreshed first lol

let(:response_hash) do
{
"trn": trn,
"ni_number": "AB123456D",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it worth stubbing out the whole response here or just the two fields we actually use? arguably its good to see an 'expected response', but it makes the test harder to understand IMO 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, will remove unnecessary response values.

end

before do
allow(Dqt::V1::Teacher).to receive(:validate_trn)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could do the Teacher service stub in a single before block at the top and then just a let(:body) {} for each context

Copy link
Contributor

@leandroalemao leandroalemao left a comment

Choose a reason for hiding this comment

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

Looks good to me. Just need to rebase branch with main. Tksss 👍

Copy link
Collaborator

@ethax-ross ethax-ross left a comment

Choose a reason for hiding this comment

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

Looks good, have we added the secrets to the necessary keyvaults?

@mooktakim
Copy link
Contributor Author

Looks good, have we added the secrets to the necessary keyvaults?

Prepod and Production API keys for DQT added to npq-reg app environments. 👍

Copy link
Collaborator

@cwrw cwrw left a comment

Choose a reason for hiding this comment

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

Looking good but I think we might need to catchup on matches: ECF has a bit more logic around confirming if matches are correct https://github.com/DFE-Digital/early-careers-framework/blob/main/app/services/dqt_record_check.rb#L45 I think we might need to port that over?

Copy link
Contributor

@leandroalemao leandroalemao left a comment

Choose a reason for hiding this comment

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

@mooktakim Looks good to me.. 👍 😄 tks

Copy link
Collaborator

@cwrw cwrw left a comment

Choose a reason for hiding this comment

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

thanks @mooktakim!

Copy link

sonarcloud bot commented Oct 8, 2024

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.

4 participants