4791 add report versioning #5533

fergie-nz · 2024-11-22T21:04:25Z

👩🏻‍💻 What does this PR do?

Two parts:

Adds only upsert if the report to be upserted is a later version than the existing report of the same code (or if is_custom && is later version than an existing custom report)
Adds filter for only the latest version of any report which is compatible with the app version

There is a unique case which could casue issues where if a db has a report version later than the app version, you wouldn't be able to upload older report versions. But I don't think we need to worry about that.

💌 Any notes for the reviewer?

You can test this report with this database which includes a number of report versions and is_custom combinations:

central_test.sqlite.zip

🧪 Testing

Go to reports
See only latest versions displayed

📃 Documentation

No documentation required: no user facing changes or a bug fix which isn't a change in behaviour

github-actions · 2024-11-22T21:09:51Z

Bundle size difference

Comparing this PR to main

Old size	New size	Diff
4.98 MB	4.98 MB	0 B (0.00%)

andreievg · 2024-11-25T23:37:17Z

server/service/src/standard_reports.rs

+                    version: report.version,
+                    code: report.code,
+                })?;
+            };


Hmm not very happy with logic here, do you mind doing a self review of the changes and existing logic in this file and seeing how it can/should be simplified ?

Hi Andrei,

I was wondering about just upserting all reports regardless of if they were older versions than the one existing on the server.
Is this what you're thinking? Or are you thinking refactoring but keeping the current logic?

andreievg · 2024-11-25T23:39:08Z

client/packages/system/src/Report/api/hooks/useLatestReportList.ts

