Maintain correct first name when merging SF contacts #188
Conversation
| @@ -88,16 +88,6 @@ lazy val salesforce = all(project in file("lib/salesforce")) | |||
| libraryDependencies ++= Seq(okhttp3, logging, scalaz, playJson, scalatest) | |||
| ) | |||
|
|
|||
| lazy val `update-sf-identityid` = all(project in file("lib/sf/update-sf-identityid")) | |||
There was a problem hiding this comment.
this was added as an attempt to share the update SF identity id code. However now that we are updating first name as well, it is diverging a lot so I have given up and had two copies.
| _ | ||
| )) | ||
| } yield Operation.noHealthcheck { | ||
| WireRequestToDomainObject { |
There was a problem hiding this comment.
previously we were trying to make a big for comprehension working from the api gateway request level all the way down to the domain level. Then we were trying to artificially split between "validate" and "update" once at the domain level.
I decided it would make more sense to split by the level, so the WireRequestToDomainObject brings us down to the domain level and then steps just worries about dealing with a domain MergeRequest
| _ <- validateLastNames(accountAndEmails.map(_.lastName)) | ||
| maybeIdentityFirstName = firstNameForIdentityAccount(accountAndEmails.map { info => (info.identityId, info.firstName) }) | ||
| maybeOldFirstName = firstNameForSFContact(mergeRequest.sFPointer.sfContactId, accountAndEmails.map { info => (info.sfContactId, info.firstName) }) | ||
| firstNameToUse <- firstNameIfNot(maybeOldFirstName, maybeIdentityFirstName) |
There was a problem hiding this comment.
should probably combine these three previous lines into one function - TODO
|
|
||
| def apply(emailAddresses: List[Option[EmailAddress]]): ApiGatewayOp[Unit] = | ||
| def apply[EmailAddress]: AssertSame[EmailAddress] = (emailAddresses: List[EmailAddress]) => |
There was a problem hiding this comment.
This is now generic right? because all the names and the error message still refer to email addresses
|
|
||
| object EnsureNoAccountWithWrongIdentityId { // make sure all accounts are either this identity id or none | ||
|
|
||
| val apply: EnsureNoAccountWithWrongIdentityId = //FIXME use the validation from new-product-api |
There was a problem hiding this comment.
was this FIXME meant to be fixed in this PR or later ?
edit: I see that this FIXME was preexisting, you just moved it so probably it's for later
There was a problem hiding this comment.
I'm not sure if we should really move the validate stuff to a lib, or whether it is simple enough to just copy and paste the one file.
| @@ -8,21 +8,21 @@ class AssertSameEmailsTest extends FlatSpec with Matchers { | |||
| it should "be all ok with the same email addresses" in { | |||
There was a problem hiding this comment.
minor: This should probably be called AssertSameTest now and maybe the test descriptions could not refer to emails
There was a problem hiding this comment.
good spot, I'm actually testing assert same specifically in the case of Email where it is an Option. So I think I will leave it the same for now, unless you prefer to have the class as AssertSameTest and just leave the descriptions mentioning it's specifically for the emails case.
There was a problem hiding this comment.
I have renamed the actual AssertSame type parameter though, as that code is generic
| if (emailAddresses.distinct.size == 1) ContinueProcessing(()) | ||
| else ReturnWithResponse(ApiGatewayResponse.notFound("those zuora accounts had differing emails")) | ||
|
|
||
| } | ||
|
|
||
| trait AssertSame[EmailAddress] { | ||
| def apply(emailAddresses: List[EmailAddress]): ApiGatewayOp[Unit] |
There was a problem hiding this comment.
If we ever use the new-product-api validation for EnsureNoAccountWithWrongIdentityId we can probably use it for this as well
|
|
||
| class GetFirstNameToUseFirstNameIfNotTest extends FlatSpec with Matchers { | ||
|
|
||
| it should "firstNameIfNot both present should use winning one" in { |
There was a problem hiding this comment.
very minor:
the description of this test would read:
"GetFirstNameToUseFirstNameIfNotTest should firstNameIfNot both present should use winning one"
maybe it should be something like
"firstNameIfNot" should "use winning one if both present"
| (maybeOld, maybeIdentity) match { | ||
| case (Some(old), _) => Some(old) | ||
| case (None, Some(identityFirstName)) => Some(identityFirstName) | ||
| case (old, _) => old // will be none |
There was a problem hiding this comment.
is this different than doing maybeOld orElse MaybeIdentity ?
if it is maybe we don't need this function at all
There was a problem hiding this comment.
oh - good spot that's the plain english version too 😆
|
|
||
| def setOrClearIdentityId(sfContactId: SFContactId, idid: SFContactUpdate): Types.ClientFailableOp[Unit] = { | ||
| order = (idid match { | ||
| def setOrClearIdentityId(sfContactId: SFContactId, sfUpdateRequest: SFContactUpdate): Types.ClientFailableOp[Unit] = { |
There was a problem hiding this comment.
@pvighi github had a funny and list your comment about idid being a funny name, but anyway I've done a bit of a rejig on this test now - it wasn't great and it still needs a var due to side effects.
| @@ -12,22 +12,35 @@ object GetEmails { | |||
| case class ContactId(value: String) extends AnyVal | |||
|
|
|||
| case class EmailAddress(value: String) extends AnyVal | |||
| case class FirstName(value: String) extends AnyVal | |||
| case class LastName(value: String) extends AnyVal | |||
| case class Record(emailAddress: Option[EmailAddress], firstName: Option[FirstName], lastName: LastName) | |||
There was a problem hiding this comment.
The naming is a little confusing now. you still call GetEmails.apply buy it now returns a map of ContactId -> Record.
Also record seems like a really generic name, maybe we could use something more descriptive like personalDetails or ZuoraContactDetails
There was a problem hiding this comment.
good spot, thanks, I'll rename these all
| val someFirstName = Some(FirstName("hi")) | ||
| val contactIdsFromZuora = List(NameForContactId(SFContactId("winningSfContact"), someFirstName)) | ||
| val actual = GetFirstNameToUse.firstNameForSFContact(SFContactId("winningSfContact"), contactIdsFromZuora) | ||
| actual should be(ContinueProcessing(someFirstName)) |
There was a problem hiding this comment.
really minor:
don't you think in cases like this we need to test that the correct value is picked by providing some other contacts and asserting that it picks the right one? or would you rather have the minimal test to keep it simple?
There was a problem hiding this comment.
I didn't do so because I wanted to test the boolean condition and its outcome rather than the built in find functionality, however I'll add one for completeness
|
|
||
| val actual = MoveIdentityId(mock.setOrClearIdentityId)(sfPointer, maybeContactId, Some(FirstName("hello"))) | ||
|
|
||
| mock.order.reverse should be(List("clear contold setname: DontChangeFirstName", "clear contnew setname: SetFirstName(FirstName(hello))")) |
There was a problem hiding this comment.
really really tiny: maybe mock.order could return the reversed list already, otherwise it should be called reverseOrder
There was a problem hiding this comment.
I'll just get rid of the reversedness in the first place, it won't be slow with only a couple of items in the list...
|
|
||
| class MoveIdentityIdTest extends FlatSpec with Matchers { | ||
|
|
||
| final class Var() { |
There was a problem hiding this comment.
Maybe this could be called mockSetOrClearIdentityId and order could be something like invocationLog or something
| "FirstName" -> JsString("firstname") | ||
| )) | ||
| val expected = new PatchRequest(expectedJson, RelativePath("/services/data/v20.0/sobjects/Contact/contactsf")) | ||
| actual should be(expected) |
There was a problem hiding this comment.
👍 I'm going to try to start using this pattern, it makes it much more testable
There was a problem hiding this comment.
if we decide to switch over, there's a lot of tech debt we can plough through to get off the old style request maker stuff.
|
👍 👍 |
When we were merging the SF contacts, we decide the winner based on the reliability of the address. This is more likely for print subscriptions.
However, where one of the source contacts was created via identity, that would have a first name, whereas the target SF one may not have.
This PR updates the validation to make sure that all zuora accounts have the same last name before going ahead with the merge. In that case, if the losing account has just a full stop for the name, it will replace it with the one from the identity user. If the first name is present in zuora already, it will write the winning one into the winning contact. If there is no first name from either, it will just replace with a full stop. This will ensure that the zuora sync will work.
I have also added a check that the winning SF contact id really is in the ones connected from zuora. If not then it could indicate an issue.
I have also been doing a bit of tidyup of the function signatures. Rather than explicitly specifying the function type, I have used a trait with a single abstract method. This is experimental and may encourage excessive numbers of parameters, but see what you think.
I think there are a few codacy things to work through and I need to check the important lines have code coverage as I have a feeling I need to add some tests. But have a look at the overall view in the mean time. @pvighi @paulbrown1982 @abhichawla