578 requisition transfer processor #579

andreievg · 2022-09-07T03:34:17Z

closes #578

These branch may not compile, check out head PR here for full changes of transfer logic (if you are viewing/testing overall changes). PR changes will be done to that branch, with link to commit in comments they address.

Added store row to requisition domain object
Added key_value_store_rows to MockData (need to insert site id in tests)
Added processors for requisition transfers and test
Generic method to get active store on site

Btw previously there was a sync_processor, and even though the logic for preparing records for processors is duplicated now, it's more direct (less enums) and the processors logic itself is simpler, previous logic is deleted in the head pr

Might want to look at diagrams in the docs as a reference to the processors logic

andreievg · 2022-09-07T03:35:01Z

server/graphql/requisition/src/mutations/request_requisition/insert.rs

-                name_row: mock_name_a(),
-            })
+            Ok(inline_init(|r: &mut Requisition| {
+                r.requisition_row = mock_request_draft_requisition()


Quite a lot of changes in this pr are from this, adding a store to requisition domain object and changing requisition initialisation to inline method

andreievg · 2022-09-07T03:36:24Z

server/service/src/processors/transfer/requisition/create_response_requisition.rs

@@ -0,0 +1,137 @@
+use crate::{number::next_number, requisition::common::get_lines_for_requisition};


The processor diagrams describe the logic here and show examples of how records travel around and when transfers are create/updated

andreievg · 2022-09-07T03:39:21Z

server/service/src/sync/mod.rs

+}
+
+#[derive(Error, Debug)]
+pub(crate) enum GetActiveStoresOnSiteError {


This is used in both processors and in sync push

jmbrunskill

Looking good added a few comments.
Will review the tests tomorrow.

jmbrunskill · 2022-09-13T04:09:19Z

server/service/src/processors/transfer/requisition/create_response_requisition.rs

+    }
+}
+
+fn generate_linked_requisition(


This name is fine, but I wonder if generate_response_requisition would be better?
We're not just linking here, we are also creating the new response requisition record.

Agree, thank you (it was an artefact of previous logic): cdf1c83

jmbrunskill · 2022-09-13T04:11:00Z

server/service/src/processors/transfer/requisition/create_response_requisition.rs

+
+    let source_requistition_row = &source_requisition.requisition_row;
+
+    let result = RequisitionRow {


:nit
I'm generally ok with calling a variable result but that suggested to me that the operation would return a Result<T,E> enum. I think something like response_requisition or just requisition would be better here.

We use it quite a bit. I tend to agree that we should be a bit more descriptive. I've been using this syntax quite a lot, to avoid extra nesting with Ok(), just makes it look slightly cleaner. I hope it's not a biggy since it's quite convenient to not think too much about naming if the variable is returned straight away with Ok(result). I am happy to reconsider and use more descriptive naming if it proves to be a problem.

Not a major at all, I use it a LOT too!
Sorry I think I've slipped into using these PR reviews to discuss/reflect on stylistic things.
In this case when reading the I found myself asking why is this a result when it's just creating a struct?
It was actually how I noticed that the number creation could fail during struct creation, so the variable naming kind of helped me in a way.

Sorry I think I've slipped into using these PR reviews to discuss/reflect on stylistic things.

Can have a chat in person sometime, and we can summaries in omSupply chat

jmbrunskill · 2022-09-13T04:12:45Z

server/service/src/processors/transfer/requisition/create_response_requisition.rs

+            connection,
+            &NumberRowType::ResponseRequisition,
+            &store_id,
+        )?,


nit:
I'd probably prefer to see this number created outside of the struct creation. It's hard to spot that this could return with an error here. (IMHO)

Fully agree, more often then not I try to do this, thanks: cdf1c83

jmbrunskill · 2022-09-13T04:15:36Z

server/service/src/processors/transfer/requisition/create_response_requisition.rs

+    for source_line in source_lines.into_iter() {
+        new_lines.push(RequisitionLineRow {
+            id: uuid(),
+            requisition_id: linked_requisition.id.clone(),


nit:
Only the id from linked_requisition is being used here, I wonder if it would be cleaner/clearer to just pass the new requisition_id in? Obviously makes it easier to grab something from the struct if this changed in future...

Sure: cdf1c83

jmbrunskill · 2022-09-13T04:23:41Z

server/service/src/processors/transfer/requisition/link_request_requisition.rs

+    RepositoryError, RequisitionRowRepository, RequisitionRowType, StorageConnection,
+};
+
+const DESCRIPTION: &'static str = "Link request requisition to response requisition";


nit:
Creating description statically is a different pattern to create_response_requisition.
Might be nice to be consistent?

Oops, thanks: cdf1c83

jmbrunskill · 2022-09-13T04:52:26Z

server/service/src/processors/transfer/requisition/create_response_requisition.rs

+        if linked_requisition.is_some() {
+            return Ok(None);
+        }
+


Not a major as I think people can read the code but might be nice to have a quick summary of the logic here.
Maybe need a second opinion on if this is might be overkill but I've found comments similar to this really helpful in the past.
e.g.

/*
At this point we have found a request requisition for our store that doesn't have a response requisition created for it yet.
We need to create a new response requisition record for this along with the associated requisition_lines.
We don't update the original record to add the linked requisition id, as this should be done by the remote store which owns the record. (See link_requisition_request for this logic)
*/

Great idea, my plan originally was to outsource this to diagrams, but have to have comments in code to at the very least justify why this would only run once: cdf1c83

fix comment: b136e62
fix typo: d4eccc7

Also did the same for shipments, and noticed that it is possible to have a double update on shipment (although it wouldn't cause infinite loop but still not ideal, fixed with new condition in UpdateInboundShipmentProcessor: 59567f1, basically Outbound Shipment is linked, and updated, which causes Inbound shipment to be updated, condition was also restricted to update InboundShipment only if OutboundShipment status was changed).

I would also like to remove a little bit confusion by calling shipment/requisitions with better names within processors (don't use source and linked, just go with request and response, inbound and outbound), I'll try tomorrow

Ohh man, i ended up updating requisition and shipments with more specific variable naming, and did a couple of extras

Use destructure + .map for requisition line generation

Fix comments

Use destructure for requisition update (saw it in one of @roxy-dao PRs, cleaner syntax then mut and slightly safer with immutability i think)

Anyways the commit is here: 61227a0, this makes the total diff from the last time you looked at this pr this: 5743790...61227a0

Looks awesome @andreievg! Those inline logic comments are super helpful.

jmbrunskill · 2022-09-13T05:09:38Z

server/service/src/processors/transfer/requisition/mod.rs

+    NameIsNotAnAciveStore(ChangelogRow),
+}
+
+// TODO transaction


Feels like some kind of transactions should be used in this process. At least where we are creating requisitions and requision_lines for example. We don't want to necessarily roll back on every error though so not sure.
Figure it's usually useful to comment on any 'todos' in a PR review anyway just to make sure it's been considered.

Ah, I see the we have a transaction wrapping
try_process_record_common so maybe that covers off this requirement?

Yep, I just left a comment to make sure i dont' forget, but added it a few times and forgot to delete this comment: cdf1c83

jmbrunskill · 2022-09-13T05:11:12Z

server/service/src/processors/transfer/requisition/mod.rs

+
+    let ctx = service_provider.context().map_err(Error::DatabaseError)?;
+
+    let active_stores =


Edge case I thought of here, is if an active store gets created during processing. I assume that's ok to ignore?

Super edge case I think, it's possible though, I think risk is super low and so is the impact, but thanks for thinking of an edge case. I think it's ok to not deal with this edge case

Can sync start while the processors are running from a previous sync? That'd stop active store changes occurring during processing. As all changes are pulled and integrated before the processors run then information the processors have would be always be complete I think.

Thinking there might be another rather peculiar edge case:

(omSupply) siteA/storeA makes a request requisition to (omSupply) siteB/storeB.

On the central server storeB is moved to be active on siteA (this queues to siteA all the records owned by storeB, as well as the records where storeB is the other party)

SiteA syncs with central server. This pushes the requisition records (these don't get queued back to siteA, as they are owned there). Then siteA pulls storeB data with all its other party records MINUS the requisition mentioned above.

SiteA is responsible for making the "response" records for storeB, but I suspect that the cursor of the processor is after the request requisition in question?

I suspect this is also think in edge case with low risk bucket too, just thinking.

Can sync start while the processors are running from a previous sync

Currently yeah it's possible, triggers for processors do not await for processing to finish. Added to improvements

I suspect this is also think in edge case with low risk bucket too, just thinking.

Yeah I think it's quite an edge case

this queues to siteA all the records owned by storeB, as well as the records where storeB is the other party)

Ouch, i was going to suggest adding sanity check on remote site which doesn't allow owned record to be synced to remote site after initialisation, above sounds like it would re-sync siteA owned records to siteA (where storeB is other party) ?

Ouch, i was going to suggest adding sanity check on remote site which doesn't allow owned record to be synced to remote site after initialisation, above sounds like it would re-sync siteA owned records to siteA (where storeB is other party) ?

Yes I believe that's what'll happen, as the "other party" records relative to storeB are records owned by siteA. It should be OK as the remote does push before pull, it's pretty guaranteed the central server should only queue data that is the latest on the remote anyway (unless central broke the rules and edited the record).

We could add logic to prevent this on the v5 API (only queue "other party" records where the owner of the records is not another store on the destination site being intialised), potentially adds a fair bit of complexity though.

jmbrunskill · 2022-09-13T05:15:40Z

server/service/src/processors/transfer/requisition/mod.rs

+            .changelogs(cursor, CHANGELOG_BATCH_SIZE, Some(filter.clone()))
+            .map_err(Error::DatabaseError)?;
+
+        if logs.is_empty() {


I wonder if we should break after a fixed 'MAX RECORDS' as well?
Just wondered if that might help if we had an infinite loop situation?
I'm just imagining some kind of loop forever situation that we can't easily see in the logs?

Related, it might be good to count and log the number of records processed for debugging in future. In normal operations i'm expecting it to process a very small number, so if we see large numbers in the logs we'd know something wasn't quite right?

Added this comment to #156 (comment), under checkbox for extra protection, we can do this in another issue i think. I was also thinking that around here would be the best place to add extra protection/logging. (I have quite a lot more to add on this subject, but will wait until that issue)

Wondering if we can get rid of the outer loop all together?

I figure the idea here is if the change_log has had records updated while the processor is running, then the processor needs to query again to make sure it's processed everything? But is that necessary? As I understand it the points the processor is triggered are:

A requisition was made to another active store on the site

Sync finished integration

From a user perspective I'd think that everything necessary is always captured by those triggers? I am assuming the calls are queued though, so if 1 occurs while 2 is still processing, the processor will just run again after finishing the current run? If so the I think it'd work without the outer loop? (Basically I'm thinking in terms of 4D workers with task queues)

If my understandering is correct then there is still a possible risk of failing to process creating transfers if omSupply crashed while several runs are queued - running all the processors on startup would address that though.

As an aside, is CHANGELOG_BATCH_SIZE necessary? Is a vec with 10,000 a no go? I'd do far larger entity selections without a worry in 4D haha. (IMAGINE ever receiving that many requisitions on a given sync cycle lol. Must have like 1000 staff picking orders. What are we, Amazon?)

I was reading the processor doc TODO

Sounds like there might not be messaging (or something like a message queue). So maybe that's the future fix to the infinite loop risk?

As an aside, is CHANGELOG_BATCH_SIZE necessary

Is it bad to have it ?

As an aside, is CHANGELOG_BATCH_SIZE necessary

Is it bad to have it ?

Nah it's probably all G, but in the insane 10,000 requisitions scenario you'll do 500 queries on the the database to process them 20 at a time rather than idk, 2 or 3 queries. Probably neglible either way -> splitting hairs -> if it's not a problem then not necessary -> cut it out -> never explain in the future again 😆 (People will one day copy paste this code and not know why it's there so it'll spread)

In that scenario, we would also be doing 10,000 updates to the cursor row in the key value store too.

If we're optimising for large scale there's likely more important performance improvements we could make,and updating the batch size is easy if required in future.

I agree, and I should clarify this is a nit (hence approving). I will highlight things that don't have apparent purpose in PRs - I've come from the 4D code base which has large amounts of code that serves no concrete purpose that makes it much harder to read and maintain. Code that solves a problem that doesn't exist, then breaks, then code fixing the code that didn't need exist in the first place. It's much easier to nip it in the bud before 6 layers of complexity have made it worse.

Just a side note, i am pretty sure in 4d selection and entity selection is not actually loaded into memory, just the cursors for those records, i think if you convert them to object you might have issues with memory in 4d. I guess what i am trying to say is I think there some purpose for the batch size, protection against loading too much into memory (although just theoretical benefit and not a conscious decision)

jmbrunskill · 2022-09-13T05:17:40Z

server/service/src/processors/transfer/requisition/mod.rs

+    #[error("Name id is missing from requisition changelog {0:?}")]
+    NameIdIsMissingFromChangelog(ChangelogRow),
+    #[error("Name is not an active store {0:?}")]
+    NameIsNotAnAciveStore(ChangelogRow),


Suggested change

NameIsNotAnAciveStore(ChangelogRow),

NameIsNotAnActiveStore(ChangelogRow),

Thanks, also had to update it's use: cdf1c83

…orRecord

jmbrunskill

Looks good to me.
I think we maybe should consider adding a few more tests? (see my comments discussing the existing test case)
I'd also like to see a concurrent update test, maybe doing something that might hit the channel buffer length for example to test out what might happen there.
Happy to work on creating a test for this if you think it's valuable.
Maybe it actually fits with the processor or something?

I actually think hitting the buffer limit might be ok, as it should just error when the buffer is full. Maybe haven't thought through all the implications of that.
If the change log record is there, we should in theory pickup all the actions in the next trigger I think, unless it causes a rollback or something.

jmbrunskill · 2022-09-13T21:26:19Z

server/service/src/processors/transfer/requisition/mod.rs

+        Ok(result)
+    }
+
+    /// Caller MUST guarantee that record.requisition.name_id is a store active on this site


Great comment

jmbrunskill · 2022-09-13T21:39:47Z

server/service/src/processors/transfer/requisition/test.rs

+                "user_id",
+                inline_init(|r: &mut UpdateResponseRequisition| {
+                    r.id = self.response_requisition.clone().map(|r| r.id).unwrap();
+                    r.status = Some(UpdateResponseRequstionStatus::Finalised);


This is actually a typo from another PR, but noticed it here...

Suggested change

r.status = Some(UpdateResponseRequstionStatus::Finalised);

r.status = Some(UpdateResponseRequisitionStatus::Finalised);

Would have to fix other uses.

In terms of priority fixing spelling errors, this doesn't feel too urgent to me.
If Finalised was spelt wrong I would consider it a higher priority as it would be persisted to the database so much harder to fix later.

server/service/src/processors/transfer/requisition/test.rs

jmbrunskill · 2022-09-13T22:49:46Z

server/service/src/processors/transfer/requisition/test.rs

+        tester.update_response_requisition_to_finalised(&service_provider);
+        delay_for_processor().await;
+        tester.check_request_requisition_status_updated(&connection);
+    };


The test looks good to me.
I think this is also a valid/more common ordering of events?

tester.insert_request_requisition(&connection).await; tester.update_request_requisition_to_sent(&service_provider); delay_for_processor().await; // likely to be a Sync here? tester.check_response_requisition_created(&connection); tester.update_response_requisition_to_finalised(&service_provider); delay_for_processor().await; // likely to be a Sync here? tester.check_request_requisition_was_linked(&connection); tester.check_request_requisition_status_updated(&connection);

This seems to pass fine btw but wondering if it's worth having tests for valid variations of ordering here?
I don't think it's possible to get completely out of order right?

I added some print lines to logged what's called in this test and get this, regardless of my test above vs the existing test and get the same outcome.

process_requisition_transfers creating requisition response linking requisition process_requisition_transfers updating requisition status

It was kind of surprising to me that process_requisition_transfers is only triggered twice even though there are 6 delay_for_processor().await;s here.

I think the delay_for_processor().await; after insert_request_requisition is not necessary for 2 reasons.

insert_request_requisition is using the extra_mock_data which inserts data using the repository rather than service layer.

Even if we had used the service layer, the insert doesn't trigger ctx.processors_trigger.trigger_requisition_transfers();

I would be tempted to make some changes to the code or comment to clarify the expectation here.

Ideas which would make it clearer to me would be to...
A) move insert_request_requisition outside the async move block, which would make it clearer that this is part of the test setup, rather than part of the test itself.
B) Manually call ctx.processors_trigger.trigger_requisition_transfers(); between insert_request_requisition and
check_response_requisition_not_created to simulate this being triggered by some other activity on the server.

BTW: I think that the order of proccesors in process_requisition_transfers is potentially important for some use cases?
I couldn't break your test by re-arranging them but wonder if it's worth adding a comment somewhere to suggest this ordering is important if it is...

let processors: Vec<Box<dyn RequisitionTransferProcessor>> = vec![ Box::new(LinkRequestRequisitionProcessor), Box::new(CreateResponseRequisitionProcessor), Box::new(UpdateRequestRequstionStatusProcessor), ];

My expectation with this ordering was thatLinkRequestRequisitionProcessor would run first which wouldn't find a Requisition Response record (because CreateResponseRequisitionProcessor hasn't run yet) so the linking would only occur on the second trigger, but that doesn't seem to be the case, I'm not entirely sure why though...

Quick summary of in person conversation.

The ordering of the processors in this vec does define the order that they run in for each record we get from the changelog.

In this case the order is unimportant because we loop over changelog records, not processor triggers.
When CreateResponseRequisitionProcessor runs, a new changelog record is created which will result in the LinkRequestRequisitionProcessor running from the same trigger. (The next loop iteration will see the new changelog record and process it)

I've added further diagram and comment as per our discussion (to document your comment above): e92057a

That diagram is excellent. Thanks!

mark-prins

nothing helpful to add.. logic looks fine to me

mark-prins · 2022-09-14T03:06:31Z

server/service/src/processors/transfer/requisition/create_response_requisition.rs

+                 available_stock_on_hand,
+                 average_monthly_consumption,
+                 snapshot_datetime,
+                 // TODO should comment transfer ?


wondered the same - and as usual, I ask WWMD? (what would mSupply do?)

[requisition]comment:=Get indexed string(17001; 175)+$t_serial+Choose([requisition]comment=""; ""; " ("+[requisition]comment+")") // "From request requisition N (comment - if any)" [requisition]requester_reference:=Get indexed string(17001; 175)+$t_serial+Choose([requisition]requester_reference=""; ""; " ("+[requisition]requester_reference+")") // "From request requisition N (requester_reference - if any)"

🙏

But I feel sooo ify about this, I've made a action plan under Questionsin this issue

Chris-Petty

Seems all good to me! Minus the rust learning curve I'm going through, I think it's pretty followable 🙂

Chris-Petty · 2022-09-13T23:35:17Z

server/service/src/processors/transfer/requisition/create_response_requisition.rs

+        sent_datetime: None,
+        finalised_datetime: None,
+        colour: None,
+        comment: None,


mSupply populates this as below but I'm not sure if it's massively useful? in omSupply you should have the linked_requisition_id record anyway if you ever wanted to display that.

As per comment to Mark's comment:

But I feel sooo ify about this, I've made a action plan under Questionsin this issue

Chris-Petty · 2022-09-13T23:51:23Z

server/service/src/processors/transfer/requisition/create_response_requisition.rs

+                 average_monthly_consumption,
+                 snapshot_datetime,
+                 // TODO should comment transfer ?
+                 comment: _,


In mSupply it does. Allows for the requester to comment per line why they want it, ordering so much etc., and for it to show up for the user that is handling responding to the requisitions.

I think this user story is a bit half baked though - the responder can then edit the comment and the original message from the requester is lost 🤷‍♂️. Anyway I think we should probably just copy it over here for now?

Or like my other comment on requisition.comment, you should have the related lines to look up comments from the requester. But to do that effectively you'd need to add requisition_line.linked_requisition_line_id as said here

the responder can then edit the comment and the original message from the requester is lost

Yeah, this is the main reason why I feel so iffy about it, can edit the comments that are transferred.

But to do that effectively you'd need to add requisition_line.linked_requisition_line_id as said #579 (comment)

I actually think it wouldn't be that costly and it's quite easy to do for requisitions, we do something similar with some transfer lines already for outbound shipment lines on request/response requisition:

https://github.com/openmsupply/open-msupply/blob/f720b50cc79a57e685fb394f4fc7be20d02e169e/server/graphql/types/src/types/requisition_line.rs#L102-L125

https://github.com/openmsupply/open-msupply/blob/f720b50cc79a57e685fb394f4fc7be20d02e169e/server/graphql/core/src/loader/invoice_line.rs#L51-L86

Chris-Petty · 2022-09-13T23:58:01Z

server/service/src/processors/transfer/requisition/create_response_requisition.rs

+                 comment: _,
+             }| RequisitionLineRow {
+                id: uuid(),
+                requisition_id: response_requisition_id.to_string(),


mSupply also has a linked_requisition_line_ID that links back to the copied line in the request requisition. Admittedly not much used at all though, was the conclusion to get rid of it?

(This omits any possible use in dashboards I'm not aware of)

I've made another question in #608, with link to this comment

Chris-Petty · 2022-09-13T23:58:18Z

server/service/src/processors/transfer/requisition/create_response_requisition.rs

+            },
+        )
+        .collect();
+


I'm not familiar with the logging in omSupply, but legacy mSupply logs the creation of the response:

Perhaps a new issue to follow on?

Added to logging section: #608

Chris-Petty · 2022-09-14T01:56:54Z

server/service/src/processors/transfer/requisition/mod.rs

+
+    let ctx = service_provider.context().map_err(Error::DatabaseError)?;
+
+    let active_stores =


Can sync start while the processors are running from a previous sync? That'd stop active store changes occurring during processing. As all changes are pulled and integrated before the processors run then information the processors have would be always be complete I think.

Thinking there might be another rather peculiar edge case:

(omSupply) siteA/storeA makes a request requisition to (omSupply) siteB/storeB.

On the central server storeB is moved to be active on siteA (this queues to siteA all the records owned by storeB, as well as the records where storeB is the other party)

SiteA syncs with central server. This pushes the requisition records (these don't get queued back to siteA, as they are owned there). Then siteA pulls storeB data with all its other party records MINUS the requisition mentioned above.

SiteA is responsible for making the "response" records for storeB, but I suspect that the cursor of the processor is after the request requisition in question?

I suspect this is also think in edge case with low risk bucket too, just thinking.

Chris-Petty · 2022-09-14T02:21:39Z

server/service/src/processors/transfer/requisition/mod.rs

+            .changelogs(cursor, CHANGELOG_BATCH_SIZE, Some(filter.clone()))
+            .map_err(Error::DatabaseError)?;
+
+        if logs.is_empty() {


Wondering if we can get rid of the outer loop all together?

I figure the idea here is if the change_log has had records updated while the processor is running, then the processor needs to query again to make sure it's processed everything? But is that necessary? As I understand it the points the processor is triggered are:

A requisition was made to another active store on the site

Sync finished integration

From a user perspective I'd think that everything necessary is always captured by those triggers? I am assuming the calls are queued though, so if 1 occurs while 2 is still processing, the processor will just run again after finishing the current run? If so the I think it'd work without the outer loop? (Basically I'm thinking in terms of 4D workers with task queues)

If my understandering is correct then there is still a possible risk of failing to process creating transfers if omSupply crashed while several runs are queued - running all the processors on startup would address that though.

As an aside, is CHANGELOG_BATCH_SIZE necessary? Is a vec with 10,000 a no go? I'd do far larger entity selections without a worry in 4D haha. (IMAGINE ever receiving that many requisitions on a given sync cycle lol. Must have like 1000 staff picking orders. What are we, Amazon?)

Chris-Petty · 2022-09-14T02:34:35Z

server/service/src/processors/transfer/requisition/mod.rs

+            .changelogs(cursor, CHANGELOG_BATCH_SIZE, Some(filter.clone()))
+            .map_err(Error::DatabaseError)?;
+
+        if logs.is_empty() {


I was reading the processor doc TODO

Sounds like there might not be messaging (or something like a message queue). So maybe that's the future fix to the infinite loop risk?

andreievg · 2022-09-16T00:24:14Z

I think i've answered all of the questions and did requested changes, there is a bit of cary over, but nothing seems crucial to do in this PR, and pr comments are getting too big now, so any further changes will be done in new PR:

Comment and their reference linkage for requisition and requisition line (summarised here: Sync carry over 14-09-22 #608)
Concurrency tests Concurrency test for processors #615
Logging ? Also in this issue:
Further protection from infinite loops ? Also in this issue:

andreievg · 2022-09-16T00:24:59Z

@jmbrunskill @mark-prins @Chris-Petty thanks for review

…-Requisition-transfer-processor

omsupply-bot bot added the sync v5 relating to finishing off full sync v5 support label Sep 7, 2022

andreievg commented Sep 7, 2022

View reviewed changes

Chris-Petty self-assigned this Sep 13, 2022

jmbrunskill reviewed Sep 13, 2022

View reviewed changes

Add further comment and update naming for requisition transfers

cdf1c83

andreievg mentioned this pull request Sep 13, 2022

Sync v5 #156

Closed

25 tasks

andreievg added 3 commits September 13, 2022 20:12

Update comment about linked requisition in RequisitionTransferProcess…

b136e62

…orRecord

Fix typos in comments

d4eccc7

Update variable names in requisition transfer processors

61227a0

andreievg mentioned this pull request Sep 13, 2022

Sync carry over 14-09-22 #608

Open

27 tasks

jmbrunskill approved these changes Sep 13, 2022

View reviewed changes

Update comment in requisition transfer test for line

bb82aa4

mark-prins reviewed Sep 14, 2022

View reviewed changes

Chris-Petty approved these changes Sep 14, 2022

View reviewed changes

andreievg added 2 commits September 15, 2022 08:44

Add further comments in requisition transfer processor

e657047

Update comments and documentation about same site requisition transfer

e92057a

Base automatically changed from 575-Processor-driver to 586-Tansfer-logic-head-and-PR-change-requests September 16, 2022 00:28

Merge branch '586-Tansfer-logic-head-and-PR-change-requests' into 578…

d45bb42

…-Requisition-transfer-processor

andreievg merged commit 65091f5 into 586-Tansfer-logic-head-and-PR-change-requests Sep 16, 2022

andreievg deleted the 578-Requisition-transfer-processor branch September 16, 2022 00:30

		@@ -0,0 +1,137 @@
		use crate::{number::next_number, requisition::common::get_lines_for_requisition};


		let source_requistition_row = &source_requisition.requisition_row;

		let result = RequisitionRow {


		let ctx = service_provider.context().map_err(Error::DatabaseError)?;

		let active_stores =

	NameIsNotAnAciveStore(ChangelogRow),
	NameIsNotAnActiveStore(ChangelogRow),

	r.status = Some(UpdateResponseRequstionStatus::Finalised);
	r.status = Some(UpdateResponseRequisitionStatus::Finalised);

578 requisition transfer processor #579

578 requisition transfer processor #579

Conversation

andreievg commented Sep 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmbrunskill 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

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

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

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

Chris-Petty Sep 14, 2022 • 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

Choose a reason for hiding this comment

jmbrunskill 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

mark-prins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chris-Petty 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

Choose a reason for hiding this comment

Chris-Petty Sep 13, 2022 • 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 commented Sep 16, 2022

andreievg commented Sep 16, 2022

andreievg commented Sep 7, 2022 •

edited

Loading

Chris-Petty Sep 14, 2022 •

edited

Loading

Chris-Petty Sep 13, 2022 •

edited

Loading