[HOLD for retest] Remove html from Close Account errors

[Beamanator]

Problem:

In the past, we didn't show errors that occurred when a user tries to close their account. Instead, we displayed a generic error modal. As of this issue: #11341, we're planning to get ride of that modal and show the error that occurred. Some of these error messages have <p> before / after the error message (example), which doesn't look good in NewDot

Solution

Let's make sure that Close Account error messages that are shown in NewDot look good!

[Beamanator]

Maybe this should be WAQ? Since these errors will now show directly in NewDot? cc @JmillsExpensify (also do you prefer being tagged like this or just having the issue directly added to WAQ project?)

[JmillsExpensify]

I'm fined being tagged! It's good to keep track of these kinds of discussions. Also tagging @trjExpensify as we have alternating holiday time between now and the end of the year, and will be subbing in and out for each other.

[JmillsExpensify]

In general, I think this is a good candidate for WAQ, because the refactors precede WAQ from a roadmap perspective, and that makes this an existing issue that should be fixed.

[JmillsExpensify]

That said, I do wonder if it's better to not solely focus on the close account errors, but instead, errors more generally. I can only imagine that this problem with HTML formatting is going to surface in more parts of the app. I pretty much bet it is.

[Beamanator]

That said, I do wonder if it's better to not solely focus on the close account errors, but instead, errors more generally. I can only imagine that this problem with HTML formatting is going to surface in more parts of the app. I pretty much bet it is.

Fair, but I'm also potentially thinking that refactoring errors elsewhere is only "necessary" once those errors start showing up in the App. Similar to how we only did API refactors for commands that we use in the App. Know what I mean? Or is this way different since this issue is a much much smaller scope (just making sure HTML tags get stripped from error messages heading to NewDot) :D

Thinking a bit while typing, I actually think we could probably have a pretty simple generic solution that would do ^ (any errors sent to NewDot would have HTML tags stripped from the beginning & end)

[trjExpensify]

I agree with that. One thing to add to this is that in the past we haven't been able to add server errors in-line on forms. @ctkochan22 has been working on an issue where we're making that happen, but if we're updating error messages let's make sure they are placed contextually in-line.

[JmillsExpensify]

Ok great discussion. So we've got two thoughts here, I'm not clear what our path forward is.

Thinking a bit while typing, I actually think we could probably have a pretty simple generic solution that would do ^ (any errors sent to NewDot would have HTML tags stripped from the beginning & end)

I love this and think it's a pretty nice change given a lot of errors have HTML formatting, at least coming from OldDot.

One thing to add to this is that in the past we haven't been able to add server errors in-line on forms. @ctkochan22 has been working on an issue where we're making that happen, but if we're updating error messages let's make sure they are placed contextually in-line.

So are we saying that we should redo this entirely and bring the error in-line instead?

[Beamanator]

One thing to add to this is that in the past we haven't been able to add server errors in-line on forms.

Good to know, hadn't thought about that before.

So are we saying that we should redo this entirely and bring the error in-line instead?

Also a good question, the original purpose of this issue was to keep the form errors where they are, just remove the HTML tags from them (as luck would have it, Applause just created this issue (#13153) which has a video of Close Account errors with HTML tags).

@trjExpensify are you suggesting we move where the error shows or were you just pointing that out for context?

[trjExpensify]

Yeah, so I think it's fine to separate out the initiatives if we want to fix the HTML tags on all error messages we display in App as a first step. You're right it's probably better to split up the work and do that first, before updating the placement.

On that though, we do have growing inconsistency in NewDot now, and we should address that at some point:

• Front-end errors are displayed in-line
• Pattern B's "pending > failed" red brick road errors are displayed in-line
• Server-side errors are anchored to the bottom of forms

[Beamanator]

• Pattern B's "pending > failed" red brick road errors are displayed in-line

Woah, really? Just wondering do you have an example of this happening right now? I thought Pattern B errors were from the server-side so I assumed they were also anchored to the bottom of forms - but maybe i've been staring at Account Settings & Image changes for too long 😅

[trjExpensify]

Manage members is one I can think of (and find a screenie) off the top of my head:

^^ This is a screenshot, don't be fooled. ;)

[Beamanator]

Ohhh ok I was taking "in-line" and assuming you were saying like "in-line in a Form" (woah you took a screenshot of a video... I was confused why it wouldn't play). The interesting thing about the error you just showed, is it's a generic error that comes from the front end (here) but we also throw an error from the back end (here) so that's a bit funky

Either way, I think the main point is that I can resume removing HTML from server-side errors that are only meant to be displayed in NewDot 👍 (Should this be WAQ or later?)

[trjExpensify]

Either way, I think the main point is that I can resume removing HTML from server-side errors that are only meant to be displayed in NewDot 👍 (Should this be WAQ or later?)

I think that should be WAQ agreed, and we can proceed with that in isolation for the time being.

Ohhh ok I was taking "in-line" and assuming you were saying like "in-line in a Form" (woah you took a screenshot of a video... I was confused why it wouldn't play).

Yeah, "in-line" distinction here meaning that a user can expect consistency that the error appears beneath the UI element in error. Here's another in a form specifically though (promise it's not a video screenshot this time):

[Beamanator]

I think that should be WAQ agreed, and we can proceed with that in isolation for the time being.

Ok sweet I added this to WAQ

@trjExpensify let me know if there's something else we were discussing about inline errors vs not that you'd like a reply on, I'm a bit lost with that part of the convo sorry 😅

[Beamanator]

Unassigning myself to be able to put more focus on my other tasks - dropped a note in #engineering-chat to try to get more 👀

[neil-marcellini]

Hey @robertjchen I wasn't aware of this issue until I saw it in your Slack WAQ status. I'm currently reviewing this PR which is solving this issue.

[JmillsExpensify]

So should we put this issue on HOLD until your PR hits and then re-test? Or close this issue outright?

[robertjchen]

Oh nice, thanks for the heads up! I've added this GH issue to the list in the PR (which doesn't seem to be tied to an issue at the moment) and will re-test things once it goes out. Will add a HOLD for now 👍

[robertjchen]

PR went out, going to re-test and close after confirmation.

[robertjchen]

Looks great, closing!