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.

Sign up for GitHub

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

Jump to bottom

[CPDNPQ-2225] qualifications API for TRS - part of DQT decommissioning #2115

Merged
merged 16 commits into from
Jan 27, 2025
Merged

[CPDNPQ-2225] qualifications API for TRS - part of DQT decommissioning #2115

merged 16 commits into from
Jan 27, 2025

Conversation

alkesh
Copy link
Contributor

@alkesh alkesh commented Jan 10, 2025 •
edited
Loading

Context

Ticket: https://dfedigital.atlassian.net/browse/CPDNPQ-2225

Changes proposed in this pull request

  • new qualifications API - returns a combination of passed participant outcomes from NPQ and the legacy participant outcomes loaded from the DQT data extract.
  • added scope to APIToken - this is either lead_provider or teacher_record_service - and a token from one scope cannot access the APIs from the other scope
  • new one off rake task to load legacy participant outcomes - which is from an extract from DQT
  • new rake task to create a teacher_record_service APIToken
  • document describing the API (docs/qualifications_api.md)

@github-actions GitHub Actions
Copy link
Contributor

Review app deployed to https://npq-registration-review-2115-web.test.teacherservices.cloud/

@alkesh alkesh force-pushed the CPDNPQ-2225-qualifications-api branch from 27a3f2e to 063224b Compare January 13, 2025 15:52
@alkesh alkesh force-pushed the CPDNPQ-2225-qualifications-api branch from 063224b to c1077ca Compare January 14, 2025 15:16
@alkesh alkesh marked this pull request as ready for review January 14, 2025 15:26
@alkesh alkesh requested a review from a team as a code owner January 14, 2025 15:26
@alkesh alkesh force-pushed the CPDNPQ-2225-qualifications-api branch from c1077ca to ac39162 Compare January 14, 2025 15:30
rwrrll
rwrrll reviewed Jan 15, 2025
View reviewed changes
app/models/participant_outcome.rb Outdated Show resolved Hide resolved
app/models/participant_outcome.rb Outdated Show resolved Hide resolved
db/migrate/20241104164154_add_scope_to_api_tokens.rb Outdated Show resolved Hide resolved
db/migrate/20241105145235_create_legacy_passed_participant_outcomes.rb Outdated Show resolved Hide resolved
app/models/api_token.rb Outdated Show resolved Hide resolved
lib/tasks/api_token.rake Outdated Show resolved Hide resolved
spec/lib/tasks/one_off/load_legacy_participant_outcomes_spec.rb Outdated Show resolved Hide resolved
spec/lib/tasks/one_off/load_legacy_participant_outcomes_spec.rb Outdated Show resolved Hide resolved
spec/models/api_token_spec.rb Outdated Show resolved Hide resolved
spec/requests/api/teacher_record_service/qualifications_spec.rb Show resolved Hide resolved
jebw
jebw reviewed Jan 15, 2025
View reviewed changes
Copy link
Contributor

@jebw jebw left a comment

Choose a reason for hiding this comment

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

LGTM - couple of comments around schema changes and API path

The actual API format makes me wince a little since this is what JSON:API has include and entity relationships for but it is following the pattern of our other APIs and does make it simpler to consume for someone just parsing as raw JSON (instead of using a JSON:API client)

config/routes.rb Outdated Show resolved Hide resolved
db/migrate/20241104164154_add_scope_to_api_tokens.rb Outdated Show resolved Hide resolved
db/schema.rb Show resolved Hide resolved
spec/factories/api_tokens.rb Outdated Show resolved Hide resolved
spec/rails_helper.rb Show resolved Hide resolved
@alkesh alkesh force-pushed the CPDNPQ-2225-qualifications-api branch from ac39162 to e3f16b4 Compare January 16, 2025 14:09
@alkesh alkesh force-pushed the CPDNPQ-2225-qualifications-api branch from e3f16b4 to 1db7239 Compare January 16, 2025 14:32
@alkesh alkesh requested review from jebw and rwrrll January 16, 2025 14:34
@alkesh alkesh force-pushed the CPDNPQ-2225-qualifications-api branch from 9c30a2f to 4df55a8 Compare January 21, 2025 15:41
@alkesh alkesh force-pushed the CPDNPQ-2225-qualifications-api branch from 4df55a8 to 829188f Compare January 21, 2025 16:01
@alkesh alkesh force-pushed the CPDNPQ-2225-qualifications-api branch from 829188f to a44338d Compare January 21, 2025 16:16
@alkesh alkesh closed this Jan 21, 2025
@alkesh alkesh reopened this Jan 21, 2025
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

jebw
jebw approved these changes Jan 27, 2025
View reviewed changes
Copy link
Contributor

@jebw jebw left a comment

Choose a reason for hiding this comment

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

LGTM

I've tested the migration both

  • loading schema from main, switching to branch and running db:migrate
  • loading schema from branch

AFAICT it constrains correctly in all scenarios now, using both approaches above 👍

@alkesh alkesh added this pull request to the merge queue Jan 27, 2025
Merged via the queue into main with commit 6d91853 Jan 27, 2025
18 checks passed
@alkesh alkesh deleted the CPDNPQ-2225-qualifications-api branch January 27, 2025 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jebw jebw jebw approved these changes

@rwrrll rwrrll Awaiting requested review from rwrrll

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@alkesh @jebw @rwrrll