dyne / reflow-os

Base scripts to run Reflow OS
7 stars 2 forks source link

PrimaryAccountable of resource with specific id is not updated after a transfer event #3

Open ocataco opened 3 years ago

ocataco commented 3 years ago

Context

I'm working on a simulation of a flow regarding medical gowns. Each gown has their own trackingIdentifier. At some point the 'hospital agent' produces a lot (with a manifest that lists the gowns it contains).

This lot (a resource which has it's own id), is at some point transferred to another 'cleaner' agent.

Screenshot 2021-09-13 at 16 11 42

The EconomicEvent is created succesfully in reflow os as you can see from the ui, but the ownership and the location of the lot resource are not updated to the receiver;

Screenshot 2021-09-13 at 16 15 28

Issue

This is a problem because when i try to consume the resource as the receiver after the transfer i get the error message: "You cannot do this since the provider is not accountable for the resource."

Below are the parameters for the transfer event i use in the client code.

event: {
        note: event_note,
        action: "transfer",
        provider: provider_id,
        receiver: receiver_id,
        hasPointInTime: ts,
        resourceInventoriedAs: lot_id,
      }

And this is when i view the state of the transfer event:

{
  "data": {
    "economicEvent": {
      "action": {
        "id": "transfer",
        "label": "transfer"
      },
      "id": "01FFFGYYJFA4V17WZZ2J1MWYW4",
      "note": "Gown Lot Transfer",
      "provider": {
        "id": "01FF84K296NSPKGQ39QD5EY30C",
        "name": "olvg",
        "note": "hospital user"
      },
      "receiver": {
        "id": "01FF84PJFC845FTTP59B3SBS6P",
        "name": "cleaner",
        "note": "I clean gowns"
      },
      "resourceInventoriedAs": {
        "accountingQuantity": {
          "hasNumericalValue": 0,
          "hasUnit": {
            "label": "om2:one",
            "symbol": "#"
          }
        },
        "currentLocation": {
          "id": "01FF84V8MWSEKNQ3ZQCRT8CPM8",
          "name": "OLVG, locatie Oost"
        },
        "id": "01FFFGYY8WE8KHM5BJ1FAAD7R3",
        "name": "Gown Lot",
        "onhandQuantity": {
          "hasNumericalValue": 0,
          "hasUnit": {
            "label": "om2:one",
            "symbol": "#"
          }
        },
        "primaryAccountable": {
          "id": "01FF84K296NSPKGQ39QD5EY30C",
          "name": "olvg"
        }
      },
      "resourceQuantity": null
    }
  }
}

Note that the name field of the primaryAccountable shows the name of the provider: "olvg" (the hospital agent) and not the name of the receiver "cleaner". Also the location is not updated to the location of the receiver.

For more context see: https://github.com/dyne/zenpub/issues/58

pral2a commented 3 years ago

@ocataco I think your issue is related that ZenPub issue but has no answer, neither https://github.com/dyne/zenpub/issues/60

densizengin commented 3 years ago

Hi @ocataco,

You should use the field toResourceInventoriedAs instead of resourceInventoriedAs when you want to use the actions transfer and move. Also, you should ask for toResourceInventoriedAs instead of resourceInventoriedAs in the response you'll get as well.

Cheers, srfsh

ocataco commented 3 years ago

Hi Srfsh,

Can confirm this works!

Screenshot 2021-09-21 at 13 03 11

the resources is produced by the provider, transferred and consumed by the receiver

Screenshot 2021-09-21 at 13 01 44

and the transfer shows in the ui on the material passport. when i try to click on "view resource" it says unrecognized, probably expected behaviour because it was consumed?

Thanks for the help srfsh :-)

pral2a commented 3 years ago

Thank you @srfsh! That's also helpful for ZenPub and WeLoop

@adam-burns I think it's super important to update the Reflow OS documentation with it to help Reflow pilots move further

densizengin commented 3 years ago

Hi Srfsh,

Can confirm this works!

Screenshot 2021-09-21 at 13 03 11

the resources is produced by the provider, transferred and consumed by the receiver

Screenshot 2021-09-21 at 13 01 44

and the transfer shows in the ui on the material passport. when i try to click on "view resource" it says unrecognized, probably expected behaviour because it was consumed?

Thanks for the help srfsh :-)

It seems like a UI bug, where it only uses the ID of resourceInventoriedAs.

Will work on that.

ocataco commented 3 years ago

Oh one more thing, i see in the screenshot that location of the lot is still OLVG and not the location of the receiver, do we need to do a seperate "move" for that?

Update:

