Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Finish "email address visibility" feature #11454

Closed
4 tasks done
timabbott opened this issue Feb 5, 2019 · 3 comments
Closed
4 tasks done

Finish "email address visibility" feature #11454

timabbott opened this issue Feb 5, 2019 · 3 comments

Comments

@timabbott
Copy link
Sponsor Member

timabbott commented Feb 5, 2019

I'm opening this issue to track the follow-up work related to our email address visibility feature (#2369) once #10850 merges. There are a few critical items:

  • Some end-to-end testing
  • Make sure we send gravatars even if client_gravatar is set, with this setting.
  • Ensure that the mobile apps won't completely break when you toggle this setting. One valid approach would be to send a "restart server" event (and/or delete all active event queues for the realm), and then confirm that the mobile app's caching of users is properly reset with that code path (it should be).
  • Figure out a way to avoid doing a linear number of queries for the do_set_realm_property code for switching an organization; the existing code will likely timeout and thus fail to finish the transition for a large enough realm. The critical bit here is refactoring the changes to do a bulk save. We roughly want this:
if name == "email_address_visibility":                                              
    user_profiles_to_save = []                                                      
    for user_profile in UserProfile.objects.filter(realm=realm, is_bot=False):      
        # Save the old email address as a temporary value on the UserProfile object.
        user_profile.old_email = user_profile.email                                 
        user_profile.email = get_display_email_address(user_profile, realm)         
        user_profiles_to_save.append(user_profile)                                  
                                                                                    
    # We do a bulk database update and then flush the realm's                       
    # caches to avoid doing database queries linear in the number                   
    # of users.                                                                     
    UserProfile.objects.bulk_update(user_profiles_to_save)                          
    # The update_fields value here is a bit of a hack.                              
    flush_realm(instance=realm, update_fields=['string_id'])                        
                                                                                    
    for user_profile in user_profiles_to_save:                                      
        # TODO: Design a bulk event for this or force-reload all                    
        # clients, since this is linear work for our Tornado                        
        # infrastructure.                                                           
        if user_profile.email != user_profile.old_email:                            
            send_user_email_update_event(user_profile)                              

but Django doesn't actually have a .objects.bulk_update feature. One reasonable solution would be to move the code we currently have to a different queue processor (e.g. the deferred_work one), where timeouts aren't as much of a problem.

@zulipbot
Copy link
Member

zulipbot commented Feb 5, 2019

Hello @zulip/server-settings members, this issue was labeled with the "area: settings (admin/org)" label, so you may want to check it out!

@varkor
Copy link

varkor commented Mar 29, 2020

Is this issue complete? There seem to be options for setting organisation email visibility now.

@timabbott
Copy link
Sponsor Member Author

Yes, closing as all of these have been resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants