Toggle to call Form526ConfirmationEmailJob in poll_form526_pdf #19478

mchae-nava · 2024-11-15T16:20:20Z

Note: Delete the description statements, complete each step. None are optional, but can be justified as to why they cannot be completed as written. Provide known gaps to testing that may raise the risk of merging to production.

Summary

*This work is behind a feature toggle (flipper): YES
(Summarize the changes that have been made to the platform)
Add a feature toggle to call Form526ConfirmationEmailJob in our polling job
(If bug, how to reproduce)
(What is the solution, why is this the solution?)
(Which team do you work for, does your team own the maintenance of this component?)
(If introducing a flipper, what is the success criteria being targeted?)
Control the availability of the new Received email so that Veterans do not experience a degraded or otherwise non-functional submission experience. A received email exists today, so it’s important that we maintain this

Related issue(s)

Link to ticket created in va.gov-team repo OR screenshot of Jira ticket if your team uses Jira
https://github.com/orgs/department-of-veterans-affairs/projects/1268/views/2?filterQuery=&sliceBy%5Bvalue%5D=Sprint+10&pane=issue&itemId=86084066&issue=department-of-veterans-affairs%7Cva.gov-team%7C96485
Link to previous change of the code/bug (if applicable)
Link to epic if not included in ticket

Testing done

New code is covered by unit tests
Describe what the old behavior was prior to the change
Describe the steps required to verify your changes are working as expected. Exclusively stating 'Specs run' is NOT acceptable as appropriate testing
If this work is behind a flipper:
- Tests need to be written for both the flipper on and flipper off scenarios. Docs.
- What is the testing plan for rolling out the feature?

Screenshots

Note: Optional

What areas of the site does it impact?

(Describe what parts of the site are impacted andifcode touched other areas)

Acceptance criteria

I fixed|updated|added unit tests and integration tests for each feature (if applicable).
No error nor warning in the console.
Events are being sent to the appropriate logging solution
Documentation has been updated (link to documentation)
No sensitive information (i.e. PII/credentials/internal URLs/etc.) is captured in logging, hardcoded, or specs
Feature/bug has a monitor built into Datadog (if applicable)
If app impacted requires authentication, did you login to a local build and verify all authenticated routes work as expected
I added a screenshot of the developed feature

Requested Feedback

(OPTIONAL)What should the reviewers know in addition to the above. Is there anything specific you wish the reviewer to assist with. Do you have any concerns with this PR, why?

tommasina-va · 2024-11-15T17:03:31Z

app/sidekiq/lighthouse/poll_form526_pdf.rb

+                                                              submission: #{submission_id} from poll_form526_pdf")
+            params = submission.personalization_parameters(first_name)
+            Form526ConfirmationEmailJob.perform_async(params)
+          end


Looks great! Just wondering what you think about pulling lines 100-106 out into a send_received_email method?

I'll do this as well with some fixes, once I'm able to

tommasina-va

LGTM!

aurora-a-k-a-lightning

the feature looks good. just a comment about the testing strategy

aurora-a-k-a-lightning · 2024-11-15T17:47:18Z

spec/models/form526_submission_spec.rb

+        subject { create(:form526_submission, :with_multiple_succesful_jobs) }
+
+        before do
+          Flipper.enable(:disability_526_call_received_email_from_polling)


feature flags in the test environment are automatically enabled.
did we mean to disable at the start, enable here, then disable in an after clause for the next tests to isolate these unit tests?
that's the usual pattern we follow.
and that may be why the tests are failing. i haven't looked at the details, but that seems like a solid guess 🤣

Ah, actually I think you're right! Will try it out and make the adjustment

aurora-a-k-a-lightning · 2024-11-15T17:47:29Z

spec/sidekiq/lighthouse/poll_form526_pdf_spec.rb

+
+    context 'with disability_526_call_received_email_from_polling enabled' do
+      before do
+        Flipper.enable(:disability_526_call_received_email_from_polling)


same for this

… into separate function

mchae-nava · 2024-11-18T22:45:22Z

@rmtolmach It seems the change is causing some flakiness with other tests we have that don't mock the Flipper call. In the interest of getting this feature toggle shipped, can we merge it with the previous changes, and I can make a follow-up ticket for my team to resolve the rest of the tests?

In the short term, this shouldn't cause any issues as my team is the only one that will be toggling and working with this flag

aurora-a-k-a-lightning

i think we can fix this easily. give it a whirl!

aurora-a-k-a-lightning · 2024-11-19T04:33:28Z

spec/models/form526_submission_spec.rb

@@ -1282,7 +1282,17 @@ def expect_no_max_cfi_logged(diagnostic_code)
      context 'with submission confirmation email when successful job statuses' do
        subject { create(:form526_submission, :with_multiple_succesful_jobs) }

