[BACK-2870] [BACK-3176] EHR Patient Tags #72

toddkazakov · 2024-09-26T15:30:09Z

Allow creating and associating patient tags with Redox create account orders
Allow creating accounts and enabling reports with a single order

… orders

ewollesen

Looks good, just a few suggestions/questions. Nothing blocking.

I noticed that very few error paths are covered by tests. When the return path is return err, this doesn't bother me much, but many of them involve sending messages or other non-trivial logic. It's probably worth trying to cover a few more of them.

ewollesen · 2024-10-02T14:53:30Z

redox/processor_neworder.go

+	if err != nil {
+		return nil, err
+	}
+	if resp.StatusCode() != http.StatusOK {


I know these clients are automatically generated, but I really hate how they leak HTTP like a sieve. Sure, I don't see us using anything other than HTTP for this, but that's only a small part of why we abstract away those details. Oh well. (Not a comment on your code, just lamenting the leakiness of the client's abstraction.)

ewollesen · 2024-10-02T14:57:34Z

redox/processor_neworder.go

+		return nil
+	}
+
+	patient := (*match.Patients)[0]


Update: this probably isn't necessary, feel free to ignore.

Suggested change

patient := (*match.Patients)[0]

if len(match.Patients) < 1 {

return fmt.Errorf("no patient in match")

}

patient := (*match.Patients)[0]

Just to prevent a possible panic in prod.

ewollesen · 2024-10-02T14:59:03Z

redox/processor_neworder.go

+		TargetDevices:                  patient.TargetDevices,
+		Tags:                           tagIds,
+	}
+	resp, err := o.clinics.UpdatePatientWithResponse(ctx, *match.Clinic.Id, *patient.Id, update)


Update: this probably isn't necessary, feel free to ignore.

Suggested change

resp, err := o.clinics.UpdatePatientWithResponse(ctx, *match.Clinic.Id, *patient.Id, update)

if match.Clinic == nil {

return fmt.Errorf("no clinic found in match")

}

resp, err := o.clinics.UpdatePatientWithResponse(ctx, *match.Clinic.Id, *patient.Id, update)

Again, just to prevent possible panics in prod.

ewollesen · 2024-10-02T15:01:23Z

redox/processor_neworder.go

