Fix temperature breach filter #2589

mark-prins · 2023-12-05T23:49:25Z

👩🏻‍💻 What does this PR do?

Could not repro the stated issue. A related issue was raised in conversation - that the breaches aren't showing.

That's the part which I've addressed here - it turns out that the breach filter was a little over complicated and didn't function as required. The filter was looking for temperature_breach records.. the query was

SELECT "temperature_log"."id", "temperature_log"."temperature", "temperature_log"."sensor_id", "temperature_log"."location_id", "temperature_log"."store_id", "temperature_log"."datetime", "temperature_log"."temperature_breach_id" 
FROM "temperature_log" 
WHERE "temperature_log"."temperature_breach_id" IN 
(SELECT "temperature_breach"."id" FROM "temperature_breach" WHERE "temperature_breach"."id" = 'fff4bac7-2256-43cf-acb9-78acfc8cc7e0') 
ORDER BY "temperature_log"."datetime" DESC

which will not work for new breaches, the breach has to be added before the breach can be added!

I've changed the filter to look for an id only, which simplifies things and allows breaches to be added.

🧪 How has/should this change been tested?

Configure a CCA with a sensor and sync with omSupply. get the sensor into a breach and check if the breach shows in oms.

💌 Any notes for the reviewer?

📃 Documentation

No user facing changes

mark-prins · 2023-12-05T23:50:07Z

client/packages/coldchain/src/Monitoring/ListView/TemperatureChart/TemperatureChart.tsx

