-
Notifications
You must be signed in to change notification settings - Fork 18
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
remove featuretoggle useAMAActivationDate #19542
Conversation
Code Climate has analyzed commit e3e889e and detected 0 issues on this pull request. View more on Code Climate. |
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.
Alright I'm done with the first pass. I left some comments. I noticed that there are still several reducers and other spots in redux where the feature toggle is still being referenced. The only one that I am unsure about is the form context and the yup validation for the ramp refiling, but I think all the others can be removed like you already have done. Let me know what you think.
app/models/decision_review.rb
Outdated
@@ -95,7 +95,7 @@ def asyncable_user | |||
end | |||
|
|||
def ama_activation_date | |||
if intake && FeatureToggle.enabled?(:use_ama_activation_date, user: intake.user) | |||
if intake |
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.
Can this else block ever be reached now?
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.
nope, i do not think so
client/app/intake/util/issues.js
Outdated
@@ -350,7 +350,7 @@ export const formatIssuesBySection = (issues) => { | |||
); | |||
}; | |||
|
|||
export const formatAddedIssues = (issues = [], useAmaActivationDate = false) => { | |||
export const formatAddedIssues = (issues = [], useAmaActivationDate = true) => { |
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 only see one place where this is being used and it's based on the feature toggle so my guess is that we can just remove the useAmaActivationDate param.
If it's always true then that means the next line can be
const amaActivationDate = new Date(DATES.AMA_ACTIVATION);
fill_in "What is the Receipt Date of this form?", with: "01/01/2019" | ||
within_fieldset("Is the claimant someone other than the Veteran?") do | ||
find("label", text: "Yes", match: :prefer_exact).click | ||
end | ||
find("label", text: "Claimant not listed", match: :prefer_exact).click | ||
find("label", text: "Bob Vance", match: :prefer_exact).click |
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.
Why did this have to be updated for your changes? It's not apparent to me at first glance since this test used to enable the feature toggle so there should be no behavior change.
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.
this was failing before for some validation reason. after i fixed that validation i forgot to revert this change.
@@ -44,8 +44,8 @@ | |||
end | |||
|
|||
step "EPs use the updated Veteran name" do | |||
Fakes::BGSService.get_veteran_record(veteran.file_number) |
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.
Is this to fix a different test failure or was this related to the feature toggle somehow?
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.
yes, test failure. for some reason it was feature toggle removing was causing this step to fail.
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 still don't see why this would be happening unless it was related to the Timecop.freeze(post_ramp_start_date) moving from 2017 to the ama activiation date of 2019.
@@ -60,7 +60,7 @@ | |||
context "processed in Caseflow" do | |||
let(:benefit_type) { "education" } | |||
|
|||
it { is_expected.to be_truthy } |
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.
This and the supplemental claim spec fail now because of the date change. I think instead of switching these two values it would be better to adjust the timecop value to be after the 2019 ama activation date.
Something like this
Timecop.freeze(Time.utc(2020, 4, 24, 12, 0, 0))
and later down on like 206 something like that
let(:receipt_date) { Time.new("2021", "03", "01").utc }
If you wanted to capture both states. You could also create a new context block with timecop set to the original date and only check those two fields to verify that they are false/falsey. I think the intent of these two test files are to simulate valid claim reviews.
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. There's a couple of code climate issues left to fix, but that's about all I notice.
else | ||
Constants::DATES["AMA_ACTIVATION_TEST"].to_date | ||
end | ||
Constants::DATES["AMA_ACTIVATION"].to_date |
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.
This is causing a code climate issue now since it doesn't use any instance data. You could remove the method and just use Constants::DATES["AMA_ACTIVATION"].to_date in the two places I see this method being used. As far as I can tell it is only being referenced once in request issues and once in this file. There's also another definition that is exactly the same in the base_contestable_issues_controller file.
You could also pull it out into a module and include it in the decision review and base contestable issues controller files.
@@ -44,8 +44,8 @@ | |||
end | |||
|
|||
step "EPs use the updated Veteran name" do | |||
Fakes::BGSService.get_veteran_record(veteran.file_number) |
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 still don't see why this would be happening unless it was related to the Timecop.freeze(post_ramp_start_date) moving from 2017 to the ama activiation date of 2019.
Closing this out since the feature toggle is not always being used and ticket is blocked |
Resolves Appeals-4974
Description
Removes - use AmaActivationDate Feature toggle.
Acceptance Criteria
Testing Plan
Frontend
User Facing Changes
Storybook Story
For Frontend (Presentation) Components
MyComponent.stories.js
alongsideMyComponent.jsx
)Backend
Database Changes
Only for Schema Changes
created_at
,updated_at
) for new tablesCaseflow::Migration
, especially when adding indexes (useadd_safe_index
) (see Writing DB migrations)migrate:rollback
works as desired (change
supported functions)make check-fks
; add any missing foreign keys or add toconfig/initializers/immigrant.rb
(see Record associations and Foreign Keys)belongs_to
for associations to enable the schema diagrams to be automatically updatedIntegrations: Adding endpoints for external APIs
Best practices
Code Documentation Updates
Tests
Test Coverage
Did you include any test coverage for your code? Check below:
Code Climate
Your code does not add any new code climate offenses? If so why?
Monitoring, Logging, Auditing, Error, and Exception Handling Checklist
Monitoring
Logging
Auditing
Error Handling
Exception Handling