[web] Display an error message when adding new user failed
ClosedPublic
Actions

Authored by tomek on Jun 27 2022, 8:02 AM.

Details

Reviewers

Commits

Summary

Catch an exception during relationship update and display error message in case of any failure.

Depends on D4315

Test Plan

Modify relationship responders to fail every second time and check if an error is displayed.

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

tomek requested review of this revision.Jun 27 2022, 8:07 AM

Nice! Some minor inline feedback, but looks good to me.

web/settings/relationship/add-users-list.react.js
192–203 ↗	(On Diff #13841)	Just so I'm clear, this set of lines merges the feedback given in the inline comment from D4315 about wrapping the call to `callUpdateRelationships` in a `try...catch` block with the new changes from this diff to set the error message upon an error, right? If so, maybe the `try...catch` change should be amended to that diff to address that feedback, and then the only change that needs to be made here is the `setErrorMessage` call to factor the changesets better. But either way, it is not a big deal.
193 ↗	(On Diff #13841)	Is this necessary? The initial value of `errorMessage` is `''`.
201 ↗	(On Diff #13841)	Aside: the `unknown error` message in `ThreadSettingsGeneralTab` has an underscore: {F84563} Not sure why the underscore is there. If it's necessary, though, it should be added in here.
224–227 ↗	(On Diff #13841)	I think you can simplify some of the logic here because values are by default `undefined` in JavaScript and we're just checking to see if `errorMessage` is falsy or not.

This revision is now accepted and ready to land.Jun 27 2022, 8:40 AM

web/settings/relationship/add-users-list.react.js
192–203 ↗	(On Diff #13841)	Yes, you're right - this diff is a response to that feedback. I added a comment there to avoid the confusion. Regarding updating that diff, I don't think it is a good idea. The goal of that diff is a single, well-defined thing - make sure that we don't close the modal when an error happens. The goal of this diff is to show a message when an error happens. Both goals stand on their own so merging them wouldn't be beneficial.
193 ↗	(On Diff #13841)	This handles the case when a user retries a request. I think we don't want to show this error after a button is clicked and before a response is received, just to avoid the user being confused.
201 ↗	(On Diff #13841)	There are a lot more places where `unknown error` is used and I can't think of a good reason why an underscore would be necessary. I guess the reason it is included in `ThreadSettingsGeneralTab` is that the server returns error codes with underscore and an author wanted to replicate that - but for me it looks like underscores shouldn't be used there.
224–227 ↗	(On Diff #13841)	Ok, that makes sense.

web/settings/relationship/add-users-list.react.js
192–203 ↗	(On Diff #13841)	Saw your comment right after I posted this, thanks! The goal of that diff is a single, well-defined thing - make sure that we don't close the modal when an error happens. The goal of this diff is to show a message when an error happens. Both goals stand on their own so merging them wouldn't be beneficial. 👍 Makes sense.
193 ↗	(On Diff #13841)	That makes sense! Thanks for clarifying. Then, shouldn't `setErrorMessage('')` also be added to `ThreadSettingsGeneralTab` (above the call to `callChangeThreadSettings`) as well? {F84616}