model: Notify user if a request fails. #714

kaustubh-nair · 2020-07-10T11:44:25Z

This commit displays an error message in the footer for server requests
involving user interactions. By this the user can be notified for
different failures like:

Hitting API rate limits.
Posting in streams where user doesn't have the required permissions.
Message sending/editing errors.
Toggling reaction errors.
etc.

Error responses from some requests are not displayed, since they are not
triggered directly by a user action. Example of these include event
polling requests, event registration, and updating presence.

Fixes #427.

sumanthvrao · 2020-07-12T04:44:15Z

@kaustubh-nair This looks fairly straightforward as you mentioned. 👍

One suggestion maybe (not sure if we already had a discussion of this) whether we can display these kinds of error messages in a different way than regular help tips ones.

neiljp

@kaustubh-nair This is definitely beneficial in terms of alerting the user to issues and so far nicely simple, if repetitive.

Have you tested this? How?

Does the same issue happen here as in another PR, namely if we get lots of failures, the repeated footer updates lead to subsequent rapid help tip updates?

There may be scope to include the server registration aspects too - these can fail too!

While we do have controller/view calls from the model already, I'm a little wary of having so many calls to controller methods in the model - the alternative is to update the calling points to update the view themselves, based on the return values which we already have in many cases.

neiljp · 2020-07-12T06:14:15Z

zulipterminal/model.py

+                if response['result'] == 'error':
+                    self.controller.view.set_footer_text(response['msg'], 3)


Given the apparent similarity in all this error handling, please use a new method for this.

Putting this in a function should make testing simple?

kaustubh-nair · 2020-07-14T14:35:48Z

@neiljp thanks for the review.
Re. displaying error for server registration - the problem is that the view would not load when it is called. Even if we wait for it to load, I'm not sure what happens if initial_data is not retrieved....would zt even show anything?

neiljp · 2020-07-14T16:28:01Z

@kaustubh-nair I agree that if initial data does not exist then we cannot easily show an error message in the UI, though we could extend the startup error handling via exceptions for that case. If we do already have initial data then we can notify of connection failure, which could occur in the same method, but possibly more in poll_for_events, ie. checking the return value.

kaustubh-nair · 2020-07-14T18:00:09Z

Opened #716

Controller.client is already mocked for all tests in a pytest fixture.

Move the return value patch for client.update_subscription_status to a default argument, so that it can be used again later.

This commit displays an error message in the footer for server requests involving user interactions. By this the user can be notified for different failures like: * Hitting API rate limits. * Posting in streams where user doesn't have the required permissions. * Message sending/editing errors. * Toggling reaction errors. * etc. Error responses from some requests are not displayed, since they are not triggered directly by a user action. Example of these include event polling requests, event registration, and updating presence. Tests added. Fixes zulip#427.

preetmishra · 2020-07-15T14:43:24Z

tests/model/test_model.py

@@ -535,11 +535,10 @@ def test_fail_get_messages(self, mocker, error_response,
    ], ids=['muting_205', 'unmuting_205', 'first_muted_205',
            'last_unmuted_205'])
    def test_toggle_stream_muted_status(self, mocker, model,
-                                        initial_muted_streams, value):
+                                        initial_muted_streams, value,
+                                        response={'result': 'success'}):


Is this to overcome pytest's caching issue or just to reuse the dict?

Just for reuse

sumanthvrao

@kaustubh-nair These changes look good now.

One aspect which @neiljp mentioned as well, is if there was a way to manually test this?

kaustubh-nair · 2020-07-24T06:37:37Z

@sumanthvrao @neiljp, I haven't tested every single one of these, but some of them can be tested like

Holding down * to get rate limited.
Holding down down to get rate limited.
Trying to post in #announce without permissions.
Trying to send a message to a stream that doesn't exist.

neiljp · 2020-07-30T21:39:50Z

@kaustubh-nair As per PMs, I just merged the last two commits together - thanks for working on this! 🎉

