Skip to content

Conversation

@siddh1004
Copy link
Contributor

@siddh1004 siddh1004 force-pushed the siddharth-agarwal/sc-14495 branch 4 times, most recently from f57b392 to 78d9ff4 Compare January 20, 2025 10:21
@siddh1004 siddh1004 changed the title Update cv risk when smoking status is updated Update cv risk business logic Jan 20, 2025
@siddh1004 siddh1004 force-pushed the siddharth-agarwal/sc-14495 branch 2 times, most recently from 68242a4 to 61c7e6b Compare January 20, 2025 13:57
@msasikanth msasikanth force-pushed the siddharth-agarwal/sc-14495 branch from 61c7e6b to be8a96f Compare January 22, 2025 04:19
@siddh1004 siddh1004 force-pushed the siddharth-agarwal/sc-14495 branch from be8a96f to 7371b6b Compare January 22, 2025 12:51
@siddh1004 siddh1004 changed the title Update cv risk business logic Show statin nudge for 90 calendar days and recalculate cvd risk after 90 days Jan 22, 2025
@siddh1004 siddh1004 marked this pull request as ready for review January 22, 2025 13:54
@simple-services simple-services requested review from a team and msasikanth and removed request for a team January 22, 2025 13:54
@github-actions github-actions bot enabled auto-merge (squash) January 22, 2025 13:54
@siddh1004 siddh1004 force-pushed the siddharth-agarwal/sc-14495 branch from a610ed4 to 43d9b55 Compare January 23, 2025 06:14
dao.saveRisk(cvdRisk)
}

fun save(cvdRisk: CVDRisk, updateTime: Instant) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep the name consistent with the data, rename to updatedAt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


val wasCVDCalculatedWithin90Days = cvdRisk.timestamps.updatedAt > ninetyDaysAgo

val patientAttribute = patientAttributeRepository.getPatientAttributeImmediate(patientUuid = patient.uuid)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just confirming, if the patient attribute changes, don't we have to recalculate or load the info again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, patient attribute only change when BMI is added, and we have a event to handle this case. So we don't need to observe this situation

import androidx.room.RawQuery
import androidx.sqlite.db.SimpleSQLiteQuery
import io.reactivex.Flowable
import io.reactivex.Observable
Copy link
Contributor

Choose a reason for hiding this comment

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

Unused import?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Comment on lines 127 to 130
medicalHistoryRepository.hasMedicalHistoryForPatientChangedSince(
patientUuid = patient.uuid,
instant = cvdRisk?.timestamps?.updatedAt ?: today,
),
Copy link
Contributor

Choose a reason for hiding this comment

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

I am just thinking, can't we just place a observable to get medical history for patient and then compare the updatedAt time with the CVD risk created time if it's not null in the update function?

Do we need another query to do this? 🤔

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 feedback. Sounds really good and implemented that

.atStartOfDay(userClock.zone)
.toInstant()
val wasBPMeasuredWithin90Days = newestBp.firstOrNull()?.updatedAt?.let { updatedAt ->
updatedAt > ninetyDaysAgo
Copy link
Contributor

Choose a reason for hiding this comment

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

According the naming semantics shouldn't the condition be updatedAt < ninetyDaysAgo? Only then it would be true, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, i think it should be the way it has been done. We need the BP to be calculated after the ninety day time. So updatedAt has to greater than ninetyDayAgo.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that's why I was mentioning that condition. This variable will be false if the BP is measured with in 90 days right? Unless I misunderstand it.

So shouldn't the variable name be isBPMeasuredMoreThan90DaysAgo or the condition needs to be <, no?

} ?: false

val wasCVDCalculatedWithin90Days = cvdRisk?.let { risk ->
risk.timestamps.updatedAt > ninetyDaysAgo
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above regarding the condition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same comment as above

Comment on lines 191 to 219
var risk: CVDRiskRange? = null

if (bloodPressure != null) {
risk = cvdRiskCalculator.calculateCvdRisk(
CVDRiskInput(
gender = patient.gender,
age = patient.ageDetails.estimateAge(userClock),
systolic = bloodPressure.reading.systolic,
isSmoker = medicalHistory.isSmoking,
bmi = patientAttribute?.bmiReading?.calculateBMI(),
)
)
}

if (risk != null) {
val existingCvdRisk = cvdRiskRepository.getCVDRiskImmediate(patient.uuid)
if (existingCvdRisk != null) {
cvdRiskRepository.save(
existingCvdRisk.copy(riskScore = risk),
updateTime = Instant.now(clock)
)
} else {
cvdRiskRepository.save(
riskScore = risk,
patientUuid = patient.uuid,
uuid = uuidGenerator.v4()
)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think these conditions should be part of the effect handler, Update function should make the decision on whether to update or create a new one and trigger respective side effects. Please make that change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, we are technically doing 2 separate side effects here. We are calculating the risk and also saving/updating them right there. Calculating them and modifying them are 2 separate actions. We can break them down accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Broke this into new events and effects

Comment on lines +139 to +145
if (event.cvdRiskRange == null ||
event.hasMedicalHistoryChanged ||
!event.wasCVDCalculatedWithin90Days) {
dispatch(CalculateCVDRisk(model.patientSummaryProfile!!.patient))
} else {
dispatch(LoadStatinInfo(model.patientUuid))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nested conditional, let's break it down

Comment on lines +148 to +151
IntOffset(
x = clampedOffsetX.toInt(),
y = 0
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you check code styling once? Diff shows whitespaces being added here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@siddh1004 siddh1004 requested a review from msasikanth January 23, 2025 10:51
is StatinPrescriptionCheckInfoLoaded -> statinPrescriptionCheckInfoLoaded(event, model)
is CVDRiskCalculated -> dispatch(LoadStatinInfo(model.patientUuid))
is CVDRiskCalculated -> saveOrUpdateCVDRisk(event, model)
is CVDRiskUpdated -> dispatch(LoadStatinInfo(model.patientUuid))
Copy link
Contributor

Choose a reason for hiding this comment

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

This was the reason why I thought we can use a observable stream for CVD risk, since that will load the statin info automatically without having to call it manually everytime it's updated or saved.

You can make this change later though

@github-actions github-actions bot merged commit 6279eff into master Jan 23, 2025
8 of 9 checks passed
@github-actions github-actions bot deleted the siddharth-agarwal/sc-14495 branch January 23, 2025 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants