1955 temperature sensor schema for temperature logs and breaches

[adrianwb]

Part of #1955

👩🏻‍💻 What does this PR do?

Diesel schemas added for remaining temperature sensor tables from mSupply - temperature_log, temperature_breach and temperature_breach_config

🧪 How has/should this change been tested?

Not sure how this can be tested as a standalone, without being integrated with the other related areas still to be done as part of #1955?

💌 Any notes for the reviewer?

For now, mostly a sanity check that I haven't done anything stupid! I've tried to share the enum breach type with the config, but there may be a better way to doing it? Also please check that the "type" field is handled correctly.

📃 Documentation

No user facing changes

[andreievg]

Looks great, thanks.

Just as per earlier, I think it's good idea to only add base things that are used, although I think it's a great idea to add both types of repositories, but probably with minimum filters and sorts (just by id).

For now, mostly a sanity check that I haven't done anything stupid!

Not at all, it all looks great thank you.

Not sure how this can be tested as a standalone

Some time we just mention that test in future PR's will cover it, or do unit tests and suggest running cargo test. For a unit test, once schema mutation is added, can do just one upsert and one query (to make sure schema in DB matches diesel), can also do enum test, here and here. Btw these tests may seem overkill, but if they are kept small and fast enough to write, they can help validate that things work without going too far ahead, and can help when adding new functionality (like a more complex query filter in the future, name joins, name is visible filter and test).

I was wondering about a couple of things:

If sensor is change to another location, even though we don't record sensor location movement, we do store location_id on temperature log, which should be enough I think.

For consequitive breach calculations, if breach is ongoing since the last time we 'synced' temperature from sensor, we should be able to resume that calculation right ? By looking at temperature previous temperature record timestamp and breach 'to' timestamp ? (just wondering since we dont' store breach on temperature_log now, which I think is a good idea)

[andreievg]

Technically, ColdCumulative will never be saved in TemperatureBreach table, I think it's a good idea to make a comment somewhere (either here or a smart comment in schema). Btw, i am not sure if we show cumulative in mobile but in cold chain app it's based on the display windows of temperature charts.

I've tried to share the enum breach type with the config, but there may be a better way to doing it?

I think it's great thank you (would assume it would also be a shared type in postgres schema)

[adrianwb]

Not sure that I understand why ColdCumulative would never be saved? For some types of FridgeTags, the sensor itself only generates cumulative breaches, and the only way to generate consecutive breaches is to calculate them... mSupply desktop shows both.

[andreievg]

Just with the Defaults, we started using them due to test cases, had a lot of test data that needed to be updated if new field was added, but if we use default for all fields apart from the ones being tested, it makes it much easier. Anyways the auto derive Default works if all of struct fields have default implementation, in this case the enum doesn't but we recently discovered `#[default]`` annotation which makes it slightly easier (btw there are other types where we have to manually implement Default, because field type doesn't implement Default and it's foreign crate type, for which we can't manually implement Default, like NaiveDate), anyways, could do this:

[andreievg]

1955-temperature-sensor-schema-for-temperature-logs-and-breaches...1955-temperature-sensor-schema-for-temperature-logs-and-breaches(Derive-default-example)

[adrianwb]

Nice! I've done that for all of them: 8bc9323

[adrianwb]

I was wondering about a couple of things:

If sensor is change to another location, even though we don't record sensor location movement, we do store location_id on temperature log, which should be enough I think.

The way it seems to work in desktop mSupply at the moment is that if you assign a new location to a sensor which doesn't already have one, then it goes through all the temperature breaches and logs linked to that sensor, and sets their location as well. I haven't checked to see what actually happens if you then change the location again to a different one, but I'd expect that it wouldn't touch the existing breach/log records, but that any new ones after that would get assigned to the new location => as I understand it, that should to be enough.

For consecutive breach calculations, if breach is ongoing since the last time we 'synced' temperature from sensor, we should be able to resume that calculation right? By looking at temperature previous temperature record timestamp and breach 'to' timestamp ? (just wondering since we dont' store breach on temperature_log now, which I think is a good idea)

Yes, that's the way the logic works on desktop - if the last import ended on a breach, it "rewinds" to the start of that breach and starts processing the temperature logs from there. That logic hasn't yet been implemented in the temperature sensor crate - so far it's just processing the raw data from the sensor - but at some point in the not too distant future, I'd hope to add it in. I think the most flexible way to do that is to pass in a breach config and it would process the temperature logs (between a start/end timestamp) and return all the breaches that match.

[adamdewey]

sorry, just chiming in:

The way it seems to work in desktop mSupply at the moment is that if you assign a new location to a sensor which doesn't already have one, then it goes through all the temperature breaches and logs linked to that sensor, and sets their location as well. I haven't checked to see what actually happens if you then change the location again to a different one, but I'd expect that it wouldn't touch the existing breach/log records, but that any new ones after that would get assigned to the new location => as I understand it, that should to be enough.

Just thinking ahead to when we want to be able to track the temperature history through the lifecycle of a stock line we would need to be able to track breaches against either the stock line directly, or against the correct location when the breach occurred (so the stock history could be derived by looking at its locations histories) - couldn't quite get my head around whether we'd be satisfying that here but just wanted to flag it.

Yes, that's the way the logic works on desktop - if the last import ended on a breach, it "rewinds" to the start of that breach and starts processing the temperature logs from there. That logic hasn't yet been implemented in the temperature sensor crate - so far it's just processing the raw data from the sensor - but at some point in the not too distant future, I'd hope to add it in. I think the most flexible way to do that is to pass in a breach config and it would process the temperature logs (between a start/end timestamp) and return all the breaches that match.

Just wanted to understand how we're recording breaches that are passed to us from the devices vs breaches that we calculate ourselves - will we have a column for each in the database against each temperature log?

[adrianwb]

Just thinking ahead to when we want to be able to track the temperature history through the lifecycle of a stock line we would need to be able to track breaches against either the stock line directly, or against the correct location when the breach occurred (so the stock history could be derived by looking at its locations histories) - couldn't quite get my head around whether we'd be satisfying that here but just wanted to flag it.

Each breach and temperature log will be associated with a location, so we should be able to work out which stock was at a breached location by looking at the breach timings and the stock location history

Just wanted to understand how we're recording breaches that are passed to us from the devices vs breaches that we calculate ourselves - will we have a column for each in the database against each temperature log?

We can easily add a [temperature_breach]calculated field (or similar), so that we can differentiate between device and calculated breaches. There's no direct link between temperature logs and breaches though, as a single temperature log could potentially be part of multiple breaches and that would need a join table in the DB which complicates things a lot - much easier just to query temperature logs based on the breach parameters (between the breach start/end time and outside the max/min threshold temperatures, depending on the breach type).

[adamdewey]

Each breach and temperature log will be associated with a location, so we should be able to work out which stock was at a breached location by looking at the breach timings and the stock location history

Perfect!

We can easily add a [temperature_breach]calculated field (or similar), so that we can differentiate between device and calculated breaches.

I think that would be really helpful to save our future selves getting confused when troubleshooting and also could potentially be valuable to have that extra set of data for comparison purposes.

There's no direct link between temperature logs and breaches though, as a single temperature log could potentially be part of multiple breaches and that would need a join table in the DB which complicates things a lot - much easier just to query temperature logs based on the breach parameters (between the breach start/end time and outside the max/min threshold temperatures, depending on the breach type).

Ah I see, yes that makes sense - the main thing is recording the logs and breaches against a location anyhow 👍