Skip to content

[AMSDK-9837] Support multi-datasets for ExperiencePlatformEvents#31

Merged
emdobrin merged 2 commits intoadobe:feature/datasetIdOverridefrom
emdobrin:AMSDK-9837
Jun 2, 2020
Merged

[AMSDK-9837] Support multi-datasets for ExperiencePlatformEvents#31
emdobrin merged 2 commits intoadobe:feature/datasetIdOverridefrom
emdobrin:AMSDK-9837

Conversation

@emdobrin
Copy link
Copy Markdown
Contributor

Description

Support multi-datasets for ExperiencePlatform events

Related Issue

The e2e flow fails with the following error:
240 - Validation An error occurred while processing the batch. Please contact support. Schema for the datasetId from catalog does not match the message; pointer to violation:
Requires this task for completion - EXEG-1745.

Motivation and Context

See Spec https://wiki.corp.adobe.com/display/adms/Dataset+ID+override+per+event

How Has This Been Tested?

Unit tests and manual test with Data Platform

Screenshots (if appropriate):

Types of changes

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

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@codecov
Copy link
Copy Markdown

codecov bot commented May 30, 2020

Codecov Report

Merging #31 into feature/datasetIdOverride will increase coverage by 0.19%.
The diff coverage is 92.86%.

@@                      Coverage Diff                      @@
##           feature/datasetIdOverride      #31      +/-   ##
=============================================================
+ Coverage                      71.34%   71.53%   +0.19%     
=============================================================
  Files                             29       29              
  Lines                            949      959      +10     
=============================================================
+ Hits                             677      686       +9     
- Misses                           272      273       +1     

Copy link
Copy Markdown
Contributor

@kevinlind kevinlind 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! Just a few minor comments.

class ExperiencePlatformEventTests: XCTestCase {
private let DATASET_ID = "datasetId"
private let XDM = "xdm"
private let DATA = "data"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[nit: style] We will need to agree on a style guide for Swift so this is up for discussion, but I think in general lower camel-case is used for all variables, even constants.
https://google.github.io/swift/#global-constants
https://swift.org/documentation/api-design-guidelines/#general-conventions

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I know.. I can change them for now and let's discuss about this more. I personally don't like the lower cased version because it's really difficult to differentiate them from other properties.

Copy link
Copy Markdown
Contributor

@nporter-adbe nporter-adbe 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, most of my comments were similar to @kevinlind

@emdobrin emdobrin requested a review from kevinlind June 1, 2020 18:45
@emdobrin emdobrin merged commit 07b82de into adobe:feature/datasetIdOverride Jun 2, 2020
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