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

toddkazakov · 2024-09-21T07:51:30Z

No description provided.

ewollesen

LGTM. No blockers, but I'll reiterate that I'm against taking a dependency for the duration parsing we're doing here.

ewollesen · 2024-09-23T20:49:54Z

ehr/reconcile/planner_test.go

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


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

ewollesen · 2024-09-23T20:54:23Z

ehr/reconcile/planner_test.go

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


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

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.

ewollesen · 2024-09-23T21:19:55Z

ehr/sync/task.go

+	cadence := data["cadence"].(string)
+	parsed, err := time.ParseDuration(cadence)
+	if err != nil {
+		return nil


I think it would simplify the logic if instead of returning nil, you returned sync.DefaultCadence. Both ScheduleNextExecution and GetReconciliationPlan check if the response is nil, and if so, use the default value.

To go one step further, you could instead return the error (func GetCandence(...) (time.Duration, error)) so the caller (who presumably has more context) could decide how to handle that (whether that means using the default, or maybe logging the error before using the default) or maybe using some other value. Just returning nil (or a default) hides the error entirely. If it's really not something we care about, that's fine. If you think there's ever a time you'd want to use that error, consider returning it.

But a minor thing to be sure, and fine how it is.

Both ScheduleNextExecution and GetReconciliationPlan check if the response is nil, and if so, use the default value.

This statement is not true. Planner doesn't use the default value, it check whether the cadence in the task is difference that the one in the settings, and if that's the case it updates the task:

// Update the task if the configured cadence has been changed 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 }

This function returns exactly what the name implies. If parsing fails, then the cadence is not correctly set so it will be updated as configured. I could return the parsing error, but it will be ignored by the caller.

This statement is not true.

You're right. I missed a case where the default might not be used.

Many minutes of reading the code later...

OK, I'm good with this now.

ewollesen · 2024-09-23T21:21:43Z

ehr/sync/task.go

+}
+
+func GetCadence(data map[string]interface{}) *time.Duration {
+	cadence := data["cadence"].(string)


Uck. I get why this is here, but it's unfortunate. :( I guess someday we could update the task service now that Generics in Go are a thing.

ewollesen · 2024-09-23T21:25:54Z

ehr/reconcile/planner.go

+import (
+	"context"
+
+	duration "github.com/xhit/go-str2duration/v2"


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.

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.

ewollesen · 2024-09-23T21:31:05Z

clinics/service.go

+		return nil, err
+	}
+	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)


Consider if this should use the existing platform errors code.

ewollesen · 2024-09-23T21:31:28Z

clinics/service.go

+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


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

ewollesen · 2024-09-23T21:36:06Z

ehr/reconcile/planner.go

+		clinicId := *clinic.Id
+		settings, err := p.clinicsClient.GetEHRSettings(ctx, clinicId)
+		if err != nil {
+			return nil, err


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.)

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

@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.

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.

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...

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

ewollesen

Thank you for pointing out my misunderstanding. Still no blockers IMO. See @darinkrauss re platform/errors usage, if my explanation isn't satisfactory.

ewollesen · 2024-10-01T15:22:25Z

ehr/reconcile/planner.go

+		clinicId := *clinic.Id
+		settings, err := p.clinicsClient.GetEHRSettings(ctx, clinicId)
+		if err != nil {
+			return nil, err


@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.

ewollesen · 2024-10-01T15:56:20Z

ehr/sync/task.go

+	cadence := data["cadence"].(string)
+	parsed, err := time.ParseDuration(cadence)
+	if err != nil {
+		return nil


This statement is not true.

You're right. I missed a case where the default might not be used.

Many minutes of reading the code later...

OK, I'm good with this now.

ewollesen · 2024-10-01T16:02:01Z

Oh, looks like there's a go import re-ordering that needs to be fixed to make the linter happy.

https://app.travis-ci.com/github/tidepool-org/platform/builds/272549249#L442

…duled-reports

toddkazakov changed the title ~~Allow specifying custom cadence for EHR reports~~ [BACK-3177] Allow specifying custom cadence for EHR reports Sep 21, 2024

toddkazakov requested a review from ewollesen September 21, 2024 07:55

Allow specifying custom cadence for EHR reports

fe0cf08

toddkazakov force-pushed the tk-ehr-disable-scheduled-reports branch from d16802a to fe0cf08 Compare September 21, 2024 07:56

ewollesen previously approved these changes Sep 23, 2024

View reviewed changes

ewollesen self-assigned this Sep 23, 2024

toddkazakov dismissed ewollesen’s stale review via d725e3e September 26, 2024 13:51

toddkazakov requested a review from ewollesen September 26, 2024 13:51

toddkazakov force-pushed the tk-ehr-disable-scheduled-reports branch from d725e3e to 7f67c05 Compare October 1, 2024 10:14

ewollesen previously approved these changes Oct 1, 2024

View reviewed changes

Address code review feedback

5100baa

toddkazakov dismissed ewollesen’s stale review via 5100baa October 2, 2024 07:08

toddkazakov force-pushed the tk-ehr-disable-scheduled-reports branch from 7f67c05 to 5100baa Compare October 2, 2024 07:08

ewollesen self-requested a review October 2, 2024 15:47

ewollesen previously approved these changes Oct 2, 2024

View reviewed changes

toddkazakov dismissed ewollesen’s stale review via a311e45 October 7, 2024 16:39

Use 'platform' errors in clinic client

2b21ec7

toddkazakov force-pushed the tk-ehr-disable-scheduled-reports branch from a311e45 to 2b21ec7 Compare October 8, 2024 22:18

Merge remote-tracking branch 'origin/master' into tk-ehr-disable-sche…

4404cc9

…duled-reports

toddkazakov force-pushed the tk-ehr-disable-scheduled-reports branch from a3ee552 to 4404cc9 Compare October 25, 2024 09:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

toddkazakov commented Sep 21, 2024

ewollesen left a comment •

edited

Loading

ewollesen Sep 23, 2024

ewollesen Sep 23, 2024

toddkazakov Sep 26, 2024

ewollesen Sep 23, 2024

toddkazakov Sep 26, 2024

ewollesen Oct 1, 2024

ewollesen Sep 23, 2024

ewollesen Sep 23, 2024

toddkazakov Sep 26, 2024

ewollesen Sep 23, 2024

ewollesen Sep 23, 2024

ewollesen Sep 23, 2024

toddkazakov Sep 26, 2024

ewollesen Oct 1, 2024

toddkazakov Oct 1, 2024

darinkrauss Oct 2, 2024

toddkazakov Oct 3, 2024

ewollesen left a comment

ewollesen Oct 1, 2024

ewollesen Oct 1, 2024

ewollesen commented Oct 1, 2024

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

Are you sure you want to change the base?

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

Conversation

toddkazakov commented Sep 21, 2024

ewollesen left a comment • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ewollesen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ewollesen commented Oct 1, 2024

ewollesen left a comment •

edited

Loading