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.

Sign up for GitHub

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

Jump to bottom

Improve handling of deleted user groups #11584

Open
timabbott opened this issue Feb 15, 2019 · 18 comments · May be fixed by #17424
Open

Improve handling of deleted user groups #11584

timabbott opened this issue Feb 15, 2019 · 18 comments · May be fixed by #17424
Labels
area: popovers Popovers, including general message actions. area: settings (admin/org) area: settings (user groups) User groups related settings issues bug help wanted priority: high

Comments

@timabbott
Copy link
Sponsor Member

The current model for user groups doesn't match how we handle deleting other objects, namely that when you "delete" it in the UI, we actually delete it from the database.

This results in our needing to do error handling hacks like 4e53110, which are bad because they can't distinguish actual bugs in the software from a deleted user group. There are two reasonable parts of a solution:

  • We should improve 4e53110 to show some sort of very basic "This user group has been deleted" version of the "user groups" popover when users click it, rather than just doing nothing, since that just looks broken. This is the high priority piece of this issue.
  • Change the model to deactivate the user group, hiding it from all UIs, rather than deleting it, when one is removed, so that we can generally distinguish bug vs. deleted in code assertions.
@zulipbot
Copy link
Member

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

@thedeveloperr
Copy link
Collaborator

@zulipbot claim

@thedeveloperr
Copy link
Collaborator

thedeveloperr commented Feb 22, 2019 •
edited

@timabbott When User group is deleted ,the delete event is sent as op="remove" Should I change it to op = deactivate or something like that after I add deactivation logic ? Will renaming the op name break anything ?

@timabbott
Copy link
Sponsor Member Author

It would break the mobile or terminal clients. In any case, I think remove is still a fine name for something which is being removed from the set of active user groups. So keep the event name as-is.

@thedeveloperr
Copy link
Collaborator

@timabbott should the deactivated user group mention be highlighted for which current user is member?

@timabbott
Copy link
Sponsor Member Author

Hmm, good question. I think it's not super important to do that (or anything else that requires sending data on the user group to the frontend).

thedeveloperr added a commit to thedeveloperr/zulip that referenced this issue Feb 25, 2019
@thedeveloperr
user_groups: Show message in popover for deactivated user groups.
497b032 
"This user group has been deleted" in popover for deactivated user
group when users click it, rather than just doing nothing.

Fixes: zulip#11584.
@thedeveloperr thedeveloperr mentioned this issue Feb 25, 2019
Open
thedeveloperr added a commit to thedeveloperr/zulip that referenced this issue Feb 26, 2019
@thedeveloperr
user_groups: Show message in popover for deactivated user groups.
5351131 
"This user group has been deleted" in popover for deactivated user
group when users click it, rather than just doing nothing.

Fixes: zulip#11584.
thedeveloperr added a commit to thedeveloperr/zulip that referenced this issue Feb 26, 2019
@thedeveloperr
user_groups: Show message in popover for deactivated user groups.
2752d54 
"This user group has been deleted" in popover for deactivated user
group when users click it, rather than just doing nothing.

Fixes: zulip#11584.
@zulipbot
Copy link
Member

zulipbot commented Mar 22, 2019 •
edited

Hello @thedeveloperr, you have been unassigned from this issue because you have not updated this issue or any referenced pull requests for over 14 days.

You can reclaim this issue or claim any other issue by commenting @zulipbot claim on that issue.

Thanks for your contributions, and hope to see you again soon!

@thedeveloperr
Copy link
Collaborator

Waiting for review On previous PR

@thedeveloperr
Copy link
Collaborator

@zulipbot waiting for review

@thedeveloperr
Copy link
Collaborator

@zulipbot claim
@timabbott PR is waiting for review but for some reason zulipbot is still unassigned me from this issue. I something wrong in my PR ?

thedeveloperr added a commit to thedeveloperr/zulip that referenced this issue Apr 30, 2019
@thedeveloperr
user_groups: Show message in popover for deactivated user groups.
56cae7c 
"This user group has been deleted" in popover for deactivated user
group when users click it, rather than just doing nothing.

Fixes: zulip#11584.
thedeveloperr added a commit to thedeveloperr/zulip that referenced this issue Apr 30, 2019
@thedeveloperr
user_groups: Show message in popover for deactivated user groups.
f20d766 
"This user group has been deleted" in popover for deactivated user
group when users click it, rather than just doing nothing.