+  return 0;
+};
+
+export const useLatestReportList = ({


Front end should be report version agnostic, this should be back end logic

Also add unique is_custom identifier to id

fergie-nz · 2024-11-27T23:57:24Z

server/repository/src/db_diesel/report_row.rs

-    pub fn find_one_by_code_and_version(
-        &self,
-        code: &str,
-        version: &str,
-    ) -> Result<Option<ReportRow>, RepositoryError> {
-        let result = report_dsl::report
-            .filter(report_dsl::code.eq(code))
-            .filter(report_dsl::version.eq(version))
-            .first(self.connection.lock().connection())
-            .optional()?;
-        Ok(result)
-    }
-


Removing as not anymore

fergie-nz · 2024-11-27T23:57:54Z

server/repository/src/mock/reports.rs

+pub fn mock_report_g() -> ReportRow {
+    inline_init(|r: &mut ReportRow| {
+        r.id = "mock_report_g".to_string();
+        r.version = "2.3.0".to_string();
+        r.is_custom = true;
+        r.code = "report_with_custom_option".to_string();
+    })
+}
+
+pub fn mock_report_h() -> ReportRow {
+    inline_init(|r: &mut ReportRow| {
+        r.id = "mock_report_h".to_string();
+        r.version = "2.3.1".to_string();
+        r.is_custom = false;
+        r.code = "report_with_custom_option".to_string();
+    })
+}
+
+pub fn mock_report_i() -> ReportRow {
+    inline_init(|r: &mut ReportRow| {
+        r.id = "mock_report_i".to_string();
+        r.version = "2.8.2".to_string();
+        r.is_custom = true;
+        r.code = "report_with_custom_option".to_string();
+    })
+}
+
+pub fn mock_report_j() -> ReportRow {
+    inline_init(|r: &mut ReportRow| {
+        r.id = "mock_report_j".to_string();
+        r.version = "3.0.1".to_string();
+        r.is_custom = false;
+        r.code = "report_with_custom_option".to_string();
+    })
+}


Added extra cases to test for prioritising is_custom reports

fergie-nz · 2024-11-27T23:59:00Z

server/service/src/report/report_service.rs

+    let reports_to_show_meta_data = report_filter_method(
+        repo.query_meta_data(Some(filter.clone()), None)?,
+        app_version,
+    );


Added querying all for metadata first with a sub selection of code, version, id, and is_custom. I thought this would help if our report list gets long and we aren't wanting to query all reports and filter out later.

Later we can collect the ids, and query all report data.

Added querying all for metadata first with a sub selection of code, version, id, and is_custom. I thought this would help if our report list gets long and we aren't wanting to query all reports and filter out later.

That's awesome, thank you

fergie-nz · 2024-11-28T00:02:15Z

server/service/src/report/report_service.rs

+    Ok(reports)
+}
+
+fn report_filter_method(


I have a feeling this logic could be more concise. However, after taking some time to think about it, I figure it is not worth spending time on now given we are only passing around report_meta_data

This is great, really easy to read and performant, thank you. There is a lot to like about this, abstraction for readability, the reports_of_code and custom_reports_of_code` reduces the cognitive load (one step at a time)

The only improvement I can think of, the query_meta_data could return a domain object, with a little bit of mapping after .load, to get the version number (from_str) and store it into that domain object (reduce mapping in compare_major_minor and in find_latest_report)

fergie-nz · 2024-11-28T00:04:52Z

server/repository/src/db_diesel/report.rs

 };

 use crate::{
    diesel_macros::{apply_equal_filter, apply_sort_no_case},
    schema_from_row, FormSchema, FormSchemaRow,
 };
-use crate::{EqualFilter, Pagination, Sort, StringFilter};


I have removed pagination as we won't use it for these queries.

andreievg

Looking very good, a joy to review. At some point we need to document this a bit more, wondering if it should be done inline in code, or as a separate .md file

server/service/src/report/report_service.rs

andreievg · 2024-12-16T01:18:43Z

server/service/src/report/report_service.rs

    let filter = filter
        .unwrap_or_default()
        .r#type(ReportType::OmSupply.equal_to());
-    Ok(repo.query(pagination, Some(filter.clone()), sort)?)
+
+    let reports_to_show_meta_data = report_filter_method(


I think it's better to return report_ids to show (that feels a bit more clear to me), maybe it's personal preference, how does it sound to you ?

Yes I like that too - will amend

andreievg · 2024-12-16T01:19:02Z

server/service/src/report/report_service.rs

+    let reports_to_show_meta_data = report_filter_method(
+        repo.query_meta_data(Some(filter.clone()), None)?,
+        app_version,
+    );


Added querying all for metadata first with a sub selection of code, version, id, and is_custom. I thought this would help if our report list gets long and we aren't wanting to query all reports and filter out later.

That's awesome, thank you

andreievg · 2024-12-16T01:26:37Z

server/repository/src/db_diesel/report.rs

+                }
+            }
+        }
+        Ok(query


That's so good !

andreievg · 2024-12-16T01:28:44Z

server/service/src/report/report_service.rs

+    Ok(reports)
+}
+
+fn report_filter_method(


This is great, really easy to read and performant, thank you. There is a lot to like about this, abstraction for readability, the reports_of_code and custom_reports_of_code` reduces the cognitive load (one step at a time)

The only improvement I can think of, the query_meta_data could return a domain object, with a little bit of mapping after .load, to get the version number (from_str) and store it into that domain object (reduce mapping in compare_major_minor and in find_latest_report)

and update tests

fergie-nz · 2024-12-17T00:00:41Z

Looking very good, a joy to review. At some point we need to document this a bit more, wondering if it should be done inline in code, or as a separate .md file

Cheers!

I am thinking in report .md file might be good? It documents a lot of other report editing how-tos, which would be accompanied by report version changes.

and update tests

fergie-nz · 2024-12-17T01:23:28Z

Have added better explanation in .md file, and added from_domain mapping to remove need for version mapping in meta data.

roxy-dao · 2024-12-17T02:31:37Z

server/repository/src/db_diesel/report_row.rs

@@ -93,6 +93,26 @@ impl Default for ReportRow {
    }
 }

+#[derive(Clone, Insertable, Queryable, Debug, PartialEq, Eq, AsChangeset, Selectable)]


You can default here so you don't need to impl default (:

andreievg · 2024-12-19T01:08:31Z

server/service/src/report/report_service.rs

-            .map(|r| r.id)
-            .collect(),
-    ));
+    let filter = ReportFilter::new().id(EqualFilter::equal_any(reports_to_show_meta_data));


this variable name doesn't make that much sense now

andreievg · 2024-12-19T01:09:50Z

server/service/src/report/report_service.rs

+        let mut report = reports
+            .clone()
+            .into_iter()
+            .filter(|r| r.id == result.clone().into_iter().next().unwrap())


result.first/pop().unwrap()

andreievg · 2024-12-19T01:12:55Z

server/repository/src/db_diesel/report.rs

@@ -21,6 +22,25 @@ pub struct Report {
    pub argument_schema: Option<FormSchema>,
 }

