6650 rename requisition line transformation and expose plugin data query to plugins #6684

andreievg · 2025-02-25T11:58:23Z

👩🏻‍💻 What does this PR do?

Sorry I did a bit more then specified by the issue.

Renamed transform_requisition_line to transform_request_requisition_line, while renaming I decided to use PluginType as source of truth, this also included updating types in the backend plugin package.json (see example branch mentioned in testing)

Added get plugin data method that is exposed now to plugins

Exposed request edit line in front end to also have columns shown

I found a bug where plugin data is set for wrong id, because id is overwritten after generating requisition line (since we are using id generated on front end), this was fixed by providing item_id -> id mapping to generate requisition line

💌 Any notes for the reviewer?

Plugin data filter is exposed to plugin, we starting to expose types that are deep in our code, which is very convenient but would require testing strategy, which will be implemented for CIV plugins.

🧪 Testing

Use this branch of examples: msupply-foundation/open-msupply-plugins#11

Create internal order, see aggregate AMC value appearing in table and edit lines view. Delete a line with 0 aggregate amc. Create manual response requisition with that item, set amc. Insert that one line again in internal order, see aggregate amc change having value. Aggregate AMC should be sum of all of the latest response requisition line AMC for that item in that store

…lugin

andreievg · 2025-02-25T11:58:55Z

client/packages/plugins/README.md

@@ -173,7 +173,7 @@ You can work on plugins as if they were part of the app (types should be shared,

 ```bash
 # From server directory
-cargo run --bin remote_server_cli -- generate-plugin-bundle -i ../client/packages/plugins/mynewplugin/frontend -o pluginbundle.json
+cargo run --bin remote_server_cli -- generate-plugin-bundle -i ../client/packages/plugins/myPluginBundle/frontend -o pluginbundle.json


decided to change this since submodule example in readme uses myPluginBundle as plugin name

andreievg · 2025-02-25T11:59:45Z

.../packages/requisitions/src/RequestRequisition/DetailView/RequestLineEdit/RequestLineEdit.tsx

@@ -83,6 +85,9 @@ export const RequestLineEdit = ({
    ?.sort((a, b) => a.name.name.localeCompare(b.name.name))
    .sort((a, b) => b.amcInUnits - a.amcInUnits)
    .sort((a, b) => b.stockInUnits - a.stockInUnits);
+
+  const line = lines.find(line => line.id === draft?.id);


For fragment to be available to editViewColumns plugin interface

andreievg · 2025-02-25T12:00:14Z

server/repository/src/db_diesel/backend_plugin_row.rs

 pub enum PluginType {
-    Amc,
-    TransformRequisitionLines,
+    AverageMonthlyConsumption,


This was not necessary, but not a huge change, sorry for the extra diff

andreievg · 2025-02-25T12:00:52Z

server/repository/src/db_diesel/backend_plugin_row.rs

@@ -8,10 +8,10 @@ use diesel_derive_enum::DbEnum;
 use serde::{Deserialize, Serialize};

 #[derive(Clone, Eq, PartialEq, Debug, Serialize, Deserialize)]
-#[serde(rename_all = "SCREAMING_SNAKE_CASE")]
+#[serde(rename_all = "snake_case")]


snake case now because this maps directly to main plugins type now, made comments in types/mod.rs

andreievg · 2025-02-25T12:01:21Z

server/repository/src/db_diesel/filter_sort_pagination.rs

 pub struct EqualFilter<T>
 where
    T: 'static,
 {
+    #[ts(optional)]


Without optional Option is treated as null, with optional treated as undefined (with former have to list all fields ..)

andreievg · 2025-02-25T12:02:33Z

server/service/src/backend_plugin/boajs/call_plugin.rs

 pub(crate) fn call_plugin<I, O>(
    input: I,
-    name: &str,
+    r#type: &PluginType,


I like this change even though it's a little bit out of scope, it avoids having to hard code string and makes PluginType source of truth

github-actions · 2025-02-25T12:03:50Z

Bundle size difference

Comparing this PR to main

Old size	New size	Diff
5.95 MB	5.95 MB	253 B (0.00%)

andreievg · 2025-02-25T12:04:07Z

server/service/src/backend_plugin/boajs/utils.rs

-        let Some(arg) = args.get(index) else {
-            return Err(Error::NoArgumentAtIndex(index));
-        };
+        let arg = args.get(index).ok_or(Error::NoArgumentAtIndex(index))?;


sorry again refactor, i added new method below which uses ok_or so thought to keep consistent, subjectively no difference in readability ¯_(ツ)_/¯

andreievg · 2025-02-25T12:04:46Z

server/service/src/backend_plugin/boajs/utils.rs

+    SerdeError(#[from] serde_json::Error),
+}
+
+pub(super) fn get_serde_argument<D: DeserializeOwned>(


this is used to get of plugin data filter

andreievg · 2025-02-25T12:05:25Z

server/service/src/backend_plugin/types/amc.rs

@@ -30,6 +31,10 @@ pub trait Trait: Send + Sync {
 // TODO as macro ? Can do types here too
 impl self::Trait for PluginInstance {
    fn call(&self, input: Input) -> PluginResult<Output> {
-        Ok(call_plugin(input, "average_monthly_consumption", self)?)


This shows why PluginType is used now, rather then a string

andreievg · 2025-02-25T12:06:22Z

server/service/src/requisition/request_requisition/add_from_master_list.rs

@@ -127,6 +129,7 @@ fn generate(
        store_id,
        &requisition_row,
        items_ids_not_in_requisition,
+        HashMap::new(),


this is the change for item to new line id mapping, see generate_requisition_lines

lache-melvin · 2025-02-26T03:58:44Z

server/repository/src/db_diesel/backend_plugin_row.rs

-        let types = PluginTypes(vec![PluginType::Amc, PluginType::Amc]);
+        let types = PluginTypes(vec![
+            PluginType::AverageMonthlyConsumption,
+            PluginType::AverageMonthlyConsumption,


nit, but should we use the TransformRequestRequisitionLines here now?

lache-melvin · 2025-02-26T04:01:40Z

server/service/src/backend_plugin/boajs/call_plugin.rs


-    let key = js_string!(name);
+    let key = js_string!(r#type.as_str());


How come this one not using plugin_type_to_string?

lache-melvin · 2025-02-26T04:02:12Z

server/service/src/backend_plugin/boajs/methods/get_plugin_data.rs

+        JsString::from("get_plugin_data"),
+        0,
+        NativeFunction::from_copy_closure(move |_, args, mut ctx| {
+            // TODO Is this actually safe ? (need to check reference counts after plugin has run)


Would like to know about this before we merge..?

lache-melvin · 2025-02-26T04:06:47Z

server/service/src/backend_plugin/boajs/methods/get_plugin_data.rs

+
+            // TODO pagination or restrictions ?
+            let plugin_data: Vec<PluginDataRow> = PluginDataRepository::new(&connection)
+                .query_by_filter(filter)


Does feel pretty loose, had same thought with transform_lines as plugin... would like to hear more about how much we intend to trust the plugins?

lache-melvin · 2025-02-26T04:19:07Z

server/service/src/requisition/request_requisition/generate.rs

@@ -46,6 +49,8 @@ pub fn generate_requisition_lines(
    store_id: &str,
    requisition_row: &RequisitionRow,
    item_ids: Vec<String>,
+    // Need for plugins since sometimes we set id passed in as parameter in graphql


Well, the insert_request_requisition_line that does this is only used from batch mutation, which is not used by our frontend? Do you know if there is an integration that we need to support here, or should we remove the batch mutation and then wouldn't need to do this?

I TAKE IT BACK IM BAD AT SEARCHING IT IS INDEED USED

lache-melvin · 2025-02-26T04:22:47Z

Looking good, would like to know about the todos you left, will test properly in the morning 🚀

lache-melvin

Requesting changes at least until hear from you what we do now vs added to carryover:

Currently transform_request_requisition_lines is not called by update_request_requisition_line but it is by update_request_requisition - should be added to line update too I think
- If plugin interface/capability is as generic as "transform" then I think should be called in all cases
- As discussed, could pass an ActionType enum into plugin, for plugin to determine if runs on all calls, only create etc.
- Other places currently not using plugin that probably should in this case: R&R form, processors, fine grained calls like use_suggested_quantity (+ maybe more?? see below, I'm worried about this, how do we ensure we don't miss any?)
Backend plugin helper methods have: // TODO Is this actually safe ? (need to check reference counts after plugin has run) ... can we remove this if you are happy it is safe?
Back and frontend plugins should have same code in package.json
- I wonder if common frontend should expose usePluginData hook rather than implementing for each plugin
  - This can extract pluginCode from the plugin itself?
- Query and mutate should only be able to access plugin data for that plugin code
  - restrict get_plugin_data
  - Maybe call_plugin method supports returning of plugin_data rows, can then get processed and plugin code applied, before returned to service call for saving? Though probs requires a second kind of call_plugin_with_data_output or something....
For thought - best practices to ensure plugin calls aren't missed in future core dev? Should live in mod file, as generic place as possible... are there better patterns to support the plugin architecture?

FYI at one point you mentioned update would be creating duplicate plugin data, but looks like backend plugin that isn't happening anymore. I wonder about frontend though, whether we should expose upsert plugin data mutation rather than 2 separately

lache-melvin · 2025-02-27T00:19:16Z

.../packages/requisitions/src/RequestRequisition/DetailView/RequestLineEdit/RequestLineEdit.tsx

@@ -213,6 +218,10 @@ export const RequestLineEdit = ({
                label={t('label.amc')}
                sx={{ marginBottom: 1 }}
              />
+              {line &&
+                plugins.requestRequisitionColumn?.editViewColumns?.map(
+                  (Column, index) => <Column key={index} line={line} />


I think we should call this Field, and editViewFields ? Column has me thinking I should be defining a column definition with cell/accessor etc..

andreievg added 3 commits February 26, 2025 00:43

Rename requisition transform plugin and expose plugin data query to p…

5fb1eec

…lugin

Expose request line edit for front end column plugin

0a2f461

Revert central server hack

57a6a5f

github-actions bot added this to the v2.6.0-RC3 milestone Feb 25, 2025

github-actions bot added enhancement New feature or request Team Ruru 🦉 Roxy, Ferg, Noel feature: plugin labels Feb 25, 2025

andreievg commented Feb 25, 2025

View reviewed changes

lache-melvin self-assigned this Feb 26, 2025

lache-melvin reviewed Feb 26, 2025

View reviewed changes

lache-melvin requested changes Feb 26, 2025

View reviewed changes

lache-melvin reviewed Feb 27, 2025

View reviewed changes

Use ts chrono impl feature

5059486

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

6650 rename requisition line transformation and expose plugin data query to plugins #6684

6650 rename requisition line transformation and expose plugin data query to plugins #6684

andreievg commented Feb 25, 2025

andreievg Feb 25, 2025

andreievg Feb 25, 2025

andreievg Feb 25, 2025

andreievg Feb 25, 2025

andreievg Feb 25, 2025 •

edited

Loading

andreievg Feb 25, 2025

github-actions bot commented Feb 25, 2025

andreievg Feb 25, 2025

andreievg Feb 25, 2025

andreievg Feb 25, 2025

andreievg Feb 25, 2025

lache-melvin Feb 26, 2025

lache-melvin Feb 26, 2025

lache-melvin Feb 26, 2025

lache-melvin Feb 26, 2025

lache-melvin Feb 26, 2025

lache-melvin Feb 26, 2025

lache-melvin commented Feb 26, 2025

lache-melvin left a comment •

edited

Loading

lache-melvin Feb 27, 2025


		let key = js_string!(name);
		let key = js_string!(r#type.as_str());

6650 rename requisition line transformation and expose plugin data query to plugins #6684

Are you sure you want to change the base?

6650 rename requisition line transformation and expose plugin data query to plugins #6684

Conversation

andreievg commented Feb 25, 2025

👩🏻‍💻 What does this PR do?

💌 Any notes for the reviewer?

🧪 Testing

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 Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 25, 2025

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

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

lache-melvin commented Feb 26, 2025

lache-melvin left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreievg Feb 25, 2025 •

edited

Loading

lache-melvin left a comment •

edited

Loading