BCDA-8351: Exclude MD TCoC requests from suppression #993

laurenkrugen-navapbc · 2024-09-18T22:35:42Z

🎫 Ticket

https://jira.cms.gov/browse/BCDA-8351

🛠 Changes

When preparing beneficiaries for a request, if the source of the request was from TCoC, then the query to get suppressed beneficiaries will be skipped.
github.com/go-testfixtures is being used in combination with github.com/golang-migrate/migrate/v4 so that tests that utilize the application database are idempotent. There is existing code that will create a test database that can have migrations and fixtures applied to it. The long term goal (besides idemtpotence) is to eventually remove all dependency on init() calls, since those prohibit our ability to run singular tests in packages that import another package that uses init().
Service_test.go has had test name updates, so that we can eventually separate our unit and integration tests in our make unit-test command.
protobuf version was bumped from v1.5.3 -> v1.5.4 due to failing build
test fixtures was bumped from v3.5.0 -> v3.7.0 to utilize additional library features that weren't available the previous library
the service folder contains a directory for data fixtures that was renamed from testdata to fixtures. Old files that were not in use were removed and new files have been added

ℹ️ Context

To onboard TCOC, we must exclude beneficiary suppression with their requests.

🧪 Validation

Added integration test will seeded database for suppression data.

laurenkrugen-navapbc · 2024-09-18T22:38:55Z

bcda/service/service.go

