Skip to content

Conversation

@samay-kothari
Copy link
Contributor

@samay-kothari samay-kothari commented Apr 4, 2022

Proposed changes

The order of mentions in the mobile app used to be according to recent conversation, but now before that the priority is given to the participants of the room. The order followed is as it is done in the web-app as mentioned here

Issue(s)

Fixes #3681

How to test or reproduce

Screenshots

Simulator.Screen.Recording.-.iPhone.12.-.2022-04-02.at.19.28.43.mp4

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • Improvement (non-breaking change which improves a current function)
  • New feature (non-breaking change which adds functionality)
  • Documentation update (if none of the other choices apply)

Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works (if applicable)
  • I have added necessary documentation (if applicable)
  • Any dependent changes have been merged and published in downstream modules

Further comments

@CLAassistant
Copy link

CLAassistant commented Apr 4, 2022

CLA assistant check
All committers have signed the CLA.

@ankar84
Copy link
Contributor

ankar84 commented Apr 15, 2022

Hello, Team!
Is that PR planed to land in 4.27?

Comment on lines +21 to +28
export enum ReverseSubscriptionType {
'p' = SubscriptionType.GROUP,
'd' = SubscriptionType.DIRECT,
'c' = SubscriptionType.CHANNEL,
'l' = SubscriptionType.OMNICHANNEL,
'e2e' = SubscriptionType.E2E,
't' = SubscriptionType.THREAD
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Contributor Author

@samay-kothari samay-kothari Apr 27, 2022

Choose a reason for hiding this comment

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

I think later in the comment you have mentioned the usage of roomTypeToApiType, but here we need an enumeration that maps the string literals 'p', 'd', etc, to Subscription type of the room, since the function getRoomMembers requires it in this data type only.
The roomType gives us data of type RoomType and I wasn't able to find a way to map RoomType to SubscriptionType.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An alternative to using this could be add an async call to getRoom function from lib/methods/getRoom and then get the roomSubscriptionType from there.
Right now I am receiving the roomType and rid as a string through props.

});
}
}
const roomTypeSub = (<any>ReverseSubscriptionType)[roomType];
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you could have used roomTypeToApiType

Copy link
Contributor Author

@samay-kothari samay-kothari Apr 27, 2022

Choose a reason for hiding this comment

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

I tried using that but I am unable to find a way to map a roomType(string) to a SubscriptionDataType.

ReverseSubscriptionType does reverse of SubscriptionType enumeration, it takes a String and gives us a SubscriptionType.

Comment on lines 175 to 178
setData = new Set([...tempData, ...data.filter(user => user.name.match(regex))]);
setData = new Set([...setData, ...data.filter(user => roomUsers.includes(user.name))]);
// TODO: add the conditions to check whether external mentions are allowed
setData = new Set([...setData, ...data]);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I got a bit lost here, can you explain why this should be done?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So here is the explanation for each step:
1: tempData stores the users with the exact username and member of the room.
2: In Line : 175 - We add the remaining users, whose usernames exactly match but are not part of the room.
3: In Line : 176 - We add the remaining users, who are members of the room containing the text in their username, name or nickname.
4: In Line : 178 - We add the remaining users, who are not members of the room but contain the text in their username, name or nickname.

Here we need to add a condition on step 2 and step 4, whether mentions of people outside room are allowed or not.

@samay-kothari samay-kothari requested a review from gerzonc April 27, 2022 16:54
@ankar84
Copy link
Contributor

ankar84 commented Jun 8, 2022

@gerzonc @diegolmello @GleidsonDaniel hey!
Is there a plans to land that fix to next release?
Thanks!

@ankar84
Copy link
Contributor

ankar84 commented Aug 26, 2022

@gerzonc @diegolmello @GleidsonDaniel hello, team!
Any updates about that PR?
That improvement is very useful and in general imply algorithm that already working in Web client for a year or even more.
It is providing same level of functionality of Web and Mobile clients. And I guess Mobile client should go in that direction.
Thanks!

@diegolmello
Copy link
Member

@gerzonc @diegolmello @GleidsonDaniel hello, team! Any updates about that PR? That improvement is very useful and in general imply algorithm that already working in Web client for a year or even more. It is providing same level of functionality of Web and Mobile clients. And I guess Mobile client should go in that direction. Thanks!

This PR was out of my radar.
It needs some polishing, like we should reuse the logic for the normal spotlight instead of fragmenting usage.
I'm going to add to next sprint, so we can review and finish it.
Thanks for pinging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Logic of mention suggestions differs of Web version

5 participants