Details

Reviewers

kamil
tomek

Commits

rCOMM6238c432bc99: [web] Add a selector fetching thread IDs according to COMMUNITIES calendar…

Summary

issue: https://linear.app/comm/issue/ENG-2794/make-it-possible-to-display-only-chats-from-a-given-community-in-the
Since D6645 the calendar filters reducer reacts to updateCalendarCommunityFilter and clearCalendarCommunityFilter actions. We want to be able to extract the thread IDs based on the filters these actions have set.

Test Plan

Dispatch updateCalendarCommunityFilter and clearCalendarCommunityFilter actions, log the value returned by filteredCommunityThreadIDsSelector. Check that it is correct. Check that it is only
possible to add an event for a chat in the selected community (in the web app) - this is a result of updating onScreenThreadInfos that is used by ThreadPickerModal

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

inka created this revision.Jan 30 2023, 3:59 AM

Herald added subscribers: atul, ashoat. · View Herald TranscriptJan 30 2023, 3:59 AM

inka edited the summary of this revision. (Show Details)Jan 30 2023, 4:01 AM

inka added a parent revision: D6458: [web] Add to calendar filters reducer logic for handling the COMMUNITY filters.Jan 30 2023, 4:07 AM

Harbormaster completed remote builds in B15834: Diff 21580.Jan 30 2023, 4:13 AM

inka requested review of this revision.Jan 30 2023, 4:13 AM

inka added a child revision: D6462: [web] Add logic for displaying only chats from a given community in calendar filters.Jan 30 2023, 5:53 AM

inka added inline comments.Jan 30 2023, 6:17 AM

lib/selectors/calendar-filter-selectors.js
36–52 ↗	(On Diff #21580)	This is very similar to `filteredThreadIDs`, it just filters out a different `calendarThreadFilterTypes` type, but I'm not sure if i should merge them. I wanted to simply take `type: CalendarThreadFilterType` as an argument, but then in line 44 I get a flow error, because for type `NOT_DELETED` `filter.threadIDs` doesn't exist. And manually specifying the names of the types that work here seems wrong to me.

Mooving the selector to web/, since state.communityFilter is only present on web/ now

Harbormaster completed remote builds in B16211: Diff 22176.Feb 7 2023, 4:02 AM

inka added a parent revision: D6645: [web] Add to calendar filters reducer logic for handling the COMMUNITY filters.Feb 7 2023, 4:53 AM

inka edited the summary of this revision. (Show Details)Feb 7 2023, 4:56 AM

Rebase

Harbormaster completed remote builds in B16217: Diff 22182.Feb 7 2023, 5:14 AM

Changes due to changes in the previous diff

Harbormaster completed remote builds in B16222: Diff 22187.Feb 7 2023, 7:04 AM

Changes due to: chainng what is kept in the redux state - keep only the community id

Harbormaster completed remote builds in B16268: Diff 22259.Feb 8 2023, 2:22 AM

kamil accepted this revision.Feb 8 2023, 8:22 AM

kamil added inline comments.

web/selectors/thread-selectors.js
135 ↗	(On Diff #22259)	This type could be skipped since it's obvious this will be the same as the return type of the entire selector but I saw in codebase versions with/without type in last callback so deffering to you

This revision is now accepted and ready to land.Feb 8 2023, 8:22 AM

inka removed a parent revision: D6645: [web] Add to calendar filters reducer logic for handling the COMMUNITY filters.Feb 9 2023, 1:11 AM

inka added a child revision: D6645: [web] Add to calendar filters reducer logic for handling the COMMUNITY filters.

inka removed a child revision: D6462: [web] Add logic for displaying only chats from a given community in calendar filters.

Address code review

inka added 1 blocking reviewer(s): tomek.Feb 9 2023, 1:35 AM

This revision now requires review to proceed.Feb 9 2023, 1:35 AM

Harbormaster completed remote builds in B16286: Diff 22283.Feb 9 2023, 1:44 AM

tomek accepted this revision.Feb 9 2023, 4:33 AM

tomek added inline comments.

web/selectors/thread-selectors.js
127 ↗	(On Diff #22283)	This name is a bit confusing. Maybe we can explain what is being filtered and by which condition, e.q. `filteredThreadIDsByCommunitySelector`? We can even go one step further and avoid saying that we're filtering.

This revision is now accepted and ready to land.Feb 9 2023, 4:33 AM

Rename

Rename again - I think it's important that these are IDs

Harbormaster completed remote builds in B16292: Diff 22292.Feb 9 2023, 5:53 AM

Harbormaster completed remote builds in B16293: Diff 22293.Feb 9 2023, 5:59 AM

Actually this selector should take into account if thread is in Filter List (threadInFilterList), because we want it to tell us about the chats in the community, that the user wants to see calendar events of. Now it just returns all chats from that community that the user has in their threadStore, which is not right.

I also believe this should be moved to web/selectors/calendar-selectors.js, to be clear that this is related to calendar stuff

use threadInFilterList and move code to calendar-selectors.js

This revision is now accepted and ready to land.Feb 9 2023, 8:10 AM

inka requested review of this revision.Feb 9 2023, 8:12 AM

inka added inline comments.

web/selectors/calendar-selectors.js
38 ↗	(On Diff #22296)	This was added relative to what had been accepted

Harbormaster failed remote builds in B16296: Diff 22296!Feb 9 2023, 2:11 PM

tomek requested changes to this revision.Feb 10 2023, 1:10 AM

tomek added inline comments.

web/selectors/calendar-selectors.js
38 ↗	(On Diff #22296)	I'm not sure if it is a good idea to include this check here. Now the function name is no longer relevant as we're returning only a subset of threads from the community. I think this function should be reverted to the previous state and the additional filtering could be performed where it is used. We can also consider renaming this function so that it is more explicit about the additional filtering.

This revision now requires changes to proceed.Feb 10 2023, 1:10 AM

inka added inline comments.Feb 10 2023, 4:21 AM

web/selectors/calendar-selectors.js
38 ↗	(On Diff #22296)	I moved the function to calendar-selectors.js to be clear that this has to do with calendar, and not simply returning threads of a community. So I'd rather rename the function to be more clear, and do the filtering here. I don't want to have to do this in the reducer, I want this function to abstract that from the reducer.