Fixes: zulip#11584.
thedeveloperr added a commit to thedeveloperr/zulip that referenced this issue Apr 30, 2019
@thedeveloperr
user_groups: Show message in popover for deactivated user groups.
a6277e5 
This user group has been deleted" in popover for deactivated user
group when users click it, rather than just doing nothing.

Fixes: zulip#11584.
thedeveloperr added a commit to thedeveloperr/zulip that referenced this issue May 17, 2019
@thedeveloperr
user_groups: Show message in popover for deactivated user groups.
523e14c 
This user group has been deleted" in popover for deactivated user
group when users click it, rather than just doing nothing.

Fixes: zulip#11584.
@zulipbot
Copy link
Member

zulipbot commented Oct 19, 2019 •
edited

Hello @thedeveloperr, you have been unassigned from this issue because you have not updated this issue or any referenced pull requests for over 14 days.

You can reclaim this issue or claim any other issue by commenting @zulipbot claim on that issue.

Thanks for your contributions, and hope to see you again soon!

tushar912 added a commit to tushar912/zulip that referenced this issue Apr 21, 2021
@tushar912
mardown: Use get_active_user_group_from_name for group mention.
10de8a1 
Use this function for group mention in markdown.js.

TODO: Implement similar function for resolving group
mention in backend.

As a part of zulip#11584
tushar912 added a commit to tushar912/zulip that referenced this issue Apr 21, 2021
@tushar912
markdown: Change function for user_group mention.
03f6390 
Earlier we were using get_user_group_name_info for
resolving user group mention in backend.
But as there can be many user groups with same
name, changed it to query only the unique active
user group.

Fixes zulip#11584
tushar912 added a commit to tushar912/zulip that referenced this issue Apr 21, 2021
@tushar912
popover(user groups): Display user group has been deleted.
86ea084 
Display a user group has been deleted popover when
one clicks on deactivated user group mention.

TODO: Currently if we mention a user group and delete
it and then create a new user group with same name.
In the mention of new user_group we still get user
group has been deleted popover. This is because in
markdown we find user group from name instead of
active user group from name and thus it requires
to be changed to use get_active_user_group_from_name.

As a part of zulip#11584
tushar912 added a commit to tushar912/zulip that referenced this issue Apr 21, 2021
@tushar912
markdown: Add get_active_user_group_from_name function to config.
1dfe3a6 
As a part of zulip#11584
tushar912 added a commit to tushar912/zulip that referenced this issue Apr 21, 2021
@tushar912
mardown: Use get_active_user_group_from_name for group mention.
5600529 
Use this function for group mention in markdown.js.

TODO: Implement similar function for resolving group
mention in backend.

As a part of zulip#11584
tushar912 added a commit to tushar912/zulip that referenced this issue Apr 21, 2021
@tushar912
markdown: Change function for user_group mention.
b735555 
Earlier we were using get_user_group_name_info for
resolving user group mention in backend.
But as there can be many user groups with same
name, changed it to query only the unique active
user group.

Fixes zulip#11584
tushar912 added a commit to tushar912/zulip that referenced this issue Apr 28, 2021
@tushar912
user groups: Add active field and change constraints.
000d733 
Add active field which is used to tell whether the group
is active or not and change the constraint to have unique active
user_group per realm while having any number of similar ones which are
inactive.
This is done for deactivating the user_groups instead of entirely
deleting them from the database.

TODO: Add backend and frontend support for the new handling of
deleting user groups using active field.

As a part of zulip#11584
tushar912 added a commit to tushar912/zulip that referenced this issue Apr 28, 2021
@tushar912
user groups: Deactivate user groups instead of delete in backend.
d629059 
This changes the way we delete user_groups in backend. Instead
of deleting, deactivate them. Send the active and inactive groups
to frontend.

Also document the feature level and do provision version bump.

We don't send active field for add user_group event as it is
true by default for a newly created user group. Only send it
for the get/fetch event. So, created new Schemas UserGroupAdd and
UserGroupGet to distinguish the fields which are sent in both.
A thing worth mentioning is as we don't send is_active field for
add event in lib/events, have explicitely added it as we need
to segregate active and inactive usergroups while sending
it to frontend.

TODO: Add frontend support for this feature.

