-
Notifications
You must be signed in to change notification settings - Fork 8
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-3818] Remove data in shared ecf declaration tables for npq #5401
base: main
Are you sure you want to change the base?
[CPDLP-3818] Remove data in shared ecf declaration tables for npq #5401
Conversation
Review app deployed to https://cpd-ecf-review-5401-web.test.teacherservices.cloud |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mooktakim All looks good.. a minor comment on cleaning en.yml file as well if you want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good just one minor comment
@@ -3,7 +3,7 @@ | |||
class DetectSidekiqMetricsIssuesJob | |||
include Sidekiq::Worker | |||
|
|||
SIDEKIQ_QUEUE_NAMES = %w[priority_mailers mailers default participant_outcomes big_query].freeze |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we can remove outcomes from fixtures? fixtures/files/api_reference/api_spec.json
@@ -3,7 +3,7 @@ | |||
class FutureDateValidator < ActiveModel::EachValidator | |||
def validate_each(record, attribute, value) | |||
if value && value > Time.zone.now | |||
record.instance_of?(::ParticipantOutcome::NPQ) ? record.errors.add(attribute, I18n.t(:future_date_outcomes, attribute:)) : record.errors.add(attribute, I18n.t(:future_date, attribute:)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can drop future_date_outcomes
from the translations file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are also some other translations we can probably bin:
https://github.com/DFE-Digital/early-careers-framework/blob/main/config/locales/en.yml#L186
https://github.com/DFE-Digital/early-careers-framework/blob/main/config/locales/en.yml#L706
Maybe we just do a sweep of these separately though 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think those are in the sweep later so its ok :)
856360e
to
673893e
Compare
7d5d1c9
to
a57b49f
Compare
a57b49f
to
b571092
Compare
Context
Changes proposed in this pull request
ParticipantOutcomeApiRequest
ParticipantOutcome::NPQ
DeclarationState
ParticipantDeclarationAttempt
ParticipantDeclaration
Guidance to review