165 cold chain notification configuration should show the real sensors #169

jmbrunskill · 2023-10-03T04:14:33Z

Closes #165

👩🏻‍💻 What does this PR do?

🧪 How has/should this change been tested?

This needs an mSupply postgres (AKA Dashboard) database setup.
Ideally with coldchain sensors configured, although might be interesting to figure how/what we should display if there are no sensor...

💌 Any notes for the reviewer?

I'm not sure if the SQL Query in the front is a good, longer term plan.
I think the longer term plan (plugin queries perhaps?) should go in a seperate issue with design, and this might need to be merged for expediency, unless another quick to implement option is thought of.

github-actions · 2023-10-03T04:18:52Z

Bundle size difference

Comparing this PR to main

Old size	New size	Diff
2.1 MB	2.1 MB	754 B (0.03%)

backend/coldchain/src/alerts.rs

jmbrunskill · 2023-10-03T05:05:38Z

backend/coldchain/src/lib.rs

-Temperature Alerts will look something like this...
-----------------------
-High temperature alert!
+pub struct ColdChainPlugin {}


Making cold chain a plugin isn't really required for this PR, as it's currently using an SQL query in the frontend to retrieve data. I think it should somehow query the plugin instead, but leaving that for a separate PR & Issue.
Figured it doesn't hurt to keep this change in this PR.

jmbrunskill · 2023-10-03T05:08:42Z

frontend/packages/system/src/Notifications/Pages/ColdChain/SensorSelectionModal.tsx

+  const onSubmit = async () => {
+    if (selectedIds.length === 0) {
+      setErrorMessage(t('messages.nothing-selected'));
+      //   return;


I'm not sure what we want here. Should someone be able to select no sensors, I can see why it could be useful when your playing around, but also see the arguement for requiring one...

jmbrunskill · 2023-10-03T05:09:47Z

frontend/packages/system/src/Notifications/Pages/ColdChain/SensorSelector.tsx

+      width: 150,
+      sortable: true,
+      accessor: ({ rowData }) => {
+        return sensorDisplayName(rowData);


Should we render the store, location and sensor separately rather than display name?

yes I would say so...

jmbrunskill · 2023-10-03T05:10:37Z

frontend/packages/system/src/Notifications/Pages/ColdChain/SensorSelector.tsx

+        onClose={onClose}
+        setSelection={setSelection}
+      />
+      <IconButton


Don't love this edit button, but keen for ideas how we could do it better, perhaps some examples from another app?

jmbrunskill · 2023-10-03T05:11:34Z

frontend/packages/system/src/Notifications/api/hooks/useColdChainSensors.ts

+      "SELECT sn.id as id, s.name as store_name,coalesce(l.description, '') as location_name, sn.name as sensor_name FROM SENSOR sn JOIN store s ON sn.storeid = s.id LEFT JOIN location l on sn.locationid = l.id WHERE sn.is_active = true ORDER BY 2,3,4 LIMIT 1000";
+    const response = await sdk.getColdChainSensors({
+      sqlQuery: sensorQuery,
+      params: '{}',


Figured if we stuck with this approach, we could pass parameters to say search if we end up with too many sensors in a big installation...

…onfiguration-should-show-the-real-locationsstores

jmbrunskill · 2023-10-03T21:03:01Z

Sorry I merged the windows service PR here, but shouldn't have. Please ignore commit 9d11fc3

…ould-show-the-real-locationsstores

clemens-msupply

Regarding, your query question. As it looks like it the backend is kept minimalistic and just provides the core functionality(?). I think then it makes sense to move the logic to the frontend (into frontend plugins I guess).

Do we need backend plugins at all? If you do, it would make more sense to provide the sensor list through the backend though(?) But what would be the generic interface to communicate between frontend and backend plugins? the frontend plugin sends a json blob to its backend plugin?

Or you make cold chain sensor a build in feature and provide proper graphql queries but as you said this would be extra work.

One thing I noticed: send_high_temperature_alert_telegram could be moved into the test, right?

clemens-msupply · 2023-10-10T23:48:26Z

frontend/packages/common/src/intl/locales/en/system.json

@@ -86,5 +86,6 @@
  "helper-text.sql-query": "Your sql query can contain parameters, which are created using double curly braces within the query. For example: SELECT * FROM my_table WHERE id = {{param1}} AND name = {{param2}}",
  "label.test-sql-query": "Test SQL Query",
  "tooltip.manage-recipient-list": "Manage Recipient List",
-  "message.no-parameters": "No parameters found"
+  "message.no-parameters": "No parameters found",
+  "message.no-sensors-selected": "No sensors selected"


Nit: do you need a distinction between common.json and system.json?

It's pretty messy right now probably should create a coldchain one at least...

frontend/packages/system/src/Notifications/api/hooks/useColdChainSensors.ts

frontend/packages/system/src/Notifications/api/operations.graphql

clemens-msupply · 2023-10-11T00:17:08Z

frontend/packages/system/src/Notifications/Pages/ColdChain/SensorSelector.tsx

+      width: 150,
+      sortable: true,
+      accessor: ({ rowData }) => {
+        return sensorDisplayName(rowData);


yes I would say so...

frontend/packages/system/src/Notifications/Pages/ColdChain/CCNotificationEditForm.tsx

jmbrunskill · 2023-10-11T03:19:38Z

One thing I noticed: send_high_temperature_alert_telegram could be moved into the test, right?

This changes quite a lot in the next PR. will leave that for review there...

jmbrunskill · 2023-10-11T03:22:58Z

Do we need backend plugins at all? If you do, it would make more sense to provide the sensor list through the backend though(?) But what would be the generic interface to communicate between frontend and backend plugins? the frontend plugin sends a json blob to its backend plugin?

Yes, well we need them in that we want to support coldchain and scheduled reports and potentially other things in the future. Right now I'm just trying to implement them in their own crates until they are properly plugins. Maybe it is ok how it is though?

The JSON blob idea isn't crazy, hadn't thought of that. Something like pluginDataRequest(plugin_name: String, requestBody: String)?

I think I'll leave it for this PR though...

jmbrunskill added 2 commits October 3, 2023 15:40

Make coldchain a plugin

58b7a74

Configure sensor_ids for Coldchain

08a84fc

jmbrunskill linked an issue Oct 3, 2023 that may be closed by this pull request

Cold Chain Notification Configuration should show the 'real' locations/stores #165

Closed

4 tasks

Modal for sensor selection

fbc7b4c

jmbrunskill changed the title ~~165 cold chain notification configuration should show the real locationsstores~~ 165 cold chain notification configuration should show the real sensors Oct 3, 2023

jmbrunskill commented Oct 3, 2023

View reviewed changes

backend/coldchain/src/alerts.rs Show resolved Hide resolved

jmbrunskill commented Oct 3, 2023

View reviewed changes

jmbrunskill marked this pull request as ready for review October 3, 2023 05:11

jmbrunskill mentioned this pull request Oct 3, 2023

#111 Cold chain: Add section titles in configuration modal #168

Merged

2 tasks

Merge branch '161-windows-service' into 165-cold-chain-notification-c…

9d11fc3

…onfiguration-should-show-the-real-locationsstores

Merge branch 'main' into 165-cold-chain-notification-configuration-sh…

f5053e9

…ould-show-the-real-locationsstores

clemens-msupply self-requested a review October 10, 2023 21:24

clemens-msupply approved these changes Oct 11, 2023

View reviewed changes

jmbrunskill added 2 commits October 11, 2023 16:17

Rename runSqlQuery graphql request

8b5e154

Clean up debug output

c9c12eb

Show table for coldchain sensors

97dadd5

jmbrunskill merged commit 60bdd14 into main Oct 11, 2023

jmbrunskill deleted the 165-cold-chain-notification-configuration-should-show-the-real-locationsstores branch October 11, 2023 03:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

165 cold chain notification configuration should show the real sensors #169

165 cold chain notification configuration should show the real sensors #169

jmbrunskill commented Oct 3, 2023 •

edited

Loading

github-actions bot commented Oct 3, 2023

jmbrunskill Oct 3, 2023

jmbrunskill Oct 3, 2023

jmbrunskill Oct 3, 2023

clemens-msupply Oct 11, 2023

jmbrunskill Oct 3, 2023

jmbrunskill Oct 3, 2023 •

edited

Loading

jmbrunskill commented Oct 3, 2023

clemens-msupply left a comment

clemens-msupply Oct 10, 2023

jmbrunskill Oct 11, 2023

jmbrunskill Oct 11, 2023

clemens-msupply Oct 11, 2023

jmbrunskill commented Oct 11, 2023

jmbrunskill commented Oct 11, 2023

165 cold chain notification configuration should show the real sensors #169

165 cold chain notification configuration should show the real sensors #169

Conversation

jmbrunskill commented Oct 3, 2023 • edited Loading

👩🏻‍💻 What does this PR do?

🧪 How has/should this change been tested?

💌 Any notes for the reviewer?

github-actions bot commented Oct 3, 2023

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

Choose a reason for hiding this comment

jmbrunskill Oct 3, 2023 • edited Loading

Choose a reason for hiding this comment

jmbrunskill commented Oct 3, 2023

clemens-msupply 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

jmbrunskill commented Oct 11, 2023

jmbrunskill commented Oct 11, 2023

jmbrunskill commented Oct 3, 2023 •

edited

Loading

jmbrunskill Oct 3, 2023 •

edited

Loading