Editing a variable sometimes updates other variables with the same name

[jeromesimeon]

Bug Report 🐛

In some cases, editing one variable would edit another variable making it impossible to distinguish them.

Expected Behavior

Update to a variable should be propagated to the same variable in other places in the document, but not to different variables.

Steps to Reproduce

Example with the copyright license template:

Both licensor and licensee variables have type AccordParty which seems to confuse the algorithm responsible for propagating variable changes.

[dselman]

Interesting. What does the CiceroMark look like? I think the editor just uses the variable name from the CiceroMark node.

[jeromesimeon]

Model

asset CopyrightLicenseContract extends AccordContract {
  /* the effective date */
  o DateTime effectiveDate

  /* licensee */
  o AccordParty licensee
  o String licenseeState
  o String licenseeEntityType
  o String licenseeAddress

  /* licensor */
  o AccordParty licensor
  o String licensorState
  o String licensorEntityType
  o String licensorAddress
...

CiceroMark

        {
          "$class": "org.accordproject.ciceromark.Variable",
          "value": ""Me"",
          "name": "partyId",
          "elementType": "String"
        }, 
        {
          "$class": "org.accordproject.commonmark.Text",
          "text": " ("Licensee"), a "
        }, 
        {
          "$class": "org.accordproject.ciceromark.Variable",
          "value": ""NY"",
          "name": "licenseeState",
          "elementType": "String"
        }, 
        {
          "$class": "org.accordproject.commonmark.Text",
          "text": " "
        }, 
        {
          "$class": "org.accordproject.ciceromark.Variable",
          "value": ""Company"",
          "name": "licenseeEntityType",
          "elementType": "String"
        }, 
        {
          "$class": "org.accordproject.commonmark.Text",
          "text": " with offices located at "
        }, 
        {
          "$class": "org.accordproject.ciceromark.Variable",
          "value": ""1 Broadway"",
          "name": "licenseeAddress",
          "elementType": "String"
        }, 
        {
          "$class": "org.accordproject.commonmark.Text",
          "text": ", and "
        }, 
        {
          "$class": "org.accordproject.ciceromark.Variable",
          "value": ""Myself"",
          "name": "partyId",
          "elementType": "String"
        }, 

...

So yes something is going very wrong in the CiceroMark there, there is no licensee or licensor.

This issue should probably be pushed down to the markdown-transform.

This is quite likely a common idiom.

[jeromesimeon]

We may need to revisit the question of fully qualified names...

Template mark holds on to the licensee and licensor variables, but those are expanded (since they are complex types) before generating the ciceromark.

templateMark

            {
              "$class": "org.accordproject.templatemark.VariableDefinition",
              "name": "licensee",
              "elementType": "org.accordproject.cicero.contract.AccordParty"
            }, 
            {
              "$class": "org.accordproject.commonmark.Text",
              "text": " ("Licensee"), a "
            }, 
            {
              "$class": "org.accordproject.templatemark.VariableDefinition",
              "name": "licenseeState",
              "elementType": "String"
            }, 
            {
              "$class": "org.accordproject.commonmark.Text",
              "text": " "
            }, 
            {
              "$class": "org.accordproject.templatemark.VariableDefinition",
              "name": "licenseeEntityType",
              "elementType": "String"
            }, 
            {
              "$class": "org.accordproject.commonmark.Text",
              "text": " with offices located at "
            }, 
            {
              "$class": "org.accordproject.templatemark.VariableDefinition",
              "name": "licenseeAddress",
              "elementType": "String"
            }, 
            {
              "$class": "org.accordproject.commonmark.Text",
              "text": ", and "
            }, 
            {
              "$class": "org.accordproject.templatemark.VariableDefinition",
              "name": "licensor",
              "elementType": "org.accordproject.cicero.contract.AccordParty"
            }, 

[dselman]

I see - so we need to distinguish between the name of the field to be updated when editing, and the name of the variable in the model (field name in the owning type). For complex types, those are two different things.

Perhaps we should set elementType in the CiceroMark for this example to AccordParty and the name should be licensee -- it is then up to the editor/transformation to decide on the best way to edit or render the complex type (e.g. we could use a popup with Concerto form for some complex types). In the default case that means editing or rendering the identifier (String).

We will need to distinguish between --> AccordParty and AccordParty because we would use different editors for those.

[jolanglinais]

I'm confused why they're both partyId in this CiceroMark.

[jeromesimeon]

I see - so we need to distinguish between the name of the field to be updated when editing, and the name of the variable in the model (field name in the owning type). For complex types, those are two different things.

Perhaps we should set elementType in the CiceroMark for this example to AccordParty and the name should be licensee -- it is then up to the editor/transformation to decide on the best way to edit or render the complex type (e.g. we could use a popup with Concerto form for some complex types). In the default case that means editing or rendering the identifier (String).

We will need to distinguish between --> AccordParty and AccordParty because we would use different editors for those.

But the variable partyId is of type String. I fear this will make pop ups difficult to handle.
Also, how would that work when a type has multiple variables (e.g., an Address)?

I think the bottom line is that you need more context (how did you get to that one variable).

[jeromesimeon]

I'm confused why they're both partyId in this CiceroMark.

Because this is the name of the variable for both the licensor and the licensee (both contain an AccordParty, and each AccordParty has a partyId).

[jeromesimeon]

Here is the same issue with the rental agreement with a RentalParty containing both a partyId and an address

Corresponding model

/**
 * The contract parties
 */
participant RentalParty extends AccordParty {
    o String address
}

/**
 * The template model
 */
asset RentalDepositClause extends AccordContract {
  o RentalParty tenant
  o RentalParty landlord
  o MonetaryAmount depositAmount
  o Period tenantDepositRestorationPeriod
  o Double monthlyBaseRentMultiple
  o String applicableLaw
  o String statute
  o String bankName
  o Period landlordDepositReturnPeriod
  o String exhibit
}

[dselman]

Also see: #175

[K-Kumar-01]

@dselman @irmerk @jeromesimeon
The main difficulty which we face here as I can understand is unable to differentiate the complex data type fields which are converted into fields with the same name. I was able to come up with two solutions:

1. If we have complex field types then can we start naming them as name-${i} where ${i} is the index of how many such fields already exist. Basically, it will distinguish them.
2. Another is when we load a template, all the variables can be provided with an arbitrary field say customName which will hold unique ids. In this way variables that have the same names like Random and Random can have id=uhasfn1 and then apply that algorithm which was used to change the variables based on variable[0].data.name to this field.

One drawback in 2nd is that I don't know whether more than different variables can have the same field.
Basically, two different persons having the same name in the same clause template can cause an issue.

[K-Kumar-01]

@jeromesimeon @dselman @irmerk
Any thoughts on the above approach?

[jolanglinais]

Sorry for the delay @K-Kumar-01! From my limited perspective on this, I think option 1 sounds like a decent approach and we should try it. As mentioned earlier I think this should probably be pushed down into markdown-transform.

[jeromesimeon]

hi @K-Kumar-01 I think we can distinguish between those variables using the context: In the example above:

Is the partyId simple variable inside the tenant complex variable or inside the landlord complex variable.

I believe the Slate Document Object Model will contain that context (to be confirmed).

[jolanglinais]

I'm making an Issue in markdown-transform and tagging you @jeromesimeon to possibly add additional context.

[jeromesimeon]

I'm making an Issue in markdown-transform and tagging you @jeromesimeon to possibly add additional context.

I think it could be entirely done in the web components. What's the reason this is better to do in the markdown transform?

[jolanglinais]

Alright, I was just going off of your #197 (comment) here :

This issue should probably be pushed down to the markdown-transform.

[jeromesimeon]

Alright, I was just going off of your #197 (comment) here :

This issue should probably be pushed down to the markdown-transform.

I realise I've am splendidly inconsistent here. But I think the crux of this is how do you know the scope of a variable and we could attempt to fix it either upstream (in the markdown transform) by having "names" be really keeping track of the context where they are used, or by making the logic which links variables together in the editor smarter.

The former approach is much more costly, so I'm wondering if someone would be interested in attempting the latter. I think mostly it would be to update the following logic in the editor itself:

web-components/packages/ui-contract-editor/src/ContractEditor/plugins/withClauses.js

Line 98 in 0667d02

// if we have edited a variable, then we ensure that all

[K-Kumar-01]

@jeromesimeon

The former approach is much more costly, so I'm wondering if someone would be interested in attempting the latter. I think mostly it would be to update the following logic in the editor itself:

The approach where we see the logic solely depends on the field i.e. name.
So if two variables have the same name, it would be ineffective in my opinion. I think somehow we need to distinguish them, which is possible only either by providing a unique id.

I think for a temporary solution we can comment out the logic to avoid any confusion and look for a more suitable approach by having a thorough discussion on this.

[jeromesimeon]

@jeromesimeon

The former approach is much more costly, so I'm wondering if someone would be interested in attempting the latter. I think mostly it would be to update the following logic in the editor itself:

The approach where we see the logic solely depends on the field i.e. name.
So if two variables have the same name, it would be ineffective in my opinion. I think somehow we need to distinguish them, which is possible only either by providing a unique id.

I'm suggesting we could change that logic to not be based only on the variable name, but also on the context for it within the Slate DOM. Not sure why you believe it would be ineffective.

I think for a temporary solution we can comment out the logic to avoid any confusion and look for a more suitable approach by having a thorough discussion on this.

Removing the feature as "not working" isn't the worst idea. I would wait for feedback from @dselman on that, though, since he implemented that feature in the first place.

[K-Kumar-01]

@jeromesimeon

I'm suggesting we could change that logic to not be based only on the variable name, but also on the context for it within the Slate DOM. Not sure why you believe it would be ineffective.

I apologize. I misunderstood the initial explanation, that's why I commented the above. I now understand the logic you are trying to say. I will see if I am able to find any suitable.