fix: Add enum value to hash ecommerce event attributes #169
Conversation
| override fun logEvent(event: CommerceEvent): List<ReportingMessage> { | ||
| val messages: MutableList<ReportingMessage> = LinkedList() | ||
| //For CommerceEvent, Event Name is not required to generate hash. So, it will be always null. | ||
| event.products?.get(0)?.customAttributes?.let { |
There was a problem hiding this comment.
you're missing most of what is mappable for a commerce event:
event.getTransactionAttributes()event.customAttributes- properties of every product object (eg price, quantity) - you're just doing customAttributes of the 1st product here?
and - does ios or web support mapping properties of impressions and promotions?
There was a problem hiding this comment.
- event.getTransactionAttributes() (adding this changes)
- event.customAttributes(adding this changes)
- properties of every product object (eg price, quantity) - you're just doing customAttributes of the 1st product here? missed this by mistake, will move to loop for all the products.
Web doesn't support mapping properties of impressions and promotions; and for IOS I think currently it only supports custom events not commerce events.
| //For CommerceEvent, Event Name is not required to generate hash. So, it will be always null. | ||
| event.products?.forEach { | ||
| it.customAttributes?.let { | ||
| changeUserArray(it, CommerceEventUtils.getEventType(event), null, true) |
There was a problem hiding this comment.
i don't think we're suppose to map custom attributes of products like this - does the ui even support mapping them? I think you mean to just have event.customAttributes here
| } | ||
| event.impressions?.forEach { impression -> | ||
| impression.products.forEach { product -> | ||
| product.customAttributes?.let { |
There was a problem hiding this comment.
same comment as above - have you tested if the UI even populates a custom attributes that's sent into the product within a impression? if so, I doubt the hash here will be right...?
| } | ||
| event.transactionAttributes?.let { | ||
| val map = mapOf( | ||
| "Affiliation" to it.affiliation, |
There was a problem hiding this comment.
you should be using the constants that are in here: https://github.com/mParticle/mparticle-android-sdk/blob/main/android-kit-base/src/main/java/com/mparticle/kits/CommerceEventUtils.java#L324
and you are missing quite a few constants/mappings - look at every single call to "hashForFiltering()" in these methods - that is a potential constant that you should likely support:
filterCommerceEventfilterCommerceEntitiesfilterCommerceEntityAttributesfilterCommerceEventAttributes.
I'd encourage you how to think about how to move this logic into that class so that we have complicated business logic close together vs separated. maybe a function in KitConfiguration or in CommerceEventUtils which accepts in a CommerceEvent and returns a map of <hash, value> - and then you can just use that map in the kit.
Instructions
developmentSummary
Testing Plan
Reference Issue (For mParticle employees only. Ignore if you are an outside contributor)