search

Atlas-Authority / marketing-automation

GNU General Public License v3.0
20 stars 16 forks source link

Error: Primary duplicate is accounted for twice: XXXXXXXXXXXX #184

Open apwidejulien opened 5 months ago

apwidejulien commented 5 months ago

It seems a new "pattern" appeared recently in our marketplace reporting; several entries with same order id (transaction id): image

For the provided example, we have:

This result raise an error in action generation:

Get action for event upgrade , Transaction ID: (AT)IN-EU-000-656-718[AEI-04a806ba-3b02-349e-a8c4-6d6cdc1c93c1]
Found duplicate deals. Pick single deal:  20358085480 , deals to delete: [ '20358085481' ]
Get action for event renewal , Transaction ID: (AT)IN-EU-000-656-718[AEI-04a806ba-3b02-349e-a8c4-6d6cdc1c93c1]
Found duplicate deals. Pick single deal:  20358085480 , deals to delete: [ '20358085481' ]
7/3/2024, 6:14:35 AM ERR! Runner Error:
7/3/2024, 6:14:35 AM ERR! Runner   Error: Primary duplicate is accounted for twice: 20358085480
7/3/2024, 6:14:35 AM ERR! Runner       at ActionGenerator.singleDeal (/usr/src/app/src/lib/deal-generator/actions.ts:210:15)
7/3/2024, 6:14:35 AM ERR! Runner       at ActionGenerator.actionForRenewal (/usr/src/app/src/lib/deal-generator/actions.ts:105:23)
7/3/2024, 6:14:35 AM ERR! Runner       at ActionGenerator.actionsFor (/usr/src/app/src/lib/deal-generator/actions.ts:46:36)
7/3/2024, 6:14:35 AM ERR! Runner       at /usr/src/app/src/lib/deal-generator/actions.ts:39:41
7/3/2024, 6:14:35 AM ERR! Runner       at Array.flatMap (<anonymous>)
7/3/2024, 6:14:35 AM ERR! Runner       at ActionGenerator.generateFrom (/usr/src/app/src/lib/deal-generator/actions.ts:39:19)
7/3/2024, 6:14:35 AM ERR! Runner       at DealGenerator.generateActionsForMatchedGroup (/usr/src/app/src/lib/deal-generator/deal-generator.ts:103:42)
7/3/2024, 6:14:35 AM ERR! Runner       at /usr/src/app/src/lib/deal-generator/deal-generator.ts:46:51
7/3/2024, 6:14:35 AM ERR! Runner       at withAutoClose (/usr/src/app/src/lib/util/helpers.ts:28:12)
7/3/2024, 6:14:35 AM ERR! Runner       at DealGenerator.run (/usr/src/app/src/lib/deal-generator/deal-generator.ts:42:25)

I would be more than happy to help with, but not sure how to deal with this:

Thanks in advance for your help!

boris-moduscreate commented 4 months ago

@nhan-nguyen-se any thoughts on this?

nhan-nguyen-se commented 4 months ago

The questions are about business logic which I have very little with this app. @apwidejulien Do we have a deduplication logic already? If so I think we can extend it to handle more duplication cases we found here.

apwidejulien commented 4 months ago

You probably have more knowledge about business logic with this app than me :smile:

To unblock the import, I ended with this crappy stuff: https://github.com/apwide/marketing-automation/commit/fd1ca73a8d56f10414466751aad73ac5ec84c3cd

If I understood correctly, except for refund, we make a hard assumption that we should have only 1 pair transactionId-licenseId which result in 1 deal into HubSpot. However, with our example, we can see this case can happen. I have the feeling Atlassian recently changed the cardinality of the transactions export to not returning transaction, but transaction lines... (a new column appeared in the mkp export named transactionLineItemId )

What I did was just suffixing the transactionId with transactionLineItemId for the duplicates after having sorted them by maintenance date, so deals should be created for each transactionId, but the problem should probably be fixed in the core to map deal using pair license/txId/txLineId.

boris-moduscreate commented 4 months ago

