zulip icon indicating copy to clipboard operation
zulip copied to clipboard

organization_settings: Add dropdown to filter users list by role.

Open palashb01 opened this issue 1 year ago • 22 comments

  • [x] Adds a dropdown in the organization settings>users list page to filter the users list by role.
  • [x] By default the user_list will be sorted according to the role, from highest permissions to lowest permissions (Owners, Administrators, moderators, members, guests).
  • [x] Dropdown works for different languages also.

Fixes: #18617


Screenshots and screen captures:

Users list header:
  • Before:

Screenshot from 2023-03-14 01-14-28


  • After:

Screenshot from 2023-03-31 05-22-38


  • Narrow Screen:

Screenshot from 2023-03-31 05-22-52

Filter users list (demo):
  • English: filter

  • Other language:

fil

Default users list:

Screenshot from 2023-03-14 01-18-26


  • Difference from previous plan:

The issue description said to implement a multiSelectDropdown, however, we have not tested the multiSelectDropdownList properly. So, we can just go ahead with the singleSelectDropdown for the first iteration and then migrate to multi once it has been tested properly.


Self-review checklist
  • [x] Self-reviewed the changes for clarity and maintainability (variable names, code reuse, readability, etc.).

Communicate decisions, questions, and potential concerns.

  • [x] Explains differences from previous plans (e.g., issue description).
  • [x] Highlights technical choices and bugs encountered.
  • [x] Calls out remaining decisions and concerns.
  • [ ] Automated tests verify logic where appropriate.

Individual commits are ready for review (see commit discipline).

  • [x] Each commit is a coherent idea.
  • [x] Commit message(s) explain reasoning and motivation for changes.

Completed manual review and testing of the following:

  • [x] Visual appearance of the changes.
  • [x] Responsiveness and internationalization.
  • [x] Strings and tooltips.
  • [x] End-to-end functionality of buttons, interactions and flows.
  • [x] Corner cases, error conditions, and easily imagined bugs.

palashb01 avatar Mar 13 '23 19:03 palashb01

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

zulipbot avatar Mar 13 '23 19:03 zulipbot

Hey @alya , I have made the suggested changes. However, the dropdown works in all languages, but the titles inside the dropdowns are not changing with respect to the language. I am working on that part CZO .For the rest, could you please review? Thanks.

palashb01 avatar Mar 13 '23 20:03 palashb01

Thanks! The issue suggested a multi-select dropdown. Is that difficult to implement? If so, the current approach seems OK.

alya avatar Mar 14 '23 05:03 alya

@alya , as aman quoted on CZO:

MultiSelectDropdownList is actually new and not used anywhere in the codebase right now. you can try using DropdownListWidget for now and we can iterate on it later when MultiSelectDropdownList has been tested properly.

We have not tested the multiSelectDropdownlist properly, so we can just go ahead with the singleSelectDropdown for the first iteration and then migrate to multi once it has been tested properly.

I have made all the changes, could you please review the PR? Thanks.

palashb01 avatar Mar 14 '23 21:03 palashb01

We have not tested the multiSelectDropdownlist properly, so we can just go ahead with the singleSelectDropdown for the first iteration and then migrate to multi once it has been tested properly.

Got it. Please add a note in the PR description. I unchecked this point in the self-review checklist for you. ;)

Explains differences from previous plans (e.g., issue description).

alya avatar Mar 15 '23 08:03 alya

The alignment of the dropdpown and filter fields isn't right when one of the users has a really long name:

Screen Shot 2023-03-15 at 1 09 10 AM

Looks good to me otherwise!

alya avatar Mar 15 '23 08:03 alya

The alignment of the dropdpown and filter fields isn't right when one of the users has a really long name:

@alya , I'm unable to reproduce this bug, even when using very long names. Could you please provide a GIF or outline the steps to recreate this issue?

For me it looks like this:

Screenshot from 2023-03-15 21-42-11

palashb01 avatar Mar 15 '23 16:03 palashb01

do you need any help in this?

imlakshaykumar avatar Mar 15 '23 19:03 imlakshaykumar

do you need any help in this?

No thank you, I will let you know whenever I want to :)

palashb01 avatar Mar 15 '23 20:03 palashb01

Hm, it seems to be unrelated to long names. This is what I'm seeing; I really haven't taken any steps other than opening the Users menu.

Screen Shot 2023-03-17 at 3 51 45 PM

alya avatar Mar 17 '23 22:03 alya

@alya , I have fixed the issue and updated the PR description accordingly, could you please review it again? thanks. :)

palashb01 avatar Mar 30 '23 01:03 palashb01

Hrm, opening the Users panel as aaron, I see this vertical alignment issue:

Screen Shot 2023-03-29 at 10 40 31 PM

alya avatar Mar 30 '23 05:03 alya

In a narrow window, I think we want less vertical space between Users<->All roles and All roles<->Filter users, and more space between Filter users and the table.

Screen Shot 2023-03-29 at 10 42 02 PM

alya avatar Mar 30 '23 05:03 alya

Hrm, opening the Users panel as aaron, I see this vertical alignment issue:

I am not sure why this issue occurred. One possible reason could be that we recently changed the logic of CSS to have a symbol before text, and this PR was updated before that. I was using tag names for the CSS, but I have made all the suggested changes, and now it is looking as expected. Please let me know if you still see any issues.

Screenshot from 2023-03-31 05-22-38

palashb01 avatar Mar 30 '23 23:03 palashb01

In a narrow window, I think we want less vertical space between Users<->All roles and All roles<->Filter users, and more space between Filter users and the table.

For most of the settings table heading, we are using Bootstrap, which sets a 10px margin-top-bottom for h3 tags (heading) by default. To maintain consistency between the heading and filters, I have made changes so that all the sections (heading <-> role filter <-> search filter <-> table) have a 10px gap. Please let me know if you want me to reduce the gap further.

@alya , could you please review the PR again, Thanks :)

Screenshot from 2023-03-31 05-22-52

palashb01 avatar Mar 31 '23 00:03 palashb01

Looks good to me! @sahil839 could you please review this one?

alya avatar Mar 31 '23 06:03 alya

@sahil839 , I have made all the suggested changes. As for refactoring the list_widget code to use only a single function, I will work on it after receiving feedback from @timabbott.

palashb01 avatar Apr 03 '23 23:04 palashb01

@timabbott Marking for integration review for you to take a look, but see @sahil839 's question above regarding the overall approach.

alya avatar Apr 18 '23 21:04 alya

Though, I am not sure about the approach here. I think we should only have a single filter function and it should consider all the filters available for the table - in this case both search input and the role dropdown rather than passing filter and then role_filter separately. To support this, careful restructuring of the list_widget code would be needed. @timabbott thoughts?

Yeah, I agree, we should have a single filter function.

timabbott avatar Apr 24 '23 23:04 timabbott

Heads up @palashb01, we just merged some commits that conflict with the changes you 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.

zulipbot avatar Jul 10 '23 20:07 zulipbot

Hello @palashb01, If you agree, I'd like to collaborate with you and finish up the work on this feature and we could Co-Author the commits.

ecxtacy avatar Feb 08 '24 08:02 ecxtacy

Hello @palashb01, If you agree, I'd like to collaborate with you and finish up the work on this feature and we could Co-Author the commits.

Hey sure, you can go ahead and co-author this feature :)

palashb01 avatar Feb 10 '24 12:02 palashb01

Closing in favor of #29597 for this issue. Thanks @palashb01!

timabbott avatar Apr 10 '24 20:04 timabbott