The documentation of EconomicEvent.atLocation states: "The place where an economic event occurs. Usually mappable." So from the description this is not the right field to specifiy where it needs to update, but it seems to be the only field available to specify a location in the EconomicEvent.

adam-burns commented 3 years ago

@srfsh @ocataco can you determine if is this a usage misunderstanding or if it is a separate but related uncovered issue?

Oh one more thing, i see in the screenshot that location of the lot is still OLVG and not the location of the receiver, do we need to do a seperate "move" for that?

Update:

* I  have tried to pass the "atLocation" field with the id of the receiver  to the "transfer" event, but the resulting location remains the location of the provider.

* I have tried to create an extra "move" event (copying the parameters of the "transfer" action, thus providing toResourceInventoriedAs, and atLocation with id of the receiver, but the resulting location remains the location of the provider.

The documentation of EconomicEvent.atLocation states: "The place where an economic event occurs. Usually mappable." So from the description this is not the right field to specifiy where it needs to update, but it seems to be the only field available to specify a location in the EconomicEvent.

fosterlynn commented 3 years ago

The documentation of EconomicEvent.atLocation states: "The place where an economic event occurs. Usually mappable." So from the description this is not the right field to specifiy where it needs to update, but it seems to be the only field available to specify a location in the EconomicEvent.

You are right, it is not the right field, but we "hacked" it in to use for updating resource location in move events, when we tightened up what resource properties can be modified by an update, and what can't be without an event. It doesn't really conflict for move we think, since move is internal to an agent, and tends to be close by, otherwise there would probably be a transportation process. But VF would be happy to fix that hack whenever the timing works for people. (We've been really careful about changes during this period of active software development, especially changes that might have some ripple effects, even just a lot of discussion time when people are busy with more important issues, and changes that are not particularly backwards compatible. But we only need to be as careful as you all want us to be.)

Also possibly relevant, the question came up should transfers also be able to update the location at the toResourceInventoriedAs. We don't think they need it. If there is a new resource created as a result of the transfer, currentLocation can be set at that time. If the transfer increments an existing resource, it probably already has its currentLocation if the agent cares about location.

adam-burns commented 3 years ago

Thanks Lynn, @ocataco does currentLocation usage work for you?

ocataco commented 3 years ago

Hi @adam-burns I understand that you could specify currentLocation when creating a new resource, but in my test I was explicitly trying to transfer these container resources without creating a new resource which I think is a valid way in use cases where cooperation is important. It's just seems to make sense to give things the same name when you are working together to get insight into the flow of resources.

As srfsh found out, the implementation allows for this by making the resourceInventoriedAs and toResourceInventoriedAs have the same id. I noticed that there was no way to update the location in this kind of transfer. the owner changed, the location did not. and when I tried the move event to adjust for this it didn't seem to update the location either.

there is no usage misunderstanding as far as I can tell. for both strategies I tried, it did not seem to update the location of the resource in the implementation, at the time of writing.

fosterlynn commented 3 years ago

It's just seems to make sense to give things the same name when you are working together to get insight into the flow of resources.

@ocataco thanks, seems like a valid (and nice) use case, just not one VF has run into. You mean the same ID too, I assume, meaning it is the same resource instance in the software. (Just to note for future reference, you can follow the flows of resources in either case, by following them through the input-process-output and transfer activities, irrespective of agents and locations. But still, good use case.)

What is you all's preference? You could use transfer event atLocation to change the resource currentLocation, if you don't need atLocation for its original purpose, which was requested by people who thought they might want to track distances for climate accounting, but afaik have not yet done so. Or we could add a property to event only for explicitly changing the resource currentLocation, if you want at this moment in your development. If we add it, when move is fixed, it could use the new field.

To me, it has become abundantly clear and we should go ahead and add the field and fix the hack, from the VF point of view. Getting it into the graphql spec, and using it in projects, can be coordinated as needed, and at projects' convenience. Opinions welcome. I'll put together a MR/PR in VF though, to get things moving, some time today.

fosterlynn commented 3 years ago

Added a property in VF called toLocation on EconomicEvent, that can be used to update currentLocation on the EconomicResource. Not yet in the graphql spec, probably not yet showing in the website gitbook.

https://lab.allmende.io/valueflows/valueflows/-/merge_requests/663/diffs

ocataco commented 3 years ago

That sounds like a very nice addition, thanks Lynn!

Added a property in VF called toLocation on EconomicEvent, that can be used to update currentLocation on the EconomicResource. Not yet in the graphql spec, probably not yet showing in the website gitbook.

https://lab.allmende.io/valueflows/valueflows/-/merge_requests/663/diffs

sbocconi commented 2 years ago

Is the new property toLocation on EconomicEvent scheduled to be implemented in Bonfire?