Closed MonilBhavsar closed 2 weeks ago
Job added to Upwork: https://www.upwork.com/jobs/~01fe75f988a6386f2e
Triggered auto assignment to Contributor-plus team member for initial proposal review - @situchan (External
)
[#fast-apis] Subscribe guides to a new presence pusher channel
Improvement.
subscribeToPrivateUserChannelEvent
function which subscribes to to private user channel events.
https://github.com/Expensify/App/blob/194f197a0c52fc95f18fbf3aa87afd42954f8184/src/libs/PusherUtils.ts#L26-L49subscribeToPrivateUserChannelEvent
which will subscribe to the new presence pusher channel.User.subscribeToUserEvents();
we can check if the user is a guide or not and then after we can subscribe to the new presence pusher channel using the newly created function.
https://github.com/Expensify/App/blob/194f197a0c52fc95f18fbf3aa87afd42954f8184/src/libs/Navigation/AppNavigator/AuthScreens.tsx#L112-L128Edited by proposal-police: This proposal was edited at 2024-08-23 01:43:52 UTC.
function subscribeToActiveGuides() {
Pusher.subscribe(`activeGuides`).catch((error: ReportError) => {
Log.hmmm('[Report] Failed to initially subscribe to Pusher channel');
});
}
function unsubscribeToActiveGuides() {
Pusher.unsubscribe(activeGuides
);
}
the ```eventName``` can be provided later. And in this case, we can skip the ```eventCallback``` because BE just needs to know if the guide is online or not.
2. Create a subscribe component.
- This component is named ```ActiveGuidesEventListener```, its responsibility is to subscribe to a active guides channel if the current user is a guide and belongs to a specific policyID. Same as what we did in component [UserTypingEventListener](https://github.com/Expensify/App/blob/194f197a0c52fc95f18fbf3aa87afd42954f8184/src/pages/home/report/UserTypingEventListener.tsx).
````javascript
function ActiveGuidesEventListener({session, allPoliciesEmployeeList}) {
const flatternPoliciesEmployee = flatten(Object.values(allPoliciesEmployeeList));
const didSubscribeToActiveGuides = useRef(false);
useEffect(
() => () => {
const sessionEmail = session.email;
const emailDomain = Str.extractEmailDomain(sessionEmail ?? '');
if (didSubscribeToActiveGuides.current) {
return;
}
if (emailDomain !== CONST.EMAIL.GUIDES_DOMAIN) {
return;
}
if (some(flatternPoliciesEmployee, (user) => user.email === sessionEmail)) {
didSubscribeToActiveGuides.current = true;
subscribeToActiveGuides();
}
},
[session, flatternPoliciesEmployee],
);
return null;
}
export default withOnyx({
session: {
key: ONYXKEYS.SESSION,
},
allPoliciesEmployeeList: {
key: ONYXKEYS.COLLECTION.POLICY,
selector: (policy) => policy.employeeList,
},
})(ActiveGuidesEventListener);
Use the above component in AuthScreens
:
https://github.com/Expensify/App/blob/194f197a0c52fc95f18fbf3aa87afd42954f8184/src/libs/Navigation/AppNavigator/AuthScreens.tsx#L526
</View>
{didPusherInit && <ActiveGuidesEventListener />}
with didPusherInit
is state:
const [didPusherInit, setDidPusherInit] = useState(false);
and we set it to true
in here.
We don't need to cleanup function because this line: https://github.com/Expensify/App/blob/194f197a0c52fc95f18fbf3aa87afd42954f8184/src/libs/Navigation/AppNavigator/AuthScreens.tsx#L344 already did that.
Based on comment, we can modify the ActiveGuidesEventListener
in main solution to:
function ActiveGuidesEventListener({user}) {
const didSubscribeToActiveGuides = useRef(false);
useEffect(
() => () => {
if (didSubscribeToActiveGuides.current) {
return;
}
if (user.isGuide) {
didSubscribeToActiveGuides.current = true;
subscribeToActiveGuides();
}
},
[user],
);
return null;
}
export default withOnyx({ user: { key: ONYXKEYS.USER, }, })(ActiveGuidesEventListener);
- We still need to keep using ```didPusherInit && <ActiveGuidesEventListener />``` as in main solution to make sure the ```Pusher.subscribe``` is called after the Pusher is init.
@MonilBhavsar In the issue's description you said:
have a team.expensify.com domain and belong to a specific policyID
if user has team.expensify.com domain but does not belongs to specific workspace, should we subscribe to the presence pusher channel?
if user has team.expensify.com domain but does not belongs to specific workspace, should we subscribe to the presence pusher channel?
No, both conditions are required
Thanks for the proposals! π @situchan could you please take a look.
[#fast-apis] Subscribe guides to a new presence pusher channel #47888
Guide users are subscribed to a private encrypted user channel only and not subscribed to a presence channel ('presence-activeGuides').
If a user is a guide, i.e. their email is an expensify team email, add a Pusher subscription to a presence channel request (PusherUtils.subscribeToPresenceChannelEvent) to the subscribeToUserEvents function in User (called in AuthScreens after Pusher init).
Add a subscribeToPresenceChannelEvent function to Pusher Utils with a Pusher subscribe request. All guides are subscribed to the same presence channel. Subscription to a presence channel adds a members property to the pusher socket, which includes a members count.
Add a PusherUtils.subscribeToPresenceChannelEvent request just after PusherUtils.subscribeToPrivateUserChannelEvent. https://github.com/Expensify/App/blob/a29977bb8490e231d28babac11c6dc8650d19de8/src/libs/actions/User.ts#L660-L674 Add a subscribeToPresenceChannelEvent function just after subscribeToPrivateUserChannelEvent. https://github.com/Expensify/App/blob/a29977bb8490e231d28babac11c6dc8650d19de8/src/libs/PusherUtils.ts#L29-L31 Include a Pusher.subscribe request in the subscribeToPresenceChannelEvent function with channel name 'presence-activeGuides'. https://github.com/Expensify/App/blob/a29977bb8490e231d28babac11c6dc8650d19de8/src/libs/PusherUtils.ts#L48
N/A
@MonilBhavsar, @situchan Whoops! This issue is 2 days overdue. Let's get this updated quick!
Thanks for the proposals.
I think @Krishna2323's solution is the most simple, though the logic to check if user is guide or not is missing. And unsubscribe logic is also not clear.
@daledah introducing new component is fine but I doubt subscribeToActiveGuides() is called before pusher init is complete. https://github.com/Expensify/App/blob/a29977bb8490e231d28babac11c6dc8650d19de8/src/libs/Navigation/AppNavigator/AuthScreens.tsx#L254-L262
@mariapeever I don't see any meaningful difference from @Krishna2323's proposal.
@situchan
I doubt subscribeToActiveGuides() is called before pusher init is complete
have a team.expensify.com domain and belongs to a specific policyID
@daledah thanks. What is the benefit of introducing new ActiveGuidesEventListener? Can't we just subscribe after subscribeToUserEvents()?
Can't we just subscribe after subscribeToUserEvents()
As mentioned in the OP's description, we just want to subscribe to a new presence pusher channel if they have a team.expensify.com domain and belong to a specific policyID. If we subscribe after subscribeToUserEvents(), it will subscribe to presence channel even if user is not in any policy.
it will subscribe to presence channel even if user is not in any policy.
Ofc we'll check if user belongs to specific policy
Ofc we'll check if user belongs to specific policy
Yeah. If we subscribe after subscribeToUserEvents(), we cannot check if user belongs to specific policy, right?
@situchan, I think we are already disconnecting from the socket when the AuthScreen
is unmounted, If we want we can explicitly from the channel. For checking if the user is a guide or not, we can use the email and compare it with CONST.EMAIL.GUIDES_DOMAIN
and we will also make sure that the user belongs to a policy by getting the policy using PolicyUtils.getPolicy(policyID)
and finding if the member is in the participants list or not.
Ofc we'll check if user belongs to specific policy
Yeah. If we subscribe after subscribeToUserEvents(), we cannot check if user belongs to specific policy, right?
Makes sense. We may need to connect ONYXKEYS.COLLECTION.POLICY in AuthScreens
@Krishna2323 what about this concern?
@Krishna2323 what about https://github.com/Expensify/App/issues/47888#issuecomment-2310968947?
I'm not sure why we can't check if user belongs to specific policy, can you please help me understand? Can't we get the policy using useOnyx
or usePolicy
hook?
@Krishna2323 pusher init is called on AuthScreens component mount
@daledah @Krishna2323 please test your solutions and confirm working
We can also make backend return a flag - isGuide, if we think we're comprising frontend performance by connecting onyx in AuthScreens. And also, we don't load all policies upfront, so it might be that a guide is not being correctly identified as guide and hence not subscribed to pusher channel
We can also make backend return a flag - isGuide, if we think we're comprising frontend performance by connecting onyx in AuthScreens. And also, we don't load all policies upfront, so it might be that a guide is not being correctly identified as guide and hence not subscribed to pusher channel
I think that would be the best option, we can just use the flag to subscribe to the new channel, if that could be done, we can subscribe just after subscribeToUserEvents
.
@situchan I added an alternative solution based on @MonilBhavsar's comment
@MonilBhavsar isGuide
will be added to session
or user
?
user
sounds more appropriate
@MonilBhavsar
we don't load all policies upfront, so it might be that a guide is not being correctly identified
We just need to check if the user belongs to at least one policy or not, so I don't think we need to load all policies. What do you think?
Also, I updated the proposal based on comment.
We just need to check if the user belongs to at least one policy or not, so I don't think we need to load all policies. What do you think?
How would we check? I mean not all policies would be loaded in Onyx
@MonilBhavsar, do you think that personal details will be loaded? I think its the same case as policies. IMO, It would be more appropriate if we can add the flag to the session object, since we don't want to subscribe to any new ONYX collection.
How would we check? I mean not all policies would be loaded in Onyx
Assume user has 100 policies, but BE just returns 20
policies. So we can check if user belongs to at least one policy based on those 20 policies, no need for all 100 policies returned.
So we can check if user belongs to at least one policy based on those 20 policies, no need for all 100 policies returned.
Yes, but if the guide policy is not returned in those 20 policies, then code can go wrong, no
if we can add the flag to the session object
user
sounds more better to me than session
user sounds more better to me than session
Then we have to subscribe to PERSONAL_DETAILS_LIST
in the auth screen and I think personal details can also be empty like policies and we will be connecting to ONYX in AuthScreens which I think isn't desired. Have I misunderstood something?
Looks like the solution would be to
Just need to make sure that user is fetched correctly at the time of pusher init @Krishna2323 please confirm
Why subscribe to PERSONAL_DETAILS_LIST?
@situchan, if we logout and login we only get isUsingExpensifyCard
property in the user object but later it shows 4 properties.
I think we need to connect ONYKEYS.USER to AuthScreens and in the callback we can subscribe to the channel if the user is a guide.
const hasCheckedUserIsGuide = useRef(false);
Onyx.connect({
key: ONYXKEYS.USER,
callback: (user) => {
if (user.isGuide === undefined || !user?.validated) {
hasCheckedUserIsGuide.current = false;
return;
}
hasCheckedUserIsGuide.current = true;
if (user.isGuide) {
subscribeToActiveGuides();
}
},
});
@Krishna2323 are you connecting onyx inside component lifecycle?
@situchan, no, it's not inside component lifecycle.
@Krishna2323 please share your test branch. And also test yourself
@situchan, I have tested the solution, here is the test branch.
@situchan I updated my alternative solution to match comment.
@situchan, my solution does not check if the pusher is initialized or not, you can review @daledah's proposal instead. TBH I don't have much idea how the pusher events works π
@situchan, my solution does not check if the pusher is initialized or no
Thanks. Was about to say that π
@daledah now that all questions are answered, can you please clarify https://github.com/Expensify/App/issues/47888#issuecomment-2310955054?
What is the benefit of introducing new ActiveGuidesEventListener? Can't we just subscribe after subscribeToUserEvents()?
Make sure we only call subscribeToUserEvents()
if pushser is init by using {didPusherInit && <ActiveGuidesEventListener />}
.
Make sure the user
data is always out-of-date.
@daledah do we need server to provide isGuide
or are we checking that in the frontend?
@MonilBhavsar, my proposal includes two solutions. For the main solution, we don't need to provide isGuide. However, for the alternative solution, we will need the isGuide flag returned by the backend via the user data, based on your suggestion:
We can also make the backend return a flag - isGuide.
Both of them work well.
π£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πΈ
Context:
Guides are agents who help new customers with onboarding.
We use Pusher to send real time updates. We have a private user channel here https://github.com/Expensify/App/blob/f7d8e4850e729493eb8bfb52efa1a49d0d344ded/src/CONST.ts#L1154
private-encrypted-user-accountID-<>
that is used to receive various events in real time like onyx updates, user typing and more.Problem
Pusher private channel is dynamic, or unique for each user as it contains an accountID. For example: A channel name could look like
private-encrypted-user-accountID-5-b8645604fe014e82b765ca84b7df1f2e
At the server side, if we want to know how many guides are online at the current time, then we need to make an API call to each pusher channel and see if it is occupied or not. If we have 20 guides, then we make 20 API calls to check guide's presence, which is not efficient.
Solution
Make guides subscribe to a presence channel -
activeGuides
https://pusher.com/docs/channels/using_channels/presence-channels/With this, a server only will have to make one API call to find all active guides or users subscribed to this channel.
If a user is a guide - i.e. have a
team.expensify.com
domain and belongs to a specific policyID, then we additionally subscribe guide to the the guides presence channel.Like the private user channel, a subscription to the channel should indicate guide's presence in the app - They should be subscribed if they login and are active, and should be unsubscribed if they sign out.
We probably have most of the code and logic for the private user channel. We need to similarly subscribe guides to the new channel.
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @trjExpensify