@@ -177,6 +177,7 @@ const Chart = ({
                      borderStyle: 'solid',
                      borderColor: theme.palette.gray.light,
                      padding: 3,
+                      textAlign: 'left',


Offroad: the legend was showing with names centred:

github-actions · 2023-12-05T23:53:28Z

Bundle size difference

Comparing this PR to main

Old size	New size	Diff
8.28 MB	8.28 MB	158 B (0.00%)

mark-prins · 2023-12-05T23:57:12Z

server/repository/src/db_diesel/temperature_log.rs

@@ -92,6 +90,9 @@ impl<'a> TemperatureLogRepository<'a> {
            query = query.order(temperature_log_dsl::datetime.desc())
        }

+        // Debug diesel query


just to help with debugging 🤷

mark-prins · 2023-12-06T00:19:54Z

server/server/src/cold_chain/sensor.rs

@@ -109,7 +109,7 @@ fn validate_sensor(sensor: &Sensor) -> Result<(), String> {
    }
    match sensor.log_delay {
        Some(log_delay) => {
-            if log_delay < 1 {
+            if log_delay < 0 {


the 'add a debug sensor' function creates a record with 0 here, which meant that the sensor mutation fails for all sensors. since positive number is 0 and up, I figured changing this test still fits the intention

I think technically positive number are greater then zero.

Ohh, what is the add a debug sensor function btw ?

in the cold chain app - under developer settings you can add a sensor, and then add random logs for it.

andreievg

@mark-prins.

I haven't tested, but I can't see the difference between the queries, also there is a comment about the breach filter.

The reason I can't see the difference, is there is a referencial contraint on logs, can't set temperature_breach_id, unless the breach exists, so I can't quite see what the difference is between the new filter and the old.

Btw does cold chain app push temperature_logs first or breaches first ? If it's the former then we might have an issue with referencial contraints.

Unforntunately I don't have the cold chain app set up, I wonder if we can share sqlite filter for it, or if there are some postman queries we can use to replicate/validate ?

andreievg · 2023-12-07T02:30:24Z

server/graphql/types/src/types/temperature_log.rs

@@ -42,7 +41,7 @@ pub struct TemperatureLogFilterInput {
    pub id: Option<EqualFilterStringInput>,
    pub sensor: Option<SensorFilterInput>,
    pub location: Option<LocationFilterInput>,
-    pub temperature_breach: Option<TemperatureBreachFilterInput>,
+    pub temperature_breach_id: Option<EqualFilterStringInput>,


There was/is a good reason to use this filter.

We can re-use existing repository functionality

Can filter temperature logs by all of the existing breach filter inputs (type of breach, acknowledge/unacknowledged etc..)

I think the breach type filter might be broken with this change ?

yes it does. I searched for any usages of the existing breach filter but didn't spot that..

andreievg · 2023-12-07T03:05:49Z

server/server/src/cold_chain/temperature_breach.rs

-                    TemperatureBreachFilter::new().id(EqualFilter::equal_to(&id)),
-                ),
-            ),
+            Some(TemperatureLogFilter::new().temperature_breach_id(EqualFilter::equal_to(&id))),


I forgot, why do we need to check that there are logs for the breach ?

that would be the simplest solution. we just remove the check.
The problem is that the breach doesn't record temperature and we were asked to show 'last temperature reading' in the notification. To do this, it's looking up the log - if the breach exists and the log doesn't then that display breaks. Easy enough to skip that data element - but we do then have strange data, with a breach that has no associated log.

mark-prins · 2023-12-07T04:53:52Z

Thanks Andrei for review.
I've reverted the filter changes, and removed the log check. Now the api will accept breaches which don't have a log in the database.

To remove any UI strangeness I've added a check in the notification component - if there's no min/max temperature then it just doesn't show. The temperature is also blank in the breach table.

mark-prins · 2023-12-07T04:58:58Z

Oh, and I'm using postgres, so don't have a sqlite file to share. You can use this postman collection to add a breach or log. I've been using CCA with some sensors that I'm shuttling in and out of the fridge to get it to breach which is a bit of a pain, but it did pick up this particular error that I'd missed earlier.

Cold Chain.postman_collection.json.zip

andreievg

Looks good, thanks for changes @mark-prins.

fix temperature breach filter

ebdfe06

mark-prins commented Dec 5, 2023

View reviewed changes

mark-prins commented Dec 6, 2023

View reviewed changes

mark-prins mentioned this pull request Dec 6, 2023

Breaches are not saved to open mSupply msupply-foundation/msupply-cold-chain#275

Closed

andreievg self-assigned this Dec 7, 2023

andreievg requested changes Dec 7, 2023

View reviewed changes

revert filter changes and remove log check

bdcd5cf

andreievg approved these changes Dec 7, 2023

View reviewed changes

mark-prins merged commit 80d4bfa into develop Dec 8, 2023
4 checks passed

mark-prins deleted the 2537-breach-not-showing branch December 8, 2023 00:00

jmbrunskill mentioned this pull request Jul 31, 2024

Add min/max for excursions #4479

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix temperature breach filter #2589

Fix temperature breach filter #2589

mark-prins commented Dec 5, 2023

mark-prins Dec 5, 2023

github-actions bot commented Dec 5, 2023 •

edited

Loading

mark-prins Dec 5, 2023

mark-prins Dec 6, 2023

andreievg Dec 7, 2023

mark-prins Dec 7, 2023

andreievg left a comment

andreievg Dec 7, 2023

andreievg Dec 7, 2023

mark-prins Dec 7, 2023

andreievg Dec 7, 2023

mark-prins Dec 7, 2023

mark-prins commented Dec 7, 2023

mark-prins commented Dec 7, 2023

andreievg left a comment

Fix temperature breach filter #2589

Fix temperature breach filter #2589

Conversation

mark-prins commented Dec 5, 2023

👩🏻‍💻 What does this PR do?

🧪 How has/should this change been tested?

💌 Any notes for the reviewer?

📃 Documentation

Choose a reason for hiding this comment

github-actions bot commented Dec 5, 2023 • edited Loading

Bundle size difference

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

andreievg 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

mark-prins commented Dec 7, 2023

mark-prins commented Dec 7, 2023

andreievg left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 5, 2023 •

edited

Loading