Confirm before unsubscribing (always, if from action sheet; only sometimes, if from "All channels") #1890
Conversation
|
In general, our pattern is to always show a privacy marker or (if not convenient) a |
…itle See Alya's feedback on zulip#1890: zulip#1890 (comment) > In general, our pattern is to always show a privacy marker or (if > not convenient) a `#` before a channel name. It can be a separate > PR, but can we add that to these confirmation dialogs? Doing this in code, rather than in the translators' source string, because we don't want it to vary by language.
There was a problem hiding this comment.
Thanks @chrisbobbe! LGTM, and tests great. Moving over to Greg's review.
Also I see there are some conflicts now that #1887 is merged.
rajveermalviya
added
integration review
There was a problem hiding this comment.
Thanks @chrisbobbe for taking care of this, and @rajveermalviya for testing it out!
Generally this all looks good; a few comments below.
In the new variant of the dialog, the body looks redundant to me:
Unsubscribe from production help?
Are you sure you want to unsubscribe?
Can we leave the body/message out and just let the title speak for it? (/cc @alya)
| return fetchedMessage?.content; | ||
| } | ||
|
|
||
| static Future<void> unsubscribeFromChannel(BuildContext context, { |
There was a problem hiding this comment.
This could use a bit of dartdoc, in particular to highlight that it may show a confirmation message.
Otherwise it sounds like it just unconditionally unsubscribes (and then if that request fails shows an error dialog, in common with other ZulipAction methods).
| await tester.tap(find.byWidget(cancelButton)); | ||
| await tester.pump(); | ||
| check(connection.lastRequest).isNull(); |
There was a problem hiding this comment.
| await tester.tap(find.byWidget(cancelButton)); | |
| await tester.pump(); | |
| check(connection.lastRequest).isNull(); | |
| await tester.tap(find.byWidget(cancelButton)); | |
| await tester.pump(duration: Duration.zero); | |
| check(connection.lastRequest).isNull(); |
Or perhaps better yet, pumpAndSettle().
Otherwise if we were to have a bug where "cancel" didn't work and instead resulted in going ahead and making the request, I think this test could easily be defeated if the request happened to come only after an await somewhere.
| await ZulipAction.unsubscribeFromChannel(context, | ||
| channelId: channel.streamId, | ||
| // Only show the confirmation dialog when you wouldn't be able to | ||
| // resubscribe. | ||
| alwaysAsk: false); |
There was a problem hiding this comment.
As a bonus, the above-mentioned dartdoc should obviate the need for this comment.
…itle See Alya's feedback on zulip#1890: zulip#1890 (comment) > In general, our pattern is to always show a privacy marker or (if > not convenient) a `#` before a channel name. It can be a separate > PR, but can we add that to these confirmation dialogs? Doing this in code, rather than in the translators' source string, because we don't want it to vary by language.
…itle See Alya's feedback on zulip#1890: zulip#1890 (comment) > In general, our pattern is to always show a privacy marker or (if > not convenient) a `#` before a channel name. It can be a separate > PR, but can we add that to these confirmation dialogs? Doing this in code, rather than in the translators' source string, because we don't want it to vary by language.
Fixes zulip#1827. (That is, always confirm when unsubscribing via the action sheet -- I've added a test that we *don't* always confirm in the "All channels" page, since that could be annoying, as mentioned in the issue.)
chrisbobbe
force-pushed
the
pr-more-unsubscribe-dialogs
branch
from
156f4d4 to
912f243
Compare
|
Thanks for the review! Revision pushed, this time atop #1893 which I've just updated. |
|
Re: #1890 (review) , here's what the dialog looks like with just the title, no body text:
|
|
Thanks! Looks good; merging. |
Stacked atop #1887.
Fixes #1827.
Fixes #1878.