+func (o *newOrderProcessor) getPatientTagCodes(match clinics.EHRMatchResponse) map[string]struct{} {
+	result := make(map[string]struct{})
+	if match.Settings.Tags.Codes != nil {
+		for _, code := range *match.Settings.Tags.Codes {


Here, like previous comments, is an unchecked pointer (Codes). This could panic. While highly unlikely, consider if it's worth a check to prevent.

...or is this all behind a HTTP request anyway that will recover from the panic I guess? Maybe my previous comments aren't really necessary either then, as I assume this being nil (at least when you expect it not to be nil) is quite rare. I don't love that, but it's probably the best solution.

I've updated my previous comments for these kinds of things under the assumption that in the rare event these were to panic, there is a recover() somewhere that would prevent the service from terminating due to nil pointer dereference.

Line 358 is if match.Settings.Tags.Codes != nil { so we are checking if Codes is nil

ewollesen · 2024-10-02T15:07:07Z

redox/processor_neworder.go

+		return nil
+	}
+
+	tagNames := make([]string, 0)


Just a curiosity question if you know off of the top of your head:

Does this offer any benefit vs tagNames := []string{}? I find this a little clearer, and I think they're equivalent, but every time I think I've got the semantics of make() memorized I find small new edge case. :)

The IntelliJ linter flags []string{} for some reason. I don't know if there's any difference.

ewollesen · 2024-10-02T15:16:30Z

redox/processor_neworder.go

+	if order.Order.ClinicalInfo != nil {
+		for _, info := range *order.Order.ClinicalInfo {
+			if info.Code != nil && info.Value != nil {
+				if _, ok := codes[*info.Code]; ok {
+					if separator == nil || *separator == "" {
+						tagNames = append(tagNames, strings.TrimSpace(*info.Value))
+					} else {
+						for _, tag := range strings.Split(*info.Value, *separator) {
+							tagNames = append(tagNames,  strings.TrimSpace(tag))
+						}
+					}
+				}
+			}
+		}
+	}


Suggested change

if order.Order.ClinicalInfo != nil {

for _, info := range *order.Order.ClinicalInfo {

if info.Code != nil && info.Value != nil {

if _, ok := codes[*info.Code]; ok {

if separator == nil || *separator == "" {

tagNames = append(tagNames, strings.TrimSpace(*info.Value))

} else {

for _, tag := range strings.Split(*info.Value, *separator) {

tagNames = append(tagNames, strings.TrimSpace(tag))

}

}

}

}

}

}

if *order.Order.ClinicalInfo == nil {

return nil

}

for _, info := range *order.Order.ClinicalInfo {

if info.Code == nil || info.Value == nil {

continue

}

if _, found := codes[*info.Code]; !found {

continue

}

if separator == nil || *separator == "" {

tagNames = append(tagNames, strings.TrimSpace(*info.Value))

continue

}

for _, tag := range strings.Split(*info.Value, *separator) {

tagNames = append(tagNames, strings.TrimSpace(tag))

}

}

Just a suggestion. I find it a bit easier to read, as it doesn't indent as much, and I feel like I don't have to keep as much in my head at once.

Thanks for the suggestion, definitely more reasble.

ewollesen · 2024-10-02T15:34:43Z

redox/processor_neworder.go

@@ -81,48 +81,57 @@ func NewNewOrderProcessor(clinics clinics.ClientWithResponsesInterface, redox Cl
 }

 func (o *newOrderProcessor) ProcessOrder(ctx context.Context, envelope models.MessageEnvelope, order models.NewOrder) error {
+	match, err := o.matchOrder(ctx, envelope.Id.Hex(), order)


I like the extraction of matchOrder here, feels good. 👍🏻

toddkazakov · 2024-10-25T09:11:43Z

/deploy qa2

tidebot · 2024-10-25T09:11:48Z

toddkazakov updated values.yaml file in qa2

tidebot · 2024-10-25T09:11:49Z

toddkazakov updated flux policies file in qa2

tidebot · 2024-10-25T09:11:50Z

toddkazakov deployed clinic-worker tk-order-patient-tags branch to qa2 namespace

…orts orders

toddkazakov added 2 commits September 26, 2024 18:29

Allow creating and associating patient tags with Redox create account…

3ab94e1

… orders

Replace patient tags on enrollment

71771fd

toddkazakov force-pushed the tk-order-patient-tags branch from aad3d51 to 71771fd Compare October 1, 2024 10:10

toddkazakov changed the title ~~Allow creating and associating patient tags with Redox create account…~~ [BACK-3176] EHR Patient Tags Oct 1, 2024

toddkazakov requested a review from ewollesen October 1, 2024 10:11

Allow creating a patient and enabling reports with a single order

67d5f95

ewollesen previously approved these changes Oct 2, 2024

View reviewed changes

Address code review feedback

4cae5b0

toddkazakov dismissed ewollesen’s stale review via 4cae5b0 October 8, 2024 10:59

toddkazakov changed the title ~~[BACK-3176] EHR Patient Tags~~ [BACK-2870] [BACK-3176] EHR Patient Tags Oct 25, 2024

Don't send successful match message for create account and enable rep…

2076bfe

…orts orders

toddkazakov force-pushed the tk-order-patient-tags branch from a2acb74 to 2076bfe Compare November 13, 2024 12:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BACK-2870] [BACK-3176] EHR Patient Tags #72

[BACK-2870] [BACK-3176] EHR Patient Tags #72

toddkazakov commented Sep 26, 2024 •

edited

Loading

ewollesen left a comment

ewollesen Oct 2, 2024

ewollesen Oct 2, 2024

ewollesen Oct 2, 2024

ewollesen Oct 2, 2024

toddkazakov Oct 2, 2024

ewollesen Oct 2, 2024

toddkazakov Oct 2, 2024

ewollesen Oct 2, 2024

toddkazakov Oct 8, 2024

ewollesen Oct 2, 2024

toddkazakov commented Oct 25, 2024

tidebot commented Oct 25, 2024

tidebot commented Oct 25, 2024

tidebot commented Oct 25, 2024

[BACK-2870] [BACK-3176] EHR Patient Tags #72

Are you sure you want to change the base?

[BACK-2870] [BACK-3176] EHR Patient Tags #72

Conversation

toddkazakov commented Sep 26, 2024 • edited Loading

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

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

toddkazakov commented Oct 25, 2024

tidebot commented Oct 25, 2024

tidebot commented Oct 25, 2024

tidebot commented Oct 25, 2024

toddkazakov commented Sep 26, 2024 •

edited

Loading