Handle 666 errors in Policy.removeMembers #7229
Conversation
Maybe we can loop in @Expensify/design to help us with this one. We don't really have a clear guidance for how to handle server errors in non-forms. Seems like there are a couple of options:
Here's a quick attempt at A button that triggers an API request should show a loader like we do for a form
If the request doesn't not succeed we should remove the option to repeat this action (just a thought but I can see not removing it as well) and replace it with a message
|
|
That looks fine, but would that mean we would block any actions while the request is being processed? I don't quite like that. |
Not sure if I understand the meaning of a "popup", but sounds kind of like you are suggesting an inline Growl. What makes the error popup go away? Is it time-based like the Growl? Can or should it be manually dismissed? Should we leave it there until the user does something else?
|
Yeah, like a growl with a small speech bubble pointing to the button that triggered the action. You would manually dismiss it by clicking on it or it can have a close button somewhere. I would guess that interacting again with the button would dismiss it too. |
|
Something similar to this but pointed at the button?
|
How do we currently handle server errors in forms? |
Yeah |
That's explained in this section: https://docs.google.com/document/d/1DI4jQ0nyfi2JczhPT440orwlXsTP_O-aGvg-HLmcbwI/edit#bookmark=id.odca5qx1yozc |
|
We add them to the bottom of the screen above the submit button. We could do the same here, but we are exploring alternatives because this is not a form. |
|
Well just because it's not a "form" doesn't mean we can't do something similar right? Personally I really like what you outlined in your comment here. |
Let's explore whether it works or not. The "form" gives the user some indication that things are loading in a button and then we show server errors above this button once the loading is done. Without the submit button the error message would just pop up at the bottom of the screen. Which does not seem much better than a Growl and also adds the question of how long the message should persist on screen. Probably this conversation is getting a little deep for this particular issue - but if we don't want to use a Growl then it would be nice to settle on various patterns and have a bunch of common UI error handling scenarios to pull from or advise people to implement when this topic comes up. |
|
Also this being a list which can (and probably will) be long, showing the error on the bottom is probably not the best, because it's possible it's out of the user's view... |
|
What about the two mockups you posted in your comment above? It seems like that would be a great way to handle this? Essentially the "form" is encapsulated into the small bottom-docked modal. |
|
I like it, but @iwiznia pointed out here that maybe this action should have an optimistic UI i.e. should the user wait to find out if it was successful and be aware of the "loading" state? Or should they simply see the change happen, then get rolled back, then see some indication that it was not successful. |
|
Got it, I hear ya. I think most of the time I agree with wanting an optimistic UI. But I can see an argument that we don't need it when taking some kind of delete/remove action like this. Otherwise I guess we could optimistically take the action, close out the modal, and then relaunch the modal with an error message if we have one. |
|
@iwiznia Can we:
|
|
Sure |
|
Not up to me though, @AndrewGable is tagged as reviewer and did not approve |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by @AndrewGable in version: 1.1.38-0 🚀
|
|
@flodnv We are unable to find ""new member's domain group restrict policy to the workspace" in the OldDot. Can you please confirm where in the profile this settings should appear? |
|
This wild be in Domain Control > Groups
…On Thu, Feb 10, 2022, 16:49 Maria Ajmera ***@***.***> wrote:
@flodnv <https://github.com/flodnv> We are unable to find ""new member's
domain group restrict policy to the workspace" in the OldDot. Can you
please confirm where in the profile this settings should appear?
—
Reply to this email directly, view it on GitHub
<#7229 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA7Y2OINHLUDWLVDYSTM4ALU2PUDNANCNFSM5L7JLATA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@flodnv Here is what we see in the Groups. What options out of these restricts Workspace? |
|
Hey @flodnv, could you confirm which switch is the correct one please |
|
Ah sorry about that, it's the last setting for "preferred policy"
…On Sat, Feb 12, 2022 at 2:27 PM Maria Ajmera ***@***.***> wrote:
@flodnv <https://github.com/flodnv> Here is what we see in the Groups.
What options out of these restricts Workspace?
[image: Screen Shot 2022-02-12 at 1 24 50 PM]
<https://user-images.githubusercontent.com/43995119/153723546-30c470ce-230f-4ea6-80af-ed12de43ed91.png>
When we tried to enable "Restrict expense policy creation/removal" It
blocked us from creating any workspaces
—
Reply to this email directly, view it on GitHub
<#7229 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA7Y2OKXSM4RLJ43KYAG2YTU22RC5ANCNFSM5L7JLATA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
We are checking |
|
@flodnv Can you please double check the issue above. We are seeing different error in PROD, but I assume its expected. Pls confirm it doesnt need to be a deploy blocker |
|
That's definitely unrelated to this PR. Thanks! |
|
🚀 Deployed to production by @Julesssss in version: 1.1.38-3 🚀
|
cc @iwiznia @marcaaron related to an internal thread here: https://expensify.slack.com/archives/C03TQ48KC/p1642178947340500
Details
This will give more accurate error messages instead of always the generic one.
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/191023
Tests
QA Steps
Tested On
Screenshots
Web
See above
Mobile Web
Desktop
iOS
Android