-		ignoredMBIs, err = s.repository.GetSuppressedMBIs(ctx, s.sp.lookbackDays, upperBound)
-		if err != nil {
-			return nil, fmt.Errorf("failed to retreive suppressedMBIs %s", err.Error())
+		if cfg, ok := s.GetACOConfigForID(conditions.CMSID); ok {


We already get the config in our middleware when a request is made, so this feels a little redundant. Having something similar to line 493 or adding it as a request condition may have been better, so I am open to discussing this one!

laurenkrugen-navapbc · 2024-09-18T22:40:25Z

bcda/service/service_test.go

@@ -170,7 +173,7 @@ func (s *ServiceTestSuite) TearDownTest() {
 	conf.SetEnv(s.T(), "PRIORITY_ACO_REG_EX", s.priorityACOsEnvVar)
 }

-func (s *ServiceTestSuite) TestIncludeSuppressedBeneficiaries() {
+func (s *ServiceTestSuite) TestIncludeSuppressedBeneficiariesIntegration() {


I really liked the strategy outlined in the second response in this post about how to discern between unit and integration tests when running tests, hence the name updates.

Quick thought: What about TestIncludeSuppressedBeneficiaries_Integration? Makes it pop out more to me.

Is there a case to be made for adding the t.Short clause or do we pretty much always want to run everything?

yes and yes! I like the snake_case to make it more visible and eventually, we would want to include the if t.short; t.skip() line in the integration tests!

laurenkrugen-navapbc · 2024-09-18T22:41:35Z

bcda/service/service_test.go

@@ -225,7 +228,7 @@ func (s *ServiceTestSuite) TestIncludeSuppressedBeneficiaries() {
 	}
 }

-func (s *ServiceTestSuite) TestGetNewAndExistingBeneficiaries() {
+func (s *ServiceTestSuite) TestGetNewAndExistingBeneficiariesIntegration() {


There are 2-3 tests that I had to make minor updates to; because we are filtering by the ACO model now, the configured test CMS ID of cmsID was not being parsed and these tests also weren't loading the config files to do the parsing.

laurenkrugen-navapbc · 2024-09-18T22:55:58Z

bcda/service/service_test.go

+}
+
+func (s *ServiceTestSuiteWithDatabase) TestGetNewAndExistingBeneficiaries_RecentSinceParameterDatabaseIntegration() {
+	db := database.Connection


this was an already existing test that it utilizing the database's package init(). I moved it under the new test suite since we should eventually remove the dependency on init() and instead use the 'CreateDatabase` function to stand up our test databases. I will continue to work on this if it fits into the sprint.

Im not fully following all of this, maybe we can talk offline and would love to get Kevins perspective here.

carlpartridge · 2024-09-19T13:23:24Z

bcda/service/service.go

-		if err != nil {
-			return nil, fmt.Errorf("failed to retreive suppressedMBIs %s", err.Error())
+		if cfg, ok := s.GetACOConfigForID(conditions.CMSID); ok {
+			if cfg.Model != "TCOCMD" {


In NewService Im seeing:

return &service{ ... sp: suppressionParameters{ includeSuppressedBeneficiaries: false, lookbackDays: cfg.SuppressionLookbackDays, }, ...

Is there a reason we cant use includeSuppressedBeneficiaries as opposed to a hardcode check of "TCOCMD"?

This means when we create the MD TCoC service (or link it to the ACO?) we will have to override the suppression parameters?

yeah, I saw that too and would much more prefer to use that but it's part of the service structure that's instantiated with our handlers (called on service start instead of per request).

Ok so a service is instantiated on app start. But we pass something in as part of each request (ACO ID?) to get the specific request ACO out of all of the available ACOConfigs of the service. It looks like we are pulling ACOConfigs from files in conf/config/{env}.yml? Is there a reason we cant add onto these a new field eg ignoreSuppressed?

Somewhat related question how do these ACOConfigs compare with what we have in the acos DB table? Looks like the DB table is just identifiers and web/auth related, no config?

carlpartridge · 2024-09-19T13:27:28Z

bcda/service/service_test.go

@@ -170,7 +173,7 @@ func (s *ServiceTestSuite) TearDownTest() {
 	conf.SetEnv(s.T(), "PRIORITY_ACO_REG_EX", s.priorityACOsEnvVar)
 }

-func (s *ServiceTestSuite) TestIncludeSuppressedBeneficiaries() {
+func (s *ServiceTestSuite) TestIncludeSuppressedBeneficiariesIntegration() {


Quick thought: What about TestIncludeSuppressedBeneficiaries_Integration? Makes it pop out more to me.

Is there a case to be made for adding the t.Short clause or do we pretty much always want to run everything?

carlpartridge · 2024-09-19T13:50:38Z

bcda/service/service_test.go

+		mbis      []string
+	}{
+		{"TCOCMD request with active suppressions", "TCOCMD", 7, []string{"MBI00000001", "MBI00000002", "MBI00000003", "MBI00000004", "MBI00000005", "MBI00000006", "MBI00000007"}},
+		{"Non TCOCMD request with active suppressions", "A0001", 4, []string{"MBI00000004", "MBI00000005", "MBI00000006", "MBI00000007"}},


Related to trying to move the check for "TCOC" to a var, could we rename these to something like:
"ACO with ignored suppressions request with active suppressions" and
"ACO request with active suppressions"
I think keeping specifics out keeps things a bit cleaner down the road.

carlpartridge · 2024-09-19T13:51:25Z

bcda/service/service_test.go

+}
+
+func (s *ServiceTestSuiteWithDatabase) TestGetNewAndExistingBeneficiaries_RecentSinceParameterDatabaseIntegration() {
+	db := database.Connection


Im not fully following all of this, maybe we can talk offline and would love to get Kevins perspective here.

carlpartridge · 2024-09-19T14:48:03Z

go.mod

 	github.com/golang-migrate/migrate/v4 v4.14.1
-	github.com/golang/protobuf v1.5.3 // indirect
+	github.com/golang/protobuf v1.5.4 // indirect


Can we close out this PR: #923?

that PR references a different package (weirdly), so we can check in to see if that PR can be merged.

exclude tcoc requests from suppression

7dccbcc

laurenkrugen-navapbc commented Sep 18, 2024

View reviewed changes

remove foo bar

cb2c983

laurenkrugen-navapbc commented Sep 18, 2024

View reviewed changes

laurenkrugen-navapbc requested review from kyeah, carlpartridge, bhagatparwinder and austincanada September 18, 2024 22:58

carlpartridge reviewed Sep 19, 2024

View reviewed changes

Merge branch 'main' into lauren/BCDA-8351

54725b9

laurenkrugen-navapbc marked this pull request as ready for review September 19, 2024 17:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BCDA-8351: Exclude MD TCoC requests from suppression #993

BCDA-8351: Exclude MD TCoC requests from suppression #993

laurenkrugen-navapbc commented Sep 18, 2024

laurenkrugen-navapbc Sep 18, 2024

laurenkrugen-navapbc Sep 18, 2024

carlpartridge Sep 19, 2024

laurenkrugen-navapbc Sep 19, 2024

laurenkrugen-navapbc Sep 18, 2024

laurenkrugen-navapbc Sep 18, 2024

carlpartridge Sep 19, 2024

carlpartridge Sep 19, 2024

laurenkrugen-navapbc Sep 19, 2024

carlpartridge Sep 19, 2024 •

edited

Loading

carlpartridge Sep 19, 2024

carlpartridge Sep 19, 2024

carlpartridge Sep 19, 2024

carlpartridge Sep 19, 2024

laurenkrugen-navapbc Sep 19, 2024

BCDA-8351: Exclude MD TCoC requests from suppression #993

Are you sure you want to change the base?

BCDA-8351: Exclude MD TCoC requests from suppression #993

Conversation

laurenkrugen-navapbc commented Sep 18, 2024

🎫 Ticket

🛠 Changes

ℹ️ Context

🧪 Validation

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

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

Choose a reason for hiding this comment

carlpartridge Sep 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlpartridge Sep 19, 2024 •

edited

Loading