1955 temperature sensor sync for temperature logs and breaches #2152

adrianwb · 2023-08-29T16:51:49Z

Fixes part of #1955

👩🏻‍💻 What does this PR do?

Add in sync for temperature_log, temperature_breach and temperature_breach_config

🧪 How has/should this change been tested?

Just the automated tests, like any other sync table

💌 Any notes for the reviewer?

📃 Documentation

No user facing changes

…pe functions from temperature_breach module public so that they can also be used here

adrianwb · 2023-08-29T16:59:40Z

It's all compiling up ok but cargo test sync_pull is failing (everything else passes):

running 1 test
test sync::test::pull_and_push::test_sync_pull_and_push ... FAILED

failures:

---- sync::test::pull_and_push::test_sync_pull_and_push stdout ----
thread 'sync::test::pull_and_push::test_sync_pull_and_push' panicked at 'TemperatureBreachConfig row not found: 997812e0c33911eb9757779d39ae2dbd', service\src\sync\test\mod.rs:187:17
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

and I can't figure out why this isn't working when the very similar TemperatureBreach & TemperatureLog added at the same time both seem to be passing - I've looked at the test_sync_pull_and_push.sqlite DB in the open-msupply\server\service\test_output folder and there is indeed no temperature_breach_config record => I'm guessing there's something which prevents it being created in the first place??

Obviously it's not ready to merge yet, until I figure this out, so I'll fill in the PR description properly at that point...

andreievg

Thanks looks great, one change request though (but approving anyways, i'll be away till next tuesday, so good to get as much as possible in before you go on holiday, btw I would like to work on some things while you away if that's ok, so if you have any carry over things, can you please put in branch)

It's all compiling up ok but cargo test sync_pull is failing (everything else passes):

It's due to this one:

diff --git a/server/service/src/sync/translations/mod.rs b/server/service/src/sync/translations/mod.rs
index e9342531c..a453da43f 100644
--- a/server/service/src/sync/translations/mod.rs
+++ b/server/service/src/sync/translations/mod.rs
@@ -80,6 +80,7 @@ pub(crate) fn all_translators() -> SyncTranslators {
         Box::new(sensor::SensorTranslation {}),
         Box::new(temperature_log::TemperatureLogTranslation {}),
         Box::new(temperature_breach::TemperatureBreachTranslation {}),
+        Box::new(temperature_breach_config::TemperatureBreachConfigTranslation {}),
         Box::new(clinician::ClinicianTranslation {}),
         Box::new(clinician_store_join::ClinicianStoreJoinTranslation {}),
         // Remote-Central (site specific)
diff --git a/server/service/src/sync/translations/temperature_breach_config.rs b/server/service/src/sync/translations/temperature_breach_config.rs
index 0f1830782..a7a850bc3 100644
--- a/server/service/src/sync/translations/temperature_breach_config.rs
+++ b/server/service/src/sync/translations/temperature_breach_config.rs
@@ -43,7 +43,7 @@ pub(crate) struct TemperatureBreachConfigTranslation {}
 impl SyncTranslation for TemperatureBreachConfigTranslation {
     fn pull_dependencies(&self) -> PullDependency {
         PullDependency {
-            table: LegacyTableName::TEMPERATURE_BREACH,
+            table: LEGACY_TABLE_NAME,
             dependencies: vec![LegacyTableName::STORE],
         }
     }

with CodeLLDB in vs code and rust analyzer, you can get this inlay hints, to debug and run tests or main etc, and breakpoints work.

I first tried to remove comment one this one and run the test (through the inlay hint, otherwise you would need --nocapture flag for cargo to see output of tests). That didn't show that translator was not found, so put breakpoint in translator, nothing hit, so put conditional breakpoint here:

sync_record.record_id == "997812e0c33911eb9757779d39ae2dbd"

nothing hit again, so realised that the records is not even being grabbed from sync_buffer.

This needs improving at some point, it's pretty easy to miss things, will look at improving at some point. I am very surprised that there are no compilation errors, since pub(crate) struct is not actually used anywhere but tests

andreievg · 2023-08-30T11:05:02Z

server/repository/src/migrations/v1_02_00/sensor.rs

+                start_timestamp {DATETIME} NOT NULL,
+                end_timestamp {DATETIME} NOT NULL,
+                acknowledged BOOLEAN,
+                threshold_minimum {DOUBLE} NOT NULL,


nice! thanks for using the common types {DOUBLE}, {DATETIME} etc.. looks great

andreievg · 2023-08-30T11:07:16Z

server/repository/src/migrations/v1_02_00/sensor.rs

+                CREATE TRIGGER temperature_breach_trigger
+                AFTER INSERT OR UPDATE OR DELETE ON temperature_breach
+                FOR EACH ROW EXECUTE PROCEDURE update_changelog();
+


kind of a personal preference, but can use the following macro in some places, instead of #[cfg] annotation

if cfg!(features = "postgres) { // this won't be greyed out by ide, and will do compilation check without features being on }

Done here: bf73a5c

andreievg · 2023-08-30T11:10:28Z

server/service/src/sync/translations/temperature_breach.rs

+}
+
+#[derive(Deserialize, Serialize, Debug)]
+pub enum LegacyTemperatureBreachType {


btw when all are renamed like this, can use rename_all: https://serde.rs/container-attrs.html#rename_all

#[serde(rename_all = "SCREAMING_SNAKE_CASE")]

Done here: bf73a5c

andreievg · 2023-08-30T12:14:53Z

We can do this to let compiler help:

diff --git a/server/service/src/sync/translations/mod.rs b/server/service/src/sync/translations/mod.rs
index e9342531c..89f35788c 100644
--- a/server/service/src/sync/translations/mod.rs
+++ b/server/service/src/sync/translations/mod.rs
@@ -81,6 +81,8 @@ pub(crate) fn all_translators() -> SyncTranslators {
         Box::new(temperature_log::TemperatureLogTranslation {}),
         Box::new(temperature_breach::TemperatureBreachTranslation {}),
         Box::new(clinician::ClinicianTranslation {}),
+        // if this is missing there will be compilation error
+        temperature_breach_config::boxed(),
         Box::new(clinician_store_join::ClinicianStoreJoinTranslation {}),
         // Remote-Central (site specific)
         Box::new(name_store_join::NameStoreJoinTranslation {}),
diff --git a/server/service/src/sync/translations/temperature_breach_config.rs b/server/service/src/sync/translations/temperature_breach_config.rs
index 0f1830782..25ac389d2 100644
--- a/server/service/src/sync/translations/temperature_breach_config.rs
+++ b/server/service/src/sync/translations/temperature_breach_config.rs
@@ -39,7 +39,13 @@ pub struct LegacyTemperatureBreachConfigRow {
     pub maximum_temperature: f64,
 }
 
-pub(crate) struct TemperatureBreachConfigTranslation {}
+// Needs to be added to all_translators()
+#[deny(dead_code)]
+pub(crate) fn boxed() -> Box<dyn SyncTranslation> {
+    Box::new(TemperatureBreachConfigTranslation {})
+}
+
+pub(super) struct TemperatureBreachConfigTranslation {}
 impl SyncTranslation for TemperatureBreachConfigTranslation {
     fn pull_dependencies(&self) -> PullDependency {
         PullDependency {

And the translation trait can be made more usable, the translator needs to implement a method that returns a table name (the translator trait has a default implementation to check that table, this can be overridden, but it means we don't need to do this ones, and also don't have to specify own table in dependency)

adrianwb · 2023-08-30T16:26:14Z

Thanks looks great, one change request though (but approving anyways, i'll be away till next tuesday, so good to get as much as possible in before you go on holiday, btw I would like to work on some things while you away if that's ok, so if you have any carry over things, can you please put in branch)

I didn't get as far as I'd hoped but by all means, please carry on and make whatever changes are needed. I'll commit any work in progress.

It's all compiling up ok but cargo test sync_pull is failing (everything else passes):

It's due to this one:

diff --git a/server/service/src/sync/translations/mod.rs b/server/service/src/sync/translations/mod.rs
index e9342531c..a453da43f 100644
--- a/server/service/src/sync/translations/mod.rs
+++ b/server/service/src/sync/translations/mod.rs
@@ -80,6 +80,7 @@ pub(crate) fn all_translators() -> SyncTranslators {
         Box::new(sensor::SensorTranslation {}),
         Box::new(temperature_log::TemperatureLogTranslation {}),
         Box::new(temperature_breach::TemperatureBreachTranslation {}),
+        Box::new(temperature_breach_config::TemperatureBreachConfigTranslation {}),
         Box::new(clinician::ClinicianTranslation {}),
         Box::new(clinician_store_join::ClinicianStoreJoinTranslation {}),
         // Remote-Central (site specific)
diff --git a/server/service/src/sync/translations/temperature_breach_config.rs b/server/service/src/sync/translations/temperature_breach_config.rs
index 0f1830782..a7a850bc3 100644
--- a/server/service/src/sync/translations/temperature_breach_config.rs
+++ b/server/service/src/sync/translations/temperature_breach_config.rs
@@ -43,7 +43,7 @@ pub(crate) struct TemperatureBreachConfigTranslation {}
 impl SyncTranslation for TemperatureBreachConfigTranslation {
     fn pull_dependencies(&self) -> PullDependency {
         PullDependency {
-            table: LegacyTableName::TEMPERATURE_BREACH,
+            table: LEGACY_TABLE_NAME,
             dependencies: vec![LegacyTableName::STORE],
         }
     }

Thanks - it's obvious now that you've pointed it out! Fixed here: bf73a5c

with CodeLLDB in vs code and rust analyzer, you can get this inlay hints, to debug and run tests or main etc, and breakpoints work.

I first tried to remove comment one this one and run the test (through the inlay hint, otherwise you would need --nocapture flag for cargo to see output of tests). That didn't show that translator was not found, so put breakpoint in translator, nothing hit, so put conditional breakpoint here:
sync_record.record_id == "997812e0c33911eb9757779d39ae2dbd"
nothing hit again, so realised that the records is not even being grabbed from sync_buffer.

This needs improving at some point, it's pretty easy to miss things, will look at improving at some point. I am very surprised that there are no compilation errors, since pub(crate) struct is not actually used anywhere but tests

Thanks for all that too - still haven't used the debugger much so that's helpful advice :-)

adrianwb · 2023-08-30T16:44:38Z

@andreievg I just added the mock data in here as well as I've run out of time and that's quicker than creating a whole new branch for it. Over to you :-)

To be exact, I think the plural of "breach" is "breaches", not "breachs" but I'm ok to let that slide as it might just confuse things!

andreievg · 2023-09-07T04:36:12Z

Thanks @adrianwb, re-tested (with sqlite) with current state of the branch, and merging

adrianwb added 7 commits August 28, 2023 13:15

Add temperature_log to sync

f49f13f

Add temperature_breach to sync

b98a658

Add missing temperature_breach.acknowledged field to SQL for migrations

95c018e

Oops - forgot to add this earlier!

5c74a8f

Add sync for temperature_breach_config, make to/from legacy breach ty…

0000f6d

…pe functions from temperature_breach module public so that they can also be used here

After cargo fmt

09d41b8

Add temperature log/breach/config tests

bf2b501

adrianwb requested a review from andreievg August 29, 2023 16:59

andreievg approved these changes Aug 30, 2023

View reviewed changes

Some(changes) as suggested by Andrei + ran cargo fmt

bf73a5c

Added mock data for temperature logs, breaches and configs

678cbd7

andreievg merged commit c68f95f into feature/cold-chain-temperature-sensors Sep 7, 2023

andreievg deleted the 1955-temperature-sensor-sync-for-temperature-logs-and-breaches branch September 7, 2023 04:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1955 temperature sensor sync for temperature logs and breaches #2152

1955 temperature sensor sync for temperature logs and breaches #2152

adrianwb commented Aug 29, 2023 •

edited

Loading

adrianwb commented Aug 29, 2023 •

edited

Loading

andreievg left a comment

andreievg Aug 30, 2023

andreievg Aug 30, 2023

adrianwb Aug 30, 2023

andreievg Aug 30, 2023

adrianwb Aug 30, 2023

andreievg commented Aug 30, 2023

adrianwb commented Aug 30, 2023

adrianwb commented Aug 30, 2023 •

edited

Loading

andreievg commented Sep 7, 2023

1955 temperature sensor sync for temperature logs and breaches #2152

1955 temperature sensor sync for temperature logs and breaches #2152

Conversation

adrianwb commented Aug 29, 2023 • edited Loading

👩🏻‍💻 What does this PR do?

🧪 How has/should this change been tested?

💌 Any notes for the reviewer?

📃 Documentation

adrianwb commented Aug 29, 2023 • edited Loading

andreievg left a comment

Choose a reason for hiding this comment

andreievg Aug 30, 2023

Choose a reason for hiding this comment

andreievg Aug 30, 2023

Choose a reason for hiding this comment

adrianwb Aug 30, 2023

Choose a reason for hiding this comment

andreievg Aug 30, 2023

Choose a reason for hiding this comment

adrianwb Aug 30, 2023

Choose a reason for hiding this comment

andreievg commented Aug 30, 2023

adrianwb commented Aug 30, 2023

adrianwb commented Aug 30, 2023 • edited Loading

andreievg commented Sep 7, 2023

adrianwb commented Aug 29, 2023 •

edited

Loading

adrianwb commented Aug 29, 2023 •

edited

Loading

adrianwb commented Aug 30, 2023 •

edited

Loading