Time to update the release notes again 👍

zulipbot added size: S [Automatic label added by zulipbot] enhancement New feature or request labels Jul 10, 2020

neiljp reviewed Jul 12, 2020

View reviewed changes

kaustubh-nair force-pushed the notify-user-on-rate-limit branch 2 times, most recently from ab56996 to 0240e28 Compare July 14, 2020 15:25

zulipbot added size: M [Automatic label added by zulipbot] and removed size: S [Automatic label added by zulipbot] labels Jul 14, 2020

tests: model: Remove redundant patch in star status test.

33c2be3

Controller.client is already mocked for all tests in a pytest fixture.

kaustubh-nair force-pushed the notify-user-on-rate-limit branch from 0240e28 to e9b352d Compare July 14, 2020 18:58

zulipbot added size: L [Automatic label added by zulipbot] and removed size: M [Automatic label added by zulipbot] labels Jul 14, 2020

kaustubh-nair added 2 commits July 15, 2020 01:20

tests: model: refactor: Use a variable for client response.

3460187

Move the return value patch for client.update_subscription_status to a default argument, so that it can be used again later.

kaustubh-nair force-pushed the notify-user-on-rate-limit branch from e9b352d to f1e98f5 Compare July 14, 2020 19:51

kaustubh-nair added the PR needs review PR requires feedback to proceed label Jul 14, 2020

preetmishra reviewed Jul 15, 2020

View reviewed changes

sumanthvrao approved these changes Jul 24, 2020

View reviewed changes

neiljp closed this Jul 30, 2020

neiljp added this to the 0.5.2 milestone Jul 30, 2020

kaustubh-nair mentioned this pull request Jul 31, 2020

Indicate if a user-activated operation failed #620

Closed

imnitishng mentioned this pull request Dec 18, 2020

Restrict sending messages to announcement streams #682

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

model: Notify user if a request fails. #714

model: Notify user if a request fails. #714

Uh oh!

kaustubh-nair commented Jul 10, 2020

Uh oh!

sumanthvrao commented Jul 12, 2020

Uh oh!

neiljp left a comment

Uh oh!

neiljp Jul 12, 2020

Uh oh!

kaustubh-nair commented Jul 14, 2020

Uh oh!

neiljp commented Jul 14, 2020

Uh oh!

kaustubh-nair commented Jul 14, 2020

Uh oh!

preetmishra Jul 15, 2020

Uh oh!

kaustubh-nair Jul 15, 2020

Uh oh!

sumanthvrao left a comment

Uh oh!

kaustubh-nair commented Jul 24, 2020 •

edited

Loading

Uh oh!

neiljp commented Jul 30, 2020

Uh oh!

Uh oh!

		if response['result'] == 'error':
		self.controller.view.set_footer_text(response['msg'], 3)

Uh oh!

model: Notify user if a request fails. #714

model: Notify user if a request fails. #714

Uh oh!

Conversation

kaustubh-nair commented Jul 10, 2020

Uh oh!

sumanthvrao commented Jul 12, 2020

Uh oh!

neiljp left a comment

Choose a reason for hiding this comment

Uh oh!

neiljp Jul 12, 2020

Choose a reason for hiding this comment

Uh oh!

kaustubh-nair commented Jul 14, 2020

Uh oh!

neiljp commented Jul 14, 2020

Uh oh!

kaustubh-nair commented Jul 14, 2020

Uh oh!

preetmishra Jul 15, 2020

Choose a reason for hiding this comment

Uh oh!

kaustubh-nair Jul 15, 2020

Choose a reason for hiding this comment

Uh oh!

sumanthvrao left a comment

Choose a reason for hiding this comment

Uh oh!

kaustubh-nair commented Jul 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neiljp commented Jul 30, 2020

Uh oh!

Uh oh!

kaustubh-nair commented Jul 24, 2020 •

edited

Loading