[Fix] Channel Members' Roles fetching from ChannelStore instead of UserStore

[thesynthax]

Brief Title

Channel Members' Roles will now fetch from ChannelStore instead of UserStore.
Checkout #517 for the complete details about this fix.

Acceptance Criteria fulfillment

• The functions fetching the channel roles from UserStore, now fetch from ChannelStore
• The console logging of the roles now no longer shown an empty roles object.

Fixes #517

Video/Screenshots

[Spiral-Memory]

This is an overriding PR to your own PR #520 , as this commit is already present in it. I will suggest you to raise only one PR and mention both of your issues in it, indicating that it fixes both if they are dependent on each other because it will create problem while merging the PRs.

[Spiral-Memory]

Your assessment is correct; it seems that we are indeed storing all user roles of the channel, so it should be in channelStores or memberStores (that will be better actually) and not in userStores. However, the way you are trying to restrict pinning of messages is not correct.

Let me explain: there is a concept of global scope and room scope in Rocket Chat. The one you are fetching with me.role is a global scope, and what we stored in channelStores is the room scope of all users in the channel.

To implement what you want, the flow will go like this:

1. Get the me.roles to find the global scope of that user.
2. Check the channel role, look for that user, and then combine both these roles.
3. After that, fetch which roles have permission to pin messages.
4. Then check if our concatenated roles contain any of those roles. If yes, then allow pinning; otherwise, don't allow.

All this has to be done just to disable the pin icon. Anyway, if you click on the pin icon, it will display an error message for pinning the message. So, anyway, the API itself won't allow you to pin.

But for user experience, if you want to implement this, all of these things have to be taken care of properly before implementing it.

You can read the following to understand more about it : https://docs.rocket.chat/use-rocket.chat/workspace-administration/permissions

Also have you verified whether roles are not being used anywhere taken from userStores because, in that case you have to change the import there as well. Also if possible, i will suggest you to store it in memberStore as that is more relevant.

[thesynthax]

This is an overriding PR to your own PR #520 , as this commit is already present in it. I will suggest you to raise only one PR and mention both of your issues in it, indicating that it fixes both if they are dependent on each other because it will create problem while merging the PRs.

Yes, I will raise another PR, drafting #520 now and will delete it after this one has been merged.

Your assessment is correct; it seems that we are indeed storing all user roles of the channel, so it should be in channelStores or memberStores (that will be better actually) and not in userStores. However, the way you are trying to restrict pinning of messages is not correct.

Let me explain: there is a concept of global scope and room scope in Rocket Chat. The one you are fetching with me.role is a global scope, and what we stored in channelStores is the room scope of all users in the channel.

To implement what you want, the flow will go like this:

1. Get the `me.roles` to find the global scope of that user.

2. Check the channel role, look for that user, and then combine both these roles.

3. After that, fetch which roles have permission to pin messages.

4. Then check if our concatenated roles contain any of those roles. If yes, then allow pinning; otherwise, don't allow.

All this has to be done just to disable the pin icon. Anyway, if you click on the pin icon, it will display an error message for pinning the message. So, anyway, the API itself won't allow you to pin.

But for user experience, if you want to implement this, all of these things have to be taken care of properly before implementing it.

You can read the following to understand more about it : https://docs.rocket.chat/use-rocket.chat/workspace-administration/permissions

Thanks again for your detailed insight into this. I will create another commit fetching the necessary permissions and will do it as you mentioned. Yes all this for pin icon to be not displayed, only for the user experience and the fact that this is also the case for Rocket.Chat

[thesynthax]

Hey @sidmohanty11 @abhinavkrin please have a look and suggest any changes if required, as my next PR (#520) also depends on this commit to be merged.

[sidmohanty11]

This is an overriding PR to your own PR #520 , as this commit is already present in it. I will suggest you to raise only one PR and mention both of your issues in it, indicating that it fixes both if they are dependent on each other because it will create problem while merging the PRs.

@thesynthax I think we should follow this, looking at this PR I cannot comprehend what its actually trying to solve. Can you please create one PR with all your changes. Also can you create a discussion thread regarding this in the #embeddedchat channel in RC. I want to understand why this change is required

[thesynthax]

Hi @sidmohanty11, as mentioned above

Yes, I will raise another PR, drafting #520 now and will delete it after this one has been merged.

I was already planning to do the same. And about the reason for this PR as reviewed by @Spiral-Memory :

Your assessment is correct; it seems that we are indeed storing all user roles of the channel, so it should be in channelStores or memberStores (that will be better actually) and not in userStores

and #517 explain the reason for this fix, please check it out.

Basically the roles object in userStore was getting overwritten by a function in useFetchChatData.js, which must store the channel members' roles in memberStore and not userStore, as the rolesObj in useFetchChatData.js contains roles for all channel members, not for one single user. userStore should be used for every individual user.

[Spiral-Memory]

@thesynthax I'll suggest you to close this PR and complete your original PR and mark it ready for review. In that PR itself, mention both issues.

This PR alone doesn't clarify why this fix is required. It can be understood after looking at your PR #520.

[thesynthax]

Should I create a new PR, squashing all those commits in #520 because there are some merge conflicts right now as the branch wasn't updated with main branch or should I keep #520 only, what do you say? I can create a new one free of any conflicts.

[Spiral-Memory]

Hey @sidmohanty11 , please merge this PR as Umang has recently created an issue where it will be useful. Let me explain why.

When we log in, at that time, the overall role of the person is fetched and saved into userStore. (Workspace level role)

However, apart from that, we also have roles specific to channels. Therefore, we fetch channel roles and then save them in the userStore again, which overrides the workspace level roles of the person in the workspace. One solution is to concatenate both roles while saving, as in EC currently we deal with only one channel. However, this solution is also fine and will keep channel-related roles separate from workspace-related roles.

Earlier, it wasn't necessary to keep this as separate PR as in his another PR, this commit was already present, but now some issues have been created in which this PR will be useful.

[sidmohanty11]

@thesynthax @Spiral-Memory is this a breaking change? I don't see where we've updated it from userStore, I just see additions made to channelStore

[Spiral-Memory]

@thesynthax @Spiral-Memory is this a breaking change? I don't see where we've updated it from userStore, I just see additions made to channelStore

No, this is not a breaking change. Currently, the role saved in 'userRoles' is not used anywhere in the code. However, since this role is more related to the channel than the workspace, he wanted to keep these roles separate as 'memberRoles' (with respect to the channel). He wanted to save workspace-level roles in 'userStore' in his another PR to achieve a better separation between them. This change is not necessary, but it will result in a better separation, that's all. It's just a design choice, nothing much.