As a part of zulip#11584
tushar912 added a commit to tushar912/zulip that referenced this issue Apr 28, 2021
@tushar912
user groups: Add active user group dict and few functions.
60edd37 
Added a active_user_group_by_id_dict to map the
active user groups to their id.

Also the following functions are added.
1. add_in_realm
To add a new user group to active_user_group_by_id_dict
and user_group_by_id_dict. For more context add only
adds user_group to user_group_by_id_dict.
2. get_active_user_group_from_id
To get the active user group from id. Return
undefined if group is not found.
3. is_active_user_group
To find if a user_group is active.
4. get_active_user_group_from_name
To get active user group from name.

TODO: Modify remove function to instead delete group
from active_user_group_by_id_dict. Also initialize
this dict from the active and non active groups
sent in page_params.

As a part of zulip#11584
tushar912 added a commit to tushar912/zulip that referenced this issue Apr 28, 2021
@tushar912
ui_init: Add realm_non_active_user_groups to user_group_params.
3eb8ad1 
As a part of zulip#11584
tushar912 added a commit to tushar912/zulip that referenced this issue Apr 28, 2021
@tushar912
user groups: Modify some functions.
76de091 
Following functions are modified to deactivate
user groups instead of deleting.
1. remove
Instead of deleting from all user groups only
delete it from active user groups.
2. get_realm_user_groups
Return only the active user groups.
3. initialize
Add all the active user groups to user_groups_by
_id_dict and active_user_groups_by_id_dict and
the inactive ones only to user_groups_by_id_dict.

TODO: Currently the user_group add event of
server_events_dispatch calls add function. This
needs to be changed to the new add_in_realm
function. Also a popover showing user group
has been deleted needs to be displayed in the
mention of deleted user_group.

As a part of zulip#11584
tushar912 added a commit to tushar912/zulip that referenced this issue Apr 28, 2021
@tushar912
events: Use add_in_realm in add event of user_groups.
035925e 
TODO: Display a user group has been deleted popover
on clicking deleted group mention.

As a part of zulip#11584
tushar912 added a commit to tushar912/zulip that referenced this issue Apr 28, 2021
@tushar912
popover(user groups): Display user group has been deleted.
0e72f1e 
Display a user group has been deleted popover when
one clicks on deactivated user group mention.

TODO: Currently if we mention a user group and delete
it and then create a new user group with same name.
In the mention of new user_group we still get user
group has been deleted popover. This is because in
markdown we find user group from name instead of
active user group from name and thus it requires
to be changed to use get_active_user_group_from_name.

As a part of zulip#11584
tushar912 added a commit to tushar912/zulip that referenced this issue Apr 28, 2021
@tushar912
markdown: Add get_active_user_group_from_name function to config.
6a9f428 
As a part of zulip#11584
tushar912 added a commit to tushar912/zulip that referenced this issue Apr 28, 2021
@tushar912
mardown: Use get_active_user_group_from_name for group mention.
46045ba 
Use this function for group mention in markdown.js.

TODO: Implement similar function for resolving group
mention in backend.

As a part of zulip#11584
tushar912 added a commit to tushar912/zulip that referenced this issue Apr 28, 2021
@tushar912
markdown: Change function for user_group mention.
0cbae94 
Earlier we were using get_user_group_name_info for
resolving user group mention in backend.
But as there can be many user groups with same
name, changed it to query only the unique active
user group.

Fixes zulip#11584
tushar912 added a commit to tushar912/zulip that referenced this issue Apr 28, 2021
@tushar912
user groups: Add active field and change constraints.
57ada16 
Add active field which is used to tell whether the group
is active or not and change the constraint to have unique active
user_group per realm while having any number of similar ones which are
inactive.
This is done for deactivating the user_groups instead of entirely
deleting them from the database.

TODO: Add backend and frontend support for the new handling of
deleting user groups using active field.

As a part of zulip#11584
tushar912 added a commit to tushar912/zulip that referenced this issue Apr 28, 2021
@tushar912
user groups: Deactivate user groups instead of delete in backend.
2e37622 
This changes the way we delete user_groups in backend. Instead
of deleting, deactivate them. Send the active and inactive groups
to frontend.

Also document the feature level and do provision version bump.

We don't send active field for add user_group event as it is
true by default for a newly created user group. Only send it
for the get/fetch event. So, created new Schemas UserGroupAdd and
UserGroupGet to distinguish the fields which are sent in both.
A thing worth mentioning is as we don't send is_active field for
add event in lib/events, have explicitely added it as we need
to segregate active and inactive usergroups while sending
it to frontend.

TODO: Add frontend support for this feature.

As a part of zulip#11584
tushar912 added a commit to tushar912/zulip that referenced this issue Apr 28, 2021
@tushar912
user groups: Add active user group dict and few functions.
affb8a3 
Added a active_user_group_by_id_dict to map the
active user groups to their id.

Also the following functions are added.
1. add_in_realm
To add a new user group to active_user_group_by_id_dict
and user_group_by_id_dict. For more context add only
adds user_group to user_group_by_id_dict.
2. get_active_user_group_from_id
To get the active user group from id. Return
undefined if group is not found.
3. is_active_user_group
To find if a user_group is active.
4. get_active_user_group_from_name
To get active user group from name.

TODO: Modify remove function to instead delete group
from active_user_group_by_id_dict. Also initialize
this dict from the active and non active groups
sent in page_params.

As a part of zulip#11584
tushar912 added a commit to tushar912/zulip that referenced this issue Apr 28, 2021
@tushar912
ui_init: Add realm_non_active_user_groups to user_group_params.
35bb289 
As a part of zulip#11584
tushar912 added a commit to tushar912/zulip that referenced this issue Apr 28, 2021
@tushar912
user groups: Modify some functions.
9287dd9 
Following functions are modified to deactivate
user groups instead of deleting.
1. remove
Instead of deleting from all user groups only
delete it from active user groups.
2. get_realm_user_groups
Return only the active user groups.
3. initialize
Add all the active user groups to user_groups_by
_id_dict and active_user_groups_by_id_dict and
the inactive ones only to user_groups_by_id_dict.

TODO: Currently the user_group add event of
server_events_dispatch calls add function. This
needs to be changed to the new add_in_realm
function. Also a popover showing user group
has been deleted needs to be displayed in the
mention of deleted user_group.

As a part of zulip#11584
tushar912 added a commit to tushar912/zulip that referenced this issue Apr 28, 2021
@tushar912
events: Use add_in_realm in add event of user_groups.
c0674d1 
TODO: Display a user group has been deleted popover
on clicking deleted group mention.

As a part of zulip#11584
tushar912 added a commit to tushar912/zulip that referenced this issue Apr 28, 2021
@tushar912
popover(user groups): Display user group has been deleted.
5e6f54a 
Display a user group has been deleted popover when
one clicks on deactivated user group mention.

TODO: Currently if we mention a user group and delete
it and then create a new user group with same name.
In the mention of new user_group we still get user
group has been deleted popover. This is because in
markdown we find user group from name instead of
active user group from name and thus it requires
to be changed to use get_active_user_group_from_name.

As a part of zulip#11584
tushar912 added a commit to tushar912/zulip that referenced this issue Apr 28, 2021
@tushar912
markdown: Add get_active_user_group_from_name function to config.
952e4bc 
As a part of zulip#11584
tushar912 added a commit to tushar912/zulip that referenced this issue Apr 28, 2021
@tushar912
mardown: Use get_active_user_group_from_name for group mention.
82bdfb2 
Use this function for group mention in markdown.js.

TODO: Implement similar function for resolving group
mention in backend.

As a part of zulip#11584
tushar912 added a commit to tushar912/zulip that referenced this issue Apr 28, 2021
@tushar912
markdown: Change function for user_group mention.
cf3d8b4 
Earlier we were using get_user_group_name_info for
resolving user group mention in backend.
But as there can be many user groups with same
name, changed it to query only the unique active
user group.

Fixes zulip#11584
@timabbott timabbott added area: popovers Popovers, including general message actions. and removed area: message feed (uncategorized) labels Jun 16, 2021
@PIG208 PIG208 mentioned this issue Nov 14, 2022
Open
@alya alya added the area: settings (user groups) User groups related settings issues label Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area: popovers Popovers, including general message actions. area: settings (admin/org) area: settings (user groups) User groups related settings issues bug help wanted priority: high
Projects
Groups migration and new settings
Status: No status
Milestone
No milestone
7 participants
@alya @timabbott @thedeveloperr @zulipbot @Tlazypanda @YashRE42 @tushar912