Page MenuHomePhabricator

[web] Add a selector fetching thread IDs according to COMMUNITIES calendar filter
ClosedPublic

Authored by inka on Jan 30 2023, 3:59 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Feb 29, 12:42 PM
Unknown Object (File)
Tue, Feb 27, 3:50 AM
Unknown Object (File)
Feb 22 2024, 10:27 PM
Unknown Object (File)
Feb 19 2024, 9:26 PM
Unknown Object (File)
Feb 19 2024, 7:53 AM
Unknown Object (File)
Feb 19 2024, 7:52 AM
Unknown Object (File)
Feb 19 2024, 7:51 AM
Unknown Object (File)
Feb 19 2024, 7:20 AM
Subscribers

Details

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
Branch
inka/drawer_web_3
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

inka requested review of this revision.Jan 30 2023, 4:13 AM
lib/selectors/calendar-filter-selectors.js
36–52

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

inka edited the summary of this revision. (Show Details)

Rebase

Changes due to changes in the previous diff

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

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
This revision now requires review to proceed.Feb 9 2023, 1:35 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 again - I think it's important that these are IDs

inka planned changes to this revision.Feb 9 2023, 7:34 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

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
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.

tomek added inline comments.
web/selectors/calendar-selectors.js
38 ↗(On Diff #22296)

Makes sense!

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