-        it 'returns one job triggered' do
+        it 'does not trigger job when disability_526_call_received_email_from_polling enabled' do
+          allow(Flipper).to receive(:enabled?).with(:disability_526_call_received_email_from_polling,


hey Mark, the tests are now failing because someone added a Flipper called saved_claim_schema_validation_disable which is called in this call chain, i'm guessing when the form526 submission record is created on line 1283, which requires a saved_claim.

all we have to do is add this line above where we're calling the Flipper mock.

allow(Flipper).to receive(:enabled?).with(:saved_claim_schema_validation_disable).and_return(false)

i think that should work.
be sure to pull latest from master as that flag looks pretty new!

aurora-a-k-a-lightning · 2024-11-19T04:33:41Z

spec/models/form526_submission_spec.rb

+        end
+
+        it 'returns one job triggered when disability_526_call_received_email_from_polling disabled' do
+          allow(Flipper).to receive(:enabled?).with(:disability_526_call_received_email_from_polling,


here too!

allow(Flipper).to receive(:enabled?).with(:saved_claim_schema_validation_disable).and_return(false)

Thanks! I'll put the same thoughts I had in our channel here, but the implications of having more and more mocked flags needing to reference each other in order to prevent flakiness concerns me, especially when these threads are owned by different teams. We could certainly solve this for our team's unit tests, but @rmtolmach maybe this is worth revisiting later?

Anyways, I've made the changes for the short term, hopefully that's the only dependency for now

hm, that is odd. I'll bring that up with the backend CoP

…vent flakiness

rmtolmach

👍 specs passing

github-actions · 2024-11-19T15:03:42Z

Backend-review-group approval confirmed.

mchae-nava added 4 commits November 14, 2024 15:31

Add feature flag to yaml

f631e07

Implement feature flag in submission and polling jobs

fc05ff4

Linting

4c6738c

Logging, unit tests, some logic adjustments

165d579

mchae-nava requested review from a team as code owners November 15, 2024 16:20

github-actions bot added require-backend-approval lint-failure and removed require-backend-approval labels Nov 15, 2024

va-vfs-bot temporarily deployed to dbex/96485-received-email-feature-toggle/main/main November 15, 2024 16:28 Inactive

github-actions bot added the test-failure label Nov 15, 2024

Linting

aa4edbd

github-actions bot added require-backend-approval and removed require-backend-approval lint-failure labels Nov 15, 2024

va-vfs-bot temporarily deployed to dbex/96485-received-email-feature-toggle/main/main November 15, 2024 16:46 Inactive

tommasina-va reviewed Nov 15, 2024

View reviewed changes

tommasina-va previously approved these changes Nov 15, 2024

View reviewed changes

aurora-a-k-a-lightning requested changes Nov 15, 2024

View reviewed changes

Fix unit tests, add new test to verify proper first name

41879fe

mchae-nava dismissed tommasina-va’s stale review via 41879fe November 15, 2024 20:59

github-actions bot added require-backend-approval lint-failure and removed require-backend-approval labels Nov 15, 2024

va-vfs-bot temporarily deployed to dbex/96485-received-email-feature-toggle/main/main November 15, 2024 21:07 Inactive

Some cleanup, consolidated unit tests, moved confirmation email logic…

f42bd48

… into separate function

github-actions bot added require-backend-approval and removed require-backend-approval labels Nov 18, 2024

github-actions bot added require-backend-approval ready-for-backend-review and removed require-backend-approval ready-for-backend-review labels Nov 18, 2024

mchae-nava requested a review from rmtolmach November 18, 2024 22:06

github-actions bot added ready-for-backend-review require-backend-approval test-failure and removed ready-for-backend-review test-passing require-backend-approval labels Nov 18, 2024

Add explicit flag on mocking call to prevent flakiness

770e7d2

github-actions bot added require-backend-approval and removed ready-for-backend-review require-backend-approval labels Nov 18, 2024

va-vfs-bot temporarily deployed to dbex/96485-received-email-feature-toggle/main/main November 18, 2024 22:35 Inactive

aurora-a-k-a-lightning requested changes Nov 19, 2024

View reviewed changes

mchae-nava added 2 commits November 19, 2024 04:15

Merge branch 'master' into dbex/96485-received-email-feature-toggle

3aa01c4

Add explicit disable for saved_claim_schema_validation_disable to pre…

5a9b04f

…vent flakiness

github-actions bot added require-backend-approval and removed require-backend-approval labels Nov 19, 2024

va-vfs-bot temporarily deployed to dbex/96485-received-email-feature-toggle/main/main November 19, 2024 09:26 Inactive

github-actions bot added test-passing and removed test-failure labels Nov 19, 2024

rmtolmach approved these changes Nov 19, 2024

View reviewed changes

aurora-a-k-a-lightning approved these changes Nov 19, 2024

View reviewed changes

mchae-nava merged commit eb2a354 into master Nov 19, 2024
40 of 41 checks passed

mchae-nava deleted the dbex/96485-received-email-feature-toggle branch November 19, 2024 16:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Toggle to call Form526ConfirmationEmailJob in poll_form526_pdf #19478

Toggle to call Form526ConfirmationEmailJob in poll_form526_pdf #19478

mchae-nava commented Nov 15, 2024 •

edited

Loading

tommasina-va Nov 15, 2024

mchae-nava Nov 15, 2024

tommasina-va left a comment

aurora-a-k-a-lightning left a comment

aurora-a-k-a-lightning Nov 15, 2024

mchae-nava Nov 15, 2024

aurora-a-k-a-lightning Nov 15, 2024

mchae-nava commented Nov 18, 2024

aurora-a-k-a-lightning left a comment

aurora-a-k-a-lightning Nov 19, 2024 •

edited

Loading

aurora-a-k-a-lightning Nov 19, 2024

mchae-nava Nov 19, 2024

rmtolmach Nov 19, 2024

rmtolmach left a comment

github-actions bot commented Nov 19, 2024

Toggle to call Form526ConfirmationEmailJob in poll_form526_pdf #19478

Toggle to call Form526ConfirmationEmailJob in poll_form526_pdf #19478

Conversation

mchae-nava commented Nov 15, 2024 • edited Loading

Summary

Related issue(s)

Testing done

Screenshots

What areas of the site does it impact?

Acceptance criteria

Requested Feedback

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tommasina-va left a comment

Choose a reason for hiding this comment

aurora-a-k-a-lightning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mchae-nava commented Nov 18, 2024

aurora-a-k-a-lightning left a comment

Choose a reason for hiding this comment

aurora-a-k-a-lightning Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmtolmach left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 19, 2024

mchae-nava commented Nov 15, 2024 •

edited

Loading

aurora-a-k-a-lightning Nov 19, 2024 •

edited

Loading