-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[MBL-1346] Support User Friendly Validate Checkout Error Messaging #2053
Conversation
we don't need to use the new errorTypes property right now
3abcd6d
to
3fe9ff2
Compare
c9c767e
to
e4e5a25
Compare
465e371
to
fcfd273
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this approach confusing, since we'd now have a ValidateCheckoutEnvelope with valid = false
and an ErrorEnvelope that basically mean the same thing. Left a more detailed comment with suggestions (that basically proposes we get rid of one of them). I'm glad we're getting better error messaging, though!
Also, note: Iirc, in the case where the error is "This payment method isn't valid for this project", I'm pretty sure we don't want to bump the user back to the confirm details page. This might be a different ticket/pr, but just wanted to raise it so you're aware of it in case it matters for your approach, since we'll want to differentiate between "error that restarts checkout" and "error that tells the user to just try something else from here".
@@ -14,6 +14,11 @@ extension ValidateCheckoutEnvelope { | |||
guard let envelope = ValidateCheckoutEnvelope.validateCheckoutEnvelope(from: data) else { | |||
return SignalProducer(error: .couldNotParseJSON) | |||
} | |||
|
|||
if envelope.valid == false { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't make sense to me. If we're always going to say that valid == false
means we create an error envelope instead, why would we have a valid
field at all on the ValidateCheckoutEnvelope
? What do you think of either a) using the current valid
field in the postCampaignCheckoutVM to decide if we should show an error banner or not or b) getting rid of the valid
field entirely and instead create the errorEnvelope immediately once we see the checkout isn't valid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ifosli 🤔 Yea we could remove valid
from ValidateCheckoutEnvelope
and instead of
if envelope.valid == false {...}
we could just check the checkout
object directly like this
guard data.checkout?.isValidForOnSessionCheckout.valid == true else {...}
Is that what you mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think that it's useful to differentiate between .couldNotParseJSON and .validateCheckoutError ErrorEnvelopes, though.
I also considered removing messages
in the same way because the extra validateCheckoutEnvelope()
seems excessive and not super useful for this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went ahead and[ removed valid
from the envelope](remove redundant valid property from ValidateCheckoutEnvelope) so that we just have the one!
As for updating the UX so that users who attempt to pledge with an invalid payment method, I already have a separate ticket to address this 😄 . Bessie should be adding an errorTypes
array to this mutation that I think we can expose and then use to do that.
c70d444
to
16957e3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple minor things - please do handle the "server doesn't have any error messages for us" case before merging - but overall looks good!
return ErrorEnvelope( | ||
errorMessages: [message], | ||
ksrCode: .ValidateCheckoutError, | ||
httpCode: 200, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: 200 seems like a weird choice for the http code for an error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, in this case, technically, the request was sent successfully even though there was an error on the backend. A 400 (Bad Request) didn't make much sense to me either.
@@ -14,6 +13,11 @@ extension ValidateCheckoutEnvelope { | |||
guard let envelope = ValidateCheckoutEnvelope.validateCheckoutEnvelope(from: data) else { | |||
return SignalProducer(error: .couldNotParseJSON) | |||
} | |||
|
|||
guard data.checkout?.isValidForOnSessionCheckout.valid == true else { | |||
return SignalProducer(error: .validateCheckoutError(envelope.messages.first ?? "")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I think the message param here should be optional instead of passing in the empty string, so that we can show the "Something went wrong" default (instead of a banner with an empty string) if the server doesn't give us an error message for some weird reason. Or we could just pass in the entire list of messages instead; that might make more sense actually.
📲 What
Displays more user-friendly error messaging from the backend if the ValidateCheckout call fails.
🤔 Why
Error messaging can now make more sense and provide more context to users than the generic "Something went wrong" messaging we currently have.
🛠 How
ErrorEnvelope.swift
that creates an envelope specific to this use caseValidateCheckoutEnvelope.envelopeProducer()
that returns anErrorEnvelope
if we could parse the GraphAPI JSON, but the server deemed the checkout invalid.✅ Acceptance criteria
Review Recommendation