Make sure the app doesn't break with the new delivery_email field #3196
Comments
|
One other aspect of this worth making explicit: the transition from the existing configuration to the new one. When an organization makes this transition, the Our planned solution is to have the server expire all event queues for the organization (for mobile clients... maybe all non-webapp clients? Tim told me last week he believes the webapp will have no trouble with the transition.) That should cause the app to reload everything, so there's no confusion. This is an important area to test, though -- there's a lot of kinds of data involved, and there may be cases where we try to still use pre-transition data combined with post-transition data, which will make things confused. |
|
Update: This setting is available for org admins on zulipchat.com to choose, though marked "beta": We just got a very helpful, detailed report from a puzzled org admin of a set of symptoms that turned out to be triggered by this setting. They found that with the setting enabled (i.e. with email addresses visible only to admins), all users in the organization saw the following symptoms on mobile (lettering mine, to clarify discussion below):
On the last point (e), the avatar specifically is blank. One further symptom found in discussion: I believe symptoms (e) and (f) are probably because we try to use the user's email address from something like Not sure about (d). That sounds like it might be the same bug, compounded with some additional bug in a reducer. The first three symptoms (a), (b), (c) all sound like we're failing to get events from the server. Not sure why this setting would lead to that failure mode, but apparently it does. |
|
This has been available for beta-testing by a few orgs (I think because this one asked for it) for a period of a couple weeks. It is not in any server release. We should definitely consider making this work properly in the mobile app a priority issue, because we have been getting regular requests for this feature and will definitely want to include it in the next release. IIRC there is an event sent when |
|
That's helpful context, thanks.
Cool. We'll want to do that, but unfortunately this will be more complex to implement than just the transition -- major steps include
|
This probably doesn't totally work as is -- there are probably some places we want the email before we have the /register response, as well as places we want the delivery_email. But I think it's the core of the fix to zulip#3196.
This probably doesn't totally work as is -- there are probably some places we want the email before we have the /register response, as well as places we want the delivery_email. But I think it's the core of the fix to zulip#3196.
This probably doesn't totally work as is -- there are probably some places we want the email before we have the /register response, as well as places we want the delivery_email. But I think it's the core of the fix to zulip#3196.
This probably doesn't totally work as is -- there are probably some places we want the email before we have the /register response, as well as places we want the delivery_email. But I think it's the core of the fix to zulip#3196.
This brings this UI in line with the corresponding UI in the webapp, which is the card that pops up on clicking on a user's name or avatar. The user's email address can be helpful information, but their name is usually more helpful. Also, under an org setting that's currently in beta after being a frequent request for a long time, users' real email addresses won't even be available to normal users -- so the `email` property will be an opaque address made up by the server. (See #3196.) So we certainly don't want to show that here. While we're here, also swap the order of the name and the status indicator, again following the webapp.
Part of #3196 -- this is an example of code that will break when hidden-emails are enabled, because the (real) email used for auth will be different from the (server-generated, fake) email used as an identifier for the user. Found by grepping for `auth.email`. There are still a few more matches for that pattern, which with one exception (in src/api/) are also broken; those we'll fix in the next few commits.
Part of #3196 -- this is another bit of code that will break when the hidden-emails setting is enabled.
Part of #3196 -- this is another bit where we were using the user's (real) auth email where their (possibly different and fake) identifier-email is what we need.
|
Just pushed (as bd6c155..2407f3f) a series of fixes to various small places where we were using the current user's real email (the one used in authentication) where what we needed was their identifier-email. Each of these was a spot that would break when the hidden-emails setting is enabled. Now they all get the identifier-email through a common codepath, Still in a draft branch is the change to make I also observed today a puzzling issue where, shortly after enabling hidden-emails on an org, I believe the server was still returning unobfuscated real emails in the |
This probably doesn't totally work as is -- there are probably some places we want the email before we have the /register response, as well as places we want the delivery_email. But I think it's the core of the fix to zulip#3196.
With the "hidden emails" feature (aka `email_address_visibility`) that's now in beta on the server, the user's "email" as used as an identifier for them in the Zulip API can be an opaque one made up by the server -- different from `auth.email` aka `delivery_email`. So we need to switch our concept of "own email" to use the identifier-email; this is #3196. The server tells us this user's identifier-email in the `/register` response, so one necessary step is to remember it. Do that. In principle we should then update this value on EVENT_USER_UPDATE. But (a) the same is true of several other values here, and more importantly (b) we already don't handle EVENT_USER_UPDATE at all, either in our `users` state-subtree or in the various denormalized spots where we store other users' emails, avatars, etc. So punt. Changing emails is pretty uncommon; and in the particular case where it's happening for all an org's users en masse because hidden-emails was turned on or off, the only near-term-feasible solution will probably be to have the server just invalidate clients' event queues.
|
Just pushed 139b7d1..c5b9370, and I believe this is now fixed! There may be yet-unknown bugs where we still use Behavior is still not good at transition time, in the period shortly after turning the setting on (or off) for an org. There are two pieces to this:
|
|
@gnprice just a note -- the plan I'd imagined was for the client to destroy its event queue when receiving an event of the email_address_policy changing; the webapp I believe should be able to (possibly with small changes to address bugs) handle such transitions without a reload because of how it already handles email address changes correctly. (We have a handful of events that on the webapp trigger a reload for similar reasons) |
As part of adding (hacky) support for not sharing users' real email addresses with everyone in the organization, we've added a new field,
delivery_email(which in most configurations just equals theemail, but ). The basic model is that theemailfield will continue to be a human-readable identifier for a user, and for most client purposes, they can just ignore thedelivery_emailfield and things will transparently work; users generally will only have access to their owndelivery_email.I expect that this hacky approach (where the
emailfield may not be the user's actual email address) is going to be how things work for a long time, because theemailfield name is used in so many places that migrating away from and then renaming it likely won't happen for a year or more from when we start doing so.It's likely that the mobile apps don't need any work; for the webapp frontend, we needed to do just these two commits: zulip/zulip@67f0b1b and zulip/zulip@bc74aba (for the "my account" settings page). (Well, we'll possibly need to add a few more changes for the administration UI, but the mobile app doesn't have one).
But we should do some testing of the mobile app for this to make sure something reasonable happens in this configuration (which is available in the development environment now thanks to the commit above); I suppose if there's a place where we display "your email address", that should be changed to use
.delivery_email. No changes should be needed for the auth/login stage; for that, everything uses the value ofdelivery_emailunder the old nameemail.(We'd like to turn this feature on for chat.zulip.org within the next few weeks assuming no blockers from e.g. mobile)
The text was updated successfully, but these errors were encountered: