Skip to content

MOEN-44005 Personalize#107

Merged
arshiya-moengage merged 24 commits into
developmentfrom
MOEN-44005_personalize
May 7, 2026
Merged

MOEN-44005 Personalize#107
arshiya-moengage merged 24 commits into
developmentfrom
MOEN-44005_personalize

Conversation

@AnirudhaSM-engg-sdk
Copy link
Copy Markdown
Contributor

Jira Ticket

https://moengagetrial.atlassian.net/browse/MOEN-44005

Description

Added personalize feature module and support

@AnirudhaSM-engg-sdk AnirudhaSM-engg-sdk changed the title Moen 44005 personalize MOEN-44005 Personalize Apr 14, 2026
@AnirudhaSM-engg-sdk AnirudhaSM-engg-sdk marked this pull request as ready for review April 14, 2026 10:50
Copy link
Copy Markdown
Contributor

@msoumya-engg-sdk msoumya-engg-sdk left a comment

Choose a reason for hiding this comment

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

Verification Review

This PR was verified against:

Contract Alignment — mostly correct

  • Public API signatures, JSON payload shapes (singular vs plural), Promise-vs-fire-and-forget patterns, and error handling paths all align with the tech doc and contract PR.
  • 2 model-level deviations flagged inline:
    • ExperienceCampaign.experienceContext typed as Record<string, any> instead of Record<string, string> (contract violation).
    • ExperienceCampaignFailure.message field is not in the contract/tech doc.

Coding Standards — significant gaps vs cards module

  • No MoEngageLogger usage anywhere — parser uses raw console.warn; handler and public API have zero logging. The cards module demonstrates the established pattern (verbose at entry, debug for init, error on failure).
  • No MODULE_TAG constant and no per-class TAG strings for log prefixes.
  • Handler is standalone functions — cards uses a class with private TAG/appId.
  • Constants naming diverges (UPPER_CASE vs the key-prefixed camelCase in cards).
  • package.json files field contains phantom entries and is missing key exclusions.

Test Coverage — gaps identified

  • Parser: Array.isArray(...) else-branches, missing data key, malformed JSON for parseExperiencesResult, unknown source per-experience.
  • Public API: fetchExperiences(keys, attrs) with explicit attrs not tested; resolved Promise values never asserted.
  • Handler: parsed-field round-trip not verified (only array lengths).
  • PayloadBuilder: empty-input edge cases untested.

Missing Component

  • No Android native bridge code present. Please confirm whether this is a separate follow-up PR.

Inline comments on specific lines/files provide concrete suggested fixes for each item.


Generated by Claude Code

Comment thread sdk/personalize/src/model/ExperienceCampaign.ts Outdated
Comment thread sdk/personalize/src/model/ExperienceCampaignFailure.ts Outdated
Comment thread sdk/personalize/src/internal/MoEPersonalizeParser.ts Outdated
Comment thread sdk/personalize/src/internal/MoEPersonalizeConstants.ts Outdated
Comment thread sdk/personalize/src/internal/MoEPersonalizeHandler.ts Outdated
Comment thread sdk/personalize/src/__tests__/ReactMoEngagePersonalize.test.ts
Comment thread sdk/personalize/src/__tests__/Handler.test.ts
Comment thread sdk/personalize/src/__tests__/PayloadBuilder.test.ts
Comment thread sdk/personalize/package.json Outdated
Comment thread sdk/personalize/src/internal/utils/PayloadBuilder.ts Outdated
Copy link
Copy Markdown
Contributor

@msoumya-engg-sdk msoumya-engg-sdk left a comment

Choose a reason for hiding this comment

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

Follow-up Review: Coding Standards & Logging Gaps

This is a follow-up to the prior verification review, adding inline comments for issues 2–10 from the earlier "not added" list, plus additional logging gaps identified across the module.

Coding standards deviations (vs. cards reference module)

  • Public API uses class instead of the repo-standard namespace — flagged for team discussion
  • Internal filenames are prefixed with MoEPersonalize* (redundant inside internal/)
  • No internal/utils/ subdirectory for utility helpers
  • Native bridge registered as 'MoEReactPersonalize' (older pattern) vs. cards' 'MoEngageCardsBridge'
  • Parser file combines extraction + mapping — cards splits these into two files
  • Models have no JSDoc (@author/@since) tags
  • Type style uses Record<string, any> instead of the codebase-standard { [k: string]: any }
  • Extra babel-jest devDependency not present in other modules
  • ExperienceCampaignFailure.reason typed as string — loses compile-time safety over known failure reasons; suggested string-literal union

Logging gaps (additional to the previous review)

  • iOS native handler (MoEReactNativePersonalizeHandler.m) — zero logging. No entry traces, no error logging on the rejection path, no logging on NSJSONSerialization failure, duplicated resolve/reject code between fetchExperiencesMeta and fetchExperiences that should be extracted into a helper with centralized logging.
  • Parser — doesn't log entry traces, malformed-payload warnings, or missing-field warnings. Silent defaults hide server/bridge issues.
  • PayloadBuilderHandler — the outgoing JSON payload and native response are not logged anywhere on the JS side, making the bridge flow entirely opaque in debug traces. At minimum, the handler should log payload-in / response-out at verbose.
  • serializeCampaign — no warning when payload or experienceContext is null/undefined; malformed inputs produce silently-broken wire payloads.

Inline comments contain concrete suggested fixes for each item.


Generated by Claude Code

Comment thread sdk/personalize/src/index.ts
Comment thread sdk/personalize/src/internal/MoEPersonalizeHandler.ts Outdated
Comment thread sdk/personalize/src/NativeMoEngagePersonalize.ts Outdated
Comment thread sdk/personalize/src/internal/MoEPersonalizeParser.ts Outdated
Comment thread sdk/personalize/src/model/ExperienceCampaign.ts Outdated
Comment thread sdk/personalize/src/model/ExperienceCampaignFailure.ts Outdated
Comment thread sdk/personalize/package.json
Comment thread sdk/personalize/ios/MoEReactNativePersonalizeHandler.m
Comment thread sdk/personalize/src/internal/MoEPersonalizeParser.ts Outdated
Comment thread sdk/personalize/src/internal/utils/PayloadBuilder.ts
Contract alignment:
- experienceContext: Record<string, any> to Record<string, string> (matches proto)
- Remove message field from ExperienceCampaignFailure (not in contract)
- Add ExperienceFailureReason string literal union (8 server reasons)
- Parser validates unknown reasons with fallback to PERSONALIZATION_FAILED

Cards-pattern alignment:
- MoEngageLogger replaces console.warn across parser, handler, builder
- MODULE_TAG + keyXxx constants (keyData, keyExperienceKey, etc.)
- Handler refactored from standalone functions to class with try/catch + error logging
- Public API: verbose entry trace on every method, debug in constructor

Naming conventions:
- TS renames: Constants.ts, MoEngagePersonalizeHandler.ts, utils/PayloadParser.ts, utils/PayloadBuilder.ts
- iOS bridge: MoEReactPersonalize to MoEngagePersonalizeBridge (class + files)
- TurboModule registration updated to match

iOS handler improvements:
- NSLog entry traces on all methods
- resolveResponse helper extracted (removes fetch duplication)
- parsePayload helper with nil guard + logging

Package hygiene:
- files field cleaned (remove phantom lib/cpp, add __tests__ exclusion)
- babel-jest removed (unused with ts-jest preset)

Test coverage:
- Parser: missing data key, non-array branches, unknown source/reason, malformed JSON, missing fields
- Handler: deep field round-trip assertions
- Public API: return-value plumbing, explicit attrs forwarding
- PayloadBuilder: empty-input edge cases

JSDoc: @author MoEngage + @SInCE 1.0.0 on all model classes and fields
Copy link
Copy Markdown
Contributor

@msoumya-engg-sdk msoumya-engg-sdk left a comment

Choose a reason for hiding this comment

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

Changes LGTM. Once dependencies for this PR are released, update the versions and run the iOS CI.

NitPick: Update the PR description to a more meaningful one.

Comment thread sdk/personalize/src/model/ExperienceFailureReason.ts Outdated
@abhishek-engg-mobilesdk abhishek-engg-mobilesdk force-pushed the MOEN-44005_personalize branch 2 times, most recently from cdaf891 to d7388fa Compare April 22, 2026 09:44
@arshiya-moengage arshiya-moengage merged commit 2d59fc1 into development May 7, 2026
8 of 12 checks passed
@arshiya-moengage arshiya-moengage deleted the MOEN-44005_personalize branch May 7, 2026 12:33
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.

4 participants