6336 Make central record processor generic #6337

andreievg · 2025-01-29T10:07:42Z

👩🏻‍💻 What does this PR do?

Allow for central record processor to be generic.
Expand central record processor trait to have extra methods:
change_log_table_names
changelogs_filter
cursor_type
should_run

Processor trigger accepts type of central record processor

Relocated processor files to be more structured (now that there is central processor, it lives in parent dir and has contact form as child)

💌 Any notes for the reviewer?

🧪 Testing

Contact form emails still work

andreievg · 2025-01-29T10:16:14Z

server/service/src/processors/central_records/mod.rs

    let changelog_repo = ChangelogRepository::new(&ctx.connection);
-    let cursor_controller = CursorController::new(KeyType::ContactFormProcessorCursor);
+
+    let cursor_controller = CursorController::new(processor.cursor_type());


processor now defines cursor_type and changelog_filter

andreievg · 2025-01-29T10:17:18Z

server/service/src/processors/mod.rs

@@ -29,14 +30,14 @@ const CHANNEL_BUFFER_SIZE: usize = 30;
 pub struct ProcessorsTrigger {
    requisition_transfer: Sender<()>,
    invoice_transfer: Sender<()>,
-    contact_form_to_email: Sender<()>,
+    central_record: Sender<CentralRecordProcessorType>,


When triggered we specify which central record type should be processed

andreievg · 2025-01-29T10:17:47Z

server/service/src/processors/central_records/mod.rs

@@ -27,21 +26,39 @@ pub(crate) enum ProcessCentralRecordsError {

 const CHANGELOG_BATCH_SIZE: u32 = 20;

+#[derive(Clone)]
+pub enum CentralRecordProcessorType {


Type is used because processor will be triggered in generic way, and need to specify which records to process

lache-melvin · 2025-01-29T23:10:41Z

server/service/src/processors/central_records/mod.rs

+    /// Extra check to see if processor should trigger, like if it's central for contact form email
+    fn should_run(&self) -> bool {
+        true
+    }


If it's the central server processor, shouldn't it always only run on central server?

lache-melvin · 2025-01-29T23:13:15Z

server/service/src/processors/central_records/mod.rs

+impl CentralRecordProcessorType {
+    pub(super) fn get_processor(&self) -> Box<dyn CentralServerProcessor> {
+        match self {
+            CentralRecordProcessorType::ContactFormEmail => Box::new(QueueContactEmailProcessor),


Should we keep the capability to have multiple processors for the same record type, like we have for invoices?

I feel like the transfer processors are a bit different, because:

They prepare data in a particular shape before processors run (maybe can be done with some generic, but a bit complex for now ?)

I think we can use the cursor increase function of this generic processors also for transfers, you are right ! I'll have a little go at this now, won't spend too long on it though

I think we can use the cursor increase function of this generic processors also for transfers, you are right ! I'll have a little go at this now, won't spend too long on it though

Actually I am getting a bit worried with 'order of operation', so will keep existing transfer logic as is for now

lache-melvin

Thanks for this Andrei, knew this needed a little cleaning up 😁

This feels now like it's just a Processor - we could consolidate the requisition and invoice processors into this one (with should_run always true) - or it should actually just be central only?

andreievg · 2025-01-30T21:22:08Z

@lache-melvin thanks for your review, here is the change for the renaming: c666f7d (sorry now git is confused, and can't seem to recognise that file was moved and changed, in overall diff, but that diff looks ok)

I think we can use this 'Processor' in requisition and invoice transfers, but thought to not change too much and was worried a little bit about ordering. The try_process_record can implement getting the required records from the changelog and then running a bunch of specific processors, this can potentially be made generic but I think it would be a big hard with generic types, like

 // fn try_process_record(
fn prepare_record_for processing(&self) -> T // Get all the pre requisite records ready to pass to ProcessRecordFucntion
fn get_process_record_functions(&sefl) ->Vec<Box<dyn ProcessRecordFunction<T>>>> { 
// ...
// The above should work but when it comes to Box Processor, need to actually specify "T" (so stuck on that)

lache-melvin

LGTM, thanks for changes 🙏

lache-melvin · 2025-02-02T20:31:52Z

server/service/src/processors/general_processor.rs

+impl ProcessorType {
+    pub(super) fn get_processor(&self) -> Box<dyn Processor> {
+        match self {
+            ProcessorType::ContactFormEmail => Box::new(QueueContactEmailProcessor),


Only thought is whether this should support a vec of processors, like we have for invoices etc... there might end up being multiple things we want to process with the contact form? But guess we can add that when we get there 😁

Make central record processor generic

c666f7d

andreievg changed the title ~~Make central record processor generic~~ 6336 Make central record processor generic Jan 29, 2025

andreievg added 2 commits January 29, 2025 23:10

Use should_run

5792f59

Add contact form changelog table name to processor implementation

c01161b

andreievg commented Jan 29, 2025

View reviewed changes

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

andreievg added Team Ruru 🦉 Roxy, Ferg, Noel and removed enhancement New feature or request Team Ruru 🦉 Roxy, Ferg, Noel labels Jan 29, 2025

lache-melvin self-assigned this Jan 29, 2025

lache-melvin added this to the v2.6.0 milestone Jan 29, 2025

lache-melvin reviewed Jan 29, 2025

View reviewed changes

lache-melvin requested changes Jan 29, 2025

View reviewed changes

Rename generic processor

11956f9

lache-melvin approved these changes Feb 2, 2025

View reviewed changes

andreievg added 2 commits February 3, 2025 18:34

Merge branch 'develop' into 6336-Central-record-processor-to-be-generic

0f7b3f6

Fix test

a22cba2

andreievg merged commit 3ab36c4 into develop Feb 3, 2025
2 checks passed

andreievg deleted the 6336-Central-record-processor-to-be-generic branch February 3, 2025 05:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

6336 Make central record processor generic #6337

6336 Make central record processor generic #6337

andreievg commented Jan 29, 2025 •

edited

Loading

andreievg Jan 29, 2025

andreievg Jan 29, 2025

andreievg Jan 29, 2025

lache-melvin Jan 29, 2025

lache-melvin Jan 29, 2025

andreievg Jan 30, 2025

andreievg Jan 30, 2025

lache-melvin left a comment

andreievg commented Jan 30, 2025 •

edited

Loading

lache-melvin left a comment

lache-melvin Feb 2, 2025

6336 Make central record processor generic #6337

6336 Make central record processor generic #6337

Conversation

andreievg commented Jan 29, 2025 • edited Loading

👩🏻‍💻 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lache-melvin left a comment

Choose a reason for hiding this comment

andreievg commented Jan 30, 2025 • edited Loading

lache-melvin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreievg commented Jan 29, 2025 •

edited

Loading

andreievg commented Jan 30, 2025 •

edited

Loading