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.

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

[BACK-3177] Allow specifying custom cadence for EHR reports #769

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions auth/service/api/v1/auth_service_mock.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions auth/test/mock.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 15 additions & 0 deletions clinics/mock.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 12 additions & 0 deletions clinics/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ var ClientModule = fx.Provide(NewClient)

type Client interface {
GetClinician(ctx context.Context, clinicID, clinicianID string) (*clinic.Clinician, error)
GetEHRSettings(ctx context.Context, clinicId string) (*clinic.EHRSettings, error)
SharePatientAccount(ctx context.Context, clinicID, patientID string) (*clinic.Patient, error)
ListEHREnabledClinics(ctx context.Context) ([]clinic.Clinic, error)
SyncEHRData(ctx context.Context, clinicID string) error
Expand Down Expand Up @@ -121,6 +122,17 @@ func (d *defaultClient) ListEHREnabledClinics(ctx context.Context) ([]clinic.Cli
return clinics, nil
}

func (d *defaultClient) GetEHRSettings(ctx context.Context, clinicId string) (*clinic.EHRSettings, error) {
response, err := d.httpClient.GetEHRSettingsWithResponse(ctx, clinicId)
if err != nil {
return nil, err
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be a place to wrap with the existing platform error code to gain the benefits it brings.

}
if response.StatusCode() != http.StatusOK || response.StatusCode() != http.StatusOK {
return nil, fmt.Errorf("unexpected response status code %v from %v", response.StatusCode(), response.HTTPResponse.Request.URL)
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider if this should use the existing platform errors code.

}
return response.JSON200, nil
}

func (d *defaultClient) SharePatientAccount(ctx context.Context, clinicID, patientID string) (*clinic.Patient, error) {
permission := make(map[string]interface{}, 0)
body := clinic.CreatePatientFromUserJSONRequestBody{
Expand Down
25 changes: 25 additions & 0 deletions clinics/test/clinics.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,28 @@ func NewRandomClinic() api.Clinic {
Website: pointer.FromAny(faker.Internet().Url()),
}
}

func NewRandomEHRSettings() *api.EHRSettings {
return &api.EHRSettings{
DestinationIds: &api.EHRDestinationIds{
Flowsheet: faker.RandomString(16),
Notes: faker.RandomString(16),
Results: faker.RandomString(16),
},
Enabled: true,
MrnIdType: "MRN",
ProcedureCodes: api.EHRProcedureCodes{
CreateAccount: pointer.FromAny(faker.RandomString(5)),
CreateAccountAndEnableReports: pointer.FromAny(faker.RandomString(5)),
DisableSummaryReports: pointer.FromAny(faker.RandomString(5)),
EnableSummaryReports: pointer.FromAny(faker.RandomString(5)),
},
Provider: "redox",
ScheduledReports: api.ScheduledReports{
Cadence: api.N14d,
OnUploadEnabled: true,
OnUploadNoteEventType: pointer.FromAny(api.ScheduledReportsOnUploadNoteEventTypeNew),
},
SourceId: faker.RandomString(16),
}
}
1 change: 1 addition & 0 deletions data/service/api/v1/mocks/mocklogger_test_gen.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

87 changes: 87 additions & 0 deletions ehr/reconcile/planner.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
package reconcile

import (
"context"

duration "github.com/xhit/go-str2duration/v2"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a fan of taking a dependency for this. We support literally four values, I don't think we need a 3rd-party dependency to handle that now, or in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the dependency. I don't want to have to update 2 services if we decide to support 60 day reports one day, so I had to implement the parsing logic myself. I think this results in less readable code.


"github.com/tidepool-org/platform/clinics"
"github.com/tidepool-org/platform/ehr/sync"
"github.com/tidepool-org/platform/log"
"github.com/tidepool-org/platform/task"
)

type Planner struct {
clinicsClient clinics.Client
logger log.Logger
}

func NewPlanner(clinicsClient clinics.Client, logger log.Logger) *Planner {
return &Planner{
clinicsClient: clinicsClient,
logger: logger,
}
}

func (p *Planner) GetReconciliationPlan(ctx context.Context, syncTasks map[string]task.Task) (*ReconciliationPlan, error) {
toCreate := make([]task.TaskCreate, 0)
toDelete := make([]task.Task, 0)
toUpdate := make(map[string]*task.TaskUpdate)

// Get the list of all EHR enabled clinics
clinicsList, err := p.clinicsClient.ListEHREnabledClinics(ctx)
if err != nil {
return nil, err
}

// At the end of the loop syncTasks will contain only the tasks that need to be deleted,
// and toCreate will contain tasks for new clinics that need to be synced.
for _, clinic := range clinicsList {
clinicId := *clinic.Id
settings, err := p.clinicsClient.GetEHRSettings(ctx, clinicId)
if err != nil {
return nil, err
Copy link
Contributor

Choose a reason for hiding this comment

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

The clinicsClient appears to be generated code that doesn't return platform errors. Therefore you might consider if these should use errors.Wrap. (I won't mention this any more for this PR, but there are other places I haven't pointed out.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you please clarify what are the benefits of using the platform errors.Wrap, because I'm not familiar with those.

Copy link
Contributor

Choose a reason for hiding this comment

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

@darinkrauss is for sure the expert on that. But I believe he'd say they bring a consistent format to errors, and sometimes they add line numbers for finding the source of the error.

I thought the errors package was still in use for new code in platform, no? If not, I'll make a note and try to stop using/suggesting it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've been using the standard library most recently. This may be incorrect, but it's not obvious or documented why the platform errors package should be used instead, hence my question.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, what do folks think about me writing a "Guide to Writing Consistent Code in the Platform Repo"? 😉

Yes, there is a reason to use the Platform errors package and, more explicitly, the errors.Wrap functionality. The errors package adds a lot more context to the error that chains correctly with multiple wraps. Now, while the current Golang errors package does some of that, the rest of Platform isn't built to handle that in any way.

So, in short, if you want errors to propagate correctly and do the "right thing" in the rest of Platform, then use the Platform errors package and use errors.Wrap when you want to add further context or explanation to the error.

I'm fine if we want to intentionally revisit this and make appropriate changes, but IMO, we should stick with how it is currently done so it works and is written as expected.

My 2 cents...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the explanation - I'll make the change, because those errors might eventually bubble up to the task error handler.

} else if settings == nil || !settings.Enabled {
continue
}

// Use the default value for all clinics which don't have a cadence
cadenceFromSettings := sync.DefaultCadence
parsed, err := duration.ParseDuration(string(settings.ScheduledReports.Cadence))
if err != nil {
p.logger.WithField("clinicId", clinicId).WithError(err).Error("unable to parse scheduled report cadence")
continue
}
cadenceFromSettings = parsed

tsk, exists := syncTasks[clinicId]
if exists {

delete(syncTasks, clinicId)
if cadenceFromSettings == 0 {
toDelete = append(toDelete, tsk)
continue
}

cadenceFromTask := sync.GetCadence(tsk.Data)
if cadenceFromTask == nil || *cadenceFromTask != cadenceFromSettings {
sync.SetCadence(tsk.Data, cadenceFromSettings)
update := task.NewTaskUpdate()
update.Data = &tsk.Data
toUpdate[tsk.ID] = update
}
} else if cadenceFromSettings != 0 {
// The task doesn't exist yet and scheduled reports are not disabled
create := sync.NewTaskCreate(clinicId, cadenceFromSettings)
toCreate = append(toCreate, *create)
}
}
for _, tsk := range syncTasks {
toDelete = append(toDelete, tsk)
}
return &ReconciliationPlan{
ToCreate: toCreate,
ToDelete: toDelete,
ToUpdate: toUpdate,
}, nil
}
190 changes: 190 additions & 0 deletions ehr/reconcile/planner_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,190 @@
package reconcile_test

import (
"context"

"github.com/golang/mock/gomock"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
. "github.com/onsi/gomega/gstruct"
api "github.com/tidepool-org/clinic/client"

"github.com/tidepool-org/platform/clinics"
"github.com/tidepool-org/platform/log"
"github.com/tidepool-org/platform/log/null"

clinicsTest "github.com/tidepool-org/platform/clinics/test"
"github.com/tidepool-org/platform/ehr/reconcile"
"github.com/tidepool-org/platform/ehr/sync"
"github.com/tidepool-org/platform/task"
"github.com/tidepool-org/platform/test"
)

var _ = Describe("Planner", func() {
var authCtrl *gomock.Controller
var clinicsCtrl *gomock.Controller
var taskCtrl *gomock.Controller

var clinicsClient *clinics.MockClient
var logger log.Logger
var planner *reconcile.Planner

BeforeEach(func() {
authCtrl = gomock.NewController(GinkgoT())
clinicsCtrl = gomock.NewController(GinkgoT())
taskCtrl = gomock.NewController(GinkgoT())
clinicsClient = clinics.NewMockClient(clinicsCtrl)
logger = null.NewLogger()
planner = reconcile.NewPlanner(clinicsClient, logger)
})

AfterEach(func() {
authCtrl.Finish()
clinicsCtrl.Finish()
taskCtrl.Finish()
})

Context("With random data", func() {
var clinics []api.Clinic
var tasks map[string]task.Task

BeforeEach(func() {
clinics = test.RandomArrayWithLength(3, clinicsTest.NewRandomClinic)
tasks = make(map[string]task.Task)
for _, clinic := range clinics {
clinic := clinic
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't necessary here is it? I ran the test both ways and it succeeded, but sometimes these things are tricky, right?

tsk, err := task.NewTask(sync.NewTaskCreate(*clinic.Id, sync.DefaultCadence))
Expect(err).ToNot(HaveOccurred())
Expect(tsk).ToNot(BeNil())
tasks[*clinic.Id] = *tsk
}
})

Describe("GetReconciliationPlan", func() {
It("returns an empty plan when each clinic has a corresponding task", func() {
clinicsClient.EXPECT().ListEHREnabledClinics(gomock.Any()).Return(clinics, nil)
setupEHRSettingsForClinics(clinicsClient, clinics)

plan, err := planner.GetReconciliationPlan(context.Background(), tasks)
Expect(err).ToNot(HaveOccurred())
Expect(plan).ToNot(BeNil())
Expect(plan.ToCreate).To(BeEmpty())
Expect(plan.ToDelete).To(BeEmpty())
})

It("returns a clinic creation task when a task for the clinic doesn't exist", func() {
clinicsClient.EXPECT().ListEHREnabledClinics(gomock.Any()).Return(clinics, nil)
setupEHRSettingsForClinics(clinicsClient, clinics)
delete(tasks, *clinics[0].Id)

plan, err := planner.GetReconciliationPlan(context.Background(), tasks)
Expect(err).ToNot(HaveOccurred())
Expect(plan).ToNot(BeNil())
Expect(plan.ToCreate).To(HaveLen(1))
Expect(plan.ToCreate[0].Name).To(PointTo(Equal(sync.TaskName(*clinics[0].Id))))
Expect(plan.ToDelete).To(BeEmpty())
})

It("returns multiple clinic creation tasks when multiple clinics don't exist", func() {
clinicsClient.EXPECT().ListEHREnabledClinics(gomock.Any()).Return(clinics, nil)
setupEHRSettingsForClinics(clinicsClient, clinics)
delete(tasks, *clinics[1].Id)
delete(tasks, *clinics[2].Id)

plan, err := planner.GetReconciliationPlan(context.Background(), tasks)
Expect(err).ToNot(HaveOccurred())
Expect(plan).ToNot(BeNil())
Expect(plan.ToCreate).To(HaveLen(2))
Expect(plan.ToCreate[0].Name).To(PointTo(Equal(sync.TaskName(*clinics[1].Id))))
Expect(plan.ToCreate[1].Name).To(PointTo(Equal(sync.TaskName(*clinics[2].Id))))
Expect(plan.ToDelete).To(BeEmpty())
})

It("returns a clinic for deletion when the task doesn't exist", func() {
deleted := clinics[2]
clinics = clinics[0:2]
clinicsClient.EXPECT().ListEHREnabledClinics(gomock.Any()).Return(clinics, nil)
setupEHRSettingsForClinics(clinicsClient, clinics)

plan, err := planner.GetReconciliationPlan(context.Background(), tasks)
Expect(err).ToNot(HaveOccurred())
Expect(plan).ToNot(BeNil())
Expect(plan.ToCreate).To(BeEmpty())
Expect(plan.ToDelete).To(HaveLen(1))
Expect(plan.ToDelete[0].Name).To(PointTo(Equal(sync.TaskName(*deleted.Id))))
})

It("returns multiple clinics for deletion when multiple tasks don't exist", func() {
firstDeleted := clinics[1]
secondDeleted := clinics[2]
clinics = []api.Clinic{clinics[0]}

clinicsClient.EXPECT().ListEHREnabledClinics(gomock.Any()).Return(clinics, nil)
setupEHRSettingsForClinics(clinicsClient, clinics)

plan, err := planner.GetReconciliationPlan(context.Background(), tasks)
Expect(err).ToNot(HaveOccurred())
Expect(plan).ToNot(BeNil())
Expect(plan.ToCreate).To(BeEmpty())
Expect(plan.ToDelete).To(HaveLen(2))
Expect(
[]string{*plan.ToDelete[0].Name, *plan.ToDelete[1].Name},
).To(
ConsistOf(sync.TaskName(*firstDeleted.Id), sync.TaskName(*secondDeleted.Id)),
)
})

It("returns a task for deletion when the report settings are disabled", func() {
settings := clinicsTest.NewRandomEHRSettings()
settings.ScheduledReports.Cadence = api.N0d
Copy link
Contributor

Choose a reason for hiding this comment

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

Hah! Here's the bit from the clinic PR that I needed to know this was a disable setting! :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed this to DISABLED. I'm not a big fan of the resulting code, but at least it makes it obvious to the reader when a task is disabled.


clinics = clinics[0:1]
clinicsClient.EXPECT().ListEHREnabledClinics(gomock.Any()).Return(clinics, nil)
clinicsClient.EXPECT().GetEHRSettings(gomock.Any(), *clinics[0].Id).Return(settings, nil)

tasks = map[string]task.Task{
*clinics[0].Id: tasks[*clinics[0].Id],
}

plan, err := planner.GetReconciliationPlan(context.Background(), tasks)
Expect(err).ToNot(HaveOccurred())
Expect(plan).ToNot(BeNil())
Expect(plan.ToCreate).To(BeEmpty())
Expect(plan.ToUpdate).To(BeEmpty())
Expect(plan.ToDelete).To(HaveLen(1))
})

It("returns a task for update when the report cadence is different", func() {
settings := clinicsTest.NewRandomEHRSettings()
settings.ScheduledReports.Cadence = api.N7d

clinics = clinics[0:1]
clinicsClient.EXPECT().ListEHREnabledClinics(gomock.Any()).Return(clinics, nil)
clinicsClient.EXPECT().GetEHRSettings(gomock.Any(), *clinics[0].Id).Return(settings, nil)

tsk := tasks[*clinics[0].Id]
tasks = map[string]task.Task{
*clinics[0].Id: tsk,
}

plan, err := planner.GetReconciliationPlan(context.Background(), tasks)
Expect(err).ToNot(HaveOccurred())
Expect(plan).ToNot(BeNil())
Expect(plan.ToCreate).To(BeEmpty())
Expect(plan.ToDelete).To(BeEmpty())
Expect(plan.ToUpdate).To(HaveLen(1))

update, exists := plan.ToUpdate[tsk.ID]
Expect(exists).To(BeTrue())
Expect(update.Data).ToNot(BeNil())
Expect((*update.Data)["cadence"]).To(Equal("168h0m0s"))
})
})
})
})

func setupEHRSettingsForClinics(clinicsClient *clinics.MockClient, clinics []api.Clinic) {
for _, clinic := range clinics {
clinicsClient.EXPECT().GetEHRSettings(gomock.Any(), *clinic.Id).Return(clinicsTest.NewRandomEHRSettings(), nil)
}
}
Loading