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

users_list: Fix updating of users list on reactivate/deactivate. #14288

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sahil839
Copy link
Collaborator

User is added to deactivated users list when user is deactivated from
active users list and it remains in the active users for re-toggle
(in case of mistake) and same happens for reactivating users from
deactivated users list.

Whenever user enters any of the list from somehwhere else, list is
up-to-date.

Fixes #14165

Currently, the last active on reactivation gives Unknown as some change is needed in presence code and issue is opened currently #14279.
I am working on that issue and will soon make a PR.

Testing Plan: This is manually tested.

GIFs or Screenshots:
14165

@sahil839 sahil839 force-pushed the deactivated_users branch 2 times, most recently from 02eff01 to 5294bca Compare March 26, 2020 21:34
@showell
Copy link
Contributor

showell commented Apr 6, 2020

@sahil839 Can you give an update on the status of this PR? I know this issue sort of sent us down the rabbit hole of making sure that we weren't showing the wrong "active" date for users after we reactivated them, but is that actually a blocker for this commit?

@sahil839
Copy link
Collaborator Author

sahil839 commented Apr 6, 2020

@showell The only problem is that this will show Unknown on reactivation of a user.

@@ -392,7 +388,6 @@ exports.on_load_success = function (realm_people_data) {
const url = '/json/users/' + encodeURIComponent(user_id) + "/reactivate";
const data = {};
const status = get_status_field();

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you undo the blank line removals here? I think those lines were there for readability, and in any case it's distracting from the rest of the changes in this commit.


populate_users(realm_people_data);

exports.register_click_handlers = function () {
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clean up this commit series to have steps like extracting register_click_handlers as a new function as independent commits? It'd be more readable that way.

@sahil839 sahil839 force-pushed the deactivated_users branch 2 times, most recently from de3ba6f to a133cf2 Compare April 21, 2020 10:54
@zulipbot zulipbot added size: S and removed size: L labels Apr 21, 2020
@sahil839 sahil839 force-pushed the deactivated_users branch 3 times, most recently from 58d69e1 to 9025fd1 Compare April 21, 2020 14:28
@sahil839
Copy link
Collaborator Author

@timabbott I have made the changes.
Circle CI failure is unrelated to my changes and is being discussed on CZO.

@showell
Copy link
Contributor

showell commented May 7, 2020

I'm working on a deeper fix here. I'll probably close this PR once I get to a pretty solid point on my branch. Thanks for working on this @sahil839 -- reading your code here has been helpful for me to understand where we can clean this up a bit.

@zulipbot
Copy link
Member

Heads up @sahil839, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

@showell
Copy link
Contributor

showell commented May 12, 2020

I haven't quite fixed this yet, but you can see that my recent commits are leading up to a fix here:

https://github.com/zulip/zulip/commits?author=showell

@showell
Copy link
Contributor

showell commented May 12, 2020

In particular the below commit is sort of a preview of how I will split up handling of Users and Deactivated Users, which should help fix this issue:

5c16bb9

There's a little bit of extra work to do, though.

Previously, when a user was deactivated, the user was
added in the deactivated users list only after opening
the settings menu again or reloading, but now the user
is added to the deactivated users list if we open the
deactivated users section. Note that sticky behavior
is stil present which means that the deactivated user
will still be present in the active users list until
we go away from that section.

And the behavior is same for reactivating a user from
deactivated users list.
@zulipbot
Copy link
Member

Hello @sahil839, it seems like you have referenced #14165 in your pull request description, but you have not referenced them in your commit message description(s). When you reference an issue in a commit message, it automatically closes the corresponding issue when the commit is merged.

Please run git commit --amend in your command line client to amend your commit message description with Fixes #14165..

An example of a correctly-formatted commit:

commit fabd5e450374c8dde65ec35f02140383940fe146
Author: zulipbot
Date:   Sat Mar 18 13:42:40 2017 -0700

    pull requests: Check PR commits reference when issue is referenced.

    Fixes #51.

Thank you for your contributions to Zulip!

@zulipbot
Copy link
Member

Heads up @sahil839, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.

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

Successfully merging this pull request may close these issues.

Deactivated users tab not being refreshed.
4 participants