Skip to content

fixed - duplicates of metas in notification call#475

Merged
ninaceban merged 2 commits intomasterfrom
personalization-notification
May 1, 2020
Merged

fixed - duplicates of metas in notification call#475
ninaceban merged 2 commits intomasterfrom
personalization-notification

Conversation

@ninaceban
Copy link
Contributor

@ninaceban ninaceban commented May 1, 2020

Description

If we have a decision with multiple items we should have only one insert in the personalization.decisions array

{
  "decisions": [
    {
      "id": "TNT:113866:1"
    },
    {
      "id": "TNT:115166:0"
    },
    {
      "id": "TNT:113712:1"
    }
  ]
}

Related Issue

Motivation and Context

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Improvement (non-breaking change which does not add functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA or I'm an Adobe employee.
  • I have made any necessary test changes and all tests pass.
  • I have run the Sandbox successfully.

});

return result;
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be extracted to a utility. Here's my thought:

// identity.js
const identity = value => value;

// dedupe.js
const dedupe = (items, keyResolver = identity) => {
  const set = new Set();
  return items.filter(item => {
    const key = keyResolver(item);
    if (set.has(key)) {
      return false;
    }

    set.add(key);
    return true;
  });
};

Example usage:

const dedupedMetas = dedupe(metas, meta => meta.id);


const collectMetas = (collect, decisionMetas) => {
const metas = flatMap(decisionMetas, identity);
const dedupedMetas = dedupe(metas.map(e => e.meta));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little confused by the metas naming, because I would expect each item in metas to be a meta, but it seems like it's an object with a meta property. This might be something to improve is something comes to mind for you.

@ninaceban
Copy link
Contributor Author

@Aaronius could you please take a look again, I have made some other changes. We need to skip some results that contain errors from metas. We will discuss this later if we should send this data to konductor.

const processMetas = (collect, logger, actionResults) => {
const results = flatMap(actionResults, identity);
const finalMetas = [];
const set = new Set();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this cause issues in IE11?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@jfkhoury jfkhoury left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, other than the Set comment I added. 👍

@ninaceban ninaceban merged commit dbd1e40 into master May 1, 2020
@ninaceban ninaceban deleted the personalization-notification branch May 1, 2020 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants