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

fix: cleaning the room filter between navigation on the admin section #32152

Merged
merged 30 commits into from
Jun 21, 2024

Conversation

AllanPazRibeiro
Copy link
Contributor

@AllanPazRibeiro AllanPazRibeiro commented Apr 8, 2024

Proposed changes (including videos or screenshots)

Resolve Room Filter Persistence Issue with Local Storage

  • Addressed a bug where room filter settings were only partially resetting during admin page navigation.
  • Now, all filters are completely cleared.

Issue(s)

https://rocketchat.atlassian.net/browse/CORE-217

Steps to test or reproduce

  1. Go to admin page
  2. Go to Room section
  3. Change the filter
  4. Navigate to another section
  5. Go back to the Room filter, the filter should be cleaned

Further comments

Screen.Recording.2024-04-24.at.12.03.18.mov

Copy link
Contributor

dionisio-bot bot commented Apr 8, 2024

Looks like this PR is not ready to merge, because of the following issues:

  • This PR is targeting the wrong base branch. It should target 6.10.0, but it targets 6.9.0

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

Copy link

changeset-bot bot commented Apr 8, 2024

🦋 Changeset detected

Latest commit: bac931e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 32 packages
Name Type
@rocket.chat/meteor Patch
@rocket.chat/core-typings Patch
@rocket.chat/rest-typings Patch
@rocket.chat/apps Patch
@rocket.chat/core-services Patch
@rocket.chat/cron Patch
@rocket.chat/fuselage-ui-kit Patch
@rocket.chat/gazzodown Patch
@rocket.chat/livechat Patch
@rocket.chat/model-typings Patch
@rocket.chat/ui-contexts Patch
@rocket.chat/account-service Patch
@rocket.chat/authorization-service Patch
@rocket.chat/ddp-streamer Patch
@rocket.chat/omnichannel-transcript Patch
@rocket.chat/presence-service Patch
@rocket.chat/queue-worker Patch
@rocket.chat/stream-hub-service Patch
@rocket.chat/api-client Patch
@rocket.chat/license Patch
@rocket.chat/omnichannel-services Patch
@rocket.chat/pdf-worker Patch
@rocket.chat/presence Patch
rocketchat-services Patch
@rocket.chat/ddp-client Patch
@rocket.chat/uikit-playground Patch
@rocket.chat/models Patch
@rocket.chat/ui-avatar Patch
@rocket.chat/ui-client Patch
@rocket.chat/ui-video-conf Patch
@rocket.chat/web-ui-registration Patch
@rocket.chat/instance-status Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

codecov bot commented Apr 8, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 56.37%. Comparing base (1ac48ae) to head (bac931e).
Report is 67 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #32152      +/-   ##
===========================================
+ Coverage    56.32%   56.37%   +0.04%     
===========================================
  Files         2434     2437       +3     
  Lines        53708    53779      +71     
  Branches     11057    11081      +24     
===========================================
+ Hits         30251    30317      +66     
- Misses       20812    20817       +5     
  Partials      2645     2645              
Flag Coverage Δ
e2e 56.07% <100.00%> (+0.07%) ⬆️
unit 72.15% <0.00%> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@AllanPazRibeiro AllanPazRibeiro force-pushed the fix/CORE-217/fixing-room-search-filter-persistence branch from 9d4b2b8 to 2b6bc96 Compare April 10, 2024 17:14
@AllanPazRibeiro AllanPazRibeiro force-pushed the fix/CORE-217/fixing-room-search-filter-persistence branch from 2b6bc96 to 16a73ee Compare April 10, 2024 19:13
@AllanPazRibeiro AllanPazRibeiro changed the title wip feat: persisting the room filter between navigation on the admin section Apr 10, 2024
@AllanPazRibeiro AllanPazRibeiro added this to the 6.8 milestone Apr 10, 2024
@AllanPazRibeiro AllanPazRibeiro self-assigned this Apr 10, 2024
.changeset/brown-lobsters-join.md Outdated Show resolved Hide resolved
apps/meteor/client/views/admin/rooms/RoomsTable.tsx Outdated Show resolved Hide resolved
apps/meteor/tests/e2e/admin-room.spec.ts Outdated Show resolved Hide resolved
apps/meteor/tests/e2e/admin-room.spec.ts Outdated Show resolved Hide resolved
apps/meteor/tests/e2e/admin-room.spec.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@tiagoevanp tiagoevanp left a comment

