feat: GraphQL address add mutation

[mikemurray]

Resolves #3919
Impact: minor
Type: feature

Issue

Update the existing addAccountAddressBookEntry and fix any issues preventing it from working properly.

Solution

Most of the implementation was already there. I made some updates to ensure it works as intended.

1. Updated accounts/addressBookAdd to return the added address
2. Allow accountId to be optional for addAccountAddressBookEntry mutation

Breaking changes

Should be none; however, the accounts/addressBookAdd was updated to return the added address. No methods depended on a specific return by that method, only that result was not undefined, which this satisfies.

Testing

1. start the app and sign in
2. navigate to localhost:3000/graphiql
3. run a mutation, one example follows...

mutation {
    addAccountAddressBookEntry(input: {
    address: {
      fullName: "Mike M."
      country: "US"
      address1: "2110 min st"
      city: "Santa Monica"
      region: "CA"
      postal: "90405"
      isBillingDefault: true
      isShippingDefault: true
      isCommercial: true
      phone: "555-555-5555"
    }
  }) {
    clientMutationId,
    address {
      fullName
      address1
    }
}
}

---

1. With the app still running
2. Add an address from the profile screen to verify that it was added correctly

[mikemurray]

Added WIP back since the address response wasn't fully transformed

[mikemurray]

@aldeed Fixed the address response, should be good for the initial testing.

[aldeed]

Looks great, but a couple concerns about proper error handling @mikemurray

[aldeed]

I think we should add at the top of decodeOpaqueIdForNamespace:

if (opaqueId === undefined || opaqueId === null) return null;

And then we don't need this accountId && check everywhere.

[ticean]

👍 Is it possible to go a level deeper and put that check in decodeOpaqueId?

[mikemurray]

@ticean decodeOpaqueId is not used in decodeOpaqueIdForNamespace. They are very identical methods though.

export const decodeOpaqueId = (opaqueId) => {
  const unencoded = Buffer.from(opaqueId, "base64").toString("utf8");
  const [namespace, id] = unencoded.split(":");
  return { namespace, id };
};

export const decodeOpaqueIdForNamespace = curry((namespace, opaqueId, error = new Error(`ID namespace must be ${namespace}`)) => {
  const unencoded = Buffer.from(opaqueId, "base64").toString("utf8");
  const [actualNamespace, id] = unencoded.split(":");
  if (actualNamespace !== namespace) throw error;
  return id;
});

[mikemurray]

As per the discussion in stand-up today, if we are going to explicitly require the accountId to be passed in, then the check accountId && decodeAccountOpaqueId(accountId) is no longer necessary.

If opaqueId is undefined, Buffer.from will throw an error that could be handled by the client, though it's vague. (Error message example: "First argument must be a string, Buffer, ArrayBuffer, Array, or array-like object." )

So...

1. Would you rather return null if opaqueId is not defined
2. Leave as is and let Buffer.from throw the error
3. throw a nicer error if opaqueId comes in undefined.

[ticean]

Why not destructure from a decodeOpaqueId() result inside decodeOpaqueIdForNamespace? Then you can put the check in decodeOpaqueId and keep it nice and DRY.

[aldeed]

Need to update the jsdoc @return. Also, it's now more confusing to return result because you don't know which object structure to expect. I'm thinking just return false for this final result, as long as our current client calls will handle that properly.

[mikemurray]

If we return false, then anywhere that looks for if(error) will not trigger, and anywhere that looks for if(result) will do nothing, leaving us in some nebulous state. It should probably just throw an error if it gets that far.

[ticean]

@aldeed apologies if I'm misunderstanding this comment. are you suggesting to return false in place of the updated address result? We do need to return the updated address in GraphQL and, more generally, return values for all operations. That was a decision made early in the the first round of API design.

[mikemurray]

I've decided to throw an error, instead of returning false in the method. That seems to be handled well by the resolver. (Unless you wanted it the return false here for other reasons?)

[ticean]

👍 Regarding my comment above, Eric explained on our standup that this is a condition where the upsert didn't change anything for some reason and would be a rare case. Agree that we shouldn't return the Mongo response there. That the address is returned in the happy path which was my only concern. Thanks!

[aldeed]

See my comment about the method return value. If you change that to return false, then you should check here. I'm thinking if (!updatedAddress) then you should probably throw an error so that it's clear to the client that the add was unsuccessful. It'll have to be a rather vague error message because we won't know why it failed, but it's also unlikely to happen.

[mikemurray]

I threw the error in "accounts/addressBookAdd", instead of returning false, so if the update failed for some rare reason, this we'll never get to this point and the error will show in the response.

[mikemurray]

@aldeed Ready for a re-review