Improve zuora/salesforce identity id backfill #213
Conversation
QuarpT
requested review from
paulbrown1982,
johnduffell,
jfsoul,
pvighi and
mariogalic
Pull Request Test Coverage Report for Build 600
💛 - Coveralls |
| @@ -13,14 +13,17 @@ object GetByEmail { | |||
|
|
|||
| sealed trait IdentityAccount | |||
| case class IdentityAccountWithValidatedEmail(identityId: IdentityId) extends IdentityAccount | |||
| case object IdentityAccountWithUnvalidatedEmail extends IdentityAccount | |||
| case class IdentityAccountWithUnvalidatedEmail(identityId: IdentityId) extends IdentityAccount | |||
There was a problem hiding this comment.
if the identity id exists on all possibilities, just move it up a level and make both of these into a case object (effectively becomes a boolean with a better name)
There was a problem hiding this comment.
I'm not sure what you mean by 'move identity id up a level'
I think a single case class
case class IdentityAccount(id: IdentityId, userEmailValidated: Boolean)
would be sufficient
There was a problem hiding this comment.
yeah you're right.... boolean makes more sense than keeping the specially named trait
|
|
||
| object RawWireModel { | ||
|
|
||
| case class StatusFields(userEmailValidated: Boolean) | ||
| implicit val statusFieldsReads: Reads[StatusFields] = Json.reads[StatusFields] | ||
|
|
||
| case class User(id: String, statusFields: StatusFields) | ||
| case class User(id: String, statusFields: Option[StatusFields]) { | ||
| val isUserEmailValidated: Boolean = statusFields.exists(_.userEmailValidated) |
There was a problem hiding this comment.
not a big difference, but I tried to move towards keeping the wire and domain models clean, and put all the conversion code in the separate wireToDomainModel function. This does apply to varying degress in the older lambdas though!
| """.stripMargin | ||
| val actual = GetByIdentityId.wrapper.toNewResponse(Json.parse(identityResponse)) | ||
| actual should be(ClientSuccess(IdentityUser(IdentityId("1234"), hasPassword = false))) | ||
| } |
| case ClientSuccess(IdentityAccountWithUnvalidatedEmail) => ReturnWithResponse(ApiGatewayResponse.notFound("identity email not validated")) | ||
| case other: ClientFailure => ReturnWithResponse(ApiGatewayResponse.internalServerError(other.toString)) | ||
| }).withLogging("GetByEmail") | ||
| maybeExistingIdentityId <- findExistingIdentityId(getByEmail, getByIdentityId, emailAddress) |
There was a problem hiding this comment.
suggestion to make it simpler/easier to test:
currently getByEmail and getByIdentityId are passed into PreReqCheck and then directly onto findExistingIdentityId. This gives PreReqCheck extra parameters it doesn't need so harder to test.
My suggestion would be to partially apply findExistingIdentityId in the wiring stage i.e. Handler on line 72, and then pass it into PreReqCheck as something like EmailAddress => ApiGatewayOp[Option[IdentityId]]
This means they can be tested separately which should help and it avoids PreReqCheck becoming all knowing.
|
looks great, the main comments would be moving up the identity id field a level and simplifying the parameters of PreReqCheck and testing the new function separately. These are important because they are keeping the structure of the program simple as the code changes. |
| @@ -150,4 +131,45 @@ class PreReqCheckTest extends FlatSpec with Matchers { | |||
| PreReqCheck.acceptableReaderType(ClientSuccess(readerTypes)).toDisjunction.leftMap(_.statusCode) should be(scalaz.-\/("404")) | |||
| } | |||
|
|
|||
| "findExistingIdentityId" should "continue processing with identity id for existing validated account" in { | |||
There was a problem hiding this comment.
yep much better with the tests split up, thanks
There was a problem hiding this comment.
my comment about the new function is there's no need to bundle these new tests in the same file as the existing ones. It would be more decoupled to put findExistingIdentityId in its own file and same goes for the tests.
Work done:
Why are we doing this:
Increase the number of users backfilled with existing accounts.