Choose a reason for hiding this comment

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

I'm thinking here, you have one single filter to use in the context. If I want to use the same context for another screen, I will kill the previous filter with the new one state, right? Is this the desired behavior? 🤔

@AllanPazRibeiro AllanPazRibeiro marked this pull request as ready for review April 12, 2024 20:01
@AllanPazRibeiro AllanPazRibeiro requested a review from a team as a code owner April 12, 2024 20:01
@AllanPazRibeiro AllanPazRibeiro force-pushed the fix/CORE-217/fixing-room-search-filter-persistence branch from 4be789d to fecfb19 Compare April 15, 2024 17:40
@KevLehman KevLehman dismissed their stale review April 15, 2024 18:07

Will leave FE folks to approve :)

tiagoevanp
tiagoevanp previously approved these changes Apr 15, 2024
apps/meteor/tests/e2e/admin-room.spec.ts Outdated Show resolved Hide resolved
apps/meteor/tests/e2e/admin-room.spec.ts Outdated Show resolved Hide resolved
apps/meteor/tests/e2e/admin-room.spec.ts Outdated Show resolved Hide resolved
@AllanPazRibeiro AllanPazRibeiro force-pushed the fix/CORE-217/fixing-room-search-filter-persistence branch from 8e8485d to 72f0ca5 Compare April 17, 2024 18:18
@AllanPazRibeiro AllanPazRibeiro force-pushed the fix/CORE-217/fixing-room-search-filter-persistence branch from 65969cf to 35aacbd Compare May 8, 2024 17:26
@dougfabris
Copy link
Member

dougfabris commented May 10, 2024

Related to the MultiSelectCustom
@MarcosSpessatto All those changes was made in order to be able to get good locators for the requested e2e tests. Despite it's not related directly I think it somehow related since we're improving the filters tests. So if we decide to remove those changes we'll have to add tests with bad locators. So IMO we have two paths:

  • Add the filters changes AND add good locators
  • Remove the filters changes AND add bad locators

@AllanPazRibeiro AllanPazRibeiro force-pushed the fix/CORE-217/fixing-room-search-filter-persistence branch from 30a1847 to 35aacbd Compare May 13, 2024 18:21
@scuciatto scuciatto removed this from the 6.9 milestone May 14, 2024
@MarcosSpessatto
Copy link
Member

Related to the MultiSelectCustom @MarcosSpessatto All those changes was made in order to be able to get good locators for the requested e2e tests. Despite it's not related directly I think it somehow related since we're improving the filters tests. So if we decide to remove those changes we'll have to add tests with bad locators. So IMO we have two paths:

  • Add the filters changes AND add good locators
  • Remove the filters changes AND add bad locators

Agree with your approach guys, we can go ahead. Thanks for the explanation.

apps/meteor/client/views/admin/rooms/RoomsTable.tsx Outdated Show resolved Hide resolved
apps/meteor/client/views/admin/rooms/RoomsTableFilters.tsx Outdated Show resolved Hide resolved
apps/meteor/tests/e2e/admin-room.spec.ts Outdated Show resolved Hide resolved
packages/mock-providers/src/MockedQueryClientWrapper.tsx Outdated Show resolved Hide resolved
@AllanPazRibeiro AllanPazRibeiro force-pushed the fix/CORE-217/fixing-room-search-filter-persistence branch 3 times, most recently from 34e53f0 to f37ac5a Compare June 7, 2024 13:42
@AllanPazRibeiro AllanPazRibeiro force-pushed the fix/CORE-217/fixing-room-search-filter-persistence branch from f37ac5a to 05765b1 Compare June 7, 2024 13:42
Copy link
Member

@gabriellsh gabriellsh left a comment

Choose a reason for hiding this comment

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

LGTM

@gabriellsh gabriellsh added this to the 6.10 milestone Jun 7, 2024
@AllanPazRibeiro AllanPazRibeiro added the stat: QA assured Means it has been tested and approved by a company insider label Jun 20, 2024
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Jun 20, 2024
@ggazzo ggazzo merged commit e47ae76 into develop Jun 21, 2024
50 checks passed
@ggazzo ggazzo deleted the fix/CORE-217/fixing-room-search-filter-persistence branch June 21, 2024 03:07
This was referenced Jun 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stat: QA assured Means it has been tested and approved by a company insider stat: ready to merge PR tested and approved waiting for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants