[FX-722] Add support for derivedCardInfo.usState to ElementState for ForagePANEditText

[devinmorgan]

What

Extend ForagePANEditText to expose usState on the getState() and the setOnChangeEventListener callback. This was a feature requested by GoPuff.

We add support for usState by making ElementState by introducing a details field that is generic based on ForagePINEditText or ForagePANEditText.

Why

The intention of this PR was to maintain that getState() and setOnChangeEventListener remain the only two places that developers need to look / be aware to find out information about the state of a ForageElement. The hope is to keep the cognitive weight of the SDK as minimal as possible compared to adding additional computed properties on either the ForagePANEditText or the ForagePINEditText

Test Plan

• ✅ I've updated unit tests for this change to ensure that the correct usState acronym get's displayed.
• ✅ | ❌ It's up to you. That the CI tests pass means that this change is reverse compatible on the sample app. However, feel free to play around for yourself.

Demo

The CI tests passing are the demo for this case. The next PR updates the Sample App and will actually show a visual demo.

How

This PR can be released as is. It's not essential that we update the Sample App at the same time as releasing this code.

[devinmorgan]

Current dependencies on/for this PR:

• main
  • PR [FX-722] Add support for derivedCardInfo.usState to ElementState for ForagePANEditText #133 👈
    • PR Update sample app to show usState of PAN #134
      • PR [FX-780] Fix isValid initial value for PIN bug #136

This stack of pull requests is managed by Graphite.

[djoksimo]

@devinmorgan

Awesome sauce! I left some questions to clarify some things, but I liked your use of generics to make the design flexible for PIN elements.

What are some examples where you expect that we'll have to support additional PIN input details in the future?

[djoksimo]

@devinmorgan Will this be a breaking change for the client? My guess is no, but just want to confirm.

i.e. will the client have to update their code to conform to the new interfaces?

// client code
// before:

ForageElement

// after (breaking change):
ForageElement<ClientInsertsDetailsTypeHere>

Also, should we explicitly declare public classes as public or is it fine to omit? Not sure what the kotlin linting rules prefer.

[devinmorgan]

I fear for the flakiness of the QA tests so I am taking a screenshot to capture that the CI tests passed for this PR at one point (see below).

Will this be a breaking change for the client?

It depends. As you can see the CI tests against this PR is that I did not change the Sample App in this PR. The significance of them passing is that this PR is reverse compatible (for the with the Sample App at least). The reason this change is reverse compatible with the Sample App is because the Sample App never explicitly refers to any of the types that this PR changes (e.g. ElementState, AbstractElement, etc). If the Sample App had referenced these types then this would have been a breaking change.

This same point applies to GoPuff. If they currently reference the types that this PR does change instead of letting the compiler infer the types, then this PR would be a breaking change. However, even if GoPuff does reference the types changed, it would only require minor updates in their code (just updating the types)

Screenshot of CI tests passing

[dleis612]

How do you feel about making derivedCardInfo non-nullable, but rather making the usState field inside nullable. I would prefer if someone can always reference .derivedCardInfo without having to null check it all the time.

[djoksimo]

+1

[devinmorgan]

excellent point! Just updated the code. Nice catch team!

[djoksimo]

Have you considered using inheritance instead of generics:

class PanElementState: ElementState  {
  derivedCardInfo: DerivedCardInfo
}

class PinElementState: ElementState {}

• less nesting (state.derivedCardInfo instead of state.details.derivedCardInfo)
• details for PIN elements will likely always be null

[devinmorgan]

Glad you brought this up Danilo! A couple of things:

1. we were hoping to be able to use inheritance initially. That would be the ideal because PIN and PAN state's could easily evolve based on their own needs and sure enough we'd also eliminate additional nesting as you're pointing out. The problem is that data classes in kotlin don't seem to support sub-classing. Definitely annoying.
2. Another option was to convert data class ElementState --> class ElementState as means of making inheritance possible. When I tried that with the sample app, it didn't break anything (i.e. appeared reverse compatible). However, data classes automatically generate some extra utility functions and given the time crunch on Friday, I didn't spend much time considering whether GoPuff is actually using any of these utility methods. If we suspect they're not using these utility methods, then it would be save to change data class ElementState --> class ElementState, and use inheritance there.

If we're open to shipping the Android tomorrow instead of today (assume we discuss at standup, changes take the afternoon, can release tomorrow morning), then we'd certainly have time to discuss this tradeoff and move forward with whatever design we prefer.

[devinmorgan]

Update:

The latest changes on this PR now satisfies these design goals:

• less nesting (state.derivedCardInfo instead of state.details.derivedCardInfo)
• details for PIN elements will likely always be null

I initially tried to pursue inheritance on class ElementState but soon realized that inheritance at the DTO level instead of generics makes it infeasible to house logic that is common to both PinElementStateManager and PanElementStateManager within the abstract ElementStateManager. Consumers of .getState() and .setOnChangeHandler() won't feel the different (all the desired properties will be . accessible so how we implement ElementState (e.g. as an interface, abstract class, or inheritance) fortunately does not matter.