+#[derive(Debug, Clone, PartialEq)]
+pub struct ReportMetaData {


our domain object are composites atm, we don't really copy them (a bit less mapping and less to change when new fields are added)

So should just have Version as another key (of type Version)

andreievg · 2024-12-19T01:14:23Z

server/service/src/report/report_service.rs

-        .filter(|r| {
-            compare_major_minor(Version::from_str(&r.version), &app_version) != Ordering::Greater
-        })
+        .filter(|r| compare_major_minor(r.version.clone(), &app_version) != Ordering::Greater)


why clone ?

andreievg

looks great thanks

fergie-nz added 8 commits November 22, 2024 09:32

Add versioning sort to ReportRepo

e6c170b

Add code and is_custom filters

841e2f3

Add only upsert standard reports

1d1548d

Also add check for don't upsert old report versions

50d3403

Fix loop logic

7d3ed26

Add code to report node api

9cb8f7c

Add version to schema

3bc4682

Add front end mapping

9cf6c6b

github-actions bot added this to the V2.4.0-RC4 milestone Nov 22, 2024

github-actions bot added enhancement New feature or request Priority: Must Have The product will not work without this Team Ruru 🦉 Roxy, Ferg, Noel feature: reports labels Nov 22, 2024

fergie-nz added 2 commits November 25, 2024 09:15

Add version compare package

5a8975c

Add version compare when comparing version strings

1e94997

andreievg self-assigned this Nov 25, 2024

andreievg reviewed Nov 25, 2024

View reviewed changes

fergie-nz added 3 commits November 27, 2024 09:16

Merge branch 'v2.4.0' into 4791-add-report-versioning

deeae60

Remove front end report filtering

834f2a2

Add get latest report version to service layer

d9240a8

fergie-nz requested a review from andreievg November 27, 2024 00:59

fergie-nz added 6 commits November 27, 2024 15:58

Remove unnecessary changes

c7ba894

Simplify upsert logic

8b3bed3

Also add unique is_custom identifier to id

Remove unused repo function

920bd33

Remove unused imports

f494a95

Reset formatting

aefe758

Rename to count

df99155

roxy-dao removed this from the V2.4.0-RC4 milestone Nov 27, 2024

fergie-nz commented Nov 27, 2024

View reviewed changes

fergie-nz commented Nov 28, 2024

View reviewed changes

Remove unused struct

b618482

roxy-dao modified the milestones: V2.4.0-RC5, V2.4.0-RC6, v2.5.0 Nov 28, 2024

roxy-dao changed the base branch from v2.4.0 to develop November 28, 2024 22:46

andreievg approved these changes Dec 16, 2024

View reviewed changes

Report filter method returns vec of ids only

1ce0ac4

and update tests

fergie-nz added 3 commits December 17, 2024 13:17

Add from domain object

06635a0

and update tests

Update readme

914c673

Further readme updates

0556fae

fergie-nz requested a review from andreievg December 17, 2024 01:23

roxy-dao reviewed Dec 17, 2024

View reviewed changes

andreievg reviewed Dec 19, 2024

View reviewed changes

andreievg approved these changes Dec 19, 2024

View reviewed changes

fergie-nz added 2 commits December 19, 2024 16:25

Merge branch 'develop' into 4791-add-report-versioning

682b122

Merge branch 'develop' into 4791-add-report-versioning

751ac3b

fergie-nz mentioned this pull request Dec 19, 2024

Add overwrite flag to upsert json cli #5863

Closed

3 tasks

fergie-nz merged commit 84f56eb into develop Dec 19, 2024
2 checks passed

fergie-nz deleted the 4791-add-report-versioning branch December 19, 2024 03:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

4791 add report versioning #5533

4791 add report versioning #5533

fergie-nz commented Nov 22, 2024

github-actions bot commented Nov 22, 2024 •

edited

Loading

andreievg Nov 25, 2024

fergie-nz Nov 26, 2024

andreievg Nov 25, 2024

fergie-nz Nov 27, 2024

fergie-nz Nov 27, 2024

fergie-nz Nov 27, 2024 •

edited

Loading

andreievg Dec 16, 2024

fergie-nz Nov 28, 2024

andreievg Dec 16, 2024

fergie-nz Nov 28, 2024

andreievg left a comment

andreievg Dec 16, 2024

fergie-nz Dec 16, 2024

fergie-nz Dec 17, 2024

andreievg Dec 16, 2024

andreievg Dec 16, 2024

andreievg Dec 16, 2024

fergie-nz commented Dec 17, 2024

fergie-nz commented Dec 17, 2024

roxy-dao Dec 17, 2024

andreievg Dec 19, 2024

andreievg Dec 19, 2024

andreievg Dec 19, 2024

andreievg Dec 19, 2024

andreievg Dec 19, 2024

andreievg left a comment

4791 add report versioning #5533

4791 add report versioning #5533

Conversation

fergie-nz commented Nov 22, 2024

👩🏻‍💻 What does this PR do?

💌 Any notes for the reviewer?

🧪 Testing

📃 Documentation

github-actions bot commented Nov 22, 2024 • 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

Choose a reason for hiding this comment

fergie-nz Nov 27, 2024 • edited Loading

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

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

Choose a reason for hiding this comment

fergie-nz commented Dec 17, 2024

fergie-nz commented Dec 17, 2024

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

andreievg left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 22, 2024 •

edited

Loading

fergie-nz Nov 27, 2024 •

edited

Loading