Page MenuHome

Fix missing duplication xaction data.
AcceptedPublic

Authored by Nathan Letwory (jesterking) on Tue, Mar 10, 11:28 AM.

Details

Summary

Both 'mergedinto' and 'mergedfrom' are now
realized 'transaction.search'.

Test Plan:

  • Check output of 'transaction.search' for a task marked as duplicate of another
  • Check output of 'transaction.search' for a task that has another task set as its duplicate

Diff Detail

Repository
rP Phabricator
Branch
jesterking/duplication_xaction_fix
Build Status
Buildable 7063
Build 7063: arc lint + arc unit

Event Timeline

I have no ways of testing, so removing self from reviewer.

src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php
279

They are needed for us to query the data. I don't see the point of this comment.

Sergey Sharybin (sergey) requested changes to this revision.Tue, Mar 10, 11:34 AM
Sergey Sharybin (sergey) added inline comments.
src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php
279

This comment raises more questions than it gives answers for. What are the null-entries? How are they an issue?

This revision now requires changes to proceed.Tue, Mar 10, 11:34 AM

Clarify comment.

src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php
279

In maniphest.gettasktransactions there will be two transaction types for the same data. That looks like:

"7": [
    {
      "taskID": "7",
      "transactionID": "76",
      "transactionPHID": "PHID-XACT-TASK-342vklr2cnna2ed",
      "transactionType": "mergedinto",
      "oldValue": null,
      "newValue": "PHID-TASK-yvbqo4vj5fo64l3q6x7p",
      "comments": null,
      "authorPHID": "PHID-USER-xz3tq2nhsgmd7neec5rn",
      "dateCreated": "1583819920"
    },
    {
      "taskID": "7",
      "transactionID": "75",
      "transactionPHID": "PHID-XACT-TASK-uuaosmxfqf365vz",
      "transactionType": "core:edge",
      "oldValue": [],
      "newValue": [
        "PHID-TASK-yvbqo4vj5fo64l3q6x7p"
      ],
      "comments": null,
      "authorPHID": "PHID-USER-xz3tq2nhsgmd7neec5rn",
      "dateCreated": "1583819920"
    },

In the unfixed transaction.search it looks like

data": [
    {
      "id": 76,
      "phid": "PHID-XACT-TASK-342vklr2cnna2ed",
      "type": null,
      "authorPHID": "PHID-USER-xz3tq2nhsgmd7neec5rn",
      "objectPHID": "PHID-TASK-halcnpiiwguws3lvnp4a",
      "dateCreated": 1583819920,
      "dateModified": 1583819920,
      "groupID": "f3h257bgkqbzjexo3raorlvb66tbw3gt",
      "comments": [],
      "fields": {}
    },
    {
      "id": 75,
      "phid": "PHID-XACT-TASK-uuaosmxfqf365vz",
      "type": null,
      "authorPHID": "PHID-USER-xz3tq2nhsgmd7neec5rn",
      "objectPHID": "PHID-TASK-halcnpiiwguws3lvnp4a",
      "dateCreated": 1583819920,
      "dateModified": 1583819920,
      "groupID": "f3h257bgkqbzjexo3raorlvb66tbw3gt",
      "comments": [],
      "fields": {}
      }
    },

Where the type is set to null.

The first two additions in this patch for edge:type realize
the data for xaction with id 75. This should be enough to get all info needed, but I added the below two as well to get the old-style output also fixed up (xaction with id 76). The type field won't be null (as per comment), and the fields entry won't hold an empty dictionary.

  • Improve comment to communicate better the intent
Nathan Letwory (jesterking) marked 2 inline comments as done.Tue, Mar 10, 11:50 AM

Updated the comment.

src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php
279

There should be a summary of the description in the comment.

  • Explain that data is now actually retrieved and type set.
Nathan Letwory (jesterking) marked an inline comment as done.Tue, Mar 10, 1:23 PM
This revision is now accepted and ready to land.Tue, Mar 10, 2:53 PM

This patch has an issue actually, which will make it likely not accepted upstream. I will paste what @Sem Mulder (SemMulder) wrote me:


I have some comments on the patch
This is what is returned by transactions.search now:

{
      "id": 862844,
      "phid": "PHID-XACT-TASK-4szq2xfiiylmjlb",
      "type": "mergedinto",
      "authorPHID": "PHID-USER-dtdqgkd7cun2vet27blk",
      "objectPHID": "PHID-TASK-doazupi7q2xwwwc7vcxe",
      "dateCreated": 1580394007,
      "dateModified": 1580394007,
      "groupID": "ywoq6kcimnufgidsactjphmutzs3ptbl",
      "comments": [],
      "fields": {
        "old": null,
        "new": "PHID-TASK-j5nkgj6wluumaqggv5f2"
      }
    },
    {
      "id": 862843,
      "phid": "PHID-XACT-TASK-7afkmnfi5h2rxh4",
      "type": "mergedinto",
      "authorPHID": "PHID-USER-dtdqgkd7cun2vet27blk",
      "objectPHID": "PHID-TASK-doazupi7q2xwwwc7vcxe",
      "dateCreated": 1580394007,
      "dateModified": 1580394007,
      "groupID": "ywoq6kcimnufgidsactjphmutzs3ptbl",
      "comments": [],
      "fields": {
        "operations": [
          {
            "operation": "add",
            "phid": "PHID-TASK-j5nkgj6wluumaqggv5f2"
          }
        ]
      }
    }

This is what is returned by maniphest.gettasktransactions:

{
      "taskID": "67223",
      "transactionID": "862844",
      "transactionPHID": "PHID-XACT-TASK-4szq2xfiiylmjlb",
      "transactionType": "mergedinto",
      "oldValue": null,
      "newValue": "PHID-TASK-j5nkgj6wluumaqggv5f2",
      "comments": null,
      "authorPHID": "PHID-USER-dtdqgkd7cun2vet27blk",
      "dateCreated": "1580394007"
    },
    {
      "taskID": "67223",
      "transactionID": "862843",
      "transactionPHID": "PHID-XACT-TASK-7afkmnfi5h2rxh4",
      "transactionType": "core:edge",
      "oldValue": [],
      "newValue": [
        "PHID-TASK-j5nkgj6wluumaqggv5f2"
      ],
      "comments": null,
      "authorPHID": "PHID-USER-dtdqgkd7cun2vet27blk",
      "dateCreated": "1580394007"
    }

In the output of transactions.search we now have two transactions with the same type field but which are actually of a different type. This is not something we want to do since the same type should imply the same structure. Also makes it unlikely that the patch will be accepted upstream.

I can work around this for now, but just pointing this out.

The following description is copied from the transaction.search description:

Not all transactions have data: by default, transactions have a null "type" and no additional data. This API does not expose raw transaction data because some of it is internal, oddly named, misspelled, confusing, not useful, or could create security or policy problems to expose directly.
New transactions are exposed (with correctly spelled, comprehensible types and useful, reasonable fields) as we become aware of use cases for them.

Which I think was the reasoning for deprecating the maniphest.gettasktransactions call.

I mention this because to me the field names old and new do not make sense for mergedinto, because old will always be null.

The problem IMO is more that without the patch we _still_ have both transactions in there, but with completely missing data.

I could remove the bit that does do the old-style mergedinto transaction and keep only the one that follows from the edge:type
transaction.

It is unclear to me how it should be named then. Keep mergedinto? I could not find anything useful regarding that naming for
this specific transaction anywhere else.

And even with the adapted patch we will still see the now-useless transaction in the output.

This patch has an issue actually, which will make it likely not accepted upstream. I will paste what @Sem Mulder (SemMulder) wrote me:

I have some comments on the patch
This is what is returned by transactions.search now:

{
      "id": 862844,
      "phid": "PHID-XACT-TASK-4szq2xfiiylmjlb",
      "type": "mergedinto",
      "authorPHID": "PHID-USER-dtdqgkd7cun2vet27blk",
      "objectPHID": "PHID-TASK-doazupi7q2xwwwc7vcxe",
      "dateCreated": 1580394007,
      "dateModified": 1580394007,
      "groupID": "ywoq6kcimnufgidsactjphmutzs3ptbl",
      "comments": [],
      "fields": {
        "old": null,
        "new": "PHID-TASK-j5nkgj6wluumaqggv5f2"
      }
    },
    {
      "id": 862843,
      "phid": "PHID-XACT-TASK-7afkmnfi5h2rxh4",
      "type": "mergedinto",
      "authorPHID": "PHID-USER-dtdqgkd7cun2vet27blk",
      "objectPHID": "PHID-TASK-doazupi7q2xwwwc7vcxe",
      "dateCreated": 1580394007,
      "dateModified": 1580394007,
      "groupID": "ywoq6kcimnufgidsactjphmutzs3ptbl",
      "comments": [],
      "fields": {
        "operations": [
          {
            "operation": "add",
            "phid": "PHID-TASK-j5nkgj6wluumaqggv5f2"
          }
        ]
      }
    }

This is what is returned by maniphest.gettasktransactions:

{
      "taskID": "67223",
      "transactionID": "862844",
      "transactionPHID": "PHID-XACT-TASK-4szq2xfiiylmjlb",
      "transactionType": "mergedinto",
      "oldValue": null,
      "newValue": "PHID-TASK-j5nkgj6wluumaqggv5f2",
      "comments": null,
      "authorPHID": "PHID-USER-dtdqgkd7cun2vet27blk",
      "dateCreated": "1580394007"
    },
    {
      "taskID": "67223",
      "transactionID": "862843",
      "transactionPHID": "PHID-XACT-TASK-7afkmnfi5h2rxh4",
      "transactionType": "core:edge",
      "oldValue": [],
      "newValue": [
        "PHID-TASK-j5nkgj6wluumaqggv5f2"
      ],
      "comments": null,
      "authorPHID": "PHID-USER-dtdqgkd7cun2vet27blk",
      "dateCreated": "1580394007"
    }

In the output of transactions.search we now have two transactions with the same type field but which are actually of a different type. This is not something we want to do since the same type should imply the same structure. Also makes it unlikely that the patch will be accepted upstream.
I can work around this for now, but just pointing this out.
The following description is copied from the transaction.search description:

Not all transactions have data: by default, transactions have a null "type" and no additional data. This API does not expose raw transaction data because some of it is internal, oddly named, misspelled, confusing, not useful, or could create security or policy problems to expose directly.
New transactions are exposed (with correctly spelled, comprehensible types and useful, reasonable fields) as we become aware of use cases for them.

Which I think was the reasoning for deprecating the maniphest.gettasktransactions call.
I mention this because to me the field names old and new do not make sense for mergedinto, because old will always be null.

The problem IMO is more that without the patch we _still_ have both transactions in there, but with completely missing data.

And even with the adapted patch we will still see the now-useless transaction in the output.

I'm not sure I understand. For me (as consumer of the API), I don't really care if there are transactions which are useless. As long as I can get the transactions I care about I'm happy. Which I can now, so I am happy.

The problems I see are that upstream might not accept this patch because of two reasons.

Reason one: we are outputting two entries with the same type field but with different structure.

Reason two: reading the transaction.search description leads me to believe that we might want to think about better names for the fields than old and new (since those don't carry any meaning in the context of mergedinto, as far as I know)

I could remove the bit that does do the old-style mergedinto transaction and keep only the one that follows from the edge:type transaction.

It is unclear to me how it should be named then. Keep mergedinto? I could not find anything useful regarding that naming for this specific transaction anywhere else.

I propose just showing one entry with a consistent structure, since they are equivalent anyway. I would only show the mergedinto old-style transactions, since I am unsure if the edge:type new-style transactions exist for very old tasks.