login: Support logging out of an account (take 2) #1010

chrisbobbe · 2024-10-19T00:41:28Z

These are the three last commits from #948, following up on the just-merged #1007 to replace #948: #948 (review)

The screenshots in #948's description still match this code. As I said on #948, there might be some improvements to make to the UI, perhaps as a followup: #948 (comment)

Fixes: #463

gnprice · 2024-10-19T01:28:24Z

lib/widgets/app.dart

    return Card(
      clipBehavior: Clip.hardEdge,
      child: ListTile(
        title: title,
        subtitle: subtitle,
+        trailing: PopupMenuButton<AccountItemOverflowMenuItem>(


It looks like the PopupMenuButton doc recommends using the newer MenuAnchor instead. What if we used that — how would it differ?

PopupMenuButton MenuAnchor

It's not much different, but I'm happy to use the more modern thing. 🤷‍♂️

Greg spotted this opportunity to be a bit more modern: zulip#1010 (comment)

chrisbobbe · 2024-10-25T01:38:45Z

Thanks for the review! Revision pushed.

I've added a commit to update the existing place we use PopupMenuButton so it uses MenuAnchor instead:

app: Update three-dots button to use new M3 MenuAnchor widget

Before	After

gnprice

Thanks! Generally this looks good. Comments below from a full review.

@alya see question below about wording for a confirmation modal.

Then one other thing this should have before we merge it is some tests, specifically for the last commit. This is kind of tricky code, and it's the sort of feature that we infrequently exercise but that is important to people when they do use it — so that makes tests extra useful.

gnprice · 2024-10-25T23:18:30Z

lib/widgets/app.dart

+        return IconButton(
+          tooltip: materialLocalizations.showMenuTooltip, // "Show menu"
+          onPressed: () {
+            if (controller.isOpen) {
+              controller.close();
+            } else {
+              controller.open();
+            }
+          },
+          icon: Icon(Icons.adaptive.more, color: designVariables.icon));


Seems annoying that with the new API we have to recite all this standard stuff. But so be it. If we find this coming up more, we can abstract it out.

gnprice · 2024-10-25T23:48:43Z

lib/widgets/app.dart

+            MenuItemButton(
+              onPressed: () => unawaited(_logOutAccount(context, accountId)),
+              child: Text(zulipLocalizations.chooseAccountPageLogOutButton)),


Before actually logging the user out, let's have a confirmation modal. This is something that would be pretty annoying to do by accident.

Some draft text for the modal:

Log out?

This will sign you out on this device from this Zulip account.

(buttons:) Log out / Cancel

with red "destructive action" styling on the "Log out" button.

Or I guess here's what we have in zulip-mobile:

showConfirmationDialog({ destructive: true, title: 'Remove account', message: { text: 'This will make the mobile app on this device forget {email} on {realmUrl}.', values: { realmUrl: realm.toString(), email },

so we could always just copy that.

@alya, do you have feedback on the possible confirmation text?

red "destructive action" styling

Sure. I've just filed #1032 for this; how about I leave this part as a TODO(#1032) in my next revision?

Hm, maybe we should try to explain the impact on the user a bit more? Something like this, assuming it's accurate?

Log out?
To use this account in the future, you will have to re-enter the URL for your organization and your account information.
(buttons:) Log out / Cancel

gnprice · 2024-10-25T23:59:39Z

lib/widgets/app.dart

+  Future<void> _logOutAccount(BuildContext context, int accountId) async {
+    final globalStore = GlobalStoreWidget.of(context);
+
+    final account = globalStore.getAccount(accountId);
+    if (account == null) return; // TODO(log)
+
+    // Unawaited, to not block removing the account on this request.
+    unawaited(_unregisterToken(globalStore, account));
+
+    await globalStore.removeAccount(accountId);
+  }
+
+  Future<void> _unregisterToken(GlobalStore globalStore, Account account) async {


From a code-organization perspective, this class and this file don't feel like the right home for these two methods — they're not really part of this UI, but rather the logic for an action the user might take, where this page just happens to be the initial home of the only place to reach that action in the UI. (It might not always be — for example in a future settings page, we might have a button to log out.)

Probably widgets/actions.dart would be good.

I also considered widgets/login.dart , since it's logically related to the things that happen there. But it doesn't really fit with any of the particular UI widgets there.

Ultimately they likely belong as methods on GlobalStore. But I wouldn't want to put them in store.dart, which is too crowded as it is — they'd be defined elsewhere and mixed in, akin to ChannelStore and its various accessor methods. So I think actions.dart is a fine home for them at least until then.

So I think actions.dart is a fine home for them at least until then.

Cool, yeah, that does look like a good place for it.

gnprice · 2024-10-26T00:11:31Z

lib/widgets/app.dart

+      await NotificationService.unregisterToken(connection, token: token);
+    } catch (e) {
+      // TODO retry? handle failures?
+    } finally {


Definitely post-launch to do anything about that — zulip-mobile doesn't do anything about failures here either.

Greg spotted this opportunity to be a bit more modern: zulip#1010 (comment)

chrisbobbe · 2024-11-09T06:08:49Z

Thanks for the review! Revision pushed, this time with a bunch of tests. 🙂

gnprice

Thanks for the revision! Generally this all looks good; just small comments below.

Would you also post a screenshot of the new confirmation dialog?

gnprice · 2024-11-13T00:15:45Z

test/widgets/actions_test.dart

+
+      final future = logOutAccount(context, eg.selfAccount.id);
+      // Unregister-token request and account removal dispatched together
+      assertSingleUnregisterRequest(newConnection);


nit: this function wraps a check call, so use the verb "check" to name it:

Suggested change

assertSingleUnregisterRequest(newConnection);

checkSingleUnregisterRequest(newConnection);

gnprice · 2024-11-13T00:19:32Z

test/widgets/actions_test.dart

+      // No unexpected requests made on either connection
+      check(connection.takeRequests()).isEmpty();
+      check(newConnection.takeRequests()).isEmpty();


I think these are redundant with the check in send — requests that are unexpected won't have a prepared response, so they'll throw, and the test framework will catch the unhandled exception and report.

gnprice · 2024-11-13T00:41:51Z

test/widgets/actions_test.dart

+      await future;
+      assertSingleUnregisterRequest(newConnection, expectedToken: '123');
+      check(newConnection.isOpen).isFalse();
+    });


Suggested change

});

}, variant: const TargetPlatformVariant({TargetPlatform.android, TargetPlatform.iOS}));

That way we exercise both the Android and iOS cases in unregisterToken.

(I think adding this to just this test is enough.)

gnprice · 2024-11-13T00:46:02Z

test/widgets/actions_test.dart

@@ -30,19 +41,247 @@ void main() {

  Future<void> prepare(WidgetTester tester, {
    UnreadMessagesSnapshot? unreadMsgs,
+    String? ackedPushToken = '123',


This change can be squashed into the commit after it, which starts using this feature.

(It doesn't touch the same code as the later commit; and it doesn't really need its own commit message, in that it's self-explanatory given the context of that other commit.)

Greg spotted this opportunity to be a bit more modern: zulip#1010 (comment)

chrisbobbe · 2024-11-13T00:57:05Z

Thanks! Revision pushed, and here's that screenshot of the new confirmation dialog:

Greg spotted this opportunity to be a bit more modern: zulip#1010 (comment)

So far this function just has two callers in uncommon paths involving app permissions. For those, and for the new caller we're about to add -- logging out an account -- it's unhelpful for the dialog to linger unchanged after the action button is tapped. Better to make it go away, preventing confusion about whether the action was actually done.

Like we added `checkErrorDialog` in 02fd562.

Without this line, if a test `globalStore.add`s an account with a non-null `ackedPushToken`, the field will be null in app code or test code that retrieves the account from `globalStore`. Thankfully no tests have been doing that, but it'll be convenient to do that soon, for testing the unregister-token part of logging out.

Fixes: zulip#463

gnprice · 2024-11-14T00:18:47Z

Thanks! Looks good; merging.

chrisbobbe added the integration review Added by maintainers when PR may be ready for integration label Oct 19, 2024

chrisbobbe requested a review from gnprice October 19, 2024 00:41

chrisbobbe assigned gnprice Oct 19, 2024

chrisbobbe mentioned this pull request Oct 19, 2024

store: Implement removeAccount #1007

Merged

gnprice reviewed Oct 19, 2024

View reviewed changes

chrisbobbe force-pushed the pr-logout-account-2 branch from 9e006cc to c0f4c78 Compare October 25, 2024 01:38

chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this pull request Oct 25, 2024

app: Update three-dots button to use new M3 MenuAnchor widget

e3830d8

Greg spotted this opportunity to be a bit more modern: zulip#1010 (comment)

chrisbobbe force-pushed the pr-logout-account-2 branch from c0f4c78 to 3fe188e Compare October 25, 2024 01:38

gnprice reviewed Oct 26, 2024

View reviewed changes

chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this pull request Nov 9, 2024

app: Update three-dots button to use new M3 MenuAnchor widget

5b10f99

Greg spotted this opportunity to be a bit more modern: zulip#1010 (comment)

chrisbobbe force-pushed the pr-logout-account-2 branch from 3fe188e to c5c1105 Compare November 9, 2024 06:08

gnprice reviewed Nov 13, 2024

View reviewed changes

chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this pull request Nov 13, 2024

app: Update three-dots button to use new M3 MenuAnchor widget

c8cd390

Greg spotted this opportunity to be a bit more modern: zulip#1010 (comment)

chrisbobbe force-pushed the pr-logout-account-2 branch from c5c1105 to 9f6a033 Compare November 13, 2024 00:57

chrisbobbe added 7 commits November 13, 2024 16:18

app: Update three-dots button to use new M3 MenuAnchor widget

e95e112

Greg spotted this opportunity to be a bit more modern: zulip#1010 (comment)

api: Add remove{Apns,Fcm}Token

89adaf3

notif: Implement NotificationService.unregisterToken

f830e8c

widget tests: Add checkSuggestedActionDialog

38213a8

Like we added `checkErrorDialog` in 02fd562.

login: Support logging out of an account

0344686

Fixes: zulip#463

gnprice force-pushed the pr-logout-account-2 branch from 9f6a033 to 0344686 Compare November 14, 2024 00:18

gnprice merged commit 0344686 into zulip:main Nov 14, 2024

chrisbobbe deleted the pr-logout-account-2 branch November 20, 2024 19:38

chrisbobbe mentioned this pull request Dec 27, 2024

Handle invalid API key on register-queue #737

Closed

PIG208 mentioned this pull request Jan 6, 2025

store: Handle invalid API key on register-queue #1183

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

login: Support logging out of an account (take 2) #1010

login: Support logging out of an account (take 2) #1010

chrisbobbe commented Oct 19, 2024 •

edited

Loading

gnprice Oct 19, 2024

chrisbobbe Oct 25, 2024 •

edited

Loading

chrisbobbe Oct 25, 2024

gnprice Oct 25, 2024

chrisbobbe commented Oct 25, 2024 •

edited

Loading

gnprice left a comment

gnprice Oct 25, 2024

gnprice Oct 25, 2024

chrisbobbe Oct 29, 2024 •

edited

Loading

gnprice Oct 29, 2024

alya Oct 31, 2024

gnprice Oct 25, 2024

chrisbobbe Oct 29, 2024

gnprice Oct 26, 2024

chrisbobbe commented Nov 9, 2024

gnprice left a comment

gnprice Nov 13, 2024

gnprice Nov 13, 2024

gnprice Nov 13, 2024

gnprice Nov 13, 2024

gnprice Nov 13, 2024

chrisbobbe commented Nov 13, 2024

gnprice commented Nov 14, 2024

	assertSingleUnregisterRequest(newConnection);
	checkSingleUnregisterRequest(newConnection);

	});
	}, variant: const TargetPlatformVariant({TargetPlatform.android, TargetPlatform.iOS}));

login: Support logging out of an account (take 2) #1010

login: Support logging out of an account (take 2) #1010

Conversation

chrisbobbe commented Oct 19, 2024 • edited Loading

Choose a reason for hiding this comment

chrisbobbe Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Oct 25, 2024 • edited Loading

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Log out?

chrisbobbe Oct 29, 2024 • edited Loading

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

Choose a reason for hiding this comment

chrisbobbe commented Nov 9, 2024

gnprice 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

Choose a reason for hiding this comment

chrisbobbe commented Nov 13, 2024

gnprice commented Nov 14, 2024

chrisbobbe commented Oct 19, 2024 •

edited

Loading

chrisbobbe Oct 25, 2024 •

edited

Loading

chrisbobbe commented Oct 25, 2024 •

edited

Loading

chrisbobbe Oct 29, 2024 •

edited

Loading