Hmm ok I think that makes sense @apwidejulien. Do you want to file a PR for the changes?

Also, any chance you can help with the core changes?

Comments at https://marketplace-vendors.slack.com/archives/CSPCQJQVB/p1721395421009669 confirm what you wrote about suffixing the two for a valid unique id.

apwidejulien commented 4 months ago

Hi @boris-moduscreate ,

I've tried to integrate transactionLineItemId in the core engine: https://github.com/apwide/marketing-automation/commit/e86f9aed5280e735d829c028bf5d3e31ae5892a5

What is in my proposal:

This last assumption makes that we face same original issue with duplicate Error in case some records exists in HubSpot with transactionId only and engine tries to process several transactionLine in MAPC for the same transactionId. I don't know exactly how to proceed with this case because there is no "data conversion" for what it is in Hubspot... We should probably find a way to migrate this...

Let me know what you think about it.

boris-moduscreate commented 4 months ago

@nhan-nguyen-se Can you take a look at the changes Julien has shared?

@apwidejulien For the case of legacy items, if we cant deterministically migrate old data then I would say by default we ignore the items and log them out to console and Slack (if configured). And optionally let the user enable some kind of "fuck it" mode where we randomly generate a transactionLine and backfill those entries with them to "fix it"?

nhan-nguyen-se commented 4 months ago

It looks fine to me.

boris-moduscreate commented 4 months ago

@apwidejulien I wanted to check back on my last comment here.

apwidejulien commented 3 months ago

@boris-moduscreate ,

Sorry, I was off.

With the current state of the PR:

Resolution for the user would be to delete, in Hubspot, the problematic deal (deal without transactionLineId referenced by multiple records in MKP with same txId but different lineId). That way, on next run, engine will create 2 different deals each associated with their transactionLineItemId, so, no more problematic deal.

Do you think it is acceptable ? Or should we avoid failing the whole process and just log problematic entries at the end ? If yes, do you have such pattern already existing in the engine I could get inspired of ? I don't see any.

boris-moduscreate commented 3 months ago

Hope you had a great time off @apwidejulien!

I don't think the engine should stop is my point. This would be a bunch of manual work for anyone and would block initial deployment. Instead we log to slack, allow the user to clean stuff up like you said, but continue the run. And then have an env variable like "DELETE_BLOCKING_DEALS=YES" to have it auto delete.

We likely want the same functionality for errors like:

Error: Primary duplicate is accounted for twice: 21214688382
    at ActionGenerator.singleDeal (/usr/src/app/src/lib/deal-generator/actions.ts:210:15)
    at ActionGenerator.actionForRenewal (/usr/src/app/src/lib/deal-generator/actions.ts:105:23)
    at ActionGenerator.actionsFor (/usr/src/app/src/lib/deal-generator/actions.ts:46:36)
    at /usr/src/app/src/lib/deal-generator/actions.ts:39:41
    at Array.flatMap (<anonymous>)
    at ActionGenerator.generateFrom (/usr/src/app/src/lib/deal-generator/actions.ts:39:19)
    at DealGenerator.generateActionsForMatchedGroup (/usr/src/app/src/lib/deal-generator/deal-generator.ts:103:42)
    at /usr/src/app/src/lib/deal-generator/deal-generator.ts:46:51
    at withAutoClose (/usr/src/app/src/lib/util/helpers.ts:28:12)
    at DealGenerator.run (/usr/src/app/src/lib/deal-generator/deal-generator.ts:42:25)
apwidejulien commented 3 months ago

Thanks @boris-moduscreate, yes, my days off were relaxing! ;)

I've created this https://github.com/Atlas-Authority/marketing-automation/pull/185 to added changes you requested + small additions:

Changes requested:

For the auto delete, we have to delete all of the deals related to a txId because they are all representing the same txId without txLineId. When this option is enabled, we have this flow:

I've also added additional commits (which I can remove or push on different PRs if you prefer):

I've ran it on our instance with auto delete enabled and after having configured the mandatory transactionLineItemId field. Execution has:

I let you review my code and let me know what you think of it.