Show alert dialog when ActionSheet action fails #3795

agrawal-d · 2020-01-13T12:40:36Z

An alert dialog is now shown, instead of silently ignoring the error.

Attached below is a screenshot showing this change:

The dialog is similar to what is shown when editing a message fails:

jainkuniya · 2020-01-13T20:03:56Z

src/message/messageActionSheet.js

@@ -58,7 +58,9 @@ const deleteMessage = async ({ auth, message, dispatch }) => {
  if (isAnOutboxMessage(message)) {
    dispatch(deleteOutboxMessage(message.timestamp));
  } else {
-    api.deleteMessage(auth, message.id);
+    api.deleteMessage(auth, message.id).catch((err: Error) => {


A good follow up of this will be to make sure this message is translated based on user selected language (also for fail to edit message).

Can you please tell me how I can do that? The /howto/translations.md does not explain much.

agrawal-d · 2020-01-15T09:46:42Z

Thanks for the review @ray-kraesig . Have made some changes.

Now works for all possible actions, and all errors are handled in a single place.

A generic message is shown as the alert title, while the alert body message itself is extracted from the error thrown.

@ray-kraesig please review.

rk-for-zulip

Localization was mentioned earlier; @gnprice will have to confirm, but I think all that's needed for that is to:

wrap the new string(s) in an _ call (see, for reference, the showToast call in copyToClipboard, and the eventual use of the .title string); and
add the relevant strings to messages_en.json.

(That would probably best go in a separate commit, though, and possibly even a separate PR if I'm wrong about how involved it is; don't let it block you.)

rk-for-zulip · 2020-01-16T02:17:40Z

src/message/messageActionSheet.js

@@ -12,7 +12,7 @@ import { navigateToMessageReactionScreen } from '../nav/navActions';
 // TODO really this belongs in a libdef.
 export type ShowActionSheetWithOptions = (
  { options: string[], cancelButtonIndex: number },
-  (number) => void,
+  (number) => Promise<void>,


This isn't our type to modify, unfortunately – as the comment notes, it shouldn't really be here at all. (Also, this modification results in a Flow error.)

On the other hand, ButtonDescription (seen below) is ours.

rk-for-zulip · 2020-01-16T02:25:48Z

src/message/messageActionSheet.js

+        ...params,
+      });
+    } catch (err) {
+      Alert.alert('Couldnt perform the requested action', err.message);


Typo: Couldn't (although preferentially Could not).

That said, it's probably worth the trouble to do better than a generic Could not perform the requested action here. We can associate a failure message with each action (Could not delete message, etc.) the same way we're associating a title.

I have replaced the message with a simple 'Error'. This will not be confusing because the alert message contains context-specific detailed message.

Counterpoint:

(Edit: This is actually the worst-case scenario, because this class of failure is often delayed: the user may not see this message until several seconds later.)

agrawal-d · 2020-01-16T08:48:18Z

Thanks for reviewing, @ray-kraesig. Have made some changes.

agrawal-d · 2020-01-16T08:49:26Z

(#3795 (comment))

agrawal-d · 2020-01-17T02:30:28Z

@ray-kraesig , so how do you propose we do it?

Hardcode the alert titles for each action?
Or generate it, for e.g. The action ${button.title} failed?

agrawal-d · 2020-01-17T05:23:52Z

This looks better:

@ray-kraesig have pushed come changes.

rk-for-zulip

@ray-kraesig , so how do you propose we do it?

Hardcode the alert titles for each action?

Or generate it, for e.g. The action ${button.title} failed?

Only the first is an option. Even if you try to do the second, you'll end up having to add all the individual generated strings to the translation databases.

I've since confirmed that the two steps I noted previously are all that's necessary for localization in most PRs, incidentally:

wrap the new string(s) in an _ call (see, for reference, the showToast call in copyToClipboard, and the eventual use of the .title string); and

add the relevant strings to messages_en.json.

rk-for-zulip · 2020-01-21T00:06:43Z

src/message/messageActionSheet.js

+          ...params,
+        });
+      } catch (err) {
+        const titleString = allButtons[optionCodes[buttonIndex]].title;


It's harmless right now, but it's probably better to cache allButtons[optionCodes[buttonIndex]] before the await is performed.

(allButtons shouldn't ever be modified – but if it is, we might end up accessing different values of allButtons before and after the await, if two actions are performed in quick succession before the first one fails.)

Not being used in the latest change at all, so haven't cached it.

agrawal-d · 2020-01-21T14:59:53Z

@ray-kraesig have pushed some changes. Please review.

rk-for-zulip

Doesn't the callback have a closure over allButtons[optionCodes[buttonIndex]], so that even if another action is performed successively, the previous callback isn't affected?

Terminological nit: the callback is a closure. It closes over the variables (not values!) allButtons and optionCodes, and has buttonIndex as a parameter.

optionCodes is local to showActionSheet, and so will be a different variable for each instance of the callback. allButtons, on the other hand, is global to the module, and anyway is a refernce to an array; if either the variable or the array are modified while we're awaiting here, the first evaluation of allButtons[optionCodes[buttonIndex]] needn't yield the same value as the second.

(Of course, in the current codebase, it shouldn't ever be modified. But code does change.)

rk-for-zulip · 2020-01-24T01:59:55Z

static/translations/messages_en.json

+  "Failed to add reaction":"Failed to add reaction",
+  "Failed to reply":"Failed to reply",
+  "Failed to copy message to clipboard":"Failed to copy message to clipboard",
+  "Failed to share message":"Failed to share message",
+  "Failed to edit message":"Failed to edit message",
+  "Failed to delete message":"Failed to delete message",
+  "Failed to star message":"Failed to star message",
+  "Failed to unstar message":"Failed to unstar message",
+  "Failed to show reactions":"Failed to show reactions",
+  "Failed to unmute topic":"Failed to unmute topic",
+  "Failed to mute topic":"Failed to mute topic",
+  "Failed to mute stream":"Failed to mute stream",
+  "Failed to unmute stream":"Failed to unmute stream",


Spaces after the colons here, again.

Im sorry, I am used to the prettier taking care of formatting, but I guess it doesn't add space after colons. Will keep this in mind in the future.

prettier is intended for code formatting, and not necessarily data formatting. We don't let it touch .json files: those are typically both generated and consumed by tooling.

rk-for-zulip · 2020-01-24T02:05:47Z

src/message/messageActionSheet.js

+const errorMessages = [
+  'Failed to add reaction',
+  'Failed to reply',
+  'Failed to copy message to clipboard',
+  'Failed to share message',
+  'Failed to edit message',
+  'Failed to delete message',
+  'Failed to star message',
+  'Failed to unstar message',
+  'Failed to show reactions',
+  'Failed to unmute topic',
+  'Failed to mute topic',
+  'Failed to mute stream',
+  'Failed to unmute stream',
+];


There's nothing keeping this lined up with the actions themselves. If your earlier delete topic PR were applied, for example, this would be suddenly and quietly broken.

It would be safer to associate actions with failure messages in the same way they're associated with titles, as I suggested earlier.

agrawal-d · 2020-01-24T07:28:30Z

Thank you for the review, @ray-kraesig. I have (hopefully) made the changes your requested, and added a commit message.

I have not added any documentary comments in the code, because it's a simple self-explanatory change; I did not feel that this change needs to be documented, but if required, I will add some.

Please review these changes, thanks.

rk-for-zulip

I have not added any documentary comments in the code, because it's a simple self-explanatory change; I did not feel that this change needs to be documented, but if required, I will add some.

Terminology nitpick: The change does need to be documented – in the commit summary. (And your commit summary does that, so all's well there.)

Can you please give me an example of how you expect the alert titles to be associated, because I am unable to understand what we want here. Thanks.

An example is present in this file: the .title field of ButtonDescription.

              type ButtonDescription = {
                // ...
/* --> */       title: string,
              };

              const reply = /* ... */;
/* --> */     reply.title = 'Reply';

I Hardcoded the error titles because you mentioned that this is expected in a comment. But I guess you meant something else.

Hardcoding the error titles is expected. The issue with the last version was putting the hardcoded titles in their own array with no explicit connection to the actions they're associated with.

rk-for-zulip · 2020-01-25T00:34:15Z

src/message/messageActionSheet.js

+    (async () => {
+      const pressedButton: ButtonDescription = allButtons[optionCodes[buttonIndex]];
+      try {
+        await allButtons[optionCodes[buttonIndex]]({


pressedButton, surely?

rk-for-zulip · 2020-01-25T00:44:33Z

src/message/messageActionSheet.js

+      } catch (err) {
+        Alert.alert(_(`Failed to ${pressedButton.title.toLowerCase()}`), err.message);
+      }


This is the approach I specifically warned against. It is much too easy to add an action to the action-sheet and its description to the localization database without noticing that the action's failure message needs to be considered as well. (It's much too easy in general to fail to localize UI strings, but that's no excuse to make it even easier.)

agrawal-d · 2020-01-30T12:02:20Z

Thanks for the review, @ray-kraesig. Have made some changes.

The errorMessage property in type ButtonDescription is optional because the callback associated with the button Cancel ( and possibly other buttons we may add in the future ) can not throw an error.

Please review these changes, thanks!

rk-for-zulip

The ideal solution here would be to declare a callback as possibly having the quality of not throwing exceptions ("being nothrow"), and then tie the necessity for an .errorMessage to whether or not the callback was nothrow. There are type systems that can do that. Sadly, Flow is not one of them.

The most straightforward solution here would be to make .errorMessage required, and give it a dummy error message (but a reasonable one, if somehow displayed). This would require at least one explanatory comment.

A precise solution here would be to make "Cancel" a special object that doesn't even have a callback to be invoked, and so is guaranteed not to throw because of that. This would a) come with more pitfalls, b) require more explanatory comments, c) need to be broken out into its own commit, and d) not be trivially extensible to other nothrow functions. (Useful provably-nothrow functions are very rare in JavaScript, and most of them don't return void; I don't think d) is a blocker.)

The errorMessage property in type ButtonDescription is optional because the callback associated with the button Cancel ( and possibly other buttons we may add in the future ) can not throw an error.

(If this were an acceptable tradeoff, it would have to be noted in inline comments.)

There is a minor typo (extra space) in the commit message. The commit summary is also a few too many characters long for GitHub to be happy with it. (Our current documented limit is 76, but perhaps we should reduce this slightly.)

rk-for-zulip · 2020-01-30T18:15:10Z

src/message/messageActionSheet.js

@@ -33,6 +33,7 @@ type ButtonDescription = {
    _: GetText,
  }): void | Promise<void>,
  title: string,
+  errorMessage?: string,


This means that it's possible to accidentally omit errorMessage – or misspell it as error_message and not notice.

rk-for-zulip · 2020-01-30T18:19:05Z

src/message/messageActionSheet.js

+        if (pressedButton.errorMessage !== undefined) {
+          Alert.alert(_(pressedButton.errorMessage), err.message);
+        }


This means that if an unexpected failure occurs in a callback that doesn't have an associated .errorMessage (for whatever reason), that failure will be silent: neither the user nor we will ever hear about it.

agrawal-d · 2020-01-30T19:42:51Z

Thanks for the review, @ray-kraesig! I went ahead with the straightforward solution you mentioned. Please review, thanks!

The commit summary is also a few too many characters long for GitHub to be happy with it. (Our current documented limit is 76, but perhaps we should reduce this slightly.)

Probably because of the 'Verified' commit badge, as I have added a GPG key to my GitHub account. I have shortened the commit summary regardless.

`toggleMessageStarred` immediately returns void, not reporting the success or failure of its async operation to its caller. Instead, propagate its implementation's Promise<ApiResponse> upward.

Currently, there is no error handling for any of the actions performed through messageActionSheet. Add error handling for all actions, and show alert dialog if any action fails. [Comments amended by Ray Kraesig.] Fixes zulip#3788.

rk-for-zulip

To avoid another go-round (which would probably be more trouble than it's worth), I've taken care of both of the issues I've mentioned here.

rk-for-zulip · 2020-01-31T23:49:21Z

src/message/messageActionSheet.js

+// In practice, the callback for `cancel` button will never throw
+// an error, and the `errorMessage` property never be used, but it
+// has still been added to ensure that `errorMessage` property can
+// be made required for the type `ButtonDescription`.


cancel is not the only callback for which this is true: addReaction, showReactions, and shareMessage also don't throw (though this is implementation-detail-dependent).

It also immediately raises the question of why errorMessage has been made required, which (as evidenced by the fact that we had to have the previous discussion) is not obvious.

rk-for-zulip · 2020-02-01T00:35:41Z

src/message/messageActionSheet.js

-const starMessage = ({ auth, message }) => {
-  api.toggleMessageStarred(auth, [message.id], true);
+const starMessage = async ({ auth, message }) => {
+  await api.toggleMessageStarred(auth, [message.id], true);


It's not obvious, but this is awaiting on void.

(That's really a bug in toggleMessageStarred, though.)

rk-for-zulip · 2020-02-01T00:43:17Z

– and merged. Thanks!

agrawal-d mentioned this pull request Jan 13, 2020

No error message is shown when deletion fails #3788

Closed

agrawal-d force-pushed the deleting-messages branch from 06da9fe to f1e3d40 Compare January 13, 2020 14:14

jainkuniya reviewed Jan 13, 2020

View reviewed changes

agrawal-d force-pushed the deleting-messages branch from f1e3d40 to 22d6782 Compare January 15, 2020 09:44

agrawal-d requested a review from rk-for-zulip January 15, 2020 09:48

rk-for-zulip reviewed Jan 16, 2020

View reviewed changes

agrawal-d force-pushed the deleting-messages branch from 22d6782 to af34c69 Compare January 16, 2020 08:46

agrawal-d requested a review from rk-for-zulip January 16, 2020 08:48

agrawal-d force-pushed the deleting-messages branch from af34c69 to 10a5cfc Compare January 17, 2020 05:25

rk-for-zulip reviewed Jan 21, 2020

View reviewed changes

rk-for-zulip mentioned this pull request Jan 21, 2020

action sheet: add and implement delete topic option #3824

Merged

agrawal-d force-pushed the deleting-messages branch from 10a5cfc to 29084bf Compare January 21, 2020 14:58

agrawal-d requested a review from rk-for-zulip January 21, 2020 14:58

agrawal-d force-pushed the deleting-messages branch from 29084bf to 51df1db Compare January 22, 2020 07:16

agrawal-d changed the title ~~Show alert dialog when deleting message fails~~ Show alert dialog when action sheet action fails Jan 22, 2020

agrawal-d changed the title ~~Show alert dialog when action sheet action fails~~ Show alert dialog when ActionSheet action fails Jan 22, 2020

rk-for-zulip reviewed Jan 24, 2020

View reviewed changes

agrawal-d force-pushed the deleting-messages branch 2 times, most recently from cad0519 to d88e276 Compare January 24, 2020 07:25

agrawal-d force-pushed the deleting-messages branch from d88e276 to 4091dd6 Compare January 24, 2020 17:49

agrawal-d requested a review from rk-for-zulip January 25, 2020 13:04

rk-for-zulip reviewed Jan 30, 2020

View reviewed changes

agrawal-d force-pushed the deleting-messages branch from 4091dd6 to d326a94 Compare January 30, 2020 11:55

agrawal-d mentioned this pull request Jan 30, 2020

Modify/don't provide 'Reply' option for messages that can't be replied to #3811

Merged

rk-for-zulip reviewed Jan 30, 2020

View reviewed changes

agrawal-d force-pushed the deleting-messages branch 3 times, most recently from ac797c4 to 9c945ed Compare January 30, 2020 19:36

agrawal-d requested a review from rk-for-zulip January 30, 2020 19:42

rk-for-zulip and others added 2 commits January 31, 2020 15:14

api: Propagate async response through toggleMessageStarred.

25fce97

`toggleMessageStarred` immediately returns void, not reporting the success or failure of its async operation to its caller. Instead, propagate its implementation's Promise<ApiResponse> upward.

rk-for-zulip reviewed Feb 1, 2020

View reviewed changes

rk-for-zulip force-pushed the deleting-messages branch from 9c945ed to b327b85 Compare February 1, 2020 00:42

rk-for-zulip merged commit fc660d6 into zulip:master Feb 1, 2020

agrawal-d mentioned this pull request Apr 13, 2020

message edit: Fix error msg is not shown when msg edit fails. #2871

Closed

agrawal-d deleted the deleting-messages branch April 13, 2020 15:52

Show alert dialog when ActionSheet action fails #3795

Show alert dialog when ActionSheet action fails #3795

Conversation

agrawal-d commented Jan 13, 2020 • edited Loading

Choose a reason for hiding this comment

agrawal-d Jan 14, 2020 • edited Loading

Choose a reason for hiding this comment

agrawal-d commented Jan 15, 2020 • edited Loading

rk-for-zulip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrawal-d Jan 16, 2020 • edited Loading

Choose a reason for hiding this comment

rk-for-zulip Jan 16, 2020 • edited Loading

Choose a reason for hiding this comment

agrawal-d commented Jan 16, 2020

agrawal-d commented Jan 16, 2020 • edited Loading

agrawal-d commented Jan 17, 2020 • edited Loading

agrawal-d commented Jan 17, 2020 • edited Loading

rk-for-zulip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrawal-d commented Jan 21, 2020

rk-for-zulip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrawal-d commented Jan 24, 2020 • edited Loading

rk-for-zulip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrawal-d commented Jan 30, 2020

rk-for-zulip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrawal-d commented Jan 30, 2020 • edited Loading

rk-for-zulip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rk-for-zulip commented Feb 1, 2020

agrawal-d commented Jan 13, 2020 •

edited

Loading

agrawal-d Jan 14, 2020 •

edited

Loading

agrawal-d commented Jan 15, 2020 •

edited

Loading

agrawal-d Jan 16, 2020 •

edited

Loading

rk-for-zulip Jan 16, 2020 •

edited

Loading

agrawal-d commented Jan 16, 2020 •

edited

Loading

agrawal-d commented Jan 17, 2020 •

edited

Loading

agrawal-d commented Jan 17, 2020 •

edited

Loading

agrawal-d commented Jan 24, 2020 •

edited

Loading

agrawal-d commented Jan 30, 